From aa724f42f295c23f3a833fbb862ffa458e253643 Mon Sep 17 00:00:00 2001 From: Javier Rodriguez Date: Sun, 14 Jun 2026 13:03:46 +0200 Subject: [PATCH] fix(jsonfilter): Sanitize input when parsing filters Signed-off-by: Javier Rodriguez --- .../pkg/biz/workflow_integration_test.go | 30 +++++++++++++ app/controlplane/pkg/data/workflow.go | 32 +++++++++----- pkg/jsonfilter/jsonfilter.go | 36 ++++++++++++++- pkg/jsonfilter/jsonfilter_test.go | 44 ++++++++++++++++++- 4 files changed, 128 insertions(+), 14 deletions(-) diff --git a/app/controlplane/pkg/biz/workflow_integration_test.go b/app/controlplane/pkg/biz/workflow_integration_test.go index 9b95214d4..842334589 100644 --- a/app/controlplane/pkg/biz/workflow_integration_test.go +++ b/app/controlplane/pkg/biz/workflow_integration_test.go @@ -28,6 +28,7 @@ import ( "github.com/chainloop-dev/chainloop/app/controlplane/pkg/biz/testhelpers" "github.com/chainloop-dev/chainloop/pkg/credentials" creds "github.com/chainloop-dev/chainloop/pkg/credentials/mocks" + "github.com/chainloop-dev/chainloop/pkg/jsonfilter" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" "github.com/google/uuid" @@ -561,6 +562,35 @@ func (s *workflowListIntegrationTestSuite) TestList() { s.Len(workflows, 1) s.Equal(2, count) }) + + s.Run("rejects a JSON filter with an unsafe field path as a validation error", func() { + opts := &biz.WorkflowListOpts{ + JSONFilters: []*jsonfilter.JSONFilter{ + { + // SQL injection attempt: a single quote breaks out of the JSON path literal. + FieldPath: "x'='x' OR (SELECT 1 FROM pg_sleep(2)) IS NOT NULL OR 'z", + Operator: jsonfilter.OpEQ, + Value: "true", + }, + }, + } + + workflows, _, err := s.Workflow.List(ctx, s.org.ID, opts, nil) + s.Error(err) + s.True(biz.IsErrValidation(err), "unsafe field path must surface as a validation error, got: %v", err) + s.Nil(workflows) + }) + + s.Run("accepts a JSON filter with a safe field path", func() { + opts := &biz.WorkflowListOpts{ + JSONFilters: []*jsonfilter.JSONFilter{ + {FieldPath: "needsAttention", Operator: jsonfilter.OpEQ, Value: "true"}, + }, + } + + _, _, err := s.Workflow.List(ctx, s.org.ID, opts, nil) + s.NoError(err) + }) } // Run the tests diff --git a/app/controlplane/pkg/data/workflow.go b/app/controlplane/pkg/data/workflow.go index d8e3a8a13..2593b98c1 100644 --- a/app/controlplane/pkg/data/workflow.go +++ b/app/controlplane/pkg/data/workflow.go @@ -281,7 +281,10 @@ func (r *WorkflowRepo) List(ctx context.Context, orgID uuid.UUID, filter *biz.Wo wfQuery := baseQuery.Where(workflow.DeletedAtIsNil()) // Apply additional filters to the Workflow query based on the provided options - wfQuery = applyWorkflowFilters(wfQuery, filter) + wfQuery, err := applyWorkflowFilters(wfQuery, filter) + if err != nil { + return nil, 0, err + } // Get the count of all filtered rows without the limit and offset count, err := wfQuery.Count(ctx) @@ -339,7 +342,7 @@ func applyWorkflowRunFilters(baseQuery *ent.WorkflowQuery, opts *biz.WorkflowLis } // applyWorkflowFilters applies filters to the Workflow query based on the provided options -func applyWorkflowFilters(wfQuery *ent.WorkflowQuery, opts *biz.WorkflowListOpts) *ent.WorkflowQuery { +func applyWorkflowFilters(wfQuery *ent.WorkflowQuery, opts *biz.WorkflowListOpts) (*ent.WorkflowQuery, error) { if opts != nil { if opts.WorkflowPublic != nil { wfQuery = wfQuery.Where(workflow.Public(*opts.WorkflowPublic)) @@ -364,16 +367,21 @@ func applyWorkflowFilters(wfQuery *ent.WorkflowQuery, opts *biz.WorkflowListOpts // Append the JSON Filters to the query if len(opts.JSONFilters) != 0 { - wfQuery = wfQuery.Where(func(selector *sql.Selector) { - // Build the predicates for each JSON filter - predicates := make([]*sql.Predicate, 0, len(opts.JSONFilters)) - for _, filter := range opts.JSONFilters { - // Include the column where the filter is applied - filter.Column = workflow.FieldMetadata - jsonPredicate, _ := jsonfilter.BuildEntSelectorFromJSONFilter(filter) - predicates = append(predicates, jsonPredicate) + // Build the predicates for each JSON filter up front so that any + // validation error (e.g. an unsafe field path) is surfaced instead + // of being silently ignored inside the query builder closure. + predicates := make([]*sql.Predicate, 0, len(opts.JSONFilters)) + for _, filter := range opts.JSONFilters { + // Include the column where the filter is applied + filter.Column = workflow.FieldMetadata + jsonPredicate, err := jsonfilter.BuildEntSelectorFromJSONFilter(filter) + if err != nil { + return nil, biz.NewErrValidation(fmt.Errorf("invalid JSON filter: %w", err)) } - // Combine the predicates using OR logic + predicates = append(predicates, jsonPredicate) + } + wfQuery = wfQuery.Where(func(selector *sql.Selector) { + // Combine the predicates using AND logic selector.Where(sql.And(predicates...)) }) } @@ -396,7 +404,7 @@ func applyWorkflowFilters(wfQuery *ent.WorkflowQuery, opts *biz.WorkflowListOpts } } - return wfQuery + return wfQuery, nil } // GetOrgScoped Gets a workflow making sure it belongs to a given org diff --git a/pkg/jsonfilter/jsonfilter.go b/pkg/jsonfilter/jsonfilter.go index 7f9767d98..998d66829 100644 --- a/pkg/jsonfilter/jsonfilter.go +++ b/pkg/jsonfilter/jsonfilter.go @@ -1,5 +1,5 @@ // -// Copyright 2025 The Chainloop Authors. +// Copyright 2025-2026 The Chainloop Authors. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -18,12 +18,24 @@ package jsonfilter import ( "errors" "fmt" + "regexp" "strings" entsql "entgo.io/ent/dialect/sql" "entgo.io/ent/dialect/sql/sqljson" ) +// fieldPathRegexp matches a safe JSON field path expressed in dot notation. +// A path is a dot-separated sequence of identifiers (starting with a letter or +// underscore) optionally followed by numeric array indices, e.g. "name", +// "labels.env" or "items[0].name". +// +// This allowlist is security-critical: the underlying ent sqljson helpers +// concatenate path segments verbatim into single-quoted PostgreSQL JSON path +// literals without escaping, so any character outside this set (e.g. a single +// quote) would allow breaking out of the literal and injecting arbitrary SQL. +var fieldPathRegexp = regexp.MustCompile(`^[A-Za-z_][A-Za-z0-9_]*(?:\.[A-Za-z_][A-Za-z0-9_]*|\[[0-9]+\])*$`) + // JSONOperator represents supported JSON filter operators. type JSONOperator string @@ -54,6 +66,13 @@ func BuildEntSelectorFromJSONFilter(jsonFilter *JSONFilter) (*entsql.Predicate, return nil, errors.New("invalid filter: column and operator are required") } + // Validate the field path before it reaches the SQL builder. The ent + // sqljson helpers concatenate the path segments unescaped into the query, + // so an unsafe value would allow SQL injection. + if err := validateFieldPath(jsonFilter.FieldPath); err != nil { + return nil, err + } + // Convert the dot notation to the path that Ent expects. dotPath := sqljson.DotPath(jsonFilter.FieldPath) @@ -85,3 +104,18 @@ func BuildEntSelectorFromJSONFilter(jsonFilter *JSONFilter) (*entsql.Predicate, return nil, fmt.Errorf("unsupported operator: %s", jsonFilter.Operator) } } + +// validateFieldPath ensures the JSON field path only contains safe characters +// before it is concatenated into the SQL query by the ent sqljson helpers. +// An empty path is allowed: it targets the column itself and is not injectable. +func validateFieldPath(fieldPath string) error { + if fieldPath == "" { + return nil + } + + if !fieldPathRegexp.MatchString(fieldPath) { + return fmt.Errorf("invalid field path %q: must be dot-separated identifiers with optional numeric array indices", fieldPath) + } + + return nil +} diff --git a/pkg/jsonfilter/jsonfilter_test.go b/pkg/jsonfilter/jsonfilter_test.go index 608beac66..20c57f586 100644 --- a/pkg/jsonfilter/jsonfilter_test.go +++ b/pkg/jsonfilter/jsonfilter_test.go @@ -22,6 +22,9 @@ import ( "github.com/stretchr/testify/require" ) +// errInvalidFieldPath is the common prefix returned when a field path fails validation. +const errInvalidFieldPath = "invalid field path" + func TestBuildEntSelectorFromJSONFilter(t *testing.T) { tests := []struct { name string @@ -81,13 +84,52 @@ func TestBuildEntSelectorFromJSONFilter(t *testing.T) { filter: &JSONFilter{Column: "metadata", FieldPath: "env", Operator: OpIN, Value: []string{"prod", "dev"}}, wantErr: "invalid value for 'in' operator: must be a slice of strings", }, + { + name: "field path with array index", + filter: &JSONFilter{Column: "metadata", FieldPath: "items[0].name", Operator: OpEQ, Value: "foo"}, + }, + { + name: "field path with single quote breaks out of literal", + filter: &JSONFilter{Column: "metadata", FieldPath: "x'='x' OR (SELECT 1 FROM pg_sleep(2)) IS NOT NULL OR 'z", Operator: OpEQ, Value: "true"}, + wantErr: errInvalidFieldPath, + }, + { + name: "field path with double quote", + filter: &JSONFilter{Column: "metadata", FieldPath: `name"`, Operator: OpEQ, Value: "foo"}, + wantErr: errInvalidFieldPath, + }, + { + name: "field path with semicolon", + filter: &JSONFilter{Column: "metadata", FieldPath: "name;DROP TABLE workflows", Operator: OpEQ, Value: "foo"}, + wantErr: errInvalidFieldPath, + }, + { + name: "field path with whitespace", + filter: &JSONFilter{Column: "metadata", FieldPath: "name OR 1=1", Operator: OpEQ, Value: "foo"}, + wantErr: errInvalidFieldPath, + }, + { + name: "field path with parenthesis", + filter: &JSONFilter{Column: "metadata", FieldPath: "pg_sleep(2)", Operator: OpEQ, Value: "foo"}, + wantErr: errInvalidFieldPath, + }, + { + name: "field path with leading digit segment", + filter: &JSONFilter{Column: "metadata", FieldPath: "1name", Operator: OpEQ, Value: "foo"}, + wantErr: errInvalidFieldPath, + }, + { + name: "field path with trailing dot", + filter: &JSONFilter{Column: "metadata", FieldPath: "name.", Operator: OpEQ, Value: "foo"}, + wantErr: errInvalidFieldPath, + }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { pred, err := BuildEntSelectorFromJSONFilter(tc.filter) if tc.wantErr != "" { - require.EqualError(t, err, tc.wantErr) + require.ErrorContains(t, err, tc.wantErr) assert.Nil(t, pred) } else { require.NoError(t, err)