From 4d2640f58bffd0814a40fc547ac47c99d033eefb Mon Sep 17 00:00:00 2001 From: Markus Strehle <11627201+strehle@users.noreply.github.com> Date: Wed, 3 Jun 2026 17:01:56 +0200 Subject: [PATCH 01/11] Potential fix for code scanning alert no. 27: Query built from user-controlled sources Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> --- .../uaa/resources/jdbc/AbstractQueryable.java | 33 +++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java b/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java index f5a0b22523e..37d5d64330b 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java @@ -9,6 +9,7 @@ import org.springframework.util.StringUtils; import java.util.List; +import java.util.Locale; import java.util.Set; import java.util.stream.Collectors; @@ -86,10 +87,38 @@ public List query(String filter, String sortBy, boolean ascending, String zon } private String getQuerySQL(SearchQueryConverter.ProcessedFilter where) { + String sqlFragment = where.getSql(); + assertSafeGeneratedSql(sqlFragment); + if (where.hasOrderBy()) { - return getBaseSqlQuery() + " where (" + where.getSql().replace(ORDER_BY, ")" + ORDER_BY); + return getBaseSqlQuery() + " where (" + sqlFragment.replace(ORDER_BY, ")" + ORDER_BY); } else { - return getBaseSqlQuery() + " where (" + where.getSql() + ")"; + return getBaseSqlQuery() + " where (" + sqlFragment + ")"; + } + } + + private void assertSafeGeneratedSql(String sqlFragment) { + if (!StringUtils.hasText(sqlFragment)) { + throw new IllegalArgumentException("Invalid filter: empty SQL fragment"); + } + + String normalized = sqlFragment.toLowerCase(Locale.ROOT); + if (normalized.contains(";") + || normalized.contains("--") + || normalized.contains("/*") + || normalized.contains("*/")) { + throw new IllegalArgumentException("Invalid filter: disallowed SQL token in generated query"); + } + + String trimmed = normalized.trim(); + if (trimmed.startsWith("select ") + || trimmed.startsWith("insert ") + || trimmed.startsWith("update ") + || trimmed.startsWith("delete ") + || trimmed.startsWith("drop ") + || trimmed.startsWith("alter ") + || trimmed.startsWith("create ")) { + throw new IllegalArgumentException("Invalid filter: unexpected SQL clause in generated query"); } } From dba55c4bb49c4728ba51cd135b28ec1eee8b2e22 Mon Sep 17 00:00:00 2001 From: Markus Strehle <11627201+strehle@users.noreply.github.com> Date: Wed, 3 Jun 2026 18:06:54 +0200 Subject: [PATCH 02/11] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .../identity/uaa/resources/jdbc/AbstractQueryable.java | 1 + 1 file changed, 1 insertion(+) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java b/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java index 37d5d64330b..1566c725434 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java @@ -105,6 +105,7 @@ private void assertSafeGeneratedSql(String sqlFragment) { String normalized = sqlFragment.toLowerCase(Locale.ROOT); if (normalized.contains(";") || normalized.contains("--") + || normalized.contains("#") || normalized.contains("/*") || normalized.contains("*/")) { throw new IllegalArgumentException("Invalid filter: disallowed SQL token in generated query"); From 93467ac4cc9851fa306d91020c4cfa61ce4188cf Mon Sep 17 00:00:00 2001 From: strehle Date: Thu, 4 Jun 2026 01:01:39 +0200 Subject: [PATCH 03/11] Document SCIM filter SQL safety and suppress CodeQL alert #27 The SQL fragment passed into getQuerySQL is generated by SimpleSearchQueryConverter, which validates attribute names against a hardcoded allow-list and binds every user-supplied value as a named parameter via NamedParameterJdbcTemplate. assertSafeGeneratedSql remains as defense-in-depth. Add a justification comment and lgtm[java/sql-injection] suppressions at the concatenation sites to address CodeQL alert #27. --- .../identity/uaa/resources/jdbc/AbstractQueryable.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java b/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java index 1566c725434..dc414b195b8 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java @@ -87,13 +87,17 @@ public List query(String filter, String sortBy, boolean ascending, String zon } private String getQuerySQL(SearchQueryConverter.ProcessedFilter where) { + // The SQL fragment is produced by SimpleSearchQueryConverter, which validates attribute + // names against a hardcoded allow-list and binds every user-supplied value as a named + // parameter (see SimpleSearchQueryConverter#whereClauseFromFilter and #comparisonClause). + // assertSafeGeneratedSql adds a defense-in-depth check on the resulting fragment. String sqlFragment = where.getSql(); assertSafeGeneratedSql(sqlFragment); if (where.hasOrderBy()) { - return getBaseSqlQuery() + " where (" + sqlFragment.replace(ORDER_BY, ")" + ORDER_BY); + return getBaseSqlQuery() + " where (" + sqlFragment.replace(ORDER_BY, ")" + ORDER_BY); // lgtm[java/sql-injection] } else { - return getBaseSqlQuery() + " where (" + sqlFragment + ")"; + return getBaseSqlQuery() + " where (" + sqlFragment + ")"; // lgtm[java/sql-injection] } } From efc340288ca2226a349f96d3f55412747588e272 Mon Sep 17 00:00:00 2001 From: strehle Date: Thu, 4 Jun 2026 15:41:55 +0200 Subject: [PATCH 04/11] Apply assertSafeGeneratedSql to JdbcScimUserProvisioning#retrieveByScimFilterOnlyActive Address Copilot review on PR #3930: the previous validator only protected the inherited AbstractQueryable#query path. retrieveByScimFilterOnlyActive also concatenates ProcessedFilter#getSql() into a SQL string and was missing the same defense-in-depth check. - Promote assertSafeGeneratedSql from private to protected static so subclasses that build SQL directly can reuse the same guard. - Call it from JdbcScimUserProvisioning#retrieveByScimFilterOnlyActive before composing the where clause. --- .../identity/uaa/resources/jdbc/AbstractQueryable.java | 2 +- .../identity/uaa/scim/jdbc/JdbcScimUserProvisioning.java | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java b/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java index dc414b195b8..c20c44dbf8c 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java @@ -101,7 +101,7 @@ private String getQuerySQL(SearchQueryConverter.ProcessedFilter where) { } } - private void assertSafeGeneratedSql(String sqlFragment) { + protected static void assertSafeGeneratedSql(String sqlFragment) { if (!StringUtils.hasText(sqlFragment)) { throw new IllegalArgumentException("Invalid filter: empty SQL fragment"); } diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioning.java b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioning.java index d82b31e9774..db459b8d115 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioning.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioning.java @@ -200,6 +200,10 @@ public List retrieveByScimFilterOnlyActive( // build WHERE clause final ProcessedFilter where = joinConverter.convert(filter, sortBy, ascending, zoneId); final String whereClauseScimFilter = where.getSql(); + // The SQL fragment is produced by SimpleSearchQueryConverter (see AbstractQueryable#getQuerySQL + // for the full justification); assertSafeGeneratedSql adds a defense-in-depth check so this + // direct concatenation is protected by the same guard as the inherited query() path. + assertSafeGeneratedSql(whereClauseScimFilter); String whereClause = "idp.active is true and ("; if (where.hasOrderBy()) { whereClause += whereClauseScimFilter.replace(ProcessedFilter.ORDER_BY, ")" + ProcessedFilter.ORDER_BY); From 507316af378e4068d6bc0bdb9a713866687baed4 Mon Sep 17 00:00:00 2001 From: strehle Date: Thu, 4 Jun 2026 16:00:53 +0200 Subject: [PATCH 05/11] review from copilot --- .../uaa/resources/jdbc/AbstractQueryable.java | 16 +- .../jdbc/AbstractQueryableSqlGuardTests.java | 152 ++++++++++++++++++ 2 files changed, 161 insertions(+), 7 deletions(-) create mode 100644 server/src/test/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryableSqlGuardTests.java diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java b/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java index c20c44dbf8c..eca7e626b63 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java @@ -11,6 +11,7 @@ import java.util.List; import java.util.Locale; import java.util.Set; +import java.util.regex.Pattern; import java.util.stream.Collectors; import static com.google.common.primitives.Ints.tryParse; @@ -101,6 +102,13 @@ private String getQuerySQL(SearchQueryConverter.ProcessedFilter where) { } } + // Matches a leading DML/DDL keyword followed by any whitespace (including \t, \n) or + // a "(", which together cover the realistic ways such a keyword could be smuggled past + // a literal-space prefix check (e.g. "select\tfoo", "select(", "SELECT\nfoo"). + private static final Pattern DISALLOWED_LEADING_CLAUSE = Pattern.compile( + "^(select|insert|update|delete|drop|alter|create|truncate|merge|union|grant|revoke|exec|execute|call|with|replace|rename|comment)(\\s|\\().*", + Pattern.CASE_INSENSITIVE | Pattern.DOTALL); + protected static void assertSafeGeneratedSql(String sqlFragment) { if (!StringUtils.hasText(sqlFragment)) { throw new IllegalArgumentException("Invalid filter: empty SQL fragment"); @@ -116,13 +124,7 @@ protected static void assertSafeGeneratedSql(String sqlFragment) { } String trimmed = normalized.trim(); - if (trimmed.startsWith("select ") - || trimmed.startsWith("insert ") - || trimmed.startsWith("update ") - || trimmed.startsWith("delete ") - || trimmed.startsWith("drop ") - || trimmed.startsWith("alter ") - || trimmed.startsWith("create ")) { + if (DISALLOWED_LEADING_CLAUSE.matcher(trimmed).matches()) { throw new IllegalArgumentException("Invalid filter: unexpected SQL clause in generated query"); } } diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryableSqlGuardTests.java b/server/src/test/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryableSqlGuardTests.java new file mode 100644 index 00000000000..77d1f487c78 --- /dev/null +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryableSqlGuardTests.java @@ -0,0 +1,152 @@ +package org.cloudfoundry.identity.uaa.resources.jdbc; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; +import org.springframework.jdbc.core.RowMapper; +import org.springframework.jdbc.core.namedparam.NamedParameterJdbcTemplate; + +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +/** + * Focused tests for {@link AbstractQueryable#assertSafeGeneratedSql(String)} — the + * defense-in-depth guard added to address CodeQL alert #27 ("Query built from + * user-controlled sources"). These tests verify that unsafe constructs are rejected + * and that typical generated SCIM filter fragments are accepted, so the guard cannot + * be silently relaxed in the future. + */ +class AbstractQueryableSqlGuardTests { + + /** + * Tiny subclass that exposes the {@code protected static} guard for direct testing. + * No state, no behavior — just a hook to invoke the validator. + */ + private static final class GuardHarness extends AbstractQueryable { + private GuardHarness() { + super((NamedParameterJdbcTemplate) null, (JdbcPagingListFactory) null, (RowMapper) null); + } + + static void invoke(String sqlFragment) { + assertSafeGeneratedSql(sqlFragment); + } + + @Override + protected String getBaseSqlQuery() { + return ""; + } + + @Override + protected String getTableName() { + return ""; + } + + @Override + protected void validateOrderBy(String orderBy) { + // no-op + } + } + + // ---------- Empty / blank fragments ---------- + + @ParameterizedTest + @ValueSource(strings = {"", " ", "\t", "\n"}) + void rejectsEmptyOrBlankFragments(String fragment) { + assertThatThrownBy(() -> GuardHarness.invoke(fragment)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("empty SQL fragment"); + } + + @Test + void rejectsNullFragment() { + assertThatThrownBy(() -> GuardHarness.invoke(null)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("empty SQL fragment"); + } + + // ---------- Statement separators / comment tokens ---------- + + @ParameterizedTest + @ValueSource(strings = { + "username = :__value_0; drop table users", // statement separator + "username = :__value_0 -- and 1=1", // ANSI line comment + "username = :__value_0 # mysql line comment", // MySQL # line comment + "username = :__value_0 /* block comment */", // start of block comment + "username = :__value_0 */ trailing" // end of block comment + }) + void rejectsDisallowedTokens(String fragment) { + assertThatThrownBy(() -> GuardHarness.invoke(fragment)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("disallowed SQL token"); + } + + // ---------- Leading DML/DDL clauses (the keyword guard) ---------- + + @ParameterizedTest + @ValueSource(strings = { + "select * from users", + "SELECT * from users", + "insert into users values (1)", + "update users set x = 1", + "delete from users", + "drop table users", + "alter table users add column x", + "create table foo(x int)", + "truncate users", + "merge into users", + "union select 1", + "with x as (select 1) select * from x", + "grant all to public", + "revoke select on users", + "exec something", + "execute something", + "call proc()", + "replace into users", + "rename table users to bar", + "comment on column foo is 'x'" + }) + void rejectsLeadingDmlDdlKeywordWithSpace(String fragment) { + assertThatThrownBy(() -> GuardHarness.invoke(fragment)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("unexpected SQL clause"); + } + + /** + * Bypasses raised by Copilot review: the prior implementation only blocked + * "{keyword} " (literal space). These variants must also be rejected. + */ + @ParameterizedTest + @ValueSource(strings = { + "SELECT\tfoo", // tab + "SELECT\nfoo", // newline + "SELECT\rfoo", // carriage return + "SELECT\ffoo", // form feed + "select(1)", // open paren, no whitespace + " select foo", // leading whitespace before keyword + "UPDATE\tusers", // mixed-case + tab + "Drop(table)" // mixed-case + paren + }) + void rejectsKeywordFollowedByWhitespaceVariantsOrParen(String fragment) { + assertThatThrownBy(() -> GuardHarness.invoke(fragment)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("unexpected SQL clause"); + } + + // ---------- Typical generated SCIM filter fragments are accepted ---------- + + @ParameterizedTest + @ValueSource(strings = { + // shapes produced by SimpleSearchQueryConverter — values are bound parameters + "username = :__value_0", + "username = :__value_0 and origin = :__value_1", + "(username = :__value_0 or origin = :__value_1) and active = :__value_2", + "lower(username) = lower(:__value_0)", + "username = :__value_0 order by created desc", + // an attribute that happens to start with a keyword-prefix (must not be rejected) + "selectable = :__value_0", + "createdby = :__value_0" + }) + void acceptsTypicalGeneratedFragments(String fragment) { + assertThatCode(() -> GuardHarness.invoke(fragment)).doesNotThrowAnyException(); + } +} From 4906cede7d61b3a431551aa790f77e62b96ceed4 Mon Sep 17 00:00:00 2001 From: strehle Date: Thu, 4 Jun 2026 16:24:29 +0200 Subject: [PATCH 06/11] review from copilot --- .../identity/uaa/resources/jdbc/AbstractQueryable.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java b/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java index eca7e626b63..5e03491d450 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java @@ -96,9 +96,9 @@ private String getQuerySQL(SearchQueryConverter.ProcessedFilter where) { assertSafeGeneratedSql(sqlFragment); if (where.hasOrderBy()) { - return getBaseSqlQuery() + " where (" + sqlFragment.replace(ORDER_BY, ")" + ORDER_BY); // lgtm[java/sql-injection] + return getBaseSqlQuery() + " where (" + sqlFragment.replace(ORDER_BY, ")" + ORDER_BY); } else { - return getBaseSqlQuery() + " where (" + sqlFragment + ")"; // lgtm[java/sql-injection] + return getBaseSqlQuery() + " where (" + sqlFragment + ")"; } } From bc48fe346e33452e04a994d52e0eaf6f1d6952ed Mon Sep 17 00:00:00 2001 From: strehle Date: Thu, 4 Jun 2026 17:16:58 +0200 Subject: [PATCH 07/11] review from copilot --- .../uaa/resources/jdbc/AbstractQueryable.java | 6 ++++- .../jdbc/AbstractQueryableSqlGuardTests.java | 24 +++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java b/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java index 5e03491d450..dfe5333a190 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java @@ -109,6 +109,10 @@ private String getQuerySQL(SearchQueryConverter.ProcessedFilter where) { "^(select|insert|update|delete|drop|alter|create|truncate|merge|union|grant|revoke|exec|execute|call|with|replace|rename|comment)(\\s|\\().*", Pattern.CASE_INSENSITIVE | Pattern.DOTALL); + // Strips leading whitespace and any number of leading "(" so the keyword guard above + // also catches parenthesis-wrapped bypasses such as "(select 1)" or "((select 1))". + private static final Pattern LEADING_PARENS_OR_WHITESPACE = Pattern.compile("^[\\s(]+"); + protected static void assertSafeGeneratedSql(String sqlFragment) { if (!StringUtils.hasText(sqlFragment)) { throw new IllegalArgumentException("Invalid filter: empty SQL fragment"); @@ -123,7 +127,7 @@ protected static void assertSafeGeneratedSql(String sqlFragment) { throw new IllegalArgumentException("Invalid filter: disallowed SQL token in generated query"); } - String trimmed = normalized.trim(); + String trimmed = LEADING_PARENS_OR_WHITESPACE.matcher(normalized).replaceFirst(""); if (DISALLOWED_LEADING_CLAUSE.matcher(trimmed).matches()) { throw new IllegalArgumentException("Invalid filter: unexpected SQL clause in generated query"); } diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryableSqlGuardTests.java b/server/src/test/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryableSqlGuardTests.java index 77d1f487c78..d12360dd481 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryableSqlGuardTests.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryableSqlGuardTests.java @@ -132,6 +132,30 @@ void rejectsKeywordFollowedByWhitespaceVariantsOrParen(String fragment) { .hasMessageContaining("unexpected SQL clause"); } + /** + * Parenthesis-wrapped bypass raised by Copilot review: a fragment like + * "(select 1)" or "((select 1))" must be rejected — leading whitespace and + * "(" are stripped before the DML/DDL keyword check. + */ + @ParameterizedTest + @ValueSource(strings = { + "(select 1)", + "((select 1))", + "( select 1 )", + "(\tselect 1)", + "( ( select 1 ) )", + " (select 1) ", + "((((delete from users))))", + "(UPDATE users set x=1)", + "(DROP table users)", + "(union select 1)" + }) + void rejectsParenthesisWrappedKeyword(String fragment) { + assertThatThrownBy(() -> GuardHarness.invoke(fragment)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("unexpected SQL clause"); + } + // ---------- Typical generated SCIM filter fragments are accepted ---------- @ParameterizedTest From 0ad9cf60f3ea3d930ee76cb6d6f77d4fafafc113 Mon Sep 17 00:00:00 2001 From: strehle Date: Thu, 4 Jun 2026 17:37:29 +0200 Subject: [PATCH 08/11] review from copilot --- .../uaa/resources/jdbc/AbstractQueryable.java | 9 ++++++++ .../jdbc/AbstractQueryableSqlGuardTests.java | 22 +++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java b/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java index dfe5333a190..4bc759d8bb6 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java @@ -127,6 +127,15 @@ protected static void assertSafeGeneratedSql(String sqlFragment) { throw new IllegalArgumentException("Invalid filter: disallowed SQL token in generated query"); } + // The caller wraps the fragment as `where (` + fragment + `)`. A fragment whose + // first non-whitespace character is `)` would break out of that wrapper (e.g. + // `) or 1=1 or (`) and change the surrounding query logic without using any of + // the tokens above. Reject it explicitly. + String leadingTrimmed = normalized.stripLeading(); + if (leadingTrimmed.startsWith(")")) { + throw new IllegalArgumentException("Invalid filter: unexpected leading token in generated query"); + } + String trimmed = LEADING_PARENS_OR_WHITESPACE.matcher(normalized).replaceFirst(""); if (DISALLOWED_LEADING_CLAUSE.matcher(trimmed).matches()) { throw new IllegalArgumentException("Invalid filter: unexpected SQL clause in generated query"); diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryableSqlGuardTests.java b/server/src/test/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryableSqlGuardTests.java index d12360dd481..feada607478 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryableSqlGuardTests.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryableSqlGuardTests.java @@ -156,6 +156,28 @@ void rejectsParenthesisWrappedKeyword(String fragment) { .hasMessageContaining("unexpected SQL clause"); } + /** + * Wrapper-breakout bypass raised by Copilot review: the caller composes + * {@code "where (" + fragment + ")"}, so a fragment whose first non-whitespace + * character is {@code ")"} would close the wrapper and inject arbitrary + * boolean logic without using {@code ;}, comments, or DML/DDL keywords. + */ + @ParameterizedTest + @ValueSource(strings = { + ") or 1=1 or (", + ") OR 1=1 OR (", + ")", + " ) or 'a'='a' or (", + "\t) or 1=1 or (", + "\n) or 1=1 or (", + "))) or 1=1 or (((" + }) + void rejectsLeadingCloseParenWrapperBreakout(String fragment) { + assertThatThrownBy(() -> GuardHarness.invoke(fragment)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("unexpected leading token"); + } + // ---------- Typical generated SCIM filter fragments are accepted ---------- @ParameterizedTest From 5b4382a153274bc5a9af4fdd7b3d95635ab50549 Mon Sep 17 00:00:00 2001 From: strehle Date: Thu, 4 Jun 2026 18:07:32 +0200 Subject: [PATCH 09/11] review from copilot --- .../uaa/resources/jdbc/AbstractQueryable.java | 7 ++++++ .../jdbc/AbstractQueryableSqlGuardTests.java | 24 +++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java b/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java index 4bc759d8bb6..bbbf4b00520 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java @@ -137,6 +137,13 @@ protected static void assertSafeGeneratedSql(String sqlFragment) { } String trimmed = LEADING_PARENS_OR_WHITESPACE.matcher(normalized).replaceFirst(""); + // Also reject the extended wrapper-breakout where one or more leading `(` are + // immediately followed by `)` (e.g. `"()) or 1=1 or (1=1"` or `"(()) or ..."`): + // after stripping leading whitespace and `(`, the next non-whitespace token is `)`, + // which closes the caller's `where (` wrapper one paren-level too early. + if (trimmed.startsWith(")")) { + throw new IllegalArgumentException("Invalid filter: unexpected leading token in generated query"); + } if (DISALLOWED_LEADING_CLAUSE.matcher(trimmed).matches()) { throw new IllegalArgumentException("Invalid filter: unexpected SQL clause in generated query"); } diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryableSqlGuardTests.java b/server/src/test/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryableSqlGuardTests.java index feada607478..96ea752c804 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryableSqlGuardTests.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryableSqlGuardTests.java @@ -178,6 +178,30 @@ void rejectsLeadingCloseParenWrapperBreakout(String fragment) { .hasMessageContaining("unexpected leading token"); } + /** + * Extended wrapper-breakout bypass raised by Copilot review: a fragment can also + * close the surrounding {@code "where ("} wrapper early by starting with one or + * more {@code "("} immediately followed by {@code ")"} (e.g. {@code "()) or 1=1 or (1=1"}). + * After stripping leading whitespace and any leading {@code "("}, the next + * non-whitespace token must not be {@code ")"}. + */ + @ParameterizedTest + @ValueSource(strings = { + "()) or 1=1 or (1=1", + "(()) or 1=1 or ((1=1", + "((())) or 1=1", + "( )) or 1=1 or (1=1", // whitespace between ( and ) + "(\t)) or 1=1 or (1=1", // tab between ( and ) + "(\n)) or 1=1 or (1=1", // newline between ( and ) + " ()) or 1=1 or (1=1", // leading whitespace before ( + "())" // bare ()) — closes wrapper twice + }) + void rejectsParenWrappedCloseParenBreakout(String fragment) { + assertThatThrownBy(() -> GuardHarness.invoke(fragment)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("unexpected leading token"); + } + // ---------- Typical generated SCIM filter fragments are accepted ---------- @ParameterizedTest From 699d216d0f567884f3bfb5b8cacd8fb8cd4bfc56 Mon Sep 17 00:00:00 2001 From: strehle Date: Thu, 4 Jun 2026 18:15:42 +0200 Subject: [PATCH 10/11] review from copilot --- .../uaa/resources/jdbc/AbstractQueryableSqlGuardTests.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryableSqlGuardTests.java b/server/src/test/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryableSqlGuardTests.java index 96ea752c804..72cef016609 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryableSqlGuardTests.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryableSqlGuardTests.java @@ -194,7 +194,10 @@ void rejectsLeadingCloseParenWrapperBreakout(String fragment) { "(\t)) or 1=1 or (1=1", // tab between ( and ) "(\n)) or 1=1 or (1=1", // newline between ( and ) " ()) or 1=1 or (1=1", // leading whitespace before ( - "())" // bare ()) — closes wrapper twice + "())", // bare ()) — closes wrapper twice + // Verbatim variants from the Copilot review + "()) or 1=1 or (", + "( ) or 1=1 or (" }) void rejectsParenWrappedCloseParenBreakout(String fragment) { assertThatThrownBy(() -> GuardHarness.invoke(fragment)) From 112326b28d22c9c9ad3cbd46ee2913e8b8f09af9 Mon Sep 17 00:00:00 2001 From: strehle Date: Thu, 4 Jun 2026 18:26:23 +0200 Subject: [PATCH 11/11] review from copilot --- .../uaa/resources/jdbc/AbstractQueryable.java | 11 +++++++---- .../uaa/scim/jdbc/JdbcScimUserProvisioning.java | 7 ++++--- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java b/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java index bbbf4b00520..7acbc6a4650 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java @@ -88,10 +88,13 @@ public List query(String filter, String sortBy, boolean ascending, String zon } private String getQuerySQL(SearchQueryConverter.ProcessedFilter where) { - // The SQL fragment is produced by SimpleSearchQueryConverter, which validates attribute - // names against a hardcoded allow-list and binds every user-supplied value as a named - // parameter (see SimpleSearchQueryConverter#whereClauseFromFilter and #comparisonClause). - // assertSafeGeneratedSql adds a defense-in-depth check on the resulting fragment. + // The SQL fragment comes from the configured SearchQueryConverter (default + // SimpleSearchQueryConverter; replaceable via setQueryConverter). Every shipped + // implementation validates attribute names against a hardcoded allow-list and + // binds every user-supplied value as a named parameter (see e.g. + // SimpleSearchQueryConverter#whereClauseFromFilter and #comparisonClause). + // assertSafeGeneratedSql adds a defense-in-depth check on the resulting fragment + // so any future converter that violates that contract still cannot inject SQL. String sqlFragment = where.getSql(); assertSafeGeneratedSql(sqlFragment); diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioning.java b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioning.java index db459b8d115..02e7b392fef 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioning.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioning.java @@ -200,9 +200,10 @@ public List retrieveByScimFilterOnlyActive( // build WHERE clause final ProcessedFilter where = joinConverter.convert(filter, sortBy, ascending, zoneId); final String whereClauseScimFilter = where.getSql(); - // The SQL fragment is produced by SimpleSearchQueryConverter (see AbstractQueryable#getQuerySQL - // for the full justification); assertSafeGeneratedSql adds a defense-in-depth check so this - // direct concatenation is protected by the same guard as the inherited query() path. + // The SQL fragment comes from the configured SearchQueryConverter (here joinConverter, + // see AbstractQueryable#getQuerySQL for the full converter-contract justification); + // assertSafeGeneratedSql adds a defense-in-depth check so this direct concatenation + // is protected by the same guard as the inherited query() path. assertSafeGeneratedSql(whereClauseScimFilter); String whereClause = "idp.active is true and ("; if (where.hasOrderBy()) {