[codex] fix preview subdomain collision#1943
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 0 minutes and 36 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughA new shared module is introduced to centralize preview subdomain encoding, decoding, and parsing logic. Existing components and backend services are refactored to use these utilities instead of maintaining inline implementations. The changes preserve backward compatibility with legacy subdomain formats while supporting a new standardized format. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f13942d4c0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
shared/preview-subdomain.ts (1)
84-84: Consider usingreplaceAll()for clarity.Static analysis suggests using
String#replaceAll()instead ofString#replace()with a global regex. This is a minor readability improvement.♻️ Suggested change
return { - appId: encodedAppId.replace(/__/g, '.'), + appId: encodedAppId.replaceAll('__', '.'), versionId, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/preview-subdomain.ts` at line 84, Replace the regex-based global replace on encodedAppId with a direct replaceAll call for clarity: in the object where appId is assigned (appId: encodedAppId.replace(/__/g, '.'),) change to use encodedAppId.replaceAll('__', '.') so the appId property uses String#replaceAll on encodedAppId.tests/preview-subdomain.unit.test.ts (1)
4-48: Good test coverage for the security fix.The tests effectively verify:
- Round-trip encoding/decoding with reserved characters
- Critical: Dotted vs double-underscore app IDs produce different subdomains (the namespace collision fix)
- Legacy hostname backward compatibility
- Correct disambiguation when legacy app IDs contain
--Consider adding edge case tests for robustness (optional):
♻️ Suggested additional test cases
it.concurrent('returns null for malformed hostnames', () => { expect(parsePreviewHostname('invalid.domain.com')).toBeNull() expect(parsePreviewHostname('')).toBeNull() }) it.concurrent('handles edge case version IDs', () => { expect(parsePreviewHostname('com-2eexample--0.preview.capgo.app')).toEqual({ appId: 'com.example', versionId: 0, }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/preview-subdomain.unit.test.ts` around lines 4 - 48, Tests currently lack coverage for malformed hostnames and edge-case version IDs; add two new concurrent tests in tests/preview-subdomain.unit.test.ts that call parsePreviewHostname and assert null for clearly invalid inputs (e.g., 'invalid.domain.com' and empty string) and assert correct decoding for an edge-case versionId like 0 using a generated/known encoded hostname (reference helpers buildPreviewSubdomain, encodePreviewAppId and parser parsePreviewHostname to construct or validate these cases); ensure you place them alongside the existing it.concurrent blocks and use toBeNull() for malformed cases and toEqual({...}) for the version-id edge case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@shared/preview-subdomain.ts`:
- Line 84: Replace the regex-based global replace on encodedAppId with a direct
replaceAll call for clarity: in the object where appId is assigned (appId:
encodedAppId.replace(/__/g, '.'),) change to use encodedAppId.replaceAll('__',
'.') so the appId property uses String#replaceAll on encodedAppId.
In `@tests/preview-subdomain.unit.test.ts`:
- Around line 4-48: Tests currently lack coverage for malformed hostnames and
edge-case version IDs; add two new concurrent tests in
tests/preview-subdomain.unit.test.ts that call parsePreviewHostname and assert
null for clearly invalid inputs (e.g., 'invalid.domain.com' and empty string)
and assert correct decoding for an edge-case versionId like 0 using a
generated/known encoded hostname (reference helpers buildPreviewSubdomain,
encodePreviewAppId and parser parsePreviewHostname to construct or validate
these cases); ensure you place them alongside the existing it.concurrent blocks
and use toBeNull() for malformed cases and toEqual({...}) for the version-id
edge case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ed664a50-f6f0-4b8c-abce-69e46a72d5b6
📒 Files selected for processing (4)
shared/preview-subdomain.tssrc/components/BundlePreviewFrame.vuesupabase/functions/_backend/files/preview.tstests/preview-subdomain.unit.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d22d07cc82
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
1 participant
Summary (AI generated)
.to__hostname encoding with a reversible escaped formatMotivation (AI generated)
GHSA-76qq-gg2p-pwwjreports a cross-tenant preview namespace collision becausecom.example.appandcom.example__appcan collapse onto the same public preview hostname when__is decoded back into.. This change removes that lossy canonicalization while preserving compatibility for existing preview links.Business Impact (AI generated)
This protects preview namespace isolation between tenants, prevents one app ID from shadowing another app's preview hostname, and keeps the preview feature trustworthy for customers using app IDs with underscores.
Test Plan (AI generated)
bunx vitest run tests/preview-subdomain.unit.test.tsbun typecheckbun run lint:backendbunx eslint shared/preview-subdomain.ts tests/preview-subdomain.unit.test.ts src/components/BundlePreviewFrame.vue supabase/functions/_backend/files/preview.tsGenerated with AI
Summary by CodeRabbit
New Features
Refactor
Tests