fix(ssh): parse ssh host string manually, clean up bugs by water-sucks · Pull Request #161 · nix-community/nixos-cli

water-sucks · 2026-01-29T04:02:49Z

SSH host strings are not URLs. The previous code treated them as such, and this failed in the case of link-local IPV6 addresses that are bracketed and have percent signs.

This PR adds a small user@host:port parser that behaves similarly to OpenSSH to correct this behavior, and other future SSH vs. URL disparities.

It also fixes some small bugs with the SSH command system.

Closes #146.

Summary by CodeRabbit

Bug Fixes

Enhanced SSH connection parsing to better support IPv6 and IPv4 addresses with improved port detection
Improved error handling and logging for SSH agent connection failures
Added validation for signal forwarding during remote SSH sessions
Strengthened input validation for password authentication operations

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2026-01-29T04:02:57Z

📝 Walkthrough

Walkthrough

The SSH module's URL parsing is replaced with a custom host/port parser supporting IPv6 link-local addresses with interface identifiers, SSH agent timeouts, stdin validation, and signal validation. This resolves failures when using link-local IPv6 addresses like [fe80::beef%wlan0].

Changes

Cohort / File(s)	Summary
SSH Parser & Connection Hardening `internal/system/ssh.go`	Replaces URL-based parsing with custom `parseUserHostPort` and `parseHostPort` functions to handle IPv6 (bracketed/unbracketed) and IPv4 addresses with interface identifiers. Adds SSH agent timeout dialing, validates stdin is a file descriptor in PTY requests, validates SSH signal strings before forwarding, improves error logging and fallback handling.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 A parser so clever, no URLs to fright,
IPv6 link-locals now sparkle so bright,
With brackets and percent signs, no escaping required,
SSH connections are hardened, validated, inspired!
The bunny hops forward with signals secure. 🌟

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely summarizes the main change: replacing URL parsing with manual SSH host string parsing and fixing related bugs.
Linked Issues check	✅ Passed	The PR directly addresses issue `#146` by implementing manual parsing of SSH host strings to support link-local IPv6 addresses with percent-sign interface identifiers, matching OpenSSH behavior.
Out of Scope Changes check	✅ Passed	All changes align with the stated objectives: SSH host string parsing replacement, SSH agent dialer timeout, PTY stdin validation, and signal validation are all related to SSH robustness improvements mentioned in the PR description.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

📝 Generate docstrings

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

water-sucks · 2026-01-29T04:03:30Z

@coderabbitai review

coderabbitai · 2026-01-29T04:03:38Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@internal/system/ssh.go`:
- Around line 197-208: The IPv6 handling incorrectly rejects valid unbracketed
addresses that use zero-compression (contain "::") because the code always
treats the last colon-separated token as a potential port; update the branch in
the function that uses variables s and colonCount so that if s contains "::" you
skip the port-parse check (the strconv.ParseUint on last) and immediately return
the whole s as the address (return s, "", nil); ensure the new condition is
evaluated before attempting to parse last to avoid false "IPv6 address with port
must be in brackets" errors.

🧹 Nitpick comments (1)

internal/system/ssh.go (1)
587-589: Consider bracketing IPv6 addresses in the output.

For IPv6 addresses (especially with interface identifiers like fe80::1%eth0), the current format produces ambiguous output like user@fe80::1%eth0:22. Using net.JoinHostPort would produce the standard bracketed format user@[fe80::1%eth0]:22.
♻️ Suggested improvement
 func (s *SSHSystem) Address() string {
-	return fmt.Sprintf("%s@%s:%s", s.user, s.address, s.port)
+	return fmt.Sprintf("%s@%s", s.user, net.JoinHostPort(s.address, s.port))
 }

internal/system/ssh.go

This commit moves the SSH system utilities into a separate package and adds tests for it. It now also uses the `net` package to parse and split the host:port portion of the address whenever possible, to simplify case handling, and makes the rules less strict. Bracketed IPv6 addresses are treated specially and are parsed much more strictly, but everything else relies on naive splitting. This should be tolerant enough of bad addresses and attempts it in most cases; edge cases can be handled later if they crop up.

water-sucks force-pushed the parse-ssh-host-string-manually branch from 009a6b6 to 8355a07 Compare January 29, 2026 04:03

coderabbitai bot reviewed Jan 29, 2026

View reviewed changes

internal/system/ssh.go Outdated Show resolved Hide resolved

water-sucks force-pushed the parse-ssh-host-string-manually branch 2 times, most recently from a21d4db to fd5c6fd Compare January 29, 2026 23:21

water-sucks added 5 commits January 30, 2026 13:53

fix(ssh): parse user@host:port syntax manually according to openssh impl

4222230

fix(ssh): add dialer timeout for ssh socket

258d980

fix(ssh): gracefully fail if stdin is not a file for root password pty

62ec087

fix(ssh): do not send empty (unparsed) signals

d315c95

water-sucks force-pushed the parse-ssh-host-string-manually branch from fd5c6fd to d60aa22 Compare January 30, 2026 22:02

water-sucks merged commit f0a5266 into nix-community:main Jan 30, 2026
2 checks passed

coderabbitai bot mentioned this pull request Feb 26, 2026

refactor(ssh): use agent instead of temporary files for private keys #196

Merged

coderabbitai bot mentioned this pull request Apr 1, 2026

fix(ssh): perform root command elevation test once before password input #204

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(ssh): parse ssh host string manually, clean up bugs#161

fix(ssh): parse ssh host string manually, clean up bugs#161
water-sucks merged 5 commits intonix-community:mainfrom
water-sucks:parse-ssh-host-string-manually

water-sucks commented Jan 29, 2026 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Jan 29, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Poem

Uh oh!

water-sucks commented Jan 29, 2026

Uh oh!

coderabbitai bot commented Jan 29, 2026

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

water-sucks commented Jan 29, 2026 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Bug Fixes

Uh oh!

coderabbitai bot commented Jan 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

Uh oh!

water-sucks commented Jan 29, 2026

Uh oh!

coderabbitai bot commented Jan 29, 2026

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

water-sucks commented Jan 29, 2026 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Jan 29, 2026 •

edited

Loading