Skip to content

Remove wildcard allowedDomains from playground app seed#2923

Closed
amikofalvy wants to merge 2 commits intomainfrom
fix/playground-seed-no-wildcard
Closed

Remove wildcard allowedDomains from playground app seed#2923
amikofalvy wants to merge 2 commits intomainfrom
fix/playground-seed-no-wildcard

Conversation

@amikofalvy
Copy link
Copy Markdown
Collaborator

Summary

  • Security fix: The 0027_seed-playground-app.sql migration seeded app_playground with allowedDomains: ["*"], accepting any origin until the startup code patched it with concrete domains
  • Changed seed to allowedDomains: [] (empty array) — the app rejects all origins until startup derives and sets the correct domains from INKEEP_AGENTS_MANAGE_UI_URL
  • Eliminates the race window between migration and first startup where the wildcard was live
  • No changes needed to startup code — the newDomains.length > 0 branch already handles adding domains to an empty list

Test plan

  • Verify fresh migration creates playground app with empty allowedDomains
  • Verify startup ensurePlaygroundAppConfig() correctly adds derived domains to the empty array
  • Verify no origin is accepted before startup patches the config
  • pnpm typecheck passes

🤖 Generated with Claude Code

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-bot
Copy link
Copy Markdown

changeset-bot Bot commented Mar 31, 2026

🦋 Changeset detected

Latest commit: a45bde3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 10 packages
Name Type
@inkeep/agents-core Patch
@inkeep/agents-api Patch
@inkeep/agents-manage-ui Patch
@inkeep/agents-cli Patch
@inkeep/agents-sdk Patch
@inkeep/agents-work-apps Patch
@inkeep/ai-sdk-provider Patch
@inkeep/create-agents Patch
@inkeep/agents-email Patch
@inkeep/agents-mcp Patch

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

@vercel
Copy link
Copy Markdown

vercel Bot commented Mar 31, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
agents-api Ready Ready Preview, Comment Mar 31, 2026 2:23am
agents-docs Ready Ready Preview, Comment Mar 31, 2026 2:23am
agents-manage-ui Ready Ready Preview, Comment Mar 31, 2026 2:23am

Request Review

@pullfrog
Copy link
Copy Markdown
Contributor

pullfrog Bot commented Mar 31, 2026

TL;DR — Closes a race-condition security gap by replacing the wildcard "*" in allowedDomains with an empty array in the playground app migration seed. Origins are now rejected until the startup code derives and sets the correct domains from INKEEP_AGENTS_MANAGE_UI_URL.

Key changes

  • Replace wildcard allowedDomains with empty array in 0027_seed-playground-app.sql — Eliminates the window between migration and first startup where any origin was accepted.
  • Add changeset for @inkeep/agents-core — Patch bump documenting the seed fix.

Summary | 2 files | 2 commits | base: mainfix/playground-seed-no-wildcard

Before: The migration seeded app_playground with allowedDomains: ["*"], accepting requests from any origin until ensurePlaygroundAppConfig() ran on startup and patched in concrete domains.
After: The migration seeds with allowedDomains: [] — all origins are rejected by default until startup derives and applies the correct domains. No changes to the startup path were needed since newDomains.length > 0 already handles populating an empty list.

.changeset/marvellous-pink-mandrill.md · 0027_seed-playground-app.sql

Pullfrog  | View workflow run | Triggered by Pullfrog | Using Claude Opus𝕏

Copy link
Copy Markdown
Contributor

@pullfrog pullfrog Bot left a comment

Choose a reason for hiding this comment

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

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.

Pullfrog  | View workflow run | Using Claude Opus𝕏

Copy link
Copy Markdown
Contributor

@claude claude Bot left a comment

Choose a reason for hiding this comment

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

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:

  1. Empty array rejects all origins — Confirmed in domain-validation.ts:5:

    if (!origin || allowedDomains.length === 0) {
      return false;
    }
  2. 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
    }
  3. 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 from INKEEP_AGENTS_MANAGE_UI_URL.

  4. Changeset is appropriate — Patch version bump for @inkeep/agents-core with 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.

Copy link
Copy Markdown
Contributor

@claude claude Bot left a comment

Choose a reason for hiding this comment

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

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.

@itoqa
Copy link
Copy Markdown

itoqa Bot commented Mar 31, 2026

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)
Category Summary Screenshot
Edge 🟠 Manage API create/update validation rejects allowedDomains: [] for web_client apps, blocking explicit fail-closed configuration. EDGE-2
Edge ⚠️ Mobile playground controls collapse and become obstructed on 390px viewport EDGE-3
🟠 Create/update validation rejects explicit empty allowedDomains for web_client apps
  • What failed: The API rejects explicit empty allowlists with a validation error (min(1)), but expected behavior is to allow creating the app and then deny all origins until domains are configured.
  • Impact: Teams cannot configure or test an explicit fail-closed (deny-all) web client app state through supported create/update APIs. This blocks the intended safe baseline flow and makes config behavior inconsistent between write-time validation and runtime auth.
  • Introduced by this PR: No – pre-existing bug (code not changed in this PR)
  • Steps to reproduce:
    1. Send a POST request to /manage/tenants/default/projects/andrew/apps for a web_client app with config.webClient.allowedDomains set to an empty array.
    2. Observe that the request is rejected with a validation error requiring at least one domain.
    3. Compare runtime origin validation behavior, which denies requests when allowedDomains is empty (fail-closed), showing write/runtime inconsistency.
  • Stub / mock context: App management requests used a local bypass bearer token instead of interactive admin login, and local project/agent seed records were created before execution so the target app path existed. No route interception or response stubbing was applied to the origin-validation code path.
  • Code analysis: I traced app creation/update request validation and runtime auth enforcement; create/update uses AppApiInsertSchema/AppApiUpdateSchema, which currently enforce allowedDomains length >= 1, while runtime auth explicitly handles empty arrays as denied origins.
  • Why this is likely a bug: The write path forbids an empty allowlist while the runtime path is explicitly coded to support empty allowlists as a deny-all state, indicating inconsistent production logic rather than test setup interference.

Relevant code:

packages/agents-core/src/validation/schemas.ts (lines 1735-1741)

export const WebClientConfigSchema = z
  .object({
    type: z.literal('web_client'),
    webClient: z.object({
      allowedDomains: z.array(AllowedDomainSchema).min(1),
      auth: WebClientAuthConfigSchema.optional(),
    }),
  })
  .openapi('WebClientConfig');

agents-api/src/domains/manage/routes/apps.ts (lines 134-140)

request: {
      params: TenantProjectParamsSchema,
      body: {
        content: {
          'application/json': {
            schema: AppApiInsertSchema,
          },
        },
      },
    },

packages/agents-core/src/utils/domain-validation.ts (lines 1-7)

export function validateOrigin(
  origin: string | null | undefined,
  allowedDomains: string[]
): boolean {
  if (!origin || allowedDomains.length === 0) {
    return false;
  }
}
⚠️ Mobile viewport playground flow remains functional
  • What failed: The chat composer shrinks to a near-unusable width and interaction is obstructed by layout overlap, instead of keeping input and send controls fully usable on mobile.
  • Impact: Mobile users can be blocked from reliably composing or sending messages in Playground. This prevents core validation flows on small devices and undermines agent testing usability.
  • Introduced by this PR: No – pre-existing bug (code not changed in this PR)
  • Steps to reproduce:
    1. Open /default/projects/andrew/agents/friendly-agent in the Manage UI.
    2. Switch to a 390x844 mobile viewport.
    3. Open Playground with the Try it action.
    4. Type and send a message while the mobile sidebar state is active.
    5. Observe the composer width and whether the send control remains usable.
  • Stub / mock context: No stubs, mocks, or bypasses were applied for this test in the recorded run.
  • Code analysis: The agent page always renders a horizontal ResizablePanelGroup with persisted desktop-style layout state and enforces a minimum playground split, but there is no mobile-specific pane strategy. In parallel, the mobile sidebar is rendered as an 18rem sheet overlay, which can consume most of a 390px viewport and overlap the already constrained workspace.
  • Why this is likely a bug: Production layout code combines desktop horizontal pane constraints with a large mobile overlay sidebar and no mobile override, which directly creates the observed input obstruction on narrow screens.

Relevant code:

agents-manage-ui/src/app/[tenantId]/projects/[projectId]/agents/[agentId]/page.client.tsx (lines 575-580)

<ResizablePanelGroup
  id="agent-panel-group"
  direction="horizontal"
  autoSaveId="agent-resizable-layout-state"
  className="relative bg-muted/20 dark:bg-background flex overflow-hidden no-parent-container"
>

agents-manage-ui/src/app/[tenantId]/projects/[projectId]/agents/[agentId]/page.client.tsx (lines 723-729)

<ResizablePanel
  minSize={25}
  id="playground-pane"
  order={3}
  className={showTraces ? 'w-full flex-none!' : ''}
>

agents-manage-ui/src/components/ui/sidebar.tsx (lines 174-183)

if (isMobile) {
  return (
    <Sheet open={openMobile} onOpenChange={setOpenMobile} {...props}>
      <SheetContent
        data-sidebar="sidebar"
        className="bg-sidebar text-sidebar-foreground w-(--sidebar-width) p-0 [&>button]:hidden"
        style={{ ['--sidebar-width' as string]: SIDEBAR_WIDTH_MOBILE }}
✅ Passed (5)
Category Summary Screenshot
Adversarial Spoofed Origin https://evil.attacker.com was denied with 403 and did not issue any anonymous token. ADV-1
Adversarial Anonymous-session call without Origin header returned 403 and did not return token fields. ADV-2
Adversarial Origin value not-a-url returned authorization denial (403), with no server error/stack-trace behavior and no token issuance. ADV-3
Logic Startup patch correctly transitions seeded app from empty allowlist to derived local domains after boot delay. LOGIC-1
Happy-path POST /run/auth/apps/app_playground/anonymous-session with Origin http://localhost:3000 returned 200 with anonymous token and expiresAt. ROUTE-2

Commit: a45bde3

View Full Run


Tell us how we did: Give Ito Feedback

@amikofalvy amikofalvy closed this Apr 1, 2026
auto-merge was automatically disabled April 1, 2026 02:23

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant