Skip to content

Resolved table and CTE authorization - #3941

Merged
Hydrocharged merged 2 commits into
mainfrom
daylon/cte-privileges
Sep 30, 2026
Merged

Hydrocharged merged 2 commits into
mainfrom
daylon/cte-privileges

Conversation

@Hydrocharged

@Hydrocharged Hydrocharged commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Fixes a bug where subqueries could bypass permissions in some instances (test cases are now returning errors), and also adds an interface for:

@itoqa

itoqa Bot commented Sep 29, 2026

Copy link
Copy Markdown

Ito QA test results
Commit: 5c1d3c1: 16 test cases ran, 16 passed ✅.

Summary

Coverage spans normal authorized reads and writes, including inserts, updates, deletes, truncation, and common table-expression queries. It also exercises edge and adversarial cases such as missing tables, denied permissions, ambiguous names, independent source/destination access, and ensuring rejected operations leave data unchanged.

Safe to merge — the run found no regressions or PR-attributable failures across the authorization, data-change, and safety behaviors exercised. No merge blocker is indicated; overall risk is low.

Tests run by Ito

View full run

Result Severity Type Description
✅ — Authorize The authorization suite passed and confirmed that a resolved physical table can be delivered to the authorization handler before the SELECT plan runs.
✅ — Authorize The in-process authorization suite passed. The truncate path resolves mydb.test, sends the resolved table node to the authorization handler, and only then builds the truncate operation.
✅ — Authorize A missing table should return a table-not-found error without calling the resolved authorization callback. Source inspection confirms the lookup error is raised before the callback can run; the recorded execution could not perform the in-process callback check because the target exposed only the MySQL wire protocol.
✅ — General CTE-based inserts succeed when the user can read the source and write to the destination. Removing either permission is rejected without changing the destination.
✅ — General When authorization rejected a table, the SELECT, INSERT, and TRUNCATE requests all returned the expected error. The existing rows stayed unchanged and no rejected request reported success.
✅ — General The permission checks worked as expected: a user without SELECT could not read the restricted table, and the same user could read the table after SELECT was granted.
✅ — Insert The authorized insert completed successfully, and the destination table received the expected row (0,0). The browser check was not applicable because port 3306 is a MySQL service, not an HTTP page.
✅ — Insert An account with read access to the source table cannot insert into the destination table without insert access, and the destination remains unchanged.
✅ — Insert The authorized update changed row 0 to v1 = 9, and the authorized delete removed row 1. Both operations kept the source and target privilege checks working independently.
✅ — Insert An insert is rejected when a CTE uses the same name as the destination table and the user lacks INSERT permission on that destination. No row is added to the destination table.
✅ — Legacy The existing authorization path handled a normal physical-table query successfully. The local privilege suite passed; the browser check was not applicable because the target port speaks the database protocol instead of HTTP.
✅ — Rev The insert from the authorized CTE succeeded and the destination stored exactly the two selected rows. The available server did not provide the recording handler needed to check callback order and decisions.
✅ — Rev A user without permission to read the physical table received the normal privilege error, and no table rows were returned.
✅ — With The authorized CTE scenario is supported by the in-process SQL engine. The recorded environment exposed only the database protocol on port 3306, so the browser could not provide an execution surface for this engine-only check.
✅ — With A query that reads mydb.test2 through a CTE was rejected because the test user had no SELECT permission on that table.
✅ — With The query returned the row from the authorized source even though the CTE name matched another table. The user did not need permission on the physical table with that name.

Tip

Reply with @itoqa to send us feedback on this test run.

@zachmu zachmu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

Comment thread sql/planbuilder/from.go Outdated
Comment thread sql/planbuilder/from.go Outdated
@itoqa

itoqa Bot commented Sep 30, 2026

Copy link
Copy Markdown

Ito QA test results

History reset (rebase or force-push detected). Starting test narrative over.

Commit: e2b5565: 10 test cases ran, 10 passed ✅.

Summary

Coverage spans normal reads and writes, including common-table-expression queries, inserts, upserts, and table truncation, along with edge cases such as missing targets and cross-database object resolution. It also exercises adversarial permission scenarios, confirming unauthorized access is blocked and authorization can be disabled without affecting planning.

Safe to merge — the exercised application behaviors passed without any PR-attributable regressions or new failures, including permission enforcement and write-operation checks. No merge-blocking issue was identified.

Tests run by Ito

View full run

Result Severity Type Description
✅ — General The authorized table was emptied, and a missing table returned a planning error without changing data. The callback-order check could not run because the local server did not expose the required authorization setup.
✅ — General The legacy permission checks passed for direct tables, aliases, common table expressions, and insert-select queries. Unauthorized reads and writes were rejected before execution, while authorized queries and upserts passed.
✅ — General Permission checks follow the table or view that each name resolves to, even when names are qualified, aliased, or cross-database. The cross-database privilege checks and the full Go test suite passed.
✅ — General The authorization-disabled path can continue planning without running either callback family. The browser could not exercise the SQL server because port 3306 speaks the MySQL protocol, but the targeted plan-builder tests passed inside the application container.
✅ — Handler The resolved authorization handler checks the destination with INSERT and then UPDATE for an upsert. The privilege tests passed, including rejection without UPDATE access and success with both permissions.
✅ — Insert The insert completed successfully, and both source rows were stored in the destination table.
✅ — Legacy The authorized SELECT returned the two expected rows from mydb.test.
✅ — Rev The exact database query could not run because the local port served MySQL traffic instead of a browser page. Source inspection and focused Go tests show that disabled authorization skips both callback paths, so the blocked run does not indicate a product failure.
✅ — Select The authorized query succeeded and returned rows (0,0) and (1,1) in ascending order.
✅ — Select A query that placed an unauthorized table inside a common table expression failed during planning with the expected privilege error, before any rows were returned.

Tip

Reply with @itoqa to send us feedback on this test run.

@Hydrocharged
Hydrocharged merged commit 71ba144 into main Sep 30, 2026
12 checks passed
@Hydrocharged
Hydrocharged deleted the daylon/cte-privileges branch September 30, 2026 08: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