Skip to content

gateguard: DESTRUCTIVE_SQL_DD is unreachable for quoted SQL — stripQuotedStrings() runs before the SQL arm #3024

Description

@mattcooperdev

DESTRUCTIVE_SQL_DD never sees quoted SQL, so every realistic SQL invocation is allowed

Summary: stripQuotedStrings() is applied to the command before the DESTRUCTIVE_SQL_DD arm tests it, at both call sites. Since SQL is almost always passed to a client inside a quoted -c / -e argument, the destructive SQL patterns are unreachable in practice.

Affected version: 2.2.1, commit 5064474d4d762dc9640234a41617cccb79185cec, installed via the ecc@ecc marketplace plugin. Installed scripts/hooks/gateguard-fact-force.js is 1347 lines.

Reproduction

Save as repro.js and run node repro.js <plugin>, where <plugin> is the installed plugin directory.

const os = require('os'), fs = require('fs'), path = require('path');

// Isolate hook state so the repro cannot touch a real ~/.gateguard directory.
process.env.GATEGUARD_STATE_DIR = fs.mkdtempSync(path.join(os.tmpdir(), 'ggrepro-'));

const { run } = require(process.argv[2] + '/scripts/hooks/gateguard-fact-force.js');

function decide(command) {
  const input = { tool_name: 'Bash', tool_input: { command }, session_id: 'repro' };
  return run(input) === input ? 'ALLOWED' : 'DENIED';
}

decide('printf ready'); // prime: the first Bash call trips the separate routine gate

for (const command of [
  'psql -c "drop table users"',
  "psql -c 'truncate audit_log'",
  'mysql -e "delete from sessions"',
  'psql -c drop table users',
]) console.log(decide(command).padEnd(8), command);

Observed results

Command Decision
psql -c "drop table users" ALLOWED
psql -c 'truncate audit_log' ALLOWED
mysql -e "delete from sessions" ALLOWED
psql -c drop table users (unquoted) DENIED

Only the unquoted spelling is caught — and that spelling is not valid for the client, since the shell would split it into separate arguments.

Root cause

Line 60 defines the arm:

const DESTRUCTIVE_SQL_DD = /\b(drop\s+table|delete\s+from|truncate|dd\s+if=)\b/i;

Both call sites strip quoted content first — lines 702-703:

const flattened = explodeSubshells(stripQuotedStrings(executable));
if (DESTRUCTIVE_SQL_DD.test(flattened)) return true;

and lines 729-730:

const stripped = stripQuotedStrings(segment);
if (DESTRUCTIVE_SQL_DD.test(stripped)) return true;

The stripping is deliberate, and the comment above line 60 explains why: it stops a commit message that mentions drop table from tripping the gate. That rationale is sound for the false-positive direction, but it also guarantees the SQL patterns can never match a real invocation, because a real invocation must quote the statement.

So the three SQL patterns (drop table, delete from, truncate) are effectively dead code. dd if= on the same line is separately unreachable for real device paths — that one is #2642, which I have commented on with a 2.2.1 confirmation.

Impact

An operator who trusts GateGuard to gate destructive commands gets no speed bump before an agent drops or truncates a table, or deletes every row. The failure is silent: the command is allowed with no indication a pattern was intended to cover it.

Suggested fix

Keep the quote-stripping for the generic scan, but scan inside the quoted argument when the executable is a known SQL client. Roughly: if the resolved command basename is one of psql, mysql, mariadb, sqlite3, mongosh, clickhouse-client, etc., extract the argument to -c / -e / --command / --execute and test the SQL patterns against that string specifically.

That preserves the commit-message protection — git commit -m "drop table cleanup" still has no SQL client in the executable position — while making the patterns reachable where they matter.

Regression cases worth pinning, all of which should deny:

psql -c "drop table users"
psql --command='truncate audit_log'
mysql -e "delete from sessions"
sqlite3 app.db "drop table users"

and this should continue to allow:

git commit -m "refactor: drop table indirection"

Notes

Searched the tracker for psql, sql, drop table, quoted, and strip before filing. The nearest existing reports are #2886 (detector matches heredoc body text) and #3023 (block-no-verify false positive on a quoted argument) — both over-matching bugs, i.e. the opposite direction from this one.

Filed as a public issue rather than a private advisory to match the maintainers' handling of the same class of defect (#2642, #2291, #2886 are all public). Happy to move it to a private advisory if you would rather triage destructive-gate bypasses that way.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions