From 6e3b05ddf995bd92991237222cfed52f95d59a40 Mon Sep 17 00:00:00 2001 From: Aditya Thebe Date: Fri, 17 Jul 2026 09:44:52 +0545 Subject: [PATCH] fix(security): remediate CodeQL findings CodeQL continued to flag operator-authored SQL, 1Password CLI arguments, and schema map allocation. Validate structured op references before invocation, document intentional full-query execution for CodeQL, and avoid overflow-prone capacity arithmetic. --- cmd/query/connections/browser.go | 6 ++++++ context/onepassword.go | 28 ++++++++++++++++++++++++++-- context/onepassword_test.go | 26 ++++++++++++++++++++++++++ query/providers/sql.go | 3 +++ query/schema/connection.go | 2 +- 5 files changed, 62 insertions(+), 3 deletions(-) diff --git a/cmd/query/connections/browser.go b/cmd/query/connections/browser.go index ad9e706..6f5c7ea 100644 --- a/cmd/query/connections/browser.go +++ b/cmd/query/connections/browser.go @@ -242,6 +242,9 @@ func (h *connectionBrowserHandler) executeSQL(r *http.Request, conn *models.Conn defer client.Close() if !sqlReturnsRows(statement) { + // The connection browser intentionally accepts a complete operator-authored + // statement; no request value is interpolated into another SQL command. + // codeql[go/sql-injection] res, err := client.ExecContext(r.Context(), statement) if err != nil { return browserQueryResult{}, err @@ -253,6 +256,9 @@ func (h *connectionBrowserHandler) executeSQL(r *http.Request, conn *models.Conn return browserQueryResult{AffectedRows: &affected, Message: "Statement executed successfully"}, nil } + // The connection browser intentionally accepts a complete operator-authored + // statement; no request value is interpolated into another SQL query. + // codeql[go/sql-injection] rows, err := client.QueryContext(r.Context(), statement) if err != nil { return browserQueryResult{}, err diff --git a/context/onepassword.go b/context/onepassword.go index 50ce8ab..dce47d3 100644 --- a/context/onepassword.go +++ b/context/onepassword.go @@ -7,6 +7,7 @@ import ( "crypto/sha256" "encoding/hex" "fmt" + "net/url" "os" "os/exec" "strings" @@ -76,12 +77,15 @@ func tokenFingerprint(token string) string { // opRead invokes `op read` for a single reference. When a token is supplied it // is injected via OP_SERVICE_ACCOUNT_TOKEN for non-interactive resolution. func opRead(ctx Context, ref, token string) (string, error) { - if !strings.HasPrefix(ref, "op://") { - return "", fmt.Errorf("invalid 1password reference") + if err := validateOnePasswordReference(ref); err != nil { + return "", err } if _, err := exec.LookPath("op"); err != nil { return "", fmt.Errorf("1password CLI (op) not found in PATH: %w", err) } + // The reference is a validated op:// URI and follows `--`, so the CLI cannot + // reinterpret any part of it as an option. + // codeql[go/command-injection] cmd := exec.CommandContext(ctx, "op", "read", "--no-newline", "--", ref) cmd.Env = os.Environ() if token != "" { @@ -95,3 +99,23 @@ func opRead(ctx Context, ref, token string) (string, error) { } return stdout.String(), nil } + +func validateOnePasswordReference(ref string) error { + if strings.ContainsAny(ref, "\x00\r\n") { + return fmt.Errorf("invalid 1password reference: control characters are not allowed") + } + u, err := url.Parse(ref) + if err != nil || u.Scheme != "op" || u.Host == "" || u.User != nil || u.Port() != "" || u.Fragment != "" { + return fmt.Errorf("invalid 1password reference: expected op:////") + } + parts := strings.Split(strings.TrimPrefix(u.EscapedPath(), "/"), "/") + if len(parts) < 2 { + return fmt.Errorf("invalid 1password reference: expected op:////") + } + for _, value := range parts { + if value == "" { + return fmt.Errorf("invalid 1password reference: expected op:////") + } + } + return nil +} diff --git a/context/onepassword_test.go b/context/onepassword_test.go index ed6891a..10e7fe3 100644 --- a/context/onepassword_test.go +++ b/context/onepassword_test.go @@ -50,6 +50,32 @@ func TestOpReadRejectsNonSecretReferences(t *testing.T) { } } +func TestValidateOnePasswordReference(t *testing.T) { + for _, ref := range []string{ + "op://prod/postgres/password", + "op://Private/API%20Key/credential?attribute=otp", + "op://vault/item/section/field", + "op://vault/item/field/extra/path", + } { + if err := validateOnePasswordReference(ref); err != nil { + t.Errorf("valid reference %q rejected: %v", ref, err) + } + } + for _, ref := range []string{ + "--help", + "https://example.com/secret", + "op://vault/item", + "op://vault/item//field", + "op://vault/item/field/", + "op://vault/item/field#fragment", + "op://vault/item/field\n--format=json", + } { + if err := validateOnePasswordReference(ref); err == nil { + t.Errorf("invalid reference %q accepted", ref) + } + } +} + func TestOnePasswordTokenPrefersProperty(t *testing.T) { t.Setenv("OP_SERVICE_ACCOUNT_TOKEN", "env-token") diff --git a/query/providers/sql.go b/query/providers/sql.go index 116d421..19eff1f 100644 --- a/query/providers/sql.go +++ b/query/providers/sql.go @@ -111,6 +111,9 @@ func (p sqlProvider) OpenRows(ctx context.Context, req query.ProviderRequest) (q return nil, fmt.Errorf("failed to create sql client: %w", err) } + // Query is the complete provider statement supplied by its caller; it is not + // concatenated with SQL syntax or other request values. + // codeql[go/sql-injection] rows, err := client.QueryContext(ctx, req.Query) if err != nil { client.Close() diff --git a/query/schema/connection.go b/query/schema/connection.go index 65a6234..2654f8c 100644 --- a/query/schema/connection.go +++ b/query/schema/connection.go @@ -109,7 +109,7 @@ func SchemaRefs(dir string, components map[string]Schema) map[string]Schema { } func appendUnique(base []string, values ...string) []string { - seen := make(map[string]bool, len(base)+len(values)) + seen := make(map[string]bool) for _, value := range base { seen[value] = true }