Repository navigation
Manage community NIP-FI policy with enable, disable and status - #9176
Conversation
Add a one-shot signed operator command and a pod runbook for testing a connected client across a policy change. Co-authored-by: Honor <e0bf479eeebe47dde1e35705212a4df70616a7bc834d907ddfb18cf0405f8b1b@seiler-test.communities.public.builderlabstaging.xyz> Signed-off-by: Brad Seiler <seiler@squareup.com>
Co-authored-by: Honor <e0bf479eeebe47dde1e35705212a4df70616a7bc834d907ddfb18cf0405f8b1b@seiler-test.communities.public.builderlabstaging.xyz> Signed-off-by: Brad Seiler <seiler@squareup.com>
Co-authored-by: Honor <e0bf479eeebe47dde1e35705212a4df70616a7bc834d907ddfb18cf0405f8b1b@seiler-test.communities.public.builderlabstaging.xyz> Signed-off-by: Brad Seiler <seiler@squareup.com>
Co-authored-by: Honor <e0bf479eeebe47dde1e35705212a4df70616a7bc834d907ddfb18cf0405f8b1b@seiler-test.communities.public.builderlabstaging.xyz> Signed-off-by: Brad Seiler <seiler@squareup.com>
Co-authored-by: Honor <e0bf479eeebe47dde1e35705212a4df70616a7bc834d907ddfb18cf0405f8b1b@seiler-test.communities.public.builderlabstaging.xyz> Signed-off-by: Brad Seiler <seiler@squareup.com>
Co-authored-by: Honor <e0bf479eeebe47dde1e35705212a4df70616a7bc834d907ddfb18cf0405f8b1b@seiler-test.communities.public.builderlabstaging.xyz> Signed-off-by: Brad Seiler <seiler@squareup.com>
Co-authored-by: Honor <e0bf479eeebe47dde1e35705212a4df70616a7bc834d907ddfb18cf0405f8b1b@seiler-test.communities.public.builderlabstaging.xyz> Signed-off-by: Brad Seiler <seiler@squareup.com>
🔐 Codex Security Review
Review SummaryOverall Risk: NONE
FindingsNo concrete security, correctness, or reliability findings were identified. Notes
Generated by Codex Security Review | |
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 I didn't find any correctness problems. The compare-and-set runs under the row lock, exact replays return the receipt before any preflight, old-endpoint retries are fenced after a disable, and migration 0084 only adds columns, so the deployed relay keeps working. Everything inline is nonblocking.
One non-code thing: the PR is marked ready, but the description still says it remains a draft and there hasn't been a human run yet, so one of those probably needs to change.
| command: nip_fi::NipFiCommand, | ||
| }, | ||
| /// Require NIP-FI through the relay's existing signed operator API. | ||
| NipFiUpgrade(nip_fi_upgrade::Args), |
There was a problem hiding this comment.
🤖 nonblocking: this adds a new nip-fi-upgrade command for the old enable-only endpoint, which nip-fi-policy replaces. main never had this command, so there aren't old saved requests for it (the operator guide says it "remains for old saved requests"), and nip-fi-policy status --operation-id already reads old-endpoint receipts. IMO drop the command and move the helpers it shares (signed_call, endpoint, read_bounded, authorization, the tuple validation) into nip_fi_policy.rs. that's most of nip_fi_upgrade.rs gone
| return Ok(PolicyOutcome::Refused("policy_conflict")); | ||
| } | ||
| } else if !old_required && old_generation != 0 { | ||
| // An uncommitted legacy retry cannot reverse a later disable. |
There was a problem hiding this comment.
🤖 question, nonblocking: does the old POST /operator/communities/nip-fi-upgrade need to stay at all? BUZZ_NIP_FI_UPGRADES_ENABLED defaults to false and I couldn't find it turned on in the deploy config I checked. if it was never opened anywhere, sending enable through the new endpoint only would let this fence, PolicyRecord::legacy(), the old status filter that hides new receipts, and most of the mixed-version rollout section in the docs go away
| let old_required: bool = row.try_get("requires_nip_fi")?; | ||
| let old_generation: i64 = row.try_get("policy_generation")?; | ||
| if let Some((_, generation, required)) = intent { | ||
| if generation < 0 || generation != old_generation || required != old_required { |
There was a problem hiding this comment.
🤖 nit: generation < 0 can't fire here. the handler already rejects negative values, and old_generation is never negative, so generation != old_generation covers it
| Ok(Json(serde_json::json!({ | ||
| "community_id": id, "expected_host": host, | ||
| "requires_nip_fi": policy.requires_nip_fi, "policy_generation": policy.policy_generation, | ||
| "expected_policy_generation": policy.policy_generation, |
There was a problem hiding this comment.
🤖 nit: this response has requires_nip_fi/policy_generation and expected_requires_nip_fi/expected_policy_generation with the same values. I'd keep one pair so callers don't wonder whether they can differ
| "unconfirmed: invalid pending reasons" | ||
| ); | ||
| match receipt["state"].as_str() { | ||
| Some("active") if reasons.is_empty() => Ok(0), |
There was a problem hiding this comment.
🤖 nit: the relay never returns active (the contract doc says so too), so this branch is dead. moot if the command goes away
| } | ||
| Command::Status { .. } => None, | ||
| }; | ||
| let mut url = endpoint(&args.origin, None)?; |
There was a problem hiding this comment.
🤖 nit: endpoint() joins the old endpoint's path and then this overwrites it. passing the path into endpoint() would make it obvious which route each command hits
| let expected_body = serde_json::to_value(&saved).unwrap(); | ||
| let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); | ||
| let origin = format!("http://{}", listener.local_addr().unwrap()); | ||
| let app = Router::new().route("/operator/communities/nip-fi-policy", any( |
There was a problem hiding this comment.
🤖 nit: rustfmt gave up on this closure, so several statements share a line. pulling the handler into a named fn would make the test a lot easier to read
| current policy before deciding whether to issue a new explicit CAS disable. | ||
| A timeout is not permission to discard the original request or edit SQL. | ||
| Disable is a supported operator action; acceptance is no longer monotonic. | ||
| Last-issuer removal is allowed after disable, and re-enable must recheck trust |
There was a problem hiding this comment.
🤖 nonblocking: this overstates it a bit. disable removes the required-policy block on removing the last issuer, but for a normal bindable community buzz-admin nip-fi unbind still passes allow_last=false (crates/buzz-admin/src/nip_fi.rs:184), so the DB still returns LastIssuer. the new DB test passes true directly, so it doesn't cover the operator path. I'd reword it to say the separate last-binding guard still applies. no behavior change needed
|
|
||
| Before changing policy, apply the schema and roll compatible management writers | ||
| across the whole fleet with `BUZZ_NIP_FI_UPGRADES_ENABLED=false`. Then explicitly | ||
| enable that gate, which controls both enable and disable. Enable additionally |
There was a problem hiding this comment.
🤖 nonblocking: since this gate now opens disable too, the older descriptions of it are stale. crates/buzz-relay/src/config.rs:411 and .env.example:473 still say it permits "irreversible" upgrade acceptances. worth aligning them with this guide so someone flipping the flag knows it allows relaxing a community's requirement
Co-authored-by: Honor <e0bf479eeebe47dde1e35705212a4df70616a7bc834d907ddfb18cf0405f8b1b@seiler-test.communities.public.builderlabstaging.xyz> Signed-off-by: Brad Seiler <seiler@squareup.com>
Co-authored-by: Honor <e0bf479eeebe47dde1e35705212a4df70616a7bc834d907ddfb18cf0405f8b1b@seiler-test.communities.public.builderlabstaging.xyz> Signed-off-by: Brad Seiler <seiler@squareup.com>
Co-authored-by: Honor <e0bf479eeebe47dde1e35705212a4df70616a7bc834d907ddfb18cf0405f8b1b@seiler-test.communities.public.builderlabstaging.xyz> Signed-off-by: Brad Seiler <seiler@squareup.com>
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 The three commits since my last review look good, and I didn't find anything new.
- The gate rename is complete. Nothing in the repo still reads or mentions
BUZZ_NIP_FI_UPGRADES_ENABLED, and the default-off parse test moved toBUZZ_NIP_FI_CHANGES. Neither name is set in the deploy config I checked, so the rename doesn't change any running relay, and setting only the old name now leaves the gate closed. - The doc fixes match the code. The unbind note is right that
buzz-admin nip-fi unbindstill keeps the last issuer for a bindable host (crates/buzz-admin/src/nip_fi.rs:184). The gate descriptions inconfig.rsand.env.examplenow say the gate covers disable too. The PR description now matches its ready state.
The earlier inline nits still stand as nonblocking.
Consolidate signing, bounded transport and input validation in the policy command; retain their functional tests and selectors. Co-authored-by: Honor <e0bf479eeebe47dde1e35705212a4df70616a7bc834d907ddfb18cf0405f8b1b@seiler-test.communities.public.builderlabstaging.xyz> Signed-off-by: Brad Seiler <seiler@squareup.com>
Keep old receipts readable through policy status without retaining an unfenced mutation path. Require explicit CAS for every new write. Co-authored-by: Honor <e0bf479eeebe47dde1e35705212a4df70616a7bc834d907ddfb18cf0405f8b1b@seiler-test.communities.public.builderlabstaging.xyz> Signed-off-by: Brad Seiler <seiler@squareup.com>
Exercise retained historical receipts and deleted targets through the supported status API; refuse UUID reuse without mutating policy. Co-authored-by: Honor <e0bf479eeebe47dde1e35705212a4df70616a7bc834d907ddfb18cf0405f8b1b@seiler-test.communities.public.builderlabstaging.xyz> Signed-off-by: Brad Seiler <seiler@squareup.com>
Verify signed bodies and query-bound reads through the functional command journey. Co-authored-by: Honor <e0bf479eeebe47dde1e35705212a4df70616a7bc834d907ddfb18cf0405f8b1b@seiler-test.communities.public.builderlabstaging.xyz> Signed-off-by: Brad Seiler <seiler@squareup.com>
* origin/main: perf(relay): serve restriction rows for 10 s with cross-pod invalidation (#9199) Pin sqlx to the upstream fix for a cancelled BEGIN (#9192) feat(cli): manage sidebar sections with `buzz sidebar sections` (#9191) Show mobile floating dates while scrolling (#9185) fix(relay): regenerate missing image thumbnails on request (#9174) feat(loadgen): model agent turns and sustained holds (#9182) fix(cli): list DMs, attach any file, validate kinds, default the relay (#9168) Signed-off-by: Brad Seiler <seiler@squareup.com>
* origin/main: fix(relay): route live REQ events by each filter's own channel scope (#8519) fix(relay): deliver multi-node fan-out concurrently and never drop silently (#9195) perf(db): bound since-limited events reads with the month-floor literal (#9197) fix(mobile): match Agents sign-in to Home’s reconnect capsule (#9204) fix(cli): clearer desktop pairing errors, paired default relay, and auth desktop list (#9196) Signed-off-by: Brad Seiler <seiler@squareup.com>
Trusted operators can choose `initial_requires_nip_fi` on create-only community provisioning. True commits required policy (generation 1), validated issuer bindings and the owner with the new community in one transaction. Omitted, null or false preserves optional creation; a non-null field in legacy ensure mode returns coded 400. Missing effective bindings refuses required insertion without leaving a row. The response reports policy read from the stored row. Same-owner create-only replay now accurately reports `existed` instead of `created`; it stays successful and read-only, including after policy disable or issuer-config changes. Different-owner conflict, legacy ensure, bootstrap and the operator authorization gate retain their behavior. This adds no schema changes or server-wide default flag. Upgrade the whole serving relay fleet before callers request true. An old writer may ignore the field and insert an optional row: response checks detect it but cannot undo it, and later lookup remains read-only. Turn caller targeting off before relay rollback. Mode off stores policy but bypasses enforcement, independently of the upgrade gate. ### Related issue Replaces closed #9172. The separate lifecycle API remains in #9176. ### Validation Draft head: `bf16a1e0aba102c3be86f9621bd53d2cae98661c`. Independent source review covers this head. Formatting, diff sanity, Clippy, build, all 14 signed HTTP scenarios and full local `just ci` pass at this head. GitHub reports 59 successful and 38 skipped checks, with none pending or failing in the collected snapshot. At parent `b9d9423a6058387f42c310f5a7e4dbe0c8d8b40f`, all 1,978 package tests and 1,346 PostgreSQL tests passed (PostgreSQL concurrency 2). Four deliberate mutations were detected and restored. Clippy then found seven needless borrows in the new HTTP test; this head removes those borrows only. Earlier validation passed 1,344/1,346 PostgreSQL tests at concurrency 8; the replay assertion was corrected and the timeout disappeared at concurrency 2. Failed-run evidence is retained. At this head, the first mandatory push-hook attempt failed all nine boot-lifecycle tests on child startup deadlines. A subsequent serial run passed all nine (seven PostgreSQL-only cases skipped), with unchanged deadlines and source. The first diagnostic was interrupted during compilation without a result; all evidence is retained. A normal-hook retry was stopped when the author explicitly authorized one push with `--no-verify` for this exact commit. That exception skips local pre-push hooks for publication only; failed hooks and remaining validation still gate readiness. Explicit human runtime confirmation remains a separate acceptance step; agents have not attested it. Generated with Codex Validation update at the same draft head: Clippy, build and all 14 signed HTTP workflow scenarios passed, including atomic required creation, stored-policy replay after disable without a current binding, and refusal without leaving a row. The first live harness run stopped after 12 checks because its SQL helper fetched rows after an UPDATE without RETURNING; that harness query was corrected and the complete workflow passed. Relay source stayed unchanged. The full local `just ci` run failed in `mobile-check`: Dart formatting reported 266 changes among 1,068 files, with unresolved `flutter_lints` configuration warnings. The format check used `--output=none`; the worktree remains clean. Repository validation is therefore not green. That failure is retained as historical evidence; see the successful setup-corrected rerun below. Explicit human runtime confirmation remains separate. Final local validation: after `just mobile-install`, `mobile-check` passed without formatting edits, and full `just ci` completed successfully at `bf16a1e0aba102c3be86f9621bd53d2cae98661c`. Tracked files remained clean after installation and after the complete run. This confirms the earlier mobile formatting failure resulted from missing worktree dependencies. All failed runs remain preserved. GitHub's current ready-for-review state is preserved; no agent added `buzz-review-completed` or attested human runtime acceptance. --------- Signed-off-by: Brad Seiler <seiler@squareup.com> Co-authored-by: Honor <e0bf479eeebe47dde1e35705212a4df70616a7bc834d907ddfb18cf0405f8b1b@buzz.block.builderlab.xyz> Co-authored-by: Honor <e0bf479eeebe47dde1e35705212a4df70616a7bc834d907ddfb18cf0405f8b1b@seiler-test.communities.public.builderlabstaging.xyz>
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 all of our earlier comments are addressed, and removing the old nip-fi-upgrade endpoint and command outright is a nicer answer than what I suggested. I didn't find anything new in the cleanup commits. the red CI on this head is main's compile break in system_channel_postgres_tests.rs (missing received_at), which #9214 fixes, not this PR. one heads-up for rollout: kgoose in cash-server still has a client for the removed route (BuzzCommunityClient.kt, AUTH_UPGRADE_ROUTE). CommunityAuthUpgradeModule isn't installed anywhere on cash-server main, so nothing live breaks, but that client will need to move to nip-fi-policy before anyone wires it up
* origin/main: fix(relay): set received_at in the system-channel fan-out test helper (#9214) feat(mobile): move Activity to a Home bell and align Home spacing (#9212) Allow operators to set initial NIP-FI policy on create (#9211) perf(loadgen): spread seed writes across channel creators (#9206) feat(relay): runtime-tunable history-read and AUTH concurrency caps via feature flags (#9170) Signed-off-by: Brad Seiler <seiler@squareup.com>
Co-authored-by: Honor <e0bf479eeebe47dde1e35705212a4df70616a7bc834d907ddfb18cf0405f8b1b@seiler-test.communities.public.builderlabstaging.xyz> Signed-off-by: Brad Seiler <seiler@squareup.com>
Unknown paths on relays without a web fallback now return 404 instead of inheriting the internal Git endpoint localhost rejection. Keep the localhost guard outermost and retain the 1 MB policy request limit. The signed removed-operator-route 404 assertion remains intact. Co-authored-by: Honor <e0bf479eeebe47dde1e35705212a4df70616a7bc834d907ddfb18cf0405f8b1b@seiler-test.communities.public.builderlabstaging.xyz> Signed-off-by: Brad Seiler <seiler@squareup.com>
Preserve main migration 84 and its checksum. Apply the additive policy expansion as migration 85, and test upgrading populated receipts from main schema 84. Keep the exact listed-name migration ledger assertions. Co-authored-by: Honor <e0bf479eeebe47dde1e35705212a4df70616a7bc834d907ddfb18cf0405f8b1b@seiler-test.communities.public.builderlabstaging.xyz> Signed-off-by: Brad Seiler <seiler@squareup.com>
Summary
Operators can enable, disable and inspect a community's stored NIP-FI requirement through signed POST/GET
/operator/communities/nip-fi-policyandbuzz-admin nip-fi-policy. This replaces the old enable-only endpoint and drops thenip-fi-upgradecommand added earlier in this PR, along with obsolete compatibility code.Each saved intent binds its desired state and observed generation/state. Every accepted new intent advances the generation, including same-state changes. Committed retries recover history without reapplying policy; stale requests are refused. Current status exposes one pair of reusable
expected_*preconditions. Historical receipts remain readable after policy changes or target deletion, including old rows with null preconditions. Their UUIDs cannot be reused for new requests.Migration 0085 stays additive; main’s default-channel recovery migration retains version 0084. It preserves old receipt rows and INSERT compatibility. Operator instructions cover use, recovery and client migration: usage guide, client contract.
Rollout and containment
The old
/operator/communities/nip-fi-upgraderoute is removed; GET and POST return 404 without its old auth handler. A pending sponsorship client still targets that route. Migrate that client to the supported API and deploy it with mutation dispatch paused before this relay version reaches its fleet. Its complete CAS preconditions must be durably saved before first dispatch and reused unchanged after a crash or lost response. This is a pending integration dependency, not a claim that the caller is deployed.Close
BUZZ_NIP_FI_UPGRADES_ENABLEDon every old relay. KeepBUZZ_NIP_FI_CHANGES=falseon new relays. Apply schema first and finish the whole-fleet rollout before opening the new gate and resuming migrated clients. Before opening the new gate, verify the deployed artifact SHA on every pod and probe each pod directly: the old route must return 404, and a fresh, well-formed, signed policy request with a new UUID must return 503upgrades_disabled. A committed retry is not a gate probe. Old pods with the old gate open can still accept unfenced enables. Do not roll back to those writers after a policy operation. The old variable is no longer read by this version; the new gate defaults off and controls both directions.Read accepted intent in
receipt, current preconditions/effective policy through status, and effective discovery through/info. Enable stayspendingbecause receipts cannot certify fleet-wide socket closure. Under globalenforce, stored disable leaves FI effectively required. Closing the gate preserves receipt recovery; use a fresh CAS disable or existing archive controls for containment. Preserve uncertain requests and their UUIDs. This PR performs no live state change or deployment.Related issue
N/A
Testing
Published repair head:
33a1c0bb87a6fa10212eb4ec60788e24189627e2, incorporating main39d5159caf. The Git policy guards now apply only to matched paths. Unknown GET/POST paths on relays without a web directory return 404 instead of inheriting the internal endpoint’s 403. The matched internal endpoint retains its localhost restriction and 1 MB body limit, including its method fallback. The signed removed-route 404 assertion is unchanged.Main’s migration 0084 is unchanged; the additive policy migration is now 0085 with identical SQL. Its populated-receipt upgrade test starts at schema 84. Formatting and commit hooks pass; independent source review has no findings. At this exact head, formatting, all-target Clippy (
-D warnings, relay/db/admin/pubsub), and test compilation (relay/db/admin,--lib --tests --no-run) pass with exit 0. The full native PostgreSQL lane passes: 1,373/1,373 tests, exit 0, including the unchanged signed removed-route 404 assertion, fresh-database migration and populated receipt upgrade from main schema 84 to 85. The first full package run used an uninitialized private database and stopped with two missing-table errors; that failed attempt is retained. After schema initialization and verification, the corrected full package rerun passes: 2,041/2,041 tests, exit 0 (1,396 skipped by unit selection). The separate PostgreSQL lane skips 2,064 tests by its selection. Both full runs use the exact published repair head. Hosted PostgreSQL Tests, Rust Unit Tests, Windows Rust and the integration suites pass. Overall hosted CI remains pending; the mesh lifecycle smoke job was still running at the latest read. The new router test reproduced the old failure with the exact localhost-only response body; the new production-router regression also passes in the corrected package run. All four individual guard mutations fail the intended assertion (exit 101): moving either guard onto the fallback, removing localhost protection, or removing the body limit. After restoration the regression passes (exit 0) and the working tree is clean at this head. Independent review has no open P1/P2 findings.Brad authorized one
git push --no-verifyof this repair conditional on passing lint and compilation. Those gates passed; the push exited 0 and both the remote branch and GitHub PR SHA were verified as33a1c0bb87a6fa10212eb4ec60788e24189627e2. The normal publication attempt was stopped before completion and its log is retained. This exception leaves remaining tests and acceptance requirements outstanding.Historical evidence below applies to the earlier published head, not the published repair.
Published head:
d31793ccbcdfd748361bcd559358915862b5ed57. This integrates main's missingChannelEvent.received_atfixture correction and corrects the newly integrated initial-policy test to usenip_fi_changes, with matching documentation. Independent source review has no open findings. At this exact head, all-target Clippy with-D warningspasses for buzz-relay, buzz-db, buzz-admin and buzz-pubsub; independent Clippy and formatting checks also passed.The normal push hooks failed (push exit 1):
buzz-acprecovery test hit its 15-second test deadline. Nextest completed 844/1101 tests: 843 passed, 1 failed, 3 skipped; 257 were not run.buzz-terminallogin-shell test received no child output within its 10-second test deadline; 26 passed, 1 failed.2 trailing images match carousel at 208.0, scale 1.0expected no loading placeholders and found two; 4,524 passed, 60 skipped, 1 failed.These are two test-level deadline failures and one assertion failure, not compiler errors. The failing source paths are unchanged from main
6c3ffd160b; no baseline reproduction or flaky classification is claimed. Desktop JS checks/tests/typecheck, branch-skew and file-size checks passed. Full hooks are not reported as passing. The first hosted PostgreSQL run at d31793c failed the removed-route assertion (expected 404, got 403); the router repair above addresses that failure. That hosted run’s Rust unit, Tauri and mobile jobs passed.Historical d31793c publication exception: Brad explicitly authorized a single
git push --no-verifyfor this head on 2026-11-06 after the failed normal-hook run. That push exited 0 and its remote SHA was verified. Failed-hook evidence is retained. The earliercf2c9ba599publication also used an explicit exception. Neither exception waives outstanding validation, review or human acceptance before merge.Historical evidence at
5b2eb6f09366b8a8f480a5fc61e8e5368f62302d: 2,006 affected-package tests and 32 disposable-relay checks passed. These predate the latest main merge and are not final-head evidence. Earlier native PostgreSQL and separatejust ci/just testattempts were incomplete, interrupted or failed and are not passing results.No staging run or human runtime confirmation is claimed. This PR is not a draft and is ready for review; readiness does not attest human runtime confirmation.
Generated with Codex