[D7] Entity Content Synchronize (ecs)

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxsmakisog2540518git

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Automated Review

No errors - http://pareview.sh/pareview/httpgitdrupalorgsandboxsmakisog2540518git

Note that perfect adherence to Drupal Coding Standard is NOT a reason to block an application, except for total disregard of them. However, modules should follow them as closely as possible.

Manual Review

Individual user account

Yes: Follows.

No duplication

Yes: Does not cause.

Master Branch

Yes: Follows.

Licensing

Yes: Follows.

3rd party assets/code

Yes: Follows.

README.txt/README.md

Yes: Follows.

Code long/complex enough for review

Yes: Follows.

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

1. (*) Don't give files execution rights.
2. (+) in file includes/ecs.helper.inc line 381 - Undefined constant fasle, please change to false
3. Just a recommendation
   file ecs.install in function _ecs_fields() duplicate keys "label"

     return array(
       'node_sync_flag' => array(
         'field_name'  => 'node_sync_flag',
         'label'       => $t('Node sync flag'),
         'entity_type' => 'node',
         'type' => 'list_boolean',
         'description' => 'Sync this content?',
         'label' => 'Sync content',

4. Just a recommendation
   in function ecs_install() line 26 - Variable 'values' might have not been defined
5. Just a recommendation
   in file includes/ecs.helper.inc line 207 - Variable 'nid' might have not been defined
6. Just a recommendation
   file menu/ecs-admin-ui.inc line 26 - Typo: in word 'remore', I think 'remote' will be right

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

This is not a full review, just some points I noticed.

1. (*) What is the conceptual difference with https://www.drupal.org/project/migrate_d2d?
   Any migration module cannot cover content structure for all sites.
   Consider this is a duplicate (probably need to stop at this point and suggest work with migrate_d2d developers).
2. (*) List module is not mandatory in the Drupal, so your module may break on installation phase if list disabled.
   You are using list_boolean field autocreation on the installation phase _ecs_fields().
3. (*) In ecs.module:182 Instead of using 'LANGUAGE_NONE' use dynamic method to determine field language.
4. (*) ecs_node_compare() will not work for multilingual sites because of 'LANGUAGE_NONE' - use dynamic method to determine field language.
5. (+)Requirements sections in README.txt missed. #2181737: README.md template

I haven't checked all the code in details, I general code looks very good.
But my point it is duplicate, hope I am wrong.

only the manual reviews are relevant for the review bonus program.

Review of the 7.x-1.x branch (commit 2cad3d9):

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  
  FILE: /home/klausi/pareview_temp/ecs.module
  ---------------------------------------------------------------------------
  FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
  ---------------------------------------------------------------------------
   168 | WARNING | Do not concatenate strings to translatable strings, they
       |         | should be part of the t() argument and you should use
       |         | placeholders
  ---------------------------------------------------------------------------
  
  
  FILE: /home/klausi/pareview_temp/menu/ecs-global-sync.inc
  ---------------------------------------------------------------------------
  FOUND 0 ERRORS AND 4 WARNINGS AFFECTING 4 LINES
  ---------------------------------------------------------------------------
   125 | WARNING | Do not concatenate strings to translatable strings, they
       |         | should be part of the t() argument and you should use
       |         | placeholders
   145 | WARNING | Do not concatenate strings to translatable strings, they
       |         | should be part of the t() argument and you should use
       |         | placeholders
   151 | WARNING | Do not concatenate strings to translatable strings, they
       |         | should be part of the t() argument and you should use
       |         | placeholders
   166 | WARNING | Do not concatenate strings to translatable strings, they
       |         | should be part of the t() argument and you should use
       |         | placeholders
  ---------------------------------------------------------------------------
  
  
  FILE: /home/klausi/pareview_temp/ecs.install
  -------------------------------------------------------------------------
  FOUND 2 ERRORS AFFECTING 2 LINES
  -------------------------------------------------------------------------
   161 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 2
   162 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 2
  -------------------------------------------------------------------------
  PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
  -------------------------------------------------------------------------
  

• DrupalPractice has found some issues with your code, but could be false positives.
  

  
  FILE: /home/klausi/pareview_temp/ecs.module
  ---------------------------------------------------------------------------
  FOUND 0 ERRORS AND 5 WARNINGS AFFECTING 5 LINES
  ---------------------------------------------------------------------------
   14 | WARNING | Open page callback found, please add a comment before the
      |         | line why there is no access restriction
   30 | WARNING | Open page callback found, please add a comment before the
      |         | line why there is no access restriction
   36 | WARNING | Open page callback found, please add a comment before the
      |         | line why there is no access restriction
   42 | WARNING | Open page callback found, please add a comment before the
      |         | line why there is no access restriction
   48 | WARNING | Open page callback found, please add a comment before the
      |         | line why there is no access restriction
  ---------------------------------------------------------------------------
  

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

1. project page: what are the differences to the deploy module https://www.drupal.org/project/deploy for example? Please add the existing modules that do similar things to the project page. Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition.
2. This module has a security issue and as part of our git admin training I'm assigning this to Manjit so that he can take a look. If he does not find anything I will post the exploit details in one week. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

@smakisog: Thanks for the contributions !! I guess there is so many 'access callback' => TRUE, for ['ecs/*'] menu items in module file. And these are accessible to anonymous user as well.

Another scenario, i have restrict the content (article) for anonymous user but still ecs/content_mappingis showing me content as an array. Please check the best way of use Access Arguments and hook_menu

Also check screenshot for the content issue.

manual review:

1. the method checksumValidate() does not exist but is used in _ecs_check_access()?
2. generateChecksum(): never use md5() for cryptography tasks since it has know weaknesses. Looks like you want to use drupal_hmac_base64() instead?
3. ECSSyncPacket class: what do all the class properties mean? Please add docs.
4. ecs_shared_secret: that variable should be initialized with a random value for the site. Otherwise if a user enables the module an attacker can access all the ecs menu callbacks while no unique ecs_shared_secret has been set.
5. ECSSyncPacket::validate() you must always use the "===" operator when comparing hashes. And if the function hash_equals() is available (since PHP 5.6) you should use that to prevent timing attacks on the hash.
6. "drupal_alter('ecs_send', $entity_sync_packet);": Hooks that are provided by a module should be documented in MODULENAME.api.php, see http://drupal.org/node/161085#api_php
7. ecs_globalsync_form_batch_generate(): why do you have to call set_time_limit() here? isn't the point of a batch to do it in multiple steps that don't run into timeouts? Please add a comment.
8. ecs_globalsync_form_batch_generate(): calling node_load() often will be slow, collect all node IDs an use a node_load_multiple() instead.
9. ecs_globalsync_finished(): the check_plain() is wrong here since no user provided text is involved. Please don't blindly fix false positive warnings from the old Coder Review module and make sure to read https://www.drupal.org/node/28984 again.
10. "$context['message'] = t('Equal: !local - !remote', array('!local' => $local_nid, '!remote' => $remote_node->title));": this is vulnerable to XSS exploits. Since node titles are user provided text you need to sanitize them with the "@" or "%" placeholder in t(). Make sure to read https://www.drupal.org/node/28984 again.

So quite a few security vulnerabilities left.

manual review:

1. _ecs_check_access(): the last line is unreachable code and should be removed.
2. I just realized that this module is almost an identical clone from https://www.drupal.org/project/entitysync . Why did you start a new project? It looks like the old module needs a new maintainer anyway since it has no stable release and the last commits are from 3 years ago? Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Please open an issue in the entitysync issue queue to discuss what you need. You should also get in contact with the maintainer(s) to offer your help to move the project forward. If you cannot reach the maintainer(s) please follow the abandoned project process.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.