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
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
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
Reviewed projects added.
I reviewed [D7]File Mime Type
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
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
Thanks klausi. I will follow this.
Doing followup review for
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
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
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
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
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
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?
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
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
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
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
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
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
I checked. Other issues are not related to full HTML text format.
Reviewed [D7] MIME file validator
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
Reviewed [D7] Common Body Class.
https://www.drupal.org/node/2771959#comment-11583051
Reviewed [D7] Bootstrap Login Authenticate
Reviewed [D7] Bootstrap Login Authenticate from Review Bonus List.
https://www.drupal.org/node/2756909#comment-11587099
Reviewed [D7] Popular Children
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
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.
knaddison blog | Morris Animal Foundation
Hi greggles, Thanks for your
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]Bootstrap Banners Widget. Found XSS vulnerability and reported.
https://www.drupal.org/node/2771627#comment-11636237
Reviewed [D7] Custom Sitename
Reviewed [D7] Custom Sitename. Found security issue.
https://www.drupal.org/node/2786217#comment-11643571
Reviewed [D7] Localist Calendar for Drupal
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
Reviewed [D7] Vide
Reviewed [D7] Vide from high priority list.
https://www.drupal.org/node/2795951#comment-11647417
All looking good here, I
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
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
Naveen Valecha
https://www.valechatech.net
Hi Arun, How are you doing? I
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,
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
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.