Skip to content

Allow SQL expressions in array condition keys #266

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

Open
opengeek wants to merge 1 commit into
base: 3.x
Choose a base branch
from
Open

Allow SQL expressions in array condition keys #266

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 11 additions & 0 deletions src/xPDO/Om/mysql/xPDOQuery.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,4 +133,15 @@ public function construct() {
$this->sql= $sql;
return (!empty ($this->sql));
}

protected function isColumnIdentifier($key)
{
$unQuoted = '(?![\d]+(?:\.|$))[A-Za-z0-9$_\x80-\xff]{1,64}';
$escapeChars = '`(?:[^`\x00]|``){1,128}`';
$dblQuote = '"(?:[^"\x00]|""){1,128}"';
$ident = "(?:{$escapeChars}|{$dblQuote}|{$unQuoted})";
$columnIdent = "(?:{$ident}\.)?{$ident}";

return (bool) preg_match("/^{$columnIdent}$/", trim($key));
}
}
11 changes: 11 additions & 0 deletions src/xPDO/Om/pgsql/xPDOQuery.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,4 +124,15 @@ public function construct() {
$this->sql= $sql;
return (!empty ($this->sql));
}

protected function isColumnIdentifier($key)
{
$unQuoted = '[A-Za-z_\x80-\xff][A-Za-z0-9_$\x80-\xff]{0,62}';
$dblQuote = '"(?:[^"\x00]|""){1,126}"';
$unicode = 'U&"(?:[^"\x00]|""|\\\\[0-9A-Fa-f]{4}|\\\\\\+[0-9A-Fa-f]{6}){1,}"(?:\s*UESCAPE\s*\'[^0-9A-Fa-f+\x27\\\\]\s*\')?';
$ident = "(?:{$unicode}|{$dblQuote}|{$unQuoted})";
$columnIdent = "(?:{$ident}\.)?{$ident}";

return (bool) preg_match("/^{$columnIdent}$/", trim($key));
}
}
12 changes: 12 additions & 0 deletions src/xPDO/Om/sqlite/xPDOQuery.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,4 +133,16 @@ public function construct() {
$this->sql= $sql;
return (!empty ($this->sql));
}

protected function isColumnIdentifier($key)
{
$unQuoted = '[A-Za-z_][A-Za-z0-9_]*';
$backtick = '`(?:[^`\x00]|``){1,128}`';
$bracket = '\[[^\]\x00]+\]';
$dblQuote = '"(?:[^"\x00]|""){1,}"';
$ident = "(?:{$dblQuote}|{$backtick}|{$bracket}|{$unQuoted})";
$columnIdent = "(?:{$ident}\.)?{$ident}";

return (bool) preg_match("/^{$columnIdent}$/", trim($key));
}
}
11 changes: 11 additions & 0 deletions src/xPDO/Om/sqlsrv/xPDOQuery.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,4 +296,15 @@ public function construct() {
$this->sql= $sql;
return (!empty ($this->sql));
}

protected function isColumnIdentifier($key)
{
$unQuoted = '(?![\d]+(?:\.|$))[A-Za-z_@#\x80-\xff][A-Za-z0-9_@#$\x80-\xff]{0,127}';
$bracket = '\[(?:[^\]\x00\xff\xff]|\]\]){1,128}\]';
$dblQuote = '"(?:[^"\x00]|""){1,128}"';
$ident = "(?:{$bracket}|{$dblQuote}|{$unQuoted})";
$columnIdent = "(?:{$ident}\.)?{$ident}";

return (bool) preg_match("/^{$columnIdent}$/", trim($key));
}
}
1 change: 1 addition & 0 deletions src/xPDO/Om/xPDOCriteria.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
* @package xPDO\Om
*/
class xPDOCriteria {
/** @var xPDO $xpdo */
public $xpdo= null;
public $sql= '';
public $stmt= null;
Expand Down
75 changes: 54 additions & 21 deletions src/xPDO/Om/xPDOQuery.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,22 @@ abstract class xPDOQuery extends xPDOCriteria {
'MIN(',
'AVG('
);
protected $_quotable= array ('string', 'password', 'date', 'datetime', 'timestamp', 'time', 'json', 'array', 'float');
protected $_quotable = [
'string',
'password',
'date',
'datetime',
'timestamp',
'time',
'json',
'array',
'float',
'double',
'object',
'resource',
'unknown type',
'resource (closed)'
];
Comment on lines +69 to +84
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

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

The _quotable array now includes PHP types returned by gettype() ('double', 'object', 'resource', 'unknown type', 'resource (closed)'), but it previously contained database field types ('string', 'password', 'date', 'datetime', 'timestamp', 'time', 'json', 'array', 'float'). This mixed usage creates confusion: the array is used with phptype from field metadata (which are database types) at line 781-783, and with gettype() results (which are PHP types) at line 969. The 'float' entry could match both phptype 'float' and gettype 'double', but 'array' is a database type that won't be returned by gettype. Consider separating these into two distinct arrays or documenting which values correspond to which use case.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member Author

@opengeek opengeek Feb 25, 2026

Choose a reason for hiding this comment

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

Actually, they are pseudo-PHP types already, not direct representations of database field types, meant to bridge actual database types to a type represented in PHP. This was added so that conditions that do not directly represent a field in the database model can handle values appropriately in the resulting PDO prepared statement.

protected $_class= null;
protected $_alias= null;
protected $_tableClass = null;
Expand Down Expand Up @@ -737,30 +752,42 @@ public function parseConditions($conditions, $conjunction = xPDOQuery::SQL_AND)
$key= $key_operator[1];
$operator= strtoupper($key_operator[2]);
}
if (strpos($key, '.') !== false) {
$key_parts= explode('.', $key);
$alias= trim($key_parts[0], " {$this->xpdo->_escapeCharOpen}{$this->xpdo->_escapeCharClose}");
$key= $key_parts[1];
}
if (!array_key_exists($key, $fieldMeta)) {
if (array_key_exists($key, $fieldAliases)) {
$key= $fieldAliases[$key];
} elseif ($this->isConditionalClause($key)) {
continue;
$operand = $key;
$isColumnIdentifier = $this->isColumnIdentifier($key);
if ($isColumnIdentifier) {
if (strpos($key, '.') !== false) {
$key_parts = explode('.', $key);
$alias = trim(
$key_parts[0],
" {$this->xpdo->_escapeCharOpen}{$this->xpdo->_escapeCharClose}"
);
$key = $key_parts[1];
}
if (!array_key_exists($key, $fieldMeta)) {
if (array_key_exists($key, $fieldAliases)) {
$key = $fieldAliases[$key];
}
}
$operand = "{$this->xpdo->escape($alias)}.{$this->xpdo->escape($key)}";
}
if (!empty($key)) {
if (!empty($key) && !$this->isConditionalClause($key)) {
if ($val === null) {
$type= \PDO::PARAM_NULL;
if (!in_array($operator, array('IS', 'IS NOT'))) {
$operator= $operator === '!=' ? 'IS NOT' : 'IS';
}
}
elseif (isset($fieldMeta[$key]) && !in_array($fieldMeta[$key]['phptype'], $this->_quotable)) {
$type= \PDO::PARAM_INT;
}
else {
$type= \PDO::PARAM_STR;
elseif ($isColumnIdentifier) {
if (isset($fieldMeta[$key]) && !in_array(
$fieldMeta[$key]['phptype'],
$this->_quotable
)) {
$type = \PDO::PARAM_INT;
} else {
$type = \PDO::PARAM_STR;
}
} else {
$type = $this->isQuotable($val) ? \PDO::PARAM_STR : \PDO::PARAM_INT;
}
if (in_array($operator, array('IN', 'NOT IN')) && is_array($val)) {
$vals = array();
Expand All @@ -785,12 +812,12 @@ public function parseConditions($conditions, $conjunction = xPDOQuery::SQL_AND)
$this->xpdo->log(xPDO::LOG_LEVEL_ERROR, "Encountered empty {$operator} condition with key {$key}");
}
$val = "(" . implode(',', $vals) . ")";
$sql = "{$this->xpdo->escape($alias)}.{$this->xpdo->escape($key)} {$operator} {$val}";
$sql = "{$operand} {$operator} {$val}";
$result[]= new xPDOQueryCondition(array('sql' => $sql, 'binding' => null, 'conjunction' => $conj));
continue;
}
$field= array ();
$field['sql']= $this->xpdo->escape($alias) . '.' . $this->xpdo->escape($key) . ' ' . $operator . ' ?';
$field['sql']= $operand . ' ' . $operator . ' ?';
Comment on lines 755 to +820
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

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

In parseConditions, non-identifier condition keys are now used verbatim as the left-hand SQL operand via the operand variable (e.g. {$operand} {$operator} ...), with only the value bound as a PDO parameter. If any part of the conditions array key can be influenced by user input (for example, dynamic filter fields), an attacker can supply arbitrary SQL expressions or subqueries here (that do not contain UNION/;), leading to SQL injection despite parameter binding on the value. To avoid this, restrict/whitelist condition keys to safe column identifiers (those that pass isColumnIdentifier) and require any raw SQL expressions to go through a separate, clearly "unsafe" API that is never used with untrusted data.

Copilot uses AI. Check for mistakes.
$field['binding']= array (
'value' => $val,
'type' => $type,
Expand All @@ -808,8 +835,8 @@ public function parseConditions($conditions, $conjunction = xPDOQuery::SQL_AND)
elseif ($this->isConditionalClause($conditions)) {
$result= new xPDOQueryCondition(array(
'sql' => $conditions
,'binding' => null
,'conjunction' => $conjunction
,'binding' => null
,'conjunction' => $conjunction
));
}
elseif (($pktype == 'integer' && is_numeric($conditions)) || ($pktype == 'string' && is_string($conditions) && static::isValidClause($conditions))) {
Expand Down Expand Up @@ -937,4 +964,10 @@ public function __debugInfo()
'bindings' => $this->bindings,
];
}

protected function isQuotable($value) {
return in_array(gettype($value), $this->_quotable);
}

abstract protected function isColumnIdentifier($key);
}
1 change: 1 addition & 0 deletions test/complete.phpunit.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
<file>./xPDO/Test/Om/xPDOObjectTest.php</file>
<file>./xPDO/Test/Om/xPDOObjectSingleTableInheritanceTest.php</file>
<file>./xPDO/Test/Om/xPDOQueryTest.php</file>
<file>./xPDO/Test/Om/xPDOQueryConditionsTest.php</file>
<file>./xPDO/Test/Om/xPDOQueryHavingTest.php</file>
<file>./xPDO/Test/Om/xPDOQueryLimitTest.php</file>
<file>./xPDO/Test/Om/xPDOQuerySortByTest.php</file>
Expand Down
1 change: 1 addition & 0 deletions test/mysql.phpunit.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
<file>./xPDO/Test/Om/xPDOObjectTest.php</file>
<file>./xPDO/Test/Om/xPDOObjectSingleTableInheritanceTest.php</file>
<file>./xPDO/Test/Om/xPDOQueryTest.php</file>
<file>./xPDO/Test/Om/xPDOQueryConditionsTest.php</file>
<file>./xPDO/Test/Om/xPDOQueryHavingTest.php</file>
<file>./xPDO/Test/Om/xPDOQueryLimitTest.php</file>
<file>./xPDO/Test/Om/xPDOQuerySortByTest.php</file>
Expand Down
1 change: 1 addition & 0 deletions test/pgsql.phpunit.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
<file>./xPDO/Test/Om/xPDOObjectTest.php</file>
<file>./xPDO/Test/Om/xPDOObjectSingleTableInheritanceTest.php</file>
<file>./xPDO/Test/Om/xPDOQueryTest.php</file>
<file>./xPDO/Test/Om/xPDOQueryConditionsTest.php</file>
<file>./xPDO/Test/Om/xPDOQueryHavingTest.php</file>
<file>./xPDO/Test/Om/xPDOQueryLimitTest.php</file>
<file>./xPDO/Test/Om/xPDOQuerySortByTest.php</file>
Expand Down
1 change: 1 addition & 0 deletions test/sqlite.phpunit.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
<file>./xPDO/Test/Om/xPDOObjectTest.php</file>
<file>./xPDO/Test/Om/xPDOObjectSingleTableInheritanceTest.php</file>
<file>./xPDO/Test/Om/xPDOQueryTest.php</file>
<file>./xPDO/Test/Om/xPDOQueryConditionsTest.php</file>
<file>./xPDO/Test/Om/xPDOQueryHavingTest.php</file>
<file>./xPDO/Test/Om/xPDOQueryLimitTest.php</file>
<file>./xPDO/Test/Om/xPDOQuerySortByTest.php</file>
Expand Down
Loading