Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions .changeset/fix-local-data-mount-watch.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@cloudflare/sandbox': patch
---

Fix bidirectional local R2 synchronization for bucket mounts outside `/workspace`, including documented paths such as `/data`. Public `watch()` and `checkChanges()` calls now consistently resolve relative paths from `/workspace` and reject paths outside it on every transport.
3 changes: 3 additions & 0 deletions .github/workflows/reusable-quality.yml
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,9 @@ jobs:
name: build-${{ inputs.artifact_key || github.sha }}
path: packages

- name: Install inotify tools
run: sudo apt-get update && sudo apt-get install -y inotify-tools

- name: Container unit tests (Bun)
run: npm test -w @repo/sandbox-container
timeout-minutes: 5
4 changes: 3 additions & 1 deletion packages/sandbox-container/src/config.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { WORKSPACE_ROOT } from '@repo/shared/internal';

/**
* How long to wait for an interpreter process to spawn and become ready.
* If an interpreter doesn't start within this time, something is fundamentally
Expand Down Expand Up @@ -50,7 +52,7 @@ const STREAM_CHUNK_DELAY_MS = 100;
*
* Default: /workspace
*/
const DEFAULT_CWD = '/workspace';
const DEFAULT_CWD = WORKSPACE_ROOT;

export const CONFIG = {
INTERPRETER_SPAWN_TIMEOUT_MS,
Expand Down
22 changes: 19 additions & 3 deletions packages/sandbox-container/src/control-plane/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1073,14 +1073,30 @@ class WatchRPCAPI extends RpcTarget {
}

async watch(request: WatchRequest): Promise<ReadableStream<Uint8Array>> {
const result = await this.#svc.watchDirectory(request.path, {
const result = await this.#svc.watchDirectory(
request.path,
this.buildWatchRequest(request)
);
return extractData<ReadableStream<Uint8Array>>(result);
}

/** @internal Watches an absolute local bucket mount root. */
async watchMount(request: WatchRequest): Promise<ReadableStream<Uint8Array>> {
const result = await this.#svc.watchMountDirectory(
request.path,
this.buildWatchRequest(request)
);
return extractData<ReadableStream<Uint8Array>>(result);
}

private buildWatchRequest(request: WatchRequest): WatchRequest {
return {
path: request.path,
sessionId: request.sessionId ?? 'default',
recursive: request.recursive,
include: request.include,
exclude: request.exclude
});
return extractData<ReadableStream<Uint8Array>>(result);
};
}

async checkChanges(
Expand Down
50 changes: 39 additions & 11 deletions packages/sandbox-container/src/handlers/watch-handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,11 @@ export class WatchHandler extends BaseHandler<Request, Response> {
const pathname = new URL(request.url).pathname;

if (pathname === '/api/watch' && request.method === 'POST') {
return this.handleWatch(request, context);
return this.handleWatch(request, context, 'public');
}

if (pathname === '/api/watch/mount' && request.method === 'POST') {
return this.handleWatch(request, context, 'mount');
}

if (pathname === '/api/watch/check' && request.method === 'POST') {
Expand All @@ -46,20 +50,28 @@ export class WatchHandler extends BaseHandler<Request, Response> {
*/
private async handleWatch(
request: Request,
context: RequestContext
context: RequestContext,
mode: 'public' | 'mount'
): Promise<Response> {
const normalizedRequest = await this.parseAndNormalizeWatchRequest(
request,
context
context,
mode
);
if (normalizedRequest instanceof Response) {
return normalizedRequest;
}

const result = await this.watchService.watchDirectory(
normalizedRequest.path,
normalizedRequest
);
const result =
mode === 'mount'
? await this.watchService.watchMountDirectory(
normalizedRequest.path,
normalizedRequest
)
: await this.watchService.watchDirectory(
normalizedRequest.path,
normalizedRequest
);

if (!result.success) {
return this.createErrorResponse(result.error, context);
Expand Down Expand Up @@ -103,7 +115,8 @@ export class WatchHandler extends BaseHandler<Request, Response> {

private async parseAndNormalizeWatchRequest(
request: Request,
context: RequestContext
context: RequestContext,
mode: 'public' | 'mount'
): Promise<WatchRequest | Response> {
let body: WatchRequest;
try {
Expand All @@ -124,7 +137,7 @@ export class WatchHandler extends BaseHandler<Request, Response> {
return this.createErrorResponse(validationError, context);
}

const pathResult = this.normalizeWatchPath(body.path);
const pathResult = this.normalizeWatchPath(body.path, mode);
if (!pathResult.success) {
return this.createErrorResponse(pathResult.error, context);
}
Expand Down Expand Up @@ -306,7 +319,10 @@ export class WatchHandler extends BaseHandler<Request, Response> {
return null;
}

private normalizeWatchPath(path: string):
private normalizeWatchPath(
path: string,
mode: 'public' | 'mount' = 'public'
):
| { success: true; path: string }
| {
success: false;
Expand All @@ -329,18 +345,30 @@ export class WatchHandler extends BaseHandler<Request, Response> {
};
}

if (mode === 'mount' && !input.startsWith('/')) {
return {
success: false,
error: {
message: 'mount watch path must be absolute',
code: ErrorCode.VALIDATION_FAILED,
details: { path }
}
};
}

Comment thread
devin-ai-integration[bot] marked this conversation as resolved.
const resolved = input.startsWith('/')
? pathPosix.resolve(input)
: pathPosix.resolve(WORKSPACE_ROOT, input);

if (
mode === 'public' &&
resolved !== WORKSPACE_ROOT &&
!resolved.startsWith(`${WORKSPACE_ROOT}/`)
) {
return {
success: false,
error: {
message: 'path must be inside /workspace',
message: `path must be inside ${WORKSPACE_ROOT}`,
code: ErrorCode.PERMISSION_DENIED,
details: {
path,
Expand Down
7 changes: 7 additions & 0 deletions packages/sandbox-container/src/routes/setup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,13 @@ export function setupRoutes(router: Router, container: Container): void {
middleware: [container.get('loggingMiddleware')]
});

router.register({
method: 'POST',
path: '/api/watch/mount',
handler: async (req, ctx) => container.get('watchHandler').handle(req, ctx),
middleware: [container.get('loggingMiddleware')]
});

router.register({
method: 'POST',
path: '/api/watch/check',
Expand Down
81 changes: 79 additions & 2 deletions packages/sandbox-container/src/services/watch-service.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { posix as pathPosix } from 'node:path';
import type {
CheckChangesRequest,
CheckChangesResult,
Expand All @@ -9,6 +10,7 @@ import type {
import { logCanonicalEvent } from '@repo/shared';
import { ErrorCode } from '@repo/shared/errors';
import type { Subprocess } from 'bun';
import { CONFIG } from '../config';
import type { ServiceResult } from '../core/types';
import { serviceError, serviceSuccess } from '../core/types';

Expand Down Expand Up @@ -87,6 +89,40 @@ export class WatchService {
path: string,
options: WatchRequest = { path }
): Promise<ServiceResult<ReadableStream<Uint8Array>>> {
const normalizedPath = this.normalizePublicWatchPath(path);
if (!normalizedPath.success) {
return serviceError(normalizedPath.error);
}
return this.startWatchDirectory(normalizedPath.path, {
...options,
path: normalizedPath.path
});
}

/** @internal Watches an absolute local bucket mount root. */
async watchMountDirectory(
path: string,
options: WatchRequest = { path }
): Promise<ServiceResult<ReadableStream<Uint8Array>>> {
if (!path.startsWith('/') || path.includes('\0')) {
return serviceError({
message: 'mount watch path must be an absolute path',
code: ErrorCode.VALIDATION_FAILED,
details: { path }
});
}

const normalizedPath = pathPosix.resolve(path);
return this.startWatchDirectory(normalizedPath, {
...options,
path: normalizedPath
});
}

private startWatchDirectory(
path: string,
options: WatchRequest
): ServiceResult<ReadableStream<Uint8Array>> {
const watchResult = this.getOrCreateWatch(path, options);
if (!watchResult.success) {
return serviceError(watchResult.error);
Expand All @@ -95,14 +131,55 @@ export class WatchService {
return serviceSuccess(this.createSubscriberStream(watchResult.data.watch));
}

private normalizePublicWatchPath(path: string):
| { success: true; path: string }
| {
success: false;
error: {
message: string;
code: string;
details: Record<string, unknown>;
};
} {
const input = path.trim();
const workspaceRoot = CONFIG.DEFAULT_CWD;
const resolved = input.startsWith('/')
? pathPosix.resolve(input)
: pathPosix.resolve(workspaceRoot, input);

if (
input.includes('\0') ||
(resolved !== workspaceRoot && !resolved.startsWith(`${workspaceRoot}/`))
) {
return {
success: false,
error: {
message: `path must be inside ${workspaceRoot}`,
code: ErrorCode.PERMISSION_DENIED,
details: { path, resolvedPath: resolved, workspaceRoot }
}
};
}

return { success: true, path: resolved };
}

/**
* Check whether a path changed since a previously returned version.
*/
async checkChanges(
path: string,
options: CheckChangesRequest = { path }
): Promise<ServiceResult<CheckChangesResult>> {
const watchResult = this.getOrCreateWatch(path, options);
const normalizedPath = this.normalizePublicWatchPath(path);
if (!normalizedPath.success) {
return serviceError(normalizedPath.error);
}

const watchResult = this.getOrCreateWatch(normalizedPath.path, {
...options,
path: normalizedPath.path
});
if (!watchResult.success) {
return serviceError(watchResult.error);
}
Expand All @@ -121,7 +198,7 @@ export class WatchService {
? error.message
: 'Failed to establish retained change state',
code: ErrorCode.WATCH_START_ERROR,
details: { path }
details: { path: normalizedPath.path }
});
}
}
Expand Down
65 changes: 63 additions & 2 deletions packages/sandbox-container/tests/handlers/watch-handler.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,16 @@ import type { WatchService } from '../../src/services/watch-service';
function createMockWatchService(): WatchService {
return {
watchDirectory: vi.fn(),
watchMountDirectory: vi.fn(),
checkChanges: vi.fn()
} as unknown as WatchService;
}

function makeRequest(body: Record<string, unknown>): Request {
return new Request('http://localhost:3000/api/watch', {
function makeRequest(
body: Record<string, unknown>,
pathname = '/api/watch'
): Request {
return new Request(`http://localhost:3000${pathname}`, {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify(body)
Expand All @@ -26,6 +30,63 @@ const defaultContext = {
};

describe('WatchHandler', () => {
describe('path authorization', () => {
it('should reject a public watch outside /workspace', async () => {
const handler = new WatchHandler(
createMockWatchService(),
createNoOpLogger()
);

const response = await handler.handle(
makeRequest({ path: '/data' }),
defaultContext
);

expect(response.status).toBe(403);
const body = (await response.json()) as { message: string };
expect(body.message).toBe('path must be inside /workspace');
});

it('should reject a relative internal mount watch path', async () => {
const handler = new WatchHandler(
createMockWatchService(),
createNoOpLogger()
);

const response = await handler.handle(
makeRequest({ path: 'data' }, '/api/watch/mount'),
defaultContext
);

expect(response.status).toBe(400);
const body = (await response.json()) as { message: string };
expect(body.message).toBe('mount watch path must be absolute');
});

it('should allow an internal mount watch at its absolute mount root', async () => {
const watchService = createMockWatchService();
const mockStream = new ReadableStream();
(
watchService.watchMountDirectory as ReturnType<typeof vi.fn>
).mockResolvedValue({
success: true,
data: mockStream
});
const handler = new WatchHandler(watchService, createNoOpLogger());

const response = await handler.handle(
makeRequest({ path: '/data' }, '/api/watch/mount'),
defaultContext
);

expect(response.status).toBe(200);
expect(watchService.watchDirectory).not.toHaveBeenCalled();
expect(watchService.watchMountDirectory).toHaveBeenCalledWith('/data', {
path: '/data'
});
});
});

describe('include/exclude validation', () => {
it('should reject requests with both include and exclude', async () => {
const handler = new WatchHandler(
Expand Down
Loading
Loading