[chore][core] enable security lint rules - #7925
Open
ar2rsawseen wants to merge 6 commits into
Open
Conversation
Every rule here was measured against this tree before being turned on, and the
counts are recorded in .eslintrc.json beside each decision.
At "error", because each reports nothing today and so costs only a forward gate:
no-proto, no-implied-eval, no-new-func, no-script-url,
security/detect-child-process, security/detect-eval-with-expression,
security/detect-pseudoRandomBytes.
detect-child-process is worth a note: it exempts the destructured
`const {spawn} = require('child_process')` that 24 files use, and fires on a
non-literal first argument to exec(), which is the anti-pattern CLAUDE.md names.
At "warn", because the findings are real but clearing them is a code change
rather than a config change: security/detect-unsafe-regex (26 across 16 files,
mostly api/utils/common.js) and no-unsanitized/property (14 innerHTML writes
across 6 frontend files). Carried as warnings rather than silenced at 40 sites.
Left off, with the measurement in the config so the next person need not redo it:
guard-for-in reports 454 here and only 6.2% of those (28) sit on a loop that
actually contains a prototype-reaching write. Those 28 are caught by the same two
predicates no-prototype-pollution-sink uses, so the rule adds no coverage over it
while adding 454 reports, and it misses the sinks that motivated that rule because
a lone if/else loop body satisfies it. Also off: detect-object-injection (fires on
nearly every obj[key]), detect-non-literal-fs-filename (145, dominated by
legitimate path building) and detect-non-literal-require (19, all the plugin loader).
The ESLint CI step installs its own eslint instead of using devDependencies, so the
two new plugins are named there as well; without that the run fails to load them.
Verified: `npx eslint .` exits 0 with 0 errors, warnings 81 -> 121.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
"^4.0.2" also satisfies 4.1.5, which declares peer eslint "^9 || ^10". Against this repo's eslint ^8.56.0 npm resolves that to ERESOLVE and `npm install` exits non-zero, which took out the NPM install step of the lint job and every job downstream of it. 4.0.x is the last line peering "^6 || ^7 || ^8", so "~4.0.2" holds it there. The workflow install line was already pinned to an exact 4.0.2 and was never affected, which is why the ESLint step itself passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
package.json gained the plugins but the lock did not, and this repo commits one. Anything running npm ci then fails on the mismatch before it installs anything. Purely additive: the two plugins plus regexp-tree and safe-regex, 48 lines, no deletions. `npm ci --ignore-scripts --dry-run` exits 0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
# Conflicts: # .github/workflows/main.yml
master and this branch each edited the same two lines of the ESLint step for different reasons, so this is a union rather than a choice. master added --rulesdir bin/eslint-rules, without which eslint 8 fails every file the no-prototype-pollution-sink override matches with "Definition for rule ... was not found". This branch added eslint-plugin-security and eslint-plugin-no-unsanitized to the install, without which the run fails with "Failed to load plugin" - the step installs its own eslint rather than using the repo devDependencies, so every plugin named in .eslintrc.json has to be listed there too. Taking either side alone breaks the job. Both comments kept, since each explains a line the other does not.
…rules forbid no-script-url and security/detect-eval-with-expression each measured zero reports when this branch was written. Two test files have landed on master since, and both trip a rule on purpose. frontend.segmentation-value-prototype.js lifts functions out of a frontend file that is not a module and evaluates them by name; it already disabled no-eval, and the security rule names the same five calls. plugins.star-rating.consent-link-scheme.js holds a table of urls the consent link must refuse. The script schemes in it are the fixtures - nothing navigates to them - so refusing to write them would remove the test rather than the risk. Both exemptions are scoped to the block that needs them and say why. 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
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.
Turns on a set of security-relevant ESLint rules. Every rule was measured against this
tree first, and the count sits in
.eslintrc.jsonnext to each decision so the trade ison the record rather than assumed.
Enabled at
error— 0 reports each, so the cost is only a forward gateno-proto,no-implied-eval,no-new-func,no-script-url,security/detect-child-process,security/detect-eval-with-expression,security/detect-pseudoRandomBytes.detect-child-processis the interesting one. It exempts the destructuredconst {spawn} = require('child_process')that 24 files here use, and fires on anon-literal first argument to
exec()— which is exactly the patternCLAUDE.mdtellspeople to avoid. There are none today, so it costs nothing and holds the line.
Enabled at
warn— real findings, but clearing them is a code changesecurity/detect-unsafe-regexapi/utils/common.jsno-unsanitized/propertyinnerHTMLwrites in frontend codeEach needs a read to separate a real problem from a bounded one, so they are surfaced
rather than either blocking the build or being silenced at 40 sites.
Left off, with the measurement recorded
guard-for-in— 454 reports here. Measured against the tree, 6.2% of them (28) siton a loop that actually contains a prototype-reaching write, and those 28 are found by
the same two predicates
no-prototype-pollution-sinkalready uses. So it adds nocoverage over that rule while adding 454 reports, and it misses the sinks that motivated
it, because a lone
if/elseloop body satisfies it.security/detect-object-injection— fires on nearly everyobj[key]read.security/detect-non-literal-fs-filename— 145 reports, dominated by legitimate pathbuilding in
Gruntfile,countlyFsand the upload handlers.security/detect-non-literal-require— 19 reports, all the plugin loader doing what aplugin loader does.
CI
The ESLint step installs its own eslint rather than using
devDependencies, so the twonew plugins are named on that line too; without it the run fails with "Failed to load
plugin". They are also added to
devDependenciesso a localnpm installcovers them.Verification
npx eslint .— the exact command CI runs — exits 0 with 0 errors. Warnings go 81 -> 121.Re-checked with the versions pinned in the workflow (eslint 8.57.0, security 3.0.1,
no-unsanitized 4.0.2), same result.
Test-merges cleanly with #7923.
🤖 Generated with Claude Code
Two exemptions added after the measurement
The error-level counts above were taken when this branch was written. Two test files landed on master since, and each is deliberately the thing a rule forbids:
test/unit-tests/frontend.segmentation-value-prototype.jslifts functions out of a non-module frontend file and evaluates them by name. It already disabledno-eval;security/detect-eval-with-expressionnames the same five calls.test/unit-tests/plugins.star-rating.consent-link-scheme.jsholds a table of urls the consent link must refuse. The script schemes are the fixtures — nothing navigates to them.Both are scoped to the block that needs them and carry the reason. No production code is exempted.