fix: add plugin cache health check#2249
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds a Node.js CLI (scripts/codex/check-plugin-cache.js), docs and tests to validate that an installed Codex plugin manifest resolves its referenced skills, MCP config, and assets in the local plugin cache, failing with diagnostics when references are missing or escape the cache. ChangesPlugin Cache Validation
Sequence DiagramsequenceDiagram
participant User as User CLI
participant Script as check-plugin-cache.js
participant FS as Filesystem
User->>Script: Run with plugin name/version/home
Script->>FS: Resolve cache directory path
FS-->>Script: Path computed
Script->>FS: Check manifest exists
alt manifest missing
FS-->>Script: Not found
Script->>User: Exit 1, list installed versions
else manifest found
FS-->>Script: Manifest file
Script->>FS: Load JSON
FS-->>Script: Parsed manifest
Script->>Script: Collect skill, MCP, asset path references
Script->>FS: Verify each reference resolves
FS-->>Script: [OK] or [FAIL] per reference
alt all references resolve
Script->>User: Exit 0, validation passed
else any reference missing
Script->>User: Exit 1, list failed references
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
| Filename | Overview |
|---|---|
| scripts/codex/check-plugin-cache.js | New health-check script; previously flagged path-traversal issues in CLI args and manifest refs are now guarded. One minor edge: validateCacheSegment is applied to the default version drawn from package.json, so a rare malformed package version would produce a confusing 'Invalid --version' error even though the user never passed the flag. |
| tests/scripts/codex-hooks.test.js | Six new cache-check tests added; the version is now read dynamically from package.json (addressing the previous hardcoded-version finding), and the hermetic env correctly propagates CODEX_HOME. |
| tests/scripts/npm-publish-surface.test.js | check-plugin-cache.js correctly added to both the expected-publish-paths helper and the required-pack-contents assertion. |
| tests/plugin-manifest.test.js | Added assertion that plugins/ecc README references check-plugin-cache.js; straightforward documentation coverage test. |
| package.json | scripts/codex/check-plugin-cache.js added to the files array so it is included in the npm publish surface. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A([check-plugin-cache.js]) --> B[parseArgs]
B --> C{plugin-dir provided?}
C -- yes --> E[Use pluginDir directly]
C -- no --> D[validateCacheSegment for marketplace / plugin / version]
D --> F[Build cacheDir from CODEX_HOME]
E --> G[cacheDir resolved]
F --> G
G --> H{manifest exists?}
H -- no --> I[listInstalledVersions and print guidance]
I --> Z([exit 1])
H -- yes --> J[readJson plugin.json]
J --> K[collectManifestRefs: skills / mcpServers / composerIcon / logo]
K --> L{for each ref}
L --> M{escapes cache boundary?}
M -- yes --> N[FAIL: escapes]
M -- no --> O{pathExists?}
O -- yes --> P[OK]
O -- no --> Q[FAIL: missing]
N --> R{more refs?}
P --> R
Q --> R
R -- yes --> L
R -- no --> S{failures gt 0?}
S -- yes --> T[print sync guidance]
T --> Z
S -- no --> U([exit 0])
Reviews (3): Last reviewed commit: "test: remove unused plugin cache fixture" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@scripts/codex/check-plugin-cache.js`:
- Around line 198-200: The three log(...) lines hardcode an ECC-specific sync
command; update the fallback to be repo-agnostic by either (A) detecting the ECC
repo root and only printing the specific "npm install && bash
scripts/sync-ecc-to-codex.sh" when the script/sync-ecc-to-codex.sh file exists
(use __dirname or process.cwd() + fs.existsSync to check) and otherwise print a
generic instruction, or (B) always replace the hardcoded command with a
generalized message like "Use the supported manual sync workflow from your ECC
installation" (update the existing log(...) calls accordingly); change the code
around the log(...) calls so the message shown is conditional on the presence
check or replaced with the generic wording.
- Line 170: Replace the hardcoded log message that says 'No installed cache
entries found for ecc/ecc.' with a dynamic message that uses the actual values
passed in via options.marketplace and options.plugin (e.g., build the string
using options.marketplace and options.plugin) so the log reflects the real
search context; locate the log call (log(...)) around where options is available
and format the message accordingly, ensuring it handles missing values
gracefully (fallback to a placeholder like '<unknown>') if necessary.
- Around line 30-75: The parseArgs function currently mutates the local options
object; refactor it to build and return a new object instead: walk argv the same
way but collect argument values into a new plain object (e.g., newOptions) or
local variables, and at the end return a composed object (with resolved
codexHome/pluginDir and defaults from PACKAGE_JSON.version) rather than
reassigning properties on the original options; update uses of options in
parseArgs (including where codexHome and pluginDir are resolved) to set values
on the new object and optionally freeze it before returning to enforce
immutability.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro
Run ID: e3bc871a-8a3b-463e-a8e9-310b7e7ebe51
📒 Files selected for processing (8)
.codex-plugin/README.mdREADME.mdpackage.jsonplugins/ecc/README.mdscripts/codex/check-plugin-cache.jstests/plugin-manifest.test.jstests/scripts/codex-hooks.test.jstests/scripts/npm-publish-surface.test.js
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (28)
- GitHub Check: Greptile Review
- GitHub Check: Test (macos-latest, Node 22.x, pnpm)
- GitHub Check: Test (macos-latest, Node 18.x, pnpm)
- GitHub Check: Test (macos-latest, Node 22.x, bun)
- GitHub Check: Test (windows-latest, Node 22.x, npm)
- GitHub Check: Test (windows-latest, Node 22.x, pnpm)
- GitHub Check: Test (windows-latest, Node 22.x, yarn)
- GitHub Check: Test (macos-latest, Node 20.x, pnpm)
- GitHub Check: Test (ubuntu-latest, Node 18.x, yarn)
- GitHub Check: Test (ubuntu-latest, Node 20.x, bun)
- GitHub Check: Test (windows-latest, Node 20.x, pnpm)
- GitHub Check: Test (macos-latest, Node 22.x, yarn)
- GitHub Check: Test (windows-latest, Node 20.x, yarn)
- GitHub Check: Test (ubuntu-latest, Node 20.x, pnpm)
- GitHub Check: Test (windows-latest, Node 18.x, npm)
- GitHub Check: Test (windows-latest, Node 18.x, pnpm)
- GitHub Check: Test (windows-latest, Node 20.x, npm)
- GitHub Check: Test (ubuntu-latest, Node 20.x, yarn)
- GitHub Check: Test (windows-latest, Node 18.x, yarn)
- GitHub Check: Test (ubuntu-latest, Node 22.x, yarn)
- GitHub Check: Test (ubuntu-latest, Node 22.x, npm)
- GitHub Check: Test (ubuntu-latest, Node 18.x, npm)
- GitHub Check: Test (ubuntu-latest, Node 22.x, pnpm)
- GitHub Check: Test (ubuntu-latest, Node 20.x, npm)
- GitHub Check: Test (ubuntu-latest, Node 22.x, bun)
- GitHub Check: Test (ubuntu-latest, Node 18.x, pnpm)
- GitHub Check: Test (ubuntu-latest, Node 18.x, bun)
- GitHub Check: Coverage
🧰 Additional context used
📓 Path-based instructions (20)
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,swift,kt,rs,c,cpp,h,hpp,properties,yml,yaml,json,env,config}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
NEVER hardcode secrets in source code - ALWAYS use environment variables or a secret manager
Files:
package.jsontests/plugin-manifest.test.jstests/scripts/codex-hooks.test.jstests/scripts/npm-publish-surface.test.jsscripts/codex/check-plugin-cache.js
{package.json,*.config.js,scripts/**/*.js}
📄 CodeRabbit inference engine (CLAUDE.md)
Package manager detection should support npm, pnpm, yarn, and bun, with configuration via CLAUDE_PACKAGE_MANAGER environment variable or project config.
Files:
package.jsonscripts/codex/check-plugin-cache.js
**/*.{js,ts,jsx,tsx,json,env*}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not hardcode secrets, API keys, passwords, or tokens
Files:
package.jsontests/plugin-manifest.test.jstests/scripts/codex-hooks.test.jstests/scripts/npm-publish-surface.test.jsscripts/codex/check-plugin-cache.js
**/*.{js,ts,jsx,tsx,py,java,cs,go,rb,php,scala,kt}
📄 CodeRabbit inference engine (.cursor/rules/common-coding-style.md)
**/*.{js,ts,jsx,tsx,py,java,cs,go,rb,php,scala,kt}: Always create new objects, never mutate existing ones. Use immutable patterns to prevent hidden side effects and enable safe concurrency
Organize code into many small files (200-400 lines typical, 800 lines max) organized by feature/domain rather than by type
Always handle errors explicitly at every level and never silently swallow errors
Always validate all user input before processing at system boundaries
Use schema-based validation where available
Fail fast with clear error messages when validation fails
Never trust external data (API responses, user input, file content)
Ensure code is readable and well-named
Keep functions small (less than 50 lines)
Keep files focused (less than 800 lines)
Avoid deep nesting (more than 4 levels)
Do not use hardcoded values; use constants or configuration instead
Files:
tests/plugin-manifest.test.jstests/scripts/codex-hooks.test.jstests/scripts/npm-publish-surface.test.jsscripts/codex/check-plugin-cache.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,swift,kt,rs,c,cpp,h,hpp}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
No hardcoded secrets (API keys, passwords, tokens) - validate before any commit
Files:
tests/plugin-manifest.test.jstests/scripts/codex-hooks.test.jstests/scripts/npm-publish-surface.test.jsscripts/codex/check-plugin-cache.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php}: All user inputs must be validated
Enable CSRF protection on all state-changing endpoints
Verify authentication and authorization for all protected endpoints
Implement rate limiting on all endpoints to prevent abuse
Ensure error messages do not leak sensitive data in responses
Files:
tests/plugin-manifest.test.jstests/scripts/codex-hooks.test.jstests/scripts/npm-publish-surface.test.jsscripts/codex/check-plugin-cache.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,sql}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
Use parameterized queries to prevent SQL injection
Files:
tests/plugin-manifest.test.jstests/scripts/codex-hooks.test.jstests/scripts/npm-publish-surface.test.jsscripts/codex/check-plugin-cache.js
**/*.{js,ts,jsx,tsx,html,php,java,cs,rb,go}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
Implement XSS prevention by sanitizing HTML output
Files:
tests/plugin-manifest.test.jstests/scripts/codex-hooks.test.jstests/scripts/npm-publish-surface.test.jsscripts/codex/check-plugin-cache.js
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/typescript-coding-style.md)
**/*.{ts,tsx,js,jsx}: Use spread operator for immutable updates in TypeScript/JavaScript instead of direct mutation
Use async/await with try-catch for error handling in TypeScript/JavaScript
Use Zod for schema-based input validation in TypeScript/JavaScript
No console.log statements in production code; use proper logging libraries instead
**/*.{ts,tsx,js,jsx}: Auto-format JavaScript/TypeScript files using Prettier after edit
Warn aboutconsole.logstatements in edited files
Check all modified files forconsole.logstatements before session ends
**/*.{ts,tsx,js,jsx}: Use the ApiResponse interface pattern with generic type parameter:interface ApiResponse<T> { success: boolean; data?: T; error?: string; meta?: { total: number; page: number; limit: number; } }
Implement custom React hooks following the pattern: export a named function with use prefix, generic type parameters, and proper useEffect cleanup for side effects
**/*.{ts,tsx,js,jsx}: Never hardcode secrets; always use environment variables for sensitive credentials like API keys
Throw an error when required environment variables are not configured to fail fast and ensure security prerequisites are metUse Playwright as the E2E testing framework for critical user flows in TypeScript/JavaScript
Sanitize HTML output to prevent XSS (Cross-Site Scripting) attacks
Files:
tests/plugin-manifest.test.jstests/scripts/codex-hooks.test.jstests/scripts/npm-publish-surface.test.jsscripts/codex/check-plugin-cache.js
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{test,spec}.{js,ts,jsx,tsx}: Write tests before implementation (test-driven development); target 80%+ coverage
Achieve minimum 80% test coverage across all three layers: Unit, Integration, and E2E
Use AAA structure (Arrange / Act / Assert) in tests with descriptive test names that explain behavior under test
Files:
tests/plugin-manifest.test.jstests/scripts/codex-hooks.test.jstests/scripts/npm-publish-surface.test.js
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,ts,jsx,tsx}: Always create new objects and never mutate in place; return new copies instead
Keep files between 200–400 lines typical, with a maximum of 800 lines
Extract helpers when a file exceeds 200 lines
Handle errors explicitly at every level; never swallow errors silently
Validate all user input before processing; use schema-based validation where available
Never trust external data (API responses, file content, query params); always validate
All user inputs must be validated and sanitized
Error messages must be scrubbed of sensitive internals
Use readable, well-named identifiers in all code
Keep functions under 50 lines
Keep files under 800 lines
Avoid nesting deeper than 4 levels
Implement comprehensive error handling in all code
Do not hardcode values; use constants or environment configuration instead
Do not use in-place mutation; always return new objects or state
Files:
tests/plugin-manifest.test.jstests/scripts/codex-hooks.test.jstests/scripts/npm-publish-surface.test.jsscripts/codex/check-plugin-cache.js
**/*.{js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,ts}: Use parameterized queries for all database writes (no string interpolation)
Auth/authz must be checked server-side for every sensitive path
Rate limiting must be applied to all public endpoints
Files:
tests/plugin-manifest.test.jstests/scripts/codex-hooks.test.jstests/scripts/npm-publish-surface.test.jsscripts/codex/check-plugin-cache.js
**/*.{jsx,tsx,js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
HTML output must be sanitized where applicable
Files:
tests/plugin-manifest.test.jstests/scripts/codex-hooks.test.jstests/scripts/npm-publish-surface.test.jsscripts/codex/check-plugin-cache.js
**/*.{js,ts,env*}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Required environment variables must be validated at startup
Files:
tests/plugin-manifest.test.jstests/scripts/codex-hooks.test.jstests/scripts/npm-publish-surface.test.jsscripts/codex/check-plugin-cache.js
**/*.{ts,tsx,js,jsx,py,go,rs,kt,java,cpp,cc,cxx,c,fs}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx,py,go,rs,kt,java,cpp,cc,cxx,c,fs}: Write tests before implementation using the TDD workflow (RED → GREEN → IMPROVE), ensuring 80%+ code coverage
Always create new objects and never mutate existing ones; return new copies with changes applied
Keep functions small (<50 lines) and files focused (<800 lines, typical 200-400 lines)
Avoid deep nesting (>4 levels) in code
No hardcoded values in code; use configuration or constants with proper naming
Validate all user inputs at system boundaries using schema-based validation; fail fast with clear error messages
Never hardcode secrets (API keys, passwords, tokens); use environment variables or secret managers instead
Use parameterized queries to prevent SQL injection attacks
Enable CSRF (Cross-Site Request Forgery) protection on all endpoints
Verify authentication and authorization for all endpoints and sensitive operations
Implement rate limiting on all endpoints
Error messages must not leak sensitive data; provide user-friendly messages in UI code and detailed context in server-side logs
Handle errors at every level of the application; never silently swallow errors
Use well-named identifiers that are readable and self-documenting
Organize code by feature/domain, not by type; maintain high cohesion and low coupling
Files:
tests/plugin-manifest.test.jstests/scripts/codex-hooks.test.jstests/scripts/npm-publish-surface.test.jsscripts/codex/check-plugin-cache.js
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Write unit tests for individual functions, utilities, and components
Files:
tests/plugin-manifest.test.jstests/scripts/codex-hooks.test.jstests/scripts/npm-publish-surface.test.js
**/*.{ts,tsx,js,jsx,py,go,rs,kt,java}
📄 CodeRabbit inference engine (AGENTS.md)
Use consistent API response format with success indicator, data payload, error message, and pagination metadata
Files:
tests/plugin-manifest.test.jstests/scripts/codex-hooks.test.jstests/scripts/npm-publish-surface.test.jsscripts/codex/check-plugin-cache.js
README.md
📄 CodeRabbit inference engine (CLAUDE.md)
When working on README.md files, use the
/readmeskill.
Files:
README.md
scripts/**/*.js
📄 CodeRabbit inference engine (CLAUDE.md)
Ensure cross-platform support for Windows, macOS, and Linux via Node.js scripts in the scripts/ directory.
Files:
scripts/codex/check-plugin-cache.js
{scripts,bin}/**
⚙️ CodeRabbit configuration file
{scripts,bin}/**: Focus on command injection, unsafe subprocess usage, path traversal, SSRF, secret exposure, and missing tests for new CLI behavior.
Files:
scripts/codex/check-plugin-cache.js
🔇 Additional comments (8)
tests/scripts/codex-hooks.test.js (1)
14-14: LGTM!Also applies to: 86-196
.codex-plugin/README.md (1)
41-47: LGTM!README.md (1)
1393-1401: LGTM!package.json (1)
84-84: LGTM!plugins/ecc/README.md (1)
36-45: LGTM!tests/plugin-manifest.test.js (1)
466-466: LGTM!tests/scripts/npm-publish-surface.test.js (1)
71-71: LGTM!Also applies to: 144-144
scripts/codex/check-plugin-cache.js (1)
125-144: Do not require extendingcollectManifestRefsfor array-formskills/mcpServers.Repo manifest checks show no existing
plugin.jsonfiles useskillsormcpServersas arrays (jq found no entries where either field’s JSON type isarray), so the current string-only handling doesn’t appear to skip any real-world references here.> Likely an incorrect or invalid review comment.
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/scripts/codex-hooks.test.js (1)
131-148: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd test coverage for malicious manifest references with path traversal.
The current test suite validates CLI argument traversal rejection but does not test whether the script properly rejects manifest references that traverse outside the cache boundary. A test case with a manifest containing
"skills": "../../../../../etc/passwd"would verify that the script detects and fails such attempts.🧪 Proposed test case
+if ( + test('check-plugin-cache rejects manifest references that escape the cache boundary', () => { + const homeDir = createTempDir('codex-plugin-cache-traversal-manifest-home-'); + const codexDir = path.join(homeDir, '.codex'); + + try { + const maliciousManifest = { + name: 'ecc', + version: packageVersion, + skills: '../../../../../etc/passwd', + }; + seedPluginCache(codexDir, maliciousManifest); + const result = runNode(pluginCacheCheckScript, [], makeHermeticCodexEnv(homeDir, codexDir)); + + assert.strictEqual(result.status, 1, `${result.stdout}\n${result.stderr}`); + assert.match(result.stdout, /\[FAIL\] skills escapes cache boundary/); + } finally { + cleanup(homeDir); + } + }) +) + passed++; +else failed++; +🤖 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 `@tests/scripts/codex-hooks.test.js` around lines 131 - 148, Add a new test (or extend the existing test 'check-plugin-cache fails when the installed cache is missing manifest-referenced files') that seeds a plugin cache manifest containing a path-traversal reference (e.g., skills: "../../../../../etc/passwd") and then invokes runNode(pluginCacheCheckScript, [], makeHermeticCodexEnv(homeDir, codexDir)); assert the process exits with status 1 and that stdout/stderr contains the expected failure messages (e.g., "[FAIL] skills missing" or an explicit "path traversal" or "manifest reference outside cache" error) so the script is verified to reject manifest entries that traverse outside the cache; reuse createTempDir, seedPluginCache, cleanup and existing assertion patterns (assert.strictEqual, assert.match) from the test harness.scripts/codex/check-plugin-cache.js (1)
204-212:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPath traversal vulnerability in manifest reference validation.
The resolved
targetis not validated to remain withincacheDir. A malicious plugin manifest can include traversal sequences (e.g.,"skills": "../../../../../etc/passwd"), andpath.resolve(cacheDir, entry.ref)will resolve to an absolute path outside the cache. The script will then probe whether that external path exists, creating a filesystem oracle vulnerability and defeating the cache self-containment check.🔒 Proposed fix to validate boundary
log(`Manifest: ${manifestPath}`); for (const entry of refs) { const target = path.resolve(cacheDir, entry.ref); + const relative = path.relative(cacheDir, target); + if (relative.startsWith('..') || path.isAbsolute(relative)) { + failures += 1; + log(`[FAIL] ${entry.label} escapes cache boundary -> ${entry.ref}`); + continue; + } if (pathExists(target, entry.kind)) { log(`[OK] ${entry.label} -> ${target}`);🤖 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 `@scripts/codex/check-plugin-cache.js` around lines 204 - 212, The loop resolving plugin manifest refs uses path.resolve(cacheDir, entry.ref) to produce target but does not validate that target stays inside cacheDir, allowing path traversal; update the loop in which target is computed (the code using cacheDir, entry.ref and target) to validate the resolved path is contained in cacheDir before probing the filesystem: compute target as now, then compute const rel = path.relative(cacheDir, target) and treat the entry as invalid if rel.startsWith('..') or path.isAbsolute(rel) && rel.indexOf('..') === 0 (or simply rel === '' or !rel.startsWith('..') to allow only same-tree paths); log/mark a failure for traversal attempts instead of calling pathExists on the external path. This keeps the cache self-containment check and prevents filesystem oracle traversal.
🤖 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.
Outside diff comments:
In `@scripts/codex/check-plugin-cache.js`:
- Around line 204-212: The loop resolving plugin manifest refs uses
path.resolve(cacheDir, entry.ref) to produce target but does not validate that
target stays inside cacheDir, allowing path traversal; update the loop in which
target is computed (the code using cacheDir, entry.ref and target) to validate
the resolved path is contained in cacheDir before probing the filesystem:
compute target as now, then compute const rel = path.relative(cacheDir, target)
and treat the entry as invalid if rel.startsWith('..') or path.isAbsolute(rel)
&& rel.indexOf('..') === 0 (or simply rel === '' or !rel.startsWith('..') to
allow only same-tree paths); log/mark a failure for traversal attempts instead
of calling pathExists on the external path. This keeps the cache
self-containment check and prevents filesystem oracle traversal.
In `@tests/scripts/codex-hooks.test.js`:
- Around line 131-148: Add a new test (or extend the existing test
'check-plugin-cache fails when the installed cache is missing
manifest-referenced files') that seeds a plugin cache manifest containing a
path-traversal reference (e.g., skills: "../../../../../etc/passwd") and then
invokes runNode(pluginCacheCheckScript, [], makeHermeticCodexEnv(homeDir,
codexDir)); assert the process exits with status 1 and that stdout/stderr
contains the expected failure messages (e.g., "[FAIL] skills missing" or an
explicit "path traversal" or "manifest reference outside cache" error) so the
script is verified to reject manifest entries that traverse outside the cache;
reuse createTempDir, seedPluginCache, cleanup and existing assertion patterns
(assert.strictEqual, assert.match) from the test harness.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 946e314c-3d30-496a-af4d-489d79dceca6
📒 Files selected for processing (2)
scripts/codex/check-plugin-cache.jstests/scripts/codex-hooks.test.js
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (28)
- GitHub Check: Greptile Review
- GitHub Check: Test (macos-latest, Node 22.x, pnpm)
- GitHub Check: Test (macos-latest, Node 22.x, yarn)
- GitHub Check: Test (macos-latest, Node 20.x, bun)
- GitHub Check: Test (windows-latest, Node 22.x, pnpm)
- GitHub Check: Test (macos-latest, Node 18.x, yarn)
- GitHub Check: Test (windows-latest, Node 22.x, yarn)
- GitHub Check: Test (windows-latest, Node 22.x, npm)
- GitHub Check: Test (windows-latest, Node 18.x, npm)
- GitHub Check: Test (ubuntu-latest, Node 22.x, yarn)
- GitHub Check: Test (windows-latest, Node 20.x, pnpm)
- GitHub Check: Test (macos-latest, Node 18.x, npm)
- GitHub Check: Test (windows-latest, Node 18.x, pnpm)
- GitHub Check: Test (windows-latest, Node 20.x, yarn)
- GitHub Check: Test (windows-latest, Node 20.x, npm)
- GitHub Check: Test (windows-latest, Node 18.x, yarn)
- GitHub Check: Test (ubuntu-latest, Node 22.x, bun)
- GitHub Check: Test (ubuntu-latest, Node 22.x, pnpm)
- GitHub Check: Test (ubuntu-latest, Node 18.x, npm)
- GitHub Check: Test (ubuntu-latest, Node 20.x, pnpm)
- GitHub Check: Test (ubuntu-latest, Node 18.x, pnpm)
- GitHub Check: Test (ubuntu-latest, Node 18.x, yarn)
- GitHub Check: Test (ubuntu-latest, Node 18.x, bun)
- GitHub Check: Test (ubuntu-latest, Node 20.x, bun)
- GitHub Check: Test (ubuntu-latest, Node 20.x, npm)
- GitHub Check: Test (ubuntu-latest, Node 20.x, yarn)
- GitHub Check: Test (ubuntu-latest, Node 22.x, npm)
- GitHub Check: Coverage
🧰 Additional context used
📓 Path-based instructions (19)
**/*.{js,ts,jsx,tsx,py,java,cs,go,rb,php,scala,kt}
📄 CodeRabbit inference engine (.cursor/rules/common-coding-style.md)
**/*.{js,ts,jsx,tsx,py,java,cs,go,rb,php,scala,kt}: Always create new objects, never mutate existing ones. Use immutable patterns to prevent hidden side effects and enable safe concurrency
Organize code into many small files (200-400 lines typical, 800 lines max) organized by feature/domain rather than by type
Always handle errors explicitly at every level and never silently swallow errors
Always validate all user input before processing at system boundaries
Use schema-based validation where available
Fail fast with clear error messages when validation fails
Never trust external data (API responses, user input, file content)
Ensure code is readable and well-named
Keep functions small (less than 50 lines)
Keep files focused (less than 800 lines)
Avoid deep nesting (more than 4 levels)
Do not use hardcoded values; use constants or configuration instead
Files:
tests/scripts/codex-hooks.test.jsscripts/codex/check-plugin-cache.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,swift,kt,rs,c,cpp,h,hpp}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
No hardcoded secrets (API keys, passwords, tokens) - validate before any commit
Files:
tests/scripts/codex-hooks.test.jsscripts/codex/check-plugin-cache.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php}: All user inputs must be validated
Enable CSRF protection on all state-changing endpoints
Verify authentication and authorization for all protected endpoints
Implement rate limiting on all endpoints to prevent abuse
Ensure error messages do not leak sensitive data in responses
Files:
tests/scripts/codex-hooks.test.jsscripts/codex/check-plugin-cache.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,sql}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
Use parameterized queries to prevent SQL injection
Files:
tests/scripts/codex-hooks.test.jsscripts/codex/check-plugin-cache.js
**/*.{js,ts,jsx,tsx,html,php,java,cs,rb,go}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
Implement XSS prevention by sanitizing HTML output
Files:
tests/scripts/codex-hooks.test.jsscripts/codex/check-plugin-cache.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,swift,kt,rs,c,cpp,h,hpp,properties,yml,yaml,json,env,config}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
NEVER hardcode secrets in source code - ALWAYS use environment variables or a secret manager
Files:
tests/scripts/codex-hooks.test.jsscripts/codex/check-plugin-cache.js
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/typescript-coding-style.md)
**/*.{ts,tsx,js,jsx}: Use spread operator for immutable updates in TypeScript/JavaScript instead of direct mutation
Use async/await with try-catch for error handling in TypeScript/JavaScript
Use Zod for schema-based input validation in TypeScript/JavaScript
No console.log statements in production code; use proper logging libraries instead
**/*.{ts,tsx,js,jsx}: Auto-format JavaScript/TypeScript files using Prettier after edit
Warn aboutconsole.logstatements in edited files
Check all modified files forconsole.logstatements before session ends
**/*.{ts,tsx,js,jsx}: Use the ApiResponse interface pattern with generic type parameter:interface ApiResponse<T> { success: boolean; data?: T; error?: string; meta?: { total: number; page: number; limit: number; } }
Implement custom React hooks following the pattern: export a named function with use prefix, generic type parameters, and proper useEffect cleanup for side effects
**/*.{ts,tsx,js,jsx}: Never hardcode secrets; always use environment variables for sensitive credentials like API keys
Throw an error when required environment variables are not configured to fail fast and ensure security prerequisites are metUse Playwright as the E2E testing framework for critical user flows in TypeScript/JavaScript
Sanitize HTML output to prevent XSS (Cross-Site Scripting) attacks
Files:
tests/scripts/codex-hooks.test.jsscripts/codex/check-plugin-cache.js
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{test,spec}.{js,ts,jsx,tsx}: Write tests before implementation (test-driven development); target 80%+ coverage
Achieve minimum 80% test coverage across all three layers: Unit, Integration, and E2E
Use AAA structure (Arrange / Act / Assert) in tests with descriptive test names that explain behavior under test
Files:
tests/scripts/codex-hooks.test.js
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,ts,jsx,tsx}: Always create new objects and never mutate in place; return new copies instead
Keep files between 200–400 lines typical, with a maximum of 800 lines
Extract helpers when a file exceeds 200 lines
Handle errors explicitly at every level; never swallow errors silently
Validate all user input before processing; use schema-based validation where available
Never trust external data (API responses, file content, query params); always validate
All user inputs must be validated and sanitized
Error messages must be scrubbed of sensitive internals
Use readable, well-named identifiers in all code
Keep functions under 50 lines
Keep files under 800 lines
Avoid nesting deeper than 4 levels
Implement comprehensive error handling in all code
Do not hardcode values; use constants or environment configuration instead
Do not use in-place mutation; always return new objects or state
Files:
tests/scripts/codex-hooks.test.jsscripts/codex/check-plugin-cache.js
**/*.{js,ts,jsx,tsx,json,env*}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not hardcode secrets, API keys, passwords, or tokens
Files:
tests/scripts/codex-hooks.test.jsscripts/codex/check-plugin-cache.js
**/*.{js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,ts}: Use parameterized queries for all database writes (no string interpolation)
Auth/authz must be checked server-side for every sensitive path
Rate limiting must be applied to all public endpoints
Files:
tests/scripts/codex-hooks.test.jsscripts/codex/check-plugin-cache.js
**/*.{jsx,tsx,js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
HTML output must be sanitized where applicable
Files:
tests/scripts/codex-hooks.test.jsscripts/codex/check-plugin-cache.js
**/*.{js,ts,env*}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Required environment variables must be validated at startup
Files:
tests/scripts/codex-hooks.test.jsscripts/codex/check-plugin-cache.js
**/*.{ts,tsx,js,jsx,py,go,rs,kt,java,cpp,cc,cxx,c,fs}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx,py,go,rs,kt,java,cpp,cc,cxx,c,fs}: Write tests before implementation using the TDD workflow (RED → GREEN → IMPROVE), ensuring 80%+ code coverage
Always create new objects and never mutate existing ones; return new copies with changes applied
Keep functions small (<50 lines) and files focused (<800 lines, typical 200-400 lines)
Avoid deep nesting (>4 levels) in code
No hardcoded values in code; use configuration or constants with proper naming
Validate all user inputs at system boundaries using schema-based validation; fail fast with clear error messages
Never hardcode secrets (API keys, passwords, tokens); use environment variables or secret managers instead
Use parameterized queries to prevent SQL injection attacks
Enable CSRF (Cross-Site Request Forgery) protection on all endpoints
Verify authentication and authorization for all endpoints and sensitive operations
Implement rate limiting on all endpoints
Error messages must not leak sensitive data; provide user-friendly messages in UI code and detailed context in server-side logs
Handle errors at every level of the application; never silently swallow errors
Use well-named identifiers that are readable and self-documenting
Organize code by feature/domain, not by type; maintain high cohesion and low coupling
Files:
tests/scripts/codex-hooks.test.jsscripts/codex/check-plugin-cache.js
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Write unit tests for individual functions, utilities, and components
Files:
tests/scripts/codex-hooks.test.js
**/*.{ts,tsx,js,jsx,py,go,rs,kt,java}
📄 CodeRabbit inference engine (AGENTS.md)
Use consistent API response format with success indicator, data payload, error message, and pagination metadata
Files:
tests/scripts/codex-hooks.test.jsscripts/codex/check-plugin-cache.js
{package.json,*.config.js,scripts/**/*.js}
📄 CodeRabbit inference engine (CLAUDE.md)
Package manager detection should support npm, pnpm, yarn, and bun, with configuration via CLAUDE_PACKAGE_MANAGER environment variable or project config.
Files:
scripts/codex/check-plugin-cache.js
scripts/**/*.js
📄 CodeRabbit inference engine (CLAUDE.md)
Ensure cross-platform support for Windows, macOS, and Linux via Node.js scripts in the scripts/ directory.
Files:
scripts/codex/check-plugin-cache.js
{scripts,bin}/**
⚙️ CodeRabbit configuration file
{scripts,bin}/**: Focus on command injection, unsafe subprocess usage, path traversal, SSRF, secret exposure, and missing tests for new CLI behavior.
Files:
scripts/codex/check-plugin-cache.js
🔇 Additional comments (12)
scripts/codex/check-plugin-cache.js (4)
30-44: LGTM!
46-93: LGTM!
188-193: LGTM!
221-227: LGTM!tests/scripts/codex-hooks.test.js (8)
18-19: LGTM!
88-103: LGTM!
106-125: LGTM!
131-148: LGTM!
154-175: LGTM!
181-194: LGTM!
199-219: LGTM!
221-242: LGTM!
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
2 participants
What Changed
scripts/codex/check-plugin-cache.jsto verify the installed plugin cache resolves the manifest's skills, MCP config, and asset references.codex plugin listis only a registration check.Why This Change
Closes #2247.
A marketplace install can report
ecc@eccas installed/enabled even when the installed cache does not contain the files referenced by.codex-plugin/plugin.json. This keeps the current single-source plugin layout intact while giving users a deterministic post-install check and a clear path back to the supported sync flow when cache references are missing.Testing Done
node tests/run-all.js)Commands run:
node scripts/codex/check-plugin-cache.js --plugin-dir plugins/eccnode tests/scripts/codex-hooks.test.jsnode tests/plugin-manifest.test.jsnode tests/scripts/npm-publish-surface.test.jsnpx eslint scripts/codex/check-plugin-cache.js tests/scripts/codex-hooks.test.js tests/scripts/npm-publish-surface.test.js tests/plugin-manifest.test.jsgit diff --checknode tests/run-all.js(2738 passed, 0 failed)Type of Change
fix:Bug fixfeat:New featurerefactor:Code refactoringdocs:Documentationtest:Testschore:Maintenance/toolingci:CI/CD changesSecurity & Quality Checklist
Documentation