-
Notifications
You must be signed in to change notification settings - Fork 40
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| --- | ||
| '@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. | ||
|
|
||
| **Breaking change:** Existing preview URL tokens stored in Durable Objects will be automatically regenerated on next access. Old preview URLs containing underscores will stop working after this update. Users will need to call `exposePort()` again to get new DNS-compliant preview URLs. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "On next access" is misleading. Tokens regenerate when the Durable Object wakes from sleep (constructor runs), not when users call What actually happens:
Suggest: **Breaking change:** Existing preview URL tokens will be automatically regenerated when Durable Objects wake from sleep after this update. Old preview URLs containing underscores will stop working immediately when the sandbox wakes, without requiring a new `exposePort()` call. Users should regenerate preview URLs proactively by calling `exposePort()` again for critical services. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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); | ||
| }); | ||
|
|
@@ -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); | ||
| }); | ||
|
|
@@ -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\/?$/ | ||
| ); | ||
| }); | ||
|
|
||
|
|
@@ -855,6 +855,43 @@ 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; | ||
|
|
||
| // Validate full hostname RFC 952/1123 compliance | ||
| // Labels cannot start or end with hyphens | ||
| expect(hostname).toMatch(/^[a-z0-9]([a-z0-9-]*[a-z0-9])?(\.[a-z0-9]([a-z0-9-]*[a-z0-9])?)*$/); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This regex validates the entire hostname, not the token specifically. The test passes even if the token has leading/trailing hyphens, as long as other hostname labels are valid. Example false positive: hostname Validate the extracted token directly: const token = match![3];
expect(token).toMatch(/^[a-z0-9]([a-z0-9-]*[a-z0-9])?$/); // RFC 952/1123 single label
expect(token).not.toMatch(/--/); // No consecutive hyphens |
||
|
|
||
| // Extract token from hostname pattern: port-sandboxId-token.domain | ||
| const match = hostname.match(/^(\d{4,5})-([^.-][^.]*?[^.-]|[^.-])-([a-z0-9-]{16})\.(.+)$/); | ||
| expect(match).toBeTruthy(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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', () => { | ||
|
|
||
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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.