🔒 Fix potential SQL Injection in execute_sql - #11337
Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Bugbot couldn't run - usage limit reachedBugbot 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_fc67504b-25cd-4da1-b177-ea7258d5cc9a) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ec7d15da3
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if action in (sqlite3.SQLITE_SELECT, sqlite3.SQLITE_READ, sqlite3.SQLITE_FUNCTION): | ||
| return sqlite3.SQLITE_OK | ||
| return sqlite3.SQLITE_DENY |
There was a problem hiding this comment.
When execute_sql queries any FTS5 table, FTS5 internally requests SQLITE_PRAGMA for data_version, which this allowlist denies; I reproduced this with SQLite 3.45.1, where a SELECT ... FROM screenshots_fts ... MATCH ... returns authorization denied. This affects the uploaded production database because desktop/macos/Desktop/Sources/Rewind/Core/RewindDatabase.swift:1329-1336 creates screenshots_fts using FTS5, so permit the narrowly scoped read-only data_version action while continuing to deny other pragmas.
Useful? React with 👍 / 👎.
| def authorizer(action: int, arg1: str | None, arg2: str | None, dbname: str | None, source: str | None) -> int: | ||
| if action in (sqlite3.SQLITE_SELECT, sqlite3.SQLITE_READ, sqlite3.SQLITE_FUNCTION): | ||
| return sqlite3.SQLITE_OK | ||
| return sqlite3.SQLITE_DENY |
There was a problem hiding this comment.
This security fix changes only production code, while the existing test at backend/tests/unit/test_agent_vm_protocol.py:676-687 still covers only a successful ordinary SELECT; it would pass if the authorizer were removed and did not expose the FTS regression above. Add behavioral tests that exercise the newly denied operation and a representative production read-only path, as the repository explicitly requires regression coverage for bug fixes.
AGENTS.md reference: AGENTS.md:L28-L28
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Confidence score: 4/5
- In
backend/agent_vm/main.py, the SQL-injection hardening viasqlite3.set_authorizeris unverified by a regression test, so future refactors could silently weaken the protection and reintroduce a security regression—add a focused regression test that reproduces the original injection path and proves it is blocked.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/agent_vm/main.py">
<violation number="1" location="backend/agent_vm/main.py:498">
P2: This is a security fix (defusing SQL injection via sqlite3.set_authorizer) but it lands without a regression test, which the repo's Definition of Done explicitly requires ('Bug fixes include the regression test that would have caught the bug'). The agent-vm DB already has a test harness in backend/tests/unit/test_agent_vm_protocol.py with an execute_sql happy-path test (test_execute_sql_serializes_sqlite_rows), so a focused test is cheap to add. Consider adding tests that (1) assert destructive statements — DROP/INSERT/UPDATE via a WITH/subquery attempt and PRAGMA — are rejected with an error rather than mutating the DB, and (2) assert the authorizer is detached in the finally block so a subsequent run_sync/upload write on the same connection still succeeds. Otherwise the security boundary has no guard against regression.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if not re.search(r"\bLIMIT\b", query, re.I): | ||
| query = query.rstrip().rstrip(";") + " LIMIT 200" | ||
| try: | ||
| def authorizer(action: int, arg1: str | None, arg2: str | None, dbname: str | None, source: str | None) -> int: |
There was a problem hiding this comment.
P2: This is a security fix (defusing SQL injection via sqlite3.set_authorizer) but it lands without a regression test, which the repo's Definition of Done explicitly requires ('Bug fixes include the regression test that would have caught the bug'). The agent-vm DB already has a test harness in backend/tests/unit/test_agent_vm_protocol.py with an execute_sql happy-path test (test_execute_sql_serializes_sqlite_rows), so a focused test is cheap to add. Consider adding tests that (1) assert destructive statements — DROP/INSERT/UPDATE via a WITH/subquery attempt and PRAGMA — are rejected with an error rather than mutating the DB, and (2) assert the authorizer is detached in the finally block so a subsequent run_sync/upload write on the same connection still succeeds. Otherwise the security boundary has no guard against regression.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/agent_vm/main.py, line 498:
<comment>This is a security fix (defusing SQL injection via sqlite3.set_authorizer) but it lands without a regression test, which the repo's Definition of Done explicitly requires ('Bug fixes include the regression test that would have caught the bug'). The agent-vm DB already has a test harness in backend/tests/unit/test_agent_vm_protocol.py with an execute_sql happy-path test (test_execute_sql_serializes_sqlite_rows), so a focused test is cheap to add. Consider adding tests that (1) assert destructive statements — DROP/INSERT/UPDATE via a WITH/subquery attempt and PRAGMA — are rejected with an error rather than mutating the DB, and (2) assert the authorizer is detached in the finally block so a subsequent run_sync/upload write on the same connection still succeeds. Otherwise the security boundary has no guard against regression.</comment>
<file context>
@@ -495,9 +495,18 @@ def execute_sql(query: str) -> str:
if not re.search(r"\bLIMIT\b", query, re.I):
query = query.rstrip().rstrip(";") + " LIMIT 200"
try:
+ def authorizer(action: int, arg1: str | None, arg2: str | None, dbname: str | None, source: str | None) -> int:
+ if action in (sqlite3.SQLITE_SELECT, sqlite3.SQLITE_READ, sqlite3.SQLITE_FUNCTION):
+ return sqlite3.SQLITE_OK
</file context>
undivisible
added
human
undivisible
left a comment
undivisible
left a comment
There was a problem hiding this comment.
The SQL authorizer blocks all SQLITE_PRAGMA actions, which can reject legitimate FTS5 reads that request data_version. Please allow only the required read-only pragma (or equivalent safe handling), and add tests covering FTS5 MATCH reads and destructive/injection attempts.
Bugbot couldn't run - usage limit reachedBugbot 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_18f3fc1e-4583-4db0-ba7c-b45c10abab8d) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27b4a4fe87
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "DELETE FROM screenshots", | ||
| "UPDATE screenshots SET id = 'changed'", | ||
| "SELECT 1; DROP TABLE screenshots", |
There was a problem hiding this comment.
The fresh parameterized cases added here are all rejected before set_authorizer is installed: DELETE and UPDATE fail the existing SELECT-only check, while the multi-statement query fails the existing semicolon check. Consequently, removing read_only_sql_authorizer entirely would leave every new denial test passing, so the security boundary remains unguarded; add a behavioral case that reaches the authorizer and requires it to return SQLITE_DENY.
AGENTS.md reference: AGENTS.md:L28-L28
Useful? React with 👍 / 👎.
| if action in (sqlite3.SQLITE_SELECT, sqlite3.SQLITE_READ, sqlite3.SQLITE_FUNCTION): | ||
| return sqlite3.SQLITE_OK |
There was a problem hiding this comment.
When a valid query uses a recursive CTE beneath an outer SELECT, such as SELECT (WITH RECURSIVE t(x) AS (...) SELECT max(x) FROM t), SQLite emits the SQLITE_RECURSIVE authorizer action. This allowlist denies that action and returns not authorized, even though the same read-only query passed the existing checks and executed before this commit; permit SQLITE_RECURSIVE while continuing to authorize each underlying read or write action separately.
Useful? React with 👍 / 👎.
|
Thanks for this security improvement, @undivisible. Good use of Verified locally:
Two items to address before merge:
No security concerns with the implementation itself — the authorizer is correctly scoped and the PRAGMA by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with |
Git-on-my-level
added
the
positive-signal
Bugbot couldn't run - usage limit reachedBugbot 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_632a73e9-0790-4f95-a346-b30b6d01a5a4) |
Git-on-my-level
left a comment
Git-on-my-level
left a comment
There was a problem hiding this comment.
Thanks for the security hardening direction, @undivisible — using sqlite3.set_authorizer as a deny-by-default allowlist alongside the existing string denylist is the right defense-in-depth pattern for execute_sql.
However, the current head (4d1f2e94ae) regresses the improvements landed in the prior commit on this branch (27b4a4fe87), and a few things need to be addressed before this can merge.
1. FTS5 read regression — backend/agent_vm/main.py (lines 498-509)
The head reverts the module-level read_only_sql_authorizer back to a narrower inline authorizer that denies every SQLITE_PRAGMA action. Commit 27b4a4fe87 had specifically added a safe exception for PRAGMA data_version because SQLite requests it during FTS5 MATCH reads. I reproduced the failure locally: with the head's authorizer, SELECT ... FROM documents WHERE documents MATCH 'hello' on an fts5 virtual table returns authorization denied. The authorizer in the head only allows SQLITE_SELECT, SQLITE_READ, and SQLITE_FUNCTION:
def authorizer(action, arg1, arg2, dbname, source):
if action in (sqlite3.SQLITE_SELECT, sqlite3.SQLITE_READ, sqlite3.SQLITE_FUNCTION):
return sqlite3.SQLITE_OK
return sqlite3.SQLITE_DENYPlease restore the module-level read_only_sql_authorizer with the PRAGMA data_version exception (or equivalent safe handling) from commit 27b4a4fe87.
2. Deleted regression tests — backend/tests/unit/test_agent_vm_protocol.py
The head deletes all 55 lines of tests that 27b4a4fe87 added: test_execute_sql_allows_fts5_reads, test_execute_sql_clears_authorizer_after_error, and the test_execute_sql_denies_destructive_queries parametrize (DELETE / UPDATE / stacked-statement). The net diff vs main shows zero test changes. Per the repo Definition of Done, a security bug fix must include the regression test that would have caught it. Please restore these tests.
3. Junk files at repo root — commit.txt, draft.md
Both files contain identical scratch content (security(agent-vm): restrict sql execution with sqlite authorizer / Failure-Class: none) and should not be committed to the repository. Please remove them.
4. CI — Hygiene, Formatting, PR Metadata Preflight failing
The first commit (4ec7d15da3) lacks the required Failure-Class: FC-<slug> | new | none trailer that the preflight validator checks across all fix: commits in the range. Run scripts/pr-preflight --suggest locally and ensure every fix: commit carries a valid declaration. The Hygiene/Formatting failures are likely related to the junk files above.
The core security approach is sound; the authorizer is correctly scoped under runtime.lock and the finally: set_authorizer(None) prevents leakage into run_sync/internal writes. Once the regression and tests are restored and the junk files removed, this should be in good shape.
Leaving for human maintainer review given the security-sensitive SQL-authorizer surface — the current head reintroduces the FTS5 read failure that the prior commit on this branch had fixed.
Review by glm-5.2 (Omi PR maintainer automation). No urgent security alert — the authorizer direction is correct, the issues are about an incomplete/inadvertent revert on this branch.
by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.
Git-on-my-level
removed
the
positive-signal
4d1f2e9 to
a003897
Compare
Bugbot couldn't run - usage limit reachedBugbot 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_4ea887e5-5944-45b7-8857-63936b964f1e) |
This commit addresses a potential SQL Injection vulnerability in the `backend/agent_vm/main.py:execute_sql` tool. The `execute_sql` function previously attempted to enforce read-only operations using basic string pattern matching and `LIMIT` checks before executing arbitrary user-provided SQL. This is an anti-pattern as it does not prevent all vectors of database modification or unauthorized schema access. We have applied SQLite's native `sqlite3.set_authorizer` mechanism around the query execution. The authorizer strictly limits permissions, rejecting operations like `UPDATE`, `DROP`, `DELETE`, `ATTACH`, `PRAGMA` etc, and returning `SQLITE_DENY`. Only `SQLITE_SELECT`, `SQLITE_READ`, and `SQLITE_FUNCTION` are allowed. The authorizer is applied securely within a `with runtime.lock:` block and safely unset via a `finally` block to prevent permissions bleeding into other concurrent operations on the connection. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
Failure-Class: none
Failure-Class: none Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
a003897 to
77a90c6
Compare
Bugbot couldn't run - usage limit reachedBugbot 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_34ff13cc-d33e-4f0b-8283-3a76494aa9dd) |
Git-on-my-level
left a comment
Git-on-my-level
left a comment
There was a problem hiding this comment.
@undivisible — following up on the prior automation review (commit 4d1f2e94ae). The new head (77a90c60fa) addresses some points but reintroduces the same core regression, so I still need to request changes. The authorizer direction remains sound; the gap is an incomplete revert.
1. FTS5 read regression still present — backend/agent_vm/main.py (lines 498-509)
The head defines the authorizer inline inside execute_sql, allowing only SQLITE_SELECT, SQLITE_READ, and SQLITE_FUNCTION and denying everything else:
def authorizer(action, arg1, arg2, dbname, source):
if action in (sqlite3.SQLITE_SELECT, sqlite3.SQLITE_READ, sqlite3.SQLITE_FUNCTION):
return sqlite3.SQLITE_OK
return sqlite3.SQLITE_DENYThis is the narrow authorizer that commit 1258274f2d ("preserve FTS5 reads under SQL authorizer") had replaced with a module-level read_only_sql_authorizer that adds a safe exception for PRAGMA data_version. SQLite requests data_version while servicing FTS5 MATCH reads, so denying all SQLITE_PRAGMA makes SELECT ... MATCH '...' against the documents fts5 virtual table return authorization denied. Please restore the module-level read_only_sql_authorizer (allowing SELECT/READ/FUNCTION plus PRAGMA data_version read-only) from 1258274f2d, or apply equivalent safe handling.
2. Regression tests deleted — backend/tests/unit/test_agent_vm_protocol.py
The head removes all 55 lines of tests that 1258274f2d added — test_execute_sql_allows_fts5_reads, test_execute_sql_clears_authorizer_after_error, and the test_execute_sql_denies_destructive_queries parametrize (DELETE / UPDATE / stacked-statement). The net test diff vs main is now zero. Per the repo Definition of Done (AGENTS.md §1: "Bug fixes include the regression test that would have caught the bug"), a security fix like this needs the regression tests that cover both the destructive-query denial and the FTS5 read path. Please restore them.
3. Junk files committed at repo root — commit.txt, draft.md
Both contain identical scratch content (security(agent-vm): restrict sql execution with sqlite authorizer / Failure-Class: none) and don't belong in the tree. Please remove both files; this should also clear the Hygiene/Formatting CI failures.
4. CI — Hygiene and Formatting failing
Both are currently failing, almost certainly from the junk files above. Removing them and running make preflight locally should resolve this.
The structural hardening is good: the authorizer is correctly acquired under runtime.lock and the finally: set_authorizer(None) prevents it from leaking into run_sync/internal writes. Once the FTS5 exception and the regression tests are restored and the scratch files removed, this is in good shape.
Security-sensitive SQL-authorizer surface reintroduces the FTS5 read failure the middle commit on this branch had already fixed — requesting changes rather than leaving a comment, because the regression is concrete and reproducible.
Review by glm-5.2 (Omi PR maintainer automation). No urgent security alert — the authorizer approach is sound, the remaining issue is an incomplete revert on this branch.
Git-on-my-level
added
the
security-review
Restore the read-only authorizer's safe PRAGMA data_version exception for FTS5 MATCH reads, bring back destructive-query and cleanup regression coverage, and remove committed scratch files. Failure-Class: none Verification: backend/tests/unit/test_agent_vm_protocol.py (29 passed); make preflight (22 checks passed)
Bugbot couldn't run - usage limit reachedBugbot 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_564a8f32-dabc-4355-8994-426017424b23) |
Git-on-my-level
removed
the
needs-tests
Git-on-my-level
added
the
positive-signal
Resolved on current head 72848cd: (1) module-level read_only_sql_authorizer with PRAGMA data_version exception restored — FTS5 MATCH reads work; (2) all 55 lines of regression tests restored (FTS5 reads, authorizer cleanup, destructive-query denial); (3) junk files commit.txt/draft.md removed; (4) CI fully green. 29/29 tests pass.
|
Thanks @undivisible — the current head ( 1. FTS5 read regression — fixed. 2. Regression tests — restored. All 55 lines are back in 3. Junk files — removed. Neither 4. CI — green. Backend Hermetic Merge Gate, Hygiene, Formatting, and PR Metadata Preflight all pass. Technical notes on the authorizer design: the deny-by-default posture is correct — only Removing Review by glm-5.2 via the Omi PR maintainer monitor, acting on behalf of the maintainer team. by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with |
2 participants
🎯 What:
The vulnerability fixed is a potential SQL Injection in the
execute_sqlfunction located inbackend/agent_vm/main.py. The function was passing an arbitrary user-provided query directly to SQLite with only fragile string blocklists for defense.If left unfixed, the agent (or an attacker guiding the agent) could craft a malicious payload (e.g. using
WITHclauses or subqueries) to modify, truncate, or drop internal local state databases used byagent_vm. It could also bypass the query blocklist to execute destructive PRAGMA commands or attach other databases on the host system.🛡️ Solution:
The fix leverages
sqlite3.set_authorizer. An authorizer is explicitly attached to the SQLite connection strictly during theexecute()call. The authorizer permits only read-only actions (SQLITE_SELECT,SQLITE_READ,SQLITE_FUNCTION) at the database engine level, entirely neutralizing any potential destructive operations while preserving the agent's ability to run legitimate data retrieval SELECT queries. The authorizer is then detached in afinallyblock to not interfere with other internal writes (e.g.run_sync).PR created automatically by Jules for task 10420643532383619873 started by @undivisible
Note
Medium Risk
Touches security-sensitive agent SQL execution on the VM database; the scoped authorizer reduces injection risk but must stay cleared so sync/upload paths can still write.
Overview
Hardens the agent VM
execute_sqltool so user/agent SQL cannot mutate the shared SQLite connection beyond what the string blocklist was meant to stop.read_only_sql_authorizeris registered on the connection only for the duration of each query (set_authorizerin atry/finallythat clears it afterward). The engine allowsSQLITE_SELECT,SQLITE_READ,SQLITE_FUNCTION, andPRAGMA data_version; everything else is denied. That blocks destructive or sneaky operations even if they slip past the existing SELECT-only and keyword checks, whilerun_syncand other internal writes stay unaffected when the authorizer is cleared.Unit tests cover FTS5
MATCHreads, authorizer cleanup after a failed query, and rejection of DELETE, UPDATE, and multi-statement payloads without changing table data.Reviewed by Cursor Bugbot for commit 72848cd. Configure here.
Failure-Class: none