Fix: Prevent SQL injection in QueueJobModel priority sorting#94
Conversation
michalsn
left a comment
michalsn
left a comment
There was a problem hiding this comment.
Thanks for the PR. Could you please adjust the PR title, description, and commit message to avoid framing this as a security vulnerability?
As discussed, the affected priority list is normally controlled by the application developer/operator through config or CLI, not by untrusted end users in normal usage. This is better described as hardening the database queue ordering logic.
| $builder->whereIn('priority', $priority); | ||
|
|
There was a problem hiding this comment.
Please add normalization:
| $builder->whereIn('priority', $priority); | |
| $priority = array_values($priority); | |
| $builder->whereIn('priority', $priority); |
Because array_map([$this->db, 'escape'], $priority) preserves string keys, a caller could pass an associative priority array and put unsafe SQL into the THEN position (for non-MySQL). Normal usage likely passes a list, but the model method accepts any array.
| public function testSetPriority(): void | ||
| { | ||
| $model = model(QueueJobModel::class); | ||
| $method = $this->getPrivateMethodInvoker($model, 'setPriority'); | ||
| $builder = $model->builder(); | ||
|
|
||
| $result = $method($builder, ['high', 'low']); | ||
|
|
||
| $sql = $result->getCompiledSelect(); | ||
|
|
||
| $this->assertStringContainsString('priority', $sql); | ||
| if ($model->db->DBDriver === 'MySQLi') { | ||
| $this->assertStringContainsString('FIELD(priority, ', $sql); | ||
| } else { | ||
| $this->assertStringContainsString('CASE ', $sql); | ||
| $this->assertStringContainsString(' WHEN ', $sql); | ||
| $this->assertStringContainsString(' THEN ', $sql); | ||
| $this->assertStringContainsString(' END', $sql); | ||
| } | ||
| } |
There was a problem hiding this comment.
The test seems a bit weak. It checks that SQL contains CASE/FIELD, but not that dangerous values are escaped or keys are normalized.
2 participants
This PR fixes a potential SQL injection vulnerability in the QueueJobModel by properly escaping priority values before using them in CASE and FIELD SQL statements. A unit test was also added to ensure correct behavior.