Skip to content

Conversation

@lufftw
Copy link

@lufftw lufftw commented Dec 28, 2025

Summary

This PR refactors path validation to use path.relative() and fixes
Windows compatibility issues.

Problem

The original validation logic had two issues:

  1. Windows incompatibility: Hardcoded '/' separator caused false
    rejections of valid installs on Windows

  2. Code clarity: String-based startsWith() check is less clear
    than filesystem-aware path.relative()

Non-issue: Path Traversal

The existing code is already safe from path traversal attacks because:

  • All skill names pass through basename() which strips ../
  • All paths are constructed with join(targetDir, basename(...))
  • This prevents any directory escapes

The tests added in this PR demonstrate hypothetical attack scenarios
if the code were written differently, but these scenarios don't
reflect actual exploitable vulnerabilities in the current implementation.

Solution

Use path.relative() for clearer intent and cross-platform compatibility:

const relative = path.relative(resolvedTargetDir, resolvedTargetPath);
if (relative.startsWith("..") || path.isAbsolute(relative)) {
  throw new Error("Installation path outside target directory");
}

Impact

  • ✅ Fixes Windows installation failures (primary benefit)
  • ✅ Improves code maintainability and clarity
  • ✅ Adds comprehensive test coverage for edge cases
  • ℹ️ No actual security vulnerabilities fixed (code was already safe)

Replace string-based prefix matching with `path.relative` to ensure
installation paths stay within the target directory. This prevents
attackers from bypassing directory constraints using similar-named
folders or relative path manipulation.
- Implement cross-platform path handling using `path.sep` and `os.homedir()`.
- Add extensive test suite for path traversal attack scenarios:
  - Supply Chain attacks (malicious GitHub repos)
  - Zip Slip attacks (shell config hijacking)
  - Directory name collisions (e.g., skills-backup vs skills)
  - Edge cases including null bytes and Windows-specific paths.
- Update `expandPath` tests to support cross-platform absolute paths.
- Refactor security check logic to use `path.relative()` for better robustness.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant