v1.8.0: SuperTokens behind an AUTH_MODE switch - #7
Draft
NeverEndingCode wants to merge 11 commits into
Draft
Conversation
Seven tasks derived from spec section 5. No v1.8 plan existed - only the
design - so this is the missing half.
Grounded against the current code rather than the spec's summary of it:
confirmed req.user.sub is 25 of the 27 req.user reads (username and avatarUrl
one each), which is what makes the "zero route handler changes" seam real;
confirmed identities.supertokens_user_id already ships unused from v1.7; and
confirmed supertokens-node@24.0.3 declares no engines constraint, so it is
importable on the Node 20 production image.
Two v1.7 lessons are carried in as global constraints. The dependency-on-a-
newer-Node trap that left CI silently red for four commits is now an explicit
check in Task 2 Step 1. The ${VAR:-default} vs ${VAR-default} distinction is
spelled out in Task 1 Step 3, because the colon form treats an explicitly
empty value as unset and would defeat the documented rollback.
Also corrected a claim the spec inherits: it warns that `postgres://` is
rejected, which v1.7 disproved for rackstack's own DATABASE_URL (pg accepts
both schemes, verified directly). The SuperTokens core is a different
component and does reject it, so the plan scopes that warning to the
SuperTokens connection URI instead of restating the debunked general claim.
Records the sequencing precondition honestly: the spec gates v1.8 on v1.7
being confirmed in production, which has not happened. That blocks running
shadow mode against real identities and blocks cutover - but not building,
since AUTH_MODE defaults to passport and every task is inert until an
operator changes it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011JYNx69xztWN2qDLe5m1aS
Establishes the strangler switch before any SuperTokens code exists, so
every later task lands behind a guard that is already tested rather than
one added around it afterwards.
server/authMode.js resolves AUTH_MODE to one of passport (default), dual or
supertokens. Two decisions worth recording:
Blank means passport, because blanking the field in the Unraid UI is the
documented rollback and must not be an error. But an unrecognised value
THROWS at boot rather than falling back - a typo'd AUTH_MODE=supertoken that
quietly served the legacy stack would be indistinguishable from a completed
rollout, and would surface weeks later from the wrong symptom. Matching is
case-sensitive for the same reason, with the error naming the likely intent
so the fix is in the message.
Documented in all four places an operator can set it: .env.example,
README.md, unraid-template.xml (Display="advanced", so it stays out of the
way of people who will never touch it), and docker-compose.yml. The compose
entry uses ${AUTH_MODE-passport}, deliberately not the colon form - per the
v1.7 finding, ${VAR:-default} treats an explicitly empty value as unset and
would substitute the default right back, defeating the documented rollback.
Also adds docs/supertokens-rollout-runbook.md rather than leaving the
README pointing at a file that does not exist. It is honest about state: a
status table of what is and is not built, and Parts B-D marked pending. Part
A (the OAuth redirect widening) is written in full now because it is pure
documentation, knowable today, and must be applied days BEFORE any cutover -
GitHub requires the redirect path to be a subdirectory of the registered
callback, and /auth/callback/github is not a subdirectory of
/auth/github/callback, so every SuperTokens GitHub login would otherwise
fail with a redirect_uri mismatch. The instruction widens the registration
to /auth, which is additive and reversible; nothing is removed and passport
keeps working throughout.
The runbook also corrects a claim inherited from the design: `postgres://`
is rejected by the SuperTokens core specifically, not by rackstack's own
DATABASE_URL, which v1.7 proved accepts either scheme.
Containment verified: npm run test:all green on both backends with no
AUTH_MODE set - sqlite 507 passed/26 skipped, postgres 530 passed/3 skipped
(+15 each, the new authMode suite). 15 tests cover the default, the empty
and whitespace cases, all three valid values, the throw, the casing
message, and set-level properties: no mode leaves both stacks disabled, and
exactly one mode runs each stack alone.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011JYNx69xztWN2qDLe5m1aS
Adds supertokens-node@24 and everything needed to bring SuperTokens up, all
of it dormant in the default passport mode.
CONTAINMENT. Every supertokens-node import is dynamic and inside
initSuperTokens(), after the mode check. This is a hard requirement, not
style: the SDK pulls in nodemailer, twilio and libphonenumber-js to serve
recipes this project never initialises, and v1.7 shipped four commits of
silently-red CI because a top-level import dragged in a package that could
not load on the runtime's Node version. A test asserts the absence of a
static import at the source level, and was confirmed to fail when one is
added. A second test asserts app.js mounts no extra middleware in passport
mode.
buildApp() is now async - not anticipated by the plan. SuperTokens'
middleware() can only be mounted after init(), and init imports dynamically,
so the mount point is necessarily async. All seven call sites updated. It
also takes an { env } override so a test can build the app in another mode
without mutating process.env and leaking that into sibling suites - the
ambient-environment trap that cost v1.7 a whole test run's validity.
Discord's scope is pinned to 'identify' to match passport-discord.
SuperTokens' built-in provider would also request 'email'; asking existing
players to consent to a new scope mid-rollout is indistinguishable from a
phishing prompt and would damage trust in the migration.
The compose service is opt-in via profiles, not a depends_on. In the default
mode RackStack never contacts the core, and blocking every deployment on a
container it will not use turns an unrelated SuperTokens problem into a
RackStack outage.
FINDING, recorded in the spec: the assumption that SuperTokens'
thirdPartyUserId equals passport's profile.id - which section 5.5 called
"load-bearing and unverified" - is now verified at source level for both
providers. supertokens-node's GitHub provider sets thirdPartyUserId =
`${user.id}` and passport-github2 sets profile.id = String(json.id); for
Discord, supertokens maps userInfoMap userId to 'id' and passport-discord
passes Discord's raw JSON through. Same field, same stringification, both
providers. This raises confidence but does NOT retire the shadow gate: what
matters is the values already in the owner's identities rows, which may
predate these library versions.
nodemailer advisory (2 high, via supertokens-node) assessed and accepted,
not fixed. No patched 8.x exists and npm's suggested remediation is
downgrading supertokens-node from 24 to 9.2.3. The vulnerable surface is
message-level `raw`, reachable only when sending email, and nodemailer is
referenced only under the emailpassword/emailverification/passwordless/
webauthn SMTP delivery services - verified by grepping the installed
package. This release initialises ThirdParty and Session only, so none is
ever constructed. Revisit if an email-bearing recipe is ever added.
Node 20 engine check passed: supertokens-node introduces no package
declaring engines.node > 20.
DOCS, updated in the same commit per the owner's instruction. The runbook
gains a full "How your existing Discord and GitHub logins carry over"
section - that no save is rewritten and no id renumbered, what a player
actually experiences in each case (nothing, in every case), the one
assumption it rests on and why the shadow gate still exists, and that
Discord and GitHub remain separate accounts as they always have. Part B
(standing up the core) is now written in full. The plan gains a findings
section recording every deviation above; the spec gains the verification
note, a scoping correction so the postgres:// warning cannot creep back into
applying to DATABASE_URL, and a status block.
Verified: test:all green on both backends - sqlite 526 passed/26 skipped,
postgres 549 passed/3 skipped (+19 each). All six e2e smoke suites pass, 39
assertions, zero errors. Real boots in both modes: passport serves
/auth/authorisationurl as SPA HTML (nothing handles it), dual answers with
SuperTokens JSON - proving the conditional mount works in both directions.
Not verified: a live SuperTokens core, whose image pull hit a Docker Hub
rate limit here.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011JYNx69xztWN2qDLe5m1aS
Gap found while reviewing the previous commit's own claim. Task 2 asserted the Node 20 engine check passed, but no test ever imported the SDK: the containment tests run in passport mode, and every configuration-error test throws at init.js's validation - all of which sits BEFORE the dynamic import. The suite would therefore have been fully green on a runtime where supertokens-node could not be loaded at all. That is the exact shape of the v1.7 failure, where four commits of CI were silently red because the broken import lived on a path no green test exercised. An engines field is a claim by the package; loading it is the check. Adds two tests that import supertokens-node, both recipes, and the express framework bindings app.js mounts, asserting each exposes what is used. CI runs Node 20 to match the production image, so this is what makes CI meaningful for this dependency rather than merely passing. Does not weaken the containment assertions: those are source-level (init.js must contain no static import of the SDK), not module-registry-level, and vitest isolates by file. 21 tests in the file, green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011JYNx69xztWN2qDLe5m1aS
NeverEndingCode
changed the title
The mechanism the release hinges on. SuperTokens mints its own internal user id; RackStack's users.id is `provider:providerId` and is the target of three FKs, every save row and every SUPER_ADMIN_IDS entry. createUserIdMapping reconciles them in one direction only - SuperTokens' id is mapped onto ours. The core rewrites user ids in every response once a mapping exists, so the mapping has to be created before the session is. Hence the override sits on the ThirdParty recipe function, not the API: SuperTokens creates the session in the API layer, after the recipe function returns. Two db interface functions on both drivers: getIdentity (side-effect-free, so the override can ask "does this player exist?" without creating them) and setSupertokensUserId (idempotent on re-login; a row rewriting its own value does not conflict with itself). Three things the plan did not anticipate: - The SDK parameter is `superTokensUserId`, capital T. The lowercase spelling is accepted silently as undefined and no mapping is ever created - which fails as the invisible wrong-save bug, not as an error. Design doc and plan both corrected. - The existing-mapping check has to run first. On a returning login the core hands back the EXTERNAL id, so createUserIdMapping reports UNKNOWN_SUPERTOKENS_USER_ID_ERROR for the correct steady state. Treating that as failure would break every login after the first. - The ordering test is vacuous against a dropped `await` unless the fake core is genuinely async. Found by mutation, not inspection: with an instantly-resolving fake the dangling promise won the race anyway. Every fake core method now crosses a macrotask boundary, and a comment says why. Verified by mutation in both directions, plus a negative control in the suite that wires the same logic after session creation and proves the end state is identical while the session id is permanently wrong. 546 tests green on SQLite, 569 on Postgres. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
requireAuth becomes a chain: SuperTokens session first, then the legacy JWT
cookie, then 401. Both branches populate an identical
`{ sub, username, avatarUrl }`, so no route handler changes - req.user.sub is
the only identity field they read.
Three decisions the plan did not anticipate:
- The chain branches on "is SuperTokens initialised in this process", not on
AUTH_MODE. requireAuth is module-level middleware shared by every route
while the mode is per-buildApp, so reading the mode there would mean
guessing which app a request belongs to.
- The login routes moved out of api.js into authRoutes.js as a factory. They
are the only routes whose registration depends on the mode, and api.js
exports a singleton router - gating them in place would leak one app's
routes into another built in the same process.
- A SuperTokens session whose subject matches no users row is rejected rather
than trusted. It would mean the mapping resolved to something users has
never heard of, which is the silent empty-save outcome by another route.
The JWT branch runs in every mode including `supertokens`. That is what makes
the rollback real: legacy cookies keep working for their full 90-day expiry
through a transition in either direction. `supertokens` mode stops issuing
them; it does not start rejecting the ones already out there.
Logout is registered in every mode and clears both stacks. A logout that only
clears half of a dual-stack session shows the user a logged-out UI while
leaving them authenticated.
requireRole confirmed unchanged by reading it - it re-derives roles from
req.user.sub on every request - and now asserted through both stacks.
Found and recorded, not fixed: the client has no SuperTokens frontend SDK, so
it cannot refresh an expired access token. Harmless in `dual` (the legacy
cookie still authenticates), but a supertokens-only cutover needs frontend
refresh handling first. Carried into the runbook in Task 7.
Mutation-verified both ways: rethrowing instead of falling through fails the
two fall-through tests; reversing the chain order fails the ordering test.
572 tests green on SQLite, 595 on Postgres, 39 smoke assertions across all six
e2e suites.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The gate that has to read 100% before anyone cuts over. Nothing here writes: it issues SELECTs only, touches no session, and is designed to be pointed at production while players are online. The plan framed shadow mode as "given a completed SuperTokens login, compare it against identities". Built literally, that is a gate you can only open after walking through the door - it needs the core reachable and someone logging in through it, which is most of what the gate is meant to clear. The way out is that the risk is asymmetric. The equality has two halves: what SuperTokens will compute, and what is already stored. Task 3 verified the first at the source level for both providers. Only the second cannot be checked by reading code, because those rows were written by whatever library versions were installed the day each player first logged in. So the gate audits exactly that half - for every identity row, does user_id equal provider:provider_id? No core, no login, no cutover; it runs against a restored export on a laptop. `npm run shadow:check` is the operator entry point and exits 0 only on a clean pass. The per-login form (createShadowRun) was still built and is useful once dual is on, but the runbook is explicit that the audit is the gate. An empty run exits non-zero and says GATE: NOT RUN. Zero comparisons gives a 100% match rate by vacuous arithmetic, and a gate that passed because it read nothing would manufacture exactly the false confidence it exists to prevent - most likely on a mistyped DATABASE_URL, i.e. when an operator is least able to notice. Verified by running the CLI against an empty database. no-identity rows are counted separately and excluded from the rate: a player who has never logged in is evidence neither for nor against the assumption. The no-write property is asserted table-wide, by snapshotting all of identities around a run. "The row I looked at is unchanged" is not the guarantee that matters here. Not run against production identities - the owner's export has not been supplied and v1.7 has not been cut over. The runbook says so plainly. 587 tests green on SQLite, 610 on Postgres. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Task 6 was mostly satisfied by the runbook's Part A already. The remaining work was propagating the same guidance into the two other places an operator configures OAuth - README's setup section and the provider blocks in .env.example - so someone setting the project up today is told about the GitHub callback widening before they hit it, not only someone who opens the runbook. All three restate that the widening is additive and reversible, that the CALLBACK_URL variables stay pointing at the passport paths, and that it must happen before AUTH_MODE changes. Task 7 finishes the runbook (cutover, rollback, and a D6 that explains why supertokens-only mode is not recommended yet), bumps to 1.8.0, and writes the changelog. New: `[auth] SuperTokens initialised (...)` at boot. Found while doing the real-boot verification - all three modes came up clean and completely silently, so nothing in the log an operator actually reads distinguished a working dual boot from one that had quietly not initialised SuperTokens. The line names the mode, the core URI and the providers. Verified by booting `node server/index.js` for real in each mode and probing over HTTP, not just checking for a listening line: passport /auth/github 302 SuperTokens NOT initialised dual /auth/github 302 initialised, providers=github,discord supertokens /auth/github 200 initialised, providers=github,discord That is the containment guarantee and the route gating confirmed at the process level rather than only under supertest. client/package.json deliberately stays at 1.5.0 - client/vite.config.js reads the root package.json as the single version authority. The runbook now LEADS with what has not been verified: shadow mode never run against production identities, no cutover anywhere, v1.7 itself not yet cut over on Unraid, no SuperTokens core ever run against this code outside tests, and supertokens-only mode pending frontend refresh work. v1.7's runbook had to be corrected for implying more had been rehearsed than had. 587 tests on SQLite, 610 on Postgres, 39 smoke assertions, all green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The mapping override is tested against a fake core, which is what makes its ordering assertions possible - but a fake answers to any method name, so nothing there would notice if the real SDK renamed or dropped these. These two tests are the only check that the object init.js hands to buildSignInUpOverride actually carries the functions it will call, and that the capital-T parameter spelling is still what the SDK reads. Worth pinning because of how it fails otherwise: an absent or misspelled createUserIdMapping means no mapping is created, and no mapping means a returning player silently lands on an empty save, with no error at the moment it happens. Verified against the installed supertokens-node 24.0.3 before writing them. 589 tests on SQLite, 612 on Postgres. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
NeverEndingCode
changed the title
The runbook's status table still listed Task 7 as not started, though the deployment docs and the 1.8.0 version bump landed in 340ccc9. The spec's progress block cited 587/610 tests, from before be68275 added two. Measured now: sqlite 589 passed/26 skipped, postgres 612 passed/3 skipped, 39 e2e smoke assertions with zero errors. Documentation-only; no behaviour change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011JYNx69xztWN2qDLe5m1aS
…eview
Both live in the seam between our configuration and supertokens-node's
defaults, which is where nothing else was looking.
HIGH - authentication bypass, potential account takeover.
SuperTokens' stock signInUpPOST accepts EITHER redirectURIInfo (the browser
authorization-code flow) OR a caller-supplied oAuthTokens object, and treats a
submitted token as proof of identity:
recipe/thirdparty/api/signinup.js
else if (bodyParams.oAuthTokens !== undefined) { oAuthTokens = ... }
The audience check that should make that safe is dead code for GitHub in the
pinned SDK. providers/github.js DEFINES config.validateAccessToken - which asks
api.github.com/applications/{client_id}/token whether the token was minted for
this app - but that is only ever invoked from the GENERIC getUserInfo in
providers/custom.js, and github.js then REPLACES getUserInfo wholesale in an
override applied last. Its replacement calls api.github.com/user with
`Bearer <token>` and asks nothing about the token's origin. Verified by reading
both files in node_modules, not inferred.
Unpatched, an unauthenticated
POST /auth/signinup {"thirdPartyId":"github","oAuthTokens":{"access_token":"..."}}
carrying ANY GitHub token able to read /user - one minted for an unrelated
OAuth app the victim authorised, or a leaked PAT - resolves to that victim's
thirdPartyUserId. Our mapping then faithfully turns it into their users.id and
issues a session. SUPER_ADMIN_IDS values are deterministic and effectively
public (github:37058311), so the owner's account is the obvious target, and
that path reaches every admin route.
This is a regression against the passport stack rather than a pre-existing
flaw: passport-github2 only ever obtains a token by exchanging an authorization
code with our own client secret, so a foreign token cannot be replayed at it.
Fixed with an `apis` override rejecting the token-submission flow entirely.
RackStack is browser-only and has no native client, so that flow has no
legitimate caller here; the redirect flow it keeps obtains its token via our
own client secret and is therefore bound to this application.
Never exploitable in production: it exists only in dual/supertokens mode, and
AUTH_MODE has never been anything but passport anywhere. It would have gone
live the moment the owner followed Part D of the runbook.
CORRECTNESS - every Discord login would have failed.
Self-inflicted, in Task 2. We pin Discord to scope ['identify'] to match
passport-discord, so returning players are not re-prompted to consent to a new
permission mid-rollout. But SuperTokens' API layer substitutes a placeholder
email only when requireEmail === false, and otherwise returns
NO_EMAIL_GIVEN_BY_PROVIDER - and Discord's built-in provider does not set it.
The two choices combined into a total Discord outage that fails in the API
layer, before the mapping override runs, where none of our own code or tests
would have seen it.
Fixed by setting requireEmail: false on the Discord provider, keeping the
narrow scope. Safe because RackStack never uses email: identity is
provider:providerId end to end and upsertUser takes only provider, providerId,
username and avatarUrl.
9 regression tests in tests/supertokens.security.test.js. Verified
discriminating: with both fixes reverted, 4 fail, including the wiring check
that the guard is actually installed in ThirdParty.init - a unit test of the
function alone cannot see that half.
Recorded in the runbook (which now leads with the review outcome) and in the
spec's risk register.
sqlite 598 passed/26 skipped, postgres 621 passed/3 skipped.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011JYNx69xztWN2qDLe5m1aS
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
1 participant
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.
Introduces SuperTokens as an alternative authentication stack behind a
strangler switch. All 7 planned tasks are built.
Upgrading changes nothing.
AUTH_MODEdefaults topassport, which isthe login stack that ships today — SuperTokens is not initialised, its
middleware is not mounted, and
supertokens-nodeis not even imported. Thewhole release is inert until an operator opts in.
What is here
AUTH_MODE:passport(default) /dual/supertokens. Anunrecognised value stops the container on purpose rather than quietly
serving the legacy stack — a typo that looked like a completed rollout
would only be noticed weeks later.
server/supertokens/mapping.js) — the mechanism therelease hinges on. SuperTokens' internal user id is mapped onto the
existing
users.id(provider:providerId), so saves, roles,SUPER_ADMIN_IDSand every foreign key resolve unchanged. No player hasanything to do; there is no "migrate your account" step.
populate an identical
req.user, so no route handler changed.npm run shadow:checkaudits every stored identityand reports whether the mapping would resolve correctly, before anything is
switched on. Read-only.
docs/supertokens-rollout-runbook.md.The part worth reviewing closely
The mapping must be created before the session is issued. The core
rewrites user ids in its responses once a mapping exists, so a session created
first carries SuperTokens' internal id permanently, and every route then
resolves a user id matching no save — a returning player silently lands on an
empty save, with no error at the moment it happens.
That is why the override sits on the ThirdParty recipe function, not the
API: SuperTokens creates the session in the API layer, after the recipe
function returns.
tests/supertokens.mapping.test.jsasserts the ordering directly rather thanasserting the end state, which is identical either way. It includes a
negative control that wires the same logic after session creation and
proves the mapping exists while the session id is wrong — so an "is there a
mapping at the end?" assertion can never be mistaken for a test of this.
Three things the plan got wrong, found while building
superTokensUserIdhas a capital T. The plan and design doc both wrotesupertokensUserId, which the SDK accepts silently asundefined— nothrow, no log, no mapping. Both documents corrected; the spelling is now
pinned by a test against the shipped type declaration.
core hands back the external id, so
createUserIdMappingreportsUNKNOWN_SUPERTOKENS_USER_ID_ERRORfor the fully-correct steady state.Treating that as a failure would break every login after the first.
await. Found bymutation, not inspection: with an instantly-resolving fake core, the
dangling promise still won the race. Every fake core method now crosses a
macrotask boundary, and a comment explains why it must stay.
Verification
npm run test:all)node server/index.jsboot in each of the three modes, probed overHTTP:
passport→ passport routes live, SuperTokens not initialised;dual→ routes live, SuperTokens up;supertokens→ passport routesabsent, SuperTokens up
What has NOT been verified
current Unraid export has not been supplied.
supertokens-only mode is not recommended yet: the client has noSuperTokens frontend SDK, so it cannot refresh an expired access token.
Harmless in
dual(the legacy cookie still authenticates), but asupertokens-only cutover needs frontend refresh work first.
dualis theintended resting state for this release.
The runbook leads with this same list rather than burying it.
🤖 Generated with Claude Code