Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
rework path token authentication #285
rework path token authentication #285
Changes from 3 commits
686a21f
cdced73
ce91f8f
b944116
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of replacing
/
with%2F
, would it work to URL-encode the token? Since there's no longer a test that thest
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i originally did this but EncodeURIComponent doesn't escape things it should and EncodeURI overeagerly escapes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or possibly the other way around. but it was sufficiently annoying that i changed it to this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see, it looks like
encodeURI()
doesn't escape/
, butencodeURIComponent()
does escape$
. Though it looks like$
is the only token character thatencodeURIComponent()
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it just broke like all the tests so i said fuck it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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$
:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems fine. But maybe replace all occurrences if you're going to do something?