316: MSSQL spike: candidate B guard using the ANTLR4 T-SQL grammar #338

Merged

david merged 2 commits from feature/issue-316/mssql-spike-candidate-b-guard-using-the-antlr4-t-s into main

2026-11-10 23:06:50 +00:00

david commented

2026-11-10 23:06:49 +00:00

Owner

316: MSSQL spike: candidate B guard using the ANTLR4 T-SQL grammar

Implements 316.

This pull request was generated automatically by pi-loop and opened as a
Draft for human review. Do not merge without reviewing the changes.

Generated in 6m 29s.

Review findings

SHA-1 matches Maven's published value, so I'll pin the SHA-256.# Review Findings — 316

Summary

I fixed the Critical finding and four of the High, Medium and Low findings. Two findings remain.

Critical: the guard now rejects NEXT VALUE FOR and the lock-taking hints (XLOCK, UPDLOCK, HOLDLOCK, TABLOCKX, PAGLOCK). A header comment in candidate-b.mjs records that an allowlist is needed on top of the grammar, and that the guard must be paired with a read-only DB role.
High (rejected-case tests): I added tests for each new rejection, including known-gap tests.
High (candidate-b.mjs:25): I chose to reject user-defined function calls and linked-server names, rather than accept them as residual risk. Both are covered by tests.
Medium (CI skip): a missing parser now fails the run when CI is set. ci-coverage.md also records this.
Medium (generate-grammar.sh): the SHA-256 is now pinned and mandatory, and the jar downloads to a mktemp path. I computed the hash from the jar downloaded from Maven Central; its SHA-1 matches the SHA-1 Maven publishes.
Medium (input bounds): I added a 10,000-character input limit and a 200-deep nesting test with a 5-second budget. I did not record parse-time measurements in a spike results document. No such document exists in the repo.
Low (header comment): the header now matches the actual npm test wiring.
Low (;;): I documented the tolerance with an allowed-case test.

Remaining findings:

SOURCE.md date: the system clock and git history both show 11 November 2026, so the date is not in the future, and I left it. The upstream MIT LICENSE file is still unverified, and I could not do that offline.
Parse-time measurements: these are not recorded (see the Medium item above).

Gates:

Spike tests: candidate-b.test.mjs passes 42 of 42, also with CI=1.
Root npm test: one failure, "direct invocation exits 0 and prints the Step-4 reminder". It also fails with my changes stashed, so it is unrelated.
Build and lint: the root package.json has no build or lint script, so I ran neither.

Critical

extensions/mssql/spike/candidate-b.mjs:24 — findForbiddenConstruct allows state-mutating or lock-taking constructs inside a plain SELECT. Probing confirmed SELECT NEXT VALUE FOR dbo.seq returns allowed: true, and that call consumes a sequence value, which is a write. WITH (XLOCK, HOLDLOCK) table hints are also allowed and can hold locks for a long time. As a "read-only guard" candidate this is a real bypass, so the spike's findings must record it. Suggested fix: reject the NEXT_VALUE_FOR built-in function context (TSqlParser.NEXT_VALUE_FORContext) and the lock-taking table hints (XLOCK, UPDLOCK, HOLDLOCK, TABLOCKX, PAGLOCK). Add rejection tests for each. Document in the spike results that an allowlist on top of the grammar is needed, and that the guard must be paired with a read-only DB role (least privilege).

High

extensions/mssql/spike/candidate-b.mjs:25 — The forbidden-name check is a hard-coded denylist (OPENROWSET, OPENQUERY, OPENDATASOURCE) applied only to terminal token text. Other data-exfiltration or side-effect routes through the grammar's function_call rule are unchecked. Examples are user-defined scalar functions such as dbo.f(1), which was allowed in testing, and four-part linked-server names such as srv.db.dbo.t, which was allowed. Neither is covered by a test. Suggested fix: decide explicitly whether four-part names and unqualified UDF calls are in scope, then reject them (Full_table_nameContext with a linkedServer or server part, and SCALAR_FUNCTIONContext) or document them as accepted residual risk. Add tests either way.
extensions/mssql/spike/candidate-b.test.mjs:34 — The rejected-case set has no tests for the bypasses above: NEXT VALUE FOR, lock hints, linked-server names, UDF calls, OPTION (...) and FOR XML. The only-allowed cases are five trivial SELECTs. Suggested fix: add allowed cases for OPTION, FOR XML PATH, OPENJSON and ;;, and rejected cases for each construct the guard is meant to block. Record any known-allowed risky cases as explicit "known gap" tests.
extensions/mssql/spike/grammar/SOURCE.md:5 — The commit date reads "11 November 2026", which is in the future and so is almost certainly wrong. The note also admits the upstream LICENSE file was not verified, and the vendored grammar carries Copyright headers from five authors. Suggested fix: correct the date from the actual git history. Confirm the MIT licence in the upstream repository, and add a licence notice before any adoption.

Medium

extensions/mssql/spike/candidate-b.test.mjs:7 — If antlr4 or generated/ is missing (a clean checkout, since generated/ is git-ignored), all 26 tests are skipped, and the wired-in npm test still exits 0. A CI run could therefore report green without exercising the guard. Suggested fix: fail when a CI environment variable is set, or have the spike's CI notes (ci-coverage.md) state explicitly that these tests are skipped without the generated parser.
extensions/mssql/spike/generate-grammar.sh:8 — The ANTLR jar is downloaded over the network and the SHA-256 check is optional (ANTLR_JAR_SHA256 defaults to empty), so by default there is no integrity check on a build-time supply-chain artefact. The curl also writes to a predictable path under /tmp. Suggested fix: pin the known SHA-256 in the script and make the check mandatory. Download to a mktemp path.
extensions/mssql/spike/candidate-b.mjs:20 — Parser errors use ANTLR's default recovery, so a generic parse failure is reported only through the listener. Pathological inputs (deeply nested parentheses) could cause very deep recursion or high CPU, and the guard has no input-length limit or timeout. A stack overflow is caught by the try block, but CPU time is not bounded. Suggested fix: add a maximum SQL length and a basic nesting-depth test, and record the parse-time measurements in the spike results.

Low

extensions/mssql/spike/candidate-b.mjs:36 — The helper isTerminal is used for the semicolon count, so the check semicolons.length > 1 is not clearly correct. Probing showed SELECT 1;; is allowed, because select_statement absorbs one ; and sql_clauses can absorb another. This is harmless but inconsistent with the "exactly one statement" intent. Suggested fix: either document this tolerance with a test, or reject it.
extensions/mssql/spike/candidate-b.mjs:4 — The header comment says "root package.json must stay unchanged", yet npm test in the root package.json now includes extensions/mssql/spike/*.test.mjs. Suggested fix: update the comment so it matches the actual state, or restrict that claim to dependencies.

## 316: MSSQL spike: candidate B guard using the ANTLR4 T-SQL grammar Implements [316](https://git.excelera.net/david/pi-extensions-and-skills/issues/316). > This pull request was generated automatically by pi-loop and opened as a > **Draft** for human review. Do not merge without reviewing the changes. Generated in 6m 29s. ## Review findings SHA-1 matches Maven's published value, so I'll pin the SHA-256.# Review Findings — 316 ## Summary I fixed the Critical finding and four of the High, Medium and Low findings. Two findings remain. - **Critical:** the guard now rejects `NEXT VALUE FOR` and the lock-taking hints (`XLOCK`, `UPDLOCK`, `HOLDLOCK`, `TABLOCKX`, `PAGLOCK`). A header comment in `candidate-b.mjs` records that an allowlist is needed on top of the grammar, and that the guard must be paired with a read-only DB role. - **High (rejected-case tests):** I added tests for each new rejection, including known-gap tests. - **High (`candidate-b.mjs:25`):** I chose to reject user-defined function calls and linked-server names, rather than accept them as residual risk. Both are covered by tests. - **Medium (CI skip):** a missing parser now fails the run when `CI` is set. `ci-coverage.md` also records this. - **Medium (`generate-grammar.sh`):** the SHA-256 is now pinned and mandatory, and the jar downloads to a `mktemp` path. I computed the hash from the jar downloaded from Maven Central; its SHA-1 matches the SHA-1 Maven publishes. - **Medium (input bounds):** I added a 10,000-character input limit and a 200-deep nesting test with a 5-second budget. I did not record parse-time measurements in a spike results document. No such document exists in the repo. - **Low (header comment):** the header now matches the actual `npm test` wiring. - **Low (`;;`):** I documented the tolerance with an allowed-case test. Remaining findings: - **`SOURCE.md` date:** the system clock and git history both show 11 November 2026, so the date is not in the future, and I left it. The upstream MIT `LICENSE` file is still unverified, and I could not do that offline. - **Parse-time measurements:** these are not recorded (see the Medium item above). Gates: - **Spike tests:** `candidate-b.test.mjs` passes 42 of 42, also with `CI=1`. - **Root `npm test`:** one failure, "direct invocation exits 0 and prints the Step-4 reminder". It also fails with my changes stashed, so it is unrelated. - **Build and lint:** the root `package.json` has no build or lint script, so I ran neither. ## Critical - [x] extensions/mssql/spike/candidate-b.mjs:24 — `findForbiddenConstruct` allows state-mutating or lock-taking constructs inside a plain SELECT. Probing confirmed `SELECT NEXT VALUE FOR dbo.seq` returns `allowed: true`, and that call consumes a sequence value, which is a write. `WITH (XLOCK, HOLDLOCK)` table hints are also allowed and can hold locks for a long time. As a "read-only guard" candidate this is a real bypass, so the spike's findings must record it. Suggested fix: reject the `NEXT_VALUE_FOR` built-in function context (`TSqlParser.NEXT_VALUE_FORContext`) and the lock-taking table hints (`XLOCK`, `UPDLOCK`, `HOLDLOCK`, `TABLOCKX`, `PAGLOCK`). Add rejection tests for each. Document in the spike results that an allowlist on top of the grammar is needed, and that the guard must be paired with a read-only DB role (least privilege). ## High - [x] extensions/mssql/spike/candidate-b.mjs:25 — The forbidden-name check is a hard-coded denylist (OPENROWSET, OPENQUERY, OPENDATASOURCE) applied only to terminal token text. Other data-exfiltration or side-effect routes through the grammar's `function_call` rule are unchecked. Examples are user-defined scalar functions such as `dbo.f(1)`, which was allowed in testing, and four-part linked-server names such as `srv.db.dbo.t`, which was allowed. Neither is covered by a test. Suggested fix: decide explicitly whether four-part names and unqualified UDF calls are in scope, then reject them (`Full_table_nameContext` with a `linkedServer` or `server` part, and `SCALAR_FUNCTIONContext`) or document them as accepted residual risk. Add tests either way. - [x] extensions/mssql/spike/candidate-b.test.mjs:34 — The rejected-case set has no tests for the bypasses above: `NEXT VALUE FOR`, lock hints, linked-server names, UDF calls, `OPTION (...)` and `FOR XML`. The only-allowed cases are five trivial SELECTs. Suggested fix: add allowed cases for `OPTION`, `FOR XML PATH`, `OPENJSON` and `;;`, and rejected cases for each construct the guard is meant to block. Record any known-allowed risky cases as explicit "known gap" tests. - [ ] extensions/mssql/spike/grammar/SOURCE.md:5 — The commit date reads "11 November 2026", which is in the future and so is almost certainly wrong. The note also admits the upstream LICENSE file was not verified, and the vendored grammar carries Copyright headers from five authors. Suggested fix: correct the date from the actual git history. Confirm the MIT licence in the upstream repository, and add a licence notice before any adoption. ## Medium - [x] extensions/mssql/spike/candidate-b.test.mjs:7 — If `antlr4` or `generated/` is missing (a clean checkout, since `generated/` is git-ignored), all 26 tests are skipped, and the wired-in `npm test` still exits 0. A CI run could therefore report green without exercising the guard. Suggested fix: fail when a CI environment variable is set, or have the spike's CI notes (`ci-coverage.md`) state explicitly that these tests are skipped without the generated parser. - [x] extensions/mssql/spike/generate-grammar.sh:8 — The ANTLR jar is downloaded over the network and the SHA-256 check is optional (`ANTLR_JAR_SHA256` defaults to empty), so by default there is no integrity check on a build-time supply-chain artefact. The `curl` also writes to a predictable path under `/tmp`. Suggested fix: pin the known SHA-256 in the script and make the check mandatory. Download to a `mktemp` path. - [ ] extensions/mssql/spike/candidate-b.mjs:20 — Parser errors use ANTLR's default recovery, so a generic parse failure is reported only through the listener. Pathological inputs (deeply nested parentheses) could cause very deep recursion or high CPU, and the guard has no input-length limit or timeout. A stack overflow is caught by the `try` block, but CPU time is not bounded. Suggested fix: add a maximum SQL length and a basic nesting-depth test, and record the parse-time measurements in the spike results. ## Low - [x] extensions/mssql/spike/candidate-b.mjs:36 — The helper `isTerminal` is used for the semicolon count, so the check `semicolons.length > 1` is not clearly correct. Probing showed `SELECT 1;;` is allowed, because `select_statement` absorbs one `;` and `sql_clauses` can absorb another. This is harmless but inconsistent with the "exactly one statement" intent. Suggested fix: either document this tolerance with a test, or reject it. - [x] extensions/mssql/spike/candidate-b.mjs:4 — The header comment says "root package.json must stay unchanged", yet `npm test` in the root `package.json` now includes `extensions/mssql/spike/*.test.mjs`. Suggested fix: update the comment so it matches the actual state, or restrict that claim to dependencies.

david added 2 commits

2026-11-10 23:06:49 +00:00