Fix security-scanner and duplicate-detector false positives (revised: addresses #63 review)#65
Open
abk1969 wants to merge 18 commits into
Open
Fix security-scanner and duplicate-detector false positives (revised: addresses #63 review)#65abk1969 wants to merge 18 commits into
abk1969 wants to merge 18 commits into
Conversation
Diagnosed via an external run against AIROI where all 10 reported security issues and the duplicate POST/GET finding were false positives, each traced to a specific regex/allowlist gap in the analyzer embedded in index.html.
…on fixes Task-by-task plan executing the approved design spec, each fix verified against the real analyzer (via card/lib/analyzer.js) before being written down, so every "run the test, expect it to fail/pass" step is grounded in actual measured behavior rather than assumption.
…nd real exec calls
… literal-safety, not just the first Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…g the other rules and the documented behavior Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ated architecture-diagram grouping issue The previous fixture (tests/fixtures/security-precision-world/test/preview.test.tsx) was correct in intent but its placement — a second file added to an already-populated fixture subdirectory — triggered a pre-existing, unrelated fragility in the architecture-diagram block-grouping logic (out of scope for this branch). Moved to a root-level markdown fixture, which exercises the same isNonProductionPath code path via its file extension instead of its directory, without perturbing directory topology elsewhere in the repo.
…t the first .match() without /g only inspected the first query/execute/raw call in a file, so a safe first call hid a vulnerable second one. Iterate all db calls with a global regex and flag if any argument list interpolates a SQL keyword via template literal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…on rule
The require() and from-import alternatives in the Command Execution regex
only matched the bare 'child_process' specifier, so
import { exec } from "node:child_process"; exec(userInput); produced no
finding. Both alternatives now accept an optional node: prefix.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ection
The prior regression test's server/logger.ts fixture satisfies the
Command Execution rule via its `from` import alone, so it couldn't
independently prove the `require` alternative's node: prefix handling.
Add an in-memory synthetic-file test (buildAnalysisData with a single
fabricated file, no fixture-tree file added) isolating a bare
require("node:child_process") call with no import and no dot-notation
exec() call.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ction paths The codeGroups loop in Parser.findDuplicates had picked up the same isNonProductionPath(fn.file) guard as the name-based fnByName loop, silently suppressing type:"code" structural duplicate findings for tests/fixtures/tooling/docs paths. Only the name-based loop guard was intentional; remove it from the structural loop to match the PR's stated scope. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
commit 9254993 introduced severity:'info' for backend Debug Statements findings but never wired it into the places that assume only high/medium/low exist: the severity sort comparator produced NaN for info (unstable ordering), the security tab totals row had no Info badge, .security-item had no info variant, and getSeverityColor treated info identically to low. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@abk1969 is attempting to deploy a commit to the braedonsaunders' projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
Author
|
Hello, |
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.
Revised version of #63, closed after the requested-changes review found four merge-blocking scanner regressions. This PR carries the original branch plus five commits addressing every finding, each with regression coverage that reproduces the reviewer's exact repro case. Full suite: 52/52 pass (
node --test; 47 at review time + 5 new regression tests).Review findings → fixes
query/execute/rawcall.match(...)with a global-regex loop over every db calllib/db.tsfixture now has a safedb.query("SELECT 1")before the vulnerable template-literal query (the reported repro); test verified RED pre-fixnode:child_processno longer detected(?:node:)?to both therequireandfromregex alternativesserver/logger.tsfixture exercisesimport { exec } from "node:child_process"+exec(userInput); acbc828 additionally proves therequire("node:child_process")alternative in isolation via an in-memory synthetic-file test (revert-verified RED)isNonProductionPath(fn.file)disabled structuraltype:"code"duplicate detection, contradicting the PR bodycodeGroupsloop only; the documented name-basedfnByNameguard is unchangedtests/+tools/paths asserts atype:'code'finding; RED pre-fixinfoseverity absent from severity sort map and High/Medium/Low UI totals/stylesinfo:3to the sort map, an Info totals badge,.security-item.infostyling, and aninfobranch ingetSeverityColorundefinedrank made the comparator return NaN, leaking scan order)The behavior of flagging a
child_processimport without a call is pre-existing and was left unchanged; the blocking defect was the missednode:specifier form.🤖 Generated with Claude Code