Skip to content

fix(Om): fix updateCollection/set() when string contains SQL operators#271

Closed
Ibochkarev wants to merge 1 commit intomodxcms:3.xfrom
Ibochkarev:fix/54-update-collection-set
Closed

fix(Om): fix updateCollection/set() when string contains SQL operators#271
Ibochkarev wants to merge 1 commit intomodxcms:3.xfrom
Ibochkarev:fix/54-update-collection-set

Conversation

@Ibochkarev
Copy link
Copy Markdown
Contributor

Fix updateCollection/set() when string value contains SQL operator keywords (e.g. IN, LIKE)

Previously, xPDOQuery::set() used isConditionalClause() to detect raw SQL expressions. String values containing SQL keywords like "IN", "LIKE", "BETWEEN", or "=" were incorrectly treated as raw SQL and not quoted, causing malformed queries and potential SQL injection.

Changes:

  • Remove isConditionalClause check in set() — string fields are now always treated as PARAM_STR and properly quoted
  • Simplify duplicate condition: replace redundant elseif (in_array(...)) with else
  • Add null type handling in driver-specific construct() for robustness
  • Add test cases: LIKE, BETWEEN, =, empty string, apostrophe (O'Brien)
  • Add descriptive names to providerUpdateCollection data sets for clearer failure messages

Security: Proper quoting via quote() reduces SQL injection risk. isConditionalClause remains used in where() and other places where SQL expressions are expected.

Fixes #54

- Remove isConditionalClause check in set() that incorrectly treated string
  values with SQL keywords (IN, LIKE, BETWEEN, =) as raw SQL expressions
- String fields are now always quoted via PARAM_STR, preventing both
  incorrect query construction and SQL injection
- Add null type handling in driver-specific construct() for robustness
- Add test cases for LIKE, BETWEEN, =, empty string, and apostrophe
- Add descriptive names to providerUpdateCollection data sets

Fixes modxcms#54
@Ibochkarev Ibochkarev marked this pull request as ready for review March 11, 2026 07:10
@opengeek
Copy link
Copy Markdown
Member

opengeek commented Apr 9, 2026

@Ibochkarev — thanks for digging into this and putting together a fix with test cases. The work you did helped bring attention to and sharpen our understanding of the problem.

This PR is being closed because the issue was patched through PR #280, which fixed the set() quoting bug and also introduced xPDOExpression as a formal way to pass raw SQL expressions through the query layer. The approaches overlap significantly, and #280 was the path we took to resolve it.

Your contribution was not wasted. Surfacing this issue and exploring the solution helped greatly. Thank you again.

@opengeek opengeek closed this Apr 9, 2026
@Ibochkarev Ibochkarev deleted the fix/54-update-collection-set branch April 10, 2026 01:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

updateCollection fails when 'set' string contains operator

2 participants