diff --git a/docs/architecture/decisions/066-from-jsonschema-as-http-adapter.md b/docs/architecture/decisions/066-from-jsonschema-as-http-adapter.md index e3146da..5ed3a79 100644 --- a/docs/architecture/decisions/066-from-jsonschema-as-http-adapter.md +++ b/docs/architecture/decisions/066-from-jsonschema-as-http-adapter.md @@ -149,6 +149,56 @@ error projection and the success sweep (SSE detection + output-schema selection), where the precedence order is: concrete 2XX statuses, then `2XX`, then `default`. +### Forwarding contract decisions (review 002 FWD-17/18/19) + +The shared forwarding core (`src/adapters/forward.rs`) used by both +`from_openapi` and `from_jsonschema` pins three contracts. Each is a +decision the code half-implied; the module doc carries the full normative +text, this section records the reasoning at the assembly-trust boundary +that ADR-066 establishes: an assembly's spec/template/base-URL inputs +are trusted configuration, while peer call-time input never is. + +**FWD-17 — SSE payload contract (decided: carry raw payload + event +name for non-JSON frames).** A subscription frame's `data:` payload +that is itself valid JSON surfaces as the decoded value (`123` stays a +number, `"123"` stays a string). Any other payload surfaces as the +`{"data": , "event": }` wrapper: a legitimately +non-JSON stream stays field-addressable and an upstream's named-event +conventions (`event: error`) are visible instead of indistinguishable +from data. The `event` value is the frame's `event:` field under +WHATWG last-wins semantics — `null` when absent, never the implicit +`message` default, so "named" and "default" stay distinguishable. JSON +frames surface as themselves even under a named event; consequence: +the event name is only visible on non-JSON frames. Chosen over the +alternative (document the old JSON-or-string contract) because the old +behavior silently erased payload shape and made an upstream's error +convention unobservable — a fidelity loss on the same axis ADR-023 +already rejects for status mapping. + +**FWD-18 — placeholder routing rule (decided: fix + structural-value +error).** A key matching a path-template placeholder is consumed by the +path and never also emits as a query parameter, whatever its value's +shape (the renderer's placeholder check precedes query routing — fixed +behavior, pinned by test). A placeholder renders exactly one literal +path segment, so a structural value (object/array) under a placeholder +key fails with `INVALID_INPUT` rather than splicing minified JSON into +the path. Scalar values (string/number/boolean/null) render +percent-encoded as before. + +**FWD-19 — literal `%` in template text (decided: documented +trade-off, not rejection).** A `%` inside a *value* is always encoded +(`%` → `%25`), so a value can never inject or fake percent escapes. A +`%` in *template or base text* survives verbatim: an assembly writing +`%2F` into a template is presumed to intend a pre-encoded segment for +upstreams that route `%2F` differently from `/`. Inputs are +assembly-supplied (this ADR's trust boundary), so the strict shape +(call-time rejection of raw `%` in template text) is rejected — it +would break legitimate pre-encoded templates without adding safety: +the surviving `%2F` still renders as a single literal segment, cannot +change the origin, and cannot be forged from peer input. The trade-off +(upstream-dependent routing of bare percent escapes) is the +assembly's choice, not this crate's. + ## Consequences **Positive**: diff --git a/src/adapters/forward.rs b/src/adapters/forward.rs index d4e3677..16161a3 100644 --- a/src/adapters/forward.rs +++ b/src/adapters/forward.rs @@ -38,6 +38,29 @@ //! the buffer grows, so the reassembly buffer can never exceed the //! line cap. //! +//! # Streaming payload contract (FWD-17) +//! +//! Each parsed SSE frame becomes one success envelope: +//! +//! - A `data:` payload that is valid JSON surfaces as the decoded value +//! itself — `123` as a number, `"123"` as a string (the raw-text +//! fallback this replaces erased that distinction). This holds even +//! when the frame carries an `event:` name. +//! - Any other payload surfaces as the +//! `{"data": , "event": }` wrapper: the raw +//! text stays field-addressable and an upstream's named-event +//! conventions (`event: error`, `event: ping`, …) are visible on +//! non-JSON frames. The name is the frame's `event:` field (WHATWG +//! last-wins); `null` when the upstream sent none — the spec's +//! implicit `message` default is *not* substituted, so "upstream +//! named it" and "default name" remain distinguishable. +//! - An empty payload is `Null`. +//! +//! Known limitation: a JSON body under a named event surfaces as the +//! decoded value only — the event name is not carried on JSON frames; +//! a non-JSON body under a named event is the only combination where +//! both are visible. +//! //! # Credentials and input routing //! //! The forwarding handler is the no-env-vars credential injection point @@ -56,6 +79,36 @@ //! request header, the declared `GATEWAY_BODY_KEY` property becomes the //! request body, and every other declared key becomes an upstream query //! parameter. +//! +//! # Input routing and path placeholders (FWD-18) +//! +//! A key that matches a `{placeholder}` in the path template is consumed +//! by the path and never also emits as a query parameter, regardless of +//! its value's shape — the placeholder check precedes query routing in +//! the request builder. A placeholder renders exactly one literal path +//! segment, so its value must be a scalar: object/array values fail with +//! `INVALID_INPUT` (structural values have no faithful single-segment +//! rendering; splicing the minified JSON into the path was the +//! pre-decision behavior and is rejected now). +//! +//! # Percent handling in the rendered path (FWD-19) +//! +//! Two different contracts apply, by design: +//! +//! - A `%` arriving inside a *value* is always encoded (`%` → `%25`), +//! so values carrying `%2F` cannot be mistaken for this crate's own +//! escapes and a value can never inject URL structure. +//! - A `%` in *template/base text* survives verbatim. Templates and +//! base URLs are assembly-supplied (ADR-066 trust boundary), so an +//! assembly that writes `/s3%2Fkeys` is presumed to mean a +//! pre-encoded segment for upstreams that route `%2F` differently +//! from `/` — that upstream-semantics choice belongs to the assembly, +//! not this crate. No injection results: the surviving `%2F` still +//! forms a single segment (the origin check plus the two-pass +//! percent-encoding over template text see to that), and the +//! value-side rule above means every bare `%` in a rendered path +//! traces to template text the assembly author wrote. +//! use std::collections::HashMap; use std::sync::Arc; @@ -420,11 +473,26 @@ fn scalar_value_to_string(value: &Value) -> String { /// Percent-encoding scheme used by [`render_path_template`]: a value /// containing `/`, `?`, `#`, or `\` — and any raw `%` it carries — is /// encoded so that a rendered value stays one literal path segment -/// (FWD-01 traversal/ smuggled-segments gate). Path parameters are never -/// rejected: they are rendered safely instead. -pub(crate) fn value_to_path_segment(value: &Value) -> String { - let raw = scalar_value_to_string(value); - utf8_percent_encode(&raw, PATH_VALUE_ENCODE_SET).to_string() +/// (FWD-01 traversal/ smuggled-segments gate). Scalars are never +/// rejected: they are rendered safely instead. Object/array values +/// under a placeholder key are rejected with `INVALID_INPUT` (FWD-18): +/// a path placeholder designates exactly one literal segment, so a +/// structural value has no faithful rendering — the pre-decision +/// behavior spliced the minified JSON into the path *and* emitted the +/// key as a query parameter (double-routed, neither faithful). +pub(crate) fn value_to_path_segment(value: &Value) -> Result { + let raw = match value { + Value::String(_) | Value::Number(_) | Value::Bool(_) | Value::Null => { + scalar_value_to_string(value) + } + other => { + return Err(CallError::invalid_input(format!( + "path placeholder value must be a scalar (string, number, boolean, or null); got {}: structural values have no faithful single-segment rendering", + type_name_of(other) + ))) + } + }; + Ok(utf8_percent_encode(&raw, PATH_VALUE_ENCODE_SET).to_string()) } /// Raw form of a scalar input value for query emission: `&`/`=` @@ -469,7 +537,7 @@ pub(crate) fn render_path_template( continue; } }; - out.push_str(&value_to_path_segment(raw_value)); + out.push_str(&value_to_path_segment(raw_value)?); rest = &tail[end + 1..]; } out.push_str(rest); @@ -804,13 +872,29 @@ async fn error_envelope( ResponseEnvelope::error(request_id, CallError::new(code, message, false)) } -/// Converts a parsed SSE event into a response envelope, JSON-decoding -/// the data payload when possible. +/// Maps a parsed SSE frame to a response envelope under the FWD-17 +/// payload contract: a payload that is valid JSON surfaces as the +/// decoded value itself (a number payload stays a number, a +/// quoted-string payload stays a string — the distinction a raw-text +/// fallback would erase); any other payload surfaces as the +/// `{"data": , "event": }` wrapper, so +/// a non-JSON stream is field-addressable and an upstream's +/// named-event conventions are visible on it. An empty payload is +/// `Null`. JSON payloads intentionally carry no `event` binding: a +/// JSON frame surfaces as itself (shape stability for JSON-first +/// upstreams, and the spec's implicit `event: message` default never +/// wraps a payload). fn sse_event_envelope(event: SseEvent, request_id: &str) -> ResponseEnvelope { let parsed = if event.data.trim().is_empty() { Value::Null } else { - serde_json::from_str(&event.data).unwrap_or(Value::String(event.data.clone())) + match serde_json::from_str::(&event.data) { + Ok(value) => value, + Err(_) => serde_json::json!({ + "data": event.data, + "event": event.event, + }), + } }; ResponseEnvelope::ok(request_id, parsed) } @@ -1022,9 +1106,14 @@ fn is_sse_content_type(content_type: &str) -> bool { content_type.split(';').next().unwrap_or_default().trim() == "text/event-stream" } -/// A parsed SSE event: the `data:` lines joined with `\n`. +/// A parsed SSE event: the `data:` lines joined with `\n`, plus the +/// frame's `event:` field name when the upstream sent one (WHATWG +/// event-stream semantics: the last `event:` line before the blank +/// line wins; absent → `None`, never the implicit `message` default — +/// consumers distinguish "upstream named it" from "default name"). pub(crate) struct SseEvent { pub(crate) data: String, + pub(crate) event: Option, } #[derive(Debug, thiserror::Error)] @@ -1053,13 +1142,16 @@ pub(crate) const SSE_EVENT_BUFFER_CAP: usize = 1024 * 1024; /// a chunk boundary is therefore not corrupted. Framing follows the /// WHATWG `text/event-stream` draft semantics for what the call /// protocol needs: lines split on `\n` with optional trailing `\r`; -/// `data:` fields accumulate and join with `\n`; `event:`, `id:`, and +/// `data:` fields accumulate and join with `\n`; `event:` is captured +/// per frame (last one before the blank line wins) and surfaced on +/// [`SseEvent::event`] for non-JSON payloads (FWD-17); `id:` and /// `retry:` fields are accepted and ignored; a blank line dispatches /// the pending event; a pending event with data is dispatched at EOF. pub(crate) struct SseParser { buf: Vec, data_lines: Vec, data_seen: bool, + event_name: Option, bom_stripped: bool, } @@ -1069,6 +1161,7 @@ impl SseParser { buf: Vec::new(), data_lines: Vec::new(), data_seen: false, + event_name: None, bom_stripped: false, } } @@ -1135,6 +1228,7 @@ impl SseParser { return if self.data_seen { self.complete_event() } else { + self.discard_event(); None }; } @@ -1148,6 +1242,8 @@ impl SseParser { if field == "data" { self.data_lines.push(value.to_string()); self.data_seen = true; + } else if field == "event" && !value.is_empty() { + self.event_name = Some(value.to_string()); } None } @@ -1156,8 +1252,15 @@ impl SseParser { self.data_seen = false; Some(SseEvent { data: std::mem::take(&mut self.data_lines).join("\n"), + event: self.event_name.take(), }) } + + fn discard_event(&mut self) { + self.data_seen = false; + self.event_name = None; + self.data_lines.clear(); + } } #[cfg(test)] @@ -1476,6 +1579,114 @@ mod tests { assert!(q.contains("q=rust") && q.contains("debug=true"), "{q}"); } + /// FWD-18, routing half pin: a key that matched a placeholder never + /// also emits as a query parameter — the skip in `build_request` + /// precedes query routing and is value-shape-agnostic. The scalar + /// case renders into the path only; the object-value case is + /// rejected (see the structural-value test below), so here the + /// companion non-placeholder key proves query routing stays intact + /// around the placeholder skip. + #[test] + fn placeholder_keys_never_double_route_as_query_params() { + let url = request_url( + "https://api.example.com", + "/repos/{owner}", + json!({"owner": "octocat", "extra": "q-val"}), + ) + .expect("scalar placeholder builds"); + assert_eq!(url.path(), "/repos/octocat"); + assert_eq!( + url.query(), + Some("extra=q-val"), + "only the non-placeholder key routes to query" + ); + + let ctx = noop_context(); + let schema = json!({ + "type": "object", + "properties": {"id": {"type": "string"}}, + }); + let err = build_request( + "https://api.example.com", + "/items/{id}", + "GET", + &None, + &TestHashMap::new(), + "svc", + &schema, + &json!({"id": "x", "undeclared": "v"}), + &ctx, + ) + .expect_err("undeclared key still rejected with a placeholder present"); + assert_eq!(err.code, "INVALID_INPUT"); + } + + /// FWD-18, structural-value decision: an object/array value under a + /// placeholder key is an `INVALID_INPUT` error, not a spliced JSON + /// segment. A path placeholder designates exactly one literal + /// segment; the pre-decision behavior embedded the minified JSON + /// into the path (and, under an additionalProperties schema, also + /// emitted the key as a query param — the double-route). + #[test] + fn object_or_array_path_values_error_instead_of_splicing_json_into_the_path() { + for value in [json!({"a": 1}), json!([1, 2])] { + let err = request_url( + "https://api.example.com", + "/things/{id}", + json!({"id": value}), + ) + .expect_err("structural path values must be rejected"); + assert_eq!(err.code, "INVALID_INPUT", "value was: {value}"); + assert!( + err.message.contains("must be a scalar"), + "value was: {value}" + ); + assert!( + !err.message.contains("{\"a\":1}") && !err.message.contains("[1,2]"), + "error must not echo the structural value: {value}" + ); + } + + for value in [json!("text"), json!(42), json!(true)] { + let url = request_url( + "https://api.example.com", + "/things/{id}", + json!({ "id": value }), + ) + .expect("scalar path values still render"); + assert!( + url.path().starts_with("/things/"), + "scalar {value} renders into the path" + ); + } + } + + /// FWD-19 pin (documented trade-off, not a rejection): literal `%` + /// in template text is preserved as-is, so an assembly-supplied + /// pre-encoded template like `/s3%2Fkeys` reaches the upstream + /// verbatim (most stacks route `%2F` differently from `/` — the + /// assembly layer owns that choice, ADR-066). A `%` arriving in a + /// *value* is always encoded to `%25`, so the only bare `%` in a + /// rendered path is one the template author placed. + #[test] + fn percent_in_template_text_survives_and_percent_in_values_are_always_encoded() { + let url = request_url( + "https://api.example.com", + "/s3%2Fkeys/{name}", + json!({"name": "x"}), + ) + .expect("template text may carry literal percent escapes"); + assert_eq!(url.path(), "/s3%2Fkeys/x"); + + let url = request_url( + "https://api.example.com", + "/files/{name}", + json!({"name": "a%2Fb"}), + ) + .expect("value percent is encoded, not preserved"); + assert_eq!(url.path(), "/files/a%252Fb"); + } + fn ctx_with_capability(namespace: &str, value: String) -> OperationContext { let mut ctx = noop_context(); ctx.capabilities = Capabilities::new().with_http_token(namespace, value); @@ -1781,6 +1992,114 @@ mod tests { assert!(envelopes[1].result.is_ok()); } + /// FWD-17: a valid-JSON payload surfaces as the decoded value + /// itself; a non-JSON payload surfaces as the + /// `{"data", "event"}` wrapper. Numbers-vs-quoted-strings are both + /// valid JSON, so both decode (the raw-text fallback would have + /// erased that distinction). + #[tokio::test] + async fn sse_payload_contract_decodes_json_and_wraps_non_json() { + let base = spawn_responder(TestArc::new(|_parts| { + http_response( + 200, + "text/event-stream", + b"data: 123\n\ndata: \"123\"\n\ndata: plain text\n\ndata: {\"n\":1}\n\n".to_vec(), + ) + })) + .await; + let stream = forward_stream( + &minimal_client(), + &base, + "/x", + "GET", + &None, + &TestHashMap::new(), + "svc", + &serde_json::json!({"type": "object"}), + &[], + json!({}), + noop_context(), + ); + let envelopes = collect_stream(stream).await; + assert_eq!(envelopes.len(), 4); + assert_eq!(envelopes[0].result.clone().unwrap(), json!(123)); + assert_eq!(envelopes[1].result.clone().unwrap(), json!("123")); + assert_eq!( + envelopes[2].result.clone().unwrap(), + json!({"data": "plain text", "event": null}) + ); + assert_eq!(envelopes[3].result.clone().unwrap(), json!({"n": 1})); + } + + /// FWD-17: an upstream `event:` field on a non-JSON payload is + /// carried in the wrapper; JSON payloads surface as themselves even + /// under a named event, and the name is reset after dispatch (the + /// second frame must not inherit the first frame's name). + #[tokio::test] + async fn sse_event_field_carrys_on_the_non_json_wrapper_and_resets() { + let base = spawn_responder(TestArc::new(|_parts| { + http_response( + 200, + "text/event-stream", + b"event: error\ndata: upstream exploded\n\nevent: custom\ndata: {\"n\":1}\n\ndata: after\n\n" + .to_vec(), + ) + })) + .await; + let stream = forward_stream( + &minimal_client(), + &base, + "/x", + "GET", + &None, + &TestHashMap::new(), + "svc", + &serde_json::json!({"type": "object"}), + &[], + json!({}), + noop_context(), + ); + let envelopes = collect_stream(stream).await; + assert_eq!(envelopes.len(), 3); + assert_eq!( + envelopes[0].result.clone().unwrap(), + json!({"data": "upstream exploded", "event": "error"}) + ); + assert_eq!(envelopes[1].result.clone().unwrap(), json!({"n": 1})); + assert_eq!( + envelopes[2].result.clone().unwrap(), + json!({"data": "after", "event": null}), + "the name must not leak across frames" + ); + } + + /// FWD-17 parser pin: WHATWG last-wins for repeated `event:` lines + /// in one frame; an `event:`-only frame (no data) dispatches + /// nothing but must not leak its name into a later frame. + #[test] + fn sse_parser_last_event_wins_and_name_does_not_leak_across_frames() { + let mut parser = SseParser::new(); + let events = parser + .feed(b"event: a\nevent: b\ndata: x\n\n", false) + .expect("ascii only"); + assert_eq!(events.len(), 1); + assert_eq!(events[0].event.as_deref(), Some("b")); + assert_eq!(events[0].data, "x"); + + let events = parser + .feed(b"event: orphan\n\n", false) + .expect("ascii only"); + assert!(events.is_empty(), "no data field, nothing dispatched"); + + let events = parser.feed(b"data: next\n\n", false).expect("ascii only"); + assert_eq!(events.len(), 1); + assert_eq!( + events[0].event.as_deref(), + None, + "an event:-only frame must not leak its name" + ); + } + #[tokio::test] async fn bearer_credential_with_control_character_fails_loudly() { let base = spawn_responder(TestArc::new(|_parts| {