fix: make preview and pin agree on which SQL the curation gate accepts - #478
Closed
mulkakhileshmj wants to merge 1 commit into
Closed
mulkakhileshmj wants to merge 1 commit into
mulkakhileshmj wants to merge 1 commit into
Conversation
POST /curation/preview and POST /curation/pin advertise the same rule, exactly one read only SELECT, but did not accept the same SQL. DuckDB types introspection statements such as PRAGMA database_list as SELECT, so they passed the shared gate. Pin ran them fine, while preview interpolated them into its DESCRIBE wrapper, which failed to parse and returned a 400 blaming the caller for a syntax error in SQL they never wrote, leaking the internal rewrite. The shared gate now also requires the statement text to read as a query: the leading keyword, past comments and parentheses, must be SELECT, WITH, FROM or VALUES. PRAGMA, DESCRIBE, SUMMARIZE and SHOW are refused on both routes with the existing single SELECT sentence. The deliberate loss is DESCRIBE and SUMMARIZE, which previously worked on both routes by accident of statement typing. As a second line of defense, a parse failure of the DESCRIBE wrapper in preview now answers with the fixed refusal sentence, so no 400 body can ever contain SQL the caller did not send. Binder errors about the caller's own SQL still travel back with DuckDB's message. A parametrized test runs the full acceptance table against both endpoints and asserts they agree. Closes Hebbian-Robotics#450
|
👋 Hi @mulkakhileshmj — thanks for the contribution! To keep starter issues available You already have #476 open, so this one is being closed automatically. |
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
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.
Closes #450.
Problem
POST /curation/previewandPOST /curation/pinadvertise the same rule, exactly one read-only SELECT, and did not accept the same SQL. DuckDB types introspection statements such asPRAGMA database_listasStatementType.SELECT, so they passed the shared gate: pin ran them (200), while preview interpolated them into itsDESCRIBE SELECT * FROM (...)wrapper, which failed to parse and returned a 400 whose body was DuckDB's diagnostic for the rewritten query, blaming the caller for a)they never typed and leaking the internal rewrite.Direction taken
The issue's first option: narrow the shared gate so both routes refuse statements that are not actually SELECT text. After the existing single-statement and statement-type check, the statement's leading keyword, scanned past whitespace, line comments, nested block comments, and parentheses, must be
SELECT,WITH,FROM, orVALUES(FROM-first and VALUES are genuine queries and subquery-legal).PRAGMA,DESCRIBE,SUMMARIZE, andSHOWare refused on both routes with the existing pinned sentence, now a shared constant so the three routes cannot drift on wording.What a user loses:
DESCRIBEandSUMMARIZEpreviously worked on both routes (verified on DuckDB 1.5.5, which can wrap both as subqueries; onlyPRAGMAfails to wrap). They now get 400 on both. They worked by accident of DuckDB's statement typing, nothing documents them, and preview-then-pin is the advertised flow, so consistency wins.Defense in depth for the leak:
_described_columnsnow answers a parse failure of its own wrapper with the fixed refusal sentence, so no 400 body can contain SQL the caller did not send. Binder and catalog errors about the caller's own SQL still travel back with DuckDB's message, keeping the #448 pins intact.Coverage
New
packages/hflow-server/tests/test_server_curation_sql_gate.py: a 15-row parametrized table (the issue's cases plus WITH, FROM-first, VALUES, parenthesized, comment-prefixed, SHOW, a comment-disguised PRAGMA, and multi-statement) runs every row against both endpoints and asserts they agree, that refusals carry the fixed sentence, and that no body contains the wrapper text. A direct unit test pins that a wrapper parse failure is never blamed on the caller.During validation the root-suite test
test_successful_retry_after_error_appends_repaired_outcomefailed once on arecorded_atordering tie and passed 12 out of 12 repeat runs on both this branch and main, so it looks like an unrelated timing flake, noted here for transparency.Validation
Run on WSL2 Ubuntu, Python 3.12:
Mutation check: with
_curation.pyreverted to main, 8 of the 16 new gate tests fail (the refusal rows); restored, all 16 pass. The server test suite also passes under Python 3.11 (251 passed).