mirror of
https://github.com/gesellix/Bose-SoundTouch.git
synced 2026-08-19 00:56:16 +00:00
fix(security): close go/path-injection alerts via filepath.IsLocal sanitiser
CodeQL flagged 34 go/path-injection alerts across datastore.go, marge.go, recorder.go, handlers_docs.go and mirror_middleware.go. The existing defences (DataStore.safeJoin's post-join prefix check, handlers_docs's HasPrefix(filepath.Clean(...))) are functionally correct but sit downstream of the join, so CodeQL's interprocedural taint tracking treats every os.* sink that consumes them as still tainted. Move the validation up-front using filepath.IsLocal, which CodeQL recognises as a path-traversal sanitiser. IsLocal rejects absolute paths, ".." segments, and (on Windows) reserved device names — the same set the existing checks intended to block, just expressed in the shape the analyser understands. Changes: * DataStore.safeJoin (datastore.go) — pre-validates each non-empty element with filepath.IsLocal before joining. Existing post-join prefix check stays as belt-and-suspenders. ~30 of the 34 alerts flow through this helper. * Recorder (recorder.go) — adds a new (*Recorder).safeJoin method with the same sanitiser. getRecordingDir, DeleteSession, GetInteractionContent and ArchiveSession route through it; their signatures already returned error so plumbing it through is local. * HandleDocs (handlers_docs.go) — replaces the post-join HasPrefix check with an up-front filepath.IsLocal gate. * Mirror parity recorder (mirror_middleware.go) — also strips backslash separators (Windows) and gates the resulting filename component on filepath.IsLocal, falling back to "invalid" rather than letting malformed paths reach os.WriteFile. No behaviour change for legitimate inputs (account IDs, device IDs, session IDs, doc paths all satisfy IsLocal). Datastore and proxy test suites pass; handler suite's pre-existing TestDocsConsistency failure is unrelated. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.7
parent
9ce42f3965
commit
648eedefde
@@ -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)
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
|
||||
|
||||
@@ -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 {
|
||||
|
||||
Reference in New Issue
Block a user