Skip to content

Commit 41f94ea

Browse files
committed
fix(logs): contain server-name-derived log path within log dir
CodeQL go/path-injection flagged the daemon log reader (internal/server/server.go) where a user-controlled server name flows into the log file path. logs.ServerLogFilename already sanitizes the name to a single path element, so the flow is safe, but CodeQL does not recognize the strings.Map char-whitelist as a barrier. Add an explicit filepath.Clean + prefix containment guard at both log-read sites (daemon reader and the no-daemon CLI reader, which feeds tail) so the resolved path can never escape the log directory. Defense-in-depth + clears the alert without weakening any check. Valid namespaced names (io.github.owner/repo) are unaffected — their sanitized filename stays a single element under logDir. Related #604
1 parent bd9a8d5 commit 41f94ea

2 files changed

Lines changed: 15 additions & 0 deletions

File tree

cmd/mcpproxy/upstream_cmd.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -774,6 +774,14 @@ func runUpstreamLogsFromFile(globalConfig *config.Config, serverName string) err
774774

775775
logFile := filepath.Join(logDir, logs.ServerLogFilename(serverName))
776776

777+
// Defense-in-depth: logs.ServerLogFilename already sanitizes the (user-controlled)
778+
// server name to a single path element, but verify the resolved path stays inside
779+
// logDir before it reaches os.Stat/tail so a crafted name can never escape the log
780+
// directory (path-injection barrier).
781+
if !strings.HasPrefix(filepath.Clean(logFile), filepath.Clean(logDir)+string(os.PathSeparator)) {
782+
return fmt.Errorf("invalid server name: %s", serverName)
783+
}
784+
777785
// Check if file exists
778786
if _, err := os.Stat(logFile); os.IsNotExist(err) {
779787
return fmt.Errorf("log file not found: %s (daemon may not have run yet)", logFile)

internal/server/server.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2284,6 +2284,13 @@ func (s *Server) GetServerLogs(serverName string, tail int) ([]contracts.LogEntr
22842284

22852285
logFile := filepath.Join(logDir, logs.ServerLogFilename(serverName))
22862286

2287+
// Defense-in-depth: logs.ServerLogFilename already sanitizes the (user-controlled)
2288+
// server name to a single path element, but verify the resolved path stays inside
2289+
// logDir so a crafted name can never escape the log directory (path-injection barrier).
2290+
if !strings.HasPrefix(filepath.Clean(logFile), filepath.Clean(logDir)+string(os.PathSeparator)) {
2291+
return nil, fmt.Errorf("invalid server name: %s", serverName)
2292+
}
2293+
22872294
// Check if file exists
22882295
if _, err := os.Stat(logFile); os.IsNotExist(err) {
22892296
return nil, fmt.Errorf("log file not found: %s (server may not have run yet)", logFile)

0 commit comments

Comments
 (0)