[D7] Div Screenshot

Hi sanjayk,

please prepend [D7] to the title of this issue.

According to the documentation of hook_menu() the title is required for a menu item.

"title": Required. The untranslated title of the menu item.

You should use a stream wrapper 'public://' to save the image in div_screenshot_create().

You should not have 3rd party libraries (like jquery.plugin.html2canvas.js) in your module.

Please pass the string that is returned by your div_screenshot_create() through t().

You should wrapp your JavaScript Code

(function ($) {
.
.
.
})(jQuery);

You are using strtotime('now') to generate the file name for the image? What happens if two images are generated in the same second? Better use drupal_get_token() to generate the image file name, this would also make the file names unpredictable.

Best regards

Hi sanjayk,

Security issue: You really should sanitize/validate the $_REQUEST['str'] before using it in div_screenshot_create() and just copying it into a file.

You should name the libraries directory like the library itself (e.g. jquery.html2canvas) and not like your module. Since your module depends on libraries module you should use this module and implement a proper hook_libraries_info().

If you require an other jQuery version than the core you should use jQuery Update and not just load an other jQuery version from an external resource.

Using drupal_get_token() without an argument would result in the same filename for all images that are generated in the same session. Is that what you wanted? If not then you shoud randomize the filename to pass a dynamic argument (e.g. drupal_get_token(rand()))

You should use file_prepare_directory() to prepare your directory in div_screenshot_create().

Since I have jQuery Update installed I get this:

Notice: Undefined index: misc/jquery.js in div_screenshot_js_alter() (line 49 of /var/www/htdocs/drupal7/sites/all/modules/sandbox/div_screenshot/div_screenshot.module).
Notice: Undefined index: misc/jquery.js in div_screenshot_js_alter() (line 49 of /var/www/htdocs/drupal7/sites/all/modules/sandbox/div_screenshot/div_screenshot.module).

Please remove the package = Custom and the duble dependencies[] = libraries from your info file.

Why this menu item?

  $items['divscreenshot/%'] = array(
    'title' => 'Create screenshot',
    'page callback' => 'div_screenshot_create',
    'page arguments' => array(1),
    'access arguments' => array('access content'),
  );

Your page callback doesn't accept any arguments.

Why this confusing comment?

/**
 * Implements div_screenshot_create().
 */
function div_screenshot_create() { ...

This function is not implementing any hook.

You are still not using the stream wrapper in div_screenshot_create():102

IMHO this module is not complicated enough to prove your drupal skills.

Best regards

Automated Review

[Best practice issues identified by pareview.sh / drupalcs / coder. Please don't copy/paste all of the results unless they are short. If there are a lot, then post a link to the automated review and mention that problems should 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

Yes: 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

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

Code long/complex enough for review

No: Does not follow the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

* All user facing text should be passed through translate t() function. Update user facing text in hook_help() with translate function.

* As you are not altering the javascript and using to just include the libraries and javascript files, remove the div_screenshot_js_alter() and move the code to hook_init() or hook_preprocess_page().

* Do not load jquery.plugin.html2canvas.js file if the file is not available. It may cause file not found javascript error.

+ In div_screenshot_create(), don't just echo strings, use drupal_set_message() instead.

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.

Code too short

This project is too short to approve you as git vetted user. We are currently discussing how much code we need, but everything with less than 120 lines of code or less than 5 functions cannot be seriously reviewed. However, we can promote this single project manually to a full project for you.

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.

Tags are separated by commas, not semicolons.

Removing review bonus tag, you have not done any manual review, you just posted the output of an automated review tool. Make sure to read through the source code of the other projects, as requested on the review bonus page.

Hi sanjayk,

My findings for your module as follows:

1) As suggested by braindrift,
Security Issue: You really should sanitize/validate the $_REQUEST['str'] before using it in div_screenshot_create() and just copying it into a file.

2) You should remove the package = Custom from your info file.

3) Suggestion: Instead of using $_REQUEST['str'], you can create a settings form to save div ids and provide capture functionality accordingly.

Thanks

Hi @sanjayk,

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: Follows the licensing requirements.

3rd party assets/code

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

README.txt/README.md

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

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

Yes: Follows Coding style and Drupal API usage standards.

manual review:

1. project page: what is the use case of the module? Why would I install it on a Drupal site? What problem does it solve? See https://www.drupal.org/node/997024
2. div_screenshot_init(): doc block is wrong, this is a hook implementation and should be documented as such. See https://www.drupal.org/coding-standards/docs#hookimpl
3. div_screenshot_init(): this function will be onvoked on every single page request. You should use hook_requirements() instead.
4. div_screenshot_jquery_ver(): this looks like q requirements check, you should depend on jquery_update in the info file instead.
5. divscreenshot.js: "Loading....": all user facing text must run through Drupal.t() for translation.
6. divscreenshot.js: you get the settings passed in, why do you access Drupal.settings here?
7. div_screenshot_create(): you are hard-coding sites/default/files/div_screenshot/ here, use public:// instead.
8. div_screenshot_create(): so this is an open page callback that receives data and writes it to disk. As an attacker I can send thousands of requests to this page callback and it will just fill up the disk on your server until the server goes down. Sure, if there is a file field on a site that an attacker can access then we have a similar situation, but at least that would be part of the Form API and CSRF protected. And Drupal automatically clears up temporary files when they are uploaded, and new entities show up when many files are uploaded. But in your case there is no log, no restriction on size, no check for duplication and no CSRF protection. I'm not entirely sure how you should solve that - do you really have to store the screenshot on the server? Can't you just stream it back as image file to the user and don't store it at all? I think we should consider the current code as a security vulnerability. 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.

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.