diff --git a/.changeset/new-students-accept.md b/.changeset/new-students-accept.md new file mode 100644 index 00000000..6f7214e3 --- /dev/null +++ b/.changeset/new-students-accept.md @@ -0,0 +1,5 @@ +--- +'@cloudflare/sandbox': patch +--- + +Redact credentials from Git URLs in logs diff --git a/.github/workflows/pullrequest.yml b/.github/workflows/pullrequest.yml index 3138cb55..733965f8 100644 --- a/.github/workflows/pullrequest.yml +++ b/.github/workflows/pullrequest.yml @@ -120,8 +120,8 @@ jobs: with: context: . file: packages/sandbox/Dockerfile - platforms: linux/amd64 # Explicit single-arch for compatibility with release-amd64 cache - load: true # Load into Docker daemon for local testing + platforms: linux/amd64 # Explicit single-arch for compatibility with release-amd64 cache + load: true # Load into Docker daemon for local testing tags: cloudflare/sandbox-test:${{ needs.unit-tests.outputs.version || '0.0.0' }} cache-from: | type=gha,scope=pr-${{ github.event.pull_request.number }}-amd64 diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 581118a6..2201f5aa 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -119,7 +119,7 @@ jobs: context: . file: packages/sandbox/Dockerfile platforms: linux/amd64 - push: false # Don't push, just cache + push: false # Don't push, just cache cache-from: type=gha,scope=release-amd64 cache-to: type=gha,mode=max,scope=release-amd64 build-args: | @@ -190,7 +190,7 @@ jobs: context: . file: packages/sandbox/Dockerfile platforms: linux/amd64 - push: false # Don't push, just cache + push: false # Don't push, just cache cache-from: type=gha,scope=release-amd64 cache-to: type=gha,mode=max,scope=release-amd64 build-args: | diff --git a/CLAUDE.md b/CLAUDE.md index c6bc4a85..f96cfcb2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -34,7 +34,7 @@ The Cloudflare Sandbox SDK enables secure, isolated code execution in containers - `CodeInterpreter`: High-level API for running Python/JavaScript with structured outputs - `proxyToSandbox()`: Request handler for preview URL routing -2. **`@repo/shared` (packages/shared/)** - Shared types and error system +2. **`@repo/shared` (packages/shared/)** - Shared utilities - Type definitions shared between SDK and container runtime - Centralized error handling and logging utilities - Not published to npm (internal workspace package) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 7baa6fe4..5bd15c1f 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -15,17 +15,20 @@ Thank you for your interest in contributing to the Cloudflare Sandbox SDK! This 1. Fork the repository to your GitHub account 2. Clone your fork: + ```bash git clone https://github.com/YOUR-USERNAME/sandbox-sdk.git cd sandbox-sdk ``` 3. Install dependencies: + ```bash npm install ``` 4. Build the packages: + ```bash npm run build ``` @@ -40,6 +43,7 @@ Thank you for your interest in contributing to the Cloudflare Sandbox SDK! This ### Making Changes 1. Create a new branch for your changes: + ```bash git checkout -b feat/your-feature-name # or @@ -49,6 +53,7 @@ Thank you for your interest in contributing to the Cloudflare Sandbox SDK! This 2. Make your changes following our coding standards (see CLAUDE.md) 3. Run code quality checks: + ```bash npm run check # Linting + type checking npm run fix # Auto-fix linting issues @@ -73,6 +78,7 @@ Follow the [7 rules for great commit messages](https://cbea.ms/git-commit/): 7. Use the body to explain what and why vs. how Example: + ``` Add session isolation for concurrent executions @@ -90,11 +96,13 @@ npx changeset ``` This will interactively guide you through: + 1. Selecting which packages to include 2. Choosing the semantic version bump (`patch`, `minor`, or `major`) 3. Writing a description of your changes Use semantic versioning: + - `patch`: Bug fixes, minor improvements - `minor`: New features, non-breaking changes - `major`: Breaking changes @@ -104,6 +112,7 @@ The changeset bot will comment on your PR if a changeset is needed. ## Submitting a Pull Request 1. Push your branch to your fork: + ```bash git push origin feat/your-feature-name ``` @@ -119,6 +128,7 @@ The changeset bot will comment on your PR if a changeset is needed. ### Review Process A maintainer will review your PR and may: + - Request changes - Ask questions - Suggest improvements @@ -135,12 +145,14 @@ We use Biome for linting and formatting. Key guidelines: - Write concise, readable code - Add comments for complex logic - Follow patterns in existing code +- Use the provided logger (`this.logger.info()`) instead of `console.log()` in production code ## Testing ### Unit Tests Located in `packages/*/tests/`: + - Test individual components in isolation - Mock external dependencies - Fast feedback loop @@ -150,6 +162,7 @@ Run with: `npm test` ### E2E Tests Located in `tests/e2e/`: + - Test full workflows against real Workers and containers - Require Docker - Slower but comprehensive diff --git a/package-lock.json b/package-lock.json index 0f03c891..34be7e2e 100644 --- a/package-lock.json +++ b/package-lock.json @@ -180,7 +180,6 @@ "integrity": "sha512-e7jT4DxYvIDLk1ZHmU/m/mB19rex9sv0c2ftBtjSBv+kVM/902eh0fINUzD7UwLLNR+jU585GxUJ8/EBfAM5fw==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "@babel/code-frame": "^7.27.1", "@babel/generator": "^7.28.5", @@ -1129,8 +1128,7 @@ "resolved": "https://registry.npmjs.org/@cloudflare/workers-types/-/workers-types-4.20251014.0.tgz", "integrity": "sha512-tEW98J/kOa0TdylIUOrLKRdwkUw0rvvYVlo+Ce0mqRH3c8kSoxLzUH9gfCvwLe0M89z1RkzFovSKAW2Nwtyn3w==", "dev": true, - "license": "MIT OR Apache-2.0", - "peer": true + "license": "MIT OR Apache-2.0" }, "node_modules/@cspotcode/source-map-support": { "version": "0.8.1", @@ -2282,7 +2280,6 @@ "integrity": "sha512-/g2d4sW9nUDJOMz3mabVQvOGhVa4e/BN/Um7yca9Bb2XTzPPnfTWHWQg+IsEYO7M3Vx+EXvaM/I2pJWIMun1bg==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "@octokit/auth-token": "^4.0.0", "@octokit/graphql": "^7.1.0", @@ -3315,7 +3312,6 @@ "resolved": "https://registry.npmjs.org/@types/react/-/react-19.2.2.tgz", "integrity": "sha512-6mDvHUFSjyT2B2yeNx2nUgMxh9LtOWvkhIU3uePn2I2oyNymUAX1NIsdgviM4CH+JSrp2D2hsMvJOkxY+0wNRA==", "license": "MIT", - "peer": true, "dependencies": { "csstype": "^3.0.2" } @@ -3453,7 +3449,6 @@ "integrity": "sha512-oukfKT9Mk41LreEW09vt45f8wx7DordoWUZMYdY/cyAk7w5TWkTRCNZYF7sX7n2wB7jyGAl74OxgwhPgKaqDMQ==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "@vitest/utils": "3.2.4", "pathe": "^2.0.3", @@ -3469,7 +3464,6 @@ "integrity": "sha512-dEYtS7qQP2CjU27QBC5oUOxLE/v5eLkGqPE0ZKEIDGMs4vKWe7IjgLOeauHsR0D5YuuycGRO5oSRXnwnmA78fQ==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "@vitest/pretty-format": "3.2.4", "magic-string": "^0.30.17", @@ -3498,7 +3492,6 @@ "integrity": "sha512-hGISOaP18plkzbWEcP/QvtRW1xDXF2+96HbEX6byqQhAUbiS5oH6/9JwW+QsQCIYON2bI6QZBF+2PvOmrRZ9wA==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "@vitest/utils": "3.2.4", "fflate": "^0.8.2", @@ -3720,7 +3713,6 @@ } ], "license": "MIT", - "peer": true, "dependencies": { "baseline-browser-mapping": "^2.8.19", "caniuse-lite": "^1.0.30001751", @@ -4973,6 +4965,7 @@ "resolved": "https://registry.npmjs.org/loose-envify/-/loose-envify-1.4.0.tgz", "integrity": "sha512-lyuxPGr/Wfhrlem2CL/UcnUc1zcqKAImBDzukY7Y5F/yQiNdko6+fRLevlw1HgMySw7f611UIY408EtxRSoK3Q==", "license": "MIT", + "peer": true, "dependencies": { "js-tokens": "^3.0.0 || ^4.0.0" }, @@ -6050,6 +6043,7 @@ "resolved": "https://registry.npmjs.org/object-assign/-/object-assign-4.1.1.tgz", "integrity": "sha512-rJgTQnkUnH1sFw8yT6VSU3zD3sWmu6sZhIseY8VX+GRu3P6F7Fu+JNDoXfklElbLJSnc3FUQHVe4cU5hj+BcUg==", "license": "MIT", + "peer": true, "engines": { "node": ">=0.10.0" } @@ -6471,7 +6465,6 @@ "resolved": "https://registry.npmjs.org/react/-/react-19.2.0.tgz", "integrity": "sha512-tmbWg6W31tQLeB5cdIBOicJDJRR2KzXsV7uSK9iNfLWQ5bIZfxuPEHp7M8wiHyHnn0DD1i7w3Zmin0FtkrwoCQ==", "license": "MIT", - "peer": true, "engines": { "node": ">=0.10.0" } @@ -6493,7 +6486,8 @@ "version": "16.13.1", "resolved": "https://registry.npmjs.org/react-is/-/react-is-16.13.1.tgz", "integrity": "sha512-24e6ynE2H+OKt4kqsOvNd8kBpV65zoxbA4BVsEOB3ARVWQki/DHzaUoC5KuON/BiccDaCCTZBuOcfZs70kR8bQ==", - "license": "MIT" + "license": "MIT", + "peer": true }, "node_modules/react-katex": { "version": "3.1.0", @@ -6678,7 +6672,6 @@ "integrity": "sha512-iMmuD72XXLf26Tqrv1cryNYLX6NNPLhZ3AmNkSf8+xda0H+yijjGJ+wVT9UdBUHOpKzq9RjKtQKRCWoEKQQBZQ==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "@oxc-project/types": "=0.95.0", "@rolldown/pluginutils": "1.0.0-beta.45" @@ -7339,7 +7332,6 @@ "integrity": "sha512-5gTmgEY/sqK6gFXLIsQNH19lWb4ebPDLA4SdLP7dsWkIXHWlG66oPuVvXSGFPppYZz8ZDZq0dYYrbHfBCVUb1Q==", "dev": true, "license": "MIT", - "peer": true, "engines": { "node": ">=12" }, @@ -7508,7 +7500,6 @@ "integrity": "sha512-ytQKuwgmrrkDTFP4LjR0ToE2nqgy886GpvRSpU0JAnrdBYppuY5rLkRUYPU1yCryb24SsKBTL/hlDQAEFVwtZg==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "esbuild": "~0.25.0", "get-tsconfig": "^4.7.5" @@ -7705,7 +7696,6 @@ "integrity": "sha512-Wj7/AMtE9MRnAXa6Su3Lk0LNCfqDYgfwVjwRFVum9U7wsto1imuHqk4kTm7Jni+5A0Hn7dttL6O/zjvUvoo+8A==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "defu": "^6.1.4", "exsolve": "^1.0.7", @@ -7924,7 +7914,6 @@ "integrity": "sha512-ZWyE8YXEXqJrrSLvYgrRP7p62OziLW7xI5HYGWFzOvupfAlrLvURSzv/FyGyy0eidogEM3ujU+kUG1zuHgb6Ug==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "esbuild": "^0.25.0", "fdir": "^6.5.0", @@ -8041,7 +8030,6 @@ "integrity": "sha512-5gTmgEY/sqK6gFXLIsQNH19lWb4ebPDLA4SdLP7dsWkIXHWlG66oPuVvXSGFPppYZz8ZDZq0dYYrbHfBCVUb1Q==", "dev": true, "license": "MIT", - "peer": true, "engines": { "node": ">=12" }, @@ -8055,7 +8043,6 @@ "integrity": "sha512-LUCP5ev3GURDysTWiP47wRRUpLKMOfPh+yKTx3kVIEiu5KOMeqzpnYNsKyOoVrULivR8tLcks4+lga33Whn90A==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "@types/chai": "^5.2.2", "@vitest/expect": "3.2.4", @@ -8211,7 +8198,6 @@ "dev": true, "hasInstallScript": true, "license": "Apache-2.0", - "peer": true, "bin": { "workerd": "bin/workerd" }, @@ -8740,7 +8726,6 @@ "integrity": "sha512-PEIGCY5tSlUt50cqyMXfCzX+oOPqN0vuGqWzbcJ2xvnkzkq46oOpz7dQaTDBdfICb4N14+GARUDw2XV2N4tvzg==", "devOptional": true, "license": "MIT", - "peer": true, "engines": { "node": ">=10.0.0" }, diff --git a/packages/sandbox-container/src/core/container.ts b/packages/sandbox-container/src/core/container.ts index 2b99698d..a5e71db2 100644 --- a/packages/sandbox-container/src/core/container.ts +++ b/packages/sandbox-container/src/core/container.ts @@ -1,5 +1,5 @@ import type { Logger } from '@repo/shared'; -import { createLogger } from '@repo/shared'; +import { createLogger, GitLogger } from '@repo/shared'; import { ExecuteHandler } from '../handlers/execute-handler'; import { FileHandler } from '../handlers/file-handler'; import { GitHandler } from '../handlers/git-handler'; @@ -96,6 +96,9 @@ export class Container { // Initialize SessionManager const sessionManager = new SessionManager(logger); + // Create git-specific logger that automatically sanitizes credentials + const gitLogger = new GitLogger(logger); + // Initialize services const processService = new ProcessService( processStore, @@ -108,7 +111,11 @@ export class Container { sessionManager ); const portService = new PortService(portStore, securityAdapter, logger); - const gitService = new GitService(securityAdapter, logger, sessionManager); + const gitService = new GitService( + securityAdapter, + gitLogger, + sessionManager + ); const interpreterService = new InterpreterService(logger); // Initialize handlers @@ -117,7 +124,7 @@ export class Container { const fileHandler = new FileHandler(fileService, logger); const processHandler = new ProcessHandler(processService, logger); const portHandler = new PortHandler(portService, logger); - const gitHandler = new GitHandler(gitService, logger); + const gitHandler = new GitHandler(gitService, gitLogger); const interpreterHandler = new InterpreterHandler( interpreterService, logger diff --git a/packages/sandbox-container/src/services/git-service.ts b/packages/sandbox-container/src/services/git-service.ts index a2810e5c..b003c464 100644 --- a/packages/sandbox-container/src/services/git-service.ts +++ b/packages/sandbox-container/src/services/git-service.ts @@ -1,12 +1,13 @@ // Git Operations Service import type { Logger } from '@repo/shared'; +import { sanitizeGitData } from '@repo/shared'; import type { GitErrorContext, ValidationFailedContext } from '@repo/shared/errors'; import { ErrorCode } from '@repo/shared/errors'; -import type { CloneOptions, ServiceResult } from '../core/types'; +import type { CloneOptions, ServiceError, ServiceResult } from '../core/types'; import { GitManager } from '../managers/git-manager'; import type { SessionManager } from './session-manager'; @@ -41,6 +42,26 @@ export class GitService { .join(' '); } + /** + * Create error result with sanitized data + */ + private returnError(error: ServiceError): ServiceResult { + return { + success: false, + error: sanitizeGitData(error) + } as ServiceResult; + } + + /** + * Create success result + */ + private returnSuccess(data: T): ServiceResult { + return { + success: true, + data + } as ServiceResult; + } + async cloneRepository( repoUrl: string, options: CloneOptions = {} @@ -49,22 +70,17 @@ export class GitService { // Validate repository URL const urlValidation = this.security.validateGitUrl(repoUrl); if (!urlValidation.isValid) { - return { - success: false, - error: { - message: `Invalid Git URL '${repoUrl}': ${urlValidation.errors.join( - ', ' - )}`, - code: ErrorCode.INVALID_GIT_URL, - details: { - validationErrors: urlValidation.errors.map((e) => ({ - field: 'repoUrl', - message: e, - code: 'INVALID_GIT_URL' - })) - } satisfies ValidationFailedContext - } - }; + return this.returnError({ + message: `Invalid Git URL '${repoUrl}': ${urlValidation.errors.join(', ')}`, + code: ErrorCode.INVALID_GIT_URL, + details: { + validationErrors: urlValidation.errors.map((e) => ({ + field: 'repoUrl', + message: e, + code: 'INVALID_GIT_URL' + })) + } satisfies ValidationFailedContext + }); } // Generate target directory if not provided @@ -74,22 +90,17 @@ export class GitService { // Validate target directory path const pathValidation = this.security.validatePath(targetDirectory); if (!pathValidation.isValid) { - return { - success: false, - error: { - message: `Invalid target directory '${targetDirectory}': ${pathValidation.errors.join( - ', ' - )}`, - code: ErrorCode.VALIDATION_FAILED, - details: { - validationErrors: pathValidation.errors.map((e) => ({ - field: 'targetDirectory', - message: e, - code: 'INVALID_PATH' - })) - } satisfies ValidationFailedContext - } - }; + return this.returnError({ + message: `Invalid target directory '${targetDirectory}': ${pathValidation.errors.join(', ')}`, + code: ErrorCode.VALIDATION_FAILED, + details: { + validationErrors: pathValidation.errors.map((e) => ({ + field: 'targetDirectory', + message: e, + code: 'INVALID_PATH' + })) + } satisfies ValidationFailedContext + }); } // Build git clone command (via manager) @@ -126,21 +137,18 @@ export class GitService { result.stderr || 'Unknown error', result.exitCode ); - return { - success: false, - error: { - message: `Failed to clone repository '${repoUrl}': ${ - result.stderr || `exit code ${result.exitCode}` - }`, - code: errorCode, - details: { - repository: repoUrl, - targetDir: targetDirectory, - exitCode: result.exitCode, - stderr: result.stderr - } satisfies GitErrorContext - } - }; + return this.returnError({ + message: `Failed to clone repository '${repoUrl}': ${ + result.stderr || `exit code ${result.exitCode}` + }`, + code: errorCode, + details: { + repository: repoUrl, + targetDir: targetDirectory, + exitCode: result.exitCode, + stderr: result.stderr + } satisfies GitErrorContext + }); } // Determine the actual branch that was checked out by querying Git @@ -177,34 +185,29 @@ export class GitService { actualBranch = options.branch || 'unknown'; } - return { - success: true, - data: { - path: targetDirectory, - branch: actualBranch - } - }; + return this.returnSuccess({ + path: targetDirectory, + branch: actualBranch + }); } catch (error) { const errorMessage = error instanceof Error ? error.message : 'Unknown error'; + this.logger.error( 'Failed to clone repository', error instanceof Error ? error : undefined, { repoUrl, options } ); - return { - success: false, - error: { - message: `Failed to clone repository '${repoUrl}': ${errorMessage}`, - code: ErrorCode.GIT_CLONE_FAILED, - details: { - repository: repoUrl, - targetDir: options.targetDir, - stderr: errorMessage - } satisfies GitErrorContext - } - }; + return this.returnError({ + message: `Failed to clone repository '${repoUrl}': ${errorMessage}`, + code: ErrorCode.GIT_CLONE_FAILED, + details: { + repository: repoUrl, + targetDir: options.targetDir, + stderr: errorMessage + } satisfies GitErrorContext + }); } } @@ -217,46 +220,37 @@ export class GitService { // Validate repository path const pathValidation = this.security.validatePath(repoPath); if (!pathValidation.isValid) { - return { - success: false, - error: { - message: `Invalid repository path '${repoPath}': ${pathValidation.errors.join( - ', ' - )}`, - code: ErrorCode.VALIDATION_FAILED, - details: { - validationErrors: pathValidation.errors.map((e) => ({ - field: 'repoPath', - message: e, - code: 'INVALID_PATH' - })) - } satisfies ValidationFailedContext - } - }; + return this.returnError({ + message: `Invalid repository path '${repoPath}': ${pathValidation.errors.join(', ')}`, + code: ErrorCode.VALIDATION_FAILED, + details: { + validationErrors: pathValidation.errors.map((e) => ({ + field: 'repoPath', + message: e, + code: 'INVALID_PATH' + })) + } satisfies ValidationFailedContext + }); } // Validate branch name (via manager) const branchValidation = this.manager.validateBranchName(branch); if (!branchValidation.isValid) { - return { - success: false, - error: { - message: `Invalid branch name '${branch}': ${ - branchValidation.error || 'Invalid format' - }`, - code: ErrorCode.VALIDATION_FAILED, - details: { - validationErrors: [ - { - field: 'branch', - message: - branchValidation.error || 'Invalid branch name format', - code: 'INVALID_BRANCH' - } - ] - } satisfies ValidationFailedContext - } - }; + return this.returnError({ + message: `Invalid branch name '${branch}': ${ + branchValidation.error || 'Invalid format' + }`, + code: ErrorCode.VALIDATION_FAILED, + details: { + validationErrors: [ + { + field: 'branch', + message: branchValidation.error || 'Invalid branch name format', + code: 'INVALID_BRANCH' + } + ] + } satisfies ValidationFailedContext + }); } // Build git checkout command (via manager) @@ -289,21 +283,18 @@ export class GitService { result.stderr || 'Unknown error', result.exitCode ); - return { - success: false, - error: { - message: `Failed to checkout branch '${branch}' in '${repoPath}': ${ - result.stderr || `exit code ${result.exitCode}` - }`, - code: errorCode, - details: { - branch, - targetDir: repoPath, - exitCode: result.exitCode, - stderr: result.stderr - } satisfies GitErrorContext - } - }; + return this.returnError({ + message: `Failed to checkout branch '${branch}' in '${repoPath}': ${ + result.stderr || `exit code ${result.exitCode}` + }`, + code: errorCode, + details: { + branch, + targetDir: repoPath, + exitCode: result.exitCode, + stderr: result.stderr + } satisfies GitErrorContext + }); } return { @@ -312,24 +303,22 @@ export class GitService { } catch (error) { const errorMessage = error instanceof Error ? error.message : 'Unknown error'; + this.logger.error( 'Failed to checkout branch', error instanceof Error ? error : undefined, { repoPath, branch } ); - return { - success: false, - error: { - message: `Failed to checkout branch '${branch}' in '${repoPath}': ${errorMessage}`, - code: ErrorCode.GIT_CHECKOUT_FAILED, - details: { - branch, - targetDir: repoPath, - stderr: errorMessage - } satisfies GitErrorContext - } - }; + return this.returnError({ + message: `Failed to checkout branch '${branch}' in '${repoPath}': ${errorMessage}`, + code: ErrorCode.GIT_CHECKOUT_FAILED, + details: { + branch, + targetDir: repoPath, + stderr: errorMessage + } satisfies GitErrorContext + }); } } @@ -341,22 +330,17 @@ export class GitService { // Validate repository path const pathValidation = this.security.validatePath(repoPath); if (!pathValidation.isValid) { - return { - success: false, - error: { - message: `Invalid repository path '${repoPath}': ${pathValidation.errors.join( - ', ' - )}`, - code: ErrorCode.VALIDATION_FAILED, - details: { - validationErrors: pathValidation.errors.map((e) => ({ - field: 'repoPath', - message: e, - code: 'INVALID_PATH' - })) - } satisfies ValidationFailedContext - } - }; + return this.returnError({ + message: `Invalid repository path '${repoPath}': ${pathValidation.errors.join(', ')}`, + code: ErrorCode.VALIDATION_FAILED, + details: { + validationErrors: pathValidation.errors.map((e) => ({ + field: 'repoPath', + message: e, + code: 'INVALID_PATH' + })) + } satisfies ValidationFailedContext + }); } // Build git branch --show-current command (via manager) @@ -382,48 +366,40 @@ export class GitService { result.stderr || 'Unknown error', result.exitCode ); - return { - success: false, - error: { - message: `Failed to get current branch in '${repoPath}': ${ - result.stderr || `exit code ${result.exitCode}` - }`, - code: errorCode, - details: { - targetDir: repoPath, - exitCode: result.exitCode, - stderr: result.stderr - } satisfies GitErrorContext - } - }; + return this.returnError({ + message: `Failed to get current branch in '${repoPath}': ${ + result.stderr || `exit code ${result.exitCode}` + }`, + code: errorCode, + details: { + targetDir: repoPath, + exitCode: result.exitCode, + stderr: result.stderr + } satisfies GitErrorContext + }); } const currentBranch = result.stdout.trim(); - return { - success: true, - data: currentBranch - }; + return this.returnSuccess(currentBranch); } catch (error) { const errorMessage = error instanceof Error ? error.message : 'Unknown error'; + this.logger.error( 'Failed to get current branch', error instanceof Error ? error : undefined, { repoPath } ); - return { - success: false, - error: { - message: `Failed to get current branch in '${repoPath}': ${errorMessage}`, - code: ErrorCode.GIT_OPERATION_FAILED, - details: { - targetDir: repoPath, - stderr: errorMessage - } satisfies GitErrorContext - } - }; + return this.returnError({ + message: `Failed to get current branch in '${repoPath}': ${errorMessage}`, + code: ErrorCode.GIT_OPERATION_FAILED, + details: { + targetDir: repoPath, + stderr: errorMessage + } satisfies GitErrorContext + }); } } @@ -435,22 +411,17 @@ export class GitService { // Validate repository path const pathValidation = this.security.validatePath(repoPath); if (!pathValidation.isValid) { - return { - success: false, - error: { - message: `Invalid repository path '${repoPath}': ${pathValidation.errors.join( - ', ' - )}`, - code: ErrorCode.VALIDATION_FAILED, - details: { - validationErrors: pathValidation.errors.map((e) => ({ - field: 'repoPath', - message: e, - code: 'INVALID_PATH' - })) - } satisfies ValidationFailedContext - } - }; + return this.returnError({ + message: `Invalid repository path '${repoPath}': ${pathValidation.errors.join(', ')}`, + code: ErrorCode.VALIDATION_FAILED, + details: { + validationErrors: pathValidation.errors.map((e) => ({ + field: 'repoPath', + message: e, + code: 'INVALID_PATH' + })) + } satisfies ValidationFailedContext + }); } // Build git branch -a command (via manager) @@ -476,49 +447,41 @@ export class GitService { result.stderr || 'Unknown error', result.exitCode ); - return { - success: false, - error: { - message: `Failed to list branches in '${repoPath}': ${ - result.stderr || `exit code ${result.exitCode}` - }`, - code: errorCode, - details: { - targetDir: repoPath, - exitCode: result.exitCode, - stderr: result.stderr - } satisfies GitErrorContext - } - }; + return this.returnError({ + message: `Failed to list branches in '${repoPath}': ${ + result.stderr || `exit code ${result.exitCode}` + }`, + code: errorCode, + details: { + targetDir: repoPath, + exitCode: result.exitCode, + stderr: result.stderr + } satisfies GitErrorContext + }); } // Parse branch output (via manager) const branches = this.manager.parseBranchList(result.stdout); - return { - success: true, - data: branches - }; + return this.returnSuccess(branches); } catch (error) { const errorMessage = error instanceof Error ? error.message : 'Unknown error'; + this.logger.error( 'Failed to list branches', error instanceof Error ? error : undefined, { repoPath } ); - return { - success: false, - error: { - message: `Failed to list branches in '${repoPath}': ${errorMessage}`, - code: ErrorCode.GIT_OPERATION_FAILED, - details: { - targetDir: repoPath, - stderr: errorMessage - } satisfies GitErrorContext - } - }; + return this.returnError({ + message: `Failed to list branches in '${repoPath}': ${errorMessage}`, + code: ErrorCode.GIT_OPERATION_FAILED, + details: { + targetDir: repoPath, + stderr: errorMessage + } satisfies GitErrorContext + }); } } } diff --git a/packages/sandbox/src/clients/git-client.ts b/packages/sandbox/src/clients/git-client.ts index 7504dcbe..06538094 100644 --- a/packages/sandbox/src/clients/git-client.ts +++ b/packages/sandbox/src/clients/git-client.ts @@ -1,4 +1,5 @@ import type { GitCheckoutResult } from '@repo/shared'; +import { GitLogger } from '@repo/shared'; import { BaseHttpClient } from './base-client'; import type { HttpClientOptions, SessionRequest } from './types'; @@ -18,6 +19,12 @@ export interface GitCheckoutRequest extends SessionRequest { * Client for Git repository operations */ export class GitClient extends BaseHttpClient { + constructor(options: HttpClientOptions = {}) { + super(options); + // Wrap logger with GitLogger to auto-redact credentials + this.logger = new GitLogger(this.logger); + } + /** * Clone a Git repository * @param repoUrl - URL of the Git repository to clone diff --git a/packages/sandbox/src/sandbox.ts b/packages/sandbox/src/sandbox.ts index 85e55543..5c5519b7 100644 --- a/packages/sandbox/src/sandbox.ts +++ b/packages/sandbox/src/sandbox.ts @@ -54,9 +54,9 @@ export function getSandbox( }); } -export function connect( - stub: { fetch: (request: Request) => Promise } -) { +export function connect(stub: { + fetch: (request: Request) => Promise; +}) { return async (request: Request, port: number) => { // Validate port before routing if (!validatePort(port)) { diff --git a/packages/sandbox/tests/git-client.test.ts b/packages/sandbox/tests/git-client.test.ts index 0da27f7f..28d67837 100644 --- a/packages/sandbox/tests/git-client.test.ts +++ b/packages/sandbox/tests/git-client.test.ts @@ -412,4 +412,76 @@ describe('GitClient', () => { expect(fullOptionsClient).toBeInstanceOf(GitClient); }); }); + + describe('credential redaction in logs', () => { + it('should redact credentials from URLs but leave public URLs unchanged', async () => { + const mockLogger = { + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + debug: vi.fn(), + child: vi.fn() + }; + + const clientWithLogger = new GitClient({ + baseUrl: 'http://test.com', + port: 3000, + logger: mockLogger + }); + + // Test with credentials + mockFetch.mockResolvedValueOnce( + new Response( + JSON.stringify({ + success: true, + stdout: "Cloning into 'private-repo'...\nDone.", + stderr: '', + exitCode: 0, + repoUrl: + 'https://oauth2:ghp_token123@github.com/user/private-repo.git', + branch: 'main', + targetDir: '/workspace/private-repo', + timestamp: '2023-01-01T00:00:00Z' + }), + { status: 200 } + ) + ); + + await clientWithLogger.checkout( + 'https://oauth2:ghp_token123@github.com/user/private-repo.git', + 'test-session' + ); + + let logDetails = mockLogger.info.mock.calls[0]?.[1]?.details; + expect(logDetails).not.toContain('ghp_token123'); + expect(logDetails).toContain( + 'https://******@github.com/user/private-repo.git' + ); + + // Test without credentials + mockFetch.mockResolvedValueOnce( + new Response( + JSON.stringify({ + success: true, + stdout: "Cloning into 'react'...\nDone.", + stderr: '', + exitCode: 0, + repoUrl: 'https://github.com/facebook/react.git', + branch: 'main', + targetDir: '/workspace/react', + timestamp: '2023-01-01T00:00:00Z' + }), + { status: 200 } + ) + ); + + await clientWithLogger.checkout( + 'https://github.com/facebook/react.git', + 'test-session' + ); + + logDetails = mockLogger.info.mock.calls[1]?.[1]?.details; + expect(logDetails).toContain('https://github.com/facebook/react.git'); + }); + }); }); diff --git a/packages/shared/package.json b/packages/shared/package.json index 7d4101f2..badf4556 100644 --- a/packages/shared/package.json +++ b/packages/shared/package.json @@ -2,7 +2,7 @@ "name": "@repo/shared", "version": "0.0.0", "private": true, - "description": "Shared types and error system for Cloudflare Sandbox SDK", + "description": "Shared utilities for Cloudflare Sandbox SDK", "type": "module", "exports": { ".": { diff --git a/packages/shared/src/git.ts b/packages/shared/src/git.ts new file mode 100644 index 00000000..e6164064 --- /dev/null +++ b/packages/shared/src/git.ts @@ -0,0 +1,143 @@ +import type { LogContext, Logger } from './logger'; + +/** + * Redact credentials from URLs for secure logging + * + * Replaces any credentials (username:password, tokens, etc.) embedded + * in URLs with ****** to prevent sensitive data exposure in logs. + * Works with URLs embedded in text (e.g., "Error: https://token@github.com/repo.git failed") + * + * @param text - String that may contain URLs with credentials + * @returns String with credentials redacted from any URLs + */ +export function redactCredentials(text: string): string { + // Scan for http(s):// URLs and redact any credentials found + let result = text; + let pos = 0; + + while (pos < result.length) { + const httpPos = result.indexOf('http://', pos); + const httpsPos = result.indexOf('https://', pos); + + let protocolPos = -1; + let protocolLen = 0; + + if (httpPos === -1 && httpsPos === -1) break; + if (httpPos !== -1 && (httpsPos === -1 || httpPos < httpsPos)) { + protocolPos = httpPos; + protocolLen = 7; // 'http://'.length + } else { + protocolPos = httpsPos; + protocolLen = 8; // 'https://'.length + } + + // Look for @ after the protocol + const searchStart = protocolPos + protocolLen; + const atPos = result.indexOf('@', searchStart); + + // Find where the URL ends (whitespace, quotes, or structural delimiters) + let urlEnd = searchStart; + while (urlEnd < result.length) { + const char = result[urlEnd]; + if (/[\s"'`<>,;{}[\]]/.test(char)) break; + urlEnd++; + } + + if (atPos !== -1 && atPos < urlEnd) { + result = `${result.substring(0, searchStart)}******${result.substring(atPos)}`; + pos = searchStart + 6; // Move past '******' + } else { + pos = protocolPos + protocolLen; + } + } + + return result; +} + +/** + * Sanitize data by redacting credentials from any strings + * Recursively processes objects and arrays to ensure credentials are never leaked + */ +export function sanitizeGitData(data: T): T { + // Handle primitives + if (typeof data === 'string') { + return redactCredentials(data) as T; + } + + if (data === null || data === undefined) { + return data; + } + + // Handle arrays + if (Array.isArray(data)) { + return data.map((item) => sanitizeGitData(item)) as T; + } + + // Handle objects - recursively sanitize all fields + if (typeof data === 'object') { + const result: Record = {}; + for (const [key, value] of Object.entries(data)) { + result[key] = sanitizeGitData(value); + } + return result as T; + } + + return data; +} + +/** + * Logger wrapper that automatically sanitizes git credentials + */ +export class GitLogger implements Logger { + constructor(private readonly baseLogger: Logger) {} + + private sanitizeContext( + context?: Partial + ): Partial | undefined { + return context + ? (sanitizeGitData(context) as Partial) + : context; + } + + private sanitizeError(error?: Error): Error | undefined { + if (!error) return error; + + // Create a new error with sanitized message and stack + const sanitized = new Error(redactCredentials(error.message)); + sanitized.name = error.name; + if (error.stack) { + sanitized.stack = redactCredentials(error.stack); + } + // Preserve other enumerable properties + const sanitizedRecord = sanitized as unknown as Record; + const errorRecord = error as unknown as Record; + for (const key of Object.keys(error)) { + if (key !== 'message' && key !== 'stack' && key !== 'name') { + sanitizedRecord[key] = sanitizeGitData(errorRecord[key]); + } + } + return sanitized; + } + + debug(message: string, context?: Partial): void { + this.baseLogger.debug(message, this.sanitizeContext(context)); + } + + info(message: string, context?: Partial): void { + this.baseLogger.info(message, this.sanitizeContext(context)); + } + + warn(message: string, context?: Partial): void { + this.baseLogger.warn(message, this.sanitizeContext(context)); + } + + error(message: string, error?: Error, context?: Partial): void { + this.baseLogger.error(message, this.sanitizeError(error), this.sanitizeContext(context)); + } + + child(context: Partial): Logger { + const sanitized = sanitizeGitData(context) as Partial; + const childLogger = this.baseLogger.child(sanitized); + return new GitLogger(childLogger); + } +} diff --git a/packages/shared/src/index.ts b/packages/shared/src/index.ts index 718e743d..0f90b15d 100644 --- a/packages/shared/src/index.ts +++ b/packages/shared/src/index.ts @@ -3,6 +3,8 @@ * Used by both client SDK and container runtime */ +// Export git utilities +export { GitLogger, redactCredentials, sanitizeGitData } from './git.js'; // Export all interpreter types export type { ChartData, diff --git a/packages/shared/src/logger/index.ts b/packages/shared/src/logger/index.ts index 03b9acf7..26dd0729 100644 --- a/packages/shared/src/logger/index.ts +++ b/packages/shared/src/logger/index.ts @@ -1,5 +1,5 @@ /** - * Logger module for Cloudflare Sandbox SDK + * Logger module * * Provides structured, trace-aware logging with: * - AsyncLocalStorage for implicit context propagation diff --git a/packages/shared/tests/git.test.ts b/packages/shared/tests/git.test.ts new file mode 100644 index 00000000..2e44a78a --- /dev/null +++ b/packages/shared/tests/git.test.ts @@ -0,0 +1,78 @@ +import { describe, expect, it, vi } from 'vitest'; +import { redactCredentials, sanitizeGitData, GitLogger } from '../src/git'; +import { createNoOpLogger } from '../src/logger'; + +describe('redactCredentials', () => { + it('should redact credentials from URLs embedded in text', () => { + expect(redactCredentials('fatal: https://oauth2:token@github.com/repo.git')).toBe( + 'fatal: https://******@github.com/repo.git' + ); + expect(redactCredentials('https://user:pass@example.com/path')).toBe( + 'https://******@example.com/path' + ); + expect(redactCredentials('https://github.com/public.git')).toBe( + 'https://github.com/public.git' + ); + }); + + it('should handle multiple URLs in a single string', () => { + expect( + redactCredentials('Error: https://token1@host1.com failed, tried https://token2@host2.com') + ).toBe('Error: https://******@host1.com failed, tried https://******@host2.com'); + }); + + it('should handle URLs in structured formats', () => { + expect(redactCredentials('{"url":"https://token@github.com/repo.git"}')).toBe( + '{"url":"https://******@github.com/repo.git"}' + ); + expect(redactCredentials('https://token@github.com/repo.git')).toBe( + 'https://******@github.com/repo.git' + ); + }); +}); + +describe('sanitizeGitData', () => { + it('should recursively sanitize credentials in any field', () => { + const data = { + repoUrl: 'https://token@github.com/repo.git', + stderr: 'fatal: https://user:pass@gitlab.com/project.git', + customField: { nested: 'Error: https://oauth2:token@example.com/path' }, + urls: ['https://ghp_abc@github.com/private.git', 'https://github.com/public.git'], + exitCode: 128 + }; + + const sanitized = sanitizeGitData(data); + + expect(sanitized.repoUrl).toBe('https://******@github.com/repo.git'); + expect(sanitized.stderr).toBe('fatal: https://******@gitlab.com/project.git'); + expect(sanitized.customField.nested).toBe('Error: https://******@example.com/path'); + expect(sanitized.urls[0]).toBe('https://******@github.com/private.git'); + expect(sanitized.urls[1]).toBe('https://github.com/public.git'); + expect(sanitized.exitCode).toBe(128); + }); + + it('should handle edge cases', () => { + expect(sanitizeGitData(null)).toBe(null); + expect(sanitizeGitData(undefined)).toBe(undefined); + expect(sanitizeGitData('https://token@github.com/repo.git')).toBe('https://******@github.com/repo.git'); + }); +}); + +describe('GitLogger', () => { + it('should sanitize Error objects in error() method', () => { + const baseLogger = createNoOpLogger(); + const errorSpy = vi.spyOn(baseLogger, 'error'); + const gitLogger = new GitLogger(baseLogger); + + const error = new Error('Auth failed for https://token@github.com/repo.git'); + gitLogger.error('Git operation failed', error); + + expect(errorSpy).toHaveBeenCalledWith( + 'Git operation failed', + expect.objectContaining({ + message: 'Auth failed for https://******@github.com/repo.git' + }), + undefined + ); + }); +}); diff --git a/tests/integration/src/index.ts b/tests/integration/src/index.ts index df78ff36..b9010029 100644 --- a/tests/integration/src/index.ts +++ b/tests/integration/src/index.ts @@ -1,8 +1,4 @@ -import { - getSandbox, - proxyToSandbox, - type Sandbox -} from '@cloudflare/sandbox'; +import { getSandbox, proxyToSandbox, type Sandbox } from '@cloudflare/sandbox'; import { codeExamples } from '../shared/examples'; import { executeCommand,