From c61b4d550a5379629a667bc9c9ee8fc42427b6cd Mon Sep 17 00:00:00 2001 From: Sonny May Date: Tue, 15 Sep 2026 14:06:51 +0000 Subject: [PATCH] fix: escape ticket_id before interpolating into PostgREST filters in ai.py POST /ai/suggest passes the caller-supplied ticket_id straight into _fetch_ticket and _fetch_notes, which built PostgREST filter strings by raw string interpolation (f"id=eq.{ticket_id}"). This is the same filter-injection class fixed for main.py's routes in #18/#19 (a crafted id could append extra query parameters, e.g. "t1&status=eq.Closed"), but ai.py's helpers were missed. Route both filters through the existing escape_filter_value helper and add regression tests confirming injected id values no longer produce extra query parameters. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01F7DCf2tVL9vazNThJWioGA --- CHANGELOG.md | 1 + backend/ai.py | 8 ++++--- backend/test_ai.py | 52 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 58 insertions(+), 3 deletions(-) create mode 100644 backend/test_ai.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 17cf542..6f934d6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed - `db_patch`, `db_delete`, and the `{id}`-based ticket routes (`GET /tickets/{id}`, `GET /tickets/{id}/notes`, `GET /tickets/{id}/history`, and the status lookup in `PUT /tickets/{id}`) now escape the `id` path parameter before interpolating it into PostgREST filters, so a crafted id can no longer append extra conditions or query parameters. - `GET /tickets/search` and `GET /tickets/filter` no longer interpolate raw user input into PostgREST filter strings. Reserved logical-filter values are now quoted with embedded backslashes and quotes escaped, and all values are percent-encoded, so a crafted `q`, `status`, `priority`, or `assigned_user_id` can no longer break out of the intended filter or inject extra query parameters. +- `POST /ai/suggest` no longer interpolates the caller-supplied `ticket_id` directly into PostgREST filters when loading the ticket and its notes. This was the same filter-injection gap already closed for the REST routes in `main.py`, but `ai.py`'s helpers were missed; they now escape `ticket_id` via `escape_filter_value` as well. ### Planned - Role-based auth (agent / lead / admin) with JWT bearer tokens diff --git a/backend/ai.py b/backend/ai.py index d16befa..5bfff01 100644 --- a/backend/ai.py +++ b/backend/ai.py @@ -8,7 +8,7 @@ from anthropic import Anthropic from dotenv import load_dotenv -from database import db_get +from database import db_get, escape_filter_value load_dotenv() @@ -28,14 +28,16 @@ def _get_client() -> Anthropic: def _fetch_ticket(ticket_id: str) -> dict[str, Any]: - result = db_get("tickets", f"id=eq.{ticket_id}") + result = db_get("tickets", f"id=eq.{escape_filter_value(ticket_id)}") if not isinstance(result, list) or not result: raise ValueError(f"Ticket {ticket_id} not found") return result[0] def _fetch_notes(ticket_id: str) -> list[dict[str, Any]]: - notes = db_get("ticket_notes", f"ticket_id=eq.{ticket_id}&order=created_at.asc") + notes = db_get( + "ticket_notes", f"ticket_id=eq.{escape_filter_value(ticket_id)}&order=created_at.asc" + ) return notes if isinstance(notes, list) else [] diff --git a/backend/test_ai.py b/backend/test_ai.py new file mode 100644 index 0000000..5727976 --- /dev/null +++ b/backend/test_ai.py @@ -0,0 +1,52 @@ +from unittest.mock import MagicMock + +import pytest + +import ai +import database + +# --------------------------------------------------------------------------- +# _fetch_ticket / _fetch_notes -- ticket_id is escaped before interpolation +# +# POST /ai/suggest passes the caller-supplied ticket_id straight into these +# helpers. Without escaping, a value such as "t1&status=eq.Closed" could +# inject extra PostgREST query parameters, the same filter-injection class +# fixed for the /tickets/search and /tickets/filter endpoints in main.py. +# --------------------------------------------------------------------------- + + +@pytest.fixture +def fake_request(monkeypatch): + monkeypatch.setattr(database, "SUPABASE_URL", "https://example.supabase.co") + monkeypatch.setattr(database, "SUPABASE_KEY", "secret") + response = MagicMock() + response.status_code = 200 + response.text = "[]" + response.json.return_value = [] + request = MagicMock(return_value=response) + monkeypatch.setattr(database.requests, "request", request) + return request + + +def test_fetch_ticket_escapes_injected_query_params(fake_request): + malicious_id = "t1&status=eq.Closed" + + with pytest.raises(ValueError, match="not found"): + ai._fetch_ticket(malicious_id) + + url = fake_request.call_args.args[1] + assert url.count("&") == 0, url + assert "status=eq.Closed" not in url + assert url.endswith("/rest/v1/tickets?id=eq." + database.escape_filter_value(malicious_id)) + + +def test_fetch_notes_escapes_injected_query_params(fake_request): + malicious_id = "t1&select=*" + + notes = ai._fetch_notes(malicious_id) + + assert notes == [] + url = fake_request.call_args.args[1] + assert url.count("&") == 1, url # only the legitimate trailing &order=... remains + assert "select=" not in url + assert "ticket_id=eq." + database.escape_filter_value(malicious_id) in url