Reviews and Mentoring for Manjit.Singh

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 manjit.singh on March 13, 2015 at 3:22pm
Last updated by manjit.singh on Tue, 2016-04-19 09:52

This page serves project reviews of Manjit.Singh.

https://www.drupal.org/project/issues/search/projectapplications?project...

I have been reviewing projects from past two years, and also submitting some patches in Drupal8.

I would keep on adding projects reviews in wiki page.

Security issues:

[D7] Global Regex : https://www.drupal.org/node/2674524#comment-10897518

[D7] Language Proficiency : https://www.drupal.org/node/2565611#comment-10699264

[D7] Delta theme: https://www.drupal.org/node/2395279#comment-9881975

[D7] DB Track: https://www.drupal.org/node/2455723#comment-9908465

[D7] Responsive Slideshow:
https://www.drupal.org/node/2479197#comment-1018226

[D7] Drupal Commerce Fulfillment
https://www.drupal.org/node/2537394#comment-10258833

[D7] Entity Content Synchronize (ecs)
https://www.drupal.org/node/2540552#comment-10284327

[D7] WET4 Slider
https://www.drupal.org/node/2479471#comment-10465383

  

Other issues:

[D7] Flame
https://www.drupal.org/node/2708265#comment-11097657

[D8] AdobeAnalytics
https://www.drupal.org/node/2682257#comment-10954403

[D7] YouTube Import
https://www.drupal.org/node/2575109#comment-11010325

[D7] Materializecss theme
https://www.drupal.org/node/2596307#comment-10705838

[D7] Fruit Blossom: https://www.drupal.org/node/1986548#comment-7406196

[D7] Parrot : https://www.drupal.org/node/1891548#comment-7048772

Elegant Blue: https://www.drupal.org/node/1924326#comment-7128242

Mainstream Responsive Theme : https://www.drupal.org/node/1942092#comment-7186970

[D7] Boot Press:
https://www.drupal.org/node/2338365#comment-9728757

[D7] Da vinci:
https://www.drupal.org/node/2407203#comment-9741033

[D7] Delta theme:
https://www.drupal.org/node/2395279#comment-9768987
https://www.drupal.org/node/2395279#comment-9803589

[d7] Trrybe:
https://www.drupal.org/node/2467397#comment-9810613

D7 GWT-Drupal:
https://www.drupal.org/node/2442479#comment-9814981

[D7] webform_request_tracker:
https://www.drupal.org/node/2471891#comment-9829393

[D7] Himalaya theme:
https://www.drupal.org/node/2470681#comment-9833941
https://www.drupal.org/node/2470681#comment-9835413
https://www.drupal.org/node/2470681#comment-9835429

[D7] User activity log
https://www.drupal.org/node/2574025#comment-10411663
https://www.drupal.org/node/2574025#comment-10411737
https://www.drupal.org/node/2574025#comment-10724658

Comments

Hi, Thanks for helping! I

Posted by klausi on April 11, 2015 at 11:54am
klausi's picture

Hi,

Thanks for helping!

I know you are a themer but I think it would be good if you could also learn developers skills, because many project applications will be Drupal modules.

For security you can start with reading https://www.drupal.org/writing-secure-code or watching https://amsterdam2014.drupal.org/session/cracking-drupal .

I also just found a security vulnerability in a module and assigned the application to you, maybe you have time to take a look and find the problem :-) https://www.drupal.org/node/2207873

Thanks Klausi for this

Posted by manjit.singh on April 11, 2015 at 12:24pm
manjit.singh's picture

Thanks Klausi for this initiate for me.

Yeah sure, start learning development and security key skills and will respond in https://www.drupal.org/node/2207873 very soon. but first i have to dig into links first that you have provided. :-)

Hi Manjit, I removed quite a

Posted by klausi on September 6, 2015 at 5:03pm
klausi's picture

Hi Manjit,

I removed quite a few review comments above that you linked because they were not actual manual reviews. You should only list review comments here where you performed a code review and actually looked at the code of the applicant. Form that we can see which issues you find and how you describe them.

Thanks for looking at all the security issues I sent your way, I think you found out about most of them.

So the next step for you is continuing with regular code reviews in the project application queue, and please link them here as usual. The key points are: security issues and licensing issues. See https://www.drupal.org/node/1587704 for a checklist when doing a review.

Thanks for helping!

Access callback

Posted by drupalove on October 6, 2015 at 8:44pm
drupalove's picture

Hi Manjit and Klausi

Thanks for your input on this page, I found it very useful.

I have a question if you don't mind.

I sometimes see 'access callback' => TRUE used in modules, especially with webservice callbacks, where the purpose is not just giving access to text.

How can I be sure the module doesn't raise a security issue. What exactly do I look for in these instances.

For example, is this sandbox module secure:
http://cgit.drupalcode.org/sandbox-ma3310-2551517/tree/wechat_lite.modul...

Thanks for your time.

That depends on what the

Posted by klausi on April 2, 2016 at 9:44am
klausi's picture

That depends on what the module is doing in the open page callback. If it performs something that should only be accessible to admins or user roles, then it is a security issue. If the page callback is needed for functionality for all/anonymous users, then it is fine.

Thanks for the reviews

Posted by klausi on April 2, 2016 at 10:03am
klausi's picture

Thanks for the reviews Manjit!

In https://www.drupal.org/node/2565611#comment-10699264 you advised to sanitize #default_value, which is not necessary. I posted a comment in that one.

In https://www.drupal.org/node/2479471#comment-10465383 you advised to sanitize in a form building function, but it should always be done in output generating functions. "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."

Can you go back to those comments you made and edit them to correct them?

Please don't add comments to your wiki page here where you just posted "Please add your manual review of project applications. ...". That is not an actual manual code review, so should not count in this list here.

Then I saw you forgot to change the status on some issues where you did a manual review. The goal is to come to a conclusion after the review: is this application ready and are all blocking issues solved? Then the new status should be RTBC. Are there security issues or licensing issues or other application blockers left that need to be fixed? Then the new status should be "needs work".

Can you go back to those reviews and flip the status one way or the other?

What is left on your road to become a git admin? I think there might still be confusion about XSS and where stuff should be sanitized. Let's see how are doing on your next issue where you find a security issue?

Thanks for your help, keep going with your reviews and list them here!