Repository navigation
fix(db): repair historical invitation revocation - #8529
bradseiler wants to merge 2 commits into
Conversation
d0c5b7d to
425d89b
Compare
🔐 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.
🤖 Automated review of 425d89b4 — no blocking findings.
The repair script meets the docs/database.md Rule 3 requirements for a pre-framework one-off repair: it is idempotent, works in batches of 500 that each commit from a top-level DO block, takes the shared community deletion lock and re-reads the community's state inside the same transaction, and ends with a validation query that expects 0. Its revocation rule matches the claim-time has_revoking_restriction and is_canonical_invite_pubkey checks in relay_invite.rs. Its lock order (shared deletion lock, then invitation-admission lock, then invite rows) matches the claim, ban and owner-link writers. Run inside a caller transaction, the first internal COMMIT errors and nothing persists.
The PostgreSQL CI lane on this head passed every new and changed repair test. A separate hands-on run against a fresh disposable PostgreSQL 17 database (migrated through 0074, then 0075) seeded 1,226 invitations, 1,224 of them in one community so the repair crossed two batch boundaries. The checked-in script then ran twice through psql. Both runs reported zero unrepaired invites, and the second run left every invitation row byte-identical. Legacy restriction history, malformed issuers, owner restrictions, native active bans, timeout-only history, other tenants, existing revocation timestamps and a fenced community all came out as expected.
Minor findings:
scripts/maintenance/backfill_invite_revocations.sql:17:SET statement_timeout = '2min'limits the wholeDOblock across every community, not each batch. The internalCOMMITs do not reset the timer: a 1-second timeout around three 0.6-second iterations that commit between them is cancelled at 1.0 s on PostgreSQL 17. On a large relay the script would stop before its validation query and need repeated reruns. Either drop the statement timeout, sincelock_timeoutalready limits lock waits and batches are capped at 500, or document inTESTING.mdthat a timeout means rerun until the validation query runs.scripts/maintenance/backfill_invite_revocations.sql:71,:85,:150: the same issuer-or-current-owner lookup againstcommunity_bansis written out three times (has_retained_restriction,has_active_ban, and the validation query). Use one lateral overcommunity_bans WHERE community_id = … AND pubkey IN (issuer, owner)withbool_or(true)/bool_or(banned AND …), the shapeprincipal_restricteduses, so the copies can't drift from each other or from the application predicate.scripts/maintenance/backfill_invite_revocations.sql:1andTESTING.md:225: "This companion must be separately approved before core deployment" and "The separately approved companion provides the operator command" describe the release process rather than the script, and "companion" isn't defined for a reader who arrives at either file. Name it as the historical invitation repair script. TheTESTING.mdparagraph also has a stray line break at "lift the first restriction before / upgrade" (:242).crates/buzz-db/src/store/relay_invite/upgrade_postgres_tests.rs:388:start_invite_repair_scriptcopies the connection-parameter parsing (host, port, user, hand-split password,current_database()) fromapply_pgschema_to_existing_database(:558). Extract one shared helper. The hand-split password isn't percent-decoded, so that helper is also the one place to fix encoded passwords.crates/buzz-db/src/store/relay_invite/upgrade_postgres_tests.rs:618: the automated repair fixtures never exceed 500 invites in one community and never fail a run after an earlier batch committed. Extend an existing repair test with more than 500 invites in one community, including spared and already-revoked rows, and check that a rerun after a later-batch failure completes and preserves the earlier timestamps.
Revoke historical legacy candidates and clear examined admission markers under the deletion/admission locks. Require separate approval before core deployment and a controlled rollout that prevents new timeout/mute history. Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Brad Seiler <seiler@squareup.com> Signed-off-by: seiler <seiler@squareup.com> Signed-off-by: Brad Seiler <seiler@squareup.com>
Co-authored-by: Sarnow <1894dcbafaa4df1ac607a9bf879ac0b7d77fbccb0e0f4619ecc252c8adbad8a2@seiler-test.communities.public.builderlabstaging.xyz> Signed-off-by: Brad Seiler <seiler@squareup.com>
425d89b to
96a2686
Compare
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 Automated review of 96a2686b — no blocking findings.
This head rebases the change onto f3a7bab0 without altering it. The repair script, both testing documents and revocation_postgres_tests.rs are byte-identical to 425d89b4. The only change is in upgrade_postgres_tests.rs, which picks up main's correction: the test now checks that migration 75 succeeded, not that it is the latest. Migrations 0076–0078 add artifact indexes and timestamps, channel_members.sync_modified_at and communities.artifact_policy_modified_at. The script reads communities, relay_invites, community_bans and users and updates only relay_invites, so none of those migrations touches anything it depends on. The revocation rule still matches the claim-time checks in relay_invite.rs, and the lock order (shared deletion lock, then the invitation-admission lock, then invite rows) still matches the claim, ban, timeout and owner-link writers.
The PostgreSQL CI lane passed every upgrade and repair test on this head. A separate hands-on run used a fresh disposable PostgreSQL 17.10 database, seeded at 0074 and then migrated through 0078. It held 1,226 invitations, 1,224 of them in one community with spared, already-revoked and to-be-revoked rows interleaved, so the repair committed batches of 500, 500 and 20. The checked-in script ran twice through psql -X -v ON_ERROR_STOP=1. Both runs reported zero unrepaired invites, and both full invitation-row snapshots had the same SHA-256. Existing revocation timestamps, other tenants' rows and the fenced-community skip matched independently computed expectations. The eight upgrade_postgres_tests tests also passed against that instance.
Minor findings. The first five carry over unchanged from the review of 425d89b4:
scripts/maintenance/backfill_invite_revocations.sql:17:SET statement_timeout = '2min'limits the wholeDOblock across every community, not each batch, because the internalCOMMITs do not reset the timer. On a large relay the script can stop before its validation query. Either drop the statement timeout, sincelock_timeoutalready bounds lock waits and batches are capped at 500, or document inTESTING.mdthat a timeout means rerunning until the validation query runs.scripts/maintenance/backfill_invite_revocations.sql:71–101and:150–165: the issuer-or-current-owner lookup againstcommunity_bansis written out three times (has_retained_restriction,has_active_banand the validation query). Collapse each into one lookup overcommunity_bans WHERE community_id = … AND pubkey IN (issuer, owner), the shapeprincipal_restricteduses atrelay_invite.rs:232–238, so the copies cannot drift from each other or from the application predicate. This corrects the earlier review, which describedprincipal_restrictedas usingbool_or. It is a singleEXISTS … pubkey IN (…)query.scripts/maintenance/backfill_invite_revocations.sql:1andTESTING.md:196,:225: "companion" and "separately approved" describe the release process rather than the script, and a reader who arrives at either file has no definition for "companion". Call it the historical invitation repair script. There is also a stray line break at "lift the first restriction before / upgrade" (TESTING.md:242–243).crates/buzz-db/src/store/relay_invite/upgrade_postgres_tests.rs:392–407and:558–573:start_invite_repair_scriptcopies the connection-parameter parsing fromapply_pgschema_to_existing_database. Extract one shared helper. The hand-split password isn't percent-decoded, so that helper is also the one place to fix encoded passwords.crates/buzz-db/src/store/relay_invite/upgrade_postgres_tests.rs:618–686: the automated repair fixtures insert one invitation per community, so no test crosses a 500-row batch inside a community or fails after an earlier batch has committed. Extend an existing repair test with more than 500 mixed invites in one community, and check that a rerun after a later-batch failure completes and preserves the earlier timestamps.TESTING.md:210–214and:237–248: the steps say to apply schema 0075, but 0075 is only the invitation migration. The relay at this head also needs 0076–0078 (for example,channel_members.rs:936writessync_modified_atfrom 0077), and those migrations have their own index-prebuild procedure for populated tables indocs/artifact-sync-deployment.md. Add a sentence that separates "the repair requires 0075" from "a current-relay upgrade applies through 0078", and link that procedure, so an operator doesn't take "apply 0075" as the complete upgrade recipe.
Summary
Repair historical v2 invitation revocation after the schema-only migration merged in #8237. The operator script revokes affected legacy invitations and clears the legacy marker on examined active-community rows, including spared invitations. Later timeouts remain temporary; active bans permanently revoke invitations.
Related issue
Companion to #8237. First layer of the community security stack: #8529 → #8241 → #8239 → #8235. #8237 and #8242 are already merged.
Operation
Use schema 0075, with the populated upgrade fixture starting through 0074. Any shared or staging database that applied an older invitation candidate needs its migration history and schema reconciled by its owner first. Never reset a human or shared database.
Follow
TESTING.md. Prevent invitation mint/claim, ban lifts and new timeout/mute writes across every writer. If existing controls cannot enforce this, stop all relay instances and other writers and drain their work.After schema 0075, run
psql -X -v ON_ERROR_STOP=1 -f scripts/maintenance/backfill_invite_revocations.sqlthrough the authorized operator connection. Run at top level, outside a caller transaction. Require zero unrepaired invitations in active communities, then rerun and confirm unchanged markers and timestamps. Keep skipped or fenced communities closed until repaired. Reopen only on the new code after all checks pass.Testing
Rebased onto main
d263e2c7; current source review found no blockers. Local baseline database validation had a reader-warm timing failure outside the repair cases. Later integrated database validation passed, but this draft does not claim a clean baseline lane. The failed receipts remain preserved.No historical repair, deployment, staging verification or human acceptance was performed by this preparation. Ready for review at Brad’s request. Operator approval and verification are still required before execution. Prior-head validation is historical and does not attest this head.
Generated with Codex