Fix preview URL DNS compliance by removing underscores #247
Conversation
Preview URLs generated by exposePort() contained underscores in tokens, violating DNS hostname requirements (RFC 952/1123). Changed token generation to use hyphens for both + and / characters instead of underscores, ensuring all generated hostnames are DNS-valid. - Modified generatePortToken() to replace / with - instead of _ - Updated regex pattern in request-handler to match new token format - Added test to verify tokens contain only DNS-valid characters - Updated existing test expectations to match new pattern Fixes #244 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: whoiskatrin <whoiskatrin@users.noreply.github.com>
🦋 Changeset detectedLatest commit: 7846d7c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Preview URL tokens now use only DNS-valid characters (lowercase alphanumeric and hyphens) per RFC 952/1123. Updated security section to clarify token format and compliance requirements. Related to cloudflare/sandbox-sdk#247
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
Critical issue: This change breaks backward compatibility with existing tokens. Tokens generated with the old format (containing underscores) persist in Durable Object storage but will fail validation after deployment because the regex now only accepts [a-z0-9-].
Impact: All existing exposed ports with active preview URLs will stop working immediately.
Required fix: Add migration logic to regenerate tokens for existing ports. In the constructor after loading tokens (sandbox.ts:190-192):
// Migrate old tokens with underscores to DNS-compliant format
for (const [port, token] of this.portTokens.entries()) {
if (token.includes('_')) {
this.portTokens.set(port, this.generatePortToken());
}
}
await this.persistPortTokens();Security consideration: Character set reduction from base64url (64 chars) to ~base36 (36 chars) reduces entropy from 96 to ~82 bits. Still secure but worth documenting.
The changeset should note this is a breaking change for existing deployments with exposed ports.
|
|
||
| // Extract token from hostname pattern: port-sandboxId-token.domain | ||
| const match = hostname.match(/^(\d{4,5})-([^.-][^.]*?[^.-]|[^.-])-([a-z0-9-]{16})\.(.+)$/); | ||
| expect(match).toBeTruthy(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
|
||
| 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. |
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.
commit: |
🐳 Docker Image PublishedFROM cloudflare/sandbox:0.0.0-pr-247-eb73cf6Version: You can use this Docker image with the preview package from this PR. |
@claude fix all this |
|
Claude finished @whoiskatrin's task —— View job ✅ Backward compatibility and documentation fixes completedAll issues identified in the code review have been addressed and committed. Changes Made
Test Results✅ All 293 tests passing Commitcf54f93 - Add token migration and DNS compliance fixes |
- Migrate old tokens with underscores to DNS-compliant format - Document entropy reduction from base64url to ~base36 - Ensure tokens don't start/end with hyphens per RFC 952/1123 - Add comprehensive DNS hostname validation in tests - Update changeset to document breaking change Co-authored-by: whoiskatrin <whoiskatrin@users.noreply.github.com>
There was a problem hiding this comment.
Claude Code Review
The entropy comment is fixed. Good work there.
However, the token generation has a critical bug that will cause unpredictable behavior in production. The hyphen removal logic at lines 1683-1690 can randomly select hyphens when trying to remove trailing hyphens, potentially creating infinite loops or severe latency spikes.
The test also doesn't validate what it claims - it checks the entire hostname rather than the token specifically, so it would pass even if tokens have leading/trailing hyphens.
Verdict: Needs fixes before merge. The token generation bug is a blocker.
packages/sandbox/src/sandbox.ts
Outdated
| // Ensure token doesn't end with hyphen (RFC 952/1123 requirement) | ||
| // Replace trailing hyphens with alphanumeric chars from the token | ||
| while (token.endsWith('-')) { | ||
| token = token.slice(0, -1) + token.charAt(Math.floor(Math.random() * (token.length - 1))); |
There was a problem hiding this comment.
CRITICAL BUG: charAt(Math.floor(Math.random() * (token.length - 1))) can select a hyphen character, not just alphanumeric. If the token is abc-def------, this could randomly pick position 3 (the hyphen) and the while loop continues indefinitely.
// Fix: only select from alphanumeric characters
const alphanumericChars = token.replace(/-/g, '').split('');
if (alphanumericChars.length === 0) {
return this.generatePortToken(); // Regenerate if all hyphens
}
while (token.endsWith('-')) {
const randomChar = alphanumericChars[Math.floor(Math.random() * alphanumericChars.length)];
token = token.slice(0, -1) + randomChar;
}
while (token.startsWith('-')) {
const randomChar = alphanumericChars[Math.floor(Math.random() * alphanumericChars.length)];
token = randomChar + token.slice(1);
}There was a problem hiding this comment.
Claude finished @whoiskatrin's task —— View job
✅ Token generation bug fixed
Fixed the critical infinite loop bug in generatePortToken() method.
Changes Made
File: packages/sandbox/src/sandbox.ts:1681-1697
The bug was in the logic that replaces leading/trailing hyphens - it could randomly select any character from the token, including hyphens, causing potential infinite loops.
Fix implemented:
- Extract alphanumeric characters only into a separate array
- Handle edge case where token is all hyphens (regenerate)
- Replace trailing/leading hyphens by randomly selecting from alphanumeric chars only
- Ensures RFC 952/1123 DNS hostname compliance
Verification
✅ All 293 tests passing
✅ Sandbox package typecheck passed
✅ Biome linter passed
✅ Changes committed and pushed
Commit
7846d7c - Fix token generation infinite loop bug
|
|
||
| // 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])?)*$/); |
There was a problem hiding this comment.
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 8080-sandbox--token--.example.com passes because 8080-sandbox starts with alphanumeric.
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|
|
||
| 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. |
There was a problem hiding this comment.
"On next access" is misleading. Tokens regenerate when the Durable Object wakes from sleep (constructor runs), not when users call exposePort().
What actually happens:
- Deploy update
- Existing DOs continue with old tokens until evicted from memory
- When DO wakes (any request to that sandbox), migration runs
- Old preview URLs stop working immediately
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.Extract alphanumeric characters only when replacing leading/trailing hyphens in generatePortToken(). Previous implementation could randomly select hyphens when trying to remove them, causing potential infinite loops. - Only select from alphanumeric chars when fixing hyphen positions - Add edge case handling for all-hyphen tokens (regenerate) - Ensures RFC 952/1123 DNS hostname compliance Co-authored-by: whoiskatrin <whoiskatrin@users.noreply.github.com>
1 participant
Preview URLs generated by exposePort() contained underscores in tokens, violating DNS hostname requirements (RFC 952/1123). Changed token generation to use hyphens for both + and / characters instead of underscores, ensuring all generated hostnames are DNS-valid.
Fixes #244
🤖 Generated with Claude Code