fix: harden SCP path and patch several leaks

[appleboy]

Summary

Two security-relevant defects in the SSH helper, plus four resource/concurrency bugs, are addressed in easyssh.go. The largest change is hardening Scp / WriteFile: the remote target path was previously passed unescaped through the user's shell (scp -tr %s) and the filename was written directly into the SCP control stream, allowing both remote command injection and SCP-protocol record injection. Additional fixes close a proxyClient leak that persisted on every successful proxied connect, a sshAgent socket leak caused by := shadowing inside getSSHConfig, a data race on the shared err variable inside Stream, and a Stream reader goroutine that infinite-spun when client.Close() returned a non-EOF read error.

AI Authorship

• No AI was used in this PR
• AI was used. Details:
  • Tool / model: Claude Opus 4.7 (Claude Code CLI), Auto mode
  • AI-authored files: easyssh.go, easyssh_test.go — all edits in commit 14141ba
  • Human line-by-line reviewed: none yet — author has not done a line-by-line pass; review was driven by four parallel review agents (code-reuse, code-quality, efficiency, security), findings aggregated, fixes applied
  • Reviewer should treat every changed hunk as needing close attention

Change classification

• Leaf node (local impact)
• Core code (broad impact — needs line-by-line review)

This library is the SSH transport for drone-ssh, drone-scp, and similar CI tooling. A regression here propagates to every downstream caller's deploy pipeline.

Plan reference

No plan.md — change is a /simplify + /security-review sweep prompted in conversation. Scope was confined to easyssh.go and easyssh_test.go; no public type signatures changed.

Verification

• Unit tests added: TestShellQuote (10 cases) and TestWriteFileRejectsInvalidTargetName (4 cases) pin the two security helpers
• go vet ./... clean
• gofmt -l easyssh.go clean
• go test -race -run='TestGetKeyFile|TestWrongRawKey|TestShellQuote|TestWriteFileRejectsInvalidTargetName' ./... — PASS
• Full integration suite (Alpine make ssh-server + make test) — NOT RUN in this session. Must be executed before merge. The changes most needing this coverage are the Stream reader/timeout rework and the proxyClient close-on-Wait goroutine, neither of which is exercised by the new unit tests.
• Stress / soak test: N/A (no new long-running code paths beyond the existing client.Wait() goroutine, which is a 1:1 lifetime mirror of the returned client)

Manual verification steps for the reviewer

make ssh-server                 # Spin up Alpine SSH container
make test                       # Full suite; pay attention to TestProxyGoroutineLeak

# Sanity-check the security fix end-to-end:
go test -race -run='TestShellQuote|TestWriteFileRejectsInvalidTargetName' -v ./...

Verifiability check

• Inputs and outputs documented in code comments where non-obvious (shellQuote quoting idiom; WriteFile rejected-char rationale)
• Reviewer can judge shellQuote correctness from the test table alone
• Sentinel errors (ErrFingerprintMismatch, ErrInvalidTargetFile) make new failure modes observable to callers via errors.Is
• Failures will surface in monitoring — N/A, this is a library; downstream callers decide

Security check

• No secrets in code
• External inputs (SCP target path) now validated and shell-quoted
• No permission boundaries changed
• Rate limits — N/A
• Error strings preserved for the one test pinning them ("Run Command Timeout: …"); new errors are sentinel values
• Known remaining hardening gaps (deliberately not fixed in this PR — would break the public API and existing tests):
  • InsecureIgnoreHostKey() is still the default when Fingerprint is unset. Several tests rely on this. Flipping requires a v2 / major bump.
  • UseInsecureCipher still ungated.
  • Private-key file permissions still not checked.
  • Variadic timeout ...time.Duration semantics unchanged.

Risk & rollback

Risk

• The Stream reader goroutine refactor changes the loop exit condition from errors.Is(err, io.EOF) only to "any error". For a well-behaved remote this is identical. For pathological readers that briefly return io.ErrShortBuffer or wrapped errors that are recoverable, behaviour is now stricter (we exit) — but this case did not exist before because the pre-change loop would spin sending empty strings to a no-longer-drained channel, so any deviation here is a bug fix not a regression.
• The new client.Wait() goroutine attached to proxied connections will keep one goroutine alive per active proxied session. This is matched to the lifetime of the caller's *ssh.Client, so callers who already correctly Close() their client see no change. Callers who never close their client previously leaked the proxyClient socket too — now they additionally leak one goroutine. Same root cause.
• The new WriteFile filename validation returns ErrInvalidTargetFile for paths that previously would have produced an opaque remote error (or worse, succeeded as an injection). This is observable behaviour change; downstream callers that pass \n/\r/\x00 in target paths should be vanishingly rare but exist hypothetically.

Rollback

• Single-commit revert: git revert 14141ba. No data migration, no schema, no config flags to clean up.

Reviewer guide

Read carefully (AI-authored, security-/concurrency-sensitive, no human pre-review):

• easyssh.go:482-547 — WriteFile validation + shellQuote helper. This is the SCP-injection fix; verify the quoting handles every metacharacter you can think of.
• easyssh.go:316-321 — the new go func() { _ = client.Wait(); _ = proxyClient.Close() }(). Check that the proxyClient is not closed prematurely or accessed after close in any error path.
• easyssh.go:295-314 — the four new _ = proxyClient.Close() calls on timeout / dial-error / NewClientConn-error paths. Confirm no double-close.
• easyssh.go:350-444 — the Stream refactor (scanner helper, closeBoth closure, ctx-aware send select). This rewrites the goroutine lifecycle; the data-race fix and the EOF→any-error change live here.
• easyssh.go:176-179 — the sshAgent shadowing fix in getSSHConfig. One-line change but verify the new sshAgent = sock actually flows into the returned closer.

Spot-check OK (mechanical changes, well-covered by signatures + tests):

• easyssh.go:26-39 — var → const plus three new sentinel errors. Additive.
• easyssh.go:196-204 — fingerprint mismatch now returns ErrFingerprintMismatch. Error string preserved.
• easyssh.go:438 — %v → %w on the timeout error. String output unchanged (already confirmed against the pinned test).
• easyssh_test.go:670-708 — the two new tests.

Commits in this PR

• 14141ba — the work described above (118 lines added / 68 deleted across 2 files)
• 459d2f8 — chore(deps): upgrade third-party dependencies (already unpushed; bumps golang.org/x/crypto to v0.51.0 and golang.org/x/sys to v0.44.0)

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.