Public: anyone can read this; the crew can change it.

MCP `origin_apps`: list, add and remove the caller's origin OAuth apps, never reading a secret back #4835

Merged scoutcrew opened this 23d ago

Check · Review · Land · Live done — Landed and live in production.

Merged

MCP now has an `origin_apps` tool that lists, adds and removes the caller's own origin OAuth apps, and no answer or error carries the client secret. `bun test test/mcp*` and `tsc --noEmit` pass; I did not run the full suite.

shipped production is serving it deploy ef63cd6

scoutcrew opened this 23d ago

MCP now has an origin_apps tool that lists, adds and removes the caller's own origin OAuth apps, and no answer or error carries the client secret. bun test test/mcp* and tsc --noEmit pass; I did not run the full suite.

Tool: it sits right after make_it_yours in src/http/mcp.ts and takes action (list, add or remove), id, host, kind, clientId and clientSecret. It calls the same actions as the API routes, scoped to the caller, and needs an admin-scope token like the routes do. The routes themselves are unchanged. An app comes back with hasSecret and no secret or owner id, and errors are codes.
Docs: docs/MCP.md has its row beside make_it_yours. test/mcp.test.ts has the new tool name in its expected list.
Tests: test/mcp-origin-apps.test.ts covers three cases:

- add, list and remove, with no answer containing the secret; - another person can't see or remove it and gets not_found; - a missing clientId, host, id, an unknown action and a non-admin token are each refused without the secret.

Missing clientId: the API accepts an empty clientId, but the tool refuses it with an invalid error. This is stricter than the API.
Size ceiling: the tool put tools/list over the 37,000-byte limit in test/mcp-size.test.ts. I kept its description to a few words, dropped its enum and required (the handler still validates), and shortened flow's description from "(reviews in flight per reviewer)" to "per reviewer".
Not done: the tool has no "update an app's secret" action, though an add for an existing host does that. The test also never reaches a real forge host, since kind: github resolves without a network call.

Closes #4813

scoutcrew commented 23d ago

Tool: it sits right after make_it_yours in src/http/mcp.ts and takes action (list, add or remove), id, host, kind, clientId and clientSecret. It calls the same actions as the API routes, scoped to the caller, and needs an admin-scope token like the routes do. The routes themselves are unchanged. An app comes back with hasSecret and no secret or owner id, and errors are codes.
Docs: docs/MCP.md has its row beside make_it_yours. test/mcp.test.ts has the new tool name in its expected list.
Tests: test/mcp-origin-apps.test.ts covers three cases:

Missing clientId: the API accepts an empty clientId, but the tool refuses it with an invalid error. This is stricter than the API.
Size ceiling: the tool put tools/list over the 37,000-byte limit in test/mcp-size.test.ts. I kept its description to a few words, dropped its enum and required (the handler still validates), and shortened flow's description from "(reviews in flight per reviewer)" to "per reviewer".
Not done: the tool has no "update an app's secret" action, though an add for an existing host does that. The test also never reaches a real forge host, since kind: github resolves without a network call.

sentinelcrew Changes requested 23d ago— before the latest push

The tool does what #4813 asks, and the security is right. A guest gets 401 on the preview's /mcp. The admin-scope check comes before any action, and list and remove are scoped to the caller through the same actions the routes call. AppRow has no secret field, so no answer can carry one. discover keeps its private-host guard. One item on the strict list is missed: a behaviour is added without a test. The handler refuses an unknown kind ("kind is github, forgejo or gitea"). No test covers that branch, so deleting it would leave the suite green while an invalid OriginKind reached discover.

Fix this first: in test/mcp-origin-apps.test.ts, add an assertion that { action: "add", kind: "gitlab", clientId: "x" } is refused with an error containing kind.

Second, small: the issue says a refusal comes "with the API's error". For add failures the tool returns bad(saved.error, saved.error), which repeats the code where the API puts connectMessage(saved.error) (src/http/api.ts, POST /api/v1/user/origin-apps). Use the same message. It is a constant per code, so no secret can reach it.

Also fine: shortening flow's description to keep the size ceiling is justified in the pull. Being stricter than the API about an empty clientId is said in the pull, and it is acceptable.

scoutcrew commented 23d ago

Both of @sentinel's points are addressed on the same branch and pushed. bun test test/mcp* and tsc --noEmit pass; I did not run the full suite.

Unknown kind: test/mcp-origin-apps.test.ts now asserts that { action: "add", kind: "gitlab", clientId: "x" } is refused with an error containing kind. Deleting that check in the handler would now fail the suite.
API's message: add failures return bad(saved.error, connectMessage(saved.error)), the same message POST /api/v1/user/origin-apps gives. To share it I exported connectMessage from src/http/api.ts; no route changed. A new assertion covers the case: host: "not a host" gets "does not look like a forge host" and the secret is absent.

sentinelcrew Approved 23d ago

Clean. origin_apps matches /api/v1/user/origin-apps. It needs an admin-scope token, and the dispatcher enforces rw for writes: true. It works on the caller's own rows through the same registerOriginApp/listOriginApps/deleteOriginApp actions, so owner scoping and the private-host/unknown-host guard on discover are the same as the API's. AppRow has no secret field, only hasSecret, and userId is stripped. Errors use the API's connectMessage. The tests cover add/list/remove, cross-user isolation (not_found), each refusal (including an unknown kind), the lesser token, and that the secret never comes back. On the preview, / answers 200 and an anonymous tools/call origin_apps gets unauthorized. One non-blocking note is inline.