-
Notifications
You must be signed in to change notification settings - Fork 14
fix(login): prevent panic with --ignore-ssl-errors on subsequent runs #549
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix(login): prevent panic with --ignore-ssl-errors on subsequent runs #549
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a panic that occurs when running octopus login --ignore-ssl-errors on subsequent runs by replacing an unsafe type assertion with proper type switching to handle multiple HTTP transport scenarios.
- Replaced unsafe type assertion with type switching to handle both direct
*http.Transportand*SpinnerRoundTripperwrapping scenarios - Added comprehensive test coverage for all transport scenarios including edge cases
- Maintained backward compatibility while preventing the panic on subsequent login attempts
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/cmd/login/login.go | Replaced unsafe type assertion with type switching to handle multiple transport types safely |
| pkg/cmd/login/login_ssl_test.go | Added comprehensive test coverage for SSL ignore logic with various transport scenarios |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
pkg/cmd/login/login_ssl_test.go
Outdated
| } | ||
| } | ||
|
|
||
| // applySSLIgnoreLogic contains the same logic as in loginRun for handling SSL |
Copilot
AI
Oct 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment should clarify that this function mirrors the SSL ignore logic from the main implementation to ensure test accuracy.
| // applySSLIgnoreLogic contains the same logic as in loginRun for handling SSL | |
| // applySSLIgnoreLogic mirrors the SSL ignore logic from the main implementation (loginRun) | |
| // to ensure test accuracy. If the SSL ignore logic in loginRun changes, this function | |
| // should be updated accordingly to keep the test valid. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment Updated
pkg/cmd/login/login_ssl_test.go
Outdated
| } else { | ||
| // If Next is not an http.Transport, replace it with one that has SSL verification disabled | ||
| transport.Next = &http.Transport{ | ||
| TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, | ||
| } | ||
| } |
Copilot
AI
Oct 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic is duplicated between the test file and the main implementation. Consider extracting this SSL ignore logic into a shared utility function to reduce code duplication and ensure consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed recommended to remove duplicate logic, and extracted SSL ignore logic to a shared functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
pkg/apiclient/ssl.go
Outdated
| transport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} | ||
| case *SpinnerRoundTripper: | ||
| // If the SpinnerRoundTripper's Next is an http.Transport, configure it | ||
| if httpTransport, ok := transport.Next.(*http.Transport); ok { | ||
| httpTransport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} |
Copilot
AI
Oct 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above - consider preserving existing TLS configuration when possible. If httpTransport.TLSClientConfig already exists, you should modify only the InsecureSkipVerify field rather than replacing the entire config.
| transport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} | |
| case *SpinnerRoundTripper: | |
| // If the SpinnerRoundTripper's Next is an http.Transport, configure it | |
| if httpTransport, ok := transport.Next.(*http.Transport); ok { | |
| httpTransport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} | |
| if transport.TLSClientConfig != nil { | |
| transport.TLSClientConfig.InsecureSkipVerify = true | |
| } else { | |
| transport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} | |
| } | |
| case *SpinnerRoundTripper: | |
| // If the SpinnerRoundTripper's Next is an http.Transport, configure it | |
| if httpTransport, ok := transport.Next.(*http.Transport); ok { | |
| if httpTransport.TLSClientConfig != nil { | |
| httpTransport.TLSClientConfig.InsecureSkipVerify = true | |
| } else { | |
| httpTransport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modified and tested
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Problem
When running
octopus login --ignore-ssl-errorsafter an initial successful login, the CLI crashes with a panic:This occurs because:
*http.Transport*SpinnerRoundTripperwrapping*http.Transport(for interactive spinner display)*http.Transportand performed unsafe type assertionSolution
Replaced the unsafe type assertion with proper type switching to handle multiple transport scenarios:
*http.Transport(first login)*SpinnerRoundTripperwrapping*http.Transport(subsequent logins)Changes
pkg/cmd/login/login.goto use type switchingpkg/cmd/login/login_ssl_test.goTesting
Related Issues
Fixes the panic described in the original issue where users couldn't run
octopus login --ignore-ssl-errorsmultiple times.