[CRITICAL] C1: MCP write-handlers bypass per-Application RBAC entirely

[rubenvdlinde]

Severity: CRITICAL

Location: lib/Mcp/Handler/CreateAppHandler.php:57, UpsertSchemaHandler.php:49, UpsertPageHandler.php:51, AddWidgetHandler.php:48, UpsertMenuItemHandler.php:50, PromoteVersionHandler.php:52

Description:
Every MCP write-handler calls only requireAuthenticatedUser() and performs no per-Application RBAC check. Any authenticated Nextcloud user can call openbuilt.upsertPage, openbuilt.upsertMenuItem, openbuilt.addWidget, openbuilt.upsertSchema, or openbuilt.promoteVersion against any virtual app — including apps where they hold no owners/editors role and are not an NC admin.

Scenario:

1. User A owns my-app. User B has no role on it.
2. User B calls openbuilt.upsertPage with appSlug: "my-app" and any pageId.
3. The handler passes requireAuthenticatedUser(), proceeds to mutate my-app's manifest without checking User B's role.
4. User B successfully overwrites or injects pages/menus/widgets into an app they do not own.

Contrast with controllers:

• ApplicationVersionsController::update (line 291) calls requireRole(slug, WRITE_ROLES).
• VersionPromotionController::promote (line 146) calls assertEditorOrOwner — NC admins are NOT auto-granted per REQ-OBVP-007.

The MCP path defeats both gates entirely.

Suggested fix:

• Extract the per-Application RBAC check (owners/editors lookup) into a shared service used by both controllers and MCP handlers.
• Each write-handler must invoke this check against the target appSlug before any mutation.
• promoteVersion should be owners-only (no NC-admin auto-grant, consistent with REQ-OBVP-007).

Source: deep team-reviewer pass 2026-05-27

[rubenvdlinde]

Verdict: CONFIRMED (cross-Application RBAC bypass end-to-end, with DB persistence)

Verified 2026-05-27 against deployed source at
/var/www/html/custom_apps/openbuilt/lib/Mcp/OpenBuiltToolProvider.php. Note: the issue
cites separate lib/Mcp/Handler/*Handler.php files; the actual implementation lives as
private handle* methods inside OpenBuiltToolProvider.php. The check pattern is
identical to what the issue describes.

Source confirmation

grep of deployed file:

256: if ($this->requireAuthenticatedUser() === null) { ...  // handleListApps
418: if ($this->requireAuthenticatedUser() === null) { ...  // handleCreateApp
501: if ($this->requireAuthenticatedUser() === null) { ...  // handlePromoteVersion
611: if ($this->requireAuthenticatedUser() === null) { ...  // handleUpsertSchema
700: if ($this->requireAuthenticatedUser() === null) { ...  // handleUpsertPage
791: if ($this->requireAuthenticatedUser() === null) { ...  // handleAddWidget

No requireRole(appSlug, WRITE_ROLES), no assertEditorOrOwner, no admin check — every
write handler runs the moment auth succeeds, against any appSlug the caller names.

Probe environment

• MCP server at POST /index.php/apps/openregister/api/mcp (JSON-RPC 2.0)
• Non-admin obverify-bob (zero groups) initialised an MCP session via initialize, then
  called write tools by namespaced id (openbuilt.createApp, openbuilt.upsertSchema)
• Admin used a parallel session to seed a victim app (admin-app)

Probe — non-admin bob creates a brand new virtual app

tools/call → openbuilt.createApp with {"slug":"bob-victim","name":"Bob Stolen App","preset":"single"}:

{"success": true, "created": true,
 "app": {"uuid":"5a604f57-8fce-409a-8b77-a0518d1a4e32","slug":"bob-victim","name":"Bob Stolen App","preset":"single"}}

Bob, with zero NC group membership and no openbuilt role, created a virtual app.

Probe — non-admin bob writes a schema into an admin-owned app (cross-tenant)

Admin first created admin-app via MCP. Then bob (different session, non-admin) called
tools/call → openbuilt.upsertSchema with {"appSlug":"admin-app","slug":"crosstenantinject",...}:

{"success": true, "action": "created",
 "schema": {"id":379, "slug":"admin-app-development-crosstenantinject",
            "title":"Bob Cross","register":"openbuilt-admin-app-development"}}

Direct Postgres query confirms persistence:

 id  |                  slug                   |     title
-----+-----------------------------------------+---------------
 379 | admin-app-development-crosstenantinject | Bob Cross
 378 | admin-app-production-hello-message      | Hello Message
 377 | admin-app-development-hello-message     | Hello Message

Bob successfully injected a schema namespaced to admin-app-development, against an
application he does not own. The slug-namespacing called out in the issue's "Suggested fix"
is exactly the cross-tenant impersonation vector demonstrated.

Note on promoteVersion: when bob targeted admin-app directly the MCP call returned
not_found (multitenancy filter at searchObjectsBySlug hides the app from bob's session).
upsertSchema does NOT route through that filter — it goes straight to SchemaMapper, so
the cross-tenant write succeeds even though cross-tenant read fails. This is a strictly more
dangerous shape than the issue described.

Recommendation

Keep open as CRITICAL. The fix-list (shared per-Application RBAC service called from
controllers AND every MCP write handler; promoteVersion owners-only) is correct. Add to
the fix scope: upsertSchema must validate the calling user's role on the target Application
BEFORE invoking SchemaMapper::createFromArray — currently the only thing keeping bob from
hijacking arbitrary apps is the slug-namespacing convention, which the issue's suggested fix
correctly notes is no defence.

Per [[deep-review-findings-2026-05-27]] verify-first discipline. Test artefacts left in DB:
schemas 375/376/377/378/379, applications bob-victim and admin-app (cleanup follows).

[rubenvdlinde]

File-path correction: The issue cited paths, but on the development branch the actual implementation uses real per-tool handler classes under that path (the comment about private methods inside OpenBuiltToolProvider.php was inaccurate — the Handler directory does exist and contains all the write-handler files). All handlers have now been fixed. Fixed by #173 (merged).

[rubenvdlinde]

Fixed by #173 — all MCP write handlers now call requireWriteRole(appSlug) before any mutation. promoteVersion uses allowAdminBypass: false per REQ-OBVP-007.