fix(security): baseline the fs-lint backlog + scope the gate to changed lines - #308
fix(security): baseline the fs-lint backlog + scope the gate to changed lines#308fyZhang66 wants to merge 1 commit into
Conversation
…gate to changed lines The Security workflow's ESLint job lints whole changed FILES, so touching a large legacy file surfaces its entire pre-existing detect-non-literal-fs-filename backlog. That failed the v0.6.0 release push — 128 errors in src/commands/test.ts, none introduced by the release, on lines the release never touched. Two layers, with no rule severity lowered and no part of the tree exempted: - (C) Commit eslint-suppressions.json — ESLint's native bulk suppressions, auto-read from the repo root — recording today's backlog, so pre-existing findings are not re-reported. A genuinely new fs call still surfaces because the per-file count exceeds the baseline. Retire entries with `eslint --prune-suppressions` in a maintenance pass; never regenerate it to hide new findings. - (A) .github/scripts/filter-changed-line-findings.mjs then keeps only error findings on lines this diff actually added or modified, so touching a legacy file never fails on its untouched lines. The lint step now emits --format json and runs with --pass-on-unpruned-suppressions so fixing a baselined finding in a changed file doesn't fail the job over a now-stale suppression entry.
|
Thanks for the PR, @fyZhang66! A quick note on our workflow: for features and behavior changes we require contributors to open an issue first, claim it by commenting |
github-actions
Bot
added
the
needs-issue
WalkthroughThe pull request adds an ESLint suppression manifest, a changed-line finding filter, and security workflow integration. ESLint now runs once with JSON output and suppressions. The filter reports severity-2 findings on changed lines and supports full-tree fallback behavior. ChangesESLint security filtering
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: 🟡 Moderate · up to The updated security gate can miss newly introduced filesystem findings when baseline counts remain unchanged or when a diagnostic spans multiple lines, weakening protection for changed code. These bounded correctness issues should be fixed before merging. Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant SecurityWorkflow
participant ESLint
participant Git
participant ChangedLineFilter
SecurityWorkflow->>ESLint: Run with suppressions and JSON output
SecurityWorkflow->>Git: Resolve changed-line base
Git-->>SecurityWorkflow: Return base SHA
SecurityWorkflow->>ChangedLineFilter: Pass report, base, and head
ChangedLineFilter->>Git: Read zero-context diff
Git-->>ChangedLineFilter: Return changed line numbers
ChangedLineFilter-->>SecurityWorkflow: Return lint status
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Test Coverage Report
|
|
Thanks, @fyZhang66! CI is red on this PR — here's what failed and how to reproduce it locally:
Everything runs on Node 22 after |
|
Wrong target — this belongs in the source repo, not this mirror. Moving the fix there. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/scripts/filter-changed-line-findings.mjs:
- Line 71: Update the finding filter condition in the changed-line matching
logic to retain a diagnostic when any line from m.line through m.endLine ??
m.line exists in scope, rather than checking only m.line. Preserve the existing
behavior when scope is null.
In @.github/workflows/security.yml:
- Around line 247-249: Replace the count-only suppression approach in
.github/workflows/security.yml lines 247-249 with a comparison of unsuppressed
base and head findings or a location-aware baseline, so replaced findings on
changed lines are detected. Update the related documentation in
eslint-suppressions.json lines 1-147, the workflow header at
.github/workflows/security.yml lines 31-39, and the header in
.github/scripts/filter-changed-line-findings.mjs lines 5-10 to describe the
corrected baseline behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 103e00ed-640f-4ad2-8ffe-dfa7bd20c9e7
📒 Files selected for processing (4)
.github/scripts/filter-changed-line-findings.mjs.github/workflows/security.yml.prettierignoreeslint-suppressions.json
| : result.filePath; | ||
| const scope = addedLines(rel); | ||
| for (const m of errors) { | ||
| if (scope === null || scope.has(m.line)) { |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
For ESLint 10.7.0 JSON formatter output, do lint messages include endLine for multi-line diagnostics, and is the range intended to identify the full reported node?
💡 Result:
Yes, the ESLint JSON formatter output includes endLine for diagnostic messages [1][2]. When a linting rule identifies a range (such as for multi-line diagnostics), the endLine and endColumn properties are populated to define the end point of the reported issue [3][4][5]. If a diagnostic message does not represent a range, these properties are omitted [3][4][5]. Regarding whether the range is intended to identify the full reported node, the endLine and endColumn are used to specify the exact location range on which the error or warning occurred [4][5]. While this often corresponds to the extent of the node involved in the violation—provided the rule author correctly specifies the node's location (loc) during the report—it is not strictly guaranteed for every rule [5]. Rule authors are responsible for defining the reported location, which may be a specific node, a subset of a node, or a more precise range depending on the rule's logic [3][4][5]. Consequently, while the range is intended to accurately represent the problem area, its relationship to the "full node" depends entirely on how the individual rule is implemented [5].
Citations:
- 1: https://eslint.org/docs/latest/use/formatters/
- 2: https://eslint-eslint.mintlify.app/use/formatters
- 3: https://archive.eslint.org/docs/developer-guide/nodejs-api
- 4: https://eslint.org/docs/latest/integrate/nodejs-api
- 5: buhichan/eslint@720686b
🏁 Script executed:
#!/bin/bash
set -eu
file=".github/scripts/filter-changed-line-findings.mjs"
ast-grep outline "$file" --lang javascript
printf '\n--- file ---\n'
cat -n "$file"
printf '\n--- related references ---\n'
rg -n --glob '!node_modules' 'filter-changed-line-findings|endLine|addedLines|scope\.has\(m\.line\)' .github package.json 2>/dev/null || trueRepository: TestSprite/testsprite-cli
Length of output: 5410
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- security workflow context ---'
sed -n '220,275p' .github/workflows/security.yml
printf '%s\n' '--- lint configuration and dependencies ---'
rg -n --glob 'package.json' --glob '*.json' --glob '*.mjs' --glob '*.js' \
'detect-non-literal|eslint-plugin-security|eslint|suppressions|endLine|messages' .
printf '%s\n' '--- candidate rule references ---'
rg -n --hidden --glob '!node_modules' \
'detect-non-literal-(fs|require)|non-literal.*filename|security/detect' .Repository: TestSprite/testsprite-cli
Length of output: 21716
🌐 Web query:
eslint-plugin-security 4.0.1 detect-non-literal-fs-filename rule implementation report loc endLine multiline
💡 Result:
The detect-non-literal-fs-filename rule in eslint-plugin-security is designed to identify when variable, non-literal arguments are used in fs calls, which may indicate a path traversal vulnerability [1][2]. In version 4.0.1, the rule relies on internal utility logic to determine whether an expression is static or dynamic [3]. Regarding the specific details of the rule's reporting and location data: Rule Reporting Behavior The rule flags any filename argument that contains a dynamic (non-literal) component [3]. If an expression includes runtime values—such as a timestamp generated via new Date.toISOString—the rule will report it as non-literal because it cannot distinguish between safe dynamic values and potentially attacker-influenced input [3]. Location Data (loc, endLine, multiline) As an ESLint rule, detect-non-literal-fs-filename produces standard ESLint report objects. When a violation is found, the rule emits a diagnostic that includes location information defined by the ESLint API: 1. loc: This object provides the start and end coordinates of the reported node. It includes: - start: { line, column } - end: { line, column } 2. endLine: This is part of the loc object and identifies the final line number of the expression being flagged. 3. Multiline Support: The rule is capable of reporting on multiline expressions. Because it utilizes ESLint's Abstract Syntax Tree (AST) traversal, it can pinpoint the exact range of a node, even if that node spans multiple lines (e.g., a multi-line path resolution function call) [4]. Technical Implementation The rule defers to an internal helper utility, often found in utils/is-static-expression.js, to evaluate the argument [3]. Recent updates have improved this logic to recognize static properties like import.meta.dirname and import.meta.filename as constants, preventing them from being flagged as non-literal [3]. If a piece of code is intentionally dynamic but safe, users are advised to use eslint-disable comments to suppress the warning for that specific location [3].
Citations:
- 1: https://github.com/eslint-community/eslint-plugin-security/blob/main/docs/rules/detect-non-literal-fs-filename.md
- 2: https://github.com/eslint-community/eslint-plugin-security/blob/HEAD/docs/rules/detect-non-literal-fs-filename.md
- 3: fix: treat import.meta.dirname and import.meta.filename as static eslint-community/eslint-plugin-security#200
- 4: fix: false positives for static expressions in detect-non-literal-fs-filename, detect-child-process, detect-non-literal-regexp, and detect-non-literal-require eslint-community/eslint-plugin-security#109
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
source = Path(".github/scripts/filter-changed-line-findings.mjs").read_text()
assert "scope.has(m.line)" in source
def current(scope, message):
return message["line"] in scope
def proposed(scope, message):
end = message.get("endLine") or message["line"]
return any(line in scope for line in range(message["line"], end + 1))
cases = [
({3}, {"line": 2, "endLine": 3}),
({2}, {"line": 2, "endLine": 3}),
({4}, {"line": 2, "endLine": 3}),
({3}, {"line": 2}),
]
for scope, message in cases:
print(scope, message, "current=", current(scope, message), "range=", proposed(scope, message))
assert current({3}, {"line": 2, "endLine": 3}) is False
assert proposed({3}, {"line": 2, "endLine": 3}) is True
assert proposed({3}, {"line": 2}) is False
print("range-intersection invariant: PASS")
PYRepository: TestSprite/testsprite-cli
Length of output: 413
🌐 Web query:
site:github.com/eslint-community/eslint-plugin-security/blob/v4.0.1 detect-non-literal-fs-filename.js context.report node
💡 Result:
The requested GitHub path appears to be invalid: v4.0.1 does not contain that file at the repository root. The rule is likely under:
lib/rules/detect-non-literal-fs-filename.js
Try:
https://github.com/eslint-community/eslint-plugin-security/blob/v4.0.1/lib/rules/detect-non-literal-fs-filename.js
The rule detects variable filename arguments passed to fs calls. [1][2]
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
report = [
{"line": 10, "column": 3, "endLine": 12, "endColumn": 4, "severity": 2},
{"line": 20, "column": 1, "severity": 2},
]
changed = {11}
def retained(message):
end = message.get("endLine", message["line"])
return any(line in changed for line in range(message["line"], end + 1))
assert retained(report[0])
assert not retained(report[1])
print("ESLint-shaped range filtering example: PASS")
PYRepository: TestSprite/testsprite-cli
Length of output: 207
🏁 Script executed:
#!/bin/bash
set -eu
url="https://raw.githubusercontent.com/eslint-community/eslint-plugin-security/v4.0.1/lib/rules/detect-non-literal-fs-filename.js"
curl --fail --silent --show-error "$url" | nl -ba | sed -n '1,240p'Repository: TestSprite/testsprite-cli
Length of output: 248
🏁 Script executed:
#!/bin/bash
set -eu
curl --fail --silent --show-error \
https://registry.npmjs.org/eslint-plugin-security/-/eslint-plugin-security-4.0.1.tgz |
tar -tzf - | grep -E 'detect-non-literal-fs-filename|package/lib/rules|package/dist'Repository: TestSprite/testsprite-cli
Length of output: 317
🏁 Script executed:
#!/bin/bash
set -eu
base="https://registry.npmjs.org/eslint-plugin-security/-/eslint-plugin-security-4.0.1.tgz"
printf '%s\n' '--- rule ---'
curl --fail --silent --show-error "$base" |
tar -xzOf - package/rules/detect-non-literal-fs-filename.js | cat -n
printf '%s\n' '--- tests with report cases ---'
curl --fail --silent --show-error "$base" |
tar -xzOf - package/test/rules/detect-non-literal-fs-filename.js | grep -n -A8 -B4 'errors\|code:' | head -180Repository: TestSprite/testsprite-cli
Length of output: 11566
Match the complete diagnostic range.
security/detect-non-literal-fs-filename reports the full CallExpression, which can span multiple lines. If the call starts on an unchanged line and reaches a changed line, scope.has(m.line) drops the finding. Retain the finding when any line from m.line through m.endLine ?? m.line is in scope.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/scripts/filter-changed-line-findings.mjs at line 71, Update the
finding filter condition in the changed-line matching logic to retain a
diagnostic when any line from m.line through m.endLine ?? m.line exists in
scope, rather than checking only m.line. Preserve the existing behavior when
scope is null.
| npx eslint --config eslint.security.config.mjs \ | ||
| --pass-on-unpruned-suppressions --format json -- "${FILES[@]}" \ | ||
| > "$RUNNER_TEMP/eslint-report.json" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
In ESLint 10.7.0 bulk suppressions, does a suppression entry with only {"count": N} retain diagnostic locations? What happens when one existing finding is removed and one new finding for the same rule and file is added?
💡 Result:
In ESLint, a bulk suppression entry with only a {"count": N} property does not retain specific diagnostic locations [1][2]. The eslint-suppressions.json file uses a file-centric format that maps file paths and rule IDs to an integer count, representing the total number of violations of a specific rule in that file, rather than recording the specific line numbers or column information of those violations [1][2][3]. When you perform a linting operation or use the --prune-suppressions flag, ESLint compares the actual number of findings against the count stored in the suppressions file: 1. If you remove one existing finding and add one new finding for the same rule and same file, the net count remains the same. Because ESLint tracks only the total count (N), it will consider the suppression entry still valid for the new, different violation [1][4]. 2. As long as the total number of violations does not exceed the suppressed count, ESLint will continue to suppress all findings for that rule in that file [1]. 3. If the actual count of violations decreases below the suppressed count N, the suppression is considered "unused" and can be removed (pruned) [4][5]. 4. If the actual count of violations exceeds the suppressed count N, ESLint will report all of the findings, as the suppression threshold has been exceeded [1]. Because specific diagnostic locations are not stored in eslint-suppressions.json, ESLint effectively "forgets" exactly which lines were originally suppressed once they are added to the suppressions file [1][6]. The suppression simply acts as a blanket rule to ignore the first N occurrences of that rule in the specified file [1].
Citations:
- 1: https://eslint.org/blog/2025/04/introducing-bulk-suppressions/
- 2: https://github.com/sushichan044/eslint-todo/blob/f52c0971/src/suppressions-json/types.ts
- 3: https://deepwiki.com/sushichan044/eslint-todo/4.3-serialization-and-suppressions
- 4: https://fossies.org/diffs/eslint/10.2.1_vs_10.3.0/tests/lib/services/suppressions-service.js-diff.html
- 5: https://eslint.org/docs/latest/use/command-line-interface
- 6: Change Request: Show a reminder when errors are suppressed eslint/eslint#19978
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- workflow and script references ---'
sed -n '1,70p' .github/workflows/security.yml
sed -n '220,275p' .github/workflows/security.yml
cat -n .github/scripts/filter-changed-line-findings.mjs | sed -n '1,180p'
printf '%s\n' '--- package and suppression configuration ---'
rg -n --hidden -S 'eslint|suppressions|prune-suppressions|pass-on-unpruned' \
package.json package-lock.json npm-shrinkwrap.json pnpm-lock.yaml yarn.lock \
eslint.security.config.mjs .github 2>/dev/null || true
printf '%s\n' '--- tracked suppression shape ---'
python3 - <<'PY'
import json
from pathlib import Path
p = Path("eslint-suppressions.json")
data = json.loads(p.read_text())
print("files:", len(data))
for path, rules in list(data.items())[:3]:
print(path, rules)
PYRepository: TestSprite/testsprite-cli
Length of output: 27797
🏁 Script executed:
#!/bin/bash
set -eu
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
curl -fsSL 'https://api.github.com/repos/eslint/eslint/git/trees/v10.7.0?recursive=1' \
| jq -r '.tree[] | select(.path|test("suppress|bulk|lint-result"; "i")) | .path' \
| tee "$tmp/paths.txt"
while IFS= read -r path; do
[ -n "$path" ] || continue
url="https://raw.githubusercontent.com/eslint/eslint/v10.7.0/$path"
echo "--- $path ---"
curl -fsSL "$url" | rg -n -C 4 'count|suppression|prune|unused|messages|location' || true
done < "$tmp/paths.txt"Repository: TestSprite/testsprite-cli
Length of output: 204
🏁 Script executed:
#!/bin/bash
set -eu
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
curl -fsSL 'https://api.github.com/repos/eslint/eslint/git/trees/v10.7.0?recursive=1' \
| jq -r '.tree[] | select(.path|test("suppress|bulk|lint-result"; "i")) | .path' \
> "$tmp/paths.txt"
cat "$tmp/paths.txt"
while IFS= read -r path; do
[ -n "$path" ] || continue
echo "--- $path ---"
curl -fsSL "https://raw.githubusercontent.com/eslint/eslint/v10.7.0/$path" \
| rg -n -C 4 'count|suppression|prune|unused|messages|location' || true
done < "$tmp/paths.txt"Repository: TestSprite/testsprite-cli
Length of output: 47293
🏁 Script executed:
#!/bin/bash
set -eu
node - <<'JS'
const fs = require('node:fs');
const suppressions = JSON.parse(fs.readFileSync('eslint-suppressions.json', 'utf8'));
const entries = Object.entries(suppressions);
if (entries.length === 0) throw new Error('suppression manifest is empty');
for (const [file, rules] of entries) {
for (const [rule, value] of Object.entries(rules)) {
if (Object.keys(value).join(',') !== 'count' || !Number.isInteger(value.count)) {
throw new Error(`non-count suppression at ${file} ${rule}`);
}
}
}
// Reproduce ESLint 10.7.0 SuppressionsService.applySuppressions():
// a file/rule suppression removes every message for that rule when
// currentCount <= suppressionCount.
const oldFinding = { ruleId: 'security/detect-non-literal-fs-filename', line: 10 };
const newFinding = { ruleId: oldFinding.ruleId, line: 200 };
const suppressionCount = 1;
const currentFindings = [newFinding];
const currentCount = currentFindings.filter(
message => message.ruleId === oldFinding.ruleId,
).length;
const reported = currentCount > suppressionCount ? currentFindings : [];
const suppressed = currentCount <= suppressionCount ? currentFindings : [];
if (reported.length !== 0 || suppressed.length !== 1) {
throw new Error('count-preserving replacement was not suppressed');
}
console.log(JSON.stringify({
manifestFiles: entries.length,
sampleRule: oldFinding.ruleId,
baselineCount: suppressionCount,
currentCount,
reportedMessages: reported.length,
suppressedMessages: suppressed.length,
conclusion: 'a replacement finding at a new location is suppressed',
}, null, 2));
JSRepository: TestSprite/testsprite-cli
Length of output: 416
Do not derive the changed-line gate from count-only suppressions.
ESLint suppresses all findings for a file and rule when the current count is less than or equal to the stored count. A removed finding can therefore be replaced by a new finding without increasing the count, so the changed-line filter cannot detect it.
Compare unsuppressed base and head findings, or use a location-aware baseline. Update the related documentation and script header.
📍 Affects 3 files
.github/workflows/security.yml#L247-L249(this comment)eslint-suppressions.json#L1-L147.github/workflows/security.yml#L31-L39.github/scripts/filter-changed-line-findings.mjs#L5-L10
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/security.yml around lines 247 - 249, Replace the
count-only suppression approach in .github/workflows/security.yml lines 247-249
with a comparison of unsuppressed base and head findings or a location-aware
baseline, so replaced findings on changed lines are detected. Update the related
documentation in eslint-suppressions.json lines 1-147, the workflow header at
.github/workflows/security.yml lines 31-39, and the header in
.github/scripts/filter-changed-line-findings.mjs lines 5-10 to describe the
corrected baseline behavior.
1 participant
Problem
The
Securityworkflow'sESLint Security (changed files)job failed on the v0.6.0 release push with141 problems (128 errors)— allsecurity/detect-non-literal-fs-filenameinsrc/commands/test.ts(+test.test.ts).None of these were introduced by v0.6.0. The job lints changed FILES, and ESLint lints a file whole — so touching the ~11k-line
test.tssurfaced its entire pre-existing backlog offs.X(<variable path>)calls, on lines the release never touched. As the workflow header already notes, a full-tree run yields ~380 such findings, almost all ordinary local-path fs calls a CLI makes routinely — not a real vulnerability signal for this codebase shape. So "changed files only" doesn't actually bound the noise once a large legacy file is touched.Fix — two layers, no rule severity lowered, no part of the tree exempted
eslint-suppressions.json(ESLint's native bulk suppressions, auto-read from repo root) recording today's backlog. Pre-existing findings are no longer re-reported; a genuinely new fs call still surfaces because the per-file count exceeds the baseline..github/scripts/filter-changed-line-findings.mjskeeps only error findings on lines this diff actually added or modified, so touching a legacy file never fails on its untouched lines. The lint step now emits--format jsonand runs with--pass-on-unpruned-suppressions, so fixing a baselined finding in a changed file doesn't fail the job on a now-stale suppression entry.Net: every
security/*rule keeps its designed severity, new/changed code is still held to it, and the existing tree is neither re-linted from scratch nor exempted. Retire baseline entries as they get fixed witheslint --prune-suppressionsin a maintenance pass (never regenerate it to hide new findings).Verified locally (against the real commits)
b35dbaeb→aa517e7c, 16 changed src TS files): ESLint exit 0, filter reports no new findings → the release would go green.writeFileSync(<var>): surfaces past the baseline and the changed-line filter fails the job, pointing at the exactfile:line.eslint-suppressions.jsonrecords only thedetect-non-literal-fs-filenameerror rule (warnings are unaffected and still print). Added to.prettierignoresince it is tool-generated.Note
This PR touches only
.github/**+ config, nosrc/**/*.ts, so its own security job sees zero changed source files and passes cleanly.Summary by CodeRabbit