diff --git a/src/adapters/forward.rs b/src/adapters/forward.rs index a8f397e..9f855cb 100644 --- a/src/adapters/forward.rs +++ b/src/adapters/forward.rs @@ -9,6 +9,18 @@ //! (ADR-014): it reads `OperationContext.capabilities`, never //! `std::env::var`. Imported error codes are `HTTP_` to avoid //! collision with the protocol-level codes (ADR-023). +//! +//! Input-schema enforcement (review 001 OAI-02): every input key must be +//! declared by the operation's `input_schema` or consumed by a path +//! template placeholder; undeclared keys are rejected with `INVALID_INPUT` +//! at call time. There is no pass-through knob — a request the forwarder +//! sends must match what `/schema` advertises, so peer-supplied input +//! cannot add upstream query parameters, headers, or craft a request body +//! the contract does not declare. Declared keys route by designation: a +//! property marked [`HEADER_PARAM_IN_MARKER`]` = "header"` is sent as a +//! request header, the declared [`GATEWAY_BODY_KEY`] property becomes the +//! request body, and every other declared key becomes an upstream query +//! parameter. use std::collections::HashMap; use std::sync::Arc; @@ -40,12 +52,33 @@ pub(crate) const RESPONSE_BODY_CAP: usize = 16 * 1024 * 1024; /// that would otherwise be discarded (FWD-10). pub(crate) const ERROR_BODY_ECHO_CAP: usize = 4096; +// Fractional-path helpers and body echo caps follow; the gateway body key +// and the header-param wire marker used by the input routing live with the +// other request-construction defaults here. + /// Upper bound on how much of a non-2xx upstream body is read, both for /// the bounded echo and for connection reuse. A larger error body is /// truncated in the surfaced message and the connection is dropped — /// a deliberate trade of pool reuse against unbounded drain time (FWD-10). const STATUS_BODY_DRAIN: usize = 64 * 1024; +/// The input key that carries the request body on the gateway shape +/// (ADR-047). Declared by `from_openapi`'s generated input schemas +/// (OAI-07 rejects a spec parameter with the same name at import) and +/// consumed by [`build_request`] ahead of the query-parameter routing. +pub(crate) const GATEWAY_BODY_KEY: &str = "body"; + +/// Property-level marker inside an operation's `input_schema` properties +/// that routes the declared value into an HTTP request header instead of +/// the query string (review 001 OAI-03). `from_openapi` stamps +/// `"wire": "header"` on properties generated from `in: header` +/// parameters; `from_jsonschema` callers can mark their declared +/// properties the same way. Unmarked properties default to query +/// placement, preserving the pre-OAI-03 wire behavior for +/// `in: query`/`in: path` parameters. +pub(crate) const HEADER_PARAM_IN_MARKER: &str = "wire"; +pub(crate) const HEADER_PARAM_MARKER_VALUE: &str = "header"; + #[derive(Clone)] pub enum HttpAuthScheme { Bearer, @@ -68,27 +101,39 @@ pub(crate) fn build_request( auth_scheme: &Option, default_headers: &HashMap, namespace: &str, + input_schema: &Value, input: &Value, context: &OperationContext, ) -> Result<(Method, Url, Option, HeaderMap), CallError> { - let inputs = input.as_object(); + let inputs = input.as_object().ok_or_else(|| { + CallError::invalid_input(format!( + "input must be a JSON object (got {}); adapter operations take a named-key input", + type_name_of(input) + )) + })?; + + enforce_input_schema(input_schema, inputs)?; let mut query_params: Vec<(String, String)> = Vec::new(); + let mut header_params: Vec<(String, String)> = Vec::new(); + let param_locations = param_locations(input_schema); let mut body: Option = None; - if let Some(obj) = inputs { - for (key, value) in obj { - if is_path_placeholder(key, path_template) { - continue; - } - if key == "body" { - body = Some(value.clone()); - } else { - query_params.push((key.clone(), value_to_query(value))); - } + for (key, value) in inputs { + if is_path_placeholder(key, path_template) { + continue; + } + if key == GATEWAY_BODY_KEY { + body = Some(value.clone()); + continue; + } + if param_locations.get(key.as_str()) == Some(&ParamLocation::Header) { + header_params.push((key.clone(), value_to_query(value))); + } else { + query_params.push((key.clone(), value_to_query(value))); } } - let rendered_path = render_path_template(path_template, inputs)?; + let rendered_path = render_path_template(path_template, Some(inputs))?; let mut url = assemble_request_url(base_url, &rendered_path)?; if !query_params.is_empty() { let mut pairs = url.query_pairs_mut(); @@ -98,6 +143,19 @@ pub(crate) fn build_request( } let mut headers = HeaderMap::new(); + for (k, v) in &header_params { + let name = HeaderName::try_from(k.as_str()).map_err(|_| { + CallError::internal(format!( + "declared header parameter `{k}` is not a valid HTTP header name; refusing to build a request that silently drops it" + )) + })?; + let value = HeaderValue::try_from(v.as_str()).map_err(|_| { + CallError::internal(format!( + "declared header parameter `{k}` has an invalid value (control or non-ASCII bytes are not permitted); refusing to build a request that silently drops it" + )) + })?; + headers.insert(name, value); + } for (k, v) in default_headers { let name = HeaderName::try_from(k.as_str()).map_err(|_| { CallError::internal(format!( @@ -161,6 +219,92 @@ pub(crate) fn build_request( Ok((http_method, url, body, headers)) } +/// Upstream placement of a declared input property (review 001 OAI-03). +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +enum ParamLocation { + Query, + Header, +} + +/// Map of declared property name → upstream placement, read from the +/// `input_schema`'s `properties` entries via [`HEADER_PARAM_IN_MARKER`] +/// (query placement is the default). +fn param_locations(input_schema: &Value) -> HashMap<&str, ParamLocation> { + let mut out = HashMap::new(); + let Some(properties) = input_schema.get("properties").and_then(|p| p.as_object()) else { + return out; + }; + for (name, schema) in properties { + let is_header = schema + .get(HEADER_PARAM_IN_MARKER) + .and_then(|w| w.as_str()) + .is_some_and(|w| w == HEADER_PARAM_MARKER_VALUE); + out.insert( + name.as_str(), + if is_header { + ParamLocation::Header + } else { + ParamLocation::Query + }, + ); + } + out +} + +fn type_name_of(value: &Value) -> &'static str { + match value { + Value::Null => "null", + Value::Bool(_) => "a boolean", + Value::Number(_) => "a number", + Value::String(_) => "a string", + Value::Array(_) => "an array", + Value::Object(_) => "an object", + } +} + +/// Input-schema enforcement at call time (review 001 OAI-02). +/// +/// Every input key must be declared by the input schema's `properties` — +/// including the gateway body property, which `from_openapi` declares as +/// `body` whenever the operation has a requestBody, and including the +/// path-consumed placeholders, which `from_openapi` also declares. An +/// explicit `"additionalProperties": true` opts the operation into +/// catch-all input (JSON Schema semantics: the schema advertises that +/// extra properties are valid), which `from_jsonschema` callers can use +/// for open-shaped endpoints. Anything else undeclared is a rejected +/// `INVALID_INPUT` rather than a silently-added upstream query parameter, +/// so peer input like `{debug: true}` or `{impersonate_id: …}` cannot +/// decorate an upstream request the contract does not advertise. +fn enforce_input_schema( + input_schema: &Value, + inputs: &serde_json::Map, +) -> Result<(), CallError> { + let declared = input_schema + .get("properties") + .and_then(|p| p.as_object()) + .map(|p| p.keys().cloned().collect::>()) + .unwrap_or_default(); + let catch_all = input_schema.get("additionalProperties") == Some(&Value::Bool(true)); + let unknown: Vec = inputs + .keys() + .filter(|key| !catch_all && !declared.iter().any(|d| d == *key)) + .cloned() + .collect(); + if let Some(first) = unknown.first() { + let declared_list = if declared.is_empty() { + "none".to_string() + } else { + declared.join(", ") + }; + return Err(CallError::invalid_input(format!( + "input key `{first}` is not declared by the operation's input schema \ + (declared: {declared_list}); undeclared keys are rejected so peer \ + input cannot shape the upstream request beyond the advertised contract" + ))); + } + Ok(()) +} + /// Percent-encode set for a spliced path-parameter value: the WHATWG /// path set (controls, space, `"`, `<`, `>`, `` ` ``, `#`, `?`, `{`, `}`) /// plus `/` so a value stays one literal segment, plus `%` `?` `#` @@ -502,6 +646,7 @@ pub(crate) async fn forward( auth_scheme: &Option, default_headers: &HashMap, namespace: &str, + input_schema: &Value, error_status_codes: &[(u16, String)], input: Value, context: OperationContext, @@ -515,6 +660,7 @@ pub(crate) async fn forward( auth_scheme, default_headers, namespace, + input_schema, &input, &context, ) { @@ -620,6 +766,7 @@ pub(crate) fn forward_stream( auth_scheme: &Option, default_headers: &HashMap, namespace: &str, + input_schema: &Value, error_status_codes: &[(u16, String)], input: Value, context: OperationContext, @@ -633,6 +780,7 @@ pub(crate) fn forward_stream( auth_scheme, default_headers, namespace, + input_schema, &input, &context, ) { @@ -973,6 +1121,7 @@ mod tests { &None, &TestHashMap::new(), "svc", + &serde_json::json!({"type": "object", "additionalProperties": true}), &input, &ctx, )?; @@ -1116,6 +1265,115 @@ mod tests { assert_eq!(url.query(), Some("lang=en&q=a%26b%3Dc+d")); } + #[test] + fn undeclared_input_keys_are_rejected_not_sent_upstream() { + let ctx = noop_context(); + let schema = json!({ + "type": "object", + "properties": {"owner": {"type": "string"}, "body": {"type": "object"}}, + }); + for input in [ + json!({"owner": "a", "debug": "true"}), + json!({"owner": "a", "impersonate_id": "x"}), + json!({"debug": "true"}), + ] { + let err = build_request( + "https://api.example.com", + "/x/{owner}", + "GET", + &None, + &TestHashMap::new(), + "svc", + &schema, + &input, + &ctx, + ) + .expect_err("undeclared peer input must be rejected"); + assert_eq!(err.code, "INVALID_INPUT", "input was: {input}"); + assert!(err.message.contains("not declared"), "input was: {input}"); + } + } + + #[test] + fn declared_body_and_header_params_route_off_the_query_string() { + let ctx = noop_context(); + let schema = json!({ + "type": "object", + "properties": { + "q": {"type": "string"}, + "X-Trace-Id": {"type": "string", "wire": "header"}, + "body": {"type": "object"}, + }, + }); + let (_, url, body, headers) = build_request( + "https://api.example.com", + "/search", + "POST", + &None, + &TestHashMap::new(), + "svc", + &schema, + &json!({"q": "rust", "X-Trace-Id": "t-1", "body": {"page": 2}}), + &ctx, + ) + .expect("declared input builds"); + assert_eq!(url.query(), Some("q=rust")); + assert_eq!( + headers + .get("x-trace-id") + .expect("header param sent as header") + .to_str() + .expect("ascii header"), + "t-1" + ); + assert_eq!(body, Some(json!({"page": 2}))); + } + + #[test] + fn non_object_input_is_rejected() { + let ctx = noop_context(); + let schema = json!({"type": "object", "properties": {"q": {"type": "string"}}}); + for input in [json!(null), json!([1]), json!("str"), json!(42)] { + let err = build_request( + "https://api.example.com", + "/x", + "GET", + &None, + &TestHashMap::new(), + "svc", + &schema, + &input, + &ctx, + ) + .expect_err("non-object input must be rejected"); + assert_eq!(err.code, "INVALID_INPUT"); + } + } + + #[test] + fn additional_properties_true_opts_into_catch_all_input() { + let ctx = noop_context(); + let schema = json!({ + "type": "object", + "properties": {"q": {"type": "string"}}, + "additionalProperties": true, + }); + let (_, url, _, _) = build_request( + "https://api.example.com", + "/search", + "GET", + &None, + &TestHashMap::new(), + "svc", + &schema, + &json!({"q": "rust", "debug": "true"}), + &ctx, + ) + .expect("catch-all input builds"); + let q = url.query().expect("query present"); + assert!(q.contains("q=rust") && q.contains("debug=true"), "{q}"); + } + fn ctx_with_capability(namespace: &str, value: String) -> OperationContext { let mut ctx = noop_context(); ctx.capabilities = Capabilities::new().with_http_token(namespace, value); @@ -1228,6 +1486,7 @@ mod tests { auth_scheme, &TestHashMap::new(), "svc", + &serde_json::json!({"type": "object"}), &[], json!({}), ctx, @@ -1310,6 +1569,7 @@ mod tests { &None, &TestHashMap::new(), "svc", + &serde_json::json!({"type": "object"}), &[], json!({}), noop_context(), @@ -1348,6 +1608,7 @@ mod tests { &None, &TestHashMap::new(), "svc", + &serde_json::json!({"type": "object"}), &[], json!({}), noop_context(), @@ -1399,6 +1660,7 @@ mod tests { }), &TestHashMap::new(), "svc", + &serde_json::json!({"type": "object"}), &json!({}), &ctx, ); @@ -1422,6 +1684,7 @@ mod tests { &None, &defaults, "svc", + &serde_json::json!({"type": "object"}), &json!({}), &noop_context(), ) @@ -1441,6 +1704,7 @@ mod tests { &None, &defaults, "svc", + &serde_json::json!({"type": "object"}), &json!({}), &noop_context(), ) diff --git a/src/adapters/from_jsonschema.rs b/src/adapters/from_jsonschema.rs index 6695ddd..8970b6a 100644 --- a/src/adapters/from_jsonschema.rs +++ b/src/adapters/from_jsonschema.rs @@ -5,8 +5,17 @@ //! One `HandlerRegistration` per call with a reqwest forwarding handler //! (the shared [`super::forward`] code path with `from_openapi`). //! Provenance is `FromJsonSchema` (leaf, `composition_authority: None`, -//! `scoped_env: None`, `Internal` by default — ADR-015/022). `Sub` op -//! type → `HandlerKind::Stream` expecting `text/event-stream` (ADR-049). +//! `scoped_env: None`). The registered operation's visibility is forced +//! to `Internal` (ADR-015: adapter-registered ops are composition +//! material) — the caller's spec is not passed through verbatim. +//! Construction validates the method, path template, and base URL +//! eagerly (review 001 OAI-09), and the spec's declared path-template +//! placeholders must be bound by the input schema. `Sub` op type → +//! `HandlerKind::Stream` expecting `text/event-stream` (ADR-049). +//! +//! The spec's `input_schema` is the forwarding allow-list (review 001 +//! OAI-02, enforced in `super::forward`); see [`FromJsonSchema::new`] +//! for the `wire: "header"` declaration (OAI-03). //! //! [ADR-066]: https://docs.rs/alkhttp (docs/architecture/decisions) @@ -18,8 +27,9 @@ use alkcall::registry::context::OperationContext; use alkcall::registry::registration::{ make_handler, make_streaming_handler, HandlerKind, HandlerRegistration, OperationProvenance, }; -use alkcall::registry::spec::{OperationSpec, OperationType}; +use alkcall::registry::spec::{OperationSpec, OperationType, Visibility}; use async_trait::async_trait; +use reqwest::Method; use serde_json::Value; use super::forward::{forward, forward_stream, HttpServiceConfig}; @@ -34,21 +44,143 @@ pub struct FromJsonSchema { } impl FromJsonSchema { + /// Register a caller-supplied [`OperationSpec`] backed by a single + /// HTTP endpoint. + /// + /// Construction validates eagerly (review 001 OAI-09): the HTTP + /// method must parse, the path template must be well-formed (balanced + /// `{}` placeholders, no empty name), and the base URL must parse as + /// an HTTP(S) origin without userinfo — malformed values fail here + /// instead of surfacing as a per-call `INTERNAL` error on first + /// invoke. The registered operation's visibility is forced to + /// `Internal` (ADR-015: adapter-registered ops are composition + /// material; the caller's spec is not passed through verbatim). + /// + /// The spec's `input_schema` is also the forwarding allow-list + /// (review 001 OAI-02): input keys not declared in its `properties` + /// are rejected at call time. Mark a property with + /// `"wire": "header"` to route it as an upstream HTTPS header + /// instead of a query parameter (review 001 OAI-03). Input key + /// [`GATEWAY_BODY_KEY`](super::forward::GATEWAY_BODY_KEY) carries + /// the request body. pub fn new( spec: OperationSpec, config: HttpServiceConfig, path_template: String, method: String, http_client: Arc, - ) -> Self { - Self { + ) -> Result { + validate_method(&method)?; + validate_path_template(&path_template)?; + Self::validate_base_url(&config.base_url)?; + if spec_name_references_undeclared(&spec, &path_template) { + return Err(AdapterError::SchemaParse { + message: format!( + "path template `{path_template}` references a placeholder the \ + operation's input_schema does not declare; the forwarded call \ + could never satisfy it" + ), + }); + } + if config.base_url.is_empty() { + return Err(AdapterError::SchemaParse { + message: "base_url must not be empty".into(), + }); + } + let spec = OperationSpec { + visibility: Visibility::Internal, + ..spec + }; + Ok(Self { spec, config, path_template, method, http_client, - } + }) } + + fn validate_base_url(base_url: &str) -> Result<(), AdapterError> { + let parsed = url::Url::parse(base_url).map_err(|e| AdapterError::SchemaParse { + message: format!("invalid base_url `{base_url}`: {e}"), + })?; + let scheme = parsed.scheme(); + if scheme != "https" && scheme != "http" { + return Err(AdapterError::SchemaParse { + message: format!("base_url `{base_url}` must be an http(s) URL; `{scheme}` is not"), + }); + } + if !parsed.username().is_empty() || parsed.password().is_some() { + return Err(AdapterError::SchemaParse { + message: format!( + "base_url `{base_url}` must not embed userinfo; credentials are \ + injected per-call from Capabilities" + ), + }); + } + Ok(()) + } +} + +fn validate_method(method: &str) -> Result<(), AdapterError> { + if method.is_empty() { + return Err(AdapterError::SchemaParse { + message: "HTTP method must not be empty".into(), + }); + } + Method::from_bytes(method.to_ascii_uppercase().as_bytes()).map_err(|_| { + AdapterError::SchemaParse { + message: format!("invalid HTTP method `{method}`"), + } + })?; + Ok(()) +} + +fn validate_path_template(path_template: &str) -> Result<(), AdapterError> { + let mut rest = path_template; + while let Some(start) = rest.find('{') { + let Some(end_rel) = rest[start..].find('}') else { + return Err(AdapterError::SchemaParse { + message: format!("path template `{path_template}` has an unterminated placeholder"), + }); + }; + if rest[start + 1..start + end_rel].is_empty() { + return Err(AdapterError::SchemaParse { + message: format!("path template `{path_template}` has an empty placeholder name"), + }); + } + rest = &rest[start + end_rel + 1..]; + } + if path_template.contains('}') && !path_template.contains('{') { + return Err(AdapterError::SchemaParse { + message: format!("path template `{path_template}` has `}}` without a matching `{{`"), + }); + } + Ok(()) +} + +fn spec_name_references_undeclared(spec: &OperationSpec, path_template: &str) -> bool { + let properties = spec + .input_schema + .get("properties") + .and_then(|p| p.as_object()) + .map(|p| p.keys().cloned().collect::>()) + .unwrap_or_default(); + if properties.is_empty() { + return false; + } + let mut rest = path_template; + while let Some(start) = rest.find('{') { + let Some(end_rel) = rest[start..].find('}') else { + return false; + }; + let name = &rest[start + 1..start + end_rel]; + if !properties.iter().any(|k| k == name) { + return true; + } + rest = &rest[start + end_rel + 1..]; + } + false } #[async_trait] @@ -62,6 +194,7 @@ impl OperationAdapter for FromJsonSchema { let namespace = self.config.namespace.clone(); let http_client = Arc::clone(&self.http_client); let op_type = self.spec.op_type; + let input_schema = self.spec.input_schema.clone(); let error_status_codes: Vec<(u16, String)> = self .spec @@ -81,6 +214,7 @@ impl OperationAdapter for FromJsonSchema { let namespace = namespace.clone(); let http_client = Arc::clone(&http_client); let error_status_codes = error_status_codes.clone(); + let input_schema = input_schema.clone(); forward_stream( &http_client, &base_url, @@ -89,6 +223,7 @@ impl OperationAdapter for FromJsonSchema { &auth_scheme, &default_headers, &namespace, + &input_schema, &error_status_codes, input, context, @@ -105,6 +240,7 @@ impl OperationAdapter for FromJsonSchema { let namespace = namespace.clone(); let http_client = Arc::clone(&http_client); let error_status_codes = error_status_codes.clone(); + let input_schema = input_schema.clone(); async move { forward( &http_client, @@ -114,6 +250,7 @@ impl OperationAdapter for FromJsonSchema { &auth_scheme, &default_headers, &namespace, + &input_schema, &error_status_codes, input, context, @@ -239,7 +376,8 @@ mod tests { "/widgets".to_string(), "GET".to_string(), test_http_client(), - ); + ) + .unwrap(); let bundles = adapter.import().await.unwrap(); assert_eq!(bundles.len(), 1); assert_eq!(bundles[0].spec.name, "svc/getWidget"); @@ -256,7 +394,8 @@ mod tests { "/widgets".to_string(), "GET".to_string(), test_http_client(), - ); + ) + .unwrap(); let bundles = adapter.import().await.unwrap(); assert!(matches!(bundles[0].handler, HandlerKind::Once(_))); } @@ -269,7 +408,8 @@ mod tests { "/stream".to_string(), "POST".to_string(), test_http_client(), - ); + ) + .unwrap(); let bundles = adapter.import().await.unwrap(); assert!(matches!(bundles[0].handler, HandlerKind::Stream(_))); } @@ -282,7 +422,8 @@ mod tests { "/widgets".to_string(), "POST".to_string(), test_http_client(), - ); + ) + .unwrap(); let bundles = adapter.import().await.unwrap(); assert!(matches!(bundles[0].handler, HandlerKind::Once(_))); } @@ -298,6 +439,7 @@ mod tests { &Some(HttpAuthScheme::Bearer), &HashMap::new(), "github", + &serde_json::json!({"type": "object", "additionalProperties": true}), &serde_json::json!({"owner":"a","repo":"b"}), &ctx, ) @@ -319,6 +461,7 @@ mod tests { &None, &HashMap::new(), "svc", + &serde_json::json!({"type": "object", "additionalProperties": true}), &serde_json::json!({"id":42,"filter":"active"}), &ctx, ) @@ -371,7 +514,8 @@ mod tests { "/data".to_string(), "GET".to_string(), test_http_client(), - ); + ) + .unwrap(); let bundles = adapter.import().await.unwrap(); let registration = &bundles[0]; let ctx = noop_context("req-10", Capabilities::new()); @@ -401,7 +545,8 @@ mod tests { "/missing".to_string(), "GET".to_string(), test_http_client(), - ); + ) + .unwrap(); let bundles = adapter.import().await.unwrap(); let registration = &bundles[0]; let ctx = noop_context("req-11", Capabilities::new()); @@ -427,7 +572,8 @@ mod tests { "/x".to_string(), "GET".to_string(), test_http_client(), - ); + ) + .unwrap(); let bundles = adapter.import().await.unwrap(); let registration = &bundles[0]; let ctx = noop_context("req-12", Capabilities::new()); @@ -451,7 +597,8 @@ mod tests { "/stream".to_string(), "POST".to_string(), test_http_client(), - ); + ) + .unwrap(); let bundles = adapter.import().await.unwrap(); let registration = &bundles[0]; let ctx = noop_context("req-13", Capabilities::new()); @@ -478,6 +625,7 @@ mod tests { &Some(HttpAuthScheme::Bearer), &HashMap::new(), "openai", + &serde_json::json!({"type": "object", "additionalProperties": true}), &serde_json::json!({"body":{"prompt":"hi"}}), &ctx, ) @@ -488,4 +636,86 @@ mod tests { ); std::env::remove_var("OPENAI_API_KEY"); } + + #[test] + fn malformed_method_path_template_and_base_url_fail_at_construction() { + for (method, path_template, base_url, expected_fragment) in [ + ("NOT/*A METHOD", "/x", "https://x", "invalid HTTP method"), + ("GET", "/x/{open", "https://x", "unterminated placeholder"), + ("GET", "/x/}", "https://x", "without a matching"), + ("GET", "/x", "https://u:p@x", "userinfo"), + ("GET", "/x", "ftp://x", "must be an http(s) URL"), + ("GET", "/x", "not a url", "invalid base_url"), + ] { + let result = FromJsonSchema::new( + test_spec("svc/x", OperationType::Query), + test_config("svc", base_url), + path_template.to_string(), + method.to_string(), + test_http_client(), + ); + match result { + Err(AdapterError::SchemaParse { message }) => { + assert!( + message.contains(expected_fragment), + "message `{message}` lacks `{expected_fragment}`" + ); + } + Ok(_) => panic!( + "malformed adapter config ({method}, {path_template}, {base_url}) must fail at construction" + ), + Err(e) => panic!("expected SchemaParse, got {e}"), + } + } + } + + #[test] + fn construction_forces_visibility_internal_and_validates_placeholder_binding() { + let spec = OperationSpec::new( + "svc/getWidget", + OperationType::Query, + Visibility::External, + serde_json::json!({"type":"object","properties":{"id":{"type":"string"}}}), + serde_json::json!({"type":"object"}), + vec![], + AccessControl::default(), + None, + ); + let adapter = FromJsonSchema::new( + spec, + test_config("svc", "https://api.example.com"), + "/widgets/{id}".to_string(), + "GET".to_string(), + test_http_client(), + ) + .expect("adapter builds"); + let bundles = futures::executor::block_on(adapter.import()).unwrap(); + assert_eq!(bundles[0].spec.visibility, Visibility::Internal); + + let spec = OperationSpec::new( + "svc/getWidget", + OperationType::Query, + Visibility::Internal, + serde_json::json!({"type":"object","properties":{"widget_id":{"type":"string"}}}), + serde_json::json!({"type":"object"}), + vec![], + AccessControl::default(), + None, + ); + let result = FromJsonSchema::new( + spec, + test_config("svc", "https://api.example.com"), + "/widgets/{id}".to_string(), + "GET".to_string(), + test_http_client(), + ); + match result { + Err(AdapterError::SchemaParse { message }) => { + assert!(message.contains("placeholder"), "message was: {message}"); + assert!(message.contains("{id}"), "message was: {message}"); + } + Ok(_) => panic!("unbound placeholder must fail at construction"), + Err(e) => panic!("expected SchemaParse, got {e}"), + } + } } diff --git a/src/adapters/from_openapi.rs b/src/adapters/from_openapi.rs index 5fdf574..4ff56a5 100644 --- a/src/adapters/from_openapi.rs +++ b/src/adapters/from_openapi.rs @@ -26,12 +26,13 @@ use alkcall::registry::spec::{ use async_trait::async_trait; use serde_json::Value; -use super::forward::{forward, forward_stream, HttpServiceConfig}; +use super::forward::{ + forward, forward_stream, HttpServiceConfig, GATEWAY_BODY_KEY, HEADER_PARAM_IN_MARKER, + HEADER_PARAM_MARKER_VALUE, +}; use super::openapi_spec::{OpenAPISpec, Operation}; use crate::client::SharedHttpClient; -const GATEWAY_BODY_KEY: &str = "body"; - fn unbound_placeholders(path_template: &str, input_schema: &Value) -> Vec { let mut unbound = Vec::new(); let mut rest = path_template; @@ -145,16 +146,44 @@ impl FromOpenAPI { Some(s) => self.spec.resolve_refs_recursive(s)?, None => serde_json::json!({"type": "string"}), }; - if param.name == GATEWAY_BODY_KEY && op.request_body.is_some() { + if param.name == GATEWAY_BODY_KEY { return Err(AdapterError::SchemaParse { message: format!( "parameter named `{GATEWAY_BODY_KEY}` collides with the gateway's \ - requestBody placeholder key; the parameter would be diverted into \ - the request body at call time — rename the parameter" + requestBody placeholder key; the declared parameter would be \ + diverted into the request body at call time — rename the \ + parameter (review 001 OAI-07)" ), }); } - properties.insert(param.name.clone(), schema); + match param.in_.as_str() { + "header" => { + properties.insert( + param.name.clone(), + serde_json::json!({ + HEADER_PARAM_IN_MARKER: HEADER_PARAM_MARKER_VALUE, + "schema": schema, + }), + ); + } + "cookie" => { + return Err(AdapterError::SchemaParse { + message: format!( + "parameter `{}` on {} {} uses `in: cookie`, which the HTTP \ + adapter does not support; cookies cannot be declared through \ + the gateway contract — define a header or query parameter \ + instead (review 001 OAI-03)", + param.name, + self.config.namespace, + op.operation_id.as_deref().unwrap_or("?") + ), + }); + } + _ => {} + } + if param.in_ != "header" { + properties.insert(param.name.clone(), schema); + } if param.required { required.push(param.name.clone()); } @@ -163,8 +192,8 @@ impl FromOpenAPI { if let Some(body) = &op.request_body { if let Some(json_schema) = body.content.get("application/json") { let resolved = self.spec.resolve_refs_recursive(json_schema)?; - properties.insert("body".to_string(), resolved); - required.push("body".to_string()); + properties.insert(GATEWAY_BODY_KEY.to_string(), resolved); + required.push(GATEWAY_BODY_KEY.to_string()); } } @@ -278,6 +307,7 @@ impl FromOpenAPI { let namespace = namespace.clone(); let http_client = Arc::clone(&http_client); let error_status_codes = error_status_codes.clone(); + let input_schema = input_schema.clone(); forward_stream( &http_client, &base_url, @@ -286,6 +316,7 @@ impl FromOpenAPI { &auth_scheme, &default_headers, &namespace, + &input_schema, &error_status_codes, input, context, @@ -302,6 +333,7 @@ impl FromOpenAPI { let namespace = namespace.clone(); let http_client = Arc::clone(&http_client); let error_status_codes = error_status_codes.clone(); + let input_schema = input_schema.clone(); async move { forward( &http_client, @@ -311,6 +343,7 @@ impl FromOpenAPI { &auth_scheme, &default_headers, &namespace, + &input_schema, &error_status_codes, input, context, @@ -788,6 +821,131 @@ mod tests { assert!(required.iter().any(|v| v == "id")); } + #[tokio::test] + async fn header_parameters_are_marked_and_cookies_rejected() { + let doc = r#"{ + "openapi":"3.0.0","info":{"title":"T","version":"1"}, + "paths":{"/track/{id}":{"get":{ + "operationId":"track", + "parameters":[ + {"name":"id","in":"path","required":true,"schema":{"type":"string"}}, + {"name":"X-Trace-Id","in":"header","schema":{"type":"string"}}, + {"name":"session","in":"cookie","schema":{"type":"string"}} + ], + "responses":{"200":{"content":{"application/json":{"schema":{}}}}} + }}} + }"#; + let spec = OpenAPISpec::from_json(doc).unwrap(); + let result = adapter(spec, config("ns", "https://x", None)) + .import() + .await; + match result { + Err(AdapterError::SchemaParse { message }) => { + assert!(message.contains("cookie"), "message was: {message}"); + assert!(message.contains("session"), "message was: {message}"); + } + Ok(bundles) => panic!( + "expected cookie-parameter rejection, got {} bundles", + bundles.len() + ), + Err(e) => panic!("expected SchemaParse, got {e}"), + } + + let doc = r#"{ + "openapi":"3.0.0","info":{"title":"T","version":"1"}, + "paths":{"/track/{id}":{"get":{ + "operationId":"track", + "parameters":[ + {"name":"id","in":"path","required":true,"schema":{"type":"string"}}, + {"name":"X-Trace-Id","in":"header","schema":{"type":"string"}} + ], + "responses":{"200":{"content":{"application/json":{"schema":{}}}}} + }}} + }"#; + let spec = OpenAPISpec::from_json(doc).unwrap(); + let bundles = adapter(spec, config("ns", "https://x", None)) + .import() + .await + .unwrap(); + let props = bundles[0] + .spec + .input_schema + .get("properties") + .unwrap() + .as_object() + .unwrap(); + let header_prop = props.get("X-Trace-Id").expect("header param declared"); + assert_eq!(header_prop["wire"], "header"); + assert!(props.get("id").is_some()); + let q_prop = props + .get("q") + .map(|v| v.get("wire").is_none()) + .unwrap_or(true); + assert!(q_prop, "query params carry no wire marker"); + } + + #[tokio::test] + async fn header_parameter_flows_as_upstream_request_header_not_query() { + let doc = r#"{ + "openapi":"3.0.0","info":{"title":"T","version":"1"}, + "paths":{"/track":{"get":{ + "operationId":"track", + "parameters":[{"name":"X-Trace-Id","in":"header","schema":{"type":"string"}}], + "responses":{"200":{"content":{"application/json":{"schema":{}}}}} + }}}}"#; + let (base, rx) = spawn_capturing_server().await; + let spec = OpenAPISpec::from_json(doc).unwrap(); + let bundles = adapter(spec, config("svc", &base, None)) + .import() + .await + .unwrap(); + let ctx = noop_context("req-hdr", Capabilities::new()); + let response = match &bundles[0].handler { + HandlerKind::Once(h) => h(serde_json::json!({"X-Trace-Id": "t-9"}), ctx).await, + _ => panic!("expected Once handler"), + }; + assert!(response.result.is_ok(), "{:?}", response.result); + let captured = rx.await.unwrap(); + assert!( + !captured.query.contains("X-Trace-Id"), + "header param must not land in the query string: {}", + captured.query + ); + assert_eq!( + captured.headers.get("x-trace-id").map(String::as_str), + Some("t-9") + ); + } + + #[tokio::test] + async fn parameter_named_body_is_rejected_at_import_even_without_request_body() { + let doc = r#"{ + "openapi":"3.0.0","info":{"title":"T","version":"1"}, + "paths":{"/x":{"post":{ + "operationId":"x", + "parameters":[{"name":"body","in":"query","schema":{"type":"string"}}], + "responses":{"200":{"content":{"application/json":{"schema":{}}}}} + }}}}"#; + let spec = OpenAPISpec::from_json(doc).unwrap(); + let result = adapter(spec, config("ns", "https://x", None)) + .import() + .await; + match result { + Err(AdapterError::SchemaParse { message }) => { + assert!( + message.contains("collides with the gateway"), + "message was: {message}" + ); + assert!(message.contains("OAI-07"), "message was: {message}"); + } + Ok(bundles) => panic!( + "expected body-key collision rejection, got {} bundles", + bundles.len() + ), + Err(e) => panic!("expected SchemaParse, got {e}"), + } + } + #[tokio::test] async fn ref_resolution_in_input_schema() { let doc = r##"{ @@ -832,6 +990,7 @@ mod tests { &Some(HttpAuthScheme::Bearer), &HashMap::new(), "github", + &serde_json::json!({"type": "object", "additionalProperties": true}), &serde_json::json!({"owner":"a","repo":"b"}), &ctx, ) @@ -857,6 +1016,7 @@ mod tests { }), &HashMap::new(), "vastai", + &serde_json::json!({"type": "object", "additionalProperties": true}), &serde_json::json!({}), &ctx, ) @@ -877,6 +1037,7 @@ mod tests { &None, &HashMap::new(), "svc", + &serde_json::json!({"type": "object", "additionalProperties": true}), &serde_json::json!({"id":42,"filter":"active"}), &ctx, ) @@ -1092,6 +1253,7 @@ mod tests { &Some(HttpAuthScheme::Bearer), &HashMap::new(), "openai", + &serde_json::json!({"type": "object", "additionalProperties": true}), &serde_json::json!({"body":{"prompt":"hi"}}), &ctx, ) @@ -1290,6 +1452,7 @@ mod tests { &Some(HttpAuthScheme::Basic), &HashMap::new(), "svc", + &serde_json::json!({"type": "object", "additionalProperties": true}), &serde_json::json!({}), &ctx, ) @@ -1328,6 +1491,7 @@ mod tests { &None, &defaults, "svc", + &serde_json::json!({"type": "object", "additionalProperties": true}), &serde_json::json!({}), &ctx, ) @@ -1408,11 +1572,7 @@ mod tests { let ctx = noop_context("req-16", Capabilities::new()); let response = match ®istration.handler { HandlerKind::Once(h) => { - h( - serde_json::json!({"id":"42","filter":"new","body":{"name":"widget"}}), - ctx, - ) - .await + h(serde_json::json!({"id":"42","body":{"name":"widget"}}), ctx).await } _ => panic!("expected Once handler"), }; @@ -1424,7 +1584,7 @@ mod tests { let captured = rx.await.unwrap(); assert_eq!(captured.method, "POST"); assert_eq!(captured.path, "/items/42"); - assert_eq!(captured.query, "filter=new"); + assert_eq!(captured.query, ""); assert_eq!( captured.headers.get("content-type").unwrap(), "application/json" diff --git a/tasks/adapters/review-001-input-schema-enforcement.md b/tasks/adapters/review-001-input-schema-enforcement.md index af1ca1b..cdafd08 100644 --- a/tasks/adapters/review-001-input-schema-enforcement.md +++ b/tasks/adapters/review-001-input-schema-enforcement.md @@ -1,7 +1,7 @@ --- id: review-001-input-schema-enforcement name: Send exactly what the input schema advertises (OAI-02, OAI-03, OAI-07, OAI-09) -status: pending +status: completed depends_on: [review-001-openapi-import-integrity] scope: narrow risk: medium @@ -40,11 +40,11 @@ forwarder sends must match the contract `/schema` advertises: ## Acceptance Criteria -- [ ] Undeclared input keys are rejected (or the pass-through is explicit config, tested) — peer input cannot add upstream query params (test) -- [ ] `in: header` parameters send as headers; `in: cookie` errors clearly at import (tests) -- [ ] `body`-named parameter collision handled loudly (test) -- [ ] `from_jsonschema` validates method/path_template/base_url at construction; `Internal`-by-default matches `from_openapi` or the doc is corrected -- [ ] `cargo test` and `cargo clippy --all-targets -- -D warnings` pass +- [x] Undeclared input keys are rejected (or the pass-through is explicit config, tested) — peer input cannot add upstream query params (test) +- [x] `in: header` parameters send as headers; `in: cookie` errors clearly at import (tests) +- [x] `body`-named parameter collision handled loudly (test) +- [x] `from_jsonschema` validates method/path_template/base_url at construction; `Internal`-by-default matches `from_openapi` or the doc is corrected +- [x] `cargo test` and `cargo clippy --all-targets -- -D warnings` pass ## References @@ -60,4 +60,43 @@ forwarder sends must match the contract `/schema` advertises: ## Summary -> Filled on completion. \ No newline at end of file +Forwarders now send exactly what the input schema advertises. + +**OAI-02** (`forward.rs`): `build_request` takes the operation's +`input_schema` and enforces it at call time — every input key must be +declared in `properties` (path placeholders included; `from_openapi` +declares them) or the input is rejected as `INVALID_INPUT` before any +outbound request is built. An explicit `"additionalProperties": true` +opts an operation into catch-all input (JSON Schema semantics — the +schema advertises that extra keys are valid); non-object inputs are +rejected. No pass-through knob: the review's preferred reject-undeclared +posture is the behavior, documented in the module doc. + +**OAI-03** (`forward.rs` + `from_openapi.rs`): the `Parameter.in_` field +is no longer dead. `from_openapi` builds header-param properties as +`{"wire": "header", "schema": …}` in the generated input schema; +`build_request` routes marked properties into HTTP request headers +(validated name/value, loud `INTERNAL` on refusal) instead of the query +string. `in: cookie` fails import with a clear `SchemaParse` message +(OAI-06's silent-degradation pattern avoided here). + +**OAI-07** (`from_openapi.rs`): a spec parameter named `body` is rejected +at import unconditionally (previously only when a requestBody coexisted) +— the magic gateway body key would divert the declared parameter. + +**OAI-09** (`from_jsonschema.rs`): `FromJsonSchema::new` now returns +`Result` and validates at construction — method parses, path template +placeholders are balanced and bound by the input schema, base URL is a +userinfo-free http(s) URL — instead of surfacing `INTERNAL` on first +invoke. The registered visibility is forced to `Internal` (ADR-015) like +`from_openapi`; the module doc corrected (it previously claimed verbatim +pass-through). + +Integration note: `from_openapi`'s declaration-side checks +(`unbound_placeholders`, collision rejection) now have a call-time +counterpart; `to_openapi`'s gateway projection was untouched (its +`CallRequest` shape passes through unchanged). + +Verification: `cargo test` (297 lib), `cargo test --all-features` (368 + +integration suites), `cargo clippy --all-targets -- -D warnings` +(default + all features), `cargo fmt --check` — all green. \ No newline at end of file