Skip to content

Update patch policy generation and tests #45799

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
allenhouchins merged 4 commits into from
May 19, 2026
Merged

Update patch policy generation and tests #45799

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d2171b6
Refactor patch policy SQL generation and tests
allenhouchins May 19, 2026
cad5c58
Fix linting issues
allenhouchins May 19, 2026
f6fe75a
Merge origin/main into allenhouchins-update-patch-policy-gen
allenhouchins May 19, 2026
f0589c2
Merge branch 'main' into allenhouchins-update-patch-policy-gen
allenhouchins May 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ee/maintained-apps/ingesters/homebrew/ingester_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ func TestIngestValidations(t *testing.T) {
)
} else {
require.Equal(t,
fmt.Sprintf("SELECT 1 WHERE NOT EXISTS ((SELECT 1 FROM apps WHERE bundle_identifier = '%s') AND version_compare(bundle_short_version, '%s') < 0);", c.inputApp.UniqueIdentifier, out.Version),
fmt.Sprintf("SELECT 1 WHERE NOT EXISTS (SELECT 1 FROM apps WHERE bundle_identifier = '%s' AND version_compare(bundle_short_version, '%s') < 0);", c.inputApp.UniqueIdentifier, out.Version),
out.Queries.Patched,
)
}
Expand Down
82 changes: 68 additions & 14 deletions pkg/patch_policy/patch_policy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,36 +19,90 @@ type PolicyData struct {
}

const (
// templateStart and templateEnd* wrap the caller-supplied exists query in an
// inner set of parentheses so that any OR in the WHERE body binds before the
// appended AND version_compare(...) clause.
templateStart = "SELECT 1 WHERE NOT EXISTS (("
templateEndDarwin = ") AND version_compare(bundle_short_version, '%s') < 0);"
templateEndWindows = ") AND version_compare(version, '%s') < 0);"
// notExistsStart is prepended to the exists query body; version_compare is appended
// to the same WHERE clause (inside NOT EXISTS), matching the pre-#45647 generator.
notExistsStart = "SELECT 1 WHERE NOT EXISTS ("
existsPrefix = "SELECT 1 FROM "
)

var (
ErrWrongPlatform = errors.New("platform should be darwin or windows")
ErrNoExistsQuery = errors.New("exists query was not provided")
)

// GenerateQueryForManifest wraps the "exists" query to create a patch policy query
// GenerateQueryForManifest wraps the "exists" query to create a patch policy query.
func GenerateQueryForManifest(p PolicyData) (string, error) {
if p.ExistsQuery == "" {
return "", ErrNoExistsQuery
}

suffix, err := versionCompareSuffix(p.Platform, p.ExistsQuery, p.Version)
if err != nil {
return "", err
}

before, _ := strings.CutSuffix(p.ExistsQuery, ";")
// Escape any literal '%' in the exists query (e.g. SQL LIKE patterns)
// so fmt.Sprintf doesn't interpret them as format verbs.
before = strings.ReplaceAll(before, "%", "%%")
before = strings.TrimSpace(before)
if strings.Contains(before, " OR ") {
before = parenthesizeWhereClause(before)
}

return notExistsStart + before + suffix, nil
Comment on lines 44 to +50

@coderabbitai coderabbitai Bot May 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Validate ExistsQuery has a WHERE clause before appending AND version_compare(...).

Line 50 can emit invalid SQL for inputs like SELECT 1 FROM apps; (... FROM apps AND version_compare(...)) and also permits unscoped matching. Guard this early and return an explicit error when WHERE is missing.

Suggested fix 
 var (
 	ErrWrongPlatform = errors.New("platform should be darwin or windows")
 	ErrNoExistsQuery = errors.New("exists query was not provided")
+	ErrInvalidExistsQuery = errors.New("exists query must include a WHERE clause")
 )
@@
 	before, _ := strings.CutSuffix(p.ExistsQuery, ";")
 	before = strings.TrimSpace(before)
+	if !strings.Contains(strings.ToUpper(before), " WHERE ") {
+		return "", ErrInvalidExistsQuery
+	}
 	if strings.Contains(before, " OR ") {
 		before = parenthesizeWhereClause(before)
 	}

As per coding guidelines: “ensure that appropriate filtering criteria are applied… Check for missing WHERE clauses…”

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/patch_policy/patch_policy.go` around lines 44 - 50, Ensure the generated
NOT EXISTS SQL only appends the version_compare predicate when the original
EXISTS query contains a WHERE clause: after computing before (from p.ExistsQuery
and parenthesizeWhereClause), validate that before contains a WHERE clause (use
a case-insensitive check or regex for word boundary "WHERE") and if not, return
an explicit error instead of returning notExistsStart + before + suffix; update
the function that builds this SQL (referencing p.ExistsQuery, before,
parenthesizeWhereClause, notExistsStart, and suffix) to perform this guard and
emit a clear error message when WHERE is missing.

}

// parenthesizeWhereClause wraps the WHERE body in parens when it contains OR so that
// the trailing AND version_compare(...) binds to the full predicate, not just the
// right-hand side of OR (SQL precedence: AND > OR).
func parenthesizeWhereClause(existsQuery string) string {
if !strings.HasPrefix(existsQuery, existsPrefix) {
return existsQuery
}
rest := strings.TrimPrefix(existsQuery, existsPrefix)
table, conditions, found := strings.Cut(rest, " WHERE ")
if !found {
return existsQuery
}
if !strings.Contains(conditions, " OR ") {
return existsQuery
}
return existsPrefix + table + " WHERE (" + conditions + ")"
}

switch p.Platform {
func versionCompareSuffix(platform, existsQuery, version string) (string, error) {
column, err := versionCompareColumn(platform, existsQuery)
if err != nil {
return "", err
}
return fmt.Sprintf(" AND version_compare(%s, '%s') < 0);", column, version), nil
}
Comment on lines +71 to +77

@coderabbitai coderabbitai Bot May 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Escape single quotes in version before SQL interpolation.

Line 76 interpolates version directly into a quoted SQL literal. A version containing ' can break SQL or alter predicate semantics. Escape with SQL-standard doubled quotes.

Suggested fix 
 func versionCompareSuffix(platform, existsQuery, version string) (string, error) {
 	column, err := versionCompareColumn(platform, existsQuery)
 	if err != nil {
 		return "", err
 	}
-	return fmt.Sprintf(" AND version_compare(%s, '%s') < 0);", column, version), nil
+	safeVersion := strings.ReplaceAll(version, "'", "''")
+	return fmt.Sprintf(" AND version_compare(%s, '%s') < 0);", column, safeVersion), nil
 }

As per coding guidelines: “Review all SQL queries for possible SQL injection.”

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func versionCompareSuffix(platform, existsQuery, version string) (string, error) {
column, err := versionCompareColumn(platform, existsQuery)
if err != nil {
return "", err
}
return fmt.Sprintf(" AND version_compare(%s, '%s') < 0);", column, version), nil
}
func versionCompareSuffix(platform, existsQuery, version string) (string, error) {
column, err := versionCompareColumn(platform, existsQuery)
if err != nil {
return "", err
}
safeVersion := strings.ReplaceAll(version, "'", "''")
return fmt.Sprintf(" AND version_compare(%s, '%s') < 0);", column, safeVersion), nil
}
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/patch_policy/patch_policy.go` around lines 71 - 77, The function
versionCompareSuffix builds an SQL literal by interpolating the version argument
directly, which allows a single quote in version to break the SQL; modify
versionCompareSuffix to escape single quotes in version (replace ' with '' per
SQL standard) before formatting the string so the produced clause is safe,
keeping use of versionCompareColumn(platform, existsQuery) unchanged and
returning the formatted AND version_compare(...) clause with the escaped
version.


func versionCompareColumn(platform, existsQuery string) (string, error) {
switch platform {
case "darwin":
return fmt.Sprintf(templateStart+before+templateEndDarwin, p.Version), nil
return "bundle_short_version", nil
case "windows":
return fmt.Sprintf(templateStart+before+templateEndWindows, p.Version), nil
if tableFromExistsQuery(existsQuery) == "file" {
return "file_version", nil
}
return "version", nil
default:
return "", ErrWrongPlatform
}
}

func tableFromExistsQuery(existsQuery string) string {
trimmed, _ := strings.CutSuffix(strings.TrimSpace(existsQuery), ";")
if !strings.HasPrefix(trimmed, existsPrefix) {
return ""
}
rest := strings.TrimPrefix(trimmed, existsPrefix)
if table, _, found := strings.Cut(rest, " WHERE "); found {
return table
}
if table, _, found := strings.Cut(rest, " "); found {
return table
}
return "", ErrWrongPlatform
return strings.TrimSpace(rest)
}

// GenerateFromInstaller creates a patch policy with all fields from an installer
Expand Down
20 changes: 11 additions & 9 deletions pkg/patch_policy/patch_policy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ func TestGenerateQueryForManifest(t *testing.T) {
Version: "1.0",
ExistsQuery: "SELECT 1 FROM apps WHERE bundle_identifier = 'com.foo';",
},
want: "SELECT 1 WHERE NOT EXISTS ((SELECT 1 FROM apps WHERE bundle_identifier = 'com.foo') AND version_compare(bundle_short_version, '1.0') < 0);",
want: "SELECT 1 WHERE NOT EXISTS (SELECT 1 FROM apps WHERE bundle_identifier = 'com.foo' AND version_compare(bundle_short_version, '1.0') < 0);",
},
{
name: "windows from exists query",
Expand All @@ -29,7 +29,7 @@ func TestGenerateQueryForManifest(t *testing.T) {
Version: "1.0",
ExistsQuery: "SELECT 1 FROM programs WHERE name = 'Foo x64' AND publisher = 'Bar, Inc.';",
},
want: "SELECT 1 WHERE NOT EXISTS ((SELECT 1 FROM programs WHERE name = 'Foo x64' AND publisher = 'Bar, Inc.') AND version_compare(version, '1.0') < 0);",
want: "SELECT 1 WHERE NOT EXISTS (SELECT 1 FROM programs WHERE name = 'Foo x64' AND publisher = 'Bar, Inc.' AND version_compare(version, '1.0') < 0);",
},
{
name: "windows from exists query with LIKE percent wildcard",
Expand All @@ -38,7 +38,7 @@ func TestGenerateQueryForManifest(t *testing.T) {
Version: "12.5.6",
ExistsQuery: "SELECT 1 FROM programs WHERE name LIKE 'Postman x64 %' AND publisher = 'Postman';",
},
want: "SELECT 1 WHERE NOT EXISTS ((SELECT 1 FROM programs WHERE name LIKE 'Postman x64 %' AND publisher = 'Postman') AND version_compare(version, '12.5.6') < 0);",
want: "SELECT 1 WHERE NOT EXISTS (SELECT 1 FROM programs WHERE name LIKE 'Postman x64 %' AND publisher = 'Postman' AND version_compare(version, '12.5.6') < 0);",
},
{
name: "windows from exists query with multiple LIKE percent wildcards",
Expand All @@ -47,16 +47,18 @@ func TestGenerateQueryForManifest(t *testing.T) {
Version: "139.0.0",
ExistsQuery: "SELECT 1 FROM programs WHERE name LIKE 'Mozilla Firefox % ESR %' AND publisher = 'Mozilla';",
},
want: "SELECT 1 WHERE NOT EXISTS ((SELECT 1 FROM programs WHERE name LIKE 'Mozilla Firefox % ESR %' AND publisher = 'Mozilla') AND version_compare(version, '139.0.0') < 0);",
want: "SELECT 1 WHERE NOT EXISTS (SELECT 1 FROM programs WHERE name LIKE 'Mozilla Firefox % ESR %' AND publisher = 'Mozilla' AND version_compare(version, '139.0.0') < 0);",
},
{
name: "windows from exists query containing OR (precedence fix)",
name: "codex-cli portable install OR precedence and file_version",
p: patch_policy.PolicyData{
Platform: "windows",
Version: "0.130.0",
ExistsQuery: "SELECT 1 FROM file WHERE path = 'C:\\a' OR path LIKE '%\\b';",
Platform: "windows",
Version: "0.130.0",
ExistsQuery: "SELECT 1 FROM file WHERE path = 'C:\\Program Files\\Codex CLI\\codex.exe' " +
"OR path LIKE '%\\AppData\\Local\\Programs\\Codex CLI\\codex.exe';",
},
want: "SELECT 1 WHERE NOT EXISTS ((SELECT 1 FROM file WHERE path = 'C:\\a' OR path LIKE '%\\b') AND version_compare(version, '0.130.0') < 0);",
want: "SELECT 1 WHERE NOT EXISTS (SELECT 1 FROM file WHERE (path = 'C:\\Program Files\\Codex CLI\\codex.exe' " +
"OR path LIKE '%\\AppData\\Local\\Programs\\Codex CLI\\codex.exe') AND version_compare(file_version, '0.130.0') < 0);",
},
}
for _, tt := range tests {
Expand Down
Loading