Skip to content

fix: ensure SSH signing key has trailing newline #834

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ashwin-ant merged 1 commit into from
Jan 16, 2026
Merged

fix: ensure SSH signing key has trailing newline #834

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/github/operations/git-config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,13 @@ export async function setupSshSigning(sshSigningKey: string): Promise<void> {
const sshDir = join(homedir(), ".ssh");
await mkdir(sshDir, { recursive: true, mode: 0o700 });

// Ensure key ends with newline (required for ssh-keygen to parse it)
const normalizedKey = sshSigningKey.endsWith("\n")
Copy link
Contributor

@claude claude bot Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider whether keys with multiple trailing newlines should be normalized to exactly one. The current logic preserves multiple newlines if they exist:

// Current: "\n\n\n" remains "\n\n\n"
// Alternative: Normalize to exactly one
const normalizedKey = sshSigningKey.trimEnd() + "\n";

However, this is likely fine given ssh-keygen's requirements. If you want to be more defensive, the trimEnd() approach ensures exactly one trailing newline regardless of input.

? sshSigningKey
: sshSigningKey + "\n";

// Write the signing key atomically with secure permissions (600)
await writeFile(SSH_SIGNING_KEY_PATH, sshSigningKey, { mode: 0o600 });
await writeFile(SSH_SIGNING_KEY_PATH, normalizedKey, { mode: 0o600 });
console.log(`✓ SSH signing key written to ${SSH_SIGNING_KEY_PATH}`);

// Configure git to use SSH signing
Expand Down
41 changes: 41 additions & 0 deletions test/ssh-signing.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,47 @@ describe("SSH Signing", () => {
expect(permissions).toBe(0o600);
});

test("should normalize key to have trailing newline", async () => {
// ssh-keygen requires a trailing newline to parse the key
const keyWithoutNewline =
"-----BEGIN OPENSSH PRIVATE KEY-----\ntest-key-content\n-----END OPENSSH PRIVATE KEY-----";
const keyWithNewline = keyWithoutNewline + "\n";

// Create directory
await mkdir(testSshDir, { recursive: true, mode: 0o700 });

// Normalize the key (same logic as setupSshSigning)
const normalizedKey = keyWithoutNewline.endsWith("\n")
? keyWithoutNewline
: keyWithoutNewline + "\n";
Copy link
Contributor

@claude claude bot Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current tests duplicate the normalization logic instead of testing the actual setupSshSigning function. Consider adding an integration test:

import { setupSshSigning } from "../src/github/operations/git-config";

test("setupSshSigning should normalize key without newline", async () => {
  const keyWithoutNewline = "-----BEGIN OPENSSH PRIVATE KEY-----\ntest\n-----END OPENSSH PRIVATE KEY-----";
  
  await setupSshSigning(keyWithoutNewline);
  
  const writtenKey = await readFile(SSH_SIGNING_KEY_PATH, "utf-8");
  expect(writtenKey.endsWith("\n")).toBe(true);
  expect(writtenKey.endsWith("\n\n")).toBe(false);
});

This would test the actual function behavior rather than duplicated logic.


await writeFile(testKeyPath, normalizedKey, { mode: 0o600 });

// Verify the written key ends with newline
const keyContent = await readFile(testKeyPath, "utf-8");
expect(keyContent).toBe(keyWithNewline);
expect(keyContent.endsWith("\n")).toBe(true);
});

test("should not add extra newline if key already has one", async () => {
const keyWithNewline =
"-----BEGIN OPENSSH PRIVATE KEY-----\ntest-key-content\n-----END OPENSSH PRIVATE KEY-----\n";

await mkdir(testSshDir, { recursive: true, mode: 0o700 });

// Normalize the key (same logic as setupSshSigning)
const normalizedKey = keyWithNewline.endsWith("\n")
? keyWithNewline
: keyWithNewline + "\n";

await writeFile(testKeyPath, normalizedKey, { mode: 0o600 });

// Verify no double newline
const keyContent = await readFile(testKeyPath, "utf-8");
expect(keyContent).toBe(keyWithNewline);
expect(keyContent.endsWith("\n\n")).toBe(false);
Copy link
Contributor

@claude claude bot Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assertion only verifies the key doesn't end with two newlines, but doesn't verify it ends with exactly one. A key ending with three newlines would pass this test. Consider:

expect(keyContent).toBe(keyWithNewline);
expect(keyContent.endsWith("\n")).toBe(true);
expect(keyContent.charAt(keyContent.length - 2)).not.toBe("\n");

});

test("should create .ssh directory with secure permissions", async () => {
// Clean up first
await rm(testSshDir, { recursive: true, force: true });
Expand Down
Loading