Merge branch 'claude/gracious-einstein-ywrwim' into claude/gracious-einstein-ywrwim-guest-errors

Author: claude

.changeset/auth-error-codes-and-help.md

@@ -7,6 +7,7 @@ Auth failures carry more help, and their messages never quote a credential or an
 - `REQUEST_ERROR`'s message no longer repeats the transport's own error, which can carry a URL with its query string. It gains `help` saying what to check.
 - `INVALID_TOKEN` for a token whose claims do not decode no longer quotes the decoder's message, which could carry a byte or a claim of the token.
 - A failed device binding reports ZeroKMS's status, not its response body.
+- `SERVER_ERROR` for a refused token exchange names the HTTP status and the auth server's `error_description`, not the response body, which from the edge in front of it is an HTML page and can echo the access key. A body that is not JSON is reported by where it broke, not by the parser's message.
 - A profile file that is not valid JSON is reported by error kind, line and column, not by the parser's message, which could quote the file.
 - `INVALID_GRANT`, `INVALID_WORKSPACE_ID` and `ALREADY_CONSUMED` gain `help`, and `NOT_AUTHENTICATED`'s help names `stash auth login`.
 - A `STORE_ERROR` carries the help of the profile failure underneath it, such as logging in again when the profile file is missing.

packages/stack-auth/src/access_key_refresher.rs

@@ -70,9 +70,9 @@ impl Refresher for AccessKeyRefresher {
             if let Some(err) = crate::error::classify_issuance_failure(status, &body) {
                 return Err(err);
             }
-            return Err(AuthError::Server(crate::error::ServerError(format!(
-                "{status}: {body}"
-            ))));
+            return Err(AuthError::Server(crate::error::ServerError::refused(
+                status, &body,
+            )));
         }
 
         let auth_resp: AuthoriseResponse = resp.json()?;

packages/stack-auth/src/device_code/mod.rs

@@ -394,9 +394,7 @@ impl PendingDeviceCode {
             }
 
             let err: ErrorResponse = serde_json::from_str(&body).map_err(|e| {
-                AuthError::Server(crate::error::ServerError(format!(
-                    "{status}: unparseable error body: {e}"
-                )))
+                AuthError::Server(crate::error::ServerError::unparseable(status, &e))
             })?;
             match err.error.as_str() {
                 "authorization_pending" => {

packages/stack-auth/src/device_session_refresher.rs

@@ -126,10 +126,12 @@ impl DeviceSessionRefresher {
         };
         let lock = tokio::task::spawn_blocking(move || store.lock_exclusive(Token::FILENAME))
             .await
-            .map_err(|e| {
-                AuthError::Server(crate::error::ServerError(format!(
-                    "refresh lock task join failed: {e}"
-                )))
+            // A join error is tokio's: it says only that the task panicked
+            // or was cancelled, and is not ours to repeat.
+            .map_err(|_| {
+                AuthError::Server(crate::error::ServerError(
+                    "refresh lock task join failed".to_owned(),
+                ))
             })?
             .map_err(|e| {
                 AuthError::Server(crate::error::ServerError(format!(

packages/stack-auth/src/error.rs

@@ -372,6 +372,12 @@ impl AuthErrorKind for UsageLimitExceeded {
 }
 
 /// An unexpected error was returned by the auth server.
+///
+/// The message the crate builds names the HTTP status and, where the auth
+/// server gave one, its `error_description`: never the response body. From
+/// the edge in front of the auth server a body is an HTML page, and any
+/// body may echo the credential the request carried (see
+/// [`ErrorPayload`] for the rule).
 #[derive(Debug, thiserror::Error, miette::Diagnostic)]
 #[error("Server error: {0}")]
 #[diagnostic(code(stack_auth::server_error))]
@@ -382,6 +388,38 @@ impl AuthErrorKind for ServerError {
     }
 }
 
+impl ServerError {
+    /// A refused exchange no classifier had anything more specific for: the
+    /// status, and the auth server's `error_description` when the body is
+    /// its JSON error.
+    pub(crate) fn refused(status: u16, body: &str) -> Self {
+        let description = serde_json::from_str::<serde_json::Value>(body)
+            .ok()
+            .and_then(|value| {
+                value
+                    .get("error_description")?
+                    .as_str()
+                    .map(str::trim)
+                    .filter(|text| !text.is_empty())
+                    .map(str::to_owned)
+            });
+        match description {
+            Some(description) => Self(format!("{status}: {description}")),
+            None => Self(status.to_string()),
+        }
+    }
+
+    /// An error body that is not the JSON the endpoint answers with: the
+    /// status, and where the JSON broke. serde_json's own message can quote
+    /// the body, so it is not used.
+    pub(crate) fn unparseable(status: u16, error: &serde_json::Error) -> Self {
+        Self(format!(
+            "{status}: unparseable error body ({})",
+            stack_profile::diagnostic::describe_json_error(error)
+        ))
+    }
+}
+
 /// A consumable handle (e.g. a device-code poll) was used after it had already
 /// been consumed. A caller bug rather than an auth outcome, but surfaced as an
 /// `AuthError` so it flows through the `Result` contract rather than throwing

packages/stack-auth/src/oidc_refresher.rs

@@ -189,9 +189,9 @@ impl OidcFederation {
             if let Some(err) = crate::error::classify_issuance_failure(status, &body) {
                 return Err(err);
             }
-            return Err(AuthError::Server(crate::error::ServerError(format!(
-                "{status}: {body}"
-            ))));
+            return Err(AuthError::Server(crate::error::ServerError::refused(
+                status, &body,
+            )));
         }
 
         let auth_resp: AuthoriseResponse = resp.json()?;

packages/stack-auth/src/token.rs

@@ -313,9 +313,7 @@ impl Token {
             }
 
             let err: RefreshErrorResponse = serde_json::from_str(&body).map_err(|e| {
-                AuthError::Server(crate::error::ServerError(format!(
-                    "{status}: unparseable error body: {e}"
-                )))
+                AuthError::Server(crate::error::ServerError::unparseable(status, &e))
             })?;
 
             return Err(match err.error.as_str() {

packages/stack-auth/src/transport.rs

@@ -882,22 +882,58 @@ mod tests {
         assert!(matches!(err, AuthError::UsageLimitExceeded(_)), "{err:?}");
     }
 
+    /// An unclassified failure names its status and never carries the
+    /// body: from the edge it is an HTML page, and it may echo the
+    /// credential. The auth server's own error description is kept.
     #[tokio::test]
-    async fn an_unclassified_failure_is_a_server_error_with_the_body() {
-        let transport: SharedTransport = Arc::new(Stub::replying(500, "boom"));
-        let refresher = AccessKeyRefresher::new(
-            SecretToken::new("CSAKid.secret"),
-            base_url(),
+    async fn an_unclassified_failure_is_a_server_error_without_the_body() {
+        const PAGE: &str = "<html><h1>403 Forbidden</h1>nginx CSAKmarker.secret</html>";
+        let refused = |body: &'static str| {
+            let transport: SharedTransport = Arc::new(Stub::replying(403, body));
+            AccessKeyRefresher::new(
+                SecretToken::new("CSAKmarker.secret"),
+                base_url(),
+                None,
+                transport,
+            )
+        };
+        let err = refused(PAGE).refresh(&()).await.unwrap_err();
+        assert!(matches!(err, AuthError::Server(_)), "{err:?}");
+        assert_eq!(err.to_string(), "Server error: 403");
+
+        let transport: SharedTransport = Arc::new(Stub::replying(403, PAGE));
+        let err = OidcFederation::new(workspace_id(), base_url(), transport)
+            .federate(&SecretToken::new("h.p.s"))
+            .await
+            .unwrap_err();
+        assert_eq!(err.to_string(), "Server error: 403");
+
+        let described = r#"{"error":"forbidden","error_description":"client is disabled"}"#;
+        let err = refused(described).refresh(&()).await.unwrap_err();
+        assert_eq!(err.to_string(), "Server error: 403: client is disabled");
+    }
+
+    /// An error body that is not JSON says where it broke, not what it held.
+    #[tokio::test]
+    async fn an_unparseable_error_body_is_not_quoted() {
+        let transport: SharedTransport =
+            Arc::new(Stub::replying(400, r#"{"error": "marker-rt-echo"#));
+        let err = Token::refresh_with(
+            &transport,
+            &SecretToken::new("rt"),
+            &base_url(),
+            "cli",
             None,
-            transport,
+        )
+        .await
+        .unwrap_err();
+        let shown = err.to_string();
+        assert!(matches!(err, AuthError::Server(_)), "{err:?}");
+        assert!(
+            shown.starts_with("Server error: 400: unparseable error body"),
+            "{shown}"
         );
-        let err = refresher.refresh(&()).await.unwrap_err();
-        match err {
-            AuthError::Server(e) => {
-                assert!(e.to_string().contains("500") && e.to_string().contains("boom"))
-            }
-            other => panic!("{other:?}"),
-        }
+        assert!(!shown.contains("marker"), "{shown}");
     }
 
     #[tokio::test]