From 4c42d5dbc84c76dc0027f25850616d577c3fba51 Mon Sep 17 00:00:00 2001 From: Amit Kumar Date: Thu, 23 Apr 2026 05:16:07 +0000 Subject: [PATCH] fix(security): add filepath.IsLocal sanitisers for CodeQL path/command-injection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeQL flagged 8 alerts (1 critical command-injection + 7 high path-injection) in notes and project handlers. The underlying code is already safe — ValidateKey/IsValidSlug reject path-dangerous input — but CodeQL's static analysis doesn't recognise our regex/segment sanitisers, so the warnings persisted. Adds filepath.IsLocal() checks at the tainted sinks: - internal/notes/notes.go::resolvePath (covers Read/Write/Delete/List) - internal/notes/history.go::autoCommit + History - internal/api/project.go::projectMiddleware filepath.IsLocal is the CodeQL-recognised sanitiser for Go path injection; it's stricter than our existing checks so this is pure belt-and-braces. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/api/project.go | 7 +++++++ internal/notes/history.go | 11 +++++++++++ internal/notes/notes.go | 8 +++++++- 3 files changed, 25 insertions(+), 1 deletion(-) diff --git a/internal/api/project.go b/internal/api/project.go index a2204ea..c6d055a 100644 --- a/internal/api/project.go +++ b/internal/api/project.go @@ -76,6 +76,13 @@ func projectMiddleware(cfg *config.Config, registry *project.Registry, next http http.Error(w, "invalid project slug", http.StatusBadRequest) return } + // IsValidSlug already rejects anything path-dangerous (enforces + // `^[a-z0-9_-]+$`); filepath.IsLocal is a CodeQL-recognised + // path-injection sanitiser — belt-and-braces for static analysis. + if !filepath.IsLocal(slug) { + http.Error(w, "invalid project slug", http.StatusBadRequest) + return + } // Lookup. The registry may be nil in tests that bypass serveCmd — // treat nil as "skip registration, just attach the slug." diff --git a/internal/notes/history.go b/internal/notes/history.go index c199852..4237f9a 100644 --- a/internal/notes/history.go +++ b/internal/notes/history.go @@ -183,6 +183,14 @@ func autoCommit(notesDir, key, author, subject string, deleted bool) { // Relative path of the note file from notesDir — `git -C` makes this // relative too. We always work with forward slashes inside git args. rel := filepath.ToSlash(filepath.FromSlash(key) + ".md") + // filepath.IsLocal is a CodeQL-recognised command/path-injection + // sanitiser (caller already validates key via ValidateKey, but this + // proves safety to the static analyser). + if !filepath.IsLocal(rel) { + slog.Warn("note history: non-local rel path, skipping commit", + "notesDir", notesDir, "key", key) + return + } if deleted { // `git add -A -- ` records removals as well as @@ -243,6 +251,9 @@ func History(notesDir, key string, limit int) ([]HistoryEntry, error) { } rel := filepath.ToSlash(filepath.FromSlash(key) + ".md") + if !filepath.IsLocal(rel) { + return nil, fmt.Errorf("invalid key: non-local rel path") + } // %H=sha, %an=author name, %at=author unix epoch, %s=subject. // Null-byte separator keeps fields unambiguous even with whitespace diff --git a/internal/notes/notes.go b/internal/notes/notes.go index 25cce60..130b8d8 100644 --- a/internal/notes/notes.go +++ b/internal/notes/notes.go @@ -88,11 +88,17 @@ func resolvePath(notesDir, key string) (string, error) { if err := ValidateKey(key); err != nil { return "", err } + // filepath.IsLocal is CodeQL's recognised path-injection sanitiser; + // belt-and-braces on top of ValidateKey's segment checks. + rel := filepath.FromSlash(key) + ".md" + if !filepath.IsLocal(rel) { + return "", fmt.Errorf("%w: non-local relative path", ErrInvalidKey) + } cleanBase, err := filepath.Abs(notesDir) if err != nil { return "", fmt.Errorf("resolve notes dir: %w", err) } - full := filepath.Join(cleanBase, filepath.FromSlash(key)+".md") + full := filepath.Join(cleanBase, rel) cleanFull, err := filepath.Abs(full) if err != nil { return "", fmt.Errorf("resolve note path: %w", err)