From 1732910b3d7791112e69ebbfd2bafc563eb045b7 Mon Sep 17 00:00:00 2001 From: Steven Enamakel Date: Thu, 17 Sep 2026 20:26:57 +0530 Subject: [PATCH] Harden endpoint and response validation --- crates/tinyjevclient/src/client/README.md | 5 +- crates/tinyjevclient/src/client/mod.rs | 10 +- crates/tinyjevclient/src/client/test.rs | 37 ++++++- crates/tinyjevclient/src/response/mod.rs | 15 ++- crates/tinyjevclient/src/response/test.rs | 23 +++++ docs/plans/system-one-client.md | 40 +++++--- docs/specs/system-one-client.md | 117 +++++++++++++++++----- 7 files changed, 202 insertions(+), 45 deletions(-) diff --git a/crates/tinyjevclient/src/client/README.md b/crates/tinyjevclient/src/client/README.md index e146256..49d5a7b 100644 --- a/crates/tinyjevclient/src/client/README.md +++ b/crates/tinyjevclient/src/client/README.md @@ -1,7 +1,7 @@ # Client module The client validates a request, sends it to the System One endpoint, classifies -HTTP failures, retries only transient failures, validates the response against +HTTP failures, retries timeouts and connection-establishment failures, validates the response against the original questions, and returns attempts and end-to-end latency. API keys remain private and render only as `[REDACTED]`. @@ -10,4 +10,5 @@ loopback IP addresses used by local tests and development services. Both successful and failed evaluations report attempts and end-to-end latency. All transport failures use the same explicit bounded retry policy because the transport error taxonomy cannot reliably distinguish transient DNS, TLS, and -connectivity failures from permanent ones. +connectivity failures from permanent ones. Other request/body/redirect errors +are terminal, and automatic redirects are disabled. diff --git a/crates/tinyjevclient/src/client/mod.rs b/crates/tinyjevclient/src/client/mod.rs index 72dd951..ab5b303 100644 --- a/crates/tinyjevclient/src/client/mod.rs +++ b/crates/tinyjevclient/src/client/mod.rs @@ -27,6 +27,7 @@ impl Client { config.validate()?; let http = reqwest::Client::builder() .timeout(config.timeout) + .redirect(reqwest::redirect::Policy::none()) .build() .map_err(|source| Error::Transport { source })?; Ok(Self { config, http }) @@ -160,6 +161,11 @@ impl ClientConfig { reason: "base URL must not contain credentials".to_owned(), }); } + if url.query().is_some() || url.fragment().is_some() { + return Err(Error::InvalidConfig { + reason: "base URL must not contain a query or fragment".to_owned(), + }); + } if url.scheme() == "http" && !url .host_str() @@ -207,11 +213,13 @@ fn classify_transport(source: reqwest::Error) -> Failure { error: Error::Timeout, retry_after: None, } - } else { + } else if source.is_connect() { Failure::Retryable { error: Error::Transport { source }, retry_after: None, } + } else { + Failure::Terminal(Error::Transport { source }) } } diff --git a/crates/tinyjevclient/src/client/test.rs b/crates/tinyjevclient/src/client/test.rs index 21d54d4..3e67df9 100644 --- a/crates/tinyjevclient/src/client/test.rs +++ b/crates/tinyjevclient/src/client/test.rs @@ -97,6 +97,13 @@ fn config(base_url: String) -> ClientConfig { config } +fn unavailable_base_url() -> String { + let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + let address = listener.local_addr().unwrap(); + drop(listener); + format!("http://{address}") +} + #[tokio::test] async fn sends_the_documented_endpoint_and_bearer_header() { let (base_url, requests) = server(vec![response( @@ -222,6 +229,17 @@ fn validates_every_configuration_bound_and_redacted_key_replacement() { Client::new(userinfo), Err(Error::InvalidConfig { .. }) )); + for base_url in [ + "https://example.com?tenant=x", + "https://example.com#fragment", + ] { + let mut component = ClientConfig::new("key"); + component.base_url = base_url.into(); + assert!(matches!( + Client::new(component), + Err(Error::InvalidConfig { .. }) + )); + } let mut timeout = ClientConfig::new("key"); timeout.timeout = Duration::ZERO; @@ -357,7 +375,7 @@ async fn local_validation_and_response_validation_report_failure_metadata() { #[tokio::test] async fn connection_failure_is_classified_as_transport() { - let failure = Client::new(config("http://127.0.0.1:1".into())) + let failure = Client::new(config(unavailable_base_url())) .unwrap() .evaluate(&request()) .await @@ -365,3 +383,20 @@ async fn connection_failure_is_classified_as_transport() { assert!(matches!(failure.error, Error::Transport { .. })); assert_eq!(failure.attempts, 1); } + +#[tokio::test] +async fn redirect_is_not_followed() { + let (base_url, requests) = server(vec![response( + 307, + "{}", + "Location: http://example.com/downgrade\r\n", + )]) + .await; + let failure = Client::new(config(base_url)) + .unwrap() + .evaluate(&request()) + .await + .unwrap_err(); + assert!(matches!(failure.error, Error::HttpStatus { status: 307 })); + assert_eq!(requests.lock().await.len(), 1); +} diff --git a/crates/tinyjevclient/src/response/mod.rs b/crates/tinyjevclient/src/response/mod.rs index 30a03e3..c08b417 100644 --- a/crates/tinyjevclient/src/response/mod.rs +++ b/crates/tinyjevclient/src/response/mod.rs @@ -25,6 +25,11 @@ impl EvaluationResponse { if self.model.trim().is_empty() { return Err(Error::invalid_response("response model must not be empty")); } + if self.model != request.model { + return Err(Error::invalid_response( + "response model must match the requested model", + )); + } let expected: BTreeSet<&str> = request.questions.keys().map(String::as_str).collect(); let actual: BTreeSet<&str> = self.answers.keys().map(String::as_str).collect(); if actual != expected { @@ -84,15 +89,19 @@ fn validate_pair(question: &Question, answer: &Answer) -> Result<()> { "score levels must exactly match request criteria", )); } - if !answer.score.is_finite() { - return Err(Error::invalid_response("score must be finite")); + let maximum = + u32::try_from(question.criteria.len() - 1).map_or(f64::from(u32::MAX), f64::from); + if !answer.score.is_finite() || !(0.0..=maximum).contains(&answer.score) { + return Err(Error::invalid_response( + "score must be finite and inside the requested scale", + )); } let expected_score: f64 = answer .probabilities .iter() .map(|(level, probability)| level.parse::().unwrap_or_default() * probability) .sum(); - if (answer.score - expected_score).abs() > SCORE_TOLERANCE { + if (answer.score - expected_score).abs() > SCORE_TOLERANCE + f64::EPSILON { return Err(Error::invalid_response( "score must equal the probability-weighted level", )); diff --git a/crates/tinyjevclient/src/response/test.rs b/crates/tinyjevclient/src/response/test.rs index 58d90c8..efb7d43 100644 --- a/crates/tinyjevclient/src/response/test.rs +++ b/crates/tinyjevclient/src/response/test.rs @@ -120,6 +120,10 @@ fn rejects_empty_model_extra_ids_and_nonmaximal_choice() { empty_model.model.clear(); assert!(empty_model.validate_for(&request()).is_err()); + let mut wrong_model = response(); + wrong_model.model = "jev-other".into(); + assert!(wrong_model.validate_for(&request()).is_err()); + let mut extra = response(); extra .answers @@ -175,6 +179,14 @@ fn rejects_nonfinite_score_and_mismatched_legend() { score.score = f64::INFINITY; assert!(nonfinite.validate_for(&request()).is_err()); + let mut outside = response(); + let Answer::Score(score) = outside.answers.get_mut("quality").unwrap() else { + panic!("fixture answer should be a score") + }; + score.probabilities = BTreeMap::from([("0".into(), 0.98), ("1".into(), 0.02)]); + score.score = -0.01; + assert!(outside.validate_for(&request()).is_err()); + let mut legend = response(); let Answer::Score(score) = legend.answers.get_mut("quality").unwrap() else { panic!("fixture answer should be a score") @@ -190,3 +202,14 @@ fn rejects_nonfinite_score_and_mismatched_legend() { score.legend.insert("1".into(), json!("low")); assert!(reversed.validate_for(&request()).is_err()); } + +#[test] +fn accepts_the_exact_score_rounding_boundary() { + let mut boundary = response(); + let Answer::Score(score) = boundary.answers.get_mut("quality").unwrap() else { + panic!("fixture answer should be a score") + }; + score.probabilities = BTreeMap::from([("0".into(), 0.97), ("1".into(), 0.03)]); + score.score = 0.05; + boundary.validate_for(&request()).unwrap(); +} diff --git a/docs/plans/system-one-client.md b/docs/plans/system-one-client.md index a07f761..7cd7d07 100644 --- a/docs/plans/system-one-client.md +++ b/docs/plans/system-one-client.md @@ -2,19 +2,24 @@ Linked specification: [`../specs/system-one-client.md`](../specs/system-one-client.md). -1. Replace the TinyBus template with the ordinary library at - `crates/tinyjevclient/`; remove the obsolete module crate, contract crate, - submodule, packaging workflow, and release documentation. -2. Define and pin Choice, Score, Noul, request, answer, usage, and response - wires under `crates/tinyjevclient/src/{request,response}/`. -3. Validate requests and request-relative response invariants in those module - roots, with every case in their adjacent `test.rs` files. -4. Implement the rustls client, secret redaction, classified failures, failure - measurements, HTTPS policy, and bounded retries under - `crates/tinyjevclient/src/{client,error}/`. -5. Update the crate example, public API test, root documentation, CI, lockfile, - dependency policy, and environment example in the same change. -6. Verify with: +1. Replace the TinyBus template with `crates/tinyjevclient/Cargo.toml` and + `crates/tinyjevclient/src/lib.rs`; remove the obsolete module/contract crates, + submodule, packaging workflow, and release documentation. Compile the empty + public surface before adding behavior. +2. Add failing wire and validation tests in + `src/request/test.rs` and `src/response/test.rs`; implement the payloads in + `src/{request,response}/types.rs` and their validators in each `mod.rs`. +3. Add failing configuration, retry, HTTP-status, transport, redirect, secret, + and failure-metadata tests in `src/client/test.rs`; implement `Client`, + `ClientConfig`, `RetryPolicy`, `EvaluationResult`, and `EvaluationFailure` in + `src/client/{mod,types}.rs`. Reject retry counts above 100 and test that + boundary so the saturating counter cannot become unbounded. +4. Add rendering/source tests in `src/error/test.rs`; implement every classified + failure in `src/error/mod.rs`. Timeouts/connect failures are retryable; + request/body/redirect failures are terminal; HTTP status policy is explicit. +5. Update `examples/basic.rs`, `tests/public_api.rs`, module READMEs, root docs, + `.env.example`, CI, `Cargo.lock`, and `deny.toml` in the same change. +6. Run the verification contract: ```sh cargo fmt --all -- --check @@ -25,3 +30,12 @@ Linked specification: [`../specs/system-one-client.md`](../specs/system-one-clie .github/scripts/check-file-coverage.sh 90 coverage.json cargo deny check all ``` + +## Completion checklist + +- [x] Template and TinyBus artifacts removed. +- [x] Choice, Score, and Noul wires pinned. +- [x] Request-relative response validation implemented. +- [x] HTTPS, redirect, credential, retry, and failure-metadata behavior tested. +- [x] Every production source file exceeds 90% line coverage. +- [x] Format, clippy, build, test, rustdoc, MSRV, supply-chain, and CI checks pass. diff --git a/docs/specs/system-one-client.md b/docs/specs/system-one-client.md index 8bb6e90..0e66f1b 100644 --- a/docs/specs/system-one-client.md +++ b/docs/specs/system-one-client.md @@ -1,27 +1,94 @@ # System One client -## Contract - -The client mirrors `POST /v1/systemone`: state is a string, object, or array; -questions are a nonempty map of Choice, Score, and Noul values; answers return -under the same ids. Choice accepts 2–255 options, Score accepts 2–10 concrete -levels, and Noul may describe its true and false criteria. - -The client validates request bounds before transport and validates response ids, -answer types, probability ranges and sums, selected maxima, Score legends, and -weighted Score values before returning. Score consistency allows two hundredths -for provider display rounding while probability sums retain strict tolerance. -Typed output is an interface guarantee, -not a truth guarantee; applications evaluate accuracy and thresholds on their -own data. - -## Failure policy - -Authentication and request errors are terminal. Transport failures, timeouts, -rate limits, overload, and server errors use a bounded caller-visible retry -policy. No retry is unbounded, and success or failure reports every attempt and -the full elapsed time. HTTP is allowed only for literal loopback addresses; -every remote endpoint requires HTTPS. - -Credentials never appear in `Debug`, error messages, or retained response -bodies. Application state and provider bodies are not logged by the crate. +- **Status:** Implemented +- **Owner:** `crates/tinyjevclient` + +## Problem + +Rust hosts need TypeSafe System One decisions without redefining the wire, +accepting malformed probability payloads, leaking credentials, or hiding failed +attempts from reliability measurements. + +## Goals + +- Model Choice, Score, and Noul requests and answers as serde types. +- Validate requests before transport and responses against their request. +- Provide bounded async HTTPS execution with classified failures and measured + attempts, usage, request id, and elapsed time. +- Keep policy, thresholds, workflow control, and side effects with the caller. + +## Non-goals + +- Generating free-form text or explanations. +- Executing a selected action or treating confidence as permission. +- Choosing application thresholds or persisting evaluation state. +- Hiding provider/network failures behind an unbounded retry loop. + +## Proposed behavior + +`EvaluationRequest` mirrors `POST /v1/systemone`: state is a string, object, or +array; questions are a nonempty map; Choice accepts 2–255 options, Score accepts +2–10 nonempty levels, and Noul may define true/false criteria. `Client::evaluate` +returns `EvaluationResult` on success or `EvaluationFailure` carrying the +classified `Error`, attempt count, and elapsed time. + +Response validation requires: + +- exact question ids and primitive types; +- the exact requested model id; +- finite probabilities in `[0, 1]`, with each distribution sum differing from + `1.0` by at most `0.000001`; +- Choice labels exactly matching criteria and the chosen label tying for the + highest probability; +- Score keys and legend values exactly matching the requested levels; +- Score inside `0..=highest_level` and differing from its probability-weighted + value by at most `0.02 + f64::EPSILON` for provider display rounding; +- Noul inside `[0, 1]`. + +Remote base URLs require HTTPS, contain no credentials, query, or fragment, and +automatic redirects are disabled. Plain HTTP is accepted only for literal +loopback IP addresses used by local test servers. + +Authentication, request validation, response decoding, and non-connect +transport failures are terminal. Timeouts, connection-establishment failures, +408, 429, 529, and server errors use the explicit retry policy. `max_retries` +cannot exceed 100, so the saturating attempt counter cannot loop forever. + +## Example + +```rust,no_run +use std::collections::BTreeMap; +use serde_json::json; +use tinyjevclient::{Client, EvaluationRequest, Noul, Question}; + +# async fn check() -> Result<(), Box> { +let request = EvaluationRequest::jev( + "Delete production now", + BTreeMap::from([( + "violation".into(), + Question::Noul(Noul { + instructions: json!("Does this violate the approval policy?"), + criteria: None, + }), + )]), +); +let result = Client::from_env()?.evaluate(&request).await?; +println!("{:?}", result.response.answers["violation"]); +# Ok(()) +# } +``` + +## Acceptance criteria + +- Every primitive and response type has an exact serde wire test. +- Every request and response invariant above has a success and rejection test. +- Mock HTTP tests cover authentication, 408/429/529/5xx classification, timeout, + connection failure, redirects, decoding, retry exhaustion, Retry-After forms, + secret redaction, and failure metadata. +- Every production source file has at least 90% line coverage. +- Format, clippy, build, tests, rustdoc, MSRV, cargo-deny, and coverage are green. + +## Open questions + +None for this version. Threshold calibration and domain accuracy belong to the +consuming application and its evaluation corpus.