From 42bc280ad675bfaa7b1bbc192330fb582bb28172 Mon Sep 17 00:00:00 2001 From: Abdeldjalil Fortas <9090674+Fcmam5@users.noreply.github.com> Date: Sun, 5 Apr 2026 22:21:43 +0200 Subject: [PATCH 01/11] fix(security): harden git arg handling and path validation Resolves: - https://github.com/Fcmam5/skilleton/security/code-scanning/1 - https://github.com/Fcmam5/skilleton/security/code-scanning/2 --- SECURITY.md | 18 ++++++++ src/adapters/git.ts | 50 +++++++++++++++++++-- tests/adapters.git.test.ts | 90 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 154 insertions(+), 4 deletions(-) create mode 100644 tests/adapters.git.test.ts diff --git a/SECURITY.md b/SECURITY.md index 77c9564..7c4bca0 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -35,6 +35,24 @@ Instead, email `au54vz9rk[at]mozmail[.]com` with the details. If you do not rece We may invite you to test the fix before release if appropriate. +### Automated Alert Triage (GitHub) + +We also monitor and triage GitHub security signals (CodeQL and Dependabot alerts). + +- **Initial triage SLA**: within 7 days of an alert opening +- **Critical/High**: patch or documented mitigation target within 14 days +- **Medium**: patch target within 30 days +- **Low**: patch target within 90 days or explicitly risk-accepted with rationale + +For each alert, we track: + +- affected file/function and exploit preconditions +- real impact in Skilleton runtime/CLI context +- patch approach, owner, and target release +- verification (tests and/or reproduction before/after) + +If an alert is a false positive or accepted risk, we record the reason and keep the decision reviewable. + ## Scope This policy covers: diff --git a/src/adapters/git.ts b/src/adapters/git.ts index 9362406..83ed7a6 100644 --- a/src/adapters/git.ts +++ b/src/adapters/git.ts @@ -7,6 +7,10 @@ import { getCacheRoot } from '../core/config'; import { ensureRepoUrl, repoCacheKey } from '../core/repos'; import { SkillInstallError } from '../core/errors'; +// Reference: https://git-scm.com/book/en/v2/Git-Tools-Revision-Selection +// Pattern to match a 40-character hexadecimal SHA-1 hash +const COMMIT_SHA_PATTERN = /^[0-9a-f]{40}$/i; + async function pathExists(target: string): Promise { try { await fs.access(target); @@ -16,6 +20,34 @@ async function pathExists(target: string): Promise { } } +// Rejects values that could be interpreted as git options or null bytes +// This is a security measure to prevent command injection when passing values to git commands as arguments +function assertSafeGitArg(value: string, label: string): void { + if (!value || value.includes('\0') || value.startsWith('-')) { + throw new SkillInstallError(`Invalid ${label}`); + } +} + +// Resolves a sub-path relative to a base directory, ensuring it stays within the base. +// Rejects absolute paths and paths that would escape the base directory. +function resolveSafeSubPath(baseDir: string, subPath?: string): string { + if (!subPath || subPath === '.') { + return baseDir; + } + + if (path.isAbsolute(subPath)) { + throw new SkillInstallError(`Invalid skill path ${subPath}`); + } + + const normalized = path.resolve(baseDir, subPath); + const relativeToBase = path.relative(baseDir, normalized); + if (relativeToBase.startsWith('..') || path.isAbsolute(relativeToBase)) { + throw new SkillInstallError(`Invalid skill path ${subPath}`); + } + + return normalized; +} + /** Git client implementation backed by `execa` shell commands. */ export class ExecaGitClient implements GitClient { constructor(private readonly cacheRoot = getCacheRoot()) {} @@ -31,9 +63,14 @@ export class ExecaGitClient implements GitClient { const cloneSource = `${repoUrl}.git`.replace(/\.git\.git$/i, '.git'); const gitEnv = { ...process.env, GIT_TERMINAL_PROMPT: '0' }; + // Security: Validate paths to prevent command injection + assertSafeGitArg(repoCachePath, 'repository cache path'); + assertSafeGitArg(cloneSource, 'repository URL'); + if (!(await pathExists(repoCachePath))) { await fs.mkdir(path.dirname(repoCachePath), { recursive: true }); - await execa('git', ['clone', '--filter=blob:none', cloneSource, repoCachePath], { + // Security: Validate paths to prevent command injection + await execa('git', ['clone', '--filter=blob:none', '--', cloneSource, repoCachePath], { timeout: 30000, env: gitEnv, stdin: 'ignore', @@ -61,6 +98,12 @@ export class ExecaGitClient implements GitClient { * @param subPath Optional subdirectory within the repository. */ async exportPath(repoPath: string, commit: string, destination: string, subPath?: string): Promise { + assertSafeGitArg(repoPath, 'repository path'); + assertSafeGitArg(destination, 'destination path'); + if (!COMMIT_SHA_PATTERN.test(commit)) { + throw new SkillInstallError(`Invalid commit SHA: ${commit}`); + } + const worktreeDir = await fs.mkdtemp(path.join(os.tmpdir(), 'skilleton-wt-')); const gitEnv = { ...process.env, GIT_TERMINAL_PROMPT: '0' }; @@ -73,10 +116,9 @@ export class ExecaGitClient implements GitClient { stderr: 'pipe', }); - const relative = subPath && subPath !== '.' ? subPath : undefined; - const sourcePath = relative ? path.join(worktreeDir, relative) : worktreeDir; + const sourcePath = resolveSafeSubPath(worktreeDir, subPath); if (!(await pathExists(sourcePath))) { - throw new SkillInstallError(`Skill path ${relative ?? '.'} not found in repository ${repoPath}`); + throw new SkillInstallError(`Skill path ${subPath ?? '.'} not found in repository ${repoPath}`); } await fs.rm(destination, { recursive: true, force: true }); diff --git a/tests/adapters.git.test.ts b/tests/adapters.git.test.ts new file mode 100644 index 0000000..e39e3a8 --- /dev/null +++ b/tests/adapters.git.test.ts @@ -0,0 +1,90 @@ +import { promises as fs } from 'node:fs'; +import { execa } from 'execa'; +import { ExecaGitClient } from '../src/adapters/git'; +import { SkillInstallError } from '../src/core/errors'; + +jest.mock('execa'); +jest.mock('node:fs', () => ({ + promises: { + access: jest.fn(), + mkdir: jest.fn(), + rm: jest.fn(), + cp: jest.fn(), + mkdtemp: jest.fn(), + }, +})); + +describe('ExecaGitClient security hardening', () => { + const mockedFs = fs as unknown as { + access: jest.Mock; + mkdir: jest.Mock; + rm: jest.Mock; + cp: jest.Mock; + mkdtemp: jest.Mock; + }; + const mockedExeca = execa as unknown as jest.Mock; + + beforeEach(() => { + jest.clearAllMocks(); + mockedFs.access.mockResolvedValue(undefined); + mockedFs.mkdir.mockResolvedValue(undefined); + mockedFs.rm.mockResolvedValue(undefined); + mockedFs.cp.mockResolvedValue(undefined); + mockedFs.mkdtemp.mockResolvedValue('/tmp/skilleton-wt-123'); + mockedExeca.mockResolvedValue({ stdout: '', stderr: '' }); + }); + + it('uses -- separator for git clone arguments', async () => { + mockedFs.access.mockRejectedValueOnce(new Error('missing')); + const client = new ExecaGitClient('/tmp/cache'); + + await client.ensureRepo('https://github.com/acme/skills'); + + expect(mockedExeca).toHaveBeenCalledWith( + 'git', + ['clone', '--filter=blob:none', '--', 'https://github.com/acme/skills.git', '/tmp/cache/https_github.com_acme_skills'], + expect.any(Object), + ); + }); + + it('rejects destination values that look like git options', async () => { + const client = new ExecaGitClient('/tmp/cache'); + + await expect(client.ensureRepo('https://github.com/acme/skills', '--upload-pack=sh')).rejects.toThrow(SkillInstallError); + await expect(client.ensureRepo('https://github.com/acme/skills', '--upload-pack=sh')).rejects.toThrow( + 'Invalid repository cache path', + ); + expect(mockedExeca).not.toHaveBeenCalled(); + }); + + it('rejects non-SHA commit inputs before running git worktree', async () => { + const client = new ExecaGitClient('/tmp/cache'); + + await expect(client.exportPath('/tmp/cache/repo', 'main', '/tmp/out', 'skills/jest')).rejects.toThrow(SkillInstallError); + await expect(client.exportPath('/tmp/cache/repo', 'main', '/tmp/out', 'skills/jest')).rejects.toThrow( + 'Invalid commit SHA: main', + ); + expect(mockedExeca).not.toHaveBeenCalled(); + expect(mockedFs.mkdtemp).not.toHaveBeenCalled(); + }); + + it('rejects path traversal in subPath', async () => { + const client = new ExecaGitClient('/tmp/cache'); + const commit = 'abcdef1234567890abcdef1234567890abcdef12'; + + await expect(client.exportPath('/tmp/cache/repo', commit, '/tmp/out', '../escape')).rejects.toThrow( + 'Invalid skill path ../escape', + ); + + expect(mockedExeca).toHaveBeenCalledWith( + 'git', + ['-C', '/tmp/cache/repo', 'worktree', 'add', '--force', '--detach', '/tmp/skilleton-wt-123', commit], + expect.any(Object), + ); + expect(mockedExeca).toHaveBeenCalledWith( + 'git', + ['-C', '/tmp/cache/repo', 'worktree', 'remove', '--force', '/tmp/skilleton-wt-123'], + expect.any(Object), + ); + }); +}); From 3f83f7259b2ebbaf15f6f0b48d5787eb67d49e8d Mon Sep 17 00:00:00 2001 From: Abdeldjalil Fortas <9090674+Fcmam5@users.noreply.github.com> Date: Sun, 5 Apr 2026 22:59:48 +0200 Subject: [PATCH 02/11] fix(security): setup eslint-plugin-security and fix findings --- bin/skilleton.ts | 42 ++++++++++++++++++++++++++++++---------- eslint.config.cjs | 5 +++++ package-lock.json | 37 +++++++++++++++++++++++++++++++++++ package.json | 1 + src/commands/describe.ts | 14 +++++++++++++- src/core/lock.ts | 19 +++++++----------- tests/describe.test.ts | 1 + 7 files changed, 96 insertions(+), 23 deletions(-) diff --git a/bin/skilleton.ts b/bin/skilleton.ts index 8bba673..63b9ce9 100644 --- a/bin/skilleton.ts +++ b/bin/skilleton.ts @@ -20,41 +20,63 @@ const commands: CommandRegistry = { audit: new AuditCommand(), }; +function getCommandHandler(command: string): Command | null { + switch (command) { + case 'add': + return commands.add; + case 'install': + return commands.install; + case 'update': + return commands.update; + case 'list': + return commands.list; + case 'describe': + return commands.describe; + case 'audit': + return commands.audit; + default: + return null; + } +} + function parseArgs(argv: string[]): { command: string | null; args: CommandArgs } { if (argv.length === 0) { return { command: null, args: { positional: [], flags: {} } }; } const [command, ...rest] = argv; + const tokens = [...rest]; const positional: string[] = []; - const flags: Record = {}; + const flagsMap = new Map(); - for (let i = 0; i < rest.length; i += 1) { - const token = rest[i]; + while (tokens.length > 0) { + const token = tokens.shift()!; if (token.startsWith('--')) { const [rawFlag, value] = token.slice(2).split('='); const flag = rawFlag === 'h' ? 'help' : rawFlag; if (value !== undefined) { - flags[flag] = value; + flagsMap.set(flag, value); continue; } - const next = rest[i + 1]; + const next = tokens[0]; if (next && !next.startsWith('-')) { - flags[flag] = next; - i += 1; + flagsMap.set(flag, next); + tokens.shift(); } else { - flags[flag] = true; + flagsMap.set(flag, true); } } else if (token.startsWith('-') && token.length > 1) { const rawFlag = token.slice(1); const flag = rawFlag === 'h' ? 'help' : rawFlag; - flags[flag] = true; + flagsMap.set(flag, true); } else { positional.push(token); } } + const flags = Object.fromEntries(flagsMap); + return { command, args: { positional, flags } }; } @@ -81,7 +103,7 @@ async function main(): Promise { } // Validate command first - const handler = commands[command]; + const handler = getCommandHandler(command); if (!handler) { console.error(`Unknown command: ${command}`); printHelp(); diff --git a/eslint.config.cjs b/eslint.config.cjs index 48f4ad8..45b387d 100644 --- a/eslint.config.cjs +++ b/eslint.config.cjs @@ -1,5 +1,6 @@ const tseslint = require('typescript-eslint'); const globals = require('globals'); +const security = require('eslint-plugin-security'); module.exports = [ { @@ -8,6 +9,9 @@ module.exports = [ ...tseslint.configs.recommended, { files: ['**/*.ts'], + plugins: { + security, + }, languageOptions: { globals: globals.node, parser: tseslint.parser, @@ -17,6 +21,7 @@ module.exports = [ }, }, rules: { + ...security.configs.recommended.rules, 'no-unused-vars': 'off', '@typescript-eslint/no-unused-vars': ['warn', { argsIgnorePattern: '^_', varsIgnorePattern: '^_' }], }, diff --git a/package-lock.json b/package-lock.json index ce7a43c..8d57b25 100644 --- a/package-lock.json +++ b/package-lock.json @@ -23,6 +23,7 @@ "@types/node": "^25.5.0", "eslint": "^10.1.0", "eslint-config-prettier": "^10.1.8", + "eslint-plugin-security": "^4.0.0", "globals": "^17.4.0", "jest": "^30.3.0", "prettier": "^3.8.1", @@ -2970,6 +2971,22 @@ "eslint": ">=7.0.0" } }, + "node_modules/eslint-plugin-security": { + "version": "4.0.0", + "resolved": "https://registry.npmjs.org/eslint-plugin-security/-/eslint-plugin-security-4.0.0.tgz", + "integrity": "sha512-tfuQT8K/Li1ZxhFzyD8wPIKtlzZxqBcPr9q0jFMQ77wWAbKBVEhaMPVQRTMTvCMUDhwBe5vPVqQPwAGk/ASfxQ==", + "dev": true, + "license": "Apache-2.0", + "dependencies": { + "safe-regex": "^2.1.1" + }, + "engines": { + "node": "^18.18.0 || ^20.9.0 || >=21.1.0" + }, + "funding": { + "url": "https://opencollective.com/eslint" + } + }, "node_modules/eslint-scope": { "version": "9.1.2", "resolved": "https://registry.npmjs.org/eslint-scope/-/eslint-scope-9.1.2.tgz", @@ -5256,6 +5273,16 @@ "dev": true, "license": "MIT" }, + "node_modules/regexp-tree": { + "version": "0.1.27", + "resolved": "https://registry.npmjs.org/regexp-tree/-/regexp-tree-0.1.27.tgz", + "integrity": "sha512-iETxpjK6YoRWJG5o6hXLwvjYAoW+FEZn9os0PD/b6AP6xQwsa/Y7lCVgIixBbUPMfhu+i2LtdeAqVTgGlQarfA==", + "dev": true, + "license": "MIT", + "bin": { + "regexp-tree": "bin/regexp-tree" + } + }, "node_modules/require-directory": { "version": "2.1.1", "resolved": "https://registry.npmjs.org/require-directory/-/require-directory-2.1.1.tgz", @@ -5298,6 +5325,16 @@ "node": ">=8" } }, + "node_modules/safe-regex": { + "version": "2.1.1", + "resolved": "https://registry.npmjs.org/safe-regex/-/safe-regex-2.1.1.tgz", + "integrity": "sha512-rx+x8AMzKb5Q5lQ95Zoi6ZbJqwCLkqi3XuJXp5P3rT8OEc6sZCJG5AE5dU3lsgRr/F4Bs31jSlVN+j5KrsGu9A==", + "dev": true, + "license": "MIT", + "dependencies": { + "regexp-tree": "~0.1.1" + } + }, "node_modules/semver": { "version": "6.3.1", "resolved": "https://registry.npmjs.org/semver/-/semver-6.3.1.tgz", diff --git a/package.json b/package.json index d411842..62c718e 100644 --- a/package.json +++ b/package.json @@ -61,6 +61,7 @@ "@types/node": "^25.5.0", "eslint": "^10.1.0", "eslint-config-prettier": "^10.1.8", + "eslint-plugin-security": "^4.0.0", "globals": "^17.4.0", "jest": "^30.3.0", "prettier": "^3.8.1", diff --git a/src/commands/describe.ts b/src/commands/describe.ts index 6478293..d07dfff 100644 --- a/src/commands/describe.ts +++ b/src/commands/describe.ts @@ -21,7 +21,7 @@ export class DescribeCommand implements Command { } const lockfile = await env.manifestRepo.readLockfileIfExists(); - const locked = lockfile?.skills?.[skillName]; + const locked = this.getLockedSkillByName(lockfile, skillName); const installPath = env.manifestRepo.skillInstallPath(skillName); const installed = await env.fs.pathExists(installPath); @@ -44,6 +44,18 @@ export class DescribeCommand implements Command { } } + private getLockedSkillByName( + lockfile: { skills: Record } | null, + skillName: string, + ): LockedSkill | undefined { + if (!lockfile) { + return undefined; + } + + const skillsByName = new Map(Object.entries(lockfile.skills)); + return skillsByName.get(skillName); + } + private printMetadata( skill: SkillDescriptor, locked: LockedSkill | undefined, diff --git a/src/core/lock.ts b/src/core/lock.ts index 6ae971d..f513144 100644 --- a/src/core/lock.ts +++ b/src/core/lock.ts @@ -40,10 +40,7 @@ export interface ReconcileLockfileResult { /** Converts resolved skills into lockfile record format keyed by skill name. */ export function serializeLockfile(skills: LockedSkill[]): SkillLockfile { - const record: SkillLockfile['skills'] = {}; - for (const skill of skills) { - record[skill.name] = skill; - } + const record = Object.fromEntries(skills.map((skill) => [skill.name, skill])) as SkillLockfile['skills']; return { skills: record }; } @@ -53,8 +50,10 @@ export function getChangedSkills(nextSkills: LockedSkill[], previous: SkillLockf return nextSkills; } + const priorByName = new Map(Object.entries(previous.skills)); + return nextSkills.filter((skill) => { - const prior = previous.skills[skill.name]; + const prior = priorByName.get(skill.name); if (!prior) { return true; } @@ -67,13 +66,9 @@ export function getChangedSkills(nextSkills: LockedSkill[], previous: SkillLockf /** Removes lockfile entries whose skill names are no longer declared. */ export function pruneLockfile(lockfile: SkillLockfile, names: string[]): SkillLockfile { const keep = new Set(names); - const nextSkills: SkillLockfile['skills'] = {}; - - for (const [name, skill] of Object.entries(lockfile.skills)) { - if (keep.has(name)) { - nextSkills[name] = skill; - } - } + const nextSkills = Object.fromEntries( + Object.entries(lockfile.skills).filter(([name]) => keep.has(name)), + ) as SkillLockfile['skills']; return { skills: nextSkills }; } diff --git a/tests/describe.test.ts b/tests/describe.test.ts index 18ea6ea..6708876 100644 --- a/tests/describe.test.ts +++ b/tests/describe.test.ts @@ -207,6 +207,7 @@ function createEnv(options: CreateEnvOptions = {}): SkilletonEnvironment { return lockfile; }, skillInstallPath(skillName: string) { + // eslint-disable-next-line security/detect-object-injection return installPaths[skillName] ?? `/repo/.skilleton/skills/${skillName}`; }, }; From 0a4b0ba8c16f795574c223fa45dc8d889ed6c382 Mon Sep 17 00:00:00 2001 From: Abdeldjalil Fortas <9090674+Fcmam5@users.noreply.github.com> Date: Sun, 5 Apr 2026 23:14:29 +0200 Subject: [PATCH 03/11] refactor: Use own FS wrapper --- eslint.config.cjs | 24 ++++++++++++++ src/adapters/git.ts | 33 ++++++++---------- src/core/filesystem.ts | 4 +++ src/core/types.ts | 1 + tests/adapters.git.test.ts | 68 ++++++++++++++++++++++---------------- tests/describe.test.ts | 6 ++++ tests/install.test.ts | 1 + 7 files changed, 88 insertions(+), 49 deletions(-) diff --git a/eslint.config.cjs b/eslint.config.cjs index 45b387d..0ae33c7 100644 --- a/eslint.config.cjs +++ b/eslint.config.cjs @@ -2,6 +2,8 @@ const tseslint = require('typescript-eslint'); const globals = require('globals'); const security = require('eslint-plugin-security'); +const FILESYSTEM_IMPORT_MESSAGE = 'Use FileSystem from src/core/filesystem.ts instead of importing node:fs directly.'; + module.exports = [ { ignores: ['dist/**', 'coverage/**', 'package-lock.json', '**/*.json'], @@ -22,10 +24,32 @@ module.exports = [ }, rules: { ...security.configs.recommended.rules, + 'no-restricted-imports': [ + 'error', + { + paths: [ + { + name: 'node:fs', + message: FILESYSTEM_IMPORT_MESSAGE, + }, + { + name: 'node:fs/promises', + message: FILESYSTEM_IMPORT_MESSAGE, + }, + ], + }, + ], 'no-unused-vars': 'off', '@typescript-eslint/no-unused-vars': ['warn', { argsIgnorePattern: '^_', varsIgnorePattern: '^_' }], }, }, + { + files: ['src/core/filesystem.ts'], + rules: { + 'no-restricted-imports': 'off', + 'security/detect-non-literal-fs-filename': 'off', // We expect controlled/validated inputs + }, + }, { files: ['eslint.config.cjs'], rules: { diff --git a/src/adapters/git.ts b/src/adapters/git.ts index 83ed7a6..ccb9a9d 100644 --- a/src/adapters/git.ts +++ b/src/adapters/git.ts @@ -1,25 +1,16 @@ -import { promises as fs } from 'node:fs'; import os from 'node:os'; import path from 'node:path'; import { execa } from 'execa'; -import { GitClient } from '../core/types'; +import { FileSystem, GitClient } from '../core/types'; import { getCacheRoot } from '../core/config'; import { ensureRepoUrl, repoCacheKey } from '../core/repos'; import { SkillInstallError } from '../core/errors'; +import { NodeFileSystem } from '../core/filesystem'; // Reference: https://git-scm.com/book/en/v2/Git-Tools-Revision-Selection // Pattern to match a 40-character hexadecimal SHA-1 hash const COMMIT_SHA_PATTERN = /^[0-9a-f]{40}$/i; -async function pathExists(target: string): Promise { - try { - await fs.access(target); - return true; - } catch { - return false; - } -} - // Rejects values that could be interpreted as git options or null bytes // This is a security measure to prevent command injection when passing values to git commands as arguments function assertSafeGitArg(value: string, label: string): void { @@ -50,7 +41,10 @@ function resolveSafeSubPath(baseDir: string, subPath?: string): string { /** Git client implementation backed by `execa` shell commands. */ export class ExecaGitClient implements GitClient { - constructor(private readonly cacheRoot = getCacheRoot()) {} + constructor( + private readonly cacheRoot = getCacheRoot(), + private readonly fs: FileSystem = new NodeFileSystem(), + ) {} /** * Ensures a repository is present in local cache and fetches updates when already cloned. @@ -67,8 +61,8 @@ export class ExecaGitClient implements GitClient { assertSafeGitArg(repoCachePath, 'repository cache path'); assertSafeGitArg(cloneSource, 'repository URL'); - if (!(await pathExists(repoCachePath))) { - await fs.mkdir(path.dirname(repoCachePath), { recursive: true }); + if (!(await this.fs.pathExists(repoCachePath))) { + await this.fs.ensureDir(path.dirname(repoCachePath)); // Security: Validate paths to prevent command injection await execa('git', ['clone', '--filter=blob:none', '--', cloneSource, repoCachePath], { timeout: 30000, @@ -104,7 +98,7 @@ export class ExecaGitClient implements GitClient { throw new SkillInstallError(`Invalid commit SHA: ${commit}`); } - const worktreeDir = await fs.mkdtemp(path.join(os.tmpdir(), 'skilleton-wt-')); + const worktreeDir = await this.fs.mkdtemp(path.join(os.tmpdir(), 'skilleton-wt-')); const gitEnv = { ...process.env, GIT_TERMINAL_PROMPT: '0' }; try { @@ -117,13 +111,12 @@ export class ExecaGitClient implements GitClient { }); const sourcePath = resolveSafeSubPath(worktreeDir, subPath); - if (!(await pathExists(sourcePath))) { + if (!(await this.fs.pathExists(sourcePath))) { throw new SkillInstallError(`Skill path ${subPath ?? '.'} not found in repository ${repoPath}`); } - await fs.rm(destination, { recursive: true, force: true }); - await fs.mkdir(path.dirname(destination), { recursive: true }); - await fs.cp(sourcePath, destination, { recursive: true }); + await this.fs.remove(destination); + await this.fs.copy(sourcePath, destination); } catch (error) { throw new SkillInstallError( `Failed to export ${subPath ?? '.'} from ${repoPath}@${commit}: ${(error as Error).message}`, @@ -136,7 +129,7 @@ export class ExecaGitClient implements GitClient { stdout: 'pipe', stderr: 'pipe', }).catch(() => undefined); - await fs.rm(worktreeDir, { recursive: true, force: true }).catch(() => undefined); + await this.fs.remove(worktreeDir).catch(() => undefined); } } } diff --git a/src/core/filesystem.ts b/src/core/filesystem.ts index f7d9b91..6710a58 100644 --- a/src/core/filesystem.ts +++ b/src/core/filesystem.ts @@ -30,6 +30,10 @@ export class NodeFileSystem implements FileSystem { await fs.mkdir(target, { recursive: true }); } + async mkdtemp(prefix: string): Promise { + return fs.mkdtemp(prefix); + } + /** * Reads and parses JSON from disk. * @param target JSON file path. diff --git a/src/core/types.ts b/src/core/types.ts index 033d667..45e4df0 100644 --- a/src/core/types.ts +++ b/src/core/types.ts @@ -44,6 +44,7 @@ export interface GitClient { export interface FileSystem { pathExists(_path: string): Promise; ensureDir(_path: string): Promise; + mkdtemp(_prefix: string): Promise; readJson(_path: string): Promise; writeJson(_path: string, _data: unknown): Promise; readFile(_path: string): Promise; diff --git a/tests/adapters.git.test.ts b/tests/adapters.git.test.ts index e39e3a8..1426163 100644 --- a/tests/adapters.git.test.ts +++ b/tests/adapters.git.test.ts @@ -1,56 +1,64 @@ -import { promises as fs } from 'node:fs'; import { execa } from 'execa'; import { ExecaGitClient } from '../src/adapters/git'; import { SkillInstallError } from '../src/core/errors'; +import { FileSystem } from '../src/core/types'; jest.mock('execa'); -jest.mock('node:fs', () => ({ - promises: { - access: jest.fn(), - mkdir: jest.fn(), - rm: jest.fn(), - cp: jest.fn(), - mkdtemp: jest.fn(), - }, -})); + +const createFsMock = (): jest.Mocked => ({ + pathExists: jest.fn(), + ensureDir: jest.fn(), + mkdtemp: jest.fn(), + readJson: jest.fn(), + writeJson: jest.fn(), + readFile: jest.fn(), + isDirectory: jest.fn(), + remove: jest.fn(), + copy: jest.fn(), + symlink: jest.fn(), + readDir: jest.fn(), +}); describe('ExecaGitClient security hardening', () => { - const mockedFs = fs as unknown as { - access: jest.Mock; - mkdir: jest.Mock; - rm: jest.Mock; - cp: jest.Mock; - mkdtemp: jest.Mock; - }; + let mockedFs: jest.Mocked; const mockedExeca = execa as unknown as jest.Mock; beforeEach(() => { jest.clearAllMocks(); - mockedFs.access.mockResolvedValue(undefined); - mockedFs.mkdir.mockResolvedValue(undefined); - mockedFs.rm.mockResolvedValue(undefined); - mockedFs.cp.mockResolvedValue(undefined); + mockedFs = createFsMock(); + mockedFs.pathExists.mockResolvedValue(true); + mockedFs.ensureDir.mockResolvedValue(undefined); + mockedFs.remove.mockResolvedValue(undefined); + mockedFs.copy.mockResolvedValue(undefined); mockedFs.mkdtemp.mockResolvedValue('/tmp/skilleton-wt-123'); mockedExeca.mockResolvedValue({ stdout: '', stderr: '' }); }); it('uses -- separator for git clone arguments', async () => { - mockedFs.access.mockRejectedValueOnce(new Error('missing')); - const client = new ExecaGitClient('/tmp/cache'); + mockedFs.pathExists.mockResolvedValueOnce(false); + const client = new ExecaGitClient('/tmp/cache', mockedFs); await client.ensureRepo('https://github.com/acme/skills'); expect(mockedExeca).toHaveBeenCalledWith( 'git', - ['clone', '--filter=blob:none', '--', 'https://github.com/acme/skills.git', '/tmp/cache/https_github.com_acme_skills'], + [ + 'clone', + '--filter=blob:none', + '--', + 'https://github.com/acme/skills.git', + '/tmp/cache/https_github.com_acme_skills', + ], expect.any(Object), ); }); it('rejects destination values that look like git options', async () => { - const client = new ExecaGitClient('/tmp/cache'); + const client = new ExecaGitClient('/tmp/cache', mockedFs); - await expect(client.ensureRepo('https://github.com/acme/skills', '--upload-pack=sh')).rejects.toThrow(SkillInstallError); + await expect(client.ensureRepo('https://github.com/acme/skills', '--upload-pack=sh')).rejects.toThrow( + SkillInstallError, + ); await expect(client.ensureRepo('https://github.com/acme/skills', '--upload-pack=sh')).rejects.toThrow( 'Invalid repository cache path', ); @@ -58,9 +66,11 @@ describe('ExecaGitClient security hardening', () => { }); it('rejects non-SHA commit inputs before running git worktree', async () => { - const client = new ExecaGitClient('/tmp/cache'); + const client = new ExecaGitClient('/tmp/cache', mockedFs); - await expect(client.exportPath('/tmp/cache/repo', 'main', '/tmp/out', 'skills/jest')).rejects.toThrow(SkillInstallError); + await expect(client.exportPath('/tmp/cache/repo', 'main', '/tmp/out', 'skills/jest')).rejects.toThrow( + SkillInstallError, + ); await expect(client.exportPath('/tmp/cache/repo', 'main', '/tmp/out', 'skills/jest')).rejects.toThrow( 'Invalid commit SHA: main', ); @@ -69,7 +79,7 @@ describe('ExecaGitClient security hardening', () => { }); it('rejects path traversal in subPath', async () => { - const client = new ExecaGitClient('/tmp/cache'); + const client = new ExecaGitClient('/tmp/cache', mockedFs); const commit = 'abcdef1234567890abcdef1234567890abcdef12'; await expect(client.exportPath('/tmp/cache/repo', commit, '/tmp/out', '../escape')).rejects.toThrow( diff --git a/tests/describe.test.ts b/tests/describe.test.ts index 6708876..35e29bb 100644 --- a/tests/describe.test.ts +++ b/tests/describe.test.ts @@ -274,6 +274,12 @@ class InMemoryFileSystem implements FileSystem { this.directories.add(this.normalize(target)); } + async mkdtemp(prefix: string): Promise { + const tempDir = this.normalize(`${prefix}tmp`); + this.directories.add(tempDir); + return tempDir; + } + async readJson(_path: string): Promise { throw new Error('Not implemented'); } diff --git a/tests/install.test.ts b/tests/install.test.ts index d28e759..3d6a054 100644 --- a/tests/install.test.ts +++ b/tests/install.test.ts @@ -6,6 +6,7 @@ import { SkillInstallError } from '../src/core/errors'; const createFsMock = (): jest.Mocked => ({ pathExists: jest.fn(), ensureDir: jest.fn(), + mkdtemp: jest.fn(), readJson: jest.fn(), writeJson: jest.fn(), readFile: jest.fn(), From 6613160803ec8655efee9a270eeaa767ad22da8b Mon Sep 17 00:00:00 2001 From: Abdeldjalil Fortas <9090674+Fcmam5@users.noreply.github.com> Date: Sun, 5 Apr 2026 23:34:50 +0200 Subject: [PATCH 04/11] fix(security): use while loop in normalizeRepoUrl instead of regex This resolves https://github.com/Fcmam5/skilleton/security/code-scanning/3 --- src/core/repos.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/core/repos.ts b/src/core/repos.ts index 7beb19d..980d777 100644 --- a/src/core/repos.ts +++ b/src/core/repos.ts @@ -24,8 +24,13 @@ export function isRepoUrl(spec: string): boolean { /** Normalizes repository URLs by removing trailing slash and `.git`. */ export function normalizeRepoUrl(url: string): string { - const withoutTrailingSlash = url.replace(/\/+$/, ''); - return withoutTrailingSlash.replace(/\.git$/i, ''); + let normalized = url; + // Using a deterministic while loop to remove trailing slashes instead of regex + // This closes "Polynomial regular expression used on uncontrolled data" warning (code scanning #3) + while (normalized.endsWith('/')) { + normalized = normalized.slice(0, -1); + } + return normalized.replace(/\.git$/i, ''); } /** Normalizes owner/repo or URL-like specs into a canonical repository URL. */ From a186fc6193c5da35f2146b42cd11fab9bfd15a29 Mon Sep 17 00:00:00 2001 From: Abdeldjalil Fortas <9090674+Fcmam5@users.noreply.github.com> Date: Sun, 5 Apr 2026 23:41:42 +0200 Subject: [PATCH 05/11] chore: update dependencies --- package-lock.json | 250 ++++++++++++++++++++++++---------------------- package.json | 10 +- 2 files changed, 137 insertions(+), 123 deletions(-) diff --git a/package-lock.json b/package-lock.json index 8d57b25..2a0e0f5 100644 --- a/package-lock.json +++ b/package-lock.json @@ -20,18 +20,18 @@ "@eslint/js": "^10.0.1", "@eslint/json": "^1.2.0", "@types/jest": "^30.0.0", - "@types/node": "^25.5.0", - "eslint": "^10.1.0", + "@types/node": "^25.5.2", + "eslint": "^10.2.0", "eslint-config-prettier": "^10.1.8", "eslint-plugin-security": "^4.0.0", "globals": "^17.4.0", "jest": "^30.3.0", "prettier": "^3.8.1", - "ts-jest": "^29.4.6", + "ts-jest": "^29.4.9", "ts-node": "^10.9.2", "typedoc": "^0.28.18", - "typescript": "^5.9.3", - "typescript-eslint": "^8.57.1" + "typescript": "^6.0.2", + "typescript-eslint": "^8.58.0" }, "engines": { "node": ">=24.0.0" @@ -638,13 +638,13 @@ } }, "node_modules/@eslint/config-array": { - "version": "0.23.3", - "resolved": "https://registry.npmjs.org/@eslint/config-array/-/config-array-0.23.3.tgz", - "integrity": "sha512-j+eEWmB6YYLwcNOdlwQ6L2OsptI/LO6lNBuLIqe5R7RetD658HLoF+Mn7LzYmAWWNNzdC6cqP+L6r8ujeYXWLw==", + "version": "0.23.4", + "resolved": "https://registry.npmjs.org/@eslint/config-array/-/config-array-0.23.4.tgz", + "integrity": "sha512-lf19F24LSMfF8weXvW5QEtnLqW70u7kgit5e9PSx0MsHAFclGd1T9ynvWEMDT1w5J4Qt54tomGeAhdoAku1Xow==", "dev": true, "license": "Apache-2.0", "dependencies": { - "@eslint/object-schema": "^3.0.3", + "@eslint/object-schema": "^3.0.4", "debug": "^4.3.1", "minimatch": "^10.2.4" }, @@ -676,13 +676,13 @@ } }, "node_modules/@eslint/config-array/node_modules/minimatch": { - "version": "10.2.4", - "resolved": "https://registry.npmjs.org/minimatch/-/minimatch-10.2.4.tgz", - "integrity": "sha512-oRjTw/97aTBN0RHbYCdtF1MQfvusSIBQM0IZEgzl6426+8jSC0nF1a/GmnVLpfB9yyr6g6FTqWqiZVbxrtaCIg==", + "version": "10.2.5", + "resolved": "https://registry.npmjs.org/minimatch/-/minimatch-10.2.5.tgz", + "integrity": "sha512-MULkVLfKGYDFYejP07QOurDLLQpcjk7Fw+7jXS2R2czRQzR56yHRveU5NDJEOviH+hETZKSkIk5c+T23GjFUMg==", "dev": true, "license": "BlueOak-1.0.0", "dependencies": { - "brace-expansion": "^5.0.2" + "brace-expansion": "^5.0.5" }, "engines": { "node": "18 || 20 || >=22" @@ -692,22 +692,22 @@ } }, "node_modules/@eslint/config-helpers": { - "version": "0.5.3", - "resolved": "https://registry.npmjs.org/@eslint/config-helpers/-/config-helpers-0.5.3.tgz", - "integrity": "sha512-lzGN0onllOZCGroKJmRwY6QcEHxbjBw1gwB8SgRSqK8YbbtEXMvKynsXc3553ckIEBxsbMBU7oOZXKIPGZNeZw==", + "version": "0.5.4", + "resolved": "https://registry.npmjs.org/@eslint/config-helpers/-/config-helpers-0.5.4.tgz", + "integrity": "sha512-jJhqiY3wPMlWWO3370M86CPJ7pt8GmEwSLglMfQhjXal07RCvhmU0as4IuUEW5SJeunfItiEetHmSxCCe9lDBg==", "dev": true, "license": "Apache-2.0", "dependencies": { - "@eslint/core": "^1.1.1" + "@eslint/core": "^1.2.0" }, "engines": { "node": "^20.19.0 || ^22.13.0 || >=24" } }, "node_modules/@eslint/core": { - "version": "1.1.1", - "resolved": "https://registry.npmjs.org/@eslint/core/-/core-1.1.1.tgz", - "integrity": "sha512-QUPblTtE51/7/Zhfv8BDwO0qkkzQL7P/aWWbqcf4xWLEYn1oKjdO0gglQBB4GAsu7u6wjijbCmzsUTy6mnk6oQ==", + "version": "1.2.0", + "resolved": "https://registry.npmjs.org/@eslint/core/-/core-1.2.0.tgz", + "integrity": "sha512-8FTGbNzTvmSlc4cZBaShkC6YvFMG0riksYWRFKXztqVdXaQbcZLXlFbSpC05s70sGEsXAw0qwhx69JiW7hQS7A==", "dev": true, "license": "Apache-2.0", "dependencies": { @@ -755,9 +755,9 @@ } }, "node_modules/@eslint/object-schema": { - "version": "3.0.3", - "resolved": "https://registry.npmjs.org/@eslint/object-schema/-/object-schema-3.0.3.tgz", - "integrity": "sha512-iM869Pugn9Nsxbh/YHRqYiqd23AmIbxJOcpUMOuWCVNdoQJ5ZtwL6h3t0bcZzJUlC3Dq9jCFCESBZnX0GTv7iQ==", + "version": "3.0.4", + "resolved": "https://registry.npmjs.org/@eslint/object-schema/-/object-schema-3.0.4.tgz", + "integrity": "sha512-55lO/7+Yp0ISKRP0PsPtNTeNGapXaO085aELZmWCVc5SH3jfrqpuU6YgOdIxMS99ZHkQN1cXKE+cdIqwww9ptw==", "dev": true, "license": "Apache-2.0", "engines": { @@ -1574,9 +1574,9 @@ "license": "MIT" }, "node_modules/@types/node": { - "version": "25.5.0", - "resolved": "https://registry.npmjs.org/@types/node/-/node-25.5.0.tgz", - "integrity": "sha512-jp2P3tQMSxWugkCUKLRPVUpGaL5MVFwF8RDuSRztfwgN1wmqJeMSbKlnEtQqU8UrhTmzEmZdu2I6v2dpp7XIxw==", + "version": "25.5.2", + "resolved": "https://registry.npmjs.org/@types/node/-/node-25.5.2.tgz", + "integrity": "sha512-tO4ZIRKNC+MDWV4qKVZe3Ql/woTnmHDr5JD8UI5hn2pwBrHEwOEMZK7WlNb5RKB6EoJ02gwmQS9OrjuFnZYdpg==", "dev": true, "license": "MIT", "dependencies": { @@ -1615,20 +1615,20 @@ "license": "MIT" }, "node_modules/@typescript-eslint/eslint-plugin": { - "version": "8.57.1", - "resolved": "https://registry.npmjs.org/@typescript-eslint/eslint-plugin/-/eslint-plugin-8.57.1.tgz", - "integrity": "sha512-Gn3aqnvNl4NGc6x3/Bqk1AOn0thyTU9bqDRhiRnUWezgvr2OnhYCWCgC8zXXRVqBsIL1pSDt7T9nJUe0oM0kDQ==", + "version": "8.58.0", + "resolved": "https://registry.npmjs.org/@typescript-eslint/eslint-plugin/-/eslint-plugin-8.58.0.tgz", + "integrity": "sha512-RLkVSiNuUP1C2ROIWfqX+YcUfLaSnxGE/8M+Y57lopVwg9VTYYfhuz15Yf1IzCKgZj6/rIbYTmJCUSqr76r0Wg==", "dev": true, "license": "MIT", "dependencies": { "@eslint-community/regexpp": "^4.12.2", - "@typescript-eslint/scope-manager": "8.57.1", - "@typescript-eslint/type-utils": "8.57.1", - "@typescript-eslint/utils": "8.57.1", - "@typescript-eslint/visitor-keys": "8.57.1", + "@typescript-eslint/scope-manager": "8.58.0", + "@typescript-eslint/type-utils": "8.58.0", + "@typescript-eslint/utils": "8.58.0", + "@typescript-eslint/visitor-keys": "8.58.0", "ignore": "^7.0.5", "natural-compare": "^1.4.0", - "ts-api-utils": "^2.4.0" + "ts-api-utils": "^2.5.0" }, "engines": { "node": "^18.18.0 || ^20.9.0 || >=21.1.0" @@ -1638,9 +1638,9 @@ "url": "https://opencollective.com/typescript-eslint" }, "peerDependencies": { - "@typescript-eslint/parser": "^8.57.1", + "@typescript-eslint/parser": "^8.58.0", "eslint": "^8.57.0 || ^9.0.0 || ^10.0.0", - "typescript": ">=4.8.4 <6.0.0" + "typescript": ">=4.8.4 <6.1.0" } }, "node_modules/@typescript-eslint/eslint-plugin/node_modules/ignore": { @@ -1654,16 +1654,16 @@ } }, "node_modules/@typescript-eslint/parser": { - "version": "8.57.1", - "resolved": "https://registry.npmjs.org/@typescript-eslint/parser/-/parser-8.57.1.tgz", - "integrity": "sha512-k4eNDan0EIMTT/dUKc/g+rsJ6wcHYhNPdY19VoX/EOtaAG8DLtKCykhrUnuHPYvinn5jhAPgD2Qw9hXBwrahsw==", + "version": "8.58.0", + "resolved": "https://registry.npmjs.org/@typescript-eslint/parser/-/parser-8.58.0.tgz", + "integrity": "sha512-rLoGZIf9afaRBYsPUMtvkDWykwXwUPL60HebR4JgTI8mxfFe2cQTu3AGitANp4b9B2QlVru6WzjgB2IzJKiCSA==", "dev": true, "license": "MIT", "dependencies": { - "@typescript-eslint/scope-manager": "8.57.1", - "@typescript-eslint/types": "8.57.1", - "@typescript-eslint/typescript-estree": "8.57.1", - "@typescript-eslint/visitor-keys": "8.57.1", + "@typescript-eslint/scope-manager": "8.58.0", + "@typescript-eslint/types": "8.58.0", + "@typescript-eslint/typescript-estree": "8.58.0", + "@typescript-eslint/visitor-keys": "8.58.0", "debug": "^4.4.3" }, "engines": { @@ -1675,18 +1675,18 @@ }, "peerDependencies": { "eslint": "^8.57.0 || ^9.0.0 || ^10.0.0", - "typescript": ">=4.8.4 <6.0.0" + "typescript": ">=4.8.4 <6.1.0" } }, "node_modules/@typescript-eslint/project-service": { - "version": "8.57.1", - "resolved": "https://registry.npmjs.org/@typescript-eslint/project-service/-/project-service-8.57.1.tgz", - "integrity": "sha512-vx1F37BRO1OftsYlmG9xay1TqnjNVlqALymwWVuYTdo18XuKxtBpCj1QlzNIEHlvlB27osvXFWptYiEWsVdYsg==", + "version": "8.58.0", + "resolved": "https://registry.npmjs.org/@typescript-eslint/project-service/-/project-service-8.58.0.tgz", + "integrity": "sha512-8Q/wBPWLQP1j16NxoPNIKpDZFMaxl7yWIoqXWYeWO+Bbd2mjgvoF0dxP2jKZg5+x49rgKdf7Ck473M8PC3V9lg==", "dev": true, "license": "MIT", "dependencies": { - "@typescript-eslint/tsconfig-utils": "^8.57.1", - "@typescript-eslint/types": "^8.57.1", + "@typescript-eslint/tsconfig-utils": "^8.58.0", + "@typescript-eslint/types": "^8.58.0", "debug": "^4.4.3" }, "engines": { @@ -1697,18 +1697,18 @@ "url": "https://opencollective.com/typescript-eslint" }, "peerDependencies": { - "typescript": ">=4.8.4 <6.0.0" + "typescript": ">=4.8.4 <6.1.0" } }, "node_modules/@typescript-eslint/scope-manager": { - "version": "8.57.1", - "resolved": "https://registry.npmjs.org/@typescript-eslint/scope-manager/-/scope-manager-8.57.1.tgz", - "integrity": "sha512-hs/QcpCwlwT2L5S+3fT6gp0PabyGk4Q0Rv2doJXA0435/OpnSR3VRgvrp8Xdoc3UAYSg9cyUjTeFXZEPg/3OKg==", + "version": "8.58.0", + "resolved": "https://registry.npmjs.org/@typescript-eslint/scope-manager/-/scope-manager-8.58.0.tgz", + "integrity": "sha512-W1Lur1oF50FxSnNdGp3Vs6P+yBRSmZiw4IIjEeYxd8UQJwhUF0gDgDD/W/Tgmh73mxgEU3qX0Bzdl/NGuSPEpQ==", "dev": true, "license": "MIT", "dependencies": { - "@typescript-eslint/types": "8.57.1", - "@typescript-eslint/visitor-keys": "8.57.1" + "@typescript-eslint/types": "8.58.0", + "@typescript-eslint/visitor-keys": "8.58.0" }, "engines": { "node": "^18.18.0 || ^20.9.0 || >=21.1.0" @@ -1719,9 +1719,9 @@ } }, "node_modules/@typescript-eslint/tsconfig-utils": { - "version": "8.57.1", - "resolved": "https://registry.npmjs.org/@typescript-eslint/tsconfig-utils/-/tsconfig-utils-8.57.1.tgz", - "integrity": "sha512-0lgOZB8cl19fHO4eI46YUx2EceQqhgkPSuCGLlGi79L2jwYY1cxeYc1Nae8Aw1xjgW3PKVDLlr3YJ6Bxx8HkWg==", + "version": "8.58.0", + "resolved": "https://registry.npmjs.org/@typescript-eslint/tsconfig-utils/-/tsconfig-utils-8.58.0.tgz", + "integrity": "sha512-doNSZEVJsWEu4htiVC+PR6NpM+pa+a4ClH9INRWOWCUzMst/VA9c4gXq92F8GUD1rwhNvRLkgjfYtFXegXQF7A==", "dev": true, "license": "MIT", "engines": { @@ -1732,21 +1732,21 @@ "url": "https://opencollective.com/typescript-eslint" }, "peerDependencies": { - "typescript": ">=4.8.4 <6.0.0" + "typescript": ">=4.8.4 <6.1.0" } }, "node_modules/@typescript-eslint/type-utils": { - "version": "8.57.1", - "resolved": "https://registry.npmjs.org/@typescript-eslint/type-utils/-/type-utils-8.57.1.tgz", - "integrity": "sha512-+Bwwm0ScukFdyoJsh2u6pp4S9ktegF98pYUU0hkphOOqdMB+1sNQhIz8y5E9+4pOioZijrkfNO/HUJVAFFfPKA==", + "version": "8.58.0", + "resolved": "https://registry.npmjs.org/@typescript-eslint/type-utils/-/type-utils-8.58.0.tgz", + "integrity": "sha512-aGsCQImkDIqMyx1u4PrVlbi/krmDsQUs4zAcCV6M7yPcPev+RqVlndsJy9kJ8TLihW9TZ0kbDAzctpLn5o+lOg==", "dev": true, "license": "MIT", "dependencies": { - "@typescript-eslint/types": "8.57.1", - "@typescript-eslint/typescript-estree": "8.57.1", - "@typescript-eslint/utils": "8.57.1", + "@typescript-eslint/types": "8.58.0", + "@typescript-eslint/typescript-estree": "8.58.0", + "@typescript-eslint/utils": "8.58.0", "debug": "^4.4.3", - "ts-api-utils": "^2.4.0" + "ts-api-utils": "^2.5.0" }, "engines": { "node": "^18.18.0 || ^20.9.0 || >=21.1.0" @@ -1757,13 +1757,13 @@ }, "peerDependencies": { "eslint": "^8.57.0 || ^9.0.0 || ^10.0.0", - "typescript": ">=4.8.4 <6.0.0" + "typescript": ">=4.8.4 <6.1.0" } }, "node_modules/@typescript-eslint/types": { - "version": "8.57.1", - "resolved": "https://registry.npmjs.org/@typescript-eslint/types/-/types-8.57.1.tgz", - "integrity": "sha512-S29BOBPJSFUiblEl6RzPPjJt6w25A6XsBqRVDt53tA/tlL8q7ceQNZHTjPeONt/3S7KRI4quk+yP9jK2WjBiPQ==", + "version": "8.58.0", + "resolved": "https://registry.npmjs.org/@typescript-eslint/types/-/types-8.58.0.tgz", + "integrity": "sha512-O9CjxypDT89fbHxRfETNoAnHj/i6IpRK0CvbVN3qibxlLdo5p5hcLmUuCCrHMpxiWSwKyI8mCP7qRNYuOJ0Uww==", "dev": true, "license": "MIT", "engines": { @@ -1775,21 +1775,21 @@ } }, "node_modules/@typescript-eslint/typescript-estree": { - "version": "8.57.1", - "resolved": "https://registry.npmjs.org/@typescript-eslint/typescript-estree/-/typescript-estree-8.57.1.tgz", - "integrity": "sha512-ybe2hS9G6pXpqGtPli9Gx9quNV0TWLOmh58ADlmZe9DguLq0tiAKVjirSbtM1szG6+QH6rVXyU6GTLQbWnMY+g==", + "version": "8.58.0", + "resolved": "https://registry.npmjs.org/@typescript-eslint/typescript-estree/-/typescript-estree-8.58.0.tgz", + "integrity": "sha512-7vv5UWbHqew/dvs+D3e1RvLv1v2eeZ9txRHPnEEBUgSNLx5ghdzjHa0sgLWYVKssH+lYmV0JaWdoubo0ncGYLA==", "dev": true, "license": "MIT", "dependencies": { - "@typescript-eslint/project-service": "8.57.1", - "@typescript-eslint/tsconfig-utils": "8.57.1", - "@typescript-eslint/types": "8.57.1", - "@typescript-eslint/visitor-keys": "8.57.1", + "@typescript-eslint/project-service": "8.58.0", + "@typescript-eslint/tsconfig-utils": "8.58.0", + "@typescript-eslint/types": "8.58.0", + "@typescript-eslint/visitor-keys": "8.58.0", "debug": "^4.4.3", "minimatch": "^10.2.2", "semver": "^7.7.3", "tinyglobby": "^0.2.15", - "ts-api-utils": "^2.4.0" + "ts-api-utils": "^2.5.0" }, "engines": { "node": "^18.18.0 || ^20.9.0 || >=21.1.0" @@ -1799,7 +1799,7 @@ "url": "https://opencollective.com/typescript-eslint" }, "peerDependencies": { - "typescript": ">=4.8.4 <6.0.0" + "typescript": ">=4.8.4 <6.1.0" } }, "node_modules/@typescript-eslint/typescript-estree/node_modules/balanced-match": { @@ -1826,13 +1826,13 @@ } }, "node_modules/@typescript-eslint/typescript-estree/node_modules/minimatch": { - "version": "10.2.4", - "resolved": "https://registry.npmjs.org/minimatch/-/minimatch-10.2.4.tgz", - "integrity": "sha512-oRjTw/97aTBN0RHbYCdtF1MQfvusSIBQM0IZEgzl6426+8jSC0nF1a/GmnVLpfB9yyr6g6FTqWqiZVbxrtaCIg==", + "version": "10.2.5", + "resolved": "https://registry.npmjs.org/minimatch/-/minimatch-10.2.5.tgz", + "integrity": "sha512-MULkVLfKGYDFYejP07QOurDLLQpcjk7Fw+7jXS2R2czRQzR56yHRveU5NDJEOviH+hETZKSkIk5c+T23GjFUMg==", "dev": true, "license": "BlueOak-1.0.0", "dependencies": { - "brace-expansion": "^5.0.2" + "brace-expansion": "^5.0.5" }, "engines": { "node": "18 || 20 || >=22" @@ -1855,16 +1855,16 @@ } }, "node_modules/@typescript-eslint/utils": { - "version": "8.57.1", - "resolved": "https://registry.npmjs.org/@typescript-eslint/utils/-/utils-8.57.1.tgz", - "integrity": "sha512-XUNSJ/lEVFttPMMoDVA2r2bwrl8/oPx8cURtczkSEswY5T3AeLmCy+EKWQNdL4u0MmAHOjcWrqJp2cdvgjn8dQ==", + "version": "8.58.0", + "resolved": "https://registry.npmjs.org/@typescript-eslint/utils/-/utils-8.58.0.tgz", + "integrity": "sha512-RfeSqcFeHMHlAWzt4TBjWOAtoW9lnsAGiP3GbaX9uVgTYYrMbVnGONEfUCiSss+xMHFl+eHZiipmA8WkQ7FuNA==", "dev": true, "license": "MIT", "dependencies": { "@eslint-community/eslint-utils": "^4.9.1", - "@typescript-eslint/scope-manager": "8.57.1", - "@typescript-eslint/types": "8.57.1", - "@typescript-eslint/typescript-estree": "8.57.1" + "@typescript-eslint/scope-manager": "8.58.0", + "@typescript-eslint/types": "8.58.0", + "@typescript-eslint/typescript-estree": "8.58.0" }, "engines": { "node": "^18.18.0 || ^20.9.0 || >=21.1.0" @@ -1875,17 +1875,17 @@ }, "peerDependencies": { "eslint": "^8.57.0 || ^9.0.0 || ^10.0.0", - "typescript": ">=4.8.4 <6.0.0" + "typescript": ">=4.8.4 <6.1.0" } }, "node_modules/@typescript-eslint/visitor-keys": { - "version": "8.57.1", - "resolved": "https://registry.npmjs.org/@typescript-eslint/visitor-keys/-/visitor-keys-8.57.1.tgz", - "integrity": "sha512-YWnmJkXbofiz9KbnbbwuA2rpGkFPLbAIetcCNO6mJ8gdhdZ/v7WDXsoGFAJuM6ikUFKTlSQnjWnVO4ux+UzS6A==", + "version": "8.58.0", + "resolved": "https://registry.npmjs.org/@typescript-eslint/visitor-keys/-/visitor-keys-8.58.0.tgz", + "integrity": "sha512-XJ9UD9+bbDo4a4epraTwG3TsNPeiB9aShrUneAVXy8q4LuwowN+qu89/6ByLMINqvIMeI9H9hOHQtg/ijrYXzQ==", "dev": true, "license": "MIT", "dependencies": { - "@typescript-eslint/types": "8.57.1", + "@typescript-eslint/types": "8.58.0", "eslint-visitor-keys": "^5.0.0" }, "engines": { @@ -2900,18 +2900,18 @@ } }, "node_modules/eslint": { - "version": "10.1.0", - "resolved": "https://registry.npmjs.org/eslint/-/eslint-10.1.0.tgz", - "integrity": "sha512-S9jlY/ELKEUwwQnqWDO+f+m6sercqOPSqXM5Go94l7DOmxHVDgmSFGWEzeE/gwgTAr0W103BWt0QLe/7mabIvA==", + "version": "10.2.0", + "resolved": "https://registry.npmjs.org/eslint/-/eslint-10.2.0.tgz", + "integrity": "sha512-+L0vBFYGIpSNIt/KWTpFonPrqYvgKw1eUI5Vn7mEogrQcWtWYtNQ7dNqC+px/J0idT3BAkiWrhfS7k+Tum8TUA==", "dev": true, "license": "MIT", "dependencies": { "@eslint-community/eslint-utils": "^4.8.0", "@eslint-community/regexpp": "^4.12.2", - "@eslint/config-array": "^0.23.3", - "@eslint/config-helpers": "^0.5.3", - "@eslint/core": "^1.1.1", - "@eslint/plugin-kit": "^0.6.1", + "@eslint/config-array": "^0.23.4", + "@eslint/config-helpers": "^0.5.4", + "@eslint/core": "^1.2.0", + "@eslint/plugin-kit": "^0.7.0", "@humanfs/node": "^0.16.6", "@humanwhocodes/module-importer": "^1.0.1", "@humanwhocodes/retry": "^0.4.2", @@ -3019,6 +3019,20 @@ "url": "https://opencollective.com/eslint" } }, + "node_modules/eslint/node_modules/@eslint/plugin-kit": { + "version": "0.7.0", + "resolved": "https://registry.npmjs.org/@eslint/plugin-kit/-/plugin-kit-0.7.0.tgz", + "integrity": "sha512-ejvBr8MQCbVsWNZnCwDXjUKq40MDmHalq7cJ6e9s/qzTUFIIo/afzt1Vui9T97FM/V/pN4YsFVoed5NIa96RDg==", + "dev": true, + "license": "Apache-2.0", + "dependencies": { + "@eslint/core": "^1.2.0", + "levn": "^0.4.1" + }, + "engines": { + "node": "^20.19.0 || ^22.13.0 || >=24" + } + }, "node_modules/eslint/node_modules/ajv": { "version": "6.14.0", "resolved": "https://registry.npmjs.org/ajv/-/ajv-6.14.0.tgz", @@ -5764,19 +5778,19 @@ } }, "node_modules/ts-jest": { - "version": "29.4.6", - "resolved": "https://registry.npmjs.org/ts-jest/-/ts-jest-29.4.6.tgz", - "integrity": "sha512-fSpWtOO/1AjSNQguk43hb/JCo16oJDnMJf3CdEGNkqsEX3t0KX96xvyX1D7PfLCpVoKu4MfVrqUkFyblYoY4lA==", + "version": "29.4.9", + "resolved": "https://registry.npmjs.org/ts-jest/-/ts-jest-29.4.9.tgz", + "integrity": "sha512-LTb9496gYPMCqjeDLdPrKuXtncudeV1yRZnF4Wo5l3SFi0RYEnYRNgMrFIdg+FHvfzjCyQk1cLncWVqiSX+EvQ==", "dev": true, "license": "MIT", "dependencies": { "bs-logger": "^0.2.6", "fast-json-stable-stringify": "^2.1.0", - "handlebars": "^4.7.8", + "handlebars": "^4.7.9", "json5": "^2.2.3", "lodash.memoize": "^4.1.2", "make-error": "^1.3.6", - "semver": "^7.7.3", + "semver": "^7.7.4", "type-fest": "^4.41.0", "yargs-parser": "^21.1.1" }, @@ -5793,7 +5807,7 @@ "babel-jest": "^29.0.0 || ^30.0.0", "jest": "^29.0.0 || ^30.0.0", "jest-util": "^29.0.0 || ^30.0.0", - "typescript": ">=4.3 <6" + "typescript": ">=4.3 <7" }, "peerDependenciesMeta": { "@babel/core": { @@ -5994,9 +6008,9 @@ } }, "node_modules/typescript": { - "version": "5.9.3", - "resolved": "https://registry.npmjs.org/typescript/-/typescript-5.9.3.tgz", - "integrity": "sha512-jl1vZzPDinLr9eUt3J/t7V6FgNEw9QjvBPdysz9KfQDD41fQrC2Y4vKQdiaUpFT4bXlb1RHhLpp8wtm6M5TgSw==", + "version": "6.0.2", + "resolved": "https://registry.npmjs.org/typescript/-/typescript-6.0.2.tgz", + "integrity": "sha512-bGdAIrZ0wiGDo5l8c++HWtbaNCWTS4UTv7RaTH/ThVIgjkveJt83m74bBHMJkuCbslY8ixgLBVZJIOiQlQTjfQ==", "dev": true, "license": "Apache-2.0", "bin": { @@ -6008,16 +6022,16 @@ } }, "node_modules/typescript-eslint": { - "version": "8.57.1", - "resolved": "https://registry.npmjs.org/typescript-eslint/-/typescript-eslint-8.57.1.tgz", - "integrity": "sha512-fLvZWf+cAGw3tqMCYzGIU6yR8K+Y9NT2z23RwOjlNFF2HwSB3KhdEFI5lSBv8tNmFkkBShSjsCjzx1vahZfISA==", + "version": "8.58.0", + "resolved": "https://registry.npmjs.org/typescript-eslint/-/typescript-eslint-8.58.0.tgz", + "integrity": "sha512-e2TQzKfaI85fO+F3QywtX+tCTsu/D3WW5LVU6nz8hTFKFZ8yBJ6mSYRpXqdR3mFjPWmO0eWsTa5f+UpAOe/FMA==", "dev": true, "license": "MIT", "dependencies": { - "@typescript-eslint/eslint-plugin": "8.57.1", - "@typescript-eslint/parser": "8.57.1", - "@typescript-eslint/typescript-estree": "8.57.1", - "@typescript-eslint/utils": "8.57.1" + "@typescript-eslint/eslint-plugin": "8.58.0", + "@typescript-eslint/parser": "8.58.0", + "@typescript-eslint/typescript-estree": "8.58.0", + "@typescript-eslint/utils": "8.58.0" }, "engines": { "node": "^18.18.0 || ^20.9.0 || >=21.1.0" @@ -6028,7 +6042,7 @@ }, "peerDependencies": { "eslint": "^8.57.0 || ^9.0.0 || ^10.0.0", - "typescript": ">=4.8.4 <6.0.0" + "typescript": ">=4.8.4 <6.1.0" } }, "node_modules/uc.micro": { diff --git a/package.json b/package.json index 62c718e..731c945 100644 --- a/package.json +++ b/package.json @@ -58,17 +58,17 @@ "@eslint/js": "^10.0.1", "@eslint/json": "^1.2.0", "@types/jest": "^30.0.0", - "@types/node": "^25.5.0", - "eslint": "^10.1.0", + "@types/node": "^25.5.2", + "eslint": "^10.2.0", "eslint-config-prettier": "^10.1.8", "eslint-plugin-security": "^4.0.0", "globals": "^17.4.0", "jest": "^30.3.0", "prettier": "^3.8.1", - "ts-jest": "^29.4.6", + "ts-jest": "^29.4.9", "ts-node": "^10.9.2", "typedoc": "^0.28.18", - "typescript": "^5.9.3", - "typescript-eslint": "^8.57.1" + "typescript": "^6.0.2", + "typescript-eslint": "^8.58.0" } } From b732ce6ae7c1da67ec4aba974c217eafa041107f Mon Sep 17 00:00:00 2001 From: Abdeldjalil Fortas <9090674+Fcmam5@users.noreply.github.com> Date: Sun, 5 Apr 2026 23:55:55 +0200 Subject: [PATCH 06/11] fix: Address review comments --- eslint.config.cjs | 8 ++++++++ src/adapters/git.ts | 1 + src/core/filesystem.ts | 5 +++++ tests/adapters.git.test.ts | 12 ++---------- tests/describe.test.ts | 4 +++- 5 files changed, 19 insertions(+), 11 deletions(-) diff --git a/eslint.config.cjs b/eslint.config.cjs index 0ae33c7..0967761 100644 --- a/eslint.config.cjs +++ b/eslint.config.cjs @@ -36,6 +36,14 @@ module.exports = [ name: 'node:fs/promises', message: FILESYSTEM_IMPORT_MESSAGE, }, + { + name: 'fs', + message: FILESYSTEM_IMPORT_MESSAGE, + }, + { + name: 'fs/promises', + message: FILESYSTEM_IMPORT_MESSAGE, + }, ], }, ], diff --git a/src/adapters/git.ts b/src/adapters/git.ts index ccb9a9d..d7cfd21 100644 --- a/src/adapters/git.ts +++ b/src/adapters/git.ts @@ -94,6 +94,7 @@ export class ExecaGitClient implements GitClient { async exportPath(repoPath: string, commit: string, destination: string, subPath?: string): Promise { assertSafeGitArg(repoPath, 'repository path'); assertSafeGitArg(destination, 'destination path'); + resolveSafeSubPath(repoPath, subPath); if (!COMMIT_SHA_PATTERN.test(commit)) { throw new SkillInstallError(`Invalid commit SHA: ${commit}`); } diff --git a/src/core/filesystem.ts b/src/core/filesystem.ts index 6710a58..6ab623a 100644 --- a/src/core/filesystem.ts +++ b/src/core/filesystem.ts @@ -30,6 +30,11 @@ export class NodeFileSystem implements FileSystem { await fs.mkdir(target, { recursive: true }); } + /** + * Creates a new temporary directory. + * @param prefix Prefix used for the temporary directory name. + * @returns Promise resolving to the created directory path. + */ async mkdtemp(prefix: string): Promise { return fs.mkdtemp(prefix); } diff --git a/tests/adapters.git.test.ts b/tests/adapters.git.test.ts index 1426163..019daa8 100644 --- a/tests/adapters.git.test.ts +++ b/tests/adapters.git.test.ts @@ -86,15 +86,7 @@ describe('ExecaGitClient security hardening', () => { 'Invalid skill path ../escape', ); - expect(mockedExeca).toHaveBeenCalledWith( - 'git', - ['-C', '/tmp/cache/repo', 'worktree', 'add', '--force', '--detach', '/tmp/skilleton-wt-123', commit], - expect.any(Object), - ); - expect(mockedExeca).toHaveBeenCalledWith( - 'git', - ['-C', '/tmp/cache/repo', 'worktree', 'remove', '--force', '/tmp/skilleton-wt-123'], - expect.any(Object), - ); + expect(mockedFs.mkdtemp).not.toHaveBeenCalled(); + expect(mockedExeca).not.toHaveBeenCalled(); }); }); diff --git a/tests/describe.test.ts b/tests/describe.test.ts index 35e29bb..44b7290 100644 --- a/tests/describe.test.ts +++ b/tests/describe.test.ts @@ -248,6 +248,7 @@ function createThrowingProxy(name: string): ProxyHandler { class InMemoryFileSystem implements FileSystem { private directories = new Set(); private files = new Map(); + private tempCounter = 0; constructor() { this.directories.add('/'); @@ -275,7 +276,8 @@ class InMemoryFileSystem implements FileSystem { } async mkdtemp(prefix: string): Promise { - const tempDir = this.normalize(`${prefix}tmp`); + this.tempCounter += 1; + const tempDir = this.normalize(`${prefix}tmp-${this.tempCounter}`); this.directories.add(tempDir); return tempDir; } From 2d494ed7919059051154fa257bc4e0a09cc36e87 Mon Sep 17 00:00:00 2001 From: Abdeldjalil Fortas <9090674+Fcmam5@users.noreply.github.com> Date: Mon, 6 Apr 2026 00:00:32 +0200 Subject: [PATCH 07/11] refactor: extract MockedFileSystem to shared helper and improve coverage 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 --- jest.config.cjs | 1 + tests/adapters.git.test.ts | 20 +--- tests/describe.test.ts | 123 +--------------------- tests/helpers/mocked-filesystem.ts | 162 +++++++++++++++++++++++++++++ tests/install.test.ts | 17 +-- 5 files changed, 173 insertions(+), 150 deletions(-) create mode 100644 tests/helpers/mocked-filesystem.ts diff --git a/jest.config.cjs b/jest.config.cjs index 84b4296..f750b30 100644 --- a/jest.config.cjs +++ b/jest.config.cjs @@ -9,6 +9,7 @@ const config = { '^.+\\.(ts|tsx)$': 'ts-jest', }, collectCoverage: true, + collectCoverageFrom: ['src/**/*.ts'], }; module.exports = config; diff --git a/tests/adapters.git.test.ts b/tests/adapters.git.test.ts index 019daa8..e2bb7c6 100644 --- a/tests/adapters.git.test.ts +++ b/tests/adapters.git.test.ts @@ -1,31 +1,17 @@ import { execa } from 'execa'; import { ExecaGitClient } from '../src/adapters/git'; import { SkillInstallError } from '../src/core/errors'; -import { FileSystem } from '../src/core/types'; +import { MockedFileSystem, createMockedFileSystem } from './helpers/mocked-filesystem'; jest.mock('execa'); -const createFsMock = (): jest.Mocked => ({ - pathExists: jest.fn(), - ensureDir: jest.fn(), - mkdtemp: jest.fn(), - readJson: jest.fn(), - writeJson: jest.fn(), - readFile: jest.fn(), - isDirectory: jest.fn(), - remove: jest.fn(), - copy: jest.fn(), - symlink: jest.fn(), - readDir: jest.fn(), -}); - describe('ExecaGitClient security hardening', () => { - let mockedFs: jest.Mocked; + let mockedFs: MockedFileSystem; const mockedExeca = execa as unknown as jest.Mock; beforeEach(() => { jest.clearAllMocks(); - mockedFs = createFsMock(); + mockedFs = createMockedFileSystem(); mockedFs.pathExists.mockResolvedValue(true); mockedFs.ensureDir.mockResolvedValue(undefined); mockedFs.remove.mockResolvedValue(undefined); diff --git a/tests/describe.test.ts b/tests/describe.test.ts index 44b7290..2b6d837 100644 --- a/tests/describe.test.ts +++ b/tests/describe.test.ts @@ -3,6 +3,7 @@ import { DescribeCommand } from '../src/commands/describe'; import { SkilletonEnvironment } from '../src/env'; import { FileSystem, SkillLockfile, SkillManifest } from '../src/core/types'; import { ManifestNotFoundError } from '../src/core/errors'; +import { createMockedFileSystem } from './helpers/mocked-filesystem'; describe('DescribeCommand', () => { let consoleLogSpy: jest.SpyInstance; @@ -85,7 +86,7 @@ describe('DescribeCommand', () => { ], }; - const fs = new InMemoryFileSystem(); + const fs = createMockedFileSystem(); fs.addDirectory(installPath); fs.addDirectory(path.join(installPath, 'rules')); fs.addFile( @@ -131,7 +132,7 @@ describe('DescribeCommand', () => { ], }; - const fs = new InMemoryFileSystem(); + const fs = createMockedFileSystem(); fs.addDirectory(installPath); fs.addFile(path.join(installPath, 'README.md'), '# Readme'); const env = createEnv({ manifest, installPaths: { [skillName]: installPath }, fileSystem: fs }); @@ -158,7 +159,7 @@ describe('DescribeCommand', () => { ], }; - const fs = new InMemoryFileSystem(); + const fs = createMockedFileSystem(); fs.addDirectory(installPath); fs.addFile(path.join(installPath, 'SKILL.md'), '# Hello world'); const env = createEnv({ manifest, installPaths: { [skillName]: installPath }, fileSystem: fs }); @@ -193,7 +194,7 @@ function createEnv(options: CreateEnvOptions = {}): SkilletonEnvironment { options.manifest ?? ({ $schema: './skilleton.schema.json', skills: [] } as SkillManifest); const lockfile = options.lockfile ?? null; const installPaths = options.installPaths ?? {}; - const fs = options.fileSystem ?? new InMemoryFileSystem(); + const fs = options.fileSystem ?? createMockedFileSystem(); const manifestError = options.manifestError ?? null; const manifestRepo = { @@ -244,117 +245,3 @@ function createThrowingProxy(name: string): ProxyHandler { }, ); } - -class InMemoryFileSystem implements FileSystem { - private directories = new Set(); - private files = new Map(); - private tempCounter = 0; - - constructor() { - this.directories.add('/'); - } - - addDirectory(dir: string): void { - const normalized = this.normalize(dir); - this.directories.add(normalized); - } - - addFile(target: string, content: string): void { - const normalized = this.normalize(target); - const dir = path.dirname(normalized); - this.directories.add(dir); - this.files.set(normalized, content); - } - - async pathExists(target: string): Promise { - const normalized = this.normalize(target); - return this.directories.has(normalized) || this.files.has(normalized); - } - - async ensureDir(target: string): Promise { - this.directories.add(this.normalize(target)); - } - - async mkdtemp(prefix: string): Promise { - this.tempCounter += 1; - const tempDir = this.normalize(`${prefix}tmp-${this.tempCounter}`); - this.directories.add(tempDir); - return tempDir; - } - - async readJson(_path: string): Promise { - throw new Error('Not implemented'); - } - - async writeJson(_path: string, _data: unknown): Promise { - throw new Error('Not implemented'); - } - - async readFile(target: string): Promise { - const normalized = this.normalize(target); - const content = this.files.get(normalized); - if (content === undefined) { - throw new Error(`File not found: ${target}`); - } - return content; - } - - async isDirectory(target: string): Promise { - return this.directories.has(this.normalize(target)); - } - - async remove(_path: string): Promise { - throw new Error('Not implemented'); - } - - async copy(_src: string, _dest: string): Promise { - throw new Error('Not implemented'); - } - - async symlink(_target: string, _path: string): Promise { - throw new Error('Not implemented'); - } - - async readDir(target: string): Promise { - const normalized = this.normalize(target); - const entries = new Set(); - const prefix = normalized.endsWith('/') ? normalized : `${normalized}/`; - - for (const dir of this.directories) { - if (dir === normalized) { - continue; - } - if (!dir.startsWith(prefix)) { - continue; - } - const relative = dir.slice(prefix.length); - if (relative.length === 0) { - continue; - } - const segment = relative.split('/')[0]; - entries.add(segment); - } - - for (const filePath of this.files.keys()) { - if (!filePath.startsWith(prefix)) { - continue; - } - const relative = filePath.slice(prefix.length); - if (relative.length === 0) { - continue; - } - const segment = relative.split('/')[0]; - entries.add(segment); - } - - if (!this.directories.has(normalized) && entries.size === 0) { - throw new Error(`Directory not found: ${target}`); - } - - return Array.from(entries); - } - - private normalize(target: string): string { - return target.replace(/\\/g, '/'); - } -} diff --git a/tests/helpers/mocked-filesystem.ts b/tests/helpers/mocked-filesystem.ts new file mode 100644 index 0000000..5846f8e --- /dev/null +++ b/tests/helpers/mocked-filesystem.ts @@ -0,0 +1,162 @@ +import path from 'node:path'; +import { FileSystem } from '../../src/core/types'; + +export class MockedFileSystem implements FileSystem { + private directories = new Set(); + private files = new Map(); + private tempCounter = 0; + + constructor() { + this.directories.add('/'); + } + + addDirectory(dir: string): void { + const normalized = this.normalize(dir); + this.directories.add(normalized); + } + + addFile(target: string, content: string): void { + const normalized = this.normalize(target); + this.directories.add(path.dirname(normalized)); + this.files.set(normalized, content); + } + + pathExists = jest.fn(async (target: string): Promise => { + const normalized = this.normalize(target); + return this.directories.has(normalized) || this.files.has(normalized); + }); + + ensureDir = jest.fn(async (target: string): Promise => { + this.directories.add(this.normalize(target)); + }); + + mkdtemp = jest.fn(async (prefix: string): Promise => { + this.tempCounter += 1; + const tempDir = this.normalize(`${prefix}tmp-${this.tempCounter}`); + this.directories.add(tempDir); + return tempDir; + }); + + readJson = jest.fn(async (target: string): Promise => { + const content = await this.readFile(target); + return JSON.parse(content); + }) as jest.MockedFunction; + + writeJson = jest.fn(async (target: string, data: unknown): Promise => { + const normalized = this.normalize(target); + this.directories.add(path.dirname(normalized)); + this.files.set(normalized, `${JSON.stringify(data, null, 2)}\n`); + }); + + readFile = jest.fn(async (target: string): Promise => { + const normalized = this.normalize(target); + const content = this.files.get(normalized); + if (content === undefined) { + throw new Error(`File not found: ${target}`); + } + return content; + }); + + isDirectory = jest.fn(async (target: string): Promise => { + return this.directories.has(this.normalize(target)); + }); + + remove = jest.fn(async (target: string): Promise => { + const normalized = this.normalize(target); + this.files.delete(normalized); + this.directories.delete(normalized); + + for (const filePath of Array.from(this.files.keys())) { + if (filePath.startsWith(`${normalized}/`)) { + this.files.delete(filePath); + } + } + + for (const dirPath of Array.from(this.directories)) { + if (dirPath.startsWith(`${normalized}/`)) { + this.directories.delete(dirPath); + } + } + }); + + copy = jest.fn(async (src: string, dest: string): Promise => { + const srcNormalized = this.normalize(src); + const destNormalized = this.normalize(dest); + + if (this.files.has(srcNormalized)) { + const content = this.files.get(srcNormalized); + if (content !== undefined) { + this.directories.add(path.dirname(destNormalized)); + this.files.set(destNormalized, content); + } + return; + } + + if (this.directories.has(srcNormalized)) { + this.directories.add(destNormalized); + for (const dirPath of Array.from(this.directories)) { + if (dirPath.startsWith(`${srcNormalized}/`)) { + const relative = dirPath.slice(srcNormalized.length); + this.directories.add(`${destNormalized}${relative}`); + } + } + for (const [filePath, content] of Array.from(this.files.entries())) { + if (filePath.startsWith(`${srcNormalized}/`)) { + const relative = filePath.slice(srcNormalized.length); + const copiedFilePath = `${destNormalized}${relative}`; + this.directories.add(path.dirname(copiedFilePath)); + this.files.set(copiedFilePath, content); + } + } + return; + } + + throw new Error(`Path not found: ${src}`); + }); + + symlink = jest.fn(async (_target: string, dest: string): Promise => { + const normalizedDest = this.normalize(dest); + this.directories.add(path.dirname(normalizedDest)); + this.files.set(normalizedDest, ''); + }); + + readDir = jest.fn(async (target: string): Promise => { + const normalized = this.normalize(target); + const entries = new Set(); + const prefix = normalized.endsWith('/') ? normalized : `${normalized}/`; + + for (const dir of this.directories) { + if (dir === normalized || !dir.startsWith(prefix)) { + continue; + } + const relative = dir.slice(prefix.length); + if (relative.length > 0) { + entries.add(relative.split('/')[0]); + } + } + + for (const filePath of this.files.keys()) { + if (!filePath.startsWith(prefix)) { + continue; + } + const relative = filePath.slice(prefix.length); + if (relative.length > 0) { + entries.add(relative.split('/')[0]); + } + } + + if (!this.directories.has(normalized) && entries.size === 0) { + throw new Error(`Directory not found: ${target}`); + } + + return Array.from(entries); + }); + + private normalize(target: string): string { + return target.replace(/\\/g, '/'); + } +} + +export function createMockedFileSystem(): MockedFileSystem { + return new MockedFileSystem(); +} diff --git a/tests/install.test.ts b/tests/install.test.ts index 3d6a054..1ca9224 100644 --- a/tests/install.test.ts +++ b/tests/install.test.ts @@ -2,20 +2,7 @@ import path from 'node:path'; import { SkillInstaller } from '../src/core/install'; import { FileSystem, GitClient, LockedSkill } from '../src/core/types'; import { SkillInstallError } from '../src/core/errors'; - -const createFsMock = (): jest.Mocked => ({ - pathExists: jest.fn(), - ensureDir: jest.fn(), - mkdtemp: jest.fn(), - readJson: jest.fn(), - writeJson: jest.fn(), - readFile: jest.fn(), - isDirectory: jest.fn(), - remove: jest.fn(), - copy: jest.fn(), - symlink: jest.fn(), - readDir: jest.fn(), -}); +import { createMockedFileSystem } from './helpers/mocked-filesystem'; const createGitMock = (): jest.Mocked => ({ ensureRepo: jest.fn(), @@ -37,7 +24,7 @@ describe('SkillInstaller', () => { let installer: SkillInstaller; beforeEach(() => { - fsMock = createFsMock(); + fsMock = createMockedFileSystem(); gitMock = createGitMock(); installer = new SkillInstaller(fsMock, gitMock); gitMock.ensureRepo.mockResolvedValue('/cache/skills/alpha'); From c5feddbc483bda8891b69c9d83291275ab7ad703 Mon Sep 17 00:00:00 2001 From: Abdeldjalil Fortas <9090674+Fcmam5@users.noreply.github.com> Date: Mon, 6 Apr 2026 00:03:23 +0200 Subject: [PATCH 08/11] test: add coverage for git fetch, exportPath, and path validation - 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 --- tests/adapters.git.test.ts | 71 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/tests/adapters.git.test.ts b/tests/adapters.git.test.ts index e2bb7c6..cc9d178 100644 --- a/tests/adapters.git.test.ts +++ b/tests/adapters.git.test.ts @@ -39,6 +39,18 @@ describe('ExecaGitClient security hardening', () => { ); }); + it('fetches existing repositories instead of cloning', async () => { + const client = new ExecaGitClient('/tmp/cache', mockedFs); + + await client.ensureRepo('https://github.com/acme/skills'); + + expect(mockedExeca).toHaveBeenCalledWith( + 'git', + ['-C', '/tmp/cache/https_github.com_acme_skills', 'fetch', '--tags', '--prune', '--force'], + expect.any(Object), + ); + }); + it('rejects destination values that look like git options', async () => { const client = new ExecaGitClient('/tmp/cache', mockedFs); @@ -75,4 +87,63 @@ describe('ExecaGitClient security hardening', () => { expect(mockedFs.mkdtemp).not.toHaveBeenCalled(); expect(mockedExeca).not.toHaveBeenCalled(); }); + + it('rejects absolute subPath values before worktree allocation', async () => { + const client = new ExecaGitClient('/tmp/cache', mockedFs); + const commit = 'abcdef1234567890abcdef1234567890abcdef12'; + + await expect(client.exportPath('/tmp/cache/repo', commit, '/tmp/out', '/etc/passwd')).rejects.toThrow( + 'Invalid skill path /etc/passwd', + ); + + expect(mockedFs.mkdtemp).not.toHaveBeenCalled(); + expect(mockedExeca).not.toHaveBeenCalled(); + }); + + it('exports path successfully and cleans up worktree', async () => { + const client = new ExecaGitClient('/tmp/cache', mockedFs); + const commit = 'abcdef1234567890abcdef1234567890abcdef12'; + + await client.exportPath('/tmp/cache/repo', commit, '/tmp/out', 'skills/jest'); + + expect(mockedExeca).toHaveBeenNthCalledWith( + 1, + 'git', + ['-C', '/tmp/cache/repo', 'worktree', 'add', '--force', '--detach', '/tmp/skilleton-wt-123', commit], + expect.any(Object), + ); + expect(mockedFs.remove).toHaveBeenNthCalledWith(1, '/tmp/out'); + expect(mockedFs.copy).toHaveBeenCalledWith('/tmp/skilleton-wt-123/skills/jest', '/tmp/out'); + expect(mockedExeca).toHaveBeenNthCalledWith( + 2, + 'git', + ['-C', '/tmp/cache/repo', 'worktree', 'remove', '--force', '/tmp/skilleton-wt-123'], + expect.any(Object), + ); + expect(mockedFs.remove).toHaveBeenNthCalledWith(2, '/tmp/skilleton-wt-123'); + }); + + it('wraps missing source path errors and still cleans up worktree', async () => { + const client = new ExecaGitClient('/tmp/cache', mockedFs); + const commit = 'abcdef1234567890abcdef1234567890abcdef12'; + mockedFs.pathExists.mockResolvedValueOnce(false); + + await expect(client.exportPath('/tmp/cache/repo', commit, '/tmp/out', 'skills/missing')).rejects.toThrow( + 'Failed to export skills/missing from /tmp/cache/repo@abcdef1234567890abcdef1234567890abcdef12: Skill path skills/missing not found in repository /tmp/cache/repo', + ); + + expect(mockedExeca).toHaveBeenNthCalledWith( + 1, + 'git', + ['-C', '/tmp/cache/repo', 'worktree', 'add', '--force', '--detach', '/tmp/skilleton-wt-123', commit], + expect.any(Object), + ); + expect(mockedExeca).toHaveBeenNthCalledWith( + 2, + 'git', + ['-C', '/tmp/cache/repo', 'worktree', 'remove', '--force', '/tmp/skilleton-wt-123'], + expect.any(Object), + ); + expect(mockedFs.remove).toHaveBeenCalledWith('/tmp/skilleton-wt-123'); + }); }); From e2f67226a3f706afcc17edd3f6ae5ee50164fa1a Mon Sep 17 00:00:00 2001 From: Abdeldjalil Fortas <9090674+Fcmam5@users.noreply.github.com> Date: Mon, 6 Apr 2026 00:16:20 +0200 Subject: [PATCH 09/11] chore: suppress TypeScript 6.0 deprecation warnings temporarily --- tsconfig.json | 1 + 1 file changed, 1 insertion(+) diff --git a/tsconfig.json b/tsconfig.json index 921ddc6..5f88875 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -3,6 +3,7 @@ "target": "ES2022", "module": "commonjs", "moduleResolution": "node", + "ignoreDeprecations": "6.0", "strict": true, "esModuleInterop": true, "allowSyntheticDefaultImports": true, From 8b55ac5d646c4026f94c0c389052f886cd52823e Mon Sep 17 00:00:00 2001 From: Abdeldjalil Fortas <9090674+Fcmam5@users.noreply.github.com> Date: Mon, 6 Apr 2026 00:19:08 +0200 Subject: [PATCH 10/11] refactor: add addDirectoryTree helper to ensure parent directories exist in MockedFileSystem --- tests/helpers/mocked-filesystem.ts | 40 +++++++++++++++++++++--------- 1 file changed, 28 insertions(+), 12 deletions(-) diff --git a/tests/helpers/mocked-filesystem.ts b/tests/helpers/mocked-filesystem.ts index 5846f8e..c1c49bc 100644 --- a/tests/helpers/mocked-filesystem.ts +++ b/tests/helpers/mocked-filesystem.ts @@ -11,13 +11,12 @@ export class MockedFileSystem implements FileSystem { } addDirectory(dir: string): void { - const normalized = this.normalize(dir); - this.directories.add(normalized); + this.addDirectoryTree(this.normalize(dir)); } addFile(target: string, content: string): void { const normalized = this.normalize(target); - this.directories.add(path.dirname(normalized)); + this.addDirectoryTree(path.dirname(normalized)); this.files.set(normalized, content); } @@ -27,13 +26,13 @@ export class MockedFileSystem implements FileSystem { }); ensureDir = jest.fn(async (target: string): Promise => { - this.directories.add(this.normalize(target)); + this.addDirectoryTree(this.normalize(target)); }); mkdtemp = jest.fn(async (prefix: string): Promise => { this.tempCounter += 1; const tempDir = this.normalize(`${prefix}tmp-${this.tempCounter}`); - this.directories.add(tempDir); + this.addDirectoryTree(tempDir); return tempDir; }); @@ -44,7 +43,7 @@ export class MockedFileSystem implements FileSystem { writeJson = jest.fn(async (target: string, data: unknown): Promise => { const normalized = this.normalize(target); - this.directories.add(path.dirname(normalized)); + this.addDirectoryTree(path.dirname(normalized)); this.files.set(normalized, `${JSON.stringify(data, null, 2)}\n`); }); @@ -86,25 +85,25 @@ export class MockedFileSystem implements FileSystem { if (this.files.has(srcNormalized)) { const content = this.files.get(srcNormalized); if (content !== undefined) { - this.directories.add(path.dirname(destNormalized)); + this.addDirectoryTree(path.dirname(destNormalized)); this.files.set(destNormalized, content); } return; } if (this.directories.has(srcNormalized)) { - this.directories.add(destNormalized); + this.addDirectoryTree(destNormalized); for (const dirPath of Array.from(this.directories)) { if (dirPath.startsWith(`${srcNormalized}/`)) { const relative = dirPath.slice(srcNormalized.length); - this.directories.add(`${destNormalized}${relative}`); + this.addDirectoryTree(`${destNormalized}${relative}`); } } for (const [filePath, content] of Array.from(this.files.entries())) { if (filePath.startsWith(`${srcNormalized}/`)) { const relative = filePath.slice(srcNormalized.length); const copiedFilePath = `${destNormalized}${relative}`; - this.directories.add(path.dirname(copiedFilePath)); + this.addDirectoryTree(path.dirname(copiedFilePath)); this.files.set(copiedFilePath, content); } } @@ -116,7 +115,8 @@ export class MockedFileSystem implements FileSystem { symlink = jest.fn(async (_target: string, dest: string): Promise => { const normalizedDest = this.normalize(dest); - this.directories.add(path.dirname(normalizedDest)); + this.files.delete(normalizedDest); + this.addDirectoryTree(path.dirname(normalizedDest)); this.files.set(normalizedDest, ''); }); @@ -145,7 +145,7 @@ export class MockedFileSystem implements FileSystem { } } - if (!this.directories.has(normalized) && entries.size === 0) { + if (!this.directories.has(normalized)) { throw new Error(`Directory not found: ${target}`); } @@ -155,6 +155,22 @@ export class MockedFileSystem implements FileSystem { private normalize(target: string): string { return target.replace(/\\/g, '/'); } + + private addDirectoryTree(dir: string): void { + const normalized = this.normalize(dir); + if (!normalized || normalized === '/') { + this.directories.add('/'); + return; + } + + const segments = normalized.split('/').filter(Boolean); + let current = normalized.startsWith('/') ? '/' : ''; + + for (const segment of segments) { + current = current === '/' ? `/${segment}` : current ? `${current}/${segment}` : segment; + this.directories.add(current); + } + } } export function createMockedFileSystem(): MockedFileSystem { From a4ea7765c6e59a18415ba5de78217862655aef1f Mon Sep 17 00:00:00 2001 From: Abdeldjalil Fortas <9090674+Fcmam5@users.noreply.github.com> Date: Mon, 6 Apr 2026 00:40:34 +0200 Subject: [PATCH 11/11] docs: Add Advisory URL --- SECURITY.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/SECURITY.md b/SECURITY.md index 7c4bca0..9192b4c 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -4,9 +4,10 @@ | Version | Supported | | ------- | --------- | -| 0.1.x | ✅ | +| >=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. ## Reporting a Vulnerability