[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 and host 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.
• New internal/client/client_test.go covers a same-origin redirect and a cross-origin 307 using two local servers, and asserts the credential header never reaches the second server.
• Review SDK regenerated; changeset added.

Testing

• TARGET=review make test-cli: passes, including the new test.
• make check-template-cli and make lint: pass.
• TARGET=primary make test-cli: the redirect test passes; the suite's interactive configure form tests need to run outside an environment that triggers automatic agent mode.

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: 3/5

• In test mode, NewClient replaces the protected client, so cross-origin redirects can forward API-key headers. Apply the same redirect policy to TestHTTPClient.

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]

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.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. 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. At templates/templates/cli/auxiliary/internal/client/client.go.stmpl, line 87:

<comment>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`.</comment>

<file context>
@@ -83,7 +84,7 @@ func NewClient(cmd *cobra.Command, allowedSecurityFields ...string) (*sdk.{{temp
     // Diagnostics and test client composition.
     // Order: test client (innermost) → diagnostics wrapper (outermost).
-    var httpClient HTTPClient = &http.Client{Transport: newPhaseBoundedTransport(cmd)}
+    var httpClient HTTPClient = &http.Client{Transport: newPhaseBoundedTransport(cmd), CheckRedirect: SameOriginRedirect}
     if testClient := testclient.NewTestHTTPClient(); testClient != nil {
         httpClient = testClient
</file context>

[TristanSpeakEasy]

Leaving this one as is. TestHTTPClient is only constructed when SPEAKEASY_TEST_NAME is set by the generated test harness, and it already bypasses the production transport (phase timeouts) entirely, so it is not a path an end user's CLI takes. The redirect policy is covered directly by TestSameOriginRedirect / TestSameOriginRedirectOrigins.

[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.