Repository navigation
security: AppSec audit remediation (0.9.0) - #36
Merged
Merged
Conversation
added 14 commits
September 24, 2026 15:12
…ssion id The cookie was set with `session.sessionId()` and every read looked the value up with `getBySessionId()`, which made the session identifier the de facto bearer credential. `getByAccessToken()` existed on SessionStore, was implemented by both stores, and had no production caller at all. Three consequences, all now closed: Session ids are not treated as secrets elsewhere. DefaultNapServer logs one on every refresh path, NapSessionFilter logs one on ACL denial, and redeemed_session_id is persisted on the challenge row. Anyone with log access held live credentials. Rotation never reached the credential. rotateRefreshToken() mints a new access token on every refresh, but the cookie carried an id that rotation leaves unchanged, so the value a browser presents was never rotated and the reuse-detection design protected a credential the cookie path did not use. The two implementations disagreed on the wire. nap (TypeScript) writes body.access_token into the cookie and authenticates with getByAccessToken(), so a cookie minted by one server could not be read by the other. nap-it covers TypeScript client interop, which makes that a supported configuration. Read paths updated together: the filter, GET /auth/session, and POST /auth/logout. Logout now resolves the cookie to a session before revoking, since revocation is keyed by session id and passing the cookie value straight through would have matched nothing while still returning 204. Existing tests encoded the old behaviour and were updated to pass the access token as the cookie value. Four regression tests added; three of them fail against the previous code, including the one asserting a rotation retires the previous cookie value. Migration: live sessions hold a cookie that no longer authenticates, so a deploy logs everyone out once. No fallback to getBySessionId() is provided deliberately, since accepting the old credential would keep the issue alive for as long as the fallback existed. Refs #27
… proof `resolveAuthorization()` fell back to reading the proof from a `proof` field in the JSON body when the Authorization header was absent. Four problems compound there: It was not in the protocol surface. Nip98Validator requires `Authorization: Nostr <base64>`, the RFC documents only that, and the TypeScript implementation has no equivalent, so this was a JVM-only accepted credential location that no spec, test vector, or interop test described. The hash coverage was self-referential. The NIP-98 `payload` tag commits to sha256(rawBody), so a proof carried inside the body is part of what it must hash. That is satisfiable only by excluding the field before hashing, which nothing specified and nothing enforced, leaving an unwritten convention on the most security-critical hash in the protocol. It parsed attacker-controlled bytes a second time. parseAuthCompleteRequest() already reads that buffer under a strict shape check; a second reader with different semantics and a blanket catch is a parser-differential surface, and the swallowed exception meant a body that parsed differently in the two places produced no signal at all. And a credential in a body is logged by anything that logs request payloads, which is the reason the refresh endpoint takes its token from a header. The test that asserted the fallback is inverted into a regression test: a valid-looking body proof must now produce a null authorization at the verifier and the uniform 401. Asserting a valid-looking proof is what makes the test meaningful, since a malformed one would be refused either way. objectMapper is now unused. The constructors keep the parameter because they are public API and auto-configuration passes the application's mapper, so removing it would break hand-wired controllers for no gain. Refs #28
Three related gaps around nap.protected-path-prefixes, which reads as though it protects those paths but only selects paths on which authentication is attempted. Enforcement lives in NapPermissionInterceptor and needs a per-handler annotation, so a handler added to a protected controller without one was served to anyone, with nothing at startup, nothing in the log, and nothing in the diff to show for it. require-annotation-on-protected-paths now defaults to true. It only takes effect when protected-path-prefixes is non-empty, so it cannot affect a deployment that has not opted into protected paths; for one that has, a forgotten annotation becomes a loud 500 rather than an open endpoint, and @PublicEndpoint already exists to say "deliberately public" in the source. The filter and the interceptor disagreed about what a path is. The filter matched the raw getRequestURI() while the interceptor stripped the servlet context path first, so under a non-empty context path the filter skipped authentication on requests the interceptor believed were covered. Both now go through one pathWithinApplication() helper. With the interceptor failing closed, that disagreement decides whether a request is authenticated at all, which is why it is fixed in the same commit rather than left as tidying. The three unauthenticated fall-through branches were silent. An operator could not distinguish "nobody is calling this endpoint" from "everybody is, without a session". Each now emits nap_guard_no_session with a reason, mirroring the NAP_GUARD_NO_SESSION code the TypeScript guards emit. Debug rather than warn, since an unauthenticated request to a protected path is ordinary before login. Behaviour change: an application relying on the previous default, with protected prefixes configured and handlers deliberately left unannotated, will now see 500s until those handlers declare @PublicEndpoint or a NAP annotation. That is the intended outcome, since the old default made the safe state the one you had to remember. Refs #29
Two defaults let `nap.enabled=true` alone produce a server that authenticates correctly and authorizes nobody in particular, both silently. AllowAllAclResolver was the default AclResolver, so anyone holding any Nostr key got a session. That is indistinguishable from a working configuration: you wire NAP, log in with your own key, see a session, and ship, with nothing reporting that the authorization layer is a no-op. Combined with the protected-path default fixed in the previous commit, "authenticated" became "reachable" for any endpoint whose annotation was forgotten. There is now no default resolver. Supply one, or set nap.allow-all-principals=true to ask for the old behaviour deliberately, which makes it a written decision rather than an omission. This mirrors what createAudienceHostAllowlist() and createMintAllowlist() already do on the TypeScript side: refuse at wiring time rather than accept a configuration that permits everything. The in-memory stores stay the default, because they are genuinely useful for local development and failing there would be the wrong trade. They now warn at startup, naming the consequence that is a security one rather than only an availability one: revokeByPrincipal() reaches a single node, so a suspended principal keeps working on every other instance until their session expires. README updated. Its setup block previously showed the fail-open configuration as the starting point and described auto-configuration as supplying an AllowAllAclResolver, so following it produced a server that looked finished and authorized everyone. Behaviour change: an application relying on the implicit allow-all will now fail to start with a message naming the property and the alternative. Refs #30
InMemoryChallengeStore and InMemorySessionStore never removed an entry. States were rewritten in place and revocations were stamped, but the maps only grew. Both are filled by unauthenticated traffic (/auth/init writes a challenge under a fresh random id, every completion writes a session), so the footprint tracked total login volume at a rate a caller can accelerate. The outstanding-challenge caps did not help: they count only records still in ISSUED, so they bound concurrency rather than memory. The repository already had the pattern and the reasoning. BoundedEventReplayGuard sweeps on insert and deprecates its unbounded predecessor with exactly this argument; InMemoryRateLimiter amortises its prune to one pass per clock tick; NapSessionFilter caps its ACL cache. The stores were the one place it had not been applied. Both now sweep on insert, CAS-guarded to once per clock tick so a burst cannot make every request walk the map. Neither owns a thread, so neither can outlive its holder. The retention bounds are the part worth reviewing. A challenge is kept until resultCacheUntil when one is set, not merely until expiry, because a redeemed challenge inside that window is what makes a client retry idempotent under RFC 13.3. A session is kept until its absolute cap or its refreshExpiresAt, whichever is later, because getByRefreshToken deliberately answers for revoked sessions so a replay stays visible; evicting on the access window alone would turn a detected reuse into an unknown token, losing the one signal that says a credential leaked. Both stores take an injectable Clock with a systemUTC default, so eviction is testable without sleeping and existing constructor calls are unaffected. Four tests. Two assert the eviction, two assert the records that must survive, and the second pair is the point: a sweep that drops a cached redemption or a live refresh window is a different bug, not a fix. Verified that disabling the sweep fails the eviction tests. Refs #31
This repository had no .github directory at all. Nothing built, nothing ran, and nothing scanned on a pull request, which is why four of the six findings from the security audit were verifiable only by reading. The job runs `mvn -B -ntp verify` on Java 21 so nap-it takes part in the reactor. The interop setup is the part worth reading rather than skimming. Correctness here is defined relative to the TypeScript reference implementation, and the two tests that assert it (TypeScriptClientInteropTest, OfficialTestVectorsTest) both call JUnit assumeTrue when the sibling `nap` checkout is missing. assumeTrue skips rather than fails, so on a bare runner the interop suite reports green while asserting nothing. That is precisely the silent regression this CI exists to catch, so the workflow clones `nap`, installs its dependencies, and then asserts the toolchain is present and fails loudly when it is not. The clone path is not configurable. TypeScriptClientInteropTest hard-codes Path.of(user.home, "IdeaProjects", "nap") with no override property, so the workflow has to match it exactly. Worth replacing with a system property later; until then the coupling is documented where someone changing it will look. OWASP Dependency-Check runs as a separate job at CVSS >= 7. The tree is managed by imani-bom, so transitive CVEs arrive here without a visible version bump, and Jackson matters directly: Nip98Validator and DefaultNapServer parse attacker-controlled JSON on the unauthenticated path. The NVD API key is passed only when the secret is set, because an empty -DnvdApiKey= is rejected outright rather than ignored. Test reports are uploaded on success as well as failure, so skip counts stay reviewable instead of hiding behind a green check. Refs #32
…D feed Two corrections found by actually running the scanner rather than reasoning about it. First, the CVSS >= 7 gate does not pass today. Running dependency-check 13.0.0 against the current tree fails with 12 CVEs in spring-core 6.2.19 and 2 in spring-security-core 6.5.11. Landing that as a blocking gate would turn every pull request red for a pre-existing condition no author introduced or can fix in their own change, which is the fastest way to teach a team that the scanner is noise to be clicked past. The job now reports without blocking. The findings are real work to triage, either an upgrade through imani-bom or suppressions for the CPE false positives spring-core is well known for, and the comment says to remove continue-on-error once that is done so the gate actually bites. Second, building the NVD database from scratch took 12 minutes locally. Paying that on every pull request would make the scan the slowest thing in CI and the first thing someone disables, so the database is now cached with a per-run key and restore-keys so each run starts from the previous database and applies only the delta. Refs #32
Runs on push, on pull requests, and weekly. The scheduled run is the one that matters: it re-analyses unchanged code against updated queries, which is how a newly published vulnerability class is found in code nobody has touched. security-extended rather than the default query pack. The default is tuned to keep false positives near zero on any repository; this is an authentication library, so a quieter scan is the wrong trade. Autobuild runs Maven, so the JDK is pinned to 21 to match what the project targets. Without that the analysis builds against whatever the runner ships and can fail on a language feature. Separate from ci.yml because CodeQL takes minutes where the build takes seconds, and a weekly cron belongs on its own workflow. Refs #32
TypeScriptClientInteropTest hard-coded Path.of(user.home, "IdeaProjects", "nap"). Combined with the assumeTrue guard on the toolchain, that is worse than a broken path: an assumption skips rather than fails, so on any machine without that exact directory the one test asserting cross-implementation agreement reported green while asserting nothing. It now reads -Dnap.typescript.dir, falling back to the old location so an existing developer checkout keeps working. OfficialTestVectorsTest already took -Dnap.test-vectors.dir; this brings the two into line. CI passes both explicitly and clones into the workspace rather than synthesising a home directory to satisfy a test constant. The precondition assertions added with the workflow stay, because a property can be wrong too, and the failure mode being guarded against is silence rather than error. Verified both directions: the default path still runs the test (1 run, 0 skipped), and an override pointing at a nonexistent directory skips rather than fails (1 run, 1 skipped), which is the assumeTrue behaviour that made the original hard-coding dangerous. The full CI invocation runs nap-it with 27 tests and 0 skipped. Refs #32
…ducible An earlier comment on this job recorded 12 CVEs in spring-core 6.2.19 and 2 in spring-security-core 6.5.11, and an issue was filed to triage them. Re-running the scanner does not reproduce any of it. dependency-check 13.0.0 against the full reactor reports BUILD SUCCESS at -DfailBuildOnCVSS=7. Running nap-spring alone at -DfailBuildOnCVSS=0, which fails on a finding of any severity, also passes. The generated report contains zero CVE identifiers. The clean result is not a hollow scan: the NVD database is 232 MB and was updated during the run, and the report lists spring-core, spring-security-core, spring-web and jackson-databind as analysed rather than skipped. The dependency versions in the original note were correct; only the finding count was not. The likely cause is the earlier run failing on "Invalid API Key, length of 0", since dependency-check rejects an empty NVD_API_KEY outright rather than ignoring it, and a run that cannot update the feed behaves differently from one that can. The job stays non-blocking, but now for a reason that does not depend on the count: it has never executed on a runner, and making an unproven scanner a merge gate on its first outing risks blocking every pull request on an environment problem rather than a real finding. The tracking issue is closed as not reproducible.
The precondition step checked for nip98.json alone. OfficialTestVectorsTest loads three vector files (payload-hash.json, nip98.json, flow.json) and calls assumeTrue on each independently, so a checkout carrying only the file the assert happened to name would skip the other two tests and still report green. That is the same silent-skip failure the step exists to prevent, one level down. Verified by extracting the step and running it against a synthetic checkout holding only nip98.json: the previous assert passed, the new one fails naming payload-hash.json. Against the real checkout it passes. Also confirmed while checking this that a wrong vector directory produces "Tests run: 3, Skipped: 3" with BUILD SUCCESS, which is precisely the green build that asserts nothing, and that the clone target resolves to a sibling of GITHUB_WORKSPACE rather than inside it, so actions/checkout will not clean it. Refs #32
Two gaps found while reviewing the changes rather than while writing them. The sweep runs on live traffic, so it races every writer, and it mutates four index maps that are views on one record. Getting that wrong does not throw: it drops a live session from one index while leaving it in another, and surfaces much later as a session that authenticates through the cookie and cannot be found by id. SweepConcurrencyTest contends 8 threads against a store seeded with dead records and asserts every live session is reachable through both getBySessionId and getByAccessToken, because the failure mode is the two disagreeing. Logout changed from revoking the cookie value directly to resolving it through getByAccessToken first, and that method filters revoked sessions. So the second logout finds nothing to revoke, and the endpoint has to stay idempotent anyway: a client clearing local state should never have to distinguish "logged out" from "was already logged out". The test drives logout twice and asserts 204 and a cleared cookie both times. Neither is a fix. Both pin behaviour the access-token change made newly load-bearing.
The repository had no entry for any of the audit fixes, and three of them change behaviour: the cookie switch ends every live session on deploy, the protected-path default turns an unannotated handler into a 500, and the ACL default fails startup. Shipping those unannounced would strand operators on symptoms with no explanation. Each is marked Breaking with the consequence stated plainly, and the retention bounds in the eviction entry are written out because they encode protocol behaviour (RFC 13.3 retry safety, replay detection) rather than implementation detail, so a future change that "simplifies" them to plain expiry would be a regression that tests alone might not explain.
Minor rather than patch because three changes alter behaviour, and one ends every live session on deploy. In order of what they cost an adopter: the cookie now carries the access token rather than the session id, so every signed-in user is logged out once; the auto-configuration no longer defaults to AllowAllAclResolver, so startup fails until a resolver is supplied or nap.allow-all-principals is set; and require-annotation-on-protected-paths defaults to true, so an unannotated handler under a protected prefix answers 500 rather than serving anyone. NapProperties is a record and gained a component, so its canonical constructor arity changed again, which is a compile break for anyone constructing it directly. Also corrects pre-existing README drift, which claimed 0.6.2 while the pom was already 0.8.0. A version in prose is a version that goes stale, and it had.
This was referenced Sep 24, 2026
No CI: nothing builds, tests, or scans on a pull request, including the TypeScript interop suite
#32
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. |
| private void setCookie(HttpServletResponse response, String sessionId) { | ||
| response.addCookie(sessionCookie(sessionId, properties.cookie().maxAgeSeconds())); | ||
| private void setCookie(HttpServletResponse response, SessionRecord session) { | ||
| response.addCookie(sessionCookie(session.accessToken(), properties.cookie().maxAgeSeconds())); |
added 2 commits
September 24, 2026 16:30
CodeQL on this branch's first CI run flagged a cookie set without Secure. It was right, and it is the defect this audit already fixed once in the TypeScript package (nap#34), reappearing where Java's type system hid it. httpOnly and secure were primitive boolean on the CookieProperties record, so Spring bound them as false whenever any sibling property was present. Setting only nap.cookie.name, the most ordinary reason to touch that section, produced a session cookie readable by script and sent in clear over http. Only the all-absent case reached the intended defaults, which is exactly why the existing tests agreed with the broken code: they constructed NapProperties by hand and took that branch. Reproduced before fixing, against the binder rather than the constructor: secure=false httpOnly=false from nap.cookie.name alone. Both are now boxed Boolean defaulting to TRUE, so absent is distinguishable from explicitly false, and an explicit false still wins for local http development. NapCookiePropertiesTest drives Binder for that reason. Mutation-checked by flipping the defaults to FALSE: 4 of 5 fail, the fifth being the explicit false case, which correctly does not depend on the default. Also in the same CodeQL run: - Log injection in NapSessionFilter. The request path is the one attacker-chosen field on that line. Not reachable through Tomcat, which leaves %0D%0A encoded through getRequestURI() (probed, not assumed), but that is the container's guarantee rather than this filter's. - Three java/user-controlled-bypass alerts on Nip98Validator, dismissed as false positives: every flagged branch returns failure, and the single success return sits after verifySignature. Nip98ValidatorStructureTest now enforces that, because a dismissed alert does not re-open itself if someone later adds an early success return. Mutation-checked by injecting exactly that shape: both cases fail. Dependency-Check: the previous comment claimed a missing NVD API key meant "slow rather than broken". The runner disproved it, failing in 52s. An empty data directory reproduces it locally whether the variable is empty or unset, and the legacy 1.1 feeds now answer 403, so a key is required rather than an optimisation. The job skips with a warning pointing at the secret instead of failing red on every PR, since a permanently red check teaches people to ignore the scanner. 206 tests, 0 failures, 0 skipped.
Three changes in this release alter runtime behaviour, and two can take an application down: startup fails without an AclResolver, and an unannotated handler under a protected prefix returns 500 where it used to be served. The changelog records what changed and why, but it is organised by finding rather than by what an operator has to do before deploying, and the order matters. Checking for a resolver bean costs a minute; discovering it at startup costs a rollback. Each claim checked against the source rather than written from memory: nap.allow-all-principals and @PublicEndpoint exist as named, and the fail-closed status is 500 rather than 403 (NapPermissionInterceptorFailClosedTest asserts it). The 500 is deliberate and the guide says why: an undeclared handler is a wiring mistake in the application, not a decision about the caller. Also covers the cross-repo case. nap 0.11.0 and nap-java 0.9.0 now agree on the wire, so a mixed fleet mid-rollout rejects the other side's cookies, which presents as users being logged out at random rather than once.
added 2 commits
September 24, 2026 16:44
The audit missed these, and so did Dependency-Check. bcprov 1.84 carries GHSA-9pwp-9qqc-pr26 (critical, a name-constraints bypass via a trailing dot in rfc822Name and URI) and GHSA-qp49-qgx5-5m26 (high, a lazy ASN.1 sequence resetting the nesting-depth guard). That is the provider behind Schnorr verification on the unauthenticated NIP-98 path. jackson-databind 2.21.4 carries two moderate @JSONVIEW deserialization bypasses, and Jackson parses attacker-controlled JSON in Nip98Validator and DefaultNapServer. Both arrive through nostr-java-core, so nothing in this repository names either version. Each pin is the lowest release carrying the fix, so it is a patch bump rather than a feature upgrade, and each can be dropped when imani-bom catches up. Verified by querying OSV against the resolved tree before and after: four advisories across two packages, then zero across nineteen. The full suite still passes, which matters here because bcprov is the crypto provider: OfficialTestVectorsTest and SignatureBindingTest exercise real Schnorr signatures through 1.85. The CI comment claiming this tree "scanned clean" was wrong, and wrong in the same way as the retracted CVE count in the closed issue #34: asserted from a local run rather than from a scan anyone could reproduce. Corrected in place rather than quietly deleted. The new osv-scan job is what found them. It gates merges, unlike Dependency-Check, on the grounds that a scanner which cannot run without a secret must not decide whether a PR merges. It scans the output of dependency:tree rather than the poms: osv-scanner reads pom.xml directly, but the sibling modules are in no registry, so resolution fails per module and it reports "0 packages affected by 0 known vulnerabilities". A green result that scanned nothing is worse than no scan, which is also why the step exits non-zero when it parses no packages. Both behaviours checked by running the extracted step against the old tree (exit 1, both packages named) and against an empty file (exit 1, vacuous pass refused).
The job failed on its first run, and it failed the way it was built to. -DoutputFile on dependency:tree is resolved per module rather than once for the reactor, so the run wrote a deptree.txt into each of the seven module directories and left the root one holding a single line: the aggregator's own coordinates. The step read only the root file, parsed nothing, and exited non-zero with "would pass vacuously" instead of reporting a green zero over an empty scan. My local check missed it because I had copied a complete tree into deptree.txt by hand before running the extracted step. That tested the parser against input the job would never produce. Re-tested by running mvn first and reading whatever it actually wrote. Globbing all seven files also widened coverage: 21 third-party packages rather than 19. The two the root-only read missed were spring-boot and spring-boot-autoconfigure, which are exactly the kind of package an SCA job exists to watch. Mutation-checked end to end by reverting the bcprov and jackson pins, re-running dependency:tree, and confirming the step exits 1 naming both packages and all five advisory IDs.
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 changes require.
Closes #27
Closes #28
Closes #29
Closes #30
Closes #31
Closes #32
Audit tracking issue: #33.
Upgrade steps for the three breaking changes are in UPGRADING.md.
What CI found on its first run
The headline: a critical and a high severity advisory in
bcprov-jdk18on1.84, the provider behind Schnorr verification on the unauthenticated NIP-98 path. GHSA-9pwp-9qqc-pr26 (name-constraints bypass via a trailing dot inrfc822Name/URI) and GHSA-qp49-qgx5-5m26 (a lazy ASN.1 sequence resetting the nesting-depth guard). Plus two moderate@JsonViewdeserialization bypasses injackson-databind2.21.4, and Jackson parses attacker-controlled JSON inNip98ValidatorandDefaultNapServer.Both arrive transitively through
nostr-java-core, so nothing in this repository names either version. Pinned to 1.85 and 2.21.5, the lowest releases carrying the fixes. OSV re-queried after: 4 advisories to 0 across 21 packages, full suite still green through real Schnorr test vectors.None of this was found by the audit, and none by Dependency-Check, which skips without an
NVD_API_KEYsecret and reports green over a scan that never ran. The new OSV job needs no key and gates merges. It found them, and it also refused to pass vacuously on its own first run, when-DoutputFileturned out to resolve per module and leave the root file nearly empty. Fixing that widened coverage from 19 packages to 21.This branch also carries the CI configuration, so this PR is the first time any of it has executed on a runner. It earned its keep immediately.
CodeQL found a real defect in the remediation itself.
CookieProperties.httpOnlyand.securewere primitiveboolean, so Spring bound them asfalsewhenever any sibling property was present. Setting onlynap.cookie.nameproduced a session cookie readable by script and sent in clear over http. Only the all-absent case reached the intended defaults, which is exactly why the existing tests agreed with the broken code: they constructNapPropertiesby hand and take that branch.It is the same defect as
nap(TypeScript) #34, which this audit already fixed once, reappearing in the language where the type system hid it. Both are now boxedBoolean, andNapCookiePropertiesTestdrives theBinderrather than the constructor for that reason.Three
java/user-controlled-bypassalerts were dismissed as false positives. Every flagged branch inNip98Validatorreturnsfailure, and the singlesuccessreturn sits afterverifySignature. A caller controlling the header picks which rejection they get, not whether verification runs.Nip98ValidatorStructureTestnow enforces that shape, because a dismissed alert does not re-open itself if someone later adds an earlysuccessreturn.One log-injection finding fixed in
NapSessionFilter, not reachable through Tomcat today (probed:%0D%0Astays encoded throughgetRequestURI()), but that is the container's guarantee rather than the filter's.Dependency-Check needs an
NVD_API_KEYsecret. The earlier note claiming a missing key meant "slow rather than broken" was wrong, and the runner disproved it in 52 seconds. Reproduced locally against an empty data directory; the legacy 1.1 feeds now answer 403, so a key is required rather than an optimisation. The job skips with a warning pointing at the secret instead of failing red on every PR, since a permanently red check teaches people to ignore the scanner. Add the secret (free from https://nvd.nist.gov/developers/request-an-api-key) and it becomes a real scan on the next run.Before you merge: three breaking changes
In order of what they cost an adopter.
getBySessionId()is provided, deliberately: accepting the old credential would keep the vulnerability alive for as long as the fallback existed.AclResolver. The auto-configuration no longer defaults toAllowAllAclResolver. Supply a resolver, or setnap.allow-all-principals=trueto ask for the old behaviour deliberately. The error names both.500.nap.require-annotation-on-protected-pathsdefaults totrue.@PublicEndpointis how a genuinely public handler says so.NapPropertiesis a record and gained a component, so its canonical constructor arity changed, which is a compile break for anyone constructing it directly.What was wrong
#27, the one that matters most. The cookie was set with
session.sessionId()and every read usedgetBySessionId(), making the session identifier the de facto bearer credential.getByAccessToken()existed onSessionStore, was implemented by both stores, and had no production caller at all.Three consequences: session ids are logged on every refresh path and on ACL denial, so anyone with log access held live credentials. Rotation never reached the credential, because
rotateRefreshToken()mints a new access token while the session id is unchanged, so the value a browser presents was never rotated and the reuse-detection design protected something the cookie path did not use. And the two implementations disagreed on the wire, whichnap-itcovers as a supported configuration.#28 removed a JVM-only fallback reading the NIP-98 proof from a
proofbody field. It had no RFC or TypeScript counterpart, asked thepayloadhash to cover a field containing the hash, and parsed attacker-controlled bytes a second time with a blanket catch.#29 also fixed a disagreement the fail-closed default made load-bearing: the filter matched the raw
getRequestURI()while the interceptor stripped the context path first, so under a non-empty context path the filter skipped authentication on requests the interceptor believed were covered.#31 bounded both in-memory stores. The retention bounds are deliberate rather than plain expiry: a challenge is kept until
resultCacheUntilbecause that window is what makes a client retry idempotent under RFC §13.3, and a session until its absolute cap orrefreshExpiresAtbecausegetByRefreshTokenanswers for revoked sessions so a replay stays detectable.The most valuable thing CI surfaced (#32)
Both interop tests called
assumeTrueon a hard-coded~/IdeaProjects/nap. An assumption skips rather than fails, so a naive CI job would have reported green while the one test asserting cross-implementation agreement asserted nothing.The workflow clones the reference implementation and asserts every vector file is present before running.
TypeScriptClientInteropTestnow takes-Dnap.typescript.dirrather than hard-coding a home-relative path. Verified both directions: the default runs the test (1 run, 0 skipped), a bogus override skips (1 run, 1 skipped), which is exactly the behaviour that made the hard-coding dangerous.How each fix was verified
Reverting
setCookiefails 3 of the #27 regression tests, includingrefresh_retiresThePreviousCookieValue— the one proving a rotation now invalidates the credential the browser was holding. Disabling the sweep fails the eviction tests while the retention tests keep passing, which is the pair that matters: a sweep dropping a cached redemption or a live refresh window is a different bug, not a fix.Two behaviours the review found the changes had made load-bearing, now pinned:
getByAccessToken()filters revoked sessions, so the second logout finds nothing to revoke. It must still return 204 with a cleared cookie.Verification
TypeScriptClientInteropTestagainst a modifiednap-corecheckout.NapAuthControllerTest67 to 77,NapSessionFilterTest17 to 19), no@Disabledintroduced.Two honest notes
CI has never executed on a runner. Everything was verified locally and by simulating the runner layout. The first real run is the proof, which is why Dependency-Check reports without blocking for now.
I filed #34 on a CVE count I had not reproduced and have since retracted it. Running the scanner myself reports
BUILD SUCCESSat-DfailBuildOnCVSS=7and at0, with a populated 232 MB NVD database and the Spring jars confirmed present as analysed. Closed as not reproducible; the CI comment citing it is corrected in this branch.Also corrects pre-existing README drift, which claimed
0.6.2while the pom was already0.8.0.Still open, deliberately
#35 (JDBC stores never delete) is not addressed here.
JdbcChallengeStoreandJdbcSessionStorecontain noDELETE, and migrations V1-V3 add no retention, sonap_sessionsholds access, refresh and previous-refresh tokens in plaintext indefinitely. It wants its own change with a migration, paired with tcheeric/nap#39.