feat(machine): SSH key detection, generation, and validation#104
Merged
nvandessel merged 8 commits intomainfrom Feb 28, 2026
Merged
feat(machine): SSH key detection, generation, and validation#104nvandessel merged 8 commits intomainfrom
nvandessel merged 8 commits intomainfrom
Conversation
Add comprehensive SSH key management capabilities: - ValidateEmail() and ValidateSSHKeyPath() validators with security hardening (flag injection, path traversal, dangerous filenames) - DetectSSHKeyFiles() scans ~/.ssh for .pub files (skips symlinks) - DetectAllSSHKeys() merges agent + file keys with deduplication - GenerateSSHKey() creates ed25519 keys with permission verification - AddKeyToAgent() and GetSSHPublicKey() with path validation - IsAgentRunning() checks ssh-agent availability - SSHKey struct gains Loaded/Source fields for tracking key origin - New CLI: g4d machine keys list, g4d machine keys generate-ssh Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as g4d machine<br/>keys generate-ssh
participant Validator as validation
participant SSH as internal/machine
participant FS as ~/.ssh
participant Agent as ssh-agent
User->>CLI: run generate-ssh --email --name
CLI->>Validator: ValidateEmail(email)
Validator-->>CLI: ok
CLI->>SSH: GenerateSSHKey(opts)
SSH->>Validator: ValidateSSHKeyPath(keyPath, sshDir)
Validator-->>SSH: ok
SSH->>FS: ensure ~/.ssh (permissions), run ssh-keygen -> create key + .pub
FS-->>SSH: private + public files
SSH->>Agent: IsAgentRunning()
Agent-->>SSH: yes/no
alt agent running
SSH->>Agent: AddKeyToAgent(keyPath)
Agent-->>SSH: ok
end
SSH->>FS: read .pub
FS-->>SSH: public key content
SSH-->>CLI: public key text
CLI-->>User: display public key
sequenceDiagram
actor User
participant CLI as g4d machine<br/>keys list
participant SSH as internal/machine
participant FS as ~/.ssh
participant Agent as ssh-agent
participant Git as internal/machine/git
User->>CLI: run keys list
CLI->>SSH: DetectAllSSHKeys(~/.ssh)
SSH->>FS: scan .pub files
FS-->>SSH: file-based keys
SSH->>Git: request agent keys
Git->>Agent: ssh-add -l
Agent-->>Git: agent keys
Git-->>SSH: parsed keys (Loaded=true, Source="agent")
SSH->>SSH: merge file & agent lists (agent prioritized)
SSH-->>CLI: combined key list with fingerprints/status
CLI-->>User: display keys (type, fingerprint, comment, loaded/source)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Greptile SummaryThis PR adds comprehensive SSH key management capabilities to the Key additions:
Security hardening:
Test coverage: Previous review feedback: Confidence Score: 4/5
|
| Filename | Overview |
|---|---|
| internal/machine/ssh.go | Adds comprehensive SSH key management: detection, generation, validation. Security hardening is strong with path validation and permission checks. Previous review issues mostly addressed (Source="both" fix, dangerous filename blocking). Minor: post-generation error cleanup still missing. |
| internal/validation/validation.go | Adds ValidateEmail() and ValidateSSHKeyPath() with excellent security coverage: blocks shell metacharacters, control chars, path traversal, dangerous filenames (including .pub variants). Comprehensive validation logic. |
| cmd/g4d/machine.go | Adds keys list and keys generate-ssh commands with proper error handling, user-friendly output, and defense-in-depth validation. Generic messaging (no hardcoded "GitHub"). Clean command structure. |
| internal/machine/ssh_test.go | Comprehensive test coverage: symlink skipping, file detection, parsing, path traversal, dangerous names. Tests cover security edge cases and normal operation. Well-structured table-driven tests. |
| internal/validation/validation_test.go | Extensive security-focused tests for email and SSH key path validation. Covers injection attacks, shell metacharacters, path traversal, dangerous filenames (including .pub variants). Excellent test coverage. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
Start[g4d machine keys generate-ssh]
GetEmail[Get email from --email flag or git config]
ValidateEmail{ValidateEmail}
GetName[Get key name from --name flag or default]
ExpandDir[Expand ~/.ssh directory path]
ValidatePath{ValidateSSHKeyPath}
CheckExists{Key file exists?}
CreateDir[Create ~/.ssh with 0700 perms]
CheckDirPerms{Dir perms secure?}
FixPerms[Fix directory permissions]
RunSSHKeygen[Run ssh-keygen -t ed25519 -C email]
VerifyPerms{Private key has 0600?}
VerifyPub{Public key exists?}
AddToAgent{Agent running?}
RunSSHAdd[ssh-add keyPath]
ReadPub[Read public key]
Display[Display public key for registration]
End[End]
Error[Error Exit]
Start --> GetEmail
GetEmail --> ValidateEmail
ValidateEmail -->|Invalid| Error
ValidateEmail -->|Valid| GetName
GetName --> ExpandDir
ExpandDir --> ValidatePath
ValidatePath -->|Invalid| Error
ValidatePath -->|Valid| CheckExists
CheckExists -->|Yes| Error
CheckExists -->|No| CreateDir
CreateDir --> CheckDirPerms
CheckDirPerms -->|Insecure| FixPerms
CheckDirPerms -->|Secure| RunSSHKeygen
FixPerms --> RunSSHKeygen
RunSSHKeygen --> VerifyPerms
VerifyPerms -->|Wrong perms| Error
VerifyPerms -->|0600| VerifyPub
VerifyPub -->|Missing| Error
VerifyPub -->|Exists| AddToAgent
AddToAgent -->|Yes| RunSSHAdd
AddToAgent -->|No| ReadPub
RunSSHAdd --> ReadPub
ReadPub --> Display
Display --> End
Last reviewed commit: cc6fa73
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
internal/machine/ssh.go (1)
284-301: Consider handling the case where input is exactly~(without trailing path).The current implementation only handles
~/...prefix. If someone passes just~as the SSH directory, it won't be expanded.💡 Handle edge case for bare tilde
func expandSSHDir(sshDir string) (string, error) { - if strings.HasPrefix(sshDir, "~/") { + if sshDir == "~" { + home, err := os.UserHomeDir() + if err != nil { + return "", fmt.Errorf("failed to get home directory: %w", err) + } + sshDir = home + } else if strings.HasPrefix(sshDir, "~/") { home, err := os.UserHomeDir() if err != nil { return "", fmt.Errorf("failed to get home directory: %w", err) } sshDir = filepath.Join(home, sshDir[2:]) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/machine/ssh.go` around lines 284 - 301, The expandSSHDir function currently only handles the case where the input starts with "~/" but does not handle the case where the input is exactly "~". Modify the condition that checks for the tilde prefix to also handle the bare tilde case, either by updating the strings.HasPrefix check to include both "~" and "~/" patterns, or by adding an additional condition to check if sshDir equals "~". When either condition is true, expand the tilde to the home directory using os.UserHomeDir() in the same way as the current implementation.internal/machine/ssh_test.go (1)
133-159: Test name is misleading - this tests ssh-keygen directly, notGenerateSSHKeyfunction.The test
TestGenerateSSHKeydirectly invokesssh-keygenviaexec.Commandrather than testing theGenerateSSHKeyfunction. This validates ssh-keygen behavior but doesn't exercise the actual function's validation, defaults, or permission checks.Consider renaming to
TestSSHKeygenBehavioror adding a test that callsGenerateSSHKey(SSHKeygenOpts{...})to verify the function's integration with ssh-keygen.💡 Suggested rename or additional test
-func TestGenerateSSHKey(t *testing.T) { +func TestSSHKeygenDirectInvocation(t *testing.T) { if _, err := exec.LookPath("ssh-keygen"); err != nil { t.Skip("ssh-keygen not available") }Or add a true integration test:
func TestGenerateSSHKeyIntegration(t *testing.T) { if _, err := exec.LookPath("ssh-keygen"); err != nil { t.Skip("ssh-keygen not available") } tmpDir := t.TempDir() keyPath, err := GenerateSSHKey(SSHKeygenOpts{ Email: "test@example.com", Name: "integration_test_key", SSHDir: tmpDir, }) // Note: This will prompt for passphrase interactively // May need to be skipped in CI or use -N "" flag modification }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/machine/ssh_test.go` around lines 133 - 159, The test named TestGenerateSSHKey is misleading because it calls ssh-keygen directly instead of exercising the GenerateSSHKey function; either rename the test to something like TestSSHKeygenBehavior to reflect it tests the external tool, or replace/add a test that calls GenerateSSHKey(SSHKeygenOpts{...}) to validate the wrapper's behavior (permissions, defaults, and error handling) and assert on the returned key path and file permissions; ensure the new test still checks for ssh-keygen presence with exec.LookPath and handles CI by skipping when unavailable.cmd/g4d/machine.go (1)
319-386: Consider validating the--nameflag to prevent path separators.The
nameflag is passed directly toSSHKeygenOpts.Namewithout validation in the CLI layer. WhileValidateSSHKeyPathinGenerateSSHKeywill catch path traversal attempts, rejecting invalid names earlier would provide better UX with clearer error messages.Additionally, the error from
GetGitUserEmailis silently ignored. Consider logging a debug message when git config lookup fails.💡 Optional: Add early validation for name flag
name, _ := cmd.Flags().GetString("name") + if strings.ContainsAny(name, "/\\") { + fmt.Fprintf(os.Stderr, "Error: key name must not contain path separators\n") + os.Exit(1) + } keyPath, err := machine.GenerateSSHKey(machine.SSHKeygenOpts{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/g4d/machine.go` around lines 319 - 386, Validate the user-supplied --name before calling GenerateSSHKey: after retrieving name via cmd.Flags().GetString("name") check for any path separators or traversal tokens (e.g. "/", "\\", string(os.PathSeparator), "..") and return a clear error and exit if present, rather than passing it into SSHKeygenOpts.Name; keep using GenerateSSHKey and ValidateSSHKeyPath as a second-line defense. Also surface or log the error from machine.GetGitUserEmail() (instead of ignoring it) so a failing git lookup emits a debug/warning via fmt.Fprintf or the existing logger while still falling back to requiring --email when needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/g4d/machine.go`:
- Around line 319-386: Validate the user-supplied --name before calling
GenerateSSHKey: after retrieving name via cmd.Flags().GetString("name") check
for any path separators or traversal tokens (e.g. "/", "\\",
string(os.PathSeparator), "..") and return a clear error and exit if present,
rather than passing it into SSHKeygenOpts.Name; keep using GenerateSSHKey and
ValidateSSHKeyPath as a second-line defense. Also surface or log the error from
machine.GetGitUserEmail() (instead of ignoring it) so a failing git lookup emits
a debug/warning via fmt.Fprintf or the existing logger while still falling back
to requiring --email when needed.
In `@internal/machine/ssh_test.go`:
- Around line 133-159: The test named TestGenerateSSHKey is misleading because
it calls ssh-keygen directly instead of exercising the GenerateSSHKey function;
either rename the test to something like TestSSHKeygenBehavior to reflect it
tests the external tool, or replace/add a test that calls
GenerateSSHKey(SSHKeygenOpts{...}) to validate the wrapper's behavior
(permissions, defaults, and error handling) and assert on the returned key path
and file permissions; ensure the new test still checks for ssh-keygen presence
with exec.LookPath and handles CI by skipping when unavailable.
In `@internal/machine/ssh.go`:
- Around line 284-301: The expandSSHDir function currently only handles the case
where the input starts with "~/" but does not handle the case where the input is
exactly "~". Modify the condition that checks for the tilde prefix to also
handle the bare tilde case, either by updating the strings.HasPrefix check to
include both "~" and "~/" patterns, or by adding an additional condition to
check if sshDir equals "~". When either condition is true, expand the tilde to
the home directory using os.UserHomeDir() in the same way as the current
implementation.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cmd/g4d/machine.gointernal/machine/git.gointernal/machine/git_test.gointernal/machine/ssh.gointernal/machine/ssh_test.gointernal/validation/validation.gointernal/validation/validation_test.go
Owner
Author
🌴 Frond StackManaged by frond |
- Use computed sshDir variable instead of hardcoded "~/.ssh" (greptile) - Add early email validation at CLI layer before GenerateSSHKey - Add early --name flag validation for path separators - Handle bare tilde "~" in expandSSHDir - Rename TestGenerateSSHKey to TestSSHKeygenDirectInvocation for clarity Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
Author
|
@greptileai review |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
cmd/g4d/machine.go (1)
268-323: Add explicitArgs: cobra.NoArgsto new key subcommands.Both
listandgenerate-sshcurrently accept extra positional args silently; rejecting them gives clearer CLI behavior.🔧 Proposed fix
var machineKeysListCmd = &cobra.Command{ Use: "list", Short: "List all detected SSH and GPG keys", + Args: cobra.NoArgs, Run: func(cmd *cobra.Command, args []string) { @@ var machineKeysGenerateSSHCmd = &cobra.Command{ Use: "generate-ssh", Short: "Generate a new ed25519 SSH key", + Args: cobra.NoArgs, Run: func(cmd *cobra.Command, args []string) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/g4d/machine.go` around lines 268 - 323, The machineKeysListCmd and machineKeysGenerateSSHCmd cobra commands currently accept extra positional arguments; update each command struct (machineKeysListCmd and machineKeysGenerateSSHCmd) to explicitly set Args: cobra.NoArgs (e.g., add Args: cobra.NoArgs alongside Use/Short) so the CLI rejects unexpected positional arguments and returns a clear error to the user.internal/machine/ssh_test.go (1)
11-119: Refactor repetitive test cases into table-driven suites.
DetectSSHKeyFiles*,GenerateSSHKey*, andGetSSHPublicKey*currently duplicate setup/assert patterns; table-driven grouping will reduce drift and improve maintainability.As per coding guidelines
**/*_test.go: Use table-driven test patterns for Go tests.Also applies to: 161-265
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/machine/ssh_test.go` around lines 11 - 119, Refactor the repetitive TestDetectSSHKeyFiles* tests into a single table-driven suite: create a slice of test cases (name, setup func to create tmpDir/files or symlink, expected keys/count and expected properties) and loop with t.Run executing the shared assertions against DetectSSHKeyFiles; extract common setup/teardown helpers and reuse those. Do the same consolidation pattern for GenerateSSHKey* and GetSSHPublicKey* tests: define cases with inputs and expected results/errors, call the target functions inside the loop, and assert outcomes consistently. Ensure each case uses descriptive names, uses t.TempDir() inside the case setup, and preserves unique checks for Path/Type/Comment/Source/Loaded where needed. Use the existing function names DetectSSHKeyFiles, GenerateSSHKey, and GetSSHPublicKey to locate tests and helpers to refactor.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/g4d/machine.go`:
- Around line 332-333: The code currently ignores the error returned by
machine.DetectAllSSHKeys(sshDir) and proceeds to show keys, which can hide
discovery issues; update the call to capture the error (e.g., keys, err :=
machine.DetectAllSSHKeys(sshDir)) and if err is non-nil handle it appropriately
before listing keys — for example log or return the error via the surrounding
function (using processLogger or the calling function's error flow) and only
proceed to the existing keys branch (the len(keys) > 0 check) when err == nil.
- Around line 390-394: The call to machine.GetSSHPublicKey (variables pubKey,
err, keyPath, sshDir) currently ignores errors; update the block so that if err
!= nil you print a clear error to the user (e.g., to stderr) explaining the
failed attempt to read keyPath+".pub" and include the err message, then
return/exit with a non-zero status or propagate the error; otherwise keep the
existing success prints for pubKey. Use the existing machine.GetSSHPublicKey
call site in machine.go to implement this error-path reporting.
In `@internal/machine/ssh_test.go`:
- Around line 121-131: TestDetectAllSSHKeys currently only logs results and
won't catch regressions; update it to include assertions: call
DetectAllSSHKeys(tmpDir) and assert err is nil for a populated temp dir case,
assert returned slice length matches the number of created SSH key files, and
verify expected file names/paths (or key types) are present; also add a subtest
for an empty dir asserting no keys (len==0) and no panic/error; reference the
TestDetectAllSSHKeys test and DetectAllSSHKeys function to locate where to add
these checks.
- Around line 60-67: Replace the hardcoded "/nonexistent/..." path in
TestDetectSSHKeyFilesNonExistent with an OS-agnostic temp-based non-existent
path: call t.TempDir() to get a base dir, build a non-existent child path with
filepath.Join(tmp, "does_not_exist"), and pass that to DetectSSHKeyFiles; ensure
you import "path/filepath" (and remove any platform-specific assumptions) so the
test runs cross-platform in TestDetectSSHKeyFilesNonExistent and still asserts
err == nil and len(keys) == 0.
In `@internal/machine/ssh.go`:
- Around line 195-197: The os.Stat check before generating the SSH key (using
keyPath) currently treats any error as "does not exist"; update the check in the
function that calls os.Stat(keyPath) so that if err != nil you explicitly test
os.IsNotExist(err) and proceed only in that case, but for any other err return a
wrapped error with context (e.g., "stat keyPath") so permission/I/O errors fail
early instead of being ignored and causing confusing downstream ssh-keygen
failures; locate the os.Stat usage and the surrounding key generation logic
(references: keyPath, os.Stat) and modify the error handling accordingly.
- Around line 140-157: DetectAllSSHKeys is returning stale metadata because
merged is populated with value copies of agentKeys while agentMap holds pointers
to the original agentKeys elements; later mutations to agentMap (when merging
fileKeys) update the originals but not the copies in merged. Fix by making
merged mirror the same pointers you mutate: either change merged to store
pointers (e.g., []*SSHKey) and append &agentKeys[i] or reconstruct merged from
agentMap after merging so the returned slice contains the updated SSHKey
instances; update code that consumes merged accordingly. Ensure this touches the
agentMap, merged, agentKeys, fileKeys and SSHKey usage in DetectAllSSHKeys.
- Around line 199-202: After calling os.MkdirAll(expandedSSHDir, 0700) in the
SSH setup code, explicitly stat expandedSSHDir and verify its permission bits
are exactly 0700 (or at least not more permissive); if the directory has broader
permissions, either tighten them with os.Chmod(expandedSSHDir, 0700) or return
an error. Update the logic around expandedSSHDir (the same area that validates
generated key permissions) to use os.Stat to get the FileMode, check
mode.Perm(), and enforce or correct permissions before proceeding.
---
Nitpick comments:
In `@cmd/g4d/machine.go`:
- Around line 268-323: The machineKeysListCmd and machineKeysGenerateSSHCmd
cobra commands currently accept extra positional arguments; update each command
struct (machineKeysListCmd and machineKeysGenerateSSHCmd) to explicitly set
Args: cobra.NoArgs (e.g., add Args: cobra.NoArgs alongside Use/Short) so the CLI
rejects unexpected positional arguments and returns a clear error to the user.
In `@internal/machine/ssh_test.go`:
- Around line 11-119: Refactor the repetitive TestDetectSSHKeyFiles* tests into
a single table-driven suite: create a slice of test cases (name, setup func to
create tmpDir/files or symlink, expected keys/count and expected properties) and
loop with t.Run executing the shared assertions against DetectSSHKeyFiles;
extract common setup/teardown helpers and reuse those. Do the same consolidation
pattern for GenerateSSHKey* and GetSSHPublicKey* tests: define cases with inputs
and expected results/errors, call the target functions inside the loop, and
assert outcomes consistently. Ensure each case uses descriptive names, uses
t.TempDir() inside the case setup, and preserves unique checks for
Path/Type/Comment/Source/Loaded where needed. Use the existing function names
DetectSSHKeyFiles, GenerateSSHKey, and GetSSHPublicKey to locate tests and
helpers to refactor.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Additional Comments (1)
The CLI checks only for path separators ( Consider either removing this redundant partial check and letting |
- Fix value copy bug in DetectAllSSHKeys (mutations to agentMap were lost) - Use os.IsNotExist for key existence check instead of ignoring all errors - Verify and enforce SSH directory permissions (0700) after MkdirAll - Add cobra.NoArgs to list and generate-ssh subcommands - Surface detection errors and pubkey read failures to user - Block dangerous .pub-suffixed SSH filenames (authorized_keys.pub, etc) - Use OS-agnostic temp path in TestDetectSSHKeyFilesNonExistent - Make TestDetectAllSSHKeys assertive with proper validations Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
Author
|
@greptileai review |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/machine/ssh_test.go (2)
272-280: Use OS-agnostic path for traversal test.The hardcoded
/tmp/outside_key.pubpath breaks on Windows. Use a secondt.TempDir()to create a cross-platform path outsidesshDir.♻️ Proposed fix
func TestGetSSHPublicKeyTraversal(t *testing.T) { tmpDir := t.TempDir() - outsidePath := "/tmp/outside_key.pub" + outsideDir := t.TempDir() + outsidePath := filepath.Join(outsideDir, "outside_key.pub") _, err := GetSSHPublicKey(outsidePath, tmpDir) if err == nil { t.Fatal("expected error for path outside sshDir, got nil") } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/machine/ssh_test.go` around lines 272 - 280, Update the TestGetSSHPublicKeyTraversal test to use an OS-agnostic outside path by creating a second temp directory via t.TempDir() instead of the hardcoded "/tmp/outside_key.pub"; call GetSSHPublicKey with a file path located in that second temp dir (ensuring the file name ends with .pub) and assert that it returns an error. Locate the test function TestGetSSHPublicKeyTraversal and replace the hardcoded outsidePath with a path constructed inside the second temp dir so the test works cross-platform.
282-293: Use OS-agnostic path for traversal test.The hardcoded
/tmp/../etc/shadowpath is Unix-specific. Construct a cross-platform path that escapessshDirusingfilepath.Join.♻️ Proposed fix
func TestAddKeyToAgentBadPath(t *testing.T) { tmpDir := t.TempDir() - // Path with traversal should fail validation - err := AddKeyToAgent("/tmp/../etc/shadow", tmpDir) + // Path escaping sshDir should fail validation + outsideDir := t.TempDir() + badPath := filepath.Join(outsideDir, "outside_key") + err := AddKeyToAgent(badPath, tmpDir) if err == nil { t.Fatal("expected error for bad path, got nil") } if !strings.Contains(err.Error(), "invalid key path") { t.Errorf("expected 'invalid key path' error, got: %v", err) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/machine/ssh_test.go` around lines 282 - 293, Replace the hardcoded Unix path in TestAddKeyToAgentBadPath with a OS-agnostic traversal path built using filepath.Join so the test attempts to escape the temporary ssh dir; e.g., use filepath.Join(tmpDir, "..", "etc", "shadow") (or similar components) as the key path passed to AddKeyToAgent, and ensure the test file imports "path/filepath" if not already present; keep assertions on err and the "invalid key path" message unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/machine/ssh_test.go`:
- Around line 272-280: Update the TestGetSSHPublicKeyTraversal test to use an
OS-agnostic outside path by creating a second temp directory via t.TempDir()
instead of the hardcoded "/tmp/outside_key.pub"; call GetSSHPublicKey with a
file path located in that second temp dir (ensuring the file name ends with
.pub) and assert that it returns an error. Locate the test function
TestGetSSHPublicKeyTraversal and replace the hardcoded outsidePath with a path
constructed inside the second temp dir so the test works cross-platform.
- Around line 282-293: Replace the hardcoded Unix path in
TestAddKeyToAgentBadPath with a OS-agnostic traversal path built using
filepath.Join so the test attempts to escape the temporary ssh dir; e.g., use
filepath.Join(tmpDir, "..", "etc", "shadow") (or similar components) as the key
path passed to AddKeyToAgent, and ensure the test file imports "path/filepath"
if not already present; keep assertions on err and the "invalid key path"
message unchanged.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/g4d/machine.gointernal/machine/ssh.gointernal/machine/ssh_test.gointernal/validation/validation.gointernal/validation/validation_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/validation/validation_test.go
- cmd/g4d/machine.go
Additional Comments (1)
While |
- Add config.pub and environment.pub to dangerous SSH filenames blocklist - Surface agent detection errors in DetectAllSSHKeys instead of discarding - Use OS-agnostic temp paths in all test assertions (no hardcoded /tmp) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
Author
|
@greptileai review |
The register subcommand is added in a later PR in the stack. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
Author
|
@greptileai review |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/machine/ssh_test.go`:
- Around line 312-313: TestExpandSSHDir currently hardcodes "/home/user/.ssh"
which is not an absolute path on Windows; update the test to compute an
OS-appropriate absolute SSH path at runtime (e.g. derive the current user's home
via os.UserHomeDir() or call filepath.Abs on filepath.Join(home, ".ssh")) and
use that value for the "absolute path" case so filepath.IsAbs/ExpandSSHDir
assertions are portable; keep the "relative path" case as ".ssh" and assert
wantErr/wantAbs accordingly, referencing the TestExpandSSHDir table entry and
any uses of filepath.IsAbs or ExpandSSHDir.
In `@internal/validation/validation.go`:
- Around line 233-235: The current email validation only rejects "\n\r\t\x00"
but allows other control characters; in the function that validates the variable
email (the block with if strings.ContainsAny(email, "\n\r\t\x00")), replace that
naive check with a proper control-character scan: iterate the runes/bytes of
email and use unicode.IsControl (or test byte values <= 0x1F or == 0x7F) to
detect any control characters, and return the same fmt.Errorf("email must not
contain control characters: %q", email) if any are found so all control bytes
(not just \n\r\t\x00) are blocked.
- Around line 271-273: The filename lookup against dangerousSSHFilenames is
case-sensitive and can be bypassed on case-insensitive filesystems; normalize
the filename to lowercase (e.g., use strings.ToLower) before checking the map so
the check like dangerousSSHFilenames[filename] becomes
dangerousSSHFilenames[normalizedFilename], updating the validation logic that
currently references filename so reserved SSH names (e.g., "config",
"known_hosts") are detected regardless of case.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/g4d/machine.gointernal/machine/ssh.gointernal/machine/ssh_test.gointernal/validation/validation.gointernal/validation/validation_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/machine/ssh.go
- internal/validation/validation_test.go
- Use generic "Git hosting service" instead of hardcoding "GitHub" - Expand email control character check to all C0/DEL control chars - Use OS-agnostic temp path in TestExpandSSHDir Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
Author
|
@greptileai review |
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/doctor/report.go (1)
30-35:⚠️ Potential issue | 🟡 MinorIncomplete fix: remaining
WriteString(fmt.Sprintf(...))patterns still trigger pipeline failure.The pipeline reports a golangci-lint QF1012 error on line 30. The same pattern persists throughout the file (lines 30-35, 43, 46, 49, 52, etc.). Apply the same
fmt.Fprintfrefactor consistently.🔧 Proposed fix for the immediate block
for _, check := range r.Checks { icon := statusIcon(check.Status) - sb.WriteString(fmt.Sprintf("%s %s\n", icon, check.Name)) - sb.WriteString(fmt.Sprintf(" %s\n", check.Message)) + fmt.Fprintf(&sb, "%s %s\n", icon, check.Name) + fmt.Fprintf(&sb, " %s\n", check.Message) if check.Fix != "" && check.Status != StatusOK { - sb.WriteString(fmt.Sprintf(" → %s\n", check.Fix)) + fmt.Fprintf(&sb, " → %s\n", check.Fix) } sb.WriteString("\n") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/doctor/report.go` around lines 30 - 35, Replace occurrences of sb.WriteString(fmt.Sprintf(...)) with fmt.Fprintf(sb, ...) to avoid unnecessary formatting allocation; specifically, update the usage around sb.WriteString(fmt.Sprintf("%s %s\n", icon, check.Name)), sb.WriteString(fmt.Sprintf(" %s\n", check.Message)), the conditional sb.WriteString(fmt.Sprintf(" → %s\n", check.Fix))) and all other similar patterns in this file (e.g., the later occurrences at the other check output lines) so each formatted string is written directly into the string builder via fmt.Fprintf(sb, format, args...). Ensure you import fmt if not already and keep the same format strings and arguments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/doctor/report.go`:
- Around line 30-35: Replace occurrences of sb.WriteString(fmt.Sprintf(...))
with fmt.Fprintf(sb, ...) to avoid unnecessary formatting allocation;
specifically, update the usage around sb.WriteString(fmt.Sprintf("%s %s\n",
icon, check.Name)), sb.WriteString(fmt.Sprintf(" %s\n", check.Message)), the
conditional sb.WriteString(fmt.Sprintf(" → %s\n", check.Fix))) and all other
similar patterns in this file (e.g., the later occurrences at the other check
output lines) so each formatted string is written directly into the string
builder via fmt.Fprintf(sb, format, args...). Ensure you import fmt if not
already and keep the same format strings and arguments.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/g4d/machine.gointernal/doctor/report.gointernal/machine/ssh_test.gointernal/validation/validation.go
Fix pre-existing QF1012 staticcheck lint issue caught by CI's newer golangci-lint version. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
nvandessel
force-pushed
the
feat/ssh-keygen-wizard
branch
from
4485d9a to
836d026
Compare
Owner
Author
|
@greptileai review |
1 similar comment
Owner
Author
|
@greptileai review |
Fix all QF1012 staticcheck lint issues across the codebase caught by CI's newer golangci-lint v2.10.1. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
nvandessel
force-pushed
the
feat/ssh-keygen-wizard
branch
from
e7402d8 to
cc6fa73
Compare
Owner
Author
|
@greptileai review |
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (56.97%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #104 +/- ##
==========================================
+ Coverage 47.99% 48.68% +0.69%
==========================================
Files 107 108 +1
Lines 11718 12059 +341
==========================================
+ Hits 5624 5871 +247
- Misses 6094 6188 +94
🚀 New features to boost your workflow:
|
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
1 participant
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
ValidateEmail()andValidateSSHKeyPath()security validators with flag injection, path traversal, and dangerous filename protectioninternal/machine/ssh.gowithDetectSSHKeyFiles(),DetectAllSSHKeys(),GenerateSSHKey(),AddKeyToAgent(),GetSSHPublicKey(),IsAgentRunning()SSHKeystruct withLoaded/Sourcefields for tracking key origing4d machine keys list,g4d machine keys generate-sshTest plan
make build && make lint && make testpassesg4d machine keys listshows detected keysg4d machine keys generate-ssh --email user@example.comcreates key with 0600 perms🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes / Validation
Tests