Skip to content

Require authentication and room authorization for room-bound MCP tools - #137

Merged
alexmontesg merged 6 commits into
mainfrom
bugfix/GH-135-fixmcp-require-authent
Oct 5, 2026
Merged

alexmontesg merged 6 commits into
mainfrom
bugfix/GH-135-fixmcp-require-authent

Conversation

@alexmontesg

@alexmontesg alexmontesg commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Closes #135.

Splits the MCP server into two endpoints:

  • /ai/v1/mcp: public, five stateless tools only.
  • /ai/v1/mcp-rooms: requires a user session or the internal token; adds get-node, add-node, update-node, delete-node.

Room tools now take roomId + pageId and check room membership and page ownership on every call. Unknown pages and non-members get the same generic error. Sessions are bound to the identity that created them. The in-app Mastra agents use the internal token over /mcp-rooms, and x-internal-token / _token are redacted from logs.

SSRF hardening (#136) and share-link lifecycle are out of scope; separate PRs.

Verification

  • npm run lint and npm run build pass; openspec validate --strict passes.
  • Local harnesses: anonymous and wrong-token calls to /mcp-rooms return 401; /mcp lists only stateless tools; the authorization helper was checked in 7 cases.

Live verification (local server, real better-auth sessions, develop Azure storage, real LLM)

  • Anonymous initialize returns 401; a member lists all 9 tools on /mcp-rooms.
  • Member add-node, get-node (from a second session), update-node and delete-node on their own page work.
  • Non-member, page from another room and unknown page all return the same generic error.
  • Session id reused by a different logged-in user returns 403; anonymous returns 401; wrong internal token returns 401.
  • AI room-edit through the chat endpoint: plan, approval, then the workflow added the requested node to the page via /mcp-rooms with the internal token. The other room's page was untouched.

Not verified

  • Visual rendering of the added nodes on the real canvas (frontend is not in this repo). Persisted document content was checked directly.

@alexmontesg
alexmontesg force-pushed the bugfix/GH-135-fixmcp-require-authent branch from 0ad231c to 459914e Compare October 2, 2026 11:20
@alexmontesg

Copy link
Copy Markdown
Contributor Author

Live verification (local server, real better-auth session, develop Azure storage, real LLM):

  • 4.3 MCP auth: anonymous init → 401; member lists all 9 tools; add/get/update/delete-node on own page OK; non-member, cross-room page and unknown page → same generic error; another user's or anonymous use of a session id → 403/401; wrong internal token → 401.
  • 5.3 AI edit: chat route → plan → approval → workflow added the requested rectangle to the page via /mcp-rooms with the internal token; the other room's page was untouched.
  • Not verified: visual rendering on the real canvas (frontend not in this repo).

@alexmontesg
alexmontesg marked this pull request as ready for review October 5, 2026 08:51
@alexmontesg
alexmontesg requested a review from a team as a code owner October 5, 2026 08:51
@alexmontesg alexmontesg added the bug Something isn't working label Oct 5, 2026
@alexmontesg alexmontesg self-assigned this Oct 5, 2026
@alexmontesg
alexmontesg requested a balanced review from Copilot October 5, 2026 08:54

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Delegated agent calls still lack enforced authorization tied to the initiating user's room access.

Review effort: Balanced
Findings: 2 High severity · 1 Medium severity

Open (3)
What changed in this PR

Separates public MCP utilities from authenticated canvas operations to address #135.

Changes:

  • Adds protected room tools, per-call authorization, and identity-bound sessions.
  • Migrates Mastra agents and workflows to pass authenticated room/page context.
  • Adds internal-token header support, log redaction, and endpoint documentation.
File Description
code/​src/​middlewares/​internal-token.ts Accepts internal-token headers.
code/​src/​middlewares/​http-logger.ts Adds credential redaction.
code/​src/​mcp/​tools/​update-node.ts Authorizes page updates.
code/​src/​mcp/​tools/​get-node.ts Authorizes node reads.
code/​src/​mcp/​tools/​delete-node.ts Authorizes node deletion.
code/​src/​mcp/​tools/​add-node.ts Authorizes node creation.
code/​src/​mcp/​tool-result.ts Centralizes tool error responses.
code/​src/​mcp/​index.ts Splits endpoints and binds sessions to callers.
code/​src/​mcp/​authorize.ts Checks membership and page ownership.
code/​src/​mastra/​workflows/​room-edition-workflow/​steps/​references-extraction.ts Supplies room/page identifiers.
code/​src/​mastra/​workflows/​room-edition-workflow/​steps/​plan.ts Carries room context into planning.
code/​src/​mastra/​workflows/​room-edition-workflow/​steps/​execution.ts Carries room context into execution.
code/​src/​mastra/​mcp-client.ts Creates the authenticated internal client.
code/​src/​mastra/​agents/​room-editor-planner-result-agent.ts Uses the internal client.
code/​src/​mastra/​agents/​room-editor-planner-agent.ts Uses the internal client.
code/​src/​mastra/​agents/​room-editor-executor-result-agent.ts Uses the internal client.
code/​src/​mastra/​agents/​room-editor-executor-agent.ts Uses the internal client.
code/​src/​mastra/​agents/​information-agent.ts Uses the internal client.
.github/​copilot-instructions.md Documents the endpoint split.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread code/src/mastra/mcp-client.ts
Comment thread code/src/middlewares/http-logger.ts Outdated
Comment thread code/src/mastra/mcp-client.ts

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Room-tool error handling and information-agent context propagation still need correction.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
Resolved since last review (3)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Mark missing-node results as MCP errors

code/​src/​mcp/​tool-result.ts:8

This helper omits isError: true, so a missing-node result from get-node is reported as a successful MCP result. For add-node, update-node, and delete-node, its { error } payload also fails the declared success schema requiring nodeId, obscuring the intended error with output validation. Mark these results as errors, as roomAccessErrorResult() already does.

Medium severity Restore missing room document guards in all handlers

code/​src/​mcp/​tools/​get-node.ts:58

Authorization checks membership and page metadata, not document availability. If getRoomDocument returns undefined, the removed guard now lets that value reach getYjsElement instead of returning a controlled error. The same guard was removed in add-node.ts:80, update-node.ts:72, and delete-node.ts:49. Restore if (!roomDocument) return roomAccessErrorResult() after loading in all four handlers, before accessing the document; templates/templates.ts:24–30 still handles this case separately.

Comment thread code/src/mastra/agents/information-agent.ts
@alexmontesg

Copy link
Copy Markdown
Contributor Author

Replies to the 'previously missed' notes, both valid, fixed in 1a624c9: (1) toolErrorResult now sets isError: true, so error results no longer fail the declared output schema (e.g. delete-node's nodeId) and mask the message; (2) I had removed the if (!roomDocument) guard when switching to the page-derived document id, which was a regression, so it is restored in all four handlers (returning the generic access error).

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

The unresolved delegated authorization gap requires fixes and final human security review.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Fix CORS headers and expose MCP session ID

code/​src/​mcp/​index.ts:249

The new endpoint inherits a CORS policy that prevents allowlisted cross-origin browser clients from completing an MCP session. middlewares/cors.ts:29–35 omits Mcp-Session-Id and MCP-Protocol-Version from allowed request headers and does not expose the returned Mcp-Session-Id. A cookie-authenticated browser cannot read the initialization session ID, and requests carrying the protocol headers fail preflight. Allow these request headers and expose the session ID for both MCP routes, including their OPTIONS handler, while preserving the origin allowlist.

],
{
memory,
requestContext: createBoundRequestContext(state.roomId, state.pageId),

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Valid, fixed in b37f4a2: resumeWorkflowTool now compares the run's saved roomId (initialState) with the request context's room, which the chat route authorized for the caller, and answers execution-not-found on mismatch or when there is no context. Checked against a real suspended Mastra run: a different room and a missing context are rejected. Note: saved state alone is no longer treated as proof of authorization.

@alexmontesg

Copy link
Copy Markdown
Contributor Author

Reply to the 'previously missed' CORS note (Mcp-Session-Id / MCP-Protocol-Version): not applying it in this PR. The CORS policy is inherited unchanged from the existing /mcp endpoint, there is no browser-based MCP client in this project (the frontend never calls MCP; only the backend's agents and server-side tools do), and widening allowed/exposed headers is a separate change that should come with an actual browser client. Happy to open a follow-up if browser MCP clients become a requirement.

@alexmontesg alexmontesg changed the title fix(mcp): require authentication and room authorization for room-bound MCP tools Require authentication and room authorization for room-bound MCP tools Oct 5, 2026
alexmontesg and others added 6 commits October 5, 2026 11:55
Split the MCP server into a public endpoint (/ai/v1/mcp, stateless tools)
and an authenticated one (/ai/v1/mcp-rooms) that adds the room tools.
Room tools now take roomId + pageId and check room membership and page
ownership on every call. The in-app AI agents use the internal token.

Refs #135

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Overwrite roomId/pageId on internal room tool calls from the request
context, fully redact percent-encoded _token query params from logs and
allow sharing INTERNAL_SERVICE_TOKEN with Mastra Studio.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…nfo agent

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@alexmontesg
alexmontesg force-pushed the bugfix/GH-135-fixmcp-require-authent branch from b37f4a2 to 67e910b Compare October 5, 2026 09:55
@sonarqubecloud

sonarqubecloud Bot commented Oct 5, 2026

Copy link
Copy Markdown

@alexmontesg
alexmontesg merged commit 080b2d4 into main Oct 5, 2026
11 checks passed
@alexmontesg
alexmontesg deployed to azure-develop October 5, 2026 09:56 — with GitHub Actions Active

This branch was successfully deployed

1 active deployment
azure-develop — 67e910bc Deployed Oct 5, 2026 by alexmontesg via Deploy to Container Apps #499
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Require authentication and room authorization for room-bound MCP tools

3 participants