Conversation
effectiveContainerTagAccess() computes per-tag read/write permission from the session's RBAC state (restricted member roles, scoped OAuth grants), but it was only ever consumed by select-space, guided-save, and upload-file -- all three use it purely to build the list of tags to offer in a picker/dropdown widget. The two tools that actually perform a write, save-memory and add_memory, accept a containerTag argument directly from the caller and never check it against the session's permissions before calling createMemory / forgetMemory. Since containerTags are discoverable (listSpaces / listContainerTags returns them) and not secret, any client can call these tools directly -- bypassing the widget entirely -- with a containerTag it only has read access to (a restricted member) or that isn't in its assigned set (a scoped OAuth grant), and the write still goes through today. Add assertWriteAccess(containerTag, session) in rbac.ts, built on the existing effectiveContainerTagAccess, and call it at the start of both tools before any client call is made. A denied call now returns a normal tool error instead of writing. Covered by 5 new rbac.test.ts cases (full access, restricted read-only, scoped-out tag, scoped read-only own tag, restricted explicit write). apps/mcp's pre-existing e2e/auth.test.ts failures are unrelated -- they hit a live OAuth origin this sandbox doesn't have and fail identically on main before this change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix(mcp): enforce write-tag permission on save-memory and add_memory
effectiveContainerTagAccess() computes per-tag read/write permission
from the session's RBAC state (restricted member roles, scoped
OAuth grants), but it was only ever consumed by select-space,
guided-save, and upload-file -- all three use it purely to build the
list of tags to offer in a picker/dropdown widget. The two tools
that actually perform a write, save-memory and add_memory, accept a
containerTag argument directly from the caller and never check it
against the session's permissions before calling createMemory /
forgetMemory.
Since containerTags are discoverable (listSpaces / listContainerTags
returns them) and not secret, any client can call these tools
directly -- bypassing the widget entirely -- with a containerTag it
only has read access to (a restricted member) or that isn't in its
assigned set (a scoped OAuth grant), and the write still goes
through today.
Add assertWriteAccess(containerTag, session) in rbac.ts, built on
the existing effectiveContainerTagAccess, and call it at the start
of both tools before any client call is made. A denied call now
returns a normal tool error instead of writing.
Covered by 5 new rbac.test.ts cases (full access, restricted
read-only, scoped-out tag, scoped read-only own tag, restricted
explicit write). apps/mcp's pre-existing e2e/auth.test.ts failures
are unrelated -- they hit a live OAuth origin this sandbox doesn't
have and fail identically on main before this change.