Reviews and Mentoring for ARUN AK

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!

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

klausi's picture

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

ARUN AK's picture

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

ARUN AK's picture

Reviewed projects added.

I reviewed [D7]File Mime Type

ARUN AK's picture

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

klausi's picture

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

ARUN AK's picture

Thanks klausi. I will follow this.

Doing followup review for

ARUN AK's picture

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

ARUN AK's picture

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

klausi's picture

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

ARUN AK's picture

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

ARUN AK's picture

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

ARUN AK's picture

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?

ARUN AK's picture

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

klausi's picture

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

ARUN AK's picture

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

klausi's picture

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

ARUN AK's picture

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

klausi's picture

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

ARUN AK's picture

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

Reviewed [D7] MIME file validator

ARUN AK's picture

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

ARUN AK's picture

Reviewed [D7] Common Body Class.

https://www.drupal.org/node/2771959#comment-11583051

Reviewed [D7] Popular Children

ARUN AK's picture

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

greggles's picture

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

ARUN AK's picture

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

Reviewed [D7] Custom Sitename

Reviewed [D7] Localist Calendar for Drupal

ARUN AK's picture

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

klausi's picture

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

naveenvalecha's picture

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

klausi's picture

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,

ARUN AK's picture

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

klausi's picture

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:

Hot content this week