Fix auth upload priority propagation#2923
Conversation
|
This pull request targeted The base branch has been automatically changed to |
There was a problem hiding this comment.
Code Review
This pull request introduces several enhancements to the CLIProxyAPI, including parameterized Docker builds, a new combined docker-compose configuration, and expanded usage reporting that now tracks client API key identifiers and session indices. It also implements API key alias support and sanitization for configuration entries. Review feedback correctly identifies the use of absolute host paths in the new docker-compose file, which should be replaced with relative paths or environment variables to ensure portability across different environments.
| volumes: | ||
| - ./config.yaml:/CLIProxyAPI/config.yaml | ||
| - ./auths:/root/.cli-proxy-api | ||
| - /Users/midai/.cli-proxy-api:/Users/midai/.cli-proxy-api |
There was a problem hiding this comment.
| ports: | ||
| - "8888:8080" | ||
| volumes: | ||
| - /Users/midai/Documents/octopus/data:/app/data |
e1ef9b7 to
42accdf
Compare
luispater
left a comment
There was a problem hiding this comment.
Summary
- Adds an
insecureflag toPOST /v0/management/api-callto optionally skip TLS certificate verification, and threads it intoapiCallTransport. - Improves observability for APICall failures by logging at
Errorand returning the underlyingDoerror string.
Blocking
- Please document the new request field
insecurein the APICall godoc (Request JSON section + curl examples). The comment currently listsauth_index,method,url,header,data, but notinsecure. - Please add a focused unit test for the insecure path (e.g.,
apiCallTransport(..., true)returns a*http.TransportwithTLSClientConfig != nilandInsecureSkipVerify == true, and thefalsepath keeps it unset). This is security-sensitive behavior and worth locking down.
Non-blocking / Suggestions
- Consider cloning
TLSClientConfigbefore mutating it when it’s already non-nil (defensive against shared configs fromproxyutil.BuildHTTPTransport). - Returning
errDo.Error()to the client could leak internal details; if that’s a concern, consider keepingerror: "request failed"and adding a separate sanitizeddetailsfield (or only include details when a debug flag is enabled). - PR metadata: the
auto-retarget-main-pr-to-dev / retargetcheck is failing; may need a rerun or base-branch adjustment depending on your workflow.
Test plan (not run locally)
go test ./...go test ./internal/api/handlers/management -run TestAPICallTransport
Summary
priorityduring runtime auth rebuildspriorityTest Plan
go test ./internal/api/handlers/managementdocker compose build cli-proxy-api