fix(auth): path-normalization authz bypass + x-forwarded gating + NIP-98 replay cache#8
Draft
shroominic wants to merge 3 commits into
Draft
fix(auth): path-normalization authz bypass + x-forwarded gating + NIP-98 replay cache#8shroominic wants to merge 3 commits into
shroominic wants to merge 3 commits into
Conversation
routstrd-auth had effectively zero security tests after the original store.test.ts was gutted to a stub in 2441ad4; nip98.ts and the proxy.ts authorization matrix had never been tested at all. Adds two network-free bun:test suites: - src/nip98.test.ts: validateNIP98Request happy path + every rejection branch (expired, future-skew, tampered sig, wrong kind, URL mismatch, method mismatch, missing/incorrect payload hash, correct payload hash), plus two documented hardening gaps with current-behavior assertions: cross-host token replay via forged x-forwarded-host, and same-token replay within the +/-60s window (no nonce store). - src/proxy.test.ts: pure static path classifiers (public/admin/ npub-restricted/restricted) and a request-level authorization matrix exercising Bearer-vs-Nostr routing against a seeded in-memory DB and a mock upstream: public no-auth -> forwarded; missing auth -> 401; Bearer on restricted/admin paths -> 403; valid Bearer -> forwarded; unknown Bearer -> 401; Bearer on /clients,/npubs -> 403; unregistered/user/admin npub matrix on admin and npub-restricted paths; expired Nostr -> 401; replay -> currently accepted (documented). Both suites are mutation-verified to fail when the corresponding security logic is broken (isRestrictedPath -> false; signature check removed). Also adds an inline SECURITY/HARDENING note to getAbsoluteRequestUrl in nip98.ts documenting the client-controlled x-forwarded-* trust gap and the recommended trusted-proxy-allowlist fix (no behavior change). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ache Two routstrd-auth hardening fixes the #7 security suite flagged as tripwires. 1) Trusted-proxy gating for x-forwarded-* in getAbsoluteRequestUrl - x-forwarded-proto / x-forwarded-host are client-controlled unless a trusted reverse proxy rewrites them. They are now IGNORED by default and only honored when ROUTSTRD_AUTH_TRUSTED_PROXY (config trustForwardedHeaders) is set. The real Host header is always honored, so a normal reverse proxy that rewrites Host (including host:port) keeps working without opt-in. - Closes a cross-host NIP-98 token-replay path: a forged x-forwarded-host could previously make a token signed for an arbitrary public host validate. 2) NIP-98 replay cache (nonce/jti) - New ReplayCache (in-memory Map, TTL = full 2*MAX_AGE validity window) keyed on the token SIGNATURE (not the event id: NIP-98 has no nonce tag, so two legitimate distinct requests with the same method+url in the same second hash to the same id; the Schnorr sig carries per-signing randomness so it uniquely identifies a presented token while a replayed wire token is byte-identical). The cache is consulted/recorded only AFTER signature verification so unauthenticated junk cannot poison it. - TTL spans the full window so a token cannot be replayed in the gap after a short-lived cache entry expires but before the token itself does. Tests: - Flipped both "CURRENTLY accepts..." tripwires in nip98.test.ts and proxy.test.ts to assert rejection. - Added focused tests: allowlist/trusted-proxy gating (reject forged by default, honor configured trusted proxy), replay rejection on default and npub-restricted paths, distinct-token acceptance, no cache poisoning on bad signatures, and ReplayCache unit tests incl. the full-window TTL boundary. bun test: 48 pass / 0 fail. bun build: ok. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The proxy's path classifiers (isAdminPath / isNpubRestrictedPath / isRestrictedPath) used exact `Set.has` plus exact-string dispatch in handle() and did ZERO normalization, while forward() shipped the raw url.pathname upstream. Because routstrd treats `/wallet/send/cashu`, `/wallet/send/cashu/`, `//wallet//send//cashu`, `/Wallet/Send/Cashu` and percent-encoded equivalents as the SAME route, a trailing slash, duplicate slash, mixed case or `%2F`/`%2e` escape dodged the admin/npub Sets. A user-role npub or a Bearer API key then reached the admin-only Cashu-SPEND and wallet endpoints (HIGH severity, live fund-loss class; proven: 200 forwarded). Fix: add AuthProxy.canonicalizePath() and canonicalize ONCE at the top of handle(), then classify + dispatch + forward on the canonical form: single percent-decode (folds encoded separators), collapse duplicate slashes, strip a single trailing slash (except root), resolve `.`/`..` (reject traversal above root -> 400), normalize backslashes. Protected- path comparison is case-insensitive (lowercased internally) so case variants are gated; the forwarded path preserves the client's original case (safe regardless of routstrd case-sensitivity — documented in the canonicalizePath docblock as a follow-up pending confirmation). Add regression tests covering trailing-slash, double-slash, mixed-case and percent-encoded forms of EVERY admin + npub-restricted path, asserting the SAME 403 as the canonical path for both Bearer and user-role npub, plus that nothing is forwarded upstream on denial and that an authorized admin is forwarded to the canonical route. Existing x-forwarded gating + replay-cache tests stay green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hardens the routstrd-auth security boundary. Builds on the #7 test suite (which documented two vulnerabilities as tripwires) and turns them into real fixes — plus closes a third, more severe authz bypass a red-team pass found. Supersedes #7.
1. 🔴 Path-normalization authz bypass (NEW — live fund-loss class)
The path classifiers used exact
Set.hasandhandle()dispatched on exact strings with zero normalization, whileforward()shipped the rawurl.pathnameupstream. Since routstrd treats/wallet/send/cashu,…cashu/,//wallet//send//cashu,/Wallet/Send/Cashu, and%2F/%2eescapes as the same route, those variants dodged the admin/npub Sets — a user-role npub or a Bearer API key reached the admin Cashu-SPEND / wallet-balance endpoints (forwarded 200). Proven.Fix: new
canonicalizePath()— single percent-decode (folds%2F/%5C/%2e), backslash→slash, collapse duplicate slashes, strip a single trailing slash, resolve./..and reject traversal-above-root / malformed encoding → 400.handle()canonicalizes once, then classifies + dispatches + forwards on that form. Protected-path comparison is case-insensitive; forwarded path preserves original case (safe under both daemon route-sensitivity behaviors — documented).2.
x-forwarded-*trusted-proxy gatingx-forwarded-proto/x-forwarded-hostare now ignored by default and only honored whenROUTSTRD_AUTH_TRUSTED_PROXYis set. The realHostheader (incl.host:port) is always honored. Closes the cross-host NIP-98 token-replay path.3. NIP-98 replay cache
New in-memory
ReplayCachekeyed on the token signature, TTL = full2*MAX_AGEwindow, consulted only after signature verification (no poisoning). Default-path validation wrapped so a replay returns 401, not 500.Tests
Both #7
CURRENTLY accepts…tripwires flipped to assert rejection. New regression tests cover trailing-slash, double-slash, mixed-case, percent-encoded-segment and percent-encoded-slash for every admin + npub-restricted path, for both Bearer and user-role npub, asserting 403 and zero upstream forwarding on denial, plus a positive control and traversal rejection.bun test: 160 pass / 0 fail (621 expect() calls). Reverting onlyproxy.ts(keeping tests) → 67 pass / 69 fail (non-tautological).bun buildgreen.Documented follow-ups
ROUTSTRD_AUTH_TRUSTED_PROXYin README/CLOUDRON.md.🤖 Generated with Claude Code