315: MSSQL spike: candidate A guard using node-sql-parser #337

Merged

david merged 2 commits from feature/issue-315/mssql-spike-candidate-a-guard-using-node-sql-parse into main

2026-11-10 22:59:36 +00:00

david commented

2026-11-10 22:59:35 +00:00

Owner

315: MSSQL spike: candidate A guard using node-sql-parser

Implements 315.

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 1m 22s.

Review findings

Review Findings — 315

Summary

I fixed all six findings and the test suite passes. node --test extensions/mssql/spike/*.test.mjs ran 34 tests with 0 failures and 0 skipped. There is no build or lint script. The skip path for a missing node-sql-parser was not exercised, because the package is installed locally.

Dependency declaration: candidate-a.test.mjs now skips with a clear message when node-sql-parser is missing. I pinned the version at 5.4.0 in the install comment and the skip message. dependency-check.md now records the version, licence and install step.
Blocklist gaps: I documented the gaps and the read-only-login recommendation in dependency-check.md and did not extend the blocklist. The UNION ... SELECT ... INTO rejection now has a test.
Verification still needed: the package's maintenance status and npm audit output were not checked in this pass. dependency-check.md flags both as open.

Critical

High

Medium

extensions/mssql/spike/candidate-a.mjs:5 — node-sql-parser is imported but not declared anywhere, because the spike deliberately uses npm install --no-save. The root npm test glob (extensions/mssql/spike/*.test.mjs) now includes this test, so it fails with a module-not-found error on any clean checkout or CI run that has not run the extra install. Suggested fix: record the extra install step in extensions/mssql/spike/ci-coverage.md or dependency-check.md and in the CI config, or have the test skip with a clear message when the module is missing. [BEST PRACTICE] Also record the package's version, licence and maintenance status in the dependency check, since it is a supply-chain addition. [SECURITY RISK] This matters more because the parser underpins a security guard.
extensions/mssql/spike/candidate-a.mjs:8 — The forbidden-name list is a blocklist. Manual probes show SELECT 1 FOR XML PATH, CROSS APPLY OPENJSON(...) and table hints such as WITH (NOLOCK) are allowed, and so is OPENJSON, which is harmless in most cases. Other constructs are not checked either, including OPENXML, xp_/sp_ function-style calls, NEXT VALUE FOR sequence, which advances a sequence, and user-defined scalar functions. Suggested fix: document these as known gaps in the spike's findings, or add them to the corpus. Run the check at the database level too, using a least-privilege read-only login as the primary control and this parser guard as defence in depth. [SECURITY RISK]
extensions/mssql/spike/candidate-a.mjs:23 — hasIntoTarget relies on the parser's AST shape (into.type or into.expr, with { position: null } when absent). If a parser upgrade changes this shape, SELECT INTO would be silently allowed. Suggested fix: pin the node-sql-parser version for the spike, and keep the selectInto and selectIntoInCte tests as regression guards. Add a UNION ... SELECT ... INTO test, which I confirmed manually is rejected.

Low

extensions/mssql/spike/candidate-a.test.mjs:30 — The rejection tests only assert that reason is truthy, so a case could be rejected for the wrong reason, for example exec failing as a parse error. Suggested fix: assert on the specific reason, or a reason substring, per case.
extensions/mssql/spike/candidate-a.test.mjs:12 — Missing tests for allowed and rejected edge cases: a trailing semicolon (SELECT 1;), comments hiding a second statement, bracket-quoted or schema-qualified names ([OPENROWSET], master..OPENROWSET), OPENDATASOURCE, and WAITFOR/DECLARE/SET batches. Suggested fix: add these cases, noting that the corpus files may already cover some.
extensions/mssql/spike/candidate-a.mjs:29 — FORBIDDEN_NAME is matched only against the value and table string properties. A forbidden function name stored under another key, such as name in a function node, would be missed. Suggested fix: confirm against the AST for OPENQUERY used as a function call, and widen the keys checked if needed.

## 315: MSSQL spike: candidate A guard using node-sql-parser Implements [315](https://git.excelera.net/david/pi-extensions-and-skills/issues/315). > 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 1m 22s. ## Review findings # Review Findings — 315 ## Summary I fixed all six findings and the test suite passes. `node --test extensions/mssql/spike/*.test.mjs` ran 34 tests with 0 failures and 0 skipped. There is no build or lint script. The skip path for a missing `node-sql-parser` was not exercised, because the package is installed locally. - **Dependency declaration:** `candidate-a.test.mjs` now skips with a clear message when `node-sql-parser` is missing. I pinned the version at 5.4.0 in the install comment and the skip message. `dependency-check.md` now records the version, licence and install step. - **Blocklist gaps:** I documented the gaps and the read-only-login recommendation in `dependency-check.md` and did not extend the blocklist. The `UNION ... SELECT ... INTO` rejection now has a test. - **Verification still needed:** the package's maintenance status and `npm audit` output were not checked in this pass. `dependency-check.md` flags both as open. ## Critical ## High ## Medium - [x] extensions/mssql/spike/candidate-a.mjs:5 — `node-sql-parser` is imported but not declared anywhere, because the spike deliberately uses `npm install --no-save`. The root `npm test` glob (`extensions/mssql/spike/*.test.mjs`) now includes this test, so it fails with a module-not-found error on any clean checkout or CI run that has not run the extra install. Suggested fix: record the extra install step in `extensions/mssql/spike/ci-coverage.md` or `dependency-check.md` and in the CI config, or have the test skip with a clear message when the module is missing. [BEST PRACTICE] Also record the package's version, licence and maintenance status in the dependency check, since it is a supply-chain addition. [SECURITY RISK] This matters more because the parser underpins a security guard. - [x] extensions/mssql/spike/candidate-a.mjs:8 — The forbidden-name list is a blocklist. Manual probes show `SELECT 1 FOR XML PATH`, `CROSS APPLY OPENJSON(...)` and table hints such as `WITH (NOLOCK)` are allowed, and so is `OPENJSON`, which is harmless in most cases. Other constructs are not checked either, including `OPENXML`, `xp_`/`sp_` function-style calls, `NEXT VALUE FOR sequence`, which advances a sequence, and user-defined scalar functions. Suggested fix: document these as known gaps in the spike's findings, or add them to the corpus. Run the check at the database level too, using a least-privilege read-only login as the primary control and this parser guard as defence in depth. [SECURITY RISK] - [x] extensions/mssql/spike/candidate-a.mjs:23 — `hasIntoTarget` relies on the parser's AST shape (`into.type` or `into.expr`, with `{ position: null }` when absent). If a parser upgrade changes this shape, `SELECT INTO` would be silently allowed. Suggested fix: pin the `node-sql-parser` version for the spike, and keep the `selectInto` and `selectIntoInCte` tests as regression guards. Add a `UNION ... SELECT ... INTO` test, which I confirmed manually is rejected. ## Low - [x] extensions/mssql/spike/candidate-a.test.mjs:30 — The rejection tests only assert that `reason` is truthy, so a case could be rejected for the wrong reason, for example `exec` failing as a parse error. Suggested fix: assert on the specific reason, or a reason substring, per case. - [x] extensions/mssql/spike/candidate-a.test.mjs:12 — Missing tests for allowed and rejected edge cases: a trailing semicolon (`SELECT 1;`), comments hiding a second statement, bracket-quoted or schema-qualified names (`[OPENROWSET]`, `master..OPENROWSET`), `OPENDATASOURCE`, and `WAITFOR`/`DECLARE`/`SET` batches. Suggested fix: add these cases, noting that the corpus files may already cover some. - [x] extensions/mssql/spike/candidate-a.mjs:29 — `FORBIDDEN_NAME` is matched only against the `value` and `table` string properties. A forbidden function name stored under another key, such as `name` in a function node, would be missed. Suggested fix: confirm against the AST for `OPENQUERY` used as a function call, and widen the keys checked if needed.

david added 2 commits

2026-11-10 22:59:35 +00:00