[D7] Commerce Amazon MWS

Git clone command for the sandbox is missing in the issue summary, please add it.

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.

Hi splendidles,

I just reviewed your module.

Individual user account

You appear to follow the Individual user accounts guidelines.

Duplication

Your module seams unique.

Master Branch

Your sandbox has a master branch

Licensing

The Amazon MWS MarketplaceWebService libraries are licensed with Apache License 2.0. I am not sure it is possible to license those under GPL version 2.0 as specified on http://www.gnu.org/licenses/license-list.html#GPLCompatibleLicenses:

Apache License, Version 2.0 (#apache2)
This is a free software license, compatible with version 3 of the GNU GPL.

Please note that this license is not compatible with GPL version 2, because it has some requirements that are not in that GPL version. These include certain patent termination and indemnification provisions. The patent termination provision is a good thing, which is why we recommend the Apache 2.0 license for substantial programs over other lax permissive licenses.

Readme.txt

I think your Readme file needs a seperate Requirements and Installation section.

Security Code

Your module appears to use db_select, etc safely.

Coding Style

I noticed that your lines become long in most files. Please consider breaking them accross a few lines to make them more readable.

Best Regards,
Helgi.

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.

This is a huge module. Reviewed as much as I could though. Very nice job!!! Moving to RTBC.

Automated Review

[Best practice issues identified by pareview.sh http://pareview.sh/pareview/httpgitdrupalorgsandboxsplendidles2629252git big list here. Should probably be addressed.

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 the guidelines for individual user accounts.

No duplication

No: DOES NOT Cause module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

No: Does not follow the guidelines for in-project documentation and/or the README Template.
Could use a few more of the required headers of the README template... README.txt is very detailed though. This doesn't seem like a huge issue.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

1. don't bother wrapping menu titles and decscriptions in the t() function
2. Make sure to use t() in user facing text in commerce_amazon_mws.admin.inc
3. A few of your function arguments don't match drupal coding standards. (Spaces in between = ) Noticed this in commerce_amazon_mws.tasks.inc
4. protected function getFieldValue($field_id, $field_key) has a loop that is doing nothing.
5. protected function getField can you break out of that switch if the condition is met? Same in some other places as well...
6. ...]

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.

Hi splendidles,

You seem to be missing some basics on this project application page which is a link to the sandbox of your project and a git clone command for reviewers who would want to download and test your project. Changing this to Needs work.

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.

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: https://www.drupal.org/node/2792673

Project 2: https://www.drupal.org/node/2642870

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

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

Closed other duplicate and re-opening this one as it is nearly done and the other one is brand new.

Evan already addressed this, but the getFieldValue function in commerce_amazon_mws.Mapping.php still has that foreach loop that doesn't do anything.

The only other big thing I noticed is in commerce_amazon_mws.reports.inc and commerce_amazon_mws.tasks.inc you make a lot of use of a "magic" number. Specifically in:

  drupal_set_time_limit(1200);

Other than that the code looks good to me.

Git errors:

• It appears you are working in the "7.x-1.x-dev" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
• The following git branches do not match the release branch pattern, you should remove/rename them. See https://www.drupal.org/node/1015226
  

  * 7.x-1.x-dev
    remotes/origin/7.x-1.x-dev
    remotes/origin/HEAD -> origin/7.x-1.x-dev
  

Review of the 7.x-1.x-dev branch (commit a6667b2):

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• ESLint has found some issues with your code (please check the JavaScript coding standards). See attachment.
• DrupalPractice has found some issues with your code, but could be false positives. See attachment.
• Codespell has found some spelling errors in your code.
  

  ./includes/commerce_amazon_mws.tasks.inc:276: dependant  ==> dependent
  

• 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. commerce_amazon_mws_install(): drupal will install the schema of the module automatically for you, so this hook can be removed.
2. commerce_amazon_mws_reports_delete_report(): filter_xss() is wrong here since you are not printing anything to HTML. Make sure to read https://www.drupal.org/node/28984 again and only apply it when you are actually printing variables to HTML.
3. Most doc blocks are wrong. We don't use @description in Drupal. Don't repeat the function name on the first line of the doc comment, just put the description there. See https://www.drupal.org/docs/develop/coding-standards/api-documentation-a...

There is a security vulnerability in this module and as part of our Git admin training I'm assigning this to visabhishek so that he can take a look. If he does not find anything in one week I'm going to post vulnerability details.

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

@klausi : Thanks for this assignment

I have manually reviewed the code, found the below issues.

Form elements - #value of #type markup and item need to be safe.
Some examples are :

  $form['amazon_mws_active_listing_report']['action_button'] = array(
    '#type'   => 'button',
    '#value'  => 'delete',
    '#attributes' => array('class'=>array('commerce_amazon_mws--delete-reports'))
  );

AS

  $form['amazon_mws_active_listing_report']['action_button'] = array(
    '#type'   => 'button',
    '#value'  => t('delete'),
    '#attributes' => array('class'=>array('commerce_amazon_mws--delete-reports'))
  );

$header = array('Report Id', 'Date');
as
$header = array(t('Report Id'), t('Date'));

Table $rows values requires sanitization :

Ex: function commerce_amazon_mws_reports_display_report

   
if (in_array('item-name', $header)) {
          if(!empty($row['item-name']) && $row['item-name'] != $product->title) {
            $data['data']['item-name'] = $row['item-name']
              . '<div class="differing-title">' . $product->title . '</div>';
            $data['class'][] = 'title-mismatch';
            $no_issues = false;
            $title_mismatch_count++;
          }
        }

$row['item-name'] should be passed through filter_xss() or filter_xss_admin() function to sanitize the values.

function commerce_amazon_mws_reports_build_report_queue_list() {
  module_load_include('inc', 'commerce_amazon_mws', '/includes/commerce_amazon_mws.report_types');
  $queues = commerce_amazon_mws_reports_report_types();

  $header = array('Request ID', 'Report Type', 'Last Time Checked');
  $rows = array();

  foreach($queues as $queue_type) {
    if($queue = CommerceAmazonMWSCache::getCache(base64_encode($queue_type), 'report_system_queue')) {
      foreach ($queue as $row) {
        $rows[] = array($row['request_id'], $row['report_type'], date('Y-m-d H:i:s', $row['last_checked']));
      }
    }
  }

  return theme('table', array('header'=> $header, 'rows'=> $rows));
}

$row['request_id'] should be passed through filter_xss() or filter_xss_admin() function to sanitize the values.

@visabhishek: thanks for taking a look.

Form elements: you are only suggesting to wrap user facing things in t(), which is good advice to keep things translatable, but is not a security risk. Only static strings controlled by the module author are used in your example, so there is no XSS threat? Or did I miss something?

commerce_amazon_mws_reports_display_report(): yes, it looks like row['item-name'] is user provided text which needs to run through check_plain() for sanitization. Same for $product->title which is printed unsanitized to table markup here, so that is also an XSS vulnerability. @splendidles make sure to read https://www.drupal.org/node/28984 again. Note that I think check_plain() is more appropriate here because we don't want any HTML tags rendered in request IDs or item IDs or product titles.

Then there is also a CSRF vulnerability. admin/reports/commerce_amazon_mws/delete does not confirm user intent, see http://epiqo.com/en/all-your-pants-are-danger-csrf-explained and https://docs.acquia.com/articles/protecting-your-drupal-module-against-c...

Then there are also lots of untranslated strings in the module like 'The report with id: ' . $report_id . ' does not exist'. Make sure that all user facing text runs through t() for translation.

@visabhishek
@klausi

Hey guys, thanks for the thorough review.

I've made some commits over the last week or so and I think I've resolved all the issues that have been brought up since splendidles last commit.

Here's a general overview of what was done:

• Default branch changed to 7.x-1.x.
• Lots of code/comment/docblock formatting via phpcbf to meet Drupal coding standards (with manual review as well).
• Some minor data validation where obvious.
• E.g: docblock states data must be unserialized before being passed to method. Created a check inside method to ensure data is not serialized before continuing.
• Wrapped output in t() where needed.
• Utilized check_plain() or filter_xss() as recommended.
• Generated a token in the reports delete form to be checked at admin/reports/commerce_amazon_mws/delete to avoid CSRF. If the token doesn't exist, access is denied.

Hi cornifex,

You covered most of the things. Some quick reviews points which are still pending :

1:Please delete all variable on hook_uninstall()
EX:

variable_set('amazon_mws_report_last_run', $time_now);

2: Please wrap user facing things in t(), i can see unwrapped text on so many places
EX : function commerce_amazon_mws_admin_form()

 $form['amazon-mws-production'] = array(
    '#type'         => 'fieldset',
    '#title'        => 'Production Settings',
    '#collapsible'  => TRUE,
    '#collapsed'    => TRUE,
  );

3: All JavaScript code MUST be declared inside a closure wrapping the whole file and this closure MUST be in strict mode.
https://www.drupal.org/docs/develop/standards/javascript/javascript-codi...

Thanks, visabhishek!

Newest changes:

• Added uninstall hook to clean up variables.
• Wrapped all applicable text in t().
• JS was already wrapped in a closure, added 'use strict'; inside closure.

Automated Review

There's a lot of issues found ... lots of comment formatting and nit picks. #2831865: Change formatting to adhere to Drupal Standards

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 the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Does follow the licensing requirements.

3rd party assets/code

No: Does not follow the guidelines for 3rd party assets/code.

See #2830601: Non-GPL code can not be included on Drupal.org to follow up.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and/or the README Template.

Extensive README.txt file.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements.

Confirmed check_plain() and t() are used extensively and effectively.

Coding style & Drupal API usage

• See #2831865: Change formatting to adhere to Drupal Standards
• Also, there's still a number of user-facing text that isn't wrapped in a t() in the commerce_amazon_mws.module file and the .install file.

This review uses the Project Application Review Template.

After I posted this comment, cornifex invited me to look a little deeper into the two major issues I located in my last review:

1. GPL issue with included 3rd party libraries.

Turns out, these have been removed for awhile. The issue hasn't been updated, but they have been gone for months

2. t() missing on strings

Turns out, all t()'s that I thought were missing were in database schema instances and hook_menu()s ... both are exempt from those requirements.

That leaves the code formatting issues, which are all admittedly nitpicky and minor requirements around documenting parameters and functions. And, as the review template suggests, this is not enough to remove the need for the project from a full project.

Therefore, I deem this project RTBC'ed.

manual review:

1. Still lots of coding standard problems reported by pareview.sh. Please fix those.
2. "t('Cron Processed Reports') . ' (' . commerce_amazon_mws_reports_cron_processed_report_count() . ')',": do not concatenate variables to translatable strings, use placeholders with t() instead. Also elsewhere. Please check all instances where you use t() and concatenate something to it.
3. commerce_amazon_mws_reports_display_report(): do not call theme() here, just return a nested render array. Drupal core will render it later for you and it is easier for other modules to alter it. Also elsewhere for all you page callbacks.
4. commerce_amazon_mws_reports_cron_processed_report_count(): instead of iterating over all rows in PHP you should issue a count query instead. See https://www.drupal.org/docs/7/api/database-api/dynamic-queries/count-que...
5. commerce_amazon_mws_report_form_submit(): doc block is wrong, this is not a hook but a form submission handler. See https://www.drupal.org/docs/develop/coding-standards/api-documentation-a...
6. drupal_get_token(): you should pass a $value parameter to it to make the token unique for this action. Example: 'commerce_amazon_mws_delete'
7. "return 'The report with id: ' . $report_id . ' does not exist';": all user facing text must run through t() for translation.
8. commerce_amazon_mws_reports_display_cron_processed_report(): this prints a table with the rows coming from the amazon_mws_cache DB table. I think you should sanitize the rows here because the data in it is coming from a third party source and therefore should be considered untrusted. Maybe I'm missing something and this is already done in the PHP library you are using? Could you clarify that and add a comment in this function?

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.