-
Notifications
You must be signed in to change notification settings - Fork 82
MM-67547: Sanitize Bifrost provider errors before log and user surfaces #579
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
Open
nickmisasi
wants to merge
2
commits into
master
Choose a base branch
from
MM-67547
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| // Copyright (c) 2023-present Mattermost, Inc. All Rights Reserved. | ||
| // See LICENSE.txt for license information. | ||
|
|
||
| package llm | ||
|
|
||
| import ( | ||
| "regexp" | ||
| "strings" | ||
| ) | ||
|
|
||
| const providerErrorRedacted = "[REDACTED]" | ||
|
|
||
| var ( | ||
| openAIAuthHeaderPattern = regexp.MustCompile(`(?i)(Authorization:\s*Bearer\s+)(\S+)`) | ||
| openAIJSONAPIKeyPattern = regexp.MustCompile(`(?i)("api(?:_|)key"\s*:\s*")([^"]+)(")`) | ||
| openAIIncorrectKeyPattern = regexp.MustCompile(`(?i)(Incorrect API key provided)(?::\s*[^"\r\n]+?)?(\.?\s+You can find your API key|["\r\n]|$)`) | ||
| openAIKeyPattern = regexp.MustCompile(`\bsk(?:-proj)?-[A-Za-z0-9_-]{10,}\b`) | ||
| anthropicKeyPattern = regexp.MustCompile(`\bsk-ant-[A-Za-z0-9_-]{20,}\b`) | ||
| ) | ||
|
|
||
| // SanitizedProviderError wraps an upstream LLM error after redacting secrets from its message. | ||
| // It implements [errors.Unwrap] so [errors.Is] / [errors.As] chains on the original error are preserved. | ||
| type SanitizedProviderError struct { | ||
| message string | ||
| err error | ||
| } | ||
|
|
||
| func (e *SanitizedProviderError) Error() string { | ||
| return e.message | ||
| } | ||
|
|
||
| func (e *SanitizedProviderError) Unwrap() error { | ||
| return e.err | ||
| } | ||
|
|
||
| // SanitizeProviderErrorMessage applies the same redaction rules as [SanitizeProviderError] to a plain string. | ||
| // configuredAPIKey is additionally redacted when it appears as a substring (word-boundary safe). | ||
| func SanitizeProviderErrorMessage(message string, configuredAPIKey string) string { | ||
| sanitized := sanitizeProviderErrorMessagePlain(message) | ||
| apiKey := strings.TrimSpace(configuredAPIKey) | ||
| if apiKey != "" { | ||
| sanitized = replaceConfiguredAPIKeyInMessage(sanitized, apiKey) | ||
| } | ||
| return sanitized | ||
| } | ||
|
|
||
| // SanitizeProviderError redacts API keys, bearer tokens, and similar material from provider errors | ||
| // before those strings are logged, streamed to clients, or returned to callers. | ||
| func SanitizeProviderError(err error, configuredAPIKey string) error { | ||
| if err == nil { | ||
| return nil | ||
| } | ||
|
|
||
| sanitized := SanitizeProviderErrorMessage(err.Error(), configuredAPIKey) | ||
| if sanitized == err.Error() { | ||
| return err | ||
| } | ||
|
|
||
| return &SanitizedProviderError{ | ||
| message: sanitized, | ||
| err: err, | ||
| } | ||
| } | ||
|
|
||
| func sanitizeProviderErrorMessagePlain(message string) string { | ||
| sanitized := openAIAuthHeaderPattern.ReplaceAllString(message, `${1}`+providerErrorRedacted) | ||
| sanitized = openAIJSONAPIKeyPattern.ReplaceAllString(sanitized, `${1}`+providerErrorRedacted+`${3}`) | ||
| sanitized = openAIIncorrectKeyPattern.ReplaceAllString(sanitized, `${1}`+`${2}`) | ||
| sanitized = openAIKeyPattern.ReplaceAllString(sanitized, providerErrorRedacted) | ||
| sanitized = anthropicKeyPattern.ReplaceAllString(sanitized, providerErrorRedacted) | ||
| return SanitizeNonPrintableChars(sanitized) | ||
| } | ||
|
|
||
| func replaceConfiguredAPIKeyInMessage(message string, apiKey string) string { | ||
| pattern := regexp.MustCompile(`(^|[^A-Za-z0-9_-])(` + regexp.QuoteMeta(apiKey) + `)([^A-Za-z0-9_-]|$)`) | ||
| return pattern.ReplaceAllString(message, `${1}`+providerErrorRedacted+`${3}`) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,135 @@ | ||
| // Copyright (c) 2023-present Mattermost, Inc. All Rights Reserved. | ||
| // See LICENSE.txt for license information. | ||
|
|
||
| package llm | ||
|
|
||
| import ( | ||
| "errors" | ||
| "fmt" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| var errStreamingTimeout = errors.New("timeout streaming") | ||
|
|
||
| func TestSanitizeProviderError(t *testing.T) { | ||
| t.Run("preserves unrelated errors", func(t *testing.T) { | ||
| sanitizedErr := SanitizeProviderError(errStreamingTimeout, "") | ||
|
|
||
| assert.Same(t, errStreamingTimeout, sanitizedErr) | ||
| }) | ||
|
|
||
| t.Run("redacts auth material from provider errors", func(t *testing.T) { | ||
| configuredKey := "this-is-my-disclosed-api-key" | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| input string | ||
| wantContains string | ||
| wantNotContains []string | ||
| }{ | ||
| { | ||
| name: "incorrect api key message", | ||
| input: `{"error":{"message":"Incorrect API key provided: this-is-my-disclosed-api-key. You can find your API key at https://platform.openai.com/account/api-keys.","type":"invalid_request_error","code":"invalid_api_key"}}`, | ||
| wantContains: `Incorrect API key provided. You can find your API key`, | ||
| wantNotContains: []string{ | ||
| "this-is-my-disclosed-api-key", | ||
| }, | ||
| }, | ||
| { | ||
| name: "progressively masked key", | ||
| input: `{"error":{"message":"Incorrect API key provided: this-is-****************-key. You can find your API key at https://platform.openai.com/account/api-keys.","type":"invalid_request_error","code":"invalid_api_key"}}`, | ||
| wantContains: `Incorrect API key provided. You can find your API key`, | ||
| wantNotContains: []string{ | ||
| "this-is-****************-key", | ||
| }, | ||
| }, | ||
| { | ||
| name: "authorization header", | ||
| input: `upstream failure: Authorization: Bearer sk-proj-1234567890abcdefghijklmnop`, | ||
| wantContains: `Authorization: Bearer [REDACTED]`, | ||
| wantNotContains: []string{ | ||
| "sk-proj-1234567890abcdefghijklmnop", | ||
| }, | ||
| }, | ||
| { | ||
| name: "standalone openai key token", | ||
| input: `provider error: leaked sk-1234567890abcdefghij token`, | ||
| wantContains: `provider error: leaked [REDACTED] token`, | ||
| wantNotContains: []string{ | ||
| "sk-1234567890abcdefghij", | ||
| }, | ||
| }, | ||
| { | ||
| name: "standalone anthropic key token", | ||
| input: `provider error: leaked sk-ant-1234567890abcdefghijklmnop`, | ||
| wantContains: `provider error: leaked [REDACTED]`, | ||
| wantNotContains: []string{ | ||
| "sk-ant-1234567890abcdefghijklmnop", | ||
| }, | ||
| }, | ||
| { | ||
| name: "json api key field", | ||
| input: `{"apiKey":"this-is-my-disclosed-api-key","detail":"request failed"}`, | ||
| wantContains: `"apiKey":"[REDACTED]"`, | ||
| wantNotContains: []string{ | ||
| "this-is-my-disclosed-api-key", | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| sanitizedErr := SanitizeProviderError(errors.New(tt.input), configuredKey) | ||
| require.NotNil(t, sanitizedErr) | ||
| assert.Contains(t, sanitizedErr.Error(), tt.wantContains) | ||
| for _, secret := range tt.wantNotContains { | ||
| assert.NotContains(t, sanitizedErr.Error(), secret) | ||
| } | ||
| }) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("redacts short configured api keys", func(t *testing.T) { | ||
| sanitizedErr := SanitizeProviderError(errors.New(`provider error: short`), "short") | ||
| require.NotNil(t, sanitizedErr) | ||
| assert.Equal(t, "provider error: [REDACTED]", sanitizedErr.Error()) | ||
| }) | ||
|
|
||
| t.Run("does not corrupt unrelated words for one character keys", func(t *testing.T) { | ||
| providerErrorMessage := `Unauthorized: Incorrect API key provided: t. You can find your API key at https://platform.openai.com/account/api-keys.` | ||
|
|
||
| sanitizedErr := SanitizeProviderError(errors.New(providerErrorMessage), "t") | ||
| require.NotNil(t, sanitizedErr) | ||
| assert.Equal(t, "Unauthorized: Incorrect API key provided. You can find your API key at https://platform.openai.com/account/api-keys.", sanitizedErr.Error()) | ||
| assert.NotContains(t, sanitizedErr.Error(), "Unau[REDACTED]horized") | ||
| assert.NotContains(t, sanitizedErr.Error(), "Incorrect API key provided: t") | ||
| }) | ||
|
|
||
| t.Run("preserves wrapped provider error chain", func(t *testing.T) { | ||
| originalErr := errors.New("provider error: short") | ||
|
|
||
| sanitizedErr := SanitizeProviderError(originalErr, "short") | ||
| require.NotNil(t, sanitizedErr) | ||
| assert.Equal(t, "provider error: [REDACTED]", sanitizedErr.Error()) | ||
| assert.ErrorIs(t, sanitizedErr, originalErr) | ||
|
|
||
| var wrapped *SanitizedProviderError | ||
| require.ErrorAs(t, sanitizedErr, &wrapped) | ||
| assert.Equal(t, "provider error: [REDACTED]", wrapped.Error()) | ||
| assert.Equal(t, originalErr, wrapped.Unwrap()) | ||
| }) | ||
| } | ||
|
|
||
| func TestSanitizeProviderError_bifrostStylePrefixes(t *testing.T) { | ||
| const key = "this-is-my-disclosed-api-key" | ||
| raw := `Incorrect API key provided: this-is-my-disclosed-api-key. You can find your API key at https://platform.openai.com/account/api-keys.` | ||
|
|
||
| err := SanitizeProviderError(fmt.Errorf("bifrost error: %s", raw), key) | ||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), "bifrost error:") | ||
| assert.Contains(t, err.Error(), "Incorrect API key provided.") | ||
| assert.NotContains(t, err.Error(), key) | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Don't drop Bedrock credentials from the redaction set.
LLMnow retains onlycfg.APIKey, but this config also acceptsAWSAccessKeyIDandAWSSecretAccessKey. Those never reachllm.SanitizeProviderError, so a Bedrock auth error that echoes either value will still leak it through the new streamed error paths. Please carry a full secret list here instead of a single API key.🔐 Suggested direction
type LLM struct { client *bifrostcore.Bifrost provider schemas.ModelProvider - apiKey string // used only to redact configured secrets from provider error surfaces + redactionSecrets []string defaultModel string inputTokenLimit int outputTokenLimit int streamingTimeout time.Duration @@ return &LLM{ client: client, provider: cfg.Provider, - apiKey: cfg.APIKey, + redactionSecrets: []string{ + cfg.APIKey, + cfg.AWSAccessKeyID, + cfg.AWSSecretAccessKey, + }, defaultModel: cfg.DefaultModel, inputTokenLimit: cfg.InputTokenLimit,Then widen the
llm.SanitizeProviderError(...)helpers to accept...stringand passb.redactionSecrets...at the call sites.Also applies to: 200-200
🤖 Prompt for AI Agents