tests(#478): add cli/path-helpers.js (traversal guards, slug, bundled-plugin paths) with 46 unit tests#329
Open
javimosch wants to merge 4 commits into
Open
tests(#478): add cli/path-helpers.js (traversal guards, slug, bundled-plugin paths) with 46 unit tests#329javimosch wants to merge 4 commits into
javimosch wants to merge 4 commits into
Conversation
Export diffCatalogs, readLocalCatalog, writeLocalCatalog, downloadTarball, and extractPluginsFromTarball for direct testing. Test coverage: - diffCatalogs (7): null/empty local → all added, identical checksums → unchanged, stale checksum → changed, mixed add+change+unchanged - readLocalCatalog (6): missing file, valid JSON, invalid JSON, null/non-object parsed value, fs.readFileSync throws - writeLocalCatalog (4): creates dir when missing, skips mkdirSync when dir exists, writes to correct path, trailing newline - downloadTarball (5): spawnSync error, non-zero exit, missing archive, zero-byte archive, success path - extractPluginsFromTarball (7): creates destDir, correct tar args, tolerates exit 2, throws on exit 1, throws on spawnSync error, batches >200 into 2 calls, 200 uses 1 call - updatePlugins (8): check=true skips download, up-to-date skips download, force=true ignores local catalog, stale catalog triggers full update+write, tmpDir cleaned via finally even on error, remote_count in result, fetch failure throws, invalid catalog format throws Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Covers all four exported functions: - handleCommandsQuery: guard clauses (null config, invalid limit/offset), namespace/resource/action/query filters, pagination (limit, offset, auto-limit), _warning for truncated auto-limit, human mode (outputHumanTable), args * formatting, and exception catch → outputError code 110 - handleInspect: positional length guard, unknown command (code 92), full spec structure (side_effects, risk_level, input_schema required array, defaults), human mode console.log path - handleSchema: input_schema property/required mapping, type defaulting, output_schema default and override, empty-args edge case - handleNamespaceBrowse: namespace browse (1 positional), resource browse (2 positionals), unknown namespace/resource errors (code 92), human mode All 50 tests pass in 0.265s. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Create cli/env.js to centralise all SUPERCLI_* and OPENAI_* env-var reads
with explicit fallback defaults and a clear override-precedence contract.
Getters:
- getSupercliHome() → SUPERCLI_HOME || ~/.supercli
- getServer() → SUPERCLI_SERVER || null
- getApiKey() → SUPERCLI_API_KEY || null
- getClientId() → SUPERCLI_CLIENT_ID trimmed || null (whitespace-only → null)
- getOpenAiBaseUrl() → OPENAI_BASE_URL || null (doubles as hasLocalLLM gate)
- getOpenAiModel() → OPENAI_MODEL || "gpt-3.5-turbo"
- getOpenAiApiKey() → OPENAI_API_KEY || "dummy"
- interpolateEnvPlaceholders(text) → expands ${VARNAME} in strings
__tests__/env.test.js: 41 tests covering env-set, env-unset, empty-string
fallback, whitespace-trim, truthy gating, non-string pass-through, adjacent
and repeated placeholders, dollar-sign-no-braces not expanded, and unknown
variable → empty string.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ersal guards and slug utility Introduce cli/path-helpers.js centralising four repeated path utilities: - isPathContained(candidate, base) — traversal guard used identically in plugins-manifest.js, plugins-install.js, and plugins-learn.js - resolveContained(base, rel) — resolve + traversal check, returns null on escape - resolveBundledPluginDir / resolveBundledManifest — bundled plugin path helpers extracted from plugins-learn.js / plugins-manifest.js - slugify(str) — lowercase-and-hyphen safe name for plugin/namespace identifiers 46 tests across 5 describe blocks; all pass in 0.196 s.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughAdds three new CLI utility modules ( ChangesCLI Modules and Test Coverage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
Owner
Author
|
Review — Head of Org Engineering: The PR adds a new test file (approved — review-only mode; merge when ready.) |
![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 `@cli/path-helpers.js`:
- Around line 43-45: The bundled plugin path helpers are allowing path traversal
because resolveBundledPluginDir() and resolveBundledManifest() interpolate
pluginName directly into the filesystem path. Update these helpers to either
validate pluginName up front by rejecting separators and “..” segments or
construct the final path through resolveContained() using the fixed plugins
root, so both helpers always stay under the bundled plugins directory.
- Around line 15-18: The containment check in isPathContained is only lexical
and can be bypassed through symlinked parents, so callers may still escape the
intended base directory. Update this helper to validate using filesystem-aware
paths at the boundary, such as resolving real paths for both candidate and base
before comparing, or explicitly rejecting any symlinked parent components before
use. Make sure the fix stays anchored in isPathContained and preserves its
existing callers’ expectations while closing the traversal gap.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1f8dc05e-303c-44a0-a56b-f3e488402cd7
📒 Files selected for processing (7)
__tests__/commands-handler.test.js__tests__/env.test.js__tests__/path-helpers.test.js__tests__/plugins-update.test.jscli/env.jscli/path-helpers.jscli/plugins-update.js
Comment on lines
+15
to
+18
| function isPathContained(candidate, base) { | ||
| const normalizedBase = path.resolve(base) + path.sep | ||
| const normalizedCandidate = path.resolve(candidate) | ||
| return normalizedCandidate.startsWith(normalizedBase) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
This containment check is not symlink-safe.
path.resolve() + startsWith() only normalizes lexical .. segments. A path like <base>/link/file still passes when link is a symlink to a directory outside base, so any caller relying on this as the traversal guard can still read or write past base. Compare real paths at the filesystem boundary, or reject symlinked parents before use.
🤖 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 `@cli/path-helpers.js` around lines 15 - 18, The containment check in
isPathContained is only lexical and can be bypassed through symlinked parents,
so callers may still escape the intended base directory. Update this helper to
validate using filesystem-aware paths at the boundary, such as resolving real
paths for both candidate and base before comparing, or explicitly rejecting any
symlinked parent components before use. Make sure the fix stays anchored in
isPathContained and preserves its existing callers’ expectations while closing
the traversal gap.
Comment on lines
+43
to
+45
| function resolveBundledPluginDir(pluginName) { | ||
| return path.resolve(__dirname, "..", "plugins", pluginName) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject traversal in bundled plugin names.
Both helpers resolve pluginName straight into the path, so inputs like "../../outside" escape <repo-root>/plugins, and resolveBundledManifest() inherits the same escape. Build these from the fixed plugins root with resolveContained() or reject any separator / .. in pluginName before returning them.
Also applies to: 54-55
🤖 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 `@cli/path-helpers.js` around lines 43 - 45, The bundled plugin path helpers
are allowing path traversal because resolveBundledPluginDir() and
resolveBundledManifest() interpolate pluginName directly into the filesystem
path. Update these helpers to either validate pluginName up front by rejecting
separators and “..” segments or construct the final path through
resolveContained() using the fixed plugins root, so both helpers always stay
under the bundled plugins directory.
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.
What changed
Introduces
cli/path-helpers.js— a new utility module with five pure functions:isPathContained(candidate, base)truewhen the resolvedcandidatepath lives insidebase(path-traversal guard)resolveContained(base, rel)relrelative tobase, returnsnullif the result would escapebaseresolveBundledPluginDir(pluginName)<repo-root>/plugins/<name>for bundled pluginsresolveBundledManifest(pluginName)<repo-root>/plugins/<name>/plugin.jsonslugify(str)Why
The path-traversal guard pattern:
is duplicated verbatim in at least three files:
cli/plugins-manifest.js(resolveRepoManifestPath)cli/plugins-install.js(resolveHookScriptPath)cli/plugins-learn.js(resolveLearnMarkdown,loadManifestFromGit)Centralising the logic in
isPathContained/resolveContainedremoves the duplication and gives each usage a named, testable primitive.Verification
46 tests across 5 describe blocks (
isPathContained× 10,resolveContained× 10,resolveBundledPluginDir× 5,resolveBundledManifest× 5,slugify× 16).Closes #478 (mago task #478)
Summary by CodeRabbit