docs(mcp): audit process-global config for the hosted profile (#306)#401
Conversation
Document the single-integrator, single-backend assumption that makes shared-settings reads safe under the remote profile, inventory every such read a tool call can hit, and state the rework trigger (one host serving multiple backends). Enforce the line with an import-linter forbidden contract: tool code cannot reach pipefy_mcp.settings, directly or transitively, outside the reviewed ignore list.
Review on #401: the SDK binds one session credential to all three API endpoints, so nothing is service-account-specific and there is no separate acceptance to verify at this layer. State the real reason Internal-API tools stay off the remote seed: unreviewed write mutations.
…tion The SDK binds the session's one credential to all three API endpoints, so the Internal API is a different URL, not a different credential. Keep the useful fact (the mutation is Internal-API-only) and lose the env-var requirement that never existed at this layer.
gbrlcustodio
left a comment
gbrlcustodio
left a comment
There was a problem hiding this comment.
One inline note on the process-global configuration section.
| value would silently apply to every user. The one truly per-user thing, | ||
| identity, never comes from settings: in hosted mode each request carries its own | ||
| validated token (`RequestScopedIdentity`), and the startup credential settings | ||
| (`settings.auth`) are only ever read in local mode. |
There was a problem hiding this comment.
issue (non-blocking): this contradicts both the code and the Transport profiles section of this same file.
McpRuntime.for_profile's remote branch reads settings.auth at startup: runtime.py:159 passes default_issuer_url=_login_issuer_url(settings), and _login_issuer_url (runtime.py:101) calls settings.auth.to_oidc_client() to derive the default inbound issuer URL.
The Resource-server profile section above states the same behavior (AGENTS.md:55-57): absent a PIPEFY_JWT_ISSUER_URL override, the inbound issuer defaults to "the one this process logs into, the OidcClient issuer". So the document both relies on the remote-mode settings.auth read and denies it here.
The read is per-deployment-safe under the single-realm assumption, so this is a wording fix, not a behavior change. Suggest narrowing the claim: the outbound startup credential is resolved only in local mode, while in remote mode settings.auth is still read once at startup for the default inbound issuer, safe under single-realm. That frames it as a sibling of the single-backend read this section is built around.
There was a problem hiding this comment.
Right on both counts — for_profile's remote branch does read settings.auth (via _login_issuer_url -> to_oidc_client) for the issuer default, and the Resource-server section above already says so. Narrowed the claim in 4c02a26: settings.auth is read in both modes but always for a per-deployment purpose — the outbound startup credential in local, the default inbound issuer in remote — safe under the same single-realm assumption this section is built around.
gbrlcustodio
left a comment
gbrlcustodio
left a comment
There was a problem hiding this comment.
One more inline note, on the enforcement claim.
Review on #401 flagged two overstatements in the process-global section: - settings.auth is read in remote mode too (for_profile derives the default inbound issuer via _login_issuer_url -> settings.auth.to_oidc_client), not only in local mode. Reframe as a per-deployment read safe under single-realm. - The import-linter contract catches settings imports, not reads. A tool can read McpRuntime.settings off its request context with no import edge, so name that as the residual manual-review surface rather than implying full coverage.
One line per paragraph and bullet; let the renderer soft-wrap. Keeps future edits to a single-line diff instead of reflowing the whole paragraph.
The forbidden-path check is the guarantee and still runs strictly (a real tools -> settings import fails the build). Only the meta-check 'is every ignore used?' is downgraded: grimp's graph discovery differs across environments — CI drops the transitive auth.resource_server -> settings edge that a local build records — so under the default error alerting no single ignore list stays green everywhere. warn keeps a genuinely stale ignore visible without failing on skew.
Brings the branch up to date with dev (29 commits: transport host allowlist, bind-safety interlock, hosted request/tool logging, SDK execution-seam unify). Reconcile the #306 settings-guard contract with the new module layout: - drop the auth.resource_server -> settings ignore (dev inlined that parse; the import no longer exists) - add core.transport_security -> settings (reads McpSettings.host / transport allowlist at startup for bind-safety — per-deployment) and auth.session_identity -> settings (Settings type for startup identity, after dev moved StartupIdentity out of core.runtime) - restore strict unmatched_ignore_imports_alerting = error (the earlier warn was based on a misread; the real cause was the stale branch, now merged) Also list the transport/bind-config read in the AGENTS.md audit inventory.
adriannoes
left a comment
adriannoes
left a comment
There was a problem hiding this comment.
Summary
Closes the #306 process-global audit for the hosted profile: the single-integrator, single-backend assumption is documented, and an import-linter forbidden contract keeps pipefy_mcp.tools from reaching pipefy_mcp.settings outside a reviewed ignore list. I reviewed on checkout (b917149c) with lint-imports (both contracts KEPT), a negative probe, and test_remote_profile (7 passed); CI lint and test are green. Ready to merge as-is; two small AGENTS.md polish items below are optional.
What worked well
- The single-backend assumption is stated as an explicit invariant with a clear rework trigger (same shape as the #302 identity move).
- The import-linter guard is mechanical and caught
unified_envelope, which the manual pass missed; the known limit (imports vs value reads viaMcpRuntime.settings) is called out honestly. - The stale service-account claim is gone from
delete_card_relation, and the remote seed is left untouched with the drift guard still green.
Also noted
- In the new process-global section, the note still says some tool docstrings (e.g.
delete_card_relation) claim to require service-account credentials, but this PR already rewrote that docstring and there are no remaining MCP tool SA-credential claims. Prefer past tense ("previously claimed") or drop the example so the note matches the tree. - The inventory is labeled a "full list" of shared-settings reads a tool call can hit. It includes
default_webhook_namefrom the SDK webhook service but omits the siblingallow_insecure_urlsread on create/update in the same service. Same remote-exposure caveat applies; one bullet (or softening "full list") would keep the #306 audit list complete.
- Note the service-account docstring claim in past tense; this PR already removed it, so present tense no longer matched the tree. - Add allow_insecure_urls alongside default_webhook_name in the shared-settings inventory (same webhook service, create/update), so the "full list" holds.
0c624bf to
c74538f
Compare
adriannoes
left a comment
adriannoes
left a comment
There was a problem hiding this comment.
Summary
Follow-up APPROVE on c74538f. The AGENTS.md polish landed (Internal API note + allow_insecure_urls in the inventory), and CI lint/test are green on this head. Still good to merge for #306.
4 participants
Motivation
Part of the hosted on-behalf-of MCP server proposal (#297). Under the hosted
remoteprofile one process serves many concurrent users, and settings loaded from the environment are shared by all of them. That is safe only while a shared value never decides anything per-user — an assumption the tool surface relied on but never stated, with nothing preventing a future tool from reading global settings for a per-user decision.Outcome
packages/mcp/AGENTS.mdgains a section documenting the single-integrator, single-backend assumption, the audited inventory of every shared-settings read a tool call can reach, why the Internal-API tools stay unmarked, and the rework trigger (one host serving multiple backends means those reads must become request-scoped, as identity already did in Request-scoped identity #302).forbiddencontract makes the line mechanical:pipefy_mcp.toolscannot importpipefy_mcp.settings, directly or transitively, outside a reviewed, commented exception list. A new settings import in tool-reachable code now failsuv run lint-importsand must be reviewed ontoignore_importsbefore it can land.meta=REMOTEchanges: the existing 19-tool remote seed passes the audit.Audit findings
Identity — the one truly per-user input — never comes from settings: in remote mode each caller is resolved from the request bearer (
RequestScopedIdentity, #302).settings.authis read at startup in both modes, but always for a per-deployment purpose (the outbound credential in local; the default inbound issuer URL in remote, safe under single-realm). Every shared-settings read a tool call can reach:PIPEFY_BASE_URLgql_reuse_fetched_graphql_schemapermission_denied_enrichment_timeout_secondstools/graphql_error_helpers.py, call timeunified_envelopecore/tool_error_envelope.py, call timedefault_webhook_nameAdjacent note: some tool docstrings (e.g.
delete_card_relation's) claimed to "require service-account credentials" for the Internal API. Review flagged this as stale, and the SDK confirms it:_bindhands the one session credential to all three endpoints, so nothing is service-account-specific. Those tools stay off the remote seed for the ordinary reason — unreviewed write mutations — and the docstring is fixed here (f1776fb).Verification
uv run lint-imports: both contracts KEPT (layers + the new one).from pipefy_mcp.settings import settingsto a tool module breaks the contract (pipefy_mcp.tools.member_tools -> pipefy_mcp.settings); clean again after revert.uv run pytest tests/tools/test_remote_profile.py: 7 passed — theREMOTE_SEEDdrift guard confirms the exposed set is unchanged.Known limit (documented, not a gap this PR closes): the contract catches settings imports, not value reads. Code holding a settings object —
McpRuntime.settings, reachable from any tool via its request context — can read a per-deployment value without an import edge, so that path stays a manual-review item.Mapping to #306's acceptance criteria: the assumption is documented (AGENTS.md section); no tool reads mutable global state for a per-user decision outside it (audit above, now import-guarded); affected tools are marked remote-safe only after review (seed untouched, and the contract + drift guard force review on every future change).
Closes #306.