Reviews and Mentoring for Ayesh

You are viewing a wiki page. You are welcome to join the group and then edit it. Be bold!

Posted by ayesh on May 25, 2015 at 3:10am
Last updated by ayesh on Thu, 2015-05-28 17:26

This wiki page is created to keep track of Ayesh's Project application reviews for him to get advice and mentored by Git administrators and anyone else who might have suggestions, advices, etc.

I would like to thank Klaus Purer, Patrick Drotleff, Matthew Donadio (mpdonadio), Karl Scheirer (kscheirer) and everyone else for their effort in reviewing applications. Drupal always had clean, secure and fast code compared to other projects; Core and contrib alike. I realized project applications play a major role in vetting and reviewing new projects, so I wanted do what I can.

A list of security issues I could find in the candidate modules:

[D7] Data Manipulate UI
Review
This module opens a menu router item that accepts GET, POST, PUT and DELETE data. It is protected with some user_access() calls, but it did not validate the user input for Cross-site requests. I provided him with a proof of concept, and guidance on how to prevent it.
[D7] JSON Filter
Review
This module accepts user-provided JSON string, and outputs them without sanitizing it first. It is possible to craft JSON string that contains malicious content, and since this module did not run check_markup() or such filter against the processed text, installed sites would be vulnerable. I provided a patch too.
[D7] Country Access
Review
This one has an admin setting to enter a custom message, and the admin page requires a special permission (which shows no warning about potential risks in the permission page). When the admin message is shown, it is not sanitized, and the admin-configured message is shown, with possible broken HTML, scripts, etc.
[D7] Spamfree Email
Review
Spamfree module is supposed to work independent of the client browser's JS capabilities. For browsers without Javascript, this module adds a <noscript><img /></noscript> snippet that contains an encrypted string, but its key was hardcoded in the module, making it possible for anyone to decrypt it themselves. Also, module generates an image, which is CPU-intensive. I also helped him to add some DoS protection.
3. [D7] Concurrent
Review
This module had a few unprotected menu router items.

List of all reviews

Following are the reviews I've made, manually entered, and are in the order of the issue's age (oldest first). I excluded some reviews that I didn't add helpful/constructive reviews.

Comments

Hi and thanks for starting

Posted by klausi on May 25, 2015 at 10:10am

Hi and thanks for starting this!

Impressive list already :-)

Here are some common templates that we use when reviewing: https://groups.drupal.org/node/184389

Thanks klausi. Could you shed

Posted by ayesh on May 25, 2015 at 4:43pm

Thanks klausi.

Could you shed a light on this case please?
I came across modules that tend to have a great name that covers a big area, but does only a little for that name. Debug tools module for example.

There is no tools provided by this module, nor I believe it would help in actually debugging process per se. I didn't want to sound any negative in the application, but if the applicant has chosen a module name that is someone inappropriate (I'm probably wrong in this application in particular), how should we proceed? This module is providing additional information for bug reports. "Bug report extras" or such name would be a better match.

They have chosen the module name, and thus the hook names, variables, etc, so it would be a big change if they have to rename the module. but at the same time, if they promote the module, it becomes a long lasting problem in module namespaces.

We can only recommend name

Posted by klausi on May 27, 2015 at 7:12pm

We can only recommend name changes if the situation is confusing (as in this case), but we should not enforce it. So we should tell the applicants that the name is bad, but we should not block applications based on that alone.

It's similar as with module duplication: we should tell them that they are just re-inventing an already existing solution instead of collaborating, but we should not block applications on that.

You are right. Thanks. I did

Posted by ayesh on May 28, 2015 at 9:50am

You are right. Thanks.
I did not block any applications before, and never will. I just wanted to know what we should do, just in case.

With blocking applications I

Posted by klausi on May 28, 2015 at 10:01am

With blocking applications I mean setting them to "needs work", so a confusing name alone should not cause the application issue to go to "needs work".

ready for git admin role

Posted by heddn on May 29, 2015 at 2:35pm

+1 on granting Ayesh git admin role.

+1 from me as well, I think

Posted by klausi on May 29, 2015 at 3:15pm

+1 from me as well, I think Ayesh is more than ready. Opened the webmaster issue, please add your +1 there! https://www.drupal.org/node/2497199

Thank you Klaus and Lucas

Posted by ayesh on May 29, 2015 at 11:39pm

Thank you Klaus and Lucas (@heddn).
This means a lot and I'll try my best to live up to it.

You have my vote. I would

Posted by mpdonadio on May 29, 2015 at 10:44pm

You have my vote. I would use the Review Template more, especially for your first review of a project. I helps make sure the important stuff gets identified, and the suggests are just suggestions.

Thank you Matthew.I will

Posted by ayesh on May 29, 2015 at 11:38pm

Thank you Matthew.
I will definitely use the template more often. Usually the PA bot marks applications with Pareview errors as Needs Work, before anyone else. If the bot has mentioned the pareview errors, I focus on other potential issues.

People tend to use Pareview quite often nowadays, and some applications are perfect in the first shot in terms of Pareview test (which is great!).

I also tend to not block any applications for small issues ( Missing t() calls, 80 char limit, etc), but I do mention them in the application without changing its status.
In this application, I marked it Needs Work because of a bug in JavaScript (please see my line "In the request recording JS file..."). The applicant looks experienced and the rest of the module was actually in good quality, except it previously had an unprotected menu router item.

I think it was OK that I marked it needs work for that JS file bug, because otherwise that module wouldn't work in any site installed in a sub folder. I submitted a patch for him to make it clear where he needed fixing.

If I was wrong or could be better in that application, let me know please :)

Congrats and welcome as git

Posted by klausi on May 30, 2015 at 6:19pm

Congrats and welcome as git admin Ayesh!

Instructions for promoting people are here: https://www.drupal.org/node/1125818

There is quite a bit of RTBC backlog in the queue, so you can start there with the oldest applications. Feel free to ask us if you have any questions!