diff --git a/pkg/service/datastore/datastore.go b/pkg/service/datastore/datastore.go index 500e41b..f709fa4 100644 --- a/pkg/service/datastore/datastore.go +++ b/pkg/service/datastore/datastore.go @@ -113,10 +113,31 @@ func NewDataStore(dataDir string) *DataStore { } // safeJoin joins the given path elements to the datastore baseDir and ensures -// that the resulting absolute path stays within baseDir. If the check fails, -// baseDir is returned to prevent directory traversal. +// that the resulting absolute path stays within baseDir. If any element would +// escape baseDir (absolute path, "..", or — on Windows — a drive/colon), the +// function falls back to baseDir to prevent directory traversal. +// +// The validation up-front uses filepath.IsLocal, which CodeQL recognises as a +// path-traversal sanitiser, so taint analysis at call sites that subsequently +// hand the result to os.ReadFile / os.Open / os.Remove etc. propagates safely. +// The post-join prefix check below stays as belt-and-suspenders for any +// unusual platform behaviour IsLocal does not cover. func (ds *DataStore) safeJoin(elem ...string) string { - // Join the base directory with the provided elements. + for _, e := range elem { + if e == "" { + // filepath.Join silently skips empty elements, but IsLocal + // returns false for "" — treat empties as a no-op. + continue + } + + if !filepath.IsLocal(e) { + // Element is absolute, contains ".." or a reserved Windows + // component. Refuse to join. + return ds.baseDir + } + } + + // Join the base directory with the (now sanitised) elements. path := filepath.Join(append([]string{ds.baseDir}, elem...)...) absPath, err := filepath.Abs(path) @@ -131,7 +152,8 @@ func (ds *DataStore) safeJoin(elem ...string) string { return absPath } - // Ensure the resolved path is within the base directory. + // Belt-and-suspenders: ensure the resolved path is within the base + // directory even if filepath.IsLocal somehow misjudged a component. baseWithSep := base if !strings.HasSuffix(baseWithSep, string(os.PathSeparator)) { baseWithSep += string(os.PathSeparator) diff --git a/pkg/service/handlers/handlers_docs.go b/pkg/service/handlers/handlers_docs.go index 2de237a..145d95c 100644 --- a/pkg/service/handlers/handlers_docs.go +++ b/pkg/service/handlers/handlers_docs.go @@ -19,13 +19,17 @@ func (s *Server) HandleDocs(w http.ResponseWriter, r *http.Request) { path = "guides/SURVIVAL-GUIDE.md" } - // Ensure we only serve files from the docs directory - filePath := filepath.Join("docs", path) - if !strings.HasPrefix(filepath.Clean(filePath), "docs") { + // Ensure we only serve files from the docs directory. filepath.IsLocal + // rejects absolute paths and ".." segments up-front and is recognised + // by CodeQL as a path-traversal sanitiser, so the os.ReadFile below + // no longer trips go/path-injection. + if !filepath.IsLocal(path) { http.Error(w, "Forbidden", http.StatusForbidden) return } + filePath := filepath.Join("docs", path) + content, err := os.ReadFile(filePath) if err != nil { http.Error(w, "File not found", http.StatusNotFound) diff --git a/pkg/service/handlers/mirror_middleware.go b/pkg/service/handlers/mirror_middleware.go index f0f93b0..0ee3ada 100644 --- a/pkg/service/handlers/mirror_middleware.go +++ b/pkg/service/handlers/mirror_middleware.go @@ -452,7 +452,18 @@ func (s *Server) saveParityMismatch(req *http.Request, local, upstream *mirrorRe dir := filepath.Join(s.ds.DataDir, "parity_mismatches") _ = os.MkdirAll(dir, 0755) - filename := fmt.Sprintf("%d_%s.json", time.Now().Unix(), strings.ReplaceAll(req.URL.Path, "/", "_")) + // Build a single filename component from req.URL.Path. After replacing + // the obvious separators, gate on filepath.IsLocal so a malicious path + // containing ".." or platform-specific separators we missed cannot + // escape `dir`. CodeQL recognises IsLocal as a path-traversal sanitiser. + pathSegment := strings.ReplaceAll(req.URL.Path, "/", "_") + pathSegment = strings.ReplaceAll(pathSegment, "\\", "_") + + if !filepath.IsLocal(pathSegment) { + pathSegment = "invalid" + } + + filename := fmt.Sprintf("%d_%s.json", time.Now().Unix(), pathSegment) _ = os.WriteFile(filepath.Join(dir, filename), data, 0644) } diff --git a/pkg/service/proxy/recorder.go b/pkg/service/proxy/recorder.go index de75434..08decc3 100644 --- a/pkg/service/proxy/recorder.go +++ b/pkg/service/proxy/recorder.go @@ -99,7 +99,11 @@ func (r *Recorder) Record(category string, req *http.Request, res *http.Response } sanitizedSegments, replacements := r.getSanitizedSegments(req.URL.Path) - dir := r.getRecordingDir(category, sanitizedSegments) + + dir, err := r.getRecordingDir(category, sanitizedSegments) + if err != nil { + return err + } if err := os.MkdirAll(dir, 0755); err != nil { return fmt.Errorf("failed to create directory %s: %w", dir, err) @@ -237,13 +241,40 @@ func (r *Recorder) getSanitizedSegments(path string) ([]string, map[string]strin return sanitizedSegments, replacements } -func (r *Recorder) getRecordingDir(category string, sanitizedSegments []string) string { +// safeJoin joins r.BaseDir with elem and refuses to construct paths that +// would escape BaseDir. Each element must satisfy filepath.IsLocal — i.e. +// it must not be absolute, must not contain ".." segments, and (on Windows) +// must not name a reserved device. CodeQL recognises filepath.IsLocal as +// a path-traversal sanitiser, so taint analysis at call sites that hand the +// result to os.* terminates here. +func (r *Recorder) safeJoin(elem ...string) (string, error) { + if r.BaseDir == "" { + return "", fmt.Errorf("recorder: BaseDir not configured") + } + + for _, e := range elem { + if e == "" { + // filepath.Join silently skips empty components, but + // filepath.IsLocal returns false for "" — treat empties as + // no-ops to preserve the existing call shapes. + continue + } + + if !filepath.IsLocal(e) { + return "", fmt.Errorf("recorder: path component %q escapes BaseDir", e) + } + } + + return filepath.Join(append([]string{r.BaseDir}, elem...)...), nil +} + +func (r *Recorder) getRecordingDir(category string, sanitizedSegments []string) (string, error) { subDir := "root" if len(sanitizedSegments) > 0 { subDir = filepath.Join(sanitizedSegments...) } - return filepath.Join(r.BaseDir, "interactions", r.SessionID, category, subDir) + return r.safeJoin("interactions", r.SessionID, category, subDir) } func (r *Recorder) getRecordingPath(dir, method string) string { @@ -733,7 +764,10 @@ func (r *Recorder) DeleteSession(sessionID string) error { return fmt.Errorf("session ID is required") } - sessionDir := filepath.Join(r.BaseDir, "interactions", sessionID) + sessionDir, err := r.safeJoin("interactions", sessionID) + if err != nil { + return err + } return os.RemoveAll(sessionDir) } @@ -781,13 +815,20 @@ func (r *Recorder) CleanupSessions(keepCount int) error { // GetInteractionContent returns the raw content of a recorded interaction. func (r *Recorder) GetInteractionContent(relPath string) ([]byte, error) { - fullPath := filepath.Join(r.BaseDir, "interactions", relPath) + fullPath, err := r.safeJoin("interactions", relPath) + if err != nil { + return nil, err + } + return os.ReadFile(fullPath) } // ArchiveSession creates a .tar.gz archive of the specified session and writes it to w. func (r *Recorder) ArchiveSession(sessionID string, w io.Writer) (err error) { - sessionDir := filepath.Join(r.BaseDir, "interactions", sessionID) + sessionDir, err := r.safeJoin("interactions", sessionID) + if err != nil { + return err + } info, statErr := os.Stat(sessionDir) if statErr != nil {