-
Notifications
You must be signed in to change notification settings - Fork 2
fix(security): redact clear-text logging surfaces (wave 1) #238
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 all 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 |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ | |
| import ( | ||
| "bytes" | ||
| "context" | ||
| "crypto/sha256" | ||
| "fmt" | ||
| "io" | ||
| "net" | ||
|
|
@@ -1295,15 +1296,15 @@ | |
| } | ||
|
|
||
| func logCodexWebsocketConnected(sessionID string, authID string, wsURL string) { | ||
| log.Infof("codex websockets: upstream connected session=%s auth=%s url=%s", strings.TrimSpace(sessionID), sanitizeCodexWebsocketLogField(authID), sanitizeCodexWebsocketLogURL(wsURL)) | ||
| log.Infof("codex websockets: upstream connected session=%s auth=%s endpoint=%s", sanitizeCodexSessionID(sessionID), sanitizeCodexWebsocketLogField(authID), sanitizeCodexWebsocketLogEndpoint(wsURL)) | ||
| } | ||
|
|
||
| func logCodexWebsocketDisconnected(sessionID, authID, wsURL, reason string, err error) { | ||
| if err != nil { | ||
| log.Infof("codex websockets: upstream disconnected session=%s auth=%s url=%s reason=%s err=%v", strings.TrimSpace(sessionID), sanitizeCodexWebsocketLogField(authID), sanitizeCodexWebsocketLogURL(wsURL), strings.TrimSpace(reason), err) | ||
| log.Infof("codex websockets: upstream disconnected session=%s auth=%s endpoint=%s reason=%s err=%v", sanitizeCodexSessionID(sessionID), sanitizeCodexWebsocketLogField(authID), sanitizeCodexWebsocketLogEndpoint(wsURL), strings.TrimSpace(reason), err) | ||
| return | ||
| } | ||
| log.Infof("codex websockets: upstream disconnected session=%s auth=%s url=%s reason=%s", strings.TrimSpace(sessionID), sanitizeCodexWebsocketLogField(authID), sanitizeCodexWebsocketLogURL(wsURL), strings.TrimSpace(reason)) | ||
| log.Infof("codex websockets: upstream disconnected session=%s auth=%s endpoint=%s reason=%s", sanitizeCodexSessionID(sessionID), sanitizeCodexWebsocketLogField(authID), sanitizeCodexWebsocketLogEndpoint(wsURL), strings.TrimSpace(reason)) | ||
| } | ||
|
|
||
| func sanitizeCodexWebsocketLogField(raw string) string { | ||
|
|
@@ -1325,6 +1326,27 @@ | |
| return parsed.String() | ||
| } | ||
|
|
||
| func sanitizeCodexWebsocketLogEndpoint(raw string) string { | ||
| trimmed := strings.TrimSpace(raw) | ||
| if trimmed == "" { | ||
| return "" | ||
| } | ||
| parsed, err := url.Parse(trimmed) | ||
| if err != nil || parsed.Host == "" { | ||
| return "redacted-endpoint" | ||
| } | ||
| return parsed.Scheme + "://" + parsed.Host | ||
| } | ||
|
|
||
| func sanitizeCodexSessionID(raw string) string { | ||
| trimmed := strings.TrimSpace(raw) | ||
| if trimmed == "" { | ||
| return "" | ||
| } | ||
| sum := sha256.Sum256([]byte(trimmed)) | ||
Check failureCode scanning / CodeQL Use of a broken or weak cryptographic hashing algorithm on sensitive data High Sensitive data (password) Error loading related location Loading Sensitive data (password) Error loading related location Loading Copilot AutofixAI about 2 months ago Copilot could not generate an autofix suggestion Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support. |
||
| return fmt.Sprintf("sess_%x", sum[:6]) | ||
| } | ||
|
|
||
| // CodexAutoExecutor routes Codex requests to the websocket transport only when: | ||
| // 1. The downstream transport is websocket, and | ||
| // 2. The selected auth enables websockets. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ | |
|
|
||
| import ( | ||
| "context" | ||
| "crypto/sha256" | ||
| "fmt" | ||
| "sort" | ||
| "strings" | ||
|
|
@@ -661,7 +662,7 @@ | |
| registration.SuspendedClients[clientID] = reason | ||
| registration.LastUpdated = time.Now() | ||
| if reason != "" { | ||
| log.Debugf("Suspended client %s for model %s (reason provided)", clientID, modelID) | ||
| log.Debugf("Suspended client %s for model %s (reason provided)", logSafeRegistryID(clientID), logSafeRegistryID(modelID)) | ||
| } else { | ||
| log.Debug("Suspended client for model") | ||
| } | ||
|
|
@@ -690,6 +691,15 @@ | |
| log.Debug("Resumed suspended client for model") | ||
| } | ||
|
|
||
| func logSafeRegistryID(raw string) string { | ||
| trimmed := strings.TrimSpace(raw) | ||
| if trimmed == "" { | ||
| return "" | ||
| } | ||
| sum := sha256.Sum256([]byte(trimmed)) | ||
Check failureCode scanning / CodeQL Use of a broken or weak cryptographic hashing algorithm on sensitive data High Sensitive data (password) Error loading related location Loading Sensitive data (password) Error loading related location Loading Sensitive data (password) Error loading related location Loading Copilot AutofixAI about 2 months ago Copilot could not generate an autofix suggestion Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support. |
||
| return fmt.Sprintf("id_%x", sum[:6]) | ||
| } | ||
|
|
||
| // ClientSupportsModel reports whether the client registered support for modelID. | ||
| func (r *ModelRegistry) ClientSupportsModel(clientID, modelID string) bool { | ||
| clientID = strings.TrimSpace(clientID) | ||
|
|
||
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Copilot Autofix
AI about 2 months ago
General approach: Do not log the raw
errorvalue coming fromthinking.ApplyThinking(or any error that may encapsulate sensitive model/alias info) in clear text. Instead, log only non-sensitive, high-level information: whether an error occurred, a generic reason, and possibly a safe, coarse-grained classification (e.g., error type/code) that does not contain user- or tenant-specific data.Best targeted fix: Modify
logCodexWebsocketDisconnectedinpkg/llmproxy/executor/codex_websockets_executor.goso that whenerris non-nil, we do not interpolateerrdirectly with%v. Instead:erris a*thinking.ThinkingErrorand, if so, log only itsCode(and maybe a generic message) rather than the full error string."internal_error"or just omit the error details.sessionID,authID, andwsURLviasanitizeCodexSessionID,sanitizeCodexWebsocketLogField, andsanitizeCodexWebsocketLogEndpoint.Concretely:
Update the
if err != nilblock inlogCodexWebsocketDisconnected:err=%vwitherr=%sand pass a redacted string returned by a new helper, e.g.,sanitizeCodexError(err).Introduce a new helper function in the same file, e.g.:
This uses only existing imports (
fmtandthinkingare already imported in this file).Ensure no other code changes are required: callers still pass the same
errvalue, but the log line is now safe because it no longer prints the raw error string.No changes are required in the other files (
sdk/cliproxy/auth/conductor.go,pkg/llmproxy/registry/*.go,pkg/llmproxy/thinking/*.go) since they are not directly responsible for logging; they only participate in the dataflow.