Skip to content

Read the hub document count with a credential that has privileges - #2512

Merged
BigSimmo merged 3 commits into
mainfrom
claude/corpus-health-panel-na3hbs
Sep 1, 2026
Merged

Read the hub document count with a credential that has privileges#2512
BigSimmo merged 3 commits into
mainfrom
claude/corpus-health-panel-na3hbs

Conversation

@BigSimmo

@BigSimmo BigSimmo commented Sep 1, 2026

Copy link
Copy Markdown
Owner

Summary

  • The developer hub's document count could never load. environment-facts.ts counted public.documents through the cookie-bound user client, on the reasoning that the documents owner read policy (owner_id = auth.uid()) would scope it in the database. It cannot, so the read returned permission denied on every hub load and the environment strip could only ever render "document count unavailable".
  • Fixed with the shape that landed for the corpus-health panel in Show which documents finished indexing and produced nothing usable #2504: the cookie-bound client identifies the caller and reads no table, the caller must carry the administrator claim DeveloperAreaGate checks, and the count filters on owner_id explicitly.
  • The source assertion that pinned the wrong client is replaced, not dropped.

Why the old read could not succeed

  • supabase/schema.sql:5299 runs revoke all privileges on all tables in schema public from anon, authenticated;, and the grant block below it names public.documents for service_role only.
  • supabase/migrations/20260725000000_audit_security_remediation.sql:81 re-applies that same blanket revoke, after every earlier grant select … to authenticated. No migration after 20260725000000 restores table SELECT to authenticated — the only to authenticated hits in later migrations are RLS policies in 20260823090000_user_favourite_sets.sql.
  • The schema comment below the grant block states the intent directly: browser clients receive no direct table privileges, signed-in access is mediated by the server routes, and the owner policies remain only as defence in depth.

Row-level security cannot restore a missing SQL SELECT privilege, so the policy sat behind a privilege the authenticated role does not hold.

Why nobody saw it. The module degrades a failed read to null by design, and its tests mock the Supabase client, so both halves reported exactly what a healthy empty result looks like. It surfaced only because the corpus-health panel copied this module as its model and hit the same wall, where Codex caught it as a P1 on #2504.

What changed

The owner filter is now the entire owner-scoping guarantee rather than an addition to row-level security, because the service-role client is not subject to those policies. A non-administrator still gets their email back rather than a blank strip, since it was already read and does not depend on the count. Every failure path still returns null and never 0, and the rejection guard is unchanged.

The test named "reads through the user-session client and never the service-role client" was backwards. It is replaced by two assertions — that the issued query carries owner_id equal to the caller's id, and that a non-administrator issues no query at all — rather than deleted quietly. Every existing test pinning null-not-zero and the rejection guard is kept unchanged.

Verification

  • npm run verify:pr-local — green on this branch, rebuilt on main at 164859d6a:
 Test Files  942 passed (942)
      Tests  11887 passed | 1 skipped (11888)
✓ Compiled successfully in 57s
- completed: check:runtime, check:installed-lock-parity, format:changed, lint, typecheck, test,
  check:repo-awareness-snapshot, build, check:rag:fixtures, check:medication-interactions,
  check:medication-lexicon-report
- failed: (none)
- not reached: (none)
  • npm run verify:ui — not applicable; no component, route, style or browser behaviour changes. The only rendered difference is that a number the strip could never fetch can now appear in place of "document count unavailable".
  • npm run check:production-readiness — provider-backed, and not run. This adds no migration, no environment variable, and no new access path; it changes which existing server-side credential performs a read the app already made.

Proven by mutation, not by a passing test

Each rule was broken on purpose, the specific test watched go red, then the file restored and confirmed byte-identical by SHA-256. Five mutations, five reds, no survivors:

  1. The count loses its owner_id filter → "filters the count by the caller's own owner id" red.
  2. The administrator claim is no longer required → "counts nothing for a signed-in user without the administrator claim" red.
  3. The read reverts to the cookie-bound user client → "reports the owner's document count and email for a signed-in user" red. This is the regression that caused the bug, and it can no longer come back silently.
  4. A failed count reports 0 instead of null → "keeps an empty corpus distinct from a count it could not read" red.
  5. The rejection guard is removed → both degradation tests red.

Risk and rollout

  • Risk: Low, and it strictly reduces a silent failure. One read-only count in the developer hub's environment strip, behind the existing DeveloperAreaGate. The realistic failure would be a count that is not owner-scoped, which mutation 1 exists to prevent.
  • Rollback: Revert the commit. The strip returns to reporting "document count unavailable", which is what it does today.
  • Provider or production effects: None. No migration, no schema change, no environment variable, no new provider call. At runtime it is the same single head: true count the hub already issued, now through the service-role client with an explicit owner filter.
  • RAG impact: none.

Notes


🤖 Generated with Claude Code

https://claude.ai/code/session_01XG7wQurapeZwWRsNhHA1PY


Generated by Claude Code


Note

Medium Risk
Uses the service-role client for a read; scoping now depends entirely on the explicit owner_id filter and administrator gate rather than RLS, so regressions could leak or mis-scope counts if those checks are removed.

Overview
Fixes the developer hub environment strip so the document count can load instead of always showing unavailable. The count no longer goes through the cookie-bound Supabase user client (which lacked SELECT on public.documents and always failed with permission denied, silently degraded to null).

resolveHubEnvironmentFacts now mirrors the corpus-health pattern (#2504): session client only resolves the user; administrators get a service-role head count on documents with an explicit owner_id filter. Signed-in non-admins still receive email but skip the count query. Failed reads still return null, not 0.

Tests split user vs admin client mocks, require administrator app_metadata for count cases, assert owner_id on the query, and add coverage that non-admins issue no count — replacing the old (incorrect) assertion that forbade the admin client.

Reviewed by Cursor Bugbot for commit b67ad3f. Configure here.

…a credential that has privileges

The hub's document count could never load. `environment-facts.ts` counted
`public.documents` through the cookie-bound user client, on the reasoning that
the `documents owner read` policy would scope it in the database. It cannot:
`schema.sql:5299` revokes all `public` table privileges from `anon` and
`authenticated` and grants that table to `service_role` only, migration
`20260725000000` re-applies the revoke after every earlier grant, and no later
migration restores it -- the only `to authenticated` hits after that date are
RLS policies. A policy cannot hand back an SQL SELECT privilege the role does
not hold, so the read returned permission denied on every hub load and the strip
could only ever render "document count unavailable".

It was invisible because the module degrades a failed read to `null` by design
and its tests mock the client, so nothing on either side could see it. It
surfaced only because the corpus-health panel copied this module as its model
and hit the same wall, where Codex caught it on #2504.

Same shape as the corpus-health fix that already landed on this branch: the
cookie-bound client identifies the caller and reads no table, the caller must
carry the administrator claim `DeveloperAreaGate` checks, and the count filters
on `owner_id` explicitly. That filter is the whole owner-scoping guarantee now,
not an addition to row-level security. A non-administrator still gets their
email back rather than a blank strip, since it was already read and does not
depend on the count.

The source assertion named "reads through the user-session client and never the
service-role client" was backwards and is replaced rather than dropped: the
tests now assert the issued query carries `owner_id` equal to the caller, and
that a non-administrator issues no query at all. Every existing test that pins
null-not-zero and the rejection guard is kept unchanged.

Proven by mutation: five rules broken on purpose, five reds, no survivors, file
restored byte-identical by SHA-256. Reverting the read to the user client is one
of them, so the regression that caused this cannot come back silently.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XG7wQurapeZwWRsNhHA1PY
@supabase

supabase Bot commented Sep 1, 2026

Copy link
Copy Markdown

This pull request has been ignored for the connected project sjrfecxgysukkwxsowpy because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


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

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 1, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-01T15:39:03.469178Z 181f381 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@cursor

cursor Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_33018e8e-aa4d-48a6-bbb2-0fb9c970ffa4)

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

CI triage

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

  • Lighthouse budgetnot baselined: this job did NOT run on the main comparison below (path-scoped skip), so that run says nothing about it either way. Treat the comparison as absent, not green, and inspect the failing step.
  • PR requiredneeds investigation: inspect the failing step and uploaded diagnostics; rerun only after classifying the cause.

Compared with main CI run #14748 (success). That run's conclusion is an aggregate and did not exercise Lighthouse budget.

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

BigSimmo commented Sep 1, 2026

Copy link
Copy Markdown
Owner Author

Lighthouse budget is red, and it is not this PR's

What failed. Lighthouse budget on c4696894, taking PR required down with it (run 33527851426). Four render-timing metrics, each marginally over a percentage tolerance:

desktop-root            lcpMs +179 (+22.8%)  tolerance +20% and +100
mobile-documents-search lcpMs +526 (+23.0%)  tolerance +20% and +100
mobile-root             lcpMs +560 (+24.6%)  tolerance +20% and +100
mobile-root             tbtMs +178 (+40.8%)  tolerance +30% and +50

Why it is not this PR's, on two independent grounds.

  1. The diff cannot reach the budgeted routes. This PR changes one module, src/lib/developer-area/environment-facts.ts, plus its test. That module has exactly one importer in the whole tree — src/app/mockups/development/page.tsx — and it is server-only. Neither / nor /documents/search imports it, directly or transitively, so nothing here can move their LCP or TBT.

  2. The same content graded green on main eleven minutes earlier. c4696894 is a merge of main into this branch; the only thing it adds to main is the module above. The UI change that plausibly moves these routes — ea7c5c3d7, "Center mode homes on tall desktop screens" (Center mode homes on tall desktop screens #2511), which touches globals.css and ClinicalDashboard.tsx — is already on main, and Lighthouse budget passed on it: job 99920997048, conclusion success, completed 15:45:57Z. This branch failed at 15:56:34Z against the same committed baseline.

Same content, same baseline, opposite verdict, on four metrics that are all wall-clock measurements on a shared runner and all within a few points of their tolerance. That is run-to-run variance on the runner, not a regression this diff introduced.

No fix ported, because there is nothing to port. No PR exists that fixes this, and there is nothing in this diff to root-cause: fixing it would mean either widening this PR into the mode-home layout it does not touch, or moving a performance tolerance to make a red run green, which is exactly the kind of gate-weakening this repository forbids. If these four metrics prove to be genuinely drifting rather than noisy, that is a baseline-refresh decision on main (#QSHHGK in the ledger already tracks the absence of a scheduled refresh), not something this PR should absorb.

Re-running the failed job once, per the one-re-run rule. If it comes back red on a second run I will treat it as real and re-open this analysis rather than repeat this comment.

Local evidence for the change itself, unchanged: npm run verify:pr-local green on this branch — 942 test files, 11,887 tests passed, 1 skipped, build compiled in 57s, all steps completed, none failed.


Generated by Claude Code

@coderabbitai

coderabbitai Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Team

Run ID: 05f05513-7f46-4a3a-a8f4-bcb9868b49ed


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.

❤️ Share

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

@cursor

cursor Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_67d29c01-06b1-42ce-a1c5-4ccf64c955a5)

@BigSimmo
BigSimmo merged commit 5fef4bf into main Sep 1, 2026
31 checks passed
@BigSimmo
BigSimmo deleted the claude/corpus-health-panel-na3hbs branch September 1, 2026 16:55
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