refactor(idp): sweep hijack-then-throw defensive pattern across all 5 interaction handlers — partial #526 - #540
Merged
melvincarvalho merged 1 commit intoJun 8, 2026
Conversation
Copy link
Copy Markdown
Contributor
Sorry, something went wrong.
… interaction handlers (#526) 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.
Copy link
Copy Markdown
There was a problem hiding this comment.
Pull request overview
This PR refactors IdP interaction completion to eliminate the repeated “reply.hijack(); interactionFinished(...)” pattern and centralize the post-hijack defensive recovery in a single helper, preventing hung sockets (gateway 504) when interactionFinished throws (notably SessionNotFound).
Changes:
- Introduces finishInteractionDefensively() to wrap provider.interactionFinished() after reply.hijack(), log failures with the full Error under the err key, and render a bounded HTML error response (400 for SessionNotFound, otherwise 500).
- Replaces the direct hijack+interactionFinished calls in all five handlers (handleLogin browser branch, handleConsent, handlePasskeyComplete, handlePasskeySkip, handleSchnorrComplete) with the helper.
- Preserves the reply.raw.headersSent rethrow behavior to avoid attempting recovery after a partial write.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
This was referenced Jun 8, 2026
melvincarvalho
added a commit
that referenced
this pull request
Jun 10, 2026
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)
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
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.
Partial fix for #526 — foundation work, not the final UX shape.
What this does
Extracts the post-#539 defensive recovery into a single helper, finishInteractionDefensively, and replaces all five reply.hijack(); return provider.interactionFinished(...) sites in src/idp/interactions.js with calls to it:
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 until the gateway 504s. It produces a bounded 4xx with a "session expired, please restart login" page. Schnorr behaviour unchanged from #539.
Helper design notes
Scope
Foundation only for #526. This is the safety-net (no more 504s); the auto-healing 303-to-retry behaviour #526 ultimately wants can layer on top of this helper without re-touching the five call sites. Updating the cross-reference on #526 once this merges.
Tests
test/schnorr-complete-defensive.test.js (3 cases from #539) continues to pass — handler now calls the helper, behaviour is byte-identical. No new tests added: each code path through the helper (success, SessionNotFound→400, generic→500, no err.message leak) is already exercised by the schnorr test. Full suite 917/917 passing.
The other four handlers are tested via the existing IdP integration tests; their behaviour on the success path is unchanged, and the new failure-recovery branch matches what schnorr's tests already cover. Happy to add direct integration tests for the other four if reviewer wants — would mirror the schnorr unit-test setup.