refactor(scan): extract the scan engine into @threatcrush/scan - #94
Merged
Conversation
The rules and the engine lived at apps/cli/src/scan/, so the CLI was the
only surface that could run a scan. Every other app either did without or
would have grown its own copy of the rules — which for a rule set whose
whole value is being carefully tuned against false positives is the worst
possible outcome.
Two entry points, split on whether a filesystem is required:
@threatcrush/scan rules, scanText, language detection,
suppressions, severity. No node: imports.
@threatcrush/scan/node scanPath tree walker, dependency scan, SARIF.
The split is the point. Browser surfaces — web, extension, desktop
renderer — import the default entry into a bundle, and one node:fs
anywhere in that graph breaks their build, not ours. So engine.ts became
text.ts (pure) plus node/walk.ts (fs), and languageOf's two node:path
calls became three lines of string arithmetic.
Enforced rather than asserted: src/__tests__/boundaries.test.ts fails if
any file outside node/ imports node:. Verified both directions with
esbuild — the default entry bundles for the browser at 65kb, the node
entry fails on node:crypto exactly as it should.
exports resolves to TypeScript source rather than a build output. The
release workflow runs
> @profullstack/threatcrush@0.7.0 build /home/anthony/src/profullstack/threatcrush/.claude/worktrees/typosquat-scope-fix/apps/cli
> tsup
�[34mCLI�[39m Building entry: {"index":"src/index.ts","daemon":"src/daemon-entry.ts"}
�[34mCLI�[39m Using tsconfig: tsconfig.json
�[34mCLI�[39m tsup v8.5.1
�[34mCLI�[39m Using tsup config: /home/anthony/src/profullstack/threatcrush/.claude/worktrees/typosquat-scope-fix/apps/cli/tsup.config.ts
�[34mCLI�[39m Target: node20
�[34mCLI�[39m Cleaning output folder
�[34mCJS�[39m Build start
�[32mCJS�[39m �[1mdist/daemon.js �[22m�[32m249.66 KB�[39m
�[32mCJS�[39m �[1mdist/index.js �[22m�[32m637.71 KB�[39m
�[32mCJS�[39m �[1mdist/daemon.js.map �[22m�[32m486.06 KB�[39m
�[32mCJS�[39m �[1mdist/index.js.map �[22m�[32m1.05 MB�[39m
�[32mCJS�[39m ⚡️ Build success in 360ms
alone, so a package needing a prior build would publish a broken CLI the
first time someone forgot — as MODULE_NOT_FOUND in the published artefact
rather than a red build. tsup bundles it via noExternal, so the published
CLI is unchanged and gains no dependency on an unpublished package.
parseFailOn's test moved to apps/cli: parsing argv is the CLI's job and
the package has no opinion about it.
Behaviour is identical. Scanned debtap and capacitor with the rebuilt CLI
and compared rule/file/line for every finding against the previous build:
8 and 26, both byte-identical. 118 tests in the package, 2 in the CLI.
Pre-existing apps/web failures (topup, release-docs) confirmed present on
master before this change.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ThreatCrush Security Scan156 finding(s) HIGH/CRITICAL: 14 | MEDIUM: 106 | LOW: 36
…and 106 more. Full results in the Security tab. Snippets are redacted; ThreatCrush never prints matched credential material. |
ralyodio
added a commit
that referenced
this pull request
Aug 11, 2026
…he CLI Extracting the scan engine (#94) added `@threatcrush/scan: workspace:*` to the CLI's dependencies. pnpm understands `workspace:*`; the release runs `npm publish`, which does not rewrite it — so the published package.json ships `workspace:*` verbatim, and every `npm install -g @profullstack/threatcrush` fails with EUNSUPPORTEDPROTOCOL. This broke all installs of 0.7.0 and 0.7.1, including the malware-test-prs scan workflow that installs @latest. The package is bundled into the CLI by tsup (`noExternal`), so it is a build-time dependency, not a runtime one — nothing requires it from the published artifact. Moving it to devDependencies is the correct classification and the fix: npm does not install a package's devDependencies, so the `workspace:*` spec is never resolved by consumers, while pnpm still links it for the build. Verified end to end: packed the 0.7.2 tarball and confirmed its published `dependencies` carries no `workspace:` spec; `npm install` of that tarball into a clean project succeeds (209 packages, no protocol error); @threatcrush/scan is not installed separately; and the installed CLI scans and detects correctly from the bundle alone. `pnpm install --frozen-lockfile` passes with the updated lockfile. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ralyodio
added a commit
that referenced
this pull request
Aug 11, 2026
…positive (v0.7.2) (#99) * fix(scan): require SQL structure so a verb-shaped word is not read as injection Release 0.7.2. The SQL keyword list matched bare verbs — INSERT, UPDATE, DELETE, DROP, a lone SELECT — each with a trailing word boundary. A word boundary sits at the hyphen in a React key `insert-${i}`, and after `Update` in a log line `Update finished in ${ms}ms`, so both read as SQL injection at critical severity. On ralyodio/ShortsStudio that was the only finding; on ionic-team/capacitor it was three of thirteen. Real SQL pairs the verb with the clause that makes it a statement: SELECT … FROM, INSERT INTO, UPDATE … SET, DELETE FROM, DROP TABLE. Requiring that structure keeps every injection shape the corpus and the unit tests exercise — all of which are SELECT … FROM or DELETE FROM — while a verb standing alone as prose no longer qualifies. The SELECT and UPDATE look-aheads are bounded to one string literal so the clause must be in the same statement. ShortsStudio: 1 finding to 0 (clean). capacitor: 13 to 10, removing all three sql-template-interpolation false positives and nothing else. Testbed coverage unchanged at TPR 65.9% / FPR 0% — no true positive lost. 125 tests, up from 123; the new false-positive test was confirmed to fail against the old pattern. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(cli): make @threatcrush/scan a devDependency so npm can install the CLI Extracting the scan engine (#94) added `@threatcrush/scan: workspace:*` to the CLI's dependencies. pnpm understands `workspace:*`; the release runs `npm publish`, which does not rewrite it — so the published package.json ships `workspace:*` verbatim, and every `npm install -g @profullstack/threatcrush` fails with EUNSUPPORTEDPROTOCOL. This broke all installs of 0.7.0 and 0.7.1, including the malware-test-prs scan workflow that installs @latest. The package is bundled into the CLI by tsup (`noExternal`), so it is a build-time dependency, not a runtime one — nothing requires it from the published artifact. Moving it to devDependencies is the correct classification and the fix: npm does not install a package's devDependencies, so the `workspace:*` spec is never resolved by consumers, while pnpm still links it for the build. Verified end to end: packed the 0.7.2 tarball and confirmed its published `dependencies` carries no `workspace:` spec; `npm install` of that tarball into a clean project succeeds (209 packages, no protocol error); @threatcrush/scan is not installed separately; and the installed CLI scans and detects correctly from the bundle alone. `pnpm install --frozen-lockfile` passes with the updated lockfile. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Extracts the scan engine into
@threatcrush/scanso every surface can share one copy of the rules.Why
The rules and the engine lived at
apps/cli/src/scan/. The CLI was the only surface that could run a scan; web, desktop, extension and mobile either did without or would have grown their own copy. For a rule set whose entire value is being carefully tuned against false positives, a second copy is the worst possible outcome — the two drift, and the one that drifts is the one nobody is measuring.Two entry points
.scanText, language detection, suppressions, severity./nodescanPathwalker, dependency scan, SARIFThe split is the point. Browser surfaces import the default entry into a bundle, and a single
node:fsanywhere in that module graph breaks their build — late, and in the wrong repository. So:engine.tsbecametext.ts(pure) +node/walk.ts(filesystem).languageOf's twonode:pathcalls became three lines of string arithmetic. Semantics match on the inputs that reach them: a leading dot is not an extension, so.envstill has none.sarif.tsanddependencies.tsmoved undernode/— they neednode:cryptoandnode:fs.Enforced, not asserted
src/__tests__/boundaries.test.tsfails if any file outsidenode/importsnode:. Verified in both directions with esbuild:And confirmed non-vacuous: adding
import { sep } from 'node:path'totypes.tsfails the test by name. (It also caught its own first draft — the initial version flagged the sentence intext.tsexplaining whynode:fsmust not appear there. Matching prose as code is precisely the bugproseLinesexists to avoid, so comment lines are now skipped.)Why
exportspoints at TypeScript sourceThe release workflow runs
pnpm --filter @profullstack/threatcrush buildand nothing else. A package requiring a prior build step would publish a broken CLI the first time someone forgot — surfacing asMODULE_NOT_FOUNDinside the published artefact rather than as a red build.So this is an internal package:
exportsresolves tosrc/*.ts, and consumers transpile it. tsup bundles it into the CLI vianoExternal, which means the published CLI is unchanged and gains no dependency on an unpublished package. The README documents what to do if it ever ships standalone (add a build, switchexportstodistunderpublishConfig; nothing else moves).One test changed sides
parseFailOnis argv parsing — the CLI's job. Its assertion moved toapps/cli/src/commands/__tests__/scan.test.ts; the package keepsmeetsFailThreshold, which is about findings. That boundary is the one the extraction draws.Verification
Behaviour is identical, checked rather than assumed. Rebuilt the CLI and compared
ruleId:file:linefor every finding against the previous build:ralyodio/debtapionic-team/capacitorpackages/scan: 118 tests (111 moved + 7 new boundary tests).apps/cli: 2 tests.tsc --noEmitclean in both.apps/webhas 3 failing test files (topup,release-docs) — confirmed present on cleanmasterby stashing this branch and re-running. Not caused by, and not fixed by, this PR.Next
Wiring the surfaces up is deliberately not in this PR — it is a behaviour-preserving move, and mixing it with per-app integration would make the "identical findings" claim unverifiable. Next.js needs
transpilePackages: ['@threatcrush/scan']; Vite works as-is.