fix(selfhost): require setup token for app wizard - #1233
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1233 +/- ##
=======================================
Coverage 95.04% 95.04%
=======================================
Files 177 177
Lines 19933 19944 +11
Branches 7176 7179 +3
=======================================
+ Hits 18945 18956 +11
Misses 395 395
Partials 593 593
🚀 New features to boost your workflow:
|
ab4495f to
432181e
Compare
| new URL(request.url).searchParams.get("token") ?? | ||
| request.headers.get("x-setup-token") ?? | ||
| request.headers.get("authorization")?.replace(/^Bearer\s+/i, ""); | ||
| if (suppliedToken !== setupToken) return new Response("invalid setup token", { status: 403 }); |
There was a problem hiding this comment.
P2: Setup token compared with timing-unsafe string operator
Setup token comparison uses !==, enabling timing side-channel attacks.
Use crypto.timingSafeEqual for constant-time token comparison.
AI prompt
Check if this security scanner issue is valid. If so, understand the root cause and fix it. If appropriate, update or add tests. Keep the change focused and preserve intended behavior.
<file name="src/server.ts">
<violation number="1" location="src/server.ts:266">
<priority>P2</priority>
<title>Setup token compared with timing-unsafe string operator</title>
<evidence>The suppliedToken is compared against setupToken using the !== operator, which short-circuits on the first mismatched character and is vulnerable to timing side-channel attacks.</evidence>
<recommendation>Use crypto.timingSafeEqual to compare the supplied token against the configured token, after first checking lengths match (or padding to a fixed length).</recommendation>
</violation>
</file>
# Conflicts: # src/server.ts
| } | ||
| if (path === "/setup") { | ||
| const suppliedToken = | ||
| new URL(request.url).searchParams.get("token") ?? |
There was a problem hiding this comment.
P2: Setup token accepted from URL query parameter, leaking secret to logs and history
Setup token can be passed in the URL query string, leaking it to logs and browser history.
Remove query parameter support; accept the setup token only via secure headers.
AI prompt
Check if this security scanner issue is valid. If so, understand the root cause and fix it. If appropriate, update or add tests. Keep the change focused and preserve intended behavior.
<file name="src/server.ts">
<violation number="1" location="src/server.ts:263">
<priority>P2</priority>
<title>Setup token accepted from URL query parameter, leaking secret to logs and history</title>
<evidence>The /setup endpoint reads the setup token from new URL(request.url).searchParams.get("token"), allowing the secret to be passed in the URL query string. This leaks the token to web server access logs, reverse-proxy logs, browser history, and shared links. The PR adds Referrer-Policy: no-referrer to mitigate referrer leakage but does not address query-string logging.</evidence>
<recommendation>Remove the query-parameter fallback for the setup token. Accept the token only via the x-setup-token or Authorization headers. Update documentation to stop instructing users to visit /setup?token=....</recommendation>
</violation>
</file>
…n via POST form, not URL Resolve the Superagent findings on the first-run setup wizard: - Compare the setup token with a constant-time timingSafeStrEqual instead of `!==`, closing a timing side-channel; reuse it for the signed setup_auth cookie check (DRY). - Stop reading the token from the URL query string (it leaked to access logs, proxies, and browser history). The browser flow now uses a token-entry form that POSTs the token in the request body (renderTokenEntryPage); scripted setups still use the x-setup-token / Authorization: Bearer header. Docs updated.
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
1 similar comment
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Motivation
Description
SELFHOST_SETUP_TOKENbefore exposing the first-run wizard and return400when it is unset and403on a missing/invalid token in requests to/setup(changes insrc/server.ts).setup_statecookie with a signed HttpOnlysetup_authcookie and add helper functionssetupAuthCookieValue,isValidSetupAuthCookie, andcookieValueinsrc/selfhost/setup-wizard.tsto generate and verify the signed cookie using an HMAC.stateparameter before callingexchangeManifestCodeand writing credentials to disk, and addReferrer-Policy: no-referrerto the setup response to avoid leaking token-bearing URLs..env.exampleanddocs/self-hosting.md) to documentPUBLIC_API_ORIGINand the new requiredSELFHOST_SETUP_TOKEN, and add unit tests covering signed-cookie generation, extraction, and invalid cases (test/unit/selfhost-setup-wizard.test.ts).Testing
npx vitest run test/unit/selfhost-setup-wizard.test.ts, which passed (8 testsall ✓).git diff --checksucceeded locally and updated docs/sample env (.env.example,docs/self-hosting.md).npm run typecheckbut it was blocked by locally missing optional dependencies/types forpgandioredisin the environment; typecheck was not completed here.npm auditbut both were blocked by local toolchain/registry issues (jsTokenscoverage remapping error andnpm audit403), so full local coverage and audit could not be completed in this environment.Codex Task