chore(core,express): post-release audit follow-ups - #113
Merged
Conversation
Bound the refresh-result cache: entries were keyed by the rotating refresh cookie and never looked up again, so the map grew unbounded and retained tokens for the process lifetime. Sweep expired entries (throttled off the hot path) and cap total size as a backstop. Memoize the JWKS key set per auth-server URL in verifySignedAuthResponse. It was rebuilt inside the function on every call, so jose's key cache and refetch cooldown never engaged and every verification made an extra request to the auth server's JWKS endpoint. Make SeamlessAuthUser.email optional and phone nullable to match the cookie payload and the upstream users/me shape. Export redactSensitiveText from core and use it to mask tokens and secrets before the Express router logs an unhandled 5xx error. The utility was tested but wired nowhere, which left a redaction safety net that did not exist. Reorder the magic-link/check cookie requirement out from under the magic-link prefix, throw on a missing route parameter instead of forwarding the literal string undefined, correct the cookieSecret error message that named a removed environment variable, and drop a redundant res.json(...).end() call.
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.
Summary
Closes the post-release follow-up items from the pre-release audit (#101). Each item was re-verified against current
main(some had comment/ordering context added by intervening PRs, but all were still present).Closes #101.
Substantive
packages/core/src/refreshAccessToken.ts). Entries were keyed by the rotating refresh cookie and only deleted on a later lookup of the same key, which never happens once a token rotates, so the map grew without bound and retained access tokens, refresh tokens, email, phone, and roles for the process lifetime. It now sweeps expired entries (throttled to at most once per TTL window, off the hot path) and caps total size at 10k as a burst backstop. The dedup behavior the cache exists for is unchanged and still covered by the existing test.packages/core/src/verifySignedAuthResponse.ts).createRemoteJWKSetwas constructed inside the function, so jose's per-instance key cache and refetch cooldown never engaged and every login, OAuth callback, OTP verify, and magic-link poll made an extra request to/.well-known/jwks.json. Now memoized in a module-level map keyed by JWKS URL (one entry per auth server).Type accuracy (minor breaking)
SeamlessAuthUser.emailis now optional andphoneisstring | null, matchingCookiePayloadand the upstream/users/meschema (phone: z.string().nullable()).requireAuthcopies these straight from the verified cookie, so the previousstring/stringtypes were unsound. README updated to match. Consumers that readphoneas a non-nullstringwill get a type error and need to handlenull.Redaction wired in (was dead but tested)
redactSensitiveTextexisted in core with a passing test but was exported from nowhere and called by nothing, leaving the false impression of a logging safety net. Rather than delete tested code, it is now exported from core (added to the public-exports test and the core README) and used in the Express router error handler to mask tokens and secrets in the logged 5xx detail. Added a test asserting a bearer token in an error message is redacted.Small correctness / hygiene
/magic-link/checkabove/magic-linkinCOOKIE_REQUIREMENTSso it is no longer shadowed by the prefix match (identical config today, so behavior is unchanged, but it would have silently stopped applying if either diverged).routeParamnow throws a clear error on a missing parameter instead of forwarding the literal string"undefined"upstream.Missing cookieSecreterror that named the removedCOOKIE_SIGNING_KEYenv var..end()afterres.status(...).json(...)infinishLogin.Deliberately not in this PR
as anyoption-object casts (audit bullet 3) are tracked separately as Remove redundant option-object casts in the Express adapter #106; left out to keep that its own reviewable change./internal/bootstrap/admin-invitequestion is server-side (seamless-auth-api), not this repo. I can verify it against the API separately.Checks
No em dashes. Changeset included (minor, both packages).