Plan: Encode BFF Backend GET Queries by Construction
On this page
Status
| Step | Description | Status |
|---|---|---|
0 |
Plan file and nav entry |
Done (2026-07-06) |
1 |
|
Done (2026-07-06) |
2 |
|
Done (2026-07-06) — href builders live in |
3 |
Migrate all 42 backend GET query sites to typed query (query-less paths) |
Done (2026-07-06) — dead |
4 |
Merge-gating guard ( |
Done (2026-07-06) — |
5 |
Tests (href encode, round-trip proptest, transport wiremock, |
Done (2026-07-06) |
6 |
CHANGELOG, verification, commit, push, MR |
Done (2026-07-06) — !925 merged as db7e5667 (regular merge commit); a broken intra-doc link the pre-push rustdoc gate caught was fixed in 9e6084a0 |
Issue: #787 (also unblocks #882)
Branch: feature/p2-web-list-query-encoding
Context
craig-web BFF handlers build backend GET queries as raw String values, splice user input in
with no percent-encoding, and bake the query into the request path. The transport does
format!("{base_url}{path}") then http.get(&url), so reqwest never encodes — &/=/#/space
in a filter value act as query delimiters (HTTP-parameter smuggling + silent pagination
corruption). Audit finding web-search-term-unescaped-query-injection, severity medium.
The root cause is the antipattern: hand-built query strings plus a transport whose path
argument secretly carries a query, so encoding responsibility is smeared across every call site
and reqwest’s own encoding is bypassed. Fixing only the encoding at each splice site (Option A)
leaves the pattern in place and adds a footgun where the next hand-built query reintroduces the
bug. This plan fixes the class by construction: handlers emit typed (key, value) pairs, the
transport assembles the URL via reqwest .query() (percent-encodes structurally), and a
merge-gating guard forbids hand-built query strings in routes/ so the antipattern cannot return.
Pre-1.0, BFF→backend is internal (no wire contract to preserve).
Surface
42 query-bearing backend GET sites, all originating in routes/** (the clients/ layer builds
none):
-
26 list handlers — carry free-text (
search/sort_by/sort_dirviaappend_search_sort, plus per-handlerstatus/worker/admin_unit/service/action/user_id/kind/severity/license_status). This is the exploitable surface. -
16 non-list sites — all provably safe (Uuid / i32 / u32 / bool / const / literal only; no free-text). Same antipattern, non-injectable. Migrated too so the pattern survives nowhere and the guard can be unconditional.
get_with_query already exists in the CLI and test-lib transports — mirror it. reqwest .query()
is core (not feature-gated); urlencoding is already a craig-web dependency.
Design
Transport + per-domain proxies (Step 1)
Add get_with_query<Q: Serialize>(base_url, path, query, user) to clients/transport.rs
(http.get(url).query(query), .query() applied before apply_identity). Add a
get_with_query(path, query, user) proxy — mirroring the existing get proxy — to the 5
per-domain clients whose non-list handlers call client.get(path) directly: Cases, Placements,
SecurityReviews, Exchange, ExchangeTransactions.
Fetch helpers + href builders (Step 2)
Add fetch_page_with_query and fetch_page_or_empty_with_query to routes/mod.rs (mirroring
fetch_page/fetch_page_or_empty, threading query: &Q into get_with_query). In
extra_params and sort_extra_params, percent-encode the value (urlencoding::encode(value);
keys are literals). This closes the same raw-concat bug on the HTML pagination-href surface
(Askama HTML-escapes but does not URL-encode). append_search_sort changes to push typed pairs
onto a Vec<(&str, &str)> instead of mutating a String.
Migration (Step 3)
Every handler builds a typed Vec<(&str, &str)> (page/per_page + present filters), passes a
query-less path, and calls the *_with_query API.
Rules:
-
Use the raw request params in the backend
Vec, under their existingSome/skip guards (params.status,explicit_worker,params.admin_unit, …). Never push the UI display values (filter,worker_filter,"all"defaults) — those default to"all"and would make the backend filter for a literal"all". Display values stay href-only inpagination_params. -
Preserve each handler’s exact current push conditions.
-
Non-string values (Uuid/i32/bool) need owned
Stringlocals to borrow as&str.
Guard (Step 4)
New xtask/src/cmd/validate_web_query_strings.rs mirroring validate_template_safety.rs (roots →
WalkDir → per-line regex → bail!). Flags any string literal in
services/craig-web/src/routes/*/.rs matching a query shape (?/& immediately followed by
<ident>=). Skips [cfg(test)]. Inline waiver // allow-query-string: <reason> (non-empty
reason), mirroring allow-secret:; the only expected use is the pre-encoded redirect Location
in routes/security/partners.rs. Wired into validate.rs phase_code_security_lints beside the
template-safety scan (pre-push + CI).
Tests
Axis-tagged, fails-before/passes-after:
-
Href encoding:
extra_params(&[("search","a&b c#d")])→&search=a%26b%20c%23d(andsort_extra_params). -
Encoding round-trip proptest (mandatory — serializer logic): for arbitrary non-empty
v != "all", the emitted&search=<v>value portion decodes back tovand contains no&/=/#/space; no panic on any input. -
Transport backend encoding via
wiremockquery_parammatcher — proves the value is encoded in transit, not smuggled. -
append_search_sort— assert the producedVecpairs (contract changed String→pairs). -
Guard scan unit test — a
?page=literal is flagged; clean + waivered samples pass.
Verification
cargo fmt --all → cargo clippy -p craig-web -p xtask --all-targets — -D warnings →
cargo nextest run -p craig-web -p xtask → cargo xtask quality-budgets (no OVER) →
cargo xtask validate --skip-docker + check-docs + plan-lint + axis-coverage. Fresh
contextless J1–J8 review of the staged diff. Manual (devstack): filter a list page with a value
containing &/space/#; confirm results are correct, links carry the encoded value, and the
backend receives the decoded value verbatim.