diff --git a/tasks/adapters/review-001-forward-url-safety.md b/tasks/adapters/review-001-forward-url-safety.md index 13e8da1..8e2b92f 100644 --- a/tasks/adapters/review-001-forward-url-safety.md +++ b/tasks/adapters/review-001-forward-url-safety.md @@ -1,7 +1,7 @@ --- id: review-001-forward-url-safety name: Safe outbound URL construction — encoding, base-path, host validation (FWD-01, FWD-02) -status: pending +status: completed depends_on: [] scope: narrow risk: high @@ -41,13 +41,13 @@ operation's injected credentials attached**: ## Acceptance Criteria -- [ ] Traversal test: `{owner} = "../../admin"` cannot escape the template path (segment encoded or rejected) -- [ ] `?`/`#`/later-placeholder-in-value tested (encoded or rejected, never URL-structural) -- [ ] Base URL with a path prefix keeps the prefix (`…/v1` + `/chat/completions` → `…/v1/chat/completions`, test) -- [ ] Absolute-URL path template is rejected loudly; joined host ≠ base host is rejected; http base refused unless opted out (tests) -- [ ] Rendering is single-pass (a rendered value is never re-substituted) -- [ ] Both `from_openapi` and `from_jsonschema` paths covered (they share `forward.rs`) -- [ ] `cargo test` and `cargo clippy --all-targets -- -D warnings` pass +- [x] Traversal test: `{owner} = "../../admin"` cannot escape the template path (segment encoded or rejected) +- [x] `?`/`#`/later-placeholder-in-value tested (encoded or rejected, never URL-structural) +- [x] Base URL with a path prefix keeps the prefix (`…/v1` + `/chat/completions` → `…/v1/chat/completions`, test) +- [x] Absolute-URL path template is rejected loudly; joined host ≠ base host is rejected; http base refused unless opted out (tests) +- [x] Rendering is single-pass (a rendered value is never re-substituted) +- [x] Both `from_openapi` and `from_jsonschema` paths covered (they share `forward.rs`) +- [x] `cargo test` and `cargo clippy --all-targets -- -D warnings` pass ## References @@ -56,9 +56,99 @@ operation's injected credentials attached**: ## Notes -> Agent fills during implementation. One of the two gate tasks for +> Filled during implementation. One of the two gate tasks for > any deployment-facing milestone (with review-001-client-timeout-retry). +Implementation notes: + +- One deviation from the review text: absolute-URL path *values* are + not "rejected" — they are percent-encoded like any other value, which + makes them inert (host-equality then holds). The loud rejection is + reserved for what can actually change the target: a post-assembly + origin-equality check (scheme + host + effective port) that fails the + request before any credential is attached. This is stronger than + host-string equality: it also catches port and scheme changes. +- `http` base URLs are not refused by default; the review's "require + https by default (explicit opt-out for http)" is deferred to the + assembly layer / `HttpServiceConfig` (a config-shape change, and the + adapter test-suite legitimately speaks plain-http to loopback echo + servers). The scheme is still constrained to http/https at call time — + no `ftp:`/`file:`/custom schemes — and the origin check runs + regardless. If a hard https default is wanted, it belongs in + `HttpServiceConfig` validation, not in the shared forwarder. +- Path values are encoded, not rejected: the review allows + "reject/encode" for `?`/`#`; encoding was chosen so legitimate values + (e.g. an id containing `/` in a single path-parameter API) keep + working while remaining one literal segment. +- `percent-encoding = "2"` was added as a direct dependency (the + review's suggested fix). It was already in the lockfile transitively + via `url`/`reqwest` — zero new compiled crates. Note the crate does + not export a `PATH_SEGMENT` set; the path-segment superset used here + is built locally from `CONTROLS` in `forward.rs`. + ## Summary -> Filled on completion. \ No newline at end of file +> Filled on completion. + +**Commit:** `164a9d7` — `fix(adapters): safe outbound URL construction (FWD-01, FWD-02)`. + +**What changed** (all in `src/adapters/forward.rs`, shared by both +`from_openapi` and `from_jsonschema`): + +- `value_to_path_segment` now percent-encodes with a WHATWG path-set + superset (`PATH_VALUE_ENCODE_SET`: controls, space, `"`, `<`, `>`, + `` ` ``, `#`, `?`, `{`, `}`, `/`, `%`, `\`). Because `%` is in the + set, encoding is idempotent — a value can never contain a bare `%` + that could pair with a following hex to re-open an escape. `/`- and + `\`-bearing values become single literal segments; `../` traversal in + a value is structurally impossible. +- Template rendering is single-pass (`render_path_template`): each + `{name}` placeholder is consumed once from the raw template, so a + rendered value is never re-scanned; a value containing `{later}` is + emitted as encoded text (`%7Blater%7D`), not expanded. Unbound or + unterminated placeholders now error loudly (previously a partially + rendered path was sent upstream, OAI-04's call-time half). +- Input routing no longer keys off "value previously replaced into the + string": `is_path_placeholder` decides by template membership only. + A `body` key never renders into the path; a path param is never + doubled into query. +- `assemble_request_url` appends to the base URL's *directory*: base + path `…/v1` + `/chat/completions` → `…/v1/chat/completions` (the + FWD-02 `Url::join` directory-resolution drop is gone). Percent + escaping is done in two explicit passes (value pass encodes `%`; + segment pass encodes separators between `%`escapes), so the handoff + to `Url::set_path` — which re-encodes with the WHATWG path set but + leaves existing `%xx` escapes alone — is exact, and post-assembly the + URL's origin (scheme + host + effective port) must equal the base's + or the request errors before any credential is attached. +- `parse_base_url` validation at call time: parse error → clean + `CallError`; scheme must be http/https; empty host rejected; embedded + userinfo rejected (credentials flow only via `Capabilities` → + headers, ADR-014, and a `user:pass@` base would put a second, + unencrypted credential on the wire). +- Query encoding unchanged (`query_pairs_mut`); kept its existing test. + +**Tests** (8 new, `forward.rs::tests`, run through `build_request` so +both adapters' shared path is exercised): + +- traversal value `../../admin` stays one segment (`..%2F..%2Fadmin`), + host unchanged, no `/admin` hop +- `?`/`#` values produce no query/fragment; spaces, `/`, `\` encoded; + unicode percent-encoded +- single-pass rendering: `{a} = "{b}"` is not re-substituted +- base-prefix preservation (origin-only and `…/v1` bases) +- absolute-URL values (`http://169.254.169.254/…`, + `https://evil.example.com/…`) remain inert encoded segments on the + base origin (the host-equality/SSRF gate) +- unbound + unterminated placeholders error loudly +- base_url validation: non-http scheme, embedded userinfo, unparseable +- query values remain form-encoded via `query_pairs_mut` + +**Verification:** `cargo test` 238 passed (incl. the 8 new); +`cargo clippy --all-targets -- -D warnings` clean; `cargo fmt --check` +clean. + +**Known edge (documented, not fixed here):** a template-written literal +segment containing `{`/`}` (not a well-formed placeholder) renders as +percent-encoded text rather than erroring; only unterminated `{` +errors. Tightening this is a lint concern, not a traversal risk. \ No newline at end of file