fix: ensure correct escaping in advisor - #407
Conversation
Coverage Report for CI Build 33747935731Coverage decreased (-0.01%) to 96.454%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
raulb
left a comment
raulb
left a comment
There was a problem hiding this comment.
Thank you also for updating the mgmt-api types 🙇♂️
| * schema and table portions independently. | ||
| */ | ||
| function quoteQualifiedName(name: string): string { | ||
| const dotIndex = name.indexOf('.'); |
There was a problem hiding this comment.
Splitting on the first dot loses the schema boundary, and the filter below has the same ambiguity. For { schema: "my.app", name: "users" }, this generates "my"."app.users"; a schema such as auth.legacy is also treated as auth and hidden. Could this use the original schema and name fields for both filtering and SQL?
barryroodt
left a comment
barryroodt
left a comment
There was a problem hiding this comment.
Nice work! The identifier escaping fix looks correct. One minor issue remains: joining schema and name, then splitting on the first dot, breaks remediation SQL for schemas such as my.app. Could we pass { schema, name, rls_enabled } into buildRlsDisabledAdvisory, use the separate fields for filtering and SQL, and keep ${schema}.${name} only for display?
Good catch! Makes sense, I'll see trace back the call path and do as suggested. It will cleanup some other schema checks we have in that same code block that also splits on the first |
Primitives are already built into the code, use them: #409 |
## What kind of change does this PR introduce? chore / fix ## What is the current behavior? Schemas are expected to never contain a `.` (dot) ## What is the new behavior? Schemas may contain a `.` (dot) and will be treated the same as a schema without. Extends #407
🤖 I have created a release *beep* *boop* --- <details><summary>mcp-utils: 0.8.0</summary> ## [0.8.0](mcp-utils-v0.7.0...mcp-utils-v0.8.0) (2026-09-04) ### Features * **mcp:** add project cost confirmation elicitation ([#391](#391)) ([fb50882](fb50882)) * **mcp:** hide legacy cost tools from form-capable clients ([#411](#411)) ([2f04461](2f04461)) </details> <details><summary>mcp-server-supabase: 0.12.0</summary> ## [0.12.0](mcp-server-supabase-v0.11.0...mcp-server-supabase-v0.12.0) (2026-09-04) ### Features * group lints in `get_advisors` response ([#390](#390)) ([48d593a](48d593a)) * **mcp:** add branch cost confirmation elicitation ([#394](#394)) ([867a160](867a160)) * **mcp:** add project cost confirmation elicitation ([#391](#391)) ([fb50882](fb50882)) * **mcp:** hide legacy cost tools from form-capable clients ([#411](#411)) ([2f04461](2f04461)) ### Bug Fixes * ensure correct escaping in advisor ([#407](#407)) ([a6cf4a0](a6cf4a0)) * make server instructions intent-based instead of naming tools ([#372](#372)) ([fc54ea2](fc54ea2)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: supabase-releaser[bot] <223506987+supabase-releaser[bot]@users.noreply.github.com>
4 participants
What kind of change does this PR introduce?
Bug fix
What is the current behavior?
Table names were not escaped in rls advisor.
What is the new behavior?
Escapes table names to ensure valid SQL in the suggested ALTER TABLE statement.