fix: three-service audit remediation (bia-admin) — DB security lockdown + atomic RPCs - #65
Merged
Conversation
…ranch) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughChangesAdmin mutation and database hardening
Audit documentation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant AdminClient
participant AdminRoute
participant SupabaseRPC
participant Database
AdminClient->>AdminRoute: Submit admin mutation
AdminRoute->>SupabaseRPC: Invoke named atomic RPC
SupabaseRPC->>Database: Validate and mutate records
Database->>Database: Persist audit, revision, or timeline state
SupabaseRPC-->>AdminRoute: Return data or affected-row result
AdminRoute-->>AdminClient: Return HTTP response
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The dead-helper cleanup stripped the _ctx parameter type, making handler.mock.calls[0][0] an empty-tuple index error under tsc --noEmit (unit tests pass without type-checking, so it slipped through). Restore the Parameters<...> annotation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rifted prod
The migration hardened approve_event_submission and outgoing_bubbles assuming
their prerequisite migrations (20260624000008, 20260625000002) were already
applied. Production skipped those (one-at-a-time MCP application), so applying
this in isolation aborted the whole transaction ('function/relation does not
exist'). Fixes:
- approve_event_submission: wrap the hardening in a to_regprocedure guard
(skip-safe — a missing function has no execute exposure; ordered apply creates
it first so it still gets hardened).
- outgoing_bubbles: fold in create-table-if-not-exists (george's claim-based
delivery hard-depends on the table, so ensure it exists rather than skip).
Validated against Postgres 16: guard skips the missing function, the table is
created and hardened when absent, both paths re-run idempotently, and the
20-assertion static test + tsc stay green.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
supabase/migrations/20260710210853_audit_security_and_state_machines.sql (1)
223-228: 🚀 Performance & Scalability | 🔵 TrivialAdding
idempotency_key NOT NULL DEFAULT gen_random_uuid()rewrites an existing table under ACCESS EXCLUSIVE.Per the surrounding comment,
outgoing_bubblesnormally already exists (prerequisite20260625000002) with rows.gen_random_uuid()is VOLATILE, soadd column ... not null default gen_random_uuid()forces a full table rewrite holding anACCESS EXCLUSIVElock, blocking reads and writes for the duration. On a fresh (empty) table this is a no-op, but against a populated production table it can stall George's delivery path during deploy. Consider adding the column nullable, backfilling in batches, then settingNOT NULL+ default, if the table can be large.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@supabase/migrations/20260710210853_audit_security_and_state_machines.sql` around lines 223 - 228, Modify the outgoing_bubbles migration to avoid adding idempotency_key with a volatile default and NOT NULL in one table-rewriting operation. Add idempotency_key as nullable without a default, backfill existing rows in bounded batches using gen_random_uuid(), then set the column’s default and enforce NOT NULL after the backfill; keep the other column additions unchanged.Source: Linters/SAST tools
bia-admin/app/api/admin/members/invite/__tests__/route.test.ts (1)
190-209: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the
audit_failederror path.The new rollback failure test is well done. However, the route's
audit_failedpath (whenwriteAuditRequiredthrows after a successful invite) is untested. This is a new error-handling behavior introduced in this PR and should be verified.🧪 Suggested test for the audit failure path
it("reports rollback failure instead of leaving a silent ghost invitation", async () => { // ... existing test ... }); + + it("returns audit_failed when the durable audit write throws", async () => { + setupServerSelfRead("super_admin"); + setupServiceFrom({ + existingAdmin: null, + insertResult: { data: { id: "inv1" }, error: null }, + }); + mockInviteUserByEmail.mockResolvedValue({ error: null }); + mockWriteAudit.mockRejectedValue(new Error("audit_write_failed: boom")); + + const res = await POST( + makeRequest({ email: "ex@example.com", role: "editor" }), + ); + + expect(res.status).toBe(500); + expect(await res.json()).toEqual({ + error: "audit_failed", + details: "Error: audit_write_failed: boom", + }); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bia-admin/app/api/admin/members/invite/__tests__/route.test.ts` around lines 190 - 209, Add a test in the invite route test suite covering the audit_failed path: configure a successful invitation followed by writeAuditRequired throwing an error, call POST with valid admin and invitation data, and assert the response status and JSON error payload match the route’s audit failure behavior, including the error details.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/audits/2026-07-10-three-service-full-audit.md`:
- Around line 52-59: Clarify the “Verification results” section as the
initial/pre-remediation snapshot to distinguish it from the post-remediation
results described earlier. Update the heading or add an explicit label
indicating these are historical results, or remove the stale table if it is not
authoritative.
In `@docs/superpowers/plans/2026-07-10-three-service-audit-fixes.md`:
- Around line 33-38: Update parseCadenceMs so “off” remains the only value
producing null, while unknown or malformed cadence values return an explicit
rejection/error result instead of silently disabling scheduling. Revise the
related interface, callers, tests, and checklist to distinguish intentional
disablement from invalid persisted data, covering both cases explicitly.
- Around line 81-87: Make migration application and security verification
mandatory before deployment: require resetting/applying the migration against
the target database, running advisor and catalog checks, and executing
anonymous/authenticated negative tests. Update the checklist item in the
migration plan to remove the conditional local-stack wording and explicitly
block service deployment until all database contract and security checks pass.
- Around line 54-64: Update the delivery contract and implementation checklist
to require a durable idempotency key for every outgoing message, plus explicit
reconciliation and terminal-failure handling when sending succeeds but
acknowledgement fails. Extend the scheduler interface and implementation items
around atomic claiming, bounded retries, and send/mark-failure behavior, and add
regression tests covering duplicate prevention and reconciliation or terminal
failure.
---
Nitpick comments:
In `@bia-admin/app/api/admin/members/invite/__tests__/route.test.ts`:
- Around line 190-209: Add a test in the invite route test suite covering the
audit_failed path: configure a successful invitation followed by
writeAuditRequired throwing an error, call POST with valid admin and invitation
data, and assert the response status and JSON error payload match the route’s
audit failure behavior, including the error details.
In `@supabase/migrations/20260710210853_audit_security_and_state_machines.sql`:
- Around line 223-228: Modify the outgoing_bubbles migration to avoid adding
idempotency_key with a volatile default and NOT NULL in one table-rewriting
operation. Add idempotency_key as nullable without a default, backfill existing
rows in bounded batches using gen_random_uuid(), then set the column’s default
and enforce NOT NULL after the backfill; keep the other column additions
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7d796290-6c02-4b2e-b523-13c03accfe57
📒 Files selected for processing (23)
bia-admin/app/api/admin/articles/[id]/__tests__/route.test.tsbia-admin/app/api/admin/articles/[id]/route.tsbia-admin/app/api/admin/articles/__tests__/route.test.tsbia-admin/app/api/admin/events/[id]/__tests__/route.test.tsbia-admin/app/api/admin/events/[id]/route.tsbia-admin/app/api/admin/events/__tests__/route.test.tsbia-admin/app/api/admin/members/[id]/__tests__/route.test.tsbia-admin/app/api/admin/members/[id]/route.tsbia-admin/app/api/admin/members/invitations/[id]/__tests__/route.test.tsbia-admin/app/api/admin/members/invitations/[id]/route.tsbia-admin/app/api/admin/members/invite/__tests__/route.test.tsbia-admin/app/api/admin/members/invite/route.tsbia-admin/app/api/admin/shipping/__tests__/role-gates.test.tsbia-admin/app/api/admin/shipping/parcels/__tests__/route.test.tsbia-admin/app/api/admin/shipping/parcels/route.tsbia-admin/lib/admin/__tests__/audit-log.test.tsbia-admin/lib/admin/__tests__/audit-security-migration.test.tsbia-admin/lib/admin/__tests__/slug.test.tsbia-admin/lib/admin/audit-log.tsbia-admin/lib/matching/__tests__/vector-builder.test.tsdocs/audits/2026-07-10-three-service-full-audit.mddocs/superpowers/plans/2026-07-10-three-service-audit-fixes.mdsupabase/migrations/20260710210853_audit_security_and_state_machines.sql
💤 Files with no reviewable changes (2)
- bia-admin/app/api/admin/articles/tests/route.test.ts
- bia-admin/app/api/admin/events/tests/route.test.ts
Comment on lines
+52
to
+59
| ### Verification results | ||
|
|
||
| | Repo | Result | | ||
| |---|---| | ||
| | bia-admin | Lint: 0 errors, 23 normal warnings. Tests: 355 passed, 25 DB integration tests skipped. Production build passed. Shared package: 67 tests passed. | | ||
| | bia-roommate | Lint: 0 errors, 2 warnings. Tests: 331 passed. Production build passed. | | ||
| | George | TypeScript build passed. With documented non-secret test placeholders: 1,354 passed, 11 skipped. Default local `npm test` is not hermetic because production env vars are required at import time. | | ||
| | Deploy status | Latest GitHub commits report successful Vercel deploys for admin/roommate and successful Railway deploy for George. | |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Disambiguate the verification snapshots.
Lines 23-28 describe post-remediation results, while this undated table reports different counts and build outcomes. Label this section as the initial/pre-remediation snapshot, or remove the stale values, so readers can identify the authoritative release evidence.
Suggested clarification
-### Verification results
+### Initial audit verification results (before remediation)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ### Verification results | |
| | Repo | Result | | |
| |---|---| | |
| | bia-admin | Lint: 0 errors, 23 normal warnings. Tests: 355 passed, 25 DB integration tests skipped. Production build passed. Shared package: 67 tests passed. | | |
| | bia-roommate | Lint: 0 errors, 2 warnings. Tests: 331 passed. Production build passed. | | |
| | George | TypeScript build passed. With documented non-secret test placeholders: 1,354 passed, 11 skipped. Default local `npm test` is not hermetic because production env vars are required at import time. | | |
| | Deploy status | Latest GitHub commits report successful Vercel deploys for admin/roommate and successful Railway deploy for George. | | |
| ### Initial audit verification results (before remediation) | |
| | Repo | Result | | |
| |---|---| | |
| | bia-admin | Lint: 0 errors, 23 normal warnings. Tests: 355 passed, 25 DB integration tests skipped. Production build passed. Shared package: 67 tests passed. | | |
| | bia-roommate | Lint: 0 errors, 2 warnings. Tests: 331 passed. Production build passed. | | |
| | George | TypeScript build passed. With documented non-secret test placeholders: 1,354 passed, 11 skipped. Default local `npm test` is not hermetic because production env vars are required at import time. | | |
| | Deploy status | Latest GitHub commits report successful Vercel deploys for admin/roommate and successful Railway deploy for George. | |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/audits/2026-07-10-three-service-full-audit.md` around lines 52 - 59,
Clarify the “Verification results” section as the initial/pre-remediation
snapshot to distinguish it from the post-remediation results described earlier.
Update the heading or add an explicit label indicating these are historical
results, or remove the stale table if it is not authoritative.
Comment on lines
+33
to
+38
| - Produces `parseCadenceMs(cadence: string): number | null` with explicit mappings for `12 hours`, `24 hours`, `7 days`, and `off`. | ||
| - Adds `claimDueFollowups(userId): Promise<FollowupRow[]>`, `markFollowupsTriggered(ids): Promise<void>`, and `releaseFollowups(ids): Promise<void>` to heartbeat dependencies. | ||
|
|
||
| - [ ] Write a test proving `7 days` is 168 hours rather than the 12-hour fallback and invalid cadence is rejected/fails closed. | ||
| - [ ] Run `npm test -- tests/jobs/heartbeat-scheduler.test.ts` and observe the weekly test fail. | ||
| - [ ] Replace regex-plus-default parsing with an exhaustive constant mapping; use `null` for `off` and unknown values. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not conflate off with an invalid cadence.
The interface returns null for both off and unknown values, while the checklist says invalid cadence must be rejected. A malformed persisted value can therefore silently disable scheduling as if the user selected off. Use null only for off and return an explicit rejection/error result for unknown values.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/superpowers/plans/2026-07-10-three-service-audit-fixes.md` around lines
33 - 38, Update parseCadenceMs so “off” remains the only value producing null,
while unknown or malformed cadence values return an explicit rejection/error
result instead of silently disabling scheduling. Revise the related interface,
callers, tests, and checklist to distinguish intentional disablement from
invalid persisted data, covering both cases explicitly.
Comment on lines
+54
to
+64
| **Interfaces:** | ||
| - `runHeartbeat` validates `response.toolCalls.length === 1` before invoking any handler. | ||
| - Outgoing rows are obtained through a claim/lease operation rather than plain `selectDue`. | ||
| - Spectrum disconnect does not report a legacy-queue enqueue as delivered. | ||
|
|
||
| - [ ] Write a failing test where two returned tool calls cause zero side effects and an error outcome. | ||
| - [ ] Enforce exactly one known tool call before execution and preserve an accurate single outcome. | ||
| - [ ] Write failing scheduler tests for two workers claiming the same row and for send-success/mark-failure ambiguity. | ||
| - [ ] Change the DB seam to claim rows atomically with worker/lease metadata and bounded retries. | ||
| - [ ] Write a failing proactive-sender test for Spectrum reconnect; require retryable failure or a Spectrum-drained queue. | ||
| - [ ] Implement the transport-specific failure behavior and adaptive idle/error polling instead of unconditional one-second requests. |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make idempotency and reconciliation explicit in the delivery contract.
A claim/lease plus bounded retries does not resolve the send-success/ack-failure case: retrying can deliver the same message twice. Require a durable idempotency key and an explicit reconciliation or terminal-failure path in the interface, implementation checklist, and regression tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/superpowers/plans/2026-07-10-three-service-audit-fixes.md` around lines
54 - 64, Update the delivery contract and implementation checklist to require a
durable idempotency key for every outgoing message, plus explicit reconciliation
and terminal-failure handling when sending succeeds but acknowledgement fails.
Extend the scheduler interface and implementation items around atomic claiming,
bounded retries, and send/mark-failure behavior, and add regression tests
covering duplicate prevention and reconciliation or terminal failure.
Comment on lines
+81
to
+87
| - [ ] Run `supabase --version` and `supabase migration new audit_security_and_state_machines`; use the generated filename. | ||
| - [ ] Add catalog/migration tests that fail for exposed definer views/functions and missing RLS. | ||
| - [ ] Add SQL for explicit grants/revokes, RLS, invoker view, obsolete-view removal, and transactional RPCs with fixed `search_path`. | ||
| - [ ] Add partial unique index `roommate_profiles(user_id) WHERE user_id IS NOT NULL` after a duplicate-detection guard/query. | ||
| - [ ] Add indexes supporting claim queries and `FOR UPDATE SKIP LOCKED` leases. | ||
| - [ ] Run SQL lint/migration tests and admin unit tests; if a local Supabase stack exists, reset and run integration tests/advisors. | ||
| - [ ] Commit as `fix: harden database contracts and atomic workflows`. |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Make migration application and security verification a hard pre-deployment gate.
The “if a local Supabase stack exists” wording makes reset, advisor, catalog, and anonymous/authenticated negative tests optional. Because the dependent services call the new RPCs and rely on the new security contracts, require applying the migration and verifying the target database before deploying those services; otherwise deployment can either fail at runtime or leave the audited exposure unresolved.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/superpowers/plans/2026-07-10-three-service-audit-fixes.md` around lines
81 - 87, Make migration application and security verification mandatory before
deployment: require resetting/applying the migration against the target
database, running advisor and catalog checks, and executing
anonymous/authenticated negative tests. Update the checklist item in the
migration plan to remove the conditional local-stack wording and explicitly
block service deployment until all database contract and security checks pass.
Owner
Author
Update — migration abort blocker RESOLVED (commit 5cf54a2)The migration no longer aborts against the current drifted production DB. Added existence handling for the two objects whose prerequisite migrations prod had skipped:
Validated against Postgres 16: guard skips the missing function without aborting; the table is created + hardened when absent; both paths re-run idempotently (no-op when the prerequisites are already applied). The 20-assertion static migration test + Still true: the deploy-ordering gate remains — apply this migration to prod (now safe to apply in isolation or via ordered |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the bia-admin fixes from the 2026-07-10 three-service audit (included in
docs/audits/). Codex-generated on an isolatedcodex/audit-fixesbranch; replayed cleanly onto currentmainand independently re-verified here.What's in it
supabase/migrations/20260710210853_audit_security_and_state_machines.sql(486 lines): locks downSECURITY DEFINERfunctions that werePUBLIC-executable (approve_event_submission,append_to_profile_block,publish_scheduled_articles) via REVOKE/GRANT toservice_role; enables RLS on internal tables (user_observations,proactive_raised_threads,identity_conflicts,student_followups,outgoing_bubbles); setssecurity_invokeron thesponsors_publicview; moves multi-row DB invariants into atomic RPCs (admin_create_parcel_atomic,admin_delete_event_atomic,admin_update_article_atomic,admin_update_member_role_atomic,admin_delete_member_atomic,admin_revoke_invitation_atomic,create_pack_request); adds claim/lease + attempts + bounded-failed columns forstudent_followups/outgoing_bubbles.writeAuditupgraded towriteAuditRequiredon the two cross-system-boundary routes.withRole+ audit preserved on every state-changing path.Verification (independently re-run)
A read-only check against production (
ujkaregrwrppaehvbahf) found the migration references two objects that do not exist in the live DB, because their prerequisite migrations were written but never applied:approve_event_submission(uuid, uuid)—ALTER FUNCTIONon line ~54 throws (function absent; prerequisite20260624000008).public.outgoing_bubbles— allALTER TABLE/claim_due_outgoing_bubblesstatements throw (table absent; prerequisite20260625000002).Supabase runs a migration in one transaction, so a mid-file failure applies none of it. Before this can go live: apply the two prerequisite migrations to prod (or add
to_regprocedure/to_regclassexistence guards — the migration already uses that pattern forsquad_member_counts), then runsupabase db reset+ advisors + anon/authenticated negative tests as the audit requires. This needs Supabase MCP/prod access and a human decision — do not merge-and-deploy the routes until the migration actually applies.Migration applied to prod → deploy bia-admin routes → deploy george → deploy bia-roommate. Every changed route (and george's + roommate's companion branches) calls an RPC created only by this migration; code-first deploy returns HTTP 500 (PostgREST "function not found").
Not addressed here (still open from the audit)
P0-3 (public George relay is an unauthenticated admin-token proxy accepting a caller-selected
userId) and P0-4 (onboarding creates auth users behind a reusable 6-char code, no rate limit/atomic claim) are not fixed by this branch — they need a separate auth/session + onboarding redesign.🤖 Generated with Claude Code
Summary by CodeRabbit