fix(mcp): list verb tools for table-less Resources to any authenticated user - #1943
Conversation
…ed user (#1940) `makeVisibleTo` returned false for a Resource with no databaseName/tableName, so every non-super user saw none of its verb tools in tools/list. That hid exactly the Resources #1920/#1921 taught to produce rich schemas, and hid nothing meaningful: tools/call already accepts these tools by name, so the gate cost discoverability without buying access control. Return true instead and let the Resource's own allow* predicates enforce at call time — the contract custom `mcpTools` have always had (they have never had a listing filter beyond authentication). The table-backed path is unchanged and still gates on per-table permissions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0188G62J9fZQg4J9rVuqLzjy
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the visibility logic for MCP tools to make table-less resources visible to any authenticated user, with corresponding unit tests added to verify this behavior. The review feedback suggests using assert.strictEqual instead of assert.equal in the new tests to align with the repository's style guide.
| assert.equal(tool.visibleTo(NOBODY), true, `${name} is listed for a user with no table grants`); | ||
| assert.equal(tool.visibleTo(SUPER), true, `${name} is listed for super_user`); |
There was a problem hiding this comment.
According to the repository style guide, we should explicitly use assert.strictEqual instead of assert.equal for strict equality assertions.
| assert.equal(tool.visibleTo(NOBODY), true, `${name} is listed for a user with no table grants`); | |
| assert.equal(tool.visibleTo(SUPER), true, `${name} is listed for super_user`); | |
| assert.strictEqual(tool.visibleTo(NOBODY), true, name + " is listed for a user with no table grants"); | |
| assert.strictEqual(tool.visibleTo(SUPER), true, name + " is listed for super_user"); |
References
- Use assert.strictEqual/assert.deepStrictEqual explicitly where strict semantics are needed. (link)
| assert.equal(getTool('get_Product').visibleTo(NOBODY), false); | ||
| assert.equal(getTool('delete_Product').visibleTo(ALICE_READ), false); |
There was a problem hiding this comment.
According to the repository style guide, we should explicitly use assert.strictEqual instead of assert.equal for strict equality assertions.
| assert.equal(getTool('get_Product').visibleTo(NOBODY), false); | |
| assert.equal(getTool('delete_Product').visibleTo(ALICE_READ), false); | |
| assert.strictEqual(getTool('get_Product').visibleTo(NOBODY), false); | |
| assert.strictEqual(getTool('delete_Product').visibleTo(ALICE_READ), false); |
References
- Use assert.strictEqual/assert.deepStrictEqual explicitly where strict semantics are needed. (link)
|
Reviewed; no blockers found. |
The first commit's `return true` was reached by anonymous callers, not just
authenticated ones. `tools/list` has no auth gate — the MCP handler mounts
`{ after: 'authentication' }`, which is ordering, not a requirement — so an
unauthenticated request arrives as `{ username: '' }` and optional-chains
straight past the super-user check into the table-less branch. Every exported
programmatic Resource's field names, docstrings, and enum values would have
been published on the application port to anyone who could reach it.
Gate on an explicit `isAuthenticated` instead. Custom `mcpTools` do list to
anonymous sessions, but that is an author opt-in — declaring `static mcpTools`
is a deliberate act of publishing — whereas verb tools are generated for every
exported Resource with no author action, so the two are not equivalent and the
original parity argument was too broad.
Replaces the second test, which duplicated existing table-backed assertions,
with the anonymous case that actually pins this.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0188G62J9fZQg4J9rVuqLzjy
|
Runtime note from verifying the sibling PR (#1944) against a booted instance — relevant to this PR's rationale. Confirmed: on the pre-#1943 build, an anonymous MCP session gets Correction to this PR's description: I justified the widening partly by saying OpenAPI "is unfiltered entirely." That overstates it — a default Also worth knowing when reading this PR: my verification fixture defined its verbs as Comment generated by kAIle (Claude Opus 4.8). |
3 participants
Closes #1940.
Problem
makeVisibleTogatedtools/listvisibility on the Resource having a backing database and table:A purely programmatic Resource — a
Resourcesubclass overriding the verbs, or one aggregating across tables — has neither, so no non-super user saw any of its verb tools. That is precisely the class of Resource #1920/#1921 just taught to emit richinputSchema/outputSchema, so the feature was invisible to the realistic MCP caller.The gate also bought no access control.
tools/calldoes not consultvisibleTo—toolRegistry.ts's own docblock says so ("explicitly NOT a security boundary") andtransport.tsdispatches straight fromgetTool(name, profile)to the handler, with an existing pass-through test for a hallucinated name. So hiding these tools cost discoverability and nothing else.Change
Table-less Resources are listed to any authenticated caller; their
allow*predicates enforce at call time. Table-backed Resources are untouched — stillgetUserTablePermissionsper mode.Two corrections to my first commit (review found both)
The first commit returned
trueunconditionally, which included anonymous callers.tools/listhas no auth gate: the MCP handler mounts{ after: 'authentication' }, which is ordering, not a requirement, so an unauthenticated request arrives as{ username: '' }and optional-chains past the super-user check. Every exported programmatic Resource's field names, docstrings, andenumvalues would have been published to anyone who could reach the application port. Fixed in8a8eb36ffwith an explicitisAuthenticatedhelper, plus a regression test covering{ username: '' },undefined, and{}.The "parity with custom
mcpTools" argument was too broad and I've dropped it. Custom tools do list to anonymous sessions — deliberately, for the public-docs case (#1609) — but declaringstatic mcpToolsis an author opt-in, a conscious act of publishing. Verb tools are generated for every exported Resource with no author action, so the defaults are not equivalent and shouldn't be aligned by pointing at each other.Where to look
enumvalues) for table-less Resources they previously couldn't enumerate. Data access is unchanged — that was always theallow*predicates' job — but descriptor text is a real disclosure surface: awithSchemacontract copies author-declared property schemas verbatim, and anenumcan encode business data (tenant codes, tier names). If you'd rather this be opt-in rather than opt-out, say so and I'll switch it to astatic mcpflag.Full MCP unit suite (477 tests) passes.
Related, not fixed here
Review also surfaced that
detectVerbsmanufactures acreate_*tool for everyResourcesubclass — it checkstypeof p.post === 'function'with no base-prototype comparison, unlikehasClassLevelVerbsone file over — which reaches Harper's ownloginResource. Filed as #1945; independent of this change (that's about which tools exist, this is about who sees them).Docs follow-up in documentation#605, which documents the old super-user-only behavior.
PR description generated by kAIle (Claude Opus 4.8).