From 4aaec4cf18da2e722af8c9a2605daa2c59435e28 Mon Sep 17 00:00:00 2001 From: Sonny May Date: Thu, 10 Sep 2026 16:02:18 +0000 Subject: [PATCH 1/3] fix: escape user input before interpolating into PostgREST filters search_tickets (GET /tickets/search) and filter_tickets (GET /tickets/filter) interpolated user-supplied values (q, status, priority, assigned_user_id) directly into PostgREST filter strings. Comma and parentheses are structural in PostgREST or() syntax and nothing encoded & or =, so a value such as "x),status.eq.Closed&limit=1" could close the or() group, append extra conditions, and inject additional query parameters. Add database.escape_filter_value, which backslash-escapes PostgREST's reserved characters (\ , ( )) and then percent-encodes the result with safe="", and route every interpolated value through it. The surrounding template (wildcards, operators, &order=) is left untouched. Tests cover plain-text passthrough, that reserved characters never survive unescaped, the exact escape-then-encode output, and end-to-end injection attempts against both endpoints. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01L6wR9fK3YEMMKDwEAwr5Ju --- CHANGELOG.md | 3 +++ backend/database.py | 26 +++++++++++++++++++++++++ backend/main.py | 18 +++++++++++++----- backend/test_database.py | 22 +++++++++++++++++++++ backend/test_main.py | 41 ++++++++++++++++++++++++++++++++++++++++ 5 files changed, 105 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 636853a..2215901 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,9 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased] +### Fixed +- `GET /tickets/search` and `GET /tickets/filter` no longer interpolate raw user input into PostgREST filter strings. Values are now backslash-escaped for PostgREST's reserved characters (`\`, `,`, `(`, `)`) and 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. + ### Planned - Role-based auth (agent / lead / admin) with JWT bearer tokens - SLA timers with breach alerts diff --git a/backend/database.py b/backend/database.py index 6b1203b..542d10f 100644 --- a/backend/database.py +++ b/backend/database.py @@ -1,10 +1,36 @@ import os +import re +from urllib.parse import quote import requests from dotenv import load_dotenv load_dotenv() +# Characters that are structural in PostgREST filter syntax: backslash (the escape +# character itself), comma (separates conditions inside or()/and()), and parentheses +# (delimit logical groups). +_POSTGREST_RESERVED = re.compile(r"[\\,()]") + + +def escape_filter_value(value: str) -> str: + """Make a user-supplied value safe to interpolate into a PostgREST filter string. + + Two layers of protection: + + 1. Backslash-escape PostgREST's reserved characters (``\\``, ``,``, ``(``, ``)``) + so the value cannot terminate an ``or=(...)`` group or add extra conditions. + 2. Percent-encode the result with ``safe=""`` so ``&``, ``=``, and everything else + that is meaningful in a query string is sent literally rather than being parsed + as additional parameters. + + The caller is responsible for the surrounding template (operators, ``*`` wildcards, + ``&order=`` etc.) -- only the untrusted value goes through this function. + """ + escaped = _POSTGREST_RESERVED.sub(lambda m: "\\" + m.group(0), value) + return quote(escaped, safe="") + + SUPABASE_URL = os.getenv("SUPABASE_URL") SUPABASE_KEY = os.getenv("SUPABASE_KEY") DATABASE_UNAVAILABLE_DETAIL = ( diff --git a/backend/main.py b/backend/main.py index b1d7063..f259a21 100644 --- a/backend/main.py +++ b/backend/main.py @@ -5,7 +5,15 @@ import ai import database -from database import DatabaseConfigError, DatabaseRequestError, db_delete, db_get, db_patch, db_post +from database import ( + DatabaseConfigError, + DatabaseRequestError, + db_delete, + db_get, + db_patch, + db_post, + escape_filter_value, +) app = FastAPI(title="SupportOps API") @@ -172,7 +180,7 @@ def search_tickets(q: str = Query(..., min_length=1, description="Search term")) Returns tickets where the term appears in title OR description, sorted by most recently created first. """ - term = q.strip() + term = escape_filter_value(q.strip()) results = db_get( "tickets", f"or=(title.ilike.*{term}*,description.ilike.*{term}*)&order=created_at.desc", @@ -196,11 +204,11 @@ def filter_tickets( """ parts: list[str] = [] if status: - parts.append(f"status=eq.{status}") + parts.append(f"status=eq.{escape_filter_value(status)}") if priority: - parts.append(f"priority=eq.{priority}") + parts.append(f"priority=eq.{escape_filter_value(priority)}") if assigned_user_id: - parts.append(f"assigned_user_id=eq.{assigned_user_id}") + parts.append(f"assigned_user_id=eq.{escape_filter_value(assigned_user_id)}") parts.append("order=created_at.desc") query = "&".join(parts) results = db_get("tickets", query) diff --git a/backend/test_database.py b/backend/test_database.py index 9b5f3a4..fd26302 100644 --- a/backend/test_database.py +++ b/backend/test_database.py @@ -1,4 +1,5 @@ from unittest.mock import MagicMock +from urllib.parse import unquote import pytest import requests @@ -90,3 +91,24 @@ def test_supabase_network_error_raises_database_error(monkeypatch, error): with pytest.raises(database.DatabaseRequestError, match="temporarily unavailable"): database.db_get("tickets") + + +# --------------------------------------------------------------------------- +# escape_filter_value -- PostgREST filter injection guard +# --------------------------------------------------------------------------- + + +def test_escape_filter_value_passes_plain_text_through(): + assert database.escape_filter_value("printer") == "printer" + assert database.escape_filter_value("agent-7") == "agent-7" + + +def test_escape_filter_value_never_leaves_reserved_chars_unescaped(): + escaped = database.escape_filter_value("\\,()&=") + for char in "\\,()&=": + assert char not in escaped, f"{char!r} survived unescaped in {escaped!r}" + + +def test_escape_filter_value_backslash_escapes_then_percent_encodes(): + escaped = database.escape_filter_value("a,b(c)d\\e") + assert unquote(escaped) == "a\\,b\\(c\\)d\\\\e" diff --git a/backend/test_main.py b/backend/test_main.py index 3260ab7..ba1e1de 100644 --- a/backend/test_main.py +++ b/backend/test_main.py @@ -6,6 +6,7 @@ import sys import types from unittest.mock import MagicMock +from urllib.parse import quote import pytest from fastapi.testclient import TestClient @@ -150,6 +151,46 @@ def test_filter_tickets_returns_empty_list_when_no_matches(client, monkeypatch): assert res.json() == [] +def test_search_tickets_escapes_postgrest_injection(client, monkeypatch): + test_client, main = client + captured = {} + + def fake_get(table, params=""): + captured["params"] = params + return [] + + monkeypatch.setattr(main, "db_get", fake_get) + + # Attempt to close the or() group, append a condition, and inject a limit param. + payload = quote("x),status.eq.Closed&limit=1", safe="") + res = test_client.get(f"/tickets/search?q={payload}") + + assert res.status_code == 200 + params = captured["params"] + assert params.count("&") == 1, params + assert params.endswith("&order=created_at.desc") + assert "),status" not in params + assert "limit=1" not in params + + +def test_filter_tickets_escapes_postgrest_injection(client, monkeypatch): + test_client, main = client + captured = {} + + def fake_get(table, params=""): + captured["params"] = params + return [] + + monkeypatch.setattr(main, "db_get", fake_get) + + payload = quote("u2&limit=1", safe="") + res = test_client.get(f"/tickets/filter?assigned_user_id={payload}") + + assert res.status_code == 200 + assert "&limit=1" not in captured["params"] + assert captured["params"].endswith("&order=created_at.desc") + + def test_health_returns_ok(client): test_client, _ = client res = test_client.get("/health") From 3059c36fa65b9640efaa7b14980dfa0ea21ff393 Mon Sep 17 00:00:00 2001 From: Sonny May Date: Thu, 10 Sep 2026 17:10:24 +0000 Subject: [PATCH 2/3] fix: escape id path parameter before interpolating into PostgREST filters db_patch, db_delete, and the {id}-based ticket routes (GET /tickets/{id}, GET /tickets/{id}/notes, GET /tickets/{id}/history, and the status-history lookup in PUT /tickets/{id}) interpolated the raw id path parameter into PostgREST filter strings such as "id=eq.{id}". FastAPI decodes the path segment before handing it over, so a crafted id like "t1&status=eq.Closed" could append additional filters or query parameters to the Supabase request. Route every id through database.escape_filter_value (introduced for the search/filter endpoints) so the value is backslash-escaped for PostgREST's reserved characters and percent-encoded before it reaches the query string. Tests cover db_patch/db_delete directly and each affected route end-to-end. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01L6wR9fK3YEMMKDwEAwr5Ju --- CHANGELOG.md | 1 + backend/database.py | 4 ++-- backend/main.py | 8 +++---- backend/test_database.py | 27 ++++++++++++++++++++++ backend/test_main.py | 48 ++++++++++++++++++++++++++++++++++++++++ 5 files changed, 82 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2215901..8fbf89b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased] ### 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. Values are now backslash-escaped for PostgREST's reserved characters (`\`, `,`, `(`, `)`) and 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. ### Planned diff --git a/backend/database.py b/backend/database.py index 542d10f..594f838 100644 --- a/backend/database.py +++ b/backend/database.py @@ -105,8 +105,8 @@ def db_post(table, data): def db_patch(table, id, data): - return request_supabase("PATCH", table, params=f"id=eq.{id}", data=data) + return request_supabase("PATCH", table, params=f"id=eq.{escape_filter_value(id)}", data=data) def db_delete(table, id): - return request_supabase("DELETE", table, params=f"id=eq.{id}") + return request_supabase("DELETE", table, params=f"id=eq.{escape_filter_value(id)}") diff --git a/backend/main.py b/backend/main.py index f259a21..e786611 100644 --- a/backend/main.py +++ b/backend/main.py @@ -217,7 +217,7 @@ def filter_tickets( @app.get("/tickets/{id}") def get_ticket(id: str): - result = db_get("tickets", f"id=eq.{id}") + result = db_get("tickets", f"id=eq.{escape_filter_value(id)}") if not result: raise HTTPException(status_code=404, detail="Ticket not found") return result[0] @@ -234,7 +234,7 @@ def create_ticket(t: Ticket): @app.put("/tickets/{id}") def update_ticket(id: str, t: Ticket): data = clean_empty_strings(model_data(t), "customer_id", "device_id", "assigned_user_id") - old = db_get("tickets", f"id=eq.{id}&select=status") + old = db_get("tickets", f"id=eq.{escape_filter_value(id)}&select=status") result = db_patch("tickets", id, data) if old and old[0]["status"] != data["status"]: db_post( @@ -252,7 +252,7 @@ def update_ticket(id: str, t: Ticket): # --- Notes --- @app.get("/tickets/{id}/notes") def get_notes(id: str): - return db_get("ticket_notes", f"ticket_id=eq.{id}&order=created_at.asc") + return db_get("ticket_notes", f"ticket_id=eq.{escape_filter_value(id)}&order=created_at.asc") @app.post("/notes") @@ -263,7 +263,7 @@ def create_note(n: TicketNote): # --- History --- @app.get("/tickets/{id}/history") def get_history(id: str): - return db_get("ticket_history", f"ticket_id=eq.{id}&order=changed_at.asc") + return db_get("ticket_history", f"ticket_id=eq.{escape_filter_value(id)}&order=changed_at.asc") # --- RMAs --- diff --git a/backend/test_database.py b/backend/test_database.py index fd26302..274ad23 100644 --- a/backend/test_database.py +++ b/backend/test_database.py @@ -112,3 +112,30 @@ def test_escape_filter_value_never_leaves_reserved_chars_unescaped(): def test_escape_filter_value_backslash_escapes_then_percent_encodes(): escaped = database.escape_filter_value("a,b(c)d\\e") assert unquote(escaped) == "a\\,b\\(c\\)d\\\\e" + + +# --------------------------------------------------------------------------- +# db_patch / db_delete -- id path parameter is escaped +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize("helper", ["db_patch", "db_delete"]) +def test_id_helpers_escape_injected_query_params(monkeypatch, helper): + monkeypatch.setattr(database, "SUPABASE_URL", "https://example.supabase.co") + monkeypatch.setattr(database, "SUPABASE_KEY", "secret") + fake_response = MagicMock() + fake_response.status_code = 204 + fake_response.text = "" + fake_request = MagicMock(return_value=fake_response) + monkeypatch.setattr(database.requests, "request", fake_request) + + malicious_id = "t1&status=eq.Closed" + if helper == "db_patch": + database.db_patch("tickets", malicious_id, {"title": "x"}) + else: + database.db_delete("tickets", 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)) diff --git a/backend/test_main.py b/backend/test_main.py index ba1e1de..1d98f92 100644 --- a/backend/test_main.py +++ b/backend/test_main.py @@ -191,6 +191,54 @@ def fake_get(table, params=""): assert captured["params"].endswith("&order=created_at.desc") +@pytest.mark.parametrize( + "path_template", + ["/tickets/{id}", "/tickets/{id}/notes", "/tickets/{id}/history"], +) +def test_ticket_id_routes_escape_postgrest_injection(client, monkeypatch, path_template): + test_client, main = client + captured = {} + + def fake_get(table, params=""): + captured["params"] = params + return [{"id": "t1"}] + + monkeypatch.setattr(main, "db_get", fake_get) + + # A crafted id that tries to append an extra filter / parameter. + payload = quote("t1&status=eq.Closed", safe="") + res = test_client.get(path_template.format(id=payload)) + + assert res.status_code == 200 + params = captured["params"] + assert "&status=eq.Closed" not in params + # Only the template's own "&order=" (if any) may remain. + assert params.count("&") <= 1, params + + +def test_update_ticket_escapes_id_in_status_lookup(client, monkeypatch): + test_client, main = client + captured = {} + + def fake_get(table, params=""): + captured["get_params"] = params + return [{"status": "Open"}] + + monkeypatch.setattr(main, "db_get", fake_get) + monkeypatch.setattr(main, "db_patch", lambda table, id, data: [{"id": id, **data}]) + monkeypatch.setattr(main, "db_post", lambda table, data: [data]) + + payload = quote("t1&status=eq.Closed", safe="") + res = test_client.put( + f"/tickets/{payload}", + json={"title": "Printer", "status": "Open", "priority": "Low"}, + ) + + assert res.status_code == 200 + assert "&status=eq.Closed" not in captured["get_params"] + assert captured["get_params"].endswith("&select=status") + + def test_health_returns_ok(client): test_client, _ = client res = test_client.get("/health") From 1993164face8728e25ef9e8f6acf2892d36de621 Mon Sep 17 00:00:00 2001 From: Sonny May <102835434+sonnymay@users.noreply.github.com> Date: Thu, 10 Sep 2026 13:01:01 -0500 Subject: [PATCH 3/3] fix: quote PostgREST logical filter values --- CHANGELOG.md | 2 +- backend/database.py | 30 ++++++++++++------------------ backend/main.py | 4 ++-- backend/test_database.py | 19 ++++++++++++------- backend/test_main.py | 20 ++++++++++++-------- 5 files changed, 39 insertions(+), 36 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2215901..4c98462 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,7 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased] ### Fixed -- `GET /tickets/search` and `GET /tickets/filter` no longer interpolate raw user input into PostgREST filter strings. Values are now backslash-escaped for PostgREST's reserved characters (`\`, `,`, `(`, `)`) and 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. +- `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. ### Planned - Role-based auth (agent / lead / admin) with JWT bearer tokens diff --git a/backend/database.py b/backend/database.py index 542d10f..731d138 100644 --- a/backend/database.py +++ b/backend/database.py @@ -1,5 +1,4 @@ import os -import re from urllib.parse import quote import requests @@ -7,28 +6,23 @@ load_dotenv() -# Characters that are structural in PostgREST filter syntax: backslash (the escape -# character itself), comma (separates conditions inside or()/and()), and parentheses -# (delimit logical groups). -_POSTGREST_RESERVED = re.compile(r"[\\,()]") - -def escape_filter_value(value: str) -> str: +def escape_filter_value(value: str, *, wildcard: bool = False) -> str: """Make a user-supplied value safe to interpolate into a PostgREST filter string. - Two layers of protection: - - 1. Backslash-escape PostgREST's reserved characters (``\\``, ``,``, ``(``, ``)``) - so the value cannot terminate an ``or=(...)`` group or add extra conditions. - 2. Percent-encode the result with ``safe=""`` so ``&``, ``=``, and everything else - that is meaningful in a query string is sent literally rather than being parsed - as additional parameters. + The value is percent-encoded so query-string delimiters cannot create new + parameters. - The caller is responsible for the surrounding template (operators, ``*`` wildcards, - ``&order=`` etc.) -- only the untrusted value goes through this function. + ``wildcard=True`` additionally quotes the complete ``*value*`` pattern for use + inside a PostgREST logical expression. Embedded backslashes and double quotes + are escaped within that quoted value, so commas and parentheses cannot terminate + an ``or=(...)`` expression. Operators and other template structure remain the + caller's responsibility. """ - escaped = _POSTGREST_RESERVED.sub(lambda m: "\\" + m.group(0), value) - return quote(escaped, safe="") + if wildcard: + escaped = value.replace("\\", "\\\\").replace('"', '\\"') + return quote(f'"*{escaped}*"', safe="*") + return quote(value, safe="") SUPABASE_URL = os.getenv("SUPABASE_URL") diff --git a/backend/main.py b/backend/main.py index f259a21..328bc5a 100644 --- a/backend/main.py +++ b/backend/main.py @@ -180,10 +180,10 @@ def search_tickets(q: str = Query(..., min_length=1, description="Search term")) Returns tickets where the term appears in title OR description, sorted by most recently created first. """ - term = escape_filter_value(q.strip()) + term = escape_filter_value(q.strip(), wildcard=True) results = db_get( "tickets", - f"or=(title.ilike.*{term}*,description.ilike.*{term}*)&order=created_at.desc", + f"or=(title.ilike.{term},description.ilike.{term})&order=created_at.desc", ) return results if isinstance(results, list) else [] diff --git a/backend/test_database.py b/backend/test_database.py index fd26302..7cf4b0f 100644 --- a/backend/test_database.py +++ b/backend/test_database.py @@ -101,14 +101,19 @@ def test_supabase_network_error_raises_database_error(monkeypatch, error): def test_escape_filter_value_passes_plain_text_through(): assert database.escape_filter_value("printer") == "printer" assert database.escape_filter_value("agent-7") == "agent-7" + assert ( + database.escape_filter_value("123e4567-e89b-12d3-a456-426614174000") + == "123e4567-e89b-12d3-a456-426614174000" + ) + + +def test_escape_filter_value_percent_encodes_query_delimiters(): + escaped = database.escape_filter_value("x,status.like.Closed&select=*&path=/tickets") + assert escaped == "x%2Cstatus.like.Closed%26select%3D%2A%26path%3D%2Ftickets" -def test_escape_filter_value_never_leaves_reserved_chars_unescaped(): - escaped = database.escape_filter_value("\\,()&=") - for char in "\\,()&=": - assert char not in escaped, f"{char!r} survived unescaped in {escaped!r}" +def test_escape_filter_value_quotes_and_escapes_wildcard_pattern(): + escaped = database.escape_filter_value('a,b("c\\d")', wildcard=True) -def test_escape_filter_value_backslash_escapes_then_percent_encodes(): - escaped = database.escape_filter_value("a,b(c)d\\e") - assert unquote(escaped) == "a\\,b\\(c\\)d\\\\e" + assert unquote(escaped) == '"*a,b(\\"c\\\\d\\")*"' diff --git a/backend/test_main.py b/backend/test_main.py index ba1e1de..879319f 100644 --- a/backend/test_main.py +++ b/backend/test_main.py @@ -6,7 +6,7 @@ import sys import types from unittest.mock import MagicMock -from urllib.parse import quote +from urllib.parse import quote, unquote import pytest from fastapi.testclient import TestClient @@ -52,7 +52,9 @@ def fake_get(table, params=""): assert res.status_code == 200 assert res.json() == rows assert captured["table"] == "tickets" - assert "printer" in captured["params"].lower() + assert captured["params"] == ( + "or=(title.ilike.%22*printer*%22,description.ilike.%22*printer*%22)&order=created_at.desc" + ) def test_search_tickets_returns_empty_list_when_no_matches(client, monkeypatch): @@ -161,16 +163,19 @@ def fake_get(table, params=""): monkeypatch.setattr(main, "db_get", fake_get) - # Attempt to close the or() group, append a condition, and inject a limit param. - payload = quote("x),status.eq.Closed&limit=1", safe="") + # Backslashes do not protect logical delimiters unless the value is quoted. + payload = quote("x),status.like.Closed&limit=1", safe="") res = test_client.get(f"/tickets/search?q={payload}") assert res.status_code == 200 params = captured["params"] assert params.count("&") == 1, params assert params.endswith("&order=created_at.desc") - assert "),status" not in params - assert "limit=1" not in params + logical_filter = unquote(params.removesuffix("&order=created_at.desc")) + assert logical_filter == ( + 'or=(title.ilike."*x),status.like.Closed&limit=1*",' + 'description.ilike."*x),status.like.Closed&limit=1*")' + ) def test_filter_tickets_escapes_postgrest_injection(client, monkeypatch): @@ -187,8 +192,7 @@ def fake_get(table, params=""): res = test_client.get(f"/tickets/filter?assigned_user_id={payload}") assert res.status_code == 200 - assert "&limit=1" not in captured["params"] - assert captured["params"].endswith("&order=created_at.desc") + assert captured["params"] == ("assigned_user_id=eq.u2%26limit%3D1&order=created_at.desc") def test_health_returns_ok(client):