security: close a read-only-guard bypass via a welded quoted identifier

[sandeep-agami]

Summary

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 (the shared lexer the SQL guard runs before matching anything) 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 rather than allowing it, and check_read_only returned None (pass).

Verified end-to-end against PostgreSQL 16

Before the fix — the database executes it, the guard passes it:

$ echo "verify" > /tmp/probe.txt
psql  -> verify                             # server-side file read succeeded
guard -> PASS                               # check_read_only returned None

After the fix, same string:

guard -> REFUSE: function `pg_read_file` is not allowed —
         server-file / OS / process-control / sleep / remote-SQL functions are blocked

Scope of the exploitable shape

The weld only bypasses when it lands on a non-leading keyword:

input	neutralized	outcome before fix
SELECT*FROM"pg_read_file"(1)	SELECT*FROMpg_read_file(1)	bypass
SELECT 1 FROM"pg_class"WHERE"pg_sleep"(10) IS NULL	…FROMpg_classWHEREpg_sleep(10)…	bypass
SELECT"pg_sleep"(10)	SELECTpg_sleep(10)	caught incidentally, by the statement-opener check

The fix

Re-supply 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 (?<!\.) lookbehind that keeps a qualified column from reading as a bare dangerous identifier.

Applied to both the package source and the vendored plugin mirror via dev.py sync-lib, so the drift check stays green and both surfaces are protected.

Why this was invisible

Every pre-existing case in the red-team corpus happens to carry a space before the quote (SELECT "pg_sleep"(10)), so the missing separator never showed. The corpus now pins 8 welded forms and, on the other side, 6 qualified-name negatives that must keep passing — so a future over-correction (a blanket space) fails just as loudly as the original gap.

Test plan

• python3 dev.py check green — 1571 passed, 1 skipped, ruff clean, gitleaks clean, vendored-lib drift check clean
• New regression tests fail on main and pass here (2 of the 8 welded cases were live bypasses)
• End-to-end verification against a live PostgreSQL 16 instance, shown above

[Copilot]

Pull request overview

This PR hardens the SQL read-only guard by fixing a lexer edge case where dropping double quotes from delimited identifiers could “weld” tokens together and bypass \b-anchored deny-list regex checks.

Changes:

• Update _neutralize in the shared SQL guard lexer to re-supply a separator around welded double-quoted identifiers.
• Add regression tests covering welded quoted-identifier bypass forms plus “don’t over-block” quoted-identifier negatives.
• Document the security fix in CHANGELOG.md.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
tests/test_sql_guard.py	Adds regression corpus for welded quoted-identifier bypass and non-overblocking cases.
plugins/agami/lib/sql_guard.py	Applies the lexer hardening to the vendored plugin mirror of the SQL guard.
packages/agami-core/src/sql_guard.py	Applies the lexer hardening to the package source-of-truth SQL guard.
CHANGELOG.md	Records the security fix and rationale in the Unreleased section.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[sandeep-agami]

Review: the separator is re-supplied on only one side of the identifier

Verified against this branch (c42f96c) and a live PostgreSQL 16.12.

1. Blocking: the closing quote welds too, and INTO slips through

The diagnosis is right but it stops halfway. A delimited identifier is self-delimiting on both ends. The fix re-supplies a separator before the identifier and nothing after it, so "x"INTO still fuses:

SELECT "x"INTO evil FROM t   ->   SELECT xINTO evil FROM t

INTO is in _DML_DDL_KEYWORDS precisely because SELECT ... INTO new_table is a write that opens with SELECT. \bINTO\b does not match xINTO, so rule 4 goes blind in exactly the way rule 6 did.

On this branch:

SQL	guard	PostgreSQL 16.12
SELECT "x"INTO evil FROM t	PASS	creates table evil
SELECT"x"INTO evil FROM t	PASS	creates table evil
SELECT t."x"INTO evil FROM t	PASS	creates table evil
SELECT 1 AS"a"INTO evil	PASS	creates table evil
WITH c AS (SELECT 1 AS "x")SELECT "x"INTO evil FROM c	PASS	creates table evil

(Run inside a transaction and rolled back. Each returned SELECT 1, and the created table held the source rows.)

One of those shapes is worse here than on main. SELECT"x"INTO evil FROM t is blocked on main, because the leading weld produced SELECTxINTO ... and rule 3 (opening keyword) rejected it incidentally. Re-supplying the leading separator restores a valid-looking SELECT opener while the trailing weld still hides INTO, so it now passes. That is a net new bypass introduced by this change.

2. Blocking: the same root cause reaches the row-lock rule

SELECT * FROM t AS"a"FOR SHARE       -> PASS   (parses and takes a share lock on PG 16.12)
SELECT * FROM t AS"a"FOR KEY SHARE   -> PASS

FOR UPDATE survives only because bare UPDATE is independently in the DML deny-list. FOR SHARE has no such backstop, so rule 5 is bypassable.

3. The stated rationale points at a regex that is not in the file

Both copies of the comment say a blanket space would "break the (?<!\.) lookbehind that keeps a qualified column from reading as a bare dangerous identifier". There is no (?<!\.) anywhere in sql_guard.py:

$ grep -rn '(?<!' packages/agami-core/src/sql_guard.py plugins/agami/lib/sql_guard.py
packages/agami-core/src/sql_guard.py:141:  # break the `(?<!\.)` lookbehind ...
plugins/agami/lib/sql_guard.py:141:        # break the `(?<!\.)` lookbehind ...

The only two hits are the comment describing itself. _DANGEROUS_FN_RE is \b-anchored with no lookbehind, so t."pg_sleep"(1) is blocked today under either design (. is already a word boundary). This comment is the load-bearing justification for the conditional, and a reader cannot check it against the code.

4. The negatives do not pin what the description says they pin

The description says the corpus makes "a future over-correction (a blanket space) fail just as loudly as the original gap". It does not. Replacing the conditional with an unconditional out.append(" ") and running the file:

253 passed in 0.88s

All 253 pass, including all 6 new qualified-name negatives. Nothing in the suite distinguishes the conditional design from the blanket one. So either the conditional is not actually required, or the case that requires it is missing. Worth settling before this lands, because the conditional is the reason the trailing side was left unhandled.

5. Minor: prev can read as empty when it is not

prev = out[-1][-1:] if out and out[-1] else "" peeks at the last element, and out.append("".join(buf)) can append "" for a zero-length identifier, which hides the real preceding char. Tracking the last emitted char in a scalar alongside out is easier to reason about than peeking at list structure. I could not build an exploit through this (the doubled-quote escape path absorbs the adjacent cases), so this is robustness rather than a defect.

6. Minor: SECURITY.md is left overclaiming

The description notes this falsifies SECURITY.md:44-45 ("bypasses hidden in string literals, comments, or double-quoted identifiers"), but the file is not touched. With the trailing weld still open, that line remains an overclaim after this merges.

A direction that checks out

Mirroring the existing conditional on the closing side closes all of the above with no false positives:

prev = out[-1][-1:] if out and out[-1] else ""
if prev.isalnum() or prev == "_":
    out.append(" ")
out.append("".join(buf))
nxt = sql[j] if j < n else ""
if nxt.isalnum() or nxt == "_":
    out.append(" ")
i = j

With that applied: all seven shapes above are blocked, t."current_user" still neutralizes to t.current_user (not t. current_user), "schema"."table" still to schema.table, and the file is 253 passed. Whatever shape the final fix takes, the corpus needs at least one case that actually fails under the wrong variant, or point 4 recurs on the next change here.

What is good here

Real bug, correctly root-caused down to the \b anchor rather than patched at the pattern level. Verified end to end against a live engine instead of test-only confidence. Both surfaces synced through sync-lib so they cannot drift. The \b-anchor explanation is the right thing to have written down, and it is what made the remaining half of the bug easy to find.

[sandeep-agami]

All six addressed in 5ef42fb. Finding 1 was correct and was the serious one — the previous commit introduced a bypass, and I've verified that independently before fixing.

1 + 2 — the closing quote welds too

Confirmed on PostgreSQL 16.12. SELECT "x"INTO evil FROM t creates a table holding the source rows, and the guard passed it. Also confirmed the net-new regression you identified:

SQL	main	previous commit	now
SELECT"x"INTO evil FROM t	REFUSE	PASS ← introduced	REFUSE
SELECT "x"INTO evil FROM t	PASS	PASS	REFUSE
SELECT t."x"INTO evil FROM t	PASS	PASS	REFUSE
SELECT 1 AS"a"INTO evil	PASS	PASS	REFUSE
WITH c AS (SELECT 1 AS "x")SELECT "x"INTO evil FROM c	PASS	PASS	REFUSE
SELECT * FROM t AS"a"FOR SHARE	PASS	PASS	REFUSE
SELECT * FROM t AS"a"FOR KEY SHARE	PASS	PASS	REFUSE

Fix is the mirrored conditional you suggested. All seven are now in the red-team corpus.

3 — the phantom lookbehind

You're right, and it was worse than a stale reference: I carried that justification over from a different guard, so it described code that isn't in this file. Removed. The real justification is the lexer's own documented invariant, four lines above the loop:

"Neutralized spans collapse to a single space (never empty — welding tokens like SELECT/**/INTO → SELECTINTO would defeat the \b word boundaries below)."

The contract was already written down; the identifier branch was the one place not honouring it. That's checkable against the file, unlike what I'd written.

4 — the corpus now discriminates

This was the most useful finding. You were right that nothing distinguished the designs. Added test_neutralize_preserves_token_structure, which asserts the neutralized string rather than the gate's yes/no. Re-ran both wrong variants against the corpus:

blanket separator (both sides)  ->  5 failed, 261 passed
leading side only (prev commit) ->  9 failed, 257 passed
correct (conditional, both)     ->  266 passed

So the conditional is now pinned by something that actually fails without it. To answer the underlying question directly: no current rule distinguishes t.col from t. col, because everything here is \b-anchored. The justification is structural — the neutralizer's job is to remove hiding places without re-tokenizing, and a blanket space turns one qualified name into two tokens. Any future rule reasoning about qualification would inherit that. Since that's a design decision with no present behavioural signal, it's asserted on the output directly.

5 — prev peeking at list structure

Replaced with _last_emitted(), which walks back over empty chunks. Agreed it was robustness rather than a defect — the doubled-quote path absorbs the reachable cases — but the zero-length-identifier hole was real.

6 — SECURITY.md

Left as-is deliberately: with both ends closed, the claim about "bypasses hidden in … double-quoted identifiers" is now true rather than aspirational. "a;b" still reduces to a;b and trips the multi-statement rule (verified). The fix makes the doc accurate; editing it would have been the wrong direction.

---

python3 dev.py check green, mirrors byte-identical via sync-lib. Thank you for catching the trailing side — a partial fix that re-opens a shape main blocked is worse than no fix, and the mutation check in 4 is the thing that stops it recurring here.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

[sandeep-agami]

Re-review of 5ef42fb: all six points addressed, verified

Re-ran everything against the new head.

The two blocking findings are closed

All nine shapes from the previous comment are now rejected, including the one that regressed against main:

BLOCKED  SELECT "x"INTO evil FROM t
BLOCKED  SELECT"x"INTO evil FROM t
BLOCKED  SELECT t."x"INTO evil FROM t
BLOCKED  SELECT 1 AS"a"INTO evil
BLOCKED  WITH c AS (SELECT 1 AS "x")SELECT "x"INTO evil FROM c
BLOCKED  SELECT * FROM t AS"a"FOR SHARE
BLOCKED  SELECT * FROM t AS"a"FOR KEY SHARE
BLOCKED  SELECT*FROM"pg_read_file"('/etc/passwd')
BLOCKED  SELECT 1 FROM"pg_class"WHERE"pg_sleep"(10) IS NULL

I also swept ten shapes that are not in the corpus, to check the fix generalizes rather than matching the reported cases. All blocked:

SELECT"a"INTO"evil"FROM t                                   -> SELECT a INTO evil FROM t
SELECT * FROM t AS"a"FOR NO KEY UPDATE                      -> SELECT * FROM t AS a FOR NO KEY UPDATE
SELECT 1 FROM t AS"a"WHERE"pg_read_file"('…')IS NULL        -> … WHERE pg_read_file( )IS NULL
SELECT"a"."b"INTO evil FROM t                               -> SELECT a.b INTO evil FROM t
WITH"c"AS(SELECT 1)SELECT"x"INTO evil FROM"c"               -> WITH c AS(SELECT 1)SELECT x INTO evil FROM c
SELECT * FROM"t"WHERE x="a;DROP SCHEMA public CASCADE"      -> multi-statement

The corpus now actually discriminates, which was the point of finding 4

This is the part I most wanted to see, and test_neutralize_preserves_token_structure does the job. I rebuilt both wrong designs against the new test file:

variant	result
blanket separator on both sides	5 failed, 261 passed
the previous leading-only fix	9 failed, 257 passed

So the suite now fails loudly in both directions: over-correction trips the structural assertions, under-correction trips the red-team cases. Previously the blanket variant passed all 253. Pinning the neutralized string rather than only the gate verdict is the right call, and the docstring explaining why the gate-level tests cannot tell the designs apart is worth keeping.

Findings 3 and 5

The (?<!\.) reference is gone, and the replacement is honest about its own status: it now says the structural choice is pinned by an explicit output test "since no current rule distinguishes the two spellings on its own", and frames qualification-aware matching as a hypothetical future rule rather than an existing one. That is checkable against the code as written.

_last_emitted handles the empty-chunk case. I checked it does not open a performance path, since the backward scan runs inside the 50KB cap:

many empty idents    len=48008    22.23 ms
many quoted idents   len=50000    10.26 ms
welded idents        len=48007    13.80 ms
one huge ident       len=49009     2.43 ms

Linear, no blowup.

Withdrawing finding 6

I was wrong to leave SECURITY.md on the list. That line is only an overclaim while the trailing weld is open; with both directions closed it is accurate again, so there is nothing to change. Confirmed the multi-statement half specifically: SELECT 1 AS"a";SELECT 2 and SELECT * FROM"t"WHERE x="a;DROP SCHEMA public CASCADE" both trip the multi-statement check.

Gate status

1584 passed, 1 skipped        (full suite)
ruff check                    All checks passed!
diff packages/… plugins/…     vendored mirror identical

One thing I probed and cleared

The trailing check keys off sql[j], the raw source char, rather than the next emitted char. That is only safe if every other branch emits at least one non-word char. I tested the interleavings: block comment, line comment, string literal, dollar-quoted literal, empty identifier on either side, and doubled-quote escapes. All blocked.

The single shape that slips is SELECT "x"$1INTO evil FROM t (x$1INTO, no \b before INTO). It is pre-existing on main, unrelated to this change, and PostgreSQL 16 rejects it anyway:

ERROR:  trailing junk after parameter at or near "$1INTO"

So there is nothing to do here. Noting it only so the assumption is on the record as tested rather than assumed.

Two small nits, neither blocking

1. if _last_emitted(out).isalnum() or _last_emitted(out) == "_": calls the helper twice. The previous revision bound prev first, which read better and scanned once. Worth rebinding.

2. ruff format --check now reports these three files; on main all three come back "already formatted". CI stays green because dev.py lint runs format with allow_fail=True, but the "unformatted backlog" rationale for that does not apply to files that were clean before the PR. It is a blank line before the nested def plus quote-style normalization on two of the new test strings. One uvx ruff@0.15.19 format clears it.

Good fix. The root cause is now stated symmetrically, the corpus defends both edges of it, and the reasoning in the comments matches what the code actually does.