Remove wildcard allowedDomains from playground app seed#2923
Remove wildcard allowedDomains from playground app seed#2923amikofalvy wants to merge 2 commits intomainfrom
Conversation
Eliminates a race window where any origin could access the playground app between migration and first startup by seeding with an empty domain list instead of ["*"]. The startup code already handles adding concrete domains to an empty list. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
🦋 Changeset detectedLatest commit: a45bde3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
TL;DR — Closes a race-condition security gap by replacing the wildcard Key changes
Summary | 2 files | 2 commits | base:
|
There was a problem hiding this comment.
Clean, focused security fix. The change from ["*"] to [] in the seed migration eliminates the race window between migration and first startup. The startup code at agents-api/src/startup/playground-app.ts:74 already handles the empty-array case correctly via the newDomains.length > 0 branch.
One note: this edits an existing migration (0027) that was merged yesterday in #2911. For environments where it's already been applied, the ON CONFLICT DO NOTHING means this edit has no retroactive effect — but the runtime ensurePlaygroundAppConfig() wildcard-replacement logic (line 66) covers those environments. Fresh deployments benefit directly from this change.
Claude Opus | 𝕏
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Analysis
This PR addresses a security race condition in the playground app seed migration. The change is minimal and well-targeted:
| Before | After |
|---|---|
allowedDomains: ["*"] |
allowedDomains: [] |
Security verification complete:
-
✅ Empty array rejects all origins — Confirmed in
domain-validation.ts:5:if (!origin || allowedDomains.length === 0) { return false; }
-
✅ Startup code handles empty arrays — Confirmed in
playground-app.ts:74:} else if (newDomains.length > 0) { const mergedDomains = [...currentDomains, ...newDomains]; // ... adds derived domains to empty array }
-
✅ Race window eliminated — Previously, the wildcard accepted ANY origin between migration and startup. Now, NO origins are accepted until
ensurePlaygroundAppConfig()derives and sets concrete domains fromINKEEP_AGENTS_MANAGE_UI_URL. -
✅ Changeset is appropriate — Patch version bump for
@inkeep/agents-corewith clear description.
✅ APPROVE
Summary: Clean, focused security fix that eliminates a time-of-check-to-time-of-use (TOCTOU) vulnerability window. The implementation is correct and requires no startup code changes since the existing newDomains.length > 0 branch handles empty arrays properly. Ship it! 🚀
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: No sub-reviewers dispatched — this is a small, focused security fix (2 files, 1 line of code changed) that doesn't require specialized domain reviewers.
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Delta Analysis
This review covers the changes since the previous automated review.
Delta scope: The synchronization was a merge commit (a45bde3c) pulling in changes from main. The 5 files in the delta are all .github/ CI infrastructure files from PR #2921 (fix(ci): teardown Vercel preview env vars on PR close):
| File | Source |
|---|---|
.github/scripts/preview/cleanup-vercel-stale-env.sh |
PR #2921 |
.github/scripts/preview/common.sh |
PR #2921 |
.github/scripts/preview/teardown-vercel-preview-env.sh |
PR #2921 |
.github/workflows/ci.yml |
PR #2921 |
.github/workflows/preview-environments.yml |
PR #2921 |
These infrastructure changes were already reviewed and merged to main separately — they are not in scope for this PR's security fix.
PR's actual changes: The security fix (allowedDomains: ["*"] → [] in 0027_seed-playground-app.sql) and changeset are unchanged since the prior review.
✅ APPROVE
Summary: No new findings since the prior review. The delta consists entirely of CI infrastructure merged from main (PR #2921) — not new work in this branch. The original security fix remains clean and correctly implemented. Ship it! 🚀
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: Sub-reviewers not dispatched — delta is a merge from main with no new PR-specific changes to review.
Ito Test Report ❌7 test cases ran. 2 failed, 5 passed. Overall, the unified run was mixed: 7 test cases executed with 5 passing and 2 failing (plus one code-first verification that produced no reportable outcomes), confirming strong behavior in anonymous-session origin enforcement and startup allowlist initialization. The key findings were that allowed local Origin requests correctly returned tokens while spoofed, missing, and malformed Origin headers were all denied with 403, and that the seeded playground app correctly transitions from an empty fail-closed allowlist to derived local domains after boot, but two pre-existing production defects remain: a medium-severity Manage API validation mismatch that wrongly rejects explicit empty web_client allowedDomains despite runtime deny-all support, and a high-severity mobile UI layout issue that compresses/obstructs the Playground composer on a 390px viewport. ❌ Failed (2)🟠 Create/update validation rejects explicit empty allowedDomains for web_client apps
Relevant code:
export const WebClientConfigSchema = z
.object({
type: z.literal('web_client'),
webClient: z.object({
allowedDomains: z.array(AllowedDomainSchema).min(1),
auth: WebClientAuthConfigSchema.optional(),
}),
})
.openapi('WebClientConfig');
request: {
params: TenantProjectParamsSchema,
body: {
content: {
'application/json': {
schema: AppApiInsertSchema,
},
},
},
},
export function validateOrigin(
origin: string | null | undefined,
allowedDomains: string[]
): boolean {
if (!origin || allowedDomains.length === 0) {
return false;
}
}
|
| Category | Summary | Screenshot |
|---|---|---|
| Adversarial | Spoofed Origin https://evil.attacker.com was denied with 403 and did not issue any anonymous token. | |
| Adversarial | Anonymous-session call without Origin header returned 403 and did not return token fields. | |
| Adversarial | Origin value not-a-url returned authorization denial (403), with no server error/stack-trace behavior and no token issuance. | |
| Logic | Startup patch correctly transitions seeded app from empty allowlist to derived local domains after boot delay. | |
| Happy-path | POST /run/auth/apps/app_playground/anonymous-session with Origin http://localhost:3000 returned 200 with anonymous token and expiresAt. |
Commit: a45bde3
Tell us how we did: Give Ito Feedback
Pull request was closed
Summary
0027_seed-playground-app.sqlmigration seededapp_playgroundwithallowedDomains: ["*"], accepting any origin until the startup code patched it with concrete domainsallowedDomains: [](empty array) — the app rejects all origins until startup derives and sets the correct domains fromINKEEP_AGENTS_MANAGE_UI_URLnewDomains.length > 0branch already handles adding domains to an empty listTest plan
allowedDomainsensurePlaygroundAppConfig()correctly adds derived domains to the empty arraypnpm typecheckpasses🤖 Generated with Claude Code