Repository navigation
fix(cli): lint config-exposed schemas in local db advisors (CLI-2824) - #7379
mdonnalley wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
🤖 AI Review
Confirmed both Claude findings: a minor remote exposure correctness concern and a documentation nit about linked config validation. Codex reported no findings. No duplicates or additional findings were identified. Verification was static; tests were not run.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| 🟡 MINOR | apps/cli/src/command-internal/db-config.layer.ts:708 |
correctness |
claude | Remote --db-url advisor runs overwrite the database's exposure setting with local config, which can misclassify exposed schemas or suppress exposure-dependent security findings when the local Data API is disabled. The output does not identify this source. |
| ⚪ NIT | apps/cli/src/command-internal/db-config.toml-read.ts:2193 |
behavior-change |
claude | Malformed SUPABASE_API_ENABLED also fails --linked commands unless a matching remote block overrides api.enabled, but the query documentation describes this failure only for --local and --db-url. |
Stats
Claude findings: 2 · Codex findings: 0 · Confirmed: 2 · Refuted: 0 · Uncertain: 0
Models: claude-opus-5-5 + gpt-6.1-sol · Trigger: auto · Workflow run
This review runs once per PR. A maintainer can request another with a /ai-review comment.
| conn: | ||
| isLocal && conn.password.length === 0 ? { ...conn, password: localPassword } : conn, | ||
| isLocal, | ||
| exposedSchemas: tomlValues.exposedSchemas, |
There was a problem hiding this comment.
🟡 MINOR · correctness · source: claude
Remote --db-url advisor runs overwrite the database's exposure setting with local config, which can misclassify exposed schemas or suppress exposure-dependent security findings when the local Data API is disabled. The output does not identify this source.
Evidence: db-config.layer.ts:658-660 reads base local config, and :704-708 attaches its exposedSchemas regardless of isLocal. advisors.handler.ts:70 writes those schemas to pgrst.db_schemas, and :113-115 applies them unconditionally when defined. The embedded rls_disabled_in_public query in advisors.lints-sql.ts:11 filters by that setting.
Suggested fix: Apply local exposure automatically only to local targets. For remote URLs, preserve database-derived exposure or require an explicit exposure override and report its source.
There was a problem hiding this comment.
Keeping local [api] exposure for --db-url, including remote URLs. This is the case the ticket targets: --db-url through Supavisor after db push, where a backend left with pgrst.db_schemas = '' made the CLI report "No issues found". Limiting exposure to local targets would leave that case broken, and a raw URL does not identify a project whose Data API settings the CLI could read. SIDE_EFFECTS.md now documents that --db-url uses local [api] (or SUPABASE_API_SCHEMAS / SUPABASE_API_ENABLED) and points to --linked for the platform-derived exposure.
| const apiEnabled = yield* resolveBoolOrFail( | ||
| "api.enabled", | ||
| apiRaw?.["enabled"], | ||
| true, | ||
| lookup, | ||
| remoteWins("api.enabled") ? undefined : envOverride("SUPABASE_API_ENABLED"), | ||
| ); |
There was a problem hiding this comment.
⚪ NIT · behavior-change · source: claude
Malformed SUPABASE_API_ENABLED also fails --linked commands unless a matching remote block overrides api.enabled, but the query documentation describes this failure only for --local and --db-url.
Evidence: db-config.toml-read.ts:2193-2199 always resolves api.enabled; :889-892 rejects malformed overrides. db-config.layer.ts:734-736 invokes this reader on the linked branch. query.handler.ts:269-275 and advisors.handler.ts:152-158 invoke that resolver for linked commands. commands/db/query/SIDE_EFFECTS.md:38 limits the documented failure scope to --local / --db-url.
Suggested fix: Document that malformed SUPABASE_API_ENABLED can fail linked commands too, including the matching remote-override exception.
There was a problem hiding this comment.
Fixed: both SIDE_EFFECTS.md files now say a malformed SUPABASE_API_ENABLED fails config load on every target unless a matching [remotes.*] block sets api.enabled.
TL;DR
supabase db advisors --local/--db-urlnow lints the schemas that[api]inconfig.tomlexposes, instead of whateverpgrst.db_schemasvalue the connection happens to carry.What's fixed?
Splinter decides which schemas to lint by reading
pgrst.db_schemas. The CLI never set it, so:public, and tables without RLS in other exposed schemas were never reported.''from then on. A pooled connection could then makedb advisorsreport "No issues found" even with unprotectedpublictables.How?
readDbTomlresolvesexposedSchemasfrom[api].schemasand[api].enabled(empty when the Data API is off), with theSUPABASE_API_SCHEMAS/SUPABASE_API_ENABLEDoverrides and[remotes.*]precedence thatsupabase startuses.DbConfigResolverpasses it on the--localand--db-urlpaths.db advisorsrunsselect set_config('pgrst.db_schemas', $1, true)inside its existing rollback transaction, before the lint setup. A Data API that is off sets'', so nothing is reported as exposed.db queryagent-mode RLS advisory binds the same schema list as$1::text[]instead of reading the setting, and is skipped when the Data API is off.--linkedis unchanged; the platform sets the value for hosted projects.Behavior change: a malformed
SUPABASE_API_ENABLEDnow fails config loading for commands that resolve the local database config, matching howSUPABASE_AUTH_ENABLEDandsupabase startalready treat it.ref: