UN-4186 [FIX] Send the CSRF token from one shared axios interceptor instead of hand-written headers - #2306
UN-4186 [FIX] Send the CSRF token from one shared axios interceptor instead of hand-written headers#2306jaseemjaskp wants to merge 2 commits into
Conversation
Add helpers/csrf.js (attachCsrfInterceptor, getCsrfHeaders) and attach it to useAxiosPrivate at instance creation and to the global axios in App.jsx. Remove ~110 hand-written X-CSRFToken headers, move the TopNavBar org switch onto axiosPrivate, document the intentional raw-axios callers, and add an orgApi() URL helper.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…lobal install The origin guard now checks where axios will actually send the request (baseURL unless the URL is absolute), so an external baseURL never receives the token. Move the App.jsx global attach into an idempotent installGlobalCsrfInterceptor() and cover it with a test against the real default axios instance.
Frontend Lint Report (Biome)✅ All checks passed! No linting or formatting issues found. |
|
Unstract test resultsPer-group results
Critical paths
|
vishnuszipstack
left a comment
vishnuszipstack
left a comment
There was a problem hiding this comment.
Reviewed the interceptor design and swept both repos for call sites that could have lost their token. No security or correctness defects found — this is solid work. A few cleanups inline, plus one test-coverage gap worth closing before merge.
Verified rather than assumed
- Interceptor semantics against the pinned axios 1.16, read out of
node_modulesrather than from memory:Axios.prototype.requestlowercasesconfig.methodand convertsconfig.headersto anAxiosHeadersbefore building the interceptor chain (axios.cjs:4948,:4955), andAxiosHeaders.set(name, value, false)only writes when the key is absent. So both load-bearing claims — "method is always present and lowercase" and "never overwrite a caller's header" — hold in production, not just in the tests. - Nothing lost its token. Exactly one
axios.create()in prod code, intercepted at creation. Every remaining raw-axioscall site in OSS and cloud resolves to a relative path, so the same-origin guard can never skip a real request. The only two non-axios transports — the Upload shim'sactionfetch here, and the lookup draftkeepalivePATCH in the cloud PR — both usegetCsrfHeaders(). - The same-origin guard can't misfire:
getBaseUrl()iswindow.location.originunconditionally, and there is noaxios.defaults.baseURLanywhere in the tree. - The cookie fallback is genuinely readable:
CSRF_COOKIE_HTTPONLYandCSRF_USE_SESSIONSare both at Django defaults, soCookies.get("csrftoken")really does work during bootstrap, as the comment claims. - vitest 645/645 passes on this branch, matching the PR notes. Biome is net −5 diagnostics vs. the PR base with no new unused variables — the single exception is
ConnectorsPage.jsx, see the inline comment. - Merge order is right, and the window between the two merges is safe: cloud
main's hand-written headers keep working against OSS-with-interceptor precisely because the interceptor doesn't overwrite.
One design note — no action needed to merge
getCsrfToken() prefers sessionDetails.csrfToken over the live cookie. That value is captured at useSessionValid.js:105, before the POST /organization/{id}/set, so if the backend ever rotates the token in that request the store holds a stale value that now wins over the fresh cookie.
This is not a regression — every hand-written header read the same stale store value, and useLogout does a full page reload, so it can't leak across sessions. But the cookie is the value Django actually compares against, so cookie-first (store as fallback) would be strictly more robust, and would let you drop the csrfToken plumbing from sessionDetails entirely. Related: axios 1.16 ships xsrfCookieName / xsrfHeaderName / withXSRFToken, which would cover most of helpers/csrf.js — but that's only worth a look if you go cookie-first, since the built-in never consults the store.
Follow-up ticket, not this PR
orgApi() is now the documented way to build org-scoped URLs, but the paired cloud PR (Zipstack/unstract-cloud#1805) touches ~25 files that still hand-roll /api/v1/unstract/${orgId}/…. The dev guide is ahead of the code — worth a ticket so it doesn't drift.
| } from "./csrf"; | ||
|
|
||
| const runRequestInterceptors = async (instance, config = {}) => { | ||
| let current = { ...config, headers: { ...config.headers } }; |
There was a problem hiding this comment.
The security-relevant assertion doesn't cover the branch that actually runs.
runRequestInterceptors builds headers as a plain object, so all 22 tests take the !headers.set fallback in setHeaderIfMissing. In production axios has already converted config.headers to an AxiosHeaders before request interceptors run (Axios.prototype.request does config.headers = AxiosHeaders.concat(...), axios.cjs:4955), so the live path is headers.set(CSRF_HEADER, value, false) — currently untested. That includes "does not overwrite a caller-supplied token", which is the one behaviour most worth locking down here.
I read AxiosHeaders.set in the pinned 1.16 and the rewrite === false semantics are correct, so this is a coverage gap rather than a bug. Suggestion:
import axios, { AxiosHeaders } from "axios";
// ...
let current = { ...config, headers: new AxiosHeaders(config.headers) };Note the assertions need to change with it — AxiosHeaders normalises keys, so result.headers[CSRF_HEADER] reads undefined and you'd want result.headers.get(CSRF_HEADER). Keeping one explicit plain-object test alongside would cover both branches.
| axiosPrivate.get(getUrl(`connector/users/${id}/`), { | ||
| headers: { "X-CSRFToken": sessionDetails?.csrfToken }, | ||
| }), | ||
| axiosPrivate.get(getUrl(`connector/users/${id}/`), {}), |
There was a problem hiding this comment.
Leftover empty config object from the mechanical edit — the argument does nothing now. Four sites in this file: here, line 68, line 188, and line 244 (which becomes post(url, updateData, {})). Worth dropping so the next reader doesn't wonder what config was intended.
| axiosPrivate.delete(getUrl(`connector/${id}/owners/${userId}/`), {}), | ||
| }), | ||
| [sessionDetails?.csrfToken], | ||
| [], |
There was a problem hiding this comment.
This went from [sessionDetails?.csrfToken] to [], but the closure still captures axiosPrivate and getUrl.
It's the one file where Biome got worse on this branch: useExhaustiveDependencies for ConnectorsPage.jsx goes 4 → 6 diagnostics vs. the PR base (I diffed the full biome check src output both ways — everything else is net −5). [axiosPrivate, getUrl] is both more correct and quieter.
2 participants
What
helpers/csrf.js:attachCsrfInterceptorsetsX-CSRFTokenon same-origin POST/PUT/PATCH/DELETE requests, reading the token from the session store with acsrftokencookie fallback.getCsrfHeaders()covers transports that bypass axios.useAxiosPrivate(at instance creation) and to the globalaxiosinApp.jsx.X-CSRFTokenheader in OSS (~110), plus thecsrfTokenlocals and hook deps that only existed to feed them.axiosontoaxiosPrivate.useSessionValid,SetOrg,FeatureFlagsData; andsocket-logs-store, which also stops swallowing errors silently).orgApi(path)helper for/api/v1/unstract/<orgId>/…URLs, used in the files rewritten here.Why
axios/fetchcalls bypassed the 401 → logout handling.How
useMemorather than the existing effect: a child handedaxiosPrivatecan fire a request from its own mount effect, which runs before the parent's effect.App.jsxmirrors the existing request-id interceptor, so the few calls that must stay on raw axios still get CSRF without hand-written headers.headersprop (Manage Documents) usesgetCsrfHeaders(), since that path isfetch.evaluateFeatureFlag/listFlagsdrop theircsrfTokenparameter; the only caller is updated andevaluateFeatureFlaghas none.Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
helpers/csrf.js, so it depends on this. This PR is safe on its own: cloud plugins still send their own header, and the interceptor never overwrites it.axiosPrivate, so a 401 there logs the user out instead of only showing an alert. That is the intended behaviour for an expired session.Database Migrations
Env Config
Relevant Docs
Related Issues or PRs
Dependencies Versions
Notes on Testing
csrf.test.js(22) andorgApi.test.js(3). OSS only: vitest 645/645 andbun run buildpass.cascade-and-affordances.test.jsxand also fail onmain(plugin CSS and existingdefaultProps).x-csrftokenpresent and 2xx on the fresh-login org set (including after Django rotated the token on login), socket-log POST, Prompt Studio project create / prompt create / prompt PATCH, PDF upload through Manage Documents, the full "Deploy as API" chain (export, workflow, endpoints, tool instance,api/deployment/201), and DELETE of deployment, workflow and project. GETs correctly carry no header.Screenshots
Checklist
I have read and understood the Contribution Guidelines.