From 285be9eef597d8fdef8b568402242de26e05b155 Mon Sep 17 00:00:00 2001 From: Evan Phyillaier Date: Sat, 8 Aug 2026 01:16:32 -0400 Subject: [PATCH] v1.8.4: dual mode would have refused to start against a good core v1.8.3 fixed the wrong-endpoint probe in the preflight and left a second copy of it in the boot path. That copy still used /recipe/users/count, which SuperTokens core 12 does not implement, and assertCoreRejectsAnonymous throws on anything that is not a 401 - so the 404 would have been read as "the core is running without API_KEYS" and stopped the container. In the preflight that bug printed an alarming line. Here it would have blocked the cutover outright and blamed the operator for a URL this code got wrong. Caught while the owner's deployment was one step away from AUTH_MODE=dual, on a core that the preflight had just confirmed answers 401 correctly. The root cause was duplication, so the fix is de-duplication: the probe now lives in server/supertokens/coreProbe.js and both callers import it. A test asserts that - neither file may hardcode a probe path of its own - because "remember to update both" is exactly what failed here. The boot guard also narrows when it throws. Only a confirmed-open core is fatal now: a known endpoint answering an unkeyed request with 200. A 404, an unexpected status, or an unreachable core all warn and let the boot proceed. A guard on the startup path should refuse only on evidence, never on the absence of it. 662 tests green on SQLite, 688 on Postgres, 39 smoke assertions. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 20 +++++++++++ Dockerfile | 2 +- package.json | 2 +- server/supertokens/coreProbe.js | 56 ++++++++++++++++++++++++++++++ server/supertokens/init.js | 56 ++++++++++++++++++------------ server/supertokens/preflight.js | 41 ++-------------------- tests/supertokens.security.test.js | 47 ++++++++++++++++++++++++- 7 files changed, 161 insertions(+), 63 deletions(-) create mode 100644 server/supertokens/coreProbe.js diff --git a/CHANGELOG.md b/CHANGELOG.md index 26468db..008567f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,25 @@ # Changelog +## v1.8.4 + +- **`AUTH_MODE=dual` would have refused to start against a correctly-secured + core.** v1.8.3 fixed the wrong-endpoint bug in the preflight but left a second + copy of the same probe in the boot path, still using `/recipe/users/count` — + which SuperTokens core 12 does not implement. That guard *throws* on anything + that is not a 401, so the 404 would have been read as "the core is running + without API_KEYS" and stopped the container from booting. + + Unlike the preflight, where the bug printed an alarming line, here it would + have blocked the cutover entirely and blamed the operator for a URL this code + got wrong. + + The probe now lives in `server/supertokens/coreProbe.js` and is imported by + both callers, so they cannot drift again — asserted by a test. The boot guard + now throws **only** on a confirmed-open core (a known endpoint answering an + unkeyed request with 200); every other outcome warns and lets the boot + proceed. + + ## v1.8.3 - **`supertokens:check` no longer reports a correctly-secured core as wide diff --git a/Dockerfile b/Dockerfile index 1e6b16e..6418082 100644 --- a/Dockerfile +++ b/Dockerfile @@ -44,7 +44,7 @@ LABEL org.opencontainers.image.licenses="MIT" # only on a pushed vX.Y.Z tag, and docker/metadata-action derives the # published image's version label from that tag - so this literal only # affects locally-built images, not what GHCR publishes. -LABEL org.opencontainers.image.version="1.8.3" +LABEL org.opencontainers.image.version="1.8.4" VOLUME ["/app/data"] EXPOSE 3000 diff --git a/package.json b/package.json index 4699cff..0699222 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "rackstack-server", - "version": "1.8.3", + "version": "1.8.4", "private": true, "type": "module", "scripts": { diff --git a/server/supertokens/coreProbe.js b/server/supertokens/coreProbe.js new file mode 100644 index 0000000..3d98a2f --- /dev/null +++ b/server/supertokens/coreProbe.js @@ -0,0 +1,56 @@ +// Asking a SuperTokens core whether it requires authentication. +// +// ONE implementation, imported by both the boot-time guard (init.js) and the +// operator preflight (preflight.js). It lives in its own module because it +// previously did not: the same probe was written twice, the path was corrected +// in the preflight, and the copy in the boot path was left behind - where a +// wrong answer does not print a scary line, it stops the container from +// starting. If you add a third caller, import this; do not copy it. + +/** + * Endpoints gated by the core's API key, newest path first. + * + * NOT `/hello` - that answers unauthenticated by design as a health check, so + * a 200 there proves nothing about whether the core is locked down. + * + * More than one because the path is tenant-scoped on modern cores + * (`//users/count`, per supertokens-node's own querier) and was not + * on older ones. v1.8.2 shipped a single guessed path that core 12 does not + * implement, so every probe returned 404. + */ +export const AUTHED_ENDPOINTS = Object.freeze([ + '/public/users/count', // cores with multitenancy (the default tenant) + '/recipe/users/count', // older cores +]); + +/** + * Probes the first endpoint this core actually implements. + * + * Returns `{ status, path }`. `status` is `null` when every candidate 404s, + * which means "this core exposes no path we know how to ask" - NOT "the core + * answered". That distinction is the entire point of this module: a 404 is + * evidence about our URL, not about the core's authentication, and inferring + * a security verdict from one is how v1.8.2 reported a correctly-secured core + * as running wide open. + */ +export async function probeAuthedEndpoint({ + connectionURI, apiKey, fetchImpl = fetch, timeoutMs = 5000, +}) { + const base = connectionURI.replace(/\/$/, ''); + const headers = { 'api-version': '3.0' }; + if (apiKey) headers['api-key'] = apiKey; + + for (const path of AUTHED_ENDPOINTS) { + // eslint-disable-next-line no-await-in-loop + const res = await fetchImpl(`${base}${path}`, { + method: 'GET', headers, signal: AbortSignal.timeout(timeoutMs), + }); + if (res.status !== 404) return { status: res.status, path }; + } + return { status: null, path: null }; +} + +/** A refusal, however the core spells it. */ +export function isRefused(status) { + return status === 401 || status === 403; +} diff --git a/server/supertokens/init.js b/server/supertokens/init.js index 78e7809..0114580 100644 --- a/server/supertokens/init.js +++ b/server/supertokens/init.js @@ -37,6 +37,7 @@ import { isSuperTokensEnabled } from '../authMode.js'; import { buildProviders, resolvePublicOrigin } from './providers.js'; import { buildSignInUpOverride } from './mapping.js'; +import { probeAuthedEndpoint, isRefused } from './coreProbe.js'; // SuperTokens' own default API base path. It is also why the runbook widens // the GitHub OAuth registration to /auth: SuperTokens serves its callbacks at @@ -135,26 +136,25 @@ export function isLoopback(uri) { /** * Confirms the core actually refuses unauthenticated callers. * - * Probes an endpoint that requires an API key when one is configured. A 401 - * means the core is closed and all is well; a 200 means it answered a caller - * holding no key at all, which is the state where anyone who can reach it can - * mint a session for any user id. + * Throws ONLY on a confirmed-open core - an endpoint we know exists answering + * an unkeyed request with 200. Every other outcome warns and lets the boot + * proceed, because this guard sits on the startup path and a wrong answer here + * does not print a scary line, it stops the container. * - * `/hello` is deliberately NOT used - it answers unauthenticated by design as - * a health check, so probing it would prove nothing. + * That distinction was missing in v1.8.2: this probed a single hardcoded path + * that core 12 does not implement, and treated the resulting 404 as proof the + * core was open - so setting AUTH_MODE=dual against a perfectly well-secured + * core would have refused to start, blaming the operator for a URL this code + * got wrong. The probe now lives in ./coreProbe.js and is shared with the + * preflight, so the two cannot drift again. */ export async function assertCoreRejectsAnonymous({ connectionURI, hasKey, fetchImpl = fetch }) { - const url = `${connectionURI.replace(/\/$/, '')}/recipe/users/count`; - let response; + let probe; try { - response = await fetchImpl(url, { - method: 'GET', - headers: { 'api-version': '3.0' }, - signal: AbortSignal.timeout(5000), - }); + probe = await probeAuthedEndpoint({ connectionURI, fetchImpl }); } catch (e) { // Unreachable, DNS failure, timeout. Cannot establish anything; the core - // may simply still be starting. Warn rather than refuse - see the caller. + // may simply still be starting. Warn rather than refuse. console.warn( `[auth] could not verify that the SuperTokens core at ${connectionURI} requires ` + `authentication (${e.message}). If it is running without API_KEYS, anyone who can ` @@ -163,16 +163,28 @@ export async function assertCoreRejectsAnonymous({ connectionURI, hasKey, fetchI return 'unverified'; } - if (response.status === 401) return 'closed'; + if (isRefused(probe.status)) return 'closed'; - throw new Error( - `The SuperTokens core at ${connectionURI} answered an unauthenticated request with ` - + `HTTP ${response.status}, which means it is running without API_KEYS. Anyone who can ` - + 'reach it can mint a session for any user id, including every value in SUPER_ADMIN_IDS, ' - + 'without any request reaching RackStack. Set API_KEYS on the core to the same value as ' - + `SUPERTOKENS_API_KEY here${hasKey ? '' : ' (which is also unset)'}, and do not publish ` - + 'its port.', + if (probe.status === 200) { + throw new Error( + `The SuperTokens core at ${connectionURI} answered an unauthenticated request to ` + + `${probe.path} with HTTP 200, which means it is running without API_KEYS. Anyone who ` + + 'can reach it can mint a session for any user id, including every value in ' + + 'SUPER_ADMIN_IDS, without any request reaching RackStack. Set API_KEYS on the core to ' + + `the same value as SUPERTOKENS_API_KEY here${hasKey ? '' : ' (which is also unset)'}, ` + + 'and do not publish its port.', + ); + } + + // A 404 from every candidate, or any other unexpected status, says our URL + // is wrong for this core version - not that the core is open. Refusing to + // boot on that would be punishing the operator for our own mistake. + console.warn( + `[auth] could not verify that the SuperTokens core at ${connectionURI} requires ` + + `authentication (${probe.status === null ? 'no known endpoint answered' : `unexpected HTTP ${probe.status}`}). ` + + 'This is NOT evidence that it is open, but do check it by hand.', ); + return 'unverified'; } /** diff --git a/server/supertokens/preflight.js b/server/supertokens/preflight.js index 405115e..19659fa 100644 --- a/server/supertokens/preflight.js +++ b/server/supertokens/preflight.js @@ -16,6 +16,7 @@ import { buildProviders, resolvePublicOrigin, PROVIDER_IDS } from './providers.js'; import { isLoopback } from './init.js'; +import { AUTHED_ENDPOINTS, probeAuthedEndpoint, isRefused } from './coreProbe.js'; const PASS = 'PASS'; const FAIL = 'FAIL'; @@ -26,48 +27,12 @@ function result(status, name, detail) { return { status, name, detail }; } -/** - * Endpoints that require an API key when one is configured, newest path first. - * - * NOT `/hello` - that answers unauthenticated by design as a health check, so a - * 200 there proves nothing about whether the core is locked down. - * - * More than one, because the path is tenant-scoped on modern cores - * (`//users/count`, per supertokens-node's own querier) and was not - * on older ones. The first version of this shipped a single guessed path, - * `/recipe/users/count`, which does not exist on core 12 - so every probe came - * back 404 and the check reported a correctly-locked-down core as running wide - * open. See the 404 handling below: that false alarm is the reason this is a - * list and not a constant. - */ -const AUTHED_ENDPOINTS = Object.freeze([ - '/public/users/count', // core with multitenancy (the default tenant) - '/recipe/users/count', // older cores -]); - async function probe(url, { apiKey, fetchImpl, timeoutMs = 5000 }) { const headers = { 'api-version': '3.0' }; if (apiKey) headers['api-key'] = apiKey; return fetchImpl(url, { method: 'GET', headers, signal: AbortSignal.timeout(timeoutMs) }); } -/** - * Probes the first endpoint this core actually implements. - * - * Returns `{ status, path }`, or `{ status: null }` when every candidate 404s - - * which means "this core does not expose any path we know how to ask", NOT - * "the core answered". The distinction is the whole point: a 404 is evidence - * about our URL, not about the core's authentication. - */ -async function probeAuthedEndpoint({ connectionURI, apiKey, fetchImpl }) { - for (const path of AUTHED_ENDPOINTS) { - // eslint-disable-next-line no-await-in-loop - const res = await probe(`${connectionURI}${path}`, { apiKey, fetchImpl }); - if (res.status !== 404) return { status: res.status, path }; - } - return { status: null, path: null }; -} - /** * Runs every deployment check and returns the results. * @@ -158,7 +123,7 @@ export async function runPreflight({ // is exactly why it is checked here rather than left to be noticed. try { const anon = await probeAuthedEndpoint({ connectionURI, fetchImpl }); - if (anon.status === 401 || anon.status === 403) { + if (isRefused(anon.status)) { checks.push(result(PASS, 'core requires authentication', `anonymous request rejected (${anon.status})`)); } else if (anon.status === 200) { checks.push(result( @@ -239,7 +204,7 @@ export async function runPreflight({ const authed = await probeAuthedEndpoint({ connectionURI, apiKey, fetchImpl }); if (authed.status === 200) { checks.push(result(PASS, 'SUPERTOKENS_API_KEY', 'accepted by the core')); - } else if (authed.status === 401 || authed.status === 403) { + } else if (isRefused(authed.status)) { checks.push(result( FAIL, 'SUPERTOKENS_API_KEY', `The core rejected it (HTTP ${authed.status}). It must be byte-identical to a value in ` diff --git a/tests/supertokens.security.test.js b/tests/supertokens.security.test.js index 3a239b2..5223f44 100644 --- a/tests/supertokens.security.test.js +++ b/tests/supertokens.security.test.js @@ -108,7 +108,52 @@ describe('rejectRawOAuthTokens (authentication bypass guard)', () => { fetchImpl: async (url) => { probed = url; return { status: 401 }; }, }); expect(probed).not.toContain('/hello'); - expect(probed).toContain('/recipe/'); + // A key-gated endpoint. The exact path is version-dependent, so assert the + // property rather than a literal - the literal is what broke in v1.8.2. + expect(probed).toContain('users/count'); + }); + + it('BOOTS against a core whose endpoints all 404, rather than calling it open', async () => { + // The v1.8.3 near-miss. The preflight's 404 handling was fixed, but this + // guard kept its own copy of the probe with the old hardcoded path - and + // unlike the preflight, a wrong answer here does not print a scary line, + // it stops the container from starting. + // + // So on core 12 (which does not implement /recipe/users/count) setting + // AUTH_MODE=dual against a perfectly well-secured core would have refused + // to boot, blaming the operator for a URL this code got wrong. The probe + // now lives in coreProbe.js and is shared, so the two cannot drift again. + const { assertCoreRejectsAnonymous } = await import('../server/supertokens/init.js'); + const noKnownEndpoint = async () => ({ status: 404 }); + + await expect(assertCoreRejectsAnonymous({ + connectionURI: 'http://core:3567', hasKey: true, fetchImpl: noKnownEndpoint, + })).resolves.toBe('unverified'); + }); + + it('finds the tenant-scoped path a modern core actually implements', async () => { + const { assertCoreRejectsAnonymous } = await import('../server/supertokens/init.js'); + const core12 = async (url) => (url.endsWith('/public/users/count') + ? { status: 401 } + : { status: 404 }); + + await expect(assertCoreRejectsAnonymous({ + connectionURI: 'http://core:3567', hasKey: true, fetchImpl: core12, + })).resolves.toBe('closed'); + }); + + it('shares one probe implementation with the preflight', async () => { + // The bug was duplication, so this asserts the de-duplication rather than + // the behaviour: both callers must import from coreProbe.js. A future + // caller that re-copies the paths reintroduces exactly this class of bug. + const init = readFileSync(new URL('../server/supertokens/init.js', import.meta.url), 'utf8'); + const preflight = readFileSync(new URL('../server/supertokens/preflight.js', import.meta.url), 'utf8'); + + expect(init).toMatch(/from '\.\/coreProbe\.js'/); + expect(preflight).toMatch(/from '\.\/coreProbe\.js'/); + // Neither may hardcode a probe path of its own. + expect(init).not.toMatch(/users\/count/); + expect(preflight).not.toMatch(/'\/(public|recipe)\/users\/count'/); }); it('refuses to boot against a core too old for this SDK', async () => {