Skip to content

feat(relay): enforce NIP-FI assertion+NIP-98 pairing on HTTP ingress - #7264

Open
wpfleger96 wants to merge 32 commits into
mainfrom
hayt/nip-fi-http-enforcement
Open

wpfleger96 wants to merge 32 commits into
mainfrom
hayt/nip-fi-http-enforcement

Conversation

@wpfleger96

@wpfleger96 wpfleger96 commented Sep 2, 2026

Copy link
Copy Markdown
Member

Implements NIP-FI HTTP ingress enforcement per the merged spec (PR #7254). Every protected HTTP surface in enforce mode requires a valid Nostr-Federated-Identity: Bearer <JWS> assertion alongside its NIP-98 authorization, bound to the same key. The NIP-98 pubkey (proven actor) must equal the assertion's nostr_pubkey; absent, mismatched, or unverifiable assertions are denied fail-closed with byte-exact text/plain responses per the spec's rejection table.

This is S5 of the buzz-enterprise-identity program. WS enforcement (S3) is separate.

What changed

New nip_fi_http module: admit_nip_fi_http() covers the full admission sequence — Nostr-Federated-Identity extraction, offline JWKS verification, NIP-98 key pairing [FI-INV-05], issuer-qualified deny-map check via (iss, pubkey, now) [FI-INV-14], and byte-exact denial for all four DenialClass variants. admit_nip_fi_http_on_state() is the single call-site wrapper for all surfaces [FI-TRACE-AUTHORITY-UNIFORM].

Protected surfaces (all gated via the uniform authority):

  • HTTP bridge: POST /events, /query, /count
  • GIF proxy: POST /gifs/search, /gifs/share
  • Workflow reads: GET /workflows/{id}/runs, GET /workflows/{id}/runs/{id}/approvals
  • Moderation reads: GET /moderation/reports, /moderation/audit, /moderation/restricted
  • Media/Blossom: PUT /upload, /media/upload
  • Git smart-HTTP: info/refs, upload-pack, receive-pack
  • Git settings control plane: GET/POST /git/:owner/:repo/default-branch (now routed through admit_nip_fi_http_on_state for full assertion key-pairing)
  • Invites: POST /api/invites (mint only; claim exempt as pre-membership enrollment door)

NIP-98 strictness in NIP-FI mode:

  • When NIP-FI is not Off, bridge POST handlers force require_auth_token = true — the X-Pubkey dev-mode fallback cannot satisfy the pairing requirement [NIP-FI.md:547-567]
  • POST bodies on /events, /query, /count require a matching payload tag in enforce mode [NIP-FI.md:579-597]
  • A payload tag present with no content (one-element ["payload"] or empty string) is rejected as a structurally invalid claim — NIP-98 requires the content to be a valid 64-char lowercase hex SHA-256 digest. Malformed payload diagnostics use a static error message (no attacker-controlled content in logs).
  • Off mode preserves pre-NIP-FI behavior per [FI-INV-15]

GIF auth ordering (Off mode compatibility): In Off mode, NIP-98 authentication for GIF routes precedes the upstream provider lookup. A missing or invalid NIP-98 credential produces a 401 before any upstream call is made. Compatibility exception: an unauthenticated Off-mode request on a configured tenant with no GIF provider changes from 404 (pre-NIP-FI: provider-not-found from unchecked lookup) to 401 (post-NIP-FI: auth check fires first). Off-mode GIF auth uses the legacy api_error() path, which produces a JSON {"error":"..."} body without a WWW-Authenticate header. The ordering change is intentional and limited to this single case.

Auth header cardinality: Active (non-Off) modes reject requests with more than one Authorization header with 403 evidence rejected\n. The cardinality gate is enforced at the NIP-FI admission boundary (admit_nip_fi_http), not via legacy auth boolean flags, so it applies uniformly to all active-mode surfaces including Git. Off mode passes through the first header value unchanged, preserving pre-NIP-FI behavior [FI-INV-15].

Denial-class remapping: In active (non-Off) modes, NIP-98 extraction/verification failures from the closure are mapped to the appropriate DenialClass: MissingEvidence when no Authorization header is present, EvidenceRejected when the header is present but invalid. Off mode propagates legacy responses unchanged [FI-INV-15]; for example, the Git missing-Authorization and wrong-scheme 401s keep their legacy bytes with no content-type header.

JWKS refresh scheduling: The refresh timer's *last checkpoint is set after the fetch completes, not from the pre-fetch timestamp, preventing interval drift under network latency. The loop is extracted to nip_fi_jwks_refresh_loop for unit-testability.

Log hygiene [NIP-FI.md:777-779]: Issuer identity is never written to logs or error messages. All startup/refresh diagnostics use an ordinal index (issuer_index = N). Config parse errors for malformed issuers use classify()/line/column codes rather than raw config values.

Admin SPA document routes: /reports, /reports/{id}, and /feedback are exempt from the NIP-FI assertion guard only on the admin host (host-qualified via is_admin_host). These paths are not added to the broad NIP_FI_EXEMPT_PREFIXES list — tenant hosts remain fully protected.

Deny-map seam (HttpDenyMap trait): signature is (issuer, pubkey, now) matching NipFiDenyMap::is_denied from S4 (PR #7265) so integration is a one-liner. Stub is AlwaysAdmitStubDenyMap — named to accurately describe stub-phase semantics (fail-open intentionally pending S4).

Route inventory: every authenticated HTTP route is classified as Protected or Exempt with justification. Documented in the F4 seam-test comment in bridge.rs.

Outer-guard tests: #[ignore = "requires Postgres"] tests in bridge.rs drive the full router (build_router) in Enforce mode with valid NIP-98 but no Nostr-Federated-Identity header. These exercise the outer nip_fi_assertion_guard middleware in router.rs — the layer that fires before any handler runs. They prove the guard's exact response bytes (401 authentication required\n, text/plain; charset=utf-8, WWW-Authenticate: Nostr) for each protected route. Per-handler admit_nip_fi_http_on_state wiring is proven separately by the cardinality test and the media/settings/git handler-level suites.

Seams

S3 seam: nip_fi_config.rs is duplicated (S3 not landed; session-deadline parts stripped). Removed when S5 rebases onto S3.

S4 seam: AlwaysAdmitStubDenyMap until S4 provides the real map. Integration is one line at the admit_nip_fi_http_on_state wrapper.

Evidence

At head bab687a52 (branch hayt/nip-fi-http-enforcement):

  • PostgreSQL lane (scripts/postgres-test-run.sh, nextest profile postgres-ci): 504 run, 504 passed. This covers the media upload/read matrix over /upload and /media/upload (missing, malformed, duplicate and mismatched proof, each with a valid assertion), the upload denial resource witness (per-key in-flight accounting unchanged across denials; admitted exhausted-permit control returns 429), exact Off-mode GET/HEAD 404 bytes, GIF/moderation/workflow mismatched-key denials, and the Git info/refs/pack-route matrix including a receive-pack admission control that reaches the Git permit gate (503 git service busy, Retry-After: 5).
  • external_infra settings test nip_fi_denied_assertion_does_not_advance_snapshot_digest (isolated Postgres, Redis and MinIO): passed. Denied assertions leave the snapshot digest unchanged; the owner POST advances it, and the new HEAD and parent link are asserted.
  • cargo fmt --check and cargo clippy -D warnings: clean.

wpfleger96 added a commit that referenced this pull request Sep 3, 2026
Amend NIP-FI HTTP ingress with an explicit Git smart-HTTP
credential-helper exemption. The exception covers method binding,
endpoint-URL binding, and the `payload` tag requirement for `info/refs`,
`git-upload-pack`, and `git-receive-pack`, while preserving per-request
NIP-FI assertion, key pairing, and deny-map enforcement.

The spec records Git's credential-protocol limitation, the required
compensating controls, and the rule that this exception is limited to
these endpoints and is superseded by per-request signing.

Related: [PR #7264](#7264)

Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Hayt and others added 4 commits September 2, 2026 20:36
…aces

Every protected HTTP ingress in enforce mode now verifies an npub-bound
assertion alongside its NIP-98 authorization.  The NIP-98 event's pubkey
(the proven actor) must equal the assertion's nostr_pubkey; absent,
mismatched, or unverifiable assertions are denied fail-closed.  Off mode
passes through unchanged.

Surfaces gated:
- HTTP bridge:   POST /events, /query, /count  (bridge.rs)
- Invites:       POST /api/invites             (invites.rs)
- Media/Blossom: PUT /upload                  (media.rs)
- Git smart-HTTP: all three transport routes  (git/transport.rs)

New modules:
- nip_fi_http.rs: check_nip_fi_http(), extract_bearer_token(),
  http_denial(), check_nip_fi_http_on_state(), HttpDenyMap trait,
  FailClosedStubDenyMap (S4 seam: always admits until S4 lands),
  NipFiHttpOutcome::{Admitted, Denied}
- nip_fi_config.rs: NipFiRelayConfig parsed from env vars (shared
  S3/S5 seam; removed after S3 merges and S5 rebases)

Per-request verification: every request re-verifies offline against the
configured issuer JWKS snapshot; no session lifetime concept for HTTP.

Deny-map seam: HttpDenyMap trait with FailClosedStubDenyMap stub.
The integration commit (when S4 lands) replaces the stub with a real
lookup; the S5 call site is unchanged.

22 NIP-FI unit tests pass; 1042 buzz-relay tests pass.
The one existing failure (mesh_demo::demo_join_forwarded_arm) is
pre-existing and unrelated to this change (external service 504).

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Hayt <9e1c23a3fd83f61da34420e4e88ff1b16e45cafcc0cd9019eb07d4ecfa8ca9b0@buzz.block.builderlab.xyz>
F1 — Gate GIF search/share, workflow runs/approvals, and moderation
reads through check_nip_fi_http_on_state. authenticate() in gifs.rs,
authorize_workflow_read() in workflows.rs, and authorize_moderation_read()
in bridge.rs all now call the NIP-FI gate after NIP-98 verification.
Route inventory with protected/exempt classification added to the F4
seam-test block so new authenticated routes must be explicitly classified.

F2 — Kill X-Pubkey fallback in NIP-FI enforce/deny-protected mode.
Bridge POST /events, /query, /count now pass
require_auth_token = config.require_auth_token || nip_fi_active
to verify_bridge_auth_with_options. When NIP-FI is not Off, a real
NIP-98 event is mandatory; X-Pubkey dev-mode fallback is disabled.
[NIP-FI.md:547-567, FI-TRACE-HTTP-INGRESS]

F3 — Require NIP-98 payload tag for bridge POST bodies in enforce mode.
POST /events, /query, /count pass require_payload = nip_fi_enforce
(Enforce mode only; off/deny-protected unchanged). Every POST body
on these routes is authorization-relevant per spec §579-597.

F4 — Production-seam tests per surface. Six handler-level tests added
to bridge.rs postgres_tests: events, query, count, moderation_reports
(shared witness for all three moderation routes), gif_search (shared
witness for both GIF routes), workflow_runs (shared witness for both
workflow routes). Each test drives the real router in Enforce mode with
valid NIP-98 but no assertion → expects 401. The test fails if the
check_nip_fi_http_on_state call is deleted from the production code.
Marked #[ignore = "requires Postgres"].

F5 — Reshape HttpDenyMap trait to match S4 NipFiDenyMap signature.
is_denied now takes (issuer: &str, pubkey: &PublicKey, now: DateTime<Utc>)
matching NipFiDenyMap::is_denied from PR #7265 (S4). The check_nip_fi_http
call site passes assertion.identity().issuer() and Utc::now() so integration
is a one-liner. Rename FailClosedStubDenyMap → AlwaysAdmitStubDenyMap to
accurately describe the stub phase semantics.

CI — Fix main.rs:530 clippy::redundant_pattern_matching warning:
if let None = ... → .is_none().

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
I1: gate GET/HEAD /media/{sha256} with NIP-FI; correct inventory — media reads
require Blossom auth + relay membership and are PROTECTED, not public.

I2: force require_auth_token || nip_fi_active in authorize_moderation_read and
authorize_workflow_read so X-Pubkey fallback cannot satisfy NIP-FI pairing when
NIP-FI is active. Mirrors the fix already applied to the bridge POSTs.

I3: reorder GIF search/share handlers to authenticate before klipy config check;
reorder workflow_runs_inner to authorize before cursor/limit validation. Admission
(NIP-98 + NIP-FI) now fires before all application-level checks.

I4: add SHA-256 payload tag to make_nip98_headers so bridge tests reach the NIP-FI
gate (previously rejected at payload verification before the gate). Add Off-mode
passthrough and DenyProtected production-seam tests (2 new cases). Add
nip_fi_off_test_state and nip_fi_deny_protected_test_state helpers.

I5: useless_format eliminated by converting format!("literal") to bare string
literal at all three bridge test call sites.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Rebase onto c328202 (git smart-HTTP exemption amendment merged as #7268).

Update NIP-FI.md line references throughout nip_fi_http.rs, bridge.rs, and
workflows.rs to match the amended spec's new line numbers:
- NIP-FI.md:547-567 / :547-578 -> :594-607 (carrier spec / no-fallback clause)
- NIP-FI.md:579-597 -> :619-637 (payload-binding clause)
- NIP-FI.md:584-587 -> :624-627 (deny-set check)

Update route inventory comment to cite the merged git exemption with PR and
commit references (#7268 / c328202, NIP-FI.md:545-583).

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
@wpfleger96
wpfleger96 force-pushed the hayt/nip-fi-http-enforcement branch from 3890aad to 7f8ed8c Compare September 3, 2026 00:42
Hayt and others added 5 commits September 2, 2026 21:57
…est design

Seam tests were signing NIP-98 events for wss://nip-fi-test.local/{path}
but nip98_expected_url() constructs https://{tenant-host}/{path} from the
request Host header.  verify_bridge_auth was rejecting all enforce-mode
test requests with 400 (URL mismatch) before the NIP-FI gate was reached,
making the 401 assertions trivially false for the wrong reason.

Fix all 6 enforce/deny-protected seam tests to sign for
format!("https://{host}/{path}") so they actually exercise the gate.

Off-mode test: was sending no auth at all; verify_bridge_auth returns 401
(missing Nostr auth) before reaching the NIP-FI gate, so the assert_ne 401
was trivially satisfied.  Fix: send X-Pubkey dev-mode header so the request
reaches check_nip_fi_http_on_state.  Add second assert_ne 503 to cover
DenyProtected denial class.  The Off-mode test correctly stays GREEN after
gate removal (Off-mode always admits).

DenyProtected test: same URL fix as enforce tests.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…red, invites seam test

I1 — Executable inventory coupling:
Replace prose comment with NIP_FI_PROTECTED_ROUTES const array and
nip_fi_route_inventory_is_complete unit test. The const names every
handler; the test asserts total=11, seam-tested=7, Blossom-debt=4, and
panics on count drift. Process coupling is declared explicitly: axum
does not expose a static route table for compile-time enforcement, so
the seam tests are the executable registration — each PROTECTED entry
has a test that goes RED if its gate is deleted.

I3 — Query extractor order residual:
Remove Query<RunsQuery> from workflow_runs signature and
Query<ModerationReadQuery> from moderation_reports/moderation_audit
signatures. Parse raw query string after admission using
serde_urlencoded::from_str so malformed params (e.g. ?limit=abc)
cannot produce a 400 before the NIP-FI gate fires. Removes
Query import from workflows.rs; adds serde_urlencoded dep.

I4 — Valid-pair admission (declared):
StaticIssuerKeySource is pub(crate) in buzz-auth by design — the
authority-construction seam is intentionally closed to external crates.
Building a relay-level valid-pair test requires either exporting test
infrastructure from buzz-auth or running a live relay. The unit test
nip_fi_http::tests::enforce_missing_assertion_is_401 (off-mode admit)
and the Gurney live-system lane cover the "real assertion → admit"
path; this is declared, not an omission.

Invites seam test (I4 and Paul's defect 4):
Add nip_fi_enforce_mint_invite_no_assertion_is_401 to invites.rs
postgres_tests. Reuses invite_test_state + config clone to enable
Enforce mode without duplicating the full state-building boilerplate.
Mutation M7: gate removal causes FAILED (assert_eq 401 vs 403 authz).

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…losed route classification

T2 — post-admission query parse error now returns 400 instead of silently
defaulting.  The previous .ok().unwrap_or_default() pattern discarded ALL
query fields on any parse failure, so GET /moderation/reports?status=open
&limit=abc returned all statuses instead of 400.

  api/mod.rs: add pub(crate) parse_query_or_400<T>() — absent/empty
  query yields Default; non-empty malformed query yields 400.  Five
  regression tests in api::parse_query_tests cover absent, empty, valid,
  malformed-with-valid-field, and malformed-standalone cases.  The key
  test (malformed_limit_is_400_not_default) would have failed against the
  old .ok().unwrap_or_default() implementation.

  bridge.rs, workflows.rs: replace the three .ok().unwrap_or_default()
  parse sites with parse_query_or_400(); map Err to Response (-> return /
  ? depending on function signature).

T1 — fail-closed NIP-FI route classification via runtime default-deny guard.

The previous NIP_FI_PROTECTED_ROUTES const + nip_fi_route_inventory_is_complete
test were a second, disconnected list: adding a route to router.rs without
updating the list kept CI green while the route could admit in Enforce mode.

  router.rs: add NIP_FI_EXEMPT_PREFIXES const (single source of truth for
  which paths are exempt) and nip_fi_assertion_guard async middleware.  The
  guard runs over the full merged router.  In Enforce mode, any non-exempt
  path without the Nostr-Federated-Identity assertion header receives 401
  authentication required before the handler is dispatched — even if the
  handler omits check_nip_fi_http_on_state.  In Off mode the guard is fully
  transparent [FI-INV-15].  Three unit tests cover exempt-path recognition,
  protected-path recognition, and the default-deny property (unclassified
  paths are not exempt by default).

  bridge.rs: remove NIP_FI_PROTECTED_ROUTES, NIP_FI_SEAM_TEST_COUNT, and
  nip_fi_route_inventory_is_complete.  Compact comment block replaced with
  a reference to router.rs::NIP_FI_EXEMPT_PREFIXES as the single source of
  truth.  Seam tests remain as per-handler wiring proof.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…rcement

* origin/main:
  🤖 fix(desktop): harden smoke E2E tests against Bestie overlay and toast timing (#7270)
  Show status and huddle indicators beside names (#7112)
  Add mobile voice notes (#7121)
  perf(desktop): publish mention sends before waking agents (#7154)
  fix(desktop): unify owned-agent cloud provenance markers (#7129)
  fix(desktop): derive agent availability from relay presence (#7127)
  fix(desktop): preserve spacing after multi-word mentions (#7128)

Signed-off-by: Hayt <9e1c23a3fd83f61da34420e4e88ff1b16e45cafcc0cd9019eb07d4ecfa8ca9b0@buzz.block.builderlab.xyz>
…empt git policy; T2 seam test

T1-IMP1: upgrade nip_fi_assertion_guard from headers.contains_key to
extract_bearer_token — the same transport-parsing function used by the
full per-handler verifier.  Now: absent → 401, junk/non-Bearer/repeated/
empty → 403.  A forgotten-gate handler cannot admit with
'Nostr-Federated-Identity: junk' — the guard rejects any malformed token
before the handler fires.  Adds unit test proving the adversarial case.

T1-IMP2: add /internal/git/policy to NIP_FI_EXEMPT_PREFIXES (exact path,
not the /internal/ subtree).  The pre-receive hook calls this localhost/
HMAC endpoint without an assertion; the guard was returning 401 in Enforce
mode and 503 in DenyProtected mode, blocking every git push.  The endpoint
retains its own require_localhost + HMAC-signed payload authorization.
Adds: (a) exemption classification test, (b) #[ignore = requires Postgres]
production-router test proving the guard passes through to the policy
handler (403 from require_localhost), not the NIP-FI guard (401).

T2-seam: add t2_admitted_malformed_query_through_moderation_reports_is_400
(#[ignore = requires Postgres]).  Builds Off-mode state, seeds actor as
community owner, sends GET /moderation/reports?status=open&limit=abc with
X-Pubkey dev-mode auth (admitted past all gates), asserts 400.  Would
return 200 against the old .ok().unwrap_or_default() behavior (all fields
silently dropped → empty list returned) — the test binds the production
seam.

cargo check -p buzz-relay: clean (0 errors, 0 warnings)
NIP-FI unit tests + new guard/classification tests: 1057 passed, 1 pre-existing
  unrelated failure (mesh demo network test, present on main since 7a9a523)

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Hayt and others added 2 commits September 3, 2026 11:09
…sts module

The Validate PostgreSQL test discovery CI script requires Postgres-gated
tests to live in a postgres_tests module or postgres_* integration binary.
nip_fi_enforce_git_policy_callback_reaches_own_auth_not_nip_fi_guard was
placed in router::tests with #[ignore = "requires Postgres"], which the
discovery script flags as an undiscoverable location.

Move the test verbatim into bridge::postgres_tests, replacing the inline
AppState builder with nip_fi_enforce_test_state() and the manual oneshot
with oneshot_request() — both already present in that module.  Test logic
and falsifying-mutation semantics are unchanged: POST /internal/git/policy
in Enforce mode with no assertion header must not get 401 from the NIP-FI
guard; it must reach require_localhost which returns 403.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…ication

The guard now verifies JWT signature, issuer, expiry, and claims before
forwarding — not just token transport shape.  A forgotten-gate handler
(one that omits check_nip_fi_http_on_state) can only be reached with a
cryptographically verified assertion; it still lacks the key-pairing and
deny-map step, which per-handler calls provide on top.

Changes:
- nip_fi_assertion_guard: after extract_bearer_token (step 1), call
  verifier.verify_assertion(token) (step 2).  No verifier (startup race)
  → 503; invalid sig/claims → 403 EvidenceRejected.
- buzz_auth: add VerifyAssertion trait (object-safe wrapper over
  FederatedAssertionVerifier<S>) so AppState.nip_fi_verifier uses
  dyn VerifyAssertion rather than a concrete ProductionJwksSource type.
- buzz_auth/test-utils: expose StaticIssuerKeySource and
  AssertionKeySet::new_for_test so integration tests in buzz-relay can
  build verifiers without a live JWKS endpoint.
- check_nip_fi_http: updated to accept dyn VerifyAssertion (removes
  the S: IssuerKeySource generic, aligns with dyn dispatch at the
  AppState boundary).
- Production-router forgotten-gate test: sends a structurally valid but
  cryptographically invalid assertion (bad sig) + no NIP-98 header to
  POST /events in Enforce mode with a real StaticIssuerKeySource verifier.
  Asserts 403 (guard denies bad sig before handler fires).  Falsifying
  mutation: remove verifier.verify_assertion from the guard → guard
  forwards → handler NIP-98 check fires → 401 ≠ 403 → test fails.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
wpfleger96 added a commit that referenced this pull request Sep 3, 2026
…#7278)

Adds the media/Blossom possession-proof exception section to
\`docs/nips/NIP-FI.md\`.

## What this changes

Encodes the condition set confirmed by Thufir's security review
(2026-09-03) as normative MUSTs in NIP-FI. Kind-24242 Blossom auth
events are admitted as the NIP-FI pairing possession proof for **media
routes only** — a named, bounded exception with an explicit precedent
fence against expansion.

### Scope fence

Kind-24242 proofs are valid only on:

| Proof type (`t` tag) | Valid route | Method |
|---|---|---|
| `upload` | `PUT /upload` (and temporary alias `PUT /media/upload`
until removed) | PUT |
| `get` | `GET /media/{hash…}`, `HEAD /media/{hash…}` | GET, HEAD |

All other protected routes MUST reject kind-24242 proofs.

### Upload proofs

Exactly one `x` tag over consumed body bytes; temporal check precedes
body consumption.

### Read proofs (the relaxation Will approved)

Host-wide MAY: no `x` required. Exactly one `server` tag matching the
resolved tenant host is a MUST. Optional `x` must match parent hash if
present.

Named residual (verbatim in spec): within at most 60 seconds from
minting (plus 5s future-skew), a captured full header set allows reading
any media blob on exactly one tenant host — read-only,
membership-checked, revocable, not state-changing, not cross-tenant.

### Freshness (Thufir option 2)

- `created_at <= now + 5s` (bounded future skew)
- `now - created_at <= 60s`
- Exactly one `expiration`, valid at admission, satisfying `expiration
<= created_at + 60s`

### Transport/cardinality

Exactly one each of `Authorization` (Nostr scheme), `t`, `expiration`,
`server`; `x` at most once; reject any duplicate, malformed, or
conflicting instance.

### Per-request pairing

Full assertion verification, exact key equality between assertion
`nostr_pubkey` and kind-24242 signer, deny-map enforcement on every
request. Stub gap named.

### Compliance note

PR #7264 implementation is explicitly non-compliant until the bounded
hardening task lands (named gaps: multi-tag acceptance, 3600s window,
optional `server`).

## Behavioral oracle

`FI-TRACE-HTTP-INGRESS` extended to cover kind-24242 admission and
denial cases.

## Scope

Docs-only. No code changes. The code hardening is a separate follow-on
task.

---------

Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Co-authored-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
…e design

Resolves T1-IMP1: structural bypass impossibility via private constructor.

NipFiAdmission<X> has a private constructor; the only source is
admit_nip_fi_http_on_state(). A handler cannot reach proven_pubkey
through the NIP-FI channel without executing the full admission
sequence: NIP-98 closure -> assert extract -> verify -> pair -> deny-map.

Adds VerifiedAssertion::new_for_test to buzz-auth (cfg(test|test-utils))
for the new enforce_key_mismatch_is_denied unit test (FI-INV-05 wiring
falsifier). Adds AssertionPolicyId::zero() and TransportContractId::zero()
test-only constructors to buzz-auth/config.rs (same cfg gate).

Adds fmt::Debug impl for NipFiAdmission<X> (redacts extra, shows pubkey
and assertion for diagnostics).

All 24 NIP-FI unit tests pass including two new tests:
- off_mode_propagates_nip98_closure_failure: Off mode still executes
  the NIP-98 closure, preserving pre-NIP-FI surface auth requirements.
- enforce_key_mismatch_is_denied: pairing-wiring falsifier using
  PairingMockVerifier + VerifiedAssertion::new_for_test; removing
  the asserted_key == proven_pubkey branch causes this test to fail.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
wpfleger96 added a commit that referenced this pull request Sep 3, 2026
… on #7264

The strict verifier exists but runs in Permissive mode until the NIP-FI
HTTP enforcement PR (#7264) merges and the stub in
blossom_strictness_from_state is replaced with the live mode derivation.
The deny-map gap (S4) is still a named known gap.

Remove the premature 'now compliant' claim and state exactly what is true:
verifier hardening implemented, engagement conditional on #7264 landing,
deny-map pending S4.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Hayt and others added 4 commits September 3, 2026 13:37
Fixes all 21 warnings introduced in the NipFiAdmission refactor commit:

- map_identity: remove .map_err(|resp| resp) identity transforms at six
  admit_nip_fi_http_on_state call sites (bridge.rs x4, gifs.rs, workflows.rs)
- question_mark: rewrite if-let-return pattern in git/transport.rs to ? operator
- redundant_closure: .map_err(|class| http_denial(class)) -> .map_err(http_denial)
  in nip_fi_http.rs
- elide_lifetimes: remove explicit 't lifetime from PairingMockVerifier test impl
- result_large_err: add #[allow] with justification comment to all function/impl
  sites that return Result<_, Response<Body>>. Response<Body> is intentionally
  large (axum's design); the Err variant IS the HTTP response. Per Paul's
  direction: prefer #[allow] with comment over boxing or signature change.
  Added #![allow] to nip_fi_http test module (inner attribute).
- must_use on discarded admission in git/transport.rs: let _ = ...?

No behavior change. cargo clippy --all-targets: 0 warnings.
24 NIP-FI unit tests: 24 passed, 0 failed.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…lers

Resolves T1-IMP1 residual: verify_bridge_auth / verify_bridge_auth_with_options
were pub(crate), letting any future protected handler obtain a bare verified
PublicKey without producing NipFiAdmission.

Shape chosen: narrow/split (Option 2 from Paul's direction).

  verify_bridge_auth and verify_bridge_auth_with_options are now private
  (fn, no visibility) to bridge.rs. Code outside bridge.rs cannot call
  them; there is no general-purpose pub(crate) raw verifier.

Three pub(crate) replacement entry points, each with a structural role:

  make_nip98_closure_for_admission(headers, method, url, body, ...) ->
    impl FnOnce() -> Result<(PublicKey, ([u8;32], Option<u64>)), Response>

    For admitted surfaces outside bridge.rs (gifs, workflows, invites
    mint). Returns a closure that is the direct argument to
    admit_nip_fi_http_on_state. The pubkey inside the closure result is
    never projected outside NipFiAdmission. Callers outside bridge.rs
    cannot project a bare PublicKey; they pass the opaque closure to
    the admission gate. [FI-TRACE-AUTHORITY-UNIFORM]

  verify_nip98_exempt_invite_claim(headers, method, url, body) ->
    BridgeAuthResult  [FI-TRACE-AUTHORITY-EXEMPT]

  verify_nip98_exempt_operator(headers, method, url, body) ->
    BridgeAuthResult  [FI-TRACE-AUTHORITY-EXEMPT]

    Named exempt entry points for the two pre-NIP-FI paths that run
    outside the NIP-FI state machine. Exemption is nameable and
    greppable via [FI-TRACE-AUTHORITY-EXEMPT].

Falsifier: a new handler outside bridge.rs that calls
verify_bridge_auth_with_options fails to compile (private). A handler
that calls make_nip98_closure_for_admission and invokes the closure
directly still gets Result<(PublicKey,...), Response>, but must
explicitly invoke and unwrap it rather than calling a named verifier
directly; this is detectable by review/grep and no longer accidental.

Bridge-internal admitted handlers (submit_event, query_events,
count_events, authorize_moderation_read) continue calling the private
function inside closures defined in bridge.rs -- no change needed.

cargo clippy --all-targets: 0 warnings.
24 NIP-FI unit tests: 24 passed, 0 failed.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…mplete

Introduce Nip98Proof<X> in nip_fi_http.rs: pub(crate) struct with a
private pubkey field and a pub(crate) constructor (new). Only
admit_nip_fi_http (same module) can destructure the key via its own
module-private access — no code outside nip_fi_http can read or project
the proven pubkey.

make_nip98_closure_for_admission now returns
impl FnOnce() -> Result<Nip98Proof<([u8;32], Option<u64>)>, Response<Body>>
instead of the bare (PublicKey, X) tuple. Calling the closure directly
yields an opaque Nip98Proof — projection is a compile error. The only
way to obtain a proven pubkey is through NipFiAdmission::proven_pubkey(),
which is produced exclusively by admit_nip_fi_http.

Updated all closure call sites:
- bridge.rs internal closures (x4): .map(|auth| Nip98Proof::new(...))
- make_nip98_closure_for_admission return type
- media.rs (x3): || Ok(Nip98Proof::new(proven_pubkey, ()))
- git/transport.rs: || Ok(crate::nip_fi_http::Nip98Proof::new(pubkey, ()))
- nip_fi_http.rs test closures (x5)

cargo clippy --all-targets: 0 warnings.
24 NIP-FI tests: all pass.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…rcement

* origin/main:
  feat(relay): add early startup lifecycle logs (#7258)
  docs(nip-fi): add Blossom kind-24242 media possession-proof exception (#7278)
  fix(desktop): wrap message tables within the available pane (#7279)

Signed-off-by: Hayt <9e1c23a3fd83f61da34420e4e88ff1b16e45cafcc0cd9019eb07d4ecfa8ca9b0@buzz.block.builderlab.xyz>
@wpfleger96
wpfleger96 marked this pull request as ready for review September 3, 2026 22:11
@wpfleger96
wpfleger96 requested a review from a team as a code owner September 3, 2026 22:11

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Changes required

Reviewed head d28fd597b056180726c6236eedee6491e4b3e9e2 against base 88687876f7808a2fd742b7eb2e4b9f87d999ad8d. The delivery contract is per-request assertion/possession pairing on protected HTTP surfaces, fail-closed admission, and preserved Off behavior. The following findings are source-traced reproductions, not executed tests.

1. [P1] Reject a payload tag with no digest before admitting a body

bridge.rs:1162–1172

In Enforce mode, send a fresh, correctly signed NIP-98 event with valid u/method but a one-element ["payload"] tag, a valid same-key assertion, and an otherwise admitted member. The presence check at bridge.rs:115–125 accepts the tag's kind. verify_nip98_event then obtains None from tag.content() and skips hashing at nip98.rs:164–174. /query proceeds to parse and execute the supplied filters with no signed body digest. The pinned nostr 0.44.7 parser accepts this tag shape; it only rejects an empty tag array. Replay detection does not prevent substitution before the proof's first use. This is not a signature bypass or access beyond the principal's permissions, but it defeats the new mandatory body binding. The permissive helper pre-exists; S5 newly depends on it for /events, /query, and /count. Require exactly one valid lowercase SHA-256 value and compare the consumed body; include the one-element tag regression.

2. [P2] Reject repeated Authorization fields in the protected admission path

bridge.rs:194–205

With a valid paired assertion, append a second Authorization field after a valid first Nostr proof. The delegated verifier uses HeaderMap::get at bridge.rs:97–100, so only the first field is examined; the admission function validates cardinality only for Nostr-Federated-Identity. An otherwise valid /query request is admitted despite the ambiguous credentials. Exact-base NIP-FI.md:695–700 and 842 require repeated Authorization fields to deny. The old parser is unchanged, but its new Enforce use does not establish that contract. Validate the complete Authorization field set before selecting a proof, while retaining legacy Off behavior.

3. [P2] Convert proof failures to the NIP-FI denial classes

nip_fi_http.rs:257–261

With a valid assertion, POST /query without Authorization returns the old JSON 401 {"error":"missing Nostr auth"}, without the required WWW-Authenticate: Nostr, instead of authentication required\n. A malformed Nostr proof also returns legacy JSON 401 diagnostics rather than the required fixed 403 evidence rejected\n. The closure returns an arbitrary legacy Response and ? forwards it unchanged; the router guard only checks the assertion and cannot normalize these failures. This breaks the advertised uniform denial protocol and exposes detailed verification reasons. Map proof extraction/verification failures through DenialClass in active mode, retaining the old responses only in Off. Test both valid-assertion/missing-proof and valid-assertion/invalid-proof through the real router.

4. [P2] Schedule JWKS refresh from completed freshness, not the pre-fetch timestamp

main.rs:587–596

The loop saves now before awaiting the fetch, whereas ProductionJwksSource timestamps fetched_at after fetching. For an allowed refresh=60s/hard-deadline=90s configuration and ~200ms successful fetches, a refresh completing at t≈60.2 makes the scheduled t≈120 call too early. get_snapshot returns its old cache without fetching, but the loop still postpones the next call to t≈180. The snapshot expires at t≈150.2, producing roughly 30 seconds of 503 responses despite a healthy issuer. The synchronous request verifier never refreshes it. Cache semantics pre-exist; this production scheduler is new. Use completion/cache due time and cover nonzero fetch latency plus a hard deadline between one and two intervals.

5. [P2] Keep private issuer identifiers out of the new logs

main.rs:545–554

Every successful Enforce startup logs the configured exact iss at info level; warm failures and background refresh failures also log it. NIP-FI.md:777–779 explicitly excludes iss from logs, metrics, and traces. This is a deterministic new privacy disclosure during normal operation, not a hypothetical debug path. Remove the issuer fields from these logs and the refresh warning at main.rs:590–594; also avoid embedding the raw issuer in the new config error at nip_fi_config.rs:178–182. Use stable non-identifying diagnostic codes instead.

6. [P2] Preserve admin SPA document routes when enabling the guard

router.rs:416–420

Enable Enforce with the existing admin web bundle, then open or reload https://<admin-host>/reports, /reports/<id>, or /feedback. The new layer wraps the SPA fallback, but its exemption list omits these document paths. A normal browser navigation has no assertion header, so it gets 401 before is_admin_spa_path can serve the bundle; DenyProtected returns 503. The admin API itself is explicitly exempt and / still loads, making deep links/reloads fail selectively. Preserve the existing host-qualified admin document/static classification without broadly exempting protected tenant routes, and test direct navigation through build_router.

Coverage and limits

Source-only review of the changed auth/config/state/startup authority and the production route inventory: bridge, GIFs, workflow/moderation reads, invite mint/claim distinction, media aliases/reads, Git smart HTTP, operator/admin routes, public probes/discovery, SPA fallback, and middleware ordering. Assertion/JWKS and media/Git received independent review lanes, both completed before publication. No checkout, build, test execution, or live relay probing was performed; CI is not claimed as validation.

The exact-base spec explicitly records the pending S4 deny-map and bounded Blossom-verifier hardening gaps (NIP-FI.md:675–684); these are not new blockers here. S3 WebSocket enforcement is separate. Duplicate payload tags are already rejected by the shared verifier; finding 1 concerns a missing value, not duplicate tags. Non-blocking compatibility note: GIF authentication now precedes the unconfigured-provider check, so an unauthenticated Off-mode request can change from 404 to 401; the byte-identical Off claim needs qualification. Git client assertion-header delivery was not exercised and is not reported as a demonstrated defect in this server-only change.

Regression coverage should use a valid assertion so the outer assertion-only guard cannot mask a missing handler pairing/proof gate.

Hayt and others added 2 commits September 21, 2026 19:20
F1 [P1] nip98.rs: reject one-element payload tag with no digest.
A `["payload"]` tag with no content was silently treated the same as
a missing tag, skipping the body-hash check. Fix: validate that the
tag content (when present) is exactly 64 lowercase hex chars; add two
regression tests (one-element-no-content treated as absent, malformed
digest rejected with 3 sub-cases).

F2 [P2] bridge.rs: reject duplicate Authorization header fields.
`HeaderMap::get` takes only the first value; a second credential field
would be silently ignored. Fix: cardinality check on the Authorization
header in verify_bridge_auth_with_options when require_auth_token is
true; Off-mode legacy behavior unchanged.

F3 [P2] nip_fi_http.rs: remap NIP-98 closure failures to NIP-FI
DenialClass in active modes. Legacy JSON 401/403 responses from the
NIP-98 verifier are now replaced with the appropriate DenialClass
response when mode is Enforce or DenyProtected:
- Absent Authorization → MissingEvidence (401)
- Present-but-invalid → EvidenceRejected (403)
Off mode propagates the closure response unchanged ([FI-INV-15]).
Add three tests with falsifier mutation evidence.

F4 [P2] main.rs: schedule JWKS refresh from post-fetch instant, not
pre-fetch. `*last = now` (pre-fetch) drifted the interval backward by
the fetch latency on every cycle. Fix: `*last = tokio::time::Instant::now()`
after the get_snapshot() call.

F5 [P2] main.rs + nip_fi_config.rs: replace raw `iss` in logs and
config error messages with non-identifying diagnostic codes
(issuer_index). NIP-FI.md:777-779 prohibits iss from appearing in
logs.

F6 [P2] router.rs: admin SPA document routes (/reports, /feedback) are
now exempt from the assertion guard when the request is on the admin
host only. They are NOT added to NIP_FI_EXEMPT_PREFIXES (that would
broadly exempt them on tenant hosts). The guard uses is_admin_spa_path
+ api::admin::is_admin_host to conditionally exempt them. Add test
proving the paths are admin-SPA-classified but not broadly exempt.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…rcement

* origin/main: (87 commits)
  feat(relay): add admin HTTP routes for member restriction management (#7302)
  fix(relay): fire kick live side effects at convergence; persist target; fence re-add race with held lock (#7298)
  feat(relay): add atomic complete read-state snapshots (#7572)
  fix(desktop): register macOS badges for new and existing installs (#7783)
  fix(mobile): avoid opening empty threads on message tap (#7756)
  fix(workflows): make deletion persistent and retryable (#7735)
  fix(mobile): preserve thread replies through refresh failures (#7757)
  fix(mobile): keep iOS message menu actions responsive after rebuilds (#7758)
  fix(relay): exclude ephemeral activity from message quota (#7736)
  release: push gateway chart 0.3.1 (#7749)
  fix(push): label plaintext push gateway service as HTTP (#7717)
  Replace personal and internal data in desktop test fixtures (#7748)
  Add mobile VISION (#7710)
  fix(mobile): keep relay sessions stable during push lease updates (#7745)
  fix(desktop): keep managed agent avatars usable across communities (#7732)
  fix(mobile): fail open when age checks are unavailable (#7714)
  fix(ci): don't run desktop tests for purely mobile client changes (#7709)
  fix(mobile): temporarily disable age gating (#7708)
  feat(db): expose connection setup metrics (#7286)
  Isolate S3 storage metrics from the relay (#7543)
  ...

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>

# Conflicts:
#	crates/buzz-auth/src/nip98.rs
#	crates/buzz-relay/Cargo.toml
…discovery, test matrix)

Addresses all three IMPORTANT blockers and the MINOR from Thufir pass 2/3:

1. Git Off-mode error-precedence fix (FI-INV-15):
   - Extract parse_git_auth_header / parse_git_auth_header_full helpers
   - Add Off-mode early-exit in GitAuth::from_request_parts that fires BEFORE
     bind_community() — restores pre-NIP-FI precedence: missing/malformed
     credentials -> 401 + WWW-Authenticate, no DB work
   - Unit tests: 5 cases in off_mode_precedence_tests (missing header, wrong
     scheme, invalid base64, invalid UTF-8, valid positive control)

2. PostgreSQL test discovery (mod rename + jsonwebtoken dev-dep):
   - mod nip_fi_seam -> mod postgres_tests under #[cfg(test)]
   - Add jsonwebtoken = { ..., features = ["use_pem"] } to [dev-dependencies]
     so EncodingKey::from_ec_pem is available in test builds

3. Settings build_router key-pairing matrix:
   - enforce_state_with_verifier() factory with real FederatedAssertionVerifier
     seeded from static P-256 PKCS#8 PEM constants
   - off_state() factory for Off-mode tests
   - mint_assertion() helper minting valid ES256 NIP-FI JWTs
   - 4 new ignored Postgres tests through build_router:
     key-A/key-B mismatch -> 403, same-key -> not-403,
     GET-token-on-POST -> 401, Off mode -> not-401

4. Actual-caller cardinality test in bridge.rs:
   - r3_cardinality_actual_caller_query_off_passes_enforce_denies exercises
     /query route with duplicate Authorization headers in both modes

5. Privacy sentinel tests in nip_fi_config.rs:
   - malformed JSON, invalid algorithm, policy-build rejection — each embeds
     a unique sentinel string and asserts it never appears in the error

6. Scheduler test corrections:
   - Fix overclaiming matrix comment (test B description corrected; test C added)
   - Add jwks_refresh_interval_is_cadence_only_not_hard_deadline (Test C)
   - Clarify .is_some() contract and hard_deadline ownership in comments

7. Admin SPA full matrix in router.rs:
   - build_router_admin_spa_full_matrix: /reports, /reports/<id>, /feedback
     x Enforce/DenyProtected x admin/tenant host = 12 cases
   - Fix TempDir lifetime bug (dirs must outlive state, not the helper fn)

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Changes requested

Re-review of 7373d702b4f355f218d7aeec036f14ef00606139 against base 77729abfb692b25a0f4ec4a69add86af2e32c0dd, focused on the four prior findings and the two corrective commits after ab42efba8229d8acab68645889a96f02f05f792b.

[P2] Media proof failures still bypass the fixed denial mapper

The shared mapper cannot normalize a failure that never reaches it. AuthenticatedUpload still returns Blossom extraction/verification errors directly as MediaError, before upload_blob calls admission. Media reads do the same, before the shared gate in GET/HEAD.

Source-traced reproduction: enable Enforce, use a mapped tenant and a valid NIP-FI assertion, then call PUT /upload (or /media/upload) or GET /media/<64-lowercase-hex>.png without Authorization. The assertion guard forwards, but the specialized extractor returns 401 {"error":"authentication failed"} without a Nostr challenge. With Authorization: Nostr !!!, it also returns JSON 401. These responses follow directly from MediaError::into_response, rather than the required missing-proof 401 authentication required\n plus WWW-Authenticate: Nostr, or invalid-proof 403 evidence rejected\n. HEAD has the same status/header defect. A malformed first Authorization field plus a second field likewise exits before the cardinality mapper. This breaks the uniform authentication challenge/status contract for media clients even though the requests are denied.

This is the remaining media portion of the prior specialized-denial finding, not a new Blossom-hardening requirement. Put media proof extraction/verification inside the active-mode admission boundary, or equivalently map its pre-admission proof failures there, while preserving legacy Off behavior. Add real-router cases with a valid assertion for missing/malformed/repeated evidence and matching-key controls; assert status, challenge/content type, and exact bodies where HTTP permits a body. The exact-base denial contract requires this distinction.

[P2] The new default-branch POST regression expects the wrong denial

settings_build_router_enforce_post_requires_correct_method_and_payload sends a GET-signed proof without a payload on POST, then asserts UNAUTHORIZED at lines 657–664. Authorization is present, so the production remapper returns 403 evidence rejected\n, not 401. The newly added integration witness therefore disagrees with the intended production behavior and will fail when run with its prerequisites. It is #[ignore]; this review did not execute it or attribute any CI failure to it.

Expect exact 403 for invalid presented proof, and separate the wrong-method and missing/wrong-payload cases so either guard's removal is detectable. Include a same-key, correct-method, exact-payload POST control that reaches downstream repository handling. The production settings pairing/body-binding repair is credited; this finding is about the contradictory new regression test, not a remaining pairing bypass.

Disposition and validation

  • Retired: default-branch A/B key-pairing bypass, valid-first duplicate Authorization admission, and the malformed-Unicode payload diagnostic panic. Git/settings proof failures now reach the common mapper; only media remains open from that finding.
  • Preserved: prior accepted payload/JWKS/redaction/admin-host repairs, the Off-mode GIF ordering exception, S3 WebSocket exclusion, deferred S4 deny-map integration, named Blossom tag/window/server gaps, and the smart-HTTP-only method/URL/payload exceptions. No unrelated hardening is required here.
  • Source-only on pinned Blox: exact-base instructions, vision, architecture, TESTING, NIP-FI contract, affected routes and corrective tests reviewed. No checkout/build/test/runtime probe or CI rerun. One exact-head CI snapshot showed Rust Lint and Windows Rust failures, with Rust units and some desktop jobs still in progress; not a green-CI claim.

Bucket A — CI-red fixes:
- A1: Remove identity .map_err(|r| r) from transport.rs:159
- A2: Add #[allow(clippy::result_large_err)] to parse_git_auth_header
- A3/A3b: Split settings_tests.rs into 3 distinct tests (wrong-method,
  missing-payload, same-key POST success) with correct expectations
- A4: Rewrite bridge.rs cardinality test with real FederatedAssertionVerifier
  seeded from P-256 PKCS#8 PEM + single-header positive control

Bucket B — Carl media proof bypass:
- Fix get_blob/head_blob to run admit_nip_fi_http_on_state before serving
- Fix upload_blob to use UploadContext extractor instead of AuthenticatedUpload;
  move admission inside the handler boundary for all three media routes

Bucket C — Thufir evidence-matrix corrections:
C.1: Fix false ordering comment in transport.rs:95-98; fix mutation comments
     on 5 parser-only tests; add 4 router-level postgres tests proving Off-mode
     early-exit fires before bind_community (unmapped host × missing/wrong-scheme/
     invalid-base64 = 401, valid-syntax = 404)
C.2: Add settings postgres_tests with enforce_state_with_verifier: wrong-method
     (403), missing-payload (403), same-key POST success (reaches handler), Off
     mode GET reaches handler
C.3: Upgrade /events, /count, moderation/reports, workflow/runs tests to use
     oneshot_request_full with exact body / content-type / WWW-Authenticate
     assertions; add enforce-mode git info/refs test with exact contract bytes
C.4: Add composition_log_does_not_leak_issuer_url: sentinel in issuer URL and
     JWKS URI must not appear in captured tracing output from ProductionJwksSource
C.5: Add composition_tests module in main.rs: A) nonzero fetch latency + not-due
     cache hit with ProductionJwksSource<ScriptedJwksFetcher> + timer loop,
     B) failure does not advance snapshot generation; also expose ScriptedJwksFetcher
     and ProductionJwksSource::new_with_clock under test-utils feature in buzz-auth
C.6: Fix build_router_admin_spa_full_matrix: check HTML body for 200, exact
     b"authorization unavailable\n" for 503 DenyProtected, exact 401 +
     b"authentication required\n" + WWW-Authenticate: Nostr for Enforce tenant

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Changes requested

Corrective re-review of e7fe4ea7f87adc3404521c9ba5c90712cb4eccbd against base 77729abfb692b25a0f4ec4a69add86af2e32c0dd, limited to 7373d702b4f355f218d7aeec036f14ef00606139..HEAD and the two prior exit criteria. The media production repair is credited. Two required regression witnesses remain incomplete, and one changed regression now contradicts the valid Off-mode path.

[P2] Correct the Off-mode cardinality test’s newly hard-coded 401

r3_cardinality_actual_caller_query_off_passes_enforce_denies repeats a valid POST proof for the exact tenant URL and body [], then newly asserts UNAUTHORIZED. The fixture helper includes the correct payload hash. In Off mode the cardinality gate is bypassed and verify_bridge_auth_with_options authenticates the first header successfully. require_auth_token = false does not make a valid presented proof fail, nor require an x-pubkey alongside it.

Source-traced reproduction: run this regression with its Postgres/Redis prerequisites and ordinary test configuration. Its Off state disables membership and installs AlwaysFreshReplayGuard; the seeded tenant and empty query proceed downstream to 200 [], not the asserted 401. That assertion fails before the same test reaches its Enforce duplicate-header case and matching-key control. Expect the real downstream success (preferably exact status/body) for this valid Off fixture, retaining the Enforce denial/control checks. This is a newly broken test, not a reopened production duplicate-header bypass.

[P2] Finish the already-requested active-mode media router witnesses

The new upload admission closure and read admission closure repair the prior denial-mapping defect. Both upload aliases, GET and HEAD now reach the common mapper; membership/upload limits still precede body consumption, and the Off-mode failure sequence is preserved.

However, this corrective delta changes no media tests. The existing media test state/router neither selects Enforce nor installs an assertion verifier, and builds a synthetic router without the production assertion guard. Its missing/invalid proof tests therefore do not distinguish the repaired closure placement from the old pre-admission extraction: Off propagates the same legacy error in both shapes. The inspected exact-head route/test inventory contains no active-mode media request providing the missing witness.

Add the previously requested real-router Enforce cases with a valid assertion: missing, malformed and repeated proof plus matching-key downstream controls for upload aliases and GET/HEAD. Assert exact status, challenge/content type and bodies where HTTP permits one; retain Off controls. Hoisting proof extraction back before admission must make these tests fail. This closes the existing media exit criterion, not a request for broader Blossom hardening.

[P2] Give the settings wrong-method case a valid payload tag

settings_build_router_enforce_post_wrong_method_is_403 still calls nip98_get_token, whose tags omit payload. POST requires payload, so the bridge’s missing-payload guard returns before method verification. Removing method checking still leaves this Enforce test green: it duplicates the missing-payload rejection rather than independently testing method binding.

The exact 403/body correction, separate missing-payload case, and same-key valid POST control are credited. Finish the remaining isolation requirement by constructing the wrong-method proof with nip98_token_for_method(&key, &url, "GET", Some(post_body)), keeping the exact body, URL and assertion key otherwise valid. Existing Off-mode method coverage is useful but does not complete this Enforce-router witness. No production method-binding bypass is alleged.

Disposition and validation

  • Stable scope: pairing, valid-first duplicate Authorization admission, and Unicode diagnostic panic remain retired. Prior accepted payload/JWKS/redaction/admin-host repairs, Off GIF ordering exception, S3 WebSocket exclusion, deferred S4 deny-map integration, named Blossom gaps, and smart-HTTP-only exceptions remain unchanged. No unrelated hardening is required.
  • Evidence: source-only analysis on pinned Blox Blox2849674; exact-base guidance and corrective call paths reviewed, with independent media, settings/non-media and metadata lanes. No checkout, build, test, runtime probe or CI rerun. The test outcomes/mutations above are source predictions, not executed results.
  • CI: one exact-head snapshot at 2026-09-22T15:38:12Z: 52 checks, 25 successful, 19 skipped, 8 in progress, none failed at that instant. No monitoring or final-green claim.

Address all 7 IMPORTANTs and MINORs from Thufir's delta review at e7fe4ea:

IMPORTANT fixes:
- Git missing-assertion test: sign repo-root URL (not full info/refs path); pin
  relay_url so git_expected_url() derives http:// deterministically; correct
  mutation comment (legacy NIP-98 accepts, not 'bad payload hash')
- Bridge Off cardinality: replace duplicate-valid-NIP-98-expects-401 with
  single/duplicate Off controls that assert NOT 403 (cardinality must not fire);
  add same-status equality assertion. Add genuine same-key assertion (keys2
  pubkey matches assertion nostr_pubkey); positive control asserts not-403 and
  not-401 before duplicate-header cardinality assertion.
- Media regression suite: 8 postgres tests covering upload/GET/HEAD in Enforce
  and Off modes (missing/malformed/duplicate proof, valid-Blossom-no-assertion)
- Settings: fix wrong-method token to use nip98_token_for_method with payload
  hash (isolates method mismatch from missing-payload); add 503 guard on POST
  positive control; add POST key-mismatch test (403 authorization denied + exact
  CT + no WWW-Authenticate); update helper return types to include headers
- Actual callers: fix /events outer-guard comment to accurately describe outer
  nip_fi_assertion_guard (not per-handler admit gate); remove overclaiming
  falsifying mutation claim
- Privacy: lowercase SENTINEL so JwksSourceContract canonicalization doesn't
  defeat the URI-leak check; add warn-captured assertion; exercise background
  timer warn! path (nip_fi_jwks_refresh_loop) with start_paused runtime
- Scheduler composition A: define callback_start_count Arc<AtomicUsize>,
  increment at callback entry to distinguish timing mutation from fetch mutation;
  fix 'does NOT advance last' comment (loop unconditionally advances last after
  callback). Composition B: add claims 3+4 (snapshot expiry at T=91, recovery
  fetch); fix https://https:// double-scheme fixture URL (ISSUER constants now
  bare hostnames); clarify T=91 comment. Fix ScriptedJwksFetcher doc (queue
  holds immediate values, not futures/latency)

MINOR fixes:
- Git: add bad-UTF8 test (non-UTF8 base64 payload → 401 'invalid utf-8', no
  WWW-Authenticate); add mapped-host missing-auth and invalid-base64 tests;
  add full body/CT/challenge assertions to unmapped-host tests; fix stale line
  references (100-102 → 101-104); note that bad-base64/bad-UTF8 do NOT carry
  WWW-Authenticate (use into_response())
- Admin SPA: distinct admin bundle with data-bundle=admin sentinel; exact HTML
  body byte assertions; Content-Type assertions on 200
- Delete dead AuthenticatedUpload::FromRequestParts impl (media.rs:168-268);
  update UploadContext comment to remove reference to deleted impl; fix test
  mutation comment to reference NIP-FI admission skip, not old extractor
- PR body: remove false WWW-Authenticate/identical-challenge claim for Off-mode
  GIF auth (legacy api_error() produces JSON without WWW-Authenticate); correct
  'Handler-level' to 'Outer-guard' for the seam tests; fix mutation claim for
  /events test

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
transport.rs:4235: collapse bad_utf8_b64 assignment onto one line
media.rs:1686: reflow dyn Future bound across three lines
media.rs:2319: expand assert_eq! to multi-line form

No logic change.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Changes requested

Corrective re-review of b29aa0ee94f368158ab14f212c96ecf8f0a6e4d2 against base 77729abfb692b25a0f4ec4a69add86af2e32c0dd, limited to the two commits after e7fe4ea7f87adc3404521c9ba5c90712cb4eccbd and the previous exit criteria. The Off-mode cardinality assertion and settings wrong-method isolation are repaired. The media witness requirement is partially complete; changed JWKS and Git response tests also now contradict their production paths.

[P2] Expire the source snapshot before requiring the timer’s no-snapshot warning

In composition_log_does_not_leak_issuer_url, the successful warm gives the snapshot a source-clock deadline of T0+90. The only later write to clock_secs is T0+61 at line 3166. Advancing Tokio time at line 3194 does not advance that independent atomic clock.

Consequently, even if the timer callback runs, an exhausted/failing scripted fetch returns the still-live cached snapshot: get_snapshot preserves it on failure and returns Some while source time is before its deadline. The callback returns true, so the timer warning cannot execute. The new assertion at lines 3213–3219 therefore fails when this test runs. Merely yielding before the time advance does not fix the live-snapshot problem.

Repair: move the source clock beyond its hard deadline for the timer phase, keep a failing fetch, and synchronize loop initialization/callback completion before cancelling. Retain positive assertions that both warning sites were captured and the sentinel is absent. This is a test-fixture regression, not a request to change production cache fallback or redaction behavior.

[P2] Match the legacy missing-auth response instead of requiring an absent Content-Type

The changed off_mode_unmapped_host_missing_auth_returns_401_before_db now requires Content-Type to start with text/plain. The missing-header producer uses Response::builder().body(Body::from(...)) and sets only WWW-Authenticate; it does not use the tuple/string IntoResponse path that supplies a content type. The fixture invokes git_router directly, whose only layer is the request-body limit.

Source-traced reproduction: with the test's database/Redis prerequisites, its missing-auth request returns the intended 401/body/challenge but no Content-Type. unwrap_or("") therefore makes the new assertion at lines 4148–4151 fail. Assert the actual legacy header absence while retaining the status/body/challenge checks; do not alter production Off-mode response bytes to make a new test expectation pass.

[P2] Finish the existing media witness matrix and same-key downstream controls

The new real-build_router fixture and active missing-proof witnesses are useful and credited. However, the complete new media.rs::tests::postgres_tests does not yet satisfy the previously requested matrix:

  • All upload requests use /upload, not the separately supported /media/upload alias.
  • GET and HEAD cover missing proof, but not malformed or repeated proof. HEAD has no Off control.
  • There is no valid assertion + single valid same-key proof request that demonstrably proceeds downstream for upload, GET, or HEAD. The upload helper is used only in the duplicate-header denial; the valid GET proof is sent without an assertion and stops at the outer guard.

Thus these denial cases do not distinguish correct admission from a handler that maps missing proof to 401 and rejects every present proof with 403. Several of the previously requested wire assertions are also missing: the duplicate-upload case ignores response headers, and the GET/HEAD missing-proof cases do not assert content type.

Repair: complete the existing fixture with table-driven requests over both upload aliases and GET/HEAD: valid assertion plus missing/malformed/repeated proof, and same-key valid single-proof downstream controls; retain the Off controls and exact status/challenge/content-type/body contract where HTTP permits a body. The downstream control need not store media: a deterministic post-admission error distinct from NIP-FI rejection is sufficient. This is the same outstanding media exit criterion, not broader Blossom hardening or a reopened production denial-mapping defect.

Disposition and validation

  • Credited: Off /query now compares duplicate proof with its valid single-proof control instead of requiring an incorrect 401. Settings wrong-method now includes the correct POST body hash. The media production fix and prior pairing, duplicate-header, Unicode, payload/JWKS/redaction/admin-host fixes remain credited. Intentional Off GIF ordering, smart-HTTP-only exceptions, S3 exclusion, deferred S4 integration, and named Blossom gaps remain unchanged.
  • Evidence: source-only review on pinned Blox2854805, including independent metadata, Git/settings, media, and JWKS-test review. No checkout, build, test, runtime probe, CI rerun, or monitoring. Test-failure and mutation outcomes above are source predictions, not executed results.
  • CI: one exact-head snapshot at 2026-09-22T17:47:39Z: 34 passing, 3 pending, 19 skipped, 0 failed/cancelled. This is not a final-green claim.

Address all 7 IMPORTANTs and MINORs from Thufir's delta review at b29aa0e:

IMPORTANT fixes:
- Fix test compilation: add http-body dev-dep, warm_nip_fi_jwks_snapshots to
  composition_tests use block, Arc::make_mut for config mutation in settings test
- Scheduler A: correct falsifying mutation comment (T≈71 → T≈120; pre-fetch
  last=T60 yields next_due=T120, post-fetch last=T70 yields next_due=T130)
- Bridge cardinality: require_auth_token=true Off state, valid-first/invalid-second
  dup (distinguishes first-value semantics), exact same-key success (200 []),
  exact duplicate denial with CT + challenge-absent headers
- Pack routes: add invalid base64 assertion → 403 EvidenceRejected + admitted
  same-key success → not-401/not-403 NIP-FI for both upload-pack and receive-pack
- Settings state re-read: Arc::make_mut fix enables the NIP-FI Enforce + verifier
  injection into the real repository fixture (already present but broken to compile)
- Media false single-layer falsifier: correct mutation comment — both outer guard
  and handler-level admit_nip_fi_http_on_state deny 401; removing only one layer
  is insufficient

MINOR fixes:
- Scheduler A comments: T≈71 → T≈120/T≈130 (pre-fetch vs post-fetch last timing)
- Mapped-host mutation comments: relabeled as compatibility controls (early-exit
  wrapper and active-mode closure both route through parse_git_auth_header_full;
  removing wrapper still yields same result; falsifying mutation updated)
- Mapped-host challenge assertion: exact WWW-Authenticate: Nostr realm="buzz",
  method="GET" (was contains("Nostr"))
- Transport.rs:346 doc comment: correct to distinguish missing/scheme (with challenge)
  vs bad-base64/bad-UTF8 (without challenge via into_response())
- Admin SPA tests: exact text/html; charset=utf-8 assertion (was starts_with);
  tenant DenyProtected 503 CT + challenge-absence; Enforce tenant 401 CT
- Stale ref: transport.rs:65 already updated (UploadContext); media old-extractor
  text already removed
- Bridge: single-layer falsifier comment for /events already corrected;
  MediaError/authentication-failed story already removed

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Address remaining IMPORTANT items from Thufir's pass-5 verdict at b29aa0e,
not yet closed in c76d83e:

IMPORTANT fixes:

- Scheduler B (composition_failure_does_not_extend_snapshot_freshness):
  bridge source clock to Tokio paused clock via t0_instant/t0_utc offsets so
  both nip_fi_jwks_refresh_loop and ProductionJwksSource share one timeline;
  spawn timer at T=61, advance to T=122, wait for callback_count >= 1 to prove
  the timer callback observed None (source past deadline=T90); also prove via
  direct source call; claim 4 recovery at T=123.  Correct falsifying mutation
  (extend deadline on failure, not generation check).

- Privacy timer C (composition_log_does_not_leak_issuer_url): move source +
  fetcher construction inside block_on so now_fn bridges the paused Tokio
  clock; spawn timer at T=0 before warm; wait for callback_count >= 1 (T=60
  callback, snapshot live → true) then >= 2 (T=120 callback, source past
  deadline → None → false → path-4 warn! fires); warm runs paths 1+2; direct
  call at T=61 runs path 3; path 4 assertion now has a producing path.
  Correct falsifying mutation description (fixed T=61 clock bridges).

- Settings state re-read (nip_fi_denied_assertion_does_not_advance_snapshot_digest):
  add step 3b — wrong-payload-hash denial (same key, valid assertion, NIP-98
  token bound to 'wrong body for hash mismatch', actual body = post_body_bytes)
  → 403 EvidenceRejected; step 4 now includes wrong-hash in the set of denials
  that must not advance the stored snapshot digest.

- Media Off matrix: add four Off-mode tests to complete the bounded matrix:
  - upload_off_malformed_auth_is_legacy_json_401: malformed Nostr token → Off
    mode propagates Blossom error as legacy JSON 401, not NIP-FI bytes;
  - upload_off_duplicate_auth_is_not_403: two valid Blossom tokens → Off skips
    cardinality gate → first value processed → admission passes (not 403);
  - get_blob_off_malformed_auth_is_legacy_json_401: same contract for GET path;
  - get_blob_off_duplicate_auth_is_not_403: two valid Blossom GET tokens → Off
    skips cardinality → admission passes → handler reached (not 403).

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Sep 22, 2026
@github-actions

github-actions Bot commented Sep 22, 2026

Copy link
Copy Markdown

🔐 Codex Security Review

Status: review required for the current range.

The current range is 312cf674fad5563c24a66ec5350d306d192ed64a...e722454efbbe33926749d0b449506a113d35492b.
A new review must complete for this exact range. When manual authorization
is required, a Block organization member must comment exactly
@buzz-security-review e722454efbbe33926749d0b449506a113d35492b to authorize a new review.
Any previous review applies only to its recorded range.

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Changes requested

Corrective re-review of 4f3bcb7f3bfac61f0ce3edd0e6fe20bf517f7b53 against base 77729abfb692b25a0f4ec4a69add86af2e32c0dd, limited to the delta after b29aa0ee94f368158ab14f212c96ecf8f0a6e4d2 and the prior exit criteria. The JWKS clock/synchronization repair is credited in source. Two prior criteria remain open, and the corrective tests introduce four additional defects. These findings concern regression coverage and failing fixtures, not newly demonstrated production authentication bypasses.

1. [P2] Preserve the actual legacy Git Content-Type behavior

transport.rs:4156–4163

The prior finding remains unchanged: off_mode_unmapped_host_missing_auth_returns_401_before_db requires text/plain, but the missing-header producer at lines 357–364 builds a response with only WWW-Authenticate, and this fixture's git_router adds no Content-Type. The intended Off response therefore has no Content-Type. Existing exact-head PostgreSQL CI fails at line 4160. Assert header absence while retaining the exact status/body/challenge checks; do not change production Off bytes to satisfy the test.

2. [P2] Finish the existing media matrix at the handler boundary

media.rs:2801–2871, same-key GET control, resource witness

The new GET/HEAD malformed/repeated-proof cases and GET same-key control are credited. However, the complete postgres_tests inventory still has no Enforce same-key downstream control for PUT or HEAD and no Off HEAD control. The alias case supplies no assertion, so the outer assertion-only guard returns 401 before either upload handler runs. Removing the alias binding or its handler admission would still pass this test. This does not finish the previously requested valid-assertion matrix over both upload aliases and GET/HEAD.

The new body/permit witness has the same masking problem: both requests omit the assertion. Moving the handler admission behind resource work cannot affect requests already rejected by the outer guard, contrary to the stated falsifying mutations. Supply a valid assertion and isolate the handler rejection, preserving all earlier prerequisites for the resource-order mutation being tested.

Exit criterion: finish the existing table-driven matrix with valid assertions, missing/malformed/repeated proof, and valid same-key single-proof downstream controls for both upload aliases and GET/HEAD; retain Off controls and exact applicable status/challenge/content-type/body assertions. Use a deterministic downstream result, not live storage. Repair the existing resource witness rather than adding another broad fixture layer. No wider Blossom policy change is requested.

3. [P2] Make the Off upload control's body match its signed hash

media.rs:2470–2509

upload_off_duplicate_auth_is_not_403 signs x = "b".repeat(64) and sends that header with an empty body, then asserts the result is not 401. Off correctly accepts the first Authorization value, but the buffered file-upload path hashes the actual body (buzz-media/src/upload.rs:81–85). The hash does not match the token, so verify_blossom_upload_auth returns HashMismatch, mapped to legacy 401. This happens before storage; the “without MinIO” comment does not describe this request. Exact-head PostgreSQL CI fails at line 2504 with 401.

Use matching body/hash/header evidence and an exact deterministic post-admission control, ideally comparing single and duplicated valid proof. Do not weaken production body-integrity enforcement or rely on an unavailable storage service for the expected result.

4. [P2] Install the Git fixture verifier before testing malformed assertions

transport.rs:4776–4794, state setup

The pack-route fixture loads ordinary Off-default configuration and changes only its NIP-FI mode to Enforce, leaving the issuer/JWKS configuration empty. AppState::new consequently has no assertion verifier. Bearer !!!not-valid-base64!!! passes envelope extraction, then admission checks verifier availability before decoding/verifying the token (nip_fi_http.rs:328–338). The actual response is correctly 503, not the expected 403; exact-head PostgreSQL CI fails at line 4788, before reaching the later same-key controls.

Inject the static verifier before these cases, not only in Case 4. Keep a separate exact 503 missing-verifier control if desired; do not change fail-closed production ordering to make this fixture pass.

5. [P2] Expect the admitted repository owner to update the default branch

settings_tests.rs:2235–2260

After verifying no writes for denied requests, nip_fi_denied_assertion_does_not_advance_snapshot_digest sends a valid same-key owner POST from seeded legacy to main, then requires the digest to remain unchanged because “Owner is not a maintainer.” But the announcement is signed by that owner (lines 1268–1279), and authorize_management explicitly accepts key == repo.pubkey (settings.rs:273–292). The admitted owner reaches the snapshot update. Existing exact-head backend-integration CI fails at line 2257 with different digests.

Keep the unchanged-digest checks after denials. For the owner positive control, assert exact success and the intended changed snapshot; alternatively use the existing non-manager member if testing a downstream authorization denial. Do not remove legitimate owner authority.

6. [P2] Point the new moderation positive control at the real route

bridge.rs:5373–5399

The new same-key moderation witness signs and requests /communities/{host}/moderation/reports. Production registers only /moderation/reports (router.rs:350–355), which is also the handler's signed path (bridge.rs:2621–2632); there is no tenant-prefix rewrite here. A valid assertion therefore reaches the fallback 404, not moderation admission. The test passes because it only excludes 401 and two denial bodies. Removing the moderation handler or making its pairing gate always deny would leave this “handler reached” witness green.

Request/sign the real path and assert a specific handler result. For the intended successful empty-queue control, seed actual moderation authority and assert 200 []; ensure_user alone does not grant the role claimed by the fixture comment.

Disposition and validation

  • Previously credited media production mapping, pairing/cardinality, Unicode/payload, JWKS scheduling/redaction, and admin-host repairs remain credited. The JWKS warning fixture now advances the source and timer on one clock and waits for callback completion. Its binary-target runtime result remains unverified: the inspected required unit/archive commands select the relay library, not these main.rs tests. That is a validation limitation, not an additional blocker in this review.
  • Existing PostgreSQL CI confirms findings 1, 3 and 4; backend-integration CI confirms finding 5, both at this exact head. Findings 2 and 6 are source-traced seam/mutation analysis, not executed mutations. Desktop smoke is also red but is not attributed to these changes or investigated as another requirement.
  • Source-only review on the pinned review-owned Blox host, with independent JWKS, media, and Git/settings lanes integrated and challenged. No checkout, build, test execution, live probe, CI rerun, or monitoring. Intentional Off GIF ordering, smart-HTTP-only exceptions, S3 WebSocket exclusion, deferred S4 integration, and named Blossom gaps remain excluded. Fix the listed fixtures/witnesses without reopening accepted production behavior or adding new infrastructure.

CI-red 1 (transport.rs): inject static NIP-FI verifier before ALL cases
requiring cryptographic validation in the pack-matrix test, not only
Case 4. Without the verifier, the malformed-assertion Case 3 reached the
absent-verifier check and returned 503 instead of the expected 403.
Hoisted verifier injection and same-key material to the top of the test;
Cases 5-7 (missing proof, malformed proof, dup proof) share the outer
same_key_assertion; Case 4 shadows it with an inner same-key pair.

CI-red 2 (transport.rs): add exact Content-Type oracle to the
off_mode_unmapped_host_missing_auth test (was only body + WWW-Auth);
added content-type header to parse_git_auth_header_full responses so
the oracle has a value to check.

CI-red 3 (media.rs): replace identical-value duplicate upload Off test
with valid-first / invalid-second structure. Phase 1 single-token control
proves admission passes; Phase 2 proves Off mode takes the first value.
Same fix for the GET Off duplicate test.

CI-red 4 / IMP1 (settings_tests.rs): correct step 5/6 of
nip_fi_denied_assertion_does_not_advance_snapshot_digest.  authorize_management
authorizes the repo author (f.owner) so the owner POST must return 200
with changed=true and a new digest.  Pin the three denial steps to their
exact bodies (authorization denied\n, evidence rejected\n).  Pin Enforce
GET same-key positive to exact 404 (not !=403).

IMP2 (transport.rs): add valid-assertion + missing/malformed/dup-proof
matrix (Cases 5-7) to the pack-route loop; add info/refs GET coverage
(missing assertion → 401, missing proof + assertion → 401, same-key → 404);
pin positive pack controls to exact downstream 404 + body.

IMP3 (media.rs): upload resource witness now passes the outer guard with
a valid assertion and rejects at handler admission (mismatched Blossom
key); instrumented CountingBody proves body is not polled; exhausted
global semaphore proves permit is not consumed; admitted same-key control
proves the boundary is reachable.  Alias handler test passes outer guard,
fires handler-level key-pairing denial.  Same-key upload/HEAD positives
added.

IMP4 (bridge.rs): GIF same-key positive pinned to exact 404 + JSON body;
moderation same-key positive pinned to exact 403 application denial body;
GIF/moderation/workflow mismatched-key witnesses added (key_a assertion +
key_b NIP-98 → 403 authorization denied\n); media Off dup upload/GET use
valid-first/invalid-second; same-key upload/HEAD positives added.

IMP5 (main.rs): replace unbounded while-loop yields with bounded
for-loops (10_000 iterations) that panic with a diagnostic if the
callback count never advances.

MINORs: remove stale skip-early-wrapper mutation claims from
transport.rs:4282-4306/4329-4352; complete unmapped-host wrong-scheme
CT + exact WWW-Authenticate oracle; fix bridge.rs GIF/moderation/workflow
no-assertion test comments from stale per-handler-removal claims to
correct outer-guard mechanism; fix B recovery comment falsifying mutation;
add Redis to bridge ignore label.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
@github-actions github-actions Bot removed the codex-security-review-current The posted Codex security review matches its recorded range. label Sep 22, 2026
settings_tests.rs: pin exact denial bodies to all three steps.  The
deny-then-check test now asserts exact body text for each step:
  - step 2 (key-mismatch): body_mismatch["error"] == "authorization denied\n"
  - step 3 (malformed NIP-98): body_malformed["error"] == "evidence rejected\n"
  - step 3b (wrong payload hash): body_wrong_hash["error"] == "evidence rejected\n"

transport.rs: separate git-receive-pack from git-upload-pack in the
same-key positive (Case 4).  upload-pack calls authorize_git_read →
repo absent → exact 404 + body.  receive-pack calls hydrate_for_write
which creates an empty bare repo then hits finalize_push/git-store →
non-NIP-FI response; assert_ne! on NIP-FI denial strings to prove
admission passed.  Both are falsified by always-deny pairing → 403.

bridge.rs: correct false single-layer falsifier claims in the
no-assertion tests for GIF and moderation.  The per-handler
admit_nip_fi_http_on_state also fires 401 on missing assertion, so
removing only the outer guard does NOT change the result.  Reworded
both comments to honestly say the test witnesses the outer guard fires
first; the same-key positives are the complement witnesses.

main.rs: delete the false "Note: removing only the early-expiry
clearing" from the composition B recovery assertion.  The note was
irrelevant to that section's falsifying mutation and potentially
misleading.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Changes requested

Bounded corrective re-review of 4f3bcb7f3bfac61f0ce3edd0e6fe20bf517f7b53e722454efbbe33926749d0b449506a113d35492b, against base 77729abfb692b25a0f4ec4a69add86af2e32c0dd. The remaining blockers are fixture correctness and the previously requested causal regression coverage, not newly demonstrated production authentication bypasses.

P2: Construct the assertion header without panicking

crates/buzz-relay/src/api/media.rs:3016,3460,3525,3740 passes buzz_auth::CLIENT_ATTACHED_HEADER ("Nostr-Federated-Identity") into HeaderName::from_static, which requires lowercase bytes. Three newly added controls panic before issuing their requests; the fourth occurrence is latent behind another failure in the permit witness. Use a valid lowercase static name or fallible header-name parsing consistently. Existing exact-head PostgreSQL CI confirms HeaderName::from_static with invalid bytes in the alias-mismatch, PUT-positive, and HEAD-positive tests. These controls currently exercise no admission behavior.

P2: Match upload bodies to their signed hashes before asserting admission success

media.rs:2477–2499,2533–2539 still signs "b".repeat(64) and sends an empty body. The new same-key PUT control repeats this with "4".repeat(64) at 3448–3470. Once past header construction, both reach the existing buffered upload hash check, which hashes the actual bytes and returns legacy HashMismatch/401 before storage. The Off single-token control already fails at line 2504 in exact-head CI; adding a second control did not repair the prior finding. Sign the actual body hash, then select an exact deterministic downstream outcome that does not depend on unavailable storage. Compare the single/duplicate Off responses at that outcome; retain production integrity checks and replace the PUT control’s broad “not 401/403” assertion.

P2: Give the body/permit witness a proof the upload handler accepts

media.rs:3615–3631 creates kind 27235, with empty content and NIP-98 tags, then reuses it for the mismatch, permit, and same-key control requests. upload_blob deliberately invokes the Blossom verifier (media.rs:266–272), whose kind check requires 24242 (crates/buzz-media/src/auth.rs:43–45). It therefore returns evidence rejected\n before key pairing, exactly as CI reports at media.rs:3698. Use the existing valid Blossom upload helper, matching the host/body hash and all preceding prerequisites, and change only the assertion key between denied and admitted controls. Keep the zero-body-polls and exhausted-permit ordering assertions, plus a control that actually reaches the resource boundary. Do not broaden production proof support to satisfy this fixture.

P2: Send both moderation controls to the actual moderation route

crates/buzz-relay/src/api/bridge.rs:5406–5411,5577–5588 still signs and requests /communities/{host}/moderation/reports; the registered route and verifier URL are /moderation/reports (router.rs:351, bridge.rs:2630). The new exact assertions expose the error but do not fix it. Exact-head CI shows the mismatch request gets the fallback response internal endpoint: localhost only, and the positive control fails its JSON-body assertion. Change both requested and signed paths to the real route. Then assert a precise handler outcome for the same-key caller (the currently unprivileged caller can produce the exact application JSON 403, or seed a real moderation role for 200), paired with exact NIP-FI mismatch bytes. ensure_user alone does not grant a moderation role. Do not weaken the assertions to accept the fallback response.

P2: Finish the already-requested media alias/Off-HEAD matrix

The alias missing-proof test (media.rs:2893–2963) still omits both the assertion and proof, so the outer assertion guard masks whether the alias is routed to the handler. The new mismatched-key case is useful after fixing its header panic, but does not replace valid-assertion missing/malformed/repeated-proof cases for both upload routes, or the previously requested Off HEAD control. Parameterize the existing cases over /upload and /media/upload, supply valid assertions in the Enforce proof cases, and add the Off HEAD witness. Preserve exact applicable status, challenge, content type, and body semantics. This is the outstanding prior exit criterion, not a request for a broader authentication matrix.

Credited repairs and validation limits

The Git pack fixture now installs its verifier before malformed-assertion cases. The owner settings control now requires 200, changed=true, and an updated digest while retaining unchanged-state checks after denials; its exact-head backend integration test passes. The bounded JWKS callback waits do not introduce a new source finding here; binary-target runtime coverage remains unverified.

Non-blocking compatibility note: Git’s missing/wrong-scheme responses now add production Content-Type headers (transport.rs:363,375) instead of asserting the previous header absence. That is a behavior change, not merely a fixture correction. No concrete client breakage was established; preserving the prior absence remains the smaller compatibility-preserving repair.

Source-only review on the pinned Blox host, with no checkout, builds, tests, or PR-code execution. Existing PostgreSQL CI has seven failing bridge/media tests explained above; the settings integration job passes the repaired snapshot test. The overall run is not green. Unrelated desktop failures were not investigated. Accepted Off/GIF ordering, smart-HTTP exceptions, S3 WebSocket exclusion, deferred S4 deny-map integration, and named Blossom gaps remain out of this corrective review.

Exit: repair these fixtures/remaining witnesses, then obtain passing exact-head existing CI coverage for them without changing the accepted production auth policy.

Hayt added 4 commits September 23, 2026 11:28
The missing-auth and wrong-scheme 401s had gained a Content-Type header that origin/main does not send, changing Off-mode bytes (FI-INV-15). Tests now assert the header is absent.

Signed-off-by: Hayt <211b96e6a2b7f45fd4047988976c7bbbeeda0c15f3ae7b32eec20834b5a55118@buzz.block.builderlab.xyz>
…outcomes

Upload controls sign the real body hash and pin the pre-storage 415 on both upload routes; the resource witness uses a kind-24242 proof, observes per-key accounting, and pins the 429 permit boundary. Moderation targets the registered route, the GIF fixture pins klipy absent, and Off GET/HEAD controls assert exact legacy bytes.

Signed-off-by: Hayt <211b96e6a2b7f45fd4047988976c7bbbeeda0c15f3ae7b32eec20834b5a55118@buzz.block.builderlab.xyz>
Receive-pack positive holds every git permit and requires the exact 503 busy response; info/refs gains malformed and duplicate proof denials with a valid assertion. Settings positive asserts persisted HEAD and parent link, and scheduler and Case 7 comments now match the traced mechanisms.

Signed-off-by: Hayt <211b96e6a2b7f45fd4047988976c7bbbeeda0c15f3ae7b32eec20834b5a55118@buzz.block.builderlab.xyz>
…rcement

* origin/main:
  fix(hooks): strip repo-local git env from pre-push test lanes (#7841)
  fix(mobile-infra): render push grant lifetimes as decimal in chart 0.3.2 (#7820)
  fix(agent): preserve Databricks Opus UC reasoning and tool continuation (#7840)
  feat(canvas): add version history with atomic restore (#6780)
  chore(release): release Buzz Desktop version 0.5.24 (#7817)
  test(desktop): stabilize unread and audio release smoke fixtures (#7821)
  fix(desktop): remember Inbox unread-only choice (#7672)
  feat(mobile-infra): support development App Attest (#7744)
  docs(nip-fi): clarify federated identity amendments (#7803)
  fix(desktop): refresh channels after access-revoked closure (#7784)
  fix(desktop): bound startup request bursts and recover quota refusals (#7790)
  fix(audit): frame hash inputs with TLV (#7492)
  fix(admin): allow cold storage worker DB startup (#7770)

Signed-off-by: Hayt <211b96e6a2b7f45fd4047988976c7bbbeeda0c15f3ae7b32eec20834b5a55118@buzz.block.builderlab.xyz>

# Conflicts:
#	crates/buzz-relay/src/api/bridge.rs

This branch is being deployed

1 in progress deployment
codex-review bab687a5 Deployed Sep 23, 2026 by wpfleger96 via Run Codex Security Review #5332
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants