Potential fix for code scanning alert no. 27: Query built from user-controlled sources#3930
Open
strehle wants to merge 12 commits into
Open
Potential fix for code scanning alert no. 27: Query built from user-controlled sources#3930strehle wants to merge 12 commits into
strehle wants to merge 12 commits into
Conversation
…ontrolled sources Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
cf-foundation-community-automation
Bot
moved this to Inbox
in Foundational Infrastructure Working Group
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to mitigate code scanning alert #27 (“Query built from user-controlled sources”) by adding a defensive validation step for the dynamically generated SQL fragment (where.getSql()) before composing and executing the final query in AbstractQueryable.
Changes:
- Add a centralized validation step in
getQuerySQL(...)to reject suspicious SQL fragment tokens before query execution. - Introduce
assertSafeGeneratedSql(...)to block statement separators/comment tokens and reject fragments that begin with unexpected SQL clauses.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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.
…imFilterOnlyActive 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.
…fix-for-SQL-validation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Potential fix for https://github.com/cloudfoundry/uaa/security/code-scanning/27
General fix approach: keep using named parameters for values, but ensure any dynamically generated SQL fragment is validated against a strict allowlist/pattern before execution. In this code path,
completeSqlis unavoidable dynamic SQL due to SCIM filter translation, so the best practical fix is to reject suspicious tokens/separators and only permit a conservative SQL subset for generated WHERE/ORDER BY clauses.Best single fix here without changing functionality: in
AbstractQueryable, validate the generated fragment fromwhere.getSql()before composingcompleteSql. Add a private method that rejects dangerous constructs (;, SQL comments, block comments) and enforces expected clause shape (must not start with top-level DML/DDL keywords). Call this validator insidegetQuerySQL(...)so both paging and non-paging executions are protected in one place. This avoids changing endpoint/controller behavior and avoids changing converter interfaces.Files/regions to change:
server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.javaLocaleimport.getQuerySQL(...)to call a newassertSafeGeneratedSql(...).assertSafeGeneratedSql(...)private method neargetQuerySQL(...).Suggested fixes powered by Copilot Autofix. Review carefully before merging.