--- id: review-001-forward-url-safety name: Safe outbound URL construction — encoding, base-path, host validation (FWD-01, FWD-02) status: completed depends_on: [] scope: narrow risk: high impact: component level: implementation tags: [adapters, review-001, security] --- ## Description Review 001 findings FWD-01 + FWD-02, both empirically verified against `src/adapters/forward.rs` and both security-bearing **with the operation's injected credentials attached**: - **FWD-01**: path-parameter values are substituted raw (`forward.rs:56-71, 133-141`). `Url::join` normalizes `..`, so `{owner} = "../../admin"` escapes a path-scoped prefix (cross-tenant IDOR); `?` and `#` in a value split/inject into the URL; values containing a later placeholder get re-expanded by the iterative substitution. Fix: percent-encode each segment (`utf8_percent_encode` with a path-segment set), reject/encode `?`/`#`, and render the template in a single pass (not iterative replace). The query path is already correctly encoded via `query_pairs_mut` — keep that shape. - **FWD-02**: `Url::join` resolves against the base *directory*, so `base_url = "https://api.openai.com/v1"` + `/chat/completions` silently drops `/v1` (`forward.rs:74-78`); every test uses origin-only base URLs so the suite can't see it. Worse: a path key that is an absolute URL replaces scheme+host entirely (verified — reqwest only rejects non-http(s) *schemes*), and credential injection happens after URL construction, so a spec-controlled absolute path sends the namespace's credentials to an arbitrary host (SSRF). Specs are assembly-layer-supplied (trusted per ADR-066) but nothing enforces that boundary. Fix: append to the base *path* (not origin), require the joined URL to keep the base host (fail loudly on host change), and require https by default (explicit opt-out for http). ## Acceptance Criteria - [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 - docs/reviews/001-initial-implementation-review.md (Part D, FWD-01, FWD-02) - docs/architecture/decisions/066-from-jsonschema-as-http-adapter.md ## Notes > 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. **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.