refactor(core): extract the session helpers and make the logger injectable - #138
Merged
Conversation
Seven handlers repeated the same block: verify the signed access token, check it describes the same subject as the response body, read the sid claim, then build the access and refresh cookie payloads. Seven copies of a security-relevant check is seven places to get an early return wrong, and the upstream fields were read untyped, so a rename on the API side surfaced as an undefined cookie field at runtime rather than a type error. issueSessionCookies does the whole thing in one call, verifyUpstreamSession covers the one flow that verifies without issuing a session, and UpstreamSessionResponse states what the API returns. The handlers drop 220 lines. The access cookie from webAuthn/register/finish now carries organizationId: null where it previously omitted the key. Every other flow already included it, and the refresh path writes it on every reissue, so that session disagreed with itself after one refresh. Closes #136
Core wrote to console directly, so an adopter could not capture, level, or silence its output. An adapter with its own logger still leaked core's lines to console, so one request could produce output in two places. setSeamlessLogger accepts anything with warn and error, which a platform logger already satisfies, and defaults back to console when called with nothing. Process-wide rather than per-request: it changes where the diagnostics go, not what context they carry. Core logs a failed signature verification and a misconfigured external-delivery setup, neither of which needs request context, and threading a logger through every handler would be a much larger change. Closes #137
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.
The two items that fell out of #72. Both are core hygiene, kept as separate commits.
Closes #136. Closes #137.
1. Session helpers (#136)
Seven handlers repeated the same block verbatim: verify the signed access token, check it describes the same subject as the response body, read the
sidclaim, then build the access and refresh cookie payloads.Seven copies of a security-relevant check is seven chances to get an early return wrong, and the upstream fields were read untyped, so a rename on the API side would surface as an undefined cookie field at runtime rather than a type error.
verifyUpstreamSessionis exported separately for the one flow that verifies without issuing a session:loginonly sets the pre-auth cookie.switchOrganizationreissues the access cookie without rotating refresh, which is now expressed by omittingrefreshCookieNamerather than by a differently shaped copy of the block.The handlers drop 220 lines (271 deleted, 51 added across seven files).
One intentional behavior change
The access cookie from
POST /webAuthn/register/finishnow carriesorganizationId: nullwhere it previously omitted the key.That was the only inconsistency between the seven copies. Every other flow already included it, and
ensureCookieswritesorganizationId: refreshed.organizationId ?? nullon every reissue, so a registration session disagreed with itself after a single refresh. Both values are falsy and it converged onnullanyway; it is now consistent from the start. Called out in the changeset.2. Injectable logger (#137)
Core wrote to
consoledirectly at two sites. An adopter could not capture, level, or silence it, and an adapter with its own logger still leaked core's lines toconsole— one request producing output in two places, which the Fastify adapter made concrete since it logs guard rejections throughrequest.log.Scope worth knowing: it is process-wide, not per-request, so it changes where diagnostics go rather than attaching request context to them. Core logs exactly two things — a failed signature verification and a misconfigured external-delivery setup — and neither depends on request context. Threading a per-request logger through every handler signature would be a much larger change, and I did not want to imply this delivers that.
grep console. packages/core/srcnow returns only the default logger's own implementation.Tests
upstreamSession.test.js: thesidclaim present, absent, and non-string; both throw paths (unverifiable response, subject mismatch); verification against the configured server and audience; the access-only variant; and that no cookies are built from a response that failed verification.logger.test.js: the console default, an injected logger, resetting, a later swap taking effect, and that the delivery warning actually routes through it rather than toconsole.Checks
pnpm buildclean.pnpm testpasses: 387 tests across the three packages, up 15.