fix(install): prevent traversal in parsed source subdirectories#225
Conversation
…Options Reject unsafe subdirs that contain path traversal (..), NUL bytes, control characters, backslashes, absolute paths, and URL-encoded traversal (single, double, or overly deep encoding). Validate derived source names and CloneURLs for NUL and control characters. SEC003: github.com/owner/repo/../../etc/passwd previously produced a Source with Subdir containing traversal segments. Now rejected.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7fa1b5644f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if err := validateRepoSubdir(subdir); err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
Validate blob subdirs before trimming SKILL.md
When the input is a GitHub blob URL ending in SKILL.md, stripGitHubBranchPrefix calls trimSkillFileSuffix before this new validation runs; that helper uses filepath.Dir, which cleans traversal segments away. For example, https://github.com/user/repo/blob/main/skills/foo/../../SKILL.md reaches this check with an empty subdir and is accepted as the repo-root skill instead of being rejected like the other a/../b cases, leaving a traversal-shaped source able to bypass the new SEC003 parser validation. Validate the raw blob path before trimming, or trim without cleaning path segments.
Useful? React with 👍 / 👎.
Add SEC003 regression cases for SSH, file, Azure, and generic HTTPS source parser branches so traversal rejection stays covered beyond the GitHub parser.
Trim SKILL.md suffixes without filepath.Dir so traversal segments remain visible to SEC003 subdir validation. Add regression coverage for GitHub, GitLab, and Bitbucket blob-style skill URLs.
Fix: #224
Summary
This PR fixes unsafe repository subdirectory parsing in
ParseSourceWithOptions.The production change is limited to
internal/install/source.go. It adds parser-level validation for parsed source fields and applies it across the source parser branches.What changed
Added validation helpers:
validateRepoSubdir./..path segments.checkSubdirSafetyvalidateRepoSubdir.validateSourceName/and\..and...validateCloneURLIntegrated validation into these parser branches:
parseGitHubparseGitSSHparseSSHURLparseFileURLparseAzureOnPremparseAzureSSHparseGitHTTPSWhy
Several parser branches previously assigned regex-captured suffixes directly to
Source.Subdirand derivedSource.Namewithout validating the resulting fields.For example, an input such as:
could produce a
SourcewhoseSubdircontains traversal segments. If downstream install logic later joins thatSubdirwith a cloned repository root, the parsed value may escape the intended repository boundary.Behavior after this change
Rejected examples:
Still accepted examples:
Scope
This PR only changes source parsing and regression tests.
It does not change:
Validation
Ran: