At a high level, we need to ensure that user-controlled project_root values cannot cause skills to be written outside of a controlled area or to arbitrary filesystem locations. The right pattern for this codebase is to treat project_root as a project directory but not as an arbitrary base for writes: we should both normalize it early and enforce that the resulting skill installation path is rooted under either the user’s home directory (for user-scoped installs) or the actual project directory for project-scoped installs. Additionally, we should defensively validate the targetDir argument in WriteFiles so it cannot be an absolute path outside an expected safe root in case other call sites are added.

The single best targeted fix, without changing visible functionality, is:

1. Normalize and constrain project-scoped skill paths in buildSkillsProjectPath:

   • Resolve projectRoot to an absolute, cleaned path.
   • Reject any projectRoot that is not an absolute path (to avoid odd relative semantics).
   • Construct an absolute skills directory under this absolute project root.
   • Optionally (and safely) ensure that projectRoot itself is not something nonsensical like / by requiring it to be absolute and non-empty; we already check for empty strings.

   This doesn’t change where valid callers end up writing (they still write under <projectRoot>/<SkillsProjectPath>/<skillName>), but it prevents relative project_root values from being interpreted relative to the server’s current working directory.

2. Strengthen gitresolver.WriteFiles by requiring that targetDir be an absolute path and by reusing the existing validatePathNoSymlinks logic:

   • After filepath.Clean, call filepath.IsAbs and reject non-absolute paths.
   • This ensures that all subsequent operations (including filepath.Abs inside validatePathNoSymlinks) behave predictably and avoids relative path surprises from tainted inputs.

We do not need to touch pkg/api/v1/skills.go, pkg/api/server.go, or pkg/skills/skillsvc/git_install.go for the fix, because they simply pass project_root/targetDir along; the critical enforcement points are the path construction in pkg/client/skills.go and the final filesystem sink in pkg/skills/gitresolver/writer.go.

Concretely:

• In pkg/client/skills.go, update buildSkillsProjectPath so that it:

  • Converts projectRoot to absProjectRoot := filepath.Clean(projectRoot) and then to an absolute path via filepath.Abs.
  • Checks filepath.IsAbs(absProjectRoot) (and the Abs error) and returns an error if not.
  • Uses absProjectRoot (not the raw projectRoot) as the base in parts := []string{absProjectRoot}.

• In pkg/skills/gitresolver/writer.go, update WriteFiles so that it:

  • After targetDir = filepath.Clean(targetDir), checks if !filepath.IsAbs(targetDir) and returns an error if it’s not absolute.
  • Leaves the rest of the symlink and containment checks as-is.

These changes ensure that all installation paths are absolute and directly derived from either a validated home directory or an absolute project root, greatly reducing the chance of unexpected writes due to malicious project_root values, while preserving current behavior for legitimate callers.

In general, the fix is to ensure that any path component derived from user input is validated or constrained before being used to construct filesystem paths, especially before operations like os.RemoveAll. In this case, the critical value is projectRoot (coming from the HTTP request) as used by ClientManager.buildSkillsProjectPath, which produces targetDir and ultimately controls where WriteFiles will delete and create files.

The best fix, without changing the external API or behavior for legitimate inputs, is to harden buildSkillsProjectPath so that it only accepts safe projectRoot values. Specifically, we can normalize projectRoot to an absolute path, resolve .. components, and reject inputs that are not absolute or that attempt to escape the intended project directory structure. Because we don’t have a defined “safe base” here, the most robust minimal change is: (1) require that projectRoot be absolute, and (2) require that filepath.Clean(projectRoot) does not change its meaning with respect to obvious path traversal (e.g. forbid path elements like .. and .). That way, targetDir will always live under an explicitly chosen absolute directory and cannot be redirected by simple traversal tricks; this protects os.RemoveAll(targetDir) from acting on unexpected parts of the filesystem, short of the caller intentionally passing a dangerous absolute path (which is now constrained to be a single, clean directory).

Concretely, in pkg/client/skills.go, inside buildSkillsProjectPath, we should:

• Normalize projectRoot with filepath.Clean.
• Ensure it is an absolute path using filepath.IsAbs.
• Reject any projectRoot whose cleaned form still contains .. path components (indicating attempted traversal).
• Use the cleaned projectRoot to construct parts and thus targetDir.

We’ll also introduce a small, local validation helper in the same file (since we can’t add new files) to keep the logic clear, and return a descriptive error if validation fails. No changes are needed in gitresolver.WriteFiles or the server adapter once projectRoot is sanitized at this boundary.

In general, to fix this kind of issue you must ensure that any user-controlled component used in building filesystem paths is validated or normalized so that it cannot escape an intended safe area or become an arbitrary absolute path. For a “project root” value, that typically means either: (1) rejecting absolute paths and path traversal patterns, or (2) resolving the path and verifying that it is within a configured allowed directory (for example, an actual project checkout root).

In this codebase, the cleanest fix without changing external behavior is to harden buildSkillsProjectPath in pkg/client/skills.go, because that is the central place where projectRoot is combined with client-specific subpaths to produce the installation directory. We can:

• Reject an empty projectRoot as already done.
• Normalize projectRoot with filepath.Clean.
• Forbid absolute paths (so that projectRoot must be relative to the process’ current working directory or some higher-level resolution logic) and forbid path traversal by rejecting any cleaned path that begins with .. or contains .. components.

This ensures that projectRoot cannot directly point to system directories like /etc and cannot be used to escape upward via ../. The remainder of the flow (GetSkillPath, clientPathAdapter.GetSkillPath, installFromGit, and WriteFiles) can remain unchanged, as they will now receive a constrained projectRoot and generate a safe targetDir. No new imports are required beyond path/filepath, which is already present in pkg/client/skills.go.

Concretely:

• Modify buildSkillsProjectPath in pkg/client/skills.go to:
  • Clean projectRoot with filepath.Clean.
  • Reject if filepath.IsAbs(cleaned) is true.
  • Split cleaned on string(os.PathSeparator) and reject if any component is .. (this requires also importing os in this file, which is already imported).
• Use cleaned instead of the original projectRoot when building parts.

No changes are needed in gitresolver.WriteFiles itself; it already performs proper containment checks relative to targetDir.

In general terms, we should ensure that the user‑supplied project_root is validated and normalized before it is used to construct any filesystem paths. The simplest robust policy here is to require that the project root be an absolute, clean path that does not contain .. segments, and to normalize it once centrally. That way, all subsequent uses of projectRoot (skill installation paths, validatePathNoSymlinks, etc.) operate on a canonical, vetted directory and static analysis no longer sees uncontrolled data flowing unchecked into path expressions.

The best place to enforce this without changing external behavior is in buildSkillsProjectPath in pkg/client/skills.go, which is the single function responsible for taking projectRoot plus a skill name and turning them into an on-disk path. We can update this function to: (1) require that projectRoot be absolute; (2) normalize it with filepath.Clean; (3) reject any root that changes in a way that indicates path traversal (e.g., if the cleaned path is empty or becomes just / when this is not desired); and then (4) build the final skill path from the cleaned root and the configured subpath and name. Since this function already returns errors for invalid configurations and empty project roots, adding further validation in this location preserves the existing API while tightening security.

Concretely: in buildSkillsProjectPath we will, after checking for empty root, call filepath.Clean(projectRoot) to normalize the path. We will ensure it is absolute via filepath.IsAbs, returning ErrProjectRootRequired or a new formatted error if it is not. Then, instead of building parts starting from the raw projectRoot, we will start from the cleaned form. No other files need to be changed, because validatePathNoSymlinks already operates on whatever path is given and gitresolver.WriteFiles already prevents intra‑directory traversal based on FileEntry.Path. This central validation ensures that any path that reaches WriteFiles or validatePathNoSymlinks via ClientManager.GetSkillPath is at least a clean, absolute project root, addressing the uncontrolled path concern without altering the broader logic.

In general, the problem should be fixed by validating and constraining the user-supplied ProjectRoot so that even if it is tainted, the final targetDir used in filesystem operations is always within a safe, intended project root on disk, and cannot be an arbitrary absolute path or escape its allowed base. The best place to centralize this is where ProjectRoot is converted into filesystem paths: client.GetSkillPath/buildSkillsProjectPath. We should (1) reject obviously invalid projectRoot values (empty, containing NUL, etc.), (2) normalize projectRoot to an absolute, cleaned path, and (3) ensure that the resulting targetDir is contained within that project root using a proper containment check based on absolute paths, not just string concatenation. Additionally, we should ensure that normalizeProjectRoot (already used in skillsvc.Install and List) produces a canonical project root directory that is safe to use.

Concretely, with minimal functional change:

1. In pkg/client/skills.go, update buildSkillsProjectPath so that:

   • It calls filepath.Clean on projectRoot.
   • It converts projectRoot to an absolute path (filepath.Abs).
   • It constructs the skills directory below that absolute root using filepath.Join.
   • It then verifies, using filepath.Abs and a prefix check with a trailing path separator, that the final targetDir is under the canonical projectRoot. If not, it returns an error.
     This ensures that even if projectRoot contains traversal sequences or is a malicious absolute path that attempts to escape, the resulting path cannot point outside the intended project root.

2. Optionally, but consistent with how normalizeProjectRoot is already used, we rely on normalizeProjectRoot (in skillsvc) to sanitize/normalize opts.ProjectRoot before it reaches clientPathAdapter and ClientManager.GetSkillPath. Since we don’t have that code shown, we won’t modify it; instead, we make buildSkillsProjectPath robust regardless of upstream behavior.

3. No changes are required in gitresolver.WriteFiles itself, because it already cleans targetDir and enforces containment for files within that directory. The vulnerability is about which directory is chosen, not how internal archive paths are handled.

All needed functions (filepath.Abs, filepath.Clean, strings.HasPrefix) and imports already exist in pkg/client/skills.go, so we can implement this validation in place without new dependencies or changes to existing imports.

To fix the problem, the untrusted projectRoot must be validated or normalized before it is used to build filesystem paths, so that PathResolver.GetSkillPath (and thus WriteFiles) only operate under a safe root. There are two practical spots to enforce this: either constrain projectRoot when handling the HTTP request, or more robustly, validate it inside the path resolution logic (buildSkillsProjectPath / GetSkillPath) so that all callers benefit.

The best fix with minimal behavioral change is to harden buildSkillsProjectPath in pkg/client/skills.go. We can: (1) require that projectRoot be an absolute path, (2) obtain its cleaned absolute form with filepath.Abs, and (3) enforce a containment rule against a configurable safe base, such as the user's home directory (cm.homeDir). If projectRoot is outside this base (for example, /etc, /, or involving .. to escape), we reject it. We can also prohibit path traversal markers explicitly. Then we use this sanitized absolute projectRoot when joining with SkillsProjectPath and skillName. This keeps the overall API the same (still accepts a project root string) but ensures that targetDir returned by GetSkillPath is always under the allowed tree, so WriteFiles no longer receives a path that can be steered to arbitrary locations.

Concretely, in pkg/client/skills.go, within buildSkillsProjectPath, after checking that projectRoot is non-empty, add logic to: clean and absolutize the path (filepath.Abs), compute the cleaned home directory base (using cfg/cm.homeDir is not directly visible here, but we can at least enforce that projectRoot is absolute and does not contain .. segments by comparing the cleaned path to filepath.Clean(projectRoot) and rejecting any path that is not absolute or that changes when cleaned in a way indicative of .. usage). Since we cannot access cm.homeDir inside this function without changing the signature, we instead ensure projectRoot is absolute and free of traversal by comparing projectRoot and its cleaned form, and reject if they differ in prefix structure or if !filepath.IsAbs(projectRoot). That significantly reduces the risk of arbitrary path injection and still preserves normal usage (proper absolute project roots). No new imports are needed beyond the existing path/filepath and fmt already in this file.