fix(llm): sanitize short user_id across transformers to comply with c… - #1812
fix(llm): sanitize short user_id across transformers to comply with c…#1812survivor998 wants to merge 1 commit into
Conversation
Greptile SummaryThis PR introduces a shared
Confidence Score: 3/5Safe to merge for Anthropic; Doubao and Zai have an incomplete fix on the User-field fallback path that could still send unsanitized data to the upstream API. The Anthropic transformer is clean. For Doubao and Zai, when llm/transformer/doubao/outbound.go and llm/transformer/zai/outbound.go need a null-out or replacement of the embedded Request.User after setting UserID. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[llm.Request] --> B{Metadata user_id set?}
B -- Yes --> C[userID = Metadata user_id]
B -- No --> D{User field set?}
D -- Yes --> E[userID = *User]
D -- No --> F[userID stays empty]
C --> G{userID != empty?}
E --> G
F --> G
G -- Yes --> H[SanitizeUserID]
G -- No --> I[Skip user field]
H --> H1[Replace invalid chars with _]
H1 --> H2{len < 6?}
H2 -- Yes --> H3[Prepend user_]
H2 -- No --> H4{len > 128?}
H3 --> H4
H4 -- Yes --> H5[Truncate to 128]
H4 -- No --> H6[Return sanitized]
H5 --> H6
H6 --> J{Transformer}
J -- Anthropic --> K[AnthropicMetadata UserID - clean]
J -- Doubao --> L[doubaoReq.UserID sanitized BUT doubaoReq.Request.User still has original]
J -- Zai --> M[zaiReq.UserID sanitized BUT zaiReq.Request.User still has original]
Reviews (1): Last reviewed commit: "fix(llm): sanitize short user_id across ..." | Re-trigger Greptile |
| var userID string | ||
| if llmReq.Metadata != nil && llmReq.Metadata["user_id"] != "" { | ||
| userID = llmReq.Metadata["user_id"] | ||
| } else if llmReq.User != nil && *llmReq.User != "" { | ||
| userID = *llmReq.User | ||
| } | ||
|
|
||
| if userID != "" { | ||
| doubaoReq.UserID = shared.SanitizeUserID(userID) | ||
| } |
There was a problem hiding this comment.
Unsanitized
user leaks into the upstream request on the fallback path
When llmReq.User is used as the source (i.e., Metadata["user_id"] is absent), openai.RequestFromLLM has already copied that same pointer to doubaoReq.Request.User (line 29 of openai/outbound_convert.go). The JSON payload therefore contains both "user_id": "<sanitized>" and "user": "<original unsanitized>" simultaneously. If the Doubao API validates the user field under the same ^[a-zA-Z0-9_-]{6,128}$ constraint, the sanitization here is bypassed. The same issue exists in zai/outbound.go. Consider clearing doubaoReq.Request.User = nil (or setting it to the sanitized value) after setting doubaoReq.UserID.
| var userID string | ||
| if llmReq.Metadata != nil && llmReq.Metadata["user_id"] != "" { | ||
| userID = llmReq.Metadata["user_id"] | ||
| } else if llmReq.User != nil && *llmReq.User != "" { | ||
| userID = *llmReq.User | ||
| } | ||
|
|
||
| if userID != "" { | ||
| zaiReq.UserID = shared.SanitizeUserID(userID) | ||
| } |
There was a problem hiding this comment.
Unsanitized
user leaks into the upstream request on the fallback path
Same issue as in doubao/outbound.go: when llmReq.User is the source, openai.RequestFromLLM copies llmReq.User into zaiReq.Request.User. The serialized request therefore includes both the sanitized "user_id" and the original unsanitized "user" field. If the Zai API validates both fields, the constraint violation this PR is fixing is still present via the user field. Clearing zaiReq.Request.User after setting zaiReq.UserID would fully close the gap.
| sanitized = "user_" + sanitized | ||
| if len(sanitized) < 6 { | ||
| sanitized = sanitized + strings.Repeat("_", 6-len(sanitized)) | ||
| } |
There was a problem hiding this comment.
Inner padding branch is unreachable dead code
After the loop, sanitized is always non-empty for any non-empty userID (every rune, including invalid UTF-8 replacement characters, writes at least one byte to the builder). So "user_" + a non-empty sanitized is always ≥ 6 bytes, and the inner if len(sanitized) < 6 condition can never be true. The block can be dropped without changing behavior.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
…onstraints