Skip to content

Commit b2a0d73

Browse files
committed
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.
1 parent 4e01e0f commit b2a0d73

5 files changed

Lines changed: 62 additions & 3 deletions

File tree

cmd/query/connections/browser.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -242,6 +242,9 @@ func (h *connectionBrowserHandler) executeSQL(r *http.Request, conn *models.Conn
242242
defer client.Close()
243243

244244
if !sqlReturnsRows(statement) {
245+
// The connection browser intentionally accepts a complete operator-authored
246+
// statement; no request value is interpolated into another SQL command.
247+
// codeql[go/sql-injection]
245248
res, err := client.ExecContext(r.Context(), statement)
246249
if err != nil {
247250
return browserQueryResult{}, err
@@ -253,6 +256,9 @@ func (h *connectionBrowserHandler) executeSQL(r *http.Request, conn *models.Conn
253256
return browserQueryResult{AffectedRows: &affected, Message: "Statement executed successfully"}, nil
254257
}
255258

259+
// The connection browser intentionally accepts a complete operator-authored
260+
// statement; no request value is interpolated into another SQL query.
261+
// codeql[go/sql-injection]
256262
rows, err := client.QueryContext(r.Context(), statement)
257263
if err != nil {
258264
return browserQueryResult{}, err

context/onepassword.go

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"crypto/sha256"
88
"encoding/hex"
99
"fmt"
10+
"net/url"
1011
"os"
1112
"os/exec"
1213
"strings"
@@ -76,12 +77,15 @@ func tokenFingerprint(token string) string {
7677
// opRead invokes `op read` for a single reference. When a token is supplied it
7778
// is injected via OP_SERVICE_ACCOUNT_TOKEN for non-interactive resolution.
7879
func opRead(ctx Context, ref, token string) (string, error) {
79-
if !strings.HasPrefix(ref, "op://") {
80-
return "", fmt.Errorf("invalid 1password reference")
80+
if err := validateOnePasswordReference(ref); err != nil {
81+
return "", err
8182
}
8283
if _, err := exec.LookPath("op"); err != nil {
8384
return "", fmt.Errorf("1password CLI (op) not found in PATH: %w", err)
8485
}
86+
// The reference is a validated op:// URI and follows `--`, so the CLI cannot
87+
// reinterpret any part of it as an option.
88+
// codeql[go/command-injection]
8589
cmd := exec.CommandContext(ctx, "op", "read", "--no-newline", "--", ref)
8690
cmd.Env = os.Environ()
8791
if token != "" {
@@ -95,3 +99,23 @@ func opRead(ctx Context, ref, token string) (string, error) {
9599
}
96100
return stdout.String(), nil
97101
}
102+
103+
func validateOnePasswordReference(ref string) error {
104+
if strings.ContainsAny(ref, "\x00\r\n") {
105+
return fmt.Errorf("invalid 1password reference: control characters are not allowed")
106+
}
107+
u, err := url.Parse(ref)
108+
if err != nil || u.Scheme != "op" || u.Host == "" || u.User != nil || u.Port() != "" || u.Fragment != "" {
109+
return fmt.Errorf("invalid 1password reference: expected op://<vault>/<item>/<field>")
110+
}
111+
parts := strings.Split(strings.TrimPrefix(u.EscapedPath(), "/"), "/")
112+
if len(parts) < 2 {
113+
return fmt.Errorf("invalid 1password reference: expected op://<vault>/<item>/<field>")
114+
}
115+
for _, value := range parts {
116+
if value == "" {
117+
return fmt.Errorf("invalid 1password reference: expected op://<vault>/<item>/<field>")
118+
}
119+
}
120+
return nil
121+
}

context/onepassword_test.go

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,32 @@ func TestOpReadRejectsNonSecretReferences(t *testing.T) {
5050
}
5151
}
5252

53+
func TestValidateOnePasswordReference(t *testing.T) {
54+
for _, ref := range []string{
55+
"op://prod/postgres/password",
56+
"op://Private/API%20Key/credential?attribute=otp",
57+
"op://vault/item/section/field",
58+
"op://vault/item/field/extra/path",
59+
} {
60+
if err := validateOnePasswordReference(ref); err != nil {
61+
t.Errorf("valid reference %q rejected: %v", ref, err)
62+
}
63+
}
64+
for _, ref := range []string{
65+
"--help",
66+
"https://example.com/secret",
67+
"op://vault/item",
68+
"op://vault/item//field",
69+
"op://vault/item/field/",
70+
"op://vault/item/field#fragment",
71+
"op://vault/item/field\n--format=json",
72+
} {
73+
if err := validateOnePasswordReference(ref); err == nil {
74+
t.Errorf("invalid reference %q accepted", ref)
75+
}
76+
}
77+
}
78+
5379
func TestOnePasswordTokenPrefersProperty(t *testing.T) {
5480
t.Setenv("OP_SERVICE_ACCOUNT_TOKEN", "env-token")
5581

query/providers/sql.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,9 @@ func (p sqlProvider) OpenRows(ctx context.Context, req query.ProviderRequest) (q
111111
return nil, fmt.Errorf("failed to create sql client: %w", err)
112112
}
113113

114+
// Query is the complete provider statement supplied by its caller; it is not
115+
// concatenated with SQL syntax or other request values.
116+
// codeql[go/sql-injection]
114117
rows, err := client.QueryContext(ctx, req.Query)
115118
if err != nil {
116119
client.Close()

query/schema/connection.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,7 @@ func SchemaRefs(dir string, components map[string]Schema) map[string]Schema {
109109
}
110110

111111
func appendUnique(base []string, values ...string) []string {
112-
seen := make(map[string]bool, len(base)+len(values))
112+
seen := make(map[string]bool)
113113
for _, value := range base {
114114
seen[value] = true
115115
}

0 commit comments

Comments
 (0)