[sftp] add support#8
Draft
capcom6 wants to merge 4 commits into
Draft
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
🤖 Pull request artifacts
|
|
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)
WalkthroughThis PR implements SFTP protocol support alongside existing FTP: adds a new SFTP client and auth helpers, refactors the FTP client into an ChangesSFTP protocol support
Sequence DiagramsequenceDiagram
participant Caller
participant ClientFactory as internal/client.New
participant FtpFactory as ftp.NewClient
participant SftpFactory as sftp.NewClient
participant SSH as ssh.Dial
Caller->>ClientFactory: New(address, logger)
alt URL scheme is "ftp"
ClientFactory->>FtpFactory: ftp.NewClient(url, logger)
FtpFactory-->>ClientFactory: *ftp.Client
else URL scheme is "sftp"
ClientFactory->>SftpFactory: sftp.NewClient(url, logger)
SftpFactory->>SSH: Dial SSH (host, auth)
SSH-->>SftpFactory: ssh.Client
SftpFactory-->>ClientFactory: *sftp.Client
else unsupported scheme
ClientFactory-->>Caller: error(types.ErrUnsupportedScheme)
end
ClientFactory-->>Caller: Client, error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
77-82:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate About section to include SFTP support.
Line 81 still says “remote FTP server,” which conflicts with the updated FTP+SFTP positioning in the rest of the README.
Suggested patch
-sftp-sync is a command-line utility for syncing a local folder with a remote FTP server on every change of files or directories. +sftp-sync is a command-line utility for syncing a local folder with a remote FTP or SFTP server on every change of files or directories.🤖 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 `@README.md` around lines 77 - 82, The About section still calls out "remote FTP server"; update the text under "About The Project" (the sftp-sync project description) to reflect SFTP support by replacing that phrase with a combined term such as "remote FTP or SFTP server" or "remote FTP/SFTP server" so it matches the README's FTP+SFTP positioning.
🤖 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/client/sftp/sftp.go`:
- Around line 70-72: The current parsing in sftp.go that checks if
strings.Contains(host, ":") and then does strings.Cut(host, ":") incorrectly
splits IPv6 addresses; replace that logic by using the URL object's helpers:
call u.Hostname() to get the host (with IPv6 brackets removed) and u.Port() to
get the port string (or blank if none) and assign to the existing host and port
variables (fall back to default port if u.Port() is empty). Update references
around variables named host, port and the URL variable u to use these methods
rather than manual string slicing.
- Around line 159-166: The recursive delete in removeDirRecur is listing the
parent directory because it calls c.sftp.ReadDirContext(ctx, path.Dir(p))
instead of the target path; update the ReadDirContext call in
Client.removeDirRecur to use p (the directory being removed) so entries are read
from the correct directory, and ensure any subsequent path joins or recursive
calls (inside removeDirRecur) continue to operate on p and its child paths
rather than path.Dir(p).
- Line 82: Replace the insecure HostKeyCallback currently set to
ssh.InsecureIgnoreHostKey() in the SSH client configuration with a known-hosts
based callback using knownhosts.New. Import golang.org/x/crypto/ssh/knownhosts
and build the known_hosts path (e.g., filepath.Join(os.UserHomeDir(), ".ssh",
"known_hosts")), call knownhosts.New(...) to get the callback, handle any error
(return or log) from knownhosts.New, and set the config's HostKeyCallback to the
returned callback instead of ssh.InsecureIgnoreHostKey(); update imports to
include os and path/filepath as needed.
- Around line 274-279: Update the error-matching logic around the notFoundErrors
slice: remove the overly broad "failure" entry and the case-sensitive "No such
file", normalize the error string with strings.ToLower(err.Error()) before
comparing, and additionally check os.IsNotExist(err) to reliably detect
file-not-found cases; i.e., keep only specific patterns like "does not exist",
"not found", "no such file" in notFoundErrors, use strings.ToLower when scanning
those patterns, and short-circuit to treat the error as a not-found only if
strings match or os.IsNotExist(err) returns true (do not suppress permission or
connection errors).
---
Outside diff comments:
In `@README.md`:
- Around line 77-82: The About section still calls out "remote FTP server";
update the text under "About The Project" (the sftp-sync project description) to
reflect SFTP support by replacing that phrase with a combined term such as
"remote FTP or SFTP server" or "remote FTP/SFTP server" so it matches the
README's FTP+SFTP positioning.
🪄 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: a61725ef-8cc6-495c-be81-e98004aebbec
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
.golangci.ymlREADME.mdgo.modinternal/cli/commands/sync/sync.gointernal/client/client.gointernal/client/ftp/ftp.gointernal/client/sftp/sftp.gointernal/client/types/errors.gomain.go
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/client/ftp/ftp.go (1)
76-83:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClose and discard partially initialized FTP sessions on setup failure.
If
LoginorChangeDirfails, the new connection remains assigned toc.client. A laterinit()can passping()and skip setup, causing operations to run in the wrong remote directory.Suggested fix
- c.client, err = ftp.Dial(u.Host, ftp.DialWithContext(ctx)) + conn, err := ftp.Dial(u.Host, ftp.DialWithContext(ctx)) if err != nil { return fmt.Errorf("can't connect to %s: %w", u.Host, err) } @@ - if loginErr := c.client.Login(u.User.Username(), password); loginErr != nil { + if loginErr := conn.Login(u.User.Username(), password); loginErr != nil { + _ = conn.Quit() return fmt.Errorf("can't login as %s: %w", u.User.Username(), loginErr) } if u.Path != "" && u.Path != "/" { - if chErr := c.client.ChangeDir(u.Path); chErr != nil { + if chErr := conn.ChangeDir(u.Path); chErr != nil { + _ = conn.Quit() return fmt.Errorf("remote path %s does not exist or is not accessible: %w", u.Path, chErr) } } + + c.client = conn🤖 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/client/ftp/ftp.go` around lines 76 - 83, When setup fails during ftp session initialization (calls to c.client.Login or c.client.ChangeDir), ensure the partially-initialized c.client is closed and discarded so subsequent init()/ping() do not reuse it; on both the Login failure branch and the ChangeDir failure branch call c.client.Close() (handle/ignore Close() error as appropriate) and then set c.client = nil before returning the wrapped error from Login or ChangeDir, so the client state is cleaned up on setup failure.
🧹 Nitpick comments (3)
internal/client/sftp/sftp_test.go (1)
49-69: ⚡ Quick winTest currently validates duplicated logic, not implementation behavior.
TestURLPathValidationRulesre-derives the rule inline, so it won’t catch regressions in the actual SFTP path-handling code. Please assert this through the real path-validation/dir-selection behavior (or a helper used by production code) instead.🤖 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/client/sftp/sftp_test.go` around lines 49 - 69, The test is re-deriving the path rule instead of exercising the real code; update TestURLPathValidationRules to call the actual production path-validation or dir-selection code (replace the inline computation `shouldUse := tt.path != "" && tt.path != "/"` in TestURLPathValidationRules with a call to the helper/function used by the SFTP client to decide whether to change directories) and assert that its return matches tt.shouldUse; if the decision is encapsulated in a method on the SFTP client, construct the minimal client or invoke the exported helper so the test validates implementation behavior rather than duplicated logic.internal/client/sftp/sftp.go (2)
92-96: 💤 Low valueConsider adding a connection timeout.
The SSH
ClientConfiglacks aTimeoutfield. While OS-level TCP timeouts exist, an explicit timeout provides predictable behavior and prevents indefinite hangs against unresponsive servers.♻️ Proposed fix
config := &ssh.ClientConfig{ User: username, Auth: authMethods, HostKeyCallback: hostKeyCallback, + Timeout: 30 * time.Second, }🤖 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/client/sftp/sftp.go` around lines 92 - 96, The ssh.ClientConfig named config currently has no connection timeout; to avoid hangs, implement a TCP-level timeout by creating a net.Dialer with a reasonable Timeout and use dialer.Dial to open the TCP connection and then wrap it with ssh.NewClientConn and ssh.NewClient (instead of calling ssh.Dial). Update the code path that uses config (the SFTP connection setup / connect function where config is passed) to use this dialer-based flow so the SSH handshake respects the configured timeout.
81-84: ⚡ Quick winUse
os.UserHomeDir()for cross-platform compatibility.
os.Getenv("HOME")doesn't work on Windows and may be empty in some environments.os.UserHomeDir()handles platform differences correctly.♻️ Proposed fix
- hostKeyCallback, err := knownhosts.New(filepath.Join(os.Getenv("HOME"), ".ssh", "known_hosts")) + home, err := os.UserHomeDir() + if err != nil { + return fmt.Errorf("can't determine home directory: %w", err) + } + hostKeyCallback, err := knownhosts.New(filepath.Join(home, ".ssh", "known_hosts")) if err != nil { return fmt.Errorf("can't load known_hosts: %w", err) }🤖 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/client/sftp/sftp.go` around lines 81 - 84, Replace the use of os.Getenv("HOME") when building the known_hosts path with os.UserHomeDir() and handle its error: call os.UserHomeDir(), check for an error, then use filepath.Join(userHome, ".ssh", "known_hosts") when calling knownhosts.New (the hostKeyCallback initialization). Ensure any error from os.UserHomeDir() is returned (similar to the existing error handling around knownhosts.New) so the function fails cleanly if the home directory can't be resolved.
🤖 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/client/sftp/auth.go`:
- Around line 84-100: The buildAgentSigners function leaks the agent socket
connection because conn is never closed; after successfully dialing the SSH
agent in buildAgentSigners (the conn returned by dialer.DialContext) defer
conn.Close() immediately (or otherwise close conn before each return) so the
file descriptor is released; ensure both the success path (after agent.NewClient
and Signers()) and the error paths close the connection by placing the defer
right after the dial succeeds.
---
Outside diff comments:
In `@internal/client/ftp/ftp.go`:
- Around line 76-83: When setup fails during ftp session initialization (calls
to c.client.Login or c.client.ChangeDir), ensure the partially-initialized
c.client is closed and discarded so subsequent init()/ping() do not reuse it; on
both the Login failure branch and the ChangeDir failure branch call
c.client.Close() (handle/ignore Close() error as appropriate) and then set
c.client = nil before returning the wrapped error from Login or ChangeDir, so
the client state is cleaned up on setup failure.
---
Nitpick comments:
In `@internal/client/sftp/sftp_test.go`:
- Around line 49-69: The test is re-deriving the path rule instead of exercising
the real code; update TestURLPathValidationRules to call the actual production
path-validation or dir-selection code (replace the inline computation `shouldUse
:= tt.path != "" && tt.path != "/"` in TestURLPathValidationRules with a call to
the helper/function used by the SFTP client to decide whether to change
directories) and assert that its return matches tt.shouldUse; if the decision is
encapsulated in a method on the SFTP client, construct the minimal client or
invoke the exported helper so the test validates implementation behavior rather
than duplicated logic.
In `@internal/client/sftp/sftp.go`:
- Around line 92-96: The ssh.ClientConfig named config currently has no
connection timeout; to avoid hangs, implement a TCP-level timeout by creating a
net.Dialer with a reasonable Timeout and use dialer.Dial to open the TCP
connection and then wrap it with ssh.NewClientConn and ssh.NewClient (instead of
calling ssh.Dial). Update the code path that uses config (the SFTP connection
setup / connect function where config is passed) to use this dialer-based flow
so the SSH handshake respects the configured timeout.
- Around line 81-84: Replace the use of os.Getenv("HOME") when building the
known_hosts path with os.UserHomeDir() and handle its error: call
os.UserHomeDir(), check for an error, then use filepath.Join(userHome, ".ssh",
"known_hosts") when calling knownhosts.New (the hostKeyCallback initialization).
Ensure any error from os.UserHomeDir() is returned (similar to the existing
error handling around knownhosts.New) so the function fails cleanly if the home
directory can't be resolved.
🪄 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: a065f693-732f-40b6-a4dd-d0c5dc4050b2
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
.golangci.ymlREADME.mdgo.modinternal/cli/commands/sync/sync.gointernal/client/client.gointernal/client/ftp/ftp.gointernal/client/ftp/ftp_test.gointernal/client/sftp/auth.gointernal/client/sftp/sftp.gointernal/client/sftp/sftp_test.gointernal/client/types/errors.gomain.go
✅ Files skipped from review due to trivial changes (4)
- go.mod
- internal/cli/commands/sync/sync.go
- .golangci.yml
- README.md
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/client/ftp/ftp.go (1)
71-78:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard
u.Userbefore callingPassword()/Username()in FTP login to avoid nil panic.
internal/client/ftp/ftp.godereferencesu.User.Password()andu.User.Username()(forconn.Loginand error formatting) without anyu.User == nilguard; a URL without userinfo (e.g.,ftp://host/path) can therefore panic during connection initialization. Add a validation check and return an invalid-params error consistent with existing error handling in this package.🤖 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/client/ftp/ftp.go` around lines 71 - 78, Guard against a nil User before calling Password() and Username(): check if u.User == nil at the start of the FTP login sequence in internal/client/ftp/ftp.go and return the package's standard "invalid-params" error (consistent with existing error handling) instead of proceeding; then use local username/password variables (from u.User.Username() and u.User.Password()) when calling conn.Login and when formatting the login error so no nil dereference can occur during conn.Login or the error path.
🤖 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/client/sftp/sftp.go`:
- Around line 83-97: Before constructing ssh.ClientConfig and calling ssh.Dial,
validate that ParseConfigFromURL returned non-empty config.Host and
config.Username and return types.ErrInvalidParams if either is empty;
specifically, check config.Host and config.Username (derived by
ParseConfigFromURL from u.Hostname() and u.User.Username()), and only proceed to
build ssh.ClientConfig and call ssh.Dial(net.JoinHostPort(config.Host,
config.Port), sshConfig) when both are present — otherwise return
types.ErrInvalidParams to fail fast on malformed SFTP URLs.
---
Outside diff comments:
In `@internal/client/ftp/ftp.go`:
- Around line 71-78: Guard against a nil User before calling Password() and
Username(): check if u.User == nil at the start of the FTP login sequence in
internal/client/ftp/ftp.go and return the package's standard "invalid-params"
error (consistent with existing error handling) instead of proceeding; then use
local username/password variables (from u.User.Username() and u.User.Password())
when calling conn.Login and when formatting the login error so no nil
dereference can occur during conn.Login or the error path.
🪄 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: 3f717e7f-d1d6-40ec-8c26-fbec69f16df0
📒 Files selected for processing (5)
internal/client/ftp/ftp.gointernal/client/sftp/auth.gointernal/client/sftp/config.gointernal/client/sftp/sftp.gointernal/client/sftp/sftp_test.go
💤 Files with no reviewable changes (1)
- internal/client/sftp/sftp_test.go
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/client/ftp/ftp.go`:
- Around line 71-80: The code currently skips Login when u.User is nil which
allows init to succeed unauthenticated and fail later; change the logic in the
FTP initialization (the block referencing u.User, conn.Login and conn.Quit) to
fail fast: if u.User is nil either perform an explicit anonymous login via
conn.Login("anonymous","") or return a clear error immediately (e.g., "missing
FTP credentials"), and ensure you still call conn.Quit() on error and wrap the
login error (as with fmt.Errorf("can't login as %s: %w", ...)) for consistent
diagnostics.
🪄 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: 34107b97-ff55-4dce-8436-f765d5143714
📒 Files selected for processing (3)
internal/client/ftp/ftp.gointernal/client/sftp/config.gointernal/client/sftp/sftp.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores