docs(tasks): review-001 forward-url-safety remediation complete

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)
+- [x] Traversal test: `{owner} = "../../admin"` cannot escape the template path (segment encoded or rejected)
-- [ ] `?`/`#`/later-placeholder-in-value tested (encoded or rejected, never URL-structural)
+- [x] `?`/`#`/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)
+- [x] 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)
+- [x] 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)
+- [x] Rendering is single-pass (a rendered value is never re-substituted)
-- [ ] Both `from_openapi` and `from_jsonschema` paths covered (they share `forward.rs`)
+- [x] Both `from_openapi` and `from_jsonschema` paths covered (they share `forward.rs`)
-- [ ] `cargo test` and `cargo clippy --all-targets -- -D warnings` pass
+- [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.
+> 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.