From 7ed3d0efe568e544ab57d0f60920a641496b85a7 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 13 Sep 2026 18:13:45 +0000 Subject: [PATCH] Decode list-query parameters through parse_query Lesson, todo, schedule, request, run, and admin-audit list handlers hand-rolled query splitting and skipped url_decode. Encoded spaces (q=rate%20limit) and plus-as-space missed rows that conversation list already found. Route those handlers through the shared decoder. Co-authored-by: Tyler Reckart --- CHANGELOG.md | 6 ++ CMakeLists.txt | 11 +++ src/api_server.cpp | 177 ++++++++++----------------------------- tests/test_http_util.cpp | 53 ++++++++++++ 4 files changed, 115 insertions(+), 132 deletions(-) create mode 100644 tests/test_http_util.cpp diff --git a/CHANGELOG.md b/CHANGELOG.md index dae12b43..97046782 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,12 @@ loosely while pre-1.0 (breaking changes can land on minor bumps). ## [Unreleased] +### Fixed +- **List-query URL decoding.** Lesson, todo, schedule, request, run, + and admin-audit list handlers now go through `parse_query()` (same as + conversation list). Encoded spaces (`q=rate%20limit`, `+`) and + percent-encoded filter values no longer miss rows. + ## [0.13.4] — 2026-09-10 ### Fixed diff --git a/CMakeLists.txt b/CMakeLists.txt index ee7a9662..66f302a6 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1272,6 +1272,17 @@ if(INDEX_BUILD_TESTS) ) add_test(NAME unit_cors COMMAND unit_cors) + # --- Unit tests for query-string decode (list handlers share parse_query) --- + add_executable(unit_http_util + tests/test_http_util.cpp + src/api/http_util.cpp + ) + target_include_directories(unit_http_util PRIVATE + ${CMAKE_SOURCE_DIR}/include + ${CMAKE_SOURCE_DIR}/third_party/doctest + ) + add_test(NAME unit_http_util COMMAND unit_http_util) + add_executable(unit_sse_mailbox tests/test_sse_mailbox.cpp) target_include_directories(unit_sse_mailbox PRIVATE ${CMAKE_SOURCE_DIR}/include diff --git a/src/api_server.cpp b/src/api_server.cpp index 019b9b40..44972993 100644 --- a/src/api_server.cpp +++ b/src/api_server.cpp @@ -330,26 +330,12 @@ void handle_admin(int fd, const HttpRequest& req, } int64_t before_id = 0; int limit = 50; - auto qpos = req.path.find('?'); - if (qpos != std::string::npos) { - std::string qs = req.path.substr(qpos + 1); - size_t i = 0; - while (i < qs.size()) { - size_t amp = qs.find('&', i); - std::string pair = qs.substr(i, amp - i); - auto eq = pair.find('='); - if (eq != std::string::npos) { - std::string k = pair.substr(0, eq); - std::string v = pair.substr(eq + 1); - if (k == "before_id") { - try { before_id = std::stoll(v); } catch (...) {} - } else if (k == "limit") { - try { limit = std::stoi(v); } catch (...) {} - } - } - if (amp == std::string::npos) break; - i = amp + 1; - } + const auto qp = parse_query(req.path); + if (auto it = qp.find("before_id"); it != qp.end()) { + try { before_id = std::stoll(it->second); } catch (...) {} + } + if (auto it = qp.find("limit"); it != qp.end()) { + try { limit = std::stoi(it->second); } catch (...) {} } auto rows = tenants.list_admin_audit(before_id, limit); auto arr = jarr(); @@ -3585,22 +3571,9 @@ request_status_to_json(const TenantStore::RequestStatus& s) { void handle_request_list(int fd, const HttpRequest& req, TenantStore& tenants, const Tenant& tenant) { int limit = 100; - auto qpos = req.path.find('?'); - if (qpos != std::string::npos) { - std::string qs = req.path.substr(qpos + 1); - size_t i = 0; - while (i < qs.size()) { - size_t amp = qs.find('&', i); - std::string pair = qs.substr(i, amp - i); - auto eq = pair.find('='); - if (eq != std::string::npos) { - std::string k = pair.substr(0, eq); - std::string v = pair.substr(eq + 1); - if (k == "limit") try { limit = std::stoi(v); } catch (...) {} - } - if (amp == std::string::npos) break; - i = amp + 1; - } + const auto qp = parse_query(req.path); + if (auto it = qp.find("limit"); it != qp.end()) { + try { limit = std::stoi(it->second); } catch (...) {} } auto rows = tenants.list_request_status(tenant.id, limit); auto arr = jarr(); @@ -3667,22 +3640,9 @@ void handle_request_events(int fd, const std::string& request_id, } int64_t since_seq = 0; - auto qpos = req.path.find('?'); - if (qpos != std::string::npos) { - std::string qs = req.path.substr(qpos + 1); - size_t i = 0; - while (i < qs.size()) { - size_t amp = qs.find('&', i); - std::string pair = qs.substr(i, amp - i); - auto eq = pair.find('='); - if (eq != std::string::npos) { - std::string k = pair.substr(0, eq); - std::string v = pair.substr(eq + 1); - if (k == "since_seq") try { since_seq = std::stoll(v); } catch (...) {} - } - if (amp == std::string::npos) break; - i = amp + 1; - } + const auto qp = parse_query(req.path); + if (auto it = qp.find("since_seq"); it != qp.end()) { + try { since_seq = std::stoll(it->second); } catch (...) {} } // Open the SSE response. CORS headers + X-Accel-Buffering: no so @@ -3906,24 +3866,14 @@ void handle_lesson_list(int fd, const HttpRequest& req, std::string agent_id; std::string query; int limit = 100; - auto qpos = req.path.find('?'); - if (qpos != std::string::npos) { - std::string qs = req.path.substr(qpos + 1); - size_t i = 0; - while (i < qs.size()) { - size_t amp = qs.find('&', i); - std::string pair = qs.substr(i, amp - i); - auto eq = pair.find('='); - if (eq != std::string::npos) { - std::string k = pair.substr(0, eq); - std::string v = pair.substr(eq + 1); - if (k == "agent_id") agent_id = v; - else if (k == "q") query = v; - else if (k == "limit") try { limit = std::stoi(v); } catch (...) {} - } - if (amp == std::string::npos) break; - i = amp + 1; - } + // parse_query url-decodes — the previous hand-rolled split left + // `q=rate%20limit` as a literal, so standard clients missed rows + // that conversation list (already on parse_query) would find. + const auto qp = parse_query(req.path); + if (auto it = qp.find("agent_id"); it != qp.end()) agent_id = it->second; + if (auto it = qp.find("q"); it != qp.end()) query = it->second; + if (auto it = qp.find("limit"); it != qp.end()) { + try { limit = std::stoi(it->second); } catch (...) {} } auto rows = query.empty() ? tenants.list_lessons(tenant.id, agent_id, limit) @@ -4091,33 +4041,22 @@ void handle_todo_list(int fd, const HttpRequest& req, TenantStore& tenants, const Tenant& tenant) { TenantStore::TodoFilter f; f.limit = 200; - auto qpos = req.path.find('?'); - if (qpos != std::string::npos) { - std::string qs = req.path.substr(qpos + 1); - size_t i = 0; - while (i < qs.size()) { - size_t amp = qs.find('&', i); - std::string pair = qs.substr(i, amp - i); - auto eq = pair.find('='); - if (eq != std::string::npos) { - std::string k = pair.substr(0, eq); - std::string v = pair.substr(eq + 1); - if (k == "conversation_id") { - // `conversation_id=tenant` is the spelled-out form - // for the unscoped-only filter; numeric values pass - // through directly (positive = OR-NULL fallback to - // unscoped, 0 = no filter, negative = unscoped only). - if (v == "tenant" || v == "unscoped") f.conversation_id = -1; - else try { f.conversation_id = std::stoll(v); } catch (...) {} - } - else if (k == "status") f.status_filter = v; - else if (k == "agent_id") f.agent_id_filter = v; - else if (k == "limit") - try { f.limit = std::stoi(v); } catch (...) {} - } - if (amp == std::string::npos) break; - i = amp + 1; - } + const auto qp = parse_query(req.path); + if (auto it = qp.find("conversation_id"); it != qp.end()) { + const std::string& v = it->second; + // `conversation_id=tenant` is the spelled-out form + // for the unscoped-only filter; numeric values pass + // through directly (positive = OR-NULL fallback to + // unscoped, 0 = no filter, negative = unscoped only). + if (v == "tenant" || v == "unscoped") f.conversation_id = -1; + else try { f.conversation_id = std::stoll(v); } catch (...) {} + } + if (auto it = qp.find("status"); it != qp.end()) + f.status_filter = it->second; + if (auto it = qp.find("agent_id"); it != qp.end()) + f.agent_id_filter = it->second; + if (auto it = qp.find("limit"); it != qp.end()) { + try { f.limit = std::stoi(it->second); } catch (...) {} } // Reject unknown / TUI-origin conversation ids. Clearing the filter // would return tenant-wide todos and leak TUI-scoped rows over HTTP. @@ -4474,24 +4413,9 @@ void handle_schedule_create(int fd, const HttpRequest& req, void handle_schedule_list(int fd, const HttpRequest& req, TenantStore& tenants, const Tenant& tenant) { std::string status_filter; - auto qpos = req.path.find('?'); - if (qpos != std::string::npos) { - std::string qs = req.path.substr(qpos + 1); - // Tiny query-string parser — only one key we look at. - size_t i = 0; - while (i < qs.size()) { - size_t amp = qs.find('&', i); - std::string pair = qs.substr(i, amp - i); - auto eq = pair.find('='); - if (eq != std::string::npos) { - std::string k = pair.substr(0, eq); - std::string v = pair.substr(eq + 1); - if (k == "status") status_filter = v; - } - if (amp == std::string::npos) break; - i = amp + 1; - } - } + const auto qp = parse_query(req.path); + if (auto it = qp.find("status"); it != qp.end()) + status_filter = it->second; auto rows = tenants.list_scheduled_tasks(tenant.id, status_filter, /*limit=*/200); auto arr = jarr(); for (const auto& r : rows) arr->as_array_mut().push_back(scheduled_task_to_json(r)); @@ -5318,23 +5242,12 @@ void handle_runs_list(int fd, const HttpRequest& req, TenantStore& tenants, const Tenant& tenant) { int64_t since = 0; int64_t task_id = 0; - auto qpos = req.path.find('?'); - if (qpos != std::string::npos) { - std::string qs = req.path.substr(qpos + 1); - size_t i = 0; - while (i < qs.size()) { - size_t amp = qs.find('&', i); - std::string pair = qs.substr(i, amp - i); - auto eq = pair.find('='); - if (eq != std::string::npos) { - std::string k = pair.substr(0, eq); - std::string v = pair.substr(eq + 1); - if (k == "since") try { since = std::stoll(v); } catch (...) {} - if (k == "task_id") try { task_id = std::stoll(v); } catch (...) {} - } - if (amp == std::string::npos) break; - i = amp + 1; - } + const auto qp = parse_query(req.path); + if (auto it = qp.find("since"); it != qp.end()) { + try { since = std::stoll(it->second); } catch (...) {} + } + if (auto it = qp.find("task_id"); it != qp.end()) { + try { task_id = std::stoll(it->second); } catch (...) {} } auto runs = tenants.list_task_runs(tenant.id, task_id, since, /*limit=*/200); auto arr = jarr(); diff --git a/tests/test_http_util.cpp b/tests/test_http_util.cpp new file mode 100644 index 00000000..c2425ed6 --- /dev/null +++ b/tests/test_http_util.cpp @@ -0,0 +1,53 @@ +// tests/test_http_util.cpp — query-string decode used by list handlers +#define DOCTEST_CONFIG_IMPLEMENT_WITH_MAIN +#include "doctest.h" +#include "api/http_util.h" + +using namespace arbiter; + +TEST_CASE("url_decode turns percent-escapes and plus into the raw value") { + CHECK(url_decode("rate%20limit") == "rate limit"); + CHECK(url_decode("rate+limit") == "rate limit"); + CHECK(url_decode("in%5Fprogress") == "in_progress"); + CHECK(url_decode("plain") == "plain"); + CHECK(url_decode("%2Ftmp") == "/tmp"); + // Invalid % sequences stay literal (same as the previous splitters). + CHECK(url_decode("100%") == "100%"); + CHECK(url_decode("%GG") == "%GG"); +} + +TEST_CASE("parse_query decodes list-handler filters the hand-rolled split missed") { + // GET /v1/lessons?q=rate%20limit — browsers and curl --data-urlencode + // send this; the old pair-splitter searched for the literal "%20". + auto lessons = parse_query("/v1/lessons?agent_id=scout&q=rate%20limit&limit=20"); + CHECK(lessons["agent_id"] == "scout"); + CHECK(lessons["q"] == "rate limit"); + CHECK(lessons["limit"] == "20"); + + auto plus = parse_query("/v1/lessons?q=rate+limit"); + CHECK(plus["q"] == "rate limit"); + + auto todos = parse_query("/v1/todos?status=in%5Fprogress&conversation_id=tenant"); + CHECK(todos["status"] == "in_progress"); + CHECK(todos["conversation_id"] == "tenant"); + + auto schedules = parse_query("/v1/schedules?status=active"); + CHECK(schedules["status"] == "active"); + + // Numeric filters: encoded digits must still parse with stoll/stoi. + auto audit = parse_query("/v1/admin/audit?before_id=%31%30&limit=%35%30"); + CHECK(audit["before_id"] == "10"); + CHECK(audit["limit"] == "50"); + + auto encoded_key = parse_query("/v1/lessons?q%3D=nope&q=ok"); + CHECK(encoded_key["q"] == "ok"); + CHECK(encoded_key["q="] == "nope"); +} + +TEST_CASE("parse_query last value wins and ignores valueless pairs") { + auto qp = parse_query("/v1/lessons?q=first&q=second&flag&limit=5"); + CHECK(qp["q"] == "second"); + CHECK(qp["limit"] == "5"); + CHECK(qp.count("flag") == 0); + CHECK(parse_query("/v1/lessons").empty()); +}