Repository navigation
Conversation
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.
There was a problem hiding this comment.
Pull request overview
Adds centralized OQL literal sanitization to prevent user-controlled query injection.
Changes:
- Adds
escapeOqlLiteral(). - Applies sanitization across four person/request search tools.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/Tools/iTopRestTools.php |
Adds shared OQL sanitization. |
src/Tools/core/get/iTopGetTools.php |
Sanitizes interpolated search parameters. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+48
to
+50
| $value = str_replace(["\\", "'", '"'], '', $value); | ||
| $value = preg_replace('/[\x00-\x1F\x7F]/u', '', $value); | ||
| return trim($value); |
Member
There was a problem hiding this comment.
Hello @agent-qv, it seems that indeed, your proposition may have unwanted behaviors in some cases.
Could you please correct it, and add unit tests so it's easier to test and avoid future regression ? :)
agent-qv
added a commit
to agent-qv/itop-mcp
that referenced
this pull request
Sep 2, 2026
searchIncidentByCallerStatusStartDate interpolates caller_email/statuses/start_date into an OQL query string, the same pattern PR Combodo#8 fixes for the pre-existing UserRequest/Person tools. Combodo#8 can't cover this site (it's based on master before Incident tools exist), so this PR escapes it locally rather than shipping a new instance of the same class of issue. escapeOqlLiteral() is duplicated from Combodo#8 rather than shared, since these are two independent, isolated proposals - intentional and temporary, to be collapsed into one definition once both land.
3 of 6 tasks
48347d5 fixed Combodo#7 upstream with quoteString(), which supersedes the escapeOqlLiteral() helper this branch introduced: the conflicts are resolved by taking upstream's code, so the helper and its four call sites are gone. What 48347d5 left out is kept: in search-user-request-by-caller-status-start-date, statuses and start_date are still interpolated raw. They now go through quoteString() too, like caller_email.
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.
Contributor
Author
|
@jf-cbd Sorry for the late answer. 48347d5 has since fixed most of #7, so I reworked this PR on top of it. The description above refers to the original version; the PR now only covers what 48347d5 left out:
Status codes such as |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #7
Problem
Several tools interpolate a user-controlled value directly into a quoted OQL string
literal, with no escaping:
get-person-from-email(email),get-person-from-fullname(
fullname),get-person-from-telephone(telephone), andsearch-user-request-by-caller-status-start-date(caller_email,statuses,start_date). A quote in any of them breaks out of the literal and lets the rest of thevalue alter the query.
Impact
The search tools run with the API account's own iTop permissions, not the caller's, so
this can widen a caller-scoped search beyond the intended caller and return tickets
belonging to someone else.
Fix
Adds
escapeOqlLiteral()on the sharediTopRestToolsbase 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 validcharacters in an email or a name; a
LIKEpattern would need its own wildcard-awaresanitization instead.
Applied to all four affected tools.