feat: Add validation for provided URL when running caib login#269
Conversation
📝 WalkthroughWalkthroughThe login command centralizes server URL handling by adding ChangesLogin Server URL Validation and Reachability
sequenceDiagram
participant CLI as rgba(46,125,50,0.5) CLI (runLogin)
participant Normalizer as rgba(30,136,229,0.5) normalizeServerURL
participant HTTP as rgba(255,138,101,0.5) checkServerReachable (HTTP)
participant Server as rgba(123,31,162,0.5) Server /v1/healthz
participant FS as rgba(3,169,244,0.5) Config File
CLI->>Normalizer: input server arg
Normalizer-->>CLI: normalized canonical URL or error
CLI->>HTTP: GET <server>/v1/healthz (10s timeout)
HTTP->>Server: request
Server-->>HTTP: response (any status)
HTTP-->>CLI: success or network error
alt reachable
CLI->>FS: persist server URL
FS-->>CLI: write success
else unreachable
CLI-->>CLI: abort, return error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
|
@ambient-code please review |
![ambient-code[bot]](https://avatars.githubusercontent.com/u/231559327?s=60&v=4){kind=small}
There was a problem hiding this comment.
Good refactoring — extracting normalizeServerURL into a standalone function makes the validation logic testable and the tests are well-structured. Two things worth discussing before merging:
Behavioral change beyond validation: This PR also reorders the login flow so that auth.GetTokenWithReauth runs before config.SaveServerURL, and changes auth failure from a warning (fmt.Printf) to a fatal error (handleError → os.Exit(1)). That means users who could previously save a URL despite auth issues (e.g., kubeconfig/token users, users not yet on VPN, expired OIDC) can no longer do so. The issue (#138) asks for syntactic validation before saving — the reachability gate is a separate concern and might warrant discussion. A lighter option would be a simple health check against /v1/healthz (like DeriveServerFromJumpstarter already does) instead of tying reachability to the full auth flow.
simulateUnreachableAuth test coupling: The helper knows to hit /v1/auth/config, which is an internal detail of auth.GetTokenWithReauth. If that endpoint changes, this test will silently stop testing the right thing. Consider noting the coupling in a comment, or testing via runLogin directly (though that requires handling the os.Exit).
|
@ambient-code please review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
cmd/caib/login.go (2)
56-58: 💤 Low valueMissing
MinVersionintls.Configflagged by static analysis.Without an explicit
MinVersion, Go's TLS client defaults to TLS 1.2, so TLS 1.0/1.1 connections are theoretically possible against legacy servers. Since this block only executes when the user has already opted into--insecure-skip-tls-verify, the practical impact is minimal, but setting an explicit floor keeps the security posture consistent.🔒 Proposed fix
- transport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} //nolint:gosec + transport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true, MinVersion: tls.VersionTLS12} //nolint:gosec🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/login.go` around lines 56 - 58, When building the TLS config in the insecure-skip branch, explicitly set a minimum TLS version to avoid allowing legacy TLS 1.0/1.1; update the transport.TLSClientConfig assignment (the tls.Config created when insecureSkipTLS is true) to include MinVersion: tls.VersionTLS12 so the config becomes &tls.Config{InsecureSkipVerify: true, MinVersion: tls.VersionTLS12}, keeping the existing nolint if needed.
65-65: 💤 Low valueRedundant error context when wrapping at the call site creates a confusing double message.
checkServerReachablereturns"server is not reachable: <cause>"(line 65), which is then wrapped at line 89 as"cannot connect to server %q: server is not reachable: <cause>". Both phrases say the same thing.Either drop the prefix in
checkServerReachableand letrunLoginown the full message, or keep the inner prefix and don't double-wrap at line 89.✂️ Option A – remove the prefix inside `checkServerReachable` (line 65)
- return fmt.Errorf("server is not reachable: %w", err) + return err✂️ Option B – don't wrap at `runLogin` (line 89)
- handleError(fmt.Errorf("cannot connect to server %q: %w", server, err)) + handleError(err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/login.go` at line 65, The inner function checkServerReachable currently returns an error prefixed with "server is not reachable: …", which gets double-wrapped by runLogin; change checkServerReachable to return the underlying error (remove the "server is not reachable: " prefix and just return err or wrap only the cause) so that runLogin's fmt.Errorf("cannot connect to server %q: %w", host, err) is the single place providing the human-facing context; update checkServerReachable's return at the spot that currently does return fmt.Errorf("server is not reachable: %w", err) to instead return the raw err (or fmt.Errorf("%w", err)) and leave runLogin's wrapping as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/caib/login_test.go`:
- Around line 109-126: The tests TestLoginDoesNotSaveConfigWhenServerUnreachable
and TestLoginSavesConfigWhenServerReachable are only calling
checkServerReachable and/or config.SaveServerURL directly so they don't exercise
runLogin's ordering/guard; either update the tests to invoke runLogin (so the
guard that calls checkServerReachable before config.SaveServerURL is validated)
by making handleError injectable or catching os.Exit via recover/test helper, or
instead rename the tests to reflect they only validate
checkServerReachable/config.SaveServerURL in isolation (e.g.,
TestCheckServerReachable_DoesNotWriteConfig and TestSaveServerURL_PersistsURL)
and add a new integration-style test that calls runLogin to assert the
conditional behavior. Ensure references to runLogin, checkServerReachable,
config.SaveServerURL, and handleError are used to locate the code to change.
In `@cmd/caib/login.go`:
- Around line 25-34: The input handling currently unconditionally prepends
"https://" to server, which makes explicit-scheme inputs like "ftp://..." be
mangled and makes the later parsedURL.Scheme check unreachable; change the logic
in the login URL parsing so you first detect whether the original raw input
includes an explicit scheme (e.g., contains "://", or use url.Parse on raw and
check parsed.Scheme), and only if there is no explicit scheme treat it as a bare
hostname and prepend "https://"; then call url.Parse on the normalized server,
validate parsedURL.Scheme against allowed schemes ("http"/"https") and produce
an accurate error for unsupported explicit schemes, using the same variables
(raw, server, parsedURL, parsedURL.Scheme) and url.Parse to locate and update
the code.
---
Nitpick comments:
In `@cmd/caib/login.go`:
- Around line 56-58: When building the TLS config in the insecure-skip branch,
explicitly set a minimum TLS version to avoid allowing legacy TLS 1.0/1.1;
update the transport.TLSClientConfig assignment (the tls.Config created when
insecureSkipTLS is true) to include MinVersion: tls.VersionTLS12 so the config
becomes &tls.Config{InsecureSkipVerify: true, MinVersion: tls.VersionTLS12},
keeping the existing nolint if needed.
- Line 65: The inner function checkServerReachable currently returns an error
prefixed with "server is not reachable: …", which gets double-wrapped by
runLogin; change checkServerReachable to return the underlying error (remove the
"server is not reachable: " prefix and just return err or wrap only the cause)
so that runLogin's fmt.Errorf("cannot connect to server %q: %w", host, err) is
the single place providing the human-facing context; update
checkServerReachable's return at the spot that currently does return
fmt.Errorf("server is not reachable: %w", err) to instead return the raw err (or
fmt.Errorf("%w", err)) and leave runLogin's wrapping as-is.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a0c5e4b1-4bed-4e2c-9384-a2fe21011ea5
📒 Files selected for processing (2)
cmd/caib/login.gocmd/caib/login_test.go
| func TestLoginDoesNotSaveConfigWhenServerUnreachable(t *testing.T) { | ||
| tmpDir := t.TempDir() | ||
| t.Setenv("XDG_CONFIG_HOME", tmpDir) | ||
|
|
||
| cfgPath := filepath.Join(tmpDir, "caib", "cli.json") | ||
| if _, err := os.Stat(cfgPath); !os.IsNotExist(err) { | ||
| t.Fatal("config file should not exist before test") | ||
| } | ||
|
|
||
| // checkServerReachable fails → SaveServerURL must not be called. | ||
| if err := checkServerReachable("http://127.0.0.1:1", false); err == nil { | ||
| t.Fatal("expected reachability error, got nil") | ||
| } | ||
|
|
||
| if _, err := os.Stat(cfgPath); !os.IsNotExist(err) { | ||
| t.Error("config file must not be written when the server is unreachable") | ||
| } | ||
| } |
There was a problem hiding this comment.
TestLoginDoesNotSaveConfigWhenServerUnreachable is trivially true and doesn't test the runLogin guard.
The test calls checkServerReachable (which errors), then asserts the config file doesn't exist — but config.SaveServerURL is never called in the test body, so the second assertion is vacuously true regardless of any implementation change. The critical integration behaviour described in the PR ("runLogin guards SaveServerURL behind checkServerReachable") is not exercised at all. If the order in runLogin were reversed (save first, then check reachability), this test would still pass.
The same applies to TestLoginSavesConfigWhenServerReachable (lines 130–160): it calls checkServerReachable and config.SaveServerURL in sequence but never invokes runLogin, so it can't catch ordering or conditional bugs in the actual command flow.
Consider either:
- Refactoring both tests to call
runLogindirectly (possibly extractinghandleErroras an injectable hook, or usingrecoverto catch theos.Exit), or - Renaming them to accurately reflect what is being tested (e.g.
TestCheckServerReachable_DoesNotWriteConfig,TestSaveServerURL_CorrectlyPersistsURL) to avoid misleading future contributors.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/caib/login_test.go` around lines 109 - 126, The tests
TestLoginDoesNotSaveConfigWhenServerUnreachable and
TestLoginSavesConfigWhenServerReachable are only calling checkServerReachable
and/or config.SaveServerURL directly so they don't exercise runLogin's
ordering/guard; either update the tests to invoke runLogin (so the guard that
calls checkServerReachable before config.SaveServerURL is validated) by making
handleError injectable or catching os.Exit via recover/test helper, or instead
rename the tests to reflect they only validate
checkServerReachable/config.SaveServerURL in isolation (e.g.,
TestCheckServerReachable_DoesNotWriteConfig and TestSaveServerURL_PersistsURL)
and add a new integration-style test that calls runLogin to assert the
conditional behavior. Ensure references to runLogin, checkServerReachable,
config.SaveServerURL, and handleError are used to locate the code to change.
There was a problem hiding this comment.
Re-review — Thanks for updating the PR. The main concern from the first review (coupling reachability to the full auth flow) has been addressed well — checkServerReachable hitting /v1/healthz is much cleaner.
Two remaining issues, one correctness and one cosmetic:
Assisted-by: claude-4.6-sonnet Signed-off-by: Bella Khizgiyaev <bkhizgiy@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/caib/login.go (1)
64-68: 💤 Low valueConsider using defer for response body cleanup.
While the current approach works, using
deferis more idiomatic and ensures cleanup even if the function is later modified to add additional logic paths.♻️ Proposed refactor
resp, err := client.Get(strings.TrimSuffix(serverURL, "/") + "/v1/healthz") if err != nil { return err } - _ = resp.Body.Close() + defer resp.Body.Close() return nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/login.go` around lines 64 - 68, Replace the immediate resp.Body.Close() call with a deferred close to ensure the response body is always cleaned up even if future logic returns early: after the client.Get(...) call and its error check (the block using client.Get and if err != nil { return err }), add defer resp.Body.Close() instead of the current "_ = resp.Body.Close()"; this uses resp from the Get call and guarantees cleanup of resp.Body.Close.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/caib/login.go`:
- Around line 56-59: The TLS config created in the login flow (the transport
variable and tls.Config used when insecureSkipTLS is true) must also set a
minimum TLS version; update the tls.Config initialization in the block that sets
InsecureSkipVerify to include MinVersion: tls.VersionTLS12 (or higher) so the
config enforces a minimum protocol even when InsecureSkipVerify is enabled;
modify the tls.Config in the transport assignment where
tls.Config{InsecureSkipVerify: true} is created to add MinVersion:
tls.VersionTLS12.
---
Nitpick comments:
In `@cmd/caib/login.go`:
- Around line 64-68: Replace the immediate resp.Body.Close() call with a
deferred close to ensure the response body is always cleaned up even if future
logic returns early: after the client.Get(...) call and its error check (the
block using client.Get and if err != nil { return err }), add defer
resp.Body.Close() instead of the current "_ = resp.Body.Close()"; this uses resp
from the Get call and guarantees cleanup of resp.Body.Close.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0d374862-6bca-42b0-9330-00e26e009dad
📒 Files selected for processing (2)
cmd/caib/login.gocmd/caib/login_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/caib/login_test.go
2 participants
Summary
Related Issues
Type of Change
Testing
make test)make lint)make manifests generate)releted to: #138
Summary by CodeRabbit
Bug Fixes
Tests