From c42f96c85c0d0c3ab7b231b66c4bec97a0e4f4ce Mon Sep 17 00:00:00 2001 From: Sandeep Date: Mon, 27 Jul 2026 15:36:15 -0700 Subject: [PATCH 1/2] security: close a read-only-guard bypass via a welded quoted identifier MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A double-quoted identifier is self-delimiting in SQL — the quote IS the token boundary — so `SELECT*FROM"pg_read_file"('/etc/passwd')` needs no whitespace and is a statement the engine runs. `_neutralize` dropped the quote characters without re-supplying that boundary, fusing two tokens into one: SELECT*FROM"pg_read_file"(...) -> SELECT*FROMpg_read_file(...) Every dangerous-function pattern is `\b`-anchored, and there is no word boundary inside `FROMpg_read_file`, so the gate stopped seeing the token at all rather than allowing it — check_read_only returned None (pass). Verified end-to-end against PostgreSQL 16: the welded form executes and returns the contents of a server-side file, while the guard passed it. After the fix the same string is refused with "function `pg_read_file` is not allowed". Note the exploitable shape is a weld onto a NON-leading keyword (`FROM"x"`, `WHERE"x"`). Welding onto the leading keyword (`SELECT"pg_sleep"(10)` -> `SELECTpg_sleep(10)`) was already caught incidentally, by the statement-opener check rather than the dangerous-function check. The fix re-supplies the separator only when the previous emitted character is a word char. A blanket space would split a qualified name (`t."current_user"` -> `t. current_user`) and defeat the `(? --- CHANGELOG.md | 17 ++++++++++ packages/agami-core/src/sql_guard.py | 14 ++++++++ plugins/agami/lib/sql_guard.py | 14 ++++++++ tests/test_sql_guard.py | 49 ++++++++++++++++++++++++++++ 4 files changed, 94 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7b335e19..908d4659 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,23 @@ is the source of truth a host installs against — bumping it is what invalidate user's plugin cache (see [CONTRIBUTING.md](CONTRIBUTING.md)). Each released section below corresponds to one such version. +## [Unreleased] + +### Security + +- **Closed a read-only-guard bypass via a welded quoted identifier.** A double-quoted + identifier is self-delimiting in SQL, so `SELECT*FROM"pg_read_file"('/etc/passwd')` is a + valid statement with no whitespace before the quote. The guard's lexer dropped the quote + characters without re-supplying that boundary, fusing two tokens into one + (`FROM"pg_class"` → `FROMpg_class`) and destroying the word-boundary anchor every + dangerous-function pattern matches on — so the gate stopped seeing the token rather than + allowing it, and returned no rejection. Verified against PostgreSQL 16: the welded form + executes and reads a server-side file while the guard passed it. The lexer now re-supplies + a separator when, and only when, the previous emitted character is a word character, so a + qualified name (`t."current_user"`) is still not split. Prior corpus cases all happened to + carry a space before the quote, which is why this stayed invisible; the regression corpus + now pins the welded forms and the qualified-name negatives. + ## [0.5.0] — 2026-07-25 ### Changed diff --git a/packages/agami-core/src/sql_guard.py b/packages/agami-core/src/sql_guard.py index 802cd0bb..dfb425cb 100644 --- a/packages/agami-core/src/sql_guard.py +++ b/packages/agami-core/src/sql_guard.py @@ -129,6 +129,20 @@ def _neutralize(sql: str) -> str: j += 1 # A pathological identifier like "a;b" reduces to a;b and trips the # multi-statement check — a deliberate, safe-direction hardening choice. + # + # The quote is ALSO the token delimiter: a delimited identifier is + # self-delimiting in SQL, so `FROM"pg_class"` needs no whitespace and is a + # valid statement the engine runs. Dropping the quotes without re-supplying a + # separator therefore fuses two tokens into one (`FROM"pg_class"` -> + # `FROMpg_class`), destroying the `\b` anchor every deny-list pattern below + # relies on — the gate stops seeing the token at all rather than allowing it. + # Re-supply the boundary ONLY when the previous emitted char is a word char: + # a blanket space would split a qualified name (`t."col"` -> `t. col`) and + # break the `(? str: j += 1 # A pathological identifier like "a;b" reduces to a;b and trips the # multi-statement check — a deliberate, safe-direction hardening choice. + # + # The quote is ALSO the token delimiter: a delimited identifier is + # self-delimiting in SQL, so `FROM"pg_class"` needs no whitespace and is a + # valid statement the engine runs. Dropping the quotes without re-supplying a + # separator therefore fuses two tokens into one (`FROM"pg_class"` -> + # `FROMpg_class`), destroying the `\b` anchor every deny-list pattern below + # relies on — the gate stops seeing the token at all rather than allowing it. + # Re-supply the boundary ONLY when the previous emitted char is a word char: + # a blanket space would split a qualified name (`t."col"` -> `t. col`) and + # break the `(? None: assert check_read_only(sql) is not None, f"Quoted-fn bypass NOT blocked: {sql!r}" +@pytest.mark.parametrize( + "sql", + [ + # WELDED quoted identifier — no whitespace between the preceding keyword and the + # opening `"`. A delimited identifier is self-delimiting in SQL (the quote IS the + # token boundary), so these are valid statements the engine happily runs; verified + # on PostgreSQL 16 (`SELECT"pg_read_file"('/tmp/x')` returns the file contents). + # Dropping the quotes without re-supplying a separator fuses two tokens into one + # (`FROM"pg_class"` -> `FROMpg_class`), which destroys the `\b` anchor every + # deny-list pattern relies on and silently blinds the gate. + "SELECT\"pg_read_file\"('/etc/passwd')", + "SELECT*FROM\"pg_read_file\"('/etc/passwd')", + 'SELECT"pg_sleep"(10)', + 'SELECT"dblink"(\'host=evil\', \'select 1\')', + 'SELECT"pg_terminate_backend"(123)', + 'SELECT"set_config"(\'statement_timeout\', \'0\', false)', + 'SELECT 1 FROM"pg_class"WHERE"pg_sleep"(10) IS NULL', + # The weld can also appear mid-statement, after a non-keyword word char. + 'SELECT a FROM t WHERE b="pg_sleep"(1)', + ], +) +def test_red_team_welded_quoted_dangerous_fn_bypass(sql: str) -> None: + """A quoted identifier welded to the preceding token must not escape the gate. + + Regression for the `_neutralize` weld: every pre-existing case in the corpus above + happens to carry a space before the `"`, so the missing separator was invisible. + """ + assert check_read_only(sql) is not None, f"Welded quoted-fn bypass NOT blocked: {sql!r}" + + +@pytest.mark.parametrize( + "sql", + [ + # The separator must be re-supplied ONLY where the quote was actually delimiting + # two word chars. A blanket space would break qualified names — `t."col"` must stay + # `t.col`, not become `t. col` — so these legitimate reads must still pass. + 'SELECT t."current_user" FROM t', + 'SELECT "order id" FROM orders', + 'SELECT "name", "email" FROM customers', + 'SELECT c."email" FROM customers c', + 'SELECT "schema"."table" FROM "schema"."table"', + 'SELECT a."b" FROM x a', + ], +) +def test_welded_fix_does_not_over_block_legitimate_quoted_identifiers(sql: str) -> None: + """The weld fix must not turn a qualified quoted column into a false positive.""" + assert check_read_only(sql) is None, f"Legitimate quoted identifier wrongly blocked: {sql!r}" + + @pytest.mark.parametrize( "sql", [ From 5ef42fbfcb520f0821ca699fc7cbcc7155e9d513 Mon Sep 17 00:00:00 2001 From: Sandeep Date: Mon, 27 Jul 2026 15:58:02 -0700 Subject: [PATCH 2/2] fix(review): re-supply the separator on BOTH ends of a quoted identifier MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review findings 1-5 on this branch. The first is the serious one: the previous commit fixed only the leading side and, in doing so, opened a shape that `main` had blocked. 1. Trailing weld (blocking). A delimited identifier is self-delimiting on BOTH ends, so the closing quote fuses too: `"x"INTO` -> `xINTO`. `INTO` is in the DML/DDL list precisely because `SELECT ... INTO ` is a write that opens with SELECT, and `\bINTO\b` cannot match `xINTO`. Verified on PostgreSQL 16 — `SELECT "x"INTO evil FROM t` creates a table holding the source rows. Net-new regression, now closed: `SELECT"x"INTO evil FROM t` was REFUSED on main (the leading weld produced `SELECTxINTO`, which failed the opening-keyword rule) and PASSED here — restoring the leading separator handed it a valid `SELECT` opener while the trailing weld still hid `INTO`. 2. Row locks reachable the same way: `FROM t AS"a"FOR SHARE`. `FOR UPDATE` survives only because bare `UPDATE` is independently in the DML list; `FOR SHARE` had no such backstop. 3. The rationale cited a `(? --- CHANGELOG.md | 28 +++++++++++------- packages/agami-core/src/sql_guard.py | 44 ++++++++++++++++++++-------- plugins/agami/lib/sql_guard.py | 44 ++++++++++++++++++++-------- tests/test_sql_guard.py | 43 ++++++++++++++++++++++++++- 4 files changed, 123 insertions(+), 36 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 908d4659..15b88dfd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,17 +15,23 @@ below corresponds to one such version. ### Security - **Closed a read-only-guard bypass via a welded quoted identifier.** A double-quoted - identifier is self-delimiting in SQL, so `SELECT*FROM"pg_read_file"('/etc/passwd')` is a - valid statement with no whitespace before the quote. The guard's lexer dropped the quote - characters without re-supplying that boundary, fusing two tokens into one - (`FROM"pg_class"` → `FROMpg_class`) and destroying the word-boundary anchor every - dangerous-function pattern matches on — so the gate stopped seeing the token rather than - allowing it, and returned no rejection. Verified against PostgreSQL 16: the welded form - executes and reads a server-side file while the guard passed it. The lexer now re-supplies - a separator when, and only when, the previous emitted character is a word character, so a - qualified name (`t."current_user"`) is still not split. Prior corpus cases all happened to - carry a space before the quote, which is why this stayed invisible; the regression corpus - now pins the welded forms and the qualified-name negatives. + identifier is self-delimiting in SQL on **both** ends, so `SELECT*FROM"pg_read_file"(…)` + and `SELECT "x"INTO evil FROM t` are valid statements with no whitespace either side of + the quote. The guard's lexer dropped the quote characters without re-supplying those + boundaries, fusing neighbouring tokens into one (`FROM"pg_class"` → `FROMpg_class`, + `"x"INTO` → `xINTO`) and destroying the word-boundary anchor every deny-list pattern + matches on — so the gate stopped *seeing* the token rather than allowing it, and returned + no rejection. Verified against PostgreSQL 16: the leading form reads a server-side file + and the trailing form creates a table from `SELECT … INTO`, both while the guard passed + them. Row locks (`FOR SHARE`) were reachable the same way. The lexer now re-supplies a + separator on either side, when and only when the quote was actually separating two word + characters — so a qualified name (`t."current_user"`) still neutralizes to one token, + `t.current_user`, rather than being split. This restores an invariant the lexer already + documented for comments and literals ("never empty"); the identifier branch was the one + place not honouring it. Prior corpus cases all happened to carry a space before the + quote, which is why this stayed invisible; the corpus now pins both weld directions and + asserts the neutralized token structure directly, so neither a one-sided fix nor a + blanket separator can pass. ## [0.5.0] — 2026-07-25 diff --git a/packages/agami-core/src/sql_guard.py b/packages/agami-core/src/sql_guard.py index dfb425cb..fb60448c 100644 --- a/packages/agami-core/src/sql_guard.py +++ b/packages/agami-core/src/sql_guard.py @@ -74,6 +74,18 @@ def _neutralize(sql: str) -> str: deliberately NOT an escape here — engines disagree (MySQL yes, standard PG no), and not honoring it can only stop a literal *early* (fail safe), never late. """ + def _last_emitted(chunks: list[str]) -> str: + """The last character actually emitted, skipping empty chunks. + + Peeking at `chunks[-1]` alone is wrong: a zero-length identifier (`""`) appends an + empty string, which would hide the real preceding character and drop a separator + that is needed. + """ + for chunk in reversed(chunks): + if chunk: + return chunk[-1] + return "" + out: list[str] = [] i, n = 0, len(sql) while i < n: @@ -130,20 +142,28 @@ def _neutralize(sql: str) -> str: # A pathological identifier like "a;b" reduces to a;b and trips the # multi-statement check — a deliberate, safe-direction hardening choice. # - # The quote is ALSO the token delimiter: a delimited identifier is - # self-delimiting in SQL, so `FROM"pg_class"` needs no whitespace and is a - # valid statement the engine runs. Dropping the quotes without re-supplying a - # separator therefore fuses two tokens into one (`FROM"pg_class"` -> - # `FROMpg_class`), destroying the `\b` anchor every deny-list pattern below - # relies on — the gate stops seeing the token at all rather than allowing it. - # Re-supply the boundary ONLY when the previous emitted char is a word char: - # a blanket space would split a qualified name (`t."col"` -> `t. col`) and - # break the `(? + # `FROMpg_class`, `"x"INTO` -> `xINTO` — which defeats the `\b` anchors every + # deny-list pattern below relies on. The gate then stops *seeing* the token + # rather than allowing it, so it returns no rejection at all. This is the same + # invariant the docstring states for comments and literals ("never empty"); + # the identifier branch is simply the one place that was not honouring it. + # + # Re-supply a boundary only where the quote was actually separating two word + # chars, so a qualified name stays one token: `t."col"` must neutralize to + # `t.col`, not `t. col`. That structural fidelity is the neutralizer's job — + # it removes hiding places without re-tokenizing the statement — and it is + # pinned by an explicit output test, since no current rule distinguishes the + # two spellings on its own. + if _last_emitted(out).isalnum() or _last_emitted(out) == "_": out.append(" ") out.append("".join(buf)) + nxt = sql[j] if j < n else "" + if nxt.isalnum() or nxt == "_": + out.append(" ") i = j elif sql[i] == "$": # dollar-quoted string literal ($$...$$ or $tag$...$tag$) # Only a `$tag$` with a MATCHING close delimiter is a literal we can blank. diff --git a/plugins/agami/lib/sql_guard.py b/plugins/agami/lib/sql_guard.py index dfb425cb..fb60448c 100644 --- a/plugins/agami/lib/sql_guard.py +++ b/plugins/agami/lib/sql_guard.py @@ -74,6 +74,18 @@ def _neutralize(sql: str) -> str: deliberately NOT an escape here — engines disagree (MySQL yes, standard PG no), and not honoring it can only stop a literal *early* (fail safe), never late. """ + def _last_emitted(chunks: list[str]) -> str: + """The last character actually emitted, skipping empty chunks. + + Peeking at `chunks[-1]` alone is wrong: a zero-length identifier (`""`) appends an + empty string, which would hide the real preceding character and drop a separator + that is needed. + """ + for chunk in reversed(chunks): + if chunk: + return chunk[-1] + return "" + out: list[str] = [] i, n = 0, len(sql) while i < n: @@ -130,20 +142,28 @@ def _neutralize(sql: str) -> str: # A pathological identifier like "a;b" reduces to a;b and trips the # multi-statement check — a deliberate, safe-direction hardening choice. # - # The quote is ALSO the token delimiter: a delimited identifier is - # self-delimiting in SQL, so `FROM"pg_class"` needs no whitespace and is a - # valid statement the engine runs. Dropping the quotes without re-supplying a - # separator therefore fuses two tokens into one (`FROM"pg_class"` -> - # `FROMpg_class`), destroying the `\b` anchor every deny-list pattern below - # relies on — the gate stops seeing the token at all rather than allowing it. - # Re-supply the boundary ONLY when the previous emitted char is a word char: - # a blanket space would split a qualified name (`t."col"` -> `t. col`) and - # break the `(? + # `FROMpg_class`, `"x"INTO` -> `xINTO` — which defeats the `\b` anchors every + # deny-list pattern below relies on. The gate then stops *seeing* the token + # rather than allowing it, so it returns no rejection at all. This is the same + # invariant the docstring states for comments and literals ("never empty"); + # the identifier branch is simply the one place that was not honouring it. + # + # Re-supply a boundary only where the quote was actually separating two word + # chars, so a qualified name stays one token: `t."col"` must neutralize to + # `t.col`, not `t. col`. That structural fidelity is the neutralizer's job — + # it removes hiding places without re-tokenizing the statement — and it is + # pinned by an explicit output test, since no current rule distinguishes the + # two spellings on its own. + if _last_emitted(out).isalnum() or _last_emitted(out) == "_": out.append(" ") out.append("".join(buf)) + nxt = sql[j] if j < n else "" + if nxt.isalnum() or nxt == "_": + out.append(" ") i = j elif sql[i] == "$": # dollar-quoted string literal ($$...$$ or $tag$...$tag$) # Only a `$tag$` with a MATCHING close delimiter is a literal we can blank. diff --git a/tests/test_sql_guard.py b/tests/test_sql_guard.py index 58f1dfaf..1e65717a 100644 --- a/tests/test_sql_guard.py +++ b/tests/test_sql_guard.py @@ -29,7 +29,7 @@ from typing import Any import pytest -from sql_guard import _MAX_SQL_CHARS, check_read_only +from sql_guard import _MAX_SQL_CHARS, _neutralize, check_read_only # --------------------------------------------------------------------------- # Accept — valid single read-only statements @@ -392,6 +392,19 @@ def test_red_team_quoted_dangerous_fn_bypass(sql: str) -> None: 'SELECT 1 FROM"pg_class"WHERE"pg_sleep"(10) IS NULL', # The weld can also appear mid-statement, after a non-keyword word char. 'SELECT a FROM t WHERE b="pg_sleep"(1)', + # The CLOSING quote delimits too, so a keyword can hide on the trailing side. + # `SELECT ... INTO
` is a write that opens with SELECT, which is exactly + # why INTO is in the DML/DDL list; `\bINTO\b` does not match `xINTO`. Verified on + # PostgreSQL 16: each of these creates a table holding the source rows. + 'SELECT "x"INTO evil FROM t', + 'SELECT"x"INTO evil FROM t', + 'SELECT t."x"INTO evil FROM t', + 'SELECT 1 AS"a"INTO evil', + 'WITH c AS (SELECT 1 AS "x")SELECT "x"INTO evil FROM c', + # Row-lock rule, same root cause. FOR UPDATE survives only because bare UPDATE is + # independently in the DML list; FOR SHARE has no such backstop. + 'SELECT * FROM t AS"a"FOR SHARE', + 'SELECT * FROM t AS"a"FOR KEY SHARE', ], ) def test_red_team_welded_quoted_dangerous_fn_bypass(sql: str) -> None: @@ -422,6 +435,34 @@ def test_welded_fix_does_not_over_block_legitimate_quoted_identifiers(sql: str) assert check_read_only(sql) is None, f"Legitimate quoted identifier wrongly blocked: {sql!r}" +@pytest.mark.parametrize( + ("sql", "expected"), + [ + # A separator is re-supplied ONLY where the quote actually separated two word + # chars — so a qualified name survives as ONE token. + ('SELECT t."current_user" FROM t', "SELECT t.current_user FROM t"), + ('SELECT "schema"."table" FROM "schema"."table"', "SELECT schema.table FROM schema.table"), + ('SELECT c."email" FROM customers c', "SELECT c.email FROM customers c"), + # ...and IS re-supplied on both sides where it was separating word chars. + ('SELECT * FROM"pg_class"', "SELECT * FROM pg_class"), + ('SELECT "x"INTO evil FROM t', "SELECT x INTO evil FROM t"), + ('SELECT 1 AS"a"INTO evil', "SELECT 1 AS a INTO evil"), + ], +) +def test_neutralize_preserves_token_structure(sql: str, expected: str) -> None: + """Pin the *shape* of the neutralized text, not just the gate's yes/no. + + Every rule here is `\\b`-anchored, so a blanket separator on both sides would block the + same attacks and pass the same negatives — the gate-level tests alone cannot tell the + two designs apart, and a future refactor could silently swap one for the other. What a + blanket space would change is token *structure*: `t."col"` would become `t. col`, two + tokens where the statement meant one. Any rule that reasons about qualification (a + pattern anchored on a preceding `.`, say) would then read a qualified column as a bare + identifier. Asserting the neutralized string keeps that decision checkable. + """ + assert _neutralize(sql) == expected + + @pytest.mark.parametrize( "sql", [