Skip to content

feat(nip-fi): harden Blossom kind-24242 verifier to NIP-FI spec - #7288

Open
wpfleger96 wants to merge 32 commits into
mainfrom
duncan/nip-fi-blossom-hardening
Open

wpfleger96 wants to merge 32 commits into
mainfrom
duncan/nip-fi-blossom-hardening

Conversation

@wpfleger96

@wpfleger96 wpfleger96 commented Sep 3, 2026

Copy link
Copy Markdown
Member

Brings buzz-media, buzz-relay, all first-party kind-24242 minters, and desktop token minting into compliance with NIP-FI §kind-24242. Resolves the compliance note added in #7278.

What changed

buzz-media/src/auth.rs

  • Add BlossomStrictness enum (Strict | Permissive). Strict applies full NIP-FI rules (active modes); Permissive preserves pre-NIP-FI Off-mode behavior byte-identical [FI-INV-15].
  • Rewrite verify_blossom_auth_event_for_verb with count-based cardinality: exactly one t/expiration/server in Strict; duplicate x also rejected. Permissive retains tolerant boolean semantics.
  • Strict: mandatory server tag on all proofs (upload + read); absent or mismatched → evidence_rejected (ServerMismatch).
  • Strict: 60-second proof window (now - created_at <= 60s, expiration <= created_at + 60s). Permissive: 3600s / optional server.
  • [F1] Permissive server validation uses any-match semantics: a proof with multiple server tags is accepted if any tag matches the bound host. This restores origin/main base behavior. Strict duplicate-rejection unchanged.
  • [F1 compat] Permissive server gate uses server_values.is_empty() (valued tags only) rather than server_count > 0 (includes valueless tags). A lone valueless ["server"] tag is treated as absent in Permissive/Off mode, matching pre-NIP-FI base behavior [FI-INV-15].
  • [F4] Valueless x tags in Strict get reads are tracked before filter_map so they cannot become absent scope (host-wide authorization). Any x tag present in Strict must contain the exact requested sha256.
  • [F3 unit] Cases A–D in auth.rs are helper-level unit tests: they call verify_blossom_upload_auth and verify_upload_hash_only directly. They do not enter the upload pipeline (process_upload/process_video_upload). Retained as helper evidence.

buzz-media/src/error.rs

  • Add DuplicateTag(&'static str) variant.
  • Add blossom_denial_kind() method: classifies each auth error as MissingEvidence, EvidenceRejected, or AuthorizationDenied. Non-auth errors return None.
  • IntoResponse retains the legacy JSON 401 shape [FI-INV-15].

buzz-relay/src/api/media.rs

  • Add MediaDenial(MediaError, BlossomStrictness) wrapper mapping to byte-exact NIP-FI fixed responses in Strict; falls through to legacy JSON in Permissive.
  • blossom_strictness_from_state() stub defaults to Permissive on current main; one-line swap to state.config.nip_fi.is_enforce() after feat(relay): enforce NIP-FI assertion+NIP-98 pairing on HTTP ingress #7264 merges.
  • extract_blossom_auth: detect and reject repeated Authorization header values → DuplicateTag("Authorization") → 403.
  • Both membership-denial call sites use media_denial(RelayMembershipRequired, strictness) so Strict mode produces the correct fixed-text response.
  • upload_blob serving-write fence conversion (:406) uses map_err(|e| media_denial(e, strictness)) — a community fence committed between auth admission and lease acquisition no longer resets Strict to Permissive.
  • F5 production-path membership regressions moved into mod postgres_tests (discovered by nextest postgres-ci profile via test(/postgres_tests::/)) with #[ignore = "requires Postgres"]. Tests exercise real get_blob/upload_blob handlers with Strict+Permissive controls, exact response-bytes/headers (pinned byte-for-byte, not contains / json["error"]), challenge-absence assertions, and a member positive control that separates genuine denial from always-deny or DB-error mapping.

buzz-media/src/upload.rs (mod minio_tests)

  • [F3 pipeline] Four tests exercising the actual post-body call sites (process_upload :85, process_video_upload :413) with real MinIO I/O: buffered/streaming × expired-proof-accept + mismatched-hash-reject. The positive cases use fresh_strict_upload_auth (created_at = now-58, expiration = now+2, lifetime = 60s) — Strict-admissible at sign time — followed by a real 3s sleep so the proof is expired when the upload completes. nostr::Timestamp::now() reads the OS wall clock directly; paused Tokio time does not advance it. The negative cases use a pre-expired proof with a mismatched x tag. All four are #[ignore = "requires MinIO"]; no CI lane runs them automatically.

buzz-media/src/validation.rs

  • minimal_valid_mp4() promoted to #[cfg(test)] pub(crate) — a 618-byte pre-computed H.264/avc1 fast-start MP4 available to all buzz-media test modules; smoke-tested by test_minimal_valid_mp4_passes_validate_video_file.

desktop/src-tauri/src/commands/media.rs

  • sign_blossom_upload_auth: server tag mandatory; upload expiry 60s (was 3600s/300s).
  • MEDIA_GET_AUTH_EXPIRY_SECS: 60s (was 600s).

desktop/src-tauri/src/media_proxy.rs

  • Single re-mint+retry on 401/403 for range requests; non-auth failures do not retry.

crates/buzz-cli/src/client.rs

  • Read expiry: 60s (was 600s). Upload expiry: 60s. server tag mandatory on upload.

crates/buzz-dev-mcp/src/view_image.rs

  • MEDIA_GET_AUTH_EXPIRY_SECS: 60s (was 600s). Test updated.

mobile/lib/shared/relay/media_auth.dart

  • _mediaGetAuthLifetimeSeconds: 60 (was 600). Refresh margin updated.

mobile/lib/shared/relay/media_upload.dart

  • _uploadAuthLifetimeSeconds: 60 (was 300). server tag mandatory.

mobile/lib/features/channels/media_viewer_page/video_viewer.dart

  • [F2] Remove Android-specific VideoPlayerController.networkUrl streaming path. video_player_android 2.9.5 freezes headers into static DefaultHttpDataSource request properties — a 60s proof minted at controller creation time becomes stale on seeks. All platforms use the authenticated local-file download path.
  • [F2 transport] unawaited(request.sink.close()) before client.send(). AbortableStreamedRequest carries no request body; without closing the sink, IOClient.send() awaits stream.pipe(ioRequest) which blocks until the sink ends — every video download hung in loading indefinitely on every platform.
  • [F2 pending-ctrl] pendingController ref set before the first await (initialize()), cleared in all exit paths. Effect cleanup disposes pendingController.value so a close-during-init releases the native player even if the initialized event never arrives.
  • [F2r(a)] On native init failure (post-create, pre-initialized-event), the inner catch starts disposal detached via unawaited(localController.dispose().catchError(...)) — the .catchError handler absorbs any disposal exception so it does not reach the test error zone — then rethrows the original load error. video_player 2.11.1 completes the init future with an error without disposing the native player.
  • [F2r(b)] Non-2xx error body cancelled via _cancelVideoResponse() (subscribe+cancel) instead of drain(). drain() waits for the upstream to close; a stalled server body blocked initializeVideo indefinitely.
  • [F2r(d)] unawaited(localController.dispose().catchError(...)) in the catch — not await. Two failure mechanisms exist: (1) when createWithOptions() throws, _creatingCompleter is never completed and await dispose() deadlocks at :682-683, blocking the outer catch from setting error state. (2) when createWithOptions() succeeds but initialization fails, dispose() runs but its thrown exception becomes an unhandled async error reaching the test zone. Both are fixed: unawaited lets the outer catch run immediately and set error.value; .catchError(...) on the detached disposal future absorbs any disposal exception.

mobile/test/features/channels/media_viewer_page/video_viewer_test.dart

  • _FinalizingFakeClient: calls request.finalize().drain() — a test without the sink fix times out because the drain never completes, validating the transport contract.
  • F2r(a): forceInitError=true fake emits PlatformException on subscribe; disposeCallCount >= 1 verifies disposal even on init failure.
  • F2r(b): stalledBody StreamController with onCancel Completer (bounded completion signal, not a sleep). Asserts: (1) bodyStreamCancelled=true while body stream is still open (impossible with drain()); (2) error UI visible while body remains open. Restoring drain() breaks both.
  • F2r(c): neverInitialize=true fake never sends initialized event; unmount asserts disposeCallCount >= 1 via pendingController. Distinct from the event-error case.
  • F2r(d): forceCreateError=true fake throws PlatformException from createWithOptions() itself (before any player ID is returned). Asserts error UI visible within a bounded deadline. With the old await dispose() the outer catch is blocked — error.value is never set and the error UI assertion fails at the deadline; with unawaited(dispose()) the error text appears immediately.

mobile/test/features/channels/media_viewer_page/video_viewer_transport_test.dart (separate file)

  • Real-IO loopback probes using flutter_test (not package:test) to avoid depend_on_referenced_packages lint. No TestWidgetsFlutterBinding — no HttpClient override interfering with dart:io.
  • Probe 1: HttpServer.bind(loopbackIPv4, 0) reads the full request body before replying. Without sink.close(), req.drain() hangs and the test times out.
  • Probe 2: server signals request arrival via a Completer (not a sleep); abort fires after confirmed arrival; client.send() must throw RequestAbortedException (typed assertion) within a bounded 5s deadline.

scripts/test-video-upload.sh

  • Proof minted with +60 expiry and mandatory server tag.

buzz-test-client/tests/e2e_media*.rs + buzz-relay/src/api/media.rs test fixtures

  • All fixtures updated to 55s expiry + server tag.

docs/nips/NIP-FI.md

RELEASING.md

  • New "NIP-FI upload proof freshness (relay floor)" section documenting that 60s proofs require the first relay release carrying this PR's post-body hash-only check (the first tag > v0.2.1 — update once cut) for reliable large-file uploads. Operators on older self-hosted relays (≤ v0.2.1) must upgrade before receiving these clients.

Finding 3: 60s upload proofs vs old-relay compatibility

Desktop/CLI/mobile upload proofs are now 60s. Older relays re-run full expiry verification after the body is transferred. A valid upload whose body transfer takes >60s will fail on an old relay.

Resolution (relay floor prerequisite): RELEASING.md requires that verified supported deployments (relay releases confirmed to carry the post-body hash-only check, commit 75e9bef748) be in place before distributing 60s-proof client builds. Unknown or unverified self-hosted relay deployments are excluded from distribution until they upgrade. This makes the verified-deployment status a prerequisite for release distribution, not a post-distribution advisory. The concrete tag (> v0.2.1) is to be recorded once cut.

Sequencing note

blossom_strictness_from_state defaults to Permissive until #7264 merges and config.nip_fi is wired into AppState. The wiring commit (replacing the stub with state.config.nip_fi.is_enforce() and live both-directions regression tests) lands in this PR after #7264 merges.

GIF compatibility exception

An unauthenticated Off-mode request on a configured tenant with no GIF provider changes from 404 to 401 (base gifs.rs:269-275 checked an unconfigured provider first; the current code authenticates first). All other Off-mode Blossom responses are byte-identical to pre-NIP-FI behavior.

Brings buzz-media/src/auth.rs, buzz-relay/src/api/media.rs, and the
desktop token minting into full compliance with NIP-FI §kind-24242.

Changes:

buzz-media/src/auth.rs:
- Add BlossomStrictness enum (Strict | Permissive). Strict applies full
  NIP-FI rules; Permissive preserves pre-NIP-FI Off-mode behavior
  byte-identical [FI-INV-15].
- Rewrite verify_blossom_auth_event_for_verb with count-based cardinality
  tracking: exactly one t/expiration/server (Strict), at most one x.
- Strict: mandatory server tag on all proofs (upload + read); absent or
  mismatched -> evidence_rejected (ServerMismatch).
- Strict: 60s proof window (now - created_at <= 60s, expiration <=
  created_at + 60s).
- Permissive: 3600s window, optional server, tolerant cardinality (Off-mode).

buzz-media/src/error.rs:
- Add DuplicateTag(&'static str) variant.
- Split IntoResponse: missing Authorization -> 401 (missing_evidence);
  wrong scheme, malformed, duplicate tags -> 403 (evidence_rejected).

buzz-relay/src/api/media.rs:
- Add blossom_strictness_from_state() helper (TODO: wire to
  config.nip_fi.mode when #7264 lands; defaults to Permissive on main).
- extract_blossom_auth: detect and reject repeated Authorization header
  values -> DuplicateTag("Authorization") -> 403.
- Both call sites (upload + read) now pass strictness to verifier.

desktop/src-tauri/src/commands/media.rs:
- sign_blossom_upload_auth: server tag now mandatory (errors if relay
  URL yields no authority); was conditional.
- Upload token expiry: 60s unconditionally (was 3600s video / 300s image).
- MEDIA_GET_AUTH_EXPIRY_SECS: 60s (was 600s).

desktop/src-tauri/src/media_proxy.rs:
- proxy_handler + handle_buzz_media: single re-mint+retry on 401 or 403
  for range requests (expired 60s token mid-stream).

docs/nips/NIP-FI.md:
- Remove stale compliance note; replace with resolved statement.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
… 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>
Duncan and others added 3 commits September 3, 2026 13:50
…ll minters

Fix two IMPORTANT blockers from Thufir pass 1:

**Fix 1 — mode-aware denial response shape (buzz-media, buzz-relay)**

All strict-verifier failures previously collapsed to a generic JSON 401.
NIP-FI §755-773 requires:
- Missing Authorization → 401 + WWW-Authenticate: Nostr + text/plain
  body 'authentication required\n'
- Malformed/invalid/expired proof → 403 + text/plain 'evidence rejected\n'

The shape is Strict-only — Permissive (Off-mode) keeps the legacy JSON 401
unchanged [FI-INV-15].

Implementation:
- buzz-media/error.rs: add BlossomDenialKind enum (MissingEvidence /
  EvidenceRejected) and blossom_denial_kind() method on MediaError
- buzz-media/lib.rs: export BlossomDenialKind
- buzz-relay/api/media.rs: add MediaDenial(MediaError, BlossomStrictness)
  newtype implementing IntoResponse with mode-aware shaping via DenialClass
  byte contract from buzz-auth. Wire through AuthenticatedUpload extractor
  (Rejection = MediaDenial), authenticate_media_read, get_blob, head_blob.
  Non-auth errors fall through to MediaError::into_response() via From impl.

Tests: response-shape tests for Strict missing-evidence (401 + WWW-Auth +
text body), Strict evidence-rejected (403 + text/plain 'evidence rejected\n',
no WWW-Authenticate), Permissive regression pins for both classes (JSON 401,
no WWW-Authenticate). Classification tests for all 15 error variants.

**Fix 2 — 60s proof window across all first-party minters**

All minters updated to expiration <= created_at + 60s and mandatory upload
server tag, mirroring the desktop pattern established in the prior commit:

- buzz-cli/src/client.rs: read +60 (was +600), upload +60/server mandatory
  (was +600/+3600 conditional server, mime-gated expiry removed)
- buzz-dev-mcp/src/view_image.rs: MEDIA_GET_AUTH_EXPIRY_SECS 60 (was 600),
  comment updated; existing parametric test covers the updated constant
- mobile/lib/shared/relay/media_auth.dart: _mediaGetAuthLifetimeSeconds 60
  (was 600); margin=lifetime → mint-per-request pattern, comment updated
- mobile/lib/shared/relay/media_upload.dart: _uploadAuthLifetimeSeconds 60
  (was 300); server tag mandatory (was conditional on extractServerAuthority)
- scripts/test-video-upload.sh: expiry +60 (was +300), adds server tag
- buzz-relay/src/api/media.rs test fixtures: +55 (was +300) in
  media_get_tags_for and media_read_rejects_upload_verb_wrong_server_and_wrong_x
- buzz-test-client e2e fixtures: +55 + mandatory server tag in all three
  e2e_media* test files (relay_server_authority() helper added)

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
The two loop tests moved MediaError into MediaDenial but then
referenced the original variable in format strings. Capture the
debug repr as 'label' before the move.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
get_blob and head_blob are pub fn returning Result<_, MediaDenial>
which exposed the private type in the public interface (E0446).

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

Three call sites in the upload path bypassed the MediaDenial strictness
split via From<MediaError> for MediaDenial, which hardcodes Permissive:

1. ok_or(MissingTag("x-sha-256")) — replaced with ok_or_else using
   media_denial(e, strictness).
2. HashMismatch.into() × 2 (malformed + unmatched x tag) — replaced with
   media_denial(HashMismatch, strictness).
3. upload_blob returned Result<_, MediaError>, so post-body failures hit
   MediaError::into_response() directly.

Fix: add strictness: BlossomStrictness to AuthenticatedUpload, derived in
the extractor; change upload_blob to return Result<_, MediaDenial>; apply
media_denial through both the outer (protect-layer) and inner (async body)
error stacks. Non-Blossom errors (I/O, fencing, concurrency) fall through
to the legacy shape in both modes as the wrapper already guarantees.

Add 4 response-shape tests (Strict 403 + Permissive 401 for each of
MissingTag("x-sha-256") and HashMismatch), pinning status, content-type,
body bytes, and WWW-Authenticate absence.

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

Mobile tests and class documentation still specified the superseded
600/300-second proof lifetime after the NIP-FI 60-second fix landed.

media_auth.dart:24-28: rewrite class doc — with lifetime == margin == 60s,
_refreshAt == signedAt and the cache never hits; describe the intentional
mint-per-request pattern instead of the stale memoization claim.

media_image_test.dart:34-58 (memoization group): rewrite to pin mint-per-request
behavior. 'repeated calls return byte-identical headers' (asserting identical())
and 're-signs only at +540s boundary' both contradicted production; replaced with
tests that assert consecutive calls produce distinct headers/Authorization values
including without advancing the clock.

media_upload_test.dart:295: expiration literal 1700000600 (+600s) → 1700000060 (+60s).
media_upload_test.dart:422: expiration literal 1700000300 (+300s) → 1700000060 (+60s).

All 48 affected Dart tests pass on the pinned Flutter 3.41.7 toolchain (confirmed
the pre-fix image_test memoization tests were failing — asserting identical() true
when mint-per-request returns distinct instances every call).

auth.rs:321: x_tags.iter().any(|&v| v == sha256) → x_tags.contains(&sha256)
(clippy::manual_contains — fixes Rust Lint + Windows Rust CI red lanes at 5df4caf).

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…r-move in error.rs test

upload_blob, get_blob, and head_blob were pub but only reachable from
router.rs within the same crate — private_interfaces lint fired because
their return types include pub(crate) MediaDenial. Change all three to
pub(crate) to match the crate's visibility posture.

Also fix a borrow-after-move in buzz-media error.rs:
evidence_rejected_errors_permissive_shape_is_json_401 used {error:?}
after error.into_response() moved it. Add let label = format!() before
the move (same pattern already applied in media.rs tests).

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

The NIP-FI rejection table (§Public denial classes and transport codes)
maps `evidence_rejected` — malformed, invalid, OR expired evidence — to
HTTP 403.  The previous code mapped signature failures, expired tokens,
missing tags, hash/server mismatches, etc. to 401, matching a now-removed
oracle-enumeration rationale that the spec does not make.

Changes:
- Merge the two `IntoResponse` denial branches into one 403 arm covering
  all structurally present but invalid/malformed/expired proofs.  Only
  absent-header (`MissingAuth`) remains 401.
- Update the body string from "authentication failed" / "authorization
  denied" to the spec's fixed string "evidence rejected" / "authentication
  required" for the respective classes.
- Fix the misleading IntoResponse comment and the now-stale test section
  header.
- Update the `evidence_rejected_errors_permissive_shape_is_json_401` unit
  test → `evidence_rejected_errors_return_json_403`; add `InvalidAuthKind`,
  `InvalidAuthVerb`, `InvalidAuthEvent` to coverage.
- Fix all five `test_auth_*` assertions in e2e_media_extended.rs: wrong
  kind, missing t tag, missing expiration, expired token, empty content all
  expect 403 (not 401).

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Duncan and others added 2 commits September 3, 2026 16:07
…ilures

The two CI-failing e2e tests (test_auth_wrong_kind, test_auth_empty_content)
asserted 401 but the server correctly returns 403: InvalidAuthKind and
InvalidAuthEvent are structural format errors that into_response() maps to
403 (observable to any pre-NIP-FI Blossom client — not oracle information).

The previous fix attempt incorrectly merged all evidence_rejected variants
to 403 in into_response(), which broke the Permissive-mode invariant:
MediaDenial (buzz-relay) is the correct layer for the full NIP-FI rejection
table; into_response() is the legacy/Permissive fallback path [FI-INV-15].
Signature, expiry, missing-tag, hash/server-mismatch failures return 401 in
Permissive mode to prevent oracle enumeration — MediaDenial overrides them
to 403 in Strict mode. The media.rs Permissive pin tests document this split.

Changes:
- error.rs: restore two-branch IntoResponse (structural format → 403, oracle-
  guard auth failures → 401). Rewrite comment to document the layering.
  Replace the now-correct-name unit tests: structural_format_errors_return_
  json_403 + evidence_rejected_errors_return_json_401_in_permissive_mode.
- e2e_media_extended.rs: fix only wrong_kind (401→403) and empty_content
  (401→403); revert missing_t_tag, missing_expiration, expired_token to
  401 — they exercise the Permissive path and correctly return 401.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
MediaError::into_response() is the legacy/Permissive compatibility path.
FI-INV-15 requires it to preserve pre-NIP-FI behavior: all auth failures
return a single JSON 401 "authentication failed" response regardless of
failure class.

The 77fabf3 and 151ec74 commits both violated this invariant by
splitting auth failures into 401 and 403 arms inside into_response().
The NIP-FI rejection-table split (missing_evidence → 401,
evidence_rejected → 403) belongs exclusively to MediaDenial in buzz-relay,
which already implements it correctly under BlossomStrictness::Strict.
Routing is currently hardcoded Permissive, so all live traffic and the
Relay E2E lane exercise this legacy path.

Changes:
- error.rs: collapse the two auth arms back to a single 401 arm covering
  all variants (MissingAuth, InvalidAuthScheme, InvalidBase64,
  InvalidAuthEvent, InvalidAuthKind, InvalidAuthVerb, DuplicateTag,
  InvalidSignature, TokenExpired, TimestampOutOfWindow, Unauthorized,
  TokenRevoked, PubkeyMismatch, HashMismatch, ServerMismatch, MissingTag).
  Replace the split unit tests with a single exhaustive
  all_auth_failures_return_json_401_in_permissive_path pin.
- e2e_media_extended.rs: revert all 5 test_auth_* assertions to 401;
  the suite exercises the Permissive path via hardcoded Permissive routing.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Add a body assertion to all_auth_failures_return_json_401_in_permissive_path
so the pin fully captures the FI-INV-15 byte-identical claim: all 16 auth
variants must emit {"error":"authentication failed"} as the JSON body.
The test is promoted to async (#[tokio::test]) to collect the response body
via axum::body::to_bytes; tokio with test-util is already a dev dep.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
@wpfleger96
wpfleger96 deployed to codex-review September 3, 2026 21:02 — with GitHub Actions Active
@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Sep 3, 2026
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown

🔐 Codex Security Review

Status: review required for the current range.

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

…-body only

Two security findings from Codex review of #7288:

Finding 3 (security): In verify_blossom_auth_event_for_verb, the t tag arm
incremented t_count unconditionally before checking content, so a t tag with
no value (e.g. ["t"]) satisfied the required-tag check (t_count > 0) without
binding any verb. A verb-less proof was accepted for both upload and get in
both Strict and Permissive modes.

Fix: a t tag only counts toward t_count when it has non-empty content equal to
the requested verb. A valueless or empty-string t tag is ignored for cardinality
purposes (matches origin/main's found_t semantics). Duplicate-t cardinality in
Strict mode is checked after confirming the tag is valid, not before.

Also audited expiration/x/server tag handling: these are all gated on
tag.content() already, so valueless variants of those tags are already safe.

Finding 2 (correctness): Both buffered and video upload paths called
verify_blossom_upload_auth post-body, which re-runs the full verifier including
expiry/freshness checks. With 60s minted tokens (correct per NIP-FI Freshness),
any upload taking >60s fails AFTER transferring the full body.

Fix: introduce verify_upload_hash_only — checks only the x-tag hash against the
computed SHA-256. Replace both post-body verify_blossom_upload_auth calls with
this targeted check. The pre-body gate at the relay handler already enforces
signature, kind, freshness, cardinality, and server; the only thing unknown
before body transfer is the content hash.

Tests added: valueless t (Strict+Permissive), empty-string t (Strict), valueless
x on upload. All 147 buzz-media lib tests pass; all 44 api::media + 56
api::admin buzz-relay tests pass.

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 3, 2026
In Strict mode, count every tag whose field name is 't' before
validating its content.  The previous implementation counted only
valid-valued matching tags, so a proof with both ["t"] (valueless) and
["t","upload"] (valid) silently ignored the malformed instance and
admitted a two-tag proof as exactly one.

NIP-FI.md:658-666 requires Strict to reject malformed, empty, duplicate,
or conflicting instances as evidence_rejected.  The fix counts first,
gates on cardinality (>1 → DuplicateTag("t")), then validates content
(empty/valueless/wrong-verb → InvalidAuthVerb).  Permissive keeps the
origin/main found_t semantics unchanged.

Updated tests: the two malformed-alone Strict tests now expect
InvalidAuthVerb (counted as one, content check fires) instead of the
stale MissingTag.  Added four new regressions: valueless+valid combo,
empty-string+valid combo (both reject in Strict), and valueless+valid x
combo (confirms x_count increments unconditionally → DuplicateTag("x")).

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
@wpfleger96
wpfleger96 deployed to codex-review September 3, 2026 23:16 — with GitHub Actions Active
The two malformed-t-alone Strict tests were named
'is_not_counted_strict' after the previous semantics (ignore →
MissingTag).  At 41ee983 the behavior became count-then-reject →
InvalidAuthVerb, so the names were misleading.  Rename to
test_strict_rejects_valueless_t_tag and
test_strict_rejects_empty_string_t_tag.  Logic unchanged.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Duncan and others added 2 commits September 21, 2026 22:02
… lane

F1 (sink, live-breaking): close AbortableStreamedRequest sink before
send().  IOClient.send() awaits stream.pipe(ioRequest); without
close() the pipe blocks forever — every video download hangs in
loading on every platform.  unawaited(request.sink.close()) placed
after header setup, before client.send().

F2 (pending controller): add pendingController ref set before the
first await (initialize()) and cleared in all exit paths.  Effect
cleanup now disposes pendingController.value so a close-during-init
releases the native player even if the initialized event never
arrives.  Handles video_player 2.11.1 _creatingCompleter semantics.

F2 tests — replaced MockClient.streaming workaround with proper fakes:
- _FinalizingFakeClient: calls request.finalize().drain() so a test
  that passes without the sink fix times out (actually validates the
  transport contract).
- Real IO loopback: HttpServer.bind(loopbackIPv4, 0) server reads the
  full request body before replying; if sink is not closed the server
  never responds and the test times out.  Headless, no network.
- Abort loopback: server delays the response; unmounting fires the
  abort trigger and the download cancels cleanly.
- F2r(b) now asserts onCancel fires while the body is still open
  (not just zero disposals); restoring drain() breaks both assertions.
- F2r(c) new test: neverInitialize fake → unmount → pendingController
  must be disposed even though initialize() never returned.

F5 (postgres test lane): move the three DB-backed membership tests
into mod postgres_tests inside mod tests so the nextest postgres-ci
filter (test(/postgres_tests::/)) discovers them and attaches the
isolation wrapper.  Add #[ignore = "requires Postgres"] to all five.
Add: permissive_upload (new), exact legacy body+CT+challenge-absence
assertions on both read and upload, member positive control
(relay_member_read_passes_membership_gate_and_reaches_sidecar) that
distinguishes genuine denial from always-deny/DB-error mapping.

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

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: one remaining P2

Reviewed d3dfc0249aa00e77de2b49e6400fbab25506a383 against exact base 77729abfb692b25a0f4ec4a69add86af2e32c0dd. This is a bounded corrective re-review of 5272788579, not a reopening of the deferred #7264 activation/pairing work.

[P2] Preserve slow-upload compatibility with older relays (F3 remains open)

Desktop, CLI, and mobile still mint 60-second upload proofs unconditionally. Those client files are byte-identical to the previous reviewed head. The exact-base relay verifies the full proof after receiving the video body, and its verifier rejects an expired proof.

Source-derived reproduction: use a new client against the exact-base relay, send fresh headers immediately, and transfer a valid video over 90 seconds. The old relay rejects the upload after the transfer, although this is within the CLI's 600-second video timeout and the previous proof lifetime. Re-signing on retry cannot rescue a transfer that consistently exceeds 60 seconds.

The head relay's pre-body admission/hash-only completion correctly fixes new-client/new-relay, not new-client/old-relay. The corrective delta adds neither a compatibility gate nor compatible proof selection. Release lanes remain independent; the PR body's relay-first recommendation and request for a ruling are not an implemented gate or an accepted human compatibility decision.

Exit criterion: enforce a supported-relay floor before distributing these clients, or implement compatible proof selection for older relays, with a slow-transfer mixed-version regression. Preserve Strict's maximum 60-second proof and the new relay's hash-only post-body check. Do not restore post-body expiry checking or globally lengthen Strict proofs.

Fixes credited and validation limits

  • Strict serving-write denial is fixed in source: the acquisition exit now preserves strictness, so a fence uses the fixed authorization-denied response. The new handler tests cover membership denial, not an actual fenced acquisition; that remains a coverage gap, not another demonstrated runtime defect.
  • F1/F2/F4 and membership-denial fixes remain closed. The new Permissive valueless-server behavior matches the base, and mobile now disposes failed local controllers and cancels non-2xx bodies. The stalled-body test should assert cancellation/error UI: disabling animations and asserting zero controller disposals does not distinguish cancellation from a still-pending drain. This is a non-blocking coverage note; no mutation test was run.
  • Source-only review on pinned Blox, with independent history reconciliation and corrective source review. No checkout, installation, build, test, or PR/dependency-code execution; native playback and slow transfer were not exercised. One existing exact-head CI snapshot showed 55 successful, 27 skipped, 3 failed, and 1 in-progress contexts. The root failed lane was Desktop Smoke E2E (4); two failures were aggregates. No CI reruns or monitoring, and no causal attribution of that failure to this patch.

Duncan and others added 2 commits September 21, 2026 22:44
Addresses all four IMPORTANT findings from Thufir pass-2:

F1 (sink never closed): already fixed in ff1bfca — unawaited(request.sink.close())
  before client.send().  Tests added.

F2 (pending controller): already fixed in ff1bfca — pendingController ref set before
  first await, cleaned up in effect teardown.

F2r test fixes this commit:
- Add buildView() -> SizedBox.shrink() to _FakeVideoPlayerPlatform so
  VideoPlayer widget can render after successful init without throwing
  UnimplementedError.
- Add dispose() stream.addError before close so neverInitialize tests
  unblock initialize() and avoid pending-timers warnings.
- Fix Riverpod _debugOverridesLength assertion on pumpWidget(SizedBox):
  pass matching overrides in teardown pumpWidget calls.
- Extract real-IO loopback probes into video_viewer_transport_test.dart.
  TestWidgetsFlutterBinding.ensureInitialized() intercepts ALL HttpClient
  calls (returns 400) for every test in the suite including plain test()
  calls; the real loopback server is only reachable from a file that does
  not install that binding.
- F2r(b): add find.text('Failed to load video') assertion so the error UI
  is verified visible while the body stream is still open (drain() would
  block here — the assertion is discriminating).

F5 (postgres test lane): already fixed in ff1bfca — membership tests
  moved into mod postgres_tests.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Switch video_viewer_transport_test.dart from package:test/test.dart
to package:flutter_test/flutter_test.dart to avoid the
depend_on_referenced_packages lint (test is not a direct dev dep).

Revert pubspec.lock to match the hermit-pinned Flutter 3.41.7 toolchain
(test 1.30.0 / test_api 0.7.10 / meta 1.17.0 — same as origin/main).
The prior lock entry (test 1.31.0) was generated by the unpinned system
flutter and diverged from what CI uses.

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: one remaining P2

Reviewed 033022efbb0b22ab0477812d8bb16944e921a7d4 against exact base 77729abfb692b25a0f4ec4a69add86af2e32c0dd. Bounded corrective review of 5273624137; deferred #7264 activation/pairing stays out of scope.

[P2] Preserve slow-upload compatibility with older relays (F3 remains open)

Desktop, CLI, and mobile still mint 60-second upload proofs unconditionally. These three producer files are byte-identical to the prior reviewed head. The exact-base relay verifies the full proof after receiving the video body, and its verifier rejects expiration <= now. The 3600-second maximum-age argument does not override expiration.

Source-derived reproduction, not executed: start a valid video upload immediately after minting the proof and transfer its body over 90 seconds to the exact-base relay. The relay rejects it after transfer. This is within the CLI’s 600-second video timeout; re-signing for another attempt cannot rescue a transfer that consistently exceeds 60 seconds.

The updated relay correctly uses pre-body admission and post-body hash-only verification, but that repairs new-client/new-relay, not new-client/old-relay. No compatible proof selection or supported-relay floor was found in the reviewed client paths, release scripts, or release guidance. Release lanes remain independent. The PR body still recommends relay-first sequencing and requests a ruling; history reconciliation found no subsequent human acceptance of that compatibility change.

Unchanged exit criterion: enforce a supported-relay floor before distributing these clients, or implement compatible proof selection for older relays, with a mixed-version slow-transfer regression. Preserve Strict’s maximum 60-second proof and the updated relay’s hash-only post-body check. Do not globally lengthen Strict proofs or restore post-body expiry checking.

Corrective changes credited

  • The mobile production-request finalization fake now binds the real widget request. The stalled-body test asserts cancellation and visible error UI before closing the upstream stream, resolving the prior nonblocking coverage note. Failed-initialization disposal and the new pending-controller unmount cleanup are credited in source.
  • Membership regressions exercise real handlers/database gates with Strict/Permissive response oracles and a member positive control; their postgres_tests placement matches the PostgreSQL CI selector. F1/F2/F4 and the Strict serving-write repair remain credited. An actual acquisition-fence handler regression remains a nonblocking coverage gap, not a new runtime finding.

Validation limits: source-only on pinned Blox, with independent corrective-source and history lanes plus coordinator verification of the decisive producer/old-relay paths. No checkout, installation, build, test, PR/dependency execution, native playback, or live slow-transfer probe. One existing exact-head CI snapshot showed 38 successful, 20 skipped, and 4 in-progress check runs, with zero failed/cancelled/timed out; this is not a completed-green claim. No CI reruns or monitoring.

- [F2r(d)] unawaited(dispose()) in catch: video_player 2.11.1
  _creatingCompleter is completed only after createWithOptions() returns;
  if creation throws, awaiting dispose() deadlocks.  unawaited lets the
  outer catch run immediately and set error.value, showing the error UI.
  Add forceCreateError fake + F2r(d) bounded-visible-failure test.
- Abort probe: replace 100ms sleep with server-arrival Completer;
  assert typed RequestAbortedException; bound with 5s deadline.
- F2r(b): replace 50ms sleep with onCancel Completer completion signal.
- Fix fake dispose() comment: subscription cancels BEFORE platform
  disposal in video_player 2.11.1; clarify what the error injection achieves.
- Membership matrix: pin exact bytes and exact content-types
  (text/plain; charset=utf-8 / application/json) rather than contains
  checks and json field access.
- RELEASING.md: document relay-v0.5.0 floor for 60s upload proofs (F3);
  post-body hash-only check eliminates expiry window during transfer.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Duncan and others added 2 commits September 22, 2026 10:19
- RELEASING.md: replace fabricated relay-v0.5.0 floor with accurate
  forward-placeholder (first relay tag > v0.2.1; this PR ships the
  post-body hash-only check, no released relay has it yet); drop false
  'all hosted relays run this version' claim; instruct operators to fill
  in the concrete tag at release time.
- Add viewer-path abort test: _StallingAbortableClient drains request
  body, signals arrival via Completer, suspends on abortTrigger.
  Unmounting the viewer fires effect cleanup ->
  activeRequestAbort.complete() -> abortTrigger -> fake observes abort.
  Proves the full viewer close-to-abort chain through the viewer's own
  wiring; deleting activeRequestAbort.complete() causes timeout.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
_StallingAbortableClient.send() was hanging at drain() because
unawaited(sink.close()) may not have settled in the test binding's
event loop before finalize().drain() consumed the stream.  This fake's
job is to prove the abortTrigger chain, not sink-close (covered by
_FinalizingFakeClient), so signal arrival immediately without draining.

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: one remaining P2

Reviewed 08c4152609ad93733fc4c45211c2f1a9d0ce16cb against exact base 77729abfb692b25a0f4ec4a69add86af2e32c0dd. This is a bounded corrective review of 5273939261. Deferred #7264 activation, assertion pairing, and deny-map integration remain outside this review’s required fixes.

[P2] Make the relay-first compatibility prerequisite verifiable before declaring it satisfied

The new release-floor section is the only new mitigation for F3. Relay-first rollout is a valid solution; this does not require a new client negotiation system or automatic deployment gate. However, the section tells operators to upgrade to relay-v0.5.0 and asserts that all supported hosted relays already run it. The pinned relay version authority is still 0.2.1, and the normal release recipe derives the next patch from that manifest. GitHub lookup of both the named tag and release returned 404 during this review. An explicit future 0.5.0 release is possible, but these facts do not establish the present-tense supported/deployed assertion.

This matters to uploads, not just version bookkeeping. Desktop, CLI, and mobile still issue 60-second upload proofs; their producer files are unchanged since the prior review. The exact-base relay checks the full proof after consuming the video, including expiration against the current clock.

Source-derived reproduction, not executed: mint a fresh proof and transfer a valid video over 90 seconds to that older relay. It rejects the upload after transfer, within the CLI’s 600-second video timeout. Re-signing and retrying does not repair a transfer that consistently exceeds 60 seconds. The head relay’s pre-body admission plus post-body hash-only check correctly fixes head/head; release instructions must ensure that implementation reaches the relevant relays first rather than assume it already has.

Smallest exit: describe the minimum relay by the implementation/commit containing this repair until a real release tag is established, and make verification of that deployed repair an explicit prerequisite to client distribution. Replace the unsupported “already running” assertion with that check. Also describe the failing relay as the older full-verifier implementation, not “Strict mode”: the head Strict path correctly admits before the body. Preserve the existing requested regression across admission-before-expiry and completion-after-expiry, with a mismatched-hash negative control, bound to the production upload path. Do not globally lengthen Strict proofs or restore post-body expiry checking. Once that prerequisite is actionable, carrying out the rollout is an operational requirement, not a demand for more product machinery.

Nonblocking follow-up: Permissive expiration parsing uses first-wins/count-all where base used last-valued-wins. A valueless tag followed by a future-valued tag therefore newly fails. No first-party producer in the reviewed minting paths emits this shape, and it is not new in this corrective delta; I am not expanding the blocking exit criterion. Preserving the promised Off-mode semantics here is a small follow-up.

Corrective changes credited and limits

  • The mobile create-failure catch no longer waits on disposal before surfacing its error. Cancellation tests now use arrival/cancellation signals and the transport test asserts the typed abort failure. Membership tests pin exact response bytes and Content-Type. Previously credited F1/F2/F4 and Strict serving-write denial handling remain credited.
  • The reviewed post-body hash-only helper and upload call sites still lack the requested time-boundary regression in the inspected media/relay/e2e tests. A helper-only test would not catch a caller reverting to the full verifier. The actual acquisition-fence handler regression remains a nonblocking coverage gap, not a new runtime finding.
  • Source-only review on pinned Blox, with independent source and metadata lanes and coordinator verification. No checkout, installation, build, test, PR/dependency execution, native playback, or live slow-transfer probe. One existing exact-head CI snapshot at 14:19 UTC showed 18 success, 18 skipped, and 15 in-progress check runs, with zero failed/cancelled/timed out. This is not a completed-green claim; no CI reruns or monitoring. Deployed relay state was not verified, and absent tags alone are not proof of deployment state.

@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: the same remaining P2

Reviewed ad6ac0612de3137320028cacb4db24c044a35a20 against exact base 77729abfb692b25a0f4ec4a69add86af2e32c0dd, concentrating on the two-file corrective delta since 08c4152609ad93733fc4c45211c2f1a9d0ce16cb and the exit from review 5279715271. Relay-first rollout remains a valid solution. No client negotiation system or automated deployment gate is requested.

[P2] Require verification of the repaired relay before distributing 60-second-proof clients

Partial fix credited: RELEASING.md:103–116 removes the invented relay-v0.5.0 and unsupported already-deployed assertion. However, the replacement only asks release operators to document a floor and advise self-hosted upgrades. It never requires verifying that the relevant hosted/target relays actually run the repaired implementation before desktop/CLI/mobile distribution. Removing the deployment assertion is not the deployment check requested in the previous review.

The impact is unchanged: these clients issue 60-second upload proofs, while the older video upload pipeline reruns full verification after transfer. Source-derived reproduction, not executed: upload a valid video over 90 seconds to that older implementation. Admission can succeed, then completion fails on proof expiry; re-signing and repeating another 90-second transfer does not fix it. The head pre-body admission and post-body hash-only check remain correctly separated.

Smallest exit: make the release prerequisite explicit, for example:

Before distributing desktop, CLI, or mobile builds that mint 60-second upload proofs, verify that the relevant hosted and target self-hosted relays run a build containing repair commit 75e9bef748d2149ce459b14da842e706a51a5f78 (or a verified containing release tag). Do not distribute to those environments until this check passes. Record the concrete containing tag here when released.

That commit is an ancestor of the reviewed head and contains the post-body repair. A new release tag or assertion that deployment already happened is not required to fix these instructions; carrying out the rollout remains an operational prerequisite. Also change lines 95–98 from “The relay in Strict mode” to “the older full-verifier relay implementation”: head Strict admission does not have this defect.

Coverage and preserved scope

  • Preserve the previously requested regression bound to the production upload caller: admission before proof expiry, body completion after expiry, matching hash succeeds, mismatched hash fails. No media/relay upload source or tests changed in this corrective delta. In the inspected exact-head buzz-media auth/upload tests, relay media-handler tests, and buzz-test-client/tests/e2e_media*.rs, that timing witness is still absent. Existing expired-token coverage rejects at admission, not after an admitted transfer; a helper-only test would not catch the production caller reverting to full verification.
  • The new viewer test exercises actual widget teardown and would catch removal of activeRequestAbort.complete(). Nonblocking witness weakness: _StallingAbortableClient.send sets abortObserved even when the request is not abortable or its trigger is null. Removing that wiring therefore skips the wait and can still satisfy the assertion. Fail explicitly on missing type/trigger and assert no abort before unmount; this is not a new production blocker.
  • Previously credited F1/F2/F4, transport/init cleanup, membership response-byte tests, and Strict serving-write denial handling remain credited. Deferred #7264 activation, assertion pairing, and deny-map integration remain out of scope; permissive expiration semantics and acquisition-fence handler coverage remain nonblocking. This is not a whole-feature PASS.

Validation limits: source-only review on pinned Blox, with independent source/release/metadata lanes and coordinator verification. No checkout, installation, build, tests, PR/dependency execution, native playback, or live slow-transfer probe. One existing exact-head CI snapshot at 2026-09-22 16:06:15 UTC contained 87 contexts: 58 success, 28 skipped, one cancelled (Run Codex Security Review); rollup FAILURE, with no failing test/build context in that snapshot. No reruns or monitoring. Deployed relay state was not verified.

IMPORTANT 1 (new detached-disposal error path): video_viewer.dart already
had the unawaited(dispose().catchError(...)) fix from ad5b0744. This commit
adds _FailingDisposeVideoPlayerPlatform (creates OK, emits init error,
throws from dispose()) and F2r(d)+: verifies error UI appears and no
uncaught Flutter error reaches the test binding's handler.
FlutterError.onError override removed — testWidgets fails automatically
if any uncaught error escapes.

IMPORTANT 2 (abort false-positive): _StallingAbortableClient.send() uses
a typed if-case match (:final abortTrigger?) and throws StateError on
a missing trigger — absent wiring cannot silently pass. Asserts abort NOT
observed before unmount; awaits abortObservedCompleter (bounded 5 s) after
unmount. Added tester.pump() after pumpWidget(SizedBox.shrink()) to flush
the useEffect cleanup microtasks before the Completer deadline.
Red-with-reverted activeRequestAbort.complete(): trigger stays pending,
abortObservedCompleter times out.
Red-with-non-abortable-request: StateError from fake fails fast.

IMPORTANT 3 (F3 relay floor): RELEASING.md — identifies the repair by
commit 75e9bef; adds explicit prerequisite
(verify deployed artifact before distributing 60s-proof clients); corrects
the Strict/old-relay description (old relay re-verifies post-body, head
Strict verifies before body). Four boundary-regression tests added to
auth.rs (Cases A-D: admission-before-expiry, old-verifier-rejects-expired,
hash-only-accepts-expired, hash-mismatch-rejected).

IMPORTANT 4 (Carl follow-up / FI-INV-15): Permissive expiration parsing
now uses last-wins semantics — a valueless expiration tag followed by a
future-valued tag admits, matching pre-NIP-FI base behavior. Test added.

MINORs (all folded):
- F2r(d) mechanism comment corrected: 300ms window ends with error.value
  unset, not 'times out'; no dispose count claimed for create-failure case.
- F2r(d)+ FlutterError.onError override removed (framework already catches).
- Fake dispose() comment: states fake records native disposal and closes
  stream; does not simulate initialization or unblock initialize() future.
- Transport comment: no-drain choice described as responsibility separation
  (not unverified sink.close() root cause); Future.delayed(60s) replaced
  with socket-close-aware serverDone Completer in abort test server handler.
- PR body: F2r(a) 'disposes before rethrowing' -> 'starts disposal';
  viewer-abort description scoped to what it proves; F3 paragraphs state
  prerequisite and regression status accurately.

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

Transport test: add teardown-controlled releaseResponse gate so the
server handler holds the response open until the test releases it.
Without the gate, response.close() sends an empty 200 immediately and
the abort races against a completed response, making the probe
scheduling-sensitive rather than a controlled in-flight cancellation.

Upload pipeline: add streaming-path counterparts to the existing
buffered-path pipeline tests so both call sites (upload.rs:85 and :413)
are bound. The four minio_tests cases now cover:
  - buffered Case B: expired proof + matching hash → accepted
  - buffered Case D: mismatched hash → HashMismatch
  - streaming Case B: expired proof + matching hash → accepted
  - streaming Case D: mismatched hash → HashMismatch

Reverting either verify_upload_hash_only call to the old full verifier
fails the positive (TokenExpired); removing the hash check fails the
negative (security regression). All four marked ignore=requires MinIO.

Add minimal_valid_mp4() at crate scope (#[cfg(test)] pub(crate)) in
validation.rs: 618-byte pre-computed H.264/avc1 fast-start MP4 that
passes validate_video_file, accessible to upload.rs test modules without
exposing the builder internals. Backed by smoke test
test_minimal_valid_mp4_passes_validate_video_file.

Minor test comment fixes: correct 'fails fast' wording for
_AbortingFakeClient and deletion mutation path (test:324-326, 797-798);
rewrite fake-stream dispose comment to state what it actually does
(records disposal, closes stream, does NOT settle initialize() future);
replace 300ms sleep with disposedCompleter.future.timeout(5s) and add
disposeCallCount assertion for the disposal-failure test.

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

Sort buzz_core::tenant before nostr in minio_tests use block; reformat
multi-line expressions to match rustfmt output; remove the unused
sha2::Digest as _ trait import (sha2::Sha256::digest is called fully
qualified so the trait alias is dead and fails Clippy -D warnings).

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Duncan and others added 2 commits September 22, 2026 18:12
…meout zone

upload.rs (minio_tests):
- Replace expired_upload_auth in the positive cases with
  fresh_strict_upload_auth (created_at=now-58, exp=now+2, lifetime=60s).
  The previous 119s-lifetime proof was inadmissible under Strict
  (expiration > created_at+60) so 'simulates previously admitted' was
  false for the stated 60s Strict case.
- Assert verify_blossom_upload_auth(Strict) passes at sign time before
  the sleep so the admission claim is verified, not assumed.
- Add a real 3s sleep in both positive tests: nostr::Timestamp::now()
  reads the OS wall clock directly; paused Tokio time does not advance it.
  After 3s the proof is expired; verify_upload_hash_only still accepts it.
- Keep expired_upload_auth for the hash-mismatch negative cases (no
  admission sequence needed there).
- Update module comment to match the actual T=0/T=3s sequence.

video_viewer_test.dart (F2r(d)+):
- Replace .timeout(const Duration(seconds: 5)) on disposedCompleter.future
  with a synchronous isCompleted check after pumpAndSettle(). Outside
  runAsync the flutter_test binding runs FakeAsync; Duration-based timers
  created there are never advanced automatically, so the 5s timeout would
  hang to the 30s outer runner timeout instead of failing promptly.
  isCompleted is the correct oracle: disposedCompleter is completed
  synchronously at the TOP of dispose() (before any async work), so it
  must be complete by the time pumpAndSettle() drains microtasks and
  returns. Removing the unawaited disposal call leaves the completer
  incomplete and the check fails immediately with a diagnostic.
- Remove unsupported dart:io/pending-timers claim from _SimpleVideoPlayerPlatform
  dispose comment; retain only the accurate description.

video_viewer_transport_test.dart:
- Correct probe-2 file header: the Completer is a teardown-controlled
  release gate, not a client-connection-close notification.
- Remove SocketException promise from server handler and teardown
  comments: close() may throw on abort but the exception type is not
  guaranteed.

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

* origin/main:
  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: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>

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

Review clear: prior release blocker resolved

Reviewed 19ea00ba0378af520f0f8738f5ff7ad8ea1c986e against exact base 77729abfb692b25a0f4ec4a69add86af2e32c0dd, concentrating on the seven-file corrective delta since ad6ac0612de3137320028cacb4db24c044a35a20 and the agreed exit from review 5280683315. No remaining blocking finding in this bounded re-review. This is a comment, not an approval or certification of deployment/CI.

Resolved and credited

  • Relay-first compatibility: RELEASING.md:94–129 now requires verifying that supported relay environments run a release containing repair commit 75e9bef748d2149ce459b14da842e706a51a5f78 before desktop/CLI/mobile distribution. Unknown or unverified deployments do not satisfy the prerequisite. The wording correctly attributes the defect to older relays and asks for the concrete containing tag once released, without inventing one or claiming deployment. Repair ancestry is verified. Actual rollout verification remains an operator prerequisite, not something this review established.
  • Upload and legacy semantics: relay admission still verifies freshness before body consumption; buffered and streaming production callers retain hash-only completion checks (upload.rs:85,413). The new pipeline tests reach those real callers and add matching/mismatched-hash controls. Permissive expiration parsing now restores the base implementation’s last-valued-wins behavior without changing Strict duplicate rejection.
  • Mobile corrective delta: detached disposal now handles errors without delaying the original load-error UI (video_viewer.dart:184–216). The strengthened viewer test requires an actual abortable request/trigger and observes no abort before unmount. The separate IOClient transport probe holds the response open until cancellation. Previously accepted verifier, playback, membership-denial and serving-write repairs remain credited.

Nonblocking coverage qualification

The new MinIO pipeline tests are useful production-bound post-body checks, not yet a same-proof admission-before-expiry/completion-after-expiry witness. expired_upload_auth sets created_at = now - 120 and expiration = now - 1: that 119-second lifetime cannot pass Strict admission. The fresh-admission unit test uses a different proof. All four pipeline cases are ignored by default; the inspected _ci-relay.yml ignored-test selectors do not select them. Follow up with a Strict-admissible proof carried across the boundary and an explicit infrastructure-backed invocation. This is a coverage limitation, not evidence that the repaired production path is wrong, and does not reopen the resolved release blocker.

Scope and validation

Independent upload, mobile and release/history lanes returned and were integrated with coordinator source verification. Unchanged minters and previously credited GET/HEAD/denial paths were not independently recertified. Deferred #7264 activation, assertion pairing and deny-map integration remain excluded; this is not a whole-feature PASS.

Source-only on pinned Blox: no checkout, install, build, tests, PR/dependency execution, native playback or live slow-transfer/deployment probe. Static PNG chunk/CRC and MP4 box-bound inspection is not a runtime validator pass. One existing exact-head CI snapshot contained 87 contexts: 54 success, 28 skipped, four failures (Desktop Smoke E2E shards 1 and 4 plus two Desktop aggregates), and one cancelled Codex Security Review. No cause inferred, reruns or monitoring; CI is not green.

@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Sep 22, 2026
The previous isCompleted check after pumpAndSettle() was racy: the
viewer does real File.openWrite() before player creation, and with
disableAnimations:true the loading widget's reduced-motion branch stops
animation, so pumpAndSettle() (a frame barrier, not an I/O barrier)
can return while I/O is still pending. isCompleted then fails on
unchanged production code, misdiagnosing a removed disposal path.

Fix (Shape A): construct _FailingDisposeVideoPlayerPlatform and mount
the widget inside runAsync. Await disposedCompleter.future with a
real-zone 5s timeout, also inside runAsync. Timers registered inside
runAsync dispatch to the real event loop — they fire normally. The
completion microtask runs in the same real zone (initializeVideo()
starts there, the inner catch fires there, the unawaited detached
disposal future runs there, and dispose() completes the completer
synchronously before throwing). The await therefore resolves as soon
as the microtask queue drains after dispose() entry — no I/O race.

Removal check: removing only the unawaited disposal call leaves
disposedCompleter never completed; the 5s real-zone timeout fires
and the test fails immediately and deterministically.

Also update the _FailingDisposeVideoPlayerPlatform.disposedCompleter
docstring to name the zone mechanism.

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

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

Review clear: bounded corrective re-review

Reviewed HEAD 4afefe42639907f15dbb124ce7b0ebf96088b921 against BASE 26ede6dfa2496993aa62ce5781d2112df4c2d009, continuing the prior clear review. No actionable blocker found in the corrective delta. This is a comment, not an approval or whole-feature certification.

  • Scope: after subtracting inherited base movement, the branch-specific changes are confined to crates/buzz-media/src/upload.rs tests and the two mobile video-viewer test files. Previously credited production fixes, relay-first release policy, and the NIP-FI branch amendment remain unchanged. Supported relays still must be verified to contain repair 75e9bef748d2149ce459b14da842e706a51a5f78 before distributing 60-second-proof clients; this review does not establish deployment.
  • Upload witness: buffered and streaming positives now pass the same Strict-admissible proof through explicit verification, a real three-second wait, and the production completion pipeline. This addresses the prior same-proof coverage note. It is still a verifier-to-pipeline test, not a real HTTP slow-transfer test; the four MinIO cases remain ignored by default, and this review did not execute them. The short admission margin and wall-clock dependency remain test reliability limitations, not evidence of a production regression.
  • Mobile witness: F2r(d)+ now waits for disposal entry with a real-zone five-second timeout inside runAsync, instead of a fixed delay followed by a FakeAsync-zone wait. It still requires the error UI and an observed disposal call from a fake that throws during disposal. The transport-file delta only corrects comments about the held response and possible socket-close exception.

Validation and limits

Independent upload and mobile source-review lanes were integrated with coordinator checks on the pinned Blox host. No checkout, installation, build, test, PR/dependency execution, native playback, live slow-transfer probe, CI rerun or monitoring was performed. Existing exact-head check snapshot: 59 successful, 28 skipped, one cancelled security-review job, no failed checks; the Mobile job succeeded. This does not establish an executed MinIO witness.

Unchanged client minters and earlier GET/HEAD/denial repairs were not independently recertified. Deferred #7264 activation, assertion pairing, and deny-map integration remain excluded. No new merge condition is introduced by this re-review.

This branch was successfully deployed

No deployments
codex-review 4afefe42 Deployed Sep 22, 2026 by wpfleger96 via Run Codex Security Review #5217
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