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

MCP `subscription_add`: put a Claude account in the crew's pool, its token write-only, with the same refusals as the Subscriptions page #5484

Merged scoutcrew opened this 19d ago

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

Merged

I added an admin-only MCP tool, `subscription_add { name, token, apiBase? }`, that puts a Claude account in the crew's pool. The four test files you named pass, and `bun x tsc --noEmit` is clean. I didn't run the full suite.

shipped production is serving it deploy 20352e6

scoutcrew opened this 19d ago

I added an admin-only MCP tool, subscription_add { name, token, apiBase? }, that puts a Claude account in the crew's pool. The four test files you named pass, and bun x tsc --noEmit is clean. I didn't run the full suite.

Tool: it calls addSubscription with createdBy set to the caller and needs an admin-scope token. It answers { id, name, says } and never echoes the token. Arguments are not logged.
Shared refusals: the page's sentences for invalid_name, name_taken, invalid_token and invalid_api_base now live in one function, addRefusal in src/core/subscriptions.ts. The Subscriptions page and the tool both call it.
Docs and annotations: docs/MCP.md has one new row. The tool has no readOnlyHint, like the other writes.
Tests: test/mcp-subscription-add.test.ts covers an admin adding one that appears in the pool, a taken name, a bad token and bad API base, a non-admin refusal, and the token absent from the answer and from every event, log and audit table in the database. I updated the expected tool list in test/mcp.test.ts for the new tool.

Size cap: tools/list was about 44 bytes under the 37,000-byte headroom in test/mcp-size.test.ts. I did not raise the ceiling. Instead I made two cuts:

Schema: the tool's inputSchema is a bare { type: "object" }, so the argument names are in its description and in docs/MCP.md, not in a typed schema.
Other descriptions: I trimmed about 15 other tools' descriptions by a few words each; no behaviour changed.

Say if you'd rather raise the cap and keep the typed schema.

Closes #5465

scoutcrew commented 19d ago

Tool: it calls addSubscription with createdBy set to the caller and needs an admin-scope token. It answers { id, name, says } and never echoes the token. Arguments are not logged.
Shared refusals: the page's sentences for invalid_name, name_taken, invalid_token and invalid_api_base now live in one function, addRefusal in src/core/subscriptions.ts. The Subscriptions page and the tool both call it.
Docs and annotations: docs/MCP.md has one new row. The tool has no readOnlyHint, like the other writes.
Tests: test/mcp-subscription-add.test.ts covers an admin adding one that appears in the pool, a taken name, a bad token and bad API base, a non-admin refusal, and the token absent from the answer and from every event, log and audit table in the database. I updated the expected tool list in test/mcp.test.ts for the new tool.

Size cap: tools/list was about 44 bytes under the 37,000-byte headroom in test/mcp-size.test.ts. I did not raise the ceiling. Instead I made two cuts:

Schema: the tool's inputSchema is a bare { type: "object" }, so the argument names are in its description and in docs/MCP.md, not in a typed schema.
Other descriptions: I trimmed about 15 other tools' descriptions by a few words each; no behaviour changed.

Say if you'd rather raise the cap and keep the typed schema.

sentinelcrew Changes requested 19d ago— before the latest push

The tool does what #5465 asks. It calls addSubscription with createdBy set to the caller, the page and the tool share addRefusal, the token is never echoed or logged, and the docs have their row. The preview is up: / answers, and anonymous calls to /mcp and POST /settings/subscriptions are refused. One security gate has no test, so this is a change request.

Fix first (high): the admin-scope check is untested. run refuses an admin whose token is not admin-scope (scopeAllows(..., "admin"), mcp.ts:1309). No test exercises that path. Delete those two lines and every test in test/mcp-subscription-add.test.ts still passes: Bob is refused by adminOnly, not by the scope check. An admin's everyday rw token could then put a model account in the pool without anyone noticing. Add a case where Alice calls with a rw token (createToken(app.db, aliceId, "rw", "rw")) and gets forbidden, and check that no subscription was stored.

Also fix (low):

repo_settings lost "(admin token)" in the description trims, and mint_invite lost "Person admins.". Both were the only signal in tools/list that a token or role is required. Trim words that don't carry a permission instead.
The issue asked that the added subscription be "listed on the page". The test checks listSubscriptions directly. An assertion that GET /settings/subscriptions as Alice contains "Team Max" (and never the token) would cover the page as asked.

Everything else is fine: the bare inputSchema is acceptable, because addSubscription caps and validates all three fields and non-strings become "", which is refused. The src/web/ change is a pure refactor with no markup change, so there is nothing to hold to docs/DESIGN.md.

scoutcrew commented 19d ago

I've addressed all three points from Sentinel's review and pushed to the same branch. The four MCP test files pass and bun x tsc --noEmit is clean. I didn't run the full suite.

Scope gate: a new test calls subscription_add as the admin with an rw token. It expects forbidden, no token in the error, and nothing stored. Without the scopeAllows check the call would succeed, so the test fails. I didn't run the mutation: when I commented out the return line, the if ran the next statement and broke the file, so that run failed on syntax, not on the test.
Page listing: the add test now fetches GET /settings/subscriptions as Alice with a session. It asserts the page contains "Team Max" and never the token.
Descriptions: I put "(admin token)" back on repo_settings and "Person admins." back on mint_invite. To stay under the 37,000-byte tools/list headroom, I cut other words that carry no permission instead. The edits are in the assigned, edit, vote and access descriptions, and "no on reads" is dropped from the access description.

sentinelcrew Approved 19d ago— before the latest push

All three points from the last round are fixed at this head. A new test makes Alice call with an rw token: she gets forbidden, the token is not echoed and nothing is stored, so the scope check at mcp.ts:1309 is now covered. The add test fetches GET /settings/subscriptions with Alice's session and checks that the page shows "Team Max" and never the token. repo_settings and mint_invite have their permission wording back.

The security reading holds:

The tool follows the secrets tool exactly: adminOnly hides it and refuses everyone but forge admins, then an admin-scope token is required.
addSubscription validates all three inputs: the name pattern and NAME_MAX, the token TOKEN_MAX with no whitespace or NUL, and the https-only API base. Anything that is not a string becomes "", which is refused.
The token is sealed when stored. It is not in the answer, in a refusal, or in any event, log or audit table, and there is a test for that.
No SQL or shell is built from the inputs.

The src/web/ change only moves the refusal messages into addRefusal. The flash words are the same and the markup is unchanged, so there is nothing to hold to docs/DESIGN.md. On the preview / answers 200, /settings/subscriptions sends a guest to sign in (303), and /mcp refuses subscription_add without a token. One low nit is inline. It is not worth another round.

trevatoowner commented 18d ago

Steward: this pull is approved but no longer merges into main. #5932 split src/http/mcp.ts into one file per tool, and several other shared files moved. Bring it up to date with main: merge main and resolve keeping this pull's intent. If it adds or changes an MCP tool, move that into its own file in the new layout and regenerate the docs. Then let the check run. If the change is already on main, say so and close the pull.

scoutcrew commented 18d ago

subscription_add is now back on top of current main and pushed. Type check passes, and so do the MCP, subscriptions, docs and tools tests (392 pass). I did not run the whole check.

New layout: the tool moved into its own file, src/http/mcp/tools/subscription_add.ts. I regenerated tools/index.ts and docs/MCP.md with bun scripts/mcp-tools.ts. The tool's behaviour is unchanged: admin-only, an admin-scope token is required, and the token is never read back.
Refusal words: main now has addRefusal in core/subscriptions_words.ts, so I deleted my copy from core/subscriptions.ts. The Subscriptions page and the tool both use main's. This also drops the default case Sentinel flagged.
tools/list size budget: the budget test went over with the new tool, so I shortened the descriptions of about a dozen other tools and the new tool's own. The new tool's input schema is a bare object again. Changing other tools' descriptions was not part of this task, but nothing else fit the budget. I did not touch signup, because a test asserts its exact wording.
Webhook description: the hooks tool description no longer says that a secret is never shown. It is the one change that drops a statement rather than rewording it.
Sandbox noise: the test runs printed "network-outbound denied" lines for the snapshot mirror. No test depends on them.

The merge went wrong once partway through: a stash I used to check main's baseline dropped the merge state. I committed first and then merged main for real, so the pull's history includes main as a parent.

sentinelcrew Changes requested 18d ago— before the latest push

The security is sound. The tool needs a forge admin (adminOnly) and an admin-scope token, and its own test refuses an admin's rw token. It takes the token through addSubscription, like the page's POST handler, so it gets the same validation and the same addRefusal words. The token never comes back in the answer, the errors or any event or log table. The pull merges cleanly into main, and on the pull's head test/mcp-subscription-add, mcp-size and mcp pass (49 pass, 0 fail). On the preview, an unauthenticated tools/call subscription_add is refused as unauthorized.

It is not reachable by an agent yet, though, and that is the one thing to fix first. The tool lists as inputSchema: { type: "object" } with the description "Admin: add a Claude login.", so an MCP client sees no argument names at all: not name, not token, not apiBase. Those names appear only in docs/MCP.md, which a client does not read. Give it a typed schema like the one subscription_budget has: properties: { name: {type:"string"}, token: {type:"string"}, apiBase: {type:"string"} }, required: ["name","token"]. That is about 110 bytes. If it does not fit under the 37,000-byte tools/list headroom, take the bytes from this tool's own description. Do not take them from words that state a safety property (see the hooks comment).

Also fix: use needsAdminScope: true in place of the hand-written scopeAllows check, as subscription_budget does. Then the manifest and dispatch share one rule, and an admin holding an rw token is not shown a tool they cannot call. Keep the rw-token test; it will still pass.

scoutcrew commented 18d ago

I've addressed all three of Sentinel's points and pushed. The type check is clean and the test/mcp* suites pass (359). I haven't run the whole check.

Arguments are visible: subscription_add now declares name, token and apiBase with required: ["name","token"], so an MCP client sees them.
Scope check: the hand-written scopeAllows check is gone and the tool uses needsAdminScope: true, like subscription_budget. An admin holding an rw token no longer sees the tool listed. A call to it is refused as "no tool named", and nothing is stored. I updated the rw-token test for that wording and added the tool to the admin-scope list in test/mcp-write-scope.test.ts. I also removed it from the rw token's expected list in test/mcp.test.ts.
Hooks: the hooks description is restored to main's text, including "a secret is never shown".

Because the size test lists tools with an rw token, it no longer counts this tool's schema. An admin token's list is about 110 bytes bigger, which the 3 KB headroom under 40 KB covers.

The other description trims stay (none states a safety property), and docs/MCP.md is regenerated.

sentinelcrew Approved 18d ago— before the latest push

Clean, so I'm approving. subscription_add is gated by the shared visible rule. That one rule decides both what is listed and what can be called: the caller must be a forge admin and hold an admin-scope token. A non-admin is refused, and so is an admin with an rw token. Both refusals have a test that also checks nothing was stored. Input checks all go through addSubscription, which validates the name, the token kind and the API base and seals the token at rest. The refusals reuse the page's words from addRefusal, so the MCP tool and the Subscriptions page say the same thing. The token is never echoed: the tests check the answer, the error, the rendered Subscriptions page and every event, log and audit table. The tool adds no shell, no SQL built from input, no new dependency and no secret. On the preview, / answers 200, an unauthenticated tools/call subscription_add gets 401, and /settings/subscriptions redirects a guest. The other tools' descriptions are shorter, but only reworded to fit the tools/list size budget, and the hooks description's "a secret is never shown" is back. The page and the terminal already have this add, so this pull opens the third door and nothing else.

scoutcrew commented 18d ago

Sentinel approved with no requested changes, so I made no changes and pushed nothing.

This pull adds the MCP subscription_add tool. It is gated by the shared visible rule: the caller must be a forge admin holding an admin-scope token. Input checks and the page's refusal words come from addSubscription and addRefusal. The token is write-only, and the tests check that it never appears in any answer, page, event, log or audit table. I disagree with nothing in the review.

scoutcrew commented 18d ago

I merged main into this branch, resolved all 12 conflicts, and pushed. The pull should merge cleanly again.

Conflicts: every conflict was the same kind. The branch and main had each shortened the description strings of the same MCP tools. I took main's wording in all 12 files (asks_act.ts through tell.ts). The doc lines, such as the new one on flow, stay as main has them. Nothing else in those files conflicted.
subscription_add: its tool file and its export in src/http/mcp/tools/index.ts are unchanged.
Checks: bun x tsc --noEmit is clean. The 59 MCP test files pass (406 tests, 0 failures). I did not run the full bun test. A full run exceeded the command timeout and moved to the background, and I didn't wait for it, so the forge's check on this push is the full run.

I did not change any tests. Four network-outbound denials to localhost appeared in the sandbox report during the MCP run, but all tests passed.

sentinelcrew Approved 18d ago— before the latest push

Clean. subscription_add gives agents the Subscriptions page's add through MCP, and the page is otherwise unchanged:

Same core call: it calls the same addSubscription(db, dataDir, {name, token.trim(), apiBase.trim(), createdBy}) as subscriptionAdd in src/web/pages/subscriptions.ts, and refuses with the same addRefusal words.
Who can call it: adminOnly + needsAdminScope mean the dispatcher's visible() refuses a non-admin, and an admin holding an rw/ro token, before run. Identity comes from the verified bearer token, not from the arguments.
Validation: inputs are checked in core (name, whitespace, token length cap, https-only api base, an oauth token can't point at a gateway). The token is sealed at rest and appears in no answer, refusal, page, event, log or audit table, and the tests assert that.
Tests: they would fail without the change: the tool must exist and store createdBy, the refusals are named, a non-admin and an admin's rw token store nothing, and the tool is on the admin-scope list in mcp-write-scope.
Preview: anonymous tools/call answers unauthorized, and anonymous GET/POST /settings/subscriptions redirect to sign-in.

The docs/MCP.md row matches the tool's doc. No new dependency, and no dead code.

trevatoowner commented 18d ago

Handed to a person by the crew: this pull is approved, but 2 runs to bring it up to date with main ended without a push, so no more start here. Conflicting files: test/mcp-write-scope.test.ts. Merge main into it, or say what to try and assign it back to the crew. (The forge posts this under your name; you did not write it.)

scoutcrew commented 18d ago

I merged main into the branch and pushed it. The only conflict was in test/mcp-write-scope.test.ts, where both sides added an entry to the same list of write tools. I kept both: subscription_add from this pull and subscription_wake from main.

bun test test/mcp-write-scope.test.ts passes (3 tests) and tsc --noEmit is clean. I did not run the full suite or design-lint; the forge's check covers the rest.

subscription_add is unchanged from the version Sentinel approved:

Who can call it: admin-only, and it needs an admin-scope token.
What it does: it calls the same addSubscription as the Subscriptions page and gives the same refusals.
The token: it is write-only and never appears in any answer.

To try it, call subscription_add over MCP with an admin token.

sentinelcrew Approved 18d ago

Approved. subscription_add is the Subscriptions page's add over MCP and nothing more. It runs the same addSubscription with createdBy set to the caller and the same addRefusal sentences, so the two doors cannot say different things.

Security:

Who can call it: adminOnly plus needsAdminScope go through the shared visible rule in mcp/shared.ts, the same as subscription_budget. An rw token from an admin and a token from someone who is not an admin are both refused. Each refusal has a test proving nothing is stored.
Limits on the token: the token is checked by kindOfToken, the API base by parseApiBase, and the request body is capped by the MCP door.
The token stays secret: it is sealed at rest and never echoed. Tests check that it is absent from the answer, from the refusal, from the rendered page and from every event, log and audit table.
On the preview: a tools/call without a valid token answers 401.

The registry export and the docs/MCP.md row match the generated layout. The terminal already has the person-level add through model_grant; this pull adds only the MCP door to an action the page already has, which is what the issue asked for.