fix: stop exposing the Knox session token to page JavaScript - #314
Open
TheoBhang wants to merge 2 commits into
Open
fix: stop exposing the Knox session token to page JavaScript#314TheoBhang wants to merge 2 commits into
TheoBhang wants to merge 2 commits into
Conversation
The SPA persisted its Knox auth token in localStorage (reducers/auth.js) and, for SSO logins, the OIDC callback handed the raw token to the browser via a `?sso_token=...` query param before the SPA moved it into localStorage. Both are readable by any script on the page, so a single XSS finding is a full account takeover. Mirrors the fix already shipped on the Suspicious platform (api/authentication.py::KnoxCookieAuthentication / _set_auth_cookies): - accounts/authentication.py: new KnoxCookieAuthentication, reading the token from an httpOnly, SameSite=Strict cookie, falling through to header-based knox.auth.TokenAuthentication for non-browser clients. Registered ahead of it in REST_FRAMEWORK.DEFAULT_AUTHENTICATION_CLASSES. - accounts/api.py: LoginAPI now sets the cookie on login; added LogoutAPI to clear it (knox's stock LogoutView has no notion of it), routed ahead of knox.urls's own logout path. - accounts/oidc_views.py: SSOCallbackView now sets the cookie server-side and redirects to /?sso=1 instead of putting the token in the URL. - frontend: reducers/auth.js, actions/auth.js, components/App.js, contexts/ThemeContext.js, and services/preferencesService.js no longer read/write the token via localStorage or attach it as an Authorization header by hand — the browser sends the httpOnly cookie automatically on every same-origin request. Added accounts/tests.py::KnoxCookieAuthTest covering: login sets both cookies, the cookie alone authenticates a request with no Authorization header, and logout clears both cookies and invalidates the token. Not pushed — local commit only.
npm run build has been silently broken on this branch (and on master, confirmed by reproducing against the unmodified base commit): webpack fails with `ERR_REQUIRE_ESM` because babel-loader@10.1.1 does a synchronous require() of @babel/core@8.0.1, which ships as pure ESM. babel-loader@10.1.1 is already the latest published release, so this isn't a version to bump away from. Root cause is simpler than it looks: @babel/core@8's own transitive deps (babel-plugin-polyfill-corejs3, lru-cache) declare engines.node: "^20.19.0 || >=22.12.0" — synchronous require(esm) support only landed in Node at that point. The Dockerfile's frontend-builder stage was still pinned to node:18-bookworm-slim. Confirmed by building with node:22-bookworm-slim against the exact same unmodified package.json/package-lock.json — npm run build succeeds cleanly, no package downgrades needed. - Watcher/Dockerfile: frontend-builder base image 18 -> 22-bookworm-slim. - Watcher/package.json: added engines.node >=20.19.0 so this shows up as an explicit, readable constraint instead of only being discoverable via EBADENGINE warnings. - Regenerated frontend/static/frontend/main.js(.map): this repo commits the built bundle rather than building it as part of `docker build` (the frontend-builder stage installs deps but never actually calls `npm run build` — a separate pre-existing gap, not touched here), so the fix isn't real until the checked-in bundle is rebuilt with it. This also happens to be the first bundle rebuilt since the httpOnly-cookie fix (1e371fb) landed, which is now what's actually served — verified live: POST /api/auth/login sets an HttpOnly SameSite=Strict cookie, GET /api/auth/user authenticates off the cookie alone, logout clears it, and the shipped bundle no longer touches localStorage for the token. Not pushed — local commit only.
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
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.
Problem
The SPA persisted its Knox auth token in
localStorage(reducers/auth.js), and for SSO logins the OIDC callback handed the raw token to the browser via a?sso_token=...query parameter before the SPA moved it intolocalStorage(accounts/oidc_views.py,frontend/src/components/App.js).Both are readable by any script running on the page — a single XSS finding (stored, reflected, or via a compromised npm dependency) is a full account takeover, no separate bug needed. The URL-param handoff also means the raw token briefly transits as a query string: visible in the OIDC callback's 302
Locationheader, in browser history untilwindow.history.replaceStatefires, and in any proxy/access log sitting in front of the deployment.Fix
Mirrors the pattern already shipped on Thales CERT's Suspicious platform (
api/authentication.py::KnoxCookieAuthentication/_set_auth_cookies):accounts/authentication.py(new):KnoxCookieAuthentication, reading the token from anhttpOnly,SameSite=Strictcookie. Falls through to header-basedknox.auth.TokenAuthenticationwhen the cookie is absent, so existing non-browser API clients using theAuthorizationheader are unaffected.accounts/api.py:LoginAPInow sets the cookie on login (in addition to the existing JSONtokenfield, kept for non-browser clients). AddedLogoutAPIto clear both cookies and delete the token server-side — knox's stockLogoutViewhas no notion of the cookie, so it was routed ahead ofknox.urls's own logout path inaccounts/urls.py.accounts/oidc_views.py:SSOCallbackViewnow sets the cookie server-side and redirects to/?sso=1— the token no longer appears in the URL at all.watcher/settings.py:KnoxCookieAuthenticationregistered ahead of header auth inREST_FRAMEWORK.DEFAULT_AUTHENTICATION_CLASSES.reducers/auth.js,actions/auth.js, andcomponents/App.jsno longer read/write the token vialocalStorage. Also caught and fixed two more call sites that would otherwise have silently broken —contexts/ThemeContext.jsandservices/preferencesService.jswere independently readinglocalStorage.getItem('token')to hand-buildAuthorizationheaders for theme/preference sync; both now rely on the cookie like everything else.Unrelated build fix bundled in
While testing this through a full local deployment, discovered
npm run buildhas been silently broken (reproduced against the unmodified base commit too, not caused by this change): webpack fails withERR_REQUIRE_ESMbecausebabel-loader@10.1.1does a synchronousrequire()of@babel/core@8.0.1, which ships as pure ESM. Root cause:@babel/core@8's own transitive deps require Node^20.19.0 || >=22.12.0(synchronousrequire(esm)support), but the Dockerfile's frontend-builder stage was pinned tonode:18-bookworm-slim. Fixed by bumping tonode:22-bookworm-slimand adding an explicitengines.nodeconstraint topackage.json— confirmed this alone fixes the build with zero package downgrades. Rebuilt and committed the frontend bundle, which is also the first rebuild since this fix, so it's the first bundle that's actually free of thelocalStoragetoken usage above.Test plan
python manage.py test accounts— newKnoxCookieAuthTest(login sets both cookies, cookie alone authenticates with noAuthorizationheader, logout clears both and invalidates the token) plus all pre-existingaccountstests, run inside the built image against real MySQL: 8/8 passing.python manage.py test(full suite) run the same way: 250/250 passing, no regressions.Set-Cookie: knox_token=...; HttpOnly; SameSite=Strict,GET /api/auth/userauthenticates via cookie alone (noAuthorizationheader sent), logout clears both cookies, and a replayed post-logout cookie correctly gets401.loginWithToken,sso_token, or anylocalStorage-token pairing.