Skip to content

Bound versioned retrieval RPC match_count to prevent unbounded results#756

Closed
BigSimmo wants to merge 4 commits into
mainfrom
codex/review-and-improve-supabase/postgres-layer
Closed

Bound versioned retrieval RPC match_count to prevent unbounded results#756
BigSimmo wants to merge 4 commits into
mainfrom
codex/review-and-improve-supabase/postgres-layer

Conversation

@BigSimmo

@BigSimmo BigSimmo commented Jul 17, 2026

Copy link
Copy Markdown
Owner

Motivation

  • Prevent unbounded result amplification when optional match_count RPC arguments are NULL or negative, which can turn LIMIT into an unbounded scan and multiply JSON hydration and response work on hot retrieval paths.

Description

  • Add a forward migration supabase/migrations/20260717170000_bound_versioned_retrieval_match_count.sql that replaces the two versioned wrappers with deterministic clamping of the exposed match_count to least(greatest(coalesce(...), 1), 96) while preserving existing signatures and defaults for callers.
  • Apply the same bound to match_document_chunks_text_v2 (text RPC) and match_document_index_units_hybrid_v2 (index-unit RPC) and keep inner scoped helpers unchanged so candidate-window work remains capped.
  • Re-assert function privileges by revoking anon/authenticated and granting execute only to service_role for the new wrappers to preserve the service-role-only execution posture.
  • Add a regression contract assertion in tests/retrieval-access-scope.test.ts that checks for the clamp expressions and the revoke/grant lines, and record the database review in docs/branch-review-ledger.md.

Testing

  • Ran npm run check:function-grants, which succeeded and confirmed SECUIRTY DEFINER functions are not re-opened to public/anon/authenticated.
  • Performed a static SQL migration-contract audit (local Node script) that verified the clamp expressions and that execute grants target only service_role (passed).
  • Ran git diff --check and static diff checks (passed).
  • Attempted focused and full unit runs: npm run test:focused -- --files tests/retrieval-access-scope.test.ts refused because changed test paths require the full suite, and npm run test failed due to missing vitest in node_modules (MODULE_NOT_FOUND), so unit-suite verification could not complete in this environment.
  • Attempted repository gates: npm run verify:cheap / npm run check:runtime could not complete because tsx/CLI is unavailable and this environment is running Node 20.x while the repo requires Node 24.x (precondition failure), so the aggregate gates were not executed.
  • Did not run provider-backed or live checks: npm run check:drift, npm run check:production-readiness, Docker-backed npm run drift:manifest, or any live Supabase actions were intentionally not executed per policy.

Files changed

  • Added supabase/migrations/20260717170000_bound_versioned_retrieval_match_count.sql (new migration wrapping/clamping the RPCs).
  • Modified tests/retrieval-access-scope.test.ts to assert the migration/regression contract.
  • Appended one entry to docs/branch-review-ledger.md documenting the database/retrieval review.

Notes and residual risk

  • This is a forward-only, data-free migration that only changes RPC wrappers; rollback is to recreate the previous pass-through wrappers from the prior migration if needed.
  • Full unit and CI gates (including Docker replay to regenerate supabase/drift-manifest.json and supabase/schema.sql parity checks) must be run in a Node 24 + Docker-enabled environment before merging and applying to hosted Supabase.
  • No live Supabase, OpenAI, or other provider-backed operations were performed or are required to land this change; operator apply/verification remains confirmation-gated.

Codex Task

Summary by CodeRabbit

  • Bug Fixes

    • Retrieval requests now consistently limit results to between 1 and 96 items, including when invalid or null counts are provided.
    • Preserved owner and public-content filtering behavior.
    • Restricted the updated retrieval operations to authorized service access.
  • Tests

    • Added coverage for result-count limits and access permissions.
  • Documentation

    • Recorded the migration review and verification status.

@supabase

supabase Bot commented Jul 17, 2026

Copy link
Copy Markdown

This pull request has been ignored for the connected project sjrfecxgysukkwxsowpy due to reaching the limit of concurrent preview branches.
Go to Project Integrations Settings ↗︎ if you wish to update this limit.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 0d0d9f29-bcfb-4d18-a0fc-02d6ca7f7d97

📥 Commits

Reviewing files that changed from the base of the PR and between ed1f71b and 21c0720.

📒 Files selected for processing (2)
  • supabase/migrations/20260717162000_bound_versioned_retrieval_match_count.sql
  • tests/retrieval-access-scope.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/retrieval-access-scope.test.ts

📝 Walkthrough

Walkthrough

Adds two versioned retrieval RPC wrappers that clamp match_count to 1–96, preserve scoped filtering, restrict execution to service_role, and add migration-level tests plus a review-ledger entry.

Changes

Retrieval RPC hardening

Layer / File(s) Summary
Bounded wrappers and access control
supabase/migrations/20260717162000_bound_versioned_retrieval_match_count.sql
Adds stable wrappers for text and hybrid retrieval, clamps nullable or negative counts to 1–96, and grants execution only to service_role.
Migration validation and ledger
tests/retrieval-access-scope.test.ts, docs/branch-review-ledger.md
Loads the migration fixture, verifies count clamping and revocations, and records verification limitations.
Estimated code review effort: 2 (Simple) ~10 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description is informative, but it does not follow the repository’s required template sections like Summary, Risk and rollout, and Clinical Governance Preflight. Rewrite the PR description to match the template headings and fill in Summary, Verification, Risk and rollout, Notes, and any applicable Clinical Governance Preflight items.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and accurately describes the main change: bounding versioned retrieval RPC match_count to avoid unbounded results.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/review-and-improve-supabase/postgres-layer

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Jul 17, 2026

Copy link
Copy Markdown

CI triage

CI failed on this PR. Automated classification of the 2 failed job(s):

  • Unit coverageneeds investigation: inspect the failing step and uploaded diagnostics; rerun only after classifying the cause.
  • PR requiredneeds investigation: inspect the failing step and uploaded diagnostics; rerun only after classifying the cause.

Compared with main CI run #2957 (success).

Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger.

@BigSimmo
BigSimmo marked this pull request as ready for review July 17, 2026 19:37
@BigSimmo
BigSimmo enabled auto-merge July 17, 2026 19:37
…and fix format

Migration 20260717170000_bound_versioned_retrieval_match_count.sql collided
with main's 20260717170000_registry_projection_cleanup.sql. Renamed to
20260717174000 (next available slot after main's 20260717173000 tip).

Also update the migration filename reference in
tests/retrieval-access-scope.test.ts and apply prettier format fix to
pass the format:check gate.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 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 `@tests/retrieval-access-scope.test.ts`:
- Around line 126-129: Update the ACL assertions in the
boundedRetrievalMigration test to verify each complete REVOKE clause, including
the authenticated role, rather than using prefix-only checks. Also assert both
full GRANT statements targeting service_role, preserving the exact function
signatures and privileges defined by the migration.
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: bd1e8520-ba70-4334-9cf0-795fa9565c61

📥 Commits

Reviewing files that changed from the base of the PR and between b04c95f and ed1f71b.

📒 Files selected for processing (3)
  • docs/branch-review-ledger.md
  • supabase/migrations/20260717174000_bound_versioned_retrieval_match_count.sql
  • tests/retrieval-access-scope.test.ts

Comment on lines +126 to +129
expect(boundedRetrievalMigration).toContain(
"revoke all on function public.match_document_chunks_text_v2(text, integer, uuid[], uuid, boolean)",
);
expect(boundedRetrievalMigration).toContain("revoke all on function public.match_document_index_units_hybrid_v2(");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win

Assert the complete ACL contract.

These prefix checks still pass if authenticated is omitted from REVOKE, or if the service_role grants are removed. Assert each full revoke clause and both GRANT ... TO service_role statements.

🤖 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 `@tests/retrieval-access-scope.test.ts` around lines 126 - 129, Update the ACL
assertions in the boundedRetrievalMigration test to verify each complete REVOKE
clause, including the authenticated role, rather than using prefix-only checks.
Also assert both full GRANT statements targeting service_role, preserving the
exact function signatures and privileges defined by the migration.

…nstraint

The supabase-schema.test.ts guard requires the final alphabetical migration to
be 20260717173000_reassert_supabase_admin_default_privileges.sql. Moving
the bound-match-count migration from 20260717174000 to 20260717162000 (a
free slot between the 161000 assert and the 170000 main cluster) satisfies
this ordering constraint while avoiding all existing timestamp collisions.

Update tests/retrieval-access-scope.test.ts to reference the new filename.
@BigSimmo BigSimmo closed this Jul 17, 2026
auto-merge was automatically disabled July 17, 2026 20:14

Pull request was closed

@BigSimmo
BigSimmo deleted the codex/review-and-improve-supabase/postgres-layer branch July 18, 2026 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants