-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add proxy git client for repository cloning via Semaphore server #3260
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
Co-authored-by: fiftin <[email protected]>
// Clone the repository | ||
err = customGitRepo.Clone() | ||
if err != nil { | ||
log.WithError(err).WithFields(log.Fields{ |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Sensitive data returned by an access to Password
Sensitive data returned by an access to Password
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
The best way to fix this problem is to ensure that any data containing sensitive fields—such as a Git URL that may have username:password@hostname
—is either omitted, obfuscated, or carefully redacted before being logged. Specifically, when logging errors related to repository operations, we should avoid logging sensitive values, or sanitize the error and associated fields before logging.
In this case, the problematic log line is:
log.WithError(err).WithFields(log.Fields{
"git_url": req.GitURL,
"git_branch": req.GitBranch,
}).Error("Failed to clone repository")
The err
itself may include the sensitive password, and req.GitURL
is known-safe (doesn't include credentials), but downstream errors (originating from e.g., constructed clone URLs with credentials) could inject sensitive data.
To fix:
- Sanitize
err.Error()
before logging:- Redact credentials from any URLs embedded in the error string prior to logging.
- Alternatively, log only a generic error message without the actual error details, or explicitly check and redact known patterns.
Implementation:
- Introduce a helper to sanitize sensitive data from error messages.
- Before logging, pass the error through this helper, and log only the sanitized error message.
- This should be applied to the affected log statement in
api/runners/runners.go
.
-
Copy modified line R30 -
Copy modified lines R33-R42 -
Copy modified lines R444-R446 -
Copy modified lines R449-R450
@@ -27,8 +27,19 @@ | ||
"github.com/semaphoreui/semaphore/services/tasks" | ||
"github.com/semaphoreui/semaphore/util" | ||
log "github.com/sirupsen/logrus" | ||
"regexp" | ||
) | ||
|
||
|
||
// sanitizeSensitiveError redacts credentials in known URL patterns from error messages | ||
func sanitizeSensitiveError(msg string) string { | ||
// This regex will replace username:password@ in URLs with username:***@ | ||
// Covers http/https/git URLs in the error string | ||
// e.g., https://user:[email protected] -> https://user:***@github.com | ||
re := regexp.MustCompile(`(https?://)([^:/\s]+):([^@/\s]+)@`) | ||
return re.ReplaceAllString(msg, "${1}${2}:***@") | ||
} | ||
|
||
func RunnerMiddleware(next http.Handler) http.Handler { | ||
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
|
||
@@ -432,11 +441,13 @@ | ||
// Clone the repository | ||
err = customGitRepo.Clone() | ||
if err != nil { | ||
log.WithError(err).WithFields(log.Fields{ | ||
// Sanitize sensitive data from err before logging | ||
safeErrMsg := sanitizeSensitiveError(err.Error()) | ||
log.WithFields(log.Fields{ | ||
"git_url": req.GitURL, | ||
"git_branch": req.GitBranch, | ||
}).Error("Failed to clone repository") | ||
helpers.WriteErrorStatus(w, "Failed to clone repository: "+err.Error(), http.StatusBadRequest) | ||
}).Error("Failed to clone repository: " + safeErrMsg) | ||
helpers.WriteErrorStatus(w, "Failed to clone repository: "+safeErrMsg, http.StatusBadRequest) | ||
return | ||
} | ||
|
type TaskStatus = task_logger.TaskStatus | ||
|
||
func (l *simpleLogger) Log(message string) { | ||
log.Info(message) |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Sensitive data returned by an access to Password
Sensitive data returned by an access to Password
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
To prevent leaking passwords in log messages, especially when logging file paths that are derived from URLs containing credentials, we should sanitize such inputs before logging. Specifically, in the context provided, whenever logging a file path (e.g., a requirements file path) that was constructed from a potentially sensitive repository URL, we need to remove any embedded credentials before writing to the log.
The best way to achieve this, without altering existing functionality, is to introduce a helper function that strips credentials from URLs. For example, we can use Go's net/url
package (standard library, well-known) to parse the URL and drop the User
field, then reconstruct the sanitized URL. We'll apply this sanitization right before logging the filename/path.
Concrete steps:
- In
db_lib/AnsibleApp.go
, modify logging statements that log a requirements file path, specifically the call tot.Log(requirementsFilePath + " has no changes. Skip galaxy install process.\n")
in functioninstallGalaxyRequirementsFile
. Sanitize the path/URL for any embedded credentials. - (Optional/future-proofing) If there are other logging statements in the flow where URLs with potential credentials are logged, apply similar sanitization.
- Define a helper method to sanitize embedded credentials from URLs/paths. Use Go's
net/url
package for parsing and reconstructing. - Add the
net/url
import if not already present.
No changes are needed in the generic logger (simpleLogger
), as the fix should be applied at the source of the potentially problematic log call.
-
Copy modified line R12 -
Copy modified lines R90-R101 -
Copy modified line R106 -
Copy modified line R120
@@ -9,6 +9,7 @@ | ||
|
||
"github.com/semaphoreui/semaphore/db" | ||
"github.com/semaphoreui/semaphore/pkg/task_logger" | ||
"net/url" | ||
) | ||
|
||
func getMD5Hash(filepath string) (string, error) { | ||
@@ -86,11 +87,23 @@ | ||
return t.Repository.GetFullPath(t.Template.ID) | ||
} | ||
|
||
// sanitizeRequirementsFilePath removes credentials from URLs that may be embedded in a file path. | ||
func sanitizeRequirementsFilePath(path string) string { | ||
// Only attempt to sanitize if it looks like a URL | ||
u, err := url.Parse(path) | ||
if err != nil || u.Scheme == "" || u.Host == "" { | ||
// Not a URL: return as is | ||
return path | ||
} | ||
u.User = nil | ||
return u.String() | ||
} | ||
|
||
func (t *AnsibleApp) installGalaxyRequirementsFile(requirementsType GalaxyRequirementsType, requirementsFilePath string) error { | ||
requirementsHashFilePath := fmt.Sprintf("%s_%s.md5", requirementsFilePath, requirementsType) | ||
|
||
if _, err := os.Stat(requirementsFilePath); err != nil { | ||
t.Log("No " + requirementsFilePath + " file found. Skip galaxy install process.\n") | ||
t.Log("No " + sanitizeRequirementsFilePath(requirementsFilePath) + " file found. Skip galaxy install process.\n") | ||
return nil | ||
} | ||
|
||
@@ -108,7 +117,7 @@ | ||
return err | ||
} | ||
} else { | ||
t.Log(requirementsFilePath + " has no changes. Skip galaxy install process.\n") | ||
t.Log(sanitizeRequirementsFilePath(requirementsFilePath) + " has no changes. Skip galaxy install process.\n") | ||
} | ||
|
||
return nil |
} | ||
|
||
func (l *simpleLogger) LogCmd(cmd *exec.Cmd) { | ||
log.Infof("Executing command: %v", cmd) |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Sensitive data returned by an access to Password
Sensitive data returned by an access to Password
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
The logging call in func (l *simpleLogger) LogCmd(cmd *exec.Cmd)
in api/runners/runners.go
currently logs the entire cmd
object. Since command arguments may contain sensitive credentials (e.g., in URLs), we should redact credentials before logging, or log only safe, non-sensitive portions of the command. The best fix is to sanitize command arguments before logging.
Approach:
- Write a helper function in
api/runners/runners.go
to redact any authentication information in URLs, such as user:password@host constructs. - In
LogCmd
, prepare a string version of the command and apply the sanitization function to all arguments before building/logging. - Only make changes inside the shown
runners.go
code. - Add helper function for redacting passwords from URLs using regular expressions.
- Change LogCmd so that, instead of logging cmd verbatim, it logs the executable and the sanitized arguments.
-
Copy modified lines R19-R20 -
Copy modified lines R508-R514 -
Copy modified lines R516-R521
@@ -16,6 +16,8 @@ | ||
"os/exec" | ||
"path/filepath" | ||
"time" | ||
"regexp" | ||
"strings" | ||
|
||
"github.com/semaphoreui/semaphore/api/helpers" | ||
"github.com/semaphoreui/semaphore/db" | ||
@@ -503,8 +505,20 @@ | ||
log.Infof(format, a...) | ||
} | ||
|
||
// redactCredentialsFromURL redacts user:password from URLs for logging. | ||
func redactCredentialsFromURL(s string) string { | ||
// Matches protocols http(s)/ftp/git/ssh/... followed by user:password@ and removes credentials | ||
re := regexp.MustCompile(`(\w+://)([^/@]+@)`) | ||
return re.ReplaceAllString(s, "$1[REDACTED]@") | ||
} | ||
|
||
func (l *simpleLogger) LogCmd(cmd *exec.Cmd) { | ||
log.Infof("Executing command: %v", cmd) | ||
// Sanitize each argument in Args | ||
safeArgs := make([]string, len(cmd.Args)) | ||
for i, arg := range cmd.Args { | ||
safeArgs[i] = redactCredentialsFromURL(arg) | ||
} | ||
log.Infof("Executing command: %s %s", redactCredentialsFromURL(cmd.Path), strings.Join(safeArgs, " ")) | ||
} | ||
|
||
func (l *simpleLogger) SetStatus(status TaskStatus) { |
|
||
// Create target directory | ||
targetPath := r.GetFullPath() | ||
err = os.MkdirAll(targetPath, 0755) |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
To remediate this vulnerability, strictly validate and constrain all filesystem path manipulations derived from user input:
- Before using
targetPath
, ensure that it is normalized (withfilepath.Abs
) and that it remains contained within a designated safe base directory (e.g., a project temp dir, as already present in parts of the codebase). - Only allow repository locations inside intended directories.
- Implement a function (
isPathWithinBase(base, target)
) to verify that the absolutetargetPath
resides under the allowed base directory. - Reject or error if the check fails, and ideally log this incident for audit.
Required Changes:
- In
db_lib/ProxyGitClient.go
, in theClone
function:- Before creating
targetPath
, resolve and validate it. - Use a helper to compute and enforce containment of
targetPath
in a safe base. - If invalid, return an error.
- Add the helper within the same file for context, and add necessary imports.
- Before creating
- Use
filepath.Abs
and string prefix checks to guard traversal or absolute path attacks.
-
Copy modified lines R18-R26 -
Copy modified lines R64-R66 -
Copy modified lines R68-R78
@@ -15,6 +15,15 @@ | ||
"github.com/semaphoreui/semaphore/util" | ||
) | ||
|
||
// isPathWithinBase returns true if path is within the base directory. | ||
func isPathWithinBase(base, path string) bool { | ||
rel, err := filepath.Rel(base, path) | ||
if err != nil { | ||
return false | ||
} | ||
return !strings.HasPrefix(rel, "..") && rel != ".." | ||
} | ||
|
||
type ProxyGitClient struct { | ||
keyInstaller AccessKeyInstaller | ||
} | ||
@@ -52,8 +61,21 @@ | ||
|
||
// Create target directory | ||
targetPath := r.GetFullPath() | ||
err = os.MkdirAll(targetPath, 0755) | ||
// Secure: ensure targetPath is within a safe base directory | ||
baseDir := util.Config.GetProjectTmpDir(r.Repository.ProjectID) | ||
absTargetPath, err := filepath.Abs(targetPath) | ||
if err != nil { | ||
return fmt.Errorf("failed to resolve target path: %w", err) | ||
} | ||
absBaseDir, err := filepath.Abs(baseDir) | ||
if err != nil { | ||
return fmt.Errorf("failed to resolve base directory: %w", err) | ||
} | ||
if !isPathWithinBase(absBaseDir, absTargetPath) { | ||
return fmt.Errorf("repository extraction path escapes the allowed directory") | ||
} | ||
err = os.MkdirAll(absTargetPath, 0755) | ||
if err != nil { | ||
return fmt.Errorf("failed to create target directory: %w", err) | ||
} | ||
|
func (c ProxyGitClient) GetLastCommitMessage(r GitRepository) (string, error) { | ||
// Try to read from .git/COMMIT_EDITMSG or use git command if available | ||
gitDir := filepath.Join(r.GetFullPath(), ".git") | ||
if _, err := os.Stat(gitDir); os.IsNotExist(err) { |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
The core issue is that user-controlled input can result in arbitrary file paths. The correct solution is to ensure that any file or directory accessed based on user input is verified to be within an allowed base directory (the "safe folder"). This can be done by resolving the resulting path to its absolute form and ensuring that it starts with the base directory path (similarly to the example in the background).
There are two main avenues:
- For repository types that treat GitURL as a pathname (e.g., local repositories), ensure that only expected subdirectories of a trusted base directory are allowed.
- For other repository types, ensure any derived paths are validated before any file operation is performed.
Thus, in the methods that interact with paths constructed from user input, insert a path validation routine that:
- Resolves the path to an absolute canonical path.
- Ensures this canonical path starts with the allowed directory root.
- If the validation fails, return an error or handle accordingly.
Specifically:
- In
ProxyGitClient.GetLastCommitMessage
andProxyGitClient.GetLastCommitHash
, add validation ongitDir
(the path to.git
). - Validation logic should be extracted into a helper function for clarity and reuse.
- Because only common packages can be used, use
filepath.Abs
andfilepath.Clean
for canonicalization. - Determine the base safe directory (could be the result of
util.Config.GetProjectTmpDir()
), and ensure all accessed paths are inside this directory.
You only need to modify the shown code in db_lib/ProxyGitClient.go
, so insert the helper and use it where needed.
-
Copy modified lines R18-R41 -
Copy modified lines R123-R129 -
Copy modified line R132 -
Copy modified lines R139-R145 -
Copy modified line R148
@@ -15,6 +15,30 @@ | ||
"github.com/semaphoreui/semaphore/util" | ||
) | ||
|
||
|
||
// validateRepoPath ensures that the given path is within the allowed base directory. | ||
// Returns cleaned absolute path or an error. | ||
func validateRepoPath(repoPath string, baseDir string) (string, error) { | ||
absRepoPath, err := filepath.Abs(repoPath) | ||
if err != nil { | ||
return "", fmt.Errorf("unable to resolve repository path: %w", err) | ||
} | ||
absBaseDir, err := filepath.Abs(baseDir) | ||
if err != nil { | ||
return "", fmt.Errorf("unable to resolve base directory: %w", err) | ||
} | ||
// Ensure trailing separator for exact prefix match | ||
baseDirPrefix := absBaseDir | ||
if !strings.HasSuffix(baseDirPrefix, string(os.PathSeparator)) { | ||
baseDirPrefix += string(os.PathSeparator) | ||
} | ||
// Also consider direct match to base directory (e.g. repository directly in base dir) | ||
if absRepoPath == absBaseDir || strings.HasPrefix(absRepoPath, baseDirPrefix) { | ||
return absRepoPath, nil | ||
} | ||
return "", fmt.Errorf("repository path is outside the allowed directory") | ||
} | ||
|
||
type ProxyGitClient struct { | ||
keyInstaller AccessKeyInstaller | ||
} | ||
@@ -96,10 +120,16 @@ | ||
func (c ProxyGitClient) GetLastCommitMessage(r GitRepository) (string, error) { | ||
// Try to read from .git/COMMIT_EDITMSG or use git command if available | ||
gitDir := filepath.Join(r.GetFullPath(), ".git") | ||
if _, err := os.Stat(gitDir); os.IsNotExist(err) { | ||
// Validate path - allow only paths in the allowed project tmp directory | ||
baseDir := util.Config.GetProjectTmpDir(r.Repository.ProjectID) | ||
validatedGitDir, err := validateRepoPath(gitDir, baseDir) | ||
if err != nil { | ||
return "", fmt.Errorf("invalid repository directory: %w", err) | ||
} | ||
if _, err := os.Stat(validatedGitDir); os.IsNotExist(err) { | ||
return "Repository cloned via proxy", nil | ||
} | ||
|
||
// Fallback to using cmd git client for local operations | ||
cmdClient := CmdGitClient{keyInstaller: c.keyInstaller} | ||
return cmdClient.GetLastCommitMessage(r) | ||
@@ -108,10 +136,16 @@ | ||
func (c ProxyGitClient) GetLastCommitHash(r GitRepository) (string, error) { | ||
// Try to read from local git info or use git command if available | ||
gitDir := filepath.Join(r.GetFullPath(), ".git") | ||
if _, err := os.Stat(gitDir); os.IsNotExist(err) { | ||
// Validate path - allow only paths in the allowed project tmp directory | ||
baseDir := util.Config.GetProjectTmpDir(r.Repository.ProjectID) | ||
validatedGitDir, err := validateRepoPath(gitDir, baseDir) | ||
if err != nil { | ||
return "", fmt.Errorf("invalid repository directory: %w", err) | ||
} | ||
if _, err := os.Stat(validatedGitDir); os.IsNotExist(err) { | ||
return "unknown", nil | ||
} | ||
|
||
// Fallback to using cmd git client for local operations | ||
cmdClient := CmdGitClient{keyInstaller: c.keyInstaller} | ||
return cmdClient.GetLastCommitHash(r) |
func (c ProxyGitClient) GetLastCommitHash(r GitRepository) (string, error) { | ||
// Try to read from local git info or use git command if available | ||
gitDir := filepath.Join(r.GetFullPath(), ".git") | ||
if _, err := os.Stat(gitDir); os.IsNotExist(err) { |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
To fix the problem, all file paths derived from user-controlled data must be validated before they are used in filesystem operations. Specifically, anything returned by GetFullPath()
for a GitRepository
must be checked to confirm it resides within a safe base directory. This can be performed by resolving the absolute path, and ensuring it is a subdirectory of a known safe location (for example, util.Config.GetProjectTmpDir(r.Repository.ProjectID)
).
The best approach is to add a validation step before any filesystem access using these paths. Since the code constructs .git
subpaths using filepath.Join(r.GetFullPath(), ".git")
, the checks should be performed immediately prior to filesystem operations in affected methods such as GetLastCommitHash
, GetLastCommitMessage
, and Pull
methods in ProxyGitClient
(db_lib/ProxyGitClient.go). For more robust defense, the GetFullPath()
function itself should avoid returning unsafe paths, but given the code shown, per-use validation provides defences closest to the vulnerability.
To implement the changes, import "path/filepath"
and "strings"
if needed, and before any call to os.Stat
, os.RemoveAll
, etc. with derived paths, ensure:
- The resolved absolute path starts with the literal absolute path of the corresponding safe working directory.
- If not, return an error and avoid performing the operation.
This does not change existing functionality, except to reject unsafe requests.
-
Copy modified lines R76-R83 -
Copy modified line R87 -
Copy modified lines R103-R108 -
Copy modified lines R121-R126
@@ -73,11 +73,18 @@ | ||
|
||
// For proxy mode, we'll just re-clone since the server provides fresh archives | ||
// This simplifies the implementation and ensures we always have the latest version | ||
err := os.RemoveAll(r.GetFullPath()) | ||
// Validate that repository path is within allowed base directory (prevents traversal attacks) | ||
repoBaseDir := util.Config.GetProjectTmpDir(r.Repository.ProjectID) | ||
absRepoPath, err := filepath.Abs(r.GetFullPath()) | ||
absBaseDir, err2 := filepath.Abs(repoBaseDir) | ||
if err != nil || err2 != nil || !strings.HasPrefix(absRepoPath, absBaseDir) { | ||
return fmt.Errorf("Invalid or unsafe repository path") | ||
} | ||
err = os.RemoveAll(r.GetFullPath()) | ||
if err != nil { | ||
return fmt.Errorf("failed to remove existing repository: %w", err) | ||
} | ||
|
||
return c.Clone(r) | ||
} | ||
|
||
@@ -96,6 +100,12 @@ | ||
func (c ProxyGitClient) GetLastCommitMessage(r GitRepository) (string, error) { | ||
// Try to read from .git/COMMIT_EDITMSG or use git command if available | ||
gitDir := filepath.Join(r.GetFullPath(), ".git") | ||
repoBaseDir := util.Config.GetProjectTmpDir(r.Repository.ProjectID) | ||
absGitDir, err := filepath.Abs(gitDir) | ||
absBaseDir, err2 := filepath.Abs(repoBaseDir) | ||
if err != nil || err2 != nil || !strings.HasPrefix(absGitDir, absBaseDir) { | ||
return "", fmt.Errorf("Invalid or unsafe repository path") | ||
} | ||
if _, err := os.Stat(gitDir); os.IsNotExist(err) { | ||
return "Repository cloned via proxy", nil | ||
} | ||
@@ -108,6 +118,12 @@ | ||
func (c ProxyGitClient) GetLastCommitHash(r GitRepository) (string, error) { | ||
// Try to read from local git info or use git command if available | ||
gitDir := filepath.Join(r.GetFullPath(), ".git") | ||
repoBaseDir := util.Config.GetProjectTmpDir(r.Repository.ProjectID) | ||
absGitDir, err := filepath.Abs(gitDir) | ||
absBaseDir, err2 := filepath.Abs(repoBaseDir) | ||
if err != nil || err2 != nil || !strings.HasPrefix(absGitDir, absBaseDir) { | ||
return "", fmt.Errorf("Invalid or unsafe repository path") | ||
} | ||
if _, err := os.Stat(gitDir); os.IsNotExist(err) { | ||
return "unknown", nil | ||
} |
|
||
// Create directory if it's a directory entry | ||
if header.Typeflag == tar.TypeDir { | ||
err = os.MkdirAll(targetFilePath, os.FileMode(header.Mode)) |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
In general terms:
You must ensure that all file system paths derived from untrusted data, especially during extraction of archives (tar, zip, etc.), are validated to prevent path traversal. This means, for every extracted entry, verifying that its target path is actually within the intended extraction directory after all symlink and relative resolution.
Best fix for the given code:
In extractArchive
, after constructing targetFilePath
, resolve its absolute path and ensure it starts with the absolute path of targetPath
plus a path separator (to prevent /some/dir_traversal_is_here/evil
matching /some/dir
). Alternatively, use Go's filepath.Rel
to compute the path relative to targetPath
and reject any that resolve to a path outside (filepath.Rel
returns a path starting with ..
in such cases).
Precisely, in db_lib/ProxyGitClient.go
, in the extractArchive
method:
- After calculating
targetFilePath
, get its absolute path and verify it's within the target directory. - If it's not, skip extraction.
- You will need to add logic for this check (and possibly import
"path/filepath"
and/or"strings"
for compatibility). - Only filter entries after joining with
targetPath
, and do not rely only on presence of".."
in the cleaned header name.
-
Copy modified lines R204-R218
@@ -198,12 +198,24 @@ | ||
|
||
// Clean the path to prevent path traversal | ||
cleanPath := filepath.Clean(header.Name) | ||
if strings.Contains(cleanPath, "..") { | ||
continue | ||
} | ||
|
||
targetFilePath := filepath.Join(targetPath, cleanPath) | ||
|
||
// Ensure targetFilePath is within targetPath | ||
absTargetPath, err := filepath.Abs(targetPath) | ||
if err != nil { | ||
return fmt.Errorf("failed to resolve absolute target directory: %w", err) | ||
} | ||
absFilePath, err := filepath.Abs(targetFilePath) | ||
if err != nil { | ||
return fmt.Errorf("failed to resolve absolute file path: %w", err) | ||
} | ||
rel, err := filepath.Rel(absTargetPath, absFilePath) | ||
if err != nil || strings.HasPrefix(rel, "..") || rel == "." || filepath.IsAbs(rel) { | ||
// skip files trying to escape the targetDir or writing directly to extraction root | ||
continue | ||
} | ||
|
||
// Create directory if it's a directory entry | ||
if header.Typeflag == tar.TypeDir { | ||
err = os.MkdirAll(targetFilePath, os.FileMode(header.Mode)) |
} | ||
|
||
// Create parent directories if they don't exist | ||
err = os.MkdirAll(filepath.Dir(targetFilePath), 0755) |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
To robustly fix uncontrolled path usage during tar archive extraction, we should validate that every output path (after joining the cleaned header name with the destination directory) stays strictly within the intended base directory. This is achieved by:
- Resolving the absolute path of both the target base directory (
targetPath
) and the destination file (targetFilePath
). - Ensuring that the absolute path of the destination file starts with the absolute base directory. If not, skip extraction for that entry.
- Additionally, skip header names that are absolute paths.
Implement these checks inside the extraction loop in extractArchive
(db_lib/ProxyGitClient.go).
No additional imports are needed, as filepath.Abs
, filepath.Join
, and strings.HasPrefix
are already imported.
Regions to change:
extractArchive
method in db_lib/ProxyGitClient.go, specifically after computingtargetFilePath
, before attempting to create the directory or file.
-
Copy modified lines R201-R202 -
Copy modified lines R207-R219 -
Copy modified line R223 -
Copy modified line R225 -
Copy modified line R231 -
Copy modified line R233 -
Copy modified line R237 -
Copy modified line R239 -
Copy modified line R245
@@ -198,37 +198,51 @@ | ||
|
||
// Clean the path to prevent path traversal | ||
cleanPath := filepath.Clean(header.Name) | ||
if strings.Contains(cleanPath, "..") { | ||
// Skip absolute paths and any path containing ".." | ||
if strings.HasPrefix(cleanPath, "/") || strings.Contains(cleanPath, "..") { | ||
continue | ||
} | ||
|
||
targetFilePath := filepath.Join(targetPath, cleanPath) | ||
absTargetPath, err := filepath.Abs(targetPath) | ||
if err != nil { | ||
return fmt.Errorf("failed to resolve target directory: %w", err) | ||
} | ||
absFilePath, err := filepath.Abs(targetFilePath) | ||
if err != nil { | ||
return fmt.Errorf("failed to resolve file path: %w", err) | ||
} | ||
// Ensure the resolved path is actually within the target directory | ||
if !strings.HasPrefix(absFilePath, absTargetPath+string(os.PathSeparator)) && absFilePath != absTargetPath { | ||
// path traversal detected; skip this file | ||
continue | ||
} | ||
|
||
// Create directory if it's a directory entry | ||
if header.Typeflag == tar.TypeDir { | ||
err = os.MkdirAll(targetFilePath, os.FileMode(header.Mode)) | ||
err = os.MkdirAll(absFilePath, os.FileMode(header.Mode)) | ||
if err != nil { | ||
return fmt.Errorf("failed to create directory %s: %w", targetFilePath, err) | ||
return fmt.Errorf("failed to create directory %s: %w", absFilePath, err) | ||
} | ||
continue | ||
} | ||
|
||
// Create parent directories if they don't exist | ||
err = os.MkdirAll(filepath.Dir(targetFilePath), 0755) | ||
err = os.MkdirAll(filepath.Dir(absFilePath), 0755) | ||
if err != nil { | ||
return fmt.Errorf("failed to create parent directory for %s: %w", targetFilePath, err) | ||
return fmt.Errorf("failed to create parent directory for %s: %w", absFilePath, err) | ||
} | ||
|
||
// Create and write file | ||
file, err := os.OpenFile(targetFilePath, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, os.FileMode(header.Mode)) | ||
file, err := os.OpenFile(absFilePath, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, os.FileMode(header.Mode)) | ||
if err != nil { | ||
return fmt.Errorf("failed to create file %s: %w", targetFilePath, err) | ||
return fmt.Errorf("failed to create file %s: %w", absFilePath, err) | ||
} | ||
|
||
_, err = io.Copy(file, tarReader) | ||
file.Close() | ||
if err != nil { | ||
return fmt.Errorf("failed to write file %s: %w", targetFilePath, err) | ||
return fmt.Errorf("failed to write file %s: %w", absFilePath, err) | ||
} | ||
} | ||
|
} | ||
|
||
// Create and write file | ||
file, err := os.OpenFile(targetFilePath, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, os.FileMode(header.Mode)) |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
To fix this vulnerability, we must ensure that files extracted from the archive are only written within the intended extraction directory, targetPath
. The robust and common approach for this is as follows:
- After joining
targetPath
andcleanPath
to gettargetFilePath
, compute the absolute path for bothtargetPath
andtargetFilePath
. - Then check (e.g., with
strings.HasPrefix
orfilepath.Rel
) that the absolutetargetFilePath
is within the absolute, canonicalizedtargetPath
. - Only allow file operations to proceed if this condition is satisfied; otherwise, skip the file, and ideally, log a warning.
- Additionally, reject files whose archive path (
header.Name
) is absolute (i.e., starts with/
or drive letter). - These checks are best performed in the file:
db_lib/ProxyGitClient.go
, functionextractArchive
(lines 173–236), in the extraction loop.
Only code within the scope of this function will be changed, following the shown context.
Required changes:
- Compute absolute path for
targetPath
at the start ofextractArchive()
. - In the extraction loop, after calculating
targetFilePath
, compute its absolute path. - Ensure
targetFilePathAbs
is withintargetPathAbs
. If not, skip extraction for this file, and possibly log the event. - Additionally, skip extraction for archive entries whose names are absolute paths.
- The required imports are already present in the file. No new dependencies are needed.
-
Copy modified lines R174-R179 -
Copy modified lines R207-R211 -
Copy modified lines R217-R225 -
Copy modified line R229 -
Copy modified line R231 -
Copy modified line R237 -
Copy modified line R239 -
Copy modified line R243 -
Copy modified line R245 -
Copy modified line R251
@@ -171,6 +171,12 @@ | ||
} | ||
|
||
func (c ProxyGitClient) extractArchive(archiveData []byte, targetPath string) error { | ||
// Compute the absolute path of the extraction root | ||
targetPathAbs, err := filepath.Abs(targetPath) | ||
if err != nil { | ||
return fmt.Errorf("failed to resolve absolute target path: %w", err) | ||
} | ||
|
||
// Create gzip reader | ||
gzReader, err := gzip.NewReader(bytes.NewReader(archiveData)) | ||
if err != nil { | ||
@@ -198,37 +204,51 @@ | ||
|
||
// Clean the path to prevent path traversal | ||
cleanPath := filepath.Clean(header.Name) | ||
// Do not allow absolute paths | ||
if filepath.IsAbs(header.Name) || filepath.IsAbs(cleanPath) { | ||
continue | ||
} | ||
// Ensure still no ".." in any segment | ||
if strings.Contains(cleanPath, "..") { | ||
continue | ||
} | ||
|
||
targetFilePath := filepath.Join(targetPath, cleanPath) | ||
targetFilePathAbs, err := filepath.Abs(targetFilePath) | ||
if err != nil { | ||
return fmt.Errorf("failed to resolve absolute file path: %w", err) | ||
} | ||
// Ensure the target file path is within the extraction root | ||
if !strings.HasPrefix(targetFilePathAbs+string(os.PathSeparator), targetPathAbs+string(os.PathSeparator)) { | ||
// Optionally log an event here | ||
continue | ||
} | ||
|
||
// Create directory if it's a directory entry | ||
if header.Typeflag == tar.TypeDir { | ||
err = os.MkdirAll(targetFilePath, os.FileMode(header.Mode)) | ||
err = os.MkdirAll(targetFilePathAbs, os.FileMode(header.Mode)) | ||
if err != nil { | ||
return fmt.Errorf("failed to create directory %s: %w", targetFilePath, err) | ||
return fmt.Errorf("failed to create directory %s: %w", targetFilePathAbs, err) | ||
} | ||
continue | ||
} | ||
|
||
// Create parent directories if they don't exist | ||
err = os.MkdirAll(filepath.Dir(targetFilePath), 0755) | ||
err = os.MkdirAll(filepath.Dir(targetFilePathAbs), 0755) | ||
if err != nil { | ||
return fmt.Errorf("failed to create parent directory for %s: %w", targetFilePath, err) | ||
return fmt.Errorf("failed to create parent directory for %s: %w", targetFilePathAbs, err) | ||
} | ||
|
||
// Create and write file | ||
file, err := os.OpenFile(targetFilePath, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, os.FileMode(header.Mode)) | ||
file, err := os.OpenFile(targetFilePathAbs, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, os.FileMode(header.Mode)) | ||
if err != nil { | ||
return fmt.Errorf("failed to create file %s: %w", targetFilePath, err) | ||
return fmt.Errorf("failed to create file %s: %w", targetFilePathAbs, err) | ||
} | ||
|
||
_, err = io.Copy(file, tarReader) | ||
file.Close() | ||
if err != nil { | ||
return fmt.Errorf("failed to write file %s: %w", targetFilePath, err) | ||
return fmt.Errorf("failed to write file %s: %w", targetFilePathAbs, err) | ||
} | ||
} | ||
|
Co-authored-by: fiftin <[email protected]>
This PR implements a new "proxy git client" feature that allows Semaphore runners to clone repositories via the Semaphore server instead of directly from git servers. This addresses scenarios where runners cannot access git servers directly due to network restrictions, firewalls, or security policies.
How it works
git_client: "proxy_git"
is configured, the Semaphore server clones repositories using standard git clients (cmd_git
orgo_git
)/api/internal/repositories/archive
)Configuration
Enable proxy mode by setting the git client type in your Semaphore configuration:
Or via environment variable:
No runner configuration changes are required - they automatically use proxy mode when the server is configured for it.
Key features
cmd_git
andgo_git
configurations remain unchangedUse cases
Implementation details
ProxyGitClientId
constant and validation rulesProxyGitClient
with fullGitClient
interface compatibility/api/internal/repositories/archive
endpoint with runner authenticationGitClientFactory
to support the new proxy modeThe implementation maintains full backward compatibility while providing a robust solution for restricted network environments.
Fixes #3259.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.