fix: stop login --ignore-ssl-errors panicking on a wrapped transport - #726
Conversation
bbe12fb to
655cee0
Compare
|
@flin-8 from the session I had with Claude from the start on this: |
|
I'm not sure how I hit the close button 😊 |
NickJosevski
left a comment
NickJosevski
left a comment
There was a problem hiding this comment.
I'm ok for this to go, but just put in the comments I pulled from what I was doing with Claude in this space.
| // The transport the client factory hands out is always spinner-wrapped, which is | ||
| // what the old type assertion tripped over. | ||
| func TestSkipTlsVerification_ThroughTheSpinner(t *testing.T) { | ||
| spinner := apiclient.NewSpinnerRoundTripper(nil) |
There was a problem hiding this comment.
@flin-8 with the session I had going with Claude that spun up context from when we first started to fix the original driver for the changes in this space, on this it brings up:
sets Next: http.DefaultTransport, the process-wide *http.Transport every client without its own uses.
|
|
||
| // The client factory wraps the transport in a spinner, so the setting has to be | ||
| // applied to the transport underneath rather than to the wrapper. | ||
| func skipTlsVerification(roundTripper http.RoundTripper) { |
There was a problem hiding this comment.
from my claude session:
skipTlsVerification recurses past the spinner and assigns TLSClientConfig on that shared object, so unrelated clients stop verifying certificates.
| } | ||
|
|
||
| httpClient.Transport.(*http.Transport).TLSClientConfig = &tls.Config{InsecureSkipVerify: true} | ||
| skipTlsVerification(httpClient.Transport) |
There was a problem hiding this comment.
from my claude session:, in general:
http.Client{}, never passed to the function, flips from rejecting a self-signed cert to accepting it; cloning instead leaves it rejecting.
|
I threw that Claude session at this also yesterday, and it proposed this PR: |
The client factory hands login a client whose transport is the spinner round tripper, so type-asserting it to *http.Transport panics. Apply the setting underneath any wrappers instead, leaving them in place. Fixes #725 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
655cee0 to
574ccda
Compare
2 participants
Fixes #725.
Fixes SF-3525: Octopus CLI crash
login --ignore-ssl-errorstype-asserted the client's transport to*http.Transport. Since #633 the client factory always installs the spinner round tripper (it was interactive-only before, so non-interactive runs had a nil transport and the assertion held), so that assertion panics for anyone with a server and API key already in config or environment — including every Octopus Jenkins plugin build with "Ignore SSL Errors" set.skipTlsVerificationwalks past the spinner and applies the setting to the transport underneath. A transport it does not recognise returns an error, rather than quietly leaving verification on after the caller asked for the opposite. Everything else, including the existing nil-transport handling, is unchanged.The login test panics without the fix — the mock factory's transport is not an
*http.Transporteither, so it now reports the unsupported transport instead. Internal tests cover the spinner path and the error.Thanks to #727, which fixed the same bug independently; the error on unrecognised transports is taken from it.
🤖 Generated with Claude Code