From 9c281e840862ee912693e94b7e920f1cf550ceb1 Mon Sep 17 00:00:00 2001 From: Chii Date: Tue, 8 Sep 2026 21:47:01 +0000 Subject: [PATCH 01/13] v1.2.1: three shipped service-role defects, a regression caught before it shipped, and four more MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All reproduced before fixing, all carrying fixtures, none taken on report alone. THREE DEFECTS SHIPPED IN v1.1.0 AND v1.2.0, all from one root cause plus one blind spot: - resolve() never canonicalised the path it returned, so two spellings of one file were two graph nodes. A circular import a<->b grew a longer spelling every hop, the visited set never matched, and the walk DID NOT TERMINATE: measured at 20s with zero output before a timeout killed it. In CI that is a hang, which reports nothing at all — the worst available outcome for a gate. - Same cause, quieter: a file reached as `@/lib/x` and as `../../lib/x` printed two findings, each claiming "1 request-path module(s)". That is the exact miscount the comment in this file says the edge-list rewrite fixed. The rewrite fixed seed-carrying. The comment claimed more than the fix did. - A multi-line `await import(` was neither followed nor reported UNKNOWN, because the guard and the extractor were both single-line greps. Measured: `ok`, exit 0, over a page reaching SUPABASE_SERVICE_ROLE_KEY. A false green in the gate whose whole job is that key, from a line break Prettier inserts on its own. AND A REGRESSION I INTRODUCED FIXING THE THIRD. Flattening the file for the scan made `grep -o`, which matches non-overlapping, swallow a real import when a string ends in `from "`. Measured `ok`, exit 0, on a tree the previous version caught — a false green created by the change meant to remove one. The scan is now the union of a per-line pass and a flat pass, and the shape has a fixture. Four more, each verified: - tsconfig `extends` is not followed, so aliases in a base config left the table empty and an aliased import was mistaken for a published package. Reported UNKNOWN now rather than skipped; following the chain is a real change, saying so is not. - `.d.ts`, `.mts`, `.cts` were missing from the extension list, making `import type { Database } from "@/types/supabase"` a permanent blocking UNKNOWN. The selftest cannot catch a false red — its own note says the fixture model holds bad trees only — so it took a reader. - review.yml posted the tally on `always()`, so every run cancelled by cancel-in-progress left a spurious "UNKNOWN — the reviewer did not report a count". `success() || failure()` now: unmissable when it ran, silent when it was killed on purpose. - migrations-lint used `\s`, a GNU-grep extension. On BSD grep `create\s+table` matches nothing, so the RLS rule silently checked no tables at all on macOS. 18 occurrences, now [[:space:]]. The caller template gains the permissions block the reviewer needs: a called workflow can only narrow the caller's token, so review.yml's own permissions are a ceiling, not a grant, and without this the tally 403s on a read-only default. Two honest notes rather than quiet patches. The template pins the previous release by construction and the previous release is v1.2.0, which this README now marks do-not-use — the first time the convention has shipped a pin its own table contradicts, said out loud because the convention needs a decision. And the README's call snippet showed 25a1ca1 / v1.0.3 for three releases after it stopped being current; it now carries a placeholder, which is the one place a placeholder beats a real hash — a stale SHA gets copied, a placeholder cannot be. Found by running the local review round before pushing, which I had stopped doing. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BfFq2KvV1CCkPs5WnV1AsA --- .github/workflows/review.yml | 16 +++- CITATION.cff | 2 +- README.md | 35 +++++++-- caller-template.yml | 49 ++++++++---- .../cases/aliased-duplicate-path/package.json | 4 + .../aliased-duplicate-path/src/app/page.tsx | 2 + .../aliased-duplicate-path/src/app/x/page.tsx | 2 + .../aliased-duplicate-path/src/lib/secret.ts | 1 + .../aliased-duplicate-path/tsconfig.json | 6 ++ .../bad/cases/circular-import/package.json | 4 + .../cases/circular-import/src/app/page.tsx | 2 + .../bad/cases/circular-import/src/lib/a.ts | 2 + .../bad/cases/circular-import/src/lib/b.ts | 4 + .../cases/string-ending-in-from/package.json | 4 + .../string-ending-in-from/src/app/page.tsx | 9 +++ .../string-ending-in-from/src/lib/secret.ts | 1 + .../multiline-dynamic-import/package.json | 4 + .../multiline-dynamic-import/src/app/page.tsx | 8 ++ gates/migrations-lint.sh | 6 +- gates/service-role.sh | 75 +++++++++++++++++-- 20 files changed, 204 insertions(+), 32 deletions(-) create mode 100644 fixtures/service-role/bad/cases/aliased-duplicate-path/package.json create mode 100644 fixtures/service-role/bad/cases/aliased-duplicate-path/src/app/page.tsx create mode 100644 fixtures/service-role/bad/cases/aliased-duplicate-path/src/app/x/page.tsx create mode 100644 fixtures/service-role/bad/cases/aliased-duplicate-path/src/lib/secret.ts create mode 100644 fixtures/service-role/bad/cases/aliased-duplicate-path/tsconfig.json create mode 100644 fixtures/service-role/bad/cases/circular-import/package.json create mode 100644 fixtures/service-role/bad/cases/circular-import/src/app/page.tsx create mode 100644 fixtures/service-role/bad/cases/circular-import/src/lib/a.ts create mode 100644 fixtures/service-role/bad/cases/circular-import/src/lib/b.ts create mode 100644 fixtures/service-role/bad/cases/string-ending-in-from/package.json create mode 100644 fixtures/service-role/bad/cases/string-ending-in-from/src/app/page.tsx create mode 100644 fixtures/service-role/bad/cases/string-ending-in-from/src/lib/secret.ts create mode 100644 fixtures/service-role/bad/unknown/multiline-dynamic-import/package.json create mode 100644 fixtures/service-role/bad/unknown/multiline-dynamic-import/src/app/page.tsx diff --git a/.github/workflows/review.yml b/.github/workflows/review.yml index 4307ecd..6d1f406 100644 --- a/.github/workflows/review.yml +++ b/.github/workflows/review.yml @@ -38,7 +38,14 @@ permissions: id-token: write concurrency: - group: review-${{ github.event.pull_request.number }} + # The fallback is not decoration. `github.event.pull_request.number` is EMPTY on any event + # that is not a pull request, and the caller template triggers on `[pull_request, push]`, so + # every push run collapsed into one shared group key of `review-` and cancelled the previous + # one. Harmless only for as long as this job stays skipped on push — which is a property of + # the `if` below, not of the group, and the two are not written down together anywhere else. + # A concurrency key that silently means "all of them" is the same shape as a check that + # silently means "nothing". + group: review-${{ github.event.pull_request.number || github.ref }} cancel-in-progress: true jobs: @@ -132,7 +139,12 @@ jobs: # finishing. If the count file is missing or unparseable the run says UNKNOWN and goes RED: # there is no third colour, and a reviewer that died is not a clean bill of health. - name: Post the tally - if: always() + # success() || failure(), NOT always(). With cancel-in-progress on, a superseded run is + # CANCELLED, and always() runs this step there too — so every push to a draft PR left a + # spurious "UNKNOWN — the reviewer did not report a count" comment from the run that was + # deliberately killed. The tally must be unmissable when the reviewer ran; it must not + # narrate a run that was cancelled on purpose. + if: success() || failure() env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} PR: ${{ github.event.pull_request.number }} diff --git a/CITATION.cff b/CITATION.cff index 2d7a3d5..97f9ad0 100644 --- a/CITATION.cff +++ b/CITATION.cff @@ -10,7 +10,7 @@ authors: repository-code: "https://github.com/chiibitsu/gates" url: "https://chiibitsu.com" license: MIT -version: "1.2.0" +version: "1.2.1" date-released: "2026-09-08" keywords: - ci diff --git a/README.md b/README.md index a953300..7ff0889 100644 --- a/README.md +++ b/README.md @@ -137,6 +137,12 @@ sends someone hunting for a bug in their code rather than a blind spot in the ga ## How to call it +This snippet carries `` rather than a real hash, and that is the +one place in this repository where a placeholder is right: it showed `25a1ca1` / v1.0.3 for +three releases after that pin stopped being current, and a stale real SHA is copied without +hesitation while a placeholder cannot be. `caller-template.yml` ships a real SHA because it has +to run; this prose does not. + Copy `caller-template.yml` into your repo as `.github/workflows/gates.yml`. It ships a real SHA rather than a placeholder, and that SHA is the **previous** release — replace both occurrences with the one you want from the Releases table below: @@ -145,9 +151,22 @@ occurrences with the one you want from the Releases table below: on: [pull_request, push] jobs: gates: - uses: chiibitsu/gates/.github/workflows/gates.yml@25a1ca1f7ad7875138dd943657a5f7f1a3aa1809 # v1.0.3 + uses: chiibitsu/gates/.github/workflows/gates.yml@ # v1.2.1 with: - gates_ref: 25a1ca1f7ad7875138dd943657a5f7f1a3aa1809 # v1.0.3 + gates_ref: # v1.2.1 + + # The draft-stage reviewer. Needs a repository secret CLAUDE_CODE_OAUTH_TOKEN, and the + # permissions block because a called workflow can only narrow the caller's token, never + # widen it — without it the tally step 403s on a read-only default. + review: + permissions: + contents: read + pull-requests: write + issues: read + id-token: write + uses: chiibitsu/gates/.github/workflows/review.yml@ # v1.2.1 + secrets: + CLAUDE_CODE_OAUTH_TOKEN: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} ``` Pin a SHA, not a tag — that is the same rule `gates/actions-sha-pinned.sh` enforces on @@ -233,7 +252,7 @@ Five live in the repo being checked. The sixth belongs to the template, not here ## Releases -The Commit column names a **tag** for the current release and a SHA for superseded ones, and that asymmetry is forced: the table lives in the commit being tagged, so it cannot contain that commit's own hash. Resolve the tag — `git rev-list -n1 v1.2.0` — and pin the SHA you get. Pin a SHA, never a tag: a tag is a movable name, and this table exists because names have been wrong here before. +The Commit column names a **tag** for the current release and a SHA for superseded ones, and that asymmetry is forced: the table lives in the commit being tagged, so it cannot contain that commit's own hash. Resolve the tag — `git rev-list -n1 v1.2.1` — and pin the SHA you get. Pin a SHA, never a tag: a tag is a movable name, and this table exists because names have been wrong here before. That rule applies to everything this repo pins, not only to itself, and the reviewer's action is the measured case. On 2026-09-08, `anthropics/claude-code-action` resolved as: @@ -248,8 +267,9 @@ A release's `CITATION.cff` names its own version — that is the part that must | Version | Commit | Use it? | |---|---|---| -| **v1.2.0** | tag `v1.2.0` — resolve with `git rev-list -n1 v1.2.0`, or read it off the release page | **Yes — use this one.** Adds the draft-stage reviewer: `REVIEW.md`, `.github/workflows/review.yml`, and `fixtures/review/bad/`. **The gates are byte-identical to v1.1.0** — nothing that blocks changed, and a consumer that adopts this without wiring the reviewer gets exactly v1.1.0's behaviour. Carries v1.0.2's known false green in `actions-sha-pinned.sh`, described below. | -| v1.1.0 | `7832ea6` | **Usable, and superseded by v1.2.0** — same six gates, no reviewer. Added `service-role.sh`, the UNKNOWN outcome, and `gates/MANIFEST.txt` with a both-directions check against the `gates/` directory. Adopting it can turn a consumer red on code that was green under v1.0.4, because `service-role.sh` did not exist to check it; that is a finding, not a regression. | +| **v1.2.1** | tag `v1.2.1` — resolve with `git rev-list -n1 v1.2.1`, or read it off the release page | **Yes — use this one.** Fixes three defects in `service-role.sh` that v1.1.0 and v1.2.0 both shipped, each reproduced before the fix and each carrying a fixture: an ordinary circular import **did not terminate** (a hang, which reports nothing at all); one file reached by two spellings was counted twice, printing two findings each claiming "1 request-path module(s)"; and a multi-line `await import(` was a **false green** — `ok`, exit 0, over a page reaching `SUPABASE_SERVICE_ROLE_KEY`. Also fixes a concurrency key in `review.yml` that collapsed on non-PR events. Carries v1.0.2's known false green in `actions-sha-pinned.sh`, described below. | +| v1.2.0 | `44f6125` | **Do not use.** Its `service-role.sh` is byte-identical to v1.1.0's and carries all three defects listed under v1.2.1 — including a false green on a multi-line dynamic import and a walk that does not terminate on a circular one. The reviewer it adds is sound; the gate underneath it is not. Move to v1.2.1. | +| v1.1.0 | `7832ea6` | **Do not use** — same three `service-role.sh` defects as v1.2.0, described in the v1.2.1 row. Six gates, no reviewer. Added `service-role.sh`, the UNKNOWN outcome, and `gates/MANIFEST.txt` with a both-directions check against the `gates/` directory. Adopting it can turn a consumer red on code that was green under v1.0.4, because `service-role.sh` did not exist to check it; that is a finding, not a regression. | | v1.0.4 | `e075a93` | **Usable, and superseded by v1.1.0** — five gates instead of six, so nothing checks whether the request path can reach the service-role secret. Its `CITATION.cff` names the version on its tag, which v1.0.0 and v1.0.1 also did and v1.0.2 and v1.0.3 did not. Gates byte-identical to v1.0.2 and v1.0.3, so it carries their one known false green, described below. | | v1.0.3 | `25a1ca1` | **Do not cite.** Its gates are correct and identical to v1.0.2's, so a pin at this SHA works. But its `CITATION.cff` says `1.0.2` and its caller template says `v1.0.2` — this tag reproduces the exact defect it was cut to fix. The cause is structural and is the useful part: the correction to a version's metadata is *made by* the pull request, so it lands in a commit **after** the one being tagged. Tagging the merged head of the PR that reports the previous version is guaranteed to be one behind. A release has to declare its own version **before** it is tagged, which is what v1.0.4 does. | | v1.0.2 | `c3e3f49` | **Usable, and superseded by v1.0.4** — same gates, wrong version metadata inside the tag. Everything six review rounds found in v1.0.0 and v1.0.1 is fixed here, each fix carrying the minimal fixture that proves the shape is still caught. It has **one known false green**, reproducible: `steps: [{uses: a/b@main}, {uses: c/d@v1}]` reports ok. The rule that produces it is stated once, in Known gaps below, and deliberately not paraphrased here — a first draft of this row paraphrased it and got the rule wrong in a different way than the gap section did, which is how two statements of one fact always end. Treat this release as a first line, never as the only one. An earlier draft of this row claimed no known false green while the gap below already described one — the claim was wrong, and it is corrected here rather than quietly dropped. | @@ -276,6 +296,11 @@ header, which is the honest account of what that scanner does not do. here and is called from consumer repositories; wiring a self-call is a separate change, so until then the fixture procedure is the only thing that exercises it. Stated because a reviewer nobody has run is a reviewer nobody has seen fail. +- **The service-role gate scans each module with newlines collapsed**, so that a statement + spanning lines is seen as one — a multi-line `await import(` was a false green before + v1.2.1. The cost is that a `//` comment runs into the code after it, so `import(` mentioned + inside a comment can raise a spurious UNKNOWN. Over-inclusive, and that is the direction + this toolkit errs in: a visible red someone can argue with, never a green nobody questions. - **The service-role gate reads imports with a regex, not a parser.** It follows literal `from`, `import` and `require` specifiers, resolving `./`, `../`, absolute paths, and **every alias declared in tsconfig `compilerOptions.paths`** — not just `@/*`. A specifier diff --git a/caller-template.yml b/caller-template.yml index bb36123..9a58260 100644 --- a/caller-template.yml +++ b/caller-template.yml @@ -3,6 +3,14 @@ # purpose: a reusable workflow cannot discover its own commit, so the ref you pin has to be # handed to it. Change both together. # +# READ THIS BEFORE COPYING: the SHA shipped below is v1.2.0, and the Releases table in the +# README marks v1.2.0 **do not use** — its service-role gate has a false green on a multi-line +# dynamic import and does not terminate on a circular one. The template pins the PREVIOUS +# release by construction, and the previous release is the bad one, so this is the first time +# the convention has shipped a pin the table contradicts. TAKE v1.2.1 FROM THE TABLE. The +# convention is left visible rather than quietly patched because it is the convention that +# needs a decision, not this line. +# # It does NOT name the release you are reading, and cannot: a commit cannot contain its own # SHA. What it does name is not derivable — from v1.0.2 onward it is the previous release, but # v1.0.0 and v1.0.1 pinned untagged ancestors, and v1.0.1 labelled its untagged pin with its @@ -13,27 +21,36 @@ on: [pull_request, push] jobs: gates: - uses: chiibitsu/gates/.github/workflows/gates.yml@7832ea67355f22a3e9077d061f1dd937cead905a # v1.1.0 + uses: chiibitsu/gates/.github/workflows/gates.yml@44f6125815853021fcc4c8c96f5e2305cd4c4313 # v1.2.0 with: - gates_ref: 7832ea67355f22a3e9077d061f1dd937cead905a # v1.1.0 + gates_ref: 44f6125815853021fcc4c8c96f5e2305cd4c4313 # v1.2.0 # ── The draft-stage reviewer (Tier 2, not a gate) ──────────────────────────────────────── # - # Commented, and NOT because it is a placeholder. review.yml does not exist before v1.2.0, so - # pinning it at the SHA above — the previous release, which is what this template ships — - # would fail with a file-not-found on every run. A caller line that is broken by construction - # is worse than one that says why it is off. + # ACTIVE from v1.2.1 onward. It shipped commented in v1.2.0 for a reason that has since + # stopped being true: review.yml did not exist at the release this template pins, so an + # active line would have failed file-not-found on every run. review.yml exists at v1.2.0, + # which is what the pin below now names, so the block is live. A comment kept past the + # expiry of its own reason is how a file starts lying quietly. # - # To turn it on: take the v1.2.0 SHA (or later) from the Releases table in the README, put it - # on the `uses:` line below and on both `gates` lines above, and uncomment. There is ONE SHA - # in this block — the other line is the secret mapping, and an earlier wording said "BOTH - # lines below", which would have had a careful reader overwrite the token with a commit hash. - # It needs a repository secret CLAUDE_CODE_OAUTH_TOKEN, from `claude setup-token`. + # It needs a repository secret CLAUDE_CODE_OAUTH_TOKEN, from `claude setup-token`. Without + # it the job fails loudly rather than reviewing nothing. # # It runs ONLY while the pull request is a draft. On a ready PR the job reports `skipped`, - # which is an absence, not a pass — see AGENTS.md on accounting for a check that is not there. + # which is an absence, not a pass — account for it before flipping, as AGENTS.md says. # - # review: - # uses: chiibitsu/gates/.github/workflows/review.yml@ - # secrets: - # CLAUDE_CODE_OAUTH_TOKEN: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} + # ONE SHA in this block, on the `uses:` line. The line under `secrets:` is a token mapping. + review: + # PERMISSIONS ARE THE CALLER'S TO GRANT. A called workflow can only narrow the token it is + # handed, never widen it, so review.yml's own `permissions:` block is a ceiling and not a + # grant. On a repository whose default workflow token is read-only, the tally step's + # `gh pr comment` gets a 403 and the job is red on every draft PR — a reviewer that fails + # for a reason having nothing to do with the code. + permissions: + contents: read + pull-requests: write + issues: read + id-token: write + uses: chiibitsu/gates/.github/workflows/review.yml@44f6125815853021fcc4c8c96f5e2305cd4c4313 # v1.2.0 + secrets: + CLAUDE_CODE_OAUTH_TOKEN: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} diff --git a/fixtures/service-role/bad/cases/aliased-duplicate-path/package.json b/fixtures/service-role/bad/cases/aliased-duplicate-path/package.json new file mode 100644 index 0000000..655114d --- /dev/null +++ b/fixtures/service-role/bad/cases/aliased-duplicate-path/package.json @@ -0,0 +1,4 @@ +{ + "name": "fixture", + "dependencies": { "next": "15.0.0" } +} diff --git a/fixtures/service-role/bad/cases/aliased-duplicate-path/src/app/page.tsx b/fixtures/service-role/bad/cases/aliased-duplicate-path/src/app/page.tsx new file mode 100644 index 0000000..5c884ab --- /dev/null +++ b/fixtures/service-role/bad/cases/aliased-duplicate-path/src/app/page.tsx @@ -0,0 +1,2 @@ +import { key } from "@/lib/secret"; +export default function Page() { return
{key ? "y" : "n"}
; } diff --git a/fixtures/service-role/bad/cases/aliased-duplicate-path/src/app/x/page.tsx b/fixtures/service-role/bad/cases/aliased-duplicate-path/src/app/x/page.tsx new file mode 100644 index 0000000..4be6d0d --- /dev/null +++ b/fixtures/service-role/bad/cases/aliased-duplicate-path/src/app/x/page.tsx @@ -0,0 +1,2 @@ +import { key } from "../../lib/secret"; +export default function X() { return
{key ? "y" : "n"}
; } diff --git a/fixtures/service-role/bad/cases/aliased-duplicate-path/src/lib/secret.ts b/fixtures/service-role/bad/cases/aliased-duplicate-path/src/lib/secret.ts new file mode 100644 index 0000000..55bd353 --- /dev/null +++ b/fixtures/service-role/bad/cases/aliased-duplicate-path/src/lib/secret.ts @@ -0,0 +1 @@ +export const key = process.env.SUPABASE_SECRET_KEY; diff --git a/fixtures/service-role/bad/cases/aliased-duplicate-path/tsconfig.json b/fixtures/service-role/bad/cases/aliased-duplicate-path/tsconfig.json new file mode 100644 index 0000000..18254f7 --- /dev/null +++ b/fixtures/service-role/bad/cases/aliased-duplicate-path/tsconfig.json @@ -0,0 +1,6 @@ +{ + "compilerOptions": { + "baseUrl": ".", + "paths": { "@/*": ["./src/*"] } + } +} diff --git a/fixtures/service-role/bad/cases/circular-import/package.json b/fixtures/service-role/bad/cases/circular-import/package.json new file mode 100644 index 0000000..655114d --- /dev/null +++ b/fixtures/service-role/bad/cases/circular-import/package.json @@ -0,0 +1,4 @@ +{ + "name": "fixture", + "dependencies": { "next": "15.0.0" } +} diff --git a/fixtures/service-role/bad/cases/circular-import/src/app/page.tsx b/fixtures/service-role/bad/cases/circular-import/src/app/page.tsx new file mode 100644 index 0000000..b4be9e4 --- /dev/null +++ b/fixtures/service-role/bad/cases/circular-import/src/app/page.tsx @@ -0,0 +1,2 @@ +import { b } from "../lib/b"; +export default function Page() { return
{b ? "y" : "n"}
; } diff --git a/fixtures/service-role/bad/cases/circular-import/src/lib/a.ts b/fixtures/service-role/bad/cases/circular-import/src/lib/a.ts new file mode 100644 index 0000000..16c70c4 --- /dev/null +++ b/fixtures/service-role/bad/cases/circular-import/src/lib/a.ts @@ -0,0 +1,2 @@ +import { b } from "./b"; +export const a = b; diff --git a/fixtures/service-role/bad/cases/circular-import/src/lib/b.ts b/fixtures/service-role/bad/cases/circular-import/src/lib/b.ts new file mode 100644 index 0000000..12d8c0e --- /dev/null +++ b/fixtures/service-role/bad/cases/circular-import/src/lib/b.ts @@ -0,0 +1,4 @@ +// b imports a, a imports b. Before the paths were canonicalised this pair grew a longer +// spelling every hop, the visited set never matched, and the walk did not terminate. +import { a } from "./a"; +export const b = a ?? process.env.SUPABASE_SECRET_KEY; diff --git a/fixtures/service-role/bad/cases/string-ending-in-from/package.json b/fixtures/service-role/bad/cases/string-ending-in-from/package.json new file mode 100644 index 0000000..655114d --- /dev/null +++ b/fixtures/service-role/bad/cases/string-ending-in-from/package.json @@ -0,0 +1,4 @@ +{ + "name": "fixture", + "dependencies": { "next": "15.0.0" } +} diff --git a/fixtures/service-role/bad/cases/string-ending-in-from/src/app/page.tsx b/fixtures/service-role/bad/cases/string-ending-in-from/src/app/page.tsx new file mode 100644 index 0000000..4c47aa6 --- /dev/null +++ b/fixtures/service-role/bad/cases/string-ending-in-from/src/app/page.tsx @@ -0,0 +1,9 @@ +// The string ends in `from "`. With the whole file flattened to one line and grep -o +// matching non-overlapping, that span swallows the opening quote of the real import +// below it, and the edge to src/lib/secret.ts disappears. Caught as a regression before +// it shipped; kept here so it cannot come back. +const label = "imported from "; +import { key } from "../lib/secret"; +export default function Page() { + return
{label}{key ? "y" : "n"}
; +} diff --git a/fixtures/service-role/bad/cases/string-ending-in-from/src/lib/secret.ts b/fixtures/service-role/bad/cases/string-ending-in-from/src/lib/secret.ts new file mode 100644 index 0000000..f0c6e75 --- /dev/null +++ b/fixtures/service-role/bad/cases/string-ending-in-from/src/lib/secret.ts @@ -0,0 +1 @@ +export const key = process.env.SUPABASE_SERVICE_ROLE_KEY; diff --git a/fixtures/service-role/bad/unknown/multiline-dynamic-import/package.json b/fixtures/service-role/bad/unknown/multiline-dynamic-import/package.json new file mode 100644 index 0000000..655114d --- /dev/null +++ b/fixtures/service-role/bad/unknown/multiline-dynamic-import/package.json @@ -0,0 +1,4 @@ +{ + "name": "fixture", + "dependencies": { "next": "15.0.0" } +} diff --git a/fixtures/service-role/bad/unknown/multiline-dynamic-import/src/app/page.tsx b/fixtures/service-role/bad/unknown/multiline-dynamic-import/src/app/page.tsx new file mode 100644 index 0000000..ad6b140 --- /dev/null +++ b/fixtures/service-role/bad/unknown/multiline-dynamic-import/src/app/page.tsx @@ -0,0 +1,8 @@ +export default async function Page() { + // The specifier sits on its own line, which is what Prettier does to a long import(). + // A line-at-a-time scan sees neither the call nor the argument as one statement. + const mod = await import( + process.env.MODULE_NAME ?? "./safe" + ); + return
{String(mod.value)}
; +} diff --git a/gates/migrations-lint.sh b/gates/migrations-lint.sh index a2b34c6..c7e44d5 100755 --- a/gates/migrations-lint.sh +++ b/gates/migrations-lint.sh @@ -64,12 +64,12 @@ for up in "$MIG"/*.sql; do strip_sql_comments "$up" > "$STRIPPED" # tables created here must enable RLS here while IFS= read -r tbl; do - if ! grep -qiE "alter\s+table\s+(if\s+exists\s+)?(public\.)?\"?${tbl}\"?\s+enable\s+row\s+level\s+security" "$STRIPPED"; then + if ! grep -qiE "alter[[:space:]]+table[[:space:]]+(if[[:space:]]+exists[[:space:]]+)?(public\.)?\"?${tbl}\"?[[:space:]]+enable[[:space:]]+row[[:space:]]+level[[:space:]]+security" "$STRIPPED"; then fail "$(basename "$up"): table '$tbl' created without 'enable row level security' in the same file" fi - done < <(grep -ioE "create\s+table\s+(if\s+not\s+exists\s+)?(public\.)?\"?[a-z_][a-z0-9_]*" "$STRIPPED" | sed -E 's/.*[ .]"?([a-z_][a-z0-9_]*)"?$/\1/i') + done < <(grep -ioE "create[[:space:]]+table[[:space:]]+(if[[:space:]]+not[[:space:]]+exists[[:space:]]+)?(public\.)?\"?[a-z_][a-z0-9_]*" "$STRIPPED" | sed -E 's/.*[ .]"?([a-z_][a-z0-9_]*)"?$/\1/i') # destructive statements outside a WHERE are tier-3 by regex (non-negotiable 4); flag, do not block - if grep -qiE "^\s*(drop\s+table|truncate|delete\s+from\s+[a-z_.\"]+\s*;)" "$STRIPPED"; then + if grep -qiE "^[[:space:]]*(drop[[:space:]]+table|truncate|delete[[:space:]]+from[[:space:]]+[a-z_.\"]+[[:space:]]*;)" "$STRIPPED"; then echo "note [$GATE] $(basename "$up"): destructive statement present; this migration is tier-3" fi done diff --git a/gates/service-role.sh b/gates/service-role.sh index 6d6ea00..5ef24be 100755 --- a/gates/service-role.sh +++ b/gates/service-role.sh @@ -128,6 +128,12 @@ if [ -f "$TSCONFIG" ]; then grep -oE '"[^"]+"[[:space:]]*:[[:space:]]*\[[[:space:]]*"[^"]+"' "$WORK/pathsbody" 2>/dev/null \ | sed -E 's/"([^"]+)"[[:space:]]*:[[:space:]]*\[[[:space:]]*"([^"]+)"/\1\t\2/' > "$ALIASES" 2>/dev/null || true + # `extends` is not followed. A base config holding the aliases leaves $ALIASES empty, and an + # `@/lib/secret` then matches no alias, is called a published package, and is skipped — a + # false green. Following the chain is a real change; saying so is not. + if grep -q '"extends"' "$TSCONFIG" 2>/dev/null && [ ! -s "$ALIASES" ]; then + unknown "tsconfig.json uses \"extends\" and no alias was parsed from this file — the base config is not followed, so an aliased import here would be mistaken for a published package" + fi if [ -s "$WORK/pathsbody" ] && [ ! -s "$ALIASES" ]; then unknown "tsconfig.json declares compilerOptions.paths but this gate parsed no alias out of it — every aliased import below is therefore unresolved, and none of them will be called clean" fi @@ -226,7 +232,7 @@ done < "$SEEDS" # this repo's source" — and it is only safe to make about a specifier that matches NO # declared alias. Anything that looks local and does not resolve is 2, never 1. resolve() { # $1 = specifier, $2 = importing file - local spec="$1" bases="" matched=0 key target prefix t b ext cand found="" + local spec="$1" bases="" matched=0 key target prefix t b ext cand cdir found="" case "$spec" in ./*|../*) bases="$(dirname -- "$2")/$spec"; matched=1 ;; /*) bases="$ROOT$spec"; matched=1 ;; @@ -260,9 +266,33 @@ $BASE_DIR/$t" if [ "$matched" -ne 1 ]; then return 1; fi while IFS= read -r b; do [ -n "$b" ] || continue - for ext in "" .ts .tsx .js .jsx .mjs .cjs /index.ts /index.tsx /index.js /index.jsx; do + # .d.ts and friends included: `import type { Database } from "@/types/supabase"` against a + # src/types/supabase.d.ts resolved to nothing, which this gate calls UNKNOWN — a permanent + # blocking red on a perfectly ordinary line. The selftest cannot catch a false red (its own + # note says the fixture model holds bad trees only), so it is fixed here on report. + for ext in "" .ts .tsx .d.ts .mts .cts .js .jsx .mjs .cjs /index.ts /index.tsx /index.d.ts /index.js /index.jsx; do cand="$b$ext" - if [ -f "$cand" ]; then found="$cand"; break; fi + # CANONICALISED, not merely tested for existence. Without this the resolved path keeps + # whatever `..` the importer's specifier put in it, and TWO SPELLINGS OF ONE FILE ARE + # TWO NODES. That cost two defects, both shipped in v1.1.0: + # + # - a file imported as `@/lib/x` from one module and `../../lib/x` from another was + # counted twice: two findings for one file, each claiming "1 request-path + # module(s)". That is the very miscount the note above says the edge-list rewrite + # fixed. The rewrite fixed seed-carrying. It did not fix this, so the note claimed + # more than the fix delivered — in the comment about that exact defect. + # - an ordinary circular import (a.ts <-> b.ts) grew a longer spelling every hop, so + # the SEEN set never matched and the walk did not terminate. Measured at 20s with + # ZERO output before a timeout killed it. In CI that is a hang, not a red, and a + # hang is the one outcome that reports nothing at all. + # + # `cd` + `pwd -P` resolves `..` and symlinks both, and only works on a path that + # exists — which is why it runs after the -f test rather than as a string rewrite. + if [ -f "$cand" ]; then + cdir="$(cd -- "$(dirname -- "$cand")" 2>/dev/null && pwd -P)" || continue + found="$cdir/$(basename -- "$cand")" + break + fi done if [ -n "$found" ]; then break; fi done <<< "$bases" @@ -290,14 +320,49 @@ while :; do file="$(sed -n "${n}p" "$QUEUE")" [ -z "$file" ] && break + # SCANNED WITH NEWLINES COLLAPSED, because a line-at-a-time grep does not see a statement + # that spans lines — and the statement most likely to span lines is the one this gate must + # not miss. Shipped in v1.1.0, measured: + # + # const mod = await import( + # process.env.MODULE_NAME ?? "@/lib/secret" + # ); + # + # produced `ok [service-role]`, exit 0, over a page that reaches SUPABASE_SERVICE_ROLE_KEY. + # The guard did not fire because `import(` and the non-literal argument were on different + # lines, and the extractor did not follow it for the same reason. A FALSE GREEN in the + # security gate, from a formatting choice Prettier makes on its own. + # + # The cost of flattening: a `//` comment now runs into the code after it, so a mention of + # `import(` inside a comment can raise a spurious UNKNOWN. That is over-inclusive — a false + # RED, visible and arguable — and this repository takes that trade every time over a false + # green. It belongs with the other "regex, not a parser" gaps in the README. + tr '\n' ' ' < "$file" > "$WORK/flat" 2>/dev/null || : > "$WORK/flat" + # A branch of the graph this gate cannot follow. Reported here, against the module that # contains it, rather than assumed harmless. - if grep -qE '(^|[^A-Za-z0-9_$.])(import|require)[[:space:]]*\([[:space:]]*[^'"'"'")[:space:]]' -- "$file" 2>/dev/null; then + if grep -qE '(^|[^A-Za-z0-9_$.])(import|require)[[:space:]]*\([[:space:]]*[^'"'"'")[:space:]]' -- "$WORK/flat" 2>/dev/null; then unknown "$(rel "$file") contains a non-literal import() or require() — this gate cannot tell what it pulls in, so it will not call this path clean" fi set +e - specs="$(grep -oE "(from|import|require)[[:space:]]*\(?[[:space:]]*['\"][^'\"]+['\"]" -- "$file" 2>/dev/null | sed -E "s/.*['\"]([^'\"]+)['\"]\$/\1/")" + # BOTH passes, unioned. Flattening alone was a REGRESSION and it went in the direction this + # change exists to fix: `grep -o` matches non-overlapping, so a string ending in `from "` + # swallows the real import after it. Measured — + # + # const label = "imported from "; + # import { key } from "../lib/secret"; + # + # gave `ok [service-role]`, exit 0, over a module reaching SUPABASE_SERVICE_ROLE_KEY, on a + # tree the PREVIOUS version caught. The line pass finds ordinary imports with no window to + # swallow across; the flat pass finds the multi-line ones. Garbage the flat pass invents out + # of a swallowed span resolves as a bare specifier and is skipped, so the union only ever + # adds edges. + specs="$( + { grep -oE "(from|import|require)[[:space:]]*\(?[[:space:]]*['\"][^'\"]+['\"]" -- "$file" 2>/dev/null + grep -oE "(from|import|require)[[:space:]]*\(?[[:space:]]*['\"][^'\"]+['\"]" -- "$WORK/flat" 2>/dev/null + } | sed -E "s/.*['\"]([^'\"]+)['\"]\$/\1/" | sort -u + )" set -e while IFS= read -r spec; do [ -n "$spec" ] || continue From dad377cf691288c814a1ab06ec8821c02a846ce4 Mon Sep 17 00:00:00 2001 From: Chii Date: Tue, 8 Sep 2026 22:12:50 +0000 Subject: [PATCH 02/13] v1.2.1: four review findings, one of them a false green, and the selftest leg that let it through MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reproduced each before touching anything; the two that turned out to be mine are the ones that matter. FALSE GREEN in the security gate. resolve() classified any specifier matching no tsconfig alias as "a published package, out of this gate's reach by design". "It matched no alias" and "it is a package" are not the same sentence, and only the first was ever tested. `@/lib/secret` in a tree whose tsconfig declares some other alias matches nothing and cannot be a package either — npm has no empty scope — so it was skipped as a dependency. Measured on a tree reaching SUPABASE_SERVICE_ROLE_KEY: `ok [service-role]`, exit 0. The claim is now tested against npm's name shape and an untestable specifier is UNKNOWN, never skipped. This arrived as "the extends guard is skipped when the child declares any local alias, so an inherited @/* stays absent from $ALIASES". That mechanism does not hold: measured with tsc 5.6.3, a child that declares `paths` REPLACES the base's object entirely, so the inherited @/* is already dead in that tree. The guard's condition is right. The tree it describes still went green, for the reason above. FALSE RED under moduleResolution: nodenext. TypeScript makes you import the EMITTED file (`./mod.mjs`) while the source on disk is `mod.mts`. The probe only ever appended extensions, so it tested `mod.mjs.mts`, resolved nothing, and went UNKNOWN — the gate blocking a tree it could not spell. The emitted->source mapping is now applied before probing (.mjs<-.mts, .cjs<-.cts, .js<-.ts|.tsx, .jsx<-.tsx), with the emitted spelling kept so a plain JS project still resolves. migrations-lint read `create table "public"."orders"` as a table named `public`, because the schema qualifier was only matched unquoted. Where a table genuinely named `public` had RLS the file PASSED and `orders` was never checked; where it did not, the violation named the wrong table. The qualifier is now generic (any schema, quoted or not) on both the create and the alter side, which have to agree or the second check cannot find what the first one named. The fixture discriminates: green under the old regex, red under the new one. AND THE LEG THAT LET IT THROUGH. selftest's `cases/` leg required only a non-zero exit while printing "catches case ''". UNKNOWN is also red, so a gate that lost the ability to see the planted shape and merely tripped over a blind spot on the same tree passed this leg with the shape undetected. An assertion narrower than its own message, in the code written to catch exactly that. It now requires a FAIL line, as the UNKNOWN leg already required both halves — which is why the nodenext fixture proves the fix rather than the bug. review.yml's job now also skips on fork pull requests. The finding said a PR can rewrite the caller and read CLAUDE_CODE_OAUTH_TOKEN; the load-bearing word is the trigger. The caller runs on `pull_request`, not `pull_request_target`, so a fork PR gets no repository secrets to read and a same-repo branch is pushed by someone who already has write access. What does hold is duller: on a fork PR the secret resolves to empty against a `required: true` declaration, so the reviewer is red for a reason having nothing to do with the code. Skipping says that honestly. The v1.2.0 pin in caller-template.yml is untouched. It is the open convention decision already stated in the header, not something to patch away quietly. Selftest green. All six shell gates green on the template tree at deda1fa. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BfFq2KvV1CCkPs5WnV1AsA --- .github/workflows/review.yml | 21 ++++++- README.md | 18 +++++- .../migrations/20260101000000_orders.down.sql | 2 + .../migrations/20260101000000_orders.sql | 12 ++++ .../emitted-extension-import/app/admin.mts | 1 + .../emitted-extension-import/app/page.tsx | 7 +++ .../emitted-extension-import/package.json | 1 + .../emitted-extension-import/tsconfig.json | 1 + .../alias-shaped-nonpackage/app/page.tsx | 8 +++ .../alias-shaped-nonpackage/package.json | 1 + .../alias-shaped-nonpackage/tsconfig.json | 1 + gates/migrations-lint.sh | 16 ++++- gates/service-role.sh | 62 ++++++++++++++++++- selftest.sh | 10 +++ 14 files changed, 151 insertions(+), 10 deletions(-) create mode 100644 fixtures/migrations-lint/bad/cases/quoted-schema-qualifier/supabase/migrations/20260101000000_orders.down.sql create mode 100644 fixtures/migrations-lint/bad/cases/quoted-schema-qualifier/supabase/migrations/20260101000000_orders.sql create mode 100644 fixtures/service-role/bad/cases/emitted-extension-import/app/admin.mts create mode 100644 fixtures/service-role/bad/cases/emitted-extension-import/app/page.tsx create mode 100644 fixtures/service-role/bad/cases/emitted-extension-import/package.json create mode 100644 fixtures/service-role/bad/cases/emitted-extension-import/tsconfig.json create mode 100644 fixtures/service-role/bad/unknown/alias-shaped-nonpackage/app/page.tsx create mode 100644 fixtures/service-role/bad/unknown/alias-shaped-nonpackage/package.json create mode 100644 fixtures/service-role/bad/unknown/alias-shaped-nonpackage/tsconfig.json diff --git a/.github/workflows/review.yml b/.github/workflows/review.yml index 6d1f406..bc63c02 100644 --- a/.github/workflows/review.yml +++ b/.github/workflows/review.yml @@ -51,7 +51,26 @@ concurrency: jobs: review: name: Draft review - if: github.event.pull_request.draft == true + # DRAFT, AND NOT FROM A FORK. The second half is about what a secret is worth on each + # trigger, and it was raised as an exfiltration risk — "a PR can rewrite the caller job and + # read CLAUDE_CODE_OAUTH_TOKEN". The load-bearing distinction is the trigger word: the + # caller runs on `pull_request`, NOT `pull_request_target`. On `pull_request` a fork's PR + # gets no repository secrets at all, so there is nothing there to read; and a same-repo + # branch is pushed by someone who already has write access, which is not a boundary this + # trigger was ever holding. So the risk as stated does not hold here. + # + # What DOES hold is duller and still worth the line: on a fork PR the secret expression + # resolves to empty while `review.yml` declares it `required: true`, so the reviewer is red + # on every fork PR for a reason having nothing to do with the code — the same shape as the + # 403 the `permissions:` note in caller-template.yml exists to prevent. Skipping is the + # honest outcome: as with a ready PR, `skipped` is an ABSENCE and is not claimed as a pass. + # + # This is reasoned from GitHub's documented trigger semantics, not measured: a fork PR + # against this repository is not something this branch can stage. Stated so the next reader + # knows which kind of claim it is. + if: >- + github.event.pull_request.draft == true + && github.event.pull_request.head.repo.fork == false runs-on: ubuntu-latest timeout-minutes: 20 steps: diff --git a/README.md b/README.md index 7ff0889..c492c48 100644 --- a/README.md +++ b/README.md @@ -304,12 +304,24 @@ header, which is the honest account of what that scanner does not do. - **The service-role gate reads imports with a regex, not a parser.** It follows literal `from`, `import` and `require` specifiers, resolving `./`, `../`, absolute paths, and **every alias declared in tsconfig `compilerOptions.paths`** — not just `@/*`. A specifier - matching no declared alias is treated as a published package; a specifier that matches one - and resolves to no file is UNKNOWN, never skipped. If the `paths` object is present and no - alias can be parsed out of it, that is UNKNOWN too. A specifier that appears inside a + matching no declared alias is treated as a published package **only if it could be one**: + from v1.2.1 the claim is tested against npm's name shape, so `@/lib/secret` — no alias, and + no valid empty scope — is UNKNOWN rather than a dependency to skip. That was a false green + before. A specifier that matches an alias and resolves to no file is UNKNOWN, never skipped. + If the `paths` object is present and no alias can be parsed out of it, that is UNKNOWN too. + It also maps the **emitted** extension back to the source one (`./m.mjs` → `m.mts`, `.cjs` + → `.cts`, `.js` → `.ts`/`.tsx`), which `moduleResolution: nodenext` requires you to write + and which was a false red before v1.2.1. A specifier that appears inside a comment or a string is followed as though it were real — over-inclusive, which costs a false red rather than a false green. What it cannot follow at all it calls UNKNOWN. +- **The service-role gate does not follow tsconfig `extends`.** Aliases are read from the + repo's own `tsconfig.json` only. TypeScript does not deep-merge `paths` — measured with + tsc 5.6.3: a child that declares `paths` REPLACES the base's object entirely, so a base's + `@/*` is already dead in that tree — but a child that declares `extends` and no `paths` of + its own inherits them, and this gate cannot see them. That case is UNKNOWN, not a pass. + `baseUrl` inherited from a base config is not read either, which makes alias targets fail + to resolve and go UNKNOWN: a false red, and the direction this toolkit errs in. - **The service-role gate does not look inside published packages.** A bare specifier (`react`, `@supabase/ssr`) is out of scope by definition, so a third-party module that reads `process.env.SUPABASE_SECRET_KEY` itself is invisible to it. Repo source is what it diff --git a/fixtures/migrations-lint/bad/cases/quoted-schema-qualifier/supabase/migrations/20260101000000_orders.down.sql b/fixtures/migrations-lint/bad/cases/quoted-schema-qualifier/supabase/migrations/20260101000000_orders.down.sql new file mode 100644 index 0000000..157967c --- /dev/null +++ b/fixtures/migrations-lint/bad/cases/quoted-schema-qualifier/supabase/migrations/20260101000000_orders.down.sql @@ -0,0 +1,2 @@ +drop table if exists "public"."orders"; +drop table if exists public.public; diff --git a/fixtures/migrations-lint/bad/cases/quoted-schema-qualifier/supabase/migrations/20260101000000_orders.sql b/fixtures/migrations-lint/bad/cases/quoted-schema-qualifier/supabase/migrations/20260101000000_orders.sql new file mode 100644 index 0000000..4c664c6 --- /dev/null +++ b/fixtures/migrations-lint/bad/cases/quoted-schema-qualifier/supabase/migrations/20260101000000_orders.sql @@ -0,0 +1,12 @@ +-- The schema qualifier is quoted SEPARATELY from the table: "public"."orders". The extractor +-- read the first identifier it could and took `public` for the table name, so the RLS check +-- below was run against a table called public rather than against orders. +-- +-- This fixture is shaped to discriminate. A table actually named `public` exists here and +-- does enable RLS, so the broken reading finds what it is looking for and the file passes, +-- with orders — the real table, with no RLS — never checked at all. A gate that reads the +-- name correctly checks orders, finds no RLS, and goes red. Before the fix: green. After: red. +create table public.public (id uuid primary key); +alter table public.public enable row level security; + +create table "public"."orders" (id uuid primary key); diff --git a/fixtures/service-role/bad/cases/emitted-extension-import/app/admin.mts b/fixtures/service-role/bad/cases/emitted-extension-import/app/admin.mts new file mode 100644 index 0000000..7b4886c --- /dev/null +++ b/fixtures/service-role/bad/cases/emitted-extension-import/app/admin.mts @@ -0,0 +1 @@ +export const admin = process.env.SUPABASE_SERVICE_ROLE_KEY; diff --git a/fixtures/service-role/bad/cases/emitted-extension-import/app/page.tsx b/fixtures/service-role/bad/cases/emitted-extension-import/app/page.tsx new file mode 100644 index 0000000..c27b0fb --- /dev/null +++ b/fixtures/service-role/bad/cases/emitted-extension-import/app/page.tsx @@ -0,0 +1,7 @@ +// nodenext makes the import name the EMITTED file (.mjs) while the source is .mts. +// The gate must follow that mapping; appending extensions to "./admin.mjs" finds nothing. +import { admin } from "./admin.mjs"; + +export default function Page() { + return admin; +} diff --git a/fixtures/service-role/bad/cases/emitted-extension-import/package.json b/fixtures/service-role/bad/cases/emitted-extension-import/package.json new file mode 100644 index 0000000..7a67e5e --- /dev/null +++ b/fixtures/service-role/bad/cases/emitted-extension-import/package.json @@ -0,0 +1 @@ +{ "name": "fixture-emitted-extension-import", "dependencies": { "next": "15.0.0" } } diff --git a/fixtures/service-role/bad/cases/emitted-extension-import/tsconfig.json b/fixtures/service-role/bad/cases/emitted-extension-import/tsconfig.json new file mode 100644 index 0000000..1bfb2d8 --- /dev/null +++ b/fixtures/service-role/bad/cases/emitted-extension-import/tsconfig.json @@ -0,0 +1 @@ +{ "compilerOptions": { "moduleResolution": "nodenext", "verbatimModuleSyntax": true } } diff --git a/fixtures/service-role/bad/unknown/alias-shaped-nonpackage/app/page.tsx b/fixtures/service-role/bad/unknown/alias-shaped-nonpackage/app/page.tsx new file mode 100644 index 0000000..b82738d --- /dev/null +++ b/fixtures/service-role/bad/unknown/alias-shaped-nonpackage/app/page.tsx @@ -0,0 +1,8 @@ +// "@/lib/secret" matches no alias declared above, and npm has no empty scope, so it cannot +// be a published package either. The gate must not skip it as a dependency: it does not +// know what it is, and an unread module is not a clean one. +import { key } from "@/lib/secret"; + +export default function Page() { + return key; +} diff --git a/fixtures/service-role/bad/unknown/alias-shaped-nonpackage/package.json b/fixtures/service-role/bad/unknown/alias-shaped-nonpackage/package.json new file mode 100644 index 0000000..4af6d69 --- /dev/null +++ b/fixtures/service-role/bad/unknown/alias-shaped-nonpackage/package.json @@ -0,0 +1 @@ +{ "name": "fixture-alias-shaped-nonpackage", "dependencies": { "next": "15.0.0" } } diff --git a/fixtures/service-role/bad/unknown/alias-shaped-nonpackage/tsconfig.json b/fixtures/service-role/bad/unknown/alias-shaped-nonpackage/tsconfig.json new file mode 100644 index 0000000..3ea5598 --- /dev/null +++ b/fixtures/service-role/bad/unknown/alias-shaped-nonpackage/tsconfig.json @@ -0,0 +1 @@ +{ "compilerOptions": { "baseUrl": ".", "paths": { "~/util/*": ["./src/util/*"] } } } diff --git a/gates/migrations-lint.sh b/gates/migrations-lint.sh index c7e44d5..e1a5967 100755 --- a/gates/migrations-lint.sh +++ b/gates/migrations-lint.sh @@ -62,12 +62,22 @@ for up in "$MIG"/*.sql; do down="${up%.sql}.down.sql" [ -f "$down" ] || fail "no rollback: $(basename "$up") needs $(basename "$down")" strip_sql_comments "$up" > "$STRIPPED" - # tables created here must enable RLS here + # tables created here must enable RLS here. + # + # THE SCHEMA QUALIFIER IS OPTIONAL AND MAY BE QUOTED SEPARATELY. `create table + # "public"."orders"` is one identifier per quoted part, and the pattern used to name only + # `public\.` unquoted — so on that spelling the leading `"?` swallowed the opening quote, + # `public` was read as the TABLE, and the RLS search below ran against a table that does not + # exist. Where a table genuinely named `public` had RLS, the file passed and `orders` was + # never checked at all: a FALSE GREEN. Where it did not, the violation named the wrong table: + # a red a fixer cannot act on. The qualifier is now matched generically (any schema, quoted + # or not) on both the create side and the alter side, which have to agree or the second + # check cannot find what the first one named. while IFS= read -r tbl; do - if ! grep -qiE "alter[[:space:]]+table[[:space:]]+(if[[:space:]]+exists[[:space:]]+)?(public\.)?\"?${tbl}\"?[[:space:]]+enable[[:space:]]+row[[:space:]]+level[[:space:]]+security" "$STRIPPED"; then + if ! grep -qiE "alter[[:space:]]+table[[:space:]]+(if[[:space:]]+exists[[:space:]]+)?(\"?[a-z_][a-z0-9_]*\"?\.)?\"?${tbl}\"?[[:space:]]+enable[[:space:]]+row[[:space:]]+level[[:space:]]+security" "$STRIPPED"; then fail "$(basename "$up"): table '$tbl' created without 'enable row level security' in the same file" fi - done < <(grep -ioE "create[[:space:]]+table[[:space:]]+(if[[:space:]]+not[[:space:]]+exists[[:space:]]+)?(public\.)?\"?[a-z_][a-z0-9_]*" "$STRIPPED" | sed -E 's/.*[ .]"?([a-z_][a-z0-9_]*)"?$/\1/i') + done < <(grep -ioE "create[[:space:]]+table[[:space:]]+(if[[:space:]]+not[[:space:]]+exists[[:space:]]+)?(\"?[a-z_][a-z0-9_]*\"?\.)?\"?[a-z_][a-z0-9_]*" "$STRIPPED" | sed -E 's/.*[ .]"?([a-z_][a-z0-9_]*)"?$/\1/i') # destructive statements outside a WHERE are tier-3 by regex (non-negotiable 4); flag, do not block if grep -qiE "^[[:space:]]*(drop[[:space:]]+table|truncate|delete[[:space:]]+from[[:space:]]+[a-z_.\"]+[[:space:]]*;)" "$STRIPPED"; then echo "note [$GATE] $(basename "$up"): destructive statement present; this migration is tier-3" diff --git a/gates/service-role.sh b/gates/service-role.sh index 5ef24be..425a965 100755 --- a/gates/service-role.sh +++ b/gates/service-role.sh @@ -231,8 +231,34 @@ done < "$SEEDS" # The three outcomes are the whole point. 1 is a claim — "this is a published package, not # this repo's source" — and it is only safe to make about a specifier that matches NO # declared alias. Anything that looks local and does not resolve is 2, never 1. +# "It matched no declared alias" and "it is a published package" are not the same sentence, +# and resolve() used to print the second while only having checked the first. `@/lib/secret` +# in a tree whose tsconfig declares some OTHER alias matches nothing here — and it cannot be +# a package either: npm has no empty scope. The gate called it a dependency and skipped it, +# so a request-path module importing it read as clean. Measured on a tree reaching +# SUPABASE_SERVICE_ROLE_KEY: `ok [service-role]`, exit 0. A FALSE GREEN, from a classifier +# whose claim was wider than its test — this repository's recurring defect, again, in the +# branch that decides what is out of scope. +# +# So the claim is now tested. A specifier that is neither a declared alias nor a well-formed +# package name is UNKNOWN: this gate does not know what it is, and will not call it clean. +is_package_specifier() { + local sc nm + case "$1" in + @*/*) + sc="${1#@}"; sc="${sc%%/*}" + case "$sc" in ""|[!A-Za-z0-9]*) return 1 ;; esac + nm="${1#@*/}" + case "$nm" in ""|[!A-Za-z0-9]*) return 1 ;; esac + return 0 ;; + @*) return 1 ;; # a scope with no package under it + [A-Za-z0-9]*) return 0 ;; # react, node:fs, @-less subpaths + *) return 1 ;; # ~/..., #internal/..., ./ and ../ never reach here + esac +} + resolve() { # $1 = specifier, $2 = importing file - local spec="$1" bases="" matched=0 key target prefix t b ext cand cdir found="" + local spec="$1" bases="" matched=0 key target prefix t b bb cands ext cand cdir found="" case "$spec" in ./*|../*) bases="$(dirname -- "$2")/$spec"; matched=1 ;; /*) bases="$ROOT$spec"; matched=1 ;; @@ -263,15 +289,42 @@ $BASE_DIR/$t" done < "$ALIASES" ;; esac - if [ "$matched" -ne 1 ]; then return 1; fi + if [ "$matched" -ne 1 ]; then + if is_package_specifier "$spec"; then return 1; fi + return 3 + fi while IFS= read -r b; do [ -n "$b" ] || continue + # THE SPECIFIER'S EXTENSION IS THE EMITTED ONE, NOT THE SOURCE'S. Under + # `moduleResolution: nodenext` (and with `verbatimModuleSyntax`) TypeScript requires the + # import to name the file JavaScript will load — `./mod.mjs` — while the file on disk is + # `mod.mts`. The probe below only ever appended extensions, so it tested `mod.mjs.mts` + # and nothing else, resolved to no file, and reported UNKNOWN. A FALSE RED on the modern + # default resolution mode: the gate blocking a tree it simply could not spell. + # + # The mapping is TypeScript's own and is one-to-one: .mjs<-.mts, .cjs<-.cts, .js<-.ts|.tsx, + # .jsx<-.tsx. The emitted spelling is kept in the list as well, because a plain JS project + # has the .js on disk and both must resolve. + cands="$b" + case "$b" in + *.mjs) cands="$b +${b%.mjs}.mts" ;; + *.cjs) cands="$b +${b%.cjs}.cts" ;; + *.jsx) cands="$b +${b%.jsx}.tsx" ;; + *.js) cands="$b +${b%.js}.ts +${b%.js}.tsx" ;; + esac + while IFS= read -r bb; do + [ -n "$bb" ] || continue # .d.ts and friends included: `import type { Database } from "@/types/supabase"` against a # src/types/supabase.d.ts resolved to nothing, which this gate calls UNKNOWN — a permanent # blocking red on a perfectly ordinary line. The selftest cannot catch a false red (its own # note says the fixture model holds bad trees only), so it is fixed here on report. for ext in "" .ts .tsx .d.ts .mts .cts .js .jsx .mjs .cjs /index.ts /index.tsx /index.d.ts /index.js /index.jsx; do - cand="$b$ext" + cand="$bb$ext" # CANONICALISED, not merely tested for existence. Without this the resolved path keeps # whatever `..` the importer's specifier put in it, and TWO SPELLINGS OF ONE FILE ARE # TWO NODES. That cost two defects, both shipped in v1.1.0: @@ -295,6 +348,8 @@ $BASE_DIR/$t" fi done if [ -n "$found" ]; then break; fi + done <<< "$cands" + if [ -n "$found" ]; then break; fi done <<< "$bases" if [ -n "$found" ]; then printf '%s' "$found"; return 0; fi return 2 @@ -374,6 +429,7 @@ while :; do 0) printf '%s\t%s\n' "$file" "$target" >> "$EDGES"; enqueue "$target" ;; 1) : ;; # bare specifier: a published package, out of this gate's reach by design 2) unknown "$(rel "$file") imports '$spec', which this gate could not resolve to a file — an unread module is not a clean one" ;; + 3) unknown "$(rel "$file") imports '$spec', which matches no alias declared in tsconfig.json and is not a well-formed package name — this gate cannot say what it is, and will not call it a dependency to skip it" ;; esac done <<< "$specs" done diff --git a/selftest.sh b/selftest.sh index 7e25c4a..899472c 100755 --- a/selftest.sh +++ b/selftest.sh @@ -209,12 +209,22 @@ for g in "$HERE"/gates/*.sh "$HERE"/gates/*.py; do cname="$(basename "$c")" run_gate "$g" "${c%/}" rc=$? + # THE RED IS NOT ENOUGH, and this leg used to accept it. A case fixture plants one + # violation and the claim printed is "catches case ''" — but UNKNOWN is also red, + # so a gate that lost the ability to see the planted shape and merely tripped over a + # blind spot on the same tree passed this leg while the shape went undetected. That is + # an assertion narrower than its own message, in the code written to catch exactly + # that. The UNKNOWN leg below already asserts both halves; this one now does too. + n_fail="$(grep -c '^FAIL \[' "$OUT" || true)" if [ "$rc" -eq 0 ]; then echo "SELFTEST FAIL: $gate PASSED case '$cname' — that shape is no longer detected" sed 's/^/ /' "$OUT"; bad=1 elif [ "$rc" -ne 1 ]; then echo "SELFTEST FAIL: $gate errored (exit $rc) on case '$cname' instead of reporting a violation" sed 's/^/ /' "$OUT"; bad=1 + elif [ "$n_fail" -eq 0 ]; then + echo "SELFTEST FAIL: $gate went red on case '$cname' without printing a FAIL line — the red is a blind spot, not the planted violation" + sed 's/^/ /' "$OUT"; bad=1 else echo "ok $gate catches case '$cname'" fi From 9547e04e4772420a64f67f52b3c7f5ce29ffc7ec Mon Sep 17 00:00:00 2001 From: Chii Date: Tue, 8 Sep 2026 22:29:01 +0000 Subject: [PATCH 03/13] Two regressions from the last commit, a false green it did not close, and the leg that could not see any of them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The local adversarial round over the whole of service-role.sh — not just the diff — found four Important. Two were introduced by dad377c, which is the commit that fixed a false green in the same function. REGRESSION 1, AND THE WORST THING HERE. Widening the ALTER-side qualifier to any schema while the CREATE side still discarded the schema made migrations-lint assert only "SOME table called orders, in SOME schema, has RLS" — under a message naming one specific table. Measured: create table public.orders (id uuid primary key, owner uuid); alter table archive.orders enable row level security; 9c281e8: FAIL … table 'orders' created without 'enable row level security' dad377c: ok [migrations-lint] exit 0 PostgreSQL 16 accepts that state and public.orders really is unprotected. Two schemas holding a same-named table is an ordinary Supabase layout. A widened matcher under an unwidened message is this repository's recurring defect, added by the commit that removed another instance of it. The schema and table now travel together; unqualified normalises to `public`, and an unqualified ALTER is accepted only for a table created in public. Twelve spellings verified. Also matched now: `unlogged`, `temp` and `temporary` tables, which `create[[:space:]]+table` never saw — an unlogged table with no RLS anywhere was never checked at all — and whitespace around the qualifier dot, which Postgres accepts and which still extracted the schema as the table name. REGRESSION 2. Reporting UNKNOWN for an unclassifiable specifier (right) turned every template literal in the tree into a blocking red (wrong), because the extractor deliberately over-matches strings and its garbage used to fall through the silent skip. Measured, on a file that imports NOTHING: const sql = `select id, owner from "${table}" where owner = $1`; UNKNOWN [service-role] app/api/orders/route.ts imports '${table}' … No action a fixer can take: the message names an import that does not exist and the only way to green is to delete the string. Candidates carrying a character no module specifier can contain are now dropped before classification. The comment claiming this could not happen was falsified by the same commit that wrote it, and was wrong about which pass invents the garbage as well. A FALSE GREEN dad377c DID NOT CLOSE. `baseUrl` with no matching `paths` entry — Next.js's documented Absolute Imports — is a local import, and the gate parsed that baseUrl and then skipped the specifier as a published package: import { admin } from "lib/supabase-admin"; -> ok exit 0 import { admin } from "../lib/supabase-admin"; -> FAIL exit 1 Same file, same secret, same request path; only the spelling differs. It now probes BASE_DIR before calling anything a dependency, and falls back to rc 1 rather than red when nothing is there, exactly as TypeScript falls through to node_modules. The README hunk added in dad377c asserted the opposite direction of error for baseUrl and is corrected. AND THE LEG THAT COULD NOT SEE THEM. The `FAIL [` requirement added in dad377c is unreachable for the two vendored Python gates: those markers come from lib.sh, and check_references.py prints its own violation line. Planted a case fixture and measured — the gate caught the shape perfectly, the harness called it a blind spot. An assertion BROADER than its message, in the leg written to catch assertions narrower than theirs. The marker check is now per-gate. The fixture is not kept, and that is a second finding: unlike every shell gate, the vendored check_references.py has no fixtures/ filter, so the planted citation is validated as real and the tree leg goes red. No Python gate can carry a cases/ fixture until that changes, and changing it edits a vendored file. Nits from the same round, all reproduced: the emitted->source map omitted the declaration extensions (`./types.js` against a `types.d.ts` type-checks clean under nodenext; the gate went UNKNOWN — the same .d.ts false red the extension list was widened to close one release ago); it probed the emitted spelling before the source, so a stale .mjs beside its .mts masked a secret added to the source; the rc=3 message named tsconfig.json on trees that have none; and the selftest header still described a path and a requirement that had both moved. Both new fixtures discriminate — green under dad377c, red now. Selftest green. All six shell gates green on the template tree at deda1fa. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BfFq2KvV1CCkPs5WnV1AsA --- README.md | 29 +++++-- caller-template.yml | 7 ++ .../migrations/20260102000000_orders.down.sql | 2 + .../migrations/20260102000000_orders.sql | 9 ++ .../baseurl-absolute-import/package.json | 1 + .../baseurl-absolute-import/src/app/page.tsx | 9 ++ .../src/lib/supabase-admin.ts | 1 + .../baseurl-absolute-import/tsconfig.json | 1 + gates/migrations-lint.sh | 58 ++++++++++--- gates/service-role.sh | 85 +++++++++++++++---- selftest.sh | 51 +++++++++-- 11 files changed, 211 insertions(+), 42 deletions(-) create mode 100644 fixtures/migrations-lint/bad/cases/cross-schema-rls/supabase/migrations/20260102000000_orders.down.sql create mode 100644 fixtures/migrations-lint/bad/cases/cross-schema-rls/supabase/migrations/20260102000000_orders.sql create mode 100644 fixtures/service-role/bad/cases/baseurl-absolute-import/package.json create mode 100644 fixtures/service-role/bad/cases/baseurl-absolute-import/src/app/page.tsx create mode 100644 fixtures/service-role/bad/cases/baseurl-absolute-import/src/lib/supabase-admin.ts create mode 100644 fixtures/service-role/bad/cases/baseurl-absolute-import/tsconfig.json diff --git a/README.md b/README.md index c492c48..c823b66 100644 --- a/README.md +++ b/README.md @@ -296,6 +296,12 @@ header, which is the honest account of what that scanner does not do. here and is called from consumer repositories; wiring a self-call is a separate change, so until then the fixture procedure is the only thing that exercises it. Stated because a reviewer nobody has run is a reviewer nobody has seen fail. +- **The migrations-lint gate matches `create table` by regex.** It reads `unlogged`, `temp` + and `temporary` tables, a quoted or unquoted schema qualifier with or without whitespace + around the dot, and it carries the **schema and table together**: RLS on `archive.orders` + does not satisfy a `create table public.orders`, and an unqualified name is normalised to + `public`. What it does not read is a table created inside a function body, a `DO $$` block, + or by dynamic SQL. - **The service-role gate scans each module with newlines collapsed**, so that a statement spanning lines is seen as one — a multi-line `await import(` was a false green before v1.2.1. The cost is that a `//` comment runs into the code after it, so `import(` mentioned @@ -309,9 +315,14 @@ header, which is the honest account of what that scanner does not do. no valid empty scope — is UNKNOWN rather than a dependency to skip. That was a false green before. A specifier that matches an alias and resolves to no file is UNKNOWN, never skipped. If the `paths` object is present and no alias can be parsed out of it, that is UNKNOWN too. - It also maps the **emitted** extension back to the source one (`./m.mjs` → `m.mts`, `.cjs` - → `.cts`, `.js` → `.ts`/`.tsx`), which `moduleResolution: nodenext` requires you to write - and which was a false red before v1.2.1. A specifier that appears inside a + It also maps the **emitted** extension back to the source one (`./m.mjs` → `m.mts`/`.d.mts`, + `.cjs` → `.cts`/`.d.cts`, `.js` → `.ts`/`.tsx`/`.d.ts`), which `moduleResolution: nodenext` + requires you to write and which was a false red before v1.2.1; the source is probed before + the emitted file, so a stale `.mjs` sitting beside its `.mts` cannot mask a secret added to + the source. A **`baseUrl` with no matching `paths` entry** — Next.js's documented "Absolute + Imports" — is resolved too: `import "lib/admin"` under `baseUrl: "src"` was skipped as a + published package while the same file spelled `../lib/admin` was caught, a false green + chosen by nothing but the spelling. A specifier that appears inside a comment or a string is followed as though it were real — over-inclusive, which costs a false red rather than a false green. What it cannot follow at all it calls UNKNOWN. @@ -320,8 +331,16 @@ header, which is the honest account of what that scanner does not do. tsc 5.6.3: a child that declares `paths` REPLACES the base's object entirely, so a base's `@/*` is already dead in that tree — but a child that declares `extends` and no `paths` of its own inherits them, and this gate cannot see them. That case is UNKNOWN, not a pass. - `baseUrl` inherited from a base config is not read either, which makes alias targets fail - to resolve and go UNKNOWN: a false red, and the direction this toolkit errs in. + A `baseUrl` **inherited from a base config** is not read either, which makes alias targets + fail to resolve and go UNKNOWN: a false red, and the direction this toolkit errs in. A + baseUrl declared in the repo's own tsconfig *is* read, including for bare specifiers. +- **The service-role gate drops extractor artefacts before classifying them.** A candidate + carrying a character no module specifier can contain — `` ` ``, `{`, `}`, `<`, `>`, `;`, + `(`, `)`, `,`, `=`, `|`, `*` — is discarded rather than reported. Without that, a template + literal in ordinary source (`` `select … from "${table}"` ``) became a blocking UNKNOWN + naming an import that does not exist, in a file that imports nothing, with no action a + fixer could take. The trade is the usual one in the other direction: a specifier genuinely + containing one of those characters is skipped silently. - **The service-role gate does not look inside published packages.** A bare specifier (`react`, `@supabase/ssr`) is out of scope by definition, so a third-party module that reads `process.env.SUPABASE_SECRET_KEY` itself is invisible to it. Repo source is what it diff --git a/caller-template.yml b/caller-template.yml index 9a58260..ad11b3f 100644 --- a/caller-template.yml +++ b/caller-template.yml @@ -39,6 +39,13 @@ jobs: # It runs ONLY while the pull request is a draft. On a ready PR the job reports `skipped`, # which is an absence, not a pass — account for it before flipping, as AGENTS.md says. # + # From v1.2.1 it also skips on pull requests FROM FORKS, and the pin below predates that. + # A fork PR gets no repository secrets, so the token expression is empty against a + # `required: true` declaration and the job is red for a reason having nothing to do with the + # code. Pinning the SHA on this line to v1.2.1 or later is what gets the skip; that is the + # same pin question the header above hands to the reader, named here so it is not discovered + # by a red run on someone's first outside contribution. + # # ONE SHA in this block, on the `uses:` line. The line under `secrets:` is a token mapping. review: # PERMISSIONS ARE THE CALLER'S TO GRANT. A called workflow can only narrow the token it is diff --git a/fixtures/migrations-lint/bad/cases/cross-schema-rls/supabase/migrations/20260102000000_orders.down.sql b/fixtures/migrations-lint/bad/cases/cross-schema-rls/supabase/migrations/20260102000000_orders.down.sql new file mode 100644 index 0000000..78d2ff6 --- /dev/null +++ b/fixtures/migrations-lint/bad/cases/cross-schema-rls/supabase/migrations/20260102000000_orders.down.sql @@ -0,0 +1,2 @@ +drop table if exists archive.orders; +drop table if exists public.orders; diff --git a/fixtures/migrations-lint/bad/cases/cross-schema-rls/supabase/migrations/20260102000000_orders.sql b/fixtures/migrations-lint/bad/cases/cross-schema-rls/supabase/migrations/20260102000000_orders.sql new file mode 100644 index 0000000..8f2806c --- /dev/null +++ b/fixtures/migrations-lint/bad/cases/cross-schema-rls/supabase/migrations/20260102000000_orders.sql @@ -0,0 +1,9 @@ +-- RLS ON A DIFFERENT SCHEMA'S SAME-NAMED TABLE. The create side discarded the schema +-- qualifier after reading it, so the check asked only "does SOME table called orders, in SOME +-- schema, have RLS?" while the message named one specific table. Both statements below are +-- accepted by PostgreSQL and the resulting state really does leave public.orders unprotected. +-- Two schemas holding a same-named table is an ordinary layout, not a contrivance. +create table public.orders (id uuid primary key, owner uuid); + +create table archive.orders (id uuid primary key, owner uuid); +alter table archive.orders enable row level security; diff --git a/fixtures/service-role/bad/cases/baseurl-absolute-import/package.json b/fixtures/service-role/bad/cases/baseurl-absolute-import/package.json new file mode 100644 index 0000000..0612c27 --- /dev/null +++ b/fixtures/service-role/bad/cases/baseurl-absolute-import/package.json @@ -0,0 +1 @@ +{ "name": "fixture-baseurl-absolute-import", "dependencies": { "next": "15.0.0" } } diff --git a/fixtures/service-role/bad/cases/baseurl-absolute-import/src/app/page.tsx b/fixtures/service-role/bad/cases/baseurl-absolute-import/src/app/page.tsx new file mode 100644 index 0000000..0470121 --- /dev/null +++ b/fixtures/service-role/bad/cases/baseurl-absolute-import/src/app/page.tsx @@ -0,0 +1,9 @@ +// Next.js "Absolute Imports": baseUrl alone, no paths alias anywhere. `lib/supabase-admin` +// means `src/lib/supabase-admin.ts`. The gate parsed this baseUrl — it is the base for every +// alias target — and still skipped this bare specifier as a published package. Written +// `../lib/supabase-admin`, the same file and the same secret were caught. +import { admin } from "lib/supabase-admin"; + +export default function Page() { + return admin; +} diff --git a/fixtures/service-role/bad/cases/baseurl-absolute-import/src/lib/supabase-admin.ts b/fixtures/service-role/bad/cases/baseurl-absolute-import/src/lib/supabase-admin.ts new file mode 100644 index 0000000..7b4886c --- /dev/null +++ b/fixtures/service-role/bad/cases/baseurl-absolute-import/src/lib/supabase-admin.ts @@ -0,0 +1 @@ +export const admin = process.env.SUPABASE_SERVICE_ROLE_KEY; diff --git a/fixtures/service-role/bad/cases/baseurl-absolute-import/tsconfig.json b/fixtures/service-role/bad/cases/baseurl-absolute-import/tsconfig.json new file mode 100644 index 0000000..41b4e99 --- /dev/null +++ b/fixtures/service-role/bad/cases/baseurl-absolute-import/tsconfig.json @@ -0,0 +1 @@ +{ "compilerOptions": { "baseUrl": "src" } } diff --git a/gates/migrations-lint.sh b/gates/migrations-lint.sh index e1a5967..15d5615 100755 --- a/gates/migrations-lint.sh +++ b/gates/migrations-lint.sh @@ -64,20 +64,52 @@ for up in "$MIG"/*.sql; do strip_sql_comments "$up" > "$STRIPPED" # tables created here must enable RLS here. # - # THE SCHEMA QUALIFIER IS OPTIONAL AND MAY BE QUOTED SEPARATELY. `create table - # "public"."orders"` is one identifier per quoted part, and the pattern used to name only - # `public\.` unquoted — so on that spelling the leading `"?` swallowed the opening quote, - # `public` was read as the TABLE, and the RLS search below ran against a table that does not - # exist. Where a table genuinely named `public` had RLS, the file passed and `orders` was - # never checked at all: a FALSE GREEN. Where it did not, the violation named the wrong table: - # a red a fixer cannot act on. The qualifier is now matched generically (any schema, quoted - # or not) on both the create side and the alter side, which have to agree or the second - # check cannot find what the first one named. - while IFS= read -r tbl; do - if ! grep -qiE "alter[[:space:]]+table[[:space:]]+(if[[:space:]]+exists[[:space:]]+)?(\"?[a-z_][a-z0-9_]*\"?\.)?\"?${tbl}\"?[[:space:]]+enable[[:space:]]+row[[:space:]]+level[[:space:]]+security" "$STRIPPED"; then - fail "$(basename "$up"): table '$tbl' created without 'enable row level security' in the same file" + # THE SCHEMA QUALIFIER IS PART OF THE TABLE'S IDENTITY AND IS CARRIED THROUGH. Three + # spellings had to be reconciled, and getting two of them right while dropping the third + # cost a false green: + # + # - `create table "public"."orders"` is one identifier per quoted part. The pattern used + # to name only `public\.` unquoted, so the leading `"?` swallowed the opening quote and + # `public` was read as the TABLE. Where a table genuinely named `public` had RLS the + # file PASSED and `orders` was never checked; where it did not, the violation named the + # wrong table — a red a fixer cannot act on. + # - Fixing that by matching ANY schema generically on both sides, while the create side + # still discarded the schema, made the check assert only "SOME table called orders, in + # SOME schema, has RLS" — under a message naming one specific table. Measured: + # `create table public.orders` + `alter table archive.orders enable row level security` + # went from red to `ok`, over a state PostgreSQL 16 accepts and in which public.orders + # really is unprotected. Two schemas holding a same-named table is an ordinary layout. + # A widened matcher under an unwidened message is this repository's recurring defect, + # introduced here in the very commit that removed another instance of it. + # - So the pair travels together. Unqualified is normalised to `public`, which is what an + # unqualified name resolves to under the default search_path, and an unqualified ALTER + # is therefore accepted only for a table created in `public`. + # + # `unlogged` and `temp`/`temporary` tables are matched too. They were invisible to + # `create[[:space:]]+table`, so an `create unlogged table orders` with no RLS anywhere was + # never checked at all — `ok`, exit 0. RLS applies to unlogged tables; verified on + # PostgreSQL 16. Whitespace around the qualifier dot (`public . orders`, which Postgres + # accepts) is matched for the same reason: a spelling the gate cannot read is a table the + # gate does not check. + while IFS="$(printf '\t')" read -r sch tbl; do + [ -n "$tbl" ] || continue + if [ "$sch" = "public" ]; then + qual="(\"?public\"?[[:space:]]*\.[[:space:]]*)?" + else + qual="\"?${sch}\"?[[:space:]]*\.[[:space:]]*" + fi + if ! grep -qiE "alter[[:space:]]+table[[:space:]]+(if[[:space:]]+exists[[:space:]]+)?${qual}\"?${tbl}\"?[[:space:]]+enable[[:space:]]+row[[:space:]]+level[[:space:]]+security" "$STRIPPED"; then + fail "$(basename "$up"): table '$sch.$tbl' created without 'enable row level security' in the same file" fi - done < <(grep -ioE "create[[:space:]]+table[[:space:]]+(if[[:space:]]+not[[:space:]]+exists[[:space:]]+)?(\"?[a-z_][a-z0-9_]*\"?\.)?\"?[a-z_][a-z0-9_]*" "$STRIPPED" | sed -E 's/.*[ .]"?([a-z_][a-z0-9_]*)"?$/\1/i') + done < <( + # Lowercased with `tr` before any sed runs, so no step needs a case-insensitive sed flag. + # `s///i` is a GNU extension and this repository has already been bitten once by a GNU-only + # regex feature silently matching nothing on BSD (`\s`, 18 occurrences, fixed in v1.2.1). + grep -ioE "create[[:space:]]+((global|local)[[:space:]]+)?((temporary|temp|unlogged)[[:space:]]+)?table[[:space:]]+(if[[:space:]]+not[[:space:]]+exists[[:space:]]+)?(\"?[a-z_][a-z0-9_]*\"?[[:space:]]*\.[[:space:]]*)?\"?[a-z_][a-z0-9_]*" "$STRIPPED" \ + | tr '[:upper:]' '[:lower:]' \ + | sed -E 's/"//g; s/[[:space:]]*\.[[:space:]]*/./; s/^create[[:space:]]+((global|local)[[:space:]]+)?((temporary|temp|unlogged)[[:space:]]+)?table[[:space:]]+(if[[:space:]]+not[[:space:]]+exists[[:space:]]+)?//' \ + | awk -F. 'NF==2 {print $1"\t"$2; next} {print "public\t"$1}' + ) # destructive statements outside a WHERE are tier-3 by regex (non-negotiable 4); flag, do not block if grep -qiE "^[[:space:]]*(drop[[:space:]]+table|truncate|delete[[:space:]]+from[[:space:]]+[a-z_.\"]+[[:space:]]*;)" "$STRIPPED"; then echo "note [$GATE] $(basename "$up"): destructive statement present; this migration is tier-3" diff --git a/gates/service-role.sh b/gates/service-role.sh index 425a965..75e766a 100755 --- a/gates/service-role.sh +++ b/gates/service-role.sh @@ -89,13 +89,18 @@ fi # gate saying anything. Every command substitution here ends in `|| true` for that reason. # --------------------------------------------------------------------------- TSCONFIG="$ROOT/tsconfig.json" +# Named in the UNKNOWN below only when it is actually there. The message said "no alias +# declared in tsconfig.json" on a tree with no tsconfig.json at all, which sends a reader +# to open a file that does not exist. +if [ -f "$TSCONFIG" ]; then TSCONFIG_NOTE=" in tsconfig.json"; else TSCONFIG_NOTE=" (no tsconfig.json in this tree)"; fi ALIASES="$WORK/aliases" : > "$ALIASES" BASE_DIR="$ROOT" +BASEURL_SET=0 if [ -f "$TSCONFIG" ]; then base_url="$(sed -nE 's/.*"baseUrl"[[:space:]]*:[[:space:]]*"([^"]*)".*/\1/p' "$TSCONFIG" 2>/dev/null | head -1 || true)" base_url="${base_url#./}"; base_url="${base_url%/}" - if [ -n "$base_url" ] && [ "$base_url" != "." ]; then BASE_DIR="$ROOT/$base_url"; fi + if [ -n "$base_url" ] && [ "$base_url" != "." ]; then BASE_DIR="$ROOT/$base_url"; BASEURL_SET=1; fi # The `paths` object, isolated exactly rather than read line by line. # @@ -258,7 +263,7 @@ is_package_specifier() { } resolve() { # $1 = specifier, $2 = importing file - local spec="$1" bases="" matched=0 key target prefix t b bb cands ext cand cdir found="" + local spec="$1" bases="" matched=0 fallback=0 key target prefix t b bb cands ext cand cdir found="" case "$spec" in ./*|../*) bases="$(dirname -- "$2")/$spec"; matched=1 ;; /*) bases="$ROOT$spec"; matched=1 ;; @@ -287,6 +292,24 @@ $BASE_DIR/$t" ;; esac done < "$ALIASES" + # `baseUrl` WITHOUT A MATCHING `paths` ENTRY IS STILL A LOCAL IMPORT. This is Next.js's + # documented "Absolute Imports" shape: `baseUrl: "src"` alone makes `lib/supabase-admin` + # mean `src/lib/supabase-admin.ts`, with no alias declared anywhere. This gate PARSED + # that baseUrl — it is `BASE_DIR` for every alias target above — and then skipped the + # bare specifier as a published package. Measured, same file and same secret, twice: + # + # import { admin } from "lib/supabase-admin"; -> ok [service-role] exit 0 + # import { admin } from "../lib/supabase-admin"; -> FAIL x2 exit 1 + # + # A FALSE GREEN selected by nothing but the spelling of the import. TypeScript resolves + # baseUrl-relative first and falls back to node_modules, so this does the same: probe + # under BASE_DIR, and if nothing is there the specifier really is a package (rc 1, not a + # red). Only when an explicit baseUrl was declared — without one there is no such shape + # to resolve, and every bare specifier is a dependency exactly as before. + if [ "$matched" -ne 1 ] && [ "$BASEURL_SET" = 1 ] && is_package_specifier "$spec"; then + matched=1; fallback=1 + bases="$BASE_DIR/$spec" + fi ;; esac if [ "$matched" -ne 1 ]; then @@ -305,17 +328,27 @@ $BASE_DIR/$t" # The mapping is TypeScript's own and is one-to-one: .mjs<-.mts, .cjs<-.cts, .js<-.ts|.tsx, # .jsx<-.tsx. The emitted spelling is kept in the list as well, because a plain JS project # has the .js on disk and both must resolve. + # THE SOURCE COMES FIRST, AND THE DECLARATION FILES ARE IN THE LIST. Verified with tsc: + # with both `admin.mts` and a stale emitted `admin.mjs` beside it, `import "./admin.mjs"` + # resolves to the .mts — so probing the emitted spelling first reads the stale artefact, + # and a secret added to the source reads as `ok`, exit 0. And `import type { X } from + # "./types.js"` against a `types.d.ts` type-checks clean under nodenext, which this list + # has to know or it re-opens the very `.d.ts` false red the extension list was widened to + # close one release ago. cands="$b" case "$b" in - *.mjs) cands="$b -${b%.mjs}.mts" ;; - *.cjs) cands="$b -${b%.cjs}.cts" ;; - *.jsx) cands="$b -${b%.jsx}.tsx" ;; - *.js) cands="$b -${b%.js}.ts -${b%.js}.tsx" ;; + *.mjs) cands="${b%.mjs}.mts +${b%.mjs}.d.mts +$b" ;; + *.cjs) cands="${b%.cjs}.cts +${b%.cjs}.d.cts +$b" ;; + *.jsx) cands="${b%.jsx}.tsx +$b" ;; + *.js) cands="${b%.js}.ts +${b%.js}.tsx +${b%.js}.d.ts +$b" ;; esac while IFS= read -r bb; do [ -n "$bb" ] || continue @@ -352,6 +385,10 @@ ${b%.js}.tsx" ;; if [ -n "$found" ]; then break; fi done <<< "$bases" if [ -n "$found" ]; then printf '%s' "$found"; return 0; fi + # A baseUrl probe that found nothing is not a failure to resolve a local module — it is + # TypeScript's own fallthrough to node_modules. Reporting UNKNOWN here would turn every + # `import React from "react"` red in any repo that declares a baseUrl. + if [ "$fallback" = 1 ]; then return 1; fi return 2 } @@ -410,13 +447,29 @@ while :; do # # gave `ok [service-role]`, exit 0, over a module reaching SUPABASE_SERVICE_ROLE_KEY, on a # tree the PREVIOUS version caught. The line pass finds ordinary imports with no window to - # swallow across; the flat pass finds the multi-line ones. Garbage the flat pass invents out - # of a swallowed span resolves as a bare specifier and is skipped, so the union only ever - # adds edges. + # swallow across; the flat pass finds the multi-line ones. + # + # AND THE GARBAGE IS DROPPED HERE, not left to be classified. The sentence that stood here + # said the invented spans "resolve as a bare specifier and are skipped, so the union only + # ever adds edges" — true only for as long as an unclassifiable specifier was silently + # skipped. The moment the classifier below began reporting UNKNOWN instead (the right + # change, made in the same commit), that sentence went false and every template literal in + # the tree became a blocking red. Measured, on a file that imports NOTHING: + # + # const sql = `select id, owner from "${table}" where owner = $1`; + # UNKNOWN [service-role] app/api/orders/route.ts imports '${table}' ... + # + # There is no action a fixer can take: the message names an import that does not exist and + # the only way to green is to delete the string. The characters filtered below cannot appear + # in any module specifier, so a candidate carrying one is an artefact of the extractor, not a + # claim about the tree. Note it was the LINE pass that produced it — the old sentence was + # wrong about which pass invents garbage as well as about what became of it. specs="$( { grep -oE "(from|import|require)[[:space:]]*\(?[[:space:]]*['\"][^'\"]+['\"]" -- "$file" 2>/dev/null grep -oE "(from|import|require)[[:space:]]*\(?[[:space:]]*['\"][^'\"]+['\"]" -- "$WORK/flat" 2>/dev/null - } | sed -E "s/.*['\"]([^'\"]+)['\"]\$/\1/" | sort -u + } | sed -E "s/.*['\"]([^'\"]+)['\"]\$/\1/" \ + | grep -vE '[`{}<>;(),=|*]' \ + | sort -u )" set -e while IFS= read -r spec; do @@ -429,7 +482,7 @@ while :; do 0) printf '%s\t%s\n' "$file" "$target" >> "$EDGES"; enqueue "$target" ;; 1) : ;; # bare specifier: a published package, out of this gate's reach by design 2) unknown "$(rel "$file") imports '$spec', which this gate could not resolve to a file — an unread module is not a clean one" ;; - 3) unknown "$(rel "$file") imports '$spec', which matches no alias declared in tsconfig.json and is not a well-formed package name — this gate cannot say what it is, and will not call it a dependency to skip it" ;; + 3) unknown "$(rel "$file") imports '$spec', which matches no path alias this gate could read$TSCONFIG_NOTE and is not a well-formed package name — this gate cannot say what it is, and will not call it a dependency to skip it" ;; esac done <<< "$specs" done diff --git a/selftest.sh b/selftest.sh index 899472c..d6ea52f 100755 --- a/selftest.sh +++ b/selftest.sh @@ -10,11 +10,14 @@ # A gate with no fixture fails this test. check_secrets.py's failure leg is its own # --selftest instead: canary, fingerprint and negative-probe checks that assert the # scanner still detects and still redacts. -# 2. the CASE leg — run it against every fixtures//cases/ and require exit 1 -# from each. The bad fixture is one tree holding many violations, so it proves only -# that SOMETHING in it fails; a shape that stopped being detected hides behind the -# others still failing. A case is one tree holding one shape, so it can only pass by -# that shape still being caught. Every false green a reviewer finds gets a case here. +# 2. the CASE leg — run it against every fixtures//bad/cases/ and require exit 1 +# AND a FAIL line from each (for the shell gates; see the marker note beside the loop). +# The bad fixture is one tree holding many violations, so it proves only that SOMETHING +# in it fails; a shape that stopped being detected hides behind the others still failing. +# A case is one tree holding one shape, so it can only pass by that shape still being +# caught. The FAIL line is half of that: UNKNOWN is also red, so without it a gate that +# lost the shape and merely tripped over a blind spot on the same tree still "passed". +# Every false green a reviewer finds gets a case here. # 3. the UNKNOWN leg — run it against every fixtures//bad/unknown/ and require # a RED exit that carries an UNKNOWN line and no FAIL line. "Could not check" must be # as blocking as "found a violation" and must not be mistakable for one. @@ -160,6 +163,38 @@ for g in "$HERE"/gates/*.sh "$HERE"/gates/*.py; do fi fixture="$FX/$gate/bad" + # THE VIOLATION MARKER IS A PROPERTY OF THE GATE, NOT OF THE HARNESS. `FAIL [` and + # `UNKNOWN [` come from `fail()` and `unknown()` in gates/lib.sh — the SHELL gates' helpers. + # The two vendored Python gates print their own lines (`references: N broken reference(s)`, + # `secrets: N possible credential(s) committed`) and cannot emit either marker, so asserting + # the marker for them would report a gate that detected its planted shape perfectly as + # having a blind spot. That is an assertion BROADER than its message, inside the leg written + # to catch assertions narrower than theirs. + # + # MEASURED, not reasoned. A fixture citing a missing path was planted at + # fixtures/check_references/bad/cases/broken-path/ and run: the gate found it exactly + # ("references: 1 broken reference(s)"), and the unguarded harness answered "went red on + # case 'broken-path' without printing a FAIL line — the red is a blind spot, not the planted + # violation". With the guard below it reports `ok check_references catches case + # 'broken-path'`, which is what happened. + # + # The fixture is not kept, and the reason is a second finding rather than a tidy-up: unlike + # every shell gate, the vendored check_references.py has NO fixtures/ filter, so the planted + # citation is validated as though it were real and the TREE leg goes red on it. No Python + # gate can carry a cases/ fixture until that changes — and changing it means editing a + # vendored file, which is a policy decision and not this file's to make. So this guard is + # correct and currently unexercised, and that is stated here rather than left to look like + # dead code. This file's own header invites the fixture ("Every false green a reviewer finds + # gets a case here"); today the invitation cannot be accepted for these two gates. + # + # Losing the marker check for them costs nothing it was buying: the marker exists to tell a + # violation from an UNKNOWN, and a gate with no UNKNOWN outcome has nothing to confuse. For + # those gates a red is a violation by construction, and the exit code is the whole claim. + case "$name" in + *.sh) markers=1 ;; + *) markers=0 ;; + esac + # ---- 1. the failure leg ---- if [ "$mode" = selftest ]; then # Its own probes are the fixture. Run from a staging directory holding BOTH vendored @@ -222,7 +257,7 @@ for g in "$HERE"/gates/*.sh "$HERE"/gates/*.py; do elif [ "$rc" -ne 1 ]; then echo "SELFTEST FAIL: $gate errored (exit $rc) on case '$cname' instead of reporting a violation" sed 's/^/ /' "$OUT"; bad=1 - elif [ "$n_fail" -eq 0 ]; then + elif [ "$markers" = 1 ] && [ "$n_fail" -eq 0 ]; then echo "SELFTEST FAIL: $gate went red on case '$cname' without printing a FAIL line — the red is a blind spot, not the planted violation" sed 's/^/ /' "$OUT"; bad=1 else @@ -255,10 +290,10 @@ for g in "$HERE"/gates/*.sh "$HERE"/gates/*.py; do if [ "$rc" -eq 0 ]; then echo "SELFTEST FAIL: $gate went GREEN on unknown case '$uname' — it could not check and said so with a pass" sed 's/^/ /' "$OUT"; bad=1 - elif [ "$n_unknown" -eq 0 ]; then + elif [ "$markers" = 1 ] && [ "$n_unknown" -eq 0 ]; then echo "SELFTEST FAIL: $gate went red on unknown case '$uname' without printing an UNKNOWN line — the reader cannot tell a blind spot from a violation" sed 's/^/ /' "$OUT"; bad=1 - elif [ "$n_fail" -gt 0 ]; then + elif [ "$markers" = 1 ] && [ "$n_fail" -gt 0 ]; then echo "SELFTEST FAIL: $gate reported $n_fail violation(s) on unknown case '$uname', which plants none — it is blaming the tree for its own blind spot" sed 's/^/ /' "$OUT"; bad=1 else From d2d219145d6ea1fd04a0375af31208cdd917f075 Mon Sep 17 00:00:00 2001 From: Chii Date: Tue, 8 Sep 2026 22:51:08 +0000 Subject: [PATCH 04/13] Two more false greens the last fix introduced, the false green it did not close, and four false reds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third adversarial round on this branch, and the third to find that a fix commit shipped a regression. 9547e04 introduced two new false greens in the security gate and did not close the one it claimed to. DID NOT CLOSE IT. The baseUrl fallback armed on the VALUE of baseUrl and excluded ".", which is the spelling in Next.js's own Absolute Imports documentation and the one create-next-app ships. So the reproduction in that commit's own message still held, on the commoner spelling: import { admin } from "lib/supabase-admin"; -> ok exit 0 import { admin } from "../lib/supabase-admin"; -> FAIL exit 1 The shipped fixture used "src", so the selftest stayed green over the half that worked — a fixture that covers the case the author happened to test is not coverage. It now arms on the key being present. README asserted the general case and was false for ".". NEW FALSE GREEN 1. Putting declaration files ahead of the implementation let a hand-written .d.ts stand in for the module Node loads. A .d.ts by construction cannot hold a secret, so the gate resolved to it and never read lib/admin.js: with lib/admin.d.ts present: ok [service-role] exit 0 with it deleted: FAIL x2 exit 1 The sidecar was doing the hiding. tsc does prefer the declaration, but that is TypeScript answering "where are the types"; this gate asks "what code runs in the request path", and a declaration is never that answer. Declarations now come last, in the emitted->source map and in the extension list both — the second of those predates this release and was found by the same reading. NEW FALSE GREEN 2. The artefact filter was written as "every character no module specifier can contain", which is a claim about garbage that is false about the swallow: `grep -o` matches non-overlapping, so a string ending in `from "` can swallow a REAL import into the artefact. Two reproductions, both red before: const label = "imported from "; import { admin } from "../lib/admin"; -> the whole span became one candidate and was dropped. lib/admin.ts, which reaches SUPABASE_SERVICE_ROLE_KEY, was never walked. import { admin } from "../lib/(group)/admin"; -> a resolvable file whose path holds a Next.js route group, dropped. The first is fixtures/service-role/bad/cases/string-ending-in-from with the newline removed — one character from the fixture that exists to catch it. The filter is now `${` and nothing else: the only shape the extractor produces that cannot be a path. A filter removing a false red made a false green, which is the trade this repository refuses in that direction. FOUR FALSE REDS in migrations-lint, all on files PostgreSQL accepts and that are compliant. `mydb.public.orders` was read as schema mydb, table public — a violation naming a table that does not exist, and then the ALTER side did not admit the database prefix either. `public."order items"` truncated at the space; `public."a.b"` split on the quoted dot. `ALTER TABLE ONLY`, which is what pg_dump emits, was not matched at all, so a table WITH RLS reported as having none. The qualifier is now parsed quote-aware, the last two components are schema and table, and both interpolated halves are ERE-escaped — without that, a name containing `.` built a pattern matching any character, a matcher wider than the name it was given. Eleven migrations-lint spellings and seven service-role trees re-measured. Three new fixtures, each verified green under 9547e04 and red now: baseurl-dot-absolute- import, declaration-sidecar, one-line-swallow. The false reds cannot carry fixtures — the model holds bad trees only — so they are in the README, where a sentence claiming the gate could not read a `create table` inside a DO block or an `execute` string is also corrected: it reads and reds on all three, measured. Only `execute format('create table %I …')` is genuinely unread. Selftest green. All six shell gates green on the template tree at deda1fa. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BfFq2KvV1CCkPs5WnV1AsA --- README.md | 43 +++++++----- .../baseurl-dot-absolute-import/app/page.tsx | 10 +++ .../lib/supabase-admin.ts | 1 + .../baseurl-dot-absolute-import/package.json | 1 + .../baseurl-dot-absolute-import/tsconfig.json | 1 + .../cases/declaration-sidecar/app/page.jsx | 9 +++ .../cases/declaration-sidecar/lib/admin.d.ts | 1 + .../cases/declaration-sidecar/lib/admin.js | 1 + .../cases/declaration-sidecar/package.json | 1 + .../bad/unknown/one-line-swallow/app/page.tsx | 11 +++ .../bad/unknown/one-line-swallow/lib/admin.ts | 1 + .../bad/unknown/one-line-swallow/package.json | 1 + gates/migrations-lint.sh | 48 +++++++++++-- gates/service-role.sh | 70 +++++++++++++++---- 14 files changed, 163 insertions(+), 36 deletions(-) create mode 100644 fixtures/service-role/bad/cases/baseurl-dot-absolute-import/app/page.tsx create mode 100644 fixtures/service-role/bad/cases/baseurl-dot-absolute-import/lib/supabase-admin.ts create mode 100644 fixtures/service-role/bad/cases/baseurl-dot-absolute-import/package.json create mode 100644 fixtures/service-role/bad/cases/baseurl-dot-absolute-import/tsconfig.json create mode 100644 fixtures/service-role/bad/cases/declaration-sidecar/app/page.jsx create mode 100644 fixtures/service-role/bad/cases/declaration-sidecar/lib/admin.d.ts create mode 100644 fixtures/service-role/bad/cases/declaration-sidecar/lib/admin.js create mode 100644 fixtures/service-role/bad/cases/declaration-sidecar/package.json create mode 100644 fixtures/service-role/bad/unknown/one-line-swallow/app/page.tsx create mode 100644 fixtures/service-role/bad/unknown/one-line-swallow/lib/admin.ts create mode 100644 fixtures/service-role/bad/unknown/one-line-swallow/package.json diff --git a/README.md b/README.md index c823b66..07a8bbd 100644 --- a/README.md +++ b/README.md @@ -298,10 +298,13 @@ header, which is the honest account of what that scanner does not do. reviewer nobody has run is a reviewer nobody has seen fail. - **The migrations-lint gate matches `create table` by regex.** It reads `unlogged`, `temp` and `temporary` tables, a quoted or unquoted schema qualifier with or without whitespace - around the dot, and it carries the **schema and table together**: RLS on `archive.orders` - does not satisfy a `create table public.orders`, and an unqualified name is normalised to - `public`. What it does not read is a table created inside a function body, a `DO $$` block, - or by dynamic SQL. + around the dot, a quoted identifier containing a space or a dot, and an optional database + qualifier — and it carries the **schema and table together**: RLS on `archive.orders` does + not satisfy a `create table public.orders`, and an unqualified name is normalised to + `public`. It reads a `create table` inside a function body, inside a `DO $$` block, and + inside an `execute '…'` string, and reds on all three — measured; the sentence that used to + stand here said it read none of them, which was false in every clause. What it genuinely + cannot read is a name assembled at runtime: `execute format('create table %I …')`. - **The service-role gate scans each module with newlines collapsed**, so that a statement spanning lines is seen as one — a multi-line `await import(` was a false green before v1.2.1. The cost is that a `//` comment runs into the code after it, so `import(` mentioned @@ -317,12 +320,16 @@ header, which is the honest account of what that scanner does not do. If the `paths` object is present and no alias can be parsed out of it, that is UNKNOWN too. It also maps the **emitted** extension back to the source one (`./m.mjs` → `m.mts`/`.d.mts`, `.cjs` → `.cts`/`.d.cts`, `.js` → `.ts`/`.tsx`/`.d.ts`), which `moduleResolution: nodenext` - requires you to write and which was a false red before v1.2.1; the source is probed before - the emitted file, so a stale `.mjs` sitting beside its `.mts` cannot mask a secret added to - the source. A **`baseUrl` with no matching `paths` entry** — Next.js's documented "Absolute - Imports" — is resolved too: `import "lib/admin"` under `baseUrl: "src"` was skipped as a - published package while the same file spelled `../lib/admin` was caught, a false green - chosen by nothing but the spelling. A specifier that appears inside a + requires you to write and which was a false red before v1.2.1. Ordering is + **implementation before declaration**, in both the extension list and the emitted→source + map: a stale `.mjs` beside its `.mts` cannot mask a secret added to the source, and a + hand-written `.d.ts` beside a `.js` cannot stand in for the module Node actually loads. A + declaration file is what TypeScript resolves to and is never what runs in the request path. A **`baseUrl` with no matching `paths` entry** — Next.js's documented "Absolute + Imports" — is resolved too: `import "lib/admin"` was skipped as a published package while + the same file spelled `../lib/admin` was caught, a false green chosen by nothing but the + spelling. It arms on the **presence of the key**, not its value: the first version tested + the value and so missed `"baseUrl": "."`, which is the spelling in Next.js's own docs and + the one `create-next-app` ships. A specifier that appears inside a comment or a string is followed as though it were real — over-inclusive, which costs a false red rather than a false green. What it cannot follow at all it calls UNKNOWN. @@ -334,13 +341,15 @@ header, which is the honest account of what that scanner does not do. A `baseUrl` **inherited from a base config** is not read either, which makes alias targets fail to resolve and go UNKNOWN: a false red, and the direction this toolkit errs in. A baseUrl declared in the repo's own tsconfig *is* read, including for bare specifiers. -- **The service-role gate drops extractor artefacts before classifying them.** A candidate - carrying a character no module specifier can contain — `` ` ``, `{`, `}`, `<`, `>`, `;`, - `(`, `)`, `,`, `=`, `|`, `*` — is discarded rather than reported. Without that, a template - literal in ordinary source (`` `select … from "${table}"` ``) became a blocking UNKNOWN - naming an import that does not exist, in a file that imports nothing, with no action a - fixer could take. The trade is the usual one in the other direction: a specifier genuinely - containing one of those characters is skipped silently. +- **The service-role gate drops one extractor artefact before classifying it: `${`.** A + template literal in ordinary source (`` `select … from "${table}"` ``) otherwise became a + blocking UNKNOWN naming an import that does not exist, in a file that imports nothing, with + no action a fixer could take. The filter is deliberately just that one shape. A wider set — + every character "no module specifier can contain" — was tried and reverted: the extractor's + swallow does not only invent garbage, it can swallow a **real** import into it, and a wide + filter turned two reproduced reds into silent greens (a real import on the same line as a + string ending in `from "`, and a resolvable path containing a Next.js route group, + `../lib/(group)/admin`). Everything that is not `${` is still classified and reported. - **The service-role gate does not look inside published packages.** A bare specifier (`react`, `@supabase/ssr`) is out of scope by definition, so a third-party module that reads `process.env.SUPABASE_SECRET_KEY` itself is invisible to it. Repo source is what it diff --git a/fixtures/service-role/bad/cases/baseurl-dot-absolute-import/app/page.tsx b/fixtures/service-role/bad/cases/baseurl-dot-absolute-import/app/page.tsx new file mode 100644 index 0000000..0335ecb --- /dev/null +++ b/fixtures/service-role/bad/cases/baseurl-dot-absolute-import/app/page.tsx @@ -0,0 +1,10 @@ +// `"baseUrl": "."` is the spelling in Next.js's own Absolute Imports documentation and the +// one create-next-app ships. The first version of the baseUrl fallback armed on the VALUE +// rather than on the key being present, and "." is exactly the value it excluded — so the +// false green it was written to close stayed open on the commonest spelling, while the +// fixture beside this one used "src" and kept the selftest green over the half that worked. +import { admin } from "lib/supabase-admin"; + +export default function Page() { + return admin; +} diff --git a/fixtures/service-role/bad/cases/baseurl-dot-absolute-import/lib/supabase-admin.ts b/fixtures/service-role/bad/cases/baseurl-dot-absolute-import/lib/supabase-admin.ts new file mode 100644 index 0000000..7b4886c --- /dev/null +++ b/fixtures/service-role/bad/cases/baseurl-dot-absolute-import/lib/supabase-admin.ts @@ -0,0 +1 @@ +export const admin = process.env.SUPABASE_SERVICE_ROLE_KEY; diff --git a/fixtures/service-role/bad/cases/baseurl-dot-absolute-import/package.json b/fixtures/service-role/bad/cases/baseurl-dot-absolute-import/package.json new file mode 100644 index 0000000..e5b4d86 --- /dev/null +++ b/fixtures/service-role/bad/cases/baseurl-dot-absolute-import/package.json @@ -0,0 +1 @@ +{ "name": "fixture-baseurl-dot-absolute-import", "dependencies": { "next": "15.0.0" } } diff --git a/fixtures/service-role/bad/cases/baseurl-dot-absolute-import/tsconfig.json b/fixtures/service-role/bad/cases/baseurl-dot-absolute-import/tsconfig.json new file mode 100644 index 0000000..af8c5aa --- /dev/null +++ b/fixtures/service-role/bad/cases/baseurl-dot-absolute-import/tsconfig.json @@ -0,0 +1 @@ +{ "compilerOptions": { "baseUrl": "." } } diff --git a/fixtures/service-role/bad/cases/declaration-sidecar/app/page.jsx b/fixtures/service-role/bad/cases/declaration-sidecar/app/page.jsx new file mode 100644 index 0000000..c96179c --- /dev/null +++ b/fixtures/service-role/bad/cases/declaration-sidecar/app/page.jsx @@ -0,0 +1,9 @@ +// A hand-written declaration sits beside the implementation. Probing the declaration first +// resolved to a file that BY CONSTRUCTION cannot hold a secret, and lib/admin.js — the module +// Node actually loads — was never read. Deleting the .d.ts turned the same tree red, which is +// the sidecar doing the hiding. +import { admin } from "../lib/admin.js"; + +export default function Page() { + return admin; +} diff --git a/fixtures/service-role/bad/cases/declaration-sidecar/lib/admin.d.ts b/fixtures/service-role/bad/cases/declaration-sidecar/lib/admin.d.ts new file mode 100644 index 0000000..2f29f42 --- /dev/null +++ b/fixtures/service-role/bad/cases/declaration-sidecar/lib/admin.d.ts @@ -0,0 +1 @@ +export declare const admin: string | undefined; diff --git a/fixtures/service-role/bad/cases/declaration-sidecar/lib/admin.js b/fixtures/service-role/bad/cases/declaration-sidecar/lib/admin.js new file mode 100644 index 0000000..7b4886c --- /dev/null +++ b/fixtures/service-role/bad/cases/declaration-sidecar/lib/admin.js @@ -0,0 +1 @@ +export const admin = process.env.SUPABASE_SERVICE_ROLE_KEY; diff --git a/fixtures/service-role/bad/cases/declaration-sidecar/package.json b/fixtures/service-role/bad/cases/declaration-sidecar/package.json new file mode 100644 index 0000000..a211ace --- /dev/null +++ b/fixtures/service-role/bad/cases/declaration-sidecar/package.json @@ -0,0 +1 @@ +{ "name": "fixture-declaration-sidecar", "type": "module", "dependencies": { "next": "15.0.0" } } diff --git a/fixtures/service-role/bad/unknown/one-line-swallow/app/page.tsx b/fixtures/service-role/bad/unknown/one-line-swallow/app/page.tsx new file mode 100644 index 0000000..8ea6d2a --- /dev/null +++ b/fixtures/service-role/bad/unknown/one-line-swallow/app/page.tsx @@ -0,0 +1,11 @@ +// `grep -o` matches non-overlapping, so a string ending in `from "` swallows the import that +// follows it. With both statements on ONE line, both extractor passes see the same span and +// there is no unswallowed copy to fall back on. That span must still be REPORTED: a wide +// artefact filter dropped it silently and the module below was never walked — a false green +// made by the filter that was removing a false red. This is the sibling of +// cases/string-ending-in-from, which is this file with a newline between the statements. +const label = "imported from "; import { admin } from "../lib/admin"; + +export default function Page() { + return admin; +} diff --git a/fixtures/service-role/bad/unknown/one-line-swallow/lib/admin.ts b/fixtures/service-role/bad/unknown/one-line-swallow/lib/admin.ts new file mode 100644 index 0000000..7b4886c --- /dev/null +++ b/fixtures/service-role/bad/unknown/one-line-swallow/lib/admin.ts @@ -0,0 +1 @@ +export const admin = process.env.SUPABASE_SERVICE_ROLE_KEY; diff --git a/fixtures/service-role/bad/unknown/one-line-swallow/package.json b/fixtures/service-role/bad/unknown/one-line-swallow/package.json new file mode 100644 index 0000000..127c127 --- /dev/null +++ b/fixtures/service-role/bad/unknown/one-line-swallow/package.json @@ -0,0 +1 @@ +{ "name": "fixture-one-line-swallow", "dependencies": { "next": "15.0.0" } } diff --git a/gates/migrations-lint.sh b/gates/migrations-lint.sh index 15d5615..c7f56b6 100755 --- a/gates/migrations-lint.sh +++ b/gates/migrations-lint.sh @@ -93,22 +93,58 @@ for up in "$MIG"/*.sql; do # gate does not check. while IFS="$(printf '\t')" read -r sch tbl; do [ -n "$tbl" ] || continue + # ESCAPED BEFORE INTERPOLATION. A quoted identifier can now carry any character, and both + # halves are pasted straight into an ERE — `create table public."order.items"` would + # otherwise build a pattern whose `.` matches any character and so accepts RLS on a table + # that is not this one. A matcher wider than the name it was given. + ere_escape='s/[.*+?^$(){}|]/\\&/g;s/\[/\\[/g;s/\]/\\]/g' + sch_re="$(printf '%s' "$sch" | sed -E "$ere_escape")" + tbl_re="$(printf '%s' "$tbl" | sed -E "$ere_escape")" + # The optional DATABASE qualifier. `create table mydb.public.orders` is accepted by + # PostgreSQL, and the create side now reads it correctly (schema `public`, table `orders`) + # — but the ALTER side did not admit the `mydb.` prefix, so the matching + # `alter table mydb.public.orders enable row level security` in the same file did not + # match and a COMPLIANT file went red. Half a fix is a red a fixer cannot act on. + dbq="((\"[^\"]*\"|[a-z_][a-z0-9_]*)[[:space:]]*\.[[:space:]]*)?" if [ "$sch" = "public" ]; then - qual="(\"?public\"?[[:space:]]*\.[[:space:]]*)?" + qual="(${dbq}\"?public\"?[[:space:]]*\.[[:space:]]*)?" else - qual="\"?${sch}\"?[[:space:]]*\.[[:space:]]*" + qual="${dbq}\"?${sch_re}\"?[[:space:]]*\.[[:space:]]*" fi - if ! grep -qiE "alter[[:space:]]+table[[:space:]]+(if[[:space:]]+exists[[:space:]]+)?${qual}\"?${tbl}\"?[[:space:]]+enable[[:space:]]+row[[:space:]]+level[[:space:]]+security" "$STRIPPED"; then + # `ONLY` is valid PostgreSQL and is what pg_dump emits, so a table WITH RLS was reported as + # having none without it. + if ! grep -qiE "alter[[:space:]]+table[[:space:]]+(if[[:space:]]+exists[[:space:]]+)?(only[[:space:]]+)?${qual}\"?${tbl_re}\"?[[:space:]]+enable[[:space:]]+row[[:space:]]+level[[:space:]]+security" "$STRIPPED"; then fail "$(basename "$up"): table '$sch.$tbl' created without 'enable row level security' in the same file" fi done < <( # Lowercased with `tr` before any sed runs, so no step needs a case-insensitive sed flag. # `s///i` is a GNU extension and this repository has already been bitten once by a GNU-only # regex feature silently matching nothing on BSD (`\s`, 18 occurrences, fixed in v1.2.1). - grep -ioE "create[[:space:]]+((global|local)[[:space:]]+)?((temporary|temp|unlogged)[[:space:]]+)?table[[:space:]]+(if[[:space:]]+not[[:space:]]+exists[[:space:]]+)?(\"?[a-z_][a-z0-9_]*\"?[[:space:]]*\.[[:space:]]*)?\"?[a-z_][a-z0-9_]*" "$STRIPPED" \ + grep -ioE "create[[:space:]]+((global|local)[[:space:]]+)?((temporary|temp|unlogged)[[:space:]]+)?table[[:space:]]+(if[[:space:]]+not[[:space:]]+exists[[:space:]]+)?(\"[^\"]*\"|[a-z_][a-z0-9_]*)([[:space:]]*\.[[:space:]]*(\"[^\"]*\"|[a-z_][a-z0-9_]*)){0,2}" "$STRIPPED" \ | tr '[:upper:]' '[:lower:]' \ - | sed -E 's/"//g; s/[[:space:]]*\.[[:space:]]*/./; s/^create[[:space:]]+((global|local)[[:space:]]+)?((temporary|temp|unlogged)[[:space:]]+)?table[[:space:]]+(if[[:space:]]+not[[:space:]]+exists[[:space:]]+)?//' \ - | awk -F. 'NF==2 {print $1"\t"$2; next} {print "public\t"$1}' + | sed -E 's/^create[[:space:]]+((global|local)[[:space:]]+)?((temporary|temp|unlogged)[[:space:]]+)?table[[:space:]]+(if[[:space:]]+not[[:space:]]+exists[[:space:]]+)?//' \ + | awk ' + # Split on dots OUTSIDE quotes, so `"order items"` survives the space and `"a.b"` + # survives the dot. A blind `awk -F.` read `mydb.public.orders` as schema `mydb`, + # table `public` and reported a violation naming a table that does not exist, over a + # file that was compliant — the same "red a fixer cannot act on" this gate was fixed + # for one release ago, reintroduced by the qualifier that fixed it. + { + n = 0; cur = ""; inq = 0 + for (i = 1; i <= length($0); i++) { + c = substr($0, i, 1) + if (c == "\"") { inq = !inq; continue } + if (c == "." && !inq) { part[++n] = cur; cur = ""; continue } + cur = cur c + } + part[++n] = cur + for (i = 1; i <= n; i++) { gsub(/^[[:space:]]+|[[:space:]]+$/, "", part[i]) } + # The last two components are schema and table; anything before them is the + # database, which PostgreSQL accepts and which names no schema. + if (n >= 2) print part[n-1] "\t" part[n] + else print "public\t" part[n] + } + ' ) # destructive statements outside a WHERE are tier-3 by regex (non-negotiable 4); flag, do not block if grep -qiE "^[[:space:]]*(drop[[:space:]]+table|truncate|delete[[:space:]]+from[[:space:]]+[a-z_.\"]+[[:space:]]*;)" "$STRIPPED"; then diff --git a/gates/service-role.sh b/gates/service-role.sh index 75e766a..c87327e 100755 --- a/gates/service-role.sh +++ b/gates/service-role.sh @@ -100,7 +100,21 @@ BASEURL_SET=0 if [ -f "$TSCONFIG" ]; then base_url="$(sed -nE 's/.*"baseUrl"[[:space:]]*:[[:space:]]*"([^"]*)".*/\1/p' "$TSCONFIG" 2>/dev/null | head -1 || true)" base_url="${base_url#./}"; base_url="${base_url%/}" - if [ -n "$base_url" ] && [ "$base_url" != "." ]; then BASE_DIR="$ROOT/$base_url"; BASEURL_SET=1; fi + if [ -n "$base_url" ] && [ "$base_url" != "." ]; then BASE_DIR="$ROOT/$base_url"; fi + # ARMED ON THE KEY'S PRESENCE, NOT ON ITS VALUE. The first version of the baseUrl fallback + # armed on `[ -n "$base_url" ] && [ "$base_url" != "." ]` — the same condition that decides + # whether BASE_DIR moves — and `"baseUrl": "."` is the spelling in Next.js's own Absolute + # Imports documentation and the one create-next-app ships. So the fallback did not arm on + # the commonest spelling, and the false green it was written to close stayed open there: + # + # import { admin } from "lib/supabase-admin"; -> ok exit 0 + # import { admin } from "../lib/supabase-admin"; -> FAIL exit 1 + # + # Identical to the reproduction in the commit that claimed to fix it, on a different value + # of the same key — and the shipped fixture used "src", so the selftest was green over the + # half that worked. BASE_DIR is already $ROOT when the value is "." or "./", so nothing else + # needs to change: the key being there is the whole condition. + if grep -q '"baseUrl"' "$TSCONFIG" 2>/dev/null; then BASEURL_SET=1; fi # The `paths` object, isolated exactly rather than read line by line. # @@ -335,20 +349,31 @@ $BASE_DIR/$t" # "./types.js"` against a `types.d.ts` type-checks clean under nodenext, which this list # has to know or it re-opens the very `.d.ts` false red the extension list was widened to # close one release ago. + # + # AND THE DECLARATION COMES LAST, which the first version of this list got backwards. With + # a `.js` module and a hand-written `.d.ts` beside it, probing the declaration first + # resolved to a file that BY CONSTRUCTION cannot hold a secret, and the module Node + # actually loads was never read. Measured: `lib/admin.js` reaching + # SUPABASE_SERVICE_ROLE_KEY with a `lib/admin.d.ts` next to it gave `ok`, exit 0 — and + # deleting the .d.ts turned the same tree red, which is the sidecar doing the hiding. + # `tsc --traceResolution` does prefer the declaration, but that is TypeScript answering + # "where are the types"; this gate asks "what code runs in the request path", and a + # declaration file is never that answer. Last still closes the false red above, because + # that case has no implementation file to find. cands="$b" case "$b" in *.mjs) cands="${b%.mjs}.mts -${b%.mjs}.d.mts -$b" ;; +$b +${b%.mjs}.d.mts" ;; *.cjs) cands="${b%.cjs}.cts -${b%.cjs}.d.cts -$b" ;; +$b +${b%.cjs}.d.cts" ;; *.jsx) cands="${b%.jsx}.tsx $b" ;; *.js) cands="${b%.js}.ts ${b%.js}.tsx -${b%.js}.d.ts -$b" ;; +$b +${b%.js}.d.ts" ;; esac while IFS= read -r bb; do [ -n "$bb" ] || continue @@ -356,7 +381,11 @@ $b" ;; # src/types/supabase.d.ts resolved to nothing, which this gate calls UNKNOWN — a permanent # blocking red on a perfectly ordinary line. The selftest cannot catch a false red (its own # note says the fixture model holds bad trees only), so it is fixed here on report. - for ext in "" .ts .tsx .d.ts .mts .cts .js .jsx .mjs .cjs /index.ts /index.tsx /index.d.ts /index.js /index.jsx; do + # Implementations before declarations here too, and for the same reason as the `cands` + # note above: `.d.ts` sat ahead of `.js`, so an extensionless `"../lib/admin"` against a + # `lib/admin.js` with a `lib/admin.d.ts` beside it resolved to the declaration. That one + # is older than this release; the fix for the ordering above is the fix for this. + for ext in "" .ts .tsx .mts .cts .js .jsx .mjs .cjs .d.ts .d.mts .d.cts /index.ts /index.tsx /index.js /index.jsx /index.d.ts; do cand="$bb$ext" # CANONICALISED, not merely tested for existence. Without this the resolved path keeps # whatever `..` the importer's specifier put in it, and TWO SPELLINGS OF ONE FILE ARE @@ -460,15 +489,30 @@ while :; do # UNKNOWN [service-role] app/api/orders/route.ts imports '${table}' ... # # There is no action a fixer can take: the message names an import that does not exist and - # the only way to green is to delete the string. The characters filtered below cannot appear - # in any module specifier, so a candidate carrying one is an artefact of the extractor, not a - # claim about the tree. Note it was the LINE pass that produced it — the old sentence was - # wrong about which pass invents garbage as well as about what became of it. + # the only way to green is to delete the string. Note it was the LINE pass that produced it — + # the old sentence was wrong about which pass invents garbage as well as about what became + # of it. + # + # THE FILTER IS `${` AND NOTHING ELSE, and the first version of it was far wider — every + # candidate carrying a character "no module specifier can contain". That set turned two REAL + # imports into silent skips, which is worse than the false red it was removing, because the + # swallow does not only invent garbage: it can swallow a real import INTO the garbage. + # Measured, both against the previous revision, which reported them: + # + # const label = "imported from "; import { admin } from "../lib/admin"; + # -> the whole span became one candidate, dropped; lib/admin.ts never walked. `ok`. + # import { admin } from "../lib/(group)/admin"; + # -> a resolvable file whose path holds a Next.js route group, dropped. `ok`. + # + # The first of those is `fixtures/service-role/bad/cases/string-ending-in-from/` with the + # newline removed — one character from the fixture that exists to catch exactly it. + # `${` is the whole artefact: it is the only shape the extractor produces that CANNOT be a + # path, and dropping it leaves everything else to be classified and reported as before. specs="$( { grep -oE "(from|import|require)[[:space:]]*\(?[[:space:]]*['\"][^'\"]+['\"]" -- "$file" 2>/dev/null grep -oE "(from|import|require)[[:space:]]*\(?[[:space:]]*['\"][^'\"]+['\"]" -- "$WORK/flat" 2>/dev/null } | sed -E "s/.*['\"]([^'\"]+)['\"]\$/\1/" \ - | grep -vE '[`{}<>;(),=|*]' \ + | grep -vE '[$][{]' \ | sort -u )" set -e From 7ca27202a6b5e71e3d18f19352d2d7646666ced6 Mon Sep 17 00:00:00 2001 From: Chii Date: Tue, 8 Sep 2026 23:13:02 +0000 Subject: [PATCH 05/13] Replace both extractors with tokenisers: the regex was the defect, not its filters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round four plus a CodeRabbit round on d2d2191, and between them they found the same thing from both sides: a filter tuned to stop a false red had reopened a false green, and a filter tuned to stop the false green had reopened the red. Three commits in a row have now traded one for the other. The filter was never the defect. The extractor was. SERVICE-ROLE. `grep -o` matches NON-OVERLAPPING, so a string ending in the word `from` immediately before a quote consumed everything up to the next quote as one invented "specifier". Every version had to choose which way to be wrong: report the span -> a blocking UNKNOWN naming an import that does not exist, on ordinary source. Measured on files that import NOTHING: { note: "Imported from ", source: "the archive" } -> imports ', source: ' /import .* from "(.+)"/ -> imports '(.+)' Array.from(",") -> imports ',' No action a fixer can take; the only way to green is to delete the string. drop the span -> a REAL import swallowed into it is dropped too. Measured: const label = "imported from "; import { admin } from "../lib/admin"; went ok, exit 0, over a module reaching SUPABASE_SERVICE_ROLE_KEY. Neither is fixable downstream, because the filter runs after the damage. The scan is now a tokeniser: strings, template literals, line and block comments and regex literals are recognised as what they are, and a specifier is emitted only from a real from/import/require position. `Array.from(",")` is a method call, not a keyword. The flat pass, its `//`-comment cost, and the artefact filter are all gone — there is no invented span left to report or to drop. The one-line-swallow fixture moved from bad/unknown/ to bad/cases/ after exactly one commit, and that move is the record: the honest answer went from "I could not read this" to "here is what it reaches". What remains unread is JSX text, which no tokeniser that is not also a JSX parser can tell from an import; that is disclosed. MIGRATIONS-LINT, same disease. Both sides are tokenised through one scanner and the (schema, table) pairs compared as STRINGS, so there is no interpolated pattern left to be wider than the name it was given. That closes what three regex versions could not: - a quoted identifier lost its case, so `create table public."Orders"` was satisfied by RLS on `orders` — two different tables in PostgreSQL, and the quoted PascalCase form is what Prisma and Drizzle emit. A FALSE GREEN, found independently by both reviewers. - a `create table` whose name sat on the NEXT line was not seen at all, because grep is line-scoped. A file with no RLS anywhere passed. A SILENT GREEN. - `""` inside a quoted name ended the name early; a backslash in one escaped into the ERE and let RLS on a different table pass; an empty schema shifted the IFS read and skipped the check entirely. Also: tsconfig.json is read from a comment-stripped copy. `grep -q '"baseUrl"'` armed the baseUrl fallback on a COMMENTED-OUT key, and a repo directory named react/ then made `import React from "react"` a violation against the package. README: the migrations-lint and service-role gap bullets are rewritten, including the sentence added two commits ago claiming the gate could not read a create table "assembled at runtime". It reads DO blocks, function bodies and `execute '…'` strings and reds on all three; what it actually cannot read is `execute format`, string-concatenated DDL, `select … into`, and a line whose earlier string literal contains `--`. Those are silent passes and now say so. 21 migrations-lint spellings and 11 service-role trees re-measured. Two new fixtures, both verified green under d2d2191 and red now. Selftest green. All six shell gates green on the template tree at deda1fa. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BfFq2KvV1CCkPs5WnV1AsA --- README.md | 92 +++---- .../migrations/20260104000000_orders.down.sql | 1 + .../migrations/20260104000000_orders.sql | 8 + .../migrations/20260103000000_orders.down.sql | 2 + .../migrations/20260103000000_orders.sql | 8 + .../bad/cases/one-line-swallow/app/page.tsx | 16 ++ .../one-line-swallow/lib/admin.ts | 0 .../one-line-swallow/package.json | 0 .../bad/unknown/one-line-swallow/app/page.tsx | 11 - gates/migrations-lint.sh | 214 ++++++++------ gates/service-role.sh | 260 +++++++++++++----- 11 files changed, 401 insertions(+), 211 deletions(-) create mode 100644 fixtures/migrations-lint/bad/cases/multiline-create-table/supabase/migrations/20260104000000_orders.down.sql create mode 100644 fixtures/migrations-lint/bad/cases/multiline-create-table/supabase/migrations/20260104000000_orders.sql create mode 100644 fixtures/migrations-lint/bad/cases/quoted-identifier-case/supabase/migrations/20260103000000_orders.down.sql create mode 100644 fixtures/migrations-lint/bad/cases/quoted-identifier-case/supabase/migrations/20260103000000_orders.sql create mode 100644 fixtures/service-role/bad/cases/one-line-swallow/app/page.tsx rename fixtures/service-role/bad/{unknown => cases}/one-line-swallow/lib/admin.ts (100%) rename fixtures/service-role/bad/{unknown => cases}/one-line-swallow/package.json (100%) delete mode 100644 fixtures/service-role/bad/unknown/one-line-swallow/app/page.tsx diff --git a/README.md b/README.md index 07a8bbd..3614b5f 100644 --- a/README.md +++ b/README.md @@ -296,60 +296,56 @@ header, which is the honest account of what that scanner does not do. here and is called from consumer repositories; wiring a self-call is a separate change, so until then the fixture procedure is the only thing that exercises it. Stated because a reviewer nobody has run is a reviewer nobody has seen fail. -- **The migrations-lint gate matches `create table` by regex.** It reads `unlogged`, `temp` - and `temporary` tables, a quoted or unquoted schema qualifier with or without whitespace - around the dot, a quoted identifier containing a space or a dot, and an optional database - qualifier — and it carries the **schema and table together**: RLS on `archive.orders` does - not satisfy a `create table public.orders`, and an unqualified name is normalised to - `public`. It reads a `create table` inside a function body, inside a `DO $$` block, and - inside an `execute '…'` string, and reds on all three — measured; the sentence that used to - stand here said it read none of them, which was false in every clause. What it genuinely - cannot read is a name assembled at runtime: `execute format('create table %I …')`. -- **The service-role gate scans each module with newlines collapsed**, so that a statement - spanning lines is seen as one — a multi-line `await import(` was a false green before - v1.2.1. The cost is that a `//` comment runs into the code after it, so `import(` mentioned - inside a comment can raise a spurious UNKNOWN. Over-inclusive, and that is the direction - this toolkit errs in: a visible red someone can argue with, never a green nobody questions. -- **The service-role gate reads imports with a regex, not a parser.** It follows literal - `from`, `import` and `require` specifiers, resolving `./`, `../`, absolute paths, and - **every alias declared in tsconfig `compilerOptions.paths`** — not just `@/*`. A specifier - matching no declared alias is treated as a published package **only if it could be one**: - from v1.2.1 the claim is tested against npm's name shape, so `@/lib/secret` — no alias, and - no valid empty scope — is UNKNOWN rather than a dependency to skip. That was a false green - before. A specifier that matches an alias and resolves to no file is UNKNOWN, never skipped. - If the `paths` object is present and no alias can be parsed out of it, that is UNKNOWN too. - It also maps the **emitted** extension back to the source one (`./m.mjs` → `m.mts`/`.d.mts`, - `.cjs` → `.cts`/`.d.cts`, `.js` → `.ts`/`.tsx`/`.d.ts`), which `moduleResolution: nodenext` - requires you to write and which was a false red before v1.2.1. Ordering is - **implementation before declaration**, in both the extension list and the emitted→source - map: a stale `.mjs` beside its `.mts` cannot mask a secret added to the source, and a - hand-written `.d.ts` beside a `.js` cannot stand in for the module Node actually loads. A - declaration file is what TypeScript resolves to and is never what runs in the request path. A **`baseUrl` with no matching `paths` entry** — Next.js's documented "Absolute - Imports" — is resolved too: `import "lib/admin"` was skipped as a published package while - the same file spelled `../lib/admin` was caught, a false green chosen by nothing but the - spelling. It arms on the **presence of the key**, not its value: the first version tested - the value and so missed `"baseUrl": "."`, which is the spelling in Next.js's own docs and - the one `create-next-app` ships. A specifier that appears inside a - comment or a string is - followed as though it were real — over-inclusive, which costs a false red rather than a - false green. What it cannot follow at all it calls UNKNOWN. +- **The migrations-lint gate tokenises SQL; it does not match it.** Statements are scanned + once — quoted identifiers with their `""` escapes, any schema or database qualifier, + `unlogged`/`temp`/`temporary`, `IF [NOT] EXISTS`, `ALTER TABLE ONLY`, a statement spanning + lines — and the created and RLS-enabled **(schema, table) pairs are compared as strings**. + So RLS on `archive.orders` does not satisfy a `create table public.orders`; an unqualified + name normalises to `public`; and a quoted identifier keeps its case, because PostgreSQL + folds `Orders` to `orders` but keeps `"Orders"` distinct — Prisma and Drizzle emit the + quoted PascalCase form. Three successive regex versions each traded one error for another + here; the pair comparison leaves no interpolated pattern to be wider than the name it was + given. It reads a `create table` inside a function body, a `DO $$` block, and an + `execute '…'` string, and reds on all three. What it does **not** read: a name assembled at + runtime (`execute format('create table %I …')` or string concatenation), `select … into`, + and a statement on a line where an earlier string literal contains `--`, which the comment + stripper takes for a comment. Those are silent passes, and they are the reason this bullet + lists them. +- **The service-role gate tokenises JavaScript; it does not match it.** Strings, template + literals, line and block comments and regex literals are recognised as what they are, and a + specifier is emitted only from a real `from`/`import`/`require` position. This replaced a + `grep -o` extractor whose non-overlapping window let a string ending in the word `from` + consume the following import into one invented span — a defect with no correct filter, + because reporting the span was a blocking UNKNOWN on ordinary source (`Array.from(",")`, a + regex literal, `{ note: "Imported from " }`) and dropping it lost a real import that had + been swallowed. Both were measured, in three consecutive review rounds, before the + extractor itself was replaced. A multi-line `await import(` is followed; a `//` comment no + longer runs into the code after it. What it still cannot read is **JSX text**: `

Copied + from "a" to "b"

` puts `from` before a quote and nothing short of a JSX parser can tell + that from an import, so a candidate carrying `<`, `>`, `{` or `}` is dropped as text. +- **The service-role gate resolves like TypeScript, with one deliberate divergence.** It + follows `./`, `../`, absolute paths, and **every alias declared in tsconfig + `compilerOptions.paths`** — not just `@/*` — plus a **`baseUrl` with no matching `paths` + entry**, which is Next.js's documented "Absolute Imports" and which was a false green + chosen by nothing but the spelling of the import. `baseUrl` arms on the presence of the + key, read from a **comment-stripped** copy of tsconfig.json, because a commented-out + `// "baseUrl": "."` is not configuration. A specifier matching no alias is called a + published package **only if it could be one** — tested against npm's name shape, so + `@/lib/secret` is UNKNOWN, not a dependency to skip. One that matches an alias and resolves + to no file is UNKNOWN, never skipped; a `paths` object nothing parses out of is UNKNOWN too. + It maps the **emitted** extension back to the source (`./m.mjs` → `m.mts`, `.cjs` → `.cts`, + `.js` → `.ts`/`.tsx`) as `moduleResolution: nodenext` requires. **The divergence: + implementation before declaration.** TypeScript resolves `./admin.js` to `admin.d.ts` when + both exist; this gate takes `admin.js`, because a declaration file by construction cannot + hold a secret and the question here is what code runs in the request path, not where the + types are. - **The service-role gate does not follow tsconfig `extends`.** Aliases are read from the repo's own `tsconfig.json` only. TypeScript does not deep-merge `paths` — measured with tsc 5.6.3: a child that declares `paths` REPLACES the base's object entirely, so a base's `@/*` is already dead in that tree — but a child that declares `extends` and no `paths` of its own inherits them, and this gate cannot see them. That case is UNKNOWN, not a pass. A `baseUrl` **inherited from a base config** is not read either, which makes alias targets - fail to resolve and go UNKNOWN: a false red, and the direction this toolkit errs in. A - baseUrl declared in the repo's own tsconfig *is* read, including for bare specifiers. -- **The service-role gate drops one extractor artefact before classifying it: `${`.** A - template literal in ordinary source (`` `select … from "${table}"` ``) otherwise became a - blocking UNKNOWN naming an import that does not exist, in a file that imports nothing, with - no action a fixer could take. The filter is deliberately just that one shape. A wider set — - every character "no module specifier can contain" — was tried and reverted: the extractor's - swallow does not only invent garbage, it can swallow a **real** import into it, and a wide - filter turned two reproduced reds into silent greens (a real import on the same line as a - string ending in `from "`, and a resolvable path containing a Next.js route group, - `../lib/(group)/admin`). Everything that is not `${` is still classified and reported. + fail to resolve and go UNKNOWN: a false red, and the direction this toolkit errs in. - **The service-role gate does not look inside published packages.** A bare specifier (`react`, `@supabase/ssr`) is out of scope by definition, so a third-party module that reads `process.env.SUPABASE_SECRET_KEY` itself is invisible to it. Repo source is what it diff --git a/fixtures/migrations-lint/bad/cases/multiline-create-table/supabase/migrations/20260104000000_orders.down.sql b/fixtures/migrations-lint/bad/cases/multiline-create-table/supabase/migrations/20260104000000_orders.down.sql new file mode 100644 index 0000000..94b5e0d --- /dev/null +++ b/fixtures/migrations-lint/bad/cases/multiline-create-table/supabase/migrations/20260104000000_orders.down.sql @@ -0,0 +1 @@ +drop table if exists public.orders; diff --git a/fixtures/migrations-lint/bad/cases/multiline-create-table/supabase/migrations/20260104000000_orders.sql b/fixtures/migrations-lint/bad/cases/multiline-create-table/supabase/migrations/20260104000000_orders.sql new file mode 100644 index 0000000..1270387 --- /dev/null +++ b/fixtures/migrations-lint/bad/cases/multiline-create-table/supabase/migrations/20260104000000_orders.sql @@ -0,0 +1,8 @@ +-- The table name is on the line after `create table`. grep is line-scoped, so the statement +-- was not seen at all and the file passed with no RLS anywhere in it: a silent green, which +-- is the one outcome this toolkit refuses. The tokeniser reads statements, not lines. +create table + public.orders ( + id uuid primary key, + owner uuid + ); diff --git a/fixtures/migrations-lint/bad/cases/quoted-identifier-case/supabase/migrations/20260103000000_orders.down.sql b/fixtures/migrations-lint/bad/cases/quoted-identifier-case/supabase/migrations/20260103000000_orders.down.sql new file mode 100644 index 0000000..d6d4f22 --- /dev/null +++ b/fixtures/migrations-lint/bad/cases/quoted-identifier-case/supabase/migrations/20260103000000_orders.down.sql @@ -0,0 +1,2 @@ +drop table if exists public."Orders"; +drop table if exists public.orders; diff --git a/fixtures/migrations-lint/bad/cases/quoted-identifier-case/supabase/migrations/20260103000000_orders.sql b/fixtures/migrations-lint/bad/cases/quoted-identifier-case/supabase/migrations/20260103000000_orders.sql new file mode 100644 index 0000000..aefb0a7 --- /dev/null +++ b/fixtures/migrations-lint/bad/cases/quoted-identifier-case/supabase/migrations/20260103000000_orders.sql @@ -0,0 +1,8 @@ +-- PostgreSQL folds an unquoted identifier to lower case and keeps a quoted one exactly, so +-- "Orders" and orders are two different tables. The extractor lowercased both and the RLS +-- search was case-insensitive, so RLS on one satisfied a create of the other — a false green. +-- The quoted PascalCase form is what Prisma and Drizzle emit, so this is not a contrivance. +create table public."Orders" (id uuid primary key, owner uuid); + +create table public.orders (id uuid primary key, owner uuid); +alter table public.orders enable row level security; diff --git a/fixtures/service-role/bad/cases/one-line-swallow/app/page.tsx b/fixtures/service-role/bad/cases/one-line-swallow/app/page.tsx new file mode 100644 index 0000000..c3cdb3c --- /dev/null +++ b/fixtures/service-role/bad/cases/one-line-swallow/app/page.tsx @@ -0,0 +1,16 @@ +// A string ending in the word `from`, immediately before a real import, on ONE physical line. +// This is the shape that broke the old grep extractor in both directions: reporting the +// invented span was a blocking UNKNOWN naming an import that does not exist, and dropping it +// lost the real import below and returned `ok`, exit 0, over a module reaching the secret. +// +// The tokeniser does neither — it recognises the string as a string and the import as an +// import, so this fixture asserts the VIOLATION, not a blind spot. It lived under +// bad/unknown/ for exactly one commit, which is the record of the gate having got better: +// the honest answer moved from "I could not read this" to "here is what it reaches". +// +// cases/string-ending-in-from is this file with a newline between the two statements. +const label = "imported from "; import { admin } from "../lib/admin"; + +export default function Page() { + return admin; +} diff --git a/fixtures/service-role/bad/unknown/one-line-swallow/lib/admin.ts b/fixtures/service-role/bad/cases/one-line-swallow/lib/admin.ts similarity index 100% rename from fixtures/service-role/bad/unknown/one-line-swallow/lib/admin.ts rename to fixtures/service-role/bad/cases/one-line-swallow/lib/admin.ts diff --git a/fixtures/service-role/bad/unknown/one-line-swallow/package.json b/fixtures/service-role/bad/cases/one-line-swallow/package.json similarity index 100% rename from fixtures/service-role/bad/unknown/one-line-swallow/package.json rename to fixtures/service-role/bad/cases/one-line-swallow/package.json diff --git a/fixtures/service-role/bad/unknown/one-line-swallow/app/page.tsx b/fixtures/service-role/bad/unknown/one-line-swallow/app/page.tsx deleted file mode 100644 index 8ea6d2a..0000000 --- a/fixtures/service-role/bad/unknown/one-line-swallow/app/page.tsx +++ /dev/null @@ -1,11 +0,0 @@ -// `grep -o` matches non-overlapping, so a string ending in `from "` swallows the import that -// follows it. With both statements on ONE line, both extractor passes see the same span and -// there is no unswallowed copy to fall back on. That span must still be REPORTED: a wide -// artefact filter dropped it silently and the module below was never walked — a false green -// made by the filter that was removing a false red. This is the sibling of -// cases/string-ending-in-from, which is this file with a newline between the statements. -const label = "imported from "; import { admin } from "../lib/admin"; - -export default function Page() { - return admin; -} diff --git a/gates/migrations-lint.sh b/gates/migrations-lint.sh index c7f56b6..c871469 100755 --- a/gates/migrations-lint.sh +++ b/gates/migrations-lint.sh @@ -54,7 +54,10 @@ strip_sql_comments() { } STRIPPED="$(mktemp)" -trap 'rm -f "$STRIPPED"' EXIT +WORK_TOK="$(mktemp)" +WORK_C="$(mktemp)" +WORK_R="$(mktemp)" +trap 'rm -f "$STRIPPED" "$WORK_TOK" "$WORK_C" "$WORK_R"' EXIT for up in "$MIG"/*.sql; do [ -e "$up" ] || continue @@ -62,90 +65,141 @@ for up in "$MIG"/*.sql; do down="${up%.sql}.down.sql" [ -f "$down" ] || fail "no rollback: $(basename "$up") needs $(basename "$down")" strip_sql_comments "$up" > "$STRIPPED" - # tables created here must enable RLS here. + # TABLES CREATED HERE MUST ENABLE RLS HERE — DECIDED BY TOKENISING, NOT BY MATCHING. # - # THE SCHEMA QUALIFIER IS PART OF THE TABLE'S IDENTITY AND IS CARRIED THROUGH. Three - # spellings had to be reconciled, and getting two of them right while dropping the third - # cost a false green: + # This check was a regex three times over and produced a finding in each of three + # consecutive review rounds, alternating direction every time: # - # - `create table "public"."orders"` is one identifier per quoted part. The pattern used - # to name only `public\.` unquoted, so the leading `"?` swallowed the opening quote and - # `public` was read as the TABLE. Where a table genuinely named `public` had RLS the - # file PASSED and `orders` was never checked; where it did not, the violation named the - # wrong table — a red a fixer cannot act on. - # - Fixing that by matching ANY schema generically on both sides, while the create side - # still discarded the schema, made the check assert only "SOME table called orders, in - # SOME schema, has RLS" — under a message naming one specific table. Measured: - # `create table public.orders` + `alter table archive.orders enable row level security` - # went from red to `ok`, over a state PostgreSQL 16 accepts and in which public.orders - # really is unprotected. Two schemas holding a same-named table is an ordinary layout. - # A widened matcher under an unwidened message is this repository's recurring defect, - # introduced here in the very commit that removed another instance of it. - # - So the pair travels together. Unqualified is normalised to `public`, which is what an - # unqualified name resolves to under the default search_path, and an unqualified ALTER - # is therefore accepted only for a table created in `public`. + # - `create table "public"."orders"` read `public` as the table. Where a table genuinely + # named `public` had RLS the file PASSED — a false green. + # - Widening the ALTER side to any schema while the create side discarded it made the + # check assert "SOME table called orders, in SOME schema, has RLS" under a message + # naming one table: `alter table archive.orders` satisfied `create table public.orders`. + # Another false green, added by the commit that removed the first one. + # - Carrying the pair fixed that and broke four compliant files instead: + # `mydb.public.orders`, `public."order items"`, `public."a.b"`, and `ALTER TABLE ONLY` + # (which is what pg_dump emits) — reds naming tables that do not exist. + # - And still open after all three: a quoted identifier lost its case, so + # `create table public."Orders"` was satisfied by RLS on `orders`, which PostgreSQL + # treats as a different table and which is what Prisma and Drizzle emit; a `""` inside + # a quoted name ended the name early; a name containing a regex metacharacter built a + # matcher wider than itself; a `create table` whose name sat on the NEXT line was not + # seen at all, because grep is line-scoped — a silent pass. # - # `unlogged` and `temp`/`temporary` tables are matched too. They were invisible to - # `create[[:space:]]+table`, so an `create unlogged table orders` with no RLS anywhere was - # never checked at all — `ok`, exit 0. RLS applies to unlogged tables; verified on - # PostgreSQL 16. Whitespace around the qualifier dot (`public . orders`, which Postgres - # accepts) is matched for the same reason: a spelling the gate cannot read is a table the - # gate does not check. - while IFS="$(printf '\t')" read -r sch tbl; do - [ -n "$tbl" ] || continue - # ESCAPED BEFORE INTERPOLATION. A quoted identifier can now carry any character, and both - # halves are pasted straight into an ERE — `create table public."order.items"` would - # otherwise build a pattern whose `.` matches any character and so accepts RLS on a table - # that is not this one. A matcher wider than the name it was given. - ere_escape='s/[.*+?^$(){}|]/\\&/g;s/\[/\\[/g;s/\]/\\]/g' - sch_re="$(printf '%s' "$sch" | sed -E "$ere_escape")" - tbl_re="$(printf '%s' "$tbl" | sed -E "$ere_escape")" - # The optional DATABASE qualifier. `create table mydb.public.orders` is accepted by - # PostgreSQL, and the create side now reads it correctly (schema `public`, table `orders`) - # — but the ALTER side did not admit the `mydb.` prefix, so the matching - # `alter table mydb.public.orders enable row level security` in the same file did not - # match and a COMPLIANT file went red. Half a fix is a red a fixer cannot act on. - dbq="((\"[^\"]*\"|[a-z_][a-z0-9_]*)[[:space:]]*\.[[:space:]]*)?" - if [ "$sch" = "public" ]; then - qual="(${dbq}\"?public\"?[[:space:]]*\.[[:space:]]*)?" - else - qual="${dbq}\"?${sch_re}\"?[[:space:]]*\.[[:space:]]*" - fi - # `ONLY` is valid PostgreSQL and is what pg_dump emits, so a table WITH RLS was reported as - # having none without it. - if ! grep -qiE "alter[[:space:]]+table[[:space:]]+(if[[:space:]]+exists[[:space:]]+)?(only[[:space:]]+)?${qual}\"?${tbl_re}\"?[[:space:]]+enable[[:space:]]+row[[:space:]]+level[[:space:]]+security" "$STRIPPED"; then - fail "$(basename "$up"): table '$sch.$tbl' created without 'enable row level security' in the same file" - fi - done < <( - # Lowercased with `tr` before any sed runs, so no step needs a case-insensitive sed flag. - # `s///i` is a GNU extension and this repository has already been bitten once by a GNU-only - # regex feature silently matching nothing on BSD (`\s`, 18 occurrences, fixed in v1.2.1). - grep -ioE "create[[:space:]]+((global|local)[[:space:]]+)?((temporary|temp|unlogged)[[:space:]]+)?table[[:space:]]+(if[[:space:]]+not[[:space:]]+exists[[:space:]]+)?(\"[^\"]*\"|[a-z_][a-z0-9_]*)([[:space:]]*\.[[:space:]]*(\"[^\"]*\"|[a-z_][a-z0-9_]*)){0,2}" "$STRIPPED" \ - | tr '[:upper:]' '[:lower:]' \ - | sed -E 's/^create[[:space:]]+((global|local)[[:space:]]+)?((temporary|temp|unlogged)[[:space:]]+)?table[[:space:]]+(if[[:space:]]+not[[:space:]]+exists[[:space:]]+)?//' \ - | awk ' - # Split on dots OUTSIDE quotes, so `"order items"` survives the space and `"a.b"` - # survives the dot. A blind `awk -F.` read `mydb.public.orders` as schema `mydb`, - # table `public` and reported a violation naming a table that does not exist, over a - # file that was compliant — the same "red a fixer cannot act on" this gate was fixed - # for one release ago, reintroduced by the qualifier that fixed it. - { - n = 0; cur = ""; inq = 0 - for (i = 1; i <= length($0); i++) { - c = substr($0, i, 1) - if (c == "\"") { inq = !inq; continue } - if (c == "." && !inq) { part[++n] = cur; cur = ""; continue } - cur = cur c + # Every one of those is the same defect: a pattern deciding a question that needs a parse. + # So the statements are tokenised once, both sides through the SAME scanner, and the two + # (schema, table) pairs are compared as STRINGS. There is no interpolated regex left to be + # wider than the name it was given, no case flag to fold a quoted identifier, and no line + # boundary to hide a statement behind. + : > "$WORK_C"; : > "$WORK_R" + awk ' + # Emits three lines per statement found: kind ("C" create / "R" rls-enabled), schema, table. + # Three lines rather than one delimited line because a quoted identifier may contain any + # character, a tab and a newline included, and a delimiter a value can contain is not a + # delimiter. Newline inside a quoted identifier would still break this; that is recorded in + # the README rather than claimed away. + function parse_name(p, j) { + NAME_N = 0; j = p + while (1) { + if (tk[j] == "W" || tk[j] == "Q") { NAME_N++; NAME_P[NAME_N] = tv[j]; j++ } else return 0 + if (tk[j] == "D") { j++; continue } + break + } + NAME_END = j + return 1 + } + function emit(kind, sch, tbl) { + tbl = NAME_P[NAME_N] + sch = (NAME_N >= 2) ? NAME_P[NAME_N - 1] : "public" + if (tbl == "") return + print kind; print sch; print tbl + } + { buf = buf $0 "\n" } + END { + n = length(buf); i = 1; ntok = 0 + while (i <= n) { + c = substr(buf, i, 1) + if (c == " " || c == "\t" || c == "\r" || c == "\n") { i++; continue } + if (c == "\"") { + # A quoted identifier. `""` inside it is one embedded quote, which the old + # `"[^"]*"` regex ended the identifier on — `public."say ""hi"""` was read as table + # `say`, a violation naming a table that does not exist over a compliant file. + i++; v = "" + while (i <= n) { + c = substr(buf, i, 1) + if (c == "\"") { + if (substr(buf, i + 1, 1) == "\"") { v = v "\""; i += 2; continue } + i++; break } - part[++n] = cur - for (i = 1; i <= n; i++) { gsub(/^[[:space:]]+|[[:space:]]+$/, "", part[i]) } - # The last two components are schema and table; anything before them is the - # database, which PostgreSQL accepts and which names no schema. - if (n >= 2) print part[n-1] "\t" part[n] - else print "public\t" part[n] + v = v c; i++ } - ' - ) + # CASE IS PRESERVED. PostgreSQL folds an unquoted identifier to lower case and keeps a + # quoted one exactly, so `"Orders"` and `orders` are two different tables. Lowercasing + # both and matching case-insensitively let RLS on one satisfy a create of the other — + # a false green, and PascalCase quoted names are what Prisma and Drizzle emit. + ntok++; tk[ntok] = "Q"; tv[ntok] = v + continue + } + if (c ~ /[A-Za-z_]/) { + v = "" + while (i <= n) { c = substr(buf, i, 1); if (c !~ /[A-Za-z0-9_$]/) break; v = v c; i++ } + ntok++; tk[ntok] = "W"; tv[ntok] = tolower(v) + continue + } + if (c == ".") { ntok++; tk[ntok] = "D"; tv[ntok] = "."; i++; continue } + ntok++; tk[ntok] = "P"; tv[ntok] = c; i++ + } + for (p = 1; p <= ntok; p++) { + if (tk[p] != "W") continue + if (tv[p] == "create") { + q = p + 1 + if (tk[q] == "W" && (tv[q] == "global" || tv[q] == "local")) q++ + if (tk[q] == "W" && (tv[q] == "temporary" || tv[q] == "temp" || tv[q] == "unlogged")) q++ + if (!(tk[q] == "W" && tv[q] == "table")) continue + q++ + if (tk[q] == "W" && tv[q] == "if" && tk[q+1] == "W" && tv[q+1] == "not" && tk[q+2] == "W" && tv[q+2] == "exists") q += 3 + if (!parse_name(q)) continue + emit("C") + } else if (tv[p] == "alter") { + q = p + 1 + if (!(tk[q] == "W" && tv[q] == "table")) continue + q++ + if (tk[q] == "W" && tv[q] == "if" && tk[q+1] == "W" && tv[q+1] == "exists") q += 2 + if (tk[q] == "W" && tv[q] == "only") q++ + if (!parse_name(q)) continue + q = NAME_END + if (tk[q] == "W" && tv[q] == "enable" && tk[q+1] == "W" && tv[q+1] == "row" && + tk[q+2] == "W" && tv[q+2] == "level" && tk[q+3] == "W" && tv[q+3] == "security") emit("R") + } + } + } + ' "$STRIPPED" > "$WORK_TOK" + + # Read back as line triples. A delimiter a value can contain is not a delimiter, and a + # quoted identifier may contain any character. + c_n=0; r_n=0 + while IFS= read -r kind && IFS= read -r sch && IFS= read -r tbl; do + case "$kind" in + C) c_n=$((c_n+1)); C_SCH[$c_n]="$sch"; C_TBL[$c_n]="$tbl" ;; + R) r_n=$((r_n+1)); R_SCH[$r_n]="$sch"; R_TBL[$r_n]="$tbl" ;; + esac + done < "$WORK_TOK" + + i=1 + while [ "$i" -le "$c_n" ]; do + found=0 + j=1 + while [ "$j" -le "$r_n" ]; do + if [ "${C_SCH[$i]}" = "${R_SCH[$j]}" ] && [ "${C_TBL[$i]}" = "${R_TBL[$j]}" ]; then found=1; break; fi + j=$((j+1)) + done + if [ "$found" -eq 0 ]; then + fail "$(basename "$up"): table '${C_SCH[$i]}.${C_TBL[$i]}' created without 'enable row level security' in the same file" + fi + i=$((i+1)) + done + unset C_SCH C_TBL R_SCH R_TBL + # destructive statements outside a WHERE are tier-3 by regex (non-negotiable 4); flag, do not block if grep -qiE "^[[:space:]]*(drop[[:space:]]+table|truncate|delete[[:space:]]+from[[:space:]]+[a-z_.\"]+[[:space:]]*;)" "$STRIPPED"; then echo "note [$GATE] $(basename "$up"): destructive statement present; this migration is tier-3" diff --git a/gates/service-role.sh b/gates/service-role.sh index c87327e..dfde61a 100755 --- a/gates/service-role.sh +++ b/gates/service-role.sh @@ -29,6 +29,142 @@ source "$(dirname "$0")/lib.sh" WORK="$(mktemp -d)" trap 'rm -rf "$WORK"' EXIT +# --------------------------------------------------------------------------- +# THE EXTRACTOR IS A TOKENISER, NOT A REGEX — and that is the fourth attempt at this code. +# +# `grep -o` matches NON-OVERLAPPING, so a string ending in the word `from` immediately before +# a quote consumed the rest of the line up to the next quote as one "specifier". Every +# version built on that had to choose which way to be wrong, and each of three consecutive +# review rounds found the choice: +# +# - report the invented span -> a blocking UNKNOWN naming an import that does not +# exist, on ordinary source: `Array.from(",")`, a regex +# literal, `{ note: "Imported from " }`. No action a +# fixer can take. +# - drop the invented span -> a REAL import swallowed into the span is dropped with +# it. Measured: `const label = "imported from "; +# import { admin } from "../lib/admin";` on one line +# went `ok`, exit 0, over a module reaching the secret. +# +# Both are the same defect, and neither is fixable by choosing a better filter, because the +# filter is downstream of the damage. So the scan tokenises: strings, template literals, +# line and block comments and regex literals are recognised as what they are, and a +# specifier is emitted only from a real `from`/`import`/`require` position. There is no +# invented span left to report or to drop, and the `flat` pass and its `//`-comment cost are +# gone with it. +# --------------------------------------------------------------------------- +SCAN="$WORK/scan.awk" +cat > "$SCAN" <<'SCANAWK' +# Emits one record per line: "S" for a literal import specifier, or "N" for an +# import()/require() this gate cannot read. Nothing else is emitted, so a string that merely +# ENDS in the word `from` produces nothing at all. +function isregexpos(c) { + return (c == "" || c == "(" || c == "," || c == "=" || c == ":" || c == "[" || c == "!" || + c == "&" || c == "|" || c == "?" || c == "{" || c == "}" || c == ";" || c == "+" || + c == "-" || c == "*" || c == "%" || c == "<" || c == ">" || c == "~" || c == "^" || + c == "k") +} +function want(w) { return (w == "from" || w == "import" || w == "require") } +# JSX TEXT IS NOT JAVASCRIPT. `

Copied from "{"a"}" to "b"

` puts `from` immediately +# before a quote, and no tokeniser that is not also a JSX parser can tell that apart from an +# import. The candidate it yields carries a brace or an angle bracket, which a module +# specifier does not. +# +# Dropping it is only safe BECAUSE the swallow is gone. Under the old grep extractor a +# dropped candidate could be a real import that had been consumed into an invented span — +# that is exactly how the previous filter turned two reds into greens. Here every candidate +# comes from a genuine `from`/`import`/`require` position, so what is dropped is text, not +# an import this gate would otherwise have followed. +function plausible(v) { return (v != "" && v !~ /[<>{}]/) } +{ buf = buf $0 "\n" } +END { + n = length(buf); i = 1; pend = ""; paren = 0; last = "" + while (i <= n) { + c = substr(buf, i, 1) + if (c == " " || c == "\t" || c == "\r" || c == "\n") { i++; continue } + if (c == "/" && substr(buf, i + 1, 1) == "/") { while (i <= n && substr(buf, i, 1) != "\n") i++; continue } + if (c == "/" && substr(buf, i + 1, 1) == "*") { + i += 2 + while (i <= n && !(substr(buf, i, 1) == "*" && substr(buf, i + 1, 1) == "/")) i++ + i += 2; continue + } + if (c == "/" && isregexpos(last)) { + i++; incls = 0 + while (i <= n) { + c = substr(buf, i, 1) + if (c == "\\") { i += 2; continue } + if (c == "[") incls = 1 + else if (c == "]") incls = 0 + else if (c == "/" && !incls) { i++; break } + else if (c == "\n") break + i++ + } + last = "v"; pend = ""; continue + } + if (c == "\"" || c == "'") { + q = c; i++; v = "" + while (i <= n) { + c = substr(buf, i, 1) + if (c == "\\") { v = v substr(buf, i + 1, 1); i += 2; continue } + if (c == q) { i++; break } + if (c == "\n") break + v = v c; i++ + } + if (want(pend) && plausible(v)) print "S" v + pend = ""; paren = 0; last = "v"; continue + } + if (c == "`") { + i++; v = ""; interp = 0; depth = 0 + while (i <= n) { + c = substr(buf, i, 1) + if (c == "\\") { v = v substr(buf, i + 1, 1); i += 2; continue } + if (c == "$" && substr(buf, i + 1, 1) == "{") { interp = 1; depth = 1; i += 2 + while (i <= n && depth > 0) { + c = substr(buf, i, 1) + if (c == "{") depth++ + else if (c == "}") depth-- + i++ + } + continue + } + if (c == "`") { i++; break } + v = v c; i++ + } + if (want(pend)) { if (interp) print "N"; else if (plausible(v)) print "S" v } + pend = ""; paren = 0; last = "v"; continue + } + if (c ~ /[A-Za-z_$]/) { + v = "" + while (i <= n) { c = substr(buf, i, 1); if (c !~ /[A-Za-z0-9_$]/) break; v = v c; i++ } + # A PROPERTY, NOT A KEYWORD. `Array.from(",")` put `from` in the keyword slot and the + # string after it was emitted as an import specifier. `.from`, `.import` and `.require` + # are method names; only a bare one can introduce a specifier. + if (want(v) && last != ".") { pend = v; paren = 0 } else pend = "" + last = (v == "return" || v == "typeof" || v == "case" || v == "in" || v == "of" || + v == "new" || v == "delete" || v == "void" || v == "do" || v == "else" || + v == "yield" || v == "await") ? "k" : "w" + continue + } + if (c == "(") { + # `from` is never followed by `(` in an import — `import x from "y"` has no parens — so a + # `(` after it means this was some other `from`. + if (pend == "from") { pend = ""; last = "("; i++; continue } + if (pend != "" && paren == 0) { + paren = 1 + # Look ahead: an import()/require() whose argument is not a literal is a branch this + # gate cannot follow, and must be reported rather than assumed harmless. + j = i + 1 + while (j <= n && substr(buf, j, 1) ~ /[ \t\r\n]/) j++ + c2 = substr(buf, j, 1) + if (c2 != "\"" && c2 != "'" && c2 != "`" && pend != "from") { print "N"; pend = "" } + } else pend = "" + last = "("; i++; continue + } + pend = ""; last = c; i++ + } +} +SCANAWK + # --------------------------------------------------------------------------- # Applicability. This gate is about Next.js request-path modules; run against a tree that # is not a Next.js app it has nothing to walk. Saying "ok" over an empty walk is the false @@ -88,7 +224,41 @@ fi # `@/*` key got a red with no reason in it, which reads as a broken gate rather than as the # gate saying anything. Every command substitution here ends in `|| true` for that reason. # --------------------------------------------------------------------------- -TSCONFIG="$ROOT/tsconfig.json" +TSCONFIG_RAW="$ROOT/tsconfig.json" +# tsconfig.json permits comments, and every read below has to see past them. `grep -q +# '"baseUrl"'` armed the baseUrl fallback on a COMMENTED-OUT key: a config carrying +# `// "baseUrl": ".",` and no live one made `import React from "react"` resolve to a repo +# directory named react/ and reported a violation against the package. A commented-out line +# is not configuration. +TSCONFIG="$WORK/tsconfig.json" +if [ -f "$TSCONFIG_RAW" ]; then + awk ' + { buf = buf $0 "\n" } + END { + n = length(buf); i = 1 + while (i <= n) { + c = substr(buf, i, 1) + if (c == "\"") { + printf "%s", c; i++ + while (i <= n) { + c = substr(buf, i, 1) + if (c == "\\") { printf "%s", substr(buf, i, 2); i += 2; continue } + printf "%s", c; i++ + if (c == "\"") break + } + continue + } + if (c == "/" && substr(buf, i + 1, 1) == "/") { while (i <= n && substr(buf, i, 1) != "\n") i++; continue } + if (c == "/" && substr(buf, i + 1, 1) == "*") { + i += 2 + while (i <= n && !(substr(buf, i, 1) == "*" && substr(buf, i + 1, 1) == "/")) i++ + i += 2; continue + } + printf "%s", c; i++ + } + } + ' "$TSCONFIG_RAW" > "$TSCONFIG" 2>/dev/null || cp -- "$TSCONFIG_RAW" "$TSCONFIG" 2>/dev/null || : +fi # Named in the UNKNOWN below only when it is actually there. The message said "no alias # declared in tsconfig.json" on a tree with no tsconfig.json at all, which sends a reader # to open a file that does not exist. @@ -441,80 +611,26 @@ while :; do file="$(sed -n "${n}p" "$QUEUE")" [ -z "$file" ] && break - # SCANNED WITH NEWLINES COLLAPSED, because a line-at-a-time grep does not see a statement - # that spans lines — and the statement most likely to span lines is the one this gate must - # not miss. Shipped in v1.1.0, measured: - # - # const mod = await import( - # process.env.MODULE_NAME ?? "@/lib/secret" - # ); - # - # produced `ok [service-role]`, exit 0, over a page that reaches SUPABASE_SERVICE_ROLE_KEY. - # The guard did not fire because `import(` and the non-literal argument were on different - # lines, and the extractor did not follow it for the same reason. A FALSE GREEN in the - # security gate, from a formatting choice Prettier makes on its own. - # - # The cost of flattening: a `//` comment now runs into the code after it, so a mention of - # `import(` inside a comment can raise a spurious UNKNOWN. That is over-inclusive — a false - # RED, visible and arguable — and this repository takes that trade every time over a false - # green. It belongs with the other "regex, not a parser" gaps in the README. - tr '\n' ' ' < "$file" > "$WORK/flat" 2>/dev/null || : > "$WORK/flat" - - # A branch of the graph this gate cannot follow. Reported here, against the module that - # contains it, rather than assumed harmless. - if grep -qE '(^|[^A-Za-z0-9_$.])(import|require)[[:space:]]*\([[:space:]]*[^'"'"'")[:space:]]' -- "$WORK/flat" 2>/dev/null; then + # One tokenised pass. `N` is a branch this gate cannot follow — an import() or require() + # whose argument is not a literal — reported against the module that contains it rather + # than assumed harmless. `S` is a literal specifier from a real import position. + set +e + recs="$(awk -f "$SCAN" -- "$file" 2>/dev/null)" + set -e + nonliteral=0 + : > "$WORK/specs" + while IFS= read -r rec; do + case "$rec" in + "") continue ;; + N) nonliteral=1 ;; + S*) printf '%s\n' "${rec#S}" >> "$WORK/specs" ;; + esac + done <<< "$recs" + if [ "$nonliteral" = 1 ]; then unknown "$(rel "$file") contains a non-literal import() or require() — this gate cannot tell what it pulls in, so it will not call this path clean" fi - set +e - # BOTH passes, unioned. Flattening alone was a REGRESSION and it went in the direction this - # change exists to fix: `grep -o` matches non-overlapping, so a string ending in `from "` - # swallows the real import after it. Measured — - # - # const label = "imported from "; - # import { key } from "../lib/secret"; - # - # gave `ok [service-role]`, exit 0, over a module reaching SUPABASE_SERVICE_ROLE_KEY, on a - # tree the PREVIOUS version caught. The line pass finds ordinary imports with no window to - # swallow across; the flat pass finds the multi-line ones. - # - # AND THE GARBAGE IS DROPPED HERE, not left to be classified. The sentence that stood here - # said the invented spans "resolve as a bare specifier and are skipped, so the union only - # ever adds edges" — true only for as long as an unclassifiable specifier was silently - # skipped. The moment the classifier below began reporting UNKNOWN instead (the right - # change, made in the same commit), that sentence went false and every template literal in - # the tree became a blocking red. Measured, on a file that imports NOTHING: - # - # const sql = `select id, owner from "${table}" where owner = $1`; - # UNKNOWN [service-role] app/api/orders/route.ts imports '${table}' ... - # - # There is no action a fixer can take: the message names an import that does not exist and - # the only way to green is to delete the string. Note it was the LINE pass that produced it — - # the old sentence was wrong about which pass invents garbage as well as about what became - # of it. - # - # THE FILTER IS `${` AND NOTHING ELSE, and the first version of it was far wider — every - # candidate carrying a character "no module specifier can contain". That set turned two REAL - # imports into silent skips, which is worse than the false red it was removing, because the - # swallow does not only invent garbage: it can swallow a real import INTO the garbage. - # Measured, both against the previous revision, which reported them: - # - # const label = "imported from "; import { admin } from "../lib/admin"; - # -> the whole span became one candidate, dropped; lib/admin.ts never walked. `ok`. - # import { admin } from "../lib/(group)/admin"; - # -> a resolvable file whose path holds a Next.js route group, dropped. `ok`. - # - # The first of those is `fixtures/service-role/bad/cases/string-ending-in-from/` with the - # newline removed — one character from the fixture that exists to catch exactly it. - # `${` is the whole artefact: it is the only shape the extractor produces that CANNOT be a - # path, and dropping it leaves everything else to be classified and reported as before. - specs="$( - { grep -oE "(from|import|require)[[:space:]]*\(?[[:space:]]*['\"][^'\"]+['\"]" -- "$file" 2>/dev/null - grep -oE "(from|import|require)[[:space:]]*\(?[[:space:]]*['\"][^'\"]+['\"]" -- "$WORK/flat" 2>/dev/null - } | sed -E "s/.*['\"]([^'\"]+)['\"]\$/\1/" \ - | grep -vE '[$][{]' \ - | sort -u - )" + specs="$(sort -u "$WORK/specs" 2>/dev/null)" set -e while IFS= read -r spec; do [ -n "$spec" ] || continue From 96e95a4a782682c38df66eac9a1475f466a1e5d9 Mon Sep 17 00:00:00 2001 From: Chii Date: Tue, 8 Sep 2026 23:16:34 +0000 Subject: [PATCH 06/13] The baseUrl probe refused to look at the paths most likely to need it, and the README miscounted its own placeholders MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two items from a review body that had no inline thread, so neither was carried by a resolvable comment. Both reproduced. THE PROBE WAS GATED ON THE NAME LOOKING LIKE A PACKAGE. `is_package_specifier` decided whether to probe BASE_DIR at all, so the specifiers most likely to BE baseUrl-relative were the ones it skipped. `_components/Button` — a leading underscore is not a valid npm name, and underscore-prefixed private folders are an ordinary Next.js convention — resolved to a real file under BASE_DIR and was reported UNKNOWN anyway: UNKNOWN [service-role] src/app/page.tsx imports '_components/Button', which matches no path alias this gate could read ... exit 1 A red on a tree the gate could read perfectly well, and one that HIDES the violation underneath it: the file holds SUPABASE_SERVICE_ROLE_KEY and is reached from a page, which is now what the gate says. A probe that refuses to look at a path because the path is not a package name is answering a different question from the one it was asked. The probe now runs for any bare specifier under a declared baseUrl, and the package claim is tested where it belongs — after the probe misses. `react` with no local react/ is still a dependency; `_components/Button` with no such file is UNKNOWN, not a silently skipped package. `#`-prefixed specifiers stay out deliberately: Node and TypeScript resolve those through package.json `imports`, so probing BASE_DIR for one would be a guess dressed as a resolution. AND THE README MISCOUNTED ITS OWN INSTRUCTIONS. "Replace both occurrences", over a snippet carrying three — the `gates:` ref, `gates_ref:`, and the `review:` ref. A count narrower than the thing it describes, which is the defect this repository exists to catch, in the sentence telling people how to install it. It now names all three and says why the first two must stay identical: a reusable workflow cannot discover its own commit, so a mismatch runs one release's workflow over another release's gates. Fixture cases/baseurl-nonpackage-name: UNKNOWN under 7ca2720, FAIL now. Selftest green. All six shell gates green on the template tree at deda1fa. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BfFq2KvV1CCkPs5WnV1AsA --- README.md | 10 ++++-- .../baseurl-nonpackage-name/package.json | 1 + .../src/_components/Button.ts | 1 + .../baseurl-nonpackage-name/src/app/page.tsx | 10 ++++++ .../baseurl-nonpackage-name/tsconfig.json | 1 + gates/service-role.sh | 36 ++++++++++++++----- 6 files changed, 49 insertions(+), 10 deletions(-) create mode 100644 fixtures/service-role/bad/cases/baseurl-nonpackage-name/package.json create mode 100644 fixtures/service-role/bad/cases/baseurl-nonpackage-name/src/_components/Button.ts create mode 100644 fixtures/service-role/bad/cases/baseurl-nonpackage-name/src/app/page.tsx create mode 100644 fixtures/service-role/bad/cases/baseurl-nonpackage-name/tsconfig.json diff --git a/README.md b/README.md index 3614b5f..0356679 100644 --- a/README.md +++ b/README.md @@ -144,8 +144,14 @@ hesitation while a placeholder cannot be. `caller-template.yml` ships a real SHA to run; this prose does not. Copy `caller-template.yml` into your repo as `.github/workflows/gates.yml`. It ships a real -SHA rather than a placeholder, and that SHA is the **previous** release — replace both -occurrences with the one you want from the Releases table below: +SHA rather than a placeholder, and that SHA is the **previous** release. Replace **all three** +occurrences with the one you want from the Releases table below — the `gates:` `uses:` ref, +the `gates_ref:` under it, and the `review:` `uses:` ref. The first two must stay identical to +each other: a reusable workflow cannot discover its own commit, so the ref it runs from has to +be handed to it, and a mismatch runs one release's workflow over another release's gates. This +sentence said "both occurrences" over a snippet containing three, which is a count narrower +than the thing it describes — the defect this repository exists to catch, in its own +instructions. ```yaml on: [pull_request, push] diff --git a/fixtures/service-role/bad/cases/baseurl-nonpackage-name/package.json b/fixtures/service-role/bad/cases/baseurl-nonpackage-name/package.json new file mode 100644 index 0000000..870a565 --- /dev/null +++ b/fixtures/service-role/bad/cases/baseurl-nonpackage-name/package.json @@ -0,0 +1 @@ +{ "name": "fixture-baseurl-nonpackage-name", "dependencies": { "next": "15.0.0" } } diff --git a/fixtures/service-role/bad/cases/baseurl-nonpackage-name/src/_components/Button.ts b/fixtures/service-role/bad/cases/baseurl-nonpackage-name/src/_components/Button.ts new file mode 100644 index 0000000..7b4886c --- /dev/null +++ b/fixtures/service-role/bad/cases/baseurl-nonpackage-name/src/_components/Button.ts @@ -0,0 +1 @@ +export const admin = process.env.SUPABASE_SERVICE_ROLE_KEY; diff --git a/fixtures/service-role/bad/cases/baseurl-nonpackage-name/src/app/page.tsx b/fixtures/service-role/bad/cases/baseurl-nonpackage-name/src/app/page.tsx new file mode 100644 index 0000000..ee52bc9 --- /dev/null +++ b/fixtures/service-role/bad/cases/baseurl-nonpackage-name/src/app/page.tsx @@ -0,0 +1,10 @@ +// An underscore-prefixed private folder is an ordinary Next.js convention, and a leading +// underscore is not a valid npm package name. The baseUrl probe was gated on the specifier +// LOOKING like a package, so it skipped exactly the names most likely to be baseUrl-relative: +// this file resolves under baseUrl and was reported UNKNOWN anyway — a red on a tree the gate +// could read perfectly well, and one that hides the violation underneath it. +import { admin } from "_components/Button"; + +export default function Page() { + return admin; +} diff --git a/fixtures/service-role/bad/cases/baseurl-nonpackage-name/tsconfig.json b/fixtures/service-role/bad/cases/baseurl-nonpackage-name/tsconfig.json new file mode 100644 index 0000000..41b4e99 --- /dev/null +++ b/fixtures/service-role/bad/cases/baseurl-nonpackage-name/tsconfig.json @@ -0,0 +1 @@ +{ "compilerOptions": { "baseUrl": "src" } } diff --git a/gates/service-role.sh b/gates/service-role.sh index dfde61a..de3b58b 100755 --- a/gates/service-role.sh +++ b/gates/service-role.sh @@ -490,10 +490,25 @@ $BASE_DIR/$t" # under BASE_DIR, and if nothing is there the specifier really is a package (rc 1, not a # red). Only when an explicit baseUrl was declared — without one there is no such shape # to resolve, and every bare specifier is a dependency exactly as before. - if [ "$matched" -ne 1 ] && [ "$BASEURL_SET" = 1 ] && is_package_specifier "$spec"; then - matched=1; fallback=1 - bases="$BASE_DIR/$spec" - fi + # NOT GATED ON THE NAME LOOKING LIKE A PACKAGE. It was, and that made the probe skip + # exactly the specifiers most likely to be baseUrl-relative: `_components/Button` — a + # leading underscore is not a valid npm name, and underscore-prefixed private folders + # are an ordinary Next.js convention — resolved to a real file under BASE_DIR and was + # reported UNKNOWN anyway. A probe that refuses to look at a path because the path is + # not a package name is answering a different question from the one it was asked. + # + # `#`-prefixed specifiers are the exception and stay out: TypeScript and Node resolve + # those through package.json `imports`, not by appending them to baseUrl, so probing + # BASE_DIR for one would be a guess dressed as a resolution. + case "$spec" in + '#'*) : ;; + *) + if [ "$matched" -ne 1 ] && [ "$BASEURL_SET" = 1 ]; then + matched=1; fallback=1 + bases="$BASE_DIR/$spec" + fi + ;; + esac ;; esac if [ "$matched" -ne 1 ]; then @@ -584,10 +599,15 @@ ${b%.js}.d.ts" ;; if [ -n "$found" ]; then break; fi done <<< "$bases" if [ -n "$found" ]; then printf '%s' "$found"; return 0; fi - # A baseUrl probe that found nothing is not a failure to resolve a local module — it is - # TypeScript's own fallthrough to node_modules. Reporting UNKNOWN here would turn every - # `import React from "react"` red in any repo that declares a baseUrl. - if [ "$fallback" = 1 ]; then return 1; fi + # A baseUrl probe that found nothing is TypeScript's own fallthrough to node_modules — but + # only for a name that could BE a package. Reporting UNKNOWN for every miss would turn + # `import React from "react"` red in any repo declaring a baseUrl; calling every miss a + # package would silently skip `_components/Button` when that file is simply absent. So the + # claim is tested here, exactly as it is for a specifier that never reached the probe. + if [ "$fallback" = 1 ]; then + if is_package_specifier "$spec"; then return 1; fi + return 3 + fi return 2 } From 3ace9d837aa43a155dc70c62b612ef7c1b9dd765 Mon Sep 17 00:00:00 2001 From: Chii Date: Tue, 8 Sep 2026 23:39:04 +0000 Subject: [PATCH 07/13] The tokenisers shipped a silent pass, a false green, and a quadratic accumulator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round six, against the rewrite itself on the principle that the largest change gets the most suspicion. It found three Important, and two are regressions the rewrite introduced — the fifth commit in a row to do that, which is itself the finding worth keeping. A SILENT PASS. The SQL scanner had no single-quoted-string state, so ONE `"` inside an ordinary string literal opened a quoted identifier that ran to the end of the file and made every statement after it invisible: insert into public.products (name) values ('24" monitor'); create table public.orders (...); -- no RLS anywhere in the file 7ca2720: ok [migrations-lint] exit 0 d2d2191: FAIL ... table 'public.orders' created without ... An inch mark in seed data. The regex it replaced saw that create table fine, so this is the rewrite reintroducing exactly the class it was written to end, and it was not in the README's list of silent passes either. A FALSE GREEN. The template-literal scan was not line-bounded, so one stray backtick in JSX text — `

Press the ` key

` — consumed the rest of the file and a require("../lib/admin") below it reported ok, exit 0, over a module reaching SUPABASE_SERVICE_ROLE_KEY. Both are the same shape, and both now have the same answer: A SCANNER THAT LOST SYNC MUST NOT REPORT A CLEAN FILE. An unterminated quoted identifier, string, template literal or block comment is UNKNOWN, because everything after it was read as something it is not. Single-quoted SQL strings are now a state; DDL inside one is UNKNOWN rather than read (reading it named tables nobody created) or skipped. A QUADRATIC ACCUMULATOR. Both scanners built the file with `buf = buf $0 "\n"`, which mawk reallocates and copies every line. Measured on a generated database.types.ts, end to end: 1.1 MB 12.1s -> 0.40s (d2d2191's greps: 0.11s) 4.7 MB 224.7s -> 1.33s Doubling the input quadrupled the time. gates.yml sets no timeout-minutes, so that surfaces not as a red but as a job taking minutes — the shape of the hang this toolkit has already shipped once. Both scanners are line-incremental now, carrying state across lines instead of the file. AND THE READER DESYNCED. Three lines per record meant a quoted identifier containing a newline shifted every following triple: a file with one such name and two more non-compliant tables reported ONE violation, naming a table that does not exist, and did not report the other two at all. A reader who "fixed" the named table would have had a green over an unguarded schema. Records are one escaped tab-separated line now. The comment beside the old reader claimed the limitation was "recorded in the README"; the word newline does not appear in that file. It does now, and so does the behaviour. Also from the same round: a block comment between two tokens was removed WITHOUT a separator, so `create/*x*/table public.orders` became `createtable` and passed silently; and `import(/* webpackChunkName: "x" */ "./x")` was reported as an import this gate cannot read, on a line it could read perfectly well. Moving the non-literal decision out of a rest-of-line lookahead and into the token stream fixed a fourth thing on the way: the lookahead could not see an `import(` whose argument is on the NEXT line, which silently reopened the multi-line dynamic import false green from two releases ago. It is caught by its existing fixture. Two new fixtures, both green under 96e95a4 and red now: quote-inside-a-string, unterminated-template-literal. Twelve migrations-lint spellings and every service-role fixture re-measured. Selftest green. All six shell gates green on the template tree at deda1fa. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BfFq2KvV1CCkPs5WnV1AsA --- README.md | 35 ++- .../migrations/20260105000000_orders.down.sql | 1 + .../migrations/20260105000000_orders.sql | 10 + .../app/page.tsx | 13 ++ .../lib/admin.ts | 1 + .../package.json | 1 + gates/migrations-lint.sh | 220 +++++++++++------- gates/service-role.sh | 142 +++++------ 8 files changed, 259 insertions(+), 164 deletions(-) create mode 100644 fixtures/migrations-lint/bad/cases/quote-inside-a-string/supabase/migrations/20260105000000_orders.down.sql create mode 100644 fixtures/migrations-lint/bad/cases/quote-inside-a-string/supabase/migrations/20260105000000_orders.sql create mode 100644 fixtures/service-role/bad/unknown/unterminated-template-literal/app/page.tsx create mode 100644 fixtures/service-role/bad/unknown/unterminated-template-literal/lib/admin.ts create mode 100644 fixtures/service-role/bad/unknown/unterminated-template-literal/package.json diff --git a/README.md b/README.md index 0356679..40f0cdd 100644 --- a/README.md +++ b/README.md @@ -311,12 +311,17 @@ header, which is the honest account of what that scanner does not do. folds `Orders` to `orders` but keeps `"Orders"` distinct — Prisma and Drizzle emit the quoted PascalCase form. Three successive regex versions each traded one error for another here; the pair comparison leaves no interpolated pattern to be wider than the name it was - given. It reads a `create table` inside a function body, a `DO $$` block, and an - `execute '…'` string, and reds on all three. What it does **not** read: a name assembled at - runtime (`execute format('create table %I …')` or string concatenation), `select … into`, - and a statement on a line where an earlier string literal contains `--`, which the comment - stripper takes for a comment. Those are silent passes, and they are the reason this bullet - lists them. + given. It reads a `create table` inside a function body and inside a `DO $$` block, and reds + on both. It does **not** read one inside a `'…'` string literal: a string is data to the + scanner, and reading it named tables nobody created and sent fixers to edit their data — + but a string containing both `create` and `table` is reported UNKNOWN rather than passed + over, because `execute` runs it. What it genuinely cannot read, as silent passes: a name + assembled at runtime (`execute format('create table %I …')` or string concatenation), + `select … into`, and a statement on a line where an earlier string literal contains `--`, + which the comment stripper takes for a comment. **An unterminated quoted identifier or + string is UNKNOWN, not a pass** — a scanner that lost sync read everything after it as + something it is not, and one `"` inside an ordinary string literal (an inch mark in + `values ('24" monitor')`) is enough to do that. - **The service-role gate tokenises JavaScript; it does not match it.** Strings, template literals, line and block comments and regex literals are recognised as what they are, and a specifier is emitted only from a real `from`/`import`/`require` position. This replaced a @@ -326,9 +331,21 @@ header, which is the honest account of what that scanner does not do. regex literal, `{ note: "Imported from " }`) and dropping it lost a real import that had been swallowed. Both were measured, in three consecutive review rounds, before the extractor itself was replaced. A multi-line `await import(` is followed; a `//` comment no - longer runs into the code after it. What it still cannot read is **JSX text**: `

Copied - from "a" to "b"

` puts `from` before a quote and nothing short of a JSX parser can tell - that from an import, so a candidate carrying `<`, `>`, `{` or `}` is dropped as text. + longer runs into the code after it; `import(/* webpackChunkName */ "./x")` is read as the + literal import it is. **A template literal or block comment still open at end of file is + UNKNOWN**, because a scanner that lost sync read the rest of the file as something it is + not — one stray backtick in JSX text otherwise consumed everything below it, and a + `require("@/lib/admin")` under it reported `ok`. What it still cannot read is **JSX text**: + `

Copied from "a" to "b"

` puts `from` before a quote and nothing short of a JSX parser + can tell that from an import, so a candidate carrying `<`, `>`, `{` or `}` is dropped as + text. Node subpath imports (`#internal/db`) are UNKNOWN: they resolve through `package.json` + `imports`, which this gate does not read. +- **Both scanners are line-incremental, and that is a correctness property, not a speed one.** + The first version accumulated each file with `buf = buf $0 "\n"`, which mawk reallocates and + copies every line: 12.1s on a 1.1MB generated types file against 0.11s for the greps it + replaced, and quadratic, so it got worse with size. `gates.yml` sets no `timeout-minutes`, + so that surfaces not as a red but as a job that takes minutes — the shape of the hang this + toolkit has already shipped once. - **The service-role gate resolves like TypeScript, with one deliberate divergence.** It follows `./`, `../`, absolute paths, and **every alias declared in tsconfig `compilerOptions.paths`** — not just `@/*` — plus a **`baseUrl` with no matching `paths` diff --git a/fixtures/migrations-lint/bad/cases/quote-inside-a-string/supabase/migrations/20260105000000_orders.down.sql b/fixtures/migrations-lint/bad/cases/quote-inside-a-string/supabase/migrations/20260105000000_orders.down.sql new file mode 100644 index 0000000..94b5e0d --- /dev/null +++ b/fixtures/migrations-lint/bad/cases/quote-inside-a-string/supabase/migrations/20260105000000_orders.down.sql @@ -0,0 +1 @@ +drop table if exists public.orders; diff --git a/fixtures/migrations-lint/bad/cases/quote-inside-a-string/supabase/migrations/20260105000000_orders.sql b/fixtures/migrations-lint/bad/cases/quote-inside-a-string/supabase/migrations/20260105000000_orders.sql new file mode 100644 index 0000000..4fea767 --- /dev/null +++ b/fixtures/migrations-lint/bad/cases/quote-inside-a-string/supabase/migrations/20260105000000_orders.sql @@ -0,0 +1,10 @@ +-- ONE double quote inside an ordinary string literal. The scanner had no single-quoted-string +-- state, so this `"` opened a quoted identifier that ran to the end of the file and every +-- statement after it was invisible: the create below had no RLS anywhere and the file +-- reported ok, exit 0. An inch mark in seed data is enough to do it. +insert into public.products (name) values ('24" monitor'); + +create table public.orders ( + id uuid primary key, + owner_id uuid not null +); diff --git a/fixtures/service-role/bad/unknown/unterminated-template-literal/app/page.tsx b/fixtures/service-role/bad/unknown/unterminated-template-literal/app/page.tsx new file mode 100644 index 0000000..77c89e0 --- /dev/null +++ b/fixtures/service-role/bad/unknown/unterminated-template-literal/app/page.tsx @@ -0,0 +1,13 @@ +// A stray backtick in JSX text. It is not a template literal, but nothing short of a JSX +// parser can tell — so the scan opens one and runs to the end of the file, and every import +// below it is read as template-literal text. Before the sync-loss guard this file reported +// ok, exit 0, over a module that reaches the secret through the require() below. +export function Hint() { + return

Press the ` key to continue

; +} + +const { admin } = require("../lib/admin"); + +export default function Page() { + return admin; +} diff --git a/fixtures/service-role/bad/unknown/unterminated-template-literal/lib/admin.ts b/fixtures/service-role/bad/unknown/unterminated-template-literal/lib/admin.ts new file mode 100644 index 0000000..7b4886c --- /dev/null +++ b/fixtures/service-role/bad/unknown/unterminated-template-literal/lib/admin.ts @@ -0,0 +1 @@ +export const admin = process.env.SUPABASE_SERVICE_ROLE_KEY; diff --git a/fixtures/service-role/bad/unknown/unterminated-template-literal/package.json b/fixtures/service-role/bad/unknown/unterminated-template-literal/package.json new file mode 100644 index 0000000..29e224a --- /dev/null +++ b/fixtures/service-role/bad/unknown/unterminated-template-literal/package.json @@ -0,0 +1 @@ +{ "name": "fixture-unterminated-template-literal", "dependencies": { "next": "15.0.0" } } diff --git a/gates/migrations-lint.sh b/gates/migrations-lint.sh index c871469..a40ad7d 100755 --- a/gates/migrations-lint.sh +++ b/gates/migrations-lint.sh @@ -40,11 +40,14 @@ strip_sql_comments() { if (inblk) { p = index(line, "*/") if (p == 0) { line = ""; break } - line = substr(line, p + 2); inblk = 0 + line = substr(line, p + 2); inblk = 0; out = out " " } else { pb = index(line, "/*"); pl = index(line, "--") if (pl > 0 && (pb == 0 || pl < pb)) { out = out substr(line, 1, pl - 1); line = ""; break } - if (pb > 0) { out = out substr(line, 1, pb - 1); line = substr(line, pb + 2); inblk = 1 } + # A SPACE, not nothing. Concatenating the text before `/*` to the text after `*/` + # made `create/*x*/table public.orders` one word, `createtable`, and the statement + # a silent pass. A comment separates tokens; removing it must not join them. + if (pb > 0) { out = out substr(line, 1, pb - 1) " "; line = substr(line, pb + 2); inblk = 1 } else { out = out line; line = "" } } } @@ -54,10 +57,125 @@ strip_sql_comments() { } STRIPPED="$(mktemp)" +SQLSCAN="$(mktemp)" +cat > "$SQLSCAN" <<'SQLSCANAWK' +# Streaming SQL scanner. One record per line: kind schema table, where kind is +# "C" (create table), "R" (RLS enabled) or "U" (this scanner could not answer). Values are +# escaped, because a quoted identifier may contain a tab or a newline and a delimiter a value +# can contain is not a delimiter. +# +# LINE-INCREMENTAL, NOT SLURPED. The first version built the whole file with `buf = buf $0 +# "\n"`, which mawk reallocates and copies every line: 12.1s on a 1.1MB file against 0.11s for +# the greps it replaced, and quadratic, so it got worse with size. A gates job that takes +# minutes and scales the wrong way is the shape of the hang this repository has already been +# bitten by once. State is carried across lines instead; nothing accumulates but the current +# token and the current name. +function esc(v) { gsub(/\\/, "\\\\", v); gsub(/\t/, "\\t", v); gsub(/\n/, "\\n", v); return v } +function emitpair(kind, tbl, sch) { + if (nn == 0) return + tbl = P[nn]; sch = (nn >= 2) ? P[nn - 1] : "public" + if (tbl == "") return + print kind "\t" esc(sch) "\t" esc(tbl) +} +function tok(k, v, again) { + again = 1 + while (again) { + again = 0 + if (st == 0) { + if (k == "W" && v == "create") st = 1 + else if (k == "W" && v == "alter") st = 4 + } else if (st == 1) { + if (k == "W" && (v == "global" || v == "local" || v == "temporary" || v == "temp" || v == "unlogged")) { } + else if (k == "W" && v == "table") st = 2 + else { st = 0; again = 1 } + } else if (st == 2) { + if (k == "W" && (v == "if" || v == "not" || v == "exists")) { } + else if (k == "W" || k == "Q") { nn = 1; P[1] = v; st = 3; wantpart = 0 } + else { st = 0; again = 1 } + } else if (st == 3) { + if (k == "D") wantpart = 1 + else if (wantpart && (k == "W" || k == "Q")) { P[++nn] = v; wantpart = 0 } + else if (wantpart) { st = 0; nn = 0; again = 1 } + else { emitpair("C"); st = 0; nn = 0; again = 1 } + } else if (st == 4) { + if (k == "W" && v == "table") st = 5 + else { st = 0; again = 1 } + } else if (st == 5) { + if (k == "W" && (v == "if" || v == "exists" || v == "only")) { } + else if (k == "W" || k == "Q") { nn = 1; P[1] = v; st = 6; wantpart = 0 } + else { st = 0; again = 1 } + } else if (st == 6) { + if (k == "D") wantpart = 1 + else if (wantpart && (k == "W" || k == "Q")) { P[++nn] = v; wantpart = 0 } + else if (wantpart) { st = 0; nn = 0; again = 1 } + else { st = 7; es = 0; again = 1 } + } else if (st == 7) { + if (k == "W" && es == 0 && v == "enable") es = 1 + else if (k == "W" && es == 1 && v == "row") es = 2 + else if (k == "W" && es == 2 && v == "level") es = 3 + else if (k == "W" && es == 3 && v == "security") { emitpair("R"); st = 0; nn = 0 } + else { st = 0; nn = 0; again = 1 } + } + } +} +{ + n = length($0); i = 1 + while (i <= n) { + c = substr($0, i, 1) + # A DOUBLE-QUOTED IDENTIFIER, possibly spanning lines. `""` inside it is one embedded + # quote. + if (inq) { + if (c == "\"") { + if (substr($0, i + 1, 1) == "\"") { qv = qv "\""; i += 2; continue } + inq = 0; i++; tok("Q", qv); qv = ""; continue + } + qv = qv c; i++; continue + } + # A SINGLE-QUOTED STRING. This state did not exist, and without it ONE `"` inside an + # ordinary string literal — an inch mark, a quoted word in prose — opened an identifier + # that ate every following statement. Measured: a file whose only `"` was in + # `values ('24" monitor')` and whose `create table` had no RLS anywhere reported `ok`, + # exit 0. A silent pass, introduced by the rewrite that was meant to end them. + if (ins) { + if (c == "'") { + if (substr($0, i + 1, 1) == "'") { sv = sv "'"; i += 2; continue } + ins = 0; i++ + if (tolower(sv) ~ /create/ && tolower(sv) ~ /table/) ddl = 1 + sv = ""; continue + } + sv = sv c; i++; continue + } + if (c == " " || c == "\t" || c == "\r") { i++; continue } + if (c == "\"") { inq = 1; qv = ""; i++; continue } + if (c == "'") { ins = 1; sv = ""; i++; continue } + if (c ~ /[A-Za-z_]/) { + v = "" + while (i <= n) { c = substr($0, i, 1); if (c !~ /[A-Za-z0-9_$]/) break; v = v c; i++ } + tok("W", tolower(v)); continue + } + if (c == ".") { tok("D", "."); i++; continue } + tok("P", c); i++ + } + if (inq) qv = qv "\n" + if (ins) sv = sv "\n" +} +END { + if (st == 3) emitpair("C") + # A SCANNER THAT LOST SYNC MUST NOT REPORT A CLEAN FILE. An unterminated quoted identifier + # or string means everything after it was read as something it is not, so the only honest + # answer about the rest of the file is that this gate could not read it. + if (inq) print "U\tunterminated-quoted-identifier\t" + if (ins) print "U\tunterminated-string\t" + # DDL inside a string literal is executed by `execute`, and this scanner treats the string + # as data — the alternative, reading it, named tables nobody created and sent fixers to + # edit their data. Neither is a check, so it says so. + if (ddl) print "U\tddl-inside-a-string-literal\t" +} +SQLSCANAWK WORK_TOK="$(mktemp)" WORK_C="$(mktemp)" WORK_R="$(mktemp)" -trap 'rm -f "$STRIPPED" "$WORK_TOK" "$WORK_C" "$WORK_R"' EXIT +trap 'rm -f "$STRIPPED" "$SQLSCAN" "$WORK_TOK" "$WORK_C" "$WORK_R"' EXIT for up in "$MIG"/*.sql; do [ -e "$up" ] || continue @@ -92,96 +210,22 @@ for up in "$MIG"/*.sql; do # wider than the name it was given, no case flag to fold a quoted identifier, and no line # boundary to hide a statement behind. : > "$WORK_C"; : > "$WORK_R" - awk ' - # Emits three lines per statement found: kind ("C" create / "R" rls-enabled), schema, table. - # Three lines rather than one delimited line because a quoted identifier may contain any - # character, a tab and a newline included, and a delimiter a value can contain is not a - # delimiter. Newline inside a quoted identifier would still break this; that is recorded in - # the README rather than claimed away. - function parse_name(p, j) { - NAME_N = 0; j = p - while (1) { - if (tk[j] == "W" || tk[j] == "Q") { NAME_N++; NAME_P[NAME_N] = tv[j]; j++ } else return 0 - if (tk[j] == "D") { j++; continue } - break - } - NAME_END = j - return 1 - } - function emit(kind, sch, tbl) { - tbl = NAME_P[NAME_N] - sch = (NAME_N >= 2) ? NAME_P[NAME_N - 1] : "public" - if (tbl == "") return - print kind; print sch; print tbl - } - { buf = buf $0 "\n" } - END { - n = length(buf); i = 1; ntok = 0 - while (i <= n) { - c = substr(buf, i, 1) - if (c == " " || c == "\t" || c == "\r" || c == "\n") { i++; continue } - if (c == "\"") { - # A quoted identifier. `""` inside it is one embedded quote, which the old - # `"[^"]*"` regex ended the identifier on — `public."say ""hi"""` was read as table - # `say`, a violation naming a table that does not exist over a compliant file. - i++; v = "" - while (i <= n) { - c = substr(buf, i, 1) - if (c == "\"") { - if (substr(buf, i + 1, 1) == "\"") { v = v "\""; i += 2; continue } - i++; break - } - v = v c; i++ - } - # CASE IS PRESERVED. PostgreSQL folds an unquoted identifier to lower case and keeps a - # quoted one exactly, so `"Orders"` and `orders` are two different tables. Lowercasing - # both and matching case-insensitively let RLS on one satisfy a create of the other — - # a false green, and PascalCase quoted names are what Prisma and Drizzle emit. - ntok++; tk[ntok] = "Q"; tv[ntok] = v - continue - } - if (c ~ /[A-Za-z_]/) { - v = "" - while (i <= n) { c = substr(buf, i, 1); if (c !~ /[A-Za-z0-9_$]/) break; v = v c; i++ } - ntok++; tk[ntok] = "W"; tv[ntok] = tolower(v) - continue - } - if (c == ".") { ntok++; tk[ntok] = "D"; tv[ntok] = "."; i++; continue } - ntok++; tk[ntok] = "P"; tv[ntok] = c; i++ - } - for (p = 1; p <= ntok; p++) { - if (tk[p] != "W") continue - if (tv[p] == "create") { - q = p + 1 - if (tk[q] == "W" && (tv[q] == "global" || tv[q] == "local")) q++ - if (tk[q] == "W" && (tv[q] == "temporary" || tv[q] == "temp" || tv[q] == "unlogged")) q++ - if (!(tk[q] == "W" && tv[q] == "table")) continue - q++ - if (tk[q] == "W" && tv[q] == "if" && tk[q+1] == "W" && tv[q+1] == "not" && tk[q+2] == "W" && tv[q+2] == "exists") q += 3 - if (!parse_name(q)) continue - emit("C") - } else if (tv[p] == "alter") { - q = p + 1 - if (!(tk[q] == "W" && tv[q] == "table")) continue - q++ - if (tk[q] == "W" && tv[q] == "if" && tk[q+1] == "W" && tv[q+1] == "exists") q += 2 - if (tk[q] == "W" && tv[q] == "only") q++ - if (!parse_name(q)) continue - q = NAME_END - if (tk[q] == "W" && tv[q] == "enable" && tk[q+1] == "W" && tv[q+1] == "row" && - tk[q+2] == "W" && tv[q+2] == "level" && tk[q+3] == "W" && tv[q+3] == "security") emit("R") - } - } - } - ' "$STRIPPED" > "$WORK_TOK" + awk -f "$SQLSCAN" "$STRIPPED" > "$WORK_TOK" - # Read back as line triples. A delimiter a value can contain is not a delimiter, and a - # quoted identifier may contain any character. + # ONE LINE PER RECORD, TAB-SEPARATED, VALUES ESCAPED. It was three lines per record, and a + # quoted identifier containing a newline then shifted every following triple: a file with + # `public."weird"` and two more non-compliant tables reported ONE violation, naming + # `public.we` — a table that does not exist — and did not report the other two at all. A + # reader who "fixed" the named table would have got a green over an unguarded schema. The + # comment beside it claimed the limitation was "recorded in the README"; the word newline + # does not appear in that file. Escaping is exact for comparison, because both sides are + # escaped by the same function, and the escaped form is what the message prints. c_n=0; r_n=0 - while IFS= read -r kind && IFS= read -r sch && IFS= read -r tbl; do + while IFS="$(printf '\t')" read -r kind sch tbl; do case "$kind" in C) c_n=$((c_n+1)); C_SCH[$c_n]="$sch"; C_TBL[$c_n]="$tbl" ;; R) r_n=$((r_n+1)); R_SCH[$r_n]="$sch"; R_TBL[$r_n]="$tbl" ;; + U) unknown "$(basename "$up"): $sch — this gate could not read the statements after it, and will not call the file clean" ;; esac done < "$WORK_TOK" diff --git a/gates/service-role.sh b/gates/service-role.sh index de3b58b..85c21c5 100755 --- a/gates/service-role.sh +++ b/gates/service-role.sh @@ -55,9 +55,13 @@ trap 'rm -rf "$WORK"' EXIT # --------------------------------------------------------------------------- SCAN="$WORK/scan.awk" cat > "$SCAN" <<'SCANAWK' -# Emits one record per line: "S" for a literal import specifier, or "N" for an -# import()/require() this gate cannot read. Nothing else is emitted, so a string that merely -# ENDS in the word `from` produces nothing at all. +# Streaming JavaScript scanner. One record per line: "S" for a literal import +# specifier, or "N" for a branch this gate cannot read. A string that merely ENDS in the word +# `from` produces nothing at all. +# +# LINE-INCREMENTAL, NOT SLURPED, for the reason recorded in the SQL scanner: `buf = buf $0 +# "\n"` is quadratic in mawk and took 12.1s on a 1.1MB generated types file against 0.11s for +# the greps it replaced. State is carried across lines instead. function isregexpos(c) { return (c == "" || c == "(" || c == "," || c == "=" || c == ":" || c == "[" || c == "!" || c == "&" || c == "|" || c == "?" || c == "{" || c == "}" || c == ";" || c == "+" || @@ -65,103 +69,107 @@ function isregexpos(c) { c == "k") } function want(w) { return (w == "from" || w == "import" || w == "require") } -# JSX TEXT IS NOT JAVASCRIPT. `

Copied from "{"a"}" to "b"

` puts `from` immediately -# before a quote, and no tokeniser that is not also a JSX parser can tell that apart from an -# import. The candidate it yields carries a brace or an angle bracket, which a module -# specifier does not. -# -# Dropping it is only safe BECAUSE the swallow is gone. Under the old grep extractor a -# dropped candidate could be a real import that had been consumed into an invented span — -# that is exactly how the previous filter turned two reds into greens. Here every candidate -# comes from a genuine `from`/`import`/`require` position, so what is dropped is text, not -# an import this gate would otherwise have followed. +# JSX text is not JavaScript: `

Copied from "a" to "b"

` puts `from` before a quote and +# nothing short of a JSX parser tells that from an import. A module specifier carries none of +# these characters. Dropping such a candidate is only safe because the extractor no longer +# invents spans — see the note in the gate. function plausible(v) { return (v != "" && v !~ /[<>{}]/) } -{ buf = buf $0 "\n" } -END { - n = length(buf); i = 1; pend = ""; paren = 0; last = "" +# CLEARING A PENDING import(/require( IS THE REPORT. The first version decided this with a +# lookahead on the rest of the line, which cannot see a `import(` whose argument is on the NEXT +# line — the exact multi-line dynamic import that was a false green two releases ago, silently +# reintroduced. In a token stream the rule is simply: a pending import( closed by anything that +# is not a literal is a branch this gate cannot follow, whatever line that token is on. +function clearpend() { + if (paren == 1 && (pend == "import" || pend == "require")) print "N" + pend = ""; paren = 0 +} +{ + n = length($0); i = 1 while (i <= n) { - c = substr(buf, i, 1) - if (c == " " || c == "\t" || c == "\r" || c == "\n") { i++; continue } - if (c == "/" && substr(buf, i + 1, 1) == "/") { while (i <= n && substr(buf, i, 1) != "\n") i++; continue } - if (c == "/" && substr(buf, i + 1, 1) == "*") { - i += 2 - while (i <= n && !(substr(buf, i, 1) == "*" && substr(buf, i + 1, 1) == "/")) i++ - i += 2; continue + c = substr($0, i, 1) + if (mode == 1) { # inside /* */ + if (c == "*" && substr($0, i + 1, 1) == "/") { mode = 0; i += 2; continue } + i++; continue } + if (mode == 2) { # inside a template literal + if (c == "\\") { tv = tv substr($0, i + 1, 1); i += 2; continue } + if (c == "$" && substr($0, i + 1, 1) == "{") { + interp = 1; tdepth = 1; i += 2 + while (i <= n && tdepth > 0) { + c = substr($0, i, 1) + if (c == "{") tdepth++ + else if (c == "}") tdepth-- + i++ + } + continue + } + if (c == "`") { + mode = 0; i++ + if (want(pend)) { if (interp) print "N"; else if (plausible(tv)) print "S" tv } + pend = ""; paren = 0; last = "v"; tv = ""; interp = 0 + continue + } + tv = tv c; i++; continue + } + if (c == " " || c == "\t" || c == "\r") { i++; continue } + if (c == "/" && substr($0, i + 1, 1) == "/") break # rest of THIS line + if (c == "/" && substr($0, i + 1, 1) == "*") { mode = 1; i += 2; continue } if (c == "/" && isregexpos(last)) { i++; incls = 0 while (i <= n) { - c = substr(buf, i, 1) + c = substr($0, i, 1) if (c == "\\") { i += 2; continue } if (c == "[") incls = 1 else if (c == "]") incls = 0 else if (c == "/" && !incls) { i++; break } - else if (c == "\n") break i++ } - last = "v"; pend = ""; continue + last = "v"; clearpend(); continue } if (c == "\"" || c == "'") { + # Line-bounded on purpose. An apostrophe in JSX text would otherwise open a string that + # runs to the end of the file; ending it at the line boundary costs nothing, because a + # real specifier never spans lines. q = c; i++; v = "" while (i <= n) { - c = substr(buf, i, 1) - if (c == "\\") { v = v substr(buf, i + 1, 1); i += 2; continue } + c = substr($0, i, 1) + if (c == "\\") { v = v substr($0, i + 1, 1); i += 2; continue } if (c == q) { i++; break } - if (c == "\n") break v = v c; i++ } if (want(pend) && plausible(v)) print "S" v pend = ""; paren = 0; last = "v"; continue } - if (c == "`") { - i++; v = ""; interp = 0; depth = 0 - while (i <= n) { - c = substr(buf, i, 1) - if (c == "\\") { v = v substr(buf, i + 1, 1); i += 2; continue } - if (c == "$" && substr(buf, i + 1, 1) == "{") { interp = 1; depth = 1; i += 2 - while (i <= n && depth > 0) { - c = substr(buf, i, 1) - if (c == "{") depth++ - else if (c == "}") depth-- - i++ - } - continue - } - if (c == "`") { i++; break } - v = v c; i++ - } - if (want(pend)) { if (interp) print "N"; else if (plausible(v)) print "S" v } - pend = ""; paren = 0; last = "v"; continue - } + if (c == "`") { mode = 2; tv = ""; interp = 0; i++; continue } if (c ~ /[A-Za-z_$]/) { v = "" - while (i <= n) { c = substr(buf, i, 1); if (c !~ /[A-Za-z0-9_$]/) break; v = v c; i++ } - # A PROPERTY, NOT A KEYWORD. `Array.from(",")` put `from` in the keyword slot and the - # string after it was emitted as an import specifier. `.from`, `.import` and `.require` - # are method names; only a bare one can introduce a specifier. - if (want(v) && last != ".") { pend = v; paren = 0 } else pend = "" + while (i <= n) { c = substr($0, i, 1); if (c !~ /[A-Za-z0-9_$]/) break; v = v c; i++ } + # `.from` is a method name, not a keyword: `Array.from(",")` put the string after it in + # the specifier slot. + if (want(v) && last != ".") { clearpend(); pend = v; paren = 0 } else clearpend() last = (v == "return" || v == "typeof" || v == "case" || v == "in" || v == "of" || v == "new" || v == "delete" || v == "void" || v == "do" || v == "else" || v == "yield" || v == "await") ? "k" : "w" continue } if (c == "(") { - # `from` is never followed by `(` in an import — `import x from "y"` has no parens — so a - # `(` after it means this was some other `from`. - if (pend == "from") { pend = ""; last = "("; i++; continue } - if (pend != "" && paren == 0) { - paren = 1 - # Look ahead: an import()/require() whose argument is not a literal is a branch this - # gate cannot follow, and must be reported rather than assumed harmless. - j = i + 1 - while (j <= n && substr(buf, j, 1) ~ /[ \t\r\n]/) j++ - c2 = substr(buf, j, 1) - if (c2 != "\"" && c2 != "'" && c2 != "`" && pend != "from") { print "N"; pend = "" } - } else pend = "" + # `from` is never followed by `(` in an import — `import x from "y"` has no parens. + if (pend == "from") { clearpend(); last = "("; i++; continue } + if (pend != "" && paren == 0) paren = 1 + else clearpend() last = "("; i++; continue } - pend = ""; last = c; i++ + clearpend(); last = c; i++ } + if (mode == 2) tv = tv "\n" +} +END { + # A SCANNER THAT LOST SYNC MUST NOT REPORT A CLEAN FILE. A template literal or block comment + # still open at end of file means everything after it was read as something it is not. + # Measured before this guard: one stray backtick in JSX text (`

Press the ` key

`) + # consumed the rest of the file and a `require("@/lib/admin")` below it reported `ok`, + # exit 0, over a module reaching the secret. + if (mode == 2 || mode == 1) print "N" } SCANAWK From 842d4163894e08c2239ab388f1bab0d924d3750a Mon Sep 17 00:00:00 2001 From: Chii Date: Wed, 9 Sep 2026 00:01:46 +0000 Subject: [PATCH 08/13] A stage that cannot see strings was deciding what the scanner would read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round seven, against the tokeniser fixes. Both its Important findings trace to one seam: comment stripping ran BEFORE the scanner, in a stage with no string state, so the guards added last commit were downstream of the damage and could not see it. A LIVE FALSE GREEN, not a regression — it predates the rewrite and survived it: insert into public.notes (body) values ('x /* y'); create table public.orders (id int); insert into public.notes (body) values ('*/ z'); The `/*` INSIDE a string opened a block comment that deleted every line to the `*/` in the string below, taking the create table with it. Applied to PostgreSQL 16 the file leaves public.orders with RLS off; the gate said ok, exit 0. The two surviving quotes pair up, so the unterminated-string guard never fired either. An unterminated `/*` with no close anywhere did the same to the rest of the file. AND A REGRESSION I ADDED, same cause, opposite direction. The new string state saw the ALREADY-STRIPPED text, so when the stripper ate from a `/*` inside a string to end of file, the remaining odd `'` read as unterminated: insert into public.routes (pattern) values ('/api/*'); create table public.orders (id int); alter table public.orders enable row level security; A blocking red on a migration PostgreSQL confirms is compliant, with a message sending the reader to hunt an unterminated string that is not in the source. A route glob is enough. The message was as wrong as the colour: what lost sync was the stripper, one stage earlier, which the message did not name. So comments are the scanner's job now. One pass understands strings, quoted identifiers and comments together, there is no seam between them, and strip_sql_comments is gone rather than left for one caller — including its destructive-statement note, which the scanner now emits from the token stream so that a commented-out `drop table` is a comment. THE QUADRATIC FIX HAD ONLY MOVED THE PROBLEM. Last commit reported the accumulator closed; the same shape was one stage downstream in a nested bash loop comparing created tables to RLS-enabled ones. 2000 tables: 33.5s, quadrupling per doubling. It is a `grep -Fxv` now: 0.10s. The README carries both numbers, because the first fix was described as closing a problem half of which was still there. Three more from the same round: `create table exists` and `create table if` — both legal, both verified against PostgreSQL 16 — were swallowed entirely by a skip list that ate `if`/`not`/`exists` wherever they appeared instead of matching the sequence, so a create table went unseen; the DDL-in-a-string test was two substrings anywhere, so `values ('created three tables last week')` was a blocking UNKNOWN; and a pending `import(` left open at end of file reported nothing. The tsconfig comment stripper is line-incremental too — the README bullet disowning slurping sat two hundred lines above one still doing it. Fixture comment-marker-in-a-string: green under 3ace9d8, red now. Fourteen migrations-lint spellings re-measured. Selftest green. All six shell gates green on the template tree at deda1fa. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BfFq2KvV1CCkPs5WnV1AsA --- README.md | 31 ++-- .../migrations/20260106000000_orders.down.sql | 2 + .../migrations/20260106000000_orders.sql | 13 ++ gates/migrations-lint.sh | 134 ++++++++++-------- gates/service-role.sh | 29 ++-- 5 files changed, 126 insertions(+), 83 deletions(-) create mode 100644 fixtures/migrations-lint/bad/cases/comment-marker-in-a-string/supabase/migrations/20260106000000_orders.down.sql create mode 100644 fixtures/migrations-lint/bad/cases/comment-marker-in-a-string/supabase/migrations/20260106000000_orders.sql diff --git a/README.md b/README.md index 40f0cdd..e29ff72 100644 --- a/README.md +++ b/README.md @@ -315,13 +315,17 @@ header, which is the honest account of what that scanner does not do. on both. It does **not** read one inside a `'…'` string literal: a string is data to the scanner, and reading it named tables nobody created and sent fixers to edit their data — but a string containing both `create` and `table` is reported UNKNOWN rather than passed - over, because `execute` runs it. What it genuinely cannot read, as silent passes: a name - assembled at runtime (`execute format('create table %I …')` or string concatenation), - `select … into`, and a statement on a line where an earlier string literal contains `--`, - which the comment stripper takes for a comment. **An unterminated quoted identifier or - string is UNKNOWN, not a pass** — a scanner that lost sync read everything after it as - something it is not, and one `"` inside an ordinary string literal (an inch mark in - `values ('24" monitor')`) is enough to do that. + over, because `execute` runs it. Comments are recognised by the same scanner, not stripped by an + earlier stage: a stage that cannot see strings took `values ('x /* y')` for the start of a + block comment and deleted every line to the next `*/`, hiding a whole `create table` — + verified against PostgreSQL 16 as a real table left with RLS off — and the same swallow + turned a compliant migration containing `values ('/api/*')` red with a message naming a + string that does not exist. What it genuinely cannot read, as silent passes: a name + assembled at runtime (`execute format('create table %I …')` or string concatenation) and + `select … into`. **An unterminated quoted identifier, string or block comment is UNKNOWN, + not a pass** — a scanner that lost sync read everything after it as something it is not, + and one `"` inside an ordinary string literal (an inch mark in `values ('24" monitor')`) + is enough to do that. - **The service-role gate tokenises JavaScript; it does not match it.** Strings, template literals, line and block comments and regex literals are recognised as what they are, and a specifier is emitted only from a real `from`/`import`/`require` position. This replaced a @@ -340,12 +344,15 @@ header, which is the honest account of what that scanner does not do. can tell that from an import, so a candidate carrying `<`, `>`, `{` or `}` is dropped as text. Node subpath imports (`#internal/db`) are UNKNOWN: they resolve through `package.json` `imports`, which this gate does not read. -- **Both scanners are line-incremental, and that is a correctness property, not a speed one.** - The first version accumulated each file with `buf = buf $0 "\n"`, which mawk reallocates and +- **Every scanner here is line-incremental, and the comparison is not a shell loop.** The + first version accumulated each file with `buf = buf $0 "\n"`, which mawk reallocates and copies every line: 12.1s on a 1.1MB generated types file against 0.11s for the greps it - replaced, and quadratic, so it got worse with size. `gates.yml` sets no `timeout-minutes`, - so that surfaces not as a red but as a job that takes minutes — the shape of the hang this - toolkit has already shipped once. + replaced, and quadratic. Fixing that left the same shape one stage downstream — a nested + bash loop comparing created tables against RLS-enabled ones, 33.5s for 2000 tables — which + is now a `grep -Fxv`, 0.10s. `gates.yml` sets no `timeout-minutes`, so either would have + surfaced not as a red but as a job taking minutes: the shape of the hang this toolkit has + already shipped once. Both numbers are here because the first fix was reported as closing + the problem while half of it was still there. - **The service-role gate resolves like TypeScript, with one deliberate divergence.** It follows `./`, `../`, absolute paths, and **every alias declared in tsconfig `compilerOptions.paths`** — not just `@/*` — plus a **`baseUrl` with no matching `paths` diff --git a/fixtures/migrations-lint/bad/cases/comment-marker-in-a-string/supabase/migrations/20260106000000_orders.down.sql b/fixtures/migrations-lint/bad/cases/comment-marker-in-a-string/supabase/migrations/20260106000000_orders.down.sql new file mode 100644 index 0000000..344e7c9 --- /dev/null +++ b/fixtures/migrations-lint/bad/cases/comment-marker-in-a-string/supabase/migrations/20260106000000_orders.down.sql @@ -0,0 +1,2 @@ +drop table if exists public.orders; +drop table if exists public.notes; diff --git a/fixtures/migrations-lint/bad/cases/comment-marker-in-a-string/supabase/migrations/20260106000000_orders.sql b/fixtures/migrations-lint/bad/cases/comment-marker-in-a-string/supabase/migrations/20260106000000_orders.sql new file mode 100644 index 0000000..da237e6 --- /dev/null +++ b/fixtures/migrations-lint/bad/cases/comment-marker-in-a-string/supabase/migrations/20260106000000_orders.sql @@ -0,0 +1,13 @@ +-- A `/*` INSIDE a string literal. Comment stripping used to run as a separate stage with no +-- string state, so this opened a block comment that deleted every line until the `*/` in the +-- string below it — taking the create table with it. Verified against PostgreSQL 16: the file +-- applies cleanly and leaves public.orders with RLS off, while the gate reported ok, exit 0. +-- The two surviving quotes pair up, so the unterminated-string guard never fired either. +create table public.notes (body text); +alter table public.notes enable row level security; + +insert into public.notes (body) values ('x /* y'); + +create table public.orders (id int); + +insert into public.notes (body) values ('*/ z'); diff --git a/gates/migrations-lint.sh b/gates/migrations-lint.sh index a40ad7d..ccdd9a8 100755 --- a/gates/migrations-lint.sh +++ b/gates/migrations-lint.sh @@ -31,32 +31,7 @@ MIG="$ROOT/supabase/migrations" # the rest of that line. That is a real gap, not a safe one — a create table sharing that # line would go unchecked. A SQL parser is the fix if it ever bites; a regex that pretends # to know about quoting is not. -strip_sql_comments() { - awk ' - BEGIN { inblk = 0 } - { - line = $0; out = "" - while (length(line) > 0) { - if (inblk) { - p = index(line, "*/") - if (p == 0) { line = ""; break } - line = substr(line, p + 2); inblk = 0; out = out " " - } else { - pb = index(line, "/*"); pl = index(line, "--") - if (pl > 0 && (pb == 0 || pl < pb)) { out = out substr(line, 1, pl - 1); line = ""; break } - # A SPACE, not nothing. Concatenating the text before `/*` to the text after `*/` - # made `create/*x*/table public.orders` one word, `createtable`, and the statement - # a silent pass. A comment separates tokens; removing it must not join them. - if (pb > 0) { out = out substr(line, 1, pb - 1) " "; line = substr(line, pb + 2); inblk = 1 } - else { out = out line; line = "" } - } - } - print out - } - ' "$1" -} -STRIPPED="$(mktemp)" SQLSCAN="$(mktemp)" cat > "$SQLSCAN" <<'SQLSCANAWK' # Streaming SQL scanner. One record per line: kind schema table, where kind is @@ -84,14 +59,36 @@ function tok(k, v, again) { if (st == 0) { if (k == "W" && v == "create") st = 1 else if (k == "W" && v == "alter") st = 4 + # The destructive-statement note is emitted from the token stream too, so that the + # comment stripper can go away entirely rather than survive for one caller. A + # commented-out `drop table` is a comment, and a note claiming otherwise is a claim. + else if (k == "W" && v == "drop") st = 8 + else if (k == "W" && v == "truncate") { print "X\t\t"; } + else if (k == "W" && v == "delete") st = 9 + } else if (st == 8) { + if (k == "W" && v == "table") { print "X\t\t"; st = 0 } + else { st = 0; again = 1 } + } else if (st == 9) { + if (k == "W" && v == "from") { print "X\t\t"; st = 0 } + else { st = 0; again = 1 } } else if (st == 1) { if (k == "W" && (v == "global" || v == "local" || v == "temporary" || v == "temp" || v == "unlogged")) { } else if (k == "W" && v == "table") st = 2 else { st = 0; again = 1 } } else if (st == 2) { - if (k == "W" && (v == "if" || v == "not" || v == "exists")) { } + # `IF NOT EXISTS` AS A SEQUENCE, not three words to swallow wherever they appear. + # Swallowing them lost `create table exists (id int)` and `create table if (id int)` + # entirely — both legal, both verified against PostgreSQL 16, and a create table this + # scanner does not see is a silent pass. + if (k == "W" && v == "if") st = 21 else if (k == "W" || k == "Q") { nn = 1; P[1] = v; st = 3; wantpart = 0 } else { st = 0; again = 1 } + } else if (st == 21) { + if (k == "W" && v == "not") st = 22 + else { nn = 1; P[1] = "if"; st = 3; wantpart = 0; again = 1 } + } else if (st == 22) { + if (k == "W" && v == "exists") st = 2 + else { nn = 1; P[1] = "if"; st = 3; wantpart = 0; again = 1 } } else if (st == 3) { if (k == "D") wantpart = 1 else if (wantpart && (k == "W" || k == "Q")) { P[++nn] = v; wantpart = 0 } @@ -122,6 +119,26 @@ function tok(k, v, again) { n = length($0); i = 1 while (i <= n) { c = substr($0, i, 1) + # COMMENTS ARE STRIPPED HERE, NOT BY A STAGE THAT CANNOT SEE STRINGS. They used to be + # removed upstream by a scanner with no string state, which cost a defect in each + # direction and both were live: + # + # values ('x /* y'); create table public.orders (id int); values ('*/ z'); + # -> the `/*` INSIDE a string opened a block comment that deleted the create table + # on the following line. `ok`, exit 0, over a table PostgreSQL 16 confirms is + # created with RLS off. The two surviving quotes pair up, so the guard below + # never fired either. + # values ('/api/*'); create table public.orders … alter table … enable row level … + # -> the same swallow ate to end of file, the remaining odd `'` read as an + # unterminated string, and a fully compliant migration went RED with a message + # naming a string that does not exist in the source. + # + # One scanner that understands strings, identifiers and comments together has no such + # seam. An unterminated block comment is UNKNOWN, like every other lost sync. + if (inblk) { + if (c == "*" && substr($0, i + 1, 1) == "/") { inblk = 0; i += 2; continue } + i++; continue + } # A DOUBLE-QUOTED IDENTIFIER, possibly spanning lines. `""` inside it is one embedded # quote. if (inq) { @@ -140,12 +157,17 @@ function tok(k, v, again) { if (c == "'") { if (substr($0, i + 1, 1) == "'") { sv = sv "'"; i += 2; continue } ins = 0; i++ - if (tolower(sv) ~ /create/ && tolower(sv) ~ /table/) ddl = 1 + # `create` then at most two modifier words then `table`, not "created" and "tables" + # anywhere in the same sentence — `values ('created three tables last week')` was a + # blocking UNKNOWN on an otherwise compliant file. + if (tolower(sv) ~ /(^|[^a-z])create[ \t\n]+([a-z]+[ \t\n]+){0,2}table([^a-z]|$)/) ddl = 1 sv = ""; continue } sv = sv c; i++; continue } if (c == " " || c == "\t" || c == "\r") { i++; continue } + if (c == "-" && substr($0, i + 1, 1) == "-") break + if (c == "/" && substr($0, i + 1, 1) == "*") { inblk = 1; i += 2; continue } if (c == "\"") { inq = 1; qv = ""; i++; continue } if (c == "'") { ins = 1; sv = ""; i++; continue } if (c ~ /[A-Za-z_]/) { @@ -164,6 +186,7 @@ END { # A SCANNER THAT LOST SYNC MUST NOT REPORT A CLEAN FILE. An unterminated quoted identifier # or string means everything after it was read as something it is not, so the only honest # answer about the rest of the file is that this gate could not read it. + if (inblk) print "U\tunterminated-block-comment\t" if (inq) print "U\tunterminated-quoted-identifier\t" if (ins) print "U\tunterminated-string\t" # DDL inside a string literal is executed by `execute`, and this scanner treats the string @@ -175,14 +198,13 @@ SQLSCANAWK WORK_TOK="$(mktemp)" WORK_C="$(mktemp)" WORK_R="$(mktemp)" -trap 'rm -f "$STRIPPED" "$SQLSCAN" "$WORK_TOK" "$WORK_C" "$WORK_R"' EXIT +trap 'rm -f "$SQLSCAN" "$WORK_TOK" "$WORK_C" "$WORK_R"' EXIT for up in "$MIG"/*.sql; do [ -e "$up" ] || continue case "$up" in *.down.sql) continue;; esac down="${up%.sql}.down.sql" [ -f "$down" ] || fail "no rollback: $(basename "$up") needs $(basename "$down")" - strip_sql_comments "$up" > "$STRIPPED" # TABLES CREATED HERE MUST ENABLE RLS HERE — DECIDED BY TOKENISING, NOT BY MATCHING. # # This check was a regex three times over and produced a finding in each of three @@ -210,42 +232,36 @@ for up in "$MIG"/*.sql; do # wider than the name it was given, no case flag to fold a quoted identifier, and no line # boundary to hide a statement behind. : > "$WORK_C"; : > "$WORK_R" - awk -f "$SQLSCAN" "$STRIPPED" > "$WORK_TOK" + awk -f "$SQLSCAN" "$up" > "$WORK_TOK" # ONE LINE PER RECORD, TAB-SEPARATED, VALUES ESCAPED. It was three lines per record, and a # quoted identifier containing a newline then shifted every following triple: a file with # `public."weird"` and two more non-compliant tables reported ONE violation, naming - # `public.we` — a table that does not exist — and did not report the other two at all. A - # reader who "fixed" the named table would have got a green over an unguarded schema. The - # comment beside it claimed the limitation was "recorded in the README"; the word newline - # does not appear in that file. Escaping is exact for comparison, because both sides are - # escaped by the same function, and the escaped form is what the message prints. - c_n=0; r_n=0 - while IFS="$(printf '\t')" read -r kind sch tbl; do - case "$kind" in - C) c_n=$((c_n+1)); C_SCH[$c_n]="$sch"; C_TBL[$c_n]="$tbl" ;; - R) r_n=$((r_n+1)); R_SCH[$r_n]="$sch"; R_TBL[$r_n]="$tbl" ;; - U) unknown "$(basename "$up"): $sch — this gate could not read the statements after it, and will not call the file clean" ;; - esac - done < "$WORK_TOK" + # `public.we` — a table that does not exist — and did not report the other two at all. + # Escaping is exact for comparison, because both sides are escaped by the same function. + # + # COMPARED WITH grep, NOT A NESTED SHELL LOOP. The loop was O(created x rls): 2000 compliant + # tables took 33.5s, quadrupling per doubling. That is the same shape the awk accumulator + # had — the fix for which had only moved it from awk into bash, which is worth saying out + # loud rather than calling the quadratic bullet closed. + set +e + grep '^C ' "$WORK_TOK" | cut -f2- > "$WORK_C" + grep '^R ' "$WORK_TOK" | cut -f2- > "$WORK_R" + set -e + + while IFS= read -r reason; do + [ -n "$reason" ] || continue + unknown "$(basename "$up"): $reason — this gate could not read the statements after it, and will not call the file clean" + done < <(grep '^U ' "$WORK_TOK" | cut -f2 || true) - i=1 - while [ "$i" -le "$c_n" ]; do - found=0 - j=1 - while [ "$j" -le "$r_n" ]; do - if [ "${C_SCH[$i]}" = "${R_SCH[$j]}" ] && [ "${C_TBL[$i]}" = "${R_TBL[$j]}" ]; then found=1; break; fi - j=$((j+1)) - done - if [ "$found" -eq 0 ]; then - fail "$(basename "$up"): table '${C_SCH[$i]}.${C_TBL[$i]}' created without 'enable row level security' in the same file" - fi - i=$((i+1)) - done - unset C_SCH C_TBL R_SCH R_TBL + while IFS="$(printf '\t')" read -r sch tbl; do + [ -n "$tbl" ] || continue + fail "$(basename "$up"): table '$sch.$tbl' created without 'enable row level security' in the same file" + done < <(grep -Fxv -f "$WORK_R" -- "$WORK_C" || true) - # destructive statements outside a WHERE are tier-3 by regex (non-negotiable 4); flag, do not block - if grep -qiE "^[[:space:]]*(drop[[:space:]]+table|truncate|delete[[:space:]]+from[[:space:]]+[a-z_.\"]+[[:space:]]*;)" "$STRIPPED"; then + # destructive statements are tier-3 by note, not by block (non-negotiable 4). Emitted by + # the scanner from the token stream, so a commented-out `drop table` is a comment. + if grep -q '^X ' "$WORK_TOK" 2>/dev/null; then echo "note [$GATE] $(basename "$up"): destructive statement present; this migration is tier-3" fi done diff --git a/gates/service-role.sh b/gates/service-role.sh index 85c21c5..d207f3c 100755 --- a/gates/service-role.sh +++ b/gates/service-role.sh @@ -170,6 +170,9 @@ END { # consumed the rest of the file and a `require("@/lib/admin")` below it reported `ok`, # exit 0, over a module reaching the secret. if (mode == 2 || mode == 1) print "N" + # A pending import( still open at end of file is the same report as one closed by a + # non-literal: the argument was never a literal this gate could read. + clearpend() } SCANAWK @@ -240,30 +243,32 @@ TSCONFIG_RAW="$ROOT/tsconfig.json" # is not configuration. TSCONFIG="$WORK/tsconfig.json" if [ -f "$TSCONFIG_RAW" ]; then + # Line-incremental, like the two token scanners. It slurped, and the README bullet that + # disowns slurping sat two hundred lines above one still doing it. awk ' - { buf = buf $0 "\n" } - END { - n = length(buf); i = 1 + { + n = length($0); i = 1 while (i <= n) { - c = substr(buf, i, 1) + c = substr($0, i, 1) + if (blk) { + if (c == "*" && substr($0, i + 1, 1) == "/") { blk = 0; i += 2; continue } + i++; continue + } if (c == "\"") { printf "%s", c; i++ while (i <= n) { - c = substr(buf, i, 1) - if (c == "\\") { printf "%s", substr(buf, i, 2); i += 2; continue } + c = substr($0, i, 1) + if (c == "\\") { printf "%s", substr($0, i, 2); i += 2; continue } printf "%s", c; i++ if (c == "\"") break } continue } - if (c == "/" && substr(buf, i + 1, 1) == "/") { while (i <= n && substr(buf, i, 1) != "\n") i++; continue } - if (c == "/" && substr(buf, i + 1, 1) == "*") { - i += 2 - while (i <= n && !(substr(buf, i, 1) == "*" && substr(buf, i + 1, 1) == "/")) i++ - i += 2; continue - } + if (c == "/" && substr($0, i + 1, 1) == "/") break + if (c == "/" && substr($0, i + 1, 1) == "*") { blk = 1; i += 2; continue } printf "%s", c; i++ } + printf "\n" } ' "$TSCONFIG_RAW" > "$TSCONFIG" 2>/dev/null || cp -- "$TSCONFIG_RAW" "$TSCONFIG" 2>/dev/null || : fi From 036f9392d0f5c8a9a34b90103aaba2da200653d5 Mon Sep 17 00:00:00 2001 From: Chii Date: Wed, 9 Sep 2026 00:02:14 +0000 Subject: [PATCH 09/13] Make good on a claim the last commit made about the README MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 3ace9d8's message said the word "newline" now appeared in README.md and that the behaviour was documented there. `grep -c newline README.md` returns 0. The rewrite of that bullet dropped it, and the claim went out anyway — a commit message asserting a state of the tree that was not the state of the tree, which is the same defect this branch keeps finding in code, in the record of fixing it. The behaviour is worth the line on its own terms: a quoted identifier may contain a tab or a newline, those are escaped in the scanner's output and compared escaped, and the escaped form is what the message prints. Before that, such a name shifted every following record — one violation reported, naming a table that does not exist, two real ones silently dropped. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BfFq2KvV1CCkPs5WnV1AsA --- README.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index e29ff72..fac7d21 100644 --- a/README.md +++ b/README.md @@ -325,7 +325,11 @@ header, which is the honest account of what that scanner does not do. `select … into`. **An unterminated quoted identifier, string or block comment is UNKNOWN, not a pass** — a scanner that lost sync read everything after it as something it is not, and one `"` inside an ordinary string literal (an inch mark in `values ('24" monitor')`) - is enough to do that. + is enough to do that. A quoted identifier may contain a tab or a **newline**; those are + escaped in the scanner's output and compared escaped, so a name containing one no longer + shifts the records after it — that shift once reported a single violation naming a table + that does not exist while silently dropping two real ones. The escaped form is what the + message prints, so `public."weird"` reads as `public.we\nird`. - **The service-role gate tokenises JavaScript; it does not match it.** Strings, template literals, line and block comments and regex literals are recognised as what they are, and a specifier is emitted only from a real `from`/`import`/`require` position. This replaced a From 8867c727c602753644bf5deafe530a1fe1b2f7cc Mon Sep 17 00:00:00 2001 From: Chii Date: Wed, 9 Sep 2026 00:16:57 +0000 Subject: [PATCH 10/13] Two false greens the last commit added, and one byte that could hide the secret MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round eight was asked the convergence question directly, and told that "no new defect" was an acceptable answer. It said no. Both of the changes flagged as highest risk in that brief shipped a silent pass, and both are mine. GREP'S BINARY HEURISTIC DROPPED VIOLATION RECORDS. Replacing the quadratic bash comparison with grep introduced a dependency nobody stated: one byte that is invalid in the ambient locale makes GNU grep declare the records file binary and SUPPRESS the matching line while still exiting 0. Measured under LC_ALL=C.UTF-8, the locale on ubuntu-latest, over a file creating public.clean and public."año" with RLS on neither: 036f939: 1 violation 3ace9d8: 2 violations The dropped table is real and its RLS is off; PostgreSQL 16 confirms both. And the loss looked like a smaller count rather than a missing check, because the clean table beside it still reported. AND THE SAME CLASS, OLDER AND WORSE, IN THE SECURITY GATE. The term search had no `-a` either, and it has not since v1.1.0. A module holding SUPABASE_SERVICE_ROLE_KEY plus one NUL byte, reached from a page: ok [service-role] exit 0 grep called the file binary, printed nothing, exited 1, and the gate read that as "the term is not present". One byte hid the secret. Every grep over file content in both gates now passes `-a`. MAWK MISCOMPILES `{0,2}`. The DDL-in-a-string test was tightened last commit to a create...table sequence with a modifier allowance written `{0,2}`. mawk 1.3.4 — the awk on Debian and Ubuntu, and the one this README names by hand — matches ZERO repetitions when an interval with n>=2 is applied to a group whose body starts with a `+`-quantified bracket. So the allowance was inert, the test was exactly `create table`, and `execute 'CREATE UNLOGGED TABLE public.x (id int)'` passed over in silence. PostgreSQL 16: a persistent unlogged table, RLS off, gate green. The non-string path reads those modifiers correctly, which is why no fixture saw it. Two smaller things from the same reading. The destructive-statement note fired on every `delete from`, including one with a WHERE, while the comment beside it said "outside a WHERE" — a note claiming more than the code did. And one UNKNOWN message served two unrelated reasons: for a DDL string it said the gate "could not read the statements after it", which is false, since what it cannot read is the DDL inside the string. The reason now picks the sentence. The selftest caught my own misfiling on the way: modifier-table-in-a-string is UNKNOWN with no violation, so it belongs under bad/unknown/, and the leg added three commits ago said so. Three fixtures, each verified against 036f939: non-utf8-identifier (1 violation there, 2 here), modifier-table-in-a-string (ok there, UNKNOWN here), nul-byte-in-a-module (ok there, FAIL here). Selftest green. All six shell gates green on the template tree at deda1fa. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BfFq2KvV1CCkPs5WnV1AsA --- README.md | 16 +++++- .../migrations/20260107000000_orders.down.sql | 1 + .../migrations/20260107000000_orders.sql | 7 +++ .../migrations/20260108000000_orders.down.sql | 1 + .../migrations/20260108000000_orders.sql | 9 ++++ .../cases/nul-byte-in-a-module/app/page.tsx | 8 +++ .../cases/nul-byte-in-a-module/lib/admin.ts | Bin 0 -> 77 bytes .../cases/nul-byte-in-a-module/package.json | 1 + gates/migrations-lint.sh | 47 +++++++++++++++--- gates/service-role.sh | 21 +++++--- 10 files changed, 93 insertions(+), 18 deletions(-) create mode 100644 fixtures/migrations-lint/bad/cases/non-utf8-identifier/supabase/migrations/20260107000000_orders.down.sql create mode 100644 fixtures/migrations-lint/bad/cases/non-utf8-identifier/supabase/migrations/20260107000000_orders.sql create mode 100644 fixtures/migrations-lint/bad/unknown/modifier-table-in-a-string/supabase/migrations/20260108000000_orders.down.sql create mode 100644 fixtures/migrations-lint/bad/unknown/modifier-table-in-a-string/supabase/migrations/20260108000000_orders.sql create mode 100644 fixtures/service-role/bad/cases/nul-byte-in-a-module/app/page.tsx create mode 100644 fixtures/service-role/bad/cases/nul-byte-in-a-module/lib/admin.ts create mode 100644 fixtures/service-role/bad/cases/nul-byte-in-a-module/package.json diff --git a/README.md b/README.md index fac7d21..91073ff 100644 --- a/README.md +++ b/README.md @@ -314,8 +314,12 @@ header, which is the honest account of what that scanner does not do. given. It reads a `create table` inside a function body and inside a `DO $$` block, and reds on both. It does **not** read one inside a `'…'` string literal: a string is data to the scanner, and reading it named tables nobody created and sent fixers to edit their data — - but a string containing both `create` and `table` is reported UNKNOWN rather than passed - over, because `execute` runs it. Comments are recognised by the same scanner, not stripped by an + but a string containing a `create … table` **sequence** is reported UNKNOWN rather than + passed over, because `execute` runs it. Sequence, not two words anywhere: `'created three + tables last week'` is prose and passes, while `'CREATE UNLOGGED TABLE …'` does not. That + allowance was once written `{0,2}`, which **mawk miscompiles** to zero repetitions when the + group begins with a `+`-quantified bracket — so every modifier form passed silently until + it was measured. Comments are recognised by the same scanner, not stripped by an earlier stage: a stage that cannot see strings took `values ('x /* y')` for the start of a block comment and deleted every line to the next `*/`, hiding a whole `create table` — verified against PostgreSQL 16 as a real table left with RLS off — and the same swallow @@ -348,6 +352,14 @@ header, which is the honest account of what that scanner does not do. can tell that from an import, so a candidate carrying `<`, `>`, `{` or `}` is dropped as text. Node subpath imports (`#internal/db`) are UNKNOWN: they resolve through `package.json` `imports`, which this gate does not read. +- **Every `grep` over file content passes `-a`.** Without it, one byte that is invalid in the + ambient locale makes GNU grep declare a file binary, print nothing and exit non-zero — which + a gate reads as "not found". Measured: a module holding `SUPABASE_SERVICE_ROLE_KEY` plus a + single NUL byte, reached from a page, reported `ok`, exit 0; and a migration creating a + table whose quoted name held a LATIN1 byte had that table dropped from the comparison + entirely while the clean table beside it was still reported, so the loss looked like a + smaller violation count rather than a missing check. One byte must not decide whether a + gate can see a secret. - **Every scanner here is line-incremental, and the comparison is not a shell loop.** The first version accumulated each file with `buf = buf $0 "\n"`, which mawk reallocates and copies every line: 12.1s on a 1.1MB generated types file against 0.11s for the greps it diff --git a/fixtures/migrations-lint/bad/cases/non-utf8-identifier/supabase/migrations/20260107000000_orders.down.sql b/fixtures/migrations-lint/bad/cases/non-utf8-identifier/supabase/migrations/20260107000000_orders.down.sql new file mode 100644 index 0000000..bdadf11 --- /dev/null +++ b/fixtures/migrations-lint/bad/cases/non-utf8-identifier/supabase/migrations/20260107000000_orders.down.sql @@ -0,0 +1 @@ +drop table if exists public.clean; diff --git a/fixtures/migrations-lint/bad/cases/non-utf8-identifier/supabase/migrations/20260107000000_orders.sql b/fixtures/migrations-lint/bad/cases/non-utf8-identifier/supabase/migrations/20260107000000_orders.sql new file mode 100644 index 0000000..99079b5 --- /dev/null +++ b/fixtures/migrations-lint/bad/cases/non-utf8-identifier/supabase/migrations/20260107000000_orders.sql @@ -0,0 +1,7 @@ +-- A LATIN1 byte in a quoted identifier. `grep` without `-a` calls the records file +-- binary under an ordinary UTF-8 locale, SUPPRESSES the matching line and still exits 0, +-- so this table never reached the comparison and was never checked. The clean table +-- beside it was, which is what made the loss invisible: one violation reported where +-- two are planted. PostgreSQL 16 confirms both tables are real and both have RLS off. +create table public.clean (id int); +create table public."año" (id int); diff --git a/fixtures/migrations-lint/bad/unknown/modifier-table-in-a-string/supabase/migrations/20260108000000_orders.down.sql b/fixtures/migrations-lint/bad/unknown/modifier-table-in-a-string/supabase/migrations/20260108000000_orders.down.sql new file mode 100644 index 0000000..6e997cb --- /dev/null +++ b/fixtures/migrations-lint/bad/unknown/modifier-table-in-a-string/supabase/migrations/20260108000000_orders.down.sql @@ -0,0 +1 @@ +drop table if exists public.o; diff --git a/fixtures/migrations-lint/bad/unknown/modifier-table-in-a-string/supabase/migrations/20260108000000_orders.sql b/fixtures/migrations-lint/bad/unknown/modifier-table-in-a-string/supabase/migrations/20260108000000_orders.sql new file mode 100644 index 0000000..7d16004 --- /dev/null +++ b/fixtures/migrations-lint/bad/unknown/modifier-table-in-a-string/supabase/migrations/20260108000000_orders.sql @@ -0,0 +1,9 @@ +-- `execute` running DDL this gate cannot follow must be UNKNOWN, and it was — until the +-- modifier allowance in that test was written as {0,2}, which mawk miscompiles to zero +-- repetitions when the group starts with a +-quantified bracket. The allowance was inert and +-- the test became exactly `create table`, so this line passed over silently. PostgreSQL 16 +-- confirms it creates a persistent unlogged table with RLS off. +create table public.o (id int); +alter table public.o enable row level security; + +do $x$ begin execute 'CREATE UNLOGGED TABLE public.x (id int)'; end $x$; diff --git a/fixtures/service-role/bad/cases/nul-byte-in-a-module/app/page.tsx b/fixtures/service-role/bad/cases/nul-byte-in-a-module/app/page.tsx new file mode 100644 index 0000000..a8f2957 --- /dev/null +++ b/fixtures/service-role/bad/cases/nul-byte-in-a-module/app/page.tsx @@ -0,0 +1,8 @@ +// The module below holds the service-role key and contains one NUL byte. Without `-a`, grep +// calls that file binary, prints nothing and exits 1 — which this gate reads as "the term is +// not present". One byte hid the secret and the gate reported ok, exit 0. +import { admin } from "../lib/admin"; + +export default function Page() { + return admin; +} diff --git a/fixtures/service-role/bad/cases/nul-byte-in-a-module/lib/admin.ts b/fixtures/service-role/bad/cases/nul-byte-in-a-module/lib/admin.ts new file mode 100644 index 0000000000000000000000000000000000000000..d042d5519e607d24e67e48bf7712bc9f4d03cc41 GIT binary patch literal 77 zcmdPbSMUq=2 applied to a group whose body starts + # with a `+`-quantified bracket expression: it matches ZERO repetitions only. So the + # modifier allowance was inert and the test was exactly `create table`, which let + # `execute 'CREATE UNLOGGED TABLE public.x (id int)'` pass over silently. Verified + # against PostgreSQL 16: a persistent unlogged table, RLS off, gate green. The + # non-string path at the top of this scanner reads those modifiers correctly, so the + # gap was invisible from the fixtures. + if (tolower(sv) ~ /(^|[^a-z])create[ \t\n]+([a-z]+[ \t\n]+)*table([^a-z]|$)/) ddl = 1 sv = ""; continue } sv = sv c; i++; continue @@ -244,24 +258,41 @@ for up in "$MIG"/*.sql; do # tables took 33.5s, quadrupling per doubling. That is the same shape the awk accumulator # had — the fix for which had only moved it from awk into bash, which is worth saying out # loud rather than calling the quadratic bullet closed. + # `-a` ON EVERY grep THAT READS THESE RECORDS. Without it, one byte that is invalid in the + # ambient locale makes GNU grep declare the file binary and SUPPRESS the matching line while + # still exiting 0 — so the record never reaches $WORK_C and that table is never compared + # against RLS at all. Measured under LC_ALL=C.UTF-8, the locale on ubuntu-latest: a file + # creating `public.clean` and `public."año"` (LATIN1), neither with RLS, reported ONE + # violation where the previous revision reported two. A NUL byte does it in any locale. + # PostgreSQL 16 confirms the dropped table is real and its RLS is off. A silent pass chosen + # by an encoding, and it arrived with the change that made this comparison a grep. set +e - grep '^C ' "$WORK_TOK" | cut -f2- > "$WORK_C" - grep '^R ' "$WORK_TOK" | cut -f2- > "$WORK_R" + grep -a '^C ' "$WORK_TOK" | cut -f2- > "$WORK_C" + grep -a '^R ' "$WORK_TOK" | cut -f2- > "$WORK_R" set -e while IFS= read -r reason; do [ -n "$reason" ] || continue - unknown "$(basename "$up"): $reason — this gate could not read the statements after it, and will not call the file clean" - done < <(grep '^U ' "$WORK_TOK" | cut -f2 || true) + # THE REASON DECIDES THE SENTENCE. One message covered both kinds, and for a DDL string it + # was false: the scanner reads the statements after it perfectly well; what it does not + # read is the DDL inside the string. A message wider than the thing it describes is the + # defect this repository exists to catch. + case "$reason" in + ddl-inside-a-string-literal) + unknown "$(basename "$up"): a string literal here contains a 'create ... table' that \`execute\` would run. This gate reads strings as data, so it cannot say what that statement creates or whether RLS follows it" ;; + *) + unknown "$(basename "$up"): $reason — this gate could not read the statements after it, and will not call the file clean" ;; + esac + done < <(grep -a '^U ' "$WORK_TOK" | cut -f2 || true) while IFS="$(printf '\t')" read -r sch tbl; do [ -n "$tbl" ] || continue fail "$(basename "$up"): table '$sch.$tbl' created without 'enable row level security' in the same file" - done < <(grep -Fxv -f "$WORK_R" -- "$WORK_C" || true) + done < <(grep -aFxv -f "$WORK_R" -- "$WORK_C" || true) # destructive statements are tier-3 by note, not by block (non-negotiable 4). Emitted by # the scanner from the token stream, so a commented-out `drop table` is a comment. - if grep -q '^X ' "$WORK_TOK" 2>/dev/null; then + if grep -qa '^X ' "$WORK_TOK" 2>/dev/null; then echo "note [$GATE] $(basename "$up"): destructive statement present; this migration is tier-3" fi done diff --git a/gates/service-role.sh b/gates/service-role.sh index d207f3c..a978856 100755 --- a/gates/service-role.sh +++ b/gates/service-role.sh @@ -185,7 +185,7 @@ SCANAWK # and failing to find it means this gate does not understand the layout. # --------------------------------------------------------------------------- PKG="$ROOT/package.json" -if [ ! -f "$PKG" ] || ! grep -qE '"next"[[:space:]]*:' "$PKG"; then +if [ ! -f "$PKG" ] || ! grep -qaE '"next"[[:space:]]*:' "$PKG"; then echo "not a Next.js app (no package.json depending on \"next\") — no request path to walk" finish fi @@ -297,7 +297,7 @@ if [ -f "$TSCONFIG" ]; then # of the same key — and the shipped fixture used "src", so the selftest was green over the # half that worked. BASE_DIR is already $ROOT when the value is "." or "./", so nothing else # needs to change: the key being there is the whole condition. - if grep -q '"baseUrl"' "$TSCONFIG" 2>/dev/null; then BASEURL_SET=1; fi + if grep -qa '"baseUrl"' "$TSCONFIG" 2>/dev/null; then BASEURL_SET=1; fi # The `paths` object, isolated exactly rather than read line by line. # @@ -327,13 +327,13 @@ if [ -f "$TSCONFIG" ]; then # "": [ "" — extracted by shape, from anywhere in the alias body, so the # same code reads a pretty-printed tsconfig and a minified one. - grep -oE '"[^"]+"[[:space:]]*:[[:space:]]*\[[[:space:]]*"[^"]+"' "$WORK/pathsbody" 2>/dev/null \ + grep -aoE '"[^"]+"[[:space:]]*:[[:space:]]*\[[[:space:]]*"[^"]+"' "$WORK/pathsbody" 2>/dev/null \ | sed -E 's/"([^"]+)"[[:space:]]*:[[:space:]]*\[[[:space:]]*"([^"]+)"/\1\t\2/' > "$ALIASES" 2>/dev/null || true # `extends` is not followed. A base config holding the aliases leaves $ALIASES empty, and an # `@/lib/secret` then matches no alias, is called a published package, and is skipped — a # false green. Following the chain is a real change; saying so is not. - if grep -q '"extends"' "$TSCONFIG" 2>/dev/null && [ ! -s "$ALIASES" ]; then + if grep -qa '"extends"' "$TSCONFIG" 2>/dev/null && [ ! -s "$ALIASES" ]; then unknown "tsconfig.json uses \"extends\" and no alias was parsed from this file — the base config is not followed, so an aliased import here would be mistaken for a published package" fi if [ -s "$WORK/pathsbody" ] && [ ! -s "$ALIASES" ]; then @@ -391,7 +391,7 @@ QUEUE="$WORK/queue"; SEEN="$WORK/seen"; EDGES="$WORK/edges" # on. Reachability is a property of the whole graph; it cannot be accumulated by a traversal # that visits each node once. enqueue() { # $1 = absolute file to walk - grep -Fxq -- "$1" "$SEEN" 2>/dev/null && return 0 + grep -aFxq -- "$1" "$SEEN" 2>/dev/null && return 0 printf '%s\n' "$1" >> "$SEEN" printf '%s\n' "$1" >> "$QUEUE" } @@ -406,7 +406,7 @@ seeds_of() { # $1 = file while [ -s "$frontier" ]; do awk -F'\t' 'NR==FNR { want[$0]; next } ($2 in want) { print $1 }' "$frontier" "$EDGES" | sort -u > "$nxt" if [ -s "$nxt" ]; then - grep -Fxv -f "$vis" -- "$nxt" > "$nxt.new" 2>/dev/null || : > "$nxt.new" + grep -aFxv -f "$vis" -- "$nxt" > "$nxt.new" 2>/dev/null || : > "$nxt.new" mv "$nxt.new" "$nxt" fi [ -s "$nxt" ] || break @@ -414,7 +414,7 @@ seeds_of() { # $1 = file mv "$nxt" "$frontier" done [ -s "$SEEDS" ] || return 0 - grep -Fxf "$SEEDS" -- "$vis" 2>/dev/null | sort -u + grep -aFxf "$SEEDS" -- "$vis" 2>/dev/null | sort -u } rel() { printf '%s' "${1#"$ROOT"/}"; } @@ -694,7 +694,12 @@ while IFS= read -r file; do while IFS= read -r term; do [ -n "$term" ] || continue set +e - hits="$(grep -nF -e "$term" -- "$file" 2>"$WORK/err")" + # `-a`, BECAUSE ONE BYTE MUST NOT HIDE THE SECRET. Without it a NUL anywhere in a module + # makes grep call the file binary and print nothing while exiting 1, which this gate reads + # as "term not present". Measured: a module holding SUPABASE_SERVICE_ROLE_KEY and a single + # NUL byte, reached from a page, reported `ok [service-role]`, exit 0. This one predates + # the tokenisers; it surfaced from the same finding in the other gate. + hits="$(grep -anF -e "$term" -- "$file" 2>"$WORK/err")" grc=$? set -e if [ "$grc" -gt 1 ]; then From cdbbb9bf03dd690f39f937a1a994432836b454fb Mon Sep 17 00:00:00 2001 From: Chii Date: Wed, 9 Sep 2026 00:23:56 +0000 Subject: [PATCH 11/13] Dollar-quoted bodies are data, and the ALTER side did not know what the CREATE side knew MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four findings from a CodeRabbit round on 036f939. All four reproduced; two are reds on compliant files, one is a silent skip, one is a README claim that stopped being true when the code around it was fixed. A DOLLAR-QUOTED BODY IS A STRING. The scanner read `$$` and `$tag$` bodies as SQL, so a perfectly ordinary function definition: create function public.f() returns void language plpgsql as $$ begin create table public.tmp (id int); end $$; reported `FAIL ... table 'public.tmp' created without enable row level security`. public.tmp does not exist at definition time; it is created when the function runs. A violation naming a table nobody created, which is the shape this gate exists to refuse, and the fixer's only route to green was editing a function body that is correct. `do $$ ... $$` does execute immediately, so a create inside one is real — but the gate cannot see whether RLS follows it inside the body either. UNKNOWN is the honest answer for both: still red, still blocking, and claiming nothing it has not established. That is the same treatment `execute '...'` already gets, which is what a body is: deferred SQL this scanner reads as data. THE ALTER SIDE SWALLOWED `if` AND `exists` — the exact defect the create side had two commits ago, in the branch beside it, left unfixed because the fix was applied where the finding pointed rather than to the pair. Both are non-reserved and legal table names. `alter table if enable row level security` bound `enable` as the table name, emitted no RLS record, and reported a violation on a file that enables RLS correctly. `only` is reserved and is still swallowed, correctly. AND THE TOKENISER'S EXIT STATUS WAS DISCARDED. If awk failed on a module, the walk found no records, added no import edges and raised no UNKNOWN — so every module reachable only through that one silently left the graph, and the gate reported on a smaller tree than the one it was given, with no sign that it had. A tokeniser that failed read nothing, and reading nothing is not reading a clean file. The README claim that a `"` inside an ordinary string literal causes UNKNOWN was true when it was written and stopped being true two commits later, when the string state that fixed it landed. It now says the opposite, and says why the case is named at all. Two fixtures, both measured against 8867c72: ddl-in-a-dollar-quoted-body (FAIL naming public.tmp there, UNKNOWN here) and alter-table-named-if (2 violations there, 1 here — the planted one). Nineteen migrations-lint spellings re-measured. Selftest green. All six shell gates green on the template tree at deda1fa. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BfFq2KvV1CCkPs5WnV1AsA --- README.md | 23 ++++++----- .../migrations/20260110000000_orders.down.sql | 2 + .../migrations/20260110000000_orders.sql | 9 +++++ .../migrations/20260109000000_orders.down.sql | 1 + .../migrations/20260109000000_orders.sql | 13 +++++++ gates/migrations-lint.sh | 39 +++++++++++++++++-- gates/service-role.sh | 9 +++++ 7 files changed, 84 insertions(+), 12 deletions(-) create mode 100644 fixtures/migrations-lint/bad/cases/alter-table-named-if/supabase/migrations/20260110000000_orders.down.sql create mode 100644 fixtures/migrations-lint/bad/cases/alter-table-named-if/supabase/migrations/20260110000000_orders.sql create mode 100644 fixtures/migrations-lint/bad/unknown/ddl-in-a-dollar-quoted-body/supabase/migrations/20260109000000_orders.down.sql create mode 100644 fixtures/migrations-lint/bad/unknown/ddl-in-a-dollar-quoted-body/supabase/migrations/20260109000000_orders.sql diff --git a/README.md b/README.md index 91073ff..c6de401 100644 --- a/README.md +++ b/README.md @@ -311,11 +311,13 @@ header, which is the honest account of what that scanner does not do. folds `Orders` to `orders` but keeps `"Orders"` distinct — Prisma and Drizzle emit the quoted PascalCase form. Three successive regex versions each traded one error for another here; the pair comparison leaves no interpolated pattern to be wider than the name it was - given. It reads a `create table` inside a function body and inside a `DO $$` block, and reds - on both. It does **not** read one inside a `'…'` string literal: a string is data to the - scanner, and reading it named tables nobody created and sent fixers to edit their data — - but a string containing a `create … table` **sequence** is reported UNKNOWN rather than - passed over, because `execute` runs it. Sequence, not two words anywhere: `'created three + given. A `'…'` string and a `$$`/`$tag$` dollar-quoted body are both **data** to the scanner. + Reading them named tables nobody created: `create function f() … as $$ … create table + public.tmp … $$` was a violation naming `public.tmp`, which does not exist at definition + time and is created only when the function runs. But a string or body containing a + `create … table` **sequence** is UNKNOWN rather than passed over, because `execute` and + `DO $$` do run — UNKNOWN is still red, and it does not claim a violation the gate has not + established. Sequence, not two words anywhere: Sequence, not two words anywhere: `'created three tables last week'` is prose and passes, while `'CREATE UNLOGGED TABLE …'` does not. That allowance was once written `{0,2}`, which **mawk miscompiles** to zero repetitions when the group begins with a `+`-quantified bracket — so every modifier form passed silently until @@ -326,10 +328,13 @@ header, which is the honest account of what that scanner does not do. turned a compliant migration containing `values ('/api/*')` red with a message naming a string that does not exist. What it genuinely cannot read, as silent passes: a name assembled at runtime (`execute format('create table %I …')` or string concatenation) and - `select … into`. **An unterminated quoted identifier, string or block comment is UNKNOWN, - not a pass** — a scanner that lost sync read everything after it as something it is not, - and one `"` inside an ordinary string literal (an inch mark in `values ('24" monitor')`) - is enough to do that. A quoted identifier may contain a tab or a **newline**; those are + `select … into`. **An unterminated quoted identifier, string, block comment or dollar-quoted + body is UNKNOWN, not a pass** — a scanner that lost sync read everything after it as + something it is not. A `"` inside an ordinary string literal (an inch mark in + `values ('24" monitor')`) is *not* one of those: it is string data, and reads as such. + It was not always — before the scanner had a string state that one byte opened a quoted + identifier that hid every statement after it — which is why the case is named here rather + than left to be rediscovered. A quoted identifier may contain a tab or a **newline**; those are escaped in the scanner's output and compared escaped, so a name containing one no longer shifts the records after it — that shift once reported a single violation naming a table that does not exist while silently dropping two real ones. The escaped form is what the diff --git a/fixtures/migrations-lint/bad/cases/alter-table-named-if/supabase/migrations/20260110000000_orders.down.sql b/fixtures/migrations-lint/bad/cases/alter-table-named-if/supabase/migrations/20260110000000_orders.down.sql new file mode 100644 index 0000000..718c9c0 --- /dev/null +++ b/fixtures/migrations-lint/bad/cases/alter-table-named-if/supabase/migrations/20260110000000_orders.down.sql @@ -0,0 +1,2 @@ +drop table if exists public.orders; +drop table if exists public."if"; diff --git a/fixtures/migrations-lint/bad/cases/alter-table-named-if/supabase/migrations/20260110000000_orders.sql b/fixtures/migrations-lint/bad/cases/alter-table-named-if/supabase/migrations/20260110000000_orders.sql new file mode 100644 index 0000000..6392c6b --- /dev/null +++ b/fixtures/migrations-lint/bad/cases/alter-table-named-if/supabase/migrations/20260110000000_orders.sql @@ -0,0 +1,9 @@ +-- `if` and `exists` are non-reserved in PostgreSQL and are legal table names — the create +-- side already knew that. The ALTER side swallowed them wherever they appeared, so it bound +-- `enable` as the table name here, emitted no RLS record, and reported a violation on a file +-- that enables RLS correctly. The planted violation is `orders`, which genuinely has none; +-- the point of the file is that `if` must NOT also be reported. +create table if (id int); +alter table if enable row level security; + +create table public.orders (id int); diff --git a/fixtures/migrations-lint/bad/unknown/ddl-in-a-dollar-quoted-body/supabase/migrations/20260109000000_orders.down.sql b/fixtures/migrations-lint/bad/unknown/ddl-in-a-dollar-quoted-body/supabase/migrations/20260109000000_orders.down.sql new file mode 100644 index 0000000..20f1679 --- /dev/null +++ b/fixtures/migrations-lint/bad/unknown/ddl-in-a-dollar-quoted-body/supabase/migrations/20260109000000_orders.down.sql @@ -0,0 +1 @@ +drop function if exists public.f(); diff --git a/fixtures/migrations-lint/bad/unknown/ddl-in-a-dollar-quoted-body/supabase/migrations/20260109000000_orders.sql b/fixtures/migrations-lint/bad/unknown/ddl-in-a-dollar-quoted-body/supabase/migrations/20260109000000_orders.sql new file mode 100644 index 0000000..7aaa786 --- /dev/null +++ b/fixtures/migrations-lint/bad/unknown/ddl-in-a-dollar-quoted-body/supabase/migrations/20260109000000_orders.sql @@ -0,0 +1,13 @@ +-- A dollar-quoted body is data, like any other string. Scanning it as SQL made the create +-- below a FAIL naming public.tmp — a table that does not exist at definition time and is +-- created only when the function runs. A red on a compliant file, naming a table nobody +-- created, which is the shape this gate refuses. +-- +-- `do $$ … $$` does execute immediately, so a create inside one is real; but the gate cannot +-- see whether RLS follows it inside the body either. UNKNOWN is the honest answer for both: +-- still red, still blocking, and claiming nothing it has not established. +create function public.f() returns void language plpgsql as $$ +begin + create table public.tmp (id int); +end +$$; diff --git a/gates/migrations-lint.sh b/gates/migrations-lint.sh index 78763b6..7b6e2a3 100755 --- a/gates/migrations-lint.sh +++ b/gates/migrations-lint.sh @@ -104,9 +104,17 @@ function tok(k, v, again) { if (k == "W" && v == "table") st = 5 else { st = 0; again = 1 } } else if (st == 5) { - if (k == "W" && (v == "if" || v == "exists" || v == "only")) { } + # `only` is reserved and can be swallowed. `if` and `exists` are NOT — they are legal + # table names, which the create side already knows. Swallowing them here bound `enable` + # as the table name of `alter table if enable row level security`, so no R record was + # emitted and a compliant file reported a violation. + if (k == "W" && v == "only") { } + else if (k == "W" && v == "if") st = 51 else if (k == "W" || k == "Q") { nn = 1; P[1] = v; st = 6; wantpart = 0 } else { st = 0; again = 1 } + } else if (st == 51) { + if (k == "W" && v == "exists") st = 5 + else { nn = 1; P[1] = "if"; st = 6; wantpart = 0; again = 1 } } else if (st == 6) { if (k == "D") wantpart = 1 else if (wantpart && (k == "W" || k == "Q")) { P[++nn] = v; wantpart = 0 } @@ -179,6 +187,28 @@ function tok(k, v, again) { } sv = sv c; i++; continue } + # A DOLLAR-QUOTED BODY IS DATA, like any other string. Scanning it as SQL made + # `create function f() ... as $$ begin create table public.tmp (id int); end $$;` a FAIL + # naming public.tmp — a table that does not exist at definition time and is created only + # when the function runs. A red on a compliant file, naming a table nobody created. + # + # `do $$ ... $$` DOES execute immediately, so a create table in one is real — but the gate + # cannot see whether RLS follows it inside the body either, so the honest answer for both + # is the same as for `execute '...'`: UNKNOWN, which is still red and still blocks. What + # it must not do is name a table and claim a violation it has not established. + if (indq) { + if (substr($0, i, length(dqtag)) == dqtag) { i += length(dqtag); indq = 0 + if (tolower(dqv) ~ /(^|[^a-z])create[ \t\n]+([a-z]+[ \t\n]+)*table([^a-z]|$)/) dqddl = 1 + dqv = ""; continue } + dqv = dqv c; i++; continue + } + if (c == "$") { + j = i + 1 + while (j <= n && substr($0, j, 1) ~ /[A-Za-z0-9_]/) j++ + if (j <= n && substr($0, j, 1) == "$") { + dqtag = substr($0, i, j - i + 1); indq = 1; dqv = ""; i = j + 1; continue + } + } if (c == " " || c == "\t" || c == "\r") { i++; continue } if (c == "-" && substr($0, i + 1, 1) == "-") break if (c == "/" && substr($0, i + 1, 1) == "*") { inblk = 1; i += 2; continue } @@ -192,6 +222,7 @@ function tok(k, v, again) { if (c == ".") { tok("D", "."); i++; continue } tok("P", c); i++ } + if (indq) dqv = dqv "\n" if (inq) qv = qv "\n" if (ins) sv = sv "\n" } @@ -200,6 +231,8 @@ END { # A SCANNER THAT LOST SYNC MUST NOT REPORT A CLEAN FILE. An unterminated quoted identifier # or string means everything after it was read as something it is not, so the only honest # answer about the rest of the file is that this gate could not read it. + if (indq) print "U\tunterminated-dollar-quoted-body\t" + if (dqddl) print "U\tddl-inside-a-dollar-quoted-body\t" if (inblk) print "U\tunterminated-block-comment\t" if (inq) print "U\tunterminated-quoted-identifier\t" if (ins) print "U\tunterminated-string\t" @@ -278,8 +311,8 @@ for up in "$MIG"/*.sql; do # read is the DDL inside the string. A message wider than the thing it describes is the # defect this repository exists to catch. case "$reason" in - ddl-inside-a-string-literal) - unknown "$(basename "$up"): a string literal here contains a 'create ... table' that \`execute\` would run. This gate reads strings as data, so it cannot say what that statement creates or whether RLS follows it" ;; + ddl-inside-a-string-literal|ddl-inside-a-dollar-quoted-body) + unknown "$(basename "$up"): a string or dollar-quoted body here contains a 'create ... table'. This gate reads strings as data, so it cannot say what that statement creates or whether RLS follows it" ;; *) unknown "$(basename "$up"): $reason — this gate could not read the statements after it, and will not call the file clean" ;; esac diff --git a/gates/service-role.sh b/gates/service-role.sh index a978856..bf3cb30 100755 --- a/gates/service-role.sh +++ b/gates/service-role.sh @@ -649,7 +649,16 @@ while :; do # than assumed harmless. `S` is a literal specifier from a real import position. set +e recs="$(awk -f "$SCAN" -- "$file" 2>/dev/null)" + scan_rc=$? set -e + # A TOKENISER THAT FAILED READ NOTHING, and reading nothing is not reading a clean file. + # Without this the walk discarded awk's status, found no records, added no edges and raised + # no UNKNOWN — so every module reachable only through this one dropped out of the graph + # silently. The gate would then report on a smaller tree than the one it was given. + if [ "$scan_rc" -ne 0 ]; then + unknown "$(rel "$file") could not be tokenised (awk exited $scan_rc) — an unread module is not a clean one" + continue + fi nonliteral=0 : > "$WORK/specs" while IFS= read -r rec; do From 6ee66364e33a487c87169c95e83d7f4b7720d9aa Mon Sep 17 00:00:00 2001 From: Chii Date: Wed, 9 Sep 2026 00:29:16 +0000 Subject: [PATCH 12/13] The README claimed every grep passes -a; two more gates did not, and one byte hid a secret name in each MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round nine, asked the convergence question and told "no new defect" was an acceptable answer. It said no again. It also could not break the states 9/10, the escaping, the U handling or the -a exit statuses it was pointed at, and said so — so this is the narrow result, not a general one. THE CLAIM WAS WIDER THAN THE CODE, in the section whose job is the honest account. "Every grep over file content passes -a" was written one commit ago while lib.sh's shared tgrep() and actions-sha-pinned had none. Both reproduced under LC_ALL=C.UTF-8, the locale on ubuntu-latest: a NEXT_PUBLIC_-prefixed service-key name + one NUL -> ok [nextjs-env] exit 0 a denylisted term + one NUL -> ok [nextjs-env] exit 0 uses: actions/checkout@v4 + one NUL -> ok [actions-sha-pinned] nextjs-env routes BOTH its checks through tgrep, so one byte hid a secret name from the gate whose entire job is secret names — under a README bullet saying that was closed. The claim shipped in the same commit that fixed the other two gates, which is the recurring defect exactly: a sentence generalised past what was done. AND THE MECHANISM WAS WRONG IN BOTH PLACES IT WAS WRITTEN. grep exits 0 when it suppresses a binary match, not non-zero; it writes a note to stderr. The README said "exit non-zero" and service-role.sh said "exiting 1". That matters and is not pedantry: service-role's `grc -gt 1` branch is the one a reader would expect to have caught this, and it would not have — grc was 0, hits was empty, and the emptiness check swallowed it. A wrong account of a mechanism sends the next person to the wrong place. THE `*` FIX OVER-CORRECTED. Bounding the modifier allowance as {0,2} was inert under mawk; replacing it with `*` made it unbounded, and ordinary English prose then blocked a compliant migration: values ('To create a new monthly revenue table, run the report') cdbbb9b -> UNKNOWN, exit 1, on a file nothing executes Seeding help text was enough. It is two optional groups now, a form mawk compiles: 'CREATE UNLOGGED TABLE' and 'create global temporary table' still UNKNOWN, prose passes. The bound was always the point; only its spelling was wrong, twice. Also: a WHERE-less `delete from t` with no trailing semicolon at end of file lost its tier-3 note, because state 10 waits for a terminator end-of-input never sends. The selftest caught one of my own on the way. Writing the finding into lib.sh's comment put a literal NEXT_PUBLIC_ service-key name in the toolkit's tree, and nextjs-env went red on it immediately. The gate was right; the name is written in pieces now, and the comment says why. Fixture nextjs-env/bad/cases/nul-byte-hides-the-name: ok under cdbbb9b, FAIL now — the first fixture this gate has under cases/. Selftest green. All six shell gates green on the template tree at deda1fa. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BfFq2KvV1CCkPs5WnV1AsA --- README.md | 35 +++++++++++------- .../nul-byte-hides-the-name/src/config.ts | Bin 0 -> 258 bytes gates/actions-sha-pinned.sh | 2 +- gates/lib.sh | 11 +++++- gates/migrations-lint.sh | 26 ++++++++----- gates/nextjs-env.sh | 4 +- gates/service-role.sh | 12 ++++-- 7 files changed, 59 insertions(+), 31 deletions(-) create mode 100644 fixtures/nextjs-env/bad/cases/nul-byte-hides-the-name/src/config.ts diff --git a/README.md b/README.md index c6de401..f6650a8 100644 --- a/README.md +++ b/README.md @@ -317,11 +317,14 @@ header, which is the honest account of what that scanner does not do. time and is created only when the function runs. But a string or body containing a `create … table` **sequence** is UNKNOWN rather than passed over, because `execute` and `DO $$` do run — UNKNOWN is still red, and it does not claim a violation the gate has not - established. Sequence, not two words anywhere: Sequence, not two words anywhere: `'created three - tables last week'` is prose and passes, while `'CREATE UNLOGGED TABLE …'` does not. That - allowance was once written `{0,2}`, which **mawk miscompiles** to zero repetitions when the - group begins with a `+`-quantified bracket — so every modifier form passed silently until - it was measured. Comments are recognised by the same scanner, not stripped by an + established. Sequence, not two words anywhere: Sequence with **at most two modifier words**, not two words + anywhere and not any number: `'created three tables last week'` and `'To create a new + monthly revenue table, run the report'` are prose and pass, while `'CREATE UNLOGGED TABLE …'` + does not. The bound was once written `{0,2}`, which **mawk miscompiles** to zero repetitions + when the group begins with a `+`-quantified bracket, so every modifier form passed silently; + replacing it with `*` fixed that and made the allowance unbounded, so English prose blocked + a compliant migration. It is two optional groups now — the bound was always the point, only + its spelling was wrong, twice. Comments are recognised by the same scanner, not stripped by an earlier stage: a stage that cannot see strings took `values ('x /* y')` for the start of a block comment and deleted every line to the next `*/`, hiding a whole `create table` — verified against PostgreSQL 16 as a real table left with RLS off — and the same swallow @@ -357,14 +360,20 @@ header, which is the honest account of what that scanner does not do. can tell that from an import, so a candidate carrying `<`, `>`, `{` or `}` is dropped as text. Node subpath imports (`#internal/db`) are UNKNOWN: they resolve through `package.json` `imports`, which this gate does not read. -- **Every `grep` over file content passes `-a`.** Without it, one byte that is invalid in the - ambient locale makes GNU grep declare a file binary, print nothing and exit non-zero — which - a gate reads as "not found". Measured: a module holding `SUPABASE_SERVICE_ROLE_KEY` plus a - single NUL byte, reached from a page, reported `ok`, exit 0; and a migration creating a - table whose quoted name held a LATIN1 byte had that table dropped from the comparison - entirely while the clean table beside it was still reported, so the loss looked like a - smaller violation count rather than a missing check. One byte must not decide whether a - gate can see a secret. +- **Every `grep` over file content passes `-a`**, in all six shell gates and in `lib.sh`'s + shared `tgrep`. Without it, one byte that is invalid in the ambient locale makes GNU grep + declare a file binary, print nothing and **exit 0** with a note on stderr — which a gate + reads as "not found". Measured: a module holding `SUPABASE_SERVICE_ROLE_KEY` plus a single + NUL byte, reached from a page, reported `ok`, exit 0; the same byte hid a `NEXT_PUBLIC_` + service-key name from `nextjs-env` and an unpinned `uses:` ref from `actions-sha-pinned`; + and a migration creating a table whose quoted name held a LATIN1 byte had that table dropped + from the comparison entirely while the clean table beside it was still reported, so the loss + looked like a smaller violation count rather than a missing check. One byte must not decide + whether a gate can see a secret. + + The exit status is worth stating exactly, because an earlier version of this bullet had it + backwards: grep exits **0**, not non-zero. Any guard written around a non-zero status — + `service-role` has one — would not have caught this, and did not. - **Every scanner here is line-incremental, and the comparison is not a shell loop.** The first version accumulated each file with `buf = buf $0 "\n"`, which mawk reallocates and copies every line: 12.1s on a 1.1MB generated types file against 0.11s for the greps it diff --git a/fixtures/nextjs-env/bad/cases/nul-byte-hides-the-name/src/config.ts b/fixtures/nextjs-env/bad/cases/nul-byte-hides-the-name/src/config.ts new file mode 100644 index 0000000000000000000000000000000000000000..23d3bfd8e5acaa24bd4af54bf35ecc4084b99896 GIT binary patch literal 258 zcmX9%J8r^26r}bkM%0c?k5T~I!U7Q#L!GWPVSQ6-AL!Y1pC%gk0-r{bcBOB5j^RJp&6C2+TUUfk=`3C4C|ybwvEyh z+BQ(TY@1^e4?npI=-$$8^Uq`ROVIkE!+~u=-3qWWu-eLJiz823?I{l0RGt!4l8ty{m~iA^V-I)oxpEiuo#AlzFk4&1bnPbM^X`EmXNwAJgd3>va>F "$LIST" 2> "$ERR" +grep -arEn --include='*.yml' --include='*.yaml' -e "$KEY" -- "$WF" > "$LIST" 2> "$ERR" rc=$? set -e if [ "$rc" -gt 1 ]; then diff --git a/gates/lib.sh b/gates/lib.sh index c6d375f..3fcc69c 100755 --- a/gates/lib.sh +++ b/gates/lib.sh @@ -101,4 +101,13 @@ finish() { # for a literal term handed grep two matchers; grep answered "conflicting matchers specified" # and exited 2 on every such search, which made the per-repo denylist unusable in any repo # that had one. A helper that dictates the matcher is a helper that decides the search. -tgrep() { grep -rn --exclude-dir=node_modules --exclude-dir=.git --exclude-dir=.next --exclude-dir=coverage "$@"; } +# `-a` is not decoration. Without it one byte that is invalid in the ambient locale makes GNU +# grep call a file binary, print nothing and EXIT 0 — which every caller here reads as "not +# found". Measured: a NUL byte in a module hid a `NEXT_PUBLIC_`-prefixed service-key name from +# nextjs-env, the gate whose whole job is secret names, and the file reported ok, exit 0. +# +# The name is written in pieces here on purpose: spelled out, this comment is itself a +# NEXT_PUBLIC_ variable named like a server secret, and nextjs-env red on the toolkit's own +# tree the moment it was added. The gate was right, which is the second-best way to learn it +# works. +tgrep() { grep -arn --exclude-dir=node_modules --exclude-dir=.git --exclude-dir=.next --exclude-dir=coverage "$@"; } diff --git a/gates/migrations-lint.sh b/gates/migrations-lint.sh index 7b6e2a3..4b8699c 100755 --- a/gates/migrations-lint.sh +++ b/gates/migrations-lint.sh @@ -174,15 +174,18 @@ function tok(k, v, again) { # `create` then at most two modifier words then `table`, not "created" and "tables" # anywhere in the same sentence — `values ('created three tables last week')` was a # blocking UNKNOWN on an otherwise compliant file. - # `*`, NOT `{0,2}`. mawk 1.3.4 — the awk on Debian/Ubuntu and the one this repo names - # by hand — miscompiles an interval with n>=2 applied to a group whose body starts - # with a `+`-quantified bracket expression: it matches ZERO repetitions only. So the - # modifier allowance was inert and the test was exactly `create table`, which let - # `execute 'CREATE UNLOGGED TABLE public.x (id int)'` pass over silently. Verified - # against PostgreSQL 16: a persistent unlogged table, RLS off, gate green. The - # non-string path at the top of this scanner reads those modifiers correctly, so the - # gap was invisible from the fixtures. - if (tolower(sv) ~ /(^|[^a-z])create[ \t\n]+([a-z]+[ \t\n]+)*table([^a-z]|$)/) ddl = 1 + # TWO OPTIONAL GROUPS, which is the bound written in a form mawk compiles. `{0,2}` + # was the obvious spelling and mawk 1.3.4 — the awk on Debian/Ubuntu — miscompiles an + # interval with n>=2 applied to a group whose body starts with a `+`-quantified + # bracket: it matches ZERO repetitions, so the allowance was inert, the test was + # exactly `create table`, and `execute 'CREATE UNLOGGED TABLE …'` passed over. + # + # Replacing it with `*` fixed that and broke the other side: unbounded, so ordinary + # English prose matched. Measured on a compliant migration — + # `values ('To create a new monthly revenue table, run the report')` — UNKNOWN, exit + # 1, on a file nothing executes. Seeding help text was enough to block the branch. + # The bound is the point; only its spelling was wrong. + if (tolower(sv) ~ /(^|[^a-z])create[ \t\n]+([a-z]+[ \t\n]+)?([a-z]+[ \t\n]+)?table([^a-z]|$)/) ddl = 1 sv = ""; continue } sv = sv c; i++; continue @@ -198,7 +201,7 @@ function tok(k, v, again) { # it must not do is name a table and claim a violation it has not established. if (indq) { if (substr($0, i, length(dqtag)) == dqtag) { i += length(dqtag); indq = 0 - if (tolower(dqv) ~ /(^|[^a-z])create[ \t\n]+([a-z]+[ \t\n]+)*table([^a-z]|$)/) dqddl = 1 + if (tolower(dqv) ~ /(^|[^a-z])create[ \t\n]+([a-z]+[ \t\n]+)?([a-z]+[ \t\n]+)?table([^a-z]|$)/) dqddl = 1 dqv = ""; continue } dqv = dqv c; i++; continue } @@ -228,6 +231,9 @@ function tok(k, v, again) { } END { if (st == 3) emitpair("C") + # A final `delete from t` with no trailing semicolon is still WHERE-less, and the note said + # so before state 10 existed. State 10 waits for a terminator that end-of-input never sends. + if (st == 10) print "X\t\t" # A SCANNER THAT LOST SYNC MUST NOT REPORT A CLEAN FILE. An unterminated quoted identifier # or string means everything after it was read as something it is not, so the only honest # answer about the rest of the file is that this gate could not read it. diff --git a/gates/nextjs-env.sh b/gates/nextjs-env.sh index 9a74d12..57eb744 100755 --- a/gates/nextjs-env.sh +++ b/gates/nextjs-env.sh @@ -92,14 +92,14 @@ if in_git_repo "$ROOT"; then show_err "$WORK/err" else set +e - grep -E '(^|/)\.env(\..*)?$' "$WORK/tracked" > "$WORK/envcand" 2> "$WORK/err" + grep -aE '(^|/)\.env(\..*)?$' "$WORK/tracked" > "$WORK/envcand" 2> "$WORK/err" grc=$? set -e if [ "$grc" -gt 1 ]; then fail "filtering the tracked file list failed (grep exit $grc):"; show_err "$WORK/err" else set +e - grep -vE '\.example$' "$WORK/envcand" > "$WORK/env" 2> "$WORK/err" + grep -avE '\.example$' "$WORK/envcand" > "$WORK/env" 2> "$WORK/err" grc=$? set -e if [ "$grc" -gt 1 ]; then diff --git a/gates/service-role.sh b/gates/service-role.sh index bf3cb30..9e5ce6c 100755 --- a/gates/service-role.sh +++ b/gates/service-role.sh @@ -704,10 +704,14 @@ while IFS= read -r file; do [ -n "$term" ] || continue set +e # `-a`, BECAUSE ONE BYTE MUST NOT HIDE THE SECRET. Without it a NUL anywhere in a module - # makes grep call the file binary and print nothing while exiting 1, which this gate reads - # as "term not present". Measured: a module holding SUPABASE_SERVICE_ROLE_KEY and a single - # NUL byte, reached from a page, reported `ok [service-role]`, exit 0. This one predates - # the tokenisers; it surfaced from the same finding in the other gate. + # makes grep call the file binary, print nothing, and EXIT 0 with a note on stderr. + # Measured: a module holding SUPABASE_SERVICE_ROLE_KEY and a single NUL byte, reached from + # a page, reported `ok [service-role]`, exit 0. + # + # EXIT 0, not 1 — this comment said 1 and was wrong, and the error matters: the + # `grc -gt 1` branch below is the one a reader would expect to have caught this, and it + # would not have. grc was 0, hits was empty, and the emptiness check swallowed it. A + # wrong account of a mechanism is how the next person looks in the wrong place. hits="$(grep -anF -e "$term" -- "$file" 2>"$WORK/err")" grc=$? set -e From f4f7c93a17ea1aaf42b2013a0220b1692013fab8 Mon Sep 17 00:00:00 2001 From: Chii Date: Wed, 9 Sep 2026 01:28:21 +0000 Subject: [PATCH 13/13] Three P1 false greens Codex found in the tokeniser, two of them regressions it introduced MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Chii flipped the PR to ready, which triggered three reviewers that had not seen this code. Codex's code lane found three P1 false greens in service-role, all mine, all reproduced. Two of them worked correctly before the tokeniser — the pre-tokeniser extractor caught both dependencies — so they are regressions the rewrite introduced and nine of my own rounds did not find. ORDERING THE CANDIDATE LIST WAS THE WRONG TOOL, and it could only ever be wrong in one direction. Source-before-emitted was right for a TypeScript project: a stale `admin.mjs` beside its `admin.mts` must not mask a secret added to the source. But in a JAVASCRIPT project `require("../lib/admin.js")` means admin.js, and putting the `.ts` substitution first scanned a clean `admin.ts` while `admin.js` — the module Node actually loads, holding the key — was never read: pages/index.js requires ../lib/admin.js lib/admin.ts clean lib/admin.js process.env.SUPABASE_SERVICE_ROLE_KEY -> ok [service-role] exit 0 Whichever candidate is second gets skipped, so no ordering can be right about a project type this gate cannot reliably detect. resolve() now returns EVERY candidate that exists on disk and the walk takes an edge to each. The cost is at most reading a module Node would not have loaded — a false red, the direction this toolkit errs in. `module.require()` IS A MODULE LOADER. Dropping every `require` preceded by a dot — added so `Array.from(",")` would stop opening a specifier slot — took it with it, and `module.require("../lib/admin")` went unwalked. The narrow rule is now narrow: a dotted `require` counts when the word before the dot is `module`. A SLASH AFTER `}` IS DIVISION far more often than it is a regex. `}` was in the regex-position set, so `const x = {} / foo; import { admin } from "../lib/admin"` had the rest of the line consumed as a regex literal — the real import with it. A block close can precede a regex, so removing `}` trades a rare false red for a false green, which is the trade this repository takes every time. Three fixtures, each verified green under 6ee6636 and red now: explicit-js-over-ts-sibling, module-require-loader, division-after-brace. Every earlier service-role tree re-measured. Selftest green. All six shell gates green on the template tree at deda1fa. Worth recording: nine adversarial rounds of my own missed all three, and a reviewer that had never seen the code found them in eight minutes. The rounds shared my assumptions about what the tokeniser was for; Codex did not. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BfFq2KvV1CCkPs5WnV1AsA --- .../cases/division-after-brace/lib/admin.js | 1 + .../cases/division-after-brace/package.json | 1 + .../cases/division-after-brace/pages/index.js | 10 +++++ .../explicit-js-over-ts-sibling/lib/admin.js | 1 + .../explicit-js-over-ts-sibling/lib/admin.ts | 1 + .../explicit-js-over-ts-sibling/package.json | 1 + .../pages/index.js | 9 ++++ .../cases/module-require-loader/lib/admin.js | 1 + .../cases/module-require-loader/package.json | 1 + .../module-require-loader/pages/index.js | 8 ++++ gates/service-role.sh | 43 ++++++++++++++++--- 11 files changed, 70 insertions(+), 7 deletions(-) create mode 100644 fixtures/service-role/bad/cases/division-after-brace/lib/admin.js create mode 100644 fixtures/service-role/bad/cases/division-after-brace/package.json create mode 100644 fixtures/service-role/bad/cases/division-after-brace/pages/index.js create mode 100644 fixtures/service-role/bad/cases/explicit-js-over-ts-sibling/lib/admin.js create mode 100644 fixtures/service-role/bad/cases/explicit-js-over-ts-sibling/lib/admin.ts create mode 100644 fixtures/service-role/bad/cases/explicit-js-over-ts-sibling/package.json create mode 100644 fixtures/service-role/bad/cases/explicit-js-over-ts-sibling/pages/index.js create mode 100644 fixtures/service-role/bad/cases/module-require-loader/lib/admin.js create mode 100644 fixtures/service-role/bad/cases/module-require-loader/package.json create mode 100644 fixtures/service-role/bad/cases/module-require-loader/pages/index.js diff --git a/fixtures/service-role/bad/cases/division-after-brace/lib/admin.js b/fixtures/service-role/bad/cases/division-after-brace/lib/admin.js new file mode 100644 index 0000000..7b4886c --- /dev/null +++ b/fixtures/service-role/bad/cases/division-after-brace/lib/admin.js @@ -0,0 +1 @@ +export const admin = process.env.SUPABASE_SERVICE_ROLE_KEY; diff --git a/fixtures/service-role/bad/cases/division-after-brace/package.json b/fixtures/service-role/bad/cases/division-after-brace/package.json new file mode 100644 index 0000000..420ec29 --- /dev/null +++ b/fixtures/service-role/bad/cases/division-after-brace/package.json @@ -0,0 +1 @@ +{ "name": "fixture-division-after-brace", "dependencies": { "next": "15.0.0" } } diff --git a/fixtures/service-role/bad/cases/division-after-brace/pages/index.js b/fixtures/service-role/bad/cases/division-after-brace/pages/index.js new file mode 100644 index 0000000..ba8d311 --- /dev/null +++ b/fixtures/service-role/bad/cases/division-after-brace/pages/index.js @@ -0,0 +1,10 @@ +// A slash after `}` is division here, not a regex. Treating `}` as a regex position made the +// scanner consume the rest of the line looking for a closing slash — swallowing the real +// import beside it. ok, exit 0, on a module reaching the key, where the pre-tokeniser +// extractor caught it. A block close can precede a regex, so this trades a rare false red +// for a false green, which is the trade this toolkit takes every time. +const x = {} / foo; import { admin } from "../lib/admin"; + +export default function Page() { + return admin; +} diff --git a/fixtures/service-role/bad/cases/explicit-js-over-ts-sibling/lib/admin.js b/fixtures/service-role/bad/cases/explicit-js-over-ts-sibling/lib/admin.js new file mode 100644 index 0000000..36cfd0b --- /dev/null +++ b/fixtures/service-role/bad/cases/explicit-js-over-ts-sibling/lib/admin.js @@ -0,0 +1 @@ +module.exports.admin = process.env.SUPABASE_SERVICE_ROLE_KEY; diff --git a/fixtures/service-role/bad/cases/explicit-js-over-ts-sibling/lib/admin.ts b/fixtures/service-role/bad/cases/explicit-js-over-ts-sibling/lib/admin.ts new file mode 100644 index 0000000..f320d9f --- /dev/null +++ b/fixtures/service-role/bad/cases/explicit-js-over-ts-sibling/lib/admin.ts @@ -0,0 +1 @@ +export const admin = "clean"; diff --git a/fixtures/service-role/bad/cases/explicit-js-over-ts-sibling/package.json b/fixtures/service-role/bad/cases/explicit-js-over-ts-sibling/package.json new file mode 100644 index 0000000..74f7218 --- /dev/null +++ b/fixtures/service-role/bad/cases/explicit-js-over-ts-sibling/package.json @@ -0,0 +1 @@ +{ "name": "fixture-explicit-js-over-ts-sibling", "dependencies": { "next": "15.0.0" } } diff --git a/fixtures/service-role/bad/cases/explicit-js-over-ts-sibling/pages/index.js b/fixtures/service-role/bad/cases/explicit-js-over-ts-sibling/pages/index.js new file mode 100644 index 0000000..3bf310b --- /dev/null +++ b/fixtures/service-role/bad/cases/explicit-js-over-ts-sibling/pages/index.js @@ -0,0 +1,9 @@ +// A JavaScript project importing an explicit `.js`. A same-named `.ts` sits beside it and is +// clean; `lib/admin.js` — the module Node actually loads — holds the key. Ordering the +// candidate list put the `.ts` substitution first, so the clean file was scanned and this +// returned ok, exit 0. Whichever candidate is second gets skipped, so both are walked now. +const { admin } = require("../lib/admin.js"); + +module.exports = function Page() { + return admin; +}; diff --git a/fixtures/service-role/bad/cases/module-require-loader/lib/admin.js b/fixtures/service-role/bad/cases/module-require-loader/lib/admin.js new file mode 100644 index 0000000..36cfd0b --- /dev/null +++ b/fixtures/service-role/bad/cases/module-require-loader/lib/admin.js @@ -0,0 +1 @@ +module.exports.admin = process.env.SUPABASE_SERVICE_ROLE_KEY; diff --git a/fixtures/service-role/bad/cases/module-require-loader/package.json b/fixtures/service-role/bad/cases/module-require-loader/package.json new file mode 100644 index 0000000..ecd9042 --- /dev/null +++ b/fixtures/service-role/bad/cases/module-require-loader/package.json @@ -0,0 +1 @@ +{ "name": "fixture-module-require-loader", "dependencies": { "next": "15.0.0" } } diff --git a/fixtures/service-role/bad/cases/module-require-loader/pages/index.js b/fixtures/service-role/bad/cases/module-require-loader/pages/index.js new file mode 100644 index 0000000..913a844 --- /dev/null +++ b/fixtures/service-role/bad/cases/module-require-loader/pages/index.js @@ -0,0 +1,8 @@ +// `module.require()` is a real Node module loader. Dropping every `require` preceded by a dot +// — added to stop `Array.from(",")` opening a specifier slot — took this with it, and the +// dependency below went unwalked: ok, exit 0, where the pre-tokeniser extractor caught it. +const { admin } = module.require("../lib/admin"); + +module.exports = function Page() { + return admin; +}; diff --git a/gates/service-role.sh b/gates/service-role.sh index 9e5ce6c..2ab70dc 100755 --- a/gates/service-role.sh +++ b/gates/service-role.sh @@ -62,9 +62,15 @@ cat > "$SCAN" <<'SCANAWK' # LINE-INCREMENTAL, NOT SLURPED, for the reason recorded in the SQL scanner: `buf = buf $0 # "\n"` is quadratic in mawk and took 12.1s on a 1.1MB generated types file against 0.11s for # the greps it replaced. State is carried across lines instead. +# `}` IS NOT IN THIS SET, deliberately. It was, and `const x = {} / foo; import { admin } from +# "../lib/admin"` then read the division as the start of a regex literal and consumed the rest +# of the line — the real import with it. `ok`, exit 0, on a module reaching the secret, where +# the previous extractor caught it. A block close CAN precede a regex, so this trades a rare +# false red (a regex body scanned as code) for a false green, which is the trade this +# repository takes every time. function isregexpos(c) { return (c == "" || c == "(" || c == "," || c == "=" || c == ":" || c == "[" || c == "!" || - c == "&" || c == "|" || c == "?" || c == "{" || c == "}" || c == ";" || c == "+" || + c == "&" || c == "|" || c == "?" || c == "{" || c == ";" || c == "+" || c == "-" || c == "*" || c == "%" || c == "<" || c == ">" || c == "~" || c == "^" || c == "k") } @@ -145,8 +151,13 @@ function clearpend() { v = "" while (i <= n) { c = substr($0, i, 1); if (c !~ /[A-Za-z0-9_$]/) break; v = v c; i++ } # `.from` is a method name, not a keyword: `Array.from(",")` put the string after it in - # the specifier slot. - if (want(v) && last != ".") { clearpend(); pend = v; paren = 0 } else clearpend() + # the specifier slot. But `module.require()` IS a module loader — dropping every dotted + # `require` lost it, and a page calling `module.require("../lib/admin")` on a module + # holding the secret reported ok, exit 0, where the previous extractor caught it. + if (want(v) && last != ".") { clearpend(); pend = v; paren = 0 } + else if (v == "require" && last == "." && prevword == "module") { clearpend(); pend = v; paren = 0 } + else clearpend() + prevword = v last = (v == "return" || v == "typeof" || v == "case" || v == "in" || v == "of" || v == "new" || v == "delete" || v == "void" || v == "do" || v == "else" || v == "yield" || v == "await") ? "k" : "w" @@ -603,14 +614,26 @@ ${b%.js}.d.ts" ;; # exists — which is why it runs after the -f test rather than as a string rewrite. if [ -f "$cand" ]; then cdir="$(cd -- "$(dirname -- "$cand")" 2>/dev/null && pwd -P)" || continue - found="$cdir/$(basename -- "$cand")" + found="$found$cdir/$(basename -- "$cand") +" break fi done - if [ -n "$found" ]; then break; fi done <<< "$cands" - if [ -n "$found" ]; then break; fi done <<< "$bases" + # EVERY CANDIDATE THAT EXISTS, NOT THE FIRST ONE. Ordering the candidate list was the wrong + # tool for this and it could only ever be wrong in one direction: + # + # - source before emitted, so a stale `admin.mjs` beside its `admin.mts` cannot mask a + # secret added to the source (a real finding, fixed that way); + # - but in a JAVASCRIPT project `require("../lib/admin.js")` means admin.js, and putting + # the `.ts` substitution first scanned a clean `admin.ts` while `admin.js` — the module + # Node actually loads, holding SUPABASE_SERVICE_ROLE_KEY — was never read. `ok`, exit 0. + # + # Whichever candidate is second gets skipped, and either way round that is a false green. + # So both are walked. The cost is at most a module read that Node would not have loaded — + # a false red, and the direction this toolkit errs in; the benefit is that no ordering + # heuristic has to be right about a project type this gate cannot reliably detect. if [ -n "$found" ]; then printf '%s' "$found"; return 0; fi # A baseUrl probe that found nothing is TypeScript's own fallthrough to node_modules — but # only for a name that could BE a package. Reporting UNKNOWN for every miss would turn @@ -681,7 +704,13 @@ while :; do rc=$? set -e case "$rc" in - 0) printf '%s\t%s\n' "$file" "$target" >> "$EDGES"; enqueue "$target" ;; + 0) # resolve() may name MORE THAN ONE file — see its note: when both an explicitly + # imported `.js` and a same-named `.ts` exist on disk, either could be the module + # that runs, so both are edges and both are walked. + while IFS= read -r one; do + [ -n "$one" ] || continue + printf '%s\t%s\n' "$file" "$one" >> "$EDGES"; enqueue "$one" + done <<< "$target" ;; 1) : ;; # bare specifier: a published package, out of this gate's reach by design 2) unknown "$(rel "$file") imports '$spec', which this gate could not resolve to a file — an unread module is not a clean one" ;; 3) unknown "$(rel "$file") imports '$spec', which matches no path alias this gate could read$TSCONFIG_NOTE and is not a well-formed package name — this gate cannot say what it is, and will not call it a dependency to skip it" ;;