Undocumented fallback accepts the NIP-98 proof from a `proof` body field, outside the signed payload hash

[tcheeric]

Summary

NapAuthController.resolveAuthorization() falls back to reading the NIP-98 proof from a proof field in the JSON request body when the Authorization header is absent. This parses the raw body with a second, independent ObjectMapper.readValue() call, outside the validated path, and accepts a credential from a location the signed payload hash cannot cover consistently.

Severity: Medium — not obviously exploitable today, but it is an undocumented second entry point into the authentication path, and it weakens a property the codebase is otherwise careful about.

private String resolveAuthorization(HttpServletRequest request, byte[] rawBody) {
    String header = request.getHeader("Authorization");
    if (header != null && !header.isBlank()) return header;

    try {
        Map<String, Object> body = objectMapper.readValue(rawBody, Map.class);
        Object proof = body.get("proof");
        if (proof instanceof String proofValue && !proofValue.isBlank()) return proofValue;
    } catch (Exception ignored) { }
    return null;
}

Why it matters

1. It is not in the protocol surface. The README documents POST /api/v1/auth/complete as "Verify the NIP-98 proof" with the proof in Authorization. Nip98Validator requires Authorization: Nostr <base64>, and the TypeScript implementation has no equivalent fallback — verifyCompletion() reads input.authorization only. So this is a JVM-only accepted credential location that no specification, test vector, or interop test describes.

2. Self-referential hash coverage. NIP-98's payload tag commits to sha256(rawBody). When the proof travels in the header, the body it hashes is the body. When the proof travels inside the body, the proof is part of what it must hash — the client has to construct a body containing a proof whose payload tag equals the hash of that same body. That is satisfiable only by excluding the proof field before hashing, which nothing here specifies or enforces. Whatever a client does to make this work is an unwritten convention on the most security-critical hash in the protocol.

3. Second parse of attacker-controlled bytes. DefaultNapServer.parseAuthCompleteRequest() already parses rawBody under a strict shape check. This adds an independent readValue(..., Map.class) on the same bytes with different semantics and a blanket catch (Exception ignored). Two parsers over one attacker-controlled buffer is a parser-differential shape — the class of bug behind HTTP request smuggling — and the swallowed exception means a body that parses differently here than there produces no signal at all.

4. Credentials in bodies get logged. The /auth/refresh handler documents exactly this concern:

The token is presented as Authorization: Bearer <token>, not in the body: it is a credential, and a body would be logged by anything that logs request payloads.

That reasoning applies identically to the NIP-98 proof, and this fallback contradicts it within the same controller.

Fix

Remove the fallback:

private static String resolveAuthorization(HttpServletRequest request) {
    return request.getHeader("Authorization");
}

Nip98Validator already returns NAP_COMPLETE_MISSING_AUTH_HEADER for a null header, so the failure stays uniform, padded, and audited.

If a body-carried proof is genuinely needed for some client — the git history would say whether it ever was — it should be specified rather than inferred: declared in the RFC, given a defined hashing rule that states which fields the payload tag covers, mirrored in TypeScript, and pinned by a nap-it interop test. An undocumented fallback that only one implementation honours is the worst of both.

Regression test

@Test
void completeRejectsAProofCarriedInTheBody() {
    var proof = buildValidProof(challenge);   // would succeed in the header
    var body = Map.of("challenge_id", challenge.challengeId(), "proof", proof);

    var response = post("/api/v1/auth/complete", body);   // no Authorization header

    assertThat(response.getStatus()).isEqualTo(401);
}

Asserting that a valid proof in the body is refused is the point — a test using an invalid proof would pass whether or not the fallback exists.

[tcheeric]

Fixed in 4fd303a (branch master, not pushed)

resolveAuthorization() is now request.getHeader("Authorization") and nothing else. Nip98Validator already returns NAP_COMPLETE_MISSING_AUTH_HEADER for a null header, so the failure stays uniform, padded, and audited.

The test that asserted the fallback (complete_usesBodyProofWhenAuthorizationHeaderIsMissing) is inverted rather than deleted, and now asserts that a valid-looking body proof produces a null authorization at the verifier and the uniform 401. Using a valid-looking proof is what makes it meaningful, since a malformed one would be refused whether or not the fallback existed.

objectMapper is now unused. The constructors keep the parameter because they are public API and auto-configuration passes the application's mapper, so dropping it would break hand-wired controllers for no gain. The field carries a comment explaining why it is retained and why nothing in that class should acquire a second JSON reader over the raw body.

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

[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.