rework path token authentication #285
Conversation
* see comment. * this is essentially so enketo doesn't forward the user to the login page. * there is sort of some half-justification here in that 403 does generally mean "just don't try again you can't." but typically a failure to auth yields a 401, not a 403. we really want to be able to communicate "401 but also probably you're just locked out don't bother."
lib/http/middleware.js
Outdated
| const queryKey = Option.of(request.query.st) | ||
| .map(decodeURIComponent) | ||
| .filter((k) => /^[a-z0-9!$]{64}$/i.test(k)); | ||
| const queryKey = Option.of(request.query.st).map(decodeURIComponent); | ||
| queryKey.ifDefined((token) => { | ||
| delete request.query.st; | ||
| // we modify the request url to ensure openRosa gives prefixed responses | ||
| // per the requested token. we have to slice off the /v1. | ||
| request.originalUrl = `/v1/key/${token}${request.originalUrl.slice(3)}`; | ||
| request.originalUrl = `/v1/key/${token.replace('/', '%2F')}${request.originalUrl.slice(3)}`; |
There was a problem hiding this comment.
Instead of replacing / with %2F, would it work to URL-encode the token? Since there's no longer a test that the st query parameter matches /^[a-z0-9!$]{64}$/i, I think the token could contain other characters that aren't URL-safe in addition to /. Or is that case considered user error?
There was a problem hiding this comment.
i originally did this but EncodeURIComponent doesn't escape things it should and EncodeURI overeagerly escapes.
There was a problem hiding this comment.
or possibly the other way around. but it was sufficiently annoying that i changed it to this.
There was a problem hiding this comment.
Ah I see, it looks like encodeURI() doesn't escape /, but encodeURIComponent() does escape $. Though it looks like $ is the only token character that encodeURIComponent() will encode. If $ is encoded, I think the URL is valid, and Backend will decode it if it used in a future request. Is the trade-off that the URL is a little longer and less human-readable? Right now I think the URL is mainly used in the form list and manifest, so maybe that's OK. My instinct is that it's better to have an over-encoded URL than a possibly under-encoded one.
There was a problem hiding this comment.
it just broke like all the tests so i said fuck it.
There was a problem hiding this comment.
Haha broken tests are no fun. I guess one question I have is how risky you consider this. If originalUrl is set to an invalid URL, could that lead to an issue?
One other thing I'm just now noticing is that only the first occurrence of / is replaced here: I think we'd need to use something like /\//g to replace all occurrences. I was wondering (and maybe this is part of this that I don't understand), why encode / but not other characters? If it's to ensure that the structure of the URL remains intact, I think we'd need to encode ? and # as well.
One option if encoding $ is breaking tests is that we could encode the token, then decode $:
const encodedToken = encodeURIComponent(token).replace(/%24/g, '$');
request.originalUrl = `/v1/key/${encodedToken}${request.originalUrl.slice(3)}`;There was a problem hiding this comment.
i consider it not remotely risky. if anybody hits this case they have already given invalid input and the likely result is already a 401. tbh the fact that i even do the replace is massive overkill. again, by the time this branch is hit the user has already given bad input and is already in a failure case.
There was a problem hiding this comment.
That seems fine. But maybe replace all occurrences if you're going to do something?
see commits