Reviews and Mentoring for er.pushpinderrana

We encourage users to post events happening in the community to the community events group on https://www.drupal.org.
You are viewing a wiki page. You are welcome to join the group and then edit it. Be bold!
Posted by er.pushpinderrana on July 25, 2014 at 3:54am
Last updated by er.pushpinderrana on Thu, 2014-09-18 08:04

This wiki page is created to keep track of er.pushpinderrana's manual reviews of projects in the project application queue. As a reviewer I hope to get some advice from more experienced reviewers. Please use the comment field to add your advice, corrections, and observations.

Reviews List:

Full Project Promote Issues:

  1. [D7] UserPoints OG
  2. [D7] Password Reset Form Tweaks
  3. [D7] Markdown Insert
  4. [D7] Media Prezi
  5. [D7] Mailman Simple
  6. field slide show j360
  7. [D7] Trebutra - Trello Bug Tracker
  8. [D7] Commerce Authorize.Net Card Present
  9. [D7] User recurring subscription
  10. [D7] webform_country_list
  11. [D7] Apachesolr repeating dates
  12. [D7] Taxonomy Tracking
  13. [D7] State Transition Control
  14. [D7] Chained Dropdown
  15. [D7] Slidesjs Slider
  16. [D7] LinkedIn Company API Suite
  17. [D7] Thundersearch
  18. [D7] Computed field order
  19. [D7] Disable Drupal User
  20. [D7] Limit Visit
  21. [D7] GPA Calculator
  22. [D7] Activity Stream Instagram
  23. [D7] Hover Card
  24. [D7] Comment Smiley
  25. [D7] Context Cookie
  26. [D7] Video Embed UStream
  27. [D7] Commerce Costs Profits
  28. [D7] User IP address
  29. [D7] orgmode
  30. [D7] Vertical Tabs Responsive
  31. [D7] OSM Route
  32. [D7] Dynamic Zendesk Forms
  33. [7] Limit Publishing Options
  34. [D7] Image styles mapping
  35. [D7] OG Admin Block
  36. [D7] Availability Calendars Show Availability
  37. [D7] Permissions by Term
  38. [D7] GPX rename
  39. [D7] Views Private Access
  40. [D7] Menu Export
  41. [D7] World of Warcraft Tools

Single Project Promote Issues:

  1. [D7] Commerce Multicurrency For Yahoo Finance
  2. [D7] Content Admin Access
  3. [D7]countryicons_bw
  4. [D7] Administration menu exclude
  5. [D7] Bulk apply comment settings
  6. [D7]Chatwee
  7. [D7] SearchIndex Wipe

Security Issues:

  1. [D7] Setup of country get list
  2. [D7] transferuj.pl
  3. [D7] Document Library
  4. BACnet [D7]
  5. [D7] Twitter Hashtag
  6. [D7] Commerce Compare Popup
  7. [D7] User profile override
  8. [D7] Shortened Comments
  9. [D7] Listserv
  10. [D7] Most Popular RSS
  11. [D7] Ubercart Ratefinder (NZ Post)
  12. [D7] Rdbanners
  13. [D7] Helpfulness Module
  14. [D7] Wikiloc

Comments

Thanks for all the help

Posted by mpdonadio on August 7, 2014 at 3:58pm
mpdonadio's picture

Thanks for all the help reviewing! A few quick notes.

Using the review template can help make sure you are covering all of the necessary points in addition to the code walkthrough (duplication, third-party code, etc). It also helps the applicants know what is a blocking issue, what is a strong recommendation, and what is friendly advice.

Adding in the results of PAReview, or the git short checksum can help reviewers down the road focus on what has changed.

Also, make sure you set the status when you are done.

Keep up the good work.

While I appreciate that

Posted by kreynen on August 7, 2014 at 9:05pm
kreynen's picture

While I appreciate that thanking people for reviewing projects helps keep them motivated to do otherwise thankless work, I'm not going to pretend everything is rainbows and unicorns with these bonus karma seeking reviews. The first part of @er.pushpinderrana's review of [D7] Taxonomy Tracking was helpful, but when you don't know anything about the code a module integrates with just say that and leave it at that. Who wants to read a review of their code that starts with "I have never used XXX, but even though I'm completely uninformed these are the things I think you should change..."

Reviewers should already be aware of any similar modules to what's being reviewed. If a previous reviewer hasn't posted a list of similar modules, that should be done before anyone starts looking at git branches or whitespaces. In the case of integrating with other systems, if the reviewer they doesn't know anything about the other system they should encourage the developer being reviewed to reach out to the maintainers and contributors to projects already integrating with that code for reviews.

I would much rather spend a few minutes looking at code from a new developer working on Drupal/CiviCRM integrations than let them experience what happened in [D7] Taxonomy Tracking and I'm sure @coderdan, @jimurl, and @GinkgoFJG would also find the time to help if asked.

Thankyou for your valuable comments, mpdonadio and kreynen

Posted by er.pushpinderrana on August 8, 2014 at 4:00am
er.pushpinderrana's picture

Thankyou for your valuable comments, mpdonadio and kreynen!

@mpdonadio, I understand using the review template is so useful, it provides detailed understanding of whole review. I would always use this template in my future reviews.

As you mentioned "set the status" when you are done, I always try to switch status after my review but sometimes I unsure whether the specific thing is blocker or not. Even I also try to mention why I am not changing the status as you can see in one of my recent review [D7] GPA Calculator, but thanks to @lokapujya to suggest me it can be moved to Need Work :).

I really appreciate all such participants who make the thing easier and work as a team. Its a great learning for me, improving myself with every review. I would take care of every note as you mentioned in my future reviews. thanks :)

@kreynen, I highly appreciate your kind comments and feedback, specifically for [D7] Taxonomy Tracking, was really helpful.

I appreciate your feedback and want if you or any other git admin have anything to say about by review "style" (in particular about things that can be improved) that would be a great bonus for me.

Pushpinder Rana

Good work, impressive list of

Posted by klausi on August 8, 2014 at 7:42am
klausi's picture

Good work, impressive list of reviews already!

Project application reviews are basically sanity checks, so while providing all details what can be improved is very valuable we have to consider what issues are really blocking approval. If we are confident that the people basically know what they are doing then we should not hold them back. With the exception of security issues and licensing issues.

So extra points for finding security issues, could you point them out in this wiki page if you find some? We use the PAReview: security tag to track security issues, so you can go looking what we usually find.

I also saw that you asked people to put more code into short projects, which I think we should not do. Short projects are fine, we just manually approve them.

I think you are on a good way to become a git administrator on drupal.org, if you want to you. Thanks!

Thanks for a great feedback,

Posted by er.pushpinderrana on August 9, 2014 at 3:48am
er.pushpinderrana's picture

Thanks for a great feedback, klausi!

I would take care of all points that you mentioned above. Other than this I am also actively looking into reviews done by you and other git admin users. Also you mentioned about the security issues, I am deeply analyzing PAReview: security tag projects and other documentations that are so useful.

I also saw that you asked people to put more code into short projects, which I think we should not do. Short projects are fine, we just manually approve them.

Yeah, I would take care of this in future. Great thanks for your guidance!

Yes, I have plan to become a git admin and I am highly excited about this. Though I am actively involved on drupal community and want to contribute as much as possible but Reviewing of projects is a great contribution and am eagerly working on this. I love this drupal community as every people work as a team here.

I am sure your mentoring would help me to become a code review administrator :)

P.S: heddn provides some good suggestion on following link, very useful. https://groups.drupal.org/node/437283#comment-1051863

Pushpinder Rana

Cool, whenever you have

Posted by klausi on August 9, 2014 at 10:13am
klausi's picture

Cool, whenever you have questions you can also join us on IRC in #drupal-codereview and ask us there!

While shortly looking over

Posted by klausi on September 2, 2014 at 9:27pm
klausi's picture

While shortly looking over your review comments I think they are all fine. I would like to take a closer look at reviews where you identified security issues - could you group them into a section in the list above?

You have been in the regular reviewer team for some time now, I would like to bring you into the git admin team soon :-)

klausi, a big thanks to you

Posted by er.pushpinderrana on September 3, 2014 at 3:32am
er.pushpinderrana's picture

klausi, a big thanks to you for taking out time to review my reviews comments. As you asked, I have grouped all security reviews comments in a separate section. I would highly appreciate your feedback, advise, corrections and observations on my reviews.

You have been in the regular reviewer team for some time now, I would like to bring you into the git admin team soon :-)

I love to review applications, at least one application per day and also encourage others to do the same :)

Pushpinder Rana

I saw that you recommended

Posted by klausi on September 5, 2014 at 10:45am
klausi's picture

I saw that you recommended once to sanitize variables that go into $_SESSION. That is not correct, you should only sanitize variables that go directly to HTML (filtering on output). Since $_SESSION is just a store in the database in should contain variables unaltered (Drupal's filtering on output only philosophy). Could you correct that in your comment in https://www.drupal.org/node/2320783#comment-9060805 ? No harm was done since another reviewer pointed out the mistake afterwards.

Same here: https://www.drupal.org/node/2224419#comment-9119249 . Don't sanitize variables when you pass them to cURL. It will issue a HTTP request to some other site, but it will not print anything to HTML. Therefore sanitization would be wrong here. Could you correct that comment as well?

Sometimes you point out that user permissions don't exist. It is perfectly fine if people re-use permissions from Drupal core if they relate to their functionality. "administer users" for example is provided by the core user.module and it is fine to use that if your module deals with user related tasks. If people use permissions that are not defined then that is not a security issue because the system will just not grant access on permissions that don't exist (only user 1 will always pass user_access()). You should not list permission problems as security issues in this case.

If you think you found security issues it is important to take the mindest of an attacker to try to exploit them. Can you trigger the XSS that you think you found in the variables passed to cURL? How can you trigger it on your local dev site? Can you access some functionality as attacker if Drupal is confronted with a permission that does not exist? Experimenting with such things takes a bit of time but it will make it much more clear to you how and where things are exploitable.

Your other reviews look fine, I like the XSS screenshots to demonstrate your point.

I have a sandbox module that

Posted by mpdonadio on September 5, 2014 at 1:28pm
mpdonadio's picture

I have a sandbox module that may help with the XSS testing: https://www.drupal.org/sandbox/matthew.donadio/2319347

I don't always use it, but I sometimes will. I may push out some more changes this weekend.

Nice Module and quick solution to fill XSS related data!

Posted by er.pushpinderrana on September 6, 2014 at 8:16am
er.pushpinderrana's picture

@mpdonadio, it's a nice module and very useful specifically for big forms. Prefill All and Prefill Empty, both options gives a quick solution to play with form values that avoid extra overhead of filling each form value one by one.

Nice Idea! I am really looking forward to make something similar for doing such type of experiments :). Your module is one of from these, a good one!

Thanks to you for sharing this.

Pushpinder Rana

I truly appreciate your

Posted by er.pushpinderrana on September 6, 2014 at 8:04am
er.pushpinderrana's picture

I truly appreciate your feedback, klausi!

I saw that you recommended once to sanitize variables that go into $_SESSION. That is not correct,..

I absolutely agree with your comment on $_SESSION issue, corrected the same on
https://www.drupal.org/node/2320783#comment-9060805. I accept this, it can't be consider as security issue. But In one of my project in past, I have experienced, if I assign a value to $_SESSION it also disables the cache for anonymous users(if not used too carefully) because once a session cookie is set, the _drupal_bootstrap_page_cache() won't load the cached page. Please correct me If I am wrong here.

Don't sanitize variables when you pass them to cURL...

I have also corrected my comment here https://www.drupal.org/node/2224419#comment-9119249. Thanks for guiding me as you mentioned It will issue a HTTP request to some other site, but it will not print anything to HTML definitely agreed on this.

Sometimes you point out that user permissions don't exist. It is perfectly fine if people re-use permissions from Drupal core if they relate to their functionality...

I would take care of this, completely agree with you, this issue can't be part of security issue. I think unintentionally, I have listed out this one under security category. Yes, re-usability always a recommended concept everywhere, hope take care of this too :)

If you think you found security issues it is important to take the mindest of an attacker to try to exploit them...

I am looking into such type of concepts and experiments, improving regularly :). Git admin reviewers's comments also so useful and help me to do something new, do some security related experiment , it's a nice learning. I am also keeping my self involves in this as much as possible.

To be honest, I am really enjoying every comment/feedback by you and other reviewers; seriously you are an excellent mentor and have inspired me to continue learning with an open and positive mind.

Once again, thank you so much for your time and guidance!

Pushpinder Rana

I think the XSS issue that

Posted by mpdonadio on September 10, 2014 at 11:22pm
mpdonadio's picture

I think the XSS issue that @klausi mentions also appears in [#2326909-24].

@mpdonadio, a big thanks to

Posted by er.pushpinderrana on September 11, 2014 at 3:31am
er.pushpinderrana's picture

@mpdonadio, a big thanks to you for pointing out these issues, corrected the same. Also I found https://www.drupal.org/node/2326909#comment-9138903 very useful, specifically Why does Drupal filter on output?.

I look forward to seeing you again and again on this page :)

Again a warm thank you for all your help, guidance and support.

Pushpinder Rana

I hope in some of my most

Posted by er.pushpinderrana on September 19, 2014 at 5:53am
er.pushpinderrana's picture

I hope in some of my most recent reviews, you will found some improvements as some new things just learned.

I'll always keep an eye on the admin-reviewers's reviews to improve my own as I'm aware I have still quite some work to do.

@mpdonadio, I need your suggestion on https://www.drupal.org/node/532400#comment-9151471.

@klausi, I'm expecting your thoughts on https://groups.drupal.org/node/405658#comment-1035298.

Sometimes during reviews I feel some doubts but luckily I get answer of same from your reviews :)

I am looking forward to hearing from you.

Pushpinder Rana

I think it is time now to

Posted by klausi on September 19, 2014 at 8:48am
klausi's picture

I think it is time now to promote you as git administrator: https://www.drupal.org/node/2341261

Please comment all on this issue to +1 that or raise any concerns you might have.

Thanks for your help and

Posted by er.pushpinderrana on September 21, 2014 at 6:52am
er.pushpinderrana's picture

Thanks for your help and support, its really a great moment for me :). I will do my very best to surpass your expectations of me.

I will continue my learning, hope day by day you will see improvements. Thanks again for this promotion.

Pushpinder Rana

Congratulations Pushpinder! I

Posted by klausi on September 21, 2014 at 11:08am
klausi's picture

Congratulations Pushpinder!

I added you to the list of git admins at https://groups.drupal.org/node/142454

Doc page how to promote user accounts: https://www.drupal.org/node/1125818

As always feel free to ask us any questions here or in IRC in #drupal-codereview.