From ac982b17362d2a06842792ddc028b428a6603f6c Mon Sep 17 00:00:00 2001 From: root Date: Thu, 18 Jun 2026 16:31:14 +0200 Subject: [PATCH] fix(registry): harden against fail-open authorization bugs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up hardening to #33, which fixed `project_data_with_impl` mapping a 404 (project not found) to `Err` instead of `Ok(None)` — a fail-open bug, since callers treat registry errors as transient outages and let the request through. This addresses the same bug class across the rest of the crate. - registry/client.rs: the #33 fix only covered the base project fetch. The `limits` and `features` sub-fetches in the same function still mapped a 404 to `RegistryError::Response`, reintroducing the identical fail-open risk for plan/rate-limit and feature enforcement. Surface them as `Ok(None)`, matching `project_data_with_limits_impl`. - registry/error.rs + parse_http_response: split the catch-all `Response` error into `Forbidden` (403), `RateLimited` (429) and `ServerError` (5xx), so callers can fail closed on terminal denials instead of classifying every non-2xx as a retryable outage. - project/types/origin.rs: anchor the origin parser regex so malformed input is rejected rather than silently truncated to an unintended host; reject empty hostname labels so a `*.example.com` wildcard cannot match `.example.com`; compare scheme and hostname labels case-insensitively. Adds regression tests for each: limits/features 404 -> Ok(None), 403/5xx error mapping, empty-label and trailing-garbage rejection, and case-insensitive matching. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/project/types/origin.rs | 78 +++++++++++++++-- src/registry/client.rs | 161 +++++++++++++++++++++++++++++++++--- src/registry/error.rs | 14 ++++ 3 files changed, 235 insertions(+), 18 deletions(-) diff --git a/src/project/types/origin.rs b/src/project/types/origin.rs index 026d153..7d6c16a 100644 --- a/src/project/types/origin.rs +++ b/src/project/types/origin.rs @@ -5,9 +5,12 @@ use { }; /// Simplified URL parser regex. Extracts only the scheme (optional), hostname -/// and port (optional). +/// and port (optional). The expression is fully anchored: an optional trailing +/// path (`/...`) is permitted and ignored, but any other unexpected suffix is +/// rejected rather than silently truncated, so a malformed allow-list entry or +/// origin can never parse to an unintended host. static ORIGIN_PARSER_REGEX: Lazy = - Lazy::new(|| Regex::new(r"^(([^:]+)://)?([^:/]+)(:([\d]+))?").unwrap()); + Lazy::new(|| Regex::new(r"^(([^:]+)://)?([^:/]+)(:([\d]+))?(/.*)?$").unwrap()); #[derive(Debug, Clone, Copy, PartialEq, Eq)] enum MatchDirection { @@ -39,8 +42,10 @@ impl Origin<'_> { } fn matches_internal(&self, other: &Origin, dir: MatchDirection) -> bool { - if self.scheme.is_some() && other.scheme.is_some() && self.scheme != other.scheme { - return false; + if let (Some(this_scheme), Some(other_scheme)) = (self.scheme, other.scheme) { + if !this_scheme.eq_ignore_ascii_case(other_scheme) { + return false; + } } if self.port.is_some() && other.port.is_some() && self.port != other.port { @@ -67,7 +72,7 @@ fn match_fold_cb(res: bool, (this, other): (&&str, &&str)) -> bool { if this == &WILDCARD { res } else { - res && this == other + res && this.eq_ignore_ascii_case(other) } } @@ -94,7 +99,14 @@ impl<'a> TryFrom<&'a str> for Origin<'a> { .map(|m| m.as_str()) .ok_or(OriginParseError::InvalidFormat)?; - let hostname_parts = hostname.split('.').collect(); + // Reject empty labels (leading/trailing/double dots). Otherwise a + // wildcard allow-list entry such as `*.example.com` would match a + // hostname like `.example.com`, because the wildcard matches the empty + // label. + let hostname_parts: Vec<&str> = hostname.split('.').collect(); + if hostname_parts.iter().any(|part| part.is_empty()) { + return Err(OriginParseError::InvalidFormat); + } let port = caps .get(5) @@ -454,4 +466,58 @@ mod test { assert!(o1.matches_rev(&o2)); } + + #[test] + fn rejects_empty_labels() { + // Leading, trailing and double dots produce empty labels and must be + // rejected so a `*` wildcard entry cannot match them. + assert_eq!( + Origin::try_from(".example.com"), + Err(OriginParseError::InvalidFormat) + ); + assert_eq!( + Origin::try_from("example.com."), + Err(OriginParseError::InvalidFormat) + ); + assert_eq!( + Origin::try_from("a..b.com"), + Err(OriginParseError::InvalidFormat) + ); + + // A wildcard entry must not match an empty-label hostname. + let entry = Origin::try_from("*.example.com").unwrap(); + assert!(Origin::try_from(".example.com").is_err()); + // And a real subdomain still matches. + assert!(entry.matches(&Origin::try_from("evil.example.com").unwrap())); + } + + #[test] + fn rejects_unexpected_trailing_garbage() { + // The regex is fully anchored: a malformed suffix is rejected rather + // than silently truncated to a host. + assert_eq!( + Origin::try_from("domain.name:notaport"), + Err(OriginParseError::InvalidFormat) + ); + + // A trailing path is still accepted and ignored. + assert_eq!( + Origin::try_from("https://a.b.domain.name/some/path") + .unwrap() + .hostname(), + "a.b.domain.name" + ); + } + + #[test] + fn matching_is_case_insensitive() { + let entry = Origin::try_from("https://App.Example.COM").unwrap(); + let origin = Origin::try_from("https://app.example.com").unwrap(); + assert!(entry.matches(&origin)); + + // Scheme comparison is case-insensitive too. + let entry = Origin::try_from("HTTPS://app.example.com").unwrap(); + let origin = Origin::try_from("https://app.example.com").unwrap(); + assert!(entry.matches(&origin)); + } } diff --git a/src/registry/client.rs b/src/registry/client.rs index 9ad031e..7d77e93 100644 --- a/src/registry/client.rs +++ b/src/registry/client.rs @@ -300,23 +300,24 @@ impl RegistryHttpClient { None => return Ok(None), }; + // A missing sub-resource (registry returns 404) must surface as + // `Ok(None)`, never `Err`, for the same reason as the base project + // data above and to match `project_data_with_limits_impl`: an `Err` + // makes callers treat a definitively-absent limit/feature record as a + // transient registry outage and fail open. let limits = match request.include_limits { - true => Some( - self.project_limits(request.id) - .await? - .ok_or_else(|| RegistryError::Response("Limits not found".to_string()))? - .plan_limits, - ), + true => match self.project_limits(request.id).await? { + Some(response) => Some(response.plan_limits), + None => return Ok(None), + }, false => None, }; let features = match request.include_features { - true => Some( - self.project_features(request.id) - .await? - .ok_or_else(|| RegistryError::Response("Features not found".to_string()))? - .features, - ), + true => match self.project_features(request.id).await? { + Some(response) => Some(response.features), + None => return Ok(None), + }, false => None, }; @@ -424,6 +425,21 @@ async fn parse_http_response( )), StatusCode::UNAUTHORIZED => Err(RegistryError::Config(INVALID_TOKEN_ERROR)), StatusCode::NOT_FOUND => Ok(None), + // Distinguish terminal denials from transient outages so callers can + // fail closed on the former instead of lumping everything into a + // generic error that gets classified as a retryable outage. + StatusCode::FORBIDDEN => Err(RegistryError::Forbidden(format!( + "status={status} body={:?}", + resp.text().await + ))), + StatusCode::TOO_MANY_REQUESTS => Err(RegistryError::RateLimited(format!( + "status={status} body={:?}", + resp.text().await + ))), + code if code.is_server_error() => Err(RegistryError::ServerError(format!( + "status={status} body={:?}", + resp.text().await + ))), _ => Err(RegistryError::Response(format!( "status={status} body={:?}", resp.text().await @@ -571,6 +587,127 @@ mod test { assert!(response.is_none()); } + #[tokio::test] + async fn project_data_with_returns_none_when_limits_not_found() { + let project_id = "a".repeat(32); + + let mock_server = MockServer::start().await; + + // The project itself exists... + Mock::given(method(Method::Get)) + .and(path(format!("/internal/project/key/{project_id}"))) + .respond_with(ResponseTemplate::new(StatusCode::OK).set_body_json(mock_project_data())) + .mount(&mock_server) + .await; + + // ...but its limits record is missing (404). + Mock::given(method(Method::Get)) + .and(path("/internal/v1/project-limits")) + .respond_with(ResponseTemplate::new(404)) + .mount(&mock_server) + .await; + + let request = crate::project::ProjectDataRequest::new(&project_id).include_limits(); + + let response = RegistryHttpClient::with_config( + mock_server.uri(), + Some(mock_server.uri()), + "auth", + TEST_ORIGIN, + "st", + "sv", + Default::default(), + ) + .unwrap() + .project_data_with(request) + .await + .unwrap(); + + // A missing limits record must be `Ok(None)`, not an `Err`: an `Err` + // makes callers treat it as a transient outage and fail open on plan / + // rate-limit enforcement. + assert!(response.is_none()); + } + + #[tokio::test] + async fn project_data_with_returns_none_when_features_not_found() { + let project_id = "a".repeat(32); + + let mock_server = MockServer::start().await; + + Mock::given(method(Method::Get)) + .and(path(format!("/internal/project/key/{project_id}"))) + .respond_with(ResponseTemplate::new(StatusCode::OK).set_body_json(mock_project_data())) + .mount(&mock_server) + .await; + + Mock::given(method(Method::Get)) + .and(path("/appkit/v1/config")) + .respond_with(ResponseTemplate::new(404)) + .mount(&mock_server) + .await; + + let request = crate::project::ProjectDataRequest::new(&project_id).include_features(); + + let response = RegistryHttpClient::with_config( + mock_server.uri(), + Some(mock_server.uri()), + "auth", + TEST_ORIGIN, + "st", + "sv", + Default::default(), + ) + .unwrap() + .project_data_with(request) + .await + .unwrap(); + + assert!(response.is_none()); + } + + #[tokio::test] + async fn forbidden_maps_to_forbidden_error() { + let project_id = "a".repeat(32); + + let mock_server = MockServer::start().await; + + Mock::given(method(Method::Get)) + .and(path(format!("/internal/project/key/{project_id}"))) + .respond_with(ResponseTemplate::new(StatusCode::FORBIDDEN)) + .mount(&mock_server) + .await; + + let result = RegistryHttpClient::new(mock_server.uri(), "auth", TEST_ORIGIN, "st", "sv") + .unwrap() + .project_data(&project_id) + .await; + + // A 403 is a terminal denial and must be distinguishable from a + // transient outage so callers can fail closed. + assert!(matches!(result, Err(RegistryError::Forbidden(_)))); + } + + #[tokio::test] + async fn server_error_maps_to_server_error() { + let project_id = "a".repeat(32); + + let mock_server = MockServer::start().await; + + Mock::given(method(Method::Get)) + .and(path(format!("/internal/project/key/{project_id}"))) + .respond_with(ResponseTemplate::new(StatusCode::INTERNAL_SERVER_ERROR)) + .mount(&mock_server) + .await; + + let result = RegistryHttpClient::new(mock_server.uri(), "auth", TEST_ORIGIN, "st", "sv") + .unwrap() + .project_data(&project_id) + .await; + + assert!(matches!(result, Err(RegistryError::ServerError(_)))); + } + #[tokio::test] async fn project_id_invalid_len() { let project_id = "a".repeat(31); diff --git a/src/registry/error.rs b/src/registry/error.rs index 997d25a..66c2375 100644 --- a/src/registry/error.rs +++ b/src/registry/error.rs @@ -17,6 +17,20 @@ pub enum RegistryError { #[error("invalid response: {0}")] Response(String), + /// The registry definitively rejected the request (HTTP 403). This is a + /// terminal denial, not a transient outage: callers must fail closed. + #[error("forbidden: {0}")] + Forbidden(String), + + /// The registry rate-limited the request (HTTP 429). + #[error("rate limited: {0}")] + RateLimited(String), + + /// The registry failed to serve the request (HTTP 5xx). This is a + /// transient, server-side error and is safe to retry. + #[error("server error: {0}")] + ServerError(String), + #[error("building URL: {0}")] UrlBuild(url::ParseError),