[D7] Pinto Image Formatter

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.

I tested the module on my drupal site, but not works well.

1. pinto js not works when I resize the browser window. No js errors show in console. So strange for it.
2. It's not include js library in hook_init(), Instead, you can put it in hook_field_formatter_view().
3. It's not necessary to put your frontend files(js, css, tpl) in theme folder. Instead, you could create js, css and templates folder for each file. or put these under module root directory directly.
4. Reorder items in you *.info file. you could refer to https://www.drupal.org/node/542202
5.Please, Provide a link to your pareview review on the issue summary.

That's all, Thanks.

@yonko.tsvetkov

Yes, It's works well on Garland theme. not Bartik.

Hi,

I have had a look through your module and good news is it worked first time for me. I have found a few point that you might consider.
1: Should you be using drupal.behaviors to attach your js? as per https://www.drupal.org/node/756722
2: When you enable the pinto image formatter the title of the field is hidden behind the pinto gallery.

This module appears to work as intended. The Pareview output did mention that you have not set a default branch to your git repo, you may want to do that.

Automated Review

There are no errors or warnings:
http://pareview.sh/pareview/httpgitdrupalorgsandboxhenryw2496335git

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: Does follow the guidelines for master branch.

Licensing
Yes: Follows the licensing requirements.

3rd party assets/code
Yes: Follows the 3rd party assets/code requirements.

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.

Hello, @yonko.tsvetkov

I reviewed your module and didn't find any problems in your code:

Automated Review

There are no issues identified by pareview.sh:
http://pareview.sh/pareview/httpgitdrupalorgsandboxwebriddle2525442git

Manual Review

Individual user account

Follows the guidelines for individual user accounts.

No duplication

Does not cause module duplication and/or fragmentation.

Master Branch

Follows the guidelines for master branch.

Licensing

Follows the licensing requirements.

3rd party assets/code

Follows the guidelines for 3rd party assets/code.

README.txt/README.md

Follows the guidelines for in-project documentation

Code long/complex enough for review

Follows the guidelines for project length and complexity.

Secure code

Meets the security requirements.

I noticed you fixed the problem noticed by @thatpatguy so I think this module is ready for the production use.

Removing review bonus tag, you have not all manual reviews, 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.

manual review:

1. "t('Image style') . ':</strong> ' . $image_styles_default": do not concatenate translatable strings with variables like that, us placeholders with t() instead. See https://api.drupal.org/api/drupal/includes!bootstrap.inc/function/t/7
2. There are multiple security issues in the code of this project. As part of our git admin training I'm assigning this to Manjit so that he can take a look if he has time. If he does not find anything I'm going to 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.

Sorry for the delay, now revealing the security vulnerabilities:

1. pinto_image_formatter_field_formatter_settings_summary(): this is vulnerable to XSS exploits. Variables such as $default_link are user provided text and need to be sanitized before printing. In your case using the appropriate placeholders with t() would solve the problem. Make sure to read https://www.drupal.org/writing-secure-code again.
2. pinto_image_formatter_field_formatter_view(): This is also vulnerable to XSS attacks. If I enter <script>alert('XSS');</script> as title or alt text for an image then you will get nasty javascript popups. Again, make sure to sanitize user provided text before printing to HTML. This is more severe, since anyone that can create content on the site can perform the XSS attack on admins.

Thanks a lot Klausi, Actually i had not found these.
@yonko.tsvetkov Can you please look into these then i will retest once you fixed.

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.

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

1. Looks good! Author seems to have a clear understanding of what's happening. Module installs. Bada bing!

This review uses the Project Application Review Template.

manual review:

1. pinto_image_formatter_field_formatter_settings_summary(): Variables such as $default_link are now double escaped, which is bad. The "@" placeholder with t() already sanitized variables for you, so the additional check_plain() should be removed.
2. pinto_image_formatter_field_formatter_settings_form(): the check_plain() calls are wrong here because the Form API will already sanitize #default_value and #options for you. Make sure to read https://www.drupal.org/node/28984 again.
3. pinto_image_formatter_field_formatter_view(): this is still vulnerable to XSS. If I enter 0});alert('XSS');});})(jQuery);// as Margin X in the field formatter settings then there will be a nasty javascript popup. Do not concatenate the settings variables to Javascript directly, use Drupal.settings to pass down variables from PHP to Javascript. See https://www.drupal.org/node/756722

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.