Overview

This module is providing a custom block which can be placed in any region and configured to display a certain number of items from the specified cision feed source.

Demo Feed

The following feed can be used for testing:
http://publish.ne.cision.com/papi/NewsFeed/A275C0BF733048FFAE9126ACA64DD08F

Project Information

Project Page: https://www.drupal.org/project/cision_block
Git clone: git clone --branch 7.x-1.x https://git.drupal.org/project/cision_block.git

Manual reviews of other projects

- https://www.drupal.org/node/2873393#comment-12154659
- https://www.drupal.org/node/2414107#comment-12157302
- https://www.drupal.org/node/2871158#comment-12081334
- https://www.drupal.org/node/2808203#comment-12157469
- https://www.drupal.org/node/2865054#comment-12083254
- https://www.drupal.org/node/2876837#comment-12084955

Comments

Cyclonecode created an issue. See original summary.

Cyclonecode’s picture

Cyclonecode commented
Issue summary: View changes
PA robot’s picture

PA robot commented
Multiple Applications
It appears that there have been multiple project applications opened under your username:

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

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

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.

PA robot’s picture

PA robot commented

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.

Cyclonecode’s picture

Cyclonecode commented
Assigned: Cyclonecode » Unassigned
Cyclonecode’s picture

Cyclonecode commented
Issue summary: View changes
Cyclonecode’s picture

Cyclonecode commented
Issue summary: View changes
Cyclonecode’s picture

Cyclonecode commented
Issue summary: View changes
Cyclonecode’s picture

Cyclonecode commented

Can someone please give me a review on this module?

tatarbj’s picture

tatarbj
Primary language Hungarian
Location Brussels
commented
Assigned: Unassigned » tatarbj

I'm starting the review.

tatarbj’s picture

tatarbj
Primary language Hungarian
Location Brussels
commented
Assigned: tatarbj » Unassigned
Status: Needs review » Needs work

Hi @Cyclonecode,
here are the details of my review:

Automated review

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

  • 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

* I recommend to not use file_get_contents() in _cision_block_get_feeds() as it doesn't handle reverse proxies, drupal_http_request should be the one in order to make it work in controlled way (not talking about currently it's not error-handled just hided if there is any issue with @)
* As the module fetches 3rd party content that cannot be a trusted source, when you print out parts of it, they should be sanitized in order to avoid XSS vulnerability. The following variables should be handled in _cision_block_get_mapping_source() function: $item['Title']; the properties of $item->Images[0] are also unsanitizedly printed out in an Only local images are allowed. HTML tag that can be used direct XSS exploit (https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet#Image_XSS...)
* I would recommend also to use drupal functions like in cision-item.tpl.php instead of date() use the format_date() (https://api.drupal.org/api/drupal/includes%21common.inc/function/format_...), the text_summary() call could happen _cision_block_get_mapping_source() in order to keep tpl files as clean as it should be.
* As a possible improvement i highly recommend to implement (or use) validations for form items in cision_block_block_configure(), also currently module allows to have only one block, it could be a great feature to have as many as the user wants to define (the one that pops up in my mind is a kinda complex solution with dedicated db table, but shows a nice way of implementing it: http://cgit.drupalcode.org/nodeblock/tree/nodeblock.module#n688)

Because of the 1st and 2nd points seem pretty blocker issues i drop the application back to 'Needs work' status, if i can help you implement them, do not hesitate to contact me :) The other issues are clearly recommendations, even feature requests, i also have some recources to contribute on the module, just let me know if you'd like to work together :)

Bests,
Balazs.

Cyclonecode’s picture

Cyclonecode commented

Thanks for your review @tatarbj

I have made a couple of modifications to resolve the above issues:

- Switched from using file_get_contents() to drupal_http_request() .
- Removed unused feed entries and also added a hook, cision_map_sources, than can be used to alter data for each feed item.
- Sanitize all feed data using check_plain() and check_url() .
- Sanitize block settings on submission.
- Use format_date() instead of date() in template.

Right now I am not removing the call to text_summary() from the template, since I don't want to change the raw values for Intro or Body. I am thinking that it is up to the user how he/she use these two fields. Since I have added a hook that let the other modules override the feed items, one could easily remove the text_summary() (and use the CustomIntro field instead) call from the template and then implement a hook like this:

function MODULE_map_cision_sources(array $item, \stdClass $release) {
  $item['CustomIntro'] = text_summary($release->Intro ? $release->Intro : $release->Body);
  return $item;
}

I would also look into your suggestions on add support to add multiple blocks and of course also to add some basic unit testing.

Cyclonecode’s picture

Cyclonecode commented
Status: Needs work » Needs review
tatarbj’s picture

tatarbj
Primary language Hungarian
Location Brussels
commented
Status: Needs review » Needs work

Hi @Cyclonecode,
i've checked your changes and seems really good job :)
That i still miss is the sanitized image items that are printed out directly, at most (but pretty ugly solution) in tpl is should happen.
Also don't fully get why you removed the sanitization in your second commit: http://cgit.drupalcode.org/cision_block/commit/?id=4ca34c71c4bebc148a756... Could you explain it a bit?
The rest are just beautiful! :)
Bests,
Balazs.

Cyclonecode’s picture

Cyclonecode commented
Status: Needs work » Needs review

Hi @tatarbj

1. According to this post: Handle text in a secure fashion textfields are automatically sanitized so there should be no need do this?

I also get the following warnings from pareview when using check_url() and check_plain() for the block settings inside cision_block_block_save() :

FILE: /root/repos/pareviewsh/pareview_temp/cision_block.module
--------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
--------------------------------------------------------------------------
100 | WARNING | Do not use the check_url() sanitization function when
| | writing values to the database, use it on output to HTML
| | instead
104 | WARNING | Do not use the check_plain() sanitization function when
| | writing values to the database, use it on output to HTML
| | instead
--------------------------------------------------------------------------

2. Since I am sanitizing all fields that is used inside the template in _cision_block_get_mapping_source() I was thinking that there should be no need to use functions such as check_plain() or check_url() inside the template or am I missing something here?

$item['Id'] = (int) $release->Id;
$item['Title'] = check_plain($release->Title);
$item['PublishDate'] = strtotime($release->PublishDate);
$item['Intro'] = check_plain($release->Intro);
$item['Body'] = check_plain($release->Body);
foreach ($release->Images as &$image) {
  $image->DownloadUrl = check_url($image->DownloadUrl);
  $image->Description = check_plain($image->Description);
}
$item['Images'] = $release->Images;
$item['CisionWireUrl'] = check_url($release->CisionWireUrl);
tatarbj’s picture

tatarbj
Primary language Hungarian
Location Brussels
commented
Status: Needs review » Active

Hi @Cyclonecode,
i see you actively implement things on the contrib (http://cgit.drupalcode.org/cision_block/log/) - could you let me know when you feel you finished and i'll be able to turn to check them all in one shot :)
Thank you in advance,
Balazs.

Cyclonecode’s picture

Cyclonecode commented

Hello again @tatarbj, I think I am pretty much finished here, atleast for the moment. So please have another look =)

Cyclonecode’s picture

Cyclonecode commented
Status: Active » Needs review
tatarbj’s picture

tatarbj
Primary language Hungarian
Location Brussels
commented
Assigned: Unassigned » tatarbj

Okido, let me get back to you today afternoon (hope we are in the same timezone :)) -- edit: sorry, it will be tomorrow morning.
Bests,
Balazs.

tatarbj’s picture

tatarbj
Primary language Hungarian
Location Brussels
commented
Assigned: tatarbj » Unassigned
Status: Needs review » Reviewed & tested by the community

Hi @Cyclonecode,
I've reviewed your module again and I have to admit it impressed me even more than firstly it did, great work!
Pareview.sh doesn't report anything (https://pareview.sh/node/1826), also the biggest issues are fixed, even more things happened as 'improvements' with the code base than just the ones that i've recommended before that are correctly implemented.
Regarding the issues that you answered:
1. Watch out, because the question is about specific form api properties that are not sanitized automatically (like #markup), so just keep in mind some tests on inputs that will be later printed to output never hurt ;) (even manual ones)
2. I've missed that detail you set the values via references, so it's also solved, thanks! :)

I would also point a few others, but none of them are blocker ones, just help your users to use this tool:
* I would drop a link to php.net reference page when you show 'See the PHP manual for date formats.' in cision_block_block_configure().
* Even phpcs doesn't detect it in tpl files, i would use some indents just to structurize the file a bit :)

As everything got implemented that were marked blocker, i see no apperant reason to not give a nice RTBC for this application, so let me do it!

Bests,
Balazs.

PS for the rest of the things some issues could be created under the public issue queue of the module in order to follow them up if someone else also would like to contribute on the module, for instance.

Cyclonecode’s picture

Cyclonecode commented

Thanks for the review @tatarbj.
I am not sure what I am suppose to do now, when this module finally has been marked as RTBC?
How does the module get marked as trusted in the project page?

keopx’s picture

keopx
Primary language Basque
Location Basque Country
commented

@Cyclonecode change priority to major, and after 15 days more to critical.

Check this: https://www.drupal.org/node/894256

Cyclonecode’s picture

Cyclonecode commented
Priority: Normal » Major
Cyclonecode’s picture

Cyclonecode commented
Priority: Major » Critical
Cyclonecode’s picture

Cyclonecode commented

I would really like any git administrator to review this project and if possible give me access to opt into security advisory coverage. I have been waiting a long time to mark my module as trusted.

Cyclonecode’s picture

Cyclonecode commented

I still need an administrator to review this module and if possible give me access to opt in for security coverage. I don't understand why this must take so long time.

visabhishek’s picture

visabhishek commented
Status: Reviewed & tested by the community » Fixed

Thanks for your contribution, Krister Andersson!

I updated your account so you can opt into security advisory coverage now.

Here are some recommended readings to help with excellent maintainership:

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.

visabhishek’s picture

visabhishek commented

Assigning Credits.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.