Skip to content

Fix auth upload priority propagation#2923

Open
xj999 wants to merge 1 commit into
router-for-me:devfrom
xj999:codex/auth-upload-priority-fix
Open

Fix auth upload priority propagation#2923
xj999 wants to merge 1 commit into
router-for-me:devfrom
xj999:codex/auth-upload-priority-fix

Conversation

@xj999
Copy link
Copy Markdown

@xj999 xj999 commented Apr 20, 2026

Summary

  • reuse the auth file synthesizer in the management upload path so uploaded OAuth files populate runtime auth attributes consistently
  • preserve top-level auth-file fields such as priority during runtime auth rebuilds
  • add a regression test covering uploaded auth files with a top-level priority

Test Plan

  • go test ./internal/api/handlers/management
  • docker compose build cli-proxy-api

@github-actions github-actions Bot changed the base branch from main to dev April 20, 2026 05:22
@github-actions
Copy link
Copy Markdown

This pull request targeted main.

The base branch has been automatically changed to dev.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread docker-compose.combined.yml Outdated
volumes:
- ./config.yaml:/CLIProxyAPI/config.yaml
- ./auths:/root/.cli-proxy-api
- /Users/midai/.cli-proxy-api:/Users/midai/.cli-proxy-api
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The volume mapping uses an absolute host path (/Users/midai/.cli-proxy-api) which is specific to a local environment. This makes the configuration non-portable and may cause issues for other developers. Please use a relative path or an environment variable instead.

Comment thread docker-compose.combined.yml Outdated
ports:
- "8888:8080"
volumes:
- /Users/midai/Documents/octopus/data:/app/data
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The volume mapping uses an absolute host path (/Users/midai/Documents/octopus/data) which is specific to a local environment. This makes the configuration non-portable. Please use a relative path or an environment variable instead.

@xj999 xj999 force-pushed the codex/auth-upload-priority-fix branch from e1ef9b7 to 42accdf Compare April 20, 2026 08:27
Copy link
Copy Markdown
Collaborator

@luispater luispater left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

  • Adds an insecure flag to POST /v0/management/api-call to optionally skip TLS certificate verification, and threads it into apiCallTransport.
  • Improves observability for APICall failures by logging at Error and returning the underlying Do error string.

Blocking

  • Please document the new request field insecure in the APICall godoc (Request JSON section + curl examples). The comment currently lists auth_index, method, url, header, data, but not insecure.
  • Please add a focused unit test for the insecure path (e.g., apiCallTransport(..., true) returns a *http.Transport with TLSClientConfig != nil and InsecureSkipVerify == true, and the false path keeps it unset). This is security-sensitive behavior and worth locking down.

Non-blocking / Suggestions

  • Consider cloning TLSClientConfig before mutating it when it’s already non-nil (defensive against shared configs from proxyutil.BuildHTTPTransport).
  • Returning errDo.Error() to the client could leak internal details; if that’s a concern, consider keeping error: "request failed" and adding a separate sanitized details field (or only include details when a debug flag is enabled).
  • PR metadata: the auto-retarget-main-pr-to-dev / retarget check 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants