Skip to content

fix(app): enforce allowManageExtensions and allowControlSettings - #3113

Open
benjaminshafii wants to merge 2 commits into
devfrom
feat/enforce-declared-desktop-policies
Open

fix(app): enforce allowManageExtensions and allowControlSettings#3113
benjaminshafii wants to merge 2 commits into
devfrom
feat/enforce-declared-desktop-policies

Conversation

@benjaminshafii

Copy link
Copy Markdown
Member

Problem

Two desktop policies are presented to organization administrators in the Cloud dashboard and are completely unenforced by the desktop app. An admin can toggle them and nothing happens.

Verified: allowManageExtensions and allowControlSettings are declared in packages/types/src/den/desktop-policies.ts (both defaultValue: true, both with userNotice copy already written) and had zero reads anywhere in apps/app/src. Only allowBuiltInExtensions was enforced.

This is a trust and compliance defect: we tell administrators they have a control they do not have. It came up directly in an enterprise review, where the ask was "we don't want to expose everything to them" — and we said the policy existed.

Change

Enforce both, reusing the existing allowBuiltInExtensions idiom (useDesktopRestriction, explanatory banner, per-card disabledReason) and each policy's own userNotice string rather than new copy.

Gated: Add Custom App · From GitHub · quick-connect install · local extension config slots · MCP Remove/Enable/Disable · skill Uninstall · plugin Add/Remove · plugin package input · sidebar Settings item · command-palette Settings/Extensions/Appearance/Updates · typed /settings/* routes.

Deliberately left open — this is the important half:

  • MCP OAuth sign-in/sign-out (that is authentication, not local extension management)
  • Signing in to an organization-assigned connection (members must still use what the org gave them)
  • Cloud Account, AI Providers, Cloud Providers, Connect, Recovery routes — needed for sign-in/out, org switching, provider auth, and recovery

An over-broad implementation here would trap a user in an unusable app with settings locked, which is worse than the bug being fixed. Blocked routes render a notice with escape paths instead of dead-ending.

Both policies default to true

Default behavior is unchanged. An organization with no policy data sees zero difference — covered by test.

Verification

pnpm typecheck                       clean
pnpm --filter @openwork/app test     428 pass, 0 fail

New apps/app/tests/desktop-policy-gates.test.ts proves, per policy: absent policy leaves the affordance enabled; false disables it and surfaces the userNotice; and the non-blockable routes stay reachable.

Note for the reviewer

Trivially conflicts with #3111 in mcp-view.tsx — two adjacent additions, both wanted. Verified resolved and green in an integration branch merging all of this work together.

@vercel

vercel Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

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

Project Deployment Actions Updated (UTC)
openwork-app Ready Ready Preview, Comment Jul 25, 2026 3:58pm
openwork-den Ready Ready Preview, Comment Jul 25, 2026 3:58pm
openwork-den-worker-proxy Ready Ready Preview, Comment Jul 25, 2026 3:58pm
openwork-diagnostics Ready Ready Preview, Comment Jul 25, 2026 3:58pm
openwork-landing Ready Ready Preview, Comment, Open in v0 Jul 25, 2026 3:58pm

@benjaminshafii

Copy link
Copy Markdown
Member Author

Validation: real app, isolated Linux sandbox, matched A/B

All nine branches of this series were merged into one integration branch (f91884c3) and driven as a real user through CDP in a fresh Daytona sandbox. Baseline dev was run in a separate, identically-provisioned fresh sandbox with the same model (GPT-4o mini) so the comparison is apples-to-apples.

core-flow step clean dev (2fda6070) this series (f91884c3)
App boots to a usable session surface ✅ 105ms ✅ 94ms
User creates a fresh task in the workspace ✅ 674ms ✅ 662ms
User writes a message and runs it (real inference) ✅ 2768ms ✅ 111405ms
User closes and reopens; the session survives ❌ not persisted ❌ not persisted

Step 4 fails identically on clean dev (Session ses_… was not listed after reopen (not persisted)), so it is pre-existing on dev and not introduced here. Against the canonical core flow this series is inert, which is exactly the claim for a cleanup/wiring series.

Validated frames: core-flow-01-booted.png, core-flow-02-session-created.png, core-flow-03-task-response.png — including the assertion "Visible text does not include 'Something went wrong'".

A real regression this caught, in this series

Booting the real app initially failed with:

TypeError: Unknown file extension ".ts" for packages/types/src/skill-markdown.ts
→ "OpenWork server did not finish starting. Please restart OpenWork."

@openwork/types is a source-only package — every exports entry resolves to raw ./src/*.ts. apps/server is built with plain tsc (no bundling), so a runtime import there survives into emitted JS and Node cannot load it. Only the bun build-bundled files under src/opencode-plugins/ may import it.

pnpm typecheck and both bun suites passed straight through this, because bun loads .ts natively and Vite bundles for the renderer. Only booting the real product surfaced it. It is fixed in #3112, and that PR adds an automated guard that fails if any non-bundled apps/server file imports @openwork/types, plus a plain-node import proof of the built output.

Static verification on the merged result

pnpm typecheck                                        clean
pnpm --filter @openwork/app test                      435 pass,  0 fail
pnpm --filter openwork-server test                    526 pass,  0 fail
pnpm --filter @openwork-ee/den-web test                73 pass,  0 fail
pnpm --filter @openwork-ee/den-web build              succeeded
den-api desktop-policy suites (real MySQL)              9 pass,  0 fail

Honest gaps

  • No per-PR fraimz flow was authored; this is the canonical core-flow inertness proof plus the matched baseline, which is the right standard for a series that is mostly wiring, deletion, and policy gating.
  • The cloud prompt-card behavior in feat(desktop-policies): let organization prompts launch a named skill #3115 is covered by unit + real-DB API tests, not by a driven cloud fraimz — that needs the Den stack and a seeded org.
  • Local macOS pnpm dev could not be used: every worktree shares one userData dir, so app.requestSingleInstanceLock() makes any second instance quit before creating a window (CDP stays up, so it looks like a hang). Worth a separate DX fix.

@benjaminshafii

Copy link
Copy Markdown
Member Author

Correction to my validation note above

In the "Honest gaps" section I wrote that local macOS pnpm dev could not be used because all worktrees share one userData dir, so app.requestSingleInstanceLock() makes any second instance quit before creating a window.

That diagnosis was wrong. I tested it properly afterwards and disproved it:

  • With the lock successfully acquired (verified: the boot banner printed and no contention message appeared), the instance still produced 0 CDP page targets.
  • With an isolated profile and OPENWORK_ELECTRON_USE_MOCK_KEYCHAIN=1, still 0 page targets.
  • From the unmodified main checkout on dev with an isolated profile, still 0 page targets.

So dev Electron currently opens no window on this macOS host regardless of profile, keychain, or worktree. It is pre-existing, unrelated to this series, and I have not root-caused it. The same commits do open a window in a clean Linux sandbox, which is where the validation above was run — so the core-flow evidence in this comment stands unchanged.

The shared-profile collision is nonetheless real (two worktrees genuinely cannot both hold the lock, and the loser lingers with an open CDP port and no log line). That is fixed separately in #3124, which adds OPENWORK_DEV_PROFILE and makes lock contention explain itself and exit.

Flagging it because the original wording would have sent a reviewer chasing the wrong cause.

…desktop-policies

# Conflicts:
#	apps/app/src/react-app/domains/settings/pages/mcp-view.tsx
@benjaminshafii

Copy link
Copy Markdown
Member Author

Merged dev to resolve the mcp-view.tsx conflict flagged above (it appeared once #3111 landed). Both sides were wanted and kept: #3111's setInventoryFilter/initialFilter plumbing alongside this PR's manageExtensionsDisabledReason. Re-verified after the merge: pnpm typecheck clean, pnpm --filter @openwork/app test 430 pass / 0 fail.

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