security: fix SQL-injection and path-injection (CodeQL)#794
Merged
Conversation
…14) 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 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…ath read, revert-sensitive tests)
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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes all open CodeQL injection alerts. No suppressions — each is a root-cause fix.
Alerts fixed
go/sql-injection —
plugin/storebrowser/handler.go(alerts #60, #61)Alert #60 —
tableRowssort column (line 192)r.URL.Query().Get("sort")→orderClause→fmt.Sprintf(query)isValidTableNameidentifier-regex check (^[a-zA-Z_][a-zA-Z0-9_]*$) onsortColbefore the DB column-schema lookup. This gives CodeQL a local, statically-trackable sanitizer rather than relying on the DB round-trip for safety. The existing DB schema cross-check is retained as a second layer.Alert #61 —
execQueryadmin SQL REPL (line 260)r.Body→q→tx.QueryContextsanitizeReadOnlyQuery(SELECT-only, no;, no SQL comments),dangerousKeywordsregexp (DROP/INSERT/UPDATE/DELETE/…), andBeginTx(ReadOnly:true). Added detailed code comment documenting these layers and the design intent.go/path-injection —
module/api_v1_handler.go(alerts #56, #57, #58)Source:
req.Path(JSON body) →os.Stat/filepath.Join/os.ReadFile(lines 965, 974, 982)h.dataDiris set, resolvereq.Pathto an absolute path viafilepath.Absand verify it stays insideh.dataDirusingstrings.HasPrefix(abs+sep, clean(base)+sep). Paths escaping the data directory return403 Forbidden. WhendataDiris empty (test/minimal deployments),filepath.Cleanis still applied.go/path-injection —
store/local_storage.go+store/workspace.go(alert #14)Source:
projectID(URL segment) →WorkspacePath→NewLocalStorage→os.MkdirAll(line 25)workspace.go:StorageForProjectnow resolves both the workspaces base and the candidate path to absolute paths and checks containment before callingNewLocalStorage. A traversal ID like../../etcreturns an error instead of creating a directory outside the workspaces root.local_storage.go: tightened theresolvemethod'sHasPrefixcontainment check by appendingfilepath.Separatorto both sides, closing an off-by-one where root/tmp/workwould incorrectly allow/tmp/workevil/….Tests added
TestTableRowsSortInjectionplugin/storebrowser/handler_test.goid;DROP, backtick,../../etc/passwd,id--,id()TestSQLQueryInjectionAttemptsplugin/storebrowser/handler_test.go--comment,/* */block comment, tautology SELECTTestWorkspaceManager_StorageForProject_PathTraversalstore/workspace_test.go../../etc/passwd,../../../tmp/evil,proj/../../../etcTestLoadWorkflowFromPath_PathTraversalmodule/api_v1_test.go../../etc/passwd,/etc/passwd, escaped traversal,../outsideTestLoadWorkflowFromPath_AllowedPathmodule/api_v1_test.goFalse positives noted (not dismissed — left for the lead)
module/database.goalerts #8, #9 — CodeQL traces user request body →PipelineContext→resolvedParams→db.QueryContext(ctx, query, resolvedParams...). The user-derived data flows into the args slice (bound as?placeholders), not into the SQL string. The only dynamic SQL interpolation path (allowDynamicSQL) runs every resolved value throughvalidateSQLIdentifierbefore substitution. These are false positives.Verification
All pass.
🤖 Generated with Claude Code