fix(idp): handleSchnorrComplete recovers from interactionFinished throw — closes #412 by melvincarvalho · Pull Request #539 · JavaScriptSolidServer/JavaScriptSolidServer

melvincarvalho · 2026-06-08T06:45:32Z

Closes #412.

Bug

GET /idp/interaction/<uid>/schnorr-complete?accountId=… hangs and the gateway returns 504 when the _interaction cookie is missing — opening the link in a different browser, third-party cookies blocked, long idle past the cookie's TTL.

Root cause at src/idp/interactions.js:822-823:

reply.hijack();
return provider.interactionFinished(request.raw, reply.raw, interaction.result, { mergeWithLastSubmission: false });

hijack() marks the reply as sent. When interactionFinished's internal #getInteraction throws SessionNotFound, Fastify can no longer write a response and the connection sits open until the gateway times out.

Fix (Approach C from the issue)

Wrap the interactionFinished call in a try/catch INSIDE the hijack window. On failure, write the error response directly to reply.raw — the underlying Node http.ServerResponse that we now own via hijack.

reply.hijack();
try {
  await provider.interactionFinished(...);
  return;
} catch (err) {
  if (reply.raw.headersSent) throw err;  // partial write — outer catch logs
  const isSessionMissing = err.name === 'SessionNotFound';
  reply.raw.writeHead(isSessionMissing ? 400 : 500, { 'Content-Type': 'text/html; charset=utf-8' });
  reply.raw.end(errorPage(/* … restart-login message … */));
}

SessionNotFound → 400 with a user-readable "session expired, please restart login from the beginning" page. Other errors → 500 with the underlying message. The headersSent guard handles the edge case where interactionFinished started writing before throwing.

Scope

Per the issue, scope-limited to handleSchnorrComplete. The same hijack-then-call-throwable anti-pattern exists at four other sites in the file (handleLogin browser branch, handleConsent, handlePasskeyComplete, handlePasskeySkip) — explicitly not touched here, separate sweep if you want one.

The issue also sketches Approach A (move OIDC finalize into handleSchnorrLogin so the cookie is in flight on the same request) as the cleaner long-term fix. That's bigger and out of scope here; C is the "should land regardless" minimum the issue explicitly calls out.

Verification

npm test — 914/914 passing, no regressions. The fix path is mechanically simple and matches the issue's Approach C verbatim; no new test added for this PR. Happy to follow up with a unit test if you want — would need a small mock provider + a fixture account, ~30 lines.

…ow — no more 504 hang (#412) 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.

Copilot

Pull request overview

Fixes a failure mode in the IdP Schnorr login completion endpoint where a missing _interaction cookie could cause provider.interactionFinished() to throw after reply.hijack(), leaving the connection open until a gateway timeout.

Changes:

Wraps provider.interactionFinished() in a try/catch inside the hijacked reply window.
On post-hijack failure (notably SessionNotFound), writes an HTML error response directly to reply.raw to avoid hanging sockets.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

melvincarvalho · 2026-06-08T07:01:09Z

+      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.');


Addressed in 52be9cd. The hijacked-recovery 500 branch no longer surfaces err.message — replaced with "Unexpected error completing login. Please try signing in again." and a comment explicitly mirroring the existing handleSwitchAccount guidance at interactions.js:365-367. Full error details remain in the request.log.warn line above. A test case asserts the internal err.message string never appears in the response body.

melvincarvalho · 2026-06-08T07:01:09Z

+    // 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();


Done in 52be9cd. Added test/schnorr-complete-defensive.test.js with three cases: SessionNotFound → 400 + 'Session expired' page (the bug this PR exists to fix); unknown error → 500 + generic message; happy path → interactionFinished called, no error response. Tests use a fixture DATA_ROOT + real createAccount plus a stub provider — no full server needed. Full suite now 917/917 passing.

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.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

melvincarvalho · 2026-06-08T07:28:48Z

+      request.log.warn(
+        { err: err.message, errName: err.name, uid, accountId },
+        'Schnorr complete: interactionFinished failed after hijack'
+      );


Done in 0fdb720. Switched the warn payload to { err, uid, accountId } so Pino (via Fastify) serializes the full Error (name + message + stack + structured fields) under the err key, instead of flattening to err.message/err.name strings.

melvincarvalho · 2026-06-08T07:28:48Z

+      // 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.


Done in 0fdb720. The warn payload now uses { err } (per the paired thread), so the 'Full error is already in request.log.warn above' wording is accurate — and I updated the comment to explicitly call out 'with stack' so it stays honest if the surrounding code drifts.

melvincarvalho · 2026-06-08T07:28:48Z

+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('') }),
+  };


Done in 0fdb720. The fake reply.raw.headersSent now flips to true on writeHead() (via a headersWritten flag), mirroring Node's http.ServerResponse. A future test that exercises 'interactionFinished writes headers then throws' will now correctly hit the if (reply.raw.headersSent) throw err; guard rather than slipping past it.

… behaviour 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.

… interaction handlers (#526) (#540) Extracts the post-#539 defensive recovery into a single helper, `finishInteractionDefensively`, and replaces all five `reply.hijack(); return provider.interactionFinished(...)` sites with calls to it: - handleLogin (browser branch) — was interactions.js:165 - handleConsent — was interactions.js:286 - handlePasskeyComplete — was interactions.js:605 - handlePasskeySkip — was interactions.js:643 - handleSchnorrComplete — was interactions.js:822 (which already had the inlined version from #539) Net effect for the four newly-covered handlers: an `interactionFinished` throw after hijack — most commonly SessionNotFound from a missing `_interaction` cookie — no longer hangs the socket to a gateway 504. It produces a bounded 4xx with a 'session expired, restart login' page instead. The behaviour on schnorr-complete is unchanged from #539. Foundation work for #526 — the auto-healing 303-to-retry shape that issue ultimately wants can layer on top of this helper without touching the five call sites again. Notes on the helper: - The Error is passed under `err` so Pino (via Fastify) serializes name + message + stack + structured fields. - A `{...logContext, err}` spread order prevents a caller-supplied `err` field from clobbering the real Error. - A `reply.raw.headersSent` guard re-throws if interactionFinished managed a partial write before throwing (socket past recovery). - Raw err.message never leaks to the response body — matches the handleSwitchAccount info-leak guidance at interactions.js:365. Tests: existing test/schnorr-complete-defensive.test.js (3 cases) continues to pass via the helper; no new tests added since each path through the helper is already exercised. Full suite 917/917 passing.

…omment Three Copilot findings (5 inline comments — the DATA_ROOT pollution was flagged 3 times pointing at the same fix): 1. **Unused test imports.** Dropped `before`, `after`, `beforeEach` from the speculative import list — only `describe`, `it`, `afterEach` are used (plus `before`, `after` are now genuinely needed by the DATA_ROOT snapshot/restore added below, so they stay). 2. **createServer({ root }) mutates process.env.DATA_ROOT.** This is confirmed at src/server.js:180. Without restoration, the test directory leaks into any subsequent test that reads DATA_ROOT. Added a snapshot/restore pattern via before/after hooks, mirroring what test/schnorr-complete-defensive.test.js (#539) already does. 3. **Misleading comment in server.js.** The previous wording said parseSize 'handles both forms uniformly' — but the code actually typeof-bypasses for numbers (parseSize would throw on a number because it calls .match). Rewrote the comment to spell out the bypass and the reason (parseSize takes strings only). Full suite 920/920 still passing.

…LIMIT) — closes #474 (#543) * feat: configurable bodyLimit via createServer / --body-limit / JSS_BODY_LIMIT (#474) The hard-coded 10 MiB cap at src/server.js:193 blocked `git push` of established app repos (reporter hit 413 trying to install solid-chat/app, a ~30 MiB pack from 3 years of history). Operators had no way to raise it. Surface added: - `createServer({ bodyLimit })` — accepts a number (bytes) or a size string ("100MB", "1GB", …). String parsing uses the existing config.parseSize helper, so the format matches the `defaultQuota` flag already in the codebase. - CLI: `--body-limit <size>` in bin/jss.js, alongside the existing --cors-proxy-max-* family. - Env: `JSS_BODY_LIMIT` in config.envMap; routed through parseEnvValue's size-coercion branch so JSS_BODY_LIMIT=100MB becomes 104857600. - Config: `bodyLimit` field in config.defaults (default 10 MiB = previous behaviour). createServer normalises with a small ternary so any caller — number or string — gets the same bytes-going-in. Default unchanged so every existing user keeps the same behaviour. Not in this PR: a per-content-type override that lets git pushes exceed the global cap (the issue's 'optional follow-up'). Separate enhancement if anyone wants it; the global option is enough for operators of personal pods to raise their own cap. Test coverage in test/body-limit.test.js (3 cases): - numeric bodyLimit → over-size request gets 413 - string bodyLimit ("100B") → same behaviour, proves parseSize path - in-bounds request → not 413 Full suite 920/920 passing (917 + 3 new). Closes #474. * review fixes (#543): unused imports + DATA_ROOT pollution + clearer comment Three Copilot findings (5 inline comments — the DATA_ROOT pollution was flagged 3 times pointing at the same fix): 1. **Unused test imports.** Dropped `before`, `after`, `beforeEach` from the speculative import list — only `describe`, `it`, `afterEach` are used (plus `before`, `after` are now genuinely needed by the DATA_ROOT snapshot/restore added below, so they stay). 2. **createServer({ root }) mutates process.env.DATA_ROOT.** This is confirmed at src/server.js:180. Without restoration, the test directory leaks into any subsequent test that reads DATA_ROOT. Added a snapshot/restore pattern via before/after hooks, mirroring what test/schnorr-complete-defensive.test.js (#539) already does. 3. **Misleading comment in server.js.** The previous wording said parseSize 'handles both forms uniformly' — but the code actually typeof-bypasses for numbers (parseSize would throw on a number because it calls .match). Rewrote the comment to spell out the bypass and the reason (parseSize takes strings only). Full suite 920/920 still passing.

Bundles everything merged since 0.0.205 was published to npm (2026-06-07): - #539 fix(idp): handleSchnorrComplete recovers from interactionFinished throw — missing _interaction cookie now gets a clean 400 instead of a hung socket / gateway 504 (closes #412) - #540 refactor(idp): sweep the hijack-then-throw defensive pattern across all 5 interaction handlers via a shared finishInteractionDefensively helper (foundation for #526) - #542 fix: align runtime port fallbacks with the canonical 4443 default — one source of truth in config.defaults (closes #477) - #543 feat: configurable bodyLimit via createServer({ bodyLimit }), --body-limit, and JSS_BODY_LIMIT — large git pushes no longer 413 at the hard-coded 10 MiB cap (closes #474) - #546 fix(well-known-did-nostr): probe profile/card.jsonld for root-path WebIDs like https://melvin.solid.social/#me, with a cross-account guard on the subdomain fallback (closes #451)

melvincarvalho requested a review from Copilot June 8, 2026 06:45

Copilot started reviewing on behalf of melvincarvalho June 8, 2026 06:46 View session

melvincarvalho mentioned this pull request Jun 8, 2026

handleSchnorrComplete 504s when the _interaction cookie is missing #412

Closed

Copilot AI reviewed Jun 8, 2026

View reviewed changes

melvincarvalho mentioned this pull request Jun 8, 2026

IdP errors with SessionNotFound on stale session cookies instead of expiring + restarting the interaction #526

Open

melvincarvalho requested a review from Copilot June 8, 2026 07:02

Copilot started reviewing on behalf of melvincarvalho June 8, 2026 07:02 View session

Copilot AI reviewed Jun 8, 2026

View reviewed changes

melvincarvalho merged commit 90bf636 into gh-pages Jun 8, 2026
1 check passed

melvincarvalho deleted the issue-412-schnorr-complete-defensive branch June 8, 2026 07:28

melvincarvalho mentioned this pull request Jun 8, 2026

refactor(idp): sweep hijack-then-throw defensive pattern across all 5 interaction handlers — partial #526 #540

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(idp): handleSchnorrComplete recovers from interactionFinished throw — closes #412#539

fix(idp): handleSchnorrComplete recovers from interactionFinished throw — closes #412#539
melvincarvalho merged 3 commits into
gh-pagesfrom
issue-412-schnorr-complete-defensive

melvincarvalho commented Jun 8, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

melvincarvalho Jun 8, 2026

Uh oh!

melvincarvalho Jun 8, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

melvincarvalho Jun 8, 2026

Uh oh!

melvincarvalho Jun 8, 2026

Uh oh!

melvincarvalho Jun 8, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

melvincarvalho commented Jun 8, 2026

Bug

Fix (Approach C from the issue)

Scope

Verification

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

melvincarvalho Jun 8, 2026

Choose a reason for hiding this comment

Uh oh!

melvincarvalho Jun 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

melvincarvalho Jun 8, 2026

Choose a reason for hiding this comment

Uh oh!

melvincarvalho Jun 8, 2026

Choose a reason for hiding this comment

Uh oh!

melvincarvalho Jun 8, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants