Reviews and Mentoring for ARUN AK

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 arun ak on March 7, 2014 at 10:12am
Last updated by arun ak on Thu, 2016年09月22日 08:29

This page serves ARUN AK's project application reviews and help to project application reviews. This is a part of mentoring to become a code review administrator eventually. Add any advice that you may have through comments.

Security Issues

[D7] Localist Calendar for Drupal
https://www.drupal.org/node/2784545#comment-11643883

[D7] Custom Sitename
https://www.drupal.org/node/2786217#comment-11643571

[D7]Bootstrap Banners Widget
https://www.drupal.org/node/2771627#comment-11636237

[D7] Multiple role login pages
https://www.drupal.org/node/2707951#comment-11561547

[D7] Client js error log
https://www.drupal.org/node/2743497#comment-11502573

[D7] Google Drive Docs Viewer
https://www.drupal.org/node/2778257#comment-11558507

Project Applications I reviewed:

[D7] Vide
https://www.drupal.org/node/2795951#comment-11647417

[D7] Bootstrap Login Authenticate
https://www.drupal.org/node/2756909#comment-11587099

[D7] Common Body Class
https://www.drupal.org/node/2771959#comment-11583051

[D7] MIME file validator
https://www.drupal.org/node/2781175#comment-11580419

[D7, D8] Liquid slider
https://www.drupal.org/node/2795077#comment-11580071

[D8]Twitter Search Block
https://www.drupal.org/node/2743377#comment-11566431

[D8]Currency Convert
https://www.drupal.org/node/2687695#comment-11564999

[D7]File Mime Type Checker
https://www.drupal.org/node/2721161#comment-11552081

[D7] Scroll Depth Analytics
https://www.drupal.org/node/2770029#comment-11437579
https://www.drupal.org/node/2770029#comment-11520463
https://www.drupal.org/node/2770029#comment-11561161

[D7] Background Video
https://www.drupal.org/node/2718335#comment-11501919
https://www.drupal.org/node/2718335#comment-11516459
https://www.drupal.org/node/2718335#comment-11547705

[D7] Exit Modal
https://www.drupal.org/node/2775385#comment-11460423
https://www.drupal.org/node/2775385#comment-11462899
https://www.drupal.org/node/2775385#comment-11463835
https://www.drupal.org/node/2775385#comment-11513071

[D7] Nearby Places Search
https://www.drupal.org/node/2733173#comment-11495123
https://www.drupal.org/node/2733173#comment-11524029

[D7] Client js error log
https://www.drupal.org/node/2743497#comment-11544607

[D7] Facebook comment notification
https://www.drupal.org/node/2649336#comment-11506761

[D8] Whippy forms
https://www.drupal.org/node/2776871#comment-11469875

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

[D8] Simple Analyse
https://www.drupal.org/node/2624458#comment-11470379

[D7] Doubtfire
https://www.drupal.org/node/2570923#comment-10357023

[D7] Twitter Lite
https://www.drupal.org/node/2770971#comment-11463265

[D7] Track User Change
https://www.drupal.org/node/2775811#comment-11463091

[D7] Dblog Quick filter
https://www.drupal.org/node/2768553#comment-11474149

[D7] InstaPost
https://www.drupal.org/node/2767837#comment-11484117

[D7] Google Drive Docs Viewer
https://www.drupal.org/node/2778257#comment-11471041
https://www.drupal.org/node/2778257#comment-11557815

[D7] Mailgun Advance
https://www.drupal.org/node/2781015#comment-11493631

[D7] Term body class
https://www.drupal.org/node/2708637#comment-11551375

[D8] Superslides Fullscreen Caption Slider
https://www.drupal.org/node/2736009#comment-11565295

[D7] Popular Children
https://www.drupal.org/node/2746485#comment-11591829

[D7] Simple braintree payment gateway
https://www.drupal.org/node/2724479#comment-11505497

I am Actively participating in Drupal.org Project applications reviews, and can request reviews by personal contact form as well as in IRC(Nickname: arun_ak).

Comments

Welcome

Posted by klausi on March 19, 2014 at 9:30pm

Thanks for starting this, looking forward to your reviews! Feel free to ask questions here or in #drupal-codereview on IRC.

Hi klausi, I have included

Posted by arun ak on August 11, 2016 at 9:15am

Hi klausi,

I have included more links to the reviews that I did and I'm very much interested to be part of drupal code review team & help clear out project review queue as a code review administrator.

Thanks,
ARUN AK

My Reviews

Posted by arun ak on August 11, 2016 at 7:58am

Reviewed projects added.

I reviewed [D7]File Mime Type

Posted by arun ak on August 29, 2016 at 9:36am

I reviewed [D7]File Mime Type Checker. I checked module in code level and it is doing great.

But this functionality already provided by one contributed module File Upload Secure Validator.

Duplication is bad, but not

Posted by klausi on August 31, 2016 at 7:47pm

Duplication is bad, but not an application blocker. Politely point out that a duplicate project already exists and ask the applicants to contribute to that project instead. But otherwise continue with the application if the applicant wants to.

Thanks klausi. I will follow

Posted by arun ak on September 4, 2016 at 8:08am

Thanks klausi. I will follow this.

Doing followup review for

Posted by arun ak on August 29, 2016 at 1:30pm

Doing followup review for [D7] Google Drive Docs Viewer.

Found, misuse of hook_field_formatter_info_alter() and missing changes mentioned in comment.

Found a security issue

Posted by arun ak on August 29, 2016 at 1:31pm

Did one more round of reviewed on [D7] Google Drive Docs Viewer. Found a security issue in code, using user entered values without passing through filter functions.

Thanks,
ARUN AK

Nice find, but why did you

Posted by klausi on September 9, 2016 at 3:07pm

Nice find, but why did you set the issue to RTBC?

I looked at your other reviews for security vulnerabilities and posted to the issue when necessary.

You mostly do a good job. Just as greggles pointed out you should be a bit more specific when describing a vulnerability. What is the name of vulnerability? Example: XSS. How can it be exploited? Example: put value X in field Y then javascript will be executed. That way the applicants can better understand what exactly they did wrong and how an attacker would leverage that.

Thanks klausi for pointing

Posted by arun ak on September 9, 2016 at 4:20pm

Thanks klausi for pointing out this. While reviewing I had noticed that security issue in the module and mentioned it in comment. Moving forward I will make sure whether user has fixed all the issues, especially security vulnerabilities before moving it in to RTBC.

Done one more round of review

Posted by arun ak on August 30, 2016 at 8:12am

Done one more round of review for [D7] Scroll Depth Analytics. Found some issues but no more blockers. Put comments over there and moved the application in to RTBC.

Thanks,
ARUN AK

Security issue reported

Posted by arun ak on August 30, 2016 at 11:37am

Reviewed [D7] Multiple role login pages. Found fiter_xss() and check_plain() functions not using properly.

Also found some other coding issues. Reported PAReview: security and moved in to Needs work.

https://www.drupal.org/node/2707951#comment-11561547

Thanks,
ARUN AK

How to handle this request?

Posted by arun ak on August 31, 2016 at 6:20am

Hi klausi,

In this project application user has mentioned three sandbox project. Do we need to review all three modules? Is this ok having three projects in a single application?

https://www.drupal.org/node/2775795

Thanks,
ARUN AK

It is only necessary to

Posted by klausi on August 31, 2016 at 7:48pm

It is only necessary to review one sandbox, if it has enough code. We just want to find out if the applicants know what they are doing. Once we are confident there is no need to review more code.

Security issue reported

Posted by arun ak on August 31, 2016 at 8:19am

Reviewed [D8] Superslides Fullscreen Caption Slider. Found XSS and reported security issue in application.
https://www.drupal.org/node/2736009#comment-11565295

I just checked the issue but

Posted by klausi on August 31, 2016 at 8:25pm

I just checked the issue but it seems there is no security issue. Can you clarify there what steps an attacker needs to do in order to trigger the vulnerability?

When I test I was added a

Posted by arun ak on September 1, 2016 at 6:19am

When I test I was added a text area field which accepting full html in superslider views. So the alert() was coming during slideshow. But it can be managed by selecting proper Text format filter in field configuration.

Ah yes, full HTML is

Posted by klausi on September 1, 2016 at 7:58am

Ah yes, full HTML is dangerous and should never be given to untrusted users. See https://www.drupal.org/node/224921

Can you check your other security issues that you found if they also require a full HTML text format to be exploitable?

I checked. Other issues are

Posted by arun ak on September 1, 2016 at 8:47am

I checked. Other issues are not related to full HTML text format.

Reviewed [D7] MIME file validator

Posted by arun ak on September 5, 2016 at 11:04am

Reviewed [D7] MIME file validator. Found similar projects and mentioned it in comments.

https://www.drupal.org/node/2781175#comment-11580419

Reviewed [D7] Common Body Class

Posted by arun ak on September 6, 2016 at 9:19am

Reviewed [D7] Popular Children

Posted by arun ak on September 8, 2016 at 1:08pm

Reviewed [D7] Popular Children. Found access callback issue for ajax url. So users can insert any random numbers in to the database.

https://www.drupal.org/node/2746485#comment-11591829

Hi ARUN AK! Thanks for doing

Posted by greggles on September 8, 2016 at 7:32pm

Hi ARUN AK!

Thanks for doing so many reviews, and especially for finding some security vulnerabilities along the way. Educating our community about secure programming is an important component of this process.

One suggestion: I've seen a few examples (1, 2 - point #2 3 point #2 where you found security issues and give advice on how to fix it, but didn't name the specific type of vulnerability (e.g. "cross site scripting" or "sql injection"). I think it's helpful for people to be able to classify security vulnerabilities and knowing the type will also help them to do research and learn more about the problems in general.

Hi greggles, Thanks for your

Posted by arun ak on September 19, 2016 at 11:11am

Hi greggles,

Thanks for your suggestion. Moving forward I will mention the name of security vulnerability along with advice.

Thanks,
ARUN AK

Reviewed [D7]Bootstrap Banners Widget

Posted by arun ak on September 19, 2016 at 10:54am

Reviewed [D7] Localist Calendar for Drupal

Posted by arun ak on September 21, 2016 at 8:49am

Reviewed [D7] Localist Calendar for Drupal. Found security issue. Admin configuration pages can be accessible by anonymous users.

https://www.drupal.org/node/2784545#comment-11643883

All looking good here, I

Posted by klausi on September 22, 2016 at 9:58am

All looking good here, I think we can promote you to git admin at https://www.drupal.org/node/2785515 .

@greggles and others: please post your +1 there or any concerns you might have.

Looks good

Posted by naveenvalecha on September 22, 2016 at 12:37pm

Reviewed couple of the reviews and it all looks good to me and I have few concerns that I have seen in the reviews.
1) If a project is short and more than 120 lines, we add the new tag "Pareview: Single promote" so that it would be easier for second reviewer to get it promoted himself.

Rest looks good to me and I agree its time to promote you

// Naveen

Hi Arun, How are you doing? I

Posted by klausi on October 3, 2016 at 7:41pm

Hi Arun,

How are you doing? I think you have not approved any applications yet as git admin, anything we can help you with to get started?

Hi Klaus, I'm Very well,

Posted by arun ak on October 4, 2016 at 4:13am

Hi Klaus,

I'm Very well, Thank you. I was supposed to ask you regarding this. I am checking the applications from the high priority list which is in RTBC status.

I would like to approve some application which I have already reviewed. But as per policy I cannot approve the applications which I have reviewed. Also checked some applications which we can promote as PAReview: Single project promote. But waiting for some more eyeballs on it.

Sure I will look into this.

It is fine to approve

Posted by klausi on October 4, 2016 at 7:50am

It is fine to approve applications that you reviewed yourself after some time. I for example do that after one week if there have not been any objections.

Code review for security advisory coverage applications

Group organizers

Group notifications

This group offers an RSS feed. Or subscribe to these personalized, sitewide feeds:

AltStyle によって変換されたページ (->オリジナル) /