fix(auth): answer htmx callers with HX-Redirect, not a 302 - #3306
Open
mickzijdel wants to merge 1 commit into
Open
fix(auth): answer htmx callers with HX-Redirect, not a 302#3306mickzijdel wants to merge 1 commit into
mickzijdel wants to merge 1 commit into
Conversation
An unauthenticated request gets a 302 to the login page, which is the right answer for a browser navigating and the wrong one for anything calling from JavaScript. XMLHttpRequest and fetch both follow a redirect themselves and hand the caller the final response, so the 302 is consumed before any client code runs and what arrives is the login page under a 200. The consequences differ by caller and neither is acceptable. htmx swaps that login page into whatever fragment it asked for, so an expired session replaces the asset table with a full login screen rendered inside the table. The uploader in home.ts is worse: it is raw XHR and treats any 2xx without an error toast as success, so it counts the file as uploaded, closes the Add asset modal, refreshes the table and reports nothing wrong. The operator watched a progress bar fill for a file that never left the browser. Send HX-Redirect instead when the request carries HX-Request. htmx navigates to the login page natively, and the uploader reads the header itself since nothing manages it. Requests without the header are untouched and still get their 302: a browser typing a URL wants a real redirect, and only six endpoints plus the uploader announce themselves as htmx. The decorator already treated htmx requests specially, dropping the fragment URL from ?next= so signing in would not land the operator on a bare table partial. This finishes that thought rather than adding a new concept, and the test covering it now asserts the same guarantee against HX-Redirect. Verified against the dev stack with auth enabled: with the session cookie cleared mid-modal, an upload now sends the operator to the login page instead of reporting a stored file, and the asset table poll navigates rather than rendering a login form inside itself. 1992 tests pass, ruff check and ruff format are clean, and mypy is clean on the changed files.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3306 +/- ##
=========================================
Coverage ? 90.31%
=========================================
Files ? 85
Lines ? 9947
Branches ? 1099
=========================================
Hits ? 8984
Misses ? 709
Partials ? 254 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
|
Love it. Not able to look at this for a few days due to travels. Will review when back. |
Contributor
Author
|
Great, sounds good. I'll submit the PR for chunking uploads too. Looking forward to your review, and safe travels. |
5 tasks
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Issues Fixed
No existing issue. I found this while working on the upload error messages in #3302: if your session expires while the Add asset modal is open, the upload reports success and the file is never stored, but nothing on screen says otherwise.
The rest is all Claude 5 Opus.
Description
@authorizedanswers an unauthenticated request with a 302 to/login/. That is the right answer for a browser navigating to a page, and but wrong for anything calling from JavaScript, because XMLHttpRequest and fetch both follow redirects themselves. The 302 is consumed before any client code runs, so what arrives is the login page with a status of 200.Two callers reach
_login_redirectfrom JavaScript, and both handle that badly:/.home.ts. It is raw XHR and treats any 2xx without an error toast as success, so it counts the file as uploaded, closes the modal, refreshes the table, and reports nothing wrong.The second one is silent data loss, which is why I went looking for a root fix rather than a client-side check. There is no client-side check available: you cannot see a redirect that the browser already followed for you.
So the server now says what it means. When the request carries
HX-Request,_login_redirectanswers204with anHX-Redirectheader instead of a 302. htmx acts on that natively and navigates to the login page. The uploader is not htmx-managed, so it reads the header itself and does the same.Requests without the header are untouched. A browser typing a URL or following a link still gets its 302 with
?next=filled in, which is what it wants. Only the six htmx endpoints and the uploader change, and all seven were already doing something wrong.The decorator already treated htmx requests specially:
_is_safe_login_next_sourcedrops the fragment URL from?next=so signing in doesn't land you on a bare table partial. This finishes that thought rather than introducing a new concept.One existing test changed.
test_authorized_drops_next_for_htmx_partialasserted the old 302 contract, so it now asserts the same guarantee againstHX-Redirect, with its docstring updated to say why.Testing
I ran a dev stack with auth enabled and deleted the session cookie to simulate expiry.
Same URL, same expired session, with and without the header:
Uploading a file with an expired session, measured on both sides:
/, modal closed/login/Master stores nothing and says nothing: the modal closes exactly as it does on success.
I also left the dashboard idle for 12 seconds with an expired session and touched nothing. On master the 5s table poll leaves you on
/with a password field where the asset table used to be. On this branch it navigates to the login page.44 tests in
tests/test_auth.py, 1992 in the suite overall, and 99 frontend tests pass.ruff check,ruff format --checkandmypyare clean on the changed files.Checklist
Tested against a dev stack rather than on hardware. My only Pi is in use at the moment so I can't test it there.
🤖 Generated with Claude Code