Fix security code scanning alerts#9
Merged
Conversation
📝 WalkthroughWalkthroughAdds security hardening and abstractions: FileSystem interface extension (mkdtemp), input validation and path-safety in the git adapter with an injectable FileSystem, ESLint security rules and restricted imports (with an override), CLI parsing/dispatch refactor, SECURITY.md triage guidance, new test helpers and tests for git behavior, and minor utility refactors. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/adapters/git.ts (1)
101-115:⚠️ Potential issue | 🟠 MajorReject invalid
subPathbefore allocating the worktree.Lines 101-115 only run the traversal check after
mkdtemp()and the external worktree setup.src/core/install.tspassesskill.pathstraight intoexportPath(), so a manifest value like../escapestill triggers side effects before we reject it. Move thesubPathvalidation ahead of any filesystem/process work, then fliptests/adapters.git.test.tsto assert nomkdtemp()or worktree calls on invalid paths.Suggested fix
+function assertSafeSubPath(subPath?: string): void { + if (!subPath || subPath === '.') { + return; + } + + if (path.isAbsolute(subPath)) { + throw new SkillInstallError(`Invalid skill path ${subPath}`); + } + + const normalized = path.normalize(subPath); + if (normalized === '..' || normalized.startsWith(`..${path.sep}`)) { + throw new SkillInstallError(`Invalid skill path ${subPath}`); + } +} + function resolveSafeSubPath(baseDir: string, subPath?: string): string { - if (!subPath || subPath === '.') { - return baseDir; - } - - if (path.isAbsolute(subPath)) { - throw new SkillInstallError(`Invalid skill path ${subPath}`); - } + assertSafeSubPath(subPath); + + if (!subPath || subPath === '.') { + return baseDir; + } const normalized = path.resolve(baseDir, subPath); const relativeToBase = path.relative(baseDir, normalized); - if (relativeToBase.startsWith('..') || path.isAbsolute(relativeToBase)) { + if (relativeToBase === '..' || relativeToBase.startsWith(`..${path.sep}`) || path.isAbsolute(relativeToBase)) { throw new SkillInstallError(`Invalid skill path ${subPath}`); } @@ async exportPath(repoPath: string, commit: string, destination: string, subPath?: string): Promise<void> { assertSafeGitArg(repoPath, 'repository path'); assertSafeGitArg(destination, 'destination path'); + assertSafeSubPath(subPath); if (!COMMIT_SHA_PATTERN.test(commit)) { throw new SkillInstallError(`Invalid commit SHA: ${commit}`); } const worktreeDir = await this.fs.mkdtemp(path.join(os.tmpdir(), 'skilleton-wt-'));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/adapters/git.ts` around lines 101 - 115, Validate the requested subPath (reject path traversal like "../...") before creating the temp worktree: call your safe path check (e.g., resolveSafeSubPath or a dedicated validateSubPath function using repoPath and subPath) and throw the SkillInstallError if invalid prior to calling this.fs.mkdtemp or running execa('git', ['worktree', ...]) so no temp dir or git worktree is allocated for bad inputs; then update tests/adapters.git.test.ts to expect no mkdtemp or git worktree calls when passing an invalid manifest path.
🧹 Nitpick comments (2)
eslint.config.cjs (1)
27-41: Consider also restricting the barefsimport.The rule blocks
node:fsandnode:fs/promisesbut not the legacyfsimport (without thenode:prefix). While modern code typically uses the prefixed form, addingfsandfs/promiseswould ensure complete coverage.♻️ Proposed addition to cover legacy imports
'no-restricted-imports': [ 'error', { paths: [ { name: 'node:fs', message: FILESYSTEM_IMPORT_MESSAGE, }, { name: 'node:fs/promises', message: FILESYSTEM_IMPORT_MESSAGE, }, + { + name: 'fs', + message: FILESYSTEM_IMPORT_MESSAGE, + }, + { + name: 'fs/promises', + message: FILESYSTEM_IMPORT_MESSAGE, + }, ], }, ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eslint.config.cjs` around lines 27 - 41, The no-restricted-imports rule currently blocks 'node:fs' and 'node:fs/promises' but not their legacy counterparts; update the 'no-restricted-imports' configuration (the rule named no-restricted-imports) to also include 'fs' and 'fs/promises' entries using the same FILESYSTEM_IMPORT_MESSAGE so legacy imports are covered alongside 'node:fs' and 'node:fs/promises'.src/core/filesystem.ts (1)
33-35: Add JSDoc comment for consistency with other methods.Other methods in
NodeFileSystemhave JSDoc comments documenting their purpose and parameters. Consider adding one here for consistency.📝 Proposed JSDoc addition
+ /** + * Creates a unique temporary directory. + * `@param` prefix Directory path prefix (must include trailing path separator if targeting a specific directory). + */ async mkdtemp(prefix: string): Promise<string> { return fs.mkdtemp(prefix); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/filesystem.ts` around lines 33 - 35, Add a JSDoc block for the NodeFileSystem.mkdtemp method to match other methods: document the purpose (create a new temporary directory), describe the prefix parameter (string used to name the temp dir), and the return type (Promise<string> resolving to the created directory path). Place the comment immediately above the async mkdtemp(prefix: string): Promise<string> method so it follows the same JSDoc style and tags used by other NodeFileSystem methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/describe.test.ts`:
- Around line 277-281: The in-memory mkdtemp implementation always returns the
same path for a given prefix; update mkdtemp to produce a unique directory per
call (matching FileSystem.mkdtemp semantics) by appending a unique suffix (e.g.,
a random string or incrementing counter) to `${prefix}tmp` before calling
this.normalize, then add that unique path to this.directories and return it;
modify the mkdtemp function (references: mkdtemp, this.normalize,
this.directories) so repeated calls with the same prefix yield different
directory paths.
---
Outside diff comments:
In `@src/adapters/git.ts`:
- Around line 101-115: Validate the requested subPath (reject path traversal
like "../...") before creating the temp worktree: call your safe path check
(e.g., resolveSafeSubPath or a dedicated validateSubPath function using repoPath
and subPath) and throw the SkillInstallError if invalid prior to calling
this.fs.mkdtemp or running execa('git', ['worktree', ...]) so no temp dir or git
worktree is allocated for bad inputs; then update tests/adapters.git.test.ts to
expect no mkdtemp or git worktree calls when passing an invalid manifest path.
---
Nitpick comments:
In `@eslint.config.cjs`:
- Around line 27-41: The no-restricted-imports rule currently blocks 'node:fs'
and 'node:fs/promises' but not their legacy counterparts; update the
'no-restricted-imports' configuration (the rule named no-restricted-imports) to
also include 'fs' and 'fs/promises' entries using the same
FILESYSTEM_IMPORT_MESSAGE so legacy imports are covered alongside 'node:fs' and
'node:fs/promises'.
In `@src/core/filesystem.ts`:
- Around line 33-35: Add a JSDoc block for the NodeFileSystem.mkdtemp method to
match other methods: document the purpose (create a new temporary directory),
describe the prefix parameter (string used to name the temp dir), and the return
type (Promise<string> resolving to the created directory path). Place the
comment immediately above the async mkdtemp(prefix: string): Promise<string>
method so it follows the same JSDoc style and tags used by other NodeFileSystem
methods.
🪄 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: 0d306203-5979-48dc-a1a8-b18b6070d2fb
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
SECURITY.mdbin/skilleton.tseslint.config.cjspackage.jsonsrc/adapters/git.tssrc/commands/describe.tssrc/core/filesystem.tssrc/core/lock.tssrc/core/repos.tssrc/core/types.tstests/adapters.git.test.tstests/describe.test.tstests/install.test.ts
…age config - Move InMemoryFileSystem from describe.test.ts to tests/helpers/mocked-filesystem.ts - Rename InMemoryFileSystem to MockedFileSystem with jest.fn() methods - Add createMockedFileSystem() factory function - Update all tests to use shared MockedFileSystem helper - Implement missing FileSystem methods (readJson, writeJson, remove, copy, symlink) - Configure Jest to collect coverage from src/**/*.ts files only
- Add test for fetching existing repositories instead of cloning - Add test for rejecting absolute subPath values before worktree allocation - Add test for successful path export with worktree cleanup - Add test for wrapping missing source path errors while ensuring worktree cleanup
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/helpers/mocked-filesystem.ts (1)
117-121: Alignsymlinkmock with production replacement semantics.Line 117–121 does not remove an existing destination before creating the mock symlink entry. Matching production behavior will make symlink-related tests more faithful.
Proposed refactor
symlink = jest.fn(async (_target: string, dest: string): Promise<void> => { const normalizedDest = this.normalize(dest); + if (await this.pathExists(normalizedDest)) { + await this.remove(normalizedDest); + } this.directories.add(path.dirname(normalizedDest)); this.files.set(normalizedDest, ''); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/helpers/mocked-filesystem.ts` around lines 117 - 121, The mock symlink implementation (symlink) should mirror production semantics by removing any existing entry at the normalized destination before creating the new symlink; update the async symlink mock to compute normalizedDest via this.normalize(dest), then if an existing file or symlink exists at normalizedDest delete/unset it (e.g., remove from this.files and any metadata) before adding the new entry and ensuring this.directories includes path.dirname(normalizedDest); refer to the symlink function and this.files/this.directories/this.normalize symbols when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/helpers/mocked-filesystem.ts`:
- Around line 13-16: The mocked addDirectory implementation only adds the target
path and not its ancestor directories, diverging from real
NodeFileSystem.ensureDir; update addDirectory (and any similar methods) to
iterate up the directory path produced by normalize and add each ancestor
directory into this.directories so that all parent directories exist (e.g., walk
parents from the normalized path to root and this.directories.add(...) for
each), ensuring test filesystem state mirrors recursive directory creation.
- Around line 148-150: The readDir implementation incorrectly only throws when a
directory is missing and entries.size === 0, letting calls succeed for
non-existent directories; update the check in readDir to throw whenever
this.directories does not contain the normalized path (i.e., remove the
entries.size condition) so any missing directory (normalized/target) always
raises an Error; reference variables/functions: readDir, this.directories,
normalized, target, entries.
---
Nitpick comments:
In `@tests/helpers/mocked-filesystem.ts`:
- Around line 117-121: The mock symlink implementation (symlink) should mirror
production semantics by removing any existing entry at the normalized
destination before creating the new symlink; update the async symlink mock to
compute normalizedDest via this.normalize(dest), then if an existing file or
symlink exists at normalizedDest delete/unset it (e.g., remove from this.files
and any metadata) before adding the new entry and ensuring this.directories
includes path.dirname(normalizedDest); refer to the symlink function and
this.files/this.directories/this.normalize symbols when making the change.
🪄 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: a8625982-ab37-4aa7-904d-ac343b6b18c3
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
eslint.config.cjsjest.config.cjspackage.jsonsrc/adapters/git.tssrc/core/filesystem.tstests/adapters.git.test.tstests/describe.test.tstests/helpers/mocked-filesystem.tstests/install.test.ts
✅ Files skipped from review due to trivial changes (2)
- tests/install.test.ts
- package.json
🚧 Files skipped from review as they are similar to previous changes (4)
- eslint.config.cjs
- tests/describe.test.ts
- tests/adapters.git.test.ts
- src/adapters/git.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@SECURITY.md`:
- Around line 7-10: Update SECURITY.md to point to the correct security advisory
URL and make the version recommendation consistent with package.json: verify the
advisory URL (replace the broken
https://github.com/Fcmam5/skilleton/security/advisories/GHSA-5g3j-89fr-r2vp with
the correct advisory link) and update the version text from `0.3.1` to the
actual released version (`0.3.0`) or, if the intent is to require `0.3.1`, bump
the version in package.json accordingly; ensure the table rows and the sentence
about upgrading use the same version string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+7
to
+10
| | >=0.3.1 | ✅ | | ||
| | <0.3.1 | ❌ (deprecated) | | ||
|
|
||
| Skilleton CLI is currently pre-1.0, but we treat every tagged release as security-supported until a newer patch is published. | ||
| Versions earlier than `0.3.1` are deprecated due to security fixes (see the [Security Advisory](https://github.com/Fcmam5/skilleton/security/advisories/GHSA-5g3j-89fr-r2vp)). Please upgrade to `0.3.1` or later. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify security advisory URL and version 0.3.1 in package.json
# Check if the security advisory URL is accessible
echo "Checking security advisory URL..."
curl -s -o /dev/null -w "%{http_code}" https://github.com/Fcmam5/skilleton/security/advisories/GHSA-5g3j-89fr-r2vp
# Check if version 0.3.1 exists in package.json
echo -e "\nChecking version in package.json..."
if [ -f package.json ]; then
cat package.json | jq -r '.version'
else
echo "package.json not found"
fi
# Search for version references in the codebase
echo -e "\nSearching for version 0.3.1 references..."
rg -n "0\.3\.1" --type=jsonRepository: Fcmam5/skilleton
Length of output: 540
Fix broken security advisory link and version mismatch in SECURITY.md.
Lines 7-10 reference a security advisory URL that returns a 404 error (GHSA-5g3j-89fr-r2vp), and the documentation recommends upgrading to version 0.3.1, but the current version in package.json is 0.3.0. Verify that the advisory URL is correct and update the version reference to match the actual released version.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@SECURITY.md` around lines 7 - 10, Update SECURITY.md to point to the correct
security advisory URL and make the version recommendation consistent with
package.json: verify the advisory URL (replace the broken
https://github.com/Fcmam5/skilleton/security/advisories/GHSA-5g3j-89fr-r2vp with
the correct advisory link) and update the version text from `0.3.1` to the
actual released version (`0.3.0`) or, if the intent is to require `0.3.1`, bump
the version in package.json accordingly; ensure the table rows and the sentence
about upgrading use the same version string.
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.
Fix
appeared in https://github.com/Fcmam5/skilleton/actions/runs/24004017072
Summary by CodeRabbit
Bug Fixes
Documentation
Refactor
Tests
Chores