diff --git a/src/adapters/openapi_spec.rs b/src/adapters/openapi_spec.rs index 457136c..0b6de5f 100644 --- a/src/adapters/openapi_spec.rs +++ b/src/adapters/openapi_spec.rs @@ -4,11 +4,18 @@ //! 1.2; JSON-first detection under `from_str`), plus `$ref` resolution //! against the raw document. -use std::collections::{BTreeMap, HashMap}; +use std::collections::{BTreeMap, HashMap, HashSet}; use alkcall::client::AdapterError; use serde_json::Value; +/// Maximum structural depth budget for recursive `$ref` resolution +/// (review 001 OAI-01). Bounds both `$ref` hop chains and schema nesting +/// height so a self-referential or pathologically deep component fails +/// import with a clean [`AdapterError::SchemaParse`] instead of +/// exhausting the stack. +pub(crate) const MAX_REF_RESOLUTION_DEPTH: usize = 64; + pub(crate) const HTTP_METHODS: &[&str] = &["get", "post", "put", "patch", "delete", "head", "options"]; @@ -141,22 +148,24 @@ impl OpenAPISpec { } let mut paths = BTreeMap::new(); - for (path, item) in paths_raw.as_object().expect("paths is object") { - if !item.is_object() { - continue; - } - let mut operations = Vec::new(); - for method in HTTP_METHODS { - if let Some(op_raw) = item.get(*method) { - if let Some(op) = parse_operation(op_raw) { - operations.push((method.to_string(), op)); + if let Some(paths_obj) = paths_raw.as_object() { + for (path, item) in paths_obj { + if !item.is_object() { + continue; + } + let mut operations = Vec::new(); + for method in HTTP_METHODS { + if let Some(op_raw) = item.get(*method) { + if let Some(op) = parse_operation(op_raw) { + operations.push((method.to_string(), op)); + } } } + if operations.is_empty() { + continue; + } + paths.insert(path.clone(), PathItem { operations }); } - if operations.is_empty() { - continue; - } - paths.insert(path.clone(), PathItem { operations }); } let components = raw @@ -167,8 +176,10 @@ impl OpenAPISpec { return None; } let mut map = HashMap::new(); - for (k, v) in schemas.as_object().expect("schemas is object") { - map.insert(k.clone(), v.clone()); + if let Some(schemas_obj) = schemas.as_object() { + for (k, v) in schemas_obj { + map.insert(k.clone(), v.clone()); + } } Some(Components { schemas: map }) }); @@ -196,23 +207,71 @@ impl OpenAPISpec { Ok(current.clone()) } + /// Resolve `$ref` pointers within `schema` against the raw document, + /// returning a fully expanded copy. + /// + /// Recursion is bounded: a visited set keyed on the JSON-pointer path + /// rejects a `$ref` that is already being expanded on the current + /// branch (a self/circular reference — trees, linked lists, cursor + /// pagination), and a depth budget rejects pathologically deep + /// nesting. Both surface as [`AdapterError::SchemaParse`] at import + /// instead of recursing to stack exhaustion. Fully-expanded inlining + /// of recursive schemas is deliberately not supported; specs that + /// rely on it fail loudly here (review 001 OAI-01). pub(crate) fn resolve_refs_recursive(&self, schema: &Value) -> Result { + self.resolve_refs_bounded(schema, &mut HashSet::new(), 0) + } + + fn resolve_refs_bounded( + &self, + schema: &Value, + resolving: &mut HashSet, + depth: usize, + ) -> Result { + if depth > MAX_REF_RESOLUTION_DEPTH { + return Err(AdapterError::SchemaParse { + message: format!( + "$ref resolution exceeded depth budget of {MAX_REF_RESOLUTION_DEPTH} \ + (self-referential or pathologically nested schema)" + ), + }); + } match schema { Value::Object(obj) => { if let Some(Value::String(reference)) = obj.get("$ref") { + if !resolving.insert(reference.clone()) { + return Err(AdapterError::SchemaParse { + message: format!( + "circular $ref detected at depth {depth}: {reference}" + ), + }); + } let resolved = self.resolve_ref(reference)?; - return self.resolve_refs_recursive(&resolved); + let out = self.resolve_refs_bounded(&resolved, resolving, depth + 1); + resolving.remove(reference); + return out; + } + if depth + 1 > MAX_REF_RESOLUTION_DEPTH { + return Err(AdapterError::SchemaParse { + message: format!( + "schema nesting exceeded depth budget of {MAX_REF_RESOLUTION_DEPTH} \ + (self-referential or pathologically nested schema)" + ), + }); } let mut out = serde_json::Map::new(); for (k, v) in obj { - out.insert(k.clone(), self.resolve_refs_recursive(v)?); + out.insert( + k.clone(), + self.resolve_refs_bounded(v, resolving, depth + 1)?, + ); } Ok(Value::Object(out)) } Value::Array(arr) => { let mut out = Vec::with_capacity(arr.len()); for v in arr { - out.push(self.resolve_refs_recursive(v)?); + out.push(self.resolve_refs_bounded(v, resolving, depth + 1)?); } Ok(Value::Array(out)) } @@ -283,3 +342,198 @@ fn parse_operation(raw: &Value) -> Option { responses, }) } + +#[cfg(test)] +mod tests { + use super::*; + use crate::adapters::{FromOpenAPI, HttpServiceConfig}; + use crate::client::{HttpClientConfig, SharedHttpClient}; + use alkcall::client::OperationAdapter; + use serde_json::json; + use std::sync::Arc; + + fn wrap_spec(raw: Value) -> OpenAPISpec { + OpenAPISpec::from_value(raw).expect("test spec is valid") + } + + fn schema_test_spec(schema: Value) -> OpenAPISpec { + wrap_spec(json!({ + "openapi": "3.0.0", + "info": {"title": "T", "version": "1"}, + "paths": { + "/x": {"get": {"operationId": "x", "responses": { + "200": {"content": {"application/json": {"schema": {}}} + }}}} + }, + "components": {"schemas": schema} + })) + } + + fn nested_object(depth: usize) -> Value { + let mut current = json!({"type": "string"}); + for _ in 0..depth { + current = json!({"type": "object", "properties": {"child": current}}); + } + current + } + + #[test] + fn self_referential_ref_errors_instead_of_aborting() { + let spec = schema_test_spec(json!({ + "Node": { + "type": "object", + "properties": { + "next": {"$ref": "#/components/schemas/Node"} + } + } + })); + let schema = spec + .components + .as_ref() + .and_then(|c| c.schemas.get("Node")) + .expect("test spec has Node") + .clone(); + let result = spec.resolve_refs_recursive(&schema); + match result { + Err(AdapterError::SchemaParse { message }) => { + assert!(message.contains("circular $ref"), "message was: {message}"); + assert!(message.contains("Node"), "message was: {message}"); + } + other => panic!("expected circular-$ref SchemaParse error, got {other:?}"), + } + } + + #[test] + fn mutually_recursive_refs_error_on_cycle() { + let spec = schema_test_spec(json!({ + "A": {"properties": {"b": {"$ref": "#/components/schemas/B"}}}, + "B": {"properties": {"a": {"$ref": "#/components/schemas/A"}}} + })); + let schema = spec + .components + .as_ref() + .and_then(|c| c.schemas.get("A")) + .expect("test spec has A") + .clone(); + let result = spec.resolve_refs_recursive(&schema); + assert!(matches!(result, Err(AdapterError::SchemaParse { .. }))); + } + + #[test] + fn over_deep_non_circular_spec_errors_cleanly() { + let depth = MAX_REF_RESOLUTION_DEPTH * 4; + let spec = schema_test_spec(json!({"Deep": nested_object(depth)})); + let schema = spec + .components + .as_ref() + .and_then(|c| c.schemas.get("Deep")) + .expect("test spec has Deep") + .clone(); + let result = spec.resolve_refs_recursive(&schema); + match result { + Err(AdapterError::SchemaParse { message }) => { + assert!(message.contains("depth budget"), "message was: {message}"); + } + other => panic!("expected depth-budget SchemaParse error, got {other:?}"), + } + } + + #[test] + fn shared_refs_to_common_schema_import_identically() { + let spec = schema_test_spec(json!({ + "Money": {"type": "object", "properties": {"amount": {"type": "number"}}}, + "Order": { + "type": "object", + "properties": {"total": {"$ref": "#/components/schemas/Money"}} + }, + "Refund": { + "type": "object", + "properties": {"amount": {"$ref": "#/components/schemas/Money"}} + } + })); + let schemas = &spec + .components + .as_ref() + .expect("test spec has schemas") + .schemas; + for name in ["Order", "Refund"] { + let schema = schemas.get(name).expect("test schema present").clone(); + let resolved = spec + .resolve_refs_recursive(&schema) + .expect("no false cycle"); + let money = &resolved["properties"][if name == "Order" { "total" } else { "amount" }]; + assert_eq!(money["type"], "object"); + assert_eq!(money["properties"]["amount"]["type"], "number"); + } + } + + #[test] + fn diamond_ref_reuse_within_one_schema_does_not_trip_cycle_guard() { + let spec = schema_test_spec(json!({ + "Id": {"type": "string"}, + "Wrapper": { + "type": "object", + "properties": { + "a": {"$ref": "#/components/schemas/Id"}, + "b": {"$ref": "#/components/schemas/Id"} + } + } + })); + let schema = spec + .components + .as_ref() + .expect("test spec has schemas") + .schemas + .get("Wrapper") + .expect("test schema present") + .clone(); + let resolved = spec + .resolve_refs_recursive(&schema) + .expect("no false cycle"); + assert_eq!(resolved["properties"]["a"]["type"], "string"); + } + + #[test] + fn import_of_self_referential_spec_returns_error_not_abort() { + let doc = r##"{ + "openapi": "3.0.0", + "info": {"title": "T", "version": "1"}, + "components": {"schemas": { + "Node": { + "type": "object", + "properties": {"next": {"$ref": "#/components/schemas/Node"}} + } + }}, + "paths": { + "/nodes": {"get": {"operationId": "listNodes", "responses": { + "200": {"content": {"application/json": { + "schema": {"$ref": "#/components/schemas/Node"} + }}} + }}} + } + }"##; + let spec = OpenAPISpec::from_json(doc).unwrap(); + let client = SharedHttpClient::new(HttpClientConfig::default()).unwrap(); + let adapter = FromOpenAPI::new( + spec, + HttpServiceConfig { + namespace: "svc".to_string(), + base_url: "https://x".to_string(), + auth: None, + default_headers: HashMap::new(), + }, + Arc::new(client), + ); + let result = adapter.import(); + match futures::executor::block_on(result) { + Err(AdapterError::SchemaParse { message }) => { + assert!(message.contains("circular $ref"), "message was: {message}"); + } + Ok(bundles) => panic!( + "expected import error for recursive spec, got {} bundles", + bundles.len() + ), + Err(e) => panic!("expected circular-$ref SchemaParse, got {e}"), + } + } +} diff --git a/tasks/adapters/review-001-ref-cycle-guard.md b/tasks/adapters/review-001-ref-cycle-guard.md index c2b7131..90620a5 100644 --- a/tasks/adapters/review-001-ref-cycle-guard.md +++ b/tasks/adapters/review-001-ref-cycle-guard.md @@ -1,7 +1,7 @@ --- id: review-001-ref-cycle-guard name: Bounded, cycle-safe $ref resolution (OAI-01, OAI-08) -status: pending +status: completed depends_on: [] scope: narrow risk: high @@ -34,11 +34,11 @@ Ride-along: **OAI-08** — guarded `expect`s in `openapi_spec.rs:144,170` ## Acceptance Criteria -- [ ] Import of a self-referential spec returns an error — the process does not abort (the review's named acceptance gate: a test importing a self-referential spec would have caught the abort immediately) -- [ ] Deeply-nested non-circular spec beyond the budget errors cleanly at import (test) -- [ ] A non-recursive spec with `$ref` sharing (refs to a common schema) still imports identically — no false cycle positives (existing suite green) -- [ ] OAI-08 `expect`s replaced -- [ ] `cargo test` and `cargo clippy --all-targets -- -D warnings` pass +- [x] Import of a self-referential spec returns an error — the process does not abort (the review's named acceptance gate: a test importing a self-referential spec would have caught the abort immediately) +- [x] Deeply-nested non-circular spec beyond the budget errors cleanly at import (test) +- [x] A non-recursive spec with `$ref` sharing (refs to a common schema) still imports identically — no false cycle positives (existing suite green) +- [x] OAI-08 `expect`s replaced +- [x] `cargo test` and `cargo clippy --all-targets -- -D warnings` pass ## References @@ -51,6 +51,46 @@ Ride-along: **OAI-08** — guarded `expect`s in `openapi_spec.rs:144,170` > work (review-001-openapi-import-integrity) so the delicate > cycle-detection change lands alone. +Implementation decisions: + +- Depth budget set to 64 (`MAX_REF_RESOLUTION_DEPTH`, pub(crate) const in + openapi_spec.rs) — far above any legitimately expandable schema height, + 2× serde_json's parse recursion limit. It bounds both `$ref` hop chains + and plain schema nesting. +- Clean loud error chosen over depth-limited expansion, per the task's + stated preference: recursive schemas (trees, linked lists, cursor + pagination) fail import with `AdapterError::SchemaParse` naming the + offending ref, rather than expanding unboundedly or truncating + silently. +- The visited set is branch-scoped (insert before recursing, remove on + return), so shared refs to one common schema — diamond or repeated — + resolve normally; only a ref re-entering its own expansion chain is a + cycle. +- OAI-08: both guarded `expect`s (`paths is object`, `schemas is + object`) replaced with `if let Some(..)`; the inner-body re-shape is + behavior-preserving (both sites were immediately preceded by an + `is_object()` check). +- Tests added in `src/adapters/openapi_spec.rs` (`mod tests`): direct + recursive + mutually-recursive error, over-deep non-circular error, two + no-false-positive cases (shared common schema, diamond reuse), and an + end-to-end `FromOpenAPI::import` of a self-referential spec asserting + an error return (process does not abort). + ## Summary -> Filled on completion. \ No newline at end of file +Implemented bounded, cycle-safe `$ref` resolution in +`src/adapters/openapi_spec.rs` (review 001 OAI-01 + OAI-08): +`resolve_refs_recursive` now delegates to a `resolve_refs_bounded` +worker carrying a branch-scoped `HashSet` of in-flight JSON-pointer refs +and a depth counter against `MAX_REF_RESOLUTION_DEPTH` (64). A ref +re-entering its own expansion chain errors with +`AdapterError::SchemaParse` ("circular $ref detected …: "); a +ref-chain or nesting height beyond the budget errors similarly ("exceeded +depth budget of 64"). Import of a self-referential spec therefore +returns a clean import error instead of aborting the process via stack +overflow (OAI-01); OAI-08's two guarded `expect`s in `from_value` became +plain `if let` paths. Six tests added in `openapi_spec.rs` covering the +cyclic, over-deep, and shared-ref matrices plus the named acceptance +gate (end-to-end self-referential import errors, no abort). 188 lib +tests green; full `cargo test`, `cargo clippy --all-targets -- -D +warnings`, and `cargo fmt --check` pass. \ No newline at end of file