Skip to content

Fix preview URL DNS compliance by removing underscores #247

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

Fix preview URL DNS compliance by removing underscores #247

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
44631d4
Fix preview URL DNS compliance by removing underscores
github-actions[bot] Nov 25, 2025
cf54f93
Add token migration and DNS compliance fixes
github-actions[bot] Nov 25, 2025
7846d7c
Fix token generation infinite loop bug
github-actions[bot] Nov 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/fix-preview-url-underscores.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
'@cloudflare/sandbox': patch
---

Fix preview URL DNS compliance by removing underscores from tokens

Preview URLs generated by `exposePort()` now use only DNS-valid characters (RFC 952/1123). The token generation now replaces both `+` and `/` characters with hyphens instead of using underscores, ensuring all generated hostnames are valid for DNS resolution.
Copy link
Contributor

@claude claude bot Nov 25, 2025

Choose a reason for hiding this comment

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

This changeset should document the breaking change. Existing tokens stored in Durable Objects will become invalid, breaking active preview URLs until they're regenerated.

Suggest adding:

**Breaking change:** Existing exposed ports will need to be re-exposed to generate new DNS-compliant tokens. Old preview URLs with underscores in tokens will stop working after this update.

Copy link
Contributor

@claude claude bot Nov 25, 2025

Choose a reason for hiding this comment

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

The breaking change section was added but the explanation is technically incorrect - tokens regenerate when DOs wake from sleep, not on "next access" of exposePort(). See new review comment for details.

3 changes: 2 additions & 1 deletion packages/sandbox/src/request-handler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,8 +117,9 @@ export async function proxyToSandbox<E extends SandboxEnv>(
function extractSandboxRoute(url: URL): RouteInfo | null {
// Parse subdomain pattern: port-sandboxId-token.domain (tokens mandatory)
// Token is always exactly 16 chars (generated by generatePortToken)
// Tokens contain only lowercase alphanumeric and hyphens (RFC 952/1123 compliant)
const subdomainMatch = url.hostname.match(
/^(\d{4,5})-([^.-][^.]*?[^.-]|[^.-])-([a-z0-9_-]{16})\.(.+)$/
/^(\d{4,5})-([^.-][^.]*?[^.-]|[^.-])-([a-z0-9-]{16})\.(.+)$/
);

if (!subdomainMatch) {
Expand Down
3 changes: 2 additions & 1 deletion packages/sandbox/src/sandbox.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1655,10 +1655,11 @@ export class Sandbox<Env = unknown> extends Container<Env> implements ISandbox {
crypto.getRandomValues(array);

// Convert to base64url format (URL-safe, no padding, lowercase)
// Use hyphen for both + and / to ensure DNS hostname compatibility (RFC 952/1123)
const base64 = btoa(String.fromCharCode(...array));
return base64
.replace(/\+/g, '-')
.replace(/\//g, '_')
.replace(/\//g, '-')
.replace(/=/g, '')
.toLowerCase();
}
Expand Down
39 changes: 36 additions & 3 deletions packages/sandbox/tests/sandbox.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -797,7 +797,7 @@ describe('Sandbox - Automatic Session Management', () => {
});

expect(result.url).toMatch(
/^https:\/\/8080-my-project-[a-z0-9_-]{16}\.example\.com\/?$/
/^https:\/\/8080-my-project-[a-z0-9-]{16}\.example\.com\/?$/
);
expect(result.port).toBe(8080);
});
Expand All @@ -815,7 +815,7 @@ describe('Sandbox - Automatic Session Management', () => {
const result = await sandbox.exposePort(4000, { hostname: 'my-app.dev' });

expect(result.url).toMatch(
/^https:\/\/4000-myproject-123-[a-z0-9_-]{16}\.my-app\.dev\/?$/
/^https:\/\/4000-myproject-123-[a-z0-9-]{16}\.my-app\.dev\/?$/
);
expect(result.port).toBe(4000);
});
Expand All @@ -835,7 +835,7 @@ describe('Sandbox - Automatic Session Management', () => {
});

expect(result.url).toMatch(
/^http:\/\/8080-test-sandbox-[a-z0-9_-]{16}\.localhost:3000\/?$/
/^http:\/\/8080-test-sandbox-[a-z0-9-]{16}\.localhost:3000\/?$/
);
});

Expand All @@ -855,6 +855,39 @@ describe('Sandbox - Automatic Session Management', () => {
/getSandbox\(ns, "MyProject-ABC", \{ normalizeId: true \}\)/
);
});

it('should generate DNS-valid tokens without underscores (RFC 952/1123)', async () => {
await sandbox.setSandboxName('test-sandbox', false);

vi.spyOn(sandbox.client.ports, 'exposePort').mockResolvedValue({
success: true,
port: 8080,
url: '',
timestamp: '2023-01-01T00:00:00Z'
});

// Generate multiple URLs to test token generation
const results = await Promise.all([
sandbox.exposePort(8080, { hostname: 'example.com' }),
sandbox.exposePort(8081, { hostname: 'example.com' }),
sandbox.exposePort(8082, { hostname: 'example.com' })
]);

for (const result of results) {
const url = result.url;
const hostname = new URL(url).hostname;

// Extract token from hostname pattern: port-sandboxId-token.domain
const match = hostname.match(/^(\d{4,5})-([^.-][^.]*?[^.-]|[^.-])-([a-z0-9-]{16})\.(.+)$/);
expect(match).toBeTruthy();
Copy link
Contributor

@claude claude bot Nov 25, 2025

Choose a reason for hiding this comment

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

Test validates token character set but doesn't verify actual DNS hostname validity. RFC 952/1123 has additional rules (e.g., labels can't start/end with hyphens).

Consider adding a comprehensive hostname validation:

expect(hostname).toMatch(/^[a-z0-9]([a-z0-9-]*[a-z0-9])?(\.[a-z0-9]([a-z0-9-]*[a-z0-9])?)*$/);

This ensures the entire hostname (not just the token) meets DNS requirements.

Copy link
Contributor

@claude claude bot Nov 25, 2025

Choose a reason for hiding this comment

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

The new test at line 859-894 still has this issue - it validates the entire hostname rather than the token component specifically. The test would pass even if tokens have leading/trailing hyphens.


const token = match![3];
// RFC 952/1123: hostnames can only contain alphanumeric and hyphens
expect(token).toMatch(/^[a-z0-9-]+$/);
expect(token).not.toContain('_');
expect(token.length).toBe(16);
}
});
});

describe('timeout configuration validation', () => {
Expand Down
Loading