security: AppSec audit remediation (0.11.0) - #40
Merged
Merged
Conversation
added 11 commits
September 24, 2026 14:58
npm audit fix resolves the six production findings (fastify request/response spoofing, fast-uri host confusion, find-my-way HTTP/2 DoS, path-to-regexp ReDoS, body-parser limit bypass). Production high/critical is now zero. One moderate qs advisory remains because express@4 pins the vulnerable range and the only fix is an express 5 major, which is separate work. vitest 2 to 4 and testcontainers 10 to 12 were taken as well, since the dev toolchain runs on every CI job and every contributor machine. The suite passes unchanged (708 passed, 15 skipped, 51 files). The only accommodation is a raised testTimeout: the parity and docs tests invoke the real TypeScript compiler per case, and a cold compile under vitest 4 exceeds the 5s default, so those failures were slowness rather than API breakage. CI gains an audit job that gates on the production tree at high, and reports the dev tree advisory-only so a dev-tool advisory cannot block unrelated PRs. Dependabot runs weekly so this does not silently regrow.
…te (#34) writeNapCookieSuccess('session') emitted `session=TOKEN; Path=/`, so the access token was readable by any script on the page, travelled in cleartext on any plain-HTTP request, and rode along on cross-site requests. That is the default path and the failure is silent: login works and the cookie is simply unprotected. The helper exists to keep the credential away from script, and toPublicSessionView already withholds access_token from GET /auth/session on the assumption of an HttpOnly cookie the default never produced. The second half was worse. Partial options replaced the attributes rather than adding to them, so a caller passing { domain: '.example.com' } to set one attribute silently lost all three protections. Caller options now spread over { httpOnly: true, secure: true, sameSite: 'lax', path: '/' }, which keeps an explicit httpOnly: false working as the local-development escape hatch. attrs is now always defined, so the set and the logout clear both stamp unconditionally, and the COOKIE_ATTRS round-trip the clear depends on is unchanged. nap-java already defaulted this way, closing a divergence where the same deployment was safe on the JVM and not on Node. Six tests added across both adapters, the partial-options case being the regression that matters. Full suite: 708 passed, 15 skipped.
InMemoryChallengeStore and InMemorySessionStore only ever flagged records: challenges were marked expired, sessions got a revoked_at, and both stayed in their maps for the life of the process. /auth/init is unauthenticated, so that growth is driven by anyone who can reach the server. Both stores now sweep on the amortised once-per-tick schedule already used by prune() in rateLimit.ts, for the reason documented there: scanning on every call makes the component that should absorb a flood amplify it. The retention bounds are the ones the protocol needs, not merely expiry. A redeemed challenge is kept until result_cache_until, because RFC 13.3 retry safety is the promise that a client repeating a completion gets its cached answer instead of a "not found". A session is kept until refresh_expires_at where refresh is enabled, because reuse detection only works while a superseded refresh token still resolves to its lineage. Both stores take an injectable Clock, defaulting to the wall clock so existing constructor calls are unaffected. Tests that pin the server to a fixed timestamp now pass that clock to the stores as well: a store sweeping on the wall clock would collect their fixtures as years expired.
Both session handlers built their guard options without `clock`, so a server configured with an injected clock still judged session expiry by the wall clock on this one endpoint. A session minted at the injected `now` was reported as absent, and the 401 gave no hint the two were disagreeing about time rather than about the session. The literal fix is one field, so instead of adding it three times the guard options for the router's own routes now come from a single builder per adapter. The three call sites drifting is what produced the bug, and a builder is the shape where adding a field cannot reach some routes and miss others. Each adapter gets a pair of tests, not one: a session live on the injected clock returns 200, and a session the injected clock has moved past still returns 401. The second exists because the first alone would also pass if expiry were simply not checked.
…ception exactUrlMatch() called new URL() on the u tag with no guard, and that tag is attacker-supplied. A completion carrying u: "not-a-url" threw TypeError out of verifyNip98Completion() instead of returning NAP_COMPLETE_URL_MISMATCH. The throw cost three things at once, on an unauthenticated endpoint. The adapter answered 500 rather than the uniform 401, so the response was distinguishable from every other failure. It escaped the padAuthResponse() floor, which exists so latency cannot reveal which check failed (RFC 15). And it happened before logFailure(), so the request produced no audit record at all: an operator saw 500s in the web server log and nothing in the NAP stream. A valid signature is needed to reach the URL check, but that costs an attacker nothing since any throwaway key will do. The asymmetry is what makes this worth fixing rather than documenting. Every other hostile input on this path is already handled deliberately, and parseVoucherSecret() writes down exactly this reasoning: a thrown parse error turns a hostile string into a 500 that is both an availability problem and an oracle. The u tag was the one attacker-controlled field that was missed. Twelve tests, in two groups. The first covers the fix: six malformed inputs neither throw nor match, in both argument positions. The second guards against a bad fix, and is the more important half. This is the audience binding, so making the function total by loosening the comparison would be an authentication bypass. A trailing slash, a different path, host, scheme or port, and userinfo must all still fail, while identical URLs and scheme/host case normalisation must still match. nap-java is unaffected: Nip98Validator.exactUrlMatch() already wraps URI.create() with a string-equality fallback. Refs #33
Closes the two parts of the dependency issue that were not files. CodeQL runs on push, on pull requests, and weekly. The schedule is the part worth keeping: it re-analyses unchanged code against updated queries, which is how a newly published vulnerability class gets found in code nobody has touched. security-extended rather than the default pack, because the default is tuned to keep false positives near zero on any repository and this one is an authentication library, where a quieter scan is the wrong trade. It is a separate workflow rather than another job in ci.yml. CodeQL takes minutes where the existing jobs take seconds, and a weekly cron only makes sense on its own file. SECURITY.md covers what a commit cannot: secret scanning and push protection, private vulnerability reporting, Dependabot alerts, and branch protection are repository settings. They are written down because a control nobody recorded is a control nobody turns back on after it is disabled. It also explains why the production dependency audit gates a pull request and the dev audit does not, so the next person does not "fix" the inconsistency. The scope notes at the end are for whoever audits this next: where the highest-severity surfaces are, that the voucher extension reaches the network from request-supplied URLs, that SQL store retention is still open, and that the response floor is load-bearing. That last one is why the malformed u tag bug mattered more than it looked: a 500 escaping the floor, unaudited. Refs #36
…read path Code review of the eviction fix found it missed the growth path it was written for. InMemorySessionStore swept from getByAccessToken() alone, so a server taking logins and serving no guarded requests never swept at all, and every expired session stayed resident. That is the shape of the attack the issue describes: /auth/complete is reachable without a session, and nothing obliges a caller to follow it with a guarded request. Measured before the fix: 500 logins, all 500 expired, 500 still resident. createForChallenge() now sweeps too. The once-per-tick bound already stops this costing anything on a busy server, and the read-path sweep stays because a long-lived server that stops taking new logins should still shed old ones. The regression test asserts a constant rather than a threshold, which is what makes it meaningful: residue is the sliding retention window, so it tracks the session TTL and not the traffic volume. Ten times the logins must leave the same amount behind. It measures 61 at both 500 and 5000 logins, and fails at 500-of-500 with the write-path sweep removed. The Java store was already correct here (it sweeps from createForChallenge), so this also closes a divergence between the two implementations rather than only fixing a leak. Refs #35
The Unreleased section documented the cookie defaults and nothing else, so four of the five fixes from the security audit would have shipped unannounced: the malformed u tag turning /auth/complete into a 500, the in-memory stores never evicting, /auth/session ignoring the injected clock, and the dependency work. Each entry says what was wrong and why it mattered rather than naming the symptom, because the reasoning is the part a reader cannot reconstruct. The u tag entry in particular records that half its tests exist to stop the fix going the other way, since making that function total by loosening the comparison would be an authentication bypass rather than a fix.
SECURITY.md told an operator to require a check called `Validate`. No such check exists: `validate` is a matrix job over two Node versions, so GitHub reports `Validate (Node 20.19.0)` and `Validate (Node 22.x)` separately. Someone following the instruction would have searched for a name that never appears and concluded branch protection could not be configured, or worse protected nothing and believed otherwise. Found while verifying every factual claim in the file against the workflows rather than against memory. The rest checked out: the audit job gates the production tree at high and reports the dev tree advisorily (confirmed by exit code, 0 at --audit-level=high and 1 at low where a moderate exists), CodeQL runs security-extended, the Postgres store genuinely has no DELETE path, and both allowlists do refuse an empty list at wiring time.
The two defects this remediation actually shipped were both invisible from inside a single package. The session store swept on the read path but not the write path, and the Express adapter gained a cookie fix the Fastify adapter did not. Neither is findable by testing either side alone, and both are the same shape: two things that should agree and do not. The per-package suites all passed throughout. So this file drives both adapters from one table rather than testing each separately, which makes an asymmetric fix a failure rather than a gap nobody looks for. Adding a third adapter means adding a row, and the existing cases then cover it. Imports come from the package names a consumer would use rather than relative paths, so a broken exports map fails here before it reaches anyone. Verified it catches the real defects rather than merely passing: reverting the Fastify cookie merge fails 2 cases, and removing the write-path sweep fails 1. The store cases assert equality across a tenfold volume difference instead of a threshold. Residue is the retention window, so it tracks the TTL and not the traffic; a threshold would pass against an unbounded store at small volumes, which is exactly how the original leak went unnoticed. The audience cases assert the fix did not also become permissive. Making exactUrlMatch total is only correct if a trailing slash, a different host, scheme or port, and userinfo all still fail, since this is what every NIP-98 proof is checked against.
Minor rather than patch, and the reason is a single line of behaviour: the session cookie now carries Secure by default. A browser will not send a Secure cookie over http://, so a deployment terminating TLS nowhere loses its sessions on upgrade. The change is strictly more secure and still breaking, which is exactly the pair that has to be announced rather than folded into a patch. The store constructors also gained an optional argument and now drop records past their retention bound, so a consumer holding a challenge_id past its TTL sees null where a stale record used to be. All eleven workspace packages share the version and pin each other exactly, so every internal dependency moved with it. Lockfile regenerated. Suite green at 740 passed, 15 skipped, and typecheck clean after the bump.
This was referenced Sep 24, 2026
Closed
Closed
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
Owner
Author
Note on issue closureThe Verified rather than assumed: the GraphQL So #33, #34, #35, #36 and #38 need closing by hand, either on merge here or when |
added 2 commits
September 24, 2026 16:32
The Secure-by-default change is the one that breaks a working deployment,
and it fails in the least helpful way available: login succeeds, the cookie
is set, and the browser then withholds it on the next request because the
origin is not https. Nothing in the logs says "Secure". An operator hitting
that has no reason to connect it to a security fix they just adopted, so
the guide names the symptom and not only the change.
Examples checked against the source, and both were wrong on the first pass:
writeNapCookieSuccess takes cookieName first and options second, and the
in-memory stores take { clock } rather than a retention bound (the bound is
derived from the record, which is what keeps RFC 13.3 retry safety intact).
The eviction note says null rather than undefined because ChallengeStore.get
returns ChallengeRecord | null.
Cross-repo deployment covered too: nap 0.11.0 and nap-java 0.9.0 now agree
on the wire, so a mixed fleet rejects the other side's cookies.
The changelog claimed the production tree went "from four high-severity findings to none". That was true only at the threshold the gate was set to. Three moderate qs advisories were underneath it the whole time: GHSA-4mjr-xmp4-gh2g, GHSA-q8mj-m7cp-5q26 and GHSA-x5fp-wj9c-mxmx. qs is Express's query parser, so these sit on the request path of every deployment using the Express adapter rather than being a build-time concern. Found by cross-checking the lockfile against OSV, which does not apply a severity floor, after the equivalent check on nap-java turned up a critical in bcprov. npm audit fix does not reach it: express pins qs at ~6.14.0 and ~ locks the minor, so no version of express 4.x resolves past 6.14.x. Hence an overrides entry, scoped under express rather than global so it constrains only the dependency that needs it. Two things worth recording, both found by checking rather than assuming: The first override I wrote was ^6.15.4, matching the advisory's "fixed in" boundary. That version does not exist; the range is 2.2.5 - 6.15.3 and the next published release is 6.16.0. npm silently ignores an unsatisfiable override rather than failing, so it looked applied and was not. Regenerating the lockfile to pick up the override also rehoisted vite, which broke typecheck with two copies of its types in examples/merchant-app. Reverted and applied the change to the existing lockfile instead: churn went from 9274 lines to 9, and typecheck is clean again. The gate moves from --audit-level=high to moderate, since the production tree is now at zero rather than at "nothing above high". Verified both directions: the new gate passes on the fixed tree and fails on the tree as it stood before this commit. 740 passed, 15 skipped, typecheck clean, npm ci reproduces qs 6.16.0.
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
2 participants
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.
Security remediation from a full AppSec audit of this repository, plus the release bump the breaking change requires.
Fixes #33, #34, #35, #36 and #38. Audit tracking issue: #37.
These do not auto-close on merge. GitHub only honours closing keywords when a PR targets the default branch, and this targets
developto match the convention here. Verified rather than assumed: this PR'sclosingIssuesReferencesis empty, while the same syntax on tcheeric/nap-java#36 (basemaster, that repo's default) resolves all six. Close them by hand whendevelop -> masterships, which is also when the fix actually reaches a consumer.Upgrade steps for the breaking change are in UPGRADING.md.
Also found after CI ran
Three moderate
qsadvisories were sitting under the audit gate. The changelog claimed the production tree went from four high findings to none, which was true only at--audit-level=high, where the gate was set. GHSA-4mjr-xmp4-gh2g, GHSA-q8mj-m7cp-5q26 and GHSA-x5fp-wj9c-mxmx were underneath it the whole time, andqsis Express's query parser, so they sit on the request path of every deployment using the Express adapter.Found by cross-checking the lockfile against OSV, which applies no severity floor, after the same check on tcheeric/nap-java#36 turned up a critical in bcprov.
npm audit fixcannot reach it: express pinsqsat~6.14.0and~locks the minor. Fixed with anoverridesentry scoped underexpress, and the gate moved to--audit-level=moderatenow that the production tree is genuinely at zero. Verified in both directions: the new gate passes on the fixed tree and fails on the tree as it stood before this branch.Before you merge: one breaking change
The session cookie now carries
Secureby default. A browser will not send aSecurecookie overhttp://, so a deployment that terminates TLS nowhere loses its sessions on upgrade. The change is strictly more secure and still breaking, which is why this is0.11.0rather than a patch.The store constructors also gained an optional argument and now drop records past their retention bound, so a consumer holding a
challenge_idpast its TTL seesnullwhere a stale record used to be.What was wrong
utag threwTypeErrorout ofverifyNip98Completion(), so/auth/completeanswered500instead of the uniform401, skipped thepadAuthResponse()floor, and wrote no audit record. Unauthenticated, one request.writeNapCookieSuccessdefaulted tosession=TOKEN; Path=/with noHttpOnly,SecureorSameSite. Worse, partial options replaced the attributes, so{ domain: '.example.com' }silently dropped all three.InMemoryChallengeStoreandInMemorySessionStorenever removed a record. Both are filled by unauthenticated traffic./auth/sessionbuilt its guard options withoutclock, so a server on an injected clock judged expiry by the wall clock on exactly that endpoint.Two of the #36 advisories sat directly under controls this repo implements: Fastify's
request.hostspoofing underneathcreateRequestDerivedBaseUrlResolver(), andbody-parsersilently disabling size enforcement underneath the 1 kB cap on an unauthenticated endpoint.How each fix was verified
Every fix was reverted or mutated to confirm the matching tests fail, rather than trusting a green run:
utag turns /auth/complete into a 500 (unauthenticated) #33: 12 tests, half of which exist to stop the fix going the other way. MakingexactUrlMatchtotal is only correct if it did not also become permissive, since this is the audience binding and a false positive is an authentication bypass.Domainand all three flags), and an explicithttpOnly: falsestill wins for local development.Two defects the review caught in the remediation itself
Worth flagging, because both were invisible from inside a single package and both passed their per-issue checks:
The eviction fix missed its own growth path. It swept from
getByAccessToken(), the read path. The growth path iscreateForChallenge(). Measured: 500 logins, 500 expired, 500 still resident. Fixed, and the regression test asserts a constant across a tenfold volume difference (61 at both 500 and 5000 logins) rather than a threshold, because a threshold passes against an unbounded store at small volumes.The Express fix initially had no Fastify counterpart. Now covered by
securityRemediation.test.ts, which drives both adapters from one table so an asymmetric fix is a failure rather than a gap nobody looks for.That harness is mutation-proved: reverting the Fastify merge fails 2 cases, removing the write-path sweep fails 1.
Verification
npm run typecheckclean.npm audit --omit=dev: 4 high to 0. The gate was proven to gate by exit code, not assumed from YAML.nap-core.Also included
SECURITY.md, covering the controls a commit cannot enable (secret scanning, push protection, branch protection) with the reasoning for each, plus scope notes for the next auditor. CodeQL and Dependabot workflows.Still open, deliberately
#39 (SQL stores never delete) is not addressed here.
nap-store-postgrescontains noDELETEstatement at all, sonap_sessionsretains access, refresh and previous-refresh tokens in plaintext indefinitely. It wants its own change with its own migration, paired with tcheeric/nap-java#35 so the retention bounds cannot drift between implementations.