Reviews and Mentoring for th_tushar

Events happening in the community are now at Drupal community events on www.drupal.org.
You are viewing a wiki page. You are welcome to join the group and then edit it. Be bold!
Posted by th_tushar on March 24, 2016 at 1:55pm
Last updated by th_tushar on Mon, 2016-05-02 07:33

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

I would like to thank Klaus Purer(klausi) for guiding me and providing the useful links about this process in Drupal Con Asia 2016.

Also, I would like to thank Matthew Donadio (mpdonadio), Greg Knaddison (greggles), Karl Scheirer (kscheirer) and everyone else for their effort in reviewing the project applications.

Reviews List:

Full Project Promote Issues:

  1. [D7] Term body class
  2. [D7] Div Screenshot
  3. [D7] Time Based Registration
  4. [D7] Views Results Inject
  5. [D7]Video Schema
    Review #1
    Review #2
  6. [D7] JS confirm Pop Up
    Review #1
    Review #2
  7. [D7]Simple tweets
  8. [D7] Commerce Alipay Global
  9. [D7] Steady modules
  10. [D8] Sodium Encryption
  11. [D7] Views Title Callback
  12. [D7] Commerce save address to address on file
  13. [D7] Video Embed UStream
  14. [D7] Simple CSV Importer
  15. [D7] Quiz Result Export
  16. [D7]Auto Refresh Views
  17. [D7] jQuery MobileMenu
  18. [D7] Internal Nodes Linkit
  19. [D7] IME
  20. [D7] Comment Smiley
  21. [D7] Bluzen
  22. [D7] Accordion Field
  23. [D7] Google A/B Testing
  24. [D7] Bulk File Upload
  25. [D6] CurdBee
  26. [D7] Commerce BluePay Hosted Forms
  27. [D7] Share Send Save Integration
  28. [D7] Jquery Clocks
  29. [D7] Relation services

Security Issues:

  1. [D7] PDFer
  2. [D7] IP Locator with Splash
  3. [D7] Uncache
  4. [D7] Meet Our Team
  5. [d8] & [d7] Skillset Inview
  6. [D7] PHP Finder
    Review #1
    Review #2
  7. [D7] prepopulatedfields
    Review #1
    Review #2
  8. [D7] Book outline
  9. [D7] Address Field Email
  10. [D8] Currency Convert
  11. [D7] FullContact

Comments

Thanks for starting this! One

Posted by klausi on March 25, 2016 at 10:51am
klausi's picture

Thanks for starting this!

One major aspect of reviewing project applications is finding security issues. I found one in https://www.drupal.org/node/2621380 and assigned it to you, do you want to take a look?

Finding security issues gives us confidence that you know what we are looking for, so extra bonus points when we consider you to be a git admin on drupal.org :-)

I had a look at

Posted by th_tushar on March 26, 2016 at 6:31am
th_tushar's picture

I had a look at https://www.drupal.org/node/2621380 issue. Thanks for assigning this issue to me.

I found the security issue and posted the same in above issue comments.

Thanks, that looks good, left

Posted by klausi on March 27, 2016 at 1:15pm
klausi's picture

Thanks, that looks good, left a comment there.

Please don't close applications that are small modules - those are perfectly fine, we just can't give the git vetted user role away for them. Please also don't close applications that duplicate existing modules. Duplication is bad and we should point that out the applicants, but it is never a reason to block an application.

Then I also removed a couple of review comments from your wiki page where you did not do a manual code review. We are interested in your code reviews here, so that we can see what kind of issues you find and how you explain that to applicants.

Thanks again for your efforts!

Hi Klaus, Thanks, I saw your

Posted by th_tushar on March 27, 2016 at 1:21pm
th_tushar's picture

Hi Klaus,

Thanks, I saw your comment on issue page.

I am worried about the project duplication. The code may be available in existing project, using which it can fetch them the git vetted user role. What can we do to avoid this?

We can quickly compare the

Posted by klausi on March 28, 2016 at 2:53pm
klausi's picture

We can quickly compare the code with the existing project. If it is almost identical, then we cannot give the git vetted user role away and will only do a single project promotion. If it has been altered then that is fine - we all copy/paste/modify code from other modules, that is the way open source code is supposed to work.

I looked through your other reviews and removed some more non-manual reviews from the list. Otherwise they look good.

Please keep going with your manual reviews and list them here. A couple more security issues on your list would be nice, so keep looking for those. I will assign security issues to you when I find some :-)

Ok. I will look for more

Posted by th_tushar on March 28, 2016 at 3:05pm
th_tushar's picture

Ok. I will look for more security issues. Thanks Klaus :-)

Hi, looks like you gave some

Posted by klausi on March 29, 2016 at 2:06pm
klausi's picture

Hi, looks like you gave some wrong advice about sanitizing data before saving to the database. Make sure to read https://www.drupal.org/node/28984 again: "When handling data, the golden rule is to store exactly what the user typed. When a user edits a post they created earlier, the form should contain the same things as it did when they first submitted it. This means that conversions are performed when content is output, not when saved to the database"

I corrected you in 2 places, could you check your other reviews where you gave that advice and correct it?

Hi Klaus, I have updated my

Posted by th_tushar on March 29, 2016 at 3:18pm
th_tushar's picture

Hi Klaus,

I have updated my comments in both the issues. Will have a look at the https://www.drupal.org/node/28984 again.

Thanks!

I saw that you reviewed book

Posted by klausi on April 5, 2016 at 7:40pm
klausi's picture

I saw that you reviewed book outline at https://www.drupal.org/node/2693095 , but that application was already in the "needs work" state.

We should look at applications first that are in the "needs review" state (or in "reviewed & tested by the community" if you are a git admin).

Can you help me review applications that are in "needs review" and have a review bonus at https://www.drupal.org/project/issues/search/projectapplications?project_issue_followers=&status[]=8&issue_tags_op=%3D&issue_tags=PAReview%3A+review+bonus ? Thanks!

Ok, Klaus.

Posted by th_tushar on April 6, 2016 at 7:21am
th_tushar's picture

Ok, Klaus. I will review applications that are in "Needs review" and have a review bonus added to it. :-)

If you discover a security

Posted by klausi on April 23, 2016 at 11:24am
klausi's picture

If you discover a security vulnerability it is important to tell the applicants what is wrong and how they can resolve it.

1) Describe how you tested the security vulnerability. Example XSS: where did you input malicious javascript? where did it trigger, on which page?
2) Provide links to documentation. Example XSS: https://www.drupal.org/node/28984 is the place to link
3) Tell them where to fix it. Example XSS: in template preprocess functions or when preparing variables that are passed to theme().

Ok Klaus, I will try to

Posted by th_tushar on April 24, 2016 at 10:47am
th_tushar's picture

Ok Klaus, I will try to explain the issues to them in detail.

Found an application w/ a

Posted by mpdonadio on April 23, 2016 at 4:25pm
mpdonadio's picture

Found an application w/ a security problem: https://www.drupal.org/node/2664534#comment-11117509 Assigned it to you to find and report.

Hi @mpdonadio, Thanks for

Posted by th_tushar on April 24, 2016 at 10:51am
th_tushar's picture

Hi @mpdonadio,

Thanks for assigning the issue to me. I have manually reviewed the code and addressed the security issues in the comment https://www.drupal.org/node/2664534#comment-11118429

Cool. Just remember to

Posted by mpdonadio on April 24, 2016 at 5:29pm
mpdonadio's picture

Cool. Just remember to explicitly mention which are the blocking issues (the review template helps with this, https://groups.drupal.org/node/184389). The code suggestions are just that, suggestions. Under current policy, only security, third-party code, and licensing issues should be the only reasons to push something back to Needs Work. I try to always use the template, but if I am following up from another good review, I will trim out the smaller parts.

Also, when you find a security problem, it helps to name the vulnerability and point to the Drupal docs (if they exist) to help the user out. Reviews part part quality control, but also mentoring opportunities.

You found XSS: https://www.drupal.org/node/28984 and CRSF: https://www.drupal.org/node/178896 (@klausi also has an excellent CRSF article).

Another issue w/ two

Posted by mpdonadio on April 24, 2016 at 12:47pm
mpdonadio's picture

Another issue w/ two problems:

https://www.drupal.org/node/2653354#comment-11118631

Hi @mpdonadio, Found the

Posted by th_tushar on April 25, 2016 at 8:47am
th_tushar's picture

Hi @mpdonadio,

Found the issues, posted them in the comment https://www.drupal.org/node/2653354#comment-11120377

I read your review. Pretty

Posted by mpdonadio on April 25, 2016 at 12:51pm
mpdonadio's picture

I read your review. Pretty good.

Remember, under new policy, while important translatable strings and other small things with the info file shouldn't block an application. We should mention them, but not mark them as blocking issues. Security, third-part code, and licensing issues are the big three things we block on (@klausi, do you remember where we documented this?) Of the things you mention, the menu path should be the only blocker.

I am not sure there is a proper name for the security issue you found. I usually called it a "naked menu path", and point users to the hook_menu() entry in the API.

There is another security problem with that module.

Hi @mpdonadio, Posted the new

Posted by th_tushar on April 25, 2016 at 2:35pm
th_tushar's picture

Hi @mpdonadio,
Posted the new findings in comment,

https://www.drupal.org/node/2653354#comment-11121773

I think it is time to promote

Posted by klausi on May 4, 2016 at 2:09pm
klausi's picture

I think it is time to promote you to git admin, webmaster issue: https://www.drupal.org/node/2718827

Hi Tushar, I think you did

Posted by klausi on October 3, 2016 at 7:44pm
klausi's picture

Hi Tushar,

I think you did not really get started as git admin yet - any open questions you have to approve applicants?

No Klausi, I don't have any

Posted by th_tushar on October 4, 2016 at 7:22am
th_tushar's picture

No Klausi, I don't have any questions. I will start reviewing and approving applicants now.