[D7] Amber

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

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.

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.

hi jlicht,

i confirmed it has no problem with pareview. sh
I tried installing the amber module it work smoothly.

I have two recommendations:

1. Would be great if you can separate the html iframe markup on seperate tpl using hook theme.
2. I found some serious floss on .install file

sites/all/modules/contrib/amber/amber.install:
+14: [critical] Potential problem: drupal_set_message() only accepts filtered text, be sure all !placeholders for $variables in t() are fully sanitized using check_plain(), filter_xss() or similar.

Thanks,

fusionx1

Please add all your reviews of other projects to the issue summary so that we can better track them.

manual review:

1. Looks like your module is too large for the online pareview.sh service, so it is not able to finish the request. I have attached the coding standard problems from a local run when the libraries folder is removed.
2. project page: how does the module work in the Drupal context? Does duplicate only nodes or arbitrary pages? Does it work with a web service API? How are the page duplicates created? I think the general description on Amber is less relevant than describing what the module does (you can always link to Amber docs elsewhere).
3. libraries folder in the module: so this seems to be your own external library? If that is the case you should include it the same way as you do with the Amazon SDK and not directly commit the code to the repository.
4. amber_dashboard_delete_item(): this is vulnerable to CSRF attacks. When you are performing data changing operations you need to confirm user intent. The form API does that for you with CSRF tokens, if you are not using that you need to validate CSRF tokens yourself. See http://epiqo.com/de/all-your-pants-are-danger-csrf-explained and https://docs.acquia.com/articles/protecting-your-drupal-module-against-c... . 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.

Forgot pareview.sh output.

Review of the 7.x-1.x branch (commit 18f7105):

• 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.
• Codespell has found some spelling errors in your code.
  

  ./amber.batch.inc:262: succesfully  ==> successfully
  ./amber.module:569: Sucessfully  ==> Successfully
  

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. still tons of automated review errors - please take a look at them. At least doc blocks on function should be there to indicate a hook implementation or the purpose of the function.
2. project page: still no information how the module works within Drupal. Can you copy the info from the README?
3. instead of "return drupal_access_denied();" you should use "return MENU_ACCESS_DENIED;" when to CSRF token is invalid.
4. amber_entity_update(): if you are only targeting nodes you should use hook_node_update() instead. Same for the insert hook.
5. node/%/cache looks vulnerable to CSRF exploits - it triggers a database update without verifying intent. Since only amber admins are allowed to do this I'm assuming this is the same CSRF vulnerability as with the delete operation we had above.
6. Wait a second ... the module archives arbitrary web sites including javascript on disk, correct? Then it delivers the foreign javascript assets under the domain of the Drupal site, thereby completely killing the same origin policy for javascript. If an attacker puts up a dangerous Javascript file on their site and you archive that site it means the dangerous JS can be delivered from your domain. Perfect for XSS! Am I missing something? How do you protect users from dangerous external javascript that suddenly runs in the context of your site under amber/cacheframe/%/assets? Looks like this is a severe security breach for any Drupal site using that module?

Nice security challenge - indeed I don't see a way out of the sandboxed iframe with the referrer checks, so that looks good.

Unfortunately you are storing your cache in the public file system. Which means I can directly link to a malicious site HTML with something like http://drupal-7.localhost/sites/default/files/amber/31ae08526f2d01078f94... . Any JS in there will be fully executed in the context of your site ==> XSS.

You either need to move your cache to the private file system (which must be outside of the docroot) or you store your cache in the database instead.

Saving snapshots now fails because the module tries to create a directory in /31ae08526f2d01078f948ea54008ed24 for example in the root file system. Can you check that the private files conversion works so that I can test again?

Did a clean install again and made sure I had a private files directory set up before installing Amber, which worked. I think you should check that the private file system works on install to avoid errors.

manual review:

1. There are still tons of Coder errors for example in the module file, attaching the results of a run. Make sure to run the recent Coder 8.x on your code.
2. require_once(libraries/AmberStorage.php): failed to open stream: No such file or directory amber.install:53. PHP Fatal error: require_once(): Failed opening required 'libraries/AmberStorage.php'. Happens when uninstalling the module. But the AWS library should not be a hard requirement right?
3. "if (strcmp($expected_referer, $referer_uri) === 0) {": why not simply $expected_referer === $referer_uri ?

But otherwise looks good to me. Assigning to Naveen as he might have time to take a final look at this.

no objections for more than a week, so ...

Thanks for your contribution, Jeffrey!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.