[MCP] Resources capability - #788
Conversation
| const port = env.get(CONFIG_PARAMS.HTTP_PORT); | ||
| if (!hostname || !port) return undefined; | ||
| return `https://${hostname}:${port}`; |
There was a problem hiding this comment.
Wrong port constant — https:// URLs silently absent in standard HTTPS deployments.
CONFIG_PARAMS.HTTP_PORT maps to http.port (the plain HTTP port, null by default in defaultConfig.yaml). The HTTPS port is CONFIG_PARAMS.HTTP_SECUREPORT → http.securePort (default 9926). In any deployment where only http.securePort is active (the standard production setup), env.get(CONFIG_PARAMS.HTTP_PORT) returns null, the !port guard fires, and guessAppHttpUrlPrefix returns undefined. Both enumerateAppHttpResources and listResourceTemplates then return empty, so the entire https:// resource surface is silently absent from resources/list and resources/templates/list.
For an HTTP-only deployment (dev/rare), this produces https://host:<httpPort> — wrong scheme.
| const port = env.get(CONFIG_PARAMS.HTTP_PORT); | |
| if (!hostname || !port) return undefined; | |
| return `https://${hostname}:${port}`; | |
| const securePort = env.get(CONFIG_PARAMS.HTTP_SECUREPORT); | |
| if (hostname && securePort) return `https://${hostname}:${securePort}`; | |
| const port = env.get(CONFIG_PARAMS.HTTP_PORT); | |
| if (!hostname || !port) return undefined; | |
| return `http://${hostname}:${port}`; |
There was a problem hiding this comment.
Yeah, consulting securePort is more correct. And this is appropriately named as a "guess". And in Fabric the guess is actually wrong, we would be better off dropping the port altogether because we map 9926 to 443. Now that we shadow secure ports with UDS, I might suggest that if CONFIG_PARAMS.TLS_UNIXDOMAINSOCKETS (Fabric indicator), we drop the port number.
There was a problem hiding this comment.
Fixed in 8323ad0c1:
guessAppHttpUrlPrefixnow prefersCONFIG_PARAMS.HTTP_SECUREPORTforhttps://URLs (was readingHTTP_PORT, which is null by default → silently emptyhttps://surface).- Falls back to
http://...:HTTP_PORTfor dev/HTTP-only setups so the helper still produces a URL there. - On Fabric (
TLS_UNIXDOMAINSOCKETSset), drops the port entirely per Kris's note — Fabric maps 9926 → 443 behind the UDS, so the published surface ishttps://<hostname>without a port.
The helper is still a "guess" — naming kept that way so future LB/proxy weirdness has somewhere honest to land.
|
Reviewed; no blockers found. |
|
CI flake on Job: Re-running just the Node 22 unit shard. |
kriszyp
left a comment
There was a problem hiding this comment.
I noted that we are running into permissions check problems (again?). The PR description vaguely suggests this might change in the next PR when we iterate custom resources. But we already iterating custom resources here.
I would actually prefer, is possible, to land this MCP functionality as a single larger PR than trying to discern how these slices fit together. I know some people prefer smaller PRs, but I would prefer larger aggregate PR(s) to review, if that's ok (I think I am doing most of the review for these).
Anyway, once these permission issues are resolved, fine with merging.
| const port = env.get(CONFIG_PARAMS.HTTP_PORT); | ||
| if (!hostname || !port) return undefined; | ||
| return `https://${hostname}:${port}`; |
There was a problem hiding this comment.
Yeah, consulting securePort is more correct. And this is appropriately named as a "guess". And in Fabric the guess is actually wrong, we would be better off dropping the port altogether because we map 9926 to 443. Now that we shadow secure ports with UDS, I might suggest that if CONFIG_PARAMS.TLS_UNIXDOMAINSOCKETS (Fabric indicator), we drop the port number.
| if (!db || !table) continue; | ||
| const key = `${db}/${table}`; | ||
| if (seen.has(key)) continue; | ||
| const perm = userTablePermissions(user, db, table); |
There was a problem hiding this comment.
This looks wrong, you can't tell if a resource will give permission based on the user table permissions, it is (usually) programmatically determined (this was mentioned on the last PR). This should just be checking the presence of the REST methods.
There was a problem hiding this comment.
Fixed in 8323ad0c1. You're right — list-time user-permission walks are misleading here because Resource access is decided per-record by each subclass's allow{Read,Create,Update,Delete} predicate. Changes:
enumerateAppHttpResourcesnow gates purely on REST-method presence — newhasRestVerbs(prototype)helper, mirrors the verb-presence pattern atresources/openApi.ts:149-153. Any class withget/put/post/patch/delete/updateon its prototype is listed.enumerateUserVisibleTableslost itsuserTablePermissionsfilter too — every Resource-backed table emits aharper://schema/{db}/{table}URI regardless of caller;readTableSchemastill enforces describe/read at read time (that path returns actual schema data, gated by Harper's static describe permission perdataLayer/schemaDescribe.ts:29-49).readAppResourcedrops its user-permission walk too. The descriptor it returns is metadata only — actual data fetches go through the tools surface where allow* runs.- Inlined
userTablePermissionssurvives but is now only used byreadTableSchema.
Tests reshaped: list-time RBAC tests become "list is the same for any caller" with read-time enforcement covered separately. Test fixture (makeTableResource) now returns a class with prototype-defined verbs so the hasRestVerbs filter actually has something to match — mirrors how Harper's TableResource auto-binds get/put/patch/delete.
This also addresses your top-level "permission check problems (again)" note. The cluster_user branch + interface field also went (unused, consistent with the #781 cleanup).
On the workflow preference — noted. Resource enumeration ties to the tool generation in #617/#618, so I think a larger bundled PR for the next round (#617 + #618 + #619) makes more sense than splitting. Will set that up once #788 lands.
44a5f3f to
8323ad0
Compare
kriszyp
left a comment
There was a problem hiding this comment.
Approving — the port/profile boundary holds up
Nice work on this. I dug into the specific concern of whether operations or non-exported tables could leak onto the application interface (9926), and the architecture is sound on both axes:
- Profile is bound to the port at mount time, never derived per-request — operations registers only on the operations server (
operationsServer.ts:195, hardcoded'operations'), application only on the app HTTP port (index.ts handleApplication, hardcoded'application'). Every gate inresources.tskeys off the closure-bakedrequest.profile, so there's no request-driven path that flips profiles. - Operations surface is gated at both list and read —
harper://operationsonly enumerates under the operations profile (resources.ts:242) andreadHarperUriexplicitly rejects it otherwise (resources.ts:351). - Non-exported tables never enter enumeration —
enumerate()walks the globalresourcesMap, which is populated only for exported resources (graphql.ts:196gatesresources.set(...)ontypeDef.export).readTableSchemaalso returns "table not found" for anything absent from that Map, on top of theread/describeRBAC walk. Solid defense-in-depth.
One thing worth confirming before/after merge: exportTypes granularity
Harper's export isn't binary — exportTypes is a per-protocol allow/deny map, and Resources.getMatch(url, exportType) honors entry.exportTypes?.[exportType] !== false. The MCP path doesn't consult it:
enumerateAppHttpResources(resources.ts:333) andenumerateTableBackedResourcesdon't checkexportTypes.readAppResourcecallsgetResources().getMatch(path)(resources.ts:469) with noexportTypeargument.
Two consequences: (1) a resource exported for some protocols but explicitly disabled for the HTTP/default type would still be enumerable/readable over MCP — a surface slightly wider than REST itself; (2) there's no way to express "expose over REST but not MCP" — anything HTTP-exported is auto-exposed on the MCP application surface.
For v1 this may well be acceptable (the descriptor isn't a data fetch — that's deferred to #618), but since the design intends MCP to mirror the public REST surface, it'd be good to confirm this matches the #465 intent. The cleanest path is to pass an MCP export type into getMatch and check exportTypes during enumeration, so MCP inherits operators' existing per-protocol export controls.
Minor test gap while you're in here: the property that actually protects against the non-exported-table leak (absence from the resources Map) isn't directly tested — current fixtures only add exported resources. A test that registers a table-backed Resource outside the Map and asserts it never enumerates would lock in the guarantee. Same for the exportTypes-disabled case.
None of this blocks merge — the boundary you'd worry about is enforced. Approving. 🎉
— Claude
Adds the MCP `resources/` surface per MCP §server/resources rev 2025-06-18. Both URI schemes documented in #465 are supported: - `https://<host>:<port>/<path>` for app-exported Resources. Resolved IN-PROCESS via `Resources.getMatch(path)` — never makes an outbound HTTP request. v1 returns a Resource descriptor (path + db + table + a hint pointing at the corresponding `get_*`/`search_*` tool); full data fetches go through the tools surface (#618) which inherits the existing `transactional()` + per-record ACL path. - `harper://` synthetic URIs for content that has no real REST endpoint: harper://about — server metadata (shares constants with `initialize` via lifecycle.ts) harper://schema/{database}/{table} — Table.attributes filtered by the user's attribute_permissions harper://openapi — generateJsonApi() output harper://operations — operations profile only; user-filtered ops catalog Unlike the tool registry (#615, parallel branch), resources are NOT registered — they're discovered at request time from Harper's global `resources` Map. Per the design doc, every read is re-checked against current RBAC; URIs are not capability tokens. Inlined RBAC walks mirror the patterns in dataLayer/schemaDescribe.ts:29-49 and resources/openApi.ts:149-153. The duplication with #615 is intentional given the parallel-branch constraint; a follow-up after both PRs merge will refactor the shared helpers into a common module. Transport: `dispatchResourcesList`, `dispatchResourceTemplatesList`, and `dispatchResourcesRead` added. Error mapping for resources/* uses JSON-RPC errors (not the isError envelope that tools/call uses — per the spec, isError is a tool-call concept). "not found" maps to -32601; "permission denied" / bad input maps to -32602. Adapters: thread the full `userObject` (role + permission tree) so the resource handlers can do their RBAC filtering. Same change as Lazy imports: Harper's resource graph initializes eagerly on import — the module would fail to load in unit tests that don't boot Harper. `getResources()` / `getOpenApiGenerator()` / `guessAppHttpUrlPrefix()` resolve via `require()` inside the function body; test seams (`_setResourcesForTest`, `_setOpenApiGeneratorForTest`, `_setHttpUrlPrefixForTest`) let unit tests inject fakes. Tests: 38 new (31 resources unit + 7 transport-level) + 4 new integration tests. Total MCP unit suite: 126 passing. Cross-model review (Gemini): findings applied — `readAbout` now reuses the lifecycle module's SERVER_INFO / SERVER_CAPABILITIES / SUPPORTED_PROTOCOL_VERSIONS constants (no drift with `initialize`); added a trace log on bad cursor decode for client-side debugging; fixed a doc/template placeholder mismatch ({db}→{database}). The remaining gemini suggestion (client-controlled list limit) is intentionally deferred: MCP §server/utilities/pagination puts page size under server control; only `cursor` is client-supplied.
- listResources no longer walks user.role.permission tables for the
https:// app-resource surface. Resource access is determined per-record
by each subclass's allow{Read,...} predicate, so a list-time RBAC walk
is misleading; the only meaningful pre-filter is "does this class
expose REST verbs?". New hasRestVerbs helper mirrors the verb-presence
pattern at resources/openApi.ts:149-153.
- harper://schema/{db}/{table} enumeration also drops the list-time perm
filter. readTableSchema still enforces describe/read at read time via
the static user.role.permission walk (consistent with Harper's
describe permission path at dataLayer/schemaDescribe.ts).
- readAppResource drops the user-table-permission walk too. It returns a
metadata descriptor only — actual data fetches go through the tools
surface where allow{Read,...} runs. Descriptor is a hint, not a token.
- guessAppHttpUrlPrefix:
* Prefer HTTP_SECUREPORT (was reading HTTP_PORT, which is null by
default in HTTPS-only deployments, silently dropping the entire
https:// surface).
* Drop the port entirely when TLS_UNIXDOMAINSOCKETS is set (Fabric
shadows the secure port behind a UDS-fronted load balancer that
publishes on 443).
* Fall back to http://...:HTTP_PORT for dev setups without TLS.
- Drop cluster_user from AuthedUser + canRoleInvokeOperation here too
(already cleaned up in toolRegistry.ts; cluster_user is not a
role-grantable permission per ROLE_TYPES_ENUM).
- Strip PR/issue numbers from docblocks.
Test fixtures updated: makeTableResource now returns a class with REST
verbs on the prototype (mirrors Harper's TableResource auto-binding).
Tests covering "tool list filtered by user perms" become "tool list is
the same for any caller" with separate read-time enforcement coverage.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
61ad994 to
e691255
Compare
|
Thanks for the careful review (and for catching the Quick disposition on the two follow-ups:
This needs to land alongside the application-profile tool generation in Test gap → also folded into Heads-up: we're consolidating the remaining MCP work into two larger PRs (per your earlier note about preferring larger aggregates). The new 🤖 Generated with Claude Code |
…ast#618) Walks the Resources registry and generates verb tools (get_/search_/ create_/update_/delete_) for each exported Resource with REST verbs on its prototype. Input schemas are derived from Table.attributes via a new derive.ts mapper; runtime RBAC stays in Harper's existing allow{Read,Create,Update,Delete} predicates (defense in depth — the schema narrowing is a UX layer only). Honors per-protocol exportTypes (kriszyp HarperFast#788 review): - Resources registered with exportTypes.mcp === false are skipped from every MCP enumeration and rejected at read time (readAppResource now passes 'mcp' to Resources.getMatch so the existing per-protocol gate at resources/Resources.ts:97 fires). - Resources with exportTypes.http === false are dropped from the https:// enumeration so the application MCP surface mirrors the public REST surface (still appear under harper://schema/ since the describe path is HTTP-independent). Tool-name sanitization: / and . become _; collisions disambiguated by prefixing the database name, then by a 6-char SHA-256 suffix. Stable across boots. Cursor pagination on search_: opaque {offset:N} encoded base64, fetch limit+1 to detect the next page without a second round-trip. - components/mcp/tools/application.ts: registration + dispatch. - components/mcp/tools/schemas/derive.ts: Table.attributes → JSON Schema mapper with attribute_permissions filtering. - components/mcp/resources.ts: isMcpExposed/isHttpExposed gates, readAppResource passes 'mcp' export type to getMatch. - components/mcp/index.ts: invoke registerApplicationTools from handleApplication. - Tests: 21 unit tests for application.ts (registration, exportTypes gating, sanitization, verb-presence, visibility, dispatch, cursor pagination, leak invariant), 10 for derive.ts (each verb's schema + type mapping + attribute_permissions narrowing), 4 for resources.ts exportTypes coverage (the kriszyp HarperFast#788 invariants). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Summary
Stacks on #763 (transport), parallel with #781 (tool registry). Adds the MCP
resources/surface per MCP §server/resources (rev 2025-06-18). Two URI schemes per the design doc:https://<host>:<port>/<path>— for app-exported Resources. Resolved in-process viaResources.getMatch(path)(never makes an outbound HTTP request). v1 returns a Resource descriptor; full data fetches go through the tools surface ([MCP] Application profile: tool generation over Resources registry #618).harper://— synthetic URIs for content with no REST endpoint:harper://about,harper://schema/{database}/{table},harper://openapi,harper://operations(ops profile only).Closes #616
Tracking: #465 (sub-issue #4 of 11)
Base:
feat/mcp-transport(rebases tomainafter #763 lands)Parallel with: #781 (tool registry)
What this PR lands
components/mcp/resources.ts— new module exportinglistResources,listResourceTemplates,readResource. RBAC-filtered enumeration, opaque-cursor pagination, every read re-checks current permissions.components/mcp/transport.ts— addsdispatchResourcesList,dispatchResourceTemplatesList,dispatchResourcesRead.NormRequestnow carriesuserObject?: AuthedUserso resource handlers see the full role tree.request.hdb_user(Fastify) /request.user(Harper-HTTP). Same shape as [MCP] Tool registry + RBAC-aware tools/list #781.Architecture notes
resourcesMap at request time. No registry-cache pattern.Resources,generateJsonApi,server). The graph initializes eagerly on import — without lazy resolution, unit tests that don't boot Harper would fail to load the module. Test seams (_setResourcesForTestetc.) let unit tests inject fakes.dataLayer/schemaDescribe.ts:29-49(table read/describe perms) and theattribute_permissionsshape Harper already uses. The duplication with [MCP] Tool registry + RBAC-aware tools/list #781'stoolRegistry.tsis intentional given the parallel-branch constraint — a follow-up after both merge will extract the shared helpers.resources/*differs fromtools/call. Resources use JSON-RPC errors (-32601"not found" /-32602"permission denied / invalid input"), not theisErrorresult envelope. Per the spec,isErroris a tool-call concept; resourcesreadfailures are protocol errors.What
resources/readreturnsharper://aboutapplication/jsonharper://schema/{database}/{table}{database, table, primaryKey, attributes, attribute_permissions}—attributesfiltered to those the user can readapplication/jsonharper://openapi(app only)generateJsonApi()outputapplication/jsonharper://operations(ops only)application/jsonhttps://...(app only){uri, path, database, table, hint}— hint points the LLM at the corresponding toolapplication/jsonhttps://URIs don't fetch actual records in v1 — that's the tools surface (#618). The AC permits this since mimeType is "varies".Where to put attention
resources.ts:enumerate— RBAC forhttps://Resource enumeration defaults to deny for non-table Resources (only super_user sees them). Conservative; could be loosened in [MCP] Application profile: tool generation over Resources registry #618 when tools start enumerating custom Resources too.resources.ts:readHarperUri—harper://URI parsing uses(host || '') + pathnameto recover the opaque path. Verified forharper://about,harper://operations,harper://schema/data/product.transport.ts:dispatchResourcesReaderror code mapping:-32601for "not found" mirrors the precedent intools/call(unknown tool →-32601);-32602for everything else feels right per JSON-RPC §5.1 but the MCP spec is sparse on this — open to changing if reviewers prefer-32603.Cross-model review (Gemini 0.42.0)
Completed. Findings applied:
readAbouthardcoded protocol versions / capabilitiesSERVER_INFO,SERVER_CAPABILITIES,SUPPORTED_PROTOCOL_VERSIONSfromlifecycle.tsso the metadata can never drift frominitialize.dispatchResourcesListignores client-suppliedlimitcursoris client-supplied. Adding a client-supplied limit would open DoS surface.harperLogger.traceon decode failure for client-side pagination debugging.{db}vs{database}){database}/{table}.Verification
Out of scope (deferred)
resources/subscribe— MCP v2notifications/resources/list_changedover a server-push SSE channel — [MCP] listChanged notifications + per-session bookkeeping #619https://URIs — handled by the tools surface ([MCP] Application profile: tool generation over Resources registry #618)Stacking
This PR is based on
feat/mcp-transport(PR #763) — the same base as #781. After #763 merges, I'll rebase both #781 and this one tomain. Reviewing the diff againstfeat/mcp-transportshows only the resources work — ~1260 LOC, 1 commit, 2 new files + targeted modifications.Generated by an AI agent (Claude Opus 4.7) following the Harper Engineering Process & Guidelines lifecycle.