Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,37 @@
# Changelog

## v1.9.1

- **The SuperTokens login button signed you in and then left you logged out.**
`POST /auth/signinup` answered `status: "OK"`, the client reported success,
the URL went back to `/` — and the very next `GET /api/me` was a 401, so the
login screen came back with nothing wrong on it.

SuperTokens picks the session's *token transfer method* at creation time from
the `st-auth-mode` request header, and when it is absent it defaults to
**header**, not cookies (`session/sessionRequestFunctions.js`: *"We default
to header if we can't 'parse' it or if it's undefined"*). The session came
back in `st-access-token` / `st-refresh-token` response headers; no cookie
was ever set. `supertokens-web-js` sends that header for you, and v1.9.0
hand-rolled the calls without it — the one responsibility of the frontend SDK
that hand-rolling quietly inherited.

Confirmed against a real core, same endpoint, both ways — note that **both
return 200**, which is why nothing anywhere reported an error:

| request | `Set-Cookie` | response headers |
|---|---|---|
| without `st-auth-mode` | *(none)* | `st-access-token`, `st-refresh-token` |
| with `st-auth-mode: cookie` | `sAccessToken`, `sRefreshToken` | *(none)* |

Both `/auth/signinup` and `/auth/session/refresh` now send it.

- **A login that sets no session now says so.** If sign-in succeeds but the
session that follows does not exist, the login screen says the server did not
set a session and suggests checking cookies, instead of silently returning to
a blank login form. The absence of that message is the only reason v1.9.0's
bug reached a production deploy — every layer reported success.

## v1.9.0

- **Every SuperTokens login was impossible, and had been since v1.8.**
Expand Down
2 changes: 1 addition & 1 deletion Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -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.9.0"
LABEL org.opencontainers.image.version="1.9.1"

VOLUME ["/app/data"]
EXPOSE 3000
Expand Down
18 changes: 17 additions & 1 deletion client/src/App.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,12 @@ export default function App() {
// code has to happen BEFORE /api/me, because it is what creates the
// session /api/me would otherwise report as absent.
let callbackFailure = null;
let completedLoginFor = null;
if (callbackProviderFromPath(window.location.pathname)) {
const result = await completeSuperTokensLogin();
if (cancelled) return;
if (!result.ok) callbackFailure = result;
if (result.ok) completedLoginFor = result.provider;
else callbackFailure = result;

// Replace rather than push, and always: leaving a spent ?code= in the
// URL means a reload re-POSTs an authorisation code the provider has
Expand Down Expand Up @@ -60,6 +62,20 @@ export default function App() {
}

if (cancelled) return;

// A sign-in that the server called OK, followed by a session that does
// not exist. There is nothing wrong on the login screen to look at, so
// without this the player just bounces back to it and the only evidence
// is in devtools. That is how v1.9.0's missing `st-auth-mode: cookie`
// header survived a release: signinup answered OK, no cookie was set,
// and the app looked like it had simply forgotten the click.
if (completedLoginFor && !authed) {
window.history.replaceState(
{}, '',
`/?authError=${encodeURIComponent(completedLoginFor)}&authReason=no_session`,
);
}

if (authed) { setUser(authed); setStatus('authed'); } else setStatus('anon');
})();

Expand Down
8 changes: 8 additions & 0 deletions client/src/game/api.js
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,14 @@ function refreshSession() {
const res = await fetch('/auth/session/refresh', {
method: 'POST',
credentials: 'include',
// Same reason as the signinup call in game/auth.js: this is what tells
// SuperTokens to put the rotated tokens back in cookies. Refresh
// tolerates its absence better than session creation does (it infers
// the method from the tokens it was given), but a refresh that
// silently switched the session to header transport would log the
// player out on the next request, which is the same invisible failure
// one step later.
headers: { 'st-auth-mode': 'cookie' },
});
return res.ok;
} catch {
Expand Down
22 changes: 21 additions & 1 deletion client/src/game/auth.js
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,25 @@ export async function completeSuperTokensLogin({
// wholesale).
const res = await requestJSON('/auth/signinup', {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
headers: {
'Content-Type': 'application/json',
// NOT optional, and its absence fails silently. SuperTokens resolves the
// token transfer method at session creation from this header, and when
// it is missing it defaults to "header" - see
// session/sessionRequestFunctions.js: "We default to header if we can't
// 'parse' it or if it's undefined". The session then comes back in
// st-access-token / st-refresh-token RESPONSE headers and no cookie is
// ever set, so signinup answers status "OK", this function reports
// success, and the very next GET /api/me is a 401. The player lands back
// on the login screen with nothing wrong on it. v1.9.0 shipped exactly
// that.
//
// supertokens-web-js sends this header for you; hand-rolling the calls
// means inheriting the responsibility. Cookies are the right choice here
// because the whole app already relies on them - `credentials:
// 'include'` everywhere, and the legacy JWT cookie works the same way.
'st-auth-mode': 'cookie',
},
body: JSON.stringify({
thirdPartyId: provider,
redirectURIInfo: {
Expand Down Expand Up @@ -227,6 +245,8 @@ export function loginErrorMessage(provider, reason) {
return `This account is not allowed to sign in with ${name}.`;
case 'no_email':
return `${name} did not share an email address, which this server requires.`;
case 'no_session':
return `${name} signed you in, but the server did not set a session. Check that cookies are allowed for this site.`;
default:
return `Login with ${name} failed. Try again.`;
}
Expand Down
10 changes: 10 additions & 0 deletions docs/authentication-methods.md
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,16 @@ serialised so a burst of concurrent 401s produces exactly one refresh call.
`GET /api/auth-info` tells the client which stack to drive, so one build serves
`passport` and `supertokens` alike and the rollback stays real.

> **If you touch those calls, keep the `st-auth-mode: cookie` header.**
> SuperTokens chooses the session's token transfer method at creation from that
> header, and **defaults to `header` when it is absent** — the session comes
> back in `st-access-token` response headers and no cookie is set. Every status
> code stays 200: `signinup` reports OK, the client believes it, and the next
> authenticated request is a 401. v1.9.0 shipped that and it survived a
> production deploy, because nothing in the chain reports an error. It is the
> one job `supertokens-web-js` does for you that hand-rolling silently
> inherits, and it applies to any future recipe you add, not just ThirdParty.

**What v1.9 also fixed, and why nothing here was ever exercised before.**
`server/supertokens/init.js` passed its OAuth providers to `ThirdParty.init`
as `signInUpFeature`. The SDK reads `signInAndUpFeature`. The key is optional
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "rackstack-server",
"version": "1.9.0",
"version": "1.9.1",
"private": true,
"type": "module",
"scripts": {
Expand Down
47 changes: 47 additions & 0 deletions tests/clientAuth.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,29 @@ describe('completeSuperTokensLogin', () => {
expect(body.oAuthTokens).toBeUndefined();
});

it('asks for the session in COOKIES, which is what actually logs the player in', async () => {
// The v1.9.0 regression, and the reason it shipped. SuperTokens picks the
// token transfer method at session creation from this header, and with it
// absent defaults to "header" - the session comes back in st-access-token
// response headers, no cookie is set, signinup still answers status OK,
// and the next /api/me is a 401. Nothing in the flow reports an error.
//
// supertokens-web-js sends this for you. Hand-rolling means owning it.
const calls = [];
globalThis.fetch = vi.fn(async (url, opts) => {
calls.push({ url, opts });
return jsonResponse({ status: 'OK', user: { id: 'github:37058311' } });
});

await completeSuperTokensLogin({
pathname: '/auth/callback/github',
search: '?code=abc123',
origin: ORIGIN,
});

expect(calls[0].opts.headers['st-auth-mode']).toBe('cookie');
});

it('does not POST when the player cancelled at the provider', async () => {
globalThis.fetch = vi.fn(async () => jsonResponse({ status: 'OK' }));

Expand Down Expand Up @@ -241,6 +264,30 @@ describe('refresh on 401', () => {
expect(seen).toEqual(['/api/state', '/auth/session/refresh', '/api/state']);
});

it('asks for the refreshed session in cookies too', async () => {
configureAuthRefresh({ loginFlow: 'supertokens' });

const calls = [];
let refreshed = false;
globalThis.fetch = vi.fn(async (url, opts) => {
calls.push({ url, opts });
if (url === '/auth/session/refresh') {
refreshed = true;
return { ok: true, status: 200, text: async () => '' };
}
if (!refreshed) return jsonResponse({ error: 'unauthorized' }, { ok: false, status: 401 });
return jsonResponse({ ok: true });
});

await fetchState();

// A refresh that silently moved the session to header transport would log
// the player out on the next request - the same invisible failure as the
// signinup one, just deferred.
const refresh = calls.find((c) => c.url === '/auth/session/refresh');
expect(refresh.opts.headers['st-auth-mode']).toBe('cookie');
});

it('gives up after a second 401 rather than looping', async () => {
configureAuthRefresh({ loginFlow: 'supertokens' });

Expand Down
39 changes: 39 additions & 0 deletions tests/e2e/smoke-v19.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,7 @@ if (!playwright) {
try {
let authUrlCalls = 0;
let signinupBody = null;
let signinupHeaders = null;

// The provider, stubbed: send the browser straight back to our own
// callback with a code, so nothing external is contacted.
Expand Down Expand Up @@ -302,6 +303,7 @@ if (!playwright) {
// the app genuinely transitions to its authenticated view.
await page.route('**/auth/signinup', async (route) => {
signinupBody = JSON.parse(route.request().postData() || '{}');
signinupHeaders = route.request().headers();
await context.addCookies([{
name: COOKIE_NAME, value: cookieFor(user), url: BASE_URL, httpOnly: true,
}]);
Expand Down Expand Up @@ -341,6 +343,14 @@ if (!playwright) {
// Never oAuthTokens - the server refuses those (rejectRawOAuthTokens).
assert(!signinupBody.oAuthTokens, 'the client must not submit raw oAuthTokens');

// Without this header SuperTokens returns the session in response
// headers instead of cookies, signinup still says OK, and the next
// /api/me is a 401. That is what v1.9.0 shipped.
assert(
signinupHeaders['st-auth-mode'] === 'cookie',
`signinup must ask for cookie transport, got ${signinupHeaders['st-auth-mode']}`,
);

// The spent code must be replaced out of the URL: a reload that re-POSTs
// a burned authorisation code fails and bounces the player to login.
const url = new URL(page.url());
Expand All @@ -367,6 +377,35 @@ if (!playwright) {
} finally { await context.close(); }
});

await check('a signin the server calls OK but that sets no session says so', async () => {
// The v1.9.0 failure mode, reproduced: signinup answers status OK and no
// session cookie is set. The player must be told, not silently returned to
// a login screen with nothing wrong on it - that is what made the missing
// st-auth-mode header survive a release and a production deploy.
const { context, page } = await newPage();
try {
await page.route('**/auth/authorisationurl*', (route) => route.fulfill({
status: 200,
contentType: 'application/json',
body: JSON.stringify({
status: 'OK',
urlWithQueryParams: `${BASE_URL}/auth/callback/github?code=fake-code`,
}),
}));

// OK, but deliberately no cookie - exactly what header transport does.
await page.route('**/auth/signinup', (route) => route.fulfill({
status: 200,
contentType: 'application/json',
body: JSON.stringify({ status: 'OK', user: { id: 'github:1' } }),
}));

await page.goto(`${BASE_URL}/`);
await page.locator('button', { hasText: 'Continue with GitHub' }).click();
await page.locator('text=/did not set a session/i').waitFor({ timeout: 15000 });
} finally { await context.close(); }
});

await check('an unconfigured provider shows a message, not a blank screen', async () => {
const { context, page } = await newPage();
try {
Expand Down
Loading