fix(legacy): use the detected CA bundle for the login token request - #138
Open
pjcdawkins wants to merge 4 commits into
Open
fix(legacy): use the detected CA bundle for the login token request#138pjcdawkins wants to merge 4 commits into
pjcdawkins wants to merge 4 commits into
Conversation
The authorization code exchange in auth:browser-login built its own Guzzle client with `verify => true`. Guzzle then enables peer verification without setting CURLOPT_CAINFO, so curl falls back to its default CA source. Every other request in the CLI passes an explicit path from Composer\CaBundle, which honors SSL_CERT_FILE and SSL_CERT_DIR. Behind a TLS-inspecting proxy that made login fail at the last step with "cURL error 60: SSL certificate problem", even though the rest of the CLI worked with the corporate root CA in SSL_CERT_FILE. On Windows the fallback is always wrong: the wrapper runs PHP with -n and -d openssl.cafile pointing at the bundled Mozilla CA list, and PHP's curl extension takes its default CAINFO from that setting, so this request could only ever trust the bundled list. Use Api::getExternalHttpClient() instead, which applies the detected CA bundle, the configured proxy, the user agent and the api.skip_ssl option. It logs the request line and status at -vvv, never message bodies, so tokens stay out of the output. Reported in #110 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The local OAuth listener server is started with the same PHP binary as the CLI, but without -n, so it parses any php.ini it can find even when the CLI itself was started without one. Under the Go wrapper that means the static PHP binary loads a system php.ini, which produced warnings like 'Module "curl" is already loaded' for one user, and could stop the server from starting at all with a php.ini that enables extensions this build cannot load. Pass -n to the server when this process parsed no php.ini either. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Guzzle reads http_proxy, https_proxy and no_proxy itself, but only when the proxy request option is absent. The CLI always sets that option from Config::getProxies(), so the no-proxy list was silently dropped for every API request. This also keeps the login token request behaving as it did before it started using the shared HTTP client, which until then was left to Guzzle's own environment handling. Both cases of the variable name are accepted, since the effect is only ever to send a request directly instead of through a proxy. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
📋 PR Summary This incremental push updates two test files in the legacy PHP CLI. Changes
|
![upsun-dispatch[bot]](https://avatars.githubusercontent.com/in/4153575?s=60&v=4){kind=small}
There was a problem hiding this comment.
📋 Upsun Dispatch Review: full · 5 files reviewed · no issues found
Review details
Commit: Commit f208375
Model: claude-opus-4-8
Panel: security · correctness · robustness · design
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes upsun login failures behind TLS-inspecting proxies by ensuring the OAuth token exchange uses the legacy CLI’s shared external HTTP client (so it respects the detected CA bundle, proxy settings, and api.skip_ssl). It also aligns the spawned local OAuth listener PHP process with the parent process’s php.ini parsing behavior and restores no_proxy support when the CLI explicitly configures Guzzle proxies.
Changes:
- Route the OAuth2 token request in
auth:browser-loginthroughApi::getExternalHttpClient()instead of creating a standalone Guzzle client. - Add
-nwhen spawning the local PHP web server if the current process did not load/scan any php.ini. - Add
Config::getNoProxy()and passno_proxyhosts through to Guzzle proxy configuration, with new unit tests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| legacy/src/Command/Auth/BrowserLoginCommand.php | Uses shared external HTTP client for token exchange; adjusts local PHP server args to optionally include -n. |
| legacy/src/Service/Api.php | Extends Guzzle proxy config to include no_proxy hosts and updates return type annotation. |
| legacy/src/Service/Config.php | Adds parsing of no_proxy/NO_PROXY into a normalized host list. |
| legacy/tests/ConfigTest.php | Adds unit test coverage for Config::getNoProxy(). |
| legacy/tests/Command/Auth/BrowserLoginCommandTest.php | Adds unit test ensuring token exchange uses the shared external HTTP client. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+132
to
+148
| public function testGetNoProxy(): void | ||
| { | ||
| $config = new Config([], $this->configFile); | ||
| try { | ||
| $this->assertEquals([], $config->getNoProxy()); | ||
|
|
||
| putenv('no_proxy=example.com, .example.net,'); | ||
| $this->assertEquals(['example.com', '.example.net'], $config->getNoProxy()); | ||
| putenv('no_proxy'); | ||
|
|
||
| putenv('NO_PROXY=example.org'); | ||
| $this->assertEquals(['example.org'], $config->getNoProxy()); | ||
| } finally { | ||
| putenv('no_proxy'); | ||
| putenv('NO_PROXY'); | ||
| } | ||
| } |
Comment on lines
+38
to
+41
| ->willReturnCallback(function (RequestInterface $request) use (&$sentRequest): Response { | ||
| $sentRequest = $request; | ||
| return new Response(200, [], '{"access_token": "test-token", "token_type": "bearer"}'); | ||
| }); |
The no_proxy test asserted an empty list without first clearing the variables, so it failed where they are already set, and it cleared them afterwards instead of restoring them. It now saves and restores both. The login test also records the request options, so that it covers the client credentials still being sent. Written by Claude Code.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related to #110. This fixes one inconsistency in the login path, but not the whole problem reported there — see "Scope" below.
Problem
Behind a TLS-inspecting proxy, with a CA bundle configured in
SSL_CERT_FILE, every request works except the last step ofupsun login:Cause
The authorization code exchange in
auth:browser-loginbuilt its own Guzzle client withverify => true. Guzzle then enables peer verification without settingCURLOPT_CAINFO, so curl falls back to its default CA source. Every other request in the CLI passes an explicit path fromComposer\CaBundle, which honorsSSL_CERT_FILEandSSL_CERT_DIR.That default is not a neutral one. The wrapper starts PHP with
-nand-d openssl.cafile=<cache>/cacert.pem, and PHP's curl extension takes its defaultCURLOPT_CAINFOfromopenssl.cafile(falling back tocurl.cainfo), ignoringSSL_CERT_FILE. So this one request always verified against the bundled Mozilla CA list, whatever the user configured.The local OAuth listener was not involved: it only redirects the browser and writes a file, and makes no outbound requests.
Changes
Api::getExternalHttpClient()for the token request. It applies the detected CA bundle, the configured proxy, the user agent and theapi.skip_ssloption. It logs the request line and response status at-vvvand never message bodies, so tokens stay out of the output.-nto the local web server when this process parsed no php.ini either. Otherwise the static PHP binary loads a system php.ini, which producedModule "curl" is already loadedwarnings for the reporter and could stop the server from starting at all.no_proxy. Guzzle reads the proxy environment variables itself, but only when theproxyrequest option is absent, and the CLI always sets it. So the no-proxy list was being dropped for every API request, and would have been dropped for the token request once it moved to the shared client.Scope
This helps users who have already pointed
SSL_CERT_FILEat a CA bundle. It does not help users whose additional root certificate is installed in the operating system's trust store, which is the usual arrangement:lib/vtls/schannel.c:CAfileimpliesuse_manual_cred_validation;schannel_verify.cbuilds the chain engine withhExclusiveRoot), so pinningopenssl.cafilemeans the Windows certificate store is never consulted./etc/ssl/cert.pem, which does not include roots added to the keychain.Composer\CaBundlecannot close this gap: it discovers bundle paths and otherwise returns its own bundled Mozilla list, with no knowledge of either OS store.Meanwhile the Go half of the same binary uses the platform verifier on Windows and ignores
SSL_CERT_FILE, so the two halves can disagree about the same certificate.That needs a separate fix — most likely making the CA bundle the wrapper pins include the OS trust store, plus a documented option for pointing both layers at a bundle explicitly. It is worth confirming on a Windows machine first: the Schannel behavior above is read from curl's source, not observed.
Verification
New unit tests cover the client used for the token request and the parsing of
no_proxy. Using the embedded PHP binary withopenssl.cafilepointed at an unrelated CA and a working bundle inSSL_CERT_FILE, the old client reproduces the reported cURL error 60 while the new one succeeds; the listener still redirects correctly when started with-n; and with a dead proxy configured, a request goes direct when its host is inno_proxyand fails when it is not.Written by Claude Code.