chore(db): prevent the possibility of future url-injection bugs #2368
Conversation
ghost
assigned 
| // url.render({ uid: 'foo' }) // returns '/account/foo/sessions' | ||
| // url.render({ uid: 'bar' }) // returns '/account/bar/sessions' | ||
| // url.render({ uid: 'foo\n' }) // throws error.unexpectedError() | ||
| // url.render({}) // throws error.unexpectedError() |
There was a problem hiding this comment.
nit: Should we throw on {} if no path params defined?
There was a problem hiding this comment.
I don't think so, because it makes calling easier for those cases. And that's what the current behaviour is too. But I'm not sure if you're saying it's wrong that we don't throw in that case, or you just didn't realise that we already aren't?
There was a problem hiding this comment.
@philbooth This does perform as you stated. It will not throw with url.render({}) if a path with no params are specified, ex. /foo/bar.
test/local/safe-url.js
Outdated
| }) | ||
| }) | ||
|
|
||
| it('logs an error and throws when param seems unsafe', () => { |
There was a problem hiding this comment.
Should we add some more test cases for errors? Maybe other special characters, undefined, nested params.
lib/pool.js
Outdated
| return P.reject(new Error('Invalid url argument')) | ||
| } | ||
|
|
||
| const path = url.render(params) |
There was a problem hiding this comment.
Just realised we shouldn't let the throw leak out of here uncaught, it needs to reject instead.
Ref: bug 1447452 (comment)
Even though we validate incoming data, we should belt-and-braces ensure that unsafe input can never make it into URLs that we request. This change seeks to do that by adding a new module to build parameterised path strings safely, failing any requests where bad input is encountered.
See also the discussion about bikeshed colours in #2361. The key difference from that PR here is that, if a raw path string is passed to
pool.request, it now fails.@mozilla/fxa-devs r?