-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Include request URL in provider error messages #9232
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -76,15 +76,15 @@ impl TetrateProvider { | |
| } | ||
| } | ||
|
|
||
| fn error_from_tetrate_error_payload(payload: Value) -> ProviderError { | ||
| fn error_from_tetrate_error_payload(payload: Value, url: &str) -> ProviderError { | ||
| let code = payload | ||
| .get("error") | ||
| .and_then(|e| e.get("code")) | ||
| .and_then(|c| c.as_u64()) | ||
| .unwrap_or(500) as u16; | ||
| let status = reqwest::StatusCode::from_u16(code) | ||
| .unwrap_or(reqwest::StatusCode::INTERNAL_SERVER_ERROR); | ||
| Self::enrich_credits_error(map_http_error_to_provider_error(status, Some(payload))) | ||
| Self::enrich_credits_error(map_http_error_to_provider_error(status, Some(payload), url)) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did it forget to sanitize here? Or is it that tetrate URLs don't have secrets in them. I wonder whether having the URL be part of the error type (outside of the string) and having it be sanitised in the Display/Debug impl might be safer than needing to remember to sanitise it before constructing the error?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, that's probably better. going to merge it though to get ahead |
||
| } | ||
| } | ||
|
|
||
|
|
@@ -173,7 +173,10 @@ impl Provider for TetrateProvider { | |
| .await | ||
| .map_err(Self::enrich_credits_error)?; | ||
| if body.get("error").is_some() { | ||
| return Err(Self::error_from_tetrate_error_payload(body)); | ||
| return Err(Self::error_from_tetrate_error_payload( | ||
| body, | ||
| "v1/chat/completions", | ||
| )); | ||
| } | ||
|
|
||
| return Err(ProviderError::ExecutionError( | ||
|
|
@@ -203,7 +206,7 @@ impl Provider for TetrateProvider { | |
|
|
||
| // Tetrate can return errors in 200 OK responses, so check explicitly | ||
| if json.get("error").is_some() { | ||
| return Err(Self::error_from_tetrate_error_payload(json)); | ||
| return Err(Self::error_from_tetrate_error_payload(json, "v1/models")); | ||
| } | ||
|
|
||
| let arr = json.get("data").and_then(|v| v.as_array()).ok_or_else(|| { | ||
|
|
@@ -257,7 +260,7 @@ mod tests { | |
| "message": "Insufficient credits" | ||
| } | ||
| }); | ||
| match TetrateProvider::error_from_tetrate_error_payload(payload) { | ||
| match TetrateProvider::error_from_tetrate_error_payload(payload, "test") { | ||
| ProviderError::CreditsExhausted { | ||
| details, | ||
| top_up_url, | ||
|
|
@@ -277,7 +280,7 @@ mod tests { | |
| "message": "Invalid API key" | ||
| } | ||
| }); | ||
| match TetrateProvider::error_from_tetrate_error_payload(payload) { | ||
| match TetrateProvider::error_from_tetrate_error_payload(payload, "test") { | ||
| ProviderError::Authentication(msg) => { | ||
| assert!(msg.contains("Invalid API key")); | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid interpolating raw
urlinto user-facingProviderErrorstrings here, because several providers accept base URLs that preserve credentials and query parameters (seeOpenAiProvider::parse_base_urltests aroundkey=...anduser:pass@...incrates/goose/src/providers/openai.rs). With this change, any non-2xx response now echoes the full request URL into errors/logs, so failed requests can leak API keys or passwords to telemetry and terminal output. Sanitize the URL (drop userinfo and sensitive query values) before including it in error text.Useful? React with 👍 / 👎.