Potential fix for code scanning alert no. 27: Query built from user-controlled sources

server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java

@@ -9,7 +9,9 @@
 import org.springframework.util.StringUtils;
 
 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;
@@ -86,10 +88,67 @@ public List<T> query(String filter, String sortBy, boolean ascending, String zon
     }
 
     private String getQuerySQL(SearchQueryConverter.ProcessedFilter where) {
+        // 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);
+
         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 + ")";
+        }
+    }
+
+    // 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);
+
+    // 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");
+        }
+
+        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");
+        }
+
+        // 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("");
+        // 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");
         }
     }
 

server/src/main/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioning.java

@@ -200,6 +200,11 @@ public List<ScimUser> retrieveByScimFilterOnlyActive(
         // build WHERE clause
         final ProcessedFilter where = joinConverter.convert(filter, sortBy, ascending, zoneId);
         final String whereClauseScimFilter = where.getSql();
+        // 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()) {
             whereClause += whereClauseScimFilter.replace(ProcessedFilter.ORDER_BY, ")" + ProcessedFilter.ORDER_BY);

server/src/test/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryableSqlGuardTests.java

@@ -0,0 +1,225 @@
+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<Object> {
+        private GuardHarness() {
+            super((NamedParameterJdbcTemplate) null, (JdbcPagingListFactory) null, (RowMapper<Object>) 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");
+    }
+
+    /**
+     * 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");
+    }
+
+    /**
+     * 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");
+    }
+
+    /**
+     * 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
+            // Verbatim variants from the Copilot review
+            "()) or 1=1 or (",
+            "( ) or 1=1 or ("
+    })
+    void rejectsParenWrappedCloseParenBreakout(String fragment) {
+        assertThatThrownBy(() -> GuardHarness.invoke(fragment))
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasMessageContaining("unexpected leading token");
+    }
+
+    // ---------- 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();
+    }
+}