Skip to content

Commit 3b5ee79

Browse files
lohanidamodarlohanidamodar
committed
Fix compatibility and exception swallowing
1 parent 295b282 commit 3b5ee79

File tree

2 files changed

+131
-36
lines changed

2 files changed

+131
-36
lines changed

‎src/Audit/Adapter/ClickHouse.php‎

Lines changed: 125 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -616,7 +616,9 @@ public function setup(): void
616616
$indexName = $index['$id'];
617617
/** @var array<string> $attributes */
618618
$attributes = $index['attributes'];
619-
$attributeList = implode(', ', $attributes);
619+
// Escape each attribute name to prevent SQL injection
620+
$escapedAttributes = array_map(fn ($attr) => $this->escapeIdentifier($attr), $attributes);
621+
$attributeList = implode(', ', $escapedAttributes);
620622
$indexes[] = "INDEX {$indexName} ({$attributeList}) TYPE bloom_filter GRANULARITY 1";
621623
}
622624

@@ -657,6 +659,51 @@ private function getColumnNames(): array
657659
return $columns;
658660
}
659661

662+
/**
663+
* Validate that an attribute name exists in the schema.
664+
* Prevents SQL injection by ensuring only valid column names are used.
665+
*
666+
* @param string $attributeName The attribute name to validate
667+
* @return bool True if valid
668+
* @throws Exception If attribute name is invalid
669+
*/
670+
private function validateAttributeName(string $attributeName): bool
671+
{
672+
// Special case: 'id' is always valid
673+
if ($attributeName === 'id') {
674+
return true;
675+
}
676+
677+
// Check if tenant is valid (only when sharedTables is enabled)
678+
if ($attributeName === 'tenant' && $this->sharedTables) {
679+
return true;
680+
}
681+
682+
// Check against defined attributes
683+
foreach ($this->getAttributes() as $attribute) {
684+
if ($attribute['$id'] === $attributeName) {
685+
return true;
686+
}
687+
}
688+
689+
throw new Exception("Invalid attribute name: {$attributeName}");
690+
}
691+
692+
/**
693+
* Format datetime values for ClickHouse parameter binding.
694+
* Removes timezone suffixes which are incompatible with DateTime64 type comparisons.
695+
*
696+
* @param mixed $value The value to format
697+
* @return string Formatted string without timezone suffix
698+
*/
699+
private function formatDateTimeParam(mixed $value): string
700+
{
701+
$strValue = $this->formatParamValue($value);
702+
// Remove timezone suffix if present (e.g., +00:00, -05:00)
703+
return preg_replace('/[+\\-]\\d{2}:\\d{2}$/', '', $strValue) ?? $strValue;
704+
}
705+
706+
660707
/**
661708
* Format datetime for ClickHouse compatibility.
662709
* Converts datetime to 'YYYY-MM-DD HH:MM:SS.mmm' format without timezone suffix.
@@ -789,11 +836,12 @@ public function getById(string $id): ?Log
789836
$tableName = $this->getTableName();
790837
$tenantFilter = $this->getTenantFilter();
791838
$escapedTable = $this->escapeIdentifier($this->database) . '.' . $this->escapeIdentifier($tableName);
839+
$escapedId = $this->escapeIdentifier('id');
792840

793841
$sql = "
794842
SELECT " . $this->getSelectColumns() . "
795843
FROM {$escapedTable}
796-
WHERE id = {id:String}{$tenantFilter}
844+
WHERE {$escapedId} = {id:String}{$tenantFilter}
797845
LIMIT 1
798846
FORMAT TabSeparated
799847
";
@@ -922,47 +970,70 @@ private function parseQueries(array $queries): array
922970

923971
switch ($method) {
924972
case Query::TYPE_EQUAL:
973+
$this->validateAttributeName($attribute);
974+
$escapedAttr = $this->escapeIdentifier($attribute);
925975
$paramName = 'param_' . $paramCounter++;
926-
$filters[] = "{$attribute} = {{$paramName}:String}";
976+
$filters[] = "{$escapedAttr} = {{$paramName}:String}";
927977
$params[$paramName] = $this->formatParamValue($values[0]);
928978
break;
929979

930980
case Query::TYPE_LESSER:
981+
$this->validateAttributeName($attribute);
982+
$escapedAttr = $this->escapeIdentifier($attribute);
931983
$paramName = 'param_' . $paramCounter++;
932-
$filters[] = "{$attribute} < {{$paramName}:String}";
984+
$filters[] = "{$escapedAttr} < {{$paramName}:String}";
933985
$params[$paramName] = $this->formatParamValue($values[0]);
934986
break;
935987

936988
case Query::TYPE_GREATER:
989+
$this->validateAttributeName($attribute);
990+
$escapedAttr = $this->escapeIdentifier($attribute);
937991
$paramName = 'param_' . $paramCounter++;
938-
$filters[] = "{$attribute} > {{$paramName}:String}";
992+
$filters[] = "{$escapedAttr} > {{$paramName}:String}";
939993
$params[$paramName] = $this->formatParamValue($values[0]);
940994
break;
941995

942996
case Query::TYPE_BETWEEN:
997+
$this->validateAttributeName($attribute);
998+
$escapedAttr = $this->escapeIdentifier($attribute);
943999
$paramName1 = 'param_' . $paramCounter++;
9441000
$paramName2 = 'param_' . $paramCounter++;
945-
$filters[] = "{$attribute} BETWEEN {{$paramName1}:String} AND {{$paramName2}:String}";
946-
$params[$paramName1] = $this->formatParamValue($values[0]);
947-
$params[$paramName2] = $this->formatParamValue($values[1]);
1001+
// Use DateTime64 type for time column, String for others
1002+
// This prevents type mismatch when comparing DateTime64 with timezone-suffixed strings
1003+
if ($attribute === 'time') {
1004+
$paramType = 'DateTime64(3)';
1005+
$filters[] = "{$escapedAttr} BETWEEN {{$paramName1}:{$paramType}} AND {{$paramName2}:{$paramType}}";
1006+
$params[$paramName1] = $this->formatDateTimeParam($values[0]);
1007+
$params[$paramName2] = $this->formatDateTimeParam($values[1]);
1008+
} else {
1009+
$filters[] = "{$escapedAttr} BETWEEN {{$paramName1}:String} AND {{$paramName2}:String}";
1010+
$params[$paramName1] = $this->formatParamValue($values[0]);
1011+
$params[$paramName2] = $this->formatParamValue($values[1]);
1012+
}
9481013
break;
9491014

9501015
case Query::TYPE_IN:
1016+
$this->validateAttributeName($attribute);
1017+
$escapedAttr = $this->escapeIdentifier($attribute);
9511018
$inParams = [];
9521019
foreach ($values as $value) {
9531020
$paramName = 'param_' . $paramCounter++;
9541021
$inParams[] = "{{$paramName}:String}";
9551022
$params[$paramName] = $this->formatParamValue($value);
9561023
}
957-
$filters[] = "{$attribute} IN (" . implode(', ', $inParams) . ")";
1024+
$filters[] = "{$escapedAttr} IN (" . implode(', ', $inParams) . ")";
9581025
break;
9591026

9601027
case Query::TYPE_ORDER_DESC:
961-
$orderBy[] = "{$attribute} DESC";
1028+
$this->validateAttributeName($attribute);
1029+
$escapedAttr = $this->escapeIdentifier($attribute);
1030+
$orderBy[] = "{$escapedAttr} DESC";
9621031
break;
9631032

9641033
case Query::TYPE_ORDER_ASC:
965-
$orderBy[] = "{$attribute} ASC";
1034+
$this->validateAttributeName($attribute);
1035+
$escapedAttr = $this->escapeIdentifier($attribute);
1036+
$orderBy[] = "{$escapedAttr} ASC";
9661037
break;
9671038

9681039
case Query::TYPE_LIMIT:
@@ -1194,20 +1265,34 @@ private function parseResults(string $result): array
11941265

11951266
/**
11961267
* Get the SELECT column list for queries.
1197-
* Returns 9 columns if not using shared tables, 10 if using shared tables.
1268+
* Escapes all column names to prevent SQL injection.
11981269
*
11991270
* @return string
12001271
*/
12011272
private function getSelectColumns(): string
12021273
{
1274+
$columns = [
1275+
$this->escapeIdentifier('id'),
1276+
$this->escapeIdentifier('userId'),
1277+
$this->escapeIdentifier('event'),
1278+
$this->escapeIdentifier('resource'),
1279+
$this->escapeIdentifier('userAgent'),
1280+
$this->escapeIdentifier('ip'),
1281+
$this->escapeIdentifier('location'),
1282+
$this->escapeIdentifier('time'),
1283+
$this->escapeIdentifier('data'),
1284+
];
1285+
12031286
if ($this->sharedTables) {
1204-
return 'id, userId, event, resource, userAgent, ip, location, time, data, tenant';
1287+
$columns[] = $this->escapeIdentifier('tenant');
12051288
}
1206-
return 'id, userId, event, resource, userAgent, ip, location, time, data';
1289+
1290+
return implode(', ', $columns);
12071291
}
12081292

12091293
/**
12101294
* Build tenant filter clause based on current tenant context.
1295+
* Escapes column name to prevent SQL injection.
12111296
*
12121297
* @return string
12131298
*/
@@ -1217,11 +1302,13 @@ private function getTenantFilter(): string
12171302
return '';
12181303
}
12191304

1220-
return " AND tenant = {$this->tenant}";
1305+
$escapedTenant = $this->escapeIdentifier('tenant');
1306+
return " AND {$escapedTenant} = {$this->tenant}";
12211307
}
12221308

12231309
/**
12241310
* Build time WHERE clause and parameters with safe parameter placeholders.
1311+
* Escapes column name to prevent SQL injection.
12251312
*
12261313
* @param \DateTime|null $after
12271314
* @param \DateTime|null $before
@@ -1245,21 +1332,23 @@ private function buildTimeClause(?\DateTime $after, ?\DateTime $before): array
12451332
$beforeStr = \Utopia\Database\DateTime::format($before);
12461333
}
12471334

1335+
$escapedTime = $this->escapeIdentifier('time');
1336+
12481337
if ($afterStr !== null && $beforeStr !== null) {
1249-
$conditions[] = 'time BETWEEN {after:String} AND {before:String}';
1338+
$conditions[] = "{$escapedTime} BETWEEN {after:String} AND {before:String}";
12501339
$params['after'] = $afterStr;
12511340
$params['before'] = $beforeStr;
12521341

12531342
return ['clause' => ' AND ' . $conditions[0], 'params' => $params];
12541343
}
12551344

12561345
if ($afterStr !== null) {
1257-
$conditions[] = 'time > {after:String}';
1346+
$conditions[] = "{$escapedTime} > {after:String}";
12581347
$params['after'] = $afterStr;
12591348
}
12601349

12611350
if ($beforeStr !== null) {
1262-
$conditions[] = 'time < {before:String}';
1351+
$conditions[] = "{$escapedTime} < {before:String}";
12631352
$params['before'] = $beforeStr;
12641353
}
12651354

@@ -1344,12 +1433,14 @@ public function getByUser(
13441433
$tableName = $this->getTableName();
13451434
$tenantFilter = $this->getTenantFilter();
13461435
$escapedTable = $this->escapeIdentifier($this->database) . '.' . $this->escapeIdentifier($tableName);
1436+
$escapedUserId = $this->escapeIdentifier('userId');
1437+
$escapedTime = $this->escapeIdentifier('time');
13471438

13481439
$sql = "
13491440
SELECT " . $this->getSelectColumns() . "
13501441
FROM {$escapedTable}
1351-
WHERE userId = {userId:String}{$tenantFilter}{$time['clause']}
1352-
ORDER BY time {$order}
1442+
WHERE {$escapedUserId} = {userId:String}{$tenantFilter}{$time['clause']}
1443+
ORDER BY {$escapedTime} {$order}
13531444
LIMIT {limit:UInt64} OFFSET {offset:UInt64}
13541445
FORMAT TabSeparated
13551446
";
@@ -1378,11 +1469,12 @@ public function countByUser(
13781469
$tableName = $this->getTableName();
13791470
$tenantFilter = $this->getTenantFilter();
13801471
$escapedTable = $this->escapeIdentifier($this->database) . '.' . $this->escapeIdentifier($tableName);
1472+
$escapedUserId = $this->escapeIdentifier('userId');
13811473

13821474
$sql = "
13831475
SELECT count()
13841476
FROM {$escapedTable}
1385-
WHERE userId = {userId:String}{$tenantFilter}{$time['clause']}
1477+
WHERE {$escapedUserId} = {userId:String}{$tenantFilter}{$time['clause']}
13861478
FORMAT TabSeparated
13871479
";
13881480

@@ -1412,12 +1504,14 @@ public function getByResource(
14121504
$tableName = $this->getTableName();
14131505
$tenantFilter = $this->getTenantFilter();
14141506
$escapedTable = $this->escapeIdentifier($this->database) . '.' . $this->escapeIdentifier($tableName);
1507+
$escapedResource = $this->escapeIdentifier('resource');
1508+
$escapedTime = $this->escapeIdentifier('time');
14151509

14161510
$sql = "
14171511
SELECT " . $this->getSelectColumns() . "
14181512
FROM {$escapedTable}
1419-
WHERE resource = {resource:String}{$tenantFilter}{$time['clause']}
1420-
ORDER BY time {$order}
1513+
WHERE {$escapedResource} = {resource:String}{$tenantFilter}{$time['clause']}
1514+
ORDER BY {$escapedTime} {$order}
14211515
LIMIT {limit:UInt64} OFFSET {offset:UInt64}
14221516
FORMAT TabSeparated
14231517
";
@@ -1446,11 +1540,12 @@ public function countByResource(
14461540
$tableName = $this->getTableName();
14471541
$tenantFilter = $this->getTenantFilter();
14481542
$escapedTable = $this->escapeIdentifier($this->database) . '.' . $this->escapeIdentifier($tableName);
1543+
$escapedResource = $this->escapeIdentifier('resource');
14491544

14501545
$sql = "
14511546
SELECT count()
14521547
FROM {$escapedTable}
1453-
WHERE resource = {resource:String}{$tenantFilter}{$time['clause']}
1548+
WHERE {$escapedResource} = {resource:String}{$tenantFilter}{$time['clause']}
14541549
FORMAT TabSeparated
14551550
";
14561551

@@ -1516,11 +1611,13 @@ public function countByUserAndEvents(
15161611
$tableName = $this->getTableName();
15171612
$tenantFilter = $this->getTenantFilter();
15181613
$escapedTable = $this->escapeIdentifier($this->database) . '.' . $this->escapeIdentifier($tableName);
1614+
$escapedUserId = $this->escapeIdentifier('userId');
1615+
$escapedEvent = $this->escapeIdentifier('event');
15191616

15201617
$sql = "
15211618
SELECT count()
15221619
FROM {$escapedTable}
1523-
WHERE userId = {userId:String} AND event IN ({$eventList['clause']}){$tenantFilter}{$time['clause']}
1620+
WHERE {$escapedUserId} = {userId:String} AND {$escapedEvent} IN ({$eventList['clause']}){$tenantFilter}{$time['clause']}
15241621
FORMAT TabSeparated
15251622
";
15261623

@@ -1586,11 +1683,13 @@ public function countByResourceAndEvents(
15861683
$tableName = $this->getTableName();
15871684
$tenantFilter = $this->getTenantFilter();
15881685
$escapedTable = $this->escapeIdentifier($this->database) . '.' . $this->escapeIdentifier($tableName);
1686+
$escapedResource = $this->escapeIdentifier('resource');
1687+
$escapedEvent = $this->escapeIdentifier('event');
15891688

15901689
$sql = "
15911690
SELECT count()
15921691
FROM {$escapedTable}
1593-
WHERE resource = {resource:String} AND event IN ({$eventList['clause']}){$tenantFilter}{$time['clause']}
1692+
WHERE {$escapedResource} = {resource:String} AND {$escapedEvent} IN ({$eventList['clause']}){$tenantFilter}{$time['clause']}
15941693
FORMAT TabSeparated
15951694
";
15961695

‎src/Audit/Adapter/Database.php‎

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -111,19 +111,15 @@ public function createBatch(array $logs): bool
111111
*/
112112
public function getById(string $id): ?Log
113113
{
114-
try {
115-
$document = $this->db->getAuthorization()->skip(function () use ($id) {
116-
return $this->db->getDocument($this->getCollectionName(), $id);
117-
});
118-
119-
if ($document->isEmpty()) {
120-
return null;
121-
}
114+
$document = $this->db->getAuthorization()->skip(function () use ($id) {
115+
return $this->db->getDocument($this->getCollectionName(), $id);
116+
});
122117

123-
return new Log($document->getArrayCopy());
124-
} catch (\Exception $e) {
118+
if ($document->isEmpty()) {
125119
return null;
126120
}
121+
122+
return new Log($document->getArrayCopy());
127123
}
128124

129125
/**

0 commit comments

Comments
 (0)