[TristanSpeakEasy]

Why

The generated CLI's HTTP client used the net/http default redirect policy. On a redirect to another host, net/http strips Authorization and Cookie but forwards every other header, so an API key sent in a custom header (for example x-api-key) reached the new host. On 307/308 the request body was replayed as well, and an https to http redirect would send the key in cleartext.

The default server URL is unlikely to redirect off-origin, so this mostly matters when --server-url points at a proxy, gateway or a mistyped host. Either way the CLI should never forward credentials to a server the user did not configure.

What changed

• internal/client now sets CheckRedirect to SameOriginRedirect. Redirects that keep the scheme, host and port (an explicit :443/:80 counts as the default) are still followed (up to 10). Anything else fails with an error naming both origins, for example refusing to follow a redirect from https://api.example.com to http://other.example.com: credentials are only sent to the configured server.
• Redirects that would change the request method are refused too. net/http turns a POST answered with 301/302/303 into a bodiless GET, so the CLI used to print the response to a different read request and exit 0. 307 and 308 keep the method and body and are still followed.
• A refused redirect is a RedirectRefusedError, classified as protocol_error / CLI_PROTOCOL with its own hint, instead of connection_error with network-connectivity hints.
• New internal/client/client_test.go covers a same-origin 307 (method and body kept), a same-origin 303 on a POST (refused), a cross-origin 307 with two local servers (the credential header never reaches the second server), the origin comparison cases, and the error classification.
• Review SDK regenerated; changeset added.

Testing

• TARGET=review make test-cli and TARGET=primary make test-cli: pass, including the new tests.
• make check-template-cli: passes.

Public-safety check

• This change contains no credentials, customer documents, private repository URLs, private filesystem paths, or unredacted private logs.
• Title, body, comments, and commit messages name no customers or customer-derived identifiers, private paths or trackers, or workflow provenance, and are understandable without private context (.claude/skills/public-repo-communication/SKILL.md).
• Generated fixtures and review SDK changes are public-safe.
• I reviewed git diff --check.

---

Summary by cubic

Fixes a credential leak in the generated CLI: net/http's default redirect policy forwards custom headers like an API key to the redirect target, which could be a different host or a scheme downgrade. The CLI now refuses redirects to a different scheme, host, or port, so credentials are only ever sent to the server the user configured; same-origin redirects still work (up to 10) and explicit default ports are treated as the same origin. A new test covers both behaviors and asserts the API key never reaches the second server.

^{Written for commit 9809f79. Summary will update on new commits.}

[cubic-dev-ai]

1 issue found across 7 files

Confidence score: 2/5

• In templates/templates/cli/auxiliary/internal/client/client.go.stmpl, test mode uses TestHTTPClient, which can forward API-key headers across cross-origin redirects. Apply the protected client's redirect policy to the test client.

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="templates/templates/cli/auxiliary/internal/client/client.go.stmpl">

<violation number="1" location="templates/templates/cli/auxiliary/internal/client/client.go.stmpl:87">
P1: Test mode bypasses this redirect policy: `NewClient` replaces the protected client with `TestHTTPClient`, whose underlying client follows cross-origin redirects and can forward API-key headers. Apply the same policy to the test client and cover the path through `NewClient`.</violation>
</file>

<sub>Shadow auto-approve: would not auto-approve because issues were found.

View guided diff | Turn on auto-fix | Re-trigger cubic</sub>

[cubic-dev-ai]

0 issues found across 5 files (changes from recent commits).

Confidence score: 5/5

• Automated review surfaced no issues in the provided summaries.
• No files require special attention.

<sub>Shadow auto-approve: would not auto-approve. Auto-approval blocked by 1 unresolved issue from previous reviews.

View guided diff | Turn on auto-fix | Re-trigger cubic</sub>

[AshGodfrey]

Verified the fix on a generated CLI against two local servers on different hostnames: 301, 302, 307 and 308 from the configured origin to the other host are refused, the custom credential header never reaches the second host, and GET operations are covered as well.

Two things before merge, both in the same CheckRedirect function:

1. Same-origin 301/302/303 on a POST still becomes a bodiless GET. net/http rewrites the method on those codes, and the same-origin policy lets it through. The CLI then sends GET <Location> with no body, prints that response as the result of the original request, and exits 0. Reproduced locally: POST /items → GET /landed, body 0 bytes, exit 0. A mutation silently turning into a read of a different resource is wrong regardless of origin. Suggest refusing when req.Method != via[len(via)-1].Method, with an error saying the server tried to convert the request. 307 and 308 keep the method and body and can stay allowed. The new client_test.go already has the two-server setup, so this is a few lines plus one case.

2. The refusal is classified as a connection error. It surfaces as Error (connection_error) / CLI_CONNECTION with "Check network connectivity" and "Verify the server URL" hints, in both human and JSON output. That points people at their network for a deliberate client-side stop. It should carry its own message, and ideally its own classification, so JSON consumers can tell "refused by client" from "could not connect".

Agree with leaving the test client as is; it only exists under the test harness.

[TristanSpeakEasy]

@AshGodfrey thanks, both done in 14793e0 (replying to #137 (comment)):

1. SameOriginRedirect now also refuses a redirect when req.Method differs from the previous hop's method, with refusing to follow a redirect to /landed that would send GET instead of POST: the server tried to convert the request. 307/308 still go through with method and body intact. client_test.go covers both (a POST through 307 lands as POST with its body, and a POST through 303 never reaches the target).
2. Refusals are now a RedirectRefusedError. It's classified as protocol_error / CLI_PROTOCOL and carries its own hint (point --server-url at the endpoint the server redirects to) in place of the connectivity hints, in both human and JSON output. I went with the existing protocol class rather than adding a new type/reason so the documented reason list doesn't change, and it already means "the server responded, but not in a way the CLI accepts". Happy to split it into its own reason if you'd rather JSON consumers get something more specific. The test asserts the classification and that no connectivity hint is attached.

[cubic-dev-ai]

0 issues found across 7 files (changes from recent commits).

Confidence score: 5/5

• Automated review surfaced no issues in the provided summaries.
• No files require special attention.

<sub>Shadow auto-approve: would auto-approve. Fixes a credential leak in the generated CLI by refusing cross-origin and method-changing redirects, with focused tests for the redirect policy and error classification; the dismissed test-client caveat is confined to the generated test harness, not user CLI paths.

View guided diff | Turn on auto-fix | Re-trigger cubic</sub>