From 23ac5001c3406c0f9fefd1fcf7f0d799d50f2e89 Mon Sep 17 00:00:00 2001 From: tcheeric Date: Thu, 24 Sep 2026 14:58:55 +0100 Subject: [PATCH 01/18] fix(spring): carry the access token in the session cookie, not the session 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 --- .../spring/controller/NapAuthController.java | 34 +++-- .../nap/spring/filter/NapSessionFilter.java | 12 +- .../controller/NapAuthControllerTest.java | 140 +++++++++++++++++- .../spring/filter/NapSessionFilterTest.java | 29 ++-- 4 files changed, 180 insertions(+), 35 deletions(-) diff --git a/nap-spring/src/main/java/xyz/tcheeric/nap/spring/controller/NapAuthController.java b/nap-spring/src/main/java/xyz/tcheeric/nap/spring/controller/NapAuthController.java index 743d025..19640e3 100644 --- a/nap-spring/src/main/java/xyz/tcheeric/nap/spring/controller/NapAuthController.java +++ b/nap-spring/src/main/java/xyz/tcheeric/nap/spring/controller/NapAuthController.java @@ -141,7 +141,7 @@ public ResponseEntity complete(HttpServletRequest request, HttpServletRespons return switch (outcome) { case VerifyCompletionOutcome.Success s -> { - setCookie(response, s.session().sessionId()); + setCookie(response, s.session()); var successResponse = napServer.toPublicAuthSuccess(s.session()); yield ResponseEntity.ok(successResponse); } @@ -177,10 +177,11 @@ public ResponseEntity refresh(HttpServletRequest request, HttpServletResponse return switch (outcome) { case RefreshSessionOutcome.Success s -> { - // The session id is unchanged by a rotation, but re-setting the cookie renews - // its Max-Age — otherwise a session that keeps refreshing still loses its - // cookie at the original absolute cap. - setCookie(response, s.session().sessionId()); + // Re-set because rotation mints a new access token: the cookie carries that + // token, so a rotation the browser never learns about would leave it holding + // the retired one. Renewing Max-Age at the same time is what stops a session + // that keeps refreshing from losing its cookie at the original absolute cap. + setCookie(response, s.session()); yield ResponseEntity.ok(napServer.toPublicAuthSuccess(s.session())); } case RefreshSessionOutcome.Failure f when f.code() == NapErrorCode.NAP_REFRESH_RATE_LIMITED -> @@ -227,12 +228,12 @@ private static ResponseEntity rateLimited(Integer retryAfterSeconds) { */ @GetMapping("/session") public ResponseEntity checkSession(HttpServletRequest request) { - String sessionId = extractCookie(request); - if (sessionId == null) { + String accessToken = extractCookie(request); + if (accessToken == null) { return sessionEnded("invalid"); } - SessionRecord record = sessionStore.getBySessionId(sessionId).orElse(null); + SessionRecord record = sessionStore.getByAccessToken(accessToken).orElse(null); if (record == null) { return sessionEnded("invalid"); } @@ -274,10 +275,15 @@ public ResponseEntity checkSession(HttpServletRequest request) { @PostMapping("/logout") public ResponseEntity logout(HttpServletRequest request, HttpServletResponse response) { - String sessionId = extractCookie(request); - if (sessionId != null) { - sessionStore.revokeBySessionId(sessionId, Instant.now().getEpochSecond()); - log.info("nap_logout"); + String accessToken = extractCookie(request); + if (accessToken != null) { + // Resolved to a session first: the cookie carries the access token, and revocation + // is keyed by session id. A revoke taking the cookie value directly would silently + // match nothing and return 204 without ending the session. + sessionStore.getByAccessToken(accessToken).ifPresent(record -> { + sessionStore.revokeBySessionId(record.sessionId(), Instant.now().getEpochSecond()); + log.info("nap_logout"); + }); } clearCookie(response); return ResponseEntity.noContent().build(); @@ -288,8 +294,8 @@ private ResponseEntity> sessionEnded(String reason) { .body(Map.of("error", "session_ended", "reason", reason)); } - 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())); } private void clearCookie(HttpServletResponse response) { diff --git a/nap-spring/src/main/java/xyz/tcheeric/nap/spring/filter/NapSessionFilter.java b/nap-spring/src/main/java/xyz/tcheeric/nap/spring/filter/NapSessionFilter.java index 3931417..7790025 100644 --- a/nap-spring/src/main/java/xyz/tcheeric/nap/spring/filter/NapSessionFilter.java +++ b/nap-spring/src/main/java/xyz/tcheeric/nap/spring/filter/NapSessionFilter.java @@ -89,13 +89,19 @@ protected void doFilterInternal(HttpServletRequest request, HttpServletResponse return; } - String sessionId = extractCookie(request); - if (sessionId == null) { + // The cookie carries the access token, not the session id. The two are not + // interchangeable: the session id is an identifier that appears in logs and on + // challenge rows, while the access token is the credential rotation replaces on + // every refresh. Authenticating on the identifier would mean the value a browser + // presents is never rotated, and that every log line naming a session is a live + // credential. + String accessToken = extractCookie(request); + if (accessToken == null) { filterChain.doFilter(request, response); return; } - var session = sessionStore.getBySessionId(sessionId); + var session = sessionStore.getByAccessToken(accessToken); if (session.isEmpty()) { filterChain.doFilter(request, response); diff --git a/nap-spring/src/test/java/xyz/tcheeric/nap/spring/controller/NapAuthControllerTest.java b/nap-spring/src/test/java/xyz/tcheeric/nap/spring/controller/NapAuthControllerTest.java index 13262ca..ca24b82 100644 --- a/nap-spring/src/test/java/xyz/tcheeric/nap/spring/controller/NapAuthControllerTest.java +++ b/nap-spring/src/test/java/xyz/tcheeric/nap/spring/controller/NapAuthControllerTest.java @@ -212,7 +212,7 @@ void checkSession_expiredSession_returns401WithReasonExpired() { sessionStore.createForChallenge(expired); MockHttpServletRequest request = new MockHttpServletRequest("GET", "/api/v1/auth/session"); - request.setCookies(new Cookie("merchant_session", "sid-expired")); + request.setCookies(new Cookie("merchant_session", "token")); ResponseEntity response = controller().checkSession(request); @@ -234,7 +234,7 @@ void checkSession_returnsCrossImplementationShapeWithoutLeakingTheToken() { sessionStore.createForChallenge(active); MockHttpServletRequest request = new MockHttpServletRequest("GET", "/api/v1/auth/session"); - request.setCookies(new Cookie("merchant_session", "sid-shape")); + request.setCookies(new Cookie("merchant_session", "token-secret")); ResponseEntity response = controller().checkSession(request); @@ -278,7 +278,7 @@ void checkSession_validSession_slidesIdleWindowAndReturnsPubkey() { sessionStore.createForChallenge(active); MockHttpServletRequest request = new MockHttpServletRequest("GET", "/api/v1/auth/session"); - request.setCookies(new Cookie("merchant_session", "sid-active")); + request.setCookies(new Cookie("merchant_session", "token-a")); ResponseEntity response = controller().checkSession(request); @@ -314,7 +314,7 @@ void checkSession_slide_isCappedByAbsoluteExpiry() { sessionStore.createForChallenge(narrow); MockHttpServletRequest request = new MockHttpServletRequest("GET", "/api/v1/auth/session"); - request.setCookies(new Cookie("merchant_session", "sid-narrow")); + request.setCookies(new Cookie("merchant_session", "token-n")); ResponseEntity response = controller().checkSession(request); @@ -377,7 +377,7 @@ void refresh_success_renewsTheCookie() { assertThat(result.getStatusCode().value()).isEqualTo(200); Cookie cookie = response.getCookie("merchant_session"); assertThat(cookie).isNotNull(); - assertThat(cookie.getValue()).isEqualTo("sid-rotated"); + assertThat(cookie.getValue()).isEqualTo("access-2"); assertThat(cookie.getMaxAge()).isEqualTo(43200); var captor = forClass(RefreshSessionInput.class); @@ -401,7 +401,7 @@ void logout_clearsCookieAndRevokesSession() { sessionStore.createForChallenge(live); MockHttpServletRequest request = new MockHttpServletRequest("POST", "/api/v1/auth/logout"); - request.setCookies(new Cookie("merchant_session", "sid-live")); + request.setCookies(new Cookie("merchant_session", "token-l")); MockHttpServletResponse response = new MockHttpServletResponse(); ResponseEntity result = controller().logout(request, response); @@ -410,10 +410,136 @@ void logout_clearsCookieAndRevokesSession() { Cookie cookie = response.getCookie("merchant_session"); assertThat(cookie).isNotNull(); assertThat(cookie.getMaxAge()).isEqualTo(0); - // Session is revoked in the store — subsequent getBySessionId filters it out. + // Revoked in the store, so the session is gone by its own id as well as by the + // access token the cookie carried. assertThat(sessionStore.getBySessionId("sid-live")).isEmpty(); } + // ----------------------------------------------------------------- + // The cookie carries the access token, never the session id (#27) + // ----------------------------------------------------------------- + + /** + * The session id and the access token are not interchangeable. The id is an identifier: + * it is logged on the refresh paths, logged on ACL denial, and persisted on the challenge + * row. The access token is the credential, and it is what rotation replaces. Putting the + * id in the cookie made every log line naming a session a live credential, and meant the + * value a browser presents was never rotated. + */ + @Test + void complete_cookieCarriesTheAccessTokenNotTheSessionId() { + MockHttpServletRequest request = new MockHttpServletRequest("POST", "/api/v1/auth/complete"); + request.setAttribute(NapServletFilter.RAW_BODY_ATTRIBUTE, "{\"challenge_id\":\"c\"}".getBytes()); + request.addHeader("Authorization", "Nostr proof"); + MockHttpServletResponse response = new MockHttpServletResponse(); + + long now = 1_700_000_000L; + SessionRecord session = SessionRecord.create( + "sid-secret", "c", "access-token-secret", + "npub1test", "a".repeat(64), + List.of(), List.of(), + now, now, now + 900, now + 43200 + ); + when(napServer.verifyCompletion(any())).thenReturn(VerifyCompletionOutcome.success(session)); + when(napServer.toPublicAuthSuccess(session)).thenReturn(new AuthSuccessResponse( + "ok", session.accessToken(), "Bearer", + session.expiresAt(), session.absoluteExpiryAt(), + new AuthSuccessResponse.Principal(session.principalNpub(), session.principalPubkey()), + session.roles(), session.permissions() + )); + + controller().complete(request, response); + + Cookie cookie = response.getCookie("merchant_session"); + assertThat(cookie).isNotNull(); + assertThat(cookie.getValue()).isEqualTo("access-token-secret"); + assertThat(cookie.getValue()).isNotEqualTo("sid-secret"); + } + + /** A cookie carrying the session id must no longer authenticate anything. */ + @Test + void checkSession_rejectsACookieCarryingTheSessionId() { + long now = Instant.now().getEpochSecond(); + SessionRecord live = SessionRecord.create( + "sid-rejected", "chal", "access-token-rejected", + "npub", "a".repeat(64), + List.of(), List.of(), + now - 60, now - 60, now + 900, now + 43200 + ); + sessionStore.createForChallenge(live); + + MockHttpServletRequest request = new MockHttpServletRequest("GET", "/api/v1/auth/session"); + request.setCookies(new Cookie("merchant_session", "sid-rejected")); + + ResponseEntity response = controller().checkSession(request); + + assertThat(response.getStatusCode().value()).isEqualTo(401); + } + + /** + * Rotation mints a new access token, and the cookie carries it, so the credential the + * browser held before the refresh stops working. That is the property the rotating-token + * design exists for and the one the session-id cookie silently removed. + */ + @Test + void refresh_retiresThePreviousCookieValue() { + long now = Instant.now().getEpochSecond(); + SessionRecord before = SessionRecord.create( + "sid-rot", "chal-r", "access-before", + "npub-r", "f".repeat(64), + List.of(), List.of(), + now - 60, now - 60, now + 900, now + 43200 + ); + sessionStore.createForChallenge(before); + + SessionRecord after = SessionRecord.create( + "sid-rot", "chal-r", "access-after", + "npub-r", "f".repeat(64), + List.of(), List.of(), + now - 60, now, now + 900, now + 43200 + ); + when(napServer.refreshSession(any())).thenReturn(new RefreshSessionOutcome.Success(after)); + when(napServer.toPublicAuthSuccess(after)).thenReturn(new AuthSuccessResponse( + "ok", after.accessToken(), "Bearer", + after.expiresAt(), after.absoluteExpiryAt(), + new AuthSuccessResponse.Principal(after.principalNpub(), after.principalPubkey()), + after.roles(), after.permissions() + )); + + MockHttpServletRequest request = new MockHttpServletRequest("POST", "/api/v1/auth/refresh"); + request.addHeader("Authorization", "Bearer refresh-1"); + MockHttpServletResponse response = new MockHttpServletResponse(); + + controller().refresh(request, response); + + Cookie cookie = response.getCookie("merchant_session"); + assertThat(cookie).isNotNull(); + assertThat(cookie.getValue()).isEqualTo("access-after"); + assertThat(cookie.getValue()).isNotEqualTo("access-before"); + } + + /** Logout resolves the cookie to a session before revoking, so a 204 really did revoke. */ + @Test + void logout_revokesTheSessionTheAccessTokenNames() { + long now = Instant.now().getEpochSecond(); + SessionRecord live = SessionRecord.create( + "sid-bye", "chal-b", "access-bye", + "npub-b", "a".repeat(64), + List.of(), List.of(), + now - 60, now - 60, now + 900, now + 43200 + ); + sessionStore.createForChallenge(live); + + MockHttpServletRequest request = new MockHttpServletRequest("POST", "/api/v1/auth/logout"); + request.setCookies(new Cookie("merchant_session", "access-bye")); + MockHttpServletResponse response = new MockHttpServletResponse(); + + ResponseEntity result = controller().logout(request, response); + + assertThat(result.getStatusCode().value()).isEqualTo(204); + assertThat(sessionStore.getBySessionId("sid-bye")).isEmpty(); + } + /** * A browser matches a deletion against name + domain + path, and drops a Set-Cookie whose * SameSite it disagrees with. A clear that omits either attribute leaves the cookie in the diff --git a/nap-spring/src/test/java/xyz/tcheeric/nap/spring/filter/NapSessionFilterTest.java b/nap-spring/src/test/java/xyz/tcheeric/nap/spring/filter/NapSessionFilterTest.java index d939f2b..1c45a3b 100644 --- a/nap-spring/src/test/java/xyz/tcheeric/nap/spring/filter/NapSessionFilterTest.java +++ b/nap-spring/src/test/java/xyz/tcheeric/nap/spring/filter/NapSessionFilterTest.java @@ -43,7 +43,7 @@ void tearDown() { @Test void doFilterInternal_deniesSuspendedSessions() throws Exception { SessionRecord session = sessionRecord(); - when(sessionStore.getBySessionId("session-123")).thenReturn(Optional.of(session)); + when(sessionStore.getByAccessToken("access-token-123")).thenReturn(Optional.of(session)); when(aclResolver.resolve(session.principalNpub(), session.principalPubkey())) .thenReturn(AclDecision.denied("suspended", true)); @@ -62,7 +62,7 @@ void doFilterInternal_deniesWithoutRevokingWhenTheDenialIsNotAffirmative() throw // replica, a row mid-rewrite — blocks this request and no more. Revoking would cost // the user a fresh NIP-98 login for someone else's transient failure. SessionRecord session = sessionRecord(); - when(sessionStore.getBySessionId("session-123")).thenReturn(Optional.of(session)); + when(sessionStore.getByAccessToken("access-token-123")).thenReturn(Optional.of(session)); when(aclResolver.resolve(session.principalNpub(), session.principalPubkey())) .thenReturn(AclDecision.denied("acl_unavailable")); @@ -91,7 +91,7 @@ private MockHttpServletResponse denyAndCapture() throws Exception { @Test void doFilterInternal_cachesAclRefreshesForTheConfiguredInterval() throws Exception { SessionRecord session = sessionRecord(); - when(sessionStore.getBySessionId("session-123")).thenReturn(Optional.of(session)); + when(sessionStore.getByAccessToken("access-token-123")).thenReturn(Optional.of(session)); when(aclResolver.resolve(session.principalNpub(), session.principalPubkey())) .thenReturn(AclDecision.allowed(List.of("admin"), List.of("admin", "read"))); @@ -167,7 +167,7 @@ void doFilterInternal_expiredSession_revokesAndPassesThrough() throws Exception List.of("merchant"), List.of("read"), now - 7200, now - 3600 // expired 1 hour ago ); - when(sessionStore.getBySessionId("session-123")).thenReturn(Optional.of(expired)); + when(sessionStore.getByAccessToken("access-token-123")).thenReturn(Optional.of(expired)); NapSessionFilter filter = new NapSessionFilter( sessionStore, aclResolver, "merchant_session", @@ -189,7 +189,7 @@ void doFilterInternal_expiredSession_revokesAndPassesThrough() throws Exception @Test void doFilterInternal_sessionNotFound_passesThrough() throws Exception { // Arrange - when(sessionStore.getBySessionId("session-123")).thenReturn(Optional.empty()); + when(sessionStore.getByAccessToken("access-token-123")).thenReturn(Optional.empty()); NapSessionFilter filter = new NapSessionFilter( sessionStore, aclResolver, "merchant_session", @@ -210,7 +210,7 @@ void doFilterInternal_sessionNotFound_passesThrough() throws Exception { private MockHttpServletRequest request() { MockHttpServletRequest request = new MockHttpServletRequest("POST", "/internal/v1/merchants/test/suspend"); - request.setCookies(new Cookie("merchant_session", "session-123")); + request.setCookies(new Cookie("merchant_session", "access-token-123")); return request; } @@ -227,10 +227,12 @@ void doFilterInternal_cachesOneDecisionPerPrincipalNotPerSession() throws Except for (int i = 0; i < 50; i++) { String sessionId = "session-" + i; - when(sessionStore.getBySessionId(sessionId)).thenReturn(Optional.of(sessionRecord(sessionId))); + String accessToken = "access-token-" + i; + when(sessionStore.getByAccessToken(accessToken)) + .thenReturn(Optional.of(sessionRecord(sessionId, accessToken))); MockHttpServletRequest request = new MockHttpServletRequest("POST", "/internal/v1/merchants/test/suspend"); - request.setCookies(new Cookie("merchant_session", sessionId)); + request.setCookies(new Cookie("merchant_session", accessToken)); filter.doFilterInternal(request, new MockHttpServletResponse(), (req, res) -> { }); } @@ -244,7 +246,7 @@ void doFilterInternal_doesNotCacheADenialTheResolverIsUnsureOf() throws Exceptio // fault. Caching it would lock the principal out for a whole refresh interval, and // because the cache is now per-principal that would take every session down with it. SessionRecord session = sessionRecord(); - when(sessionStore.getBySessionId("session-123")).thenReturn(Optional.of(session)); + when(sessionStore.getByAccessToken("access-token-123")).thenReturn(Optional.of(session)); when(aclResolver.resolve(session.principalNpub(), session.principalPubkey())) .thenReturn(AclDecision.denied("acl_unavailable")) .thenReturn(AclDecision.allowed(List.of("merchant"), List.of("read"))); @@ -266,12 +268,17 @@ void doFilterInternal_doesNotCacheADenialTheResolverIsUnsureOf() throws Exceptio verify(aclResolver, times(2)).resolve(session.principalNpub(), session.principalPubkey()); } - private SessionRecord sessionRecord(String sessionId) { + /** + * A session of the shared principal, with its own id and its own access token. Distinct + * tokens are what make the per-principal cache assertion meaningful: N sessions now look + * up N different credentials and must still collapse to one cache entry. + */ + private SessionRecord sessionRecord(String sessionId, String accessToken) { long now = java.time.Instant.now().getEpochSecond(); return SessionRecord.create( sessionId, "challenge-123", - "access-token-123", + accessToken, "npub1test", "a".repeat(64), List.of("merchant"), From 4fd303acac0d5f876c532badd36ddbdb70297545 Mon Sep 17 00:00:00 2001 From: tcheeric Date: Thu, 24 Sep 2026 15:01:56 +0100 Subject: [PATCH 02/18] fix(spring): drop the undocumented body-field fallback for the NIP-98 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 `, 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 --- .../spring/controller/NapAuthController.java | 56 +++++++++++++------ .../controller/NapAuthControllerTest.java | 42 +++++++------- 2 files changed, 57 insertions(+), 41 deletions(-) diff --git a/nap-spring/src/main/java/xyz/tcheeric/nap/spring/controller/NapAuthController.java b/nap-spring/src/main/java/xyz/tcheeric/nap/spring/controller/NapAuthController.java index 19640e3..fe70ba1 100644 --- a/nap-spring/src/main/java/xyz/tcheeric/nap/spring/controller/NapAuthController.java +++ b/nap-spring/src/main/java/xyz/tcheeric/nap/spring/controller/NapAuthController.java @@ -42,6 +42,17 @@ public class NapAuthController { private final NapServer napServer; private final SessionStore sessionStore; private final NapProperties properties; + /** + * Retained for constructor compatibility only, and deliberately unused. + * + *

Its one reader was the body-proof fallback removed in #28. The constructors stay as they + * are because they are public API and auto-configuration passes the application's mapper; + * dropping the parameter would break every hand-wired controller for no gain. Nothing in this + * class should acquire a second JSON reader over the raw body: {@code parseAuthCompleteRequest()} + * in NapServer owns that parse, and a second one with different semantics is the + * parser-differential surface #28 closed. + */ + @SuppressWarnings("unused") private final ObjectMapper objectMapper; private final AudienceResolver audienceResolver; private final RawBodyExtractor rawBodyExtractor; @@ -134,7 +145,7 @@ public ResponseEntity complete(HttpServletRequest request, HttpServletRespons } String authUrl = audienceResolver.resolve(request); - String authorization = resolveAuthorization(request, rawBody); + String authorization = resolveAuthorization(request); VerifyCompletionOutcome outcome = napServer.verifyCompletion(new VerifyCompletionInput( authorization, "POST", authUrl, rawBody, clientIpResolver.resolve(request))); @@ -333,22 +344,31 @@ private String extractCookie(HttpServletRequest request) { .orElse(null); } - private String resolveAuthorization(HttpServletRequest request, byte[] rawBody) { - String header = request.getHeader("Authorization"); - if (header != null && !header.isBlank()) { - return header; - } - - try { - @SuppressWarnings("unchecked") - Map body = objectMapper.readValue(rawBody, Map.class); - Object proof = body.get("proof"); - if (proof instanceof String proofValue && !proofValue.isBlank()) { - return proofValue; - } - } catch (Exception ignored) { - // Raw-body validation happens in NapServer; fallback extraction is best effort. - } - return null; + /** + * The NIP-98 proof, from {@code Authorization} and nowhere else. + * + *

A fallback used to read the proof from a {@code proof} field in the JSON body when the + * header was absent. It is gone, for four reasons that compound. + * + *

It was not in the protocol surface. {@link Nip98Validator} requires + * {@code Authorization: Nostr }, the RFC documents only that, and the TypeScript + * implementation has no equivalent, so it was a JVM-only credential location that no + * specification, test vector, or interop test described. + * + *

The hash coverage was self-referential. NIP-98's {@code payload} tag commits to + * {@code sha256(rawBody)}. A proof carried inside the body is part of what it must hash, so + * the construction is satisfiable only by excluding the field before hashing, which nothing + * specified and nothing enforced. + * + *

It parsed attacker-controlled bytes a second time. {@code parseAuthCompleteRequest()} + * already reads this 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 parsing differently in the two places produced no signal. + * + *

And a credential in a body gets logged by anything that logs request payloads, which is + * the reason {@code /auth/refresh} takes its token from a header rather than the body. + */ + private static String resolveAuthorization(HttpServletRequest request) { + return request.getHeader("Authorization"); } } diff --git a/nap-spring/src/test/java/xyz/tcheeric/nap/spring/controller/NapAuthControllerTest.java b/nap-spring/src/test/java/xyz/tcheeric/nap/spring/controller/NapAuthControllerTest.java index ca24b82..7e1e26c 100644 --- a/nap-spring/src/test/java/xyz/tcheeric/nap/spring/controller/NapAuthControllerTest.java +++ b/nap-spring/src/test/java/xyz/tcheeric/nap/spring/controller/NapAuthControllerTest.java @@ -82,8 +82,16 @@ private NapAuthController controller(NapProperties props) { return new NapAuthController(napServer, sessionStore, props, objectMapper); } + /** + * A proof in the body is not a credential this server accepts (#28). + * + *

The fallback that read it there was JVM-only, had no RFC or TypeScript counterpart, and + * asked the NIP-98 {@code payload} hash to cover a field that contains the hash. Asserting a + * valid-looking proof is refused is the point: a malformed one would be rejected + * whether or not the fallback existed. + */ @Test - void complete_usesBodyProofWhenAuthorizationHeaderIsMissing() { + void complete_doesNotAcceptAProofCarriedInTheBody() { String requestBody = """ {"challenge_id":"challenge-123","proof":"Nostr legacy-proof"} """; @@ -91,32 +99,20 @@ void complete_usesBodyProofWhenAuthorizationHeaderIsMissing() { request.setAttribute(NapServletFilter.RAW_BODY_ATTRIBUTE, requestBody.getBytes()); MockHttpServletResponse response = new MockHttpServletResponse(); - long now = 1_700_000_000L; - SessionRecord session = SessionRecord.create( - "session-1", "challenge-123", "access-token", - "npub1test", "a".repeat(64), - List.of("merchant"), List.of("read"), - now, now, now + 900, now + 43200 - ); - when(napServer.verifyCompletion(any())).thenReturn(VerifyCompletionOutcome.success(session)); - when(napServer.toPublicAuthSuccess(session)).thenReturn(new AuthSuccessResponse( - "ok", session.accessToken(), "Bearer", - session.expiresAt(), session.absoluteExpiryAt(), - new AuthSuccessResponse.Principal(session.principalNpub(), session.principalPubkey()), - session.roles(), session.permissions() - )); + when(napServer.verifyCompletion(any())) + .thenReturn(VerifyCompletionOutcome.failure(NapErrorCode.NAP_COMPLETE_MISSING_AUTH_HEADER)); + when(napServer.toPublicAuthFailure()) + .thenReturn(new NapServer.PublicFailureResponse(401, AuthFailureResponse.authenticationFailed())); - Object body = controller().complete(request, response).getBody(); + ResponseEntity result = controller().complete(request, response); + // The body proof never reaches the verifier, so the server sees a completion with no + // authorization at all and answers the same uniform 401 as any other failure. var captor = forClass(VerifyCompletionInput.class); verify(napServer).verifyCompletion(captor.capture()); - VerifyCompletionInput completionInput = captor.getValue(); - assertThat(completionInput.authorization()).isEqualTo("Nostr legacy-proof"); - assertThat(completionInput.method()).isEqualTo("POST"); - assertThat(completionInput.url()).isEqualTo("https://account.imani.casa/api/v1/auth/complete"); - assertThat(completionInput.rawBody()).isEqualTo(requestBody.getBytes()); - assertThat(response.getCookie("merchant_session")).isNotNull(); - assertThat(body).isNotNull(); + assertThat(captor.getValue().authorization()).isNull(); + assertThat(result.getStatusCode().value()).isEqualTo(401); + assertThat(response.getCookie("merchant_session")).isNull(); } @Test From f3f9d48b75425f4e58a3dcb64d912a8d3c4ea183 Mon Sep 17 00:00:00 2001 From: tcheeric Date: Thu, 24 Sep 2026 15:04:13 +0100 Subject: [PATCH 03/18] fix(spring): fail closed on protected paths, and agree on what a path is 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 --- .../nap/spring/config/NapProperties.java | 9 +++- .../filter/NapPermissionInterceptor.java | 11 ++-- .../nap/spring/filter/NapSessionFilter.java | 54 +++++++++++++++++-- .../spring/filter/NapSessionFilterTest.java | 36 +++++++++++++ 4 files changed, 97 insertions(+), 13 deletions(-) diff --git a/nap-spring/src/main/java/xyz/tcheeric/nap/spring/config/NapProperties.java b/nap-spring/src/main/java/xyz/tcheeric/nap/spring/config/NapProperties.java index 8bb0631..5305164 100644 --- a/nap-spring/src/main/java/xyz/tcheeric/nap/spring/config/NapProperties.java +++ b/nap-spring/src/main/java/xyz/tcheeric/nap/spring/config/NapProperties.java @@ -78,7 +78,14 @@ public record NapProperties( if (stepUpTtlSeconds <= 0) stepUpTtlSeconds = 600; if (aclRefreshIntervalSeconds <= 0) aclRefreshIntervalSeconds = 300; if (protectedPathPrefixes == null) protectedPathPrefixes = List.of(); - if (requireAnnotationOnProtectedPaths == null) requireAnnotationOnProtectedPaths = Boolean.FALSE; + // Defaults to true, and only bites when protected-path-prefixes is non-empty: a + // deployment that has named its protected paths has stated an intent, and a handler + // under one of them that declares no NAP annotation is served to anyone. That failure + // is silent -- nothing at startup, nothing in the log, and a diff showing a new + // endpoint with no guard removed. @PublicEndpoint is how a genuinely public handler + // says so in the source, which is the statement the missing annotation used to make + // only by omission. + if (requireAnnotationOnProtectedPaths == null) requireAnnotationOnProtectedPaths = Boolean.TRUE; if (cookie == null) cookie = new CookieProperties("merchant_session", true, true, "Lax", "/", "", 0); // Default cookie maxAge to the (effective) absolute session cap so the // browser retains the cookie for the full server-side lifetime. diff --git a/nap-spring/src/main/java/xyz/tcheeric/nap/spring/filter/NapPermissionInterceptor.java b/nap-spring/src/main/java/xyz/tcheeric/nap/spring/filter/NapPermissionInterceptor.java index 35352d1..ac6b29a 100644 --- a/nap-spring/src/main/java/xyz/tcheeric/nap/spring/filter/NapPermissionInterceptor.java +++ b/nap-spring/src/main/java/xyz/tcheeric/nap/spring/filter/NapPermissionInterceptor.java @@ -148,14 +148,9 @@ private boolean requiresExplicitDeclaration(HttpServletRequest request, HandlerM || AnnotatedElementUtils.findMergedAnnotation(handler.getBeanType(), PublicEndpoint.class) != null) { return false; } - String path = request.getRequestURI(); - if (path == null) { - return false; - } - String contextPath = request.getContextPath(); - if (contextPath != null && !contextPath.isEmpty() && path.startsWith(contextPath)) { - path = path.substring(contextPath.length()); - } + // One path helper for both, so the filter and this interceptor cannot disagree about + // which requests fall under a protected prefix. + String path = NapSessionFilter.pathWithinApplication(request); for (String prefix : protectedPathPrefixes) { if (prefix != null && !prefix.isBlank() && path.startsWith(prefix)) { return true; diff --git a/nap-spring/src/main/java/xyz/tcheeric/nap/spring/filter/NapSessionFilter.java b/nap-spring/src/main/java/xyz/tcheeric/nap/spring/filter/NapSessionFilter.java index 7790025..c0a27a0 100644 --- a/nap-spring/src/main/java/xyz/tcheeric/nap/spring/filter/NapSessionFilter.java +++ b/nap-spring/src/main/java/xyz/tcheeric/nap/spring/filter/NapSessionFilter.java @@ -75,7 +75,7 @@ public NapSessionFilter(SessionStore sessionStore, @Override protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain) throws ServletException, IOException { - String path = request.getRequestURI(); + String path = pathWithinApplication(request); boolean isProtected = protectedPrefixes.stream().anyMatch(path::startsWith); if (!isProtected) { @@ -97,21 +97,21 @@ protected void doFilterInternal(HttpServletRequest request, HttpServletResponse // credential. String accessToken = extractCookie(request); if (accessToken == null) { - filterChain.doFilter(request, response); + unauthenticated(request, response, filterChain, "no_cookie", null); return; } var session = sessionStore.getByAccessToken(accessToken); if (session.isEmpty()) { - filterChain.doFilter(request, response); + unauthenticated(request, response, filterChain, "unknown_token", null); return; } SessionRecord record = session.get(); if (isExpired(record)) { sessionStore.revokeBySessionId(record.sessionId(), Instant.now().getEpochSecond()); - filterChain.doFilter(request, response); + unauthenticated(request, response, filterChain, "expired", record.principalPubkey()); return; } @@ -143,6 +143,52 @@ protected void doFilterInternal(HttpServletRequest request, HttpServletResponse } } + /** + * Continue the chain without an authentication, leaving a record that it happened. + * + *

This filter does not refuse the request: whether an unauthenticated caller may reach a + * handler is {@link NapPermissionInterceptor}'s decision, and an adapter cannot know which + * endpoints are meant to be public. But the three ways a protected request arrives + * unauthenticated used to produce no output at all, so an operator could not tell "nobody is + * calling this" from "everybody is, without a session". The TypeScript guards emit + * {@code NAP_GUARD_NO_SESSION} per refusal for the same reason. + * + *

Debug rather than warn: an unauthenticated request to a protected path is ordinary + * before login. What matters is that it is greppable. + */ + private void unauthenticated(HttpServletRequest request, HttpServletResponse response, + FilterChain filterChain, String reason, String pubkey) + throws ServletException, IOException { + if (log.isDebugEnabled()) { + log.debug("nap_guard_no_session reason={} path={} pubkey={}", + reason, pathWithinApplication(request), pubkey); + } + filterChain.doFilter(request, response); + } + + /** + * The request path with any servlet context path stripped, which is what + * {@code nap.protected-path-prefixes} is written against. + * + *

Shared with {@link NapPermissionInterceptor} so the filter and the interceptor cannot + * disagree about which requests are protected. They did: this filter matched the raw + * {@code getRequestURI()} while the interceptor stripped the context path first, so under a + * non-empty context path the filter skipped requests the interceptor believed were covered. + * With the interceptor now failing closed on undeclared handlers, that disagreement decides + * whether a request is authenticated at all. + */ + static String pathWithinApplication(HttpServletRequest request) { + String path = request.getRequestURI(); + if (path == null) { + return ""; + } + String contextPath = request.getContextPath(); + if (contextPath != null && !contextPath.isEmpty() && path.startsWith(contextPath)) { + return path.substring(contextPath.length()); + } + return path; + } + private String extractCookie(HttpServletRequest request) { if (request.getCookies() == null) return null; return Arrays.stream(request.getCookies()) diff --git a/nap-spring/src/test/java/xyz/tcheeric/nap/spring/filter/NapSessionFilterTest.java b/nap-spring/src/test/java/xyz/tcheeric/nap/spring/filter/NapSessionFilterTest.java index 1c45a3b..a84943f 100644 --- a/nap-spring/src/test/java/xyz/tcheeric/nap/spring/filter/NapSessionFilterTest.java +++ b/nap-spring/src/test/java/xyz/tcheeric/nap/spring/filter/NapSessionFilterTest.java @@ -288,6 +288,42 @@ private SessionRecord sessionRecord(String sessionId, String accessToken) { ); } + /** + * Protection must not depend on the servlet context path (#29). + * + *

This filter matched the raw {@code getRequestURI()} while NapPermissionInterceptor + * stripped the context path first, so a deployment under {@code /app} had a filter that + * skipped authentication on requests the interceptor believed were guarded. With the + * interceptor now failing closed by default, that disagreement decides whether a request is + * authenticated at all. + */ + @Test + void doFilterInternal_appliesProtectionUnderANonEmptyContextPath() throws Exception { + SessionRecord session = sessionRecord(); + when(sessionStore.getByAccessToken("access-token-123")).thenReturn(Optional.of(session)); + when(aclResolver.resolve(session.principalNpub(), session.principalPubkey())) + .thenReturn(AclDecision.allowed(List.of("merchant"), List.of("read"))); + + NapSessionFilter filter = new NapSessionFilter( + sessionStore, aclResolver, "merchant_session", + List.of("/internal/v1/merchants"), Duration.ofMinutes(5) + ); + + MockHttpServletRequest request = + new MockHttpServletRequest("POST", "/app/internal/v1/merchants/test/suspend"); + request.setContextPath("/app"); + request.setCookies(new Cookie("merchant_session", "access-token-123")); + + AtomicReference captured = new AtomicReference<>(); + filter.doFilterInternal(request, new MockHttpServletResponse(), (req, res) -> + captured.set(SecurityContextHolder.getContext().getAuthentication())); + + // The prefix matches only once the context path is stripped, so an authentication + // being present is what proves the filter treated this as protected. + assertThat(captured.get()).isNotNull(); + assertThat(captured.get().isAuthenticated()).isTrue(); + } + private SessionRecord sessionRecord() { long now = java.time.Instant.now().getEpochSecond(); return SessionRecord.create( From 569df2b448dc28cc3528bcd71ddf7787e7424c7e Mon Sep 17 00:00:00 2001 From: tcheeric Date: Thu, 24 Sep 2026 15:07:20 +0100 Subject: [PATCH 04/18] fix(spring): stop the auto-configuration defaults from failing open 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 --- README.md | 51 +++++++++----- .../spring/config/NapAutoConfiguration.java | 41 ++++++++++- .../nap/spring/config/NapProperties.java | 6 ++ .../config/NapAutoConfigurationAclTest.java | 70 +++++++++++++++++++ .../controller/NapAuthControllerTest.java | 1 + .../spring/filter/NapServletFilterTest.java | 1 + 6 files changed, 152 insertions(+), 18 deletions(-) create mode 100644 nap-spring/src/test/java/xyz/tcheeric/nap/spring/config/NapAutoConfigurationAclTest.java diff --git a/README.md b/README.md index d6538dc..628eee5 100644 --- a/README.md +++ b/README.md @@ -57,14 +57,32 @@ nap: refresh-ttl-seconds: 0 # 0 = refresh disabled protected-path-prefixes: [/api/v1/merchant] trusted-proxies: [] # see "Rate limiting behind a proxy" below - require-annotation-on-protected-paths: false cookie: name: merchant_session ``` -Auto-configuration supplies `NapServer`, in-memory stores, an `AllowAllAclResolver`, the -controller, and the permission interceptor — each `@ConditionalOnMissingBean`, so supplying -your own `SessionStore` (e.g. `JdbcSessionStore`) replaces it. +**You must supply an `AclResolver` bean.** There is no default. The auto-configuration used to +fall back to `AllowAllAclResolver`, which authorizes every principal who can prove key control, +and nothing reported it: you wire NAP, log in with your own key, see a session, and ship with the +authorization layer a no-op. Supply `RegistryAclResolver` (or your own), or set +`nap.allow-all-principals: true` to ask for the old behaviour deliberately. + +```java +@Bean +AclResolver aclResolver(AclStore aclStore) { + return RegistryAclResolver.create(myPermissionRegistry, aclStore, /* autoProvision */ false); +} +``` + +Auto-configuration supplies `NapServer`, in-memory stores, the controller, and the permission +interceptor — each `@ConditionalOnMissingBean`, so supplying your own `SessionStore` (e.g. +`JdbcSessionStore`) replaces it. + +**The in-memory stores are for development.** They are the default because they need no +configuration, and they log a warning at startup saying so. Sessions are lost on restart and are +not shared between instances, which makes revocation per-node: `revokeByPrincipal()` on a +suspension reaches only the instance that served the request, and the principal keeps working +everywhere else until their session expires. Use `JdbcSessionStore` for anything multi-instance. **The two filters are not auto-registered** — a second registration would consume the request body twice. Register them yourself and pass the settings; there are no defaulting constructors: @@ -80,19 +98,18 @@ Guard endpoints with `@RequiresPermission` (preferred), `@RequiresRole`, `@Requi `@RequiresSession` when the endpoint is for signed-in users generally and no permission distinguishes them. -**A handler that declares none of these is not guarded.** `NapSessionFilter` populates the -`SecurityContext` on `nap.protected-path-prefixes` but lets unauthenticated requests through, and -the interceptor only rejects handlers that declare a requirement — so a protected prefix means -"authenticate here if you can", not "login required". `@RequiresSession` is how a handler says -the latter. - -That default is defensible — the adapter cannot know which endpoints are meant to be public — -but it is also what forgetting looks like, so a handler added to a protected controller without -an annotation is exposed with nothing in the diff to show for it. Set -`nap.require-annotation-on-protected-paths: true` to make the declaration mandatory inside -`nap.protected-path-prefixes`: an undeclared handler there is refused with `500` (a wiring bug, -not a caller error — no credential would help). Genuinely public endpoints stay expressible with -`@PublicEndpoint("why")`, which states in the source what omission used to state only by accident. +**A handler that declares none of these is refused inside a protected prefix.** +`NapSessionFilter` populates the `SecurityContext` on `nap.protected-path-prefixes` but lets +unauthenticated requests through, and the interceptor is what enforces. Since a handler under a +protected prefix that declares nothing would otherwise be served to anyone, +`nap.require-annotation-on-protected-paths` **defaults to `true`**: an undeclared handler there is +refused with `500` (a wiring bug, not a caller error, so no credential would help). Genuinely +public endpoints stay expressible with `@PublicEndpoint("why")`, which states in the source what +omission used to state only by accident. + +Set it to `false` to restore the previous behaviour, where an unannotated handler is public. That +is the configuration in which forgetting an annotation exposes an endpoint with nothing in the +diff to show for it, so prefer `@PublicEndpoint`. ## Rate limiting behind a proxy diff --git a/nap-spring/src/main/java/xyz/tcheeric/nap/spring/config/NapAutoConfiguration.java b/nap-spring/src/main/java/xyz/tcheeric/nap/spring/config/NapAutoConfiguration.java index b596a0f..081ec97 100644 --- a/nap-spring/src/main/java/xyz/tcheeric/nap/spring/config/NapAutoConfiguration.java +++ b/nap-spring/src/main/java/xyz/tcheeric/nap/spring/config/NapAutoConfiguration.java @@ -1,6 +1,8 @@ package xyz.tcheeric.nap.spring.config; import org.springframework.boot.autoconfigure.AutoConfiguration; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.springframework.boot.autoconfigure.jackson.JacksonAutoConfiguration; import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; @@ -38,21 +40,58 @@ @EnableConfigurationProperties(NapProperties.class) public class NapAutoConfiguration { + private static final Logger log = LoggerFactory.getLogger(NapAutoConfiguration.class); + @Bean @ConditionalOnMissingBean public ChallengeStore challengeStore() { + log.warn("nap_in_memory_challenge_store: challenges are lost on restart and are not " + + "shared between instances. Supply a JdbcChallengeStore bean for any " + + "multi-instance or production deployment."); return new InMemoryChallengeStore(); } @Bean @ConditionalOnMissingBean public SessionStore sessionStore() { + // Warned rather than refused: the in-memory stores are genuinely useful for local + // development, so failing here would be the wrong trade. What was wrong was that a + // deployment could reach production on them without ever being told, and the + // revocation consequence is a security one rather than only an availability one. + log.warn("nap_in_memory_session_store: sessions are lost on restart and are not shared " + + "between instances, so revokeByPrincipal reaches only this node and a " + + "suspended principal keeps working elsewhere until their session expires. " + + "Supply a JdbcSessionStore bean for any multi-instance or production " + + "deployment."); return new InMemorySessionStore(); } + /** + * There is deliberately no default {@link AclResolver}. + * + *

The previous default was {@link AllowAllAclResolver}, which authorizes every principal + * who can prove key control. That is indistinguishable from a working configuration: an + * operator wires NAP, logs in with their own key, sees a session, and ships, with nothing + * reporting that the authorization layer is a no-op. + * + *

The escape hatch remains, as a written decision rather than an omission. This mirrors + * what {@code createAudienceHostAllowlist()} and {@code createMintAllowlist()} already do on + * the TypeScript side: refuse at wiring time rather than accept a configuration that permits + * everything, because an allowlist that allows everything is the state they exist to make + * unrepresentable. + */ @Bean @ConditionalOnMissingBean - public AclResolver aclResolver() { + public AclResolver aclResolver(NapProperties properties) { + if (!properties.allowAllPrincipals()) { + throw new IllegalStateException( + "NAP requires an AclResolver bean. Supply RegistryAclResolver (or your own), " + + "or set nap.allow-all-principals=true to authorize every principal " + + "who proves key control, which is what the previous default did " + + "silently."); + } + log.warn("nap_acl_allow_all_enabled: every principal proving key control is authorized, " + + "with no roles or permissions. This is nap.allow-all-principals=true."); return new AllowAllAclResolver(); } diff --git a/nap-spring/src/main/java/xyz/tcheeric/nap/spring/config/NapProperties.java b/nap-spring/src/main/java/xyz/tcheeric/nap/spring/config/NapProperties.java index 5305164..822d5e4 100644 --- a/nap-spring/src/main/java/xyz/tcheeric/nap/spring/config/NapProperties.java +++ b/nap-spring/src/main/java/xyz/tcheeric/nap/spring/config/NapProperties.java @@ -45,6 +45,11 @@ public record NapProperties( // first request rather than an endpoint quietly serving anyone; @PublicEndpoint is how // a genuinely public handler says so. Boolean requireAnnotationOnProtectedPaths, + // Authorize every principal who proves key control, with no roles or permissions. The + // auto-configured AclResolver used to do this silently; it now has to be asked for, + // because a no-op authorization layer is indistinguishable from a working one until + // someone who should not have access uses it. + Boolean allowAllPrincipals, CookieProperties cookie ) { @@ -86,6 +91,7 @@ public record NapProperties( // says so in the source, which is the statement the missing annotation used to make // only by omission. if (requireAnnotationOnProtectedPaths == null) requireAnnotationOnProtectedPaths = Boolean.TRUE; + if (allowAllPrincipals == null) allowAllPrincipals = Boolean.FALSE; if (cookie == null) cookie = new CookieProperties("merchant_session", true, true, "Lax", "/", "", 0); // Default cookie maxAge to the (effective) absolute session cap so the // browser retains the cookie for the full server-side lifetime. diff --git a/nap-spring/src/test/java/xyz/tcheeric/nap/spring/config/NapAutoConfigurationAclTest.java b/nap-spring/src/test/java/xyz/tcheeric/nap/spring/config/NapAutoConfigurationAclTest.java new file mode 100644 index 0000000..a87a40c --- /dev/null +++ b/nap-spring/src/test/java/xyz/tcheeric/nap/spring/config/NapAutoConfigurationAclTest.java @@ -0,0 +1,70 @@ +package xyz.tcheeric.nap.spring.config; + +import org.junit.jupiter.api.Test; +import xyz.tcheeric.nap.core.AclDecision; +import xyz.tcheeric.nap.server.AclResolver; +import xyz.tcheeric.nap.server.AllowAllAclResolver; + +import java.util.List; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +/** + * The auto-configuration must not hand out authorization by default (#30). + * + *

The previous default {@link AllowAllAclResolver} authorized every principal who could prove + * key control, and nothing reported it. An operator wired NAP, logged in with their own key, saw + * a session, and shipped, with the authorization layer a silent no-op. + * + *

Exercised against the bean method directly rather than through a Spring context: this module + * does not depend on spring-boot-test, and adding that dependency to assert a two-branch guard + * would cost more than it proves. The {@code @ConditionalOnMissingBean} half (an application's own + * resolver wins) is Spring's behaviour and not this class's to re-test. + */ +class NapAutoConfigurationAclTest { + + private final NapAutoConfiguration autoConfiguration = new NapAutoConfiguration(); + + @Test + void refusesToBuildAResolverWithoutAnExplicitOptIn() { + assertThatThrownBy(() -> autoConfiguration.aclResolver(propertiesWithAllowAll(null))) + .isInstanceOf(IllegalStateException.class) + // The message has to name the property and the escape hatch, since the whole + // problem was that the previous behaviour was invisible. + .hasMessageContaining("nap.allow-all-principals") + .hasMessageContaining("AclResolver"); + } + + @Test + void refusesWhenTheOptInIsExplicitlyFalse() { + assertThatThrownBy(() -> autoConfiguration.aclResolver(propertiesWithAllowAll(false))) + .isInstanceOf(IllegalStateException.class); + } + + @Test + void buildsAllowAllWhenItIsAskedForExplicitly() { + AclResolver resolver = autoConfiguration.aclResolver(propertiesWithAllowAll(true)); + + assertThat(resolver).isInstanceOf(AllowAllAclResolver.class); + + // Still allow-all, which is the point: the behaviour is unchanged, only the way you + // arrive at it is. + AclDecision decision = resolver.resolve("npub1anyone", "a".repeat(64)); + assertThat(decision.allowed()).isTrue(); + assertThat(decision.roles()).isEmpty(); + assertThat(decision.permissions()).isEmpty(); + } + + private static NapProperties propertiesWithAllowAll(Boolean allowAllPrincipals) { + return new NapProperties( + true, "https://account.imani.casa", + 60, 3600, 900, 43200, 30, 60, 600, 0, 300, + null, 0, 0, null, null, null, null, null, 0, + List.of(), + List.of("/internal/v1/merchants"), + false, + allowAllPrincipals, + new NapProperties.CookieProperties("session", true, true, "Lax", "/", "", 43200)); + } +} diff --git a/nap-spring/src/test/java/xyz/tcheeric/nap/spring/controller/NapAuthControllerTest.java b/nap-spring/src/test/java/xyz/tcheeric/nap/spring/controller/NapAuthControllerTest.java index 7e1e26c..78471d4 100644 --- a/nap-spring/src/test/java/xyz/tcheeric/nap/spring/controller/NapAuthControllerTest.java +++ b/nap-spring/src/test/java/xyz/tcheeric/nap/spring/controller/NapAuthControllerTest.java @@ -70,6 +70,7 @@ private static NapProperties propertiesWith(NapProperties.CookieProperties cooki List.of(), // trustedProxies — none, so the limiter counts the TCP peer List.of("/internal/v1/merchants"), false, // requireAnnotationOnProtectedPaths + true, // allowAllPrincipals — these tests mock NapServer, so no resolver is wired cookie ); } diff --git a/nap-spring/src/test/java/xyz/tcheeric/nap/spring/filter/NapServletFilterTest.java b/nap-spring/src/test/java/xyz/tcheeric/nap/spring/filter/NapServletFilterTest.java index c822424..2660a9b 100644 --- a/nap-spring/src/test/java/xyz/tcheeric/nap/spring/filter/NapServletFilterTest.java +++ b/nap-spring/src/test/java/xyz/tcheeric/nap/spring/filter/NapServletFilterTest.java @@ -96,6 +96,7 @@ private static NapProperties propertiesWithMaxBodyBytes(int maxBodyBytes) { List.of(), // trustedProxies List.of("/internal/v1/merchants"), false, + true, // allowAllPrincipals new NapProperties.CookieProperties("session", true, true, "Lax", "/", "", 43200)); } From d252fdddde173ccf25c1c3cd9f0e00f5f0db9681 Mon Sep 17 00:00:00 2001 From: tcheeric Date: Thu, 24 Sep 2026 15:10:34 +0100 Subject: [PATCH 05/18] fix(server): evict dead records from the in-memory stores 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 --- .../server/store/InMemoryChallengeStore.java | 49 ++++++ .../server/store/InMemorySessionStore.java | 65 +++++++ .../store/InMemoryStoreEvictionTest.java | 166 ++++++++++++++++++ 3 files changed, 280 insertions(+) create mode 100644 nap-server/src/test/java/xyz/tcheeric/nap/server/store/InMemoryStoreEvictionTest.java diff --git a/nap-server/src/main/java/xyz/tcheeric/nap/server/store/InMemoryChallengeStore.java b/nap-server/src/main/java/xyz/tcheeric/nap/server/store/InMemoryChallengeStore.java index 4b6dc8d..d76c2f2 100644 --- a/nap-server/src/main/java/xyz/tcheeric/nap/server/store/InMemoryChallengeStore.java +++ b/nap-server/src/main/java/xyz/tcheeric/nap/server/store/InMemoryChallengeStore.java @@ -8,23 +8,72 @@ import xyz.tcheeric.nap.core.RedeemParams; import xyz.tcheeric.nap.core.RedeemResult; +import java.time.Clock; import java.util.Objects; import java.util.Optional; import java.util.OptionalInt; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicLong; /** * In-memory ChallengeStore for testing and single-instance deployments. + * + *

Bounded. Records are dropped once they can no longer affect a decision, because the map is + * filled by {@code /auth/init}, which is unauthenticated: without eviction the footprint grows + * with login volume at a rate the caller sets. The outstanding-challenge caps do not help, since + * they count only records still in {@code ISSUED} and so bound concurrency rather than memory. */ public final class InMemoryChallengeStore implements ChallengeStore { private final ConcurrentHashMap store = new ConcurrentHashMap<>(); + private final AtomicLong lastSweptAt = new AtomicLong(Long.MIN_VALUE); + private final Clock clock; + + public InMemoryChallengeStore() { + this(Clock.systemUTC()); + } + + /** @param clock injectable so eviction is testable without sleeping. */ + public InMemoryChallengeStore(Clock clock) { + this.clock = clock; + } @Override public void create(ChallengeRecord record) { + // Swept here rather than on a timer: the store owns no thread and cannot outlive its + // holder, which is the same shape BoundedEventReplayGuard uses. create() is also the + // method an attacker drives, so the work lands where the growth comes from. + sweep(clock.instant().getEpochSecond()); store.put(record.challengeId(), record); } + /** + * Drop records that can no longer affect a decision. + * + *

The bound is {@code resultCacheUntil} when one is set, and {@code expiresAt} otherwise. + * That distinction is load-bearing: a redeemed challenge inside its result-cache window is + * what makes a client retry idempotent (RFC §13.3), so evicting on expiry alone would turn a + * duplicate submission into a fresh login attempt against a challenge that no longer exists. + * + *

Rate-limited to once per clock tick. Without that a burst makes every request walk the + * whole map, which is the load profile eviction exists to prevent. + */ + private void sweep(long now) { + long last = lastSweptAt.get(); + if (now <= last || !lastSweptAt.compareAndSet(last, now)) { + return; + } + store.values().removeIf(record -> { + Long cacheUntil = record.resultCacheUntil(); + return (cacheUntil != null ? cacheUntil : record.expiresAt()) < now; + }); + } + + /** Records currently retained. For tests and diagnostics. */ + public int size() { + return store.size(); + } + @Override public Optional get(String challengeId) { return Optional.ofNullable(store.get(challengeId)); diff --git a/nap-server/src/main/java/xyz/tcheeric/nap/server/store/InMemorySessionStore.java b/nap-server/src/main/java/xyz/tcheeric/nap/server/store/InMemorySessionStore.java index 8a1b47f..376a63f 100644 --- a/nap-server/src/main/java/xyz/tcheeric/nap/server/store/InMemorySessionStore.java +++ b/nap-server/src/main/java/xyz/tcheeric/nap/server/store/InMemorySessionStore.java @@ -4,11 +4,18 @@ import xyz.tcheeric.nap.core.SessionRecord; import xyz.tcheeric.nap.core.SessionStore; +import java.time.Clock; import java.util.Optional; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicLong; /** * In-memory SessionStore for testing and single-instance deployments. + * + *

Bounded. A session that has passed its absolute cap, and whose refresh window has closed + * too, can no longer affect a decision, so it is dropped rather than retained with a + * {@code revokedAt} stamp. Without that the four indexes below only ever grew, at a rate driven + * by login volume. */ public final class InMemorySessionStore implements SessionStore { @@ -17,9 +24,67 @@ public final class InMemorySessionStore implements SessionStore { private final ConcurrentHashMap byChallengeId = new ConcurrentHashMap<>(); /** Holds both the current and the previous refresh token — see {@link #getByRefreshToken}. */ private final ConcurrentHashMap byRefreshToken = new ConcurrentHashMap<>(); + private final AtomicLong lastSweptAt = new AtomicLong(Long.MIN_VALUE); + private final Clock clock; + + public InMemorySessionStore() { + this(Clock.systemUTC()); + } + + /** @param clock injectable so eviction is testable without sleeping. */ + public InMemorySessionStore(Clock clock) { + this.clock = clock; + } + + /** + * Drop sessions that can no longer authenticate or be replayed. + * + *

The bound is the absolute cap, extended to {@code refreshExpiresAt} when it is later. + * That extension is deliberate: {@link #getByRefreshToken} keeps answering for a revoked + * session precisely so a replay stays visible in the audit log, and evicting on the access + * window alone would make a stolen refresh token presented just after expiry look like an + * unknown token rather than a reuse. + * + *

Every index is swept together, so a record cannot survive in one map after being + * dropped from another. Rate-limited to once per clock tick, matching the challenge store + * and {@code InMemoryRateLimiter}. + */ + private void sweep(long now) { + long last = lastSweptAt.get(); + if (now <= last || !lastSweptAt.compareAndSet(last, now)) { + return; + } + bySessionId.values().removeIf(session -> { + if (!isDead(session, now)) { + return false; + } + byAccessToken.remove(session.accessToken()); + byChallengeId.remove(session.challengeId()); + if (session.refreshToken() != null) { + byRefreshToken.remove(session.refreshToken()); + } + if (session.previousRefreshToken() != null) { + byRefreshToken.remove(session.previousRefreshToken()); + } + return true; + }); + } + + private static boolean isDead(SessionRecord session, long now) { + long deadline = session.refreshExpiresAt() != null + ? Math.max(session.absoluteExpiryAt(), session.refreshExpiresAt()) + : session.absoluteExpiryAt(); + return deadline < now; + } + + /** Sessions currently retained. For tests and diagnostics. */ + public int size() { + return bySessionId.size(); + } @Override public SessionRecord createForChallenge(SessionRecord record) { + sweep(clock.instant().getEpochSecond()); var existing = byChallengeId.putIfAbsent(record.challengeId(), record); if (existing != null) { return existing; diff --git a/nap-server/src/test/java/xyz/tcheeric/nap/server/store/InMemoryStoreEvictionTest.java b/nap-server/src/test/java/xyz/tcheeric/nap/server/store/InMemoryStoreEvictionTest.java new file mode 100644 index 0000000..61ea116 --- /dev/null +++ b/nap-server/src/test/java/xyz/tcheeric/nap/server/store/InMemoryStoreEvictionTest.java @@ -0,0 +1,166 @@ +package xyz.tcheeric.nap.server.store; + +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import xyz.tcheeric.nap.core.ChallengeRecord; +import xyz.tcheeric.nap.core.RedeemParams; +import xyz.tcheeric.nap.core.SessionRecord; + +import java.time.Clock; +import java.time.Instant; +import java.time.ZoneOffset; +import java.util.List; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * The in-memory stores must not grow without bound (#31). + * + *

Both maps are filled by unauthenticated traffic: {@code /auth/init} writes a challenge under + * a fresh random id, and every completion writes a session. Marking a record expired or revoked + * and keeping it means the footprint tracks total login volume, which is something a caller can + * accelerate. + * + *

What makes these tests worth more than a size assertion is the second half of each: the + * records that must survive. A sweep that also drops those is not a fix, it is a + * different bug. + */ +class InMemoryStoreEvictionTest { + + private static final long NOW = 1_700_000_000L; + + /** A clock the test moves by hand, so eviction is exercised without sleeping. */ + private static final class MutableClock extends Clock { + private long epochSecond; + + MutableClock(long epochSecond) { + this.epochSecond = epochSecond; + } + + void advanceSeconds(long seconds) { + epochSecond += seconds; + } + + @Override public java.time.ZoneId getZone() { return ZoneOffset.UTC; } + @Override public Clock withZone(java.time.ZoneId zone) { return this; } + @Override public Instant instant() { return Instant.ofEpochSecond(epochSecond); } + } + + @Nested + class ChallengeStore { + + private static ChallengeRecord issued(String id, long issuedAt, long expiresAt) { + return ChallengeRecord.issued( + id, "challenge-" + id, "npub1test", "pubkey-abc", + "https://auth.example.com", "nip98", issuedAt, expiresAt); + } + + @Test + void dropsChallengesThatCanNoLongerBeRedeemed() { + MutableClock clock = new MutableClock(NOW); + InMemoryChallengeStore store = new InMemoryChallengeStore(clock); + + for (int i = 0; i < 1_000; i++) { + store.create(issued("expired-" + i, NOW, NOW + 60)); + } + assertThat(store.size()).isEqualTo(1_000); + + clock.advanceSeconds(120); + store.create(issued("live", NOW + 120, NOW + 180)); + + assertThat(store.size()).isEqualTo(1); + assertThat(store.get("live")).isPresent(); + assertThat(store.get("expired-0")).isEmpty(); + } + + /** + * RFC §13.3 makes a repeat submission of the same completion return the cached result + * rather than a second login. That only works while the redeemed challenge is still + * there, so the result-cache window is the retention bound and not the expiry. + */ + @Test + void keepsARedeemedChallengeUntilItsResultCacheExpires() { + MutableClock clock = new MutableClock(NOW); + InMemoryChallengeStore store = new InMemoryChallengeStore(clock); + store.create(issued("redeemed", NOW, NOW + 60)); + store.redeem("redeemed", new RedeemParams("event-1", "session-1", NOW, NOW + 300)); + + // Past the challenge's own expiry, still inside the result cache. + clock.advanceSeconds(120); + store.create(issued("trigger-a", NOW + 120, NOW + 180)); + + assertThat(store.get("redeemed")).isPresent(); + + // Past the result cache too, so a retry can no longer be answered from it. + clock.advanceSeconds(300); + store.create(issued("trigger-b", NOW + 420, NOW + 480)); + + assertThat(store.get("redeemed")).isEmpty(); + } + } + + @Nested + class SessionStore { + + private static SessionRecord session(String id, long absoluteExpiryAt) { + return SessionRecord.create( + id, "challenge-" + id, "access-" + id, + "npub1test", "a".repeat(64), + List.of(), List.of(), + NOW, NOW, NOW + 900, absoluteExpiryAt); + } + + @Test + void dropsSessionsPastTheirAbsoluteCap() { + MutableClock clock = new MutableClock(NOW); + InMemorySessionStore store = new InMemorySessionStore(clock); + + for (int i = 0; i < 500; i++) { + store.createForChallenge(session("dead-" + i, NOW + 60)); + } + assertThat(store.size()).isEqualTo(500); + + clock.advanceSeconds(120); + store.createForChallenge(session("live", NOW + 100_000)); + + assertThat(store.size()).isEqualTo(1); + assertThat(store.getBySessionId("live")).isPresent(); + // Swept from every index, not just the primary one. + assertThat(store.getByAccessToken("access-dead-0")).isEmpty(); + assertThat(store.getBySessionId("dead-0")).isEmpty(); + } + + /** + * A refresh token outliving the access window is the case reuse detection depends on: + * {@code getByRefreshToken} deliberately answers for revoked sessions so a replay is + * recognisable. Evicting on the access window alone would turn a detected reuse into an + * unknown token, which is the one signal that says a credential leaked. + */ + @Test + void keepsASessionWhoseRefreshWindowIsStillOpen() { + MutableClock clock = new MutableClock(NOW); + InMemorySessionStore store = new InMemorySessionStore(clock); + + SessionRecord withRefresh = new SessionRecord( + "sid-refresh", "chal-r", "access-r", + "npub1test", "a".repeat(64), + List.of(), List.of(), + NOW, NOW, NOW + 60, NOW + 60, + null, null, null, + "refresh-r", NOW + 86_400, null); + store.createForChallenge(withRefresh); + + // Well past the absolute cap, still inside the refresh window. + clock.advanceSeconds(3_600); + store.createForChallenge(session("trigger", NOW + 100_000)); + + assertThat(store.getByRefreshToken("refresh-r")).isPresent(); + + // Past the refresh window too. + clock.advanceSeconds(90_000); + store.createForChallenge(session("trigger-2", NOW + 200_000)); + + assertThat(store.getByRefreshToken("refresh-r")).isEmpty(); + } + } +} From f88bf0ae09b9bb64b1fdd6ae54fc9a7edd839630 Mon Sep 17 00:00:00 2001 From: tcheeric Date: Thu, 24 Sep 2026 15:12:35 +0100 Subject: [PATCH 06/18] ci: build, test and scan on every pull request 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 --- .github/dependabot.yml | 21 +++++++ .github/workflows/ci.yml | 127 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 148 insertions(+) create mode 100644 .github/dependabot.yml create mode 100644 .github/workflows/ci.yml diff --git a/.github/dependabot.yml b/.github/dependabot.yml new file mode 100644 index 0000000..545cb18 --- /dev/null +++ b/.github/dependabot.yml @@ -0,0 +1,21 @@ +# Dependency updates arrive as reviewable PRs rather than silent drift. +# The Java tree is managed by imani-bom, so most transitive upgrades land through that +# single coordinate: watching the root pom is what surfaces them at all. +version: 2 +updates: + - package-ecosystem: maven + directory: "/" + schedule: + interval: weekly + open-pull-requests-limit: 10 + labels: + - dependencies + + # The workflows above pin actions by major tag, which still moves underneath us. + # Keeping them updated here means CI infrastructure ages visibly. + - package-ecosystem: github-actions + directory: "/" + schedule: + interval: weekly + labels: + - dependencies diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 0000000..a51a6e1 --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,127 @@ +name: CI + +on: + push: + branches: [main, master, develop] + pull_request: + +concurrency: + group: ci-${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + +permissions: + contents: read + +jobs: + build: + name: Build and test (Java ${{ matrix.java }}) + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + java: ['21'] + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-java@v4 + with: + java-version: ${{ matrix.java }} + distribution: temurin + cache: maven + + # TypeScriptClientInteropTest spawns the real @imani/nap-client-http package through + # tsx, so a node toolchain must exist before Maven runs. + - uses: actions/setup-node@v4 + with: + node-version: '20' + + # Correctness in this repository is defined relative to the TypeScript reference + # implementation, and the two tests that assert that (OfficialTestVectorsTest and + # TypeScriptClientInteropTest) both call JUnit assumeTrue when the sibling `nap` + # checkout is absent. assumeTrue SKIPS rather than fails, so without this clone the + # interop suite reports green while asserting nothing, which is precisely the silent + # regression this CI exists to catch. + # + # The clone target is not configurable: TypeScriptClientInteropTest hard-codes + # Path.of(user.home, "IdeaProjects", "nap") with no override property, so the path + # below has to match it exactly. OfficialTestVectorsTest does accept + # -Dnap.test-vectors.dir, but pointing both at one checkout keeps them consistent. + - name: Check out the TypeScript reference implementation + run: | + git clone --depth 1 https://github.com/tcheeric/nap.git "$HOME/IdeaProjects/nap" + + # The interop test looks for node_modules/.bin/tsx inside that checkout. Installing + # dependencies is what flips the test from skipped to actually executed. + - name: Install TypeScript client dependencies + run: npm ci --prefix "$HOME/IdeaProjects/nap" + + # A skipped test is indistinguishable from a passing one in a green check, so assert + # the interop preconditions explicitly and fail loudly when they are missing. + - name: Assert the interop toolchain is present + run: | + test -x "$HOME/IdeaProjects/nap/node_modules/.bin/tsx" \ + || { echo "tsx missing: the interop test would silently skip"; exit 1; } + test -f "$HOME/IdeaProjects/nap/packages/nap-core/test-vectors/nip98.json" \ + || { echo "test vectors missing: vector tests would silently skip"; exit 1; } + + # verify rather than test so the nap-it module runs as part of the reactor. + # Note: every class in nap-it is named *Test, so surefire runs them all and the + # configured failsafe plugin currently matches nothing. The interop coverage is real + # but it arrives through surefire, not failsafe. + - name: Build and test + run: mvn -B -ntp verify + + # Publish the reports so skip counts stay reviewable rather than hidden behind a check. + - name: Upload test reports + if: always() + uses: actions/upload-artifact@v4 + with: + name: surefire-reports-java-${{ matrix.java }} + path: | + **/target/surefire-reports/** + **/target/failsafe-reports/** + if-no-files-found: warn + + dependency-check: + name: OWASP Dependency-Check + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-java@v4 + with: + java-version: '21' + distribution: temurin + cache: maven + + # The dependency tree is managed by imani-bom, so transitive CVEs arrive without any + # visible version bump in this repository. Jackson matters directly: Nip98Validator + # and DefaultNapServer parse attacker-controlled JSON on the unauthenticated path. + # + # CVSS >= 7 is the opening gate. A scanner developers learn to ignore has negative + # value, so tighten only once the baseline is known clean. + # + # The NVD API key is passed only when the secret is actually set. Passing an empty + # -DnvdApiKey= is not equivalent to omitting it: the plugin rejects it outright with + # "Invalid API Key, length of 0", verified locally against dependency-check 13.0.0. + # Without a key the NVD feed is rate limited to roughly one request per six seconds, + # so the first run is slow rather than broken. Set the NVD_API_KEY repository secret + # (free from https://nvd.nist.gov/developers/request-an-api-key) to make it fast. + - name: Run Dependency-Check + env: + NVD_API_KEY: ${{ secrets.NVD_API_KEY }} + run: | + if [ -n "$NVD_API_KEY" ]; then + mvn -B -ntp org.owasp:dependency-check-maven:check \ + -DfailBuildOnCVSS=7 -DnvdApiKey="$NVD_API_KEY" + else + echo "NVD_API_KEY is not set: falling back to the rate limited public NVD feed." + mvn -B -ntp org.owasp:dependency-check-maven:check -DfailBuildOnCVSS=7 + fi + + - name: Upload Dependency-Check report + if: always() + uses: actions/upload-artifact@v4 + with: + name: dependency-check-report + path: '**/target/dependency-check-report.html' + if-no-files-found: warn From f65e79ebb55193dfe410874ab31e6597b1634c14 Mon Sep 17 00:00:00 2001 From: tcheeric Date: Thu, 24 Sep 2026 15:16:37 +0100 Subject: [PATCH 07/18] ci: stop Dependency-Check blocking on pre-existing CVEs, cache the NVD 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 --- .github/workflows/ci.yml | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a51a6e1..298d8b9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -85,6 +85,16 @@ jobs: dependency-check: name: OWASP Dependency-Check runs-on: ubuntu-latest + # Reports without blocking, for now. Verified locally on 2026-09-24 against + # dependency-check 13.0.0: -DfailBuildOnCVSS=7 FAILS today on the current tree, + # flagging 12 CVEs in spring-core 6.2.19 and 2 in spring-security-core 6.5.11. + # + # Making this blocking immediately would turn every PR red for a pre-existing + # condition that no PR author introduced or can fix, which is the fastest way to + # teach a team to ignore a scanner. The correct order is: land the signal, triage + # the 14 findings (upgrade through imani-bom, or suppress the CPE false positives + # that spring-core is well known for), then remove this line so the gate bites. + continue-on-error: true steps: - uses: actions/checkout@v4 - uses: actions/setup-java@v4 @@ -93,6 +103,18 @@ jobs: distribution: temurin cache: maven + # Building the NVD database from scratch took 12 minutes locally. Caching it keeps + # this job to the scan itself on subsequent runs. The key rolls per run so the cache + # is refreshed, and restore-keys lets each run start from the previous database and + # apply only the delta. + - name: Cache the NVD database + uses: actions/cache@v4 + with: + path: ~/.m2/repository/org/owasp/dependency-check-data + key: nvd-${{ runner.os }}-${{ github.run_id }} + restore-keys: | + nvd-${{ runner.os }}- + # The dependency tree is managed by imani-bom, so transitive CVEs arrive without any # visible version bump in this repository. Jackson matters directly: Nip98Validator # and DefaultNapServer parse attacker-controlled JSON on the unauthenticated path. From 40e3591f9d59257e4b38ed66734f2fb1cf23d136 Mon Sep 17 00:00:00 2001 From: tcheeric Date: Thu, 24 Sep 2026 15:22:50 +0100 Subject: [PATCH 08/18] ci: add CodeQL analysis for Java 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 --- .github/workflows/codeql.yml | 64 ++++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) create mode 100644 .github/workflows/codeql.yml diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml new file mode 100644 index 0000000..71772f6 --- /dev/null +++ b/.github/workflows/codeql.yml @@ -0,0 +1,64 @@ +name: CodeQL + +# Separate from ci.yml deliberately. CodeQL takes minutes rather than seconds, and +# a weekly schedule only makes sense on its own workflow; folding it into the PR +# job would make every pull request wait on an analysis that rarely changes its +# answer between commits. +on: + push: + branches: [main, master, develop] + pull_request: + branches: [main, master, develop] + schedule: + # Monday 07:00 UTC. The scheduled run is the one that matters: it re-analyses + # unchanged code against updated queries, which is how a newly published + # vulnerability class gets found in code nobody has touched. + - cron: '0 7 * * 1' + +concurrency: + group: codeql-${{ github.ref }} + cancel-in-progress: true + +jobs: + analyze: + name: Analyze (${{ matrix.language }}) + runs-on: ubuntu-latest + permissions: + # Least privilege: the analysis needs to read the tree and write findings, + # and nothing else. + actions: read + contents: read + security-events: write + strategy: + fail-fast: false + matrix: + language: ['java-kotlin'] + + steps: + - uses: actions/checkout@v4 + + # Autobuild runs Maven, which needs a JDK matching the one the project + # targets. Without this the analysis builds against whatever the runner + # ships and can fail on a Java 21 language feature. + - uses: actions/setup-java@v4 + with: + java-version: '21' + distribution: temurin + cache: maven + + - name: Initialize CodeQL + uses: github/codeql-action/init@v3 + with: + languages: ${{ matrix.language }} + # security-extended over the default pack. The default set is tuned to + # keep false positives near zero on any repository; this one is an + # authentication library, so a quieter scan is the wrong trade. + queries: security-extended + + - name: Autobuild + uses: github/codeql-action/autobuild@v3 + + - name: Perform CodeQL analysis + uses: github/codeql-action/analyze@v3 + with: + category: /language:${{ matrix.language }} From c0c37c865b79c34a1877d2a98de8670327c74707 Mon Sep 17 00:00:00 2001 From: tcheeric Date: Thu, 24 Sep 2026 15:37:09 +0100 Subject: [PATCH 09/18] test(it): make the TypeScript checkout location configurable 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 --- .github/workflows/ci.yml | 25 ++++++++++++------- .../nap/it/TypeScriptClientInteropTest.java | 22 +++++++++++++++- 2 files changed, 37 insertions(+), 10 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 298d8b9..8fdb6a4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -42,34 +42,41 @@ jobs: # interop suite reports green while asserting nothing, which is precisely the silent # regression this CI exists to catch. # - # The clone target is not configurable: TypeScriptClientInteropTest hard-codes - # Path.of(user.home, "IdeaProjects", "nap") with no override property, so the path - # below has to match it exactly. OfficialTestVectorsTest does accept - # -Dnap.test-vectors.dir, but pointing both at one checkout keeps them consistent. + # Cloned into the workspace rather than $HOME: both tests now take an explicit + # directory property, so the location is chosen here instead of being dictated by a + # hard-coded home-relative path in test source. - name: Check out the TypeScript reference implementation run: | - git clone --depth 1 https://github.com/tcheeric/nap.git "$HOME/IdeaProjects/nap" + git clone --depth 1 https://github.com/tcheeric/nap.git "$GITHUB_WORKSPACE/../nap-ts" + echo "NAP_TS_DIR=$(cd "$GITHUB_WORKSPACE/../nap-ts" && pwd)" >> "$GITHUB_ENV" # The interop test looks for node_modules/.bin/tsx inside that checkout. Installing # dependencies is what flips the test from skipped to actually executed. - name: Install TypeScript client dependencies - run: npm ci --prefix "$HOME/IdeaProjects/nap" + run: npm ci --prefix "$NAP_TS_DIR" # A skipped test is indistinguishable from a passing one in a green check, so assert # the interop preconditions explicitly and fail loudly when they are missing. - name: Assert the interop toolchain is present run: | - test -x "$HOME/IdeaProjects/nap/node_modules/.bin/tsx" \ + test -x "$NAP_TS_DIR/node_modules/.bin/tsx" \ || { echo "tsx missing: the interop test would silently skip"; exit 1; } - test -f "$HOME/IdeaProjects/nap/packages/nap-core/test-vectors/nip98.json" \ + test -f "$NAP_TS_DIR/packages/nap-core/test-vectors/nip98.json" \ || { echo "test vectors missing: vector tests would silently skip"; exit 1; } # verify rather than test so the nap-it module runs as part of the reactor. # Note: every class in nap-it is named *Test, so surefire runs them all and the # configured failsafe plugin currently matches nothing. The interop coverage is real # but it arrives through surefire, not failsafe. + # + # Both interop directories are passed explicitly. Without them the tests fall back to + # a home-relative default that does not exist on a runner, and assumeTrue would skip + # them silently rather than fail. - name: Build and test - run: mvn -B -ntp verify + run: | + mvn -B -ntp verify \ + -Dnap.typescript.dir="$NAP_TS_DIR" \ + -Dnap.test-vectors.dir="$NAP_TS_DIR/packages/nap-core/test-vectors" # Publish the reports so skip counts stay reviewable rather than hidden behind a check. - name: Upload test reports diff --git a/nap-it/src/test/java/xyz/tcheeric/nap/it/TypeScriptClientInteropTest.java b/nap-it/src/test/java/xyz/tcheeric/nap/it/TypeScriptClientInteropTest.java index 373f3ce..7bc3046 100644 --- a/nap-it/src/test/java/xyz/tcheeric/nap/it/TypeScriptClientInteropTest.java +++ b/nap-it/src/test/java/xyz/tcheeric/nap/it/TypeScriptClientInteropTest.java @@ -51,10 +51,30 @@ private static String derivePubkeyHex(String privateKeyHex) { } } + /** + * Where the TypeScript reference implementation is checked out. + * + *

{@code -Dnap.typescript.dir} first, then the historical + * {@code ~/IdeaProjects/nap} default so an existing developer checkout keeps working. + * + *

The property matters because of what happens when this path is wrong: the caller + * {@code assumeTrue}s on the toolchain, and an assumption skips rather than fails. + * A home-relative default on a CI runner therefore produces a green build in which the one + * test asserting cross-implementation agreement asserted nothing at all. {@code OfficialTestVectorsTest} + * already takes {@code -Dnap.test-vectors.dir} for the same reason; this brings the two into line. + */ + private static Path typescriptCheckoutRoot() { + String override = System.getProperty("nap.typescript.dir"); + if (override != null && !override.isBlank()) { + return Path.of(override.trim()); + } + return Path.of(System.getProperty("user.home"), "IdeaProjects", "nap"); + } + // Authenticates against the Java server by spawning the real TypeScript client package through tsx @Test void typescriptClientAuthenticatesAgainstJavaServer() throws Exception { - Path napRoot = Path.of(System.getProperty("user.home"), "IdeaProjects", "nap"); + Path napRoot = typescriptCheckoutRoot(); Path tsxBinary = napRoot.resolve("node_modules/.bin/tsx"); assumeTrue(Files.exists(tsxBinary), "tsx toolchain not available — skipping interop test"); From 7f32eb9b86c0b289701cf963a49ef07ada6ea230 Mon Sep 17 00:00:00 2001 From: tcheeric Date: Thu, 24 Sep 2026 15:39:06 +0100 Subject: [PATCH 10/18] ci: correct the Dependency-Check note, the 14 findings were not reproducible 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. --- .github/workflows/ci.yml | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8fdb6a4..4bf5b69 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -92,15 +92,21 @@ jobs: dependency-check: name: OWASP Dependency-Check runs-on: ubuntu-latest - # Reports without blocking, for now. Verified locally on 2026-09-24 against - # dependency-check 13.0.0: -DfailBuildOnCVSS=7 FAILS today on the current tree, - # flagging 12 CVEs in spring-core 6.2.19 and 2 in spring-security-core 6.5.11. + # Reports without blocking, for now, even though the tree currently scans clean. # - # Making this blocking immediately would turn every PR red for a pre-existing - # condition that no PR author introduced or can fix, which is the fastest way to - # teach a team to ignore a scanner. The correct order is: land the signal, triage - # the 14 findings (upgrade through imani-bom, or suppress the CPE false positives - # that spring-core is well known for), then remove this line so the gate bites. + # An earlier note here claimed 14 findings in spring-core and spring-security-core. + # That was not reproducible: re-running dependency-check 13.0.0 on 2026-09-24 with a + # populated NVD database (232 MB, updated during the run) reports BUILD SUCCESS at + # -DfailBuildOnCVSS=7, and also at -DfailBuildOnCVSS=0, which fails on a finding of + # any severity. The likely cause of the original number was a run that could not + # update the NVD feed, since an empty NVD_API_KEY is rejected outright rather than + # ignored. + # + # It stays non-blocking anyway, for a reason that does not depend on the count: this + # job has never executed on a runner. Making an unproven scanner a merge gate on its + # first outing risks blocking every PR on an environment problem rather than a real + # finding, which is how a team learns to click past a scanner. Remove this line once + # the job has run green in CI a few times, so the gate actually bites. continue-on-error: true steps: - uses: actions/checkout@v4 From aae7cf666c4f18338951785d9d500185059bd043 Mon Sep 17 00:00:00 2001 From: tcheeric Date: Thu, 24 Sep 2026 15:46:35 +0100 Subject: [PATCH 11/18] ci: assert every interop vector file, not just one 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 --- .github/workflows/ci.yml | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4bf5b69..ef9988d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -57,12 +57,18 @@ jobs: # A skipped test is indistinguishable from a passing one in a green check, so assert # the interop preconditions explicitly and fail loudly when they are missing. + # + # Every vector file is checked, not just one. OfficialTestVectorsTest loads three + # (payload-hash.json, nip98.json, flow.json) and assumeTrue's on each independently, + # so a checkout missing only one of them would skip that test and still report green. - name: Assert the interop toolchain is present run: | test -x "$NAP_TS_DIR/node_modules/.bin/tsx" \ || { echo "tsx missing: the interop test would silently skip"; exit 1; } - test -f "$NAP_TS_DIR/packages/nap-core/test-vectors/nip98.json" \ - || { echo "test vectors missing: vector tests would silently skip"; exit 1; } + for vector in payload-hash.json nip98.json flow.json; do + test -f "$NAP_TS_DIR/packages/nap-core/test-vectors/$vector" \ + || { echo "test vector $vector missing: that vector test would silently skip"; exit 1; } + done # verify rather than test so the nap-it module runs as part of the reactor. # Note: every class in nap-it is named *Test, so surefire runs them all and the From 04019f7272723e93288cb2c3aa6d530ae8f51366 Mon Sep 17 00:00:00 2001 From: tcheeric Date: Thu, 24 Sep 2026 15:56:43 +0100 Subject: [PATCH 12/18] test: cover the sweep under contention and logout idempotency 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. --- .../server/store/SweepConcurrencyTest.java | 83 +++++++++++++++++++ .../controller/NapAuthControllerTest.java | 34 ++++++++ 2 files changed, 117 insertions(+) create mode 100644 nap-server/src/test/java/xyz/tcheeric/nap/server/store/SweepConcurrencyTest.java diff --git a/nap-server/src/test/java/xyz/tcheeric/nap/server/store/SweepConcurrencyTest.java b/nap-server/src/test/java/xyz/tcheeric/nap/server/store/SweepConcurrencyTest.java new file mode 100644 index 0000000..5d4659f --- /dev/null +++ b/nap-server/src/test/java/xyz/tcheeric/nap/server/store/SweepConcurrencyTest.java @@ -0,0 +1,83 @@ +package xyz.tcheeric.nap.server.store; + +import org.junit.jupiter.api.Test; +import xyz.tcheeric.nap.core.SessionRecord; +import java.time.Clock; +import java.time.Instant; +import java.time.ZoneOffset; +import java.util.List; +import java.util.concurrent.*; +import static org.assertj.core.api.Assertions.assertThat; + +/** + * The sweep runs on live traffic, so it races every writer. + * + *

Eviction touches four index maps that are views on one record, and it does so while + * other threads are inserting. Getting that wrong does not throw: it silently drops a live + * session from one index while leaving it in another, which presents much later as a + * session that authenticates through the cookie and then cannot be found by id. Worth a + * test that actually contends rather than reasoning about ConcurrentHashMap semantics. + */ +class SweepConcurrencyTest { + private static final long NOW = 1_700_000_000L; + + private static final class MutClock extends Clock { + volatile long s; + MutClock(long s){this.s=s;} + @Override public java.time.ZoneId getZone(){return ZoneOffset.UTC;} + @Override public Clock withZone(java.time.ZoneId z){return this;} + @Override public Instant instant(){return Instant.ofEpochSecond(s);} + } + + /** + * Concurrent creates while sweeping must not lose a live session or leave a stale index. + * + *

Asserts reachability through both {@code getBySessionId} and {@code getByAccessToken}, + * because the failure mode this guards against is the two disagreeing. + */ + @Test + void concurrentCreatesDuringSweepKeepIndexesConsistent() throws Exception { + MutClock clock = new MutClock(NOW); + InMemorySessionStore store = new InMemorySessionStore(clock); + + // Seed dead sessions. + for (int i = 0; i < 200; i++) { + store.createForChallenge(new SessionRecord( + "dead"+i, "c-dead"+i, "a-dead"+i, "npub", "a".repeat(64), + List.of(), List.of(), NOW, NOW, NOW+10, NOW+10, null, null, null, null, null, null)); + } + clock.s = NOW + 1000; // everything above is now dead + + // Hammer createForChallenge from several threads while sweeps fire. + ExecutorService pool = Executors.newFixedThreadPool(8); + CountDownLatch go = new CountDownLatch(1); + for (int t = 0; t < 8; t++) { + final int tid = t; + pool.submit(() -> { + go.await(); + for (int i = 0; i < 50; i++) { + String id = "live-" + tid + "-" + i; + store.createForChallenge(new SessionRecord( + id, "c-"+id, "a-"+id, "npub", "a".repeat(64), + List.of(), List.of(), clock.s, clock.s, clock.s+3600, clock.s+3600, + null, null, null, null, null, null)); + } + return null; + }); + } + go.countDown(); + pool.shutdown(); + assertThat(pool.awaitTermination(30, TimeUnit.SECONDS)).isTrue(); + + // Every live session must be reachable through BOTH indexes. + for (int t = 0; t < 8; t++) { + for (int i = 0; i < 50; i++) { + String id = "live-" + t + "-" + i; + assertThat(store.getBySessionId(id)).as("by id: " + id).isPresent(); + assertThat(store.getByAccessToken("a-" + id)).as("by token: " + id).isPresent(); + } + } + // And the dead ones are gone from the primary index. + assertThat(store.size()).isEqualTo(400); + } +} diff --git a/nap-spring/src/test/java/xyz/tcheeric/nap/spring/controller/NapAuthControllerTest.java b/nap-spring/src/test/java/xyz/tcheeric/nap/spring/controller/NapAuthControllerTest.java index 78471d4..eadf89a 100644 --- a/nap-spring/src/test/java/xyz/tcheeric/nap/spring/controller/NapAuthControllerTest.java +++ b/nap-spring/src/test/java/xyz/tcheeric/nap/spring/controller/NapAuthControllerTest.java @@ -515,6 +515,40 @@ void refresh_retiresThePreviousCookieValue() { assertThat(cookie.getValue()).isNotEqualTo("access-before"); } + /** + * Logout stays idempotent after the switch to access-token lookup. + * + *

getByAccessToken filters revoked sessions, so the second call finds nothing to + * revoke. It must still clear the cookie and answer 204: a client clearing local state + * should never have to distinguish "logged out" from "was already logged out", and a + * double-submit or a retry is ordinary. + */ + @Test + void logout_isIdempotent() { + long now = Instant.now().getEpochSecond(); + SessionRecord live = SessionRecord.create( + "sid-twice", "chal-t", "access-twice", + "npub-t", "a".repeat(64), + List.of(), List.of(), + now - 60, now - 60, now + 900, now + 43200 + ); + sessionStore.createForChallenge(live); + + for (int attempt = 1; attempt <= 2; attempt++) { + MockHttpServletRequest request = new MockHttpServletRequest("POST", "/api/v1/auth/logout"); + request.setCookies(new Cookie("merchant_session", "access-twice")); + MockHttpServletResponse response = new MockHttpServletResponse(); + + ResponseEntity result = controller().logout(request, response); + + assertThat(result.getStatusCode().value()).as("attempt " + attempt).isEqualTo(204); + Cookie cookie = response.getCookie("merchant_session"); + assertThat(cookie).as("cookie cleared on attempt " + attempt).isNotNull(); + assertThat(cookie.getMaxAge()).isEqualTo(0); + } + assertThat(sessionStore.getBySessionId("sid-twice")).isEmpty(); + } + /** Logout resolves the cookie to a session before revoking, so a 204 really did revoke. */ @Test void logout_revokesTheSessionTheAccessTokenNames() { From 9f8a258081dafdf8605289c0f8935c379bf872d8 Mon Sep 17 00:00:00 2001 From: tcheeric Date: Thu, 24 Sep 2026 15:58:16 +0100 Subject: [PATCH 13/18] docs(changelog): add the Unreleased security section 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. --- CHANGELOG.md | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 87 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5c8d109..0feea3c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,93 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +Security fixes from an application-security audit. **Three of these change behaviour**, and +one of them ends every live session on deploy. `NapProperties` is a record and gained a +component, so its canonical constructor arity changed again. + +### Security + +- **The session cookie now carries the access token, not the session id** (#27). The cookie + was set with `session.sessionId()` and every read looked it 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. + + 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, so anyone with log + access held live credentials. Rotation never reached the credential, because + `rotateRefreshToken()` mints a new access token while the session id it replaced 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: `nap` (TypeScript) writes `access_token` into the cookie and + authenticates with `getByAccessToken()`, so a cookie minted by one server could not be + read by the other, which `nap-it` covers as a supported configuration. + + **Breaking:** live sessions hold a cookie that no longer authenticates, so deploying logs + everyone out once. No fallback to `getBySessionId()` is provided, deliberately: accepting + the old credential would keep the issue alive for as long as the fallback existed. + +- **The NIP-98 proof is read from `Authorization` and nowhere else** (#28). A fallback read + it from a `proof` field in the JSON body when the header was absent. It was JVM-only with + no RFC or TypeScript counterpart; it asked the NIP-98 `payload` hash to cover a field + containing the hash; it parsed attacker-controlled bytes a second time with different + semantics and a blanket catch, which is a parser-differential surface; and a credential in + a body is logged by anything that logs request payloads, which is why `/auth/refresh` + takes its token from a header. + +- **`protected-path-prefixes` fails closed** (#29). + `nap.require-annotation-on-protected-paths` now defaults to `true`, so a handler under a + protected prefix that declares no NAP annotation is refused rather than served to anyone. + `@PublicEndpoint` states in the source what omission used to state only by accident. + + The filter and the interceptor also 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 share one `pathWithinApplication()` helper. + The three unauthenticated fall-through branches emit `nap_guard_no_session` with a reason, + mirroring the TypeScript guards, so an operator can tell "nobody is calling this" from + "everybody is, without a session". + + **Breaking:** an application relying on the previous default, with protected prefixes + configured and handlers deliberately unannotated, will see `500` until those handlers + declare `@PublicEndpoint` or a NAP annotation. + +- **No implicit allow-all authorization** (#30). The auto-configuration defaulted to + `AllowAllAclResolver`, which authorizes every principal who can prove key control and + reported nothing, so an operator could wire NAP, log in with their own key, see a session, + and ship with the authorization layer a no-op. There is now no default: supply an + `AclResolver`, or set `nap.allow-all-principals=true` to ask for the old behaviour + deliberately. The in-memory stores remain the default for local development but warn at + startup, naming the consequence that is a security one rather than only an availability + one, since `revokeByPrincipal()` reaches a single node. + + **Breaking:** an application relying on the implicit allow-all will fail to start, with a + message naming the property and the alternative. + +- **The in-memory stores evict** (#31). Both only ever grew: states were rewritten in place + and revocations stamped, but nothing was removed, and both maps are filled by + unauthenticated traffic. Retention bounds are deliberate rather than plain expiry. A + challenge is kept until `resultCacheUntil` when set, 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 `refreshExpiresAt`, whichever is later, because + `getByRefreshToken` answers for revoked sessions so a replay stays detectable, and + evicting earlier would turn a detected reuse into an unknown token. + +### Added + +- **CI** (#32). The repository had no `.github` directory: nothing built, tested, or scanned + on a pull request. Both interop tests call `assumeTrue` on a hard-coded + `~/IdeaProjects/nap`, and an assumption *skips* rather than fails, so a naive job would + have reported green while the one test asserting cross-implementation agreement asserted + nothing. The workflow clones the reference implementation, installs it, and asserts every + vector file is present before running. `TypeScriptClientInteropTest` now takes + `-Dnap.typescript.dir` rather than hard-coding a home-relative path. + +- CodeQL analysis, and OWASP Dependency-Check reporting without blocking until it has run + green on a runner. + ## [0.8.0] - 2026-09-06 Minor rather than patch: `NapProperties` is a record and gained two components, so its canonical From f8babb5d227e910e27a979eca14dbf60d041eef2 Mon Sep 17 00:00:00 2001 From: tcheeric Date: Thu, 24 Sep 2026 16:15:11 +0100 Subject: [PATCH 14/18] chore(release): 0.9.0 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. --- CHANGELOG.md | 13 +++++++++---- README.md | 4 ++-- nap-client/pom.xml | 2 +- nap-core/pom.xml | 2 +- nap-it/pom.xml | 2 +- nap-jdbc/pom.xml | 2 +- nap-server/pom.xml | 2 +- nap-spring/pom.xml | 2 +- pom.xml | 2 +- 9 files changed, 18 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0feea3c..a7d3a4b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,11 +5,16 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). -## [Unreleased] +## [0.9.0] - 2026-09-24 -Security fixes from an application-security audit. **Three of these change behaviour**, and -one of them ends every live session on deploy. `NapProperties` is a record and gained a -component, so its canonical constructor arity changed again. +Minor rather than patch: three of these change behaviour, and one ends every live session +on deploy. `NapProperties` is a record and gained a component, so its canonical constructor +arity changed again. + +Read the three **Breaking** notes below before upgrading. In order of what they cost: the +cookie switch logs every signed-in user out once, the ACL default fails startup until an +`AclResolver` is supplied or the opt-in property is set, and the protected-path default +turns an unannotated handler under a protected prefix into a `500`. ### Security diff --git a/README.md b/README.md index 628eee5..62e60cc 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,7 @@ Java implementation of the **Nostr Authentication Protocol (NAP) v2** — challe login with a NIP-98 signed event, server-side sessions, rotating refresh tokens, and role/permission ACLs. Framework-agnostic core, optional Spring Boot adapter. -Requires Java 21. Current version: `0.6.2`. +Requires Java 21. Current version: `0.9.0`. Versions are managed by `imani-bom`; consumers that import it should omit the version entirely. 0.6.0 added the authorization layer (`AclResolver`, @@ -44,7 +44,7 @@ returns `429` with `Retry-After`. xyz.tcheeric nap-spring - 0.6.2 + 0.9.0 ``` diff --git a/nap-client/pom.xml b/nap-client/pom.xml index 556181b..6a69f50 100644 --- a/nap-client/pom.xml +++ b/nap-client/pom.xml @@ -7,7 +7,7 @@ xyz.tcheeric nap-java - 0.8.0 + 0.9.0 nap-client diff --git a/nap-core/pom.xml b/nap-core/pom.xml index de5c478..5bfcbf8 100644 --- a/nap-core/pom.xml +++ b/nap-core/pom.xml @@ -7,7 +7,7 @@ xyz.tcheeric nap-java - 0.8.0 + 0.9.0 nap-core diff --git a/nap-it/pom.xml b/nap-it/pom.xml index b68f162..a300111 100644 --- a/nap-it/pom.xml +++ b/nap-it/pom.xml @@ -7,7 +7,7 @@ xyz.tcheeric nap-java - 0.8.0 + 0.9.0 nap-it diff --git a/nap-jdbc/pom.xml b/nap-jdbc/pom.xml index e6743fe..284e2df 100644 --- a/nap-jdbc/pom.xml +++ b/nap-jdbc/pom.xml @@ -7,7 +7,7 @@ xyz.tcheeric nap-java - 0.8.0 + 0.9.0 nap-jdbc diff --git a/nap-server/pom.xml b/nap-server/pom.xml index 8f5f0d5..02d3f88 100644 --- a/nap-server/pom.xml +++ b/nap-server/pom.xml @@ -7,7 +7,7 @@ xyz.tcheeric nap-java - 0.8.0 + 0.9.0 nap-server diff --git a/nap-spring/pom.xml b/nap-spring/pom.xml index d13510d..ec63e1f 100644 --- a/nap-spring/pom.xml +++ b/nap-spring/pom.xml @@ -7,7 +7,7 @@ xyz.tcheeric nap-java - 0.8.0 + 0.9.0 nap-spring diff --git a/pom.xml b/pom.xml index d7e9138..855ff33 100644 --- a/pom.xml +++ b/pom.xml @@ -6,7 +6,7 @@ xyz.tcheeric nap-java - 0.8.0 + 0.9.0 pom NAP Java Nostr Authentication Protocol (NAP) v2 Java library — standalone, framework-agnostic implementation with Spring Boot adapter From 0637d974c23be9fa0e932bc9dd87f646a1ec2b02 Mon Sep 17 00:00:00 2001 From: tcheeric Date: Thu, 24 Sep 2026 16:30:03 +0100 Subject: [PATCH 15/18] security: restore cookie defaults lost under partial binding 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. --- .github/workflows/ci.yml | 25 +++-- CHANGELOG.md | 20 ++++ .../nap/core/Nip98ValidatorStructureTest.java | 104 ++++++++++++++++++ .../nap/spring/config/NapProperties.java | 14 ++- .../nap/spring/filter/NapSessionFilter.java | 25 ++++- .../config/NapCookiePropertiesTest.java | 95 ++++++++++++++++ 6 files changed, 271 insertions(+), 12 deletions(-) create mode 100644 nap-core/src/test/java/xyz/tcheeric/nap/core/Nip98ValidatorStructureTest.java create mode 100644 nap-spring/src/test/java/xyz/tcheeric/nap/spring/config/NapCookiePropertiesTest.java diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ef9988d..edcc7d8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -141,12 +141,20 @@ jobs: # CVSS >= 7 is the opening gate. A scanner developers learn to ignore has negative # value, so tighten only once the baseline is known clean. # - # The NVD API key is passed only when the secret is actually set. Passing an empty - # -DnvdApiKey= is not equivalent to omitting it: the plugin rejects it outright with - # "Invalid API Key, length of 0", verified locally against dependency-check 13.0.0. - # Without a key the NVD feed is rate limited to roughly one request per six seconds, - # so the first run is slow rather than broken. Set the NVD_API_KEY repository secret - # (free from https://nvd.nist.gov/developers/request-an-api-key) to make it fast. + # An NVD API key is required, not merely an optimisation. An earlier version of this + # comment claimed a missing key meant "slow rather than broken"; the first run on a + # runner disproved that, failing in 52s with "Invalid API Key, length of 0 too short". + # Reproduced locally against an empty data directory: the keyless run fails the same + # way whether NVD_API_KEY is empty or entirely unset, so it is the absent key rather + # than the empty string that breaks it. The message is misleading, since it describes + # key masking rather than the rejected request underneath. The legacy 1.1 JSON feeds + # are not a way around it either: nvd.nist.gov now answers those with 403. + # + # So the job skips with an explanation instead of failing red on every PR until the + # secret exists. A permanently red check that everyone knows to ignore trains people + # to ignore the scanner, which is worse than not running it. Get a free key at + # https://nvd.nist.gov/developers/request-an-api-key and add it as the NVD_API_KEY + # repository secret, and this becomes a real scan on the next run. - name: Run Dependency-Check env: NVD_API_KEY: ${{ secrets.NVD_API_KEY }} @@ -155,8 +163,9 @@ jobs: mvn -B -ntp org.owasp:dependency-check-maven:check \ -DfailBuildOnCVSS=7 -DnvdApiKey="$NVD_API_KEY" else - echo "NVD_API_KEY is not set: falling back to the rate limited public NVD feed." - mvn -B -ntp org.owasp:dependency-check-maven:check -DfailBuildOnCVSS=7 + echo "::warning title=Dependency-Check skipped::NVD_API_KEY secret is not set, so the NVD database cannot be built and no scan ran. Add the secret (free key: https://nvd.nist.gov/developers/request-an-api-key) to enable this job." + echo "Skipping the scan: without an API key the NVD update fails outright rather" + echo "than running slowly, so there is nothing to scan against." fi - name: Upload Dependency-Check report diff --git a/CHANGELOG.md b/CHANGELOG.md index a7d3a4b..8c57629 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,6 +46,26 @@ turns an unannotated handler under a protected prefix into a `500`. a body is logged by anything that logs request payloads, which is why `/auth/refresh` takes its token from a header. +- **A partial `nap.cookie.*` config no longer strips `HttpOnly` and `Secure`.** `httpOnly` + and `secure` were primitive `boolean` on the `CookieProperties` record, so Spring bound + them as `false` the moment 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 why no existing test caught it: a hand-constructed + `NapProperties` took that branch and looked correct. Both are now boxed `Boolean` + defaulting to `true`, so absent is distinguishable from explicitly `false`, and an + explicit `false` is still honoured for local http development. + + Found by CodeQL on the first CI run of this branch, not by the audit. It is the same + defect as `nap` (TypeScript) #34, in the language where the type system hid it. + +- **The guard log escapes line breaks in the request path** (`NapSessionFilter`). The path + is the one attacker-chosen field on that line, and a newline lets a caller forge a second + entry that looks like ours. Not reachable through Tomcat, which leaves `%0D%0A` encoded + through `getRequestURI()` (verified, not assumed), but that is the container's guarantee + rather than this filter's, and it does not survive a different container or a rewritten + URI. + - **`protected-path-prefixes` fails closed** (#29). `nap.require-annotation-on-protected-paths` now defaults to `true`, so a handler under a protected prefix that declares no NAP annotation is refused rather than served to anyone. diff --git a/nap-core/src/test/java/xyz/tcheeric/nap/core/Nip98ValidatorStructureTest.java b/nap-core/src/test/java/xyz/tcheeric/nap/core/Nip98ValidatorStructureTest.java new file mode 100644 index 0000000..48db003 --- /dev/null +++ b/nap-core/src/test/java/xyz/tcheeric/nap/core/Nip98ValidatorStructureTest.java @@ -0,0 +1,104 @@ +package xyz.tcheeric.nap.core; + +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.List; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Structural guard over {@code Nip98Validator.verifyNip98Completion}. + * + *

CodeQL's {@code java/user-controlled-bypass} rule flags the early returns in that method, + * because each is a branch on attacker-controlled input that decides whether signature + * verification runs. Those alerts were dismissed as false positives on one specific ground: + * every early return produces a {@code failure}, so a caller choosing the branch chooses which + * rejection they receive and never whether verification happens. + * + *

That is a claim about shape, and a dismissed alert does not re-open itself when the shape + * changes. A future guard clause that returns {@code success} early, say a fast path for a + * cached or pre-validated header, would make the original alert correct again and silently + * inherit the dismissal. This test is what turns the reasoning behind that dismissal into + * something enforced. + * + *

It reads the source rather than exercising behaviour because the property is structural: + * "there is exactly one success return, and signature verification precedes it" is not + * observable from any single call. The behavioural cases live in {@link Nip98ValidatorTest}, + * including a tampered signature being refused. + */ +class Nip98ValidatorStructureTest { + + private static final Path SOURCE = Path.of( + "src/main/java/xyz/tcheeric/nap/core/Nip98Validator.java"); + + /** + * The body of {@code verifyNip98Completion}, from its signature to the first line that is + * flush against the class indent, which is where the next member begins. + */ + private static List methodBody() throws IOException { + List lines = Files.readAllLines(SOURCE); + List body = new ArrayList<>(); + boolean inMethod = false; + for (String line : lines) { + if (line.contains("public static Nip98ValidationResult verifyNip98Completion(")) { + inMethod = true; + continue; + } + if (inMethod) { + if (line.equals(" }")) { + break; + } + body.add(line); + } + } + assertThat(inMethod) + .as("verifyNip98Completion not found in %s: this test is stale", SOURCE) + .isTrue(); + assertThat(body).as("method body parsed as empty").isNotEmpty(); + return body; + } + + @Test + @DisplayName("every early return is a failure: exactly one success return exists") + void onlyOneSuccessReturn() throws IOException { + List successReturns = methodBody().stream() + .filter(line -> line.contains("return Nip98ValidationResult.success(")) + .toList(); + + assertThat(successReturns) + .as("A second success return would mean a caller-controlled branch can reach " + + "an authenticated result without passing every check. That is the " + + "condition the dismissed CodeQL alerts assumed was impossible.") + .hasSize(1); + } + + @Test + @DisplayName("signature verification precedes the success return") + void signatureIsVerifiedBeforeSuccess() throws IOException { + List body = methodBody(); + + int verifyAt = -1; + int successAt = -1; + for (int i = 0; i < body.size(); i++) { + String line = body.get(i); + if (verifyAt < 0 && line.contains("verifySignature(event)")) { + verifyAt = i; + } + if (successAt < 0 && line.contains("return Nip98ValidationResult.success(")) { + successAt = i; + } + } + + assertThat(verifyAt).as("verifySignature(event) call not found").isNotNegative(); + assertThat(successAt).as("success return not found").isNotNegative(); + assertThat(verifyAt) + .as("Signature verification must precede the only success return, otherwise an " + + "unsigned or forged event can be accepted.") + .isLessThan(successAt); + } +} diff --git a/nap-spring/src/main/java/xyz/tcheeric/nap/spring/config/NapProperties.java b/nap-spring/src/main/java/xyz/tcheeric/nap/spring/config/NapProperties.java index 822d5e4..55abdb8 100644 --- a/nap-spring/src/main/java/xyz/tcheeric/nap/spring/config/NapProperties.java +++ b/nap-spring/src/main/java/xyz/tcheeric/nap/spring/config/NapProperties.java @@ -92,7 +92,7 @@ public record NapProperties( // only by omission. if (requireAnnotationOnProtectedPaths == null) requireAnnotationOnProtectedPaths = Boolean.TRUE; if (allowAllPrincipals == null) allowAllPrincipals = Boolean.FALSE; - if (cookie == null) cookie = new CookieProperties("merchant_session", true, true, "Lax", "/", "", 0); + if (cookie == null) cookie = new CookieProperties(null, null, null, null, null, null, 0); // Default cookie maxAge to the (effective) absolute session cap so the // browser retains the cookie for the full server-side lifetime. if (cookie.maxAgeSeconds() <= 0) { @@ -105,8 +105,8 @@ public record NapProperties( public record CookieProperties( String name, - boolean httpOnly, - boolean secure, + Boolean httpOnly, + Boolean secure, String sameSite, String path, String domain, @@ -114,6 +114,14 @@ public record CookieProperties( ) { public CookieProperties { if (name == null || name.isBlank()) name = "merchant_session"; + // Boxed so that "not configured" is distinguishable from "configured false". + // As primitives these defaulted to false the moment any sibling property was + // bound, so a deployment setting only `nap.cookie.name` silently got a session + // cookie with neither HttpOnly nor Secure: readable by script and sent in clear + // over http. Only the all-absent case reached the true defaults, which is the + // case a test constructing NapProperties by hand is most likely to exercise. + if (httpOnly == null) httpOnly = Boolean.TRUE; + if (secure == null) secure = Boolean.TRUE; if (sameSite == null) sameSite = "Lax"; if (path == null) path = "/"; // maxAgeSeconds ≤ 0 is treated as "unset" by the enclosing NapProperties diff --git a/nap-spring/src/main/java/xyz/tcheeric/nap/spring/filter/NapSessionFilter.java b/nap-spring/src/main/java/xyz/tcheeric/nap/spring/filter/NapSessionFilter.java index c0a27a0..0853327 100644 --- a/nap-spring/src/main/java/xyz/tcheeric/nap/spring/filter/NapSessionFilter.java +++ b/nap-spring/src/main/java/xyz/tcheeric/nap/spring/filter/NapSessionFilter.java @@ -161,11 +161,34 @@ private void unauthenticated(HttpServletRequest request, HttpServletResponse res throws ServletException, IOException { if (log.isDebugEnabled()) { log.debug("nap_guard_no_session reason={} path={} pubkey={}", - reason, pathWithinApplication(request), pubkey); + reason, forLog(pathWithinApplication(request)), pubkey); } filterChain.doFilter(request, response); } + /** + * Strips line breaks from a value before it reaches the log. + * + *

The path is the one field here an attacker chooses. {@code reason} is a literal and + * {@code pubkey} comes from the session store, but the URI is whatever was requested, and + * a newline in a log line lets a caller append a second line that looks like ours: a + * forged {@code nap_guard_no_session} entry naming someone else's pubkey, or a fabricated + * success hiding a real refusal. Log analysis is parsed by line, so this is a truthfulness + * problem for the audit trail rather than an availability one. + * + *

Not reachable through Tomcat today, which rejects raw CR/LF in the request line and + * leaves {@code %0D%0A} percent-encoded through {@code getRequestURI()}. Verified, not + * assumed. The escaping belongs here anyway: that safety is the container's decision, not + * this filter's, and it does not survive a different servlet container, a forwarded or + * rewritten URI, or a future caller passing an already-decoded path. + */ + private static String forLog(String value) { + if (value == null) { + return null; + } + return value.replace('\r', '_').replace('\n', '_'); + } + /** * The request path with any servlet context path stripped, which is what * {@code nap.protected-path-prefixes} is written against. diff --git a/nap-spring/src/test/java/xyz/tcheeric/nap/spring/config/NapCookiePropertiesTest.java b/nap-spring/src/test/java/xyz/tcheeric/nap/spring/config/NapCookiePropertiesTest.java new file mode 100644 index 0000000..4af3082 --- /dev/null +++ b/nap-spring/src/test/java/xyz/tcheeric/nap/spring/config/NapCookiePropertiesTest.java @@ -0,0 +1,95 @@ +package xyz.tcheeric.nap.spring.config; + +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.springframework.boot.context.properties.bind.Binder; +import org.springframework.boot.context.properties.source.ConfigurationPropertySource; +import org.springframework.boot.context.properties.source.MapConfigurationPropertySource; + +import java.util.HashMap; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Binding-level tests for the session cookie attributes. + * + *

These go through {@link Binder} rather than calling the record constructor, because the + * defect they cover only existed under binding. {@code httpOnly} and {@code secure} were + * primitive {@code boolean}, so Spring materialised them as {@code false} whenever any sibling + * property was present, while a hand-constructed {@code NapProperties} took the all-absent + * branch and looked correct. A unit test that never bound anything would have agreed with the + * broken code. + */ +class NapCookiePropertiesTest { + + private static NapProperties bind(Map properties) { + ConfigurationPropertySource source = new MapConfigurationPropertySource(properties); + return new Binder(source).bind("nap", NapProperties.class).get(); + } + + @Test + @DisplayName("cookie section absent entirely: HttpOnly and Secure are on") + void defaultsWhenCookieSectionAbsent() { + NapProperties properties = bind(Map.of("nap.session-ttl-seconds", "900")); + + assertTrue(properties.cookie().httpOnly()); + assertTrue(properties.cookie().secure()); + assertEquals("Lax", properties.cookie().sameSite()); + } + + @Test + @DisplayName("an unrelated cookie property does not silently clear HttpOnly and Secure") + void unrelatedCookiePropertyKeepsSecurityAttributes() { + // The regression. Naming the cookie is the most ordinary reason to touch this section, + // and it used to strip both protections from the session credential. + NapProperties properties = bind(Map.of("nap.cookie.name", "merchant_session")); + + assertTrue(properties.cookie().httpOnly(), "HttpOnly must survive a partial cookie config"); + assertTrue(properties.cookie().secure(), "Secure must survive a partial cookie config"); + assertEquals("merchant_session", properties.cookie().name()); + } + + @Test + @DisplayName("setting only the domain keeps HttpOnly and Secure") + void domainOnlyKeepsSecurityAttributes() { + NapProperties properties = bind(Map.of("nap.cookie.domain", "example.test")); + + assertTrue(properties.cookie().httpOnly()); + assertTrue(properties.cookie().secure()); + assertEquals("example.test", properties.cookie().domain()); + } + + @Test + @DisplayName("an explicit false is still honoured") + void explicitFalseIsRespected() { + // The fix must not become an override. Local development over http has to be able to + // turn Secure off deliberately, otherwise the browser withholds the cookie and the + // operator's only remaining route is to stop using the property. + Map config = new HashMap<>(); + config.put("nap.cookie.secure", "false"); + config.put("nap.cookie.http-only", "false"); + + NapProperties properties = bind(config); + + assertFalse(properties.cookie().secure()); + assertFalse(properties.cookie().httpOnly()); + } + + @Test + @DisplayName("maxAge defaulting does not discard the security attributes on the way through") + void maxAgeDefaultingPreservesSecurityAttributes() { + // NapProperties rebuilds CookieProperties to fill in maxAge. That copy passes every + // other field positionally, so it is a second place the attributes could be dropped. + NapProperties properties = bind(Map.of( + "nap.session-absolute-ttl-seconds", "3600", + "nap.cookie.name", "merchant_session" + )); + + assertEquals(3600, properties.cookie().maxAgeSeconds()); + assertTrue(properties.cookie().httpOnly()); + assertTrue(properties.cookie().secure()); + } +} From cc8385b2e4fcc2f1ab6653b13f3483c843004ee4 Mon Sep 17 00:00:00 2001 From: tcheeric Date: Thu, 24 Sep 2026 16:32:45 +0100 Subject: [PATCH 16/18] docs: add an upgrade guide for the 0.9.0 breaking changes 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. --- README.md | 7 ++++ UPGRADING.md | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 113 insertions(+) create mode 100644 UPGRADING.md diff --git a/README.md b/README.md index 62e60cc..b9a3ff6 100644 --- a/README.md +++ b/README.md @@ -143,6 +143,13 @@ mvn -q test # unit tests mvn -q verify # + integration tests (Docker required for Testcontainers) ``` +## Upgrading + +[UPGRADING.md](UPGRADING.md) covers what breaks between releases. Read it before taking +0.9.0: startup now fails without an `AclResolver`, every live session ends once when the +cookie switches to the access token, and an unannotated handler under a protected prefix +returns `500` instead of being served. + ## Specification The protocol spec lives in the sibling `nap` repo: `docs/NAP-v2-RFC.md`. That repo also holds diff --git a/UPGRADING.md b/UPGRADING.md new file mode 100644 index 0000000..2854587 --- /dev/null +++ b/UPGRADING.md @@ -0,0 +1,106 @@ +# Upgrading + +## 0.8.0 to 0.9.0 + +Three changes alter behaviour at runtime. Two of them can take an application down at +startup or serve `500`s from handlers that worked yesterday, so read this before deploying +rather than after. + +Nothing here needs a database migration. The work is configuration and one unavoidable +logout. + +### Before you deploy + +Run through these in order. Each is a thing you can check now, on the running system, in +less time than the rollback would take. + +**1. Supply an `AclResolver`, or opt in to allowing everyone (#30).** + +Auto-configuration used to fall back to `AllowAllAclResolver` in silence, so an application +that never declared one authorised every authenticated principal for everything and gave no +sign of it. Startup now fails instead. + +If you have a resolver bean, nothing changes. If you were relying on the old default, say +so explicitly: + +```yaml +nap: + allow-all-principals: true +``` + +That property exists so the permissive case is a sentence in your configuration rather than +an accident of what you left out. Prefer a real resolver where you can. + +**2. Audit handlers under `nap.protected-path-prefixes` (#29).** + +`nap.require-annotation-on-protected-paths` now defaults to `true`. A handler under a +protected prefix that carries no NAP annotation is refused with a `500` rather than served +to anyone. A `500` rather than a `403` because the condition is a wiring mistake in the +application, not a decision about the caller: nobody is authorised to reach a handler whose +access rules were never stated. + +This only bites if `protected-path-prefixes` is non-empty. If it is, list the handlers +underneath it and give each one an annotation. A genuinely public endpoint says so: + +```java +@PublicEndpoint +@GetMapping("/api/health") +public Health health() { ... } +``` + +To stage the change, set `nap.require-annotation-on-protected-paths: false`, deploy, fix +what the logs show, then remove the property. Leaving it `false` permanently puts you back +where you started, where a new unannotated endpoint is public and nothing says so. + +**3. Expect every signed-in user to be logged out once (#27).** + +The session cookie now carries the access token rather than the session id. Cookies minted +by 0.8.0 no longer authenticate, so every live session ends the moment the new version +serves traffic. Users log in again with NIP-98 and it does not recur. + +There is no compatibility window on offer. Accepting both formats would mean continuing to +accept the session id as a bearer credential, which is the vulnerability being closed. +Schedule accordingly: a quiet hour costs less than the same logout at peak. + +**4. Check your `nap.cookie.*` block.** + +If you set any property under `nap.cookie`, 0.8.0 silently dropped `HttpOnly` and `Secure` +from the session cookie. That is fixed, and the fix is the safe direction, but it means the +cookie now carries `Secure` where it previously did not, and a browser will not send a +`Secure` cookie over plain `http://`. + +Any environment serving over `http` needs to say so: + +```yaml +nap: + cookie: + secure: false +``` + +Production should terminate TLS instead. + +### Deploying alongside `nap` (TypeScript) + +The two implementations now agree on the wire: both put the access token in the cookie and +authenticate with `getByAccessToken()`. Before 0.9.0 they did not, so a cookie minted by one +server could not be presented to the other. + +If both run against one session store, deploy `nap-java` 0.9.0 and `nap` 0.11.0 together. +A mixed fleet mid-rollout will reject cookies issued by the other side, which looks like +users being logged out at random rather than once. + +### Rolling back + +Rolling back to 0.8.0 logs everyone out a second time, for the same reason the upgrade did, +and reopens the session-id-as-credential exposure. If you roll back, treat any session id +that reached a log during the 0.9.0 window as a credential that was written down. + +### Verifying afterwards + +```bash +curl -si https://your-host/auth/session | grep -i set-cookie +``` + +You want `HttpOnly`, `Secure`, and a `SameSite` on that line. If `Secure` is missing and you +did not set `nap.cookie.secure: false` on purpose, the configuration is not being read from +where you think it is. From 3a8c1e4e6e0f5986fbdb27e15c096bfec1244204 Mon Sep 17 00:00:00 2001 From: tcheeric Date: Thu, 24 Sep 2026 16:44:42 +0100 Subject: [PATCH 17/18] security: pin bcprov 1.85 and jackson-databind 2.21.5, add an OSV gate 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). --- .github/workflows/ci.yml | 95 +++++++++++++++++++++++++++++++++++----- CHANGELOG.md | 26 +++++++++++ pom.xml | 35 +++++++++++++++ 3 files changed, 144 insertions(+), 12 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index edcc7d8..102f7a9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -95,24 +95,95 @@ jobs: **/target/failsafe-reports/** if-no-files-found: warn + osv-scan: + name: OSV vulnerability scan + runs-on: ubuntu-latest + # The SCA that actually runs. Dependency-Check below needs an NVD API key and skips + # without one, so on a fork or before the secret exists it scans nothing while still + # reporting a green check. OSV needs no key and no 12 minute database build. + # + # It earns its place rather than duplicating: on the first run it found bcprov 1.84 + # carrying a CRITICAL and a HIGH, and jackson-databind 2.21.4 carrying two MODERATEs. + # Both arrive transitively through nostr-java-core, so nothing in this repository + # names them, which is exactly the blind spot an SCA job exists to cover. + # + # Blocking, unlike Dependency-Check. A scan that cannot run without a secret must not + # gate a merge; this one can always run, so it can. + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-java@v4 + with: + java-version: '21' + distribution: temurin + cache: maven + + # Scanning the resolved tree, not the poms. osv-scanner reads pom.xml directly, but + # this is a multi-module build whose siblings are not in any registry, so resolution + # fails per module and it reports "0 packages affected by 0 known vulnerabilities": + # a green result that scanned nothing. Verified locally before choosing this route. + # `mvn dependency:tree` resolves against the real BOM, including the overrides in the + # parent pom, so what gets scanned is what actually ships. + - name: Resolve the dependency tree + run: mvn -q -B -ntp dependency:tree -DoutputFile=deptree.txt -DappendOutput=true + + - name: Query OSV + run: | + python3 - <<'PY' + import json, re, sys, urllib.request + + # Third-party runtime and compile dependencies only. The xyz.tcheeric modules are + # this build's own artifacts and exist in no advisory database. + packages = {} + for line in open('deptree.txt'): + match = re.search(r'([\w.\-]+):([\w.\-]+):jar:([\w.\-]+):(compile|runtime)', line) + if match: + group, artifact, version, _ = match.groups() + if not group.startswith('xyz.tcheeric'): + packages[f'{group}:{artifact}'] = version + + if not packages: + sys.exit('No packages parsed from deptree.txt: the scan would pass vacuously.') + + ordered = sorted(packages.items()) + payload = json.dumps({'queries': [ + {'package': {'name': name, 'ecosystem': 'Maven'}, 'version': version} + for name, version in ordered + ]}).encode() + + request = urllib.request.Request('https://api.osv.dev/v1/querybatch', data=payload) + results = json.load(urllib.request.urlopen(request, timeout=120))['results'] + + findings = [] + for (name, version), result in zip(ordered, results): + ids = [v['id'] for v in (result.get('vulns') or [])] + if ids: + findings.append(f'{name}@{version}: {", ".join(ids)}') + + print(f'Scanned {len(ordered)} third-party packages.') + for finding in findings: + print(f'::error title=Known vulnerability::{finding}') + + if findings: + sys.exit(f'{len(findings)} package(s) with known advisories.') + print('No known advisories.') + PY + dependency-check: name: OWASP Dependency-Check runs-on: ubuntu-latest - # Reports without blocking, for now, even though the tree currently scans clean. + # Secondary to osv-scan above, which is the gate. This one reports without blocking + # because it cannot run at all without the NVD_API_KEY secret, and a check that turns + # red on a missing secret rather than a real finding is how a team learns to click past + # a scanner. # # An earlier note here claimed 14 findings in spring-core and spring-security-core. - # That was not reproducible: re-running dependency-check 13.0.0 on 2026-09-24 with a - # populated NVD database (232 MB, updated during the run) reports BUILD SUCCESS at - # -DfailBuildOnCVSS=7, and also at -DfailBuildOnCVSS=0, which fails on a finding of - # any severity. The likely cause of the original number was a run that could not - # update the NVD feed, since an empty NVD_API_KEY is rejected outright rather than - # ignored. + # That was not reproducible and was retracted. A later note claimed the tree "currently + # scans clean", which was also wrong, just less visibly: this job had never run on a + # runner, and OSV found four advisories the moment anything actually scanned. Two of + # them, bcprov 1.84, were CRITICAL and HIGH. Both claims came from a local run against + # a populated database, which is a weaker check than it looks. # - # It stays non-blocking anyway, for a reason that does not depend on the count: this - # job has never executed on a runner. Making an unproven scanner a merge gate on its - # first outing risks blocking every PR on an environment problem rather than a real - # finding, which is how a team learns to click past a scanner. Remove this line once - # the job has run green in CI a few times, so the gate actually bites. + # Keep it non-blocking until it has run green on a runner with a key a few times. continue-on-error: true steps: - uses: actions/checkout@v4 diff --git a/CHANGELOG.md b/CHANGELOG.md index 8c57629..1c98913 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -66,6 +66,32 @@ turns an unannotated handler under a protected prefix into a `500`. rather than this filter's, and it does not survive a different container or a rewritten URI. +- **Pinned `bcprov-jdk18on` 1.85 and `jackson-databind` 2.21.5**, closing four advisories + the audit missed entirely. bcprov 1.84 carried 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 + carried two moderate `@JsonView` deserialization bypasses, and Jackson parses + attacker-controlled JSON in `Nip98Validator` and `DefaultNapServer`. + + Both arrive transitively through `nostr-java-core`, so nothing in this repository names + either version. Each pin is the lowest release carrying the fix, and each can be dropped + once `imani-bom` catches up. + + Found by the new OSV job below, not by the audit, and not by Dependency-Check either. The + earlier claim in the CI config that the tree "scanned clean" came from a local run and was + wrong. + +- **Added an OSV scan to CI, and it gates merges** (#32). Dependency-Check cannot run + without an `NVD_API_KEY` secret, so until that exists it skips and reports green while + scanning nothing. OSV needs no key. + + It scans the resolved dependency tree rather than the poms: `osv-scanner` reads `pom.xml` + directly, but this is a multi-module build whose siblings 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. The job + exits non-zero if it parses no packages, for the same reason. + - **`protected-path-prefixes` fails closed** (#29). `nap.require-annotation-on-protected-paths` now defaults to `true`, so a handler under a protected prefix that declares no NAP annotation is refused rather than served to anyone. diff --git a/pom.xml b/pom.xml index 855ff33..602dba9 100644 --- a/pom.xml +++ b/pom.xml @@ -28,6 +28,18 @@ 0.1.81 + + 1.85 + 2.21.5 + 3.13.0 3.5.2 3.2.5 @@ -43,6 +55,29 @@ import + + + org.bouncycastle + bcprov-jdk18on + ${bouncycastle.version} + + + com.fasterxml.jackson.core + jackson-databind + ${jackson.version} + + xyz.tcheeric From 66222359d6d195141c712eb386107df456dfd5ec Mon Sep 17 00:00:00 2001 From: tcheeric Date: Thu, 24 Sep 2026 16:54:10 +0100 Subject: [PATCH 18/18] fix: read every module's dependency tree in the OSV scan 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. --- .github/workflows/ci.yml | 26 ++++++++++++++++++-------- .gitignore | 5 +++++ 2 files changed, 23 insertions(+), 8 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 102f7a9..170d025 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -123,26 +123,36 @@ jobs: # a green result that scanned nothing. Verified locally before choosing this route. # `mvn dependency:tree` resolves against the real BOM, including the overrides in the # parent pom, so what gets scanned is what actually ships. + # + # -DoutputFile is resolved per module, not once for the reactor, so this writes a + # deptree.txt into each module directory and leaves the root one holding only the + # aggregator's own line. The query step globs for all of them rather than reading + # the root file, which is what it did on the first run: the scan failed closed with + # "would pass vacuously" instead of reporting a green zero. - name: Resolve the dependency tree run: mvn -q -B -ntp dependency:tree -DoutputFile=deptree.txt -DappendOutput=true - name: Query OSV run: | python3 - <<'PY' - import json, re, sys, urllib.request + import json, pathlib, re, sys, urllib.request # Third-party runtime and compile dependencies only. The xyz.tcheeric modules are # this build's own artifacts and exist in no advisory database. packages = {} - for line in open('deptree.txt'): - match = re.search(r'([\w.\-]+):([\w.\-]+):jar:([\w.\-]+):(compile|runtime)', line) - if match: - group, artifact, version, _ = match.groups() - if not group.startswith('xyz.tcheeric'): - packages[f'{group}:{artifact}'] = version + trees = sorted(pathlib.Path('.').glob('**/deptree.txt')) + print(f'Reading {len(trees)} dependency tree file(s): ' + + ', '.join(str(t) for t in trees)) + for tree in trees: + for line in tree.read_text().splitlines(): + match = re.search(r'([\w.\-]+):([\w.\-]+):jar:([\w.\-]+):(compile|runtime)', line) + if match: + group, artifact, version, _ = match.groups() + if not group.startswith('xyz.tcheeric'): + packages[f'{group}:{artifact}'] = version if not packages: - sys.exit('No packages parsed from deptree.txt: the scan would pass vacuously.') + sys.exit('No packages parsed from any deptree.txt: the scan would pass vacuously.') ordered = sorted(packages.items()) payload = json.dumps({'queries': [ diff --git a/.gitignore b/.gitignore index 11e3bd2..f4443be 100644 --- a/.gitignore +++ b/.gitignore @@ -5,3 +5,8 @@ target/ .idea/ *.iml /.claude/ + +# Written per module by the OSV scan's dependency:tree step, including into the +# working tree rather than target/, so it would otherwise show up as untracked +# noise after anyone reproduces that job locally. +deptree.txt