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
JEP: Websocket token authentication with subprotocols #121
base: master
Are you sure you want to change the base?
JEP: Websocket token authentication with subprotocols #121
Changes from 1 commit
0db16dd
f7dd99d
07b7fe1
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.
Ref here for next comment.
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.
For reference, this bit describes current behavior. We can propose new behavior that doesn't support the new scheme (e.g. 401 on no creds), but if we are talking about backward compatibility, we have to handle what implementations do today without modification.
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.
Is the use of
SHALL
in some sentences andMUST
in other sentences significant?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.
These SHALLs should be SHOULDs. I had rfc2119 in mind, where MUST is a requirement, while SHOULD is a recommendation. Sending tokens this way is not required because all existing mechanisms still work, but it is recommended where supported.
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.
Will it be clear to clients if the server didn't support the subprotocol or whether there was something wrong with the token? Ref other comment above, it seems like an overload of 403. If my token is expired, I don't want the client to fall back on trying a less secure method.
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.
Unfortunately, I don't think so. It would perhaps have been better to fail with 401 when no recognized credentials are provided, but it doesn't really make sense to define new behavior for not supporting the new scheme.
Perhaps there is a header we can set on the error response that might be readable by the client error handler, so it can know the token was rejected, not unsupported? Initial poking around suggests that the error handler doesn't preserve a handle on the response (yet again confirming that browsers consistently have the least capable websocket implementation for some reason), so unfortunately that doesn't seem to be an option.
If we had another status code to use for "token recognized and rejected" that would also work, but I don't think there is one, and 403 is really correct for "recognized but not authorized."
If we can't do it feasibly on the response, we may need to have explicit capability detection somewhere, either:
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 realize the comment below is related to this: browsers don't report status codes, so there's no distinguishable difference for clients between 403, 404, 500, or any other reason a websocket request could fail (e.g. not a websocket endpoint at all). So status codes are not helpful for browsers (it's still the right thing to do for the server to log the right error code).