fix(security): mask API keys in log output - #1579
Conversation
Add MarshalLog and LogValue (slog.LogValuer) to AuthConfig so slog outputs API keys as sk-xxxx****xxxx instead of plaintext. Prevents credential leakage in application logs. Also adds a local maskAPIKey helper function consistent with the existing pattern in internal/server/orchestrator/tester.go.
There was a problem hiding this comment.
Code Review
This pull request implements API key masking for the AuthConfig struct by adding a LogValue method and a maskAPIKey helper function. Review feedback indicates that the Request struct should also implement LogValuer to ensure masking is applied when the parent object is logged. Additionally, the maskedAuthConfig struct should include JSON tags to maintain consistent field naming in log outputs.
| func (a AuthConfig) LogValue() slog.Value { | ||
| return slog.AnyValue(maskedAuthConfig{ | ||
| Type: a.Type, | ||
| APIKey: a.MarshalLog(), | ||
| HeaderKey: a.HeaderKey, | ||
| }) | ||
| } |
There was a problem hiding this comment.
The implementation of LogValue on AuthConfig is a good step, but it will not automatically mask the API key when the parent Request object is logged via slog.Any("request", request) (as seen in client.go:187).
Standard slog handlers (and the custom zap bridge in this repo) use reflection to log structs and do not recursively check for LogValuer implementations on nested fields. To fully address the security issue described in the PR, the Request struct should also implement slog.LogValuer to explicitly handle the masking of its Auth field.
| }) | ||
| } | ||
|
|
||
| type maskedAuthConfig struct{ Type, APIKey, HeaderKey string } |
There was a problem hiding this comment.
The maskedAuthConfig struct should include JSON tags to ensure that the log output remains consistent with the original AuthConfig field names and behavior (e.g., using snake_case and omitempty). Without these tags, a JSON log handler will output capitalized field names.
type maskedAuthConfig struct {
Type string "json:\"type\""
APIKey string "json:\"api_key\""
HeaderKey string "json:\"header_key,omitempty\""
}
Greptile SummaryThis PR adds
Confidence Score: 4/5Not safe to merge as-is — the primary credential-leakage path in client.go:187 is unaddressed. There is one P1 finding: the LogValue() implementation does not protect the main leakage site (slog.Any("request", request)) because slog's JSON handler uses json.Marshal for nested struct serialisation, which does not respect slog.LogValuer. The PR adds partial protection for direct attribute logging, but the stated goal of masking the key at client.go:187 is not achieved. llm/httpclient/model.go — LogValue() and maskedAuthConfig both need revision; client.go:187 also needs attention.
|
| Filename | Overview |
|---|---|
| llm/httpclient/model.go | Adds MarshalLog(), LogValue(), maskedAuthConfig, and maskAPIKey to AuthConfig — but LogValue() only fires when AuthConfig is a direct slog attribute, not when nested inside *Request (the primary leakage site in client.go:187); maskedAuthConfig also lacks json struct tags. |
Sequence Diagram
sequenceDiagram
participant Caller
participant slog
participant JSONHandler
participant json_Marshal as json.Marshal
Note over Caller,json_Marshal: Current path — LogValue is NOT invoked
Caller->>slog: slog.Any("request", ptr to Request)
slog->>slog: AnyValue → KindAny, not LogValuer
slog->>JSONHandler: handle KindAny value
JSONHandler->>json_Marshal: json.Marshal(Request struct)
json_Marshal-->>JSONHandler: api_key field serialised via struct tags — key exposed
Note over Caller,json_Marshal: Direct attribute path — LogValue IS invoked
Caller->>slog: slog.Any("auth", AuthConfig value)
slog->>slog: AnyValue → KindLogValuer
slog->>slog: Resolve calls LogValue()
slog-->>JSONHandler: maskedAuthConfig with APIKey masked
JSONHandler-->>Caller: masked output
Reviews (1): Last reviewed commit: "fix(security): mask API keys in log outp..." | Re-trigger Greptile
| func (a AuthConfig) LogValue() slog.Value { | ||
| return slog.AnyValue(maskedAuthConfig{ | ||
| Type: a.Type, | ||
| APIKey: a.MarshalLog(), | ||
| HeaderKey: a.HeaderKey, | ||
| }) | ||
| } |
There was a problem hiding this comment.
LogValue() does not protect the primary leakage path
The PR's stated goal is to fix slog.Any("request", request) in client.go:187, but LogValue() is not invoked there. When slog's handler encounters a KindAny value it cannot resolve as a LogValuer at the attribute level, it falls back to json.Marshal (for JSONHandler) or fmt.Sprintf (for TextHandler). Neither serializer is aware of slog.LogValuer, so *Request is serialized field-by-field via struct tags, and Auth *AuthConfig ends up as {"api_key":"sk-actual-key"} in the log — unchanged.
LogValue() is only invoked when AuthConfig/*AuthConfig is the direct value of a slog attribute (e.g., slog.Any("auth", config)), not when it is a nested field inside another struct.
To protect client.go:187, one of the following is needed:
- Have
Requestimplementslog.LogValuerand return a safe representation. - Have
AuthConfigimplementjson.Marshalerso thatjson.Marshalmasks the key when traversing*Request. - Replace
slog.Any("request", request)with individual safe attribute calls.
| }) | ||
| } | ||
|
|
||
| type maskedAuthConfig struct{ Type, APIKey, HeaderKey string } |
There was a problem hiding this comment.
maskedAuthConfig missing JSON struct tags
maskedAuthConfig has no json struct tags, so when slog's JSON handler serializes it (via json.Marshal), the output uses Go's exported field names — Type, APIKey, HeaderKey — instead of the snake_case tags (type, api_key, header_key) used by the original AuthConfig. Any log-parsing tooling or dashboards that match on the original key names will not find the masked values.
| type maskedAuthConfig struct{ Type, APIKey, HeaderKey string } | |
| type maskedAuthConfig struct { | |
| Type string `json:"type"` | |
| APIKey string `json:"api_key"` | |
| HeaderKey string `json:"header_key,omitempty"` | |
| } |
| // maskAPIKey returns a masked version of the API key for display. | ||
| // Shows first 4 and last 4 characters with **** in between. | ||
| func maskAPIKey(key string) string { | ||
| if len(key) <= 8 { | ||
| return "****" | ||
| } | ||
| return key[:4] + "****" + key[len(key)-4:] | ||
| } |
There was a problem hiding this comment.
Problem
API keys are logged in plaintext via slog (e.g.
slog.Any("request", request)inllm/httpclient/client.go:187), exposing credentials in application logs.Fix
Add
MarshalLogandLogValue(implementingslog.LogValuer) toAuthConfigso that:MarshalLog()returns the masked key string (sk-xxxx****xxxx) for explicit callsLogValue()ensures automatic masking whenAuthConfigis serialized by slogThe masked output preserves struct fields (Type, APIKey, HeaderKey) while masking only the sensitive APIKey value.
Also adds a local
maskAPIKeyhelper function consistent with the existing pattern ininternal/server/orchestrator/tester.go.Scope
Single file:
llm/httpclient/model.go