I run a small service that checks every pull request for environment variables the code reads but nobody declared, and watches the sites those repos deploy to. Two weeks before launching it, I asked for a security review of the whole thing: about 12,500 lines of TypeScript, read by four reviewers in parallel, each taking one surface (web auth, the GitHub App and ingest, the worker, the CLI and supply chain).
I expected to hear about tenant isolation. That part held: no cross-tenant read or write, no auth bypass, no way to make the server fetch an internal address. What came back instead was a different category of problem, and I'd never have found it myself: one hostile pull request could stop uptime checks and alerts for every user at once.
A .gitignore line that takes 27 seconds
The PR check reads the repo's .gitignore so it skips files the author doesn't track. My matcher turned each glob into a regular expression: * became [^/]*, bracket classes were copied through, and the result went into new RegExp.
That's a backtracking engine fed by whoever opens the PR. A pattern like *a*a*a*a*a*a*a*a*a*b tested against a filename of forty as backtracks polynomially; the reviewer measured 26.9 seconds for a single test() with nine stars, and another saw twelve stars run past two minutes. A bracket class like []|(a+)+b] is worse, because the raw copy turns it into a top-level alternation with (a+)+ inside, which is exponential. And [z-a] doesn't hang at all: it throws, which has its own consequences I'll get to.
Anyone who can open a PR on a repo with the app installed can write a .gitignore. On a public repo that includes fork PRs. And the matcher ran synchronously inside the one worker process that also does the uptime checks, so while it spun, nothing was checked and nobody was alerted. Worse, the job queue can't expire a job while the event loop is blocked, and after a restart it retries the same job. Six times.
The fix had two parts, and the first was embarrassing: git doesn't apply .gitignore to files that are already in the tree, and the PR check only ever reads tracked files, so the matcher shouldn't have been running there at all. The CLI still needs one for untracked working directories, so it got a linear-time wildcard matcher modelled on git's own wildmatch, bracket classes built safely, and caps on rule count and length.
The quadratic loop that only shows up at 37,000 references
The second finding was the same shape with no regex in it. The code that groups a file's env-var references by name copied the array on every push. Harmless at fifty references. A 512 KB TypeScript file consisting of process.env.A lines has about 37,000, and that loop alone took 12 seconds; at 74,000 it was 54. A chain of 8,000 Pydantic settings classes in reverse order didn't go quadratic, it ran out of stack.
Both were on files the PR author controls, both ran in the shared worker, same blast radius. The fix for each was ordinary (push in place; resolve the class chain iteratively with a parent map). The fix that mattered was general: every PR check now runs in its own worker thread with a hard wall-clock limit, and a check that hits it is killed and reported as "couldn't be checked", neutral, never retried. No tenant's pull request can block another tenant's monitoring, whatever I haven't thought of yet.
Failing open is a security bug too
Remember [z-a], the pattern that throws? It wasn't a hang. The check just crashed, and a crashed check posts nothing: no comment, no check run, no red mark. On a repo with strict mode and branch protection, a PR that commits a secret and happens to also break the matcher shows up as having no check at all, which looks like "not required" to GitHub.
The review found three more ways to reach the same silence: a line with roughly 166,000 destructured keys blew a spread into RangeError; the check-run output had no size cap, and GitHub rejects anything over 65,535 characters with a 422; and the code that looks for its own earlier comment trusted any bot's comment containing the marker. Each one ends in nothing posted. The rule that came out of it: an unexpected error produces a neutral "couldn't be checked" run, every time. Silence is never an outcome.
The one that touched the promise
My tool's whole pitch is that it only ever sees variable names, file paths and line numbers, never values. The review found a way for a fragment of a value to reach the server as a name.
An .env file with an unterminated quote:
KEY="abc
wJalrXUtnFEMIK7MDENGbPxRfiCYEXAMPLEKEY=
OTHER=1
My parser fell through the unterminated quote on purpose, re-parsed every following line as KEY=value, and so produced a key called wJalrXUtnFEMIK7MDENGbPxRfiCYEXAMPLEKEY. That key became an "unused variable" finding, and findings get sent to the server. A PEM block pasted unquoted into .env does the same with its last base64 line.
Needs a malformed env file, so rare in CI and more likely on a laptop, and dry-run mode sends nothing. Still a lie on the security page. Now an unterminated quote swallows the rest of the file as that value, unquoted -----BEGIN to -----END is skipped, and the server validates every finding name against the env-name pattern, so even a buggy client can't push a value-shaped string through.
The test that never tested anything
While fixing the quadratic loops I wrote timing tests: feed the hostile input, assert it finishes under a bound. One of them passed on the first run. That should have been the clue. When I checked out the pre-fix code and ran the same test, it passed there too. The input wasn't big enough to trigger the slow path on a fast machine, so the test was asserting nothing.
Two rules came out of that, now written down in the repo: a timing test on hostile input has to be shown to fail against the code before the fix, or it isn't a test; and its bound needs about four times the headroom you measure locally, because the CI runner is slower and a flaky security test gets deleted by the next person it annoys.
What I'd tell someone running a multi-tenant worker
Availability is a tenant-isolation property. If one customer's input can block the loop that serves everyone, your isolation is only as good as your parsers. Anything you compile from user input into a regex is a denial-of-service you haven't measured yet. Anything you run on an attacker's file needs to be linear or run somewhere with a wall clock. And a check that can crash has to have an answer for what it posts when it does, because "nothing" reads as "fine".
The whole fix was one focused week before launch, in the scanner and the worker, and none of it changed the product. The review itself was the valuable part; the findings are in the repo's security doc if you want the full list, including the ones I decided to live with.
Source: Utsav-mishra-25/deployhealth.
If you run user-supplied globs or regexes anywhere, when did you last time them with a hostile input?
Top comments (0)