fix(registry): harden against fail-open authorization bugs (#34)

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: root <root@v2202606366752468913.goodsrv.de>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: 3 people

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<Regex> =
-    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));
+    }
 }

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<T: DeserializeOwned>(
         )),
         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);

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),