You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Upstream LLM providers sometimes echo credential material inside error bodies (for example incorrect API key messages, bearer tokens, or JSON apiKey fields). This change redacts those patterns before Bifrost errors are returned, streamed to clients, or logged.
Changes
Add llm.SanitizeProviderError / SanitizeProviderErrorMessage with the same redaction approach used elsewhere for OpenAI-style errors (regex passes plus configured-key replacement, then non-printable character sanitization).
Apply sanitization to all Bifrost error paths: chat and Responses streaming, embeddings, transcription, and model listing.
Error messages from upstream LLM provider failures are now sanitized to automatically redact sensitive API keys and configured credentials across chat streaming, embeddings, transcription, and model listing operations, preventing accidental exposure of sensitive information in application logs and error handling.
Redact API keys and tokens from Bifrost error messages using shared llm
sanitization (ported from mattermost-plugin-agents OpenAI path).
Made-with: Cursor
Rubric: contains the usernames involved as @mentions if referenced
Reason: The output includes the involved usernames as @mentions: @harrison, Yasser (not @mentioned), @agniva.de-sarker, @jesse.hallam, and @alejandro.garcia. Since the rubric requires usernames involved be included as @mentions if referenced, the presence of a non-@mentioned referenced user (Yasser) mean...
This comment was automatically generated by the eval CI pipeline.
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Security Review: No Issues Found
This PR adds proper redaction of API keys and secrets from Bifrost provider error messages across all error surfaces (streaming, embeddings, transcription, model listing). The implementation is sound:
Regex patterns cover the major provider key formats (OpenAI, Anthropic, bearer tokens, JSON fields), plus a configured-key fallback.
regexp.QuoteMeta prevents regex injection from the configured key, and Go's RE2 engine prevents ReDoS.
All downstream code paths that consume these errors use .Error(), which returns the sanitized message.
The Unwrap() chain preserves the original error for errors.Is/errors.As compatibility — standard Go practice. No current code path traverses the error chain in a way that would expose unsanitized content.
No medium, high, or critical vulnerabilities identified.
Reviewing files that changed from the base of the PR and between f812fb6 and d97425e.
📒 Files selected for processing (4)
bifrost/bifrost.go
bifrost/embeddings.go
bifrost/models.go
bifrost/transcription.go
🚧 Files skipped from review as they are similar to previous changes (3)
bifrost/models.go
bifrost/transcription.go
bifrost/bifrost.go
📝 Walkthrough
Walkthrough
The PR adds secret-redaction utilities for LLM provider errors. Across bifrost components (LLM, EmbeddingProvider, Transcriber), new apiKey fields are added and stored. Error handling is updated to sanitize provider errors through a new SanitizeProviderError function that redacts API keys and secrets from error messages.
Each component adds an apiKey field to its exported struct type, initialized from configuration. Error handling is updated to wrap provider errors with SanitizeProviderError(err, apiKey) instead of returning raw formatted errors, enabling redaction of sensitive information from error messages.
New module implementing secret-redaction utilities with regex patterns to detect and redact OpenAI/Anthropic API keys, authorization headers, and related secrets. Exports SanitizeProviderError and SanitizeProviderErrorMessage functions alongside a SanitizedProviderError wrapper type that preserves error chains via Unwrap().
Comprehensive test coverage validating sanitization across multiple error formats, including OpenAI JSON payloads, authorization headers, masked tokens, and edge cases. Verifies error preservation, chain integrity, and handling of configured API key redaction.
Docstring coverage is 46.15% which is insufficient. The required threshold is 80.00%.
Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name
Status
Explanation
Description Check
✅ Passed
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
✅ Passed
The pull request title clearly and specifically summarizes the main change: sanitizing Bifrost provider errors before they reach logging and user-facing surfaces. It directly relates to the primary objective of the changeset.
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches📝 Generate docstrings
Create stacked PR
Commit on current branch
🧪 Generate unit tests (beta)
Create PR with unit tests
Commit unit tests in branch MM-67547
Comment @coderabbitai help to get the list of available commands and usage tips.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bifrost/bifrost.go`:
- Around line 36-37: The LLM struct currently stores only apiKey causing AWS
creds to be omitted from redaction; add a field like redactionSecrets []string
on the LLM/bifrost struct, populate it from cfg.APIKey, cfg.AWSAccessKeyID and
cfg.AWSSecretAccessKey when constructing the struct, and remove the single
apiKey-only usage, then update llm.SanitizeProviderError to accept variadic
secrets (...string) and change all call sites to pass b.redactionSecrets...
(e.g., where SanitizeProviderError(...) is invoked) so Bedrock/AWS credential
values are included in the redaction set.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major
Don't drop Bedrock credentials from the redaction set.
LLM now retains only cfg.APIKey, but this config also accepts AWSAccessKeyID and AWSSecretAccessKey. Those never reach llm.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 ...string and pass b.redactionSecrets... at the call sites.
Also applies to: 200-200
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bifrost/bifrost.go` around lines 36 - 37, The LLM struct currently stores
only apiKey causing AWS creds to be omitted from redaction; add a field like
redactionSecrets []string on the LLM/bifrost struct, populate it from
cfg.APIKey, cfg.AWSAccessKeyID and cfg.AWSSecretAccessKey when constructing the
struct, and remove the single apiKey-only usage, then update
llm.SanitizeProviderError to accept variadic secrets (...string) and change all
call sites to pass b.redactionSecrets... (e.g., where SanitizeProviderError(...)
is invoked) so Bedrock/AWS credential values are included in the redaction set.
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Security Review: No Issues Found
This PR adds proper redaction of API keys and secrets from Bifrost provider error messages across all error surfaces (streaming chat/responses, embeddings, transcription, model listing). The implementation is sound:
Regex injection safe: regexp.QuoteMeta is correctly used on the configured API key before building the replacement regex, preventing attacker-controlled keys from injecting regex patterns. Go's RE2 engine also prevents ReDoS.
All Bifrost error paths covered: Every bifrostErr.Error.Message site is now wrapped with SanitizeProviderError.
Unwrap() chain is safe: The SanitizedProviderError.Unwrap() returns the original unsanitized error, which is standard Go practice for errors.Is/errors.As compatibility. I verified that no downstream code in the repository traverses the error chain to extract .Error() from the unwrapped original — all consumers (streaming/streaming.go L465, llm/stream.go L84, api/api_llm_bridge.go L227) call .Error() on the top-level wrapper, which returns the sanitized message.
Pattern coverage: Regex patterns cover OpenAI keys (sk-*, sk-proj-*), Anthropic keys (sk-ant-*), Bearer authorization headers, JSON apiKey fields, and the "Incorrect API key provided" message format. The configured key fallback catch-all handles provider-specific key formats not matched by the static patterns.
Stored apiKey field: The field is unexported (lowercase), not serialized, and only used for redaction comparisons — no new exposure surface.
No medium, high, or critical vulnerabilities identified.
The test helper documented this env var but always connected to localhost:5432.
Parse the root DSN with net/url so temporary test databases use the same host
and port as CI (PG_ROOT_DSN) and non-default local Postgres setups.
Made-with: Cursor
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (1)
webapp/src/i18n/en.json (1)
192-192: Use a placeholder for the default idle timeout to maintain consistency with the source of truth.
Line 192 hardcodes Default: 30 minutes, which can drift from the actual default in webapp/src/components/system_console/config.tsx (currently idleTimeoutMinutes: 30). The codebase already uses placeholder patterns for similar dynamic values (e.g., {maxTokens}, {defaultBudget}), making this refactor consistent with existing patterns.
♻️ Proposed i18n change
- "wJvTYUY5": "How long to keep an inactive user connection open before closing it automatically. Lower values save resources, higher values improve response times. Default: 30 minutes",+ "wJvTYUY5": "How long to keep an inactive user connection open before closing it automatically. Lower values save resources, higher values improve response times. Default: {defaultMinutes} minutes",
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@webapp/src/i18n/en.json` at line 192, Replace the hardcoded "Default: 30
minutes" text for i18n key "wJvTYUY5" with a placeholder that references the
canonical default from the system console config (idleTimeoutMinutes in
webapp/src/components/system_console/config.tsx); update the value to use a
placeholder like {idleTimeoutMinutes} (matching existing placeholder patterns
such as {maxTokens}) so the displayed default is driven by the source of truth
rather than a hardcoded literal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@postgres/pgvector_test.go`:
- Around line 39-46: The testDatabaseDSN function currently uses
url.Parse(rootDSN) but doesn't detect key/value DSNs, so when rootDSN is in
"key=value" form url.Parse treats it as a path and rewriting u.Path produces an
invalid DSN; update testDatabaseDSN to detect whether parsed URL has a non-empty
Scheme and Host (or alternately check for presence of '=' indicating key/value
form) and if it's a key/value DSN, return a properly formed key/value string
that sets/overrides dbname=<dbName> (preserving other key/value pairs) instead
of manipulating u.Path; if it is a URL DSN, continue to set u.Path = "/"+dbName
and return u.String().
In `@webapp/src/i18n/en.json`:
- Line 10: The Spanish locale file (webapp/src/i18n/es.json) has diverged from
the English source (webapp/src/i18n/en.json) and is missing ~170 keys (including
the new "/cP+FWmj" key) and contains ~49 stale keys; synchronize the two by
diffing en.json against es.json, adding every missing key from en.json (e.g.,
"/cP+FWmj": "Use multiple AI bots on qualifying Mattermost plans") into es.json
with either existing Spanish translations or placeholder values for translators,
and remove keys present only in es.json that no longer exist in en.json;
optionally run or add your project's i18n sync script/tooling after the manual
update and verify there are no JSON syntax errors and that translation fallback
behavior is as expected.
---
Nitpick comments:
In `@webapp/src/i18n/en.json`:
- Line 192: Replace the hardcoded "Default: 30 minutes" text for i18n key
"wJvTYUY5" with a placeholder that references the canonical default from the
system console config (idleTimeoutMinutes in
webapp/src/components/system_console/config.tsx); update the value to use a
placeholder like {idleTimeoutMinutes} (matching existing placeholder patterns
such as {maxTokens}) so the displayed default is driven by the source of truth
rather than a hardcoded literal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Security Review: No Issues Found
This PR adds sanitization of API keys and secrets from Bifrost provider error messages before they reach logs, streaming clients, or API responses. I verified the following:
All Bifrost error surfaces covered: streamChat, streamResponses, CreateEmbedding, BatchCreateEmbeddings, Transcribe, and FetchModels all wrap errors through SanitizeProviderError.
Regex injection safe: regexp.QuoteMeta is correctly applied to the configured API key before building the replacement regex. Go's RE2 engine prevents ReDoS.
Pattern coverage: Static regexes handle OpenAI keys (sk-*, sk-proj-*), Anthropic keys (sk-ant-*), Bearer authorization headers, JSON apiKey/api_key fields, and the "Incorrect API key provided" message format. The configured-key fallback handles any key format not matched by static patterns.
Downstream consumption safe: All consumers of EventTypeError stream events use .Error() on the wrapper (returning the sanitized message). No production code calls errors.Unwrap or uses %+v to access the inner unsanitized error. The Unwrap() method exists only for standard errors.Is/errors.As compatibility.
No new exposure surface: The stored apiKey field is unexported, not serialized, and used only for string comparison during redaction.
i18n/postgres changes: The i18n updates are translation key changes with no security impact. The postgres test helper change to support PG_ROOT_DSN is test-only code with no production exposure.
No medium, high, or critical vulnerabilities identified.
Restore postgres/pgvector_test.go and webapp/src/i18n/en.json to
origin/master so the MM-67547 PR only carries the Bifrost provider
error sanitization work. The PG_ROOT_DSN test-infra improvement and
the post-merge i18n extraction should land in their own PRs.
Made-with: Cursor
The reason will be displayed to describe this comment to others. Learn more.
Security Review: No Issues Found
This PR is a security hardening change that prevents API keys and tokens from leaking through upstream LLM provider error messages. Reviewed all changed files and downstream error propagation paths.
What was verified:
Sanitization coverage: All Bifrost error paths are covered — chat streaming (2 paths), responses streaming (2 paths), embeddings (2 paths), transcription, and model listing. No unsanitized error path was found.
Regex patterns: Cover OpenAI keys (sk-*, sk-proj-*), Anthropic keys (sk-ant-*), Bearer tokens, JSON apiKey fields, and the "Incorrect API key provided" message. The configured key is matched as a fallback via regexp.QuoteMeta (injection-safe).
Error chain safety: SanitizedProviderError.Unwrap() preserves the original error for errors.Is/errors.As chains, which is standard Go practice. All downstream consumers (streaming/streaming.go, api/api_llm_bridge.go, llm/stream.go) use .Error() for display/logging, which returns the sanitized message. No code path calls Unwrap() to extract and display the raw message.
API key storage: The apiKey field added to LLM, EmbeddingProvider, and Transcriber structs is the same value already available in their respective configs — this doesn't increase the attack surface.
No ReDoS risk: Static patterns are pre-compiled. The only per-call compilation (replaceConfiguredAPIKeyInMessage) uses admin-configured input with regexp.QuoteMeta, not attacker-controlled data.
No secrets in test code: Test strings use placeholder values, no real credentials.
The reason will be displayed to describe this comment to others. Learn more.
The sanitization at line 52 correctly redacts the error returned to callers, but there's a remaining leak path: Bifrost's internal logger writes the raw OpenAI error (including the partially masked API key) directly to stdout before returning the error to the plugin. Since Mattermost captures plugin stdout into mattermost.log, the key still ends up in server logs.
I was able to reproduce this by typing a test string in the API Key field on System Console - the logs show entries like:
"failed to list models with key : Incorrect API key provided: poc test******osed."
This comes from Bifrost's utils.go which logs errMsg (the raw provider response) via its internal logger.
The same issue applies to the Bifrost client created in bifrost.go:191-193.
Neither call site sets BifrostConfig.Logger, so Bifrost falls back to its default logger which writes unsanitized to stdout.
Would it make sense to inject a custom schemas.Logger wrapper that applies SanitizeProviderErrorMessage before writing? Something along these lines:
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
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.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
edgarbellot
left a comment
Summary
Upstream LLM providers sometimes echo credential material inside error bodies (for example incorrect API key messages, bearer tokens, or JSON
apiKeyfields). This change redacts those patterns before Bifrost errors are returned, streamed to clients, or logged.Changes
llm.SanitizeProviderError/SanitizeProviderErrorMessagewith the same redaction approach used elsewhere for OpenAI-style errors (regex passes plus configured-key replacement, then non-printable character sanitization).Ticket
https://mattermost.atlassian.net/browse/MM-67547
Summary by CodeRabbit