From 23d78a304525672ba949a177e133d655b9ef434d Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Fri, 29 May 2026 07:48:26 -0400 Subject: [PATCH 1/2] security: fix SQL-injection and path-injection (CodeQL alerts #56-61, #14) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SQL-injection fixes (alerts #60, #61 — plugin/storebrowser/handler.go): - tableRows sort-column path: add isValidTableName identifier-regex guard on the `sort` query param before the DB column-schema lookup so static analysis has a local sanitizer it can track; update comments and nolint annotations. - execQuery (admin read-only SQL REPL): document the mitigation layers (sanitizeReadOnlyQuery + dangerousKeywords regex + ReadOnly transaction) explicitly in code so the intentional trade-off is clear to reviewers. Path-injection fixes (alerts #56, #57, #58 — module/api_v1_handler.go): - loadWorkflowFromPath: when h.dataDir is configured, verify the resolved absolute path stays inside h.dataDir using filepath.Abs + HasPrefix(path+sep) before calling os.Stat / filepath.Join / os.ReadFile. Paths that escape the data directory receive 403 Forbidden. Path-injection fix (alert #14 — store/workspace.go + store/local_storage.go): - WorkspaceManager.StorageForProject: validate that the projectID (URL segment, user-controlled) does not produce a workspace path outside the workspaces base directory before calling NewLocalStorage (which calls os.MkdirAll). - LocalStorage.resolve: tighten the HasPrefix containment check by appending a path separator so that a root of "/tmp/work" cannot accidentally allow "/tmp/workevil/..." (off-by-one in bare HasPrefix without separator). Tests added: - TestTableRowsSortInjection: feeds semicolons, backticks, dot-traversal, dash- comments, parens as sort column values; asserts 400 rejection. - TestSQLQueryInjectionAttempts: feeds semicolon+DROP, SQL comments, block comments, tautology (SELECT-only passes); asserts correct status codes. - TestWorkspaceManager_StorageForProject_PathTraversal: feeds ../../etc/passwd and similar traversal IDs; asserts error. - TestLoadWorkflowFromPath_PathTraversal: feeds traversal paths to the endpoint with dataDir set; asserts non-201 rejection. - TestLoadWorkflowFromPath_AllowedPath: confirms a path inside dataDir succeeds. False-positive notes (not dismissed — left for the lead): - database.go alerts #8, #9: user request data flows into `resolvedParams` (bound as ? placeholders to parameterized queries), NOT into the SQL string itself. Only allowDynamicSQL paths interpolate resolved values, and those are validated by validateSQLIdentifier. These are false positives. Co-Authored-By: Claude Sonnet 4.6 --- module/api_v1_handler.go | 30 +++++++- module/api_v1_test.go | 65 +++++++++++++++++ plugin/storebrowser/handler.go | 44 +++++++++--- plugin/storebrowser/handler_test.go | 107 ++++++++++++++++++++++++++++ store/local_storage.go | 12 +++- store/workspace.go | 26 ++++++- store/workspace_test.go | 48 +++++++++++++ 7 files changed, 316 insertions(+), 16 deletions(-) diff --git a/module/api_v1_handler.go b/module/api_v1_handler.go index 9308cf8b..8503ca67 100644 --- a/module/api_v1_handler.go +++ b/module/api_v1_handler.go @@ -960,8 +960,31 @@ func (h *V1APIHandler) loadWorkflowFromPath(w http.ResponseWriter, r *http.Reque req.ProjectID = "00000000-0000-0000-0000-000000000002" } - // Resolve the config file path + // Resolve the config file path and enforce path containment. + // + // When h.dataDir is set (production), the resolved path must stay inside + // h.dataDir so that a malicious or misconfigured request cannot read + // arbitrary files from the host filesystem (e.g. /etc/passwd or + // ../../secrets). When dataDir is empty (e.g. in tests that construct the + // handler without SetDataDir), we still call filepath.Clean to normalise + // the path but accept it as-is because the caller is responsible for + // providing a trusted value in that mode. configPath := filepath.Clean(req.Path) + if h.dataDir != "" { + // Anchor: all paths must resolve to inside the configured data directory. + baseClean := filepath.Clean(h.dataDir) + string(os.PathSeparator) + absConfig, absErr := filepath.Abs(configPath) + if absErr != nil { + writeJSON(w, http.StatusBadRequest, map[string]string{"error": "invalid path"}) + return + } + if !strings.HasPrefix(absConfig+string(os.PathSeparator), baseClean) { + writeJSON(w, http.StatusForbidden, map[string]string{"error": "path is outside the allowed data directory"}) + return + } + configPath = absConfig + } + info, err := os.Stat(configPath) if err != nil { writeJSON(w, http.StatusBadRequest, map[string]string{"error": fmt.Sprintf("path not found: %s", configPath)}) @@ -978,8 +1001,9 @@ func (h *V1APIHandler) loadWorkflowFromPath(w http.ResponseWriter, r *http.Reque 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..db4b8486 100644 --- a/module/api_v1_test.go +++ b/module/api_v1_test.go @@ -1100,3 +1100,68 @@ 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") + + 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) + + // Must be rejected; we expect 400 (path not found / invalid path) + // or 403 (outside allowed directory). 200/201 would mean the handler + // served a file from outside the allowed base. + if rr.Code == http.StatusCreated || rr.Code == http.StatusOK { + t.Errorf("path %q: expected rejection, got %d (body: %s)", + p, 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..c4963fe0 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,109 @@ func TestInvalidTableName(t *testing.T) { t.Fatalf("expected 400 for invalid table name, got %d", w.Code) } } + +// 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..27f3108d 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,31 @@ 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)) + + // 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) From fcdabf990d9b911328581be91cbb9b9c76d706b0 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Fri, 29 May 2026 08:02:04 -0400 Subject: [PATCH 2/2] security: address adversarial review (wire SetDataDir, default-deny path read, revert-sensitive tests) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CRITICAL 1 — path-injection guard was dead in production: - cmd/server/main.go: wire v1Handler.SetDataDir(*dataDir) right after NewV1APIHandler so the loadWorkflowFromPath containment guard is actually reached in the running binary (previously SetDataDir was never called). - module/api_v1_handler.go: make containment active BY DEFAULT. If no dataDir is configured, fall back to the process working directory (filepath.Abs(".")) as the containment base rather than reading arbitrary absolute paths. Default-deny. IMPORTANT 2 — SetDataDir now normalizes to absolute: - SetDataDir stores filepath.Clean(filepath.Abs(dir)) so the HasPrefix compare in loadWorkflowFromPath is absolute-vs-absolute. A relative dataDir no longer makes the prefix check spuriously fail. IMPORTANT 3 — TestLoadWorkflowFromPath_PathTraversal now revert-sensitive: - Asserts EXACTLY http.StatusForbidden (403) for each out-of-base path — a status only the containment guard produces. Added TestLoadWorkflowFromPath_ContainedByDefault (no SetDataDir) asserting 403 for /etc/passwd via the working-dir default base. - Revert-restore proof: with the guard disabled, /etc/passwd is read and returned 201 with full file contents (live exploit); traversal-to-missing returns 400. All assertions fail without the guard; restored → pass. IMPORTANT 4 — storebrowser sort-injection tests now prove the NEW guard: - Added TestIsValidTableName_RejectsMalicious (direct unit test, fails to compile if the function is removed) and TestTableRowsSortInjection_GuardCatchesSchemaColumn, which creates a real column named "bad col" (passes the schema allowlist) and asserts the isValidTableName guard rejects it with 400 before the DB is queried. - Revert-restore proof: with isValidTableName removed, "bad col" reaches the DB and the request returns 500 (unsafe); restored → 400 pass. MINOR 5 — api_v1_handler.go no longer echoes the resolved absolute path in the "path not found" / "no workflow.yaml" error responses. MINOR 6 — store/workspace.go StorageForProject rejects projectID of "", ".", "..", or any value containing a path separator before computing the workspace path, preserving project isolation. database.go FP claim (#8, #9) confirmed by review — no change; lead to dismiss. Re-verified: go build ./...; go test ./module/... ./plugin/storebrowser/... ./store/... ./cmd/server/...; go vet; gofmt -l (clean); golangci-lint (clean). Co-Authored-By: Claude Opus 4.8 --- cmd/server/main.go | 5 ++ module/api_v1_handler.go | 62 ++++++++++++++++-------- module/api_v1_test.go | 33 +++++++++++-- plugin/storebrowser/handler_test.go | 74 +++++++++++++++++++++++++++++ store/workspace.go | 10 ++++ 5 files changed, 159 insertions(+), 25 deletions(-) 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 8503ca67..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. @@ -962,32 +975,41 @@ func (h *V1APIHandler) loadWorkflowFromPath(w http.ResponseWriter, r *http.Reque // Resolve the config file path and enforce path containment. // - // When h.dataDir is set (production), the resolved path must stay inside - // h.dataDir so that a malicious or misconfigured request cannot read - // arbitrary files from the host filesystem (e.g. /etc/passwd or - // ../../secrets). When dataDir is empty (e.g. in tests that construct the - // handler without SetDataDir), we still call filepath.Clean to normalise - // the path but accept it as-is because the caller is responsible for - // providing a trusted value in that mode. - configPath := filepath.Clean(req.Path) - if h.dataDir != "" { - // Anchor: all paths must resolve to inside the configured data directory. - baseClean := filepath.Clean(h.dataDir) + string(os.PathSeparator) - absConfig, absErr := filepath.Abs(configPath) - if absErr != nil { - writeJSON(w, http.StatusBadRequest, map[string]string{"error": "invalid path"}) - return + // 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 } - configPath = absConfig } 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 } @@ -995,7 +1017,7 @@ 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 diff --git a/module/api_v1_test.go b/module/api_v1_test.go index db4b8486..1eb71257 100644 --- a/module/api_v1_test.go +++ b/module/api_v1_test.go @@ -1114,6 +1114,11 @@ func TestLoadWorkflowFromPath_PathTraversal(t *testing.T) { 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", @@ -1126,17 +1131,35 @@ func TestLoadWorkflowFromPath_PathTraversal(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) - // Must be rejected; we expect 400 (path not found / invalid path) - // or 403 (outside allowed directory). 200/201 would mean the handler - // served a file from outside the allowed base. - if rr.Code == http.StatusCreated || rr.Code == http.StatusOK { - t.Errorf("path %q: expected rejection, got %d (body: %s)", + 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) { diff --git a/plugin/storebrowser/handler_test.go b/plugin/storebrowser/handler_test.go index c4963fe0..61e49b13 100644 --- a/plugin/storebrowser/handler_test.go +++ b/plugin/storebrowser/handler_test.go @@ -616,6 +616,80 @@ func TestInvalidTableName(t *testing.T) { } } +// 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). diff --git a/store/workspace.go b/store/workspace.go index 27f3108d..71d075d6 100644 --- a/store/workspace.go +++ b/store/workspace.go @@ -31,6 +31,16 @@ func (wm *WorkspaceManager) StorageForProject(projectID string) (*LocalStorage, return nil, fmt.Errorf("project ID is required") } + // 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 {