[openstreetmap/openstreetmap-website] Use Brakeman for static code analysis (#2723)

[Tom Hughes]

Out of interest I did a run with all the checks turned back on to see what they found... My quick summary is:

#### Command Injection: 6

These I believe all relate to interpolating trace file names into commands are are all safe as the names in question have been generated by us. Some of the easier ones have been fixed but the ones which redirect output are harder I suspect.

#### Cross-Site Scripting: 18

These all relate to using the `legale` parameter on the terms page to lookup the correct languages/ It's safe in that the worse you can do is pass a bogus language which we would then fail to lookup. I'm not really sure what it is about that use of a parameter (over all others) that it doesn't like, or what a fix might look like.

#### Dynamic Render Path: 3

These all appear to relate to it just not understanding the `render @collection` construct in rails and thinking that the collection is in fact an action which is dependent on a parameter when in fact the action is fixed it's the set of objects to be rendered that depends on the parameter.

#### File Access: 6

This is the trace parameter file names again - these ones in particular should go away once the trace files are moved to attachments.

#### Redirect: 10

This is a mixture of cases where we use a `referer` parameter to preserve an original URL that we later redirect back to and cases where we preserve a subset of input parameters across a redirect.

It's not immediately clear to me if these are really a problem or what would be considered a "safe" way of writing them.

I mean clearly somebody could say publish a link to our login page with a referer parameter pointing to a third party site but it's not clear what it gets them - in principle maybe allows whitewashing by making it look like a link to us but it's one where the user would have to have an OSM account to proceed through to the target which makes it seem unlikely to be a popular target.

We could validate that the referer pointed to openstreetmap.org but would that silence brakeman?

#### SQL Injection: 6

A few of these are just silly and can be easily fixed, others are deliberate cases of injecting code (generated by us, so safe) to do complicated selections and are hard to avoid.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/2723#issuecomment-662444458
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20200722/d2b56b67/attachment-0001.htm>