Skip to content
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

jwt.ParseRequest should also look for the token in cookies #1097

Closed
tonsV2 opened this issue Mar 9, 2024 · 3 comments · Fixed by #1098
Closed

jwt.ParseRequest should also look for the token in cookies #1097

tonsV2 opened this issue Mar 9, 2024 · 3 comments · Fixed by #1098
Assignees

Comments

@tonsV2
Copy link

tonsV2 commented Mar 9, 2024

Abstract

I've been using github.com/lestrrat-go/jwx/jwt in a few different projects by now. Especially the jwt.ParseRequest is making my life easier.

However after trying to ramp up security on a specific project I realized it could be made even better.
jwt.ParseRequest only looks for the token in the http headers. But the token could also be submitted as a cookie value.

Describe the proposed solution/change

jwt already have jwt.WithHeaderKey for specifying a custom header key. A smilair construct could be introduced for cookies.

jwt.ParseRequest(req, jwt.WithCookieKey("accessToken"))

Analysis

I'm well aware that I could try my luck with parse request first and if that fails, I could manually extract the token from the cookie and pass the value to jwt.Parse. But I do feel that since jwt.ParseRequest it might as well also consider cookies since those are part of the request.

Additional context

When it comes browser based applications it's advised to use a secure httpOnly cookie over something like LocalStorage if it's necessary to store a token client side. Sadly this isn't used as much as it should be.

In addition to the above when dealing with SSE it's simply not possible to use http headers to submit the token. The only way to do so is by using cookies or passing the token as a query string (which is not a good idea).

@lestrrat
Copy link
Collaborator

lestrrat commented Mar 9, 2024

Sounds reasonable. I whipped up #1098, do you want to take it for a whirl and let me know? Please do note that I wrote this in about 30 minutes, so there very well could be loose ends.

@tonsV2
Copy link
Author

tonsV2 commented Mar 10, 2024

Wow! Thanks 🙏 It seems to work great

@lestrrat
Copy link
Collaborator

lestrrat commented Mar 10, 2024

I added jwt.WithCookies() so that you can extract which *http.Cookie the token came from.

I think I'm going to merge the PR, but please do note that I won't be making a release for a bit, as I'd like to wait for the dust to settle a bit and possibly have another look afresh before making non-critical releases. Meanwhile, please holler if you find anything odd with this new feature.

Thanks for the headsup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants