feat(e2b): SSH key injection for private repository access - #45
Conversation
Implements opt-in SSH key injection for E2B sandboxes to enable cloning and pushing to private Git repositories. Features: - Key validation (existence, permissions, format, type detection) - Interactive security warning with explicit consent - Known hosts auto-config for GitHub, GitLab, Bitbucket - SSH config with StrictHostKeyChecking accept-new - Automatic cleanup after execution (even on errors) - Sensitive data redaction in logs (SSH keys, API keys, tokens) CLI Options: - --ssh-key <path>: Path to SSH private key - --confirm-ssh-key: Skip interactive prompt (for CI/CD) Security: - Keys transmitted over TLS to E2B sandbox - Keys cleaned up in finally block - Log redaction prevents key leakage - Warns about passphrase-protected keys Tests: - 42 tests for SSH key injector module - 24 tests for logger sensitive data redaction - All 66 new tests passing Note: Pre-commit secret detection flagged test strings as false positives. These are test patterns for validating redaction, not actual secrets.
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 24691427 | Triggered | RSA Private Key | 87d1aeb | tests/e2b/ssh-key-injector.test.ts | View secret |
| 24691428 | Triggered | GitHub Personal Access Token | 87d1aeb | tests/logger-redaction.test.ts | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
|
Caution Review failedThe pull request is closed. WalkthroughAdds SSH key injection support for E2B sandboxes (validate -> inject -> cleanup), CLI flags ( Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Validator as SSH Validator
participant SandboxManager
participant Sandbox
participant Injector as SSH Injector
participant Logger
CLI->>Validator: validateSSHKeyPath(path)
Validator-->>CLI: { isValid, warnings, keyType }
CLI->>CLI: prompt/confirm if needed
CLI->>SandboxManager: createSandbox(sessionId)
SandboxManager-->>CLI: { sandbox, sandboxId }
CLI->>Injector: injectSSHKey(sandbox, path, logger)
Injector->>Sandbox: create ~/.ssh, write key, set perms
Injector->>Sandbox: configure known_hosts and ssh_config
Injector-->>CLI: { success, keyFilename, fingerprint, keyType }
CLI->>Logger: log(injected info)
CLI->>Sandbox: run commands (e.g., git clone)
CLI->>Injector: cleanupSSHKey(sandbox, logger, keyFilename)
Injector->>Sandbox: remove key files and config
Injector-->>CLI: cleanup complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Comment |
Add SSH key injection with validation and cleanup to
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
|
|
||
| try { | ||
| // Write key content | ||
| await sandbox.files.write(`/root/.ssh/${keyFilename}`, keyContent); |
There was a problem hiding this comment.
Suggestion: unify SSH key path/filename handling—resolve $HOME in the sandbox and use one absolute .ssh path for injection and commands; persist the injected keyFilename and use it in cleanup instead of id_* patterns.
🚀 Want me to fix this? Reply ex: "fix it for me".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @tests/logger-redaction.test.ts:
- Around line 211-219: The test uses a truncated SSH key so it doesn't exercise
Logger.warn's redaction; update the sensitiveMessage in the test to include a
realistic SSH public key base64 segment (40+ base64 chars after the key type,
e.g., "ssh-rsa " followed by a long base64 string) so it matches the redaction
regex, and change the assertion to verify the logged payload was redacted (use
consoleWarnSpy to assert the message contains the redaction token such as
"[REDACTED]" or that the base64 segment is removed) instead of only checking
that consoleWarnSpy was called.
🧹 Nitpick comments (5)
src/logger.ts (1)
83-99: Minor: Redundant conditional branches.Lines 91-95 have identical behavior in both branches of the
typeof replacementcheck. The code works correctly, but this can be simplified.♻️ Suggested simplification
for (const { pattern, replacement } of REDACTION_PATTERNS) { - if (typeof replacement === 'function') { - result = result.replace(pattern, replacement); - } else { - result = result.replace(pattern, replacement); - } + result = result.replace(pattern, replacement as string | ((...args: any[]) => string)); }src/e2b/ssh-key-injector.ts (1)
373-374: Unused variableremoteKeyPath.The variable is declared but never used. Consider removing it.
♻️ Remove unused variable
// Step 2: Write SSH key with proper permissions - const remoteKeyPath = `~/.ssh/${keyFilename}`; - try {CLAUDE.md (1)
740-795: Excellent documentation for the SSH Key Injection feature.The documentation covers:
- Usage examples for interactive and non-interactive modes
- Clear security flow explanation
- Security considerations and best practices
- Supported key types with recommendations
- Troubleshooting table for common errors
One minor markdown lint issue: add a blank line before the troubleshooting table (line 789) per MD058.
📝 Add blank line before table
**Troubleshooting:** + | Error | Solution |tests/logger-redaction.test.ts (1)
171-232: Logger integration tests verify redaction across all log levels.Good practice to test the integration between redaction and logging at each level. The tests properly set up mocks and verify that sensitive data is not present in logged output.
Consider adding
afterEachto restoreprocess.env.PARALLEL_CC_LOG_LEVELto avoid test pollution.♻️ Add environment cleanup
describe('Logger integration', () => { let consoleSpy: any; let consoleErrorSpy: any; let consoleWarnSpy: any; + let originalLogLevel: string | undefined; beforeEach(() => { + originalLogLevel = process.env.PARALLEL_CC_LOG_LEVEL; consoleSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); consoleWarnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); }); + afterEach(() => { + process.env.PARALLEL_CC_LOG_LEVEL = originalLogLevel; + });tests/e2b/ssh-key-injector.test.ts (1)
17-31: Consider removing local interface definitions.The
SSHInjectionResultandSSHValidationResultinterfaces are already exported fromsrc/e2b/ssh-key-injector.ts. While the local definitions don't cause issues (TypeScript uses structural typing), they could drift from the source definitions over time.♻️ Import interfaces from source module
-// Types for our module (will be implemented) -interface SSHInjectionResult { - success: boolean; - keyFingerprint?: string; - keyType?: string; - error?: string; -} - -interface SSHValidationResult { - valid: boolean; - keyType?: string; - permissionsOk: boolean; - permissionsWarning?: string; - error?: string; -} +import type { SSHInjectionResult, SSHValidationResult } from '../../src/e2b/ssh-key-injector.js';
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CLAUDE.mdsrc/cli.tssrc/e2b/ssh-key-injector.tssrc/logger.tstests/e2b/ssh-key-injector.test.tstests/logger-redaction.test.ts
🧰 Additional context used
📓 Path-based instructions (7)
tests/**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.test.ts: Use Vitest framework for unit and integration testing
Use describe/it blocks in Vitest for organizing test cases with clear assertions
Files:
tests/logger-redaction.test.tstests/e2b/ssh-key-injector.test.ts
tests/e2b/**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Mock E2B SDK completely in tests with real filesystem/database/git operations for E2E tests
Files:
tests/e2b/ssh-key-injector.test.ts
src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.ts: Use TypeScript strict mode for all TypeScript files
Use async/await syntax instead of callbacks for asynchronous operations
Implement explicit error handling for all operations
Use meaningful and descriptive variable names following camelCase convention
Export type definitions from types.ts for all shared interfaces and types
Implement comprehensive logging using logger.ts for all operations
Use ESLint for code linting with project-specific configuration
Files:
src/e2b/ssh-key-injector.tssrc/logger.tssrc/cli.ts
src/e2b/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Implement E2B sandbox lifecycle management for autonomous Claude execution
Files:
src/e2b/ssh-key-injector.ts
src/e2b/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Support three-tier git identity priority: CLI flags > environment variables > local git config > default
Files:
src/e2b/ssh-key-injector.ts
CLAUDE.md
📄 CodeRabbit inference engine (CLAUDE.md)
Document all new features in CLAUDE.md with version history and architecture updates
Files:
CLAUDE.md
src/cli.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use Commander.js for CLI argument parsing and command routing
Files:
src/cli.ts
🧠 Learnings (17)
📓 Common learnings
Learnt from: CR
Repo: frankbria/parallel-cc PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T03:37:18.645Z
Learning: Applies to tests/e2b/**/*.test.ts : Mock E2B SDK completely in tests with real filesystem/database/git operations for E2E tests
Learnt from: CR
Repo: frankbria/parallel-cc PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T03:37:18.645Z
Learning: Applies to src/e2b/file-sync.ts : Implement automatic credential scanning and exclusion from E2B sandbox uploads
📚 Learning: 2026-01-13T03:37:18.645Z
Learnt from: CR
Repo: frankbria/parallel-cc PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T03:37:18.645Z
Learning: Applies to src/**/*.ts : Implement comprehensive logging using logger.ts for all operations
Applied to files:
tests/logger-redaction.test.tsCLAUDE.mdsrc/logger.ts
📚 Learning: 2026-01-13T03:37:18.645Z
Learnt from: CR
Repo: frankbria/parallel-cc PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T03:37:18.645Z
Learning: Applies to tests/e2b/**/*.test.ts : Mock E2B SDK completely in tests with real filesystem/database/git operations for E2E tests
Applied to files:
tests/logger-redaction.test.tstests/e2b/ssh-key-injector.test.tssrc/e2b/ssh-key-injector.tsCLAUDE.mdsrc/cli.ts
📚 Learning: 2026-01-13T03:37:18.645Z
Learnt from: CR
Repo: frankbria/parallel-cc PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T03:37:18.645Z
Learning: Applies to tests/**/*.test.ts : Use describe/it blocks in Vitest for organizing test cases with clear assertions
Applied to files:
tests/logger-redaction.test.tsCLAUDE.md
📚 Learning: 2026-01-13T03:37:18.645Z
Learnt from: CR
Repo: frankbria/parallel-cc PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T03:37:18.645Z
Learning: Applies to tests/**/*.test.ts : Use Vitest framework for unit and integration testing
Applied to files:
tests/logger-redaction.test.tsCLAUDE.md
📚 Learning: 2026-01-13T03:37:18.645Z
Learnt from: CR
Repo: frankbria/parallel-cc PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T03:37:18.645Z
Learning: Applies to src/e2b/file-sync.ts : Implement automatic credential scanning and exclusion from E2B sandbox uploads
Applied to files:
tests/e2b/ssh-key-injector.test.tssrc/e2b/ssh-key-injector.tssrc/cli.ts
📚 Learning: 2026-01-13T03:37:18.645Z
Learnt from: CR
Repo: frankbria/parallel-cc PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T03:37:18.645Z
Learning: Applies to src/e2b/*.ts : Implement E2B sandbox lifecycle management for autonomous Claude execution
Applied to files:
src/e2b/ssh-key-injector.tsCLAUDE.mdsrc/cli.ts
📚 Learning: 2026-01-13T03:37:18.645Z
Learnt from: CR
Repo: frankbria/parallel-cc PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T03:37:18.645Z
Learning: Applies to CLAUDE.md : Document all new features in CLAUDE.md with version history and architecture updates
Applied to files:
CLAUDE.md
📚 Learning: 2026-01-13T03:37:18.646Z
Learnt from: CR
Repo: frankbria/parallel-cc PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T03:37:18.646Z
Learning: Run 'npm test' with coverage reports to ensure minimum 85% coverage before merging
Applied to files:
CLAUDE.md
📚 Learning: 2026-01-13T03:37:18.645Z
Learnt from: CR
Repo: frankbria/parallel-cc PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T03:37:18.645Z
Learning: Applies to vitest.config.ts : Use heap snapshots and v8 coverage for test analysis (Vitest with v8 coverage)
Applied to files:
CLAUDE.md
📚 Learning: 2026-01-13T03:37:18.645Z
Learnt from: CR
Repo: frankbria/parallel-cc PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T03:37:18.645Z
Learning: Applies to src/mcp/**/*.ts : Implement MCP server with stdio transport and expose exactly 16 tools for session and conflict management
Applied to files:
CLAUDE.md
📚 Learning: 2026-01-13T03:37:18.645Z
Learnt from: CR
Repo: frankbria/parallel-cc PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T03:37:18.645Z
Learning: Applies to src/mcp/**/*.ts : Use Zod for input validation and schema definition in MCP tools
Applied to files:
CLAUDE.md
📚 Learning: 2026-01-13T03:37:18.645Z
Learnt from: CR
Repo: frankbria/parallel-cc PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T03:37:18.645Z
Learning: Applies to src/**/*.ts : Use meaningful and descriptive variable names following camelCase convention
Applied to files:
CLAUDE.md
📚 Learning: 2026-01-13T03:37:18.645Z
Learnt from: CR
Repo: frankbria/parallel-cc PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T03:37:18.645Z
Learning: Applies to src/e2b/**/*.ts : Support three-tier git identity priority: CLI flags > environment variables > local git config > default
Applied to files:
CLAUDE.mdsrc/cli.ts
📚 Learning: 2026-01-13T03:37:18.645Z
Learnt from: CR
Repo: frankbria/parallel-cc PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T03:37:18.645Z
Learning: Applies to src/**/*.ts : Use TypeScript strict mode for all TypeScript files
Applied to files:
CLAUDE.md
📚 Learning: 2026-01-13T03:37:18.645Z
Learnt from: CR
Repo: frankbria/parallel-cc PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T03:37:18.645Z
Learning: Applies to src/e2b/file-sync.ts : Use compression for file uploads/downloads in E2B sandbox file synchronization
Applied to files:
CLAUDE.mdsrc/cli.ts
📚 Learning: 2026-01-13T03:37:18.645Z
Learnt from: CR
Repo: frankbria/parallel-cc PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T03:37:18.645Z
Learning: Applies to src/e2b/sandbox-manager.ts : Enforce 1-hour maximum timeout for E2B sandboxes with soft warnings at 30min and 50min marks
Applied to files:
CLAUDE.mdsrc/cli.ts
🧬 Code graph analysis (5)
tests/logger-redaction.test.ts (1)
src/logger.ts (4)
redactSensitive(83-99)REDACTION_PATTERNS(26-69)logger(189-189)Logger(116-187)
tests/e2b/ssh-key-injector.test.ts (1)
src/e2b/ssh-key-injector.ts (7)
SSHInjectionResult(50-59)SSHValidationResult(34-45)validateSSHKeyPath(117-249)injectSSHKey(346-458)cleanupSSHKey(475-494)getSecurityWarning(303-329)detectKeyType(258-279)
src/e2b/ssh-key-injector.ts (1)
src/logger.ts (3)
error(140-157)logger(189-189)Logger(116-187)
src/logger.ts (1)
scripts/print.mjs (1)
message(11-11)
src/cli.ts (2)
src/e2b/ssh-key-injector.ts (4)
validateSSHKeyPath(117-249)getSecurityWarning(303-329)injectSSHKey(346-458)cleanupSSHKey(475-494)src/logger.ts (1)
logger(189-189)
🪛 Gitleaks (8.30.0)
tests/logger-redaction.test.ts
[high] 13-21: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
[high] 29-36: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
[high] 110-131: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
[high] 189-203: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
tests/e2b/ssh-key-injector.test.ts
[high] 241-263: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
[high] 284-299: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
[high] 531-540: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
[high] 549-558: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
[high] 568-588: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
src/e2b/ssh-key-injector.ts
[high] 69-71: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
🪛 markdownlint-cli2 (0.18.1)
CLAUDE.md
789-789: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: opencode-review
- GitHub Check: Macroscope - Correctness Check
🔇 Additional comments (23)
src/logger.ts (2)
26-69: LGTM! Comprehensive redaction patterns for sensitive data.The patterns cover SSH private/public keys, key fingerprints, Anthropic API keys, GitHub tokens, generic API key patterns, and long base64 strings. The partial redaction for fingerprints (keeping first 8 chars) is a nice touch for debugging while maintaining security.
140-186: LGTM! Logger methods properly integrate redaction.All log methods (error, warn, info, debug) correctly apply
redactSensitive()to messages, error messages, stack traces, and serialized data before output.src/e2b/ssh-key-injector.ts (4)
34-59: LGTM! Well-designed interfaces for validation and injection results.Clear separation of concerns with dedicated result types that include all necessary information for error handling and logging.
68-74: False positive from static analysis - these are regex patterns, not actual secrets.The Gitleaks warning is expected here as these are intentionally patterns to detect SSH key headers for validation purposes, not actual private keys.
117-249: Solid validation logic with comprehensive security checks.The validation function properly checks:
- Path length limits (defense against buffer issues)
- Dangerous shell characters (command injection prevention)
- File existence and readability
- File type (not directory)
- Permission mode (warns on overly permissive)
- Key format validity
- Public vs private key detection
- Encrypted key detection with appropriate warning
346-458: Well-structured injection workflow with appropriate error handling.The injection follows a logical sequence:
- Create .ssh directory with 700 permissions
- Write key with 600 permissions
- Configure known_hosts for common providers
- Create SSH config with appropriate settings
- Extract fingerprint for logging (non-critical, graceful failure)
Good use of non-blocking warnings for non-critical failures while properly failing on critical operations.
CLAUDE.md (2)
137-142: LGTM! Test documentation updated to reflect new coverage.Test file list and counts are updated to include the new SSH key injector tests and logger redaction tests.
876-879: LGTM! Module structure updated to include new SSH key injector.The E2B module file tree now correctly shows the new
ssh-key-injector.tsfile.tests/logger-redaction.test.ts (2)
1-9: LGTM! Well-structured test file with clear documentation.The test file follows Vitest conventions with proper imports and clear documentation of what's being tested. Based on learnings, the test uses describe/it blocks as required.
12-154: Comprehensive unit tests for the redactSensitive function.Good coverage of:
- All SSH key formats (RSA, OpenSSH, DSA, EC, generic)
- Partial fingerprint redaction
- Long base64 strings
- SSH public key formats
- API keys and GitHub tokens
- Edge cases (empty, null, undefined, multiple patterns)
- Preservation of surrounding text
The static analysis hints about private keys are false positives - these are test strings for validating redaction.
src/cli.ts (6)
35-35: LGTM! Clean import of SSH key injector utilities.All necessary functions are imported for the CLI integration.
1395-1396: LGTM! New CLI options follow Commander.js conventions.The options are well-named and clearly documented in the command description above.
1510-1564: Well-implemented SSH key validation and confirmation flow.Good security practices:
- Path expansion for
~prefix- Validation before any sandbox operations
- Permissions warning displayed to user
- Interactive security warning with explicit confirmation
- JSON mode requires
--confirm-ssh-keyflag (fail-safe for CI/CD)The flow correctly handles user cancellation by clearing the
options.sshKeyvalue.
1702-1731: LGTM! SSH key injection properly integrated into sandbox workflow.The injection happens after sandbox creation and tarball upload, which is the correct sequence. Success/failure handling is appropriate with structured JSON output for automation.
2008-2017: LGTM! SSH key cleanup in finally block ensures cleanup even on errors.The cleanup correctly:
- Only runs if
sshKeyInjectedis true- Only runs if
sandboxexists- Catches and logs errors without re-throwing (won't mask original errors)
- Uses
logger.warnfor cleanup failuresThis is the correct pattern for cleanup operations.
1672-1682: Good refactoring to handle structured createSandbox return value.The destructuring correctly extracts both
sandboxandsandboxIdfrom the result object.tests/e2b/ssh-key-injector.test.ts (7)
1-12: LGTM! Clear test file documentation.The header clearly documents what's being tested and confirms that all operations are mocked.
33-56: LGTM! Appropriate mocking strategy.Mocks for
fs/promises,fs, andchild_processalign with the guideline to mock E2B SDK completely in tests. The mock logger provides all necessary methods for verifying log calls.
80-293: Comprehensive validation tests covering all scenarios.Excellent coverage including:
- Valid key file validation
- Non-existent file rejection
- Permission denied handling
- Overly permissive permissions warning
- Valid read-only permissions (400)
- Invalid format rejection
- Directory rejection
- Key type detection (RSA, Ed25519, ECDSA, DSA)
Static analysis hints about private keys are false positives - these are test fixture strings.
295-423: Thorough injection tests covering the complete workflow.Tests verify:
- Directory creation with correct permissions
- Key file writing with 600 permissions
- known_hosts configuration for all providers
- SSH config creation with StrictHostKeyChecking
- Success result with fingerprint
- Error handling for file write failures
- Error handling for command failures
- Filename preservation
- Logging verification
425-494: Good cleanup tests including error resilience.Tests verify that cleanup:
- Removes key files, known_hosts, and config
- Logs completion
- Handles errors gracefully (warns but doesn't throw)
- Handles missing files gracefully
This matches the implementation's behavior in the finally block.
575-656: Excellent integration and error recovery tests.The full workflow test (validate → inject → use → cleanup) validates the complete user journey. The error recovery tests ensure the system degrades gracefully when sandbox operations fail.
658-748: Good edge case coverage.Tests cover important edge cases:
- Passphrase-protected keys (with warning detection)
- Very long paths (rejected)
- Paths with special characters (shell injection prevention)
- Empty key files
- Public key mistakenly provided instead of private key
These edge cases align well with the security-focused implementation.
PR Review: feat(e2b): SSH key injection for private repository accessOverall AssessmentThis is a well-implemented feature with excellent security considerations and comprehensive test coverage. The implementation follows the project's coding standards and integrates cleanly with the existing E2B sandbox architecture. Score: ✅ Approve with minor suggestions ✅ StrengthsSecurity Design
Code Quality
Test Coverage
|
| Check | Status | Notes |
|---|---|---|
| TypeScript compilation | ✅ Passes | No issues (existing @types/node errors unrelated) |
| ESLint | ✅ Passes | No linting issues in new files |
| Test coverage | ✅ Excellent | 42 + 24 tests, 100% passing |
| Documentation | ✅ Complete | JSDoc comments, CLAUDE.md updated |
| Error handling | ✅ Good | Try/catch blocks, meaningful errors |
| Security | ✅ Strong | Validation, redaction, cleanup |
📝 Minor Code Style Suggestions
1. src/cli.ts:1537-1541 - Duplicate error handling for SSH key validation
The same error handling block appears twice. Consider extracting to a helper:
function handleSSHKeyValidationError(
validation: Awaited<ReturnType<typeof validateSSHKeyPath>>,
sshKeyPath: string,
jsonMode: boolean
): never {
const error = `SSH key validation failed: ${validation.error}`;
if (jsonMode) {
console.log(JSON.stringify({ success: false, error, keyPath: sshKeyPath }));
} else {
console.error(chalk.red(`✗ ${error}`));
}
process.exit(1);
}2. src/logger.ts:83 - Null check could be more defensive
export function redactSensitive(message: string): string {
if (!message || typeof message !== 'string') {
return ''; // Could return '<null>' or '<undefined>' for debugging
}
// ...
}🚀 Performance Considerations
| Operation | Current | Optimized |
|---|---|---|
| SSH key validation | ~5-10ms | ✅ Good |
| Known hosts setup | 90s (3 providers × 30s) | |
| Key injection | ~1-2s | ✅ Good |
| Cleanup | ~100ms | ✅ Good |
The 90s timeout for ssh-keyscan is the only performance concern. Consider:
- Reducing timeout to 10s per provider
- Making it configurable via CLI flag
- Failing fast if
ssh-keyscanis missing
🔒 Security Review Summary
| Aspect | Rating | Notes |
|---|---|---|
| Key transmission | ✅ Secure | E2B uses TLS encryption |
| Key storage | ✅ Secure | 600 permissions, temporary sandbox |
| Key cleanup | ✅ Secure | Finally block ensures removal |
| Log redaction | ✅ Excellent | Comprehensive patterns |
| Permission checks | ✅ Good | Warns on insecure permissions |
| Path validation | Symlink check recommended | |
| Shell injection protection | ✅ Good | Dangerous char filtering |
🧪 Test Coverage
New tests:
-
✅
tests/e2b/ssh-key-injector.test.ts- 42 tests- Validation (existence, permissions, format, key types)
- Injection (permissions, known_hosts, SSH config)
- Cleanup (key removal, error handling)
- Edge cases (encrypted keys, long paths, public keys)
-
✅
tests/logger-redaction.test.ts- 24 tests- SSH key redaction (RSA, OpenSSH, DSA, EC, generic)
- Public key redaction
- API key redaction (Anthropic, GitHub)
- Fingerprint partial redaction
- Logger integration
Coverage: ⭐ Excellent - 100% passing
✅ Final Recommendation
Status: ✅ Approve with minor suggestions
This is a production-ready implementation with strong security and excellent test coverage. The suggested improvements (symlink check, cleanup pattern, path validation) are nice-to-haves but not blocking issues.
Before merge:
- Consider adding symlink validation (5-line change, high security value)
- Optionally tighten cleanup to only remove injected key
Post-merge:
- Monitor for path validation false positives in production
- Consider caching known_hosts entries for performance
Great work! 🎉
|
PR review posted successfully. Summary: This is a well-implemented feature with excellent security design and comprehensive test coverage (66 new tests, 100% passing). The implementation follows project standards and integrates cleanly with the E2B sandbox architecture. Key findings:
Recommendation: ✅ Approve with minor suggestions The suggestions are nice-to-haves, not blocking issues. Consider adding symlink validation before merge for enhanced security. Review posted at: #45 (comment) |
- Track injected key filename and pass to cleanup for precise removal - cleanupSSHKey now accepts optional keyFilename parameter - Falls back to id_* pattern only when filename not provided - Fix logger redaction test to use complete 40+ char public key pattern - Add test coverage for specific key filename cleanup
|
|
||
| } finally { | ||
| // Best-effort cleanup of SSH key from sandbox (before termination) | ||
| if (sshKeyInjected && sandbox) { |
There was a problem hiding this comment.
--dry-run promises the sandbox stays active for inspection, but the finally block still removes the SSH key. Consider skipping SSH key cleanup when --dry-run so inspection can test private repo access.
| if (sshKeyInjected && sandbox) { | |
| if (!options.dryRun && sshKeyInjected && sandbox) { |
🚀 Want me to fix this? Reply ex: "fix it for me".
|
|
||
| // Set correct permissions (600) | ||
| const chmodResult = await sandbox.commands.run( | ||
| `chmod 600 ~/.ssh/${keyFilename}`, |
There was a problem hiding this comment.
Unsanitized keyFilename is interpolated into shell/config, which can break and enable injection. Suggest validating it (e.g., strict filename) and consistently quoting/escaping before building commands and paths.
🚀 Want me to fix this? Reply ex: "fix it for me".
| // Step 3: Configure known_hosts for common git providers | ||
| for (const provider of GIT_PROVIDERS) { | ||
| const keyscanResult = await sandbox.commands.run( | ||
| `ssh-keyscan -H ${provider} >> ~/.ssh/known_hosts 2>/dev/null || true`, |
There was a problem hiding this comment.
The exitCode check here will never trigger because the command uses || true, forcing success. Consider removing || true so ssh-keyscan failures are detected and the warn is logged.
| `ssh-keyscan -H ${provider} >> ~/.ssh/known_hosts 2>/dev/null || true`, | |
| `ssh-keyscan -H ${provider} >> ~/.ssh/known_hosts 2>/dev/null`, |
🚀 Want me to fix this? Reply ex: "fix it for me".
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/e2b/ssh-key-injector.ts (1)
375-388: Path inconsistency: hardcoded/root/.sshvs~/.sshexpansion.The code writes the key to
/root/.ssh/${keyFilename}(line 380) but references~/.sshin commands (lines 363, 392). This works if the sandbox runs as root, but could cause issues if the sandbox user changes.Consider using a consistent approach - either always use
/root/.sshor resolve~programmatically.♻️ Suggested fix for consistency
- const remoteKeyPath = `~/.ssh/${keyFilename}`; + const sshDir = '/root/.ssh'; + const remoteKeyPath = `${sshDir}/${keyFilename}`; try { // Write key content - await sandbox.files.write(`/root/.ssh/${keyFilename}`, keyContent); + await sandbox.files.write(`${sshDir}/${keyFilename}`, keyContent);tests/logger-redaction.test.ts (1)
171-236: Good Logger integration tests verifying redaction at each level.The tests correctly verify that sensitive data is redacted through Logger methods. The updated test at line 216 uses a complete 40+ character public key pattern to match the redaction regex (per commit message).
Consider adding an
afterEachto restoreprocess.env.PARALLEL_CC_LOG_LEVELto prevent test pollution, though this is minor since each test sets its own value.♻️ Optional: Add env var cleanup
beforeEach(() => { consoleSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); consoleWarnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); }); + afterEach(() => { + delete process.env.PARALLEL_CC_LOG_LEVEL; + });tests/e2b/ssh-key-injector.test.ts (1)
17-31: Local interface definitions are redundant.These interfaces are already exported from
src/e2b/ssh-key-injector.ts. Consider importing them to avoid duplication:♻️ Optional: Import interfaces from module
import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import type { Sandbox } from 'e2b'; +import type { SSHValidationResult, SSHInjectionResult } from '../../src/e2b/ssh-key-injector.js'; -// Types for our module (will be implemented) -interface SSHInjectionResult { - success: boolean; - keyFingerprint?: string; - keyType?: string; - error?: string; -} - -interface SSHValidationResult { - valid: boolean; - keyType?: string; - permissionsOk: boolean; - permissionsWarning?: string; - error?: string; -}
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/cli.tssrc/e2b/ssh-key-injector.tstests/e2b/ssh-key-injector.test.tstests/logger-redaction.test.ts
🧰 Additional context used
📓 Path-based instructions (6)
src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.ts: Use TypeScript strict mode for all TypeScript files
Use async/await syntax instead of callbacks for asynchronous operations
Implement explicit error handling for all operations
Use meaningful and descriptive variable names following camelCase convention
Export type definitions from types.ts for all shared interfaces and types
Implement comprehensive logging using logger.ts for all operations
Use ESLint for code linting with project-specific configuration
Files:
src/e2b/ssh-key-injector.tssrc/cli.ts
src/e2b/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Implement E2B sandbox lifecycle management for autonomous Claude execution
Files:
src/e2b/ssh-key-injector.ts
src/e2b/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Support three-tier git identity priority: CLI flags > environment variables > local git config > default
Files:
src/e2b/ssh-key-injector.ts
src/cli.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use Commander.js for CLI argument parsing and command routing
Files:
src/cli.ts
tests/**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.test.ts: Use Vitest framework for unit and integration testing
Use describe/it blocks in Vitest for organizing test cases with clear assertions
Files:
tests/logger-redaction.test.tstests/e2b/ssh-key-injector.test.ts
tests/e2b/**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Mock E2B SDK completely in tests with real filesystem/database/git operations for E2E tests
Files:
tests/e2b/ssh-key-injector.test.ts
🧠 Learnings (9)
📚 Learning: 2026-01-13T03:37:18.645Z
Learnt from: CR
Repo: frankbria/parallel-cc PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T03:37:18.645Z
Learning: Applies to src/e2b/file-sync.ts : Implement automatic credential scanning and exclusion from E2B sandbox uploads
Applied to files:
src/e2b/ssh-key-injector.tssrc/cli.ts
📚 Learning: 2026-01-13T03:37:18.645Z
Learnt from: CR
Repo: frankbria/parallel-cc PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T03:37:18.645Z
Learning: Applies to tests/e2b/**/*.test.ts : Mock E2B SDK completely in tests with real filesystem/database/git operations for E2E tests
Applied to files:
src/e2b/ssh-key-injector.tssrc/cli.tstests/logger-redaction.test.tstests/e2b/ssh-key-injector.test.ts
📚 Learning: 2026-01-13T03:37:18.645Z
Learnt from: CR
Repo: frankbria/parallel-cc PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T03:37:18.645Z
Learning: Applies to src/e2b/*.ts : Implement E2B sandbox lifecycle management for autonomous Claude execution
Applied to files:
src/e2b/ssh-key-injector.tssrc/cli.ts
📚 Learning: 2026-01-13T03:37:18.645Z
Learnt from: CR
Repo: frankbria/parallel-cc PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T03:37:18.645Z
Learning: Applies to src/e2b/**/*.ts : Support three-tier git identity priority: CLI flags > environment variables > local git config > default
Applied to files:
src/cli.ts
📚 Learning: 2026-01-13T03:37:18.645Z
Learnt from: CR
Repo: frankbria/parallel-cc PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T03:37:18.645Z
Learning: Applies to src/e2b/file-sync.ts : Use compression for file uploads/downloads in E2B sandbox file synchronization
Applied to files:
src/cli.ts
📚 Learning: 2026-01-13T03:37:18.645Z
Learnt from: CR
Repo: frankbria/parallel-cc PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T03:37:18.645Z
Learning: Applies to src/e2b/sandbox-manager.ts : Enforce 1-hour maximum timeout for E2B sandboxes with soft warnings at 30min and 50min marks
Applied to files:
src/cli.ts
📚 Learning: 2026-01-13T03:37:18.645Z
Learnt from: CR
Repo: frankbria/parallel-cc PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T03:37:18.645Z
Learning: Applies to src/**/*.ts : Implement comprehensive logging using logger.ts for all operations
Applied to files:
tests/logger-redaction.test.ts
📚 Learning: 2026-01-13T03:37:18.645Z
Learnt from: CR
Repo: frankbria/parallel-cc PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T03:37:18.645Z
Learning: Applies to tests/**/*.test.ts : Use describe/it blocks in Vitest for organizing test cases with clear assertions
Applied to files:
tests/logger-redaction.test.tstests/e2b/ssh-key-injector.test.ts
📚 Learning: 2026-01-13T03:37:18.645Z
Learnt from: CR
Repo: frankbria/parallel-cc PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T03:37:18.645Z
Learning: Applies to tests/**/*.test.ts : Use Vitest framework for unit and integration testing
Applied to files:
tests/logger-redaction.test.ts
🧬 Code graph analysis (4)
src/e2b/ssh-key-injector.ts (1)
src/logger.ts (3)
error(140-157)logger(189-189)Logger(116-187)
src/cli.ts (2)
src/e2b/ssh-key-injector.ts (4)
validateSSHKeyPath(119-251)getSecurityWarning(305-331)injectSSHKey(348-461)cleanupSSHKey(479-501)src/logger.ts (1)
logger(189-189)
tests/logger-redaction.test.ts (1)
src/logger.ts (4)
redactSensitive(83-99)REDACTION_PATTERNS(26-69)logger(189-189)Logger(116-187)
tests/e2b/ssh-key-injector.test.ts (1)
src/e2b/ssh-key-injector.ts (7)
SSHInjectionResult(50-61)SSHValidationResult(34-45)validateSSHKeyPath(119-251)injectSSHKey(348-461)cleanupSSHKey(479-501)getSecurityWarning(305-331)detectKeyType(260-281)
🪛 Gitleaks (8.30.0)
src/e2b/ssh-key-injector.ts
[high] 71-73: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
tests/logger-redaction.test.ts
[high] 13-21: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
[high] 29-36: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
[high] 110-131: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
[high] 189-203: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
tests/e2b/ssh-key-injector.test.ts
[high] 241-263: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
[high] 284-299: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
[high] 547-556: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
[high] 565-574: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
[high] 584-604: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: opencode-review
- GitHub Check: Macroscope - Correctness Check
🔇 Additional comments (25)
src/e2b/ssh-key-injector.ts (8)
1-25: Well-structured module with comprehensive security documentation.The module header clearly documents the security features and usage workflow. Good use of TypeScript strict typing with explicit interfaces.
31-61: Clean type definitions with good documentation.The interfaces are well-documented with JSDoc comments explaining each field's purpose. The
keyFilenamefield inSSHInjectionResultenables precise cleanup as noted in the commit message.
67-100: Comprehensive pattern definitions for key detection and security.The
DANGEROUS_PATH_CHARSregex provides good protection against shell injection. The SSH key patterns cover all common private key formats.Note: The static analysis hint about "private key" on lines 71-73 is a false positive - these are regex patterns used for key format detection, not actual private keys.
119-251: Thorough validation with proper layered checks.The validation function implements defense-in-depth with multiple security checks:
- Path sanitization (length, dangerous characters)
- File accessibility (existence, readability, file type)
- Permission checks (warns on overly permissive modes)
- Content validation (format, empty file, public key detection)
- Encryption detection with appropriate warnings
The permission check correctly identifies any group/other permissions, not just read access.
260-293: Good fallback strategy for key type detection.The two-tier detection (content-based then filename-based) handles the OpenSSH format well, which uses a generic header for all key types.
305-331: Clear and comprehensive security warning.The warning effectively communicates risks and provides actionable recommendations (deploy keys, rotation, monitoring).
400-426: Robust known_hosts configuration with appropriate error handling.The
|| truepattern ensures keyscan failures don't abort the injection, which is appropriate since the SSH config also setsStrictHostKeyChecking accept-newas a fallback.
479-501: Good cleanup implementation with precise key targeting.The optional
keyFilenameparameter (added per review feedback) enables precise cleanup of the specific injected key. The fallback toid_*pattern provides backward compatibility while the CLI code properly tracks and passes the injected key filename.src/cli.ts (6)
35-35: Clean import of SSH key injection utilities.The import follows the existing pattern and brings in the necessary public API functions.
1395-1396: Well-designed CLI options following Commander.js conventions.The
--ssh-keyand--confirm-ssh-keyoptions provide flexibility for both interactive and non-interactive (CI/CD) usage.
1510-1564: Secure validation flow with proper interactive/non-interactive handling.The validation correctly:
- Expands
~to the home directory- Validates the key before proceeding
- Requires explicit
--confirm-ssh-keyin JSON/non-interactive mode- Shows security warning and prompts in interactive mode
- Gracefully handles user declining to proceed
1672-1683: Correct handling of updated SandboxManager.createSandbox API.The destructuring
{ sandbox, sandboxId } = createResultcorrectly handles the API change mentioned in the AI summary. The tracking variables are properly initialized before the try block for access in the finally cleanup.
1703-1733: Proper SSH key injection with good error handling.The injection is correctly placed after sandbox creation/upload but before execution. On failure, it properly terminates the sandbox and exits. The
injectedKeyFilenametracking enables precise cleanup.
2010-2019: Robust SSH key cleanup in finally block.The cleanup correctly:
- Uses best-effort pattern (catches errors to avoid masking original errors)
- Only attempts cleanup if injection succeeded
- Passes the specific
injectedKeyFilenamefor precise removal- Runs in finally block to ensure execution even on errors
tests/logger-redaction.test.ts (3)
1-9: Good test file structure with clear documentation.The file header clearly explains the test purpose. Imports follow the Vitest framework pattern per coding guidelines.
Note: Static analysis hints about "private keys" in this file are false positives - these are test strings used to validate that the redaction system works correctly.
11-154: Comprehensive test coverage for redactSensitive function.Tests cover all key formats (RSA, OpenSSH, DSA, EC, generic), public key patterns, fingerprints, long base64, API keys, and GitHub tokens. Good inclusion of negative tests (what should NOT be redacted) and edge cases (empty, null/undefined, multiple patterns).
156-169: Good API contract verification.Tests ensure
REDACTION_PATTERNSis properly exported and contains the expected SSH-related patterns.tests/e2b/ssh-key-injector.test.ts (8)
1-16: Comprehensive test file with clear documentation.The header clearly documents test coverage areas. The mocking strategy properly isolates tests from real file system operations per the coding guidelines.
Note: All static analysis hints about "private keys" in this file are false positives - these are test fixtures for validating key detection and injection logic.
33-78: Well-structured mock setup following E2B SDK patterns.The mocks correctly isolate tests from real file system operations. The
mockSandboxstructure matches the E2B Sandbox API used by the injector module.
80-293: Comprehensive validation test coverage.Tests cover all validation scenarios including permissions, key types, format validation, and edge cases. The dynamic import pattern correctly applies mocks before testing.
295-423: Thorough injection workflow testing.Tests verify each step of the injection process: directory creation, key writing, permissions, known_hosts configuration, SSH config, and success/error handling. The test at lines 437-451 specifically verifies the new
keyFilenameparameter for precise cleanup.
425-510: Good cleanup test coverage including precise filename targeting.Tests at lines 437-451 specifically verify the
keyFilenameparameter functionality, ensuring the specific injected key is removed rather than using the fallbackid_*pattern. Error handling tests confirm graceful failure.
512-541: Adequate security warning tests.Tests verify the warning contains essential information about SSH key transmission and security implications.
543-589: Good key type detection test coverage.Tests verify content-based detection for legacy formats and filename-based fallback for OpenSSH format.
591-765: Excellent integration and edge case coverage.The full workflow test (lines 592-636) validates the complete injection lifecycle. Edge case tests cover security-sensitive scenarios including:
- Passphrase-protected keys
- Path length limits
- Shell metacharacter injection (line 721)
- Empty files and public key detection
Code Review: SSH Key Injection for Private Repository AccessOverall Assessment: ✅ Approve with Minor SuggestionsThis is a well-implemented security feature with excellent design and comprehensive test coverage. The implementation follows project conventions and properly addresses feedback from previous review cycles. Key Strengths:
✅ What's Already Well-Implemented
|
| Aspect | Status | Notes |
|---|---|---|
| TypeScript compilation | ✅ Passes | npm run build succeeds |
| Test coverage | ✅ Excellent | 66 new tests, all passing (per PR description) |
| Security design | ✅ Strong | Multi-layer validation, redaction, cleanup |
| Error handling | ✅ Good | Try/catch blocks, meaningful error messages |
| Documentation | ✅ Complete | JSDoc comments, CLAUDE.md updated |
| Follows conventions | ✅ Yes | Matches existing patterns in codebase |
| Addresses PR feedback | ✅ Yes | Fix commit cabbf68 resolves major issues |
🔒 Security Review
| Security Aspect | Rating | Notes |
|---|---|---|
| Key transmission | ✅ Secure | E2B uses TLS encryption |
| Key storage | ✅ Secure | 600 permissions, temporary sandbox |
| Key cleanup | ✅ Secure | Finally block, filename tracking |
| Log redaction | ✅ Excellent | Comprehensive patterns |
| Path validation | ✅ Good | Shell injection protection, symlink check recommended |
| Interactive confirmation | ✅ Required | Warns about security implications |
| CI/CD support | ✅ Safe | Requires --confirm-ssh-key flag |
📝 GitGuardian False Positives
The warnings about "hardcoded secrets" are expected:
tests/e2b/ssh-key-injector.test.ts:671- Test string for encrypted key patterntests/logger-redaction.test.ts:149- Test string for GitHub token pattern
These are intentionally part of test fixtures to validate redaction functionality, not actual secrets. Correctly acknowledged in PR description.
🚀 Performance Considerations
- SSH key validation: ~5-10ms ✅ Good
- Known hosts setup: 90s max (3 providers × 30s)
⚠️ Could reduce to 30s - Key injection: ~1-2s ✅ Good
- Cleanup: ~100ms ✅ Good
The ssh-keyscan timeout is the only potential performance concern. Consider the suggestions in point #3 above.
✅ Final Recommendation
Status: ✅ Approve with minor suggestions
This is production-ready code with excellent security design and test coverage. The suggestions above are:
- Symlink validation (5-line change, high security value)
- Path regex refinement (low priority, minor UX improvement)
- ssh-keyscan optimization (nice-to-have, not blocking)
- Code cleanup (unused variable, redundant conditional) - trivial
None of these are blocking issues. The PR can be merged as-is, with potential follow-up PRs for enhancements.
Before merge: Consider adding symlink validation (suggestion #1) for enhanced security.
Post-merge: Monitor for path validation false positives and consider ssh-keyscan optimizations.
Great work! 🎉
References
- Previous review feedback addressed in commit
cabbf68 - Test file coverage:
tests/e2b/ssh-key-injector.test.ts,tests/logger-redaction.test.ts - Security documentation:
CLAUDE.mdlines 740-795
|
Review posted successfully! SummaryI've reviewed PR #45 and provided comprehensive feedback covering: Key Findings:
Issues Identified:
All issues are non-blocking - the code is production-ready. The review is now available at: #45 (comment) |
Test files contain intentional fake secrets (SSH keys, API keys, tokens) for validating the redaction functionality. These are false positives.
1 participant
Summary
Implements opt-in SSH key injection for E2B sandboxes to enable cloning and pushing to private Git repositories.
Features
--confirm-ssh-keyfor CI/CD)StrictHostKeyChecking accept-newCLI Options
Security Flow
~/.sshwith 600 permissionsTest Plan
npm run build)Files Changed
src/e2b/ssh-key-injector.ts- New SSH key injection modulesrc/cli.ts- Added--ssh-keyand--confirm-ssh-keyoptionssrc/logger.ts- Added sensitive data redactiontests/e2b/ssh-key-injector.test.ts- 42 comprehensive teststests/logger-redaction.test.ts- 24 logger security testsCLAUDE.md- Documentation for SSH key injectionNote
Pre-commit secret detection flagged test strings as false positives. These are test patterns for validating the redaction functionality, not actual secrets.
Summary by CodeRabbit
New Features
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.