-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix: enhance OAuth token handling in useConnection hook to prevent infinite auth loops #830
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
Merged
Merged
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
17ace55
fix: enhance OAuth token handling in useConnection hook to prevent in…
kentcdodds 30f9855
fix: update authorization header handling to prevent empty values and…
kentcdodds 0da5bc2
refactor: streamline authorization header validation in useConnection…
kentcdodds de71fb9
Merge branch 'main' into auth-fix
cliffhall 34534ed
fix: update default authorization header to include empty Bearer toke…
kentcdodds 9e95eec
Merge branch 'main' into auth-fix
kentcdodds 5b4edef
Update client/src/lib/hooks/useConnection.ts
cliffhall File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.
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.
Would an better way to do this be to leave it, but disable it? So for the common use case of adding an authorization header they just enable it and add their token, but otherwise it's just ignored when headers are sent? Makes it handy, but removes the footgun. With the robust checking you've added above in useConnection, we can be confident in the proper behavior should they switch it on but fail to add a token.
Uh oh!
There was an error while loading. Please reload this page.
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 wonder if there's a situation where someone would want to test the lack of an auth header without actually removing the auth token from their client 🤔
Disabling the Auth header would be how I would want to do that.
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.
So instead of removing this chunk, should we just set the default to
enabled: false?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'm a little confused why this chunk was added in the first place.
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 guess for myself, I always just use the auth flow that's built into the inspector. But I suppose some people have something more complicated and they have to get an auth token through some other means. But to me, that seems like more of an edge case. And a person in that situation would know how to add a new header called authorization with a bearer prefix. I don't think that they need our help to do that.
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.
Just speaking for myself and a few others I've seen chime in on the original feature request - I've worked with quite a few servers that don't support OAuth and where I need to pass something like a Personal Access Token through the authorization header, so this UI option seems more intuitive for that scenario?
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'm happy to go whatever direction you all want with this. Just seems odd to me to have the headers include an invalid auth header (even if it's disabled) when it's quite easy to add and then it even saves to localstorage once you have 🤷♂️
Additionally, I do think this is a much less common case so we are increasing confusion for more folks to make things a tiny bit easier for fewer folks 🤔
Uh oh!
There was an error while loading. Please reload this page.
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.
@kentcdodds can we just leave it and turn it off by default? The point is that it saves the person who needs to add a bearer token some typing and possible fat-finger errors. I don't see how it's really going to sow confusion. Anyone monkeying with headers will know the bearer token header pattern, and if they don't, they'll probably just leave it turned off. If they do just turn it on and mash connect, they'll get a toast about it being incomplete and see the error of their ways. It's not a footgun, it's just a convenience.
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.
Sounds good. I changed it though so it does not automatically replace an empty auth header to reduce "magic." People will see the toast and go disable the header. I'm happy with that :)