Feat: use api-proxy for user app uptime check instead of direct call #222
Conversation
|
Claude finished @AshishKumar4's task —— View job Code & Security Review (Critical Path)Recommendation: REQUEST CHANGES SummaryThis PR successfully addresses the Cloudflare Zero Trust access issue by routing preview availability checks through the main application proxy. The architectural approach is sound and follows good DRY principles by extracting shared logic into Security IssuesHigh Severity
Medium Severity
Code Quality IssuesMedium Severity
Positive Aspects
Required ChangesBefore this PR can be approved:
Testing Recommendations
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Check preview availability for an app (HEAD request only) | ||
| app.on('HEAD', '/api/apps/:id/preview-status', setAuthLevel(AuthConfig.public), async (c) => { | ||
| const env = c.env; | ||
| const appId = c.req.param('id'); |
There was a problem hiding this comment.
SECURITY: Path Traversal Risk (Medium)
The appId parameter is extracted directly from the URL without validation. While buildUserWorkerUrl() constructs a subdomain, malicious values could potentially bypass intended behavior.
Recommendation: Add input validation to ensure appId matches expected format:
const appId = c.req.param('id');
// Validate appId format (alphanumeric, hyphens, underscores only)
if (!/^[a-zA-Z0-9_-]+$/.test(appId)) {
return new Response(null, { status: 400 });
}
// Additional length check to prevent abuse
if (appId.length > 63) { // DNS subdomain limit
return new Response(null, { status: 400 });
}
| * @param env - Worker environment | ||
| * @returns PreviewResult with availability status and response | ||
| */ | ||
| export async function resolvePreview( |
There was a problem hiding this comment.
SECURITY: Potential SSRF via Request Forwarding (High)
The function accepts an arbitrary Request object and forwards it to sandbox/dispatcher without proper sanitization. If the caller doesn't properly sanitize the request, sensitive headers (cookies, auth tokens) could be leaked to user-controlled sandboxes.
Current mitigation: The previewProxyRoutes.ts creates a "clean" request with only the HEAD method, but handleUserAppRequest() in worker/index.ts forwards the full original request.
Recommendation: Ensure this function always sanitizes requests before forwarding:
export async function resolvePreview(
appId: string,
request: Request,
env: Env,
sanitize: boolean = true // Add flag to enforce sanitization
): Promise<PreviewResult> {
// Create sanitized request for security-sensitive calls
const testRequest = sanitize ? new Request(request.url, {
method: request.method,
// Only forward safe headers
headers: new Headers({
'User-Agent': request.headers.get('User-Agent') || '',
'Accept': request.headers.get('Accept') || '',
}),
}) : request;
const sandboxResponse = await proxyToSandbox(testRequest, env);
// ...
}
| logger.info(`Preview available in dispatcher for: ${appId}`); | ||
|
|
||
| return { | ||
| available: dispatcherResponse.ok, |
There was a problem hiding this comment.
CODE QUALITY: Inconsistent Error Handling
The function returns available: dispatcherResponse.ok but this could be misleading. A 404 or 500 response would make ok === false, but the error handling below catches exceptions separately.
Issue: If a dispatcher returns 403/404/500, this reports available: false with no error message, making debugging difficult.
Recommendation:
const dispatcherResponse = await worker.fetch(request);
logger.info(`Preview available in dispatcher for: ${appId}, status: ${dispatcherResponse.status}`);
if (!dispatcherResponse.ok) {
return {
available: false,
error: `Dispatcher returned status ${dispatcherResponse.status}`,
};
}
return {
available: true,
type: 'dispatcher',
response: dispatcherResponse,
};
| return null; | ||
| } | ||
|
|
||
| const statusUrl = `/api/apps/${appId}/preview-status`; |
There was a problem hiding this comment.
SECURITY: Insufficient URL Validation (Medium)
The appId is extracted from a user-provided URL without validation before being used in an API request. A malicious URL could inject path traversal characters or special characters.
Example attack: If url = "https://../../admin.vibesdk.com/", the extracted appId would be ../../admin, potentially causing issues.
Recommendation: Add validation after extraction:
const appId = getAppIdFromUrl(url);
if (!appId || !/^[a-zA-Z0-9_-]+$/.test(appId)) {
console.log('Invalid app URL or appId:', url);
return null;
}
| async function handleUserAppRequest(request: Request, env: Env): Promise<Response> { | ||
| const url = new URL(request.url); | ||
| const { hostname } = url; | ||
| const appId = hostname.split('.')[0]; |
There was a problem hiding this comment.
CODE QUALITY: No Input Validation on appId
The appId is extracted from hostname without validation before being passed to resolvePreview(). This could allow malicious subdomains to trigger unexpected behavior.
Recommendation: Add validation consistent with the sandbox's extractSandboxRoute() pattern:
const appId = hostname.split('.')[0];
// Validate appId (consistent with DNS subdomain limits and security)
if (!appId || appId.length > 63 || !/^[a-zA-Z0-9_-]+$/.test(appId)) {
logger.warn(`Invalid appId extracted from hostname: ${hostname}`);
return new Response('Invalid application identifier', { status: 400 });
}1 participant
Summary
This PR addresses issues with Cloudflare Zero Trust access for preview iframes by routing availability checks through the main application proxy instead of direct subdomain calls.
Changes
/api/apps/:id/preview-status(public, HEAD only) that checks preview availability through the proxyworker/utils/previewResolver.tsto centralize preview availability logic (sandbox → dispatcher fallback)worker/index.tsto delegate preview resolution to shared utilitypreview-iframe.tsxto use new proxy endpoint instead of direct subdomain HEAD requestsMotivation
Users with Cloudflare Zero Trust policies configured were unable to load preview iframes because the availability check attempted to access user app subdomains directly, which Zero Trust policies blocked. By routing the check through the main application proxy (which users are already authenticated to), the preview availability check bypasses Zero Trust restrictions.
Testing
Related Issues
This may help with preview loading issues mentioned in:
This PR description was automatically generated by Claude Code