diff --git a/cmd/server/main.go b/cmd/server/main.go
index b7d5dbde..f6d3ba2b 100644
--- a/cmd/server/main.go
+++ b/cmd/server/main.go
@@ -538,6 +538,11 @@ func (app *serverApp) initStores(logger *slog.Logger) error {
// Create V1 API handler
v1Handler := module.NewV1APIHandler(store, secret)
+ // Constrain server-local path reads (loadWorkflowFromPath) to the configured
+ // data directory. Without this, the path-injection guard in the handler is
+ // inactive and an authenticated admin could read arbitrary files off the host
+ // via the /workflows/load-from-path endpoint.
+ v1Handler.SetDataDir(*dataDir)
v1Handler.SetReloadFunc(func(configYAML string) error {
cfg, parseErr := config.LoadFromString(configYAML)
if parseErr != nil {
diff --git a/module/api_v1_handler.go b/module/api_v1_handler.go
index 9308cf8b..6aa5597b 100644
--- a/module/api_v1_handler.go
+++ b/module/api_v1_handler.go
@@ -50,9 +50,22 @@ func (h *V1APIHandler) SetRuntimeManager(rm *RuntimeManager) {
h.runtimeManager = rm
}
-// SetDataDir sets the base data directory used for workspace extraction during import.
+// SetDataDir sets the base data directory used for workspace extraction during
+// import and for containing server-local path reads. The directory is normalised
+// to an absolute, cleaned path so that the containment check in
+// loadWorkflowFromPath compares like with like (absolute vs absolute).
func (h *V1APIHandler) SetDataDir(dir string) {
- h.dataDir = dir
+ if dir == "" {
+ h.dataDir = ""
+ return
+ }
+ if abs, err := filepath.Abs(dir); err == nil {
+ h.dataDir = filepath.Clean(abs)
+ return
+ }
+ // Fall back to the cleaned (possibly relative) path if Abs fails; the
+ // containment check itself also calls filepath.Abs on both operands.
+ h.dataDir = filepath.Clean(dir)
}
// ServeHTTP implements http.Handler for config-driven delegate dispatch.
@@ -960,11 +973,43 @@ func (h *V1APIHandler) loadWorkflowFromPath(w http.ResponseWriter, r *http.Reque
req.ProjectID = "00000000-0000-0000-0000-000000000002"
}
- // Resolve the config file path
- configPath := filepath.Clean(req.Path)
+ // Resolve the config file path and enforce path containment.
+ //
+ // The resolved path MUST stay inside an allowed base directory so that an
+ // authenticated request cannot read arbitrary files from the host filesystem
+ // (e.g. /etc/passwd or ../../secrets). Containment is active BY DEFAULT:
+ // - In production, cmd/server wires SetDataDir(--data-dir), so the base is
+ // the configured data directory.
+ // - If no data directory was configured, we fall back to the process
+ // working directory (filepath.Abs(".")) rather than leaving the read
+ // unguarded. This keeps the endpoint safe by default.
+ base := h.dataDir
+ if base == "" {
+ // Default-deny posture: contain to the current working directory.
+ if wd, wdErr := filepath.Abs("."); wdErr == nil {
+ base = wd
+ }
+ }
+
+ absConfig, absErr := filepath.Abs(filepath.Clean(req.Path))
+ if absErr != nil {
+ writeJSON(w, http.StatusBadRequest, map[string]string{"error": "invalid path"})
+ return
+ }
+ configPath := absConfig
+
+ if base != "" {
+ baseClean := filepath.Clean(base) + string(os.PathSeparator)
+ if !strings.HasPrefix(absConfig+string(os.PathSeparator), baseClean) {
+ writeJSON(w, http.StatusForbidden, map[string]string{"error": "path is outside the allowed data directory"})
+ return
+ }
+ }
+
info, err := os.Stat(configPath)
if err != nil {
- writeJSON(w, http.StatusBadRequest, map[string]string{"error": fmt.Sprintf("path not found: %s", configPath)})
+ // Do not echo the resolved absolute path back to the client.
+ writeJSON(w, http.StatusBadRequest, map[string]string{"error": "path not found"})
return
}
@@ -972,14 +1017,15 @@ func (h *V1APIHandler) loadWorkflowFromPath(w http.ResponseWriter, r *http.Reque
if info.IsDir() {
yamlPath := filepath.Join(configPath, "workflow.yaml")
if _, err := os.Stat(yamlPath); err != nil {
- writeJSON(w, http.StatusBadRequest, map[string]string{"error": fmt.Sprintf("no workflow.yaml in %s", configPath)})
+ writeJSON(w, http.StatusBadRequest, map[string]string{"error": "no workflow.yaml in the specified directory"})
return
}
configPath = yamlPath
}
- // Read the config file
- data, err := os.ReadFile(configPath)
+ // Read the config file. configPath has been validated above: it is either
+ // contained within h.dataDir (when set) or passed through filepath.Clean.
+ data, err := os.ReadFile(configPath) //nolint:gosec // G304: path validated against h.dataDir base directory above
if err != nil {
writeJSON(w, http.StatusInternalServerError, map[string]string{"error": fmt.Sprintf("failed to read file: %v", err)})
return
diff --git a/module/api_v1_test.go b/module/api_v1_test.go
index 514f318c..1eb71257 100644
--- a/module/api_v1_test.go
+++ b/module/api_v1_test.go
@@ -1100,3 +1100,91 @@ func TestV1Store_ExecutionFailure(t *testing.T) {
t.Errorf("got failed count %d, want 1", counts["failed"])
}
}
+
+// TestLoadWorkflowFromPath_PathTraversal verifies that the loadWorkflowFromPath
+// endpoint rejects paths that escape h.dataDir when that field is configured.
+// This covers the go/path-injection CodeQL alerts #56, #57, #58 in
+// module/api_v1_handler.go (os.Stat / filepath.Join / os.ReadFile sinks).
+func TestLoadWorkflowFromPath_PathTraversal(t *testing.T) {
+ handler, _, secret := setupTestHandler(t)
+
+ // Create a temporary directory to act as the allowed data directory.
+ allowedDir := t.TempDir()
+ handler.SetDataDir(allowedDir)
+
+ token := generateTestToken(secret, "1", "admin@test.com", "admin")
+
+ // Each of these resolves to a path OUTSIDE allowedDir. The containment guard
+ // is the only code path that produces 403 Forbidden — os.Stat on a missing
+ // file would yield 400, and a readable file would yield 201/500. Asserting
+ // EXACTLY 403 makes this test revert-sensitive: removing the guard changes
+ // the status code (to 400 for the nonexistent traversal targets), failing it.
+ maliciousPaths := []string{
+ "../../etc/passwd",
+ "/etc/passwd",
+ allowedDir + "/../../etc/passwd",
+ "../outside",
+ }
+
+ for _, p := range maliciousPaths {
+ t.Run(p, func(t *testing.T) {
+ body := fmt.Sprintf(`{"path": %q, "project_id": "00000000-0000-0000-0000-000000000002"}`, p)
+ rr := doRequest(handler, "POST", "/api/v1/workflows/load-from-path", body, token)
+
+ if rr.Code != http.StatusForbidden {
+ t.Errorf("path %q: expected 403 Forbidden from containment guard, got %d (body: %s)",
+ p, rr.Code, rr.Body.String())
+ }
+ })
+ }
+}
+
+// TestLoadWorkflowFromPath_ContainedByDefault verifies that even with NO dataDir
+// configured, the endpoint is contained to the process working directory (default-
+// deny posture) rather than reading arbitrary absolute paths. Covers CRITICAL: the
+// production handler does not always have SetDataDir called.
+func TestLoadWorkflowFromPath_ContainedByDefault(t *testing.T) {
+ handler, _, secret := setupTestHandler(t)
+ // Intentionally do NOT call SetDataDir — exercise the default-deny fallback.
+
+ token := generateTestToken(secret, "1", "admin@test.com", "admin")
+
+ // /etc/passwd is outside the process working directory, so the default
+ // containment base (filepath.Abs(".")) must reject it with 403.
+ body := `{"path": "/etc/passwd", "project_id": "00000000-0000-0000-0000-000000000002"}`
+ rr := doRequest(handler, "POST", "/api/v1/workflows/load-from-path", body, token)
+
+ if rr.Code != http.StatusForbidden {
+ t.Errorf("expected 403 (default containment to working dir), got %d (body: %s)",
+ rr.Code, rr.Body.String())
+ }
+}
+
+// TestLoadWorkflowFromPath_AllowedPath verifies that a path inside dataDir is
+// still accepted when loadWorkflowFromPath has a dataDir configured.
+func TestLoadWorkflowFromPath_AllowedPath(t *testing.T) {
+ handler, _, secret := setupTestHandler(t)
+
+ // Set up a data dir with a workflow config inside it.
+ dataDir := t.TempDir()
+ handler.SetDataDir(dataDir)
+
+ // Create a valid workflow.yaml inside the allowed dir.
+ wfDir := filepath.Join(dataDir, "my-workflow")
+ if err := os.MkdirAll(wfDir, 0o750); err != nil {
+ t.Fatalf("MkdirAll: %v", err)
+ }
+ wfFile := filepath.Join(wfDir, "workflow.yaml")
+ if err := os.WriteFile(wfFile, []byte("name: test\npipelines: []\n"), 0o640); err != nil {
+ t.Fatalf("WriteFile: %v", err)
+ }
+
+ token := generateTestToken(secret, "1", "admin@test.com", "admin")
+
+ body := fmt.Sprintf(`{"path": %q, "project_id": "00000000-0000-0000-0000-000000000002"}`, wfDir)
+ rr := doRequest(handler, "POST", "/api/v1/workflows/load-from-path", body, token)
+
+ if rr.Code != http.StatusCreated {
+ t.Errorf("expected 201, got %d (body: %s)", rr.Code, rr.Body.String())
+ }
+}
diff --git a/plugin/storebrowser/handler.go b/plugin/storebrowser/handler.go
index e7b498b6..5e15934a 100644
--- a/plugin/storebrowser/handler.go
+++ b/plugin/storebrowser/handler.go
@@ -172,7 +172,16 @@ func (h *handler) tableRows(w http.ResponseWriter, r *http.Request) {
orderClause := ""
if sortCol != "" {
- // Validate the column name exists in this table.
+ // Reject sort column names that contain non-identifier characters before
+ // even querying the database. This gives static analysis tools (e.g. CodeQL)
+ // a clear, local sanitizer to track, and prevents any injection attempt
+ // from reaching the DB-schema lookup.
+ if !isValidTableName(sortCol) {
+ writeError(w, http.StatusBadRequest, fmt.Sprintf("invalid sort column: %s", sortCol))
+ return
+ }
+ // Cross-check the column name against the actual schema of this table so
+ // that we never reference a column that does not exist.
validCols, err := getTableColumns(h.db, tableName)
if err != nil {
writeError(w, http.StatusInternalServerError, fmt.Sprintf("get columns: %v", err))
@@ -182,14 +191,18 @@ func (h *handler) tableRows(w http.ResponseWriter, r *http.Request) {
writeError(w, http.StatusBadRequest, fmt.Sprintf("invalid sort column: %s", sortCol))
return
}
- orderClause = fmt.Sprintf(" ORDER BY %s %s", sortCol, order)
- }
-
- // tableName is validated against the allowlist returned by getValidTables() above,
- // and orderClause uses a column validated against getTableColumns(). Both are safe
- // from injection. Parameters are bound via ? placeholders.
- query := fmt.Sprintf("SELECT * FROM %s%s LIMIT ? OFFSET ?", tableName, orderClause) //nolint:gosec // tableName and orderClause are validated against DB schema above
- rows, err := h.db.QueryContext(r.Context(), query, limit, offset) //nolint:gosec // G701: query built from validated table name and column
+ // Both sortCol (identifier-validated + schema-checked) and order (normalised
+ // to the literal strings "ASC"/"DESC" above) are safe to interpolate.
+ orderClause = fmt.Sprintf(" ORDER BY %s %s", sortCol, order) //nolint:gosec // G201: sortCol validated by isValidTableName + schema allowlist; order is "ASC"|"DESC"
+ }
+
+ // tableName is validated against the allowlist returned by getValidTables() above.
+ // orderClause is empty or " ORDER BY ASC|DESC" where passed
+ // isValidTableName (identifier regexp) and was cross-checked against the live
+ // DB schema. LIMIT/OFFSET are bound as ? placeholders. No user input is
+ // interpolated without prior validation.
+ query := fmt.Sprintf("SELECT * FROM %s%s LIMIT ? OFFSET ?", tableName, orderClause) //nolint:gosec // G201: identifiers validated; values parameterised
+ rows, err := h.db.QueryContext(r.Context(), query, limit, offset)
if err != nil {
writeError(w, http.StatusInternalServerError, fmt.Sprintf("query rows: %v", err))
return
@@ -257,7 +270,18 @@ func (h *handler) execQuery(w http.ResponseWriter, r *http.Request) {
}
defer func() { _ = tx.Rollback() }()
- rows, err := tx.QueryContext(r.Context(), q) //nolint:gosec // G701: query validated by sanitizeReadOnlyQuery
+ // The query is user-supplied SQL executed in a read-only transaction (BeginTx
+ // with TxOptions{ReadOnly:true}). Before reaching this point the query has
+ // been validated by:
+ // 1. sanitizeReadOnlyQuery — enforces SELECT-only, no statement separators (;),
+ // no SQL comments (-- / /*).
+ // 2. dangerousKeywords regexp — blocks DROP/DELETE/INSERT/UPDATE/ALTER/CREATE
+ // and other write keywords regardless of case.
+ // Parameterisation is intentionally impossible here because the user supplies
+ // the full SQL structure (this is an admin read-only SQL REPL). The read-only
+ // transaction boundary means write operations would be rolled back even if
+ // they somehow bypassed the keyword checks.
+ rows, err := tx.QueryContext(r.Context(), q) //nolint:gosec // G201: admin-only read-only SQL REPL; query validated by sanitizeReadOnlyQuery + dangerousKeywords + ReadOnly tx
if err != nil {
writeError(w, http.StatusBadRequest, fmt.Sprintf("query error: %v", err))
return
diff --git a/plugin/storebrowser/handler_test.go b/plugin/storebrowser/handler_test.go
index 993cfed9..61e49b13 100644
--- a/plugin/storebrowser/handler_test.go
+++ b/plugin/storebrowser/handler_test.go
@@ -7,6 +7,7 @@ import (
"encoding/json"
"net/http"
"net/http/httptest"
+ "net/url"
"testing"
"github.com/GoCodeAlone/workflow/store"
@@ -614,3 +615,183 @@ func TestInvalidTableName(t *testing.T) {
t.Fatalf("expected 400 for invalid table name, got %d", w.Code)
}
}
+
+// TestIsValidTableName_RejectsMalicious is a direct unit test of the identifier
+// guard added to fix the go/sql-injection CodeQL alert (#60). It is inherently
+// revert-sensitive: if isValidTableName is removed the test fails to compile, and
+// if its regexp is loosened the malicious cases below start passing.
+func TestIsValidTableName_RejectsMalicious(t *testing.T) {
+ malicious := []string{
+ "id;DROP",
+ "id`",
+ "../../etc/passwd",
+ "id--",
+ "id()",
+ "id OR 1=1",
+ "1 UNION SELECT",
+ "name\"",
+ "col,other",
+ "",
+ }
+ for _, m := range malicious {
+ if isValidTableName(m) {
+ t.Errorf("isValidTableName(%q) = true, want false (injection-prone identifier accepted)", m)
+ }
+ }
+
+ valid := []string{"id", "name", "user_id", "_private", "Col2", "a1b2"}
+ for _, v := range valid {
+ if !isValidTableName(v) {
+ t.Errorf("isValidTableName(%q) = false, want true (legitimate identifier rejected)", v)
+ }
+ }
+}
+
+// TestTableRowsSortInjection_GuardCatchesSchemaColumn proves the NEW isValidTableName
+// guard in tableRows, not the pre-existing schema allowlist. It creates a table with
+// a column whose real name contains a space ("bad col"). That column IS present in the
+// schema (so getTableColumns/validCols would accept it), but isValidTableName rejects
+// it BEFORE the query is built. If the isValidTableName guard is removed, this column
+// passes the schema check and the request would proceed to the DB (status 200),
+// which this test asserts must NOT happen.
+func TestTableRowsSortInjection_GuardCatchesSchemaColumn(t *testing.T) {
+ db := newTestDB(t)
+ // Column name with a space is a valid SQLite identifier when quoted, so it
+ // exists in the schema, but it is not a safe bare SQL identifier.
+ _, err := db.Exec(`CREATE TABLE guard_test (id INTEGER PRIMARY KEY, "bad col" TEXT)`)
+ if err != nil {
+ t.Fatal(err)
+ }
+ db.Exec(`INSERT INTO guard_test (id, "bad col") VALUES (1, 'x')`)
+
+ // Sanity check: the malicious-looking column really is in the schema allowlist,
+ // so a passing result here would mean the schema check alone did NOT protect us.
+ cols, err := getTableColumns(db, "guard_test")
+ if err != nil {
+ t.Fatal(err)
+ }
+ if !cols["bad col"] {
+ t.Fatalf("precondition failed: %q expected in schema columns %v", "bad col", cols)
+ }
+
+ h := &handler{db: db}
+ mux := newTestMux(h)
+
+ u := "/tables/guard_test/rows?sort=" + url.QueryEscape("bad col")
+ req := httptest.NewRequest("GET", u, nil)
+ w := httptest.NewRecorder()
+ mux.ServeHTTP(w, req)
+
+ // The isValidTableName guard must reject this with 400 before it reaches the DB.
+ // Without the guard, "bad col" passes the schema allowlist and the handler would
+ // return 200 (or 500 if the unquoted identifier produced a SQL error).
+ if w.Code != http.StatusBadRequest {
+ t.Errorf("expected 400 (isValidTableName guard), got %d (body: %s)", w.Code, w.Body.String())
+ }
+}
+
+// TestTableRowsSortInjection verifies that a malicious sort column value is
+// rejected before it can reach the database. This exercises the isValidTableName
+// guard added to fix the go/sql-injection CodeQL alert on handler.go (alert #60).
+func TestTableRowsSortInjection(t *testing.T) {
+ db := newTestDB(t)
+ _, err := db.Exec("CREATE TABLE inj_test (id INTEGER PRIMARY KEY, name TEXT)")
+ if err != nil {
+ t.Fatal(err)
+ }
+ db.Exec("INSERT INTO inj_test (id, name) VALUES (1, 'a')")
+
+ h := &handler{db: db}
+ mux := newTestMux(h)
+
+ maliciousInputs := []struct {
+ name string
+ value string
+ }{
+ {"semicolon_drop", "id;DROP"}, // semicolon injection (no space to avoid URL panic)
+ {"backtick", "id`"}, // backtick injection
+ {"dot_traversal", "../../etc/passwd"}, // path-like traversal
+ {"dash_comment", "id--"}, // double-dash SQL comment
+ {"parens", "id()"}, // function-call injection
+ }
+
+ for _, tc := range maliciousInputs {
+ t.Run(tc.name, func(t *testing.T) {
+ // Build the request using url.Values so the sort param is properly
+ // encoded rather than hand-concatenated into a URL string.
+ u := "/tables/inj_test/rows?sort=" + url.QueryEscape(tc.value)
+ req := httptest.NewRequest("GET", u, nil)
+ w := httptest.NewRecorder()
+ mux.ServeHTTP(w, req)
+
+ // Must be rejected with 400; must NOT be 200 or 500 (which would
+ // indicate the input reached the database layer).
+ if w.Code != http.StatusBadRequest {
+ t.Errorf("expected 400 for sort %q (%s), got %d (body: %s)",
+ tc.value, tc.name, w.Code, w.Body.String())
+ }
+ })
+ }
+}
+
+// TestSQLQueryInjectionAttempts verifies the /query endpoint rejects common
+// SQL injection patterns. This covers the go/sql-injection CodeQL alert #61
+// (the admin read-only SQL REPL enforces SELECT-only with no semicolons).
+func TestSQLQueryInjectionAttempts(t *testing.T) {
+ db := newTestDB(t)
+ h := &handler{db: db}
+ mux := newTestMux(h)
+
+ tests := []struct {
+ name string
+ query string
+ wantStatusCode int
+ }{
+ {
+ name: "semicolon_separation",
+ query: "SELECT 1; DROP TABLE foo--",
+ // dangerousKeywords (DROP) fires before sanitizeReadOnlyQuery checks ";",
+ // so the response is 403 Forbidden, not 400 Bad Request. The key
+ // property is that neither the semicolon nor the DROP reaches the DB.
+ wantStatusCode: http.StatusForbidden,
+ },
+ {
+ name: "comment_injection",
+ query: "SELECT 1 -- injected comment",
+ wantStatusCode: http.StatusBadRequest, // sanitizeReadOnlyQuery blocks "--"
+ },
+ {
+ name: "block_comment",
+ query: "SELECT /* injected */ 1",
+ wantStatusCode: http.StatusBadRequest, // sanitizeReadOnlyQuery blocks "/*"
+ },
+ {
+ name: "uppercase_drop",
+ query: "SELECT 1 WHERE DROP TABLE foo",
+ wantStatusCode: http.StatusForbidden, // dangerousKeywords blocks DROP
+ },
+ {
+ name: "tautology_or",
+ // A valid SELECT with tautology — this should execute as a read-only
+ // query (the DB is empty so it returns 0 rows); the key property is
+ // that it does NOT return 500 or inject anything.
+ query: "SELECT 1 WHERE 1=1 OR 1=1",
+ wantStatusCode: http.StatusOK,
+ },
+ }
+
+ for _, tc := range tests {
+ t.Run(tc.name, func(t *testing.T) {
+ body, _ := json.Marshal(map[string]string{"query": tc.query})
+ req := httptest.NewRequest("POST", "/query", bytes.NewReader(body))
+ req.Header.Set("Content-Type", "application/json")
+ w := httptest.NewRecorder()
+ mux.ServeHTTP(w, req)
+
+ if w.Code != tc.wantStatusCode {
+ t.Errorf("query %q: expected status %d, got %d (body: %s)",
+ tc.query, tc.wantStatusCode, w.Code, w.Body.String())
+ }
+ })
+ }
+}
diff --git a/store/local_storage.go b/store/local_storage.go
index a85fefb4..6963ca6e 100644
--- a/store/local_storage.go
+++ b/store/local_storage.go
@@ -35,11 +35,19 @@ func (l *LocalStorage) Root() string {
// resolve converts a relative storage path to an absolute filesystem path,
// ensuring the result stays within the root directory.
+//
+// The containment check appends a path separator to both sides so that a root
+// of "/tmp/work" does NOT accidentally allow "/tmp/workevil/..." (a naive
+// HasPrefix check without the separator would pass in that case).
func (l *LocalStorage) resolve(path string) (string, error) {
clean := filepath.Clean(path)
abs := filepath.Join(l.root, clean)
- // Prevent path traversal
- if !strings.HasPrefix(abs, l.root) {
+ // Append separator to enforce strict directory containment.
+ // l.root is already absolute (set in NewLocalStorage via filepath.Abs).
+ rootWithSep := l.root + string(filepath.Separator)
+ // The resolved path must either equal the root exactly or start with
+ // root+separator (i.e. be inside the root directory tree).
+ if abs != l.root && !strings.HasPrefix(abs, rootWithSep) {
return "", fmt.Errorf("path %q escapes storage root", path)
}
return abs, nil
diff --git a/store/workspace.go b/store/workspace.go
index d66de061..71d075d6 100644
--- a/store/workspace.go
+++ b/store/workspace.go
@@ -2,7 +2,9 @@ package store
import (
"fmt"
+ "os"
"path/filepath"
+ "strings"
)
// WorkspaceManager manages project workspace directories and their storage.
@@ -21,9 +23,41 @@ func (wm *WorkspaceManager) WorkspacePath(projectID string) string {
}
// StorageForProject returns a LocalStorage provider scoped to a project workspace.
+// The projectID is user-supplied (from a URL segment), so we verify the resolved
+// workspace path stays inside wm.dataDir/workspaces/ to prevent path traversal
+// (e.g. projectID="../../etc" must not escape the workspaces base directory).
func (wm *WorkspaceManager) StorageForProject(projectID string) (*LocalStorage, error) {
if projectID == "" {
return nil, fmt.Errorf("project ID is required")
}
- return NewLocalStorage(wm.WorkspacePath(projectID))
+
+ // Reject project IDs that are not a single, plain path segment. "." and ".."
+ // would resolve to the workspaces root or its parent (breaking isolation /
+ // escaping the base), and any path separator means the caller is trying to
+ // address a nested or sibling location rather than a single project dir.
+ if projectID == "." || projectID == ".." ||
+ strings.ContainsRune(projectID, '/') ||
+ strings.ContainsRune(projectID, os.PathSeparator) {
+ return nil, fmt.Errorf("invalid project ID %q: must be a single path segment", projectID)
+ }
+
+ // Resolve the base directory (workspaces root) to an absolute path.
+ workspacesBase, err := filepath.Abs(filepath.Join(wm.dataDir, "workspaces"))
+ if err != nil {
+ return nil, fmt.Errorf("resolve workspaces base: %w", err)
+ }
+ workspacesBase = filepath.Clean(workspacesBase) + string(os.PathSeparator)
+
+ // Resolve the candidate workspace path.
+ candidate, err := filepath.Abs(filepath.Join(wm.dataDir, "workspaces", projectID))
+ if err != nil {
+ return nil, fmt.Errorf("resolve workspace path: %w", err)
+ }
+
+ // Enforce containment: the workspace must live inside the workspaces base.
+ if !strings.HasPrefix(candidate+string(os.PathSeparator), workspacesBase) {
+ return nil, fmt.Errorf("project ID %q resolves outside the workspaces directory", projectID)
+ }
+
+ return NewLocalStorage(candidate)
}
diff --git a/store/workspace_test.go b/store/workspace_test.go
index aff7d63c..11b5918c 100644
--- a/store/workspace_test.go
+++ b/store/workspace_test.go
@@ -5,6 +5,7 @@ import (
"context"
"io"
"path/filepath"
+ "strings"
"testing"
)
@@ -46,6 +47,53 @@ func TestWorkspaceManager_StorageForProject_EmptyID(t *testing.T) {
}
}
+// TestWorkspaceManager_StorageForProject_PathTraversal verifies that a
+// malicious projectID cannot escape the workspaces base directory.
+// This covers the go/path-injection CodeQL alert #14 (source: workspace_handler.go).
+func TestWorkspaceManager_StorageForProject_PathTraversal(t *testing.T) {
+ dir := t.TempDir()
+ wm := NewWorkspaceManager(dir)
+
+ // These IDs contain raw path separators / dot-dot sequences that filepath.Join
+ // will collapse, allowing traversal outside the workspaces root.
+ shouldBeRejected := []string{
+ "../../etc/passwd",
+ "../../../tmp/evil",
+ "proj/../../../etc",
+ }
+
+ for _, id := range shouldBeRejected {
+ t.Run(id, func(t *testing.T) {
+ _, err := wm.StorageForProject(id)
+ if err == nil {
+ t.Errorf("StorageForProject(%q): expected error for path traversal, got nil", id)
+ }
+ })
+ }
+
+ // URL-encoded dot-dot sequences (%2e%2e%2f...) are treated literally by the
+ // filesystem — the path becomes /workspaces/%2e%2e%2fetc (inside root).
+ // This is the expected safe behaviour; the test documents it explicitly.
+ t.Run("url_encoded_traversal_is_safe", func(t *testing.T) {
+ id := "%2e%2e%2f%2e%2e%2fetc%2fpasswd"
+ storage, err := wm.StorageForProject(id)
+ if err != nil {
+ // Also acceptable — any error is safe.
+ return
+ }
+ // If no error, verify the root is still inside the workspaces base.
+ root := storage.Root()
+ workspacesBase := filepath.Join(dir, "workspaces")
+ if !filepath.IsAbs(root) {
+ t.Errorf("Root() returned non-absolute path %q", root)
+ }
+ // The root must start with the workspaces base.
+ if !strings.HasPrefix(root, workspacesBase) {
+ t.Errorf("Root() %q escapes workspaces base %q", root, workspacesBase)
+ }
+ })
+}
+
func TestWorkspaceManager_ProjectIsolation(t *testing.T) {
dir := t.TempDir()
wm := NewWorkspaceManager(dir)