feat: add server ssh-info command for non-interactive SSH credentials#245
Hidden character warning
Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds an ChangesSSH Info Command Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/cmd/server/sshinfo/sshinfo.go`:
- Around line 83-85: The error returned when validating opts.id is too specific;
update the check in sshinfo.go (the code that currently tests opts.id == "" in
the function handling the sshinfo command) to return a generic message like
"server id is required" or "server id must be provided (via --id or positional
argument)" instead of "--id is required" so users know they can supply either
--id or the positional server-id; keep the existing validation logic but change
only the error string.
- Around line 100-104: The current code swallows errors from
f.ApiClient.RevealServerPassword for managed servers (server.IsManaged) which
can cause an empty password to be treated as success; change the handling in the
server.IsManaged branch that calls f.ApiClient.RevealServerPassword(ctx,
opts.id) so that if err != nil you return or surface the error (or log and
return a non-nil error) instead of ignoring it, and only assign password = pw
when err == nil and pw != ""; ensure the error from RevealServerPassword is
included in the returned/logged message so failures are visible.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c684a17b-6198-4823-bc70-2aa8a7bee576
📒 Files selected for processing (2)
internal/cmd/server/server.gointernal/cmd/server/sshinfo/sshinfo.go
Panel Review — zeabur/cli PR #2451. What problem does this PR solve?The CLI lacks a non-interactive way to retrieve SSH connection credentials for dedicated servers. Agents/automation in sandbox environments need machine-readable access to IP, port, username, and password without spawning an interactive SSH session. 2. How does it solve it?Adds a 3. What are the tradeoffs?
4. VerdictConsensus: request changes 🔴 SUGGESTED CHANGES
⚖️ Unresolved disagreements
🟡 NIT
🟢 INFO
|
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Panel Review Round 2 — zeabur/cli PR #245Fix commit:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/cmd/server/sshinfo/sshinfo.go (1)
120-120:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd CodeQL suppression comment for intentional credential output.
CodeQL flagged this line for "clear-text logging of sensitive information". Since outputting the password is by-design for agent consumption (per PR objectives), add a suppression comment to prevent CI failures.
🛡️ Proposed fix with suppression comment
+ // nosec G104 - Intentionally outputting credentials for agent consumption fmt.Println(string(data)) return nilOr use
#nosemgrepor similar depending on your security scanner.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cmd/server/sshinfo/sshinfo.go` at line 120, Add a CodeQL suppression comment directly above the fmt.Println(string(data)) call to mark the clear-text credential output as intentional for agent consumption; the comment should reference CodeQL rule (or your scanner alias, e.g., codeql-suppress or `#nosemgrep`) and include a short justification like "intentional credential output for agent" so CI will not fail while preserving the existing fmt.Println(string(data)) behavior.
♻️ Duplicate comments (1)
internal/cmd/server/sshinfo/sshinfo.go (1)
100-106:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate that revealed password is non-empty for managed servers.
The code now correctly returns an error if
RevealServerPasswordfails (good!), but if the API call succeeds with an empty password (pw == ""), the command will output{"password": ""}, which is indistinguishable from a non-managed server's output. This addresses the F1 blocking concern raised in the PR objectives about indistinguishable failure states.🔒 Proposed fix to validate empty password
var password string if server.IsManaged { pw, err := f.ApiClient.RevealServerPassword(ctx, opts.id) if err != nil { return fmt.Errorf("reveal server password failed: %w", err) } + if pw == "" { + return fmt.Errorf("managed server returned empty password") + } password = pw }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cmd/server/sshinfo/sshinfo.go` around lines 100 - 106, When server.IsManaged is true and you call f.ApiClient.RevealServerPassword(ctx, opts.id) (in sshinfo.go), validate that the returned pw is non-empty; if pw == "" return an error (e.g., fmt.Errorf("revealed empty password for managed server %s", opts.id)) instead of assigning it to password, so the command does not print an empty password field; update the block that currently assigns password = pw to perform this check and return on empty result.
🧹 Nitpick comments (3)
internal/cmd/server/sshinfo/sshinfo.go (3)
26-28: ⚡ Quick winAdd
Long:field with security warning.Per reviewer feedback in the PR objectives, the command should include a
Long:description warning that this command emits plaintext credentials and should not be piped into shared logs or executed in untrusted environments.📝 Suggested addition
cmd := &cobra.Command{ Use: "ssh-info [server-id]", Short: "Get SSH connection info for a dedicated server", + Long: "Get SSH connection info for a dedicated server.\n\nWARNING: This command outputs plaintext credentials including passwords. Do not pipe output to shared logs or execute in untrusted environments.", Args: cobra.MaximumNArgs(1),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cmd/server/sshinfo/sshinfo.go` around lines 26 - 28, Add a Long field to the cobra command (the cmd variable of type &cobra.Command) that contains a clear security warning stating this command emits plaintext credentials and must not be piped into shared logs, stored in plaintext, or run in untrusted environments; update the command declaration for ssh-info (Use: "ssh-info [server-id]") to include Long: with that warning text so it appears in help output and documents the risk to users.
115-120: 💤 Low valueConsider using
f.Printer.JSONfor consistency.Per reviewer feedback in the PR objectives, the implementation bypasses the CLI's
f.Printer.JSONconvention by using rawjson.Marshal. Using the factory's printer would align with CLI conventions, though raw JSON may be intentional for machine-readable output.🔄 Alternative using factory printer
- data, err := json.Marshal(info) - if err != nil { - return fmt.Errorf("marshal failed: %w", err) - } - - fmt.Println(string(data)) + return f.Printer.JSON(info)If raw
json.Marshalis intentional (e.g., to guarantee compact JSON without formatting), consider adding a comment explaining the choice.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cmd/server/sshinfo/sshinfo.go` around lines 115 - 120, Replace the raw json.Marshal + fmt.Println output with the CLI factory printer by calling f.Printer.JSON on the SSH info object instead of json.Marshal(info) and fmt.Println; update the code that currently references json.Marshal and fmt.Println to use the printer method (e.g., f.Printer.JSON(info)) to follow the CLI convention used elsewhere, and if compact/raw JSON was intentional leave the json.Marshal call but add a clear comment above it explaining why the factory printer is intentionally bypassed.
54-57: ⚡ Quick winConsider adding a spinner for the API call.
The
ListServersAPI call could take time, and per coding guidelines, CLI commands should usecmdutil.SpinnerCharSet,cmdutil.SpinnerInterval, andcmdutil.SpinnerColorfor spinner configuration during potentially long-running operations.🔄 Example spinner usage
func runSSHInfoInteractive(f *cmdutil.Factory, opts *Options) error { + spinner := f.Prompter.Spinner("Loading servers...", cmdutil.SpinnerCharSet, cmdutil.SpinnerInterval, cmdutil.SpinnerColor) + spinner.Start() servers, err := f.ApiClient.ListServers(context.Background()) + spinner.Stop() if err != nil { return fmt.Errorf("list servers failed: %w", err) }As per coding guidelines: Use
cmdutil.SpinnerCharSet,cmdutil.SpinnerInterval, andcmdutil.SpinnerColorfor spinner configuration in CLI commands.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cmd/server/sshinfo/sshinfo.go` around lines 54 - 57, Wrap the potentially slow API call to f.ApiClient.ListServers in a CLI spinner using cmdutil.NewSpinner (configured with cmdutil.SpinnerCharSet, cmdutil.SpinnerInterval, and cmdutil.SpinnerColor): start the spinner before calling ListServers, stop/succeed or stop/fail it after the call depending on error, and include the same error return (fmt.Errorf("list servers failed: %w", err)) when stopping on failure; update the block around ListServers to ensure the spinner is always stopped (defer or explicit stop) and provide a short success message on completion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/cmd/server/sshinfo/sshinfo.go`:
- Line 120: Add a CodeQL suppression comment directly above the
fmt.Println(string(data)) call to mark the clear-text credential output as
intentional for agent consumption; the comment should reference CodeQL rule (or
your scanner alias, e.g., codeql-suppress or `#nosemgrep`) and include a short
justification like "intentional credential output for agent" so CI will not fail
while preserving the existing fmt.Println(string(data)) behavior.
---
Duplicate comments:
In `@internal/cmd/server/sshinfo/sshinfo.go`:
- Around line 100-106: When server.IsManaged is true and you call
f.ApiClient.RevealServerPassword(ctx, opts.id) (in sshinfo.go), validate that
the returned pw is non-empty; if pw == "" return an error (e.g.,
fmt.Errorf("revealed empty password for managed server %s", opts.id)) instead of
assigning it to password, so the command does not print an empty password field;
update the block that currently assigns password = pw to perform this check and
return on empty result.
---
Nitpick comments:
In `@internal/cmd/server/sshinfo/sshinfo.go`:
- Around line 26-28: Add a Long field to the cobra command (the cmd variable of
type &cobra.Command) that contains a clear security warning stating this command
emits plaintext credentials and must not be piped into shared logs, stored in
plaintext, or run in untrusted environments; update the command declaration for
ssh-info (Use: "ssh-info [server-id]") to include Long: with that warning text
so it appears in help output and documents the risk to users.
- Around line 115-120: Replace the raw json.Marshal + fmt.Println output with
the CLI factory printer by calling f.Printer.JSON on the SSH info object instead
of json.Marshal(info) and fmt.Println; update the code that currently references
json.Marshal and fmt.Println to use the printer method (e.g.,
f.Printer.JSON(info)) to follow the CLI convention used elsewhere, and if
compact/raw JSON was intentional leave the json.Marshal call but add a clear
comment above it explaining why the factory printer is intentionally bypassed.
- Around line 54-57: Wrap the potentially slow API call to
f.ApiClient.ListServers in a CLI spinner using cmdutil.NewSpinner (configured
with cmdutil.SpinnerCharSet, cmdutil.SpinnerInterval, and cmdutil.SpinnerColor):
start the spinner before calling ListServers, stop/succeed or stop/fail it after
the call depending on error, and include the same error return (fmt.Errorf("list
servers failed: %w", err)) when stopping on failure; update the block around
ListServers to ensure the spinner is always stopped (defer or explicit stop) and
provide a short success message on completion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f5cd2bef-7c5c-444f-a3ab-4f158febfc8c
📒 Files selected for processing (1)
internal/cmd/server/sshinfo/sshinfo.go
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/cmd/server/sshinfo/sshinfo.go`:
- Around line 120-121: The stdout write error for the credential output is being
ignored at the fmt.Println(string(data)) call; capture and propagate that error
instead of always returning nil so callers see failures (e.g., change the
fmt.Println(string(data)) call to check its error result and return that error).
Keep the intentional gosec nolint comment around the print but ensure the
function returns the write error (replace the current return nil with the
captured error) so broken pipes or stdout failures surface to callers.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 60a85166-6324-47ad-9e7a-75d63507ddd1
📒 Files selected for processing (1)
internal/cmd/server/sshinfo/sshinfo.go
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
|
||
| var password string | ||
| if server.IsManaged { | ||
| pw, err := f.ApiClient.RevealServerPassword(ctx, opts.id) |
There was a problem hiding this comment.
[bug] In runSSHInfoNonInteractive, the call pw, err := f.ApiClient.RevealServerPassword(ctx, opts.id) for server.IsManaged only populates password on err == nil && pw != ""; any error (permission, server no longer has an initial password, transient failure, etc.) is discarded and password remains the empty string. The command then emits a success JSON object with no indication that credential retrieval failed. This is acceptable for the ssh sibling (which falls back to interactive password entry) but incorrect for a dedicated non-interactive credential-export command whose primary consumer is automated agents.
Suggestion: Propagate the error (return fmt.Errorf("reveal server password failed: %w", err)) when IsManaged and reveal fails, or extend SSHInfo with an explicit success indicator (e.g. PasswordRevealed bool) and populate it accordingly. At minimum, surface the error via f.Log (when not silenced) before continuing.
|
|
||
| func runSSHInfoNonInteractive(f *cmdutil.Factory, opts *Options) error { | ||
| if opts.id == "" { | ||
| return fmt.Errorf("--id is required") |
There was a problem hiding this comment.
[suggestion] The non-interactive path (reached both when f.Interactive == false and when an ID was supplied even under interactive mode) emits fmt.Errorf("--id is required") when opts.id == "". However, NewCmdSSHInfo documents Use: "ssh-info [server-id]", Args: cobra.MaximumNArgs(1), and the RunE block explicitly accepts a positional argument and copies it into opts.id.
Suggestion: Align the message with the more accurate wording already used in rename/rename.go:89: fmt.Errorf("server-id or --id is required"). (The same imprecise message exists in the sibling ssh/ssh.go:81, get/get.go:78 and reboot/reboot.go:80, but the new command should not perpetuate it.)
| serverRenameCmd "github.com/zeabur/cli/internal/cmd/server/rename" | ||
| serverRentCmd "github.com/zeabur/cli/internal/cmd/server/rent" | ||
| serverSSHCmd "github.com/zeabur/cli/internal/cmd/server/ssh" | ||
| serverSSHInfoCmd "github.com/zeabur/cli/internal/cmd/server/sshinfo" |
There was a problem hiding this comment.
[suggestion] The implementation lives under internal/cmd/server/sshinfo/sshinfo.go (package sshinfo) and is imported as .../sshinfo. All other hyphenated subcommands follow a hyphenated directory that matches their Cobra Use name: port-forward/, search-repo/, auto-recharge/, etc. (with an internal package name that drops the hyphen, e.g. package portforward).
Suggestion: Move the package to internal/cmd/server/ssh-info/sshinfo.go (package clause and file name may stay sshinfo) so the on-disk layout matches the command token "ssh-info" and the precedent set by peer commands. Update the import alias in server.go accordingly.
| Password: password, | ||
| } | ||
|
|
||
| data, err := json.Marshal(info) |
There was a problem hiding this comment.
[suggestion] runSSHInfoNonInteractive performs data, err := json.Marshal(info); fmt.Println(string(data)) unconditionally, bypassing both the f.JSON flag and f.Printer.JSON (which performs json.MarshalIndent(v, "", " ")). Every other command in the tree that can emit machine-readable output either gates JSON emission on f.JSON or uses the printer. When --json is supplied the root pre-run silences the logger but this command still writes directly; when it is not supplied the emitted JSON is compact rather than the pretty form produced elsewhere.
Suggestion: Either (a) adopt return f.Printer.JSON(info) (accepting pretty-printed output for consistency) or (b) keep the compact direct emission but document that server ssh-info is a special "always-JSON" command and still route through the printer (or a thin helper) so future printer changes are respected.
| func NewCmdSSHInfo(f *cmdutil.Factory) *cobra.Command { | ||
| opts := &Options{} | ||
|
|
||
| cmd := &cobra.Command{ |
There was a problem hiding this comment.
[nit] The &cobra.Command literal only sets Use, Short, Args and RunE. Peer commands with richer help text (e.g. port-forward/port_forward.go) include multi-line examples and a Long description.
Suggestion: Add a short Long or Example field (or at least one-line examples in --help) so zeabur server ssh-info --help and the global help --all output are self-documenting for the non-interactive agent use case.
leechenghsiu
deleted the
matthewlee/des-633-cli-新增-server-ssh-info-命令輸出非互動式-ssh-連線資訊
branch
3 participants
Summary
server ssh-infocommand that outputs SSH connection info (IP, port, username, password) as JSONRevealServerPasswordAPI; non-managed servers return empty passwordCloses DES-633
Test plan
--helpdisplays usage correctly--idin non-interactive mode returns errorNo such servererror--idflag)jq🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.