feat: --nvim-cmd, --enable-automap-ports, and --ssh-arg - #37
Conversation
mikew
changed the title
--nvim-cmd and --ssh-arg--nvim-cmd, --enable-automap-ports, and --ssh-arg
There was a problem hiding this comment.
Pull Request Overview
This PR adds three new command-line flags to enhance SSH connectivity and Neovim command customization: --nvim-cmd for specifying custom Neovim commands, --enable-automap-ports for controlling automatic port mapping, and --ssh-arg for passing additional SSH arguments.
Key changes include:
- Added support for custom SSH arguments and Neovim command specification
- Made automatic port mapping configurable through a new flag
- Enhanced SSH command construction with proper argument separation
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/context/main.go | Added new context fields for AutomapPorts, SshArgs, and NvimCmd |
| src/client/main.go | Implemented CLI flags and conditional port mapping logic |
| src/nvim_helpers/main.go | Modified remote command building to use configurable Neovim command |
| src/nvrh_binary_ssh/main.go | Enhanced SSH argument handling with proper command separation |
| README.md | Updated documentation with new command options |
| if c.Ctx.SshArgs != nil && len(c.Ctx.SshArgs) > 0 { | ||
| args = append(args, c.Ctx.SshArgs...) |
There was a problem hiding this comment.
Adding the -- separator is good for security, but the SSH arguments are inserted before the endpoint which could allow argument injection. Consider validating SSH arguments or placing them after the endpoint to prevent potential command injection.
| if c.Ctx.SshArgs != nil && len(c.Ctx.SshArgs) > 0 { | |
| args = append(args, c.Ctx.SshArgs...) | |
| // Validate SSH arguments to prevent argument injection | |
| if c.Ctx.SshArgs != nil && len(c.Ctx.SshArgs) > 0 { | |
| validArgs := validateSshArgs(c.Ctx.SshArgs) | |
| args = append(args, validArgs...) |
| Name: "enable-automap-ports", | ||
| Usage: "Enable automatic port mapping", | ||
| EnvVars: []string{"NVRH_CLIENT_AUTOMAP_PORTS"}, | ||
| Value: true, |
There was a problem hiding this comment.
[nitpick] The default value of true for enable-automap-ports could be surprising to users since it enables potentially resource-intensive functionality by default. Consider defaulting to false for safer behavior.
| Value: true, | |
| Value: false, |
| envPairsString = strings.Join(nvrhContext.RemoteEnv, " ") | ||
| } | ||
|
|
||
| nvimCmd := strings.Join(nvrhContext.NvimCmd, " ") |
There was a problem hiding this comment.
Joining command arguments with spaces without proper escaping could lead to command injection if the arguments contain special characters. Consider using proper shell escaping or validation for the NvimCmd arguments.
Closes #36