AppSec audit — nap-java, v0.8.0: findings and remediation order

[tcheeric]

Scope

Full application-security review of nap-java at v0.8.0: all six modules (nap-core, nap-server, nap-jdbc, nap-client, nap-spring, nap-it), the protocol core, the Spring adapter and its filters, the JDBC stores, dependencies, and CI. Reviewed against the OWASP Top 10 and CWE Top 25, with the NAP v2 RFC as the specification and the TypeScript implementation as the interop reference.

Findings

#	Severity	Finding
#27	Medium–High	Session cookie carries the session id, not the access token
#28	Medium	Undocumented proof body-field fallback for the NIP-98 proof
#29	Medium	protected-path-prefixes does not fail closed
#30	Medium	Auto-configuration defaults fail open (AllowAllAclResolver, in-memory stores)
#31	Medium	In-memory stores never evict
#32	Medium	No CI at all

Three of these are cross-implementation divergences rather than bugs in isolation, which is what makes them worth treating as a group. nap-java must agree with nap on the wire, and #27 and #28 are places where it does not.

Suggested order

1. No CI: nothing builds, tests, or scans on a pull request, including the TypeScript interop suite #32 (CI) first, even though it is not the most severe. Nothing else here is verifiable without it, and the existing test suite — official test vectors, TypeScript interop, Postgres round trips — is already strong enough to catch regressions the moment it runs automatically.
2. Session cookie carries the session id, not the access token: rotation never reaches the credential and session ids are logged #27 — the behavioural fix with real consequences: rotation currently never reaches the credential the browser sends, and session ids are written to logs. Needs a migration note, since it logs existing sessions out once.
3. protected-path-prefixes does not fail closed: an unannotated handler under a protected prefix serves unauthenticated requests #29 + Auto-configuration defaults fail open: AllowAllAclResolver and in-memory stores are the silent default #30 together — both are one-line default changes plus a CHANGELOG entry, and they address the same theme of failing open.
4. Undocumented fallback accepts the NIP-98 proof from a proof body field, outside the signed payload hash #28 — deleting the fallback is small; the only real question is whether any client depends on it.
5. In-memory challenge and session stores never evict: unbounded growth driven by unauthenticated /auth/init #31 — most design work, lowest exploitability.

What the review did not find

• No injection. Every JdbcSessionStore / JdbcChallengeStore query uses PreparedStatement with bound parameters. findBy(column, value) interpolates a column name, but only from three private call sites passing compile-time constants — not attacker-reachable. Worth an enum for defence in depth, not a finding.
• The NIP-98 validator is correct, and better than it had to be. verifySignature() recomputes the event id from the canonical serialization before verifying, with a comment explaining precisely why trusting the supplied id would let any note the victim ever published be re-dressed as a completion. That is the subtle bug in this protocol and it is handled.
• Constant-time comparison throughout. MessageDigest.isEqual for refresh tokens and step-up tokens, including across length differences.
• Timing side channels handled. padAuthResponse() in a finally so a store outage answers on the same schedule as a refusal; 429s deliberately unpadded.
• ClientIpResolver is exemplary. The javadoc states the problem (a proxy collapsing every caller into one rate-limit bucket), why the naive fix is worse (client-settable X-Forwarded-For removes the limit entirely), and the right-to-left walk is implemented correctly.
• EventReplayGuard is bounded, sized to the clock-skew allowance, sweeps without owning a thread, and the unbounded variant is deprecated with an accurate explanation. In-memory challenge and session stores never evict: unbounded growth driven by unauthenticated /auth/init #31 is the same treatment not yet applied to the stores.
• NapSessionFilter's ACL cache is keyed by principal (not session), bounded, and caches only grants — the reasoning for not caching denials is correct.
• No secrets committed.

The protocol implementation is careful and the comments explain the reasoning rather than the mechanics, which is what made this productive to audit. Most findings are about defaults and configuration surface rather than the core: the code does the right thing when wired correctly, and does not always insist on being wired correctly.

[tcheeric]

Verification record

Re-checked every claim in this audit after filing. nap-java has no CI and I did not stand up a Spring context, so these were verified against source and call-graph rather than by running the app — stated plainly so the evidence is not overread.

#27 — the central claim is confirmed mechanically, not by inspection.

• NapAuthController sets the cookie with s.session().sessionId() at both call sites (lines 144 and 183 — completion and refresh).
• NapSessionFilter looks up via sessionStore.getBySessionId(sessionId) (line 98).
• getByAccessToken has zero production callers. Grepping the tree excluding /target/, /test/, the SessionStore interface, and the two store implementations returns nothing. So the access token is minted, persisted, rotated, and returned in the JSON body while authenticating nothing — which is the finding, now established by call-graph rather than argued.

That line-183 detail sharpens the issue: refresh re-sets the cookie to the same session id, so rotation genuinely never reaches the value the browser sends.

#29 — both halves confirmed.

• All three pre-ACL branches (sessionId == null, session.isEmpty(), isExpired) call filterChain.doFilter(request, response) and return. Enforcement really does depend entirely on the interceptor finding an annotation.
• The context-path asymmetry is real: getContextPath() appears only in NapPermissionInterceptor (line 155) and nowhere in NapSessionFilter. The filter matches the raw URI, the interceptor matches the stripped path, so under a non-empty context path the two disagree about what is protected. The third regression test in that issue is the one that pins this.
• The README claim checked out: line 60 shows require-annotation-on-protected-paths: false in the example config, so the documented starting point is the fail-open one.

#32 — .github does not exist (ls: cannot access '.github': No such file or directory), and no workflow YAML exists anywhere outside target/. Confirmed.

#30, #31 — claims are structural (default beans, absence of eviction) and were verified by reading the relevant constructors and maps. No execution was involved and none is claimed.

Correction applied: #30 originally cited tcheeric/nap-java#5 for the store-eviction work. That number is a merged pull request about RFC gaps, not the store issue. It now reads #31, with tcheeric/nap#35 named as the TypeScript counterpart. All cross-references in this repository were then resolved against the API and confirmed to point at the intended title and state.

Recommended first step remains #32. Of the six findings here, four are verifiable only by reading, because there is no pipeline to run them against. Standing up mvn verify on PR converts the existing suite — official test vectors, TypeScript interop, Postgres round trips — from a thing someone remembers to run into a thing that holds. It is also what would have caught #27 and #28 at the commit that introduced them.

No finding changed severity as a result of this pass.

[tcheeric]

Coverage follow-up

A confidence check on the TypeScript half of this audit surfaced a clock-consistency divergence (tcheeric/nap#38): /auth/session there builds its guard options without the injected Clock, so a session live on that clock is rejected. Checking the equivalent paths here found the same shape, more broadly.

NapServerOptions carries a Clock, and DefaultNapServer uses it correctly throughout — options.clock().instant().getEpochSecond() on every decision in issueChallenge, verifyCompletion, and refreshSession. The Spring layer does not inherit it:

• NapAuthController.checkSession() — Instant.now().getEpochSecond() for the expiry comparison and the slide
• NapAuthController.logout() — Instant.now() for the revocation stamp
• NapSessionFilter.isExpired() and resolveAcl() — Instant.now() for expiry and ACL cache validity

So a deployment with an injected Clock has a server that decides sessions on one clock and a filter, controller, and ACL cache that decide on another. The consequences match what NapExpressGuardOptions.clock documents on the TypeScript side: a login that succeeds followed by a request the guard refuses, and — specific to logout() — a revoked_at stamp written in wall-clock time into a store the server reads on the injected clock, which is a revocation dated in the future or the past.

Severity: Low, and deliberately not filed as its own issue. Unlike tcheeric/nap#38 this is not a regression against an intent already expressed in the code — the Spring layer never claimed to honour an injected clock, and no Clock is threaded to it. It is a testability limitation rather than a defect: it means session expiry, the sliding window, and ACL cache behaviour here cannot be tested under a pinned clock, which is why #31's suggested regression tests require a MutableClock the current wiring cannot supply.

Worth folding into whichever of #27 or #31 is picked up first, since both touch these classes anyway: inject the Clock from NapProperties/NapServerOptions into NapAuthController and NapSessionFilter, defaulting to Clock.systemUTC().

Also confirmed in this pass: every issue body in this repository has balanced code fences and a complete closing paragraph, so nothing was truncated at filing. The six findings and their severities are unchanged.

[tcheeric]

Remediation complete: all 6 findings fixed, plus one correction

Ten commits on master, not pushed, issues left open for your review.

#	Commit	Verified by
#27	23ac500	Reverted setCookie and re-ran: 3 regression tests fail against the old code
#28	4fd303a	Inverted the test that asserted the fallback; a valid-looking body proof now yields a null authorization and a 401
#29	f3f9d48	Context-path test asserts an authentication is present, which only happens if the filter treated the request as protected
#30	569df2b	3 tests: refuses with no opt-in, refuses on explicit false, builds allow-all when asked
#31	d252fdd	Mutation: disabling the sweep fails the eviction tests while the retention tests keep passing
#32	f88bf0a, f65e79e, c0c37c8, 7f32eb9	CI from zero, plus the interop trap below

Suite: 197 tests, 0 failures, 0 skipped, including TypeScriptClientInteropTest. Tree clean.

A correction I owe you

I filed #34 on a CVE count I had not reproduced, and it was wrong. I verified the dependency versions independently, then treated the finding count as established because the versions checked out. Those are different claims.

Running the scanner myself: BUILD SUCCESS at -DfailBuildOnCVSS=7, and also at -DfailBuildOnCVSS=0, which fails on a finding of any severity. Zero CVE identifiers in the generated report. I ruled out 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 likely cause of the original number is a run that failed on Invalid API Key, length of 0, since dependency-check rejects an empty NVD_API_KEY outright rather than ignoring it.

#34 is closed as not reproducible and the CI comment citing it is corrected in 7f32eb9. Dependency-Check stays non-blocking, but now for a reason that survives the correction: it has never run on a runner, and making an unproven scanner a merge gate on its first outing blocks PRs on environment problems rather than findings.

The most valuable thing CI surfaced

Both interop tests called assumeTrue on a hard-coded ~/IdeaProjects/nap. 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. c0c37c8 gives it -Dnap.typescript.dir, CI passes both directories explicitly, and the workflow asserts the toolchain is present so the failure mode is loud rather than silent.

Verified both directions: default path 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.

New finding

#35 — JDBC stores never delete. JdbcChallengeStore and JdbcSessionStore contain no DELETE, and migrations V1-V3 add no retention. Beyond table growth, nap_sessions holds access_token, refresh_token and previous_refresh_token in plaintext indefinitely. Paired with tcheeric/nap#39 so the retention bounds cannot drift between implementations.

Behaviour changes needing release notes

• Session cookie carries the session id, not the access token: rotation never reaches the credential and session ids are logged #27 logs every live session out once. No getBySessionId() fallback, deliberately.
• protected-path-prefixes does not fail closed: an unannotated handler under a protected prefix serves unauthenticated requests #29 turns an unannotated handler under a protected prefix into a 500.
• Auto-configuration defaults fail open: AllowAllAclResolver and in-memory stores are the silent default #30 fails startup without an AclResolver unless nap.allow-all-principals=true.

[tcheeric]

All six findings are now in #36, released as 0.9.0

#	Sev	State
#27	Med-High	Fixed. Reverting setCookie fails 3 regression tests
#28	Med	Body-proof fallback removed, the asserting test inverted
#29	Med	Fails closed, plus the filter/interceptor path disagreement
#30	Med	No implicit allow-all; startup names the property
#31	Med	Both stores evict, retention bounds preserved
#32	Med	CI from zero, including the silent-skip trap it surfaced

0.9.0 rather than a patch, and three of these change behaviour. In order of cost: the cookie switch logs every user out once, the ACL default fails startup, and the protected-path default turns an unannotated handler into a 500. All three are marked Breaking in the changelog with the consequence stated.

#35 stays open deliberately — the JDBC stores have no DELETE path, so expired credentials persist indefinitely. Paired with tcheeric/nap#39.

#34 was closed as not reproducible. I filed it on a CVE count I had not verified, having checked only the dependency versions. Running the scanner reports clean at CVSS >= 7 and at 0. The CI comment citing those 14 findings is corrected in this branch.

Two things the review added that were not in the original findings: logout idempotency (getByAccessToken filters revoked sessions, so the second call finds nothing) and sweep-under-contention (8 threads, both indexes consistent). Neither is a fix; both pin behaviour the access-token change made newly load-bearing.

[tcheeric]

CI first run: one real defect, one retraction

The CI configuration ships in PR #36, so that PR is the first time any of it has run on a runner. Results worth recording here.

CodeQL found a defect the audit missed, in remediation code this audit wrote. CookieProperties.httpOnly and .secure were primitive boolean, so Spring bound them false whenever any sibling property was present: nap.cookie.name alone produced a session cookie readable by script and sent in clear over http. The existing tests agreed with the broken code because they construct NapProperties by hand, which takes the all-absent branch where the defaults are correct.

This is the same defect as nap (TypeScript) #34, which the audit did find, in the language where the type system hid it. The audit read the Java record's declared defaults and they looked right; it never exercised the binder, which is the only place the difference appears. Fixed with boxed Boolean plus a Binder-driven test, mutation-checked.

Three java/user-controlled-bypass alerts dismissed as false positives, with Nip98ValidatorStructureTest added to keep the dismissal honest: a later early success return would make the original alerts correct again and silently inherit the dismissal.

A second correction to the Dependency-Check note. After retracting the CVE count in #34, the remaining claim was that a missing NVD_API_KEY meant "slow rather than broken". The runner disproved that too, failing in 52s. Reproduced locally against an empty data directory, and the legacy 1.1 JSON feeds now answer 403, so an API key is required rather than an optimisation. The job now skips with a warning naming the secret instead of failing red on every PR.

Current state on #36: Build and test, CodeQL, and Dependency-Check all green.

[tcheeric]

Shipped: 0.9.0 is on master

#36 merged (07483ea), closing all six findings automatically.

Issue	Severity	State
#27 session cookie carried the session id, not the access token	High	Closed
#28 undocumented proof body fallback	Med	Closed
#29 protected-path-prefixes did not fail closed	High	Closed
#30 AllowAllAclResolver silent default	Med	Closed
#31 in-memory stores never evict	Med	Closed
#32 no CI at all	Med	Closed

master verified after the merge: 206 tests, 0 failures, 0 skipped, including TypeScriptClientInteropTest against the modified TS source.

What the audit itself missed

Worth recording, because it is the part that did not come from reading code. CI ran for the first time on that PR and found two things the audit did not:

• A cookie defect in the remediation itself. httpOnly and secure were primitive boolean on a Spring @ConfigurationProperties record, so the binder set them false whenever any sibling property was present. The same defect as nap Triage 14 Dependency-Check findings in spring-core and spring-security-core, then make the CVSS gate blocking #34, in the language where the type system hid it.
• A critical and a high in bcprov 1.84, the provider behind Schnorr verification on the unauthenticated NIP-98 path, arriving transitively through nostr-java-core. Dependency-Check had reported green over a scan that never ran, because it skips without an NVD_API_KEY. The OSV job added alongside it needs no key and gates merges.

Still open, deliberately: #35 (JDBC stores retain expired credentials) and #37 (a local mvn install can overwrite a released artifact in ~/.m2, which is how tcheeric/nap#41 happened).

Closing this tracker.