Last modified: 2013-12-14 01:07:13 UTC
Security review needed before initial production deployment.
Assigning security review to Chris Steipp based on prior conversations with Chris and Greg.
https://git.wikimedia.org/tree/wikimedia%2Fwikimania-scholarships.git/HEAD
In general, everything looks good. Just a few things: Wikimania/Scholarship/Controllers/Login.php * handlePost - sanity check $next before you redirect Wikimania/Scholarship/Controllers/Review/Application.php * handleGet - $id = $this->request->get( 'id' ); seems like this isn't validated Wikimania/Scholarship/Controllers/Review/Phase1List.php and Phase2List.php * handleGet - Please strip any leading "=". Probably want to quote entries in case they contain commas. Wikimania/Scholarship/Forms/Apply.php * Do you really need IP in the db? Is there a cleanup job to remove them to comply with privacy policy? Can I get source of js/flexigrid.pack.js?
(In reply to comment #3) > Wikimania/Scholarship/Forms/Apply.php > * Do you really need IP in the db? Is there a cleanup job to remove them to > comply with privacy policy? This was the behavior of the legacy application. From what I can see the IP is never actually used in an business logic. It is shown in the full application dump and would theoretically be exported to the spreadsheet dump (that doesn't currently exist). I'm ok with removing it all together. The only thing I could see it viably being used for is spam moderation (eg mark all applications from a particular IP as spam).
(In reply to comment #4) > I'm ok with removing it all together. The only thing I could see it viably > being used for is spam moderation (eg mark all applications from a particular > IP as spam). I'd say remove it, or make it a config option that we turn off by default.
(In reply to comment #3) > Can I get source of js/flexigrid.pack.js? I assume this was the original source: https://git.wikimedia.org/blob/wikimedia%2Fwikimania-scholarships/b2442423dc1dfcb555910b013bc775ffb3777a5f/public%2Fjs%2Fflexigrid.js I did not pack the script myself. The packed and unpacked versions were part of the original legacy application files.
Oh, as for flexigrid, it looks ok using it as your currently are. If you want to do anything else with it, that would need another check. The library itself does a lot of stuff that is probably not safe. So as is, with the updates, LGTM.
