security: add 7 new rules to extension scanner#49
Merged
Conversation
…d exports - Add 5 new line rules: fs-write-outside-home, privilege-escalation, network-listener, prototype-pollution, unsafe-deserialization - Add 2 new source rules: credential-logging, dynamic-require (info severity) - Add --json flag for structured CI-consumable output - Export scanSource(), LINE_RULES, SOURCE_RULES for programmatic use - Add 7 new tests (22 total, up from 15)
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Greptile SummaryEnhances the extension/skill scanner with 7 new security rules (
Confidence Score: 3/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["CLI: node scan-extensions.mjs [--json] [dirs]"] --> B{Parse args}
B --> C["--json flag?"]
B --> D["Directory list"]
D --> E["For each directory"]
E --> F["walkDir: collect .js/.ts files"]
F --> G["For each file"]
G --> H["scanSource(source, filePath)"]
H --> I["LINE_RULES: per-line patterns\n(dangerous-exec, eval, crypto-mining,\nfs-write-outside-home, privilege-escalation,\nnetwork-listener, prototype-pollution,\nunsafe-deserialization)"]
H --> J["SOURCE_RULES: full-file patterns\n(exfiltration, obfuscation, env-harvesting,\ncredential-logging, dynamic-require)"]
I --> K["Collect findings"]
J --> K
K --> L{Output mode}
C -->|yes| L
C -->|no| L
L -->|--json| M["JSON report to stdout\n{timestamp, scanned, summary, findings}"]
L -->|text| N["Human-readable console output"]
M --> O{Exit code}
N --> O
O -->|critical > 0| P["exit(2)"]
O -->|warn > 0| Q["exit(1)"]
O -->|else| R["exit(0)"]
Last reviewed commit: 7c7ff9f |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+62
to
+66
| id: "fs-write-outside-home", | ||
| severity: "critical", | ||
| message: "Filesystem write to system path detected", | ||
| pattern: /writeFileSync?\s*\(\s*["'](\/etc\/|\/usr\/|\/var\/|\/root\/)/, | ||
| }, |
There was a problem hiding this comment.
Template literal bypass in path pattern
The fs-write-outside-home pattern only matches single (') and double (") quotes, but not backtick template literals. Code like writeFileSync(`/etc/passwd`, data) will bypass this rule entirely.
Suggested change
| id: "fs-write-outside-home", | |
| severity: "critical", | |
| message: "Filesystem write to system path detected", | |
| pattern: /writeFileSync?\s*\(\s*["'](\/etc\/|\/usr\/|\/var\/|\/root\/)/, | |
| }, | |
| { | |
| id: "fs-write-outside-home", | |
| severity: "critical", | |
| message: "Filesystem write to system path detected", | |
| pattern: /writeFileSync?\s*\(\s*["'`](\/etc\/|\/usr\/|\/var\/|\/root\/)/, | |
| }, |
Prompt To Fix With AI
This is a comment left during a code review.
Path: bin/scan-extensions.mjs
Line: 62-66
Comment:
**Template literal bypass in path pattern**
The `fs-write-outside-home` pattern only matches single (`'`) and double (`"`) quotes, but not backtick template literals. Code like `` writeFileSync(`/etc/passwd`, data) `` will bypass this rule entirely.
```suggestion
{
id: "fs-write-outside-home",
severity: "critical",
message: "Filesystem write to system path detected",
pattern: /writeFileSync?\s*\(\s*["'`](\/etc\/|\/usr\/|\/var\/|\/root\/)/,
},
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
357
to
366
| if (totalCritical > 0) { | ||
| console.log(`🚨 ${totalCritical} critical finding(s) — review immediately!`); | ||
| if (!jsonOutput) console.log(`🚨 ${totalCritical} critical finding(s) — review immediately!`); | ||
| process.exit(2); | ||
| } else if (totalWarn > 0) { | ||
| console.log(`⚠️ ${totalWarn} warning(s) — review recommended.`); | ||
| if (!jsonOutput) console.log(`⚠️ ${totalWarn} warning(s) — review recommended.`); | ||
| process.exit(1); | ||
| } else { | ||
| console.log("✅ All clean."); | ||
| if (!jsonOutput) console.log("✅ All clean."); | ||
| process.exit(0); | ||
| } |
There was a problem hiding this comment.
"All clean" message printed even with info-level findings
When only info-severity findings exist (e.g., dynamic-require), the exit-code logic falls through to the else branch, printing "All clean" and exiting 0. This is misleading since findings were reported. The info count is shown in the summary block (line 353), but the final status message contradicts it.
Consider something like:
Suggested change
| if (totalCritical > 0) { | |
| console.log(`🚨 ${totalCritical} critical finding(s) — review immediately!`); | |
| if (!jsonOutput) console.log(`🚨 ${totalCritical} critical finding(s) — review immediately!`); | |
| process.exit(2); | |
| } else if (totalWarn > 0) { | |
| console.log(`⚠️ ${totalWarn} warning(s) — review recommended.`); | |
| if (!jsonOutput) console.log(`⚠️ ${totalWarn} warning(s) — review recommended.`); | |
| process.exit(1); | |
| } else { | |
| console.log("✅ All clean."); | |
| if (!jsonOutput) console.log("✅ All clean."); | |
| process.exit(0); | |
| } | |
| if (totalCritical > 0) { | |
| if (!jsonOutput) console.log(`🚨 ${totalCritical} critical finding(s) — review immediately!`); | |
| process.exit(2); | |
| } else if (totalWarn > 0) { | |
| if (!jsonOutput) console.log(`⚠️ ${totalWarn} warning(s) — review recommended.`); | |
| process.exit(1); | |
| } else { | |
| if (!jsonOutput) { | |
| if (totalInfo > 0) console.log(`ℹ️ ${totalInfo} info finding(s) — no action required.`); | |
| else console.log("✅ All clean."); | |
| } | |
| process.exit(0); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: bin/scan-extensions.mjs
Line: 357-366
Comment:
**"All clean" message printed even with info-level findings**
When only `info`-severity findings exist (e.g., `dynamic-require`), the exit-code logic falls through to the `else` branch, printing "All clean" and exiting 0. This is misleading since findings *were* reported. The info count is shown in the summary block (line 353), but the final status message contradicts it.
Consider something like:
```suggestion
if (totalCritical > 0) {
if (!jsonOutput) console.log(`🚨 ${totalCritical} critical finding(s) — review immediately!`);
process.exit(2);
} else if (totalWarn > 0) {
if (!jsonOutput) console.log(`⚠️ ${totalWarn} warning(s) — review recommended.`);
process.exit(1);
} else {
if (!jsonOutput) {
if (totalInfo > 0) console.log(`ℹ️ ${totalInfo} info finding(s) — no action required.`);
else console.log("✅ All clean.");
}
process.exit(0);
}
```
How can I resolve this? If you propose a fix, please make it concise.| assert.ok(output.includes("prototype") || output.includes("pollution"), output); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Missing tests for 3 new rules
The new rules credential-logging, unsafe-deserialization, and dynamic-require were added to the scanner but have no corresponding tests. The PR description states "22 total (up from 15)" tests, but those 3 rules are uncovered. Per the project conventions in CLAUDE.md: "All security code must have tests before merging."
Context Used: Context from dashboard - CLAUDE.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: bin/scan-extensions.test.mjs
Line: 277
Comment:
**Missing tests for 3 new rules**
The new rules `credential-logging`, `unsafe-deserialization`, and `dynamic-require` were added to the scanner but have no corresponding tests. The PR description states "22 total (up from 15)" tests, but those 3 rules are uncovered. Per the project conventions in `CLAUDE.md`: "All security code must have tests before merging."
**Context Used:** Context from `dashboard` - CLAUDE.md ([source](https://app.greptile.com/review/custom-context?memory=301c5d5e-f39f-40b0-952d-acd5ddc604f4))
How can I resolve this? If you propose a fix, please make it concise.| severity: "warn", | ||
| message: "Possible credential logging (process.env written to log/console)", | ||
| pattern: /process\.env/, | ||
| requiresContext: /console\.(log|info|warn|error|debug)\s*\(.*process\.env/, |
There was a problem hiding this comment.
requiresContext regex can't match across lines
The .* in this regex won't match newlines (no s flag), so requiresContext only fires when console.log( and process.env appear on the same line. Since this is a SOURCE_RULE tested against the full file source (line 189), a multi-line pattern will not be caught:
const val = process.env.TOKEN;
console.log(val); // not detected — different linesOnly the inline form console.log(process.env.TOKEN) is detected. If that's intentional (to limit false positives), consider documenting the limitation in a comment. If it should match across lines, the regex needs [\s\S]*? instead of .*.
Prompt To Fix With AI
This is a comment left during a code review.
Path: bin/scan-extensions.mjs
Line: 130
Comment:
**`requiresContext` regex can't match across lines**
The `.*` in this regex won't match newlines (no `s` flag), so `requiresContext` only fires when `console.log(` and `process.env` appear on the **same** line. Since this is a SOURCE_RULE tested against the full file source (line 189), a multi-line pattern will not be caught:
```js
const val = process.env.TOKEN;
console.log(val); // not detected — different lines
```
Only the inline form `console.log(process.env.TOKEN)` is detected. If that's intentional (to limit false positives), consider documenting the limitation in a comment. If it should match across lines, the regex needs `[\s\S]*?` instead of `.*`.
How can I resolve this? If you propose a fix, please make it concise.…missing tests - Add backtick to fs-write-outside-home pattern (template literal bypass) - Use [\s\S]*? in credential-logging requiresContext for cross-line matching - Fix 'All clean' message showing with info-level findings - Add 5 missing tests: credential-logging (same-line + multiline), unsafe-deserialization, dynamic-require, template literal fs write - 27 tests total (up from 22)
Strip dead code: --json output nobody consumes, exports nobody imports. The scanner is a CLI tool run manually or via security-audit.sh --deep.
benvinegar
changed the title
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.
Adds new detection rules to
bin/scan-extensions.mjs:fs-write-outside-homewriteFileSync('/etc/...')including template literalsprivilege-escalationsudo/chmod/chownvia child_processnetwork-listenercreateServer()/.listen(port)with http/net importsprototype-pollution__proto__,Object.setPrototypeOfunsafe-deserializationJSON.parse(req.body)on external inputcredential-loggingconsole.log(process.env.SECRET)(direct + multiline)dynamic-requirerequire(variable)— dynamic module loadingAlso fixes the template literal bypass in path matching (backtick was missing from the character class) and uses
[\s\S]*?for cross-line regex matching in credential-logging.25 tests (up from 15).