Skip to content

fix(jwt): store raw JWT token in cookies without Bearer prefix#4552

Merged
provinzkraut merged 6 commits intolitestar-org:mainfrom
disrupted:fix/jwt-cookie-auth
Jan 24, 2026
Merged

fix(jwt): store raw JWT token in cookies without Bearer prefix#4552
provinzkraut merged 6 commits intolitestar-org:mainfrom
disrupted:fix/jwt-cookie-auth

Conversation

@disrupted
Copy link
Copy Markdown
Contributor

@disrupted disrupted commented Jan 10, 2026 •
edited
Loading

Description

when switching to JWTCookieAuth I noticed how it incorrectly stores the JWT token with a "Bearer " prefix in the cookie value.

AFAICT this violates:

  • RFC 6265 (HTTP cookies): cookies should contain just the token value
  • RFC 6750 (Bearer token usage): "Bearer" is an Authorization header scheme, bearer tokens should not be stored in cookies

I also updated the middleware accordingly. OAuth2PasswordBearerAuth was also affected.

Closes

Todo

  • update tests

@disrupted disrupted requested review from a team as code owners January 10, 2026 10:59
@disrupted disrupted marked this pull request as draft January 10, 2026 11:13
provinzkraut
provinzkraut requested changes Jan 10, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@provinzkraut provinzkraut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR completely changes how the auth middleware works. While it might be questionable whether it should implicitly authenticate against a token found either in the header or the cookie, now it does not handle authentication headers at all anymore, which will break existing applications.

If you want to fix the cookie encoding, I suggest you keep the change at just that.

If you want to change where the token is read from, I suggest you make it configurable, and keep in mind that it should not break existing applications.

@disrupted
Copy link
Copy Markdown
Contributor Author

disrupted commented Jan 10, 2026 •
edited
Loading

is it really expected behavior that the JWTCookieAuthenticationMiddleware reads from Authorization header? I certainly did not expect it and was surprised to find this ambiguity in the security layer. imo the next major release (3.0) would be a good opportunity to make such breaking changes if you agree that the behavior should be more predictable.
I am mostly interested in fixing the cookie auth which is currently broken. I can partially rollback the middleware changes but I believe it would still make sense to discuss this further.

@provinzkraut
Copy link
Copy Markdown
Member

provinzkraut commented Jan 10, 2026 •
edited
Loading

is it really expected behavior that the JWTCookieAuthenticationMiddleware reads from Authorization header?

No, it doesn't. That's why I said it's questionable :)

I just wanted to point out that, if this change is being made, it would need to be an intentional breaking change. That's a separate change from the one that this PR initially suggested to make:

when switching to JWTCookieAuth I noticed how it incorrectly stores the JWT token with a "Bearer " prefix in the cookie value.

Should be separate PRs IMO, the individual changes are fine.

@disrupted
Copy link
Copy Markdown
Contributor Author

disrupted commented Jan 11, 2026 •
edited
Loading

Should be separate PRs IMO

agreed

@codecov
Copy link
Copy Markdown

codecov bot commented Jan 23, 2026 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.85%. Comparing base (971ee79) to head (01d9aee).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #4552      +/-   ##
==========================================
- Coverage   97.85%   97.85%   -0.01%     
==========================================
  Files         297      297              
  Lines       15336    15335       -1     
  Branches     1720     1720              
==========================================
- Hits        15007    15006       -1     
  Misses        188      188              
  Partials      141      141

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@disrupted disrupted marked this pull request as ready for review January 23, 2026 21:47
@disrupted
Copy link
Copy Markdown
Contributor Author

disrupted commented Jan 23, 2026

not sure why the build-docs job is failing

@euri10
Copy link
Copy Markdown
Contributor

euri10 commented Jan 24, 2026

not sure why the build-docs job is failing

nothing woring on your side there are issues reaching the alchemy docs

@provinzkraut provinzkraut merged commit 594fb07 into litestar-org:main Jan 24, 2026
28 of 29 checks passed
@provinzkraut
Copy link
Copy Markdown
Member

provinzkraut commented Jan 24, 2026

Thanks @disrupted!

@disrupted disrupted deleted the fix/jwt-cookie-auth branch January 24, 2026 12:44
@disrupted disrupted mentioned this pull request Jan 24, 2026
Open
provinzkraut pushed a commit that referenced this pull request Jan 25, 2026
@disrupted @provinzkraut
fix(jwt): store raw JWT token in cookies without Bearer prefix (#4552)
1022c5e 
* fix(jwt): store raw JWT token in cookies without Bearer prefix

* fix(jwt): update OAuth2PasswordBearerAuth as well

* fix(jwt): restore backwards compatibility

* test(jwt): add assertion for correct cookie format

* fix(jwt): parse correct and legacy token formats

* docs: restore pydoc string

(cherry picked from commit 594fb07)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@provinzkraut provinzkraut provinzkraut approved these changes

Assignees

No one assigned

Labels

pr/external size: small Triage Required 🏥 This requires triage type/bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@disrupted @provinzkraut @euri10