From 8d17e612cf08a4dbb5f8d1c4f816584f571316ff Mon Sep 17 00:00:00 2001 From: agent-qv Date: Tue, 1 Sep 2026 11:16:15 +0200 Subject: [PATCH 1/2] Escape user-controlled values before interpolating them into OQL caller_email, telephone, email, fullname, statuses and start_date were all interpolated as-is into a quoted OQL string literal. A quote in any of them breaks out of the literal and lets the rest of the value alter the query - for instance widening a caller-scoped ticket search to other callers, since the search runs with the API account's own permissions rather than the caller's. Adds escapeOqlLiteral() on the shared iTopRestTools base class - it strips quotes, the escape character and control characters, which is enough for exact-match values (equality, IN lists). It intentionally leaves % and _ alone, since those are valid characters in an email or a name. --- src/Tools/core/get/iTopGetTools.php | 12 +++++++----- src/Tools/iTopRestTools.php | 13 +++++++++++++ 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/src/Tools/core/get/iTopGetTools.php b/src/Tools/core/get/iTopGetTools.php index 0a5e031..9308b82 100644 --- a/src/Tools/core/get/iTopGetTools.php +++ b/src/Tools/core/get/iTopGetTools.php @@ -41,7 +41,7 @@ public function getPersonFromEmail(#[Schema(format: 'email')] string $email): st { $this->mcpLogger->info('[Tool called] get-person-from-email'); return $this->runToolFromTemplates('getPersonFromEmail', 'Anything', [ - 'email' => $email, + 'email' => $this->escapeOqlLiteral($email), 'fields' => $this->datamodel->getListZlist('Person'), ]); } @@ -54,7 +54,7 @@ public function getPersonFromFullname(string $fullname): string { $this->mcpLogger->info('[Tool called] get-person-from-fullname'); return $this->runToolFromTemplates('getPersonFromFullname', 'Anything', [ - 'fullname' => $fullname, + 'fullname' => $this->escapeOqlLiteral($fullname), 'fields' => $this->datamodel->getListZlist('Person'), ]); } @@ -71,6 +71,7 @@ public function getPersonFromTelephone(string $telephone): string if (count($phones) === 0) { throw new \Exception('Datamodel issue: there seem to be no phone number attribute on the Person class!'); } + $telephone = $this->escapeOqlLiteral($telephone); $conditions = array_map(function($item) use($telephone) { return "$item = '$telephone'"; }, $phones); $whereClause = implode(' OR ', $conditions); return $this->runToolFromTemplates('getPersonFromTelephone', 'Anything', [ @@ -105,11 +106,12 @@ protected function getPhoneFieldsFromPerson(): array #[McpTool(name: TOOL_PREFIX.'search-user-request-by-caller-status-start-date', annotations: new ToolAnnotations(null, true, false, true, false))] public function searchUserRequestByCallerStatusStartDate(#[Schema(format: 'email')] string $caller_email, array $statuses = [], string $start_date = '', string $condition = 'greater_than'): string { + $statuses = array_map(fn($status) => $this->escapeOqlLiteral((string)$status), $statuses); return $this->runToolFromTemplates('searchUserRequestFromCallerStatusDate', 'UserRequest', [ - 'caller_email' => $caller_email, - 'statuses' => count($statuses) > 0 ? "'".implode("','", $statuses)."'" : '', - 'start_date' => $start_date, + 'caller_email' => $this->escapeOqlLiteral($caller_email), + 'statuses' => count($statuses) > 0 ? "'".implode("','", $statuses)."'" : '', + 'start_date' => $this->escapeOqlLiteral($start_date), 'condition' => $condition === 'greater_than' ? '>=' : '<=', ] ); diff --git a/src/Tools/iTopRestTools.php b/src/Tools/iTopRestTools.php index 1398397..726c8eb 100644 --- a/src/Tools/iTopRestTools.php +++ b/src/Tools/iTopRestTools.php @@ -36,4 +36,17 @@ protected function postJsonToItop(string $json): string { return $this->iTopClient->postJsonToItop($json); } + + /** + * Removes the characters that would let a value break out of its OQL string literal: + * quotes, the escape character, and control characters. Meant for exact-match values + * (equality, IN lists) - a LIKE pattern also needs its wildcards (%, _) stripped, + * which callers must handle themselves. + */ + protected function escapeOqlLiteral(string $value): string + { + $value = str_replace(["\\", "'", '"'], '', $value); + $value = preg_replace('/[\x00-\x1F\x7F]/u', '', $value); + return trim($value); + } } \ No newline at end of file From 63fa8dc80ee8f8fd38eba89f4d14d914534051a4 Mon Sep 17 00:00:00 2001 From: agent-qv Date: Mon, 19 Oct 2026 11:15:42 +0200 Subject: [PATCH 2/2] Test that statuses and start_date stay inside their OQL literals Requested in the review of this PR: a unit test, so the behaviour is easy to check and a regression would show. The test feeds a status and a start date carrying an injection attempt to search-user-request-by-caller-status-start-date and checks the OQL the tool sends, once the JSON is decoded as iTop does: each value stays one quoted literal. --- tests/phpunit/ToolsFromTemplateTest.php | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/tests/phpunit/ToolsFromTemplateTest.php b/tests/phpunit/ToolsFromTemplateTest.php index 6d1156d..624af4d 100644 --- a/tests/phpunit/ToolsFromTemplateTest.php +++ b/tests/phpunit/ToolsFromTemplateTest.php @@ -167,4 +167,25 @@ public function testQuoteString(): void $this->assertEquals("Underscore\\_Test", iTopGetTools::quoteString("Underscore_Test")); $this->assertEquals("Backslash\\\\Test", iTopGetTools::quoteString("Backslash\\Test")); } -} \ No newline at end of file + + /** + * Every caller-supplied value must stay inside its own OQL string literal: a quote in a + * status or in the start date must not close it and add conditions to the query. + * The OQL is checked once the JSON is decoded, as iTop does, so that the assertion does + * not depend on how Twig escapes each character. + */ + public function testSearchUserRequestQuotesStatusesAndStartDate(): void + { + $expectedKey = <<<'OQL' +SELECT UserRequest AS U JOIN Person AS P ON U.caller_id = P.id WHERE P.email='john@example.com' AND U.status IN('new','x\') OR (\'1\'=\'1') AND U.start_date >= '2026-01-01\' OR \'1\'=\'1' +OQL; + $mockiTopClient = $this->createMock(iTopClient::class); + $mockiTopClient->expects(self::once()) + ->method('postJsonToItop') + ->with($this->callback(fn(string $json) => (json_decode($json, true)['key'] ?? null) === $expectedKey)) + ->willReturn(json_encode(['objects' => null, 'code' => 0, 'message' => 'Found: 0'])); + + $tools = new iTopGetTools($this->twigEnvironment, $mockiTopClient, $this->logger, $this->datamodel); + $tools->searchUserRequestByCallerStatusStartDate('john@example.com', ['new', "x') OR ('1'='1"], "2026-01-01' OR '1'='1"); + } +}