[ajitpratap0]

Summary

Closes the ScanSQL bypasses verified in the 2026-10-28 audit. ScanSQL also backs the MCP security_scan tool, the C binding and WASM.

• Dollar tags inside strings: SELECT '$x$'; DROP TABLE users; SELECT '$x$' scanned clean because $x$...$x$ was stripped as a dollar quote. A tag is now stripped only when every quoting interpretation agrees it sits outside strings and comments. The interpretations are backslash vs doubled-quote escapes, PostgreSQL E-strings, MySQL "..." strings, [bracket] identifiers, nested comments and # comments.
• 10 KB truncation: tautology and comment checks only looked at the first 10,000 bytes. They now cover the whole input. Inputs above 10 MiB (the tokenizer's limit) get a CRITICAL INPUT_TOO_LARGE finding at every MinSeverity, so unscanned input never looks clean.
• Inline-comment evasion: UNION/**/SELECT, SLEEP/**/(5), ;/**/DROP, UNION--\nSELECT, # comments, /*/ and MySQL 1--1 all evaded matching. Patterns now also run over comment-free views of the text, one per quoting convention. The raw text is still checked, and the extra views can only add findings. MySQL /*! */ and MariaDB /*M! */ bodies are unwrapped and still flagged.
• Dead AST check: the UNION NULL-padding check looked for *ast.Identifier "NULL"; the parser produces a null *ast.LiteralValue. It now matches that.
• Memory: the tautology check compares operands without allocating per candidate. 1 MiB of 1=2 drops from about 50 MB to about 1 MB allocated.

This overlaps #879 (Scan AST walker) only in checkUnionSelect, kept as a separate hunk.

Test Plan

• wp_p_bypass_test.go: every audit repro and every input from two independent Sonnet security reviews, plus false-positive guards for benign SQL with comments and strings. Also covers allocation bounds, the exact 10 MiB boundary, and an n vs 4n linear-time ratio (4.04×).
• sqltext_test.go with FuzzSQLTextViews. The reviewer ran 3M differential reference-lexer cases (MySQL, SQL Server, SQLite, PostgreSQL, standard SQL) and 400k prefilter-on/off cases with no differences after the fixes.
• task check (55 packages ok, golangci-lint 0 issues) and task test:cbinding pass locally. The security race suite takes about 50 s (was 34 s), within the 120 s package timeout.
• Benchmarks vs origin/main:

Benchmark	main	branch
BenchmarkScanSQL	5.5 µs	7.0 µs (full-input scan plus comment views)
CleanSQL_Raw	5.5 µs	4.0 µs
TautologyDetection	266 ns	89 ns
MixedPatterns	655 ns	282 ns

Breaking Changes

• Oversized input (>10 MiB) now always yields a CRITICAL INPUT_TOO_LARGE finding.
• Some inputs that used to scan clean now report findings. Those are the bypasses above.

Known limits, documented in code: a MySQL $x$ identifier shaped like a dollar quote is still stripped (ScanSQL has no dialect parameter). Fragments that start with an unbalanced quote (x' OR/**/TRUE--) and some existing tautology shapes (1=01, '1'=1, (1)=(1)) are not detected. At 10 MiB, an adversarial input that turns on every view can take about 6 s.

🤖 Generated with Claude Code

https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated
gosqlx-website	Ready	Preview	Oct 28, 2026 8:36pm UTC

[github-actions]

Claude finished @ajitpratap0's task in 36s —— View job

---

Review of PR #885: ScanSQL bypass fixes

I read the diff for scanner.go, dollar_quote.go and sqltext.go. I skimmed the tests and did not run them, so the PR's claims about the suite, fuzzing and benchmarks are unchecked.

Overall: The approach is sound. Matching over several comment-free views while always keeping the raw view means the extra views can only add findings. Stripping a dollar tag only when every quoting interpretation agrees it is outside strings and comments is the right conservative rule. The allocation-free tautology comparison and the \s → [\s\v] change are correct, because Go's \s excludes \v. The prefilter reasoning also holds: the required literals contain no whitespace, so removing comments cannot introduce a literal that is absent from the raw text.

Concerns

1. Memory and CPU amplification on large inputs (sqltext.go:199-215, dollar_quote.go)

   • commentViews can build up to 32 modes, and nested variants double that, each a copy of up to 10 MiB. Peak memory in the adversarial case could reach several hundred MiB per scan.
   • stripDollarQuotedStrings steps 32 lexer states per byte on any input containing $.
   • The PR notes about 6 s at 10 MiB. ScanSQL backs the MCP tool and WASM, so I'd consider a lower practical cap or a total-view-bytes budget. If the budget is exceeded, the scanner should fail closed with a finding rather than silently skip views.

2. Duplicated size constant (scanner.go, MaxScanInputSize)

   • security deliberately doesn't import tokenizer, so the constant is copied. Add a test, in a test file that may import the tokenizer, asserting MaxScanInputSize == tokenizer.MaxInputSize. Otherwise the two can drift silently.

3. New exported PatternInputTooLarge

   • Please check that the MCP security_scan tool, cbinding and WASM don't switch exhaustively on PatternType or map it to a fixed list. It's also worth documenting in the user-facing docs, since this is a breaking change.

4. /*/ handling comment is misleading (sqltext.go:278)

   • The comment says flat comments "may close at the overlapping */ in /*/". But j starts at i+2, so the overlapping close at i+1 is never considered. If the intent is that /*/ is treated as a complete comment in some view, the code doesn't do it. Either fix the code or the comment. The PR description lists /*/ as covered, so confirm the test exercises this path.

5. checkUnionSelect

   • It depends on the literal Type being the string "null". Please confirm this against the parser's constant, or reference a shared constant, rather than a string match with EqualFold. The nil-guards are good.

6. Minor

   • The doc comment on stripDollarQuotedStrings has an awkward line wrap ("...before text / is hidden").
   • normalizeComments is now a one-line wrapper. It could be inlined.
   • The known limits (1=01, '1'=1, (1)=(1), unbalanced-quote fragments) are documented in the code, but a tracking issue would help.

Test coverage

The coverage looks strong: audit repros, differential fuzzing, boundary and linear-time tests, and false-positive guards. A test that pins the peak allocation for a worst-case 10 MiB input would guard concern 1.

No blocking correctness bugs found in the code I read. I'd address concern 1 or explicitly accept it, and fix concern 4 before merge.

[github-actions]

Claude finished @ajitpratap0's task in 26s —— View job

---

Review of PR #885: ScanSQL bypass fixes

I read sqltext.go in full and grepped the size-limit and PatternInputTooLarge usage. I did not run the tests, fuzzers or benchmarks, so the PR's claims about them are unchecked.

Overall: The design is sound. Matching over extra comment-free views while always keeping the raw view means the new views can only add findings. Stripping a dollar tag only when every quoting interpretation agrees it is outside strings and comments is the right conservative rule. I found no blocking correctness bugs.

Corrections to the earlier automated review

• Size constant drift is already guarded. wp_p_bypass_test.go:345 asserts MaxScanInputSize == tokenizer.MaxInputSize. No further test is needed.
• The /*/ comment is only imprecise, not wrong (sqltext.go:278). The scan for a comment's close starts at i+2, so the opener's own * is never reused. The overlap in the comment applies to a nested /* found inside a flat comment. There, j++ lets the * of that opener pair with the next / (for example /* /*/ closes). The code does what the author intended, but the wording suggests otherwise. Suggested rewording: "A nested /* inside a flat comment advances one byte, so its * can pair with a following /."

Remaining suggestions

1. Worst-case cost at 10 MiB (sqltext.go:165-217)

   • commentViews can build up to 32 modes, plus a nested variant for each, and every view is a copy of the input. The PR notes about 6 s on adversarial input.
   • ScanSQL backs the MCP tool and WASM, where this is a CPU and memory amplification point. Consider a total view-bytes budget that fails closed with a finding, in the same way as INPUT_TOO_LARGE. A test pinning peak allocation on a worst-case input would also help.
   • A cheap partial mitigation is to skip modes whose flags make no difference to the output. The existing add already dedupes results, but only after the work is done.

2. Check consumers of the new PatternInputTooLarge

   • Confirm the MCP security_scan tool, cbinding and WASM don't switch exhaustively on PatternType.
   • Mention the new finding in the user-facing docs, since the PR lists it as a breaking change.

3. checkUnionSelect

   • It matches the null literal by type string. Prefer a shared constant if the parser exposes one, and otherwise pin the string with a test that parses NULL.

4. Minor

   • normalizeComments (sqltext.go:219) is a one-line wrapper with a single call site. Inline it.
   • Open a tracking issue for the documented known limits (1=01, '1'=1, (1)=(1), unbalanced-quote fragments, MySQL $x$ identifiers).

Test coverage

Coverage looks strong: audit repros, differential fuzzing, the exact 10 MiB boundary, a linear-time ratio, and false-positive guards. The only gap is the worst-case peak memory test in suggestion 1.

Verdict: Fine to merge once suggestion 1 is either addressed or explicitly accepted. The rest is optional.

[github-actions]

Claude finished @ajitpratap0's task in 19s —— View job

---

Review of PR #885: ScanSQL bypass fixes (re-review on synchronize)

I read the commit list and sqltext.go (commentViews and normalizeCommentView). I did not run the tests, fuzzers or benchmarks. This repeats and refines the two earlier reviews, and I found no new blocking bugs.

Overall: The design is sound. The raw view is always kept and the comment-free views only add findings, so they can't hide anything. The mode gating in commentViews already skips quoting modes that can't change the input, which partly answers the earlier "skip no-op modes" suggestion.

Still open

1. Worst-case cost at 10 MiB (sqltext.go, commentViews)

   • Up to 31 non-zero modes plus nested variants can each build a full-size copy. The PR itself quotes about 6 s for adversarial input.
   • ScanSQL backs the MCP tool and WASM. Either add a total view-bytes budget that fails closed with a finding, or explicitly accept the cost. The PR description already documents it as a known limit, so accepting it is reasonable.

2. Misleading comment (sqltext.go, the else branch where j++)

   • The text "Flat comments may close at the overlapping */ in /*/" is imprecise. The scan starts at i+2, so the opener's own * is never reused. What the code does is let a nested /* found inside a flat comment advance one byte, so its * can pair with a following /, as in /* /*/.
   • Suggested rewording: "A nested /* inside a flat comment advances one byte, so its * can pair with a following /."

3. PatternInputTooLarge consumers

   • Confirm that the MCP security_scan tool, cbinding and WASM don't switch exhaustively on PatternType. In the files I grepped, only pkg/sql/security references it, but I didn't check the other packages.
   • Mention the new finding in the user docs, since the PR lists it as a breaking change.

4. checkUnionSelect

   • The null literal is matched by type string. Prefer a shared constant if the parser exposes one. Otherwise add a test that parses NULL and asserts the type string.

5. Minor

   • normalizeComments is a one-line wrapper with one call site. Inline it.
   • Open a tracking issue for the documented limits: 1=01, '1'=1, (1)=(1), unbalanced-quote fragments and MySQL $x$ identifiers.

Already handled

• The test file asserts MaxScanInputSize == tokenizer.MaxInputSize, so the duplicated constant can't drift unnoticed.
• Test coverage is strong: audit repros, differential fuzzing, the exact 10 MiB boundary, a linear-time ratio and false-positive guards. The only gap is a worst-case peak-memory test, which would cover item 1.

Verdict: Fine to merge once item 1 is either addressed or explicitly accepted. Fixing the comment in item 2 is a cheap change. The rest is optional.