From be57dab612dea7363dc07bedadadc33011936889 Mon Sep 17 00:00:00 2001 From: Melvin Carvalho Date: Mon, 8 Jun 2026 08:45:12 +0200 Subject: [PATCH 1/3] =?UTF-8?q?fix(idp):=20handleSchnorrComplete=20recover?= =?UTF-8?q?s=20from=20interactionFinished=20throw=20=E2=80=94=20no=20more?= =?UTF-8?q?=20504=20hang=20(#412)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The handler called `reply.hijack()` BEFORE `provider.interactionFinished`. When the `_interaction` cookie is missing — opening the URL in a different browser, third-party cookies blocked, long idle past the cookie TTL — oidc-provider's internal `#getInteraction` throws `SessionNotFound`. Hijack had already marked the reply sent, so Fastify could not write a response and the connection sat open until the gateway 504'd. Approach C from the issue: wrap `interactionFinished` in a try/catch INSIDE the hijack window and, on failure, write a 4xx error response directly to `reply.raw` (where the socket actually lives once hijacked). SessionNotFound → 400 with a user-readable 'session expired, restart login' page; other errors → 500 with the message. A guard skips the recovery if `reply.raw.headersSent` — meaning interactionFinished managed a partial write before throwing, leaving the socket past recovery — and lets the outer catch log it. Scope-limited to handleSchnorrComplete per the issue. The same hijack-then-throw anti-pattern exists at handleLogin (browser branch), handleConsent, handlePasskeyComplete, and handlePasskeySkip; deliberately leaving those alone — separate sweep if/when wanted. Full test suite: 914/914 passing. --- src/idp/interactions.js | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/src/idp/interactions.js b/src/idp/interactions.js index d6adcb0c..d8d865b1 100644 --- a/src/idp/interactions.js +++ b/src/idp/interactions.js @@ -819,8 +819,37 @@ export async function handleSchnorrComplete(request, reply, provider) { request.log.info({ accountId: account.id, uid }, 'Schnorr login completed'); + // Hijack so oidc-provider can write the OIDC continue redirect + // straight to the underlying socket. Wrap the call so a throw + // doesn't leave the hijacked reply unsent — the common failure is + // SessionNotFound from oidc-provider's `#getInteraction` when the + // `_interaction` cookie is missing (URL pasted into a different + // browser, third-party cookies blocked, long idle past the cookie + // TTL). Without this guard, hijack-then-throw leaves Fastify unable + // to send a response and the connection hangs until the gateway + // 504s. See #412. reply.hijack(); - return provider.interactionFinished(request.raw, reply.raw, interaction.result, { mergeWithLastSubmission: false }); + try { + await provider.interactionFinished(request.raw, reply.raw, interaction.result, { mergeWithLastSubmission: false }); + return; + } catch (err) { + // If interactionFinished managed to write headers before throwing, + // the socket is past recovery — propagate so the outer catch logs. + if (reply.raw.headersSent) throw err; + request.log.warn( + { err: err.message, errName: err.name, uid, accountId }, + 'Schnorr complete: interactionFinished failed after hijack' + ); + const isSessionMissing = err.name === 'SessionNotFound'; + const status = isSessionMissing ? 400 : 500; + const title = isSessionMissing ? 'Session expired' : 'Login error'; + const message = isSessionMissing + ? 'Your login session cookie is missing or expired. This usually means the link was opened in a different browser, third-party cookies are blocked, or too much time passed between steps. Please restart the login from the beginning.' + : (err.message || 'Unexpected error completing login.'); + reply.raw.writeHead(status, { 'Content-Type': 'text/html; charset=utf-8' }); + reply.raw.end(errorPage(title, message)); + return; + } } catch (err) { request.log.error(err, 'Schnorr complete error'); return reply.code(500).type('text/html').send(errorPage('Error', err.message)); From 52be9cd71dad97a227fc88602c5e9aec1f619666 Mon Sep 17 00:00:00 2001 From: Melvin Carvalho Date: Mon, 8 Jun 2026 09:00:51 +0200 Subject: [PATCH 2/3] review fixes (#539): stop leaking err.message + add regression tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses both Copilot comments on PR #539: 1. **Soft info-leak in the 500 branch.** The hijacked-recovery 500 path surfaced raw err.message (adapter / oidc-provider error strings on an auth endpoint). Replace with a generic message; full details remain in request.log.warn. Mirrors the existing handleSwitchAccount guidance at interactions.js:365-367. 2. **No automated test.** Add test/schnorr-complete-defensive.test.js covering: - SessionNotFound → 400 with the 'Session expired' page (the bug this PR exists to fix) - Unknown error → 500 with a generic message AND the test asserts the internal err.message does NOT appear in the body (regression guard for the info-leak fix) - Happy path → interactionFinished called, no error response written (sanity check) Tests use a fixture DATA_ROOT + real createAccount, plus a stub provider — no full server needed. 3 new tests, full suite now 917/917 passing. --- src/idp/interactions.js | 6 +- test/schnorr-complete-defensive.test.js | 159 ++++++++++++++++++++++++ 2 files changed, 164 insertions(+), 1 deletion(-) create mode 100644 test/schnorr-complete-defensive.test.js diff --git a/src/idp/interactions.js b/src/idp/interactions.js index d8d865b1..fb86657d 100644 --- a/src/idp/interactions.js +++ b/src/idp/interactions.js @@ -840,12 +840,16 @@ export async function handleSchnorrComplete(request, reply, provider) { { err: err.message, errName: err.name, uid, accountId }, 'Schnorr complete: interactionFinished failed after hijack' ); + // Don't surface raw err.message — adapter/provider errors and + // stack-leaking strings on an auth endpoint are a soft info-leak + // (mirrors handleSwitchAccount's guidance above). Full error is + // already in request.log.warn above. const isSessionMissing = err.name === 'SessionNotFound'; const status = isSessionMissing ? 400 : 500; const title = isSessionMissing ? 'Session expired' : 'Login error'; const message = isSessionMissing ? 'Your login session cookie is missing or expired. This usually means the link was opened in a different browser, third-party cookies are blocked, or too much time passed between steps. Please restart the login from the beginning.' - : (err.message || 'Unexpected error completing login.'); + : 'Unexpected error completing login. Please try signing in again.'; reply.raw.writeHead(status, { 'Content-Type': 'text/html; charset=utf-8' }); reply.raw.end(errorPage(title, message)); return; diff --git a/test/schnorr-complete-defensive.test.js b/test/schnorr-complete-defensive.test.js new file mode 100644 index 00000000..fbd57b52 --- /dev/null +++ b/test/schnorr-complete-defensive.test.js @@ -0,0 +1,159 @@ +/** + * Regression test for #412 — handleSchnorrComplete must not hang the + * connection (→ gateway 504) when `provider.interactionFinished` throws + * after `reply.hijack()`. The common trigger is `SessionNotFound` from + * oidc-provider's `#getInteraction` when the `_interaction` cookie is + * missing (cross-browser URL paste, third-party cookies blocked, long + * idle past the cookie TTL). + * + * Strategy: direct unit-test of the handler with a real account on a + * fixture DATA_ROOT (so `findById` returns a hit) plus a stub provider + * that throws SessionNotFound from `interactionFinished`. Captures the + * post-hijack write to `reply.raw` and asserts a bounded 400 response + * with the "session expired, restart login" page — instead of hanging. + */ + +import { describe, it, before, after } from 'node:test'; +import assert from 'node:assert'; +import fs from 'fs-extra'; +import path from 'path'; +import { handleSchnorrComplete } from '../src/idp/interactions.js'; +import { createAccount } from '../src/idp/accounts.js'; + +const DATA_DIR = path.resolve('./test-data-schnorr-complete-defensive'); + +function makeFakeReplyAndCapture() { + let hijacked = false; + let status = null; + let headers = null; + const bodyChunks = []; + let ended = false; + return { + reply: { + hijack: () => { hijacked = true; }, + code: () => { throw new Error('reply.code called after hijack — should not happen'); }, + raw: { + get headersSent() { return ended; }, + writeHead: (s, h) => { status = s; headers = h; }, + end: (body) => { ended = true; if (body) bodyChunks.push(body); }, + }, + }, + captured: () => ({ hijacked, status, headers, body: bodyChunks.join('') }), + }; +} + +function makeFakeRequest({ uid, accountId }) { + const logs = []; + return { + params: { uid }, + query: { accountId }, + raw: {}, + log: { + info: (...args) => logs.push(['info', args]), + warn: (...args) => logs.push(['warn', args]), + error: (...args) => logs.push(['error', args]), + }, + _logs: logs, + }; +} + +describe('handleSchnorrComplete defensive recovery (#412)', () => { + let originalDataRoot; + let account; + + before(async () => { + originalDataRoot = process.env.DATA_ROOT; + process.env.DATA_ROOT = DATA_DIR; + await fs.remove(DATA_DIR); + await fs.ensureDir(DATA_DIR); + account = await createAccount({ + username: 'schnorrtest', + email: 'schnorr@example.org', + password: 'test12345', + webId: 'https://example.org/schnorrtest/profile/card#me', + }); + }); + + after(async () => { + await fs.remove(DATA_DIR); + if (originalDataRoot === undefined) delete process.env.DATA_ROOT; + else process.env.DATA_ROOT = originalDataRoot; + }); + + it('returns 400 with a "session expired" page (not 504) when interactionFinished throws SessionNotFound', async () => { + const sessionErr = new Error('invalid_request — interaction session id cookie not found'); + sessionErr.name = 'SessionNotFound'; + + const provider = { + Interaction: { + find: async () => ({ + result: { login: { accountId: account.id } }, + exp: Math.floor(Date.now() / 1000) + 600, + }), + }, + interactionFinished: async () => { throw sessionErr; }, + }; + + const { reply, captured } = makeFakeReplyAndCapture(); + const request = makeFakeRequest({ uid: 'fake-uid', accountId: account.id }); + + await handleSchnorrComplete(request, reply, provider); + + const c = captured(); + assert.strictEqual(c.hijacked, true, 'must hijack before calling interactionFinished'); + assert.strictEqual(c.status, 400, 'SessionNotFound must produce 400, not a hang'); + assert.ok(c.headers && /text\/html/.test(c.headers['Content-Type']), 'response must be HTML'); + assert.ok(/Session expired/.test(c.body), 'page title should be "Session expired"'); + assert.ok(/restart the login/i.test(c.body), 'page should tell the user to restart'); + }); + + it('returns 500 with a generic message (no err.message leak) when interactionFinished throws an unknown error', async () => { + // Soft info-leak guard — mirrors handleSwitchAccount's pattern. + // The internal error message must NOT appear in the response body. + const internalErr = new Error('INTERNAL_DETAIL_xyz123_should_not_leak'); + + const provider = { + Interaction: { + find: async () => ({ + result: { login: { accountId: account.id } }, + exp: Math.floor(Date.now() / 1000) + 600, + }), + }, + interactionFinished: async () => { throw internalErr; }, + }; + + const { reply, captured } = makeFakeReplyAndCapture(); + const request = makeFakeRequest({ uid: 'fake-uid', accountId: account.id }); + + await handleSchnorrComplete(request, reply, provider); + + const c = captured(); + assert.strictEqual(c.status, 500, 'unknown error must produce 500'); + assert.ok(!/INTERNAL_DETAIL_xyz123_should_not_leak/.test(c.body), + 'response body must not surface the raw err.message'); + assert.ok(/Login error/.test(c.body), 'page title should be the generic "Login error"'); + }); + + it('completes successfully (no error response) when interactionFinished resolves', async () => { + let interactionFinishedCalled = false; + const provider = { + Interaction: { + find: async () => ({ + result: { login: { accountId: account.id } }, + exp: Math.floor(Date.now() / 1000) + 600, + }), + }, + interactionFinished: async () => { interactionFinishedCalled = true; /* oidc-provider would write to reply.raw here */ }, + }; + + const { reply, captured } = makeFakeReplyAndCapture(); + const request = makeFakeRequest({ uid: 'fake-uid', accountId: account.id }); + + await handleSchnorrComplete(request, reply, provider); + + const c = captured(); + assert.strictEqual(interactionFinishedCalled, true, 'interactionFinished must be called'); + assert.strictEqual(c.hijacked, true, 'reply must be hijacked'); + assert.strictEqual(c.status, null, 'no error response should be written when interactionFinished succeeds'); + }); +}); From 0fdb72096d41320e1475afbc9a08f144d5585bca Mon Sep 17 00:00:00 2001 From: Melvin Carvalho Date: Mon, 8 Jun 2026 09:28:29 +0200 Subject: [PATCH 3/3] review pass-3 (#539): log payload preserves stack + mock matches Node behaviour MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three Copilot findings: 1. request.log.warn was logging `{ err: err.message, errName: err.name }` as flat strings, dropping the stack. Pass the Error under `err` so Pino (via Fastify) serializes name + message + stack + structured fields. 2. The adjacent comment said 'Full error is already in request.log.warn above' — true now that #1 lands. Updated the wording to explicitly call out the stack. 3. Test mock's `reply.raw.headersSent` flipped only on end(); in real Node it flips on writeHead(). Track `headersWritten` instead so the fake mirrors http.ServerResponse and the `if (reply.raw.headersSent) throw err;` guard can't be silently bypassed by a future test. 917/917 still passing. --- src/idp/interactions.js | 9 ++++++--- test/schnorr-complete-defensive.test.js | 12 ++++++++---- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/src/idp/interactions.js b/src/idp/interactions.js index fb86657d..101190e7 100644 --- a/src/idp/interactions.js +++ b/src/idp/interactions.js @@ -836,14 +836,17 @@ export async function handleSchnorrComplete(request, reply, provider) { // If interactionFinished managed to write headers before throwing, // the socket is past recovery — propagate so the outer catch logs. if (reply.raw.headersSent) throw err; + // Pass the Error itself under `err` — Pino (via Fastify) serializes + // it properly (name, message, stack, structured fields). Logging + // err.message + err.name as flat strings would drop the stack. request.log.warn( - { err: err.message, errName: err.name, uid, accountId }, + { err, uid, accountId }, 'Schnorr complete: interactionFinished failed after hijack' ); // Don't surface raw err.message — adapter/provider errors and // stack-leaking strings on an auth endpoint are a soft info-leak - // (mirrors handleSwitchAccount's guidance above). Full error is - // already in request.log.warn above. + // (mirrors handleSwitchAccount's guidance above). The full error + // (with stack) is in request.log.warn above. const isSessionMissing = err.name === 'SessionNotFound'; const status = isSessionMissing ? 400 : 500; const title = isSessionMissing ? 'Session expired' : 'Login error'; diff --git a/test/schnorr-complete-defensive.test.js b/test/schnorr-complete-defensive.test.js index fbd57b52..e38b347f 100644 --- a/test/schnorr-complete-defensive.test.js +++ b/test/schnorr-complete-defensive.test.js @@ -27,15 +27,19 @@ function makeFakeReplyAndCapture() { let status = null; let headers = null; const bodyChunks = []; - let ended = false; + // Mirror Node's http.ServerResponse: `headersSent` flips to true the + // moment writeHead() commits the status line + headers to the socket, + // not only after end(). Modelling it any other way would let the + // `if (reply.raw.headersSent) throw err;` guard pass when it shouldn't. + let headersWritten = false; return { reply: { hijack: () => { hijacked = true; }, code: () => { throw new Error('reply.code called after hijack — should not happen'); }, raw: { - get headersSent() { return ended; }, - writeHead: (s, h) => { status = s; headers = h; }, - end: (body) => { ended = true; if (body) bodyChunks.push(body); }, + get headersSent() { return headersWritten; }, + writeHead: (s, h) => { status = s; headers = h; headersWritten = true; }, + end: (body) => { headersWritten = true; if (body) bodyChunks.push(body); }, }, }, captured: () => ({ hijacked, status, headers, body: bodyChunks.join('') }),