protected-path-prefixes does not fail closed: an unannotated handler under a protected prefix serves unauthenticated requests

[tcheeric]

Summary

NapSessionFilter treats a request as unauthenticated and calls filterChain.doFilter() anyway on every failure branch: no cookie, unknown session, expired session. It only short-circuits on an affirmative ACL denial (403). Whether the request is then refused depends entirely on NapPermissionInterceptor finding an annotation on the handler — and by default an unannotated handler under a protected prefix is allowed through.

Severity: Medium — the safe state depends on a developer remembering an annotation, and the failure is silent.

Why it matters

protectedPathPrefixes reads like it protects those paths. It does not. It selects paths on which the filter will attempt authentication; enforcement lives in the interceptor and requires a per-handler annotation. So:

@RestController
@RequestMapping("/api/v1/merchant")   // inside nap.protected-path-prefixes
class MerchantController {

    @GetMapping("/orders")
    @RequiresPermission("merchant:read")
    public List<Order> orders() { ... }        // guarded

    @GetMapping("/orders/{id}")                 // annotation forgotten
    public Order order(@PathVariable String id) { ... }   // serves anyone
}

The second handler is reachable with no cookie at all. Nothing fails at startup, nothing appears in the log, and the diff that added it shows a new endpoint and no removed guard. NapPermissionInterceptor's own javadoc names this:

it makes the safe state the one you have to remember, and a handler added to a protected controller without an annotation is exposed silently, with nothing in the diff to show for it.

The mitigation exists — nap.require-annotation-on-protected-paths=true — and is off by default, documented as "off by default because it can only break a working app."

That default is the finding. A deployment that has gone to the trouble of listing protected-path-prefixes has stated its intent; the configuration that honours that intent should not be a second opt-in that most operators will not know exists. The README shows require-annotation-on-protected-paths: false in its example configuration block, so the documented starting point is the unsafe one.

Two aggravating details:

• The unauthenticated fall-through is silent. Three of the four early-return branches call doFilter() with no log line. An operator cannot tell "nobody is hitting this endpoint" from "everybody is, unauthenticated." The TypeScript guards emit NAP_GUARD_NO_SESSION per refusal for exactly this reason ([EXT-0001] Fix guard-level audit logging (CONTEXT.md finding 12) nap#21); the Java filter has no equivalent.
• Prefix matching is String::startsWith on getRequestURI(). requiresExplicitDeclaration() correctly strips the context path before matching, but doFilterInternal() does not — it matches the raw URI. Under a non-empty context path the two disagree about which requests are protected, so a deployment on a context path can have the filter skip a request the interceptor believes is covered.

Fix

1. Default require-annotation-on-protected-paths to true in NapProperties:

if (requireAnnotationOnProtectedPaths == null) requireAnnotationOnProtectedPaths = Boolean.TRUE;

It only takes effect when protectedPathPrefixes 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 instead of an open endpoint, and @PublicEndpoint already exists to say "deliberately public" in the source. Note this in CHANGELOG.md as a behaviour change.

2. Use the same path for both decisions. Extract the context-path stripping into one helper and call it from both doFilterInternal() and requiresExplicitDeclaration(), so the filter and the interceptor cannot disagree about what is protected.

3. Log the unauthenticated fall-through, matching the TypeScript NAP_GUARD_NO_SESSION code:

log.debug("nap_guard_no_session path={}", path);

Debug rather than warn — an unauthenticated request to a protected path is normal before login — but it must be greppable.

Regression test

@Test
void anUnannotatedHandlerUnderAProtectedPrefixIsRefused() {
    // with protected-path-prefixes=[/api/v1/merchant] and the new default
    assertThat(get("/api/v1/merchant/unannotated").getStatus()).isEqualTo(500);
}

@Test
void publicEndpointStaysReachableInsideAProtectedPrefix() {
    assertThat(get("/api/v1/merchant/health").getStatus()).isEqualTo(200);
}

@Test
void protectionAppliesUnderANonEmptyContextPath() {
    // server.servlet.context-path=/app
    assertThat(get("/app/api/v1/merchant/orders").getStatus()).isEqualTo(401);
}

The third pins the filter/interceptor disagreement, which is the part most likely to survive a fix to the default alone.

[tcheeric]

Fixed in f3f9d48 (branch master, not pushed)

All three parts.

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.

The filter and interceptor now agree on what a path is. This was the half most likely to survive a fix to the default alone. NapSessionFilter.doFilterInternal() matched the raw getRequestURI() while NapPermissionInterceptor.requiresExplicitDeclaration() stripped the context path first, so under a non-empty context path the filter skipped authentication on requests the interceptor believed were covered. Both now call one pathWithinApplication() helper. With the interceptor failing closed, that disagreement decides whether a request is authenticated at all, which is why it could not be left for later.

The unauthenticated fall-through is no longer silent. Each of the three branches emits nap_guard_no_session with a reason (no_cookie, unknown_token, expired), 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.

Regression test doFilterInternal_appliesProtectionUnderANonEmptyContextPath covers the context-path case, asserting an authentication is present (which only happens if the filter treated the request as protected).

README updated: its example config showed require-annotation-on-protected-paths: false, so the documented starting point was the fail-open one.

Full suite green: 197 tests, 0 failures, 0 skipped.

Behaviour change worth flagging at release

An application 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, but it is a breaking change for anyone who was relying on the old default.

[tcheeric]

Fixed in #36 (branch security/appsec-audit-remediation, released as 0.9.0). The PR body carries the per-finding verification; this issue's earlier comments carry the detail. Will close on merge.