From d95d60e138a3f0c84193cef9e0da85eb8afc2328 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Fri, 23 Oct 2026 13:59:46 -0700 Subject: [PATCH 01/12] docs(adr): specify confined host GitHub broker path Co-authored-by: kai Agent-Model: codex/gpt-6-sol/medium Agent-Model: codex/gpt-6.1-sol/medium --- ...ADR-028-work-claims-and-decision-ledger.md | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/docs/adr/ADR-028-work-claims-and-decision-ledger.md b/docs/adr/ADR-028-work-claims-and-decision-ledger.md index 8a93db0de7..b2badf94a7 100644 --- a/docs/adr/ADR-028-work-claims-and-decision-ledger.md +++ b/docs/adr/ADR-028-work-claims-and-decision-ledger.md @@ -640,6 +640,54 @@ cannot be tied to the acts they authorised records intent, not accountability. install (the removed route's own note argues the latter). That is a build decision with a real cost difference and it should be ratified, not assumed — added as ratification point 7. +#### Shared-host Codex seat application (TASK-284, 2026-10-23) + +Connector chose a broker for private Codex seats on the shared Mac (decision +`6adbc2b48522fc8abce606e5`). This settles the **host-seat path only**; it does +not settle D13's general per-user App question. The old measurement above is +time-stamped: the current `toolBrokerService` already resolves a connected +GitHub App row from a room grant, calls `GitHubAppService` with `forceApp`, checks +the grant again on each call, and writes `ToolCall` records. Reuse that backend +boundary instead of adding a second host-held GitHub credential. + +The local daemon owns one broker socket per running seat. It creates the socket +outside the Codex sandbox and passes only its path to a confined `gh` shim; the +shim has no fallback to the host's real `gh`. The daemon binds the socket to its +own `(agent user, installation, runtime token)` tuple, never to a seat name or +grant ID supplied by the shim. It forwards a structured request to the backend +with that runtime token; `agentRuntimeAuth` must reject a revoked or expired +token, and the broker must resolve an active room grant, its target pod and its +connected GitHub App row on **every** operation. The shim and Codex shell never +receive the runtime token, the App token, the operator's `gh` config, or a +Keychain handle. A removed seat, grant, connection, or pod membership refuses +the next request. No generic URL, repository, method, header, or `gh api` +passthrough is part of this interface. + +The initial command grammar is exact: `pr create`, `pr view`, `pr comment`, +`pr review --comment`, and `issue comment`. The backend checks the grant's +read/write mode and the connection's pinned owner/repo, validates command +arguments, and returns bounded response fields. `pr review --approve` and +`--request-changes`, `pr merge/close`, issue close, branch deletion, arbitrary +API calls, and unknown flags refuse before any GitHub call. This is an +allowlist, not a set of denials layered on the present `seat-gh-guard` (which +explicitly says it is not a security boundary). The existing App tool broker +may supply operations it already implements; missing PR operations must be +added there with the same grant and audit checks, never through `routes/github`'s +legacy `anyAuth` path. + +Git push is a separate smart-HTTP channel on the same socket, forwarded by the +backend to the grant's pinned repository with an App token **only in the +backend-to-GitHub request**. It accepts one non-delete, non-force update to a +seat-owned `refs/heads/` prefix, bounds the pack size, and refuses extra refs, +tags and protected branches before forwarding. The child Git process gets no +credential helper that returns a token. The backend records a `ToolCall`-style +receipt containing seat, grant, repo, operation, branch/ref, argument digest, +outcome, and time; never token or pack bytes. Apply per-runtime-token and +per-grant limits (30 reads, 10 writes, 2 pushes per minute) before the provider +call, with a bounded request body. Refusals and upstream failures retain +distinct codes. A real staging walk must branch, test, push and open a PR +through this path before TASK-280 can roll out. + --- ## Sequencing and acceptance (Sam, 2026-08-31 — pod 61474) From 7764197ddc01747b33a568a285c2eca53d50c8e1 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Fri, 23 Oct 2026 14:05:16 -0700 Subject: [PATCH 02/12] docs(adr): define host broker isolation and push rules Co-authored-by: kai Agent-Model: codex/gpt-6-sol/medium Agent-Model: codex/gpt-6.1-sol/medium --- ...ADR-028-work-claims-and-decision-ledger.md | 56 +++++++++++++++---- 1 file changed, 44 insertions(+), 12 deletions(-) diff --git a/docs/adr/ADR-028-work-claims-and-decision-ledger.md b/docs/adr/ADR-028-work-claims-and-decision-ledger.md index b2badf94a7..c336580d62 100644 --- a/docs/adr/ADR-028-work-claims-and-decision-ledger.md +++ b/docs/adr/ADR-028-work-claims-and-decision-ledger.md @@ -663,6 +663,19 @@ Keychain handle. A removed seat, grant, connection, or pod membership refuses the next request. No generic URL, repository, method, header, or `gh api` passthrough is part of this interface. +Each listener lives under a daemon-owned directory that the seat cannot read, +write, enumerate, or replace. The Codex permission profile grants **connect +only** to that seat's exact socket path, denies every sibling socket and daemon +control/config path, and cannot write or rebind its own endpoint. A path's +secrecy and same-UID Unix permissions are not authentication. The daemon derives +the tuple from trusted launch state and binds a fresh, unguessable listener +generation to the running child; stopping or replacing that child closes and +unlinks the old listener before a successor is admitted. If the native profile +cannot enforce this exact-path separation, the host-seat broker must refuse to +start. The acceptance walk uses two real confined, same-UID seats: A can use its +own socket, but A cannot connect to B's **known** path before any backend call; +repeat after B restarts and against its stale endpoint. + The initial command grammar is exact: `pr create`, `pr view`, `pr comment`, `pr review --comment`, and `issue comment`. The backend checks the grant's read/write mode and the connection's pinned owner/repo, validates command @@ -675,18 +688,37 @@ may supply operations it already implements; missing PR operations must be added there with the same grant and audit checks, never through `routes/github`'s legacy `anyAuth` path. -Git push is a separate smart-HTTP channel on the same socket, forwarded by the -backend to the grant's pinned repository with an App token **only in the -backend-to-GitHub request**. It accepts one non-delete, non-force update to a -seat-owned `refs/heads/` prefix, bounds the pack size, and refuses extra refs, -tags and protected branches before forwarding. The child Git process gets no -credential helper that returns a token. The backend records a `ToolCall`-style -receipt containing seat, grant, repo, operation, branch/ref, argument digest, -outcome, and time; never token or pack bytes. Apply per-runtime-token and -per-grant limits (30 reads, 10 writes, 2 pushes per minute) before the provider -call, with a bounded request body. Refusals and upstream failures retain -distinct codes. A real staging walk must branch, test, push and open a PR -through this path before TASK-280 can roll out. +Git push is a separate smart-HTTP channel on the same socket. It is registered +as `github.push_branch` in the broker catalogue, and goes through the same +exact-tool, active-grant, pinned-repository and audit checks as the CLI channel. +`read` grants refuse. `write-with-confirm` grants refuse push until a later +implementation can park immutable repo/ref/old-OID/new-OID/pack-digest inputs, +then revalidate them after approval; no provider write occurs on that path now. +Only an explicit `github.push_branch` tool on an autonomous `write` grant can +push. A grant for issue comments never implies push authority. + +The backend accepts one bounded receive-pack request, parses the **complete** +command framing before a provider call, and requires exactly one update to the +authenticated seat's `refs/heads/seat//` prefix. It refuses +deletion, tags, second refs, protected branches, malformed framing and any +client-supplied repo/prefix override. The claimed old OID must equal the +provider's current ref (or zero for a fresh branch). It unpacks into an isolated +quarantine object store and verifies the old commit is an ancestor of the new +commit; it refuses when ancestry cannot be established. A trusted server-side +push then uses a compare-and-swap lease on that same old OID, so a concurrent +ref move fails. The client has no control over this lease or fast-forward check: +the wire protocol does not attest to the client's `--force` choice. The App +token stays only in the backend-to-GitHub request; the child never receives a +credential helper that returns it. Acceptance covers fresh branch and existing +fast-forward success, and divergent update with correct old OID, stale old OID, +second ref, deletion, tag and another seat's prefix refusal. + +The backend records a `ToolCall` receipt containing seat, grant, repo, +operation, branch/ref, argument digest, outcome and time; never token or pack +bytes. Apply per-runtime-token and per-grant limits (30 reads, 10 writes, 2 +pushes per minute) before the provider call, with a bounded request body. +Refusals and upstream failures retain distinct codes. A real staging walk must +branch, test, push and open a PR through this path before TASK-280 can roll out. --- From a5c2c96d94e4d15466211046e857e385e6861ba8 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Fri, 23 Oct 2026 14:13:27 -0700 Subject: [PATCH 03/12] feat(broker): add confined seat GitHub command channel Co-authored-by: kai Agent-Model: codex/gpt-6-sol/medium Agent-Model: codex/gpt-6.1-sol/medium --- .../unit/routes/mcpGrants.hostGh.test.js | 119 +++++++++++++++++ .../unit/services/toolBrokerService.test.js | 78 +++++++++++- backend/routes/mcpGrants.ts | 87 +++++++++++++ backend/services/githubAppService.ts | 34 ++++- backend/services/toolBrokerService.ts | 120 ++++++++++++++++++ cli/__tests__/host-gh-broker.test.mjs | 78 ++++++++++++ .../security/verify-host-gh-socket.mjs | 75 +++++++++++ cli/src/lib/host-gh-broker.js | 117 +++++++++++++++++ cli/src/lib/host-gh-shim.mjs | 94 ++++++++++++++ ...ADR-028-work-claims-and-decision-ledger.md | 14 +- 10 files changed, 810 insertions(+), 6 deletions(-) create mode 100644 backend/__tests__/unit/routes/mcpGrants.hostGh.test.js create mode 100644 cli/__tests__/host-gh-broker.test.mjs create mode 100644 cli/scripts/security/verify-host-gh-socket.mjs create mode 100644 cli/src/lib/host-gh-broker.js create mode 100644 cli/src/lib/host-gh-shim.mjs diff --git a/backend/__tests__/unit/routes/mcpGrants.hostGh.test.js b/backend/__tests__/unit/routes/mcpGrants.hostGh.test.js new file mode 100644 index 0000000000..3503f1c3d3 --- /dev/null +++ b/backend/__tests__/unit/routes/mcpGrants.hostGh.test.js @@ -0,0 +1,119 @@ +const express = require('express'); +const request = require('supertest'); + +const mockCallTool = jest.fn(); +const mockListToolsForGrant = jest.fn(); +const mockReserveBudgetLineage = jest.fn(); +const AGENT_ID = '507f1f77bcf86cd799439011'; + +jest.mock('../../../middleware/agentRuntimeAuth', () => (req, _res, next) => { + req.agentUser = { _id: AGENT_ID, username: 'kai', botMetadata: { agentName: 'kai', instanceId: 'default' } }; + req.agentInstallation = { agentName: 'kai', instanceId: 'default' }; + req.agentTokenHash = 'a'.repeat(64); + next(); +}); +jest.mock('../../../services/toolBrokerService', () => ({ + listToolsForGrant: mockListToolsForGrant, + callTool: mockCallTool, +})); +jest.mock('../../../models/ToolCall', () => ({ reserveBudgetLineage: mockReserveBudgetLineage })); + +// eslint-disable-next-line import/no-unresolved, import/extensions +const router = require('../../../routes/mcpGrants'); + +const app = express(); +app.use(express.json()); +app.use('/api/mcp/grants', router); + +const operations = [ + ['pr.create', 'github.create_pull_request', { title: 'Work', head: `seat/${AGENT_ID}/work`, base: 'main' }], + ['pr.view', 'github.get_pull_request', { pullNumber: 12 }], + ['pr.comment', 'github.comment_on_pull_request', { pullNumber: 12, body: 'A comment' }], + ['pr.review_comment', 'github.review_pull_request_comment', { pullNumber: 12, body: 'A review' }], + ['issue.comment', 'github.comment_on_issue', { issueNumber: 12, body: 'A comment' }], +]; +const refusedOperations = ['pr.merge', 'pr.close', 'issue.close', 'api', 'branch.delete', 'pr.review_approve']; + +beforeEach(() => { + jest.clearAllMocks(); + mockReserveBudgetLineage.mockResolvedValue(true); + mockListToolsForGrant.mockResolvedValue(operations.map(([, name]) => ({ name }))); + mockCallTool.mockResolvedValue({ callId: 'call-1', outcome: 'ok', result: { number: 12 } }); +}); + +describe('host GitHub broker command boundary', () => { + it.each(operations)('maps %s to its fixed broker tool', async (operation, tool, args) => { + const response = await request(app).post('/api/mcp/grants/grant-1/cli').send({ operation, args }); + expect(response.status).toBe(200); + expect(response.body).toMatchObject({ callId: 'call-1', outcome: 'ok' }); + expect(mockCallTool).toHaveBeenCalledWith({ + grantId: 'grant-1', agentUserId: AGENT_ID, agentName: 'kai', instanceId: 'default', tool, args, + }); + expect(mockReserveBudgetLineage).toHaveBeenCalledWith(expect.arrayContaining([ + expect.objectContaining({ calls: operation === 'pr.view' ? 30 : 10, windowMs: 60000 }), + ])); + }); + + it.each(refusedOperations)('refuses %s before grant disclosure or provider call', async (operation) => { + const response = await request(app).post('/api/mcp/grants/grant-1/cli').send({ operation, args: {} }); + expect(response.status).toBe(400); + expect(response.body.error).toBe('operation_not_allowed'); + expect(mockListToolsForGrant).not.toHaveBeenCalled(); + expect(mockCallTool).not.toHaveBeenCalled(); + }); + + it('refuses a PR head outside the authenticated seat prefix', async () => { + const response = await request(app).post('/api/mcp/grants/grant-1/cli').send({ + operation: 'pr.create', args: { title: 'Work', head: 'other/work', base: 'main' }, + }); + expect(response.status).toBe(403); + expect(response.body.error).toBe('branch_not_owned_by_seat'); + expect(mockCallTool).not.toHaveBeenCalled(); + }); + + it.each([ + `seat/${AGENT_ID}/../other`, + `seat/${AGENT_ID}//other`, + `seat/${AGENT_ID}/topic.lock`, + `seat/${AGENT_ID}/topic.`, + `seat/${AGENT_ID}/topic?x`, + ])('refuses malformed seat branch %s before provider call', async (head) => { + const response = await request(app).post('/api/mcp/grants/grant-1/cli').send({ + operation: 'pr.create', args: { title: 'Work', head, base: 'main' }, + }); + expect(response.status).toBe(403); + expect(mockCallTool).not.toHaveBeenCalled(); + }); + + it('refuses an operation missing from this seat grant before budget use', async () => { + mockListToolsForGrant.mockResolvedValue([{ name: 'github.get_pull_request' }]); + const response = await request(app).post('/api/mcp/grants/grant-1/cli').send({ + operation: 'issue.comment', args: { issueNumber: 12, body: 'Comment' }, + }); + expect(response.status).toBe(403); + expect(response.body.error).toBe('operation_not_granted'); + expect(mockReserveBudgetLineage).not.toHaveBeenCalled(); + expect(mockCallTool).not.toHaveBeenCalled(); + }); + + it('refuses an exhausted token or grant budget', async () => { + mockReserveBudgetLineage.mockResolvedValue(false); + const response = await request(app).post('/api/mcp/grants/grant-1/cli').send({ + operation: 'pr.view', args: { pullNumber: 12 }, + }); + expect(response.status).toBe(429); + expect(mockCallTool).not.toHaveBeenCalled(); + }); + + it('returns approval state without executing another operation', async () => { + mockCallTool.mockRejectedValue(Object.assign(new Error('pending'), { + code: 'approval_required', details: { approvalId: 'approval-1' }, + })); + const response = await request(app).post('/api/mcp/grants/grant-1/cli').send({ + operation: 'pr.create', args: { title: 'Work', head: `seat/${AGENT_ID}/work`, base: 'main' }, + }); + expect(response.status).toBe(202); + expect(response.body).toEqual({ status: 'pending_approval', approvalId: 'approval-1' }); + expect(mockCallTool).toHaveBeenCalledTimes(1); + }); +}); diff --git a/backend/__tests__/unit/services/toolBrokerService.test.js b/backend/__tests__/unit/services/toolBrokerService.test.js index 2019f3b11d..21cf0aa30b 100644 --- a/backend/__tests__/unit/services/toolBrokerService.test.js +++ b/backend/__tests__/unit/services/toolBrokerService.test.js @@ -1,3 +1,5 @@ +const { createHash } = require('crypto'); + const mockRoomGrant = { findOne: jest.fn() }; const mockPod = { findById: jest.fn() }; const mockIntegration = { findOne: jest.fn(), findById: jest.fn() }; @@ -8,6 +10,8 @@ const mockGithub = { createIssue: jest.fn(), addIssueComment: jest.fn(), closeIssue: jest.fn(), + createPullRequest: jest.fn(), + createPullReview: jest.fn(), }; const mockReserveBudgetLineage = jest.fn(); const mockDmService = { getOrCreateAgentRoom: jest.fn() }; @@ -70,13 +74,14 @@ jest.mock('../../../services/roomGrantService', () => { }); // eslint-disable-next-line import/no-unresolved, import/extensions -const { callTool } = require('../../../services/toolBrokerService'); +const { callTool, executeApprovedToolCall } = require('../../../services/toolBrokerService'); // eslint-disable-next-line import/no-unresolved, import/extensions const { assertGrantUsable, getGrantLineage, RoomGrantError } = require('../../../services/roomGrantService'); // The row a grant was minted for always predates the grant; the guard compares // these two, so both fixtures carry the field the models always write. const GRANT_CREATED_AT = new Date('2026-01-02T00:00:00.000Z'); +const digest = (args) => createHash('sha256').update(JSON.stringify(args)).digest('hex'); const seatGrant = (overrides = {}) => ({ grantId: 'grant-1', @@ -505,3 +510,74 @@ describe('a suspended github-app row owner (§7)', () => { expect(mockUser.findById).not.toHaveBeenCalled(); }); }); + +describe('host GitHub command tools', () => { + const cases = [ + { + tool: 'github.create_pull_request', + args: { title: 'Change', head: 'seat/agent-a/change', base: 'main' }, + method: 'createPullRequest', + result: { number: 12, title: 'Change', html_url: 'https://github.com/Team-Commonly/commonly/pull/12' }, + expected: { title: 'Change', head: 'seat/agent-a/change', base: 'main' }, + }, + { + tool: 'github.comment_on_pull_request', + args: { pullNumber: 12, body: 'Please check this' }, + method: 'addIssueComment', + result: { id: 42, html_url: 'https://github.com/Team-Commonly/commonly/pull/12#issuecomment-42' }, + expected: { issueNumber: 12, body: 'Please check this' }, + }, + { + tool: 'github.review_pull_request_comment', + args: { pullNumber: 12, body: 'Inline feedback' }, + method: 'createPullReview', + result: { id: 43, html_url: 'https://github.com/Team-Commonly/commonly/pull/12#pullrequestreview-43' }, + expected: { pullNumber: 12, body: 'Inline feedback', event: 'COMMENT' }, + }, + ]; + + it.each(cases)('$tool parks before provider use, then executes only with App auth', async ({ + tool, args, method, result, expected, + }) => { + mockRoomGrant.findOne.mockResolvedValue(seatGrant({ tools: [tool], writeMode: 'write' })); + mockGithub[method].mockResolvedValue(result); + await expect(callTool({ + grantId: 'grant-1', + agentUserId: 'agent-a', + tool, + args, + })) + .rejects.toMatchObject({ code: 'approval_required' }); + expect(mockGithub[method]).not.toHaveBeenCalled(); + + const approvedArgs = { ...args, owner: 'Team-Commonly', repo: 'commonly' }; + await expect(executeApprovedToolCall({ + grantId: 'grant-1', + agentUserId: 'agent-a', + tool, + args: approvedArgs, + expectedArgsDigest: digest(approvedArgs), + approvalId: 'approval-test', + })).resolves.toMatchObject({ outcome: 'ok' }); + expect(mockGithub[method]).toHaveBeenCalledWith(expect.objectContaining({ + owner: 'Team-Commonly', + repo: 'commonly', + installationId: 'gh-install-1', + forceApp: true, + ...expected, + })); + }); + + it('refuses an ungranted PR write before provider use', async () => { + mockRoomGrant.findOne.mockResolvedValue(seatGrant({ + tools: ['github.get_pull_request'], writeMode: 'write', + })); + await expect(callTool({ + grantId: 'grant-1', + agentUserId: 'agent-a', + tool: 'github.create_pull_request', + args: { title: 'Change', head: 'seat/agent-a/change', base: 'main' }, + })).rejects.toMatchObject({ code: 'tool_not_allowed' }); + expect(mockGithub.createPullRequest).not.toHaveBeenCalled(); + }); +}); diff --git a/backend/routes/mcpGrants.ts b/backend/routes/mcpGrants.ts index 1dfd38d9ee..b9b167bd88 100644 --- a/backend/routes/mcpGrants.ts +++ b/backend/routes/mcpGrants.ts @@ -1,7 +1,9 @@ import express from 'express'; import rateLimit from 'express-rate-limit'; +import { createHash } from 'crypto'; import { callTool, listToolsForGrant } from '../services/toolBrokerService'; import { untrustedConnectorMarker } from '../services/brokerResultMarker'; +import { reserveBudgetLineage } from '../models/ToolCall'; // The SDK is CommonJS-compatible, but its package exports use subpath entry // points. Requiring them here keeps this route compatible with the backend's @@ -43,6 +45,91 @@ const errorPayload = (error: unknown): Record => { }; }; +// This host-seat surface is deliberately smaller than the grant's MCP tool +// catalogue. The shim sends one of these operation names, never a gh argv or +// arbitrary GitHub URL. An agent cannot reach merge/close through this route. +const HOST_GH_OPERATIONS = { + 'pr.create': { tool: 'github.create_pull_request', tier: 'write' }, + 'pr.view': { tool: 'github.get_pull_request', tier: 'read' }, + 'pr.comment': { tool: 'github.comment_on_pull_request', tier: 'write' }, + 'pr.review_comment': { tool: 'github.review_pull_request_comment', tier: 'write' }, + 'issue.comment': { tool: 'github.comment_on_issue', tier: 'write' }, +} as const; + +const validSeatBranch = (head: unknown, agentUserId: string): boolean => ( + typeof head === 'string' + && head.startsWith(`seat/${agentUserId}/`) + && head.length <= 200 + && head.split('/').every((segment) => segment.length > 0 + && /^[A-Za-z0-9][A-Za-z0-9._-]*$/.test(segment) + && !segment.includes('..') && !segment.endsWith('.lock') && !segment.endsWith('.')) +); + +/** A token and grant both consume a slot before a GitHub call. */ +const reserveHostGhCall = async (tokenHash: string, grantId: string, tier: 'read' | 'write'): Promise => { + const digest = createHash('sha256').update(grantId).digest('hex'); + const calls = tier === 'read' ? 30 : 10; + return reserveBudgetLineage([ + { grantId: `host-gh:${tier}:token:${tokenHash}`, calls, windowMs: 60_000 }, + { grantId: `host-gh:${tier}:grant:${digest}`, calls, windowMs: 60_000 }, + ]); +}; + +/** No body or status ever carries a provider credential. */ +router.post('/:grantId/cli', brokerRateLimit, agentRuntimeAuth, async (req: express.Request, res: express.Response) => { + const agent = req.agentUser; + const agentUserId = String(agent?._id || ''); + if (!agentUserId || !req.agentTokenHash) return res.status(401).json({ error: 'agent_identity_required' }); + const body = req.body; + if (!body || typeof body !== 'object' || Array.isArray(body) + || Object.keys(body).some((key) => key !== 'operation' && key !== 'args') + || Buffer.byteLength(JSON.stringify(body), 'utf8') > 64 * 1024) { + return res.status(400).json({ error: 'invalid_request' }); + } + const operation = HOST_GH_OPERATIONS[body.operation as keyof typeof HOST_GH_OPERATIONS]; + if (!operation) return res.status(400).json({ error: 'operation_not_allowed' }); + const args = body.args; + if (!args || typeof args !== 'object' || Array.isArray(args)) { + return res.status(400).json({ error: 'invalid_tool_args' }); + } + if (body.operation === 'pr.create' && !validSeatBranch(args.head, agentUserId)) { + return res.status(403).json({ error: 'branch_not_owned_by_seat' }); + } + try { + const installation = req.agentInstallation; + const identity = { + grantId: String(req.params.grantId), + agentUserId, + agentName: installation?.agentName || agent?.botMetadata?.agentName || agent?.username, + instanceId: installation?.instanceId || agent?.botMetadata?.instanceId || 'default', + }; + const listed = await listToolsForGrant(identity); + if (!listed.some((definition) => definition.name === operation.tool)) { + return res.status(403).json({ error: 'operation_not_granted' }); + } + if (!await reserveHostGhCall(req.agentTokenHash, String(req.params.grantId), operation.tier)) { + return res.status(429).json({ error: 'host_github_rate_limited' }); + } + const result = await callTool({ + ...identity, + tool: operation.tool, + args, + }); + const data = result.result && typeof result.result === 'object' && !Array.isArray(result.result) + ? result.result as Record : {}; + // A PR body is untrusted and can be arbitrarily long. Preserve only a + // bounded preview; the operation receipt is always small. + const response = { ...data, ...(typeof data.body === 'string' ? { body: data.body.slice(0, 32 * 1024) } : {}) }; + return res.json({ callId: result.callId, outcome: result.outcome, result: response }); + } catch (error) { + const e = error as { statusCode?: number; code?: string; details?: { approvalId?: string } }; + if (e.code === 'approval_required') { + return res.status(202).json({ status: 'pending_approval', approvalId: e.details?.approvalId || null }); + } + return res.status(typeof e.statusCode === 'number' ? e.statusCode : 500).json(errorPayload(error)); + } +}); + /** * Stateless MCP endpoint. The bearer agent token authenticates the caller; * the grant id in the URL supplies capabilities. No credential or grant diff --git a/backend/services/githubAppService.ts b/backend/services/githubAppService.ts index bbdb3adbba..9f3287d33b 100644 --- a/backend/services/githubAppService.ts +++ b/backend/services/githubAppService.ts @@ -94,6 +94,20 @@ interface PullReviewOptions { pullNumber: number; event: PullReviewEvent; body?: string; + installationId?: string; + forceApp?: boolean; +} + +interface CreatePullRequestOptions { + owner?: string; + repo?: string; + title: string; + head: string; + base: string; + body?: string; + draft?: boolean; + installationId?: string; + forceApp?: boolean; } /** @@ -279,6 +293,20 @@ class GitHubAppService { return res.data; } + /** Create a pull request without returning the App credential to the seat. */ + static async createPullRequest({ + owner = 'Team-Commonly', repo = 'commonly', title, head, base, body, draft, + installationId, forceApp, + }: CreatePullRequestOptions): Promise { + const headers = await this._apiHeaders(undefined, installationId, forceApp); + const res = await axios.post( + `https://api.github.com/repos/${owner}/${repo}/pulls`, + { title, head, base, ...(body !== undefined ? { body } : {}), ...(draft !== undefined ? { draft } : {}) }, + { headers }, + ); + return res.data; + } + /** List files changed by one pull request. */ static async listPullRequestFiles({ owner = 'Team-Commonly', repo = 'commonly', pullNumber, installationId, forceApp }: PullRequestOptions): Promise { const headers = await this._apiHeaders(undefined, installationId, forceApp); @@ -330,8 +358,10 @@ class GitHubAppService { * `event` is one of APPROVE | REQUEST_CHANGES | COMMENT. GitHub requires a * non-empty `body` for REQUEST_CHANGES and COMMENT (the route enforces this). */ - static async createPullReview({ owner = 'Team-Commonly', repo = 'commonly', pullNumber, event, body }: PullReviewOptions): Promise { - const headers = await this._apiHeaders(); + static async createPullReview({ + owner = 'Team-Commonly', repo = 'commonly', pullNumber, event, body, installationId, forceApp, + }: PullReviewOptions): Promise { + const headers = await this._apiHeaders(undefined, installationId, forceApp); const payload: Record = { event }; if (body) payload.body = body; const res = await axios.post( diff --git a/backend/services/toolBrokerService.ts b/backend/services/toolBrokerService.ts index c7f6a70a37..3c678eb7b9 100644 --- a/backend/services/toolBrokerService.ts +++ b/backend/services/toolBrokerService.ts @@ -307,6 +307,123 @@ const getPullRequest: GithubToolDefinition = { }, }; +const createPullRequest: GithubToolDefinition = { + name: 'github.create_pull_request', + description: 'Open a pull request in the connected GitHub repository.', + requiredWriteMode: 'write-with-confirm', + connectionType: 'github-app', + irreversible: true, + receipt: [{ field: 'number', kind: 'positive-integer' }], + inputSchema: { + type: 'object', + properties: { + title: { type: 'string', minLength: 1, maxLength: 256 }, + head: { type: 'string', minLength: 1, maxLength: 200 }, + base: { type: 'string', minLength: 1, maxLength: 200 }, + body: { type: 'string', maxLength: 65536 }, + draft: { type: 'boolean' }, + }, + required: ['title', 'head', 'base'], + additionalProperties: false, + }, + async prepareApproval(rawArgs, connection) { + return pinConnectionRepository(rawArgs, connection); + }, + async call(rawArgs, connection) { + assertNoUnknown(rawArgs, ['title', 'head', 'base', 'body', 'draft']); + const title = nonEmptyString(rawArgs.title, 'title'); + const head = nonEmptyString(rawArgs.head, 'head'); + const base = nonEmptyString(rawArgs.base, 'base'); + if (title.length > 256 || !/^[A-Za-z0-9][A-Za-z0-9._/-]{0,199}$/.test(head) + || !/^[A-Za-z0-9][A-Za-z0-9._/-]{0,199}$/.test(base)) { + throw new RoomGrantError('invalid_tool_args', 'title or branch is invalid', 400); + } + if (rawArgs.body !== undefined && (typeof rawArgs.body !== 'string' || rawArgs.body.length > 65536)) { + throw new RoomGrantError('invalid_tool_args', 'body must be a string of at most 65536 characters', 400); + } + if (rawArgs.draft !== undefined && typeof rawArgs.draft !== 'boolean') { + throw new RoomGrantError('invalid_tool_args', 'draft must be boolean', 400); + } + const pull = await GitHubAppService.createPullRequest({ + owner: connection.owner, + repo: connection.repo, + installationId: connection.installationId, + forceApp: true, + title, + head, + base, + body: rawArgs.body as string | undefined, + draft: rawArgs.draft as boolean | undefined, + }); + return pullView(pull as Record); + }, +}; + +const commentPullRequest: GithubToolDefinition = { + name: 'github.comment_on_pull_request', + description: 'Add a plain conversation comment to a pull request in the connected GitHub repository.', + requiredWriteMode: 'write-with-confirm', + connectionType: 'github-app', + irreversible: true, + inputSchema: { + type: 'object', + properties: { pullNumber: { type: 'integer', minimum: 1 }, body: { type: 'string', minLength: 1, maxLength: 65536 } }, + required: ['pullNumber', 'body'], + additionalProperties: false, + }, + async prepareApproval(rawArgs, connection) { + return pinConnectionRepository(rawArgs, connection); + }, + async call(rawArgs, connection) { + assertNoUnknown(rawArgs, ['pullNumber', 'body']); + const pullNumber = positiveInteger(rawArgs.pullNumber, 'pullNumber'); + const body = nonEmptyString(rawArgs.body, 'body'); + if (body.length > 65536) throw new RoomGrantError('invalid_tool_args', 'body is too long', 400); + const comment = await GitHubAppService.addIssueComment({ + owner: connection.owner, + repo: connection.repo, + installationId: connection.installationId, + forceApp: true, + issueNumber: pullNumber, + body, + }) as Record; + return { id: comment.id, url: comment.html_url, pullNumber }; + }, +}; + +const reviewPullRequestComment: GithubToolDefinition = { + name: 'github.review_pull_request_comment', + description: 'Submit a comment-only review of a pull request in the connected GitHub repository.', + requiredWriteMode: 'write-with-confirm', + connectionType: 'github-app', + irreversible: true, + inputSchema: { + type: 'object', + properties: { pullNumber: { type: 'integer', minimum: 1 }, body: { type: 'string', minLength: 1, maxLength: 65536 } }, + required: ['pullNumber', 'body'], + additionalProperties: false, + }, + async prepareApproval(rawArgs, connection) { + return pinConnectionRepository(rawArgs, connection); + }, + async call(rawArgs, connection) { + assertNoUnknown(rawArgs, ['pullNumber', 'body']); + const pullNumber = positiveInteger(rawArgs.pullNumber, 'pullNumber'); + const body = nonEmptyString(rawArgs.body, 'body'); + if (body.length > 65536) throw new RoomGrantError('invalid_tool_args', 'body is too long', 400); + const review = await GitHubAppService.createPullReview({ + owner: connection.owner, + repo: connection.repo, + installationId: connection.installationId, + forceApp: true, + pullNumber, + event: 'COMMENT', + body, + }) as Record; + return { id: review.id, url: review.html_url, pullNumber }; + }, +}; + /** * A review needs the code, not just the file names: each file's `patch` (its * unified-diff hunks, as GitHub returns them) rides along, capped so one large @@ -529,6 +646,9 @@ export const TOOL_DEFINITIONS: Record = { [listIssues.name]: listIssues, [getIssue.name]: getIssue, [getPullRequest.name]: getPullRequest, + [createPullRequest.name]: createPullRequest, + [commentPullRequest.name]: commentPullRequest, + [reviewPullRequestComment.name]: reviewPullRequestComment, [listPullRequestFiles.name]: listPullRequestFiles, [createIssue.name]: createIssue, [commentIssue.name]: commentIssue, diff --git a/cli/__tests__/host-gh-broker.test.mjs b/cli/__tests__/host-gh-broker.test.mjs new file mode 100644 index 0000000000..6d1e1e831d --- /dev/null +++ b/cli/__tests__/host-gh-broker.test.mjs @@ -0,0 +1,78 @@ +import { afterEach, beforeEach, expect, it, jest } from '@jest/globals'; +import { execFile } from 'child_process'; +import { mkdtemp, rm } from 'fs/promises'; +import { connect } from 'net'; +import { tmpdir } from 'os'; +import { join } from 'path'; +import { promisify } from 'util'; +import { startSeatGhBroker } from '../src/lib/host-gh-broker.js'; + +const execFileAsync = promisify(execFile); +const TOKEN = `cm_agent_${'T'.repeat(64)}`; +let testRoot; +let broker; +let fetchImpl; + +beforeEach(async () => { + testRoot = await mkdtemp(join(tmpdir(), 'commonly-host-gh-test-')); + fetchImpl = jest.fn().mockResolvedValue(new Response(JSON.stringify({ + callId: 'call-1', outcome: 'ok', result: { number: 7 }, + }), { status: 200, headers: { 'Content-Type': 'application/json' } })); + broker = await startSeatGhBroker({ + runtimeToken: TOKEN, instanceUrl: 'https://api.example.invalid', root: testRoot, fetchImpl, + }); +}); + +afterEach(async () => { + await broker?.close(); + await rm(testRoot, { recursive: true, force: true }); +}); + +const run = (...argv) => execFileAsync(broker.shimPath, argv, { + env: { ...process.env, COMMONLY_GH_SOCKET: broker.socketPath, COMMONLY_AGENT_TOKEN: '' }, + timeout: 10000, +}); + +it('forwards a fixed operation and the daemon-owned bearer, never a URL or token from the shim', async () => { + const { stdout } = await run('pr', 'view', '7', '--grant-id', 'grant-1'); + expect(JSON.parse(stdout).result.number).toBe(7); + expect(fetchImpl).toHaveBeenCalledTimes(1); + const [url, options] = fetchImpl.mock.calls[0]; + expect(String(url)).toBe('https://api.example.invalid/api/mcp/grants/grant-1/cli'); + expect(options.headers.Authorization).toBe(`Bearer ${TOKEN}`); + expect(JSON.parse(options.body)).toEqual({ operation: 'pr.view', args: { pullNumber: 7 } }); +}); + +it.each([ + [['pr', 'create', '--title', 'Change', '--head', 'seat/agent-a/change', '--base', 'main', '--draft'], + 'pr.create', { title: 'Change', head: 'seat/agent-a/change', base: 'main', draft: true }], + [['pr', 'comment', '7', '--body', 'A note'], 'pr.comment', { pullNumber: 7, body: 'A note' }], + [['pr', 'review', '7', '--comment', '--body', 'Review note'], + 'pr.review_comment', { pullNumber: 7, body: 'Review note' }], + [['issue', 'comment', '8', '--body', 'Issue note'], 'issue.comment', { issueNumber: 8, body: 'Issue note' }], +])('accepts only the listed %j grammar', async (argv, operation, args) => { + await run(...argv, '--grant-id', 'grant-1'); + expect(JSON.parse(fetchImpl.mock.calls[0][1].body)).toEqual({ operation, args }); +}); + +it.each([ + ['pr', 'merge', '7'], + ['pr', 'review', '7', '--approve'], + ['api', 'repos/Team-Commonly/commonly'], + ['pr', 'create', '--title', 'X', '--head', 'seat/a/x', '--base', 'main', '--repo', 'other/repo'], +])('refuses unlisted argv before contacting the backend', async (...argv) => { + await expect(run(...argv, '--grant-id', 'grant-1')).rejects.toMatchObject({ code: 2 }); + expect(fetchImpl).not.toHaveBeenCalled(); +}); + +it('rejects a removed seat endpoint after teardown', async () => { + const socketPath = broker.socketPath; + await broker.close(); + broker = null; + await expect(new Promise((resolve, reject) => { + const socket = connect(socketPath); + socket.on('connect', resolve); + socket.on('error', reject); + })).rejects.toMatchObject({ code: 'ENOENT' }); + expect(fetchImpl).not.toHaveBeenCalled(); +}); diff --git a/cli/scripts/security/verify-host-gh-socket.mjs b/cli/scripts/security/verify-host-gh-socket.mjs new file mode 100644 index 0000000000..d3b535a207 --- /dev/null +++ b/cli/scripts/security/verify-host-gh-socket.mjs @@ -0,0 +1,75 @@ +#!/usr/bin/env node +/** Real macOS permission-profile probe; no provider credentials or model turn. */ +import { execFile } from 'child_process'; +import { mkdtemp, rm } from 'fs/promises'; +import { tmpdir } from 'os'; +import { join } from 'path'; +import { promisify } from 'util'; +import { startSeatGhBroker } from '../../src/lib/host-gh-broker.js'; + +const runFile = promisify(execFile); +const root = await mkdtemp(join(tmpdir(), 'commonly-gh-isolation-')); +const calls = []; +const seat = async (name) => startSeatGhBroker({ + runtimeToken: `cm_agent_${name.repeat(64)}`, + instanceUrl: 'https://api.example.invalid', + root, + fetchImpl: async () => { + calls.push(name); + return new Response(JSON.stringify({ callId: name, outcome: 'ok', result: { number: 1 } }), { + status: 200, headers: { 'Content-Type': 'application/json' }, + }); + }, +}); + +const profile = (own, featureOn = true) => [ + '-c', 'default_permissions="host_gh_probe"', + '-c', `permissions.host_gh_probe.filesystem={":minimal"="read","/opt/homebrew"="read",${JSON.stringify(root)}="deny",${JSON.stringify(own.directory)}="read"}`, + '-c', `features.network_proxy=${featureOn}`, + '-c', 'permissions.host_gh_probe.network.enabled=true', + '-c', `permissions.host_gh_probe.network.unix_sockets={${JSON.stringify(own.socketPath)}="allow"}`, +]; + +const connectProbe = 'const n=require("net");const s=n.connect(process.argv[1]);s.on("connect",()=>{console.log("CONNECTED");s.end()});s.on("error",e=>{console.log(e.code);process.exitCode=2})'; +const run = async (profileArgs, command, args = [], env = process.env) => { + try { + const result = await runFile('codex', ['sandbox', ...profileArgs, command, ...args], { + env, timeout: 15000, + }); + return { code: 0, stdout: result.stdout, stderr: result.stderr }; + } catch (error) { + return { code: error.code, stdout: error.stdout || '', stderr: error.stderr || '' }; + } +}; +const expect = (condition, message) => { if (!condition) throw new Error(message); }; + +let a; +let b; +try { + a = await seat('A'); + b = await seat('B'); + const aProfile = profile(a); + const own = await run(aProfile, a.shimPath, + ['pr', 'view', '1', '--grant-id', 'grant-1'], + { ...process.env, COMMONLY_GH_SOCKET: a.socketPath }); + expect(own.code === 0 && calls.length === 1 && calls[0] === 'A', `own socket failed: ${own.stderr}`); + + const other = await run(aProfile, process.execPath, ['-e', connectProbe, b.socketPath]); + expect(other.code === 2 && other.stdout.includes('EPERM') && calls.length === 1, + `cross-seat socket reached: ${other.stdout} ${other.stderr}`); + + const oldPath = b.socketPath; + await b.close(); + b = await seat('B'); + const restarted = await run(aProfile, process.execPath, ['-e', connectProbe, b.socketPath]); + expect(restarted.code === 2 && restarted.stdout.includes('EPERM') && calls.length === 1, + `restarted cross-seat socket reached: ${restarted.stdout} ${restarted.stderr}`); + const stale = await run(aProfile, process.execPath, ['-e', connectProbe, oldPath]); + expect(stale.code !== 0 && calls.length === 1, 'stale socket remained reachable'); + + process.stdout.write('PASS: own call, known sibling, restarted sibling, stale endpoint\n'); +} finally { + await a?.close(); + await b?.close(); + await rm(root, { recursive: true, force: true }); +} diff --git a/cli/src/lib/host-gh-broker.js b/cli/src/lib/host-gh-broker.js new file mode 100644 index 0000000000..b4de334695 --- /dev/null +++ b/cli/src/lib/host-gh-broker.js @@ -0,0 +1,117 @@ +/** + * Per-spawn GitHub command bridge for a confined host seat. The socket carries + * no secret: its trusted owner holds the runtime token and chooses the backend + * URL. The Codex permission profile must allow only this seat's exact socket. + */ +import { createServer } from 'net'; +import { chmod, lstat, mkdir, mkdtemp, readFile, rm, writeFile } from 'fs/promises'; +import { homedir } from 'os'; +import { fileURLToPath } from 'url'; +import { join } from 'path'; + +const MAX_REQUEST_BYTES = 64 * 1024; +const MAX_RESPONSE_BYTES = 128 * 1024; +const SHIM_SOURCE = fileURLToPath(new URL('./host-gh-shim.mjs', import.meta.url)); + +const reply = (socket, status, body) => { + if (!socket.destroyed) socket.end(JSON.stringify({ status, body })); +}; + +export const startSeatGhBroker = async ({ + runtimeToken, instanceUrl, fetchImpl = fetch, root = join(homedir(), '.commonly', 'host-gh'), +}) => { + if (typeof runtimeToken !== 'string' || !runtimeToken.startsWith('cm_agent_')) { + throw new Error('host GitHub broker requires a runtime token'); + } + const endpoint = new URL(instanceUrl); + if (!['https:', 'http:'].includes(endpoint.protocol) || endpoint.username || endpoint.password) { + throw new Error('host GitHub broker requires an HTTP instance URL'); + } + if (endpoint.protocol === 'http:' && !['localhost', '127.0.0.1', '[::1]'].includes(endpoint.hostname)) { + throw new Error('host GitHub broker requires HTTPS outside localhost'); + } + await mkdir(root, { recursive: true, mode: 0o700 }); + const rootStats = await lstat(root); + if (!rootStats.isDirectory() || rootStats.isSymbolicLink()) { + throw new Error('host GitHub broker root must be a real directory'); + } + await chmod(root, 0o700); + const directory = await mkdtemp(join(root, 'seat-')); + const socketPath = join(directory, 'broker.sock'); + const shimPath = join(directory, 'gh'); + let server; + try { + const shimSource = await readFile(SHIM_SOURCE, 'utf8'); + await writeFile(shimPath, `#!${process.execPath}\n${shimSource.replace(/^#![^\n]*\n/, '')}`, { mode: 0o500 }); + await chmod(shimPath, 0o555); + server = createServer({ allowHalfOpen: true }, (socket) => { + let chunks = []; + let size = 0; + socket.setTimeout(15_000, () => socket.destroy()); + socket.on('error', () => {}); + socket.on('data', (chunk) => { + size += chunk.length; + if (size > MAX_REQUEST_BYTES) { + reply(socket, 413, { error: 'request_too_large' }); + chunks = []; + } else { + chunks.push(chunk); + } + }); + socket.on('end', async () => { + if (socket.destroyed || size > MAX_REQUEST_BYTES) return; + let request; + try { request = JSON.parse(Buffer.concat(chunks).toString('utf8')); } catch { + reply(socket, 400, { error: 'invalid_request' }); + return; + } + const { grantId, operation, args } = request || {}; + if (!/^[A-Za-z0-9_-]{1,128}$/.test(grantId) + || typeof operation !== 'string' || !args || typeof args !== 'object' + || Array.isArray(args) || Object.keys(request).some((key) => !['grantId', 'operation', 'args'].includes(key))) { + reply(socket, 400, { error: 'invalid_request' }); + return; + } + try { + const url = new URL(`/api/mcp/grants/${encodeURIComponent(grantId)}/cli`, endpoint); + const response = await fetchImpl(url, { + method: 'POST', + headers: { Authorization: `Bearer ${runtimeToken}`, 'Content-Type': 'application/json' }, + body: JSON.stringify({ operation, args }), + signal: AbortSignal.timeout(15_000), + }); + const bodyText = await response.text(); + if (Buffer.byteLength(bodyText, 'utf8') > MAX_RESPONSE_BYTES) { + reply(socket, 502, { error: 'broker_response_too_large' }); + return; + } + let body; + try { body = JSON.parse(bodyText); } catch { body = { error: 'broker_response_invalid' }; } + reply(socket, response.status, body); + } catch { + reply(socket, 502, { error: 'broker_unavailable' }); + } + }); + }); + await new Promise((resolve, reject) => { + server.once('error', reject); + server.listen(socketPath, () => { + server.off('error', reject); + resolve(); + }); + }); + return { + socketPath, + shimPath, + directory, + async close() { + await new Promise((resolve) => server.close(resolve)); + await rm(directory, { recursive: true, force: true }); + }, + }; + } catch (error) { + if (server?.listening) await new Promise((resolve) => server.close(resolve)); + await rm(directory, { recursive: true, force: true }); + throw error; + } +}; diff --git a/cli/src/lib/host-gh-shim.mjs b/cli/src/lib/host-gh-shim.mjs new file mode 100644 index 0000000000..6b4101ccae --- /dev/null +++ b/cli/src/lib/host-gh-shim.mjs @@ -0,0 +1,94 @@ +#!/usr/bin/env node +/** Narrow gh-compatible entrypoint for confined Commonly host seats. */ +import { connect } from 'net'; + +const fail = (message) => { + process.stderr.write(`${message}\n`); + process.exitCode = 2; +}; + +const parse = (argv) => { + const parts = [...argv]; + const grantFlag = parts.indexOf('--grant-id'); + if (grantFlag < 0 || grantFlag + 1 >= parts.length) throw new Error('pass --grant-id '); + const [grantId] = parts.splice(grantFlag + 1, 1); + parts.splice(grantFlag, 1); + if (!/^[A-Za-z0-9_-]{1,128}$/.test(grantId)) throw new Error('invalid grant id'); + + const [area, action, ...rest] = parts; + let operation; + let requiredFlags = []; + let allowedFlags = []; + let numberField; + if (area === 'pr' && action === 'create') { + operation = 'pr.create'; + requiredFlags = ['--title', '--head', '--base']; + allowedFlags = [...requiredFlags, '--body', '--draft']; + } else if (area === 'pr' && action === 'view') { + operation = 'pr.view'; numberField = 'pullNumber'; + } else if (area === 'pr' && action === 'comment') { + operation = 'pr.comment'; numberField = 'pullNumber'; + requiredFlags = ['--body']; allowedFlags = requiredFlags; + } else if (area === 'pr' && action === 'review') { + operation = 'pr.review_comment'; numberField = 'pullNumber'; + requiredFlags = ['--comment', '--body']; allowedFlags = requiredFlags; + } else if (area === 'issue' && action === 'comment') { + operation = 'issue.comment'; numberField = 'issueNumber'; + requiredFlags = ['--body']; allowedFlags = requiredFlags; + } else { + throw new Error('gh operation is not allowed for this seat'); + } + + const args = {}; + if (numberField) { + const value = rest.shift(); + if (!/^[1-9][0-9]*$/.test(value || '') || !Number.isSafeInteger(Number(value))) { + throw new Error('pass a positive issue or PR number'); + } + args[numberField] = Number(value); + } + const flags = new Set(); + while (rest.length) { + const flag = rest.shift(); + if (!allowedFlags.includes(flag) || flags.has(flag)) throw new Error(`flag is not allowed: ${flag}`); + flags.add(flag); + if (flag === '--draft' || flag === '--comment') continue; + const value = rest.shift(); + if (value === undefined || value.startsWith('--')) throw new Error(`missing value for ${flag}`); + if (value.length > 32 * 1024) throw new Error(`value is too long for ${flag}`); + args[flag.slice(2)] = value; + } + if (requiredFlags.some((flag) => !flags.has(flag))) throw new Error('required flag is missing'); + if (flags.has('--draft')) args.draft = true; + return { grantId, operation, args }; +}; + +const main = async () => { + const request = parse(process.argv.slice(2)); + const socketPath = process.env.COMMONLY_GH_SOCKET; + if (!socketPath || !socketPath.startsWith('/')) throw new Error('seat GitHub broker is unavailable'); + const answer = await new Promise((resolve, reject) => { + const socket = connect(socketPath); + const chunks = []; + let size = 0; + socket.setTimeout(20_000, () => socket.destroy(new Error('seat GitHub broker timed out'))); + socket.on('connect', () => socket.end(JSON.stringify(request))); + socket.on('data', (chunk) => { + size += chunk.length; + if (size > 128 * 1024) socket.destroy(new Error('seat GitHub response is too large')); + else chunks.push(chunk); + }); + socket.on('error', reject); + socket.on('end', () => { + try { resolve(JSON.parse(Buffer.concat(chunks).toString('utf8'))); } catch { + reject(new Error('seat GitHub response is invalid')); + } + }); + }); + if (answer.status < 200 || answer.status >= 300 || answer.status === 202) { + throw new Error(answer.body?.error || answer.body?.status || 'GitHub operation refused'); + } + process.stdout.write(`${JSON.stringify(answer.body, null, 2)}\n`); +}; + +main().catch((error) => fail(error.message)); diff --git a/docs/adr/ADR-028-work-claims-and-decision-ledger.md b/docs/adr/ADR-028-work-claims-and-decision-ledger.md index c336580d62..055dc86e32 100644 --- a/docs/adr/ADR-028-work-claims-and-decision-ledger.md +++ b/docs/adr/ADR-028-work-claims-and-decision-ledger.md @@ -664,9 +664,17 @@ the next request. No generic URL, repository, method, header, or `gh api` passthrough is part of this interface. Each listener lives under a daemon-owned directory that the seat cannot read, -write, enumerate, or replace. The Codex permission profile grants **connect -only** to that seat's exact socket path, denies every sibling socket and daemon -control/config path, and cannot write or rebind its own endpoint. A path's +write, enumerate, or replace. The Codex permission profile sets +`features.network_proxy = true` and +`network.unix_sockets. = "allow"`, leaves +`dangerously_allow_all_unix_sockets = false`, and has no sibling socket entry; +its filesystem rules deny the daemon directory and control/config paths. It +cannot write or rebind its own endpoint. The [Codex permission reference](https://learn.chatgpt.com/docs/permissions) +documents this exact-path Unix socket allowlist. The proxy feature is +load-bearing: a local macOS `codex sandbox` probe connected to a sibling socket +when the feature was off, despite the allowlist, and refused it with `EPERM` +when the feature was on. The two-seat attack test below must repeat that proof +in the real adapter. A path's secrecy and same-UID Unix permissions are not authentication. The daemon derives the tuple from trusted launch state and binds a fresh, unguessable listener generation to the running child; stopping or replacing that child closes and From 9792a63206b8ac9afcef791d2f7f280e4f7e7fed Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Fri, 23 Oct 2026 14:15:51 -0700 Subject: [PATCH 04/12] test(broker): prove Unix socket proxy isolation Co-authored-by: kai Agent-Model: codex/gpt-6-sol/medium Agent-Model: codex/gpt-6.1-sol/medium --- cli/scripts/security/verify-host-gh-socket.mjs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/cli/scripts/security/verify-host-gh-socket.mjs b/cli/scripts/security/verify-host-gh-socket.mjs index d3b535a207..29089b4da5 100644 --- a/cli/scripts/security/verify-host-gh-socket.mjs +++ b/cli/scripts/security/verify-host-gh-socket.mjs @@ -57,6 +57,11 @@ try { const other = await run(aProfile, process.execPath, ['-e', connectProbe, b.socketPath]); expect(other.code === 2 && other.stdout.includes('EPERM') && calls.length === 1, `cross-seat socket reached: ${other.stdout} ${other.stderr}`); + // Mutation control: without the proxy feature, the nominal socket allowlist + // does not confine direct AF_UNIX connects on this macOS build. + const unconfined = await run(profile(a, false), process.execPath, ['-e', connectProbe, b.socketPath]); + expect(unconfined.code === 0 && unconfined.stdout.includes('CONNECTED'), + `expected the no-proxy control to expose B: ${unconfined.stdout} ${unconfined.stderr}`); const oldPath = b.socketPath; await b.close(); @@ -67,7 +72,7 @@ try { const stale = await run(aProfile, process.execPath, ['-e', connectProbe, oldPath]); expect(stale.code !== 0 && calls.length === 1, 'stale socket remained reachable'); - process.stdout.write('PASS: own call, known sibling, restarted sibling, stale endpoint\n'); + process.stdout.write('PASS: own call, known sibling, no-proxy control, restarted sibling, stale endpoint\n'); } finally { await a?.close(); await b?.close(); From 499ea1ed6cad366154095dddc80f86c2e8de5eda Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Fri, 23 Oct 2026 14:24:12 -0700 Subject: [PATCH 05/12] feat(broker): validate confined seat branch pushes Co-authored-by: kai Agent-Model: codex/gpt-6-sol/medium Agent-Model: codex/gpt-6.1-sol/medium --- backend/Dockerfile | 3 +- .../unit/routes/mcpGrants.hostGh.test.js | 35 ++++ .../unit/services/seatGithubPush.test.js | 111 +++++++++++++ .../unit/services/toolBrokerService.test.js | 43 +++++ backend/config/schema.sql | 4 +- backend/models/ToolCall.ts | 20 ++- backend/routes/mcpGrants.ts | 36 ++++ backend/server.ts | 4 + backend/services/seatGithubPush.ts | 155 ++++++++++++++++++ backend/services/toolBrokerService.ts | 58 ++++++- cli/__tests__/git-remote-commonly.test.mjs | 67 ++++++++ cli/src/lib/git-remote-commonly.mjs | 100 +++++++++++ cli/src/lib/host-gh-broker.js | 21 ++- ...ADR-028-work-claims-and-decision-ledger.md | 21 ++- 14 files changed, 654 insertions(+), 24 deletions(-) create mode 100644 backend/__tests__/unit/services/seatGithubPush.test.js create mode 100644 backend/services/seatGithubPush.ts create mode 100644 cli/__tests__/git-remote-commonly.test.mjs create mode 100644 cli/src/lib/git-remote-commonly.mjs diff --git a/backend/Dockerfile b/backend/Dockerfile index 37f76c0536..9ae71126c9 100644 --- a/backend/Dockerfile +++ b/backend/Dockerfile @@ -30,7 +30,7 @@ WORKDIR /app # than a claim of sameness, and `officecli --version` in each container is the # check that keeps it honest. ARG OPENCLAW_OFFICECLI_VERSION=1.0.70 -RUN apt-get update && apt-get install -y --no-install-recommends curl ca-certificates && \ +RUN apt-get update && apt-get install -y --no-install-recommends curl ca-certificates git && \ ARCH="$(uname -m)" && \ case "$ARCH" in \ x86_64) ASSET="officecli-linux-x64" ;; \ @@ -47,6 +47,7 @@ RUN apt-get update && apt-get install -y --no-install-recommends curl ca-certifi rm -f /tmp/officecli-SHA256SUMS && \ chmod +x /usr/local/bin/officecli && \ officecli --version && \ + git --version && \ apt-get clean && rm -rf /var/lib/apt/lists/* COPY package*.json ./ diff --git a/backend/__tests__/unit/routes/mcpGrants.hostGh.test.js b/backend/__tests__/unit/routes/mcpGrants.hostGh.test.js index 3503f1c3d3..8f8b3a3a47 100644 --- a/backend/__tests__/unit/routes/mcpGrants.hostGh.test.js +++ b/backend/__tests__/unit/routes/mcpGrants.hostGh.test.js @@ -117,3 +117,38 @@ describe('host GitHub broker command boundary', () => { expect(mockCallTool).toHaveBeenCalledTimes(1); }); }); + +describe('host GitHub push boundary', () => { + const push = { + ref: `refs/heads/seat/${AGENT_ID}/work`, baseOid: 'a'.repeat(40), + newOid: 'b'.repeat(40), bundle: 'YnVuZGxl', + }; + + it('requires the exact push tool and reserves two token/grant slots', async () => { + mockListToolsForGrant.mockResolvedValue([{ name: 'github.push_branch' }]); + const response = await request(app).post('/api/mcp/grants/grant-1/push').send(push); + expect(response.status).toBe(200); + expect(mockCallTool).toHaveBeenCalledWith(expect.objectContaining({ + grantId: 'grant-1', agentUserId: AGENT_ID, tool: 'github.push_branch', args: push, + })); + expect(mockReserveBudgetLineage).toHaveBeenCalledWith(expect.arrayContaining([ + expect.objectContaining({ calls: 2, windowMs: 60000 }), + ])); + }); + + it('refuses a read-only or omitted push tool before budget use', async () => { + mockListToolsForGrant.mockResolvedValue([{ name: 'github.get_pull_request' }]); + const response = await request(app).post('/api/mcp/grants/grant-1/push').send(push); + expect(response.status).toBe(403); + expect(mockReserveBudgetLineage).not.toHaveBeenCalled(); + expect(mockCallTool).not.toHaveBeenCalled(); + }); + + it('refuses exhausted push budget before any provider call', async () => { + mockListToolsForGrant.mockResolvedValue([{ name: 'github.push_branch' }]); + mockReserveBudgetLineage.mockResolvedValue(false); + const response = await request(app).post('/api/mcp/grants/grant-1/push').send(push); + expect(response.status).toBe(429); + expect(mockCallTool).not.toHaveBeenCalled(); + }); +}); diff --git a/backend/__tests__/unit/services/seatGithubPush.test.js b/backend/__tests__/unit/services/seatGithubPush.test.js new file mode 100644 index 0000000000..78c6eed16e --- /dev/null +++ b/backend/__tests__/unit/services/seatGithubPush.test.js @@ -0,0 +1,111 @@ +const { execFile } = require('child_process'); +const { mkdtemp, rm, writeFile } = require('fs/promises'); +const { tmpdir } = require('os'); +const { join } = require('path'); +const { promisify } = require('util'); +const { parseSeatPushArgs, pushWithLease, validateSeatBundle } = require('../../../services/seatGithubPush'); + +const runFile = promisify(execFile); +const env = { ...process.env, GIT_AUTHOR_NAME: 'Test', GIT_AUTHOR_EMAIL: 'test@example.com', + GIT_COMMITTER_NAME: 'Test', GIT_COMMITTER_EMAIL: 'test@example.com', + GIT_CONFIG_NOSYSTEM: '1', GIT_CONFIG_GLOBAL: '/dev/null', GIT_NO_REPLACE_OBJECTS: '1' }; +const git = async (args, cwd, failure) => { + try { return (await runFile('git', args, { cwd, env })).stdout.trim(); } catch { + throw failure || new Error('git failed'); + } +}; + +describe('seat GitHub push proof', () => { + let root; + let source; + let receiver; + let baseOid; + let oldOid; + let newOid; + let bundlePath; + + beforeEach(async () => { + root = await mkdtemp(join(tmpdir(), 'seat-push-proof-')); + source = join(root, 'source'); + receiver = join(root, 'receiver.git'); + bundlePath = join(root, 'branch.bundle'); + await git(['init', '-b', 'main', source], root); + await writeFile(join(source, 'README.md'), 'base\n'); + await git(['add', '.'], source); + await git(['commit', '-m', 'base'], source); + baseOid = await git(['rev-parse', 'HEAD'], source); + await git(['checkout', '-b', 'seat'], source); + await writeFile(join(source, 'README.md'), 'old\n'); + await git(['commit', '-am', 'old'], source); + oldOid = await git(['rev-parse', 'HEAD'], source); + await writeFile(join(source, 'README.md'), 'new\n'); + await git(['commit', '-am', 'new'], source); + newOid = await git(['rev-parse', 'HEAD'], source); + await git(['bundle', 'create', bundlePath, 'HEAD', `^${baseOid}`], source); + await git(['init', '--bare', receiver], root); + await git(['fetch', source, baseOid], receiver); + await git(['fetch', source, oldOid], receiver); + }); + + afterEach(async () => { await rm(root, { recursive: true, force: true }); }); + + it('accepts an incremental bundle whose old ref is an ancestor', async () => { + await expect(validateSeatBundle({ git, repo: receiver, bundlePath, baseOid, oldOid, newOid })) + .resolves.toBeUndefined(); + }); + + it('uses a zero-OID lease for a fresh branch and refuses a stale provider ref', async () => { + const provider = join(root, 'provider.git'); + const ref = 'refs/heads/seat/agent123/work'; + await git(['init', '--bare', provider], root); + await validateSeatBundle({ git, repo: receiver, bundlePath, baseOid, oldOid: '0'.repeat(40), newOid }); + await pushWithLease({ git, repo: receiver, url: provider, ref, oldOid: '0'.repeat(40), newOid }); + expect(await git(['rev-parse', ref], provider)).toBe(newOid); + await git(['update-ref', ref, oldOid], provider); + await expect(pushWithLease({ + git, repo: receiver, url: provider, ref, oldOid: '0'.repeat(40), newOid, + })).rejects.toMatchObject({ code: 'stale_ref', statusCode: 409 }); + }); + + it('updates an existing branch only while its old OID matches', async () => { + const provider = join(root, 'provider.git'); + const ref = 'refs/heads/seat/agent123/work'; + await git(['init', '--bare', provider], root); + await git(['fetch', source, oldOid], provider); + await git(['update-ref', ref, oldOid], provider); + await validateSeatBundle({ git, repo: receiver, bundlePath, baseOid, oldOid, newOid }); + await pushWithLease({ git, repo: receiver, url: provider, ref, oldOid, newOid }); + expect(await git(['rev-parse', ref], provider)).toBe(newOid); + await expect(pushWithLease({ git, repo: receiver, url: provider, ref, oldOid, newOid: baseOid })) + .rejects.toMatchObject({ code: 'stale_ref' }); + }); + + it('refuses a divergent update even when the old OID is current', async () => { + await git(['checkout', '-b', 'divergent', baseOid], source); + await writeFile(join(source, 'README.md'), 'other\n'); + await git(['commit', '-am', 'other'], source); + const divergentOid = await git(['rev-parse', 'HEAD'], source); + await git(['bundle', 'create', bundlePath, 'HEAD', `^${baseOid}`], source); + await expect(validateSeatBundle({ + git, repo: receiver, bundlePath, baseOid, oldOid, newOid: divergentOid, + })).rejects.toMatchObject({ code: 'non_fast_forward', statusCode: 409 }); + }); + + it('refuses a second advertised ref before unbundling', async () => { + await git(['branch', 'second', oldOid], source); + await git(['bundle', 'create', bundlePath, 'HEAD', 'refs/heads/second', `^${baseOid}`], source); + await expect(validateSeatBundle({ git, repo: receiver, bundlePath, baseOid, oldOid, newOid })) + .rejects.toMatchObject({ code: 'bundle_ref_invalid' }); + }); + + it('binds the ref to the authenticated seat and refuses tags/deletion shapes', () => { + const args = { ref: 'refs/heads/seat/agent123/work', baseOid, newOid, bundle: 'YWJj' }; + expect(parseSeatPushArgs(args, 'agent123')).toEqual(args); + for (const ref of ['refs/heads/main', 'refs/tags/v1', 'refs/heads/seat/other/work', + 'refs/heads/seat/agent123/../main', 'refs/heads/seat/agent123/work.lock']) { + expect(() => parseSeatPushArgs({ ...args, ref }, 'agent123')).toThrow('push_args_invalid'); + } + expect(() => parseSeatPushArgs({ ...args, repository: 'other/repo' }, 'agent123')) + .toThrow('push_args_invalid'); + }); +}); diff --git a/backend/__tests__/unit/services/toolBrokerService.test.js b/backend/__tests__/unit/services/toolBrokerService.test.js index 21cf0aa30b..946aabb190 100644 --- a/backend/__tests__/unit/services/toolBrokerService.test.js +++ b/backend/__tests__/unit/services/toolBrokerService.test.js @@ -14,6 +14,7 @@ const mockGithub = { createPullReview: jest.fn(), }; const mockReserveBudgetLineage = jest.fn(); +const mockPushSeatBranch = jest.fn(); const mockDmService = { getOrCreateAgentRoom: jest.fn() }; jest.mock('../../../models/RoomGrant', () => ({ __esModule: true, default: mockRoomGrant })); @@ -40,6 +41,10 @@ jest.mock('../../../services/seatGrantConfinement', () => ({ judgeSeatConfinement: (...args) => mockSeatConfinement(...args), })); jest.mock('../../../services/githubAppService', () => mockGithub); +jest.mock('../../../services/seatGithubPush', () => ({ + parseSeatPushArgs: jest.fn(), + pushSeatBranch: (...args) => mockPushSeatBranch(...args), +})); jest.mock('../../../services/outsiderGrantPodEligibility', () => ({ outsiderGrantPodRefusal: jest.fn(async () => null), outsiderGrantRefusalMessage: jest.fn((reason) => `Use a separate pod: ${reason}`), @@ -581,3 +586,41 @@ describe('host GitHub command tools', () => { expect(mockGithub.createPullRequest).not.toHaveBeenCalled(); }); }); + +describe('host GitHub push authority', () => { + const args = { + ref: 'refs/heads/seat/agent-a/work', baseOid: 'a'.repeat(40), + newOid: 'b'.repeat(40), bundle: 'YWJj', + }; + + it.each(['read', 'write-with-confirm'])('refuses %s mode before an App push', async (writeMode) => { + mockRoomGrant.findOne.mockResolvedValue(seatGrant({ tools: ['github.push_branch'], writeMode })); + await expect(callTool({ + grantId: 'grant-1', agentUserId: 'agent-a', tool: 'github.push_branch', args, + })).rejects.toMatchObject({ code: 'write_mode_not_allowed' }); + expect(mockPushSeatBranch).not.toHaveBeenCalled(); + }); + + it('requires an explicit push pin even on an autonomous write grant', async () => { + mockRoomGrant.findOne.mockResolvedValue(seatGrant({ tools: ['github.comment_on_issue'], writeMode: 'write' })); + await expect(callTool({ + grantId: 'grant-1', agentUserId: 'agent-a', tool: 'github.push_branch', args, + })).rejects.toMatchObject({ code: 'tool_not_allowed' }); + expect(mockPushSeatBranch).not.toHaveBeenCalled(); + }); + + it('binds the executing seat identity and uses the pinned App repository', async () => { + mockRoomGrant.findOne.mockResolvedValue(seatGrant({ tools: ['github.push_branch'], writeMode: 'write' })); + mockPushSeatBranch.mockResolvedValue({ ref: args.ref, oldOid: '0'.repeat(40), newOid: args.newOid }); + await expect(callTool({ + grantId: 'grant-1', agentUserId: 'agent-a', tool: 'github.push_branch', args, + })).resolves.toMatchObject({ outcome: 'ok' }); + expect(mockPushSeatBranch).toHaveBeenCalledWith(args, expect.objectContaining({ + owner: 'Team-Commonly', repo: 'commonly', installationId: 'gh-install-1', + }), 'agent-a'); + expect(mockToolCall.create).toHaveBeenCalledWith(expect.objectContaining({ + tool: 'github.push_branch', providerRepo: 'Team-Commonly/commonly', + providerRef: args.ref, outcome: 'ok', + })); + }); +}); diff --git a/backend/config/schema.sql b/backend/config/schema.sql index 93d142b6c5..96f63e560e 100644 --- a/backend/config/schema.sql +++ b/backend/config/schema.sql @@ -230,7 +230,9 @@ CREATE TABLE IF NOT EXISTS tool_calls ( -- which is what a call refused before a connection resolved reports. credential_owner_id VARCHAR(255), vendor_status INTEGER, - vendor_error VARCHAR(200) + vendor_error VARCHAR(200), + provider_repo VARCHAR(255), + provider_ref VARCHAR(255) ); CREATE INDEX IF NOT EXISTS idx_tool_calls_grant_at ON tool_calls(grant_id, occurred_at DESC); -- Per-user, per-thread state (W-T, TASK-029). ONE record carrying both diff --git a/backend/models/ToolCall.ts b/backend/models/ToolCall.ts index d6716eb2b9..42ff746a4e 100644 --- a/backend/models/ToolCall.ts +++ b/backend/models/ToolCall.ts @@ -79,6 +79,9 @@ export interface ToolCallRecord { reason?: string; vendorStatus?: number; vendorError?: string; + /** Pinned provider repository and seat branch for a brokered Git push. */ + providerRepo?: string; + providerRef?: string; approvalId?: string; durationMs?: number; } @@ -104,7 +107,9 @@ CREATE TABLE IF NOT EXISTS tool_calls ( duration_ms INTEGER, credential_owner_id VARCHAR(255), vendor_status INTEGER, - vendor_error VARCHAR(200) + vendor_error VARCHAR(200), + provider_repo VARCHAR(255), + provider_ref VARCHAR(255) ); CREATE INDEX IF NOT EXISTS idx_tool_calls_grant_at ON tool_calls(grant_id, occurred_at DESC); -- The table self-bootstraps rather than being part of schema.sql, so an @@ -113,6 +118,8 @@ CREATE INDEX IF NOT EXISTS idx_tool_calls_grant_at ON tool_calls(grant_id, occur ALTER TABLE tool_calls ADD COLUMN IF NOT EXISTS credential_owner_id VARCHAR(255); ALTER TABLE tool_calls ADD COLUMN IF NOT EXISTS vendor_status INTEGER; ALTER TABLE tool_calls ADD COLUMN IF NOT EXISTS vendor_error VARCHAR(200); +ALTER TABLE tool_calls ADD COLUMN IF NOT EXISTS provider_repo VARCHAR(255); +ALTER TABLE tool_calls ADD COLUMN IF NOT EXISTS provider_ref VARCHAR(255); `; let ensured = false; @@ -245,8 +252,8 @@ class ToolCall { `INSERT INTO tool_calls (call_id, grant_id, pod_id, installation_id, agent_user_id, tool, args_digest, occurred_at, outcome, reason, approval_id, duration_ms, - credential_owner_id, vendor_status, vendor_error) - VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15)`, + credential_owner_id, vendor_status, vendor_error, provider_repo, provider_ref) + VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17)`, [ record.callId, record.grantId, @@ -263,6 +270,8 @@ class ToolCall { record.credentialOwnerId || null, record.vendorStatus ?? null, record.vendorError || null, + record.providerRepo || null, + record.providerRef || null, ], ); } @@ -273,7 +282,8 @@ class ToolCall { `SELECT tc.call_id, tc.grant_id, tc.pod_id, tc.installation_id, tc.agent_user_id, tc.tool, tc.args_digest, tc.occurred_at, tc.credential_owner_id, ${EFFECTIVE_OUTCOME} AS effective_outcome, - tc.reason, tc.approval_id, tc.duration_ms, tc.vendor_status, tc.vendor_error + tc.reason, tc.approval_id, tc.duration_ms, tc.vendor_status, tc.vendor_error, + tc.provider_repo, tc.provider_ref FROM tool_calls tc LEFT JOIN ${RESOLVED_SIBLINGS} ON resolved.grant_id = tc.grant_id AND resolved.approval_id = tc.approval_id @@ -296,6 +306,8 @@ class ToolCall { vendorStatus: row.vendor_status === null || row.vendor_status === undefined ? undefined : Number(row.vendor_status), vendorError: row.vendor_error ? String(row.vendor_error) : undefined, + providerRepo: row.provider_repo ? String(row.provider_repo) : undefined, + providerRef: row.provider_ref ? String(row.provider_ref) : undefined, approvalId: row.approval_id ? String(row.approval_id) : undefined, durationMs: row.duration_ms === null || row.duration_ms === undefined ? undefined : Number(row.duration_ms), diff --git a/backend/routes/mcpGrants.ts b/backend/routes/mcpGrants.ts index b9b167bd88..6eb96fefd3 100644 --- a/backend/routes/mcpGrants.ts +++ b/backend/routes/mcpGrants.ts @@ -75,6 +75,42 @@ const reserveHostGhCall = async (tokenHash: string, grantId: string, tier: 'read ]); }; +router.post('/:grantId/push', brokerRateLimit, agentRuntimeAuth, async ( + req: express.Request, res: express.Response, +) => { + const agent = req.agentUser; + const agentUserId = String(agent?._id || ''); + if (!agentUserId || !req.agentTokenHash) return res.status(401).json({ error: 'agent_identity_required' }); + const args = req.body; + if (!args || typeof args !== 'object' || Array.isArray(args) + || Object.keys(args).some((key) => !['ref', 'baseOid', 'newOid', 'bundle'].includes(key)) + || Buffer.byteLength(JSON.stringify(args), 'utf8') > 9 * 1024 * 1024) { + return res.status(400).json({ error: 'invalid_push_request' }); + } + const installation = req.agentInstallation; + const identity = { + grantId: String(req.params.grantId), + agentUserId, + agentName: installation?.agentName || agent?.botMetadata?.agentName || agent?.username, + instanceId: installation?.instanceId || agent?.botMetadata?.instanceId || 'default', + }; + try { + if (!(await listToolsForGrant(identity)).some((tool) => tool.name === 'github.push_branch')) { + return res.status(403).json({ error: 'operation_not_granted' }); + } + const digest = createHash('sha256').update(identity.grantId).digest('hex'); + if (!await reserveBudgetLineage([ + { grantId: `host-gh:push:token:${req.agentTokenHash}`, calls: 2, windowMs: 60_000 }, + { grantId: `host-gh:push:grant:${digest}`, calls: 2, windowMs: 60_000 }, + ])) return res.status(429).json({ error: 'host_github_push_rate_limited' }); + const result = await callTool({ ...identity, tool: 'github.push_branch', args }); + return res.json({ callId: result.callId, outcome: result.outcome, result: result.result }); + } catch (error) { + const e = error as { statusCode?: number }; + return res.status(typeof e.statusCode === 'number' ? e.statusCode : 500).json(errorPayload(error)); + } +}); + /** No body or status ever carries a provider credential. */ router.post('/:grantId/cli', brokerRateLimit, agentRuntimeAuth, async (req: express.Request, res: express.Response) => { const agent = req.agentUser; diff --git a/backend/server.ts b/backend/server.ts index 34ac77daa9..f096837c5f 100644 --- a/backend/server.ts +++ b/backend/server.ts @@ -233,6 +233,10 @@ app.use('/api/webhooks/github', githubGigWebhookRoutes.githubGigRawBody); app.use('/api/webhooks/github', githubGigWebhookRoutes.githubGigRawError); // JSON parsing for all other routes +// The seat push channel carries a bounded incremental Git bundle. Keep its +// larger parser scoped to this one authenticated route; every other JSON API +// retains the default body limit. +app.use('/api/mcp/grants/:grantId/push', express.json({ limit: '9mb' })); app.use(express.json()); // Routes diff --git a/backend/services/seatGithubPush.ts b/backend/services/seatGithubPush.ts new file mode 100644 index 0000000000..3a5213f394 --- /dev/null +++ b/backend/services/seatGithubPush.ts @@ -0,0 +1,155 @@ +/** Trusted half of the confined host seat's push channel. Git runs only in a fresh bare repo. */ +import { execFile } from 'child_process'; +import { mkdtemp, rm, writeFile } from 'fs/promises'; +import { tmpdir } from 'os'; +import { join } from 'path'; +import { promisify } from 'util'; +import type { GithubToolConnection } from './toolBrokerService'; + +// eslint-disable-next-line @typescript-eslint/no-require-imports +const GitHubAppService = require('./githubAppService'); + +const runFile = promisify(execFile); +const OID = /^[0-9a-f]{40}$/; +const ZERO_OID = '0'.repeat(40); +const MAX_BUNDLE_BYTES = 6 * 1024 * 1024; +const safeRepoPart = (part: string) => /^[A-Za-z0-9][A-Za-z0-9_.-]{0,99}$/.test(part) + && !part.includes('..') && !part.endsWith('.'); +const safeRef = (ref: string, agentUserId: string) => ( + ref.startsWith(`refs/heads/seat/${agentUserId}/`) + && ref.length <= 220 + && ref.split('/').every((part) => safeRepoPart(part) && !part.endsWith('.lock')) +); + +export class SeatPushError extends Error { + constructor(public code: string, public statusCode = 403) { super(code); } +} + +interface PushArgs { + ref: string; + baseOid: string; + newOid: string; + bundle: string; +} + +/** Validate the wire shape before spending a provider call or writing a temp file. */ +export const parseSeatPushArgs = (raw: Record, agentUserId: string): PushArgs => { + if (Object.keys(raw).some((key) => !['ref', 'baseOid', 'newOid', 'bundle'].includes(key))) { + throw new SeatPushError('push_args_invalid', 400); + } + const { ref, baseOid, newOid, bundle } = raw; + if (typeof ref !== 'string' || !safeRef(ref, agentUserId) + || typeof baseOid !== 'string' || !OID.test(baseOid) + || typeof newOid !== 'string' || !OID.test(newOid) + || typeof bundle !== 'string' || bundle.length > Math.ceil((MAX_BUNDLE_BYTES * 4) / 3) + 4 + || !/^[A-Za-z0-9+/]*={0,2}$/.test(bundle)) { + throw new SeatPushError('push_args_invalid', 400); + } + return { ref, baseOid, newOid, bundle }; +}; + +/** Purely local Git proof after the provider objects have been fetched. */ +export const validateSeatBundle = async (options: { + git: (args: string[], cwd?: string, failure?: SeatPushError) => Promise; + repo: string; + bundlePath: string; + baseOid: string; + newOid: string; + oldOid: string; +}): Promise => { + const { git, repo, bundlePath, baseOid, newOid, oldOid } = options; + await git(['cat-file', '-e', `${baseOid}^{commit}`], repo, + new SeatPushError('bundle_base_not_in_provider', 400)); + const heads = await git(['bundle', 'list-heads', bundlePath]); + const advertised = heads.split('\n').filter(Boolean); + if (advertised.length !== 1 || advertised[0].split(/\s+/, 1)[0] !== newOid) { + throw new SeatPushError('bundle_ref_invalid', 400); + } + await git(['bundle', 'verify', bundlePath], repo, new SeatPushError('bundle_invalid', 400)); + await git(['bundle', 'unbundle', bundlePath], repo, new SeatPushError('bundle_invalid', 400)); + await git(['cat-file', '-e', `${newOid}^{commit}`], repo, new SeatPushError('bundle_invalid', 400)); + await git(['fsck', '--no-reflogs', '--connectivity-only', newOid], repo, + new SeatPushError('bundle_invalid', 400)); + await git(['merge-base', '--is-ancestor', baseOid, newOid], repo, + new SeatPushError('non_fast_forward', 409)); + if (oldOid !== ZERO_OID) await git(['merge-base', '--is-ancestor', oldOid, newOid], repo, + new SeatPushError('non_fast_forward', 409)); +}; + +/** Provider CAS is independent of client push flags or advertised remote state. */ +export const pushWithLease = async (options: { + git: (args: string[], cwd?: string, failure?: SeatPushError) => Promise; + repo: string; + url: string; + ref: string; + oldOid: string; + newOid: string; +}): Promise => { + const { git, repo, url, ref, oldOid, newOid } = options; + try { + await git(['push', '--porcelain', `--force-with-lease=${ref}:${oldOid === ZERO_OID ? '' : oldOid}`, + url, `${newOid}:${ref}`], repo); + } catch { + const latest = await git(['ls-remote', url, ref], repo); + const latestOid = latest ? latest.split(/\s+/, 1)[0] : ZERO_OID; + if (latestOid !== oldOid) throw new SeatPushError('stale_ref', 409); + throw new SeatPushError('provider_git_failed', 502); + } +}; + +/** All subprocess settings are backend-owned. The bundle controls no Git config, hook or remote. */ +export const pushSeatBranch = async ( + raw: Record, connection: GithubToolConnection, agentUserId: string, +): Promise<{ ref: string; oldOid: string; newOid: string }> => { + const { ref, baseOid, newOid, bundle } = parseSeatPushArgs(raw, agentUserId); + if (!safeRepoPart(connection.owner) || !safeRepoPart(connection.repo)) { + throw new SeatPushError('repository_invalid'); + } + const bytes = Buffer.from(bundle, 'base64'); + if (!bytes.length || bytes.length > MAX_BUNDLE_BYTES || bytes.toString('base64') !== bundle) { + throw new SeatPushError('bundle_invalid', 400); + } + const directory = await mkdtemp(join(tmpdir(), 'seat-git-push-')); + const repo = join(directory, 'repo.git'); + const bundlePath = join(directory, 'input.bundle'); + const url = `https://github.com/${connection.owner}/${connection.repo}.git`; + try { + const { token } = await GitHubAppService.getInstallationToken(connection.installationId); + const basic = Buffer.from(`x-access-token:${token}`).toString('base64'); + const env = { + PATH: '/usr/bin:/bin', HOME: directory, GIT_CONFIG_NOSYSTEM: '1', + GIT_CONFIG_GLOBAL: '/dev/null', GIT_NO_REPLACE_OBJECTS: '1', GIT_TERMINAL_PROMPT: '0', + GIT_CONFIG_COUNT: '1', GIT_CONFIG_KEY_0: `http.${url}.extraheader`, + GIT_CONFIG_VALUE_0: `AUTHORIZATION: basic ${basic}`, + }; + const git = async (args: string[], cwd = repo, failure?: SeatPushError) => { + try { + const answer = await runFile('/usr/bin/git', args, { + cwd, env, timeout: 45_000, maxBuffer: 64 * 1024, + }); + return answer.stdout.trim(); + } catch { + // Git stderr may contain URL or credential-helper output. No provider + // diagnostic is reflected to the untrusted seat. + throw failure || new SeatPushError('provider_git_failed', 502); + } + }; + await git(['init', '--bare', repo], directory); + const defaultHead = await git(['ls-remote', '--symref', url, 'HEAD']); + const defaultRef = defaultHead.match(/^ref: (refs\/heads\/[A-Za-z0-9._/-]+)\s+HEAD/m)?.[1]; + if (!defaultRef) throw new SeatPushError('provider_default_branch_missing', 502); + const current = await git(['ls-remote', url, ref]); + const oldOid = current ? current.split(/\s+/, 1)[0] : ZERO_OID; + if (!OID.test(oldOid)) throw new SeatPushError('provider_ref_invalid', 502); + await git(['fetch', '--no-tags', url, defaultRef]); + if (oldOid !== ZERO_OID && oldOid !== baseOid) await git(['fetch', '--no-tags', url, ref]); + await writeFile(bundlePath, bytes, { mode: 0o600 }); + await validateSeatBundle({ git, repo, bundlePath, baseOid, newOid, oldOid }); + // The server, not the child, chooses the compare-and-swap lease. A ref + // move between ls-remote and this push causes a refusal upstream. + await pushWithLease({ git, repo, url, ref, oldOid, newOid }); + return { ref, oldOid, newOid }; + } finally { + await rm(directory, { recursive: true, force: true }); + } +}; diff --git a/backend/services/toolBrokerService.ts b/backend/services/toolBrokerService.ts index 3c678eb7b9..346fa543e9 100644 --- a/backend/services/toolBrokerService.ts +++ b/backend/services/toolBrokerService.ts @@ -15,6 +15,7 @@ import { judgeSeatConfinement } from './seatGrantConfinement'; import { findHostedToolDefinition, hostedToolDefinitions } from './hostedMcpToolDefinitions'; import { HostedMcpCredentialError } from './hostedMcpCredentialError'; import { outsiderGrantPodRefusal, outsiderGrantRefusalMessage } from './outsiderGrantPodEligibility'; +import { parseSeatPushArgs, pushSeatBranch } from './seatGithubPush'; // eslint-disable-next-line @typescript-eslint/no-require-imports const GitHubAppService = require('./githubAppService'); @@ -61,7 +62,7 @@ export interface GithubToolDefinition extends ToolDefinitionBase { args: Record, connection: GithubToolConnection, ) => Promise>; - call: (args: Record, connection: GithubToolConnection) => Promise; + call: (args: Record, connection: GithubToolConnection, agentUserId?: string) => Promise; } /** @@ -359,6 +360,29 @@ const createPullRequest: GithubToolDefinition = { }, }; +const pushSeatBranchDefinition: GithubToolDefinition = { + name: 'github.push_branch', + description: 'Push one fast-forward seat branch to the connected GitHub repository.', + requiredWriteMode: 'write', + connectionType: 'github-app', + inputSchema: { + type: 'object', + properties: { + ref: { type: 'string', maxLength: 220 }, + baseOid: { type: 'string', pattern: '^[0-9a-f]{40}$' }, + newOid: { type: 'string', pattern: '^[0-9a-f]{40}$' }, + bundle: { type: 'string', maxLength: 8388612 }, + }, + required: ['ref', 'baseOid', 'newOid', 'bundle'], + additionalProperties: false, + }, + async call(args, connection, agentUserId) { + if (!agentUserId) throw new RoomGrantError('agent_identity_required', 'seat identity is required', 403); + parseSeatPushArgs(args, agentUserId); + return pushSeatBranch(args, connection, agentUserId); + }, +}; + const commentPullRequest: GithubToolDefinition = { name: 'github.comment_on_pull_request', description: 'Add a plain conversation comment to a pull request in the connected GitHub repository.', @@ -367,7 +391,10 @@ const commentPullRequest: GithubToolDefinition = { irreversible: true, inputSchema: { type: 'object', - properties: { pullNumber: { type: 'integer', minimum: 1 }, body: { type: 'string', minLength: 1, maxLength: 65536 } }, + properties: { + pullNumber: { type: 'integer', minimum: 1 }, + body: { type: 'string', minLength: 1, maxLength: 65536 }, + }, required: ['pullNumber', 'body'], additionalProperties: false, }, @@ -399,7 +426,10 @@ const reviewPullRequestComment: GithubToolDefinition = { irreversible: true, inputSchema: { type: 'object', - properties: { pullNumber: { type: 'integer', minimum: 1 }, body: { type: 'string', minLength: 1, maxLength: 65536 } }, + properties: { + pullNumber: { type: 'integer', minimum: 1 }, + body: { type: 'string', minLength: 1, maxLength: 65536 }, + }, required: ['pullNumber', 'body'], additionalProperties: false, }, @@ -647,6 +677,7 @@ export const TOOL_DEFINITIONS: Record = { [getIssue.name]: getIssue, [getPullRequest.name]: getPullRequest, [createPullRequest.name]: createPullRequest, + [pushSeatBranchDefinition.name]: pushSeatBranchDefinition, [commentPullRequest.name]: commentPullRequest, [reviewPullRequestComment.name]: reviewPullRequestComment, [listPullRequestFiles.name]: listPullRequestFiles, @@ -1112,11 +1143,12 @@ const runDefinition = ( definition: ToolDefinition, args: Record, connection: ToolConnection, + agentUserId: string, ): Promise => { const provider = providerConnection(connection); return definition.connectionType === 'hosted-mcp' ? definition.call(args, asHostedConnection(provider)) - : definition.call(args, asGithubConnection(provider)); + : definition.call(args, asGithubConnection(provider), agentUserId); }; /** @@ -1212,6 +1244,7 @@ const recordCall = async ( * their id on the error (see `refusedCredentialOwner`). */ credentialOwnerId?: string; + providerRepo?: string; vendorStatus?: number; vendorError?: string; }, @@ -1231,6 +1264,11 @@ const recordCall = async ( reason, vendorStatus: overrides?.vendorStatus, vendorError: overrides?.vendorError?.split(/[\r\n]/, 1)[0].slice(0, 200), + providerRepo: overrides?.providerRepo, + providerRef: input.tool === 'github.push_branch' + && typeof (input.args as { ref?: unknown })?.ref === 'string' + && /^refs\/heads\/[A-Za-z0-9._/-]{1,220}$/.test((input.args as { ref: string }).ref) + ? (input.args as { ref: string }).ref : undefined, approvalId: overrides?.approvalId, durationMs: Math.max(0, Date.now() - startedAt), }); @@ -1272,6 +1310,7 @@ const recordExecutionResult = async ( approvalId?: string; args?: unknown; credentialOwnerId?: string; + providerRepo?: string; }, ): Promise<{ callId: string; outcome: 'ok' | 'failed' }> => { const upstreamToolError = isToolErrorResult(result); @@ -1316,6 +1355,7 @@ export const callTool = async (input: BrokerCallInput): Promise 0 && !await reserveBudgetLineage(budgetEntries)) { throw new RoomGrantError('budget_exhausted', 'grant call budget is exhausted', 403); } - const result = await runDefinition(definition, executionArgs, connection); + const result = await runDefinition(definition, executionArgs, connection, input.agentUserId); const recorded = await recordExecutionResult( { grantId: input.grantId, agentUserId: input.agentUserId, tool: input.tool, args: input.args }, grant, diff --git a/cli/__tests__/git-remote-commonly.test.mjs b/cli/__tests__/git-remote-commonly.test.mjs new file mode 100644 index 0000000000..60c0e6356a --- /dev/null +++ b/cli/__tests__/git-remote-commonly.test.mjs @@ -0,0 +1,67 @@ +import { execFile } from 'child_process'; +import { mkdtemp, rm, writeFile } from 'fs/promises'; +import { join } from 'path'; +import { promisify } from 'util'; +import { startSeatGhBroker } from '../src/lib/host-gh-broker.js'; + +const runFile = promisify(execFile); +const run = (cwd, args, env = process.env) => runFile('git', args, { cwd, env, timeout: 15_000 }); + +describe('confined Git remote helper', () => { + let root; + let repo; + let broker; + let forwarded; + const seatRef = 'refs/heads/seat/agent123/feature'; + + beforeEach(async () => { + // macOS AF_UNIX paths have a small byte limit; use a short test root. + root = await mkdtemp('/tmp/hgr-'); + repo = join(root, 'clone'); + await run(root, ['init', '-b', 'main', repo]); + const env = { ...process.env, GIT_AUTHOR_NAME: 'Test', GIT_AUTHOR_EMAIL: 'test@example.com', + GIT_COMMITTER_NAME: 'Test', GIT_COMMITTER_EMAIL: 'test@example.com' }; + await writeFile(join(repo, 'README.md'), 'base\n'); + await run(repo, ['add', '.'], env); + await run(repo, ['commit', '-m', 'base'], env); + const { stdout } = await run(repo, ['rev-parse', 'HEAD'], env); + await run(repo, ['update-ref', 'refs/remotes/origin/main', stdout.trim()], env); + await writeFile(join(repo, 'README.md'), 'change\n'); + await run(repo, ['commit', '-am', 'change'], env); + forwarded = []; + broker = await startSeatGhBroker({ + runtimeToken: 'cm_agent_test', instanceUrl: 'http://localhost:5000', root: join(root, 'broker'), + fetchImpl: async (url, request) => { + forwarded.push({ url: String(url), request: JSON.parse(request.body) }); + return { status: 200, text: async () => JSON.stringify({ result: { ref: seatRef } }) }; + }, + }); + }); + + afterEach(async () => { + await broker?.close(); + await rm(root, { recursive: true, force: true }); + }); + + const seatEnv = () => ({ ...process.env, PATH: `${broker.directory}:${process.env.PATH}`, + COMMONLY_GH_SOCKET: broker.socketPath }); + + it('sends one incremental bundle and no credential to the helper', async () => { + await run(repo, ['push', 'commonly::grant_123', `HEAD:${seatRef}`], seatEnv()); + expect(forwarded).toHaveLength(1); + expect(forwarded[0].url).toMatch(/\/api\/mcp\/grants\/grant_123\/push$/); + expect(forwarded[0].request.ref).toBe(seatRef); + expect(forwarded[0].request.baseOid).toMatch(/^[0-9a-f]{40}$/); + expect(forwarded[0].request.newOid).toMatch(/^[0-9a-f]{40}$/); + expect(Buffer.from(forwarded[0].request.bundle, 'base64').subarray(0, 7).toString()).toBe('# v2 gi'); + expect(JSON.stringify(forwarded)).not.toContain('cm_agent_test'); + }); + + it('refuses a protected branch and multiple refs before forwarding', async () => { + await expect(run(repo, ['push', 'commonly::grant_123', 'HEAD:refs/heads/main'], seatEnv())) + .rejects.toThrow(); + await expect(run(repo, ['push', 'commonly::grant_123', `HEAD:${seatRef}`, 'HEAD:refs/heads/main'], seatEnv())) + .rejects.toThrow(); + expect(forwarded).toHaveLength(0); + }); +}); diff --git a/cli/src/lib/git-remote-commonly.mjs b/cli/src/lib/git-remote-commonly.mjs new file mode 100644 index 0000000000..db96ad89e1 --- /dev/null +++ b/cli/src/lib/git-remote-commonly.mjs @@ -0,0 +1,100 @@ +#!/usr/bin/env node +/** Push-only Git remote helper for a confined host seat. No provider credential enters this process. */ +import { spawn } from 'child_process'; +import { connect } from 'net'; +import { mkdtemp, readFile, rm } from 'fs/promises'; +import { createInterface } from 'readline'; +import { join } from 'path'; + +const MAX_BUNDLE_BYTES = 6 * 1024 * 1024; +const OID = /^[0-9a-f]{40}$/; +const REF = /^refs\/heads\/seat\/[A-Za-z0-9._/-]+$/; +const grantId = String(process.argv[3] || '').replace(/^commonly::/, ''); + +const git = (args) => new Promise((resolve, reject) => { + const child = spawn('git', args, { stdio: ['ignore', 'pipe', 'pipe'] }); + let stdout = ''; + let stderr = ''; + child.stdout.on('data', (chunk) => { stdout += chunk; if (stdout.length > 4096) child.kill(); }); + child.stderr.on('data', (chunk) => { stderr += chunk; if (stderr.length > 4096) child.kill(); }); + child.on('error', reject); + child.on('close', (code) => (code === 0 ? resolve(stdout.trim()) : reject(new Error(stderr.trim() || 'git command failed')))); +}); + +const broker = (args) => new Promise((resolve, reject) => { + const path = process.env.COMMONLY_GH_SOCKET; + if (!path?.startsWith('/')) return reject(new Error('seat GitHub broker is unavailable')); + const socket = connect(path); + const chunks = []; + let bytes = 0; + socket.setTimeout(185_000, () => socket.destroy(new Error('push broker timed out'))); + socket.on('connect', () => socket.end(JSON.stringify({ grantId, operation: 'push.branch', args }))); + socket.on('data', (chunk) => { + bytes += chunk.length; + if (bytes > 128 * 1024) socket.destroy(new Error('push broker response is too large')); + else chunks.push(chunk); + }); + socket.on('error', reject); + socket.on('end', () => { + try { resolve(JSON.parse(Buffer.concat(chunks).toString('utf8'))); } catch { + reject(new Error('push broker response is invalid')); + } + }); +}); + +const push = async (commands) => { + if (commands.length !== 1 || !commands[0].startsWith('push ')) { + throw new Error('exactly one branch push is allowed'); + } + const spec = commands[0].slice(5); + if (spec.startsWith('+') || spec.split(':').length !== 2) throw new Error('force and deletion are not allowed'); + const [source, ref] = spec.split(':'); + if (!/^(HEAD|refs\/heads\/[A-Za-z0-9._/-]+)$/.test(source) + || !REF.test(ref) || ref.includes('..') || ref.endsWith('.lock') || ref.endsWith('.')) { + throw new Error('branch ref is not allowed'); + } + const newOid = await git(['rev-parse', '--verify', `${source}^{commit}`]); + const baseOid = await git(['merge-base', newOid, 'refs/remotes/origin/main']); + if (!OID.test(newOid) || !OID.test(baseOid)) throw new Error('invalid local commit'); + const gitPath = await git(['rev-parse', '--git-path', 'commonly-push']); + const directory = await mkdtemp(`${gitPath}-`); + try { + const file = join(directory, 'push.bundle'); + await git(['bundle', 'create', file, source, `^${baseOid}`]); + const bundle = await readFile(file); + if (bundle.length > MAX_BUNDLE_BYTES) throw new Error('push bundle exceeds 6 MiB'); + const answer = await broker({ ref, baseOid, newOid, bundle: bundle.toString('base64') }); + if (answer.status !== 200) throw new Error(answer.body?.error || 'broker refused push'); + return ref; + } finally { + await rm(directory, { recursive: true, force: true }); + } +}; + +const main = async () => { + if (!/^[A-Za-z0-9_-]{1,128}$/.test(grantId)) throw new Error('invalid broker grant id'); + const lines = createInterface({ input: process.stdin, crlfDelay: Infinity }); + let batch = []; + for await (const line of lines) { + if (line === 'capabilities') { + process.stdout.write('push\n\n'); + } else if (line === 'list for-push') { + process.stdout.write('\n'); + } else if (line.startsWith('push ')) { + batch.push(line); + } else if (line === '' && batch.length) { + const ref = batch[0].split(':').at(-1); + try { + const accepted = await push(batch); + process.stdout.write(`ok ${accepted}\n\n`); + } catch (error) { + process.stdout.write(`error ${ref} ${String(error.message).replace(/[\r\n]/g, ' ')}\n\n`); + } + batch = []; + } else if (line !== '') { + throw new Error('unsupported Git remote command'); + } + } +}; + +main().catch((error) => { process.stderr.write(`${error.message}\n`); process.exitCode = 2; }); diff --git a/cli/src/lib/host-gh-broker.js b/cli/src/lib/host-gh-broker.js index b4de334695..d1f6dc1f65 100644 --- a/cli/src/lib/host-gh-broker.js +++ b/cli/src/lib/host-gh-broker.js @@ -12,6 +12,8 @@ import { join } from 'path'; const MAX_REQUEST_BYTES = 64 * 1024; const MAX_RESPONSE_BYTES = 128 * 1024; const SHIM_SOURCE = fileURLToPath(new URL('./host-gh-shim.mjs', import.meta.url)); +const PUSH_HELPER_SOURCE = fileURLToPath(new URL('./git-remote-commonly.mjs', import.meta.url)); +const MAX_PUSH_REQUEST_BYTES = 9 * 1024 * 1024; const reply = (socket, status, body) => { if (!socket.destroyed) socket.end(JSON.stringify({ status, body })); @@ -39,11 +41,15 @@ export const startSeatGhBroker = async ({ const directory = await mkdtemp(join(root, 'seat-')); const socketPath = join(directory, 'broker.sock'); const shimPath = join(directory, 'gh'); + const pushHelperPath = join(directory, 'git-remote-commonly'); let server; try { const shimSource = await readFile(SHIM_SOURCE, 'utf8'); await writeFile(shimPath, `#!${process.execPath}\n${shimSource.replace(/^#![^\n]*\n/, '')}`, { mode: 0o500 }); await chmod(shimPath, 0o555); + const helperSource = await readFile(PUSH_HELPER_SOURCE, 'utf8'); + await writeFile(pushHelperPath, `#!${process.execPath}\n${helperSource.replace(/^#![^\n]*\n/, '')}`, { mode: 0o500 }); + await chmod(pushHelperPath, 0o555); server = createServer({ allowHalfOpen: true }, (socket) => { let chunks = []; let size = 0; @@ -51,7 +57,7 @@ export const startSeatGhBroker = async ({ socket.on('error', () => {}); socket.on('data', (chunk) => { size += chunk.length; - if (size > MAX_REQUEST_BYTES) { + if (size > MAX_PUSH_REQUEST_BYTES) { reply(socket, 413, { error: 'request_too_large' }); chunks = []; } else { @@ -72,13 +78,19 @@ export const startSeatGhBroker = async ({ reply(socket, 400, { error: 'invalid_request' }); return; } + if (operation !== 'push.branch' && size > MAX_REQUEST_BYTES) { + reply(socket, 413, { error: 'request_too_large' }); + return; + } + if (operation === 'push.branch') socket.setTimeout(180_000, () => socket.destroy()); try { - const url = new URL(`/api/mcp/grants/${encodeURIComponent(grantId)}/cli`, endpoint); + const suffix = operation === 'push.branch' ? 'push' : 'cli'; + const url = new URL(`/api/mcp/grants/${encodeURIComponent(grantId)}/${suffix}`, endpoint); const response = await fetchImpl(url, { method: 'POST', headers: { Authorization: `Bearer ${runtimeToken}`, 'Content-Type': 'application/json' }, - body: JSON.stringify({ operation, args }), - signal: AbortSignal.timeout(15_000), + body: JSON.stringify(operation === 'push.branch' ? args : { operation, args }), + signal: AbortSignal.timeout(operation === 'push.branch' ? 180_000 : 15_000), }); const bodyText = await response.text(); if (Buffer.byteLength(bodyText, 'utf8') > MAX_RESPONSE_BYTES) { @@ -103,6 +115,7 @@ export const startSeatGhBroker = async ({ return { socketPath, shimPath, + pushHelperPath, directory, async close() { await new Promise((resolve) => server.close(resolve)); diff --git a/docs/adr/ADR-028-work-claims-and-decision-ledger.md b/docs/adr/ADR-028-work-claims-and-decision-ledger.md index 055dc86e32..d291ec6c5c 100644 --- a/docs/adr/ADR-028-work-claims-and-decision-ledger.md +++ b/docs/adr/ADR-028-work-claims-and-decision-ledger.md @@ -696,7 +696,12 @@ may supply operations it already implements; missing PR operations must be added there with the same grant and audit checks, never through `routes/github`'s legacy `anyAuth` path. -Git push is a separate smart-HTTP channel on the same socket. It is registered +Git push is a separate bounded bundle channel on the same socket. The seat's +`git-remote-commonly` helper implements Git's push-only remote-helper protocol: +it makes an incremental bundle against a local base commit, then sends one +branch update to the daemon. The daemon forwards it with the seat's runtime +token; the backend alone holds the GitHub App token. This avoids tunneling an +interactive Git wire protocol through the grant route. It is registered as `github.push_branch` in the broker catalogue, and goes through the same exact-tool, active-grant, pinned-repository and audit checks as the CLI channel. `read` grants refuse. `write-with-confirm` grants refuse push until a later @@ -705,17 +710,19 @@ then revalidate them after approval; no provider write occurs on that path now. Only an explicit `github.push_branch` tool on an autonomous `write` grant can push. A grant for issue comments never implies push authority. -The backend accepts one bounded receive-pack request, parses the **complete** -command framing before a provider call, and requires exactly one update to the +The backend accepts one bounded bundle request, checks its single advertised +head and prerequisite against objects fetched from the pinned provider repo, +and requires exactly one update to the authenticated seat's `refs/heads/seat//` prefix. It refuses deletion, tags, second refs, protected branches, malformed framing and any -client-supplied repo/prefix override. The claimed old OID must equal the -provider's current ref (or zero for a fresh branch). It unpacks into an isolated -quarantine object store and verifies the old commit is an ancestor of the new +client-supplied repo/prefix override. The backend reads the old OID from the +provider's current ref (or zero for a fresh branch). It unbundles into a fresh +backend-owned bare object store with no inherited Git config or hooks, checks +connectivity, and verifies the old commit is an ancestor of the new commit; it refuses when ancestry cannot be established. A trusted server-side push then uses a compare-and-swap lease on that same old OID, so a concurrent ref move fails. The client has no control over this lease or fast-forward check: -the wire protocol does not attest to the client's `--force` choice. The App +the remote-helper protocol does not attest to the client's `--force` choice. The App token stays only in the backend-to-GitHub request; the child never receives a credential helper that returns it. Acceptance covers fresh branch and existing fast-forward success, and divergent update with correct old OID, stale old OID, From 8fc02a7c7d5528d1e5aff61f85cb0f2678e4aca0 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Fri, 23 Oct 2026 14:26:00 -0700 Subject: [PATCH 06/12] fix(broker): bound provider push redirects and report parked writes Co-authored-by: kai Agent-Model: codex/gpt-6-sol/medium Agent-Model: codex/gpt-6.1-sol/medium --- .../unit/models/ToolCall.credentialOwnerRetrofit.test.js | 4 ++++ backend/services/seatGithubPush.ts | 3 ++- cli/__tests__/host-gh-broker.test.mjs | 9 +++++++++ cli/src/lib/host-gh-shim.mjs | 6 +++++- 4 files changed, 20 insertions(+), 2 deletions(-) diff --git a/backend/__tests__/unit/models/ToolCall.credentialOwnerRetrofit.test.js b/backend/__tests__/unit/models/ToolCall.credentialOwnerRetrofit.test.js index 6ea94b73cc..f2c1f6e998 100644 --- a/backend/__tests__/unit/models/ToolCall.credentialOwnerRetrofit.test.js +++ b/backend/__tests__/unit/models/ToolCall.credentialOwnerRetrofit.test.js @@ -33,6 +33,10 @@ describe('the trail schema retrofit', () => { const statements = pool.query.mock.calls.map(([sql]) => String(sql)); expect(statements.some((sql) => /ALTER TABLE tool_calls ADD COLUMN IF NOT EXISTS credential_owner_id/.test(sql))) .toBe(true); + expect(statements.some((sql) => /ALTER TABLE tool_calls ADD COLUMN IF NOT EXISTS provider_repo/.test(sql))) + .toBe(true); + expect(statements.some((sql) => /ALTER TABLE tool_calls ADD COLUMN IF NOT EXISTS provider_ref/.test(sql))) + .toBe(true); const insert = statements.find((sql) => /INSERT INTO tool_calls/.test(sql)); expect(insert).toMatch(/credential_owner_id/); // The value is bound, not interpolated: an owner id is data. diff --git a/backend/services/seatGithubPush.ts b/backend/services/seatGithubPush.ts index 3a5213f394..b9c9c50f67 100644 --- a/backend/services/seatGithubPush.ts +++ b/backend/services/seatGithubPush.ts @@ -119,8 +119,9 @@ export const pushSeatBranch = async ( const env = { PATH: '/usr/bin:/bin', HOME: directory, GIT_CONFIG_NOSYSTEM: '1', GIT_CONFIG_GLOBAL: '/dev/null', GIT_NO_REPLACE_OBJECTS: '1', GIT_TERMINAL_PROMPT: '0', - GIT_CONFIG_COUNT: '1', GIT_CONFIG_KEY_0: `http.${url}.extraheader`, + GIT_CONFIG_COUNT: '2', GIT_CONFIG_KEY_0: `http.${url}.extraheader`, GIT_CONFIG_VALUE_0: `AUTHORIZATION: basic ${basic}`, + GIT_CONFIG_KEY_1: 'http.followRedirects', GIT_CONFIG_VALUE_1: 'false', }; const git = async (args: string[], cwd = repo, failure?: SeatPushError) => { try { diff --git a/cli/__tests__/host-gh-broker.test.mjs b/cli/__tests__/host-gh-broker.test.mjs index 6d1e1e831d..2eaf0622e8 100644 --- a/cli/__tests__/host-gh-broker.test.mjs +++ b/cli/__tests__/host-gh-broker.test.mjs @@ -76,3 +76,12 @@ it('rejects a removed seat endpoint after teardown', async () => { })).rejects.toMatchObject({ code: 'ENOENT' }); expect(fetchImpl).not.toHaveBeenCalled(); }); + +it('reports a parked write as pending approval without claiming it executed', async () => { + fetchImpl.mockResolvedValue(new Response(JSON.stringify({ + status: 'pending_approval', approvalId: 'approval-1', + }), { status: 202, headers: { 'Content-Type': 'application/json' } })); + const { stdout } = await run('pr', 'create', '--title', 'Change', '--head', 'seat/agent-a/change', + '--base', 'main', '--grant-id', 'grant-1'); + expect(JSON.parse(stdout)).toEqual({ status: 'pending_approval', approvalId: 'approval-1' }); +}); diff --git a/cli/src/lib/host-gh-shim.mjs b/cli/src/lib/host-gh-shim.mjs index 6b4101ccae..f79fe60900 100644 --- a/cli/src/lib/host-gh-shim.mjs +++ b/cli/src/lib/host-gh-shim.mjs @@ -85,7 +85,11 @@ const main = async () => { } }); }); - if (answer.status < 200 || answer.status >= 300 || answer.status === 202) { + if (answer.status === 202 && answer.body?.status === 'pending_approval') { + process.stdout.write(`${JSON.stringify(answer.body, null, 2)}\n`); + return; + } + if (answer.status < 200 || answer.status >= 300) { throw new Error(answer.body?.error || answer.body?.status || 'GitHub operation refused'); } process.stdout.write(`${JSON.stringify(answer.body, null, 2)}\n`); From 4252b686f580278906b94a05b22fd945f2998ebc Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Fri, 23 Oct 2026 14:26:29 -0700 Subject: [PATCH 07/12] test(broker): guard large push parser wiring Co-authored-by: kai Agent-Model: codex/gpt-6-sol/medium Agent-Model: codex/gpt-6.1-sol/medium --- .../unit/routes/mcpGrants.hostGh.test.js | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/backend/__tests__/unit/routes/mcpGrants.hostGh.test.js b/backend/__tests__/unit/routes/mcpGrants.hostGh.test.js index 8f8b3a3a47..79a3a84dbf 100644 --- a/backend/__tests__/unit/routes/mcpGrants.hostGh.test.js +++ b/backend/__tests__/unit/routes/mcpGrants.hostGh.test.js @@ -1,5 +1,7 @@ const express = require('express'); const request = require('supertest'); +const { readFileSync } = require('fs'); +const { join } = require('path'); const mockCallTool = jest.fn(); const mockListToolsForGrant = jest.fn(); @@ -22,6 +24,7 @@ jest.mock('../../../models/ToolCall', () => ({ reserveBudgetLineage: mockReserve const router = require('../../../routes/mcpGrants'); const app = express(); +app.use('/api/mcp/grants/:grantId/push', express.json({ limit: '9mb' })); app.use(express.json()); app.use('/api/mcp/grants', router); @@ -151,4 +154,18 @@ describe('host GitHub push boundary', () => { expect(response.status).toBe(429); expect(mockCallTool).not.toHaveBeenCalled(); }); + + it('preserves the larger push parser before the global JSON parser in the real server', async () => { + const source = readFileSync(join(__dirname, '../../../server.ts'), 'utf8'); + expect(source.indexOf("app.use('/api/mcp/grants/:grantId/push', express.json({ limit: '9mb' }))")) + .toBeGreaterThan(-1); + expect(source.indexOf("app.use('/api/mcp/grants/:grantId/push'")) + .toBeLessThan(source.indexOf('app.use(express.json());')); + mockListToolsForGrant.mockResolvedValue([{ name: 'github.push_branch' }]); + const response = await request(app).post('/api/mcp/grants/grant-1/push').send({ + ...push, bundle: 'A'.repeat(200_000), + }); + expect(response.status).toBe(200); + expect(mockCallTool).toHaveBeenCalledTimes(1); + }); }); From ef83fcae2a138c22df4272e3ff92827201cbf965 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Fri, 23 Oct 2026 14:27:37 -0700 Subject: [PATCH 08/12] fix(broker): limit push bodies before parsing Co-authored-by: kai Agent-Model: codex/gpt-6-sol/medium Agent-Model: codex/gpt-6.1-sol/medium --- .../__tests__/unit/routes/mcpGrants.hostGh.test.js | 2 +- backend/routes/mcpGrants.ts | 13 +++++++++++++ backend/server.ts | 6 ++++-- docs/adr/ADR-028-work-claims-and-decision-ledger.md | 11 ++++++----- 4 files changed, 24 insertions(+), 8 deletions(-) diff --git a/backend/__tests__/unit/routes/mcpGrants.hostGh.test.js b/backend/__tests__/unit/routes/mcpGrants.hostGh.test.js index 79a3a84dbf..0eeaaba963 100644 --- a/backend/__tests__/unit/routes/mcpGrants.hostGh.test.js +++ b/backend/__tests__/unit/routes/mcpGrants.hostGh.test.js @@ -157,7 +157,7 @@ describe('host GitHub push boundary', () => { it('preserves the larger push parser before the global JSON parser in the real server', async () => { const source = readFileSync(join(__dirname, '../../../server.ts'), 'utf8'); - expect(source.indexOf("app.use('/api/mcp/grants/:grantId/push', express.json({ limit: '9mb' }))")) + expect(source.indexOf("app.use('/api/mcp/grants/:grantId/push', mcpGrantsRoutes.pushBodyRateLimit,")) .toBeGreaterThan(-1); expect(source.indexOf("app.use('/api/mcp/grants/:grantId/push'")) .toBeLessThan(source.indexOf('app.use(express.json());')); diff --git a/backend/routes/mcpGrants.ts b/backend/routes/mcpGrants.ts index 6eb96fefd3..c9e6d2d2f8 100644 --- a/backend/routes/mcpGrants.ts +++ b/backend/routes/mcpGrants.ts @@ -21,6 +21,8 @@ const { ListToolsRequestSchema, CallToolRequestSchema, McpError, ErrorCode } = m const agentRuntimeAuth = require('../middleware/agentRuntimeAuth'); // eslint-disable-next-line @typescript-eslint/no-require-imports const { agentRateLimitKeyGenerator } = require('../middleware/agentRateLimit'); +// eslint-disable-next-line @typescript-eslint/no-require-imports +const { cloudflareIpRateLimitKeyGenerator } = require('../middleware/ipRateLimit'); const router = express.Router(); @@ -35,6 +37,17 @@ const brokerRateLimit = rateLimit({ keyGenerator: agentRateLimitKeyGenerator, handler: (_req, res) => res.status(429).json({ error: 'rate_limit_exceeded' }), }); +// Mounted before the 9 MiB push parser in server.ts. An unauthenticated caller +// can vary Authorization headers, so this early budget keys on the edge IP, +// not the token header. The route separately applies the token-aware limiter. +export const pushBodyRateLimit = rateLimit({ + windowMs: 60_000, + max: 20, + standardHeaders: true, + legacyHeaders: false, + keyGenerator: cloudflareIpRateLimitKeyGenerator, + handler: (_req, res) => res.status(429).json({ error: 'push_body_rate_limited' }), +}); const errorPayload = (error: unknown): Record => { const e = error as { code?: string; message?: string; details?: unknown }; diff --git a/backend/server.ts b/backend/server.ts index f096837c5f..1ad6a629af 100644 --- a/backend/server.ts +++ b/backend/server.ts @@ -236,7 +236,9 @@ app.use('/api/webhooks/github', githubGigWebhookRoutes.githubGigRawError); // The seat push channel carries a bounded incremental Git bundle. Keep its // larger parser scoped to this one authenticated route; every other JSON API // retains the default body limit. -app.use('/api/mcp/grants/:grantId/push', express.json({ limit: '9mb' })); +const mcpGrantsRoutes = require('./routes/mcpGrants'); +app.use('/api/mcp/grants/:grantId/push', mcpGrantsRoutes.pushBodyRateLimit, + express.json({ limit: '9mb' })); app.use(express.json()); // Routes @@ -283,7 +285,7 @@ app.use('/api/v1/tasks', tasksApiRoutes); // Task management for dev agents app.use('/api/registry', registryRoutes); // Agent Registry (package manager for agents) app.use('/api/credentials', require('./routes/credentials')); // ADR-026 Phase 0: credential lineage + revocation app.use('/api/grants', require('./routes/grants')); // ADR-001 room-grant record: server-enforced attenuation + cascade revocation -app.use('/api/mcp/grants', require('./routes/mcpGrants')); // ADR-001 tool broker: stateless MCP transport + attributed trail +app.use('/api/mcp/grants', mcpGrantsRoutes); // ADR-001 tool broker: stateless MCP transport + attributed trail app.use('/api/agent-binding', require('./routes/agentBinding')); // ADR-026 D3: machine adoption CAS app.use('/api/machines', require('./routes/machines')); // ADR-026 Phase 1: local daemon lifecycle app.use('/api/hosted', require('./routes/hosted')); // ADR-023 W2: hosted runtime provision surface (metered) diff --git a/docs/adr/ADR-028-work-claims-and-decision-ledger.md b/docs/adr/ADR-028-work-claims-and-decision-ledger.md index d291ec6c5c..9387c9d331 100644 --- a/docs/adr/ADR-028-work-claims-and-decision-ledger.md +++ b/docs/adr/ADR-028-work-claims-and-decision-ledger.md @@ -718,11 +718,12 @@ deletion, tags, second refs, protected branches, malformed framing and any client-supplied repo/prefix override. The backend reads the old OID from the provider's current ref (or zero for a fresh branch). It unbundles into a fresh backend-owned bare object store with no inherited Git config or hooks, checks -connectivity, and verifies the old commit is an ancestor of the new -commit; it refuses when ancestry cannot be established. A trusted server-side -push then uses a compare-and-swap lease on that same old OID, so a concurrent -ref move fails. The client has no control over this lease or fast-forward check: -the remote-helper protocol does not attest to the client's `--force` choice. The App +connectivity, and verifies the old commit is an ancestor of the new commit +when the branch already exists; it refuses when ancestry cannot be established. +A fresh branch instead uses an absence lease. A trusted server-side push uses +a compare-and-swap lease on the provider's old OID, so a concurrent ref move +fails. The client has no control over this lease or fast-forward check: the +remote-helper protocol carries a force indicator, but it is untrusted. The App token stays only in the backend-to-GitHub request; the child never receives a credential helper that returns it. Acceptance covers fresh branch and existing fast-forward success, and divergent update with correct old OID, stale old OID, From b766d6324dcd34a1d79fff142b46ca9586cb0b71 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Fri, 23 Oct 2026 14:30:56 -0700 Subject: [PATCH 09/12] Separate host GitHub shims from broker sockets Co-authored-by: kai Agent-Model: codex/gpt-6-sol/medium Agent-Model: codex/gpt-6.1-sol/medium --- .../unit/routes/mcpGrants.hostGh.test.js | 1 + cli/__tests__/git-remote-commonly.test.mjs | 3 +- cli/__tests__/host-gh-broker.test.mjs | 29 ++++++++++++-- .../security/verify-host-gh-socket.mjs | 13 ++++--- cli/src/lib/host-gh-broker.js | 38 +++++++++++++++---- ...ADR-028-work-claims-and-decision-ledger.md | 6 ++- 6 files changed, 72 insertions(+), 18 deletions(-) diff --git a/backend/__tests__/unit/routes/mcpGrants.hostGh.test.js b/backend/__tests__/unit/routes/mcpGrants.hostGh.test.js index 0eeaaba963..7bd257e0ef 100644 --- a/backend/__tests__/unit/routes/mcpGrants.hostGh.test.js +++ b/backend/__tests__/unit/routes/mcpGrants.hostGh.test.js @@ -156,6 +156,7 @@ describe('host GitHub push boundary', () => { }); it('preserves the larger push parser before the global JSON parser in the real server', async () => { + expect(typeof router.pushBodyRateLimit).toBe('function'); const source = readFileSync(join(__dirname, '../../../server.ts'), 'utf8'); expect(source.indexOf("app.use('/api/mcp/grants/:grantId/push', mcpGrantsRoutes.pushBodyRateLimit,")) .toBeGreaterThan(-1); diff --git a/cli/__tests__/git-remote-commonly.test.mjs b/cli/__tests__/git-remote-commonly.test.mjs index 60c0e6356a..3d040f8c22 100644 --- a/cli/__tests__/git-remote-commonly.test.mjs +++ b/cli/__tests__/git-remote-commonly.test.mjs @@ -30,7 +30,8 @@ describe('confined Git remote helper', () => { await run(repo, ['commit', '-am', 'change'], env); forwarded = []; broker = await startSeatGhBroker({ - runtimeToken: 'cm_agent_test', instanceUrl: 'http://localhost:5000', root: join(root, 'broker'), + runtimeToken: 'cm_agent_test', instanceUrl: 'http://localhost:5000', + root: join(root, 'broker'), shimRoot: root, fetchImpl: async (url, request) => { forwarded.push({ url: String(url), request: JSON.parse(request.body) }); return { status: 200, text: async () => JSON.stringify({ result: { ref: seatRef } }) }; diff --git a/cli/__tests__/host-gh-broker.test.mjs b/cli/__tests__/host-gh-broker.test.mjs index 2eaf0622e8..4e68220b49 100644 --- a/cli/__tests__/host-gh-broker.test.mjs +++ b/cli/__tests__/host-gh-broker.test.mjs @@ -2,7 +2,6 @@ import { afterEach, beforeEach, expect, it, jest } from '@jest/globals'; import { execFile } from 'child_process'; import { mkdtemp, rm } from 'fs/promises'; import { connect } from 'net'; -import { tmpdir } from 'os'; import { join } from 'path'; import { promisify } from 'util'; import { startSeatGhBroker } from '../src/lib/host-gh-broker.js'; @@ -14,12 +13,14 @@ let broker; let fetchImpl; beforeEach(async () => { - testRoot = await mkdtemp(join(tmpdir(), 'commonly-host-gh-test-')); + // macOS AF_UNIX paths have a small byte limit. + testRoot = await mkdtemp('/tmp/hgt-'); fetchImpl = jest.fn().mockResolvedValue(new Response(JSON.stringify({ callId: 'call-1', outcome: 'ok', result: { number: 7 }, }), { status: 200, headers: { 'Content-Type': 'application/json' } })); broker = await startSeatGhBroker({ - runtimeToken: TOKEN, instanceUrl: 'https://api.example.invalid', root: testRoot, fetchImpl, + runtimeToken: TOKEN, instanceUrl: 'https://api.example.invalid', + root: join(testRoot, 'sockets'), shimRoot: testRoot, fetchImpl, }); }); @@ -77,6 +78,28 @@ it('rejects a removed seat endpoint after teardown', async () => { expect(fetchImpl).not.toHaveBeenCalled(); }); +it('keeps shims outside the protected socket directory', () => { + expect(broker.shimPath.startsWith(broker.socketDirectory)).toBe(false); + expect(broker.pushHelperPath.startsWith(broker.socketDirectory)).toBe(false); + expect(broker.socketPath.startsWith(broker.socketDirectory)).toBe(true); +}); + +it('forwards a bounded bundle larger than the command limit', async () => { + const response = await new Promise((resolve, reject) => { + const socket = connect(broker.socketPath); + const chunks = []; + socket.on('connect', () => socket.end(JSON.stringify({ + grantId: 'grant-1', operation: 'push.branch', args: { bundle: 'A'.repeat(70 * 1024) }, + }))); + socket.on('data', (chunk) => chunks.push(chunk)); + socket.on('end', () => resolve(JSON.parse(Buffer.concat(chunks).toString('utf8')))); + socket.on('error', reject); + }); + expect(response.status).toBe(200); + expect(fetchImpl).toHaveBeenCalledTimes(1); + expect(String(fetchImpl.mock.calls[0][0])).toMatch(/\/api\/mcp\/grants\/grant-1\/push$/); +}); + it('reports a parked write as pending approval without claiming it executed', async () => { fetchImpl.mockResolvedValue(new Response(JSON.stringify({ status: 'pending_approval', approvalId: 'approval-1', diff --git a/cli/scripts/security/verify-host-gh-socket.mjs b/cli/scripts/security/verify-host-gh-socket.mjs index 29089b4da5..702d425dd7 100644 --- a/cli/scripts/security/verify-host-gh-socket.mjs +++ b/cli/scripts/security/verify-host-gh-socket.mjs @@ -1,19 +1,22 @@ #!/usr/bin/env node /** Real macOS permission-profile probe; no provider credentials or model turn. */ import { execFile } from 'child_process'; -import { mkdtemp, rm } from 'fs/promises'; -import { tmpdir } from 'os'; +import { mkdir, mkdtemp, rm } from 'fs/promises'; import { join } from 'path'; import { promisify } from 'util'; import { startSeatGhBroker } from '../../src/lib/host-gh-broker.js'; const runFile = promisify(execFile); -const root = await mkdtemp(join(tmpdir(), 'commonly-gh-isolation-')); +const root = await mkdtemp('/tmp/hgi-'); +const socketRoot = join(root, 'sockets'); +const shimRoot = join(root, 'shims'); +await mkdir(shimRoot); const calls = []; const seat = async (name) => startSeatGhBroker({ runtimeToken: `cm_agent_${name.repeat(64)}`, instanceUrl: 'https://api.example.invalid', - root, + root: socketRoot, + shimRoot, fetchImpl: async () => { calls.push(name); return new Response(JSON.stringify({ callId: name, outcome: 'ok', result: { number: 1 } }), { @@ -24,7 +27,7 @@ const seat = async (name) => startSeatGhBroker({ const profile = (own, featureOn = true) => [ '-c', 'default_permissions="host_gh_probe"', - '-c', `permissions.host_gh_probe.filesystem={":minimal"="read","/opt/homebrew"="read",${JSON.stringify(root)}="deny",${JSON.stringify(own.directory)}="read"}`, + '-c', `permissions.host_gh_probe.filesystem={":minimal"="read","/opt/homebrew"="read",${JSON.stringify(socketRoot)}="deny",${JSON.stringify(own.directory)}="read"}`, '-c', `features.network_proxy=${featureOn}`, '-c', 'permissions.host_gh_probe.network.enabled=true', '-c', `permissions.host_gh_probe.network.unix_sockets={${JSON.stringify(own.socketPath)}="allow"}`, diff --git a/cli/src/lib/host-gh-broker.js b/cli/src/lib/host-gh-broker.js index d1f6dc1f65..df05e52f99 100644 --- a/cli/src/lib/host-gh-broker.js +++ b/cli/src/lib/host-gh-broker.js @@ -7,7 +7,7 @@ import { createServer } from 'net'; import { chmod, lstat, mkdir, mkdtemp, readFile, rm, writeFile } from 'fs/promises'; import { homedir } from 'os'; import { fileURLToPath } from 'url'; -import { join } from 'path'; +import { join, resolve } from 'path'; const MAX_REQUEST_BYTES = 64 * 1024; const MAX_RESPONSE_BYTES = 128 * 1024; @@ -20,7 +20,8 @@ const reply = (socket, status, body) => { }; export const startSeatGhBroker = async ({ - runtimeToken, instanceUrl, fetchImpl = fetch, root = join(homedir(), '.commonly', 'host-gh'), + runtimeToken, instanceUrl, shimRoot, fetchImpl = fetch, + root = join(homedir(), '.commonly', 'host-gh'), }) => { if (typeof runtimeToken !== 'string' || !runtimeToken.startsWith('cm_agent_')) { throw new Error('host GitHub broker requires a runtime token'); @@ -32,18 +33,32 @@ export const startSeatGhBroker = async ({ if (endpoint.protocol === 'http:' && !['localhost', '127.0.0.1', '[::1]'].includes(endpoint.hostname)) { throw new Error('host GitHub broker requires HTTPS outside localhost'); } + if (typeof shimRoot !== 'string' || !shimRoot) { + throw new Error('host GitHub broker requires a seat-visible shim root'); + } + if (resolve(shimRoot) === resolve(root)) { + throw new Error('host GitHub shims cannot share the protected socket root'); + } await mkdir(root, { recursive: true, mode: 0o700 }); const rootStats = await lstat(root); if (!rootStats.isDirectory() || rootStats.isSymbolicLink()) { throw new Error('host GitHub broker root must be a real directory'); } await chmod(root, 0o700); - const directory = await mkdtemp(join(root, 'seat-')); - const socketPath = join(directory, 'broker.sock'); - const shimPath = join(directory, 'gh'); - const pushHelperPath = join(directory, 'git-remote-commonly'); + const shimRootStats = await lstat(shimRoot); + if (!shimRootStats.isDirectory() || shimRootStats.isSymbolicLink()) { + throw new Error('host GitHub shim root must be a real directory'); + } + let socketDirectory; + let directory; let server; + const connections = new Set(); try { + socketDirectory = await mkdtemp(join(root, 'seat-')); + directory = await mkdtemp(join(shimRoot, '.commonly-gh-')); + const socketPath = join(socketDirectory, 'broker.sock'); + const shimPath = join(directory, 'gh'); + const pushHelperPath = join(directory, 'git-remote-commonly'); const shimSource = await readFile(SHIM_SOURCE, 'utf8'); await writeFile(shimPath, `#!${process.execPath}\n${shimSource.replace(/^#![^\n]*\n/, '')}`, { mode: 0o500 }); await chmod(shimPath, 0o555); @@ -51,6 +66,8 @@ export const startSeatGhBroker = async ({ await writeFile(pushHelperPath, `#!${process.execPath}\n${helperSource.replace(/^#![^\n]*\n/, '')}`, { mode: 0o500 }); await chmod(pushHelperPath, 0o555); server = createServer({ allowHalfOpen: true }, (socket) => { + connections.add(socket); + socket.on('close', () => connections.delete(socket)); let chunks = []; let size = 0; socket.setTimeout(15_000, () => socket.destroy()); @@ -65,7 +82,7 @@ export const startSeatGhBroker = async ({ } }); socket.on('end', async () => { - if (socket.destroyed || size > MAX_REQUEST_BYTES) return; + if (socket.destroyed || size > MAX_PUSH_REQUEST_BYTES) return; let request; try { request = JSON.parse(Buffer.concat(chunks).toString('utf8')); } catch { reply(socket, 400, { error: 'invalid_request' }); @@ -117,14 +134,19 @@ export const startSeatGhBroker = async ({ shimPath, pushHelperPath, directory, + socketDirectory, async close() { + for (const socket of connections) socket.destroy(); await new Promise((resolve) => server.close(resolve)); await rm(directory, { recursive: true, force: true }); + await rm(socketDirectory, { recursive: true, force: true }); }, }; } catch (error) { + for (const socket of connections) socket.destroy(); if (server?.listening) await new Promise((resolve) => server.close(resolve)); - await rm(directory, { recursive: true, force: true }); + if (directory) await rm(directory, { recursive: true, force: true }); + if (socketDirectory) await rm(socketDirectory, { recursive: true, force: true }); throw error; } }; diff --git a/docs/adr/ADR-028-work-claims-and-decision-ledger.md b/docs/adr/ADR-028-work-claims-and-decision-ledger.md index 9387c9d331..116c88e82e 100644 --- a/docs/adr/ADR-028-work-claims-and-decision-ledger.md +++ b/docs/adr/ADR-028-work-claims-and-decision-ledger.md @@ -664,7 +664,11 @@ the next request. No generic URL, repository, method, header, or `gh api` passthrough is part of this interface. Each listener lives under a daemon-owned directory that the seat cannot read, -write, enumerate, or replace. The Codex permission profile sets +write, enumerate, or replace. The executable shims live in a distinct +per-spawn, seat-visible directory supplied by the trusted launcher; that +directory contains no socket or credential. The launcher must allow that shim +directory through the filesystem profile while continuing to deny the listener +directory. The Codex permission profile sets `features.network_proxy = true` and `network.unix_sockets. = "allow"`, leaves `dangerously_allow_all_unix_sockets = false`, and has no sibling socket entry; From 46cc9146067e72b1ccd787daf15dc8d9d797b522 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Fri, 23 Oct 2026 14:44:45 -0700 Subject: [PATCH 10/12] Enforce shared GitHub push and PR broker guards Co-authored-by: kai Agent-Model: codex/gpt-6-sol/medium Agent-Model: codex/gpt-6.1-sol/medium --- .../service/toolCallProviderRef.test.js | 106 ++++++++++++++++++ .../unit/routes/mcpGrants.hostGh.test.js | 20 ++-- .../unit/services/seatGithubPush.test.js | 20 +++- .../unit/services/toolBrokerService.test.js | 48 +++++++- backend/routes/mcpGrants.ts | 10 +- backend/services/seatGithubPush.ts | 18 ++- backend/services/toolBrokerService.ts | 54 +++++++-- 7 files changed, 249 insertions(+), 27 deletions(-) create mode 100644 backend/__tests__/service/toolCallProviderRef.test.js diff --git a/backend/__tests__/service/toolCallProviderRef.test.js b/backend/__tests__/service/toolCallProviderRef.test.js new file mode 100644 index 0000000000..cb864f1651 --- /dev/null +++ b/backend/__tests__/service/toolCallProviderRef.test.js @@ -0,0 +1,106 @@ +/** Exercise the provider audit columns through the real PostgreSQL model. */ +const { Pool } = require('pg'); +// eslint-disable-next-line import/no-unresolved, import/extensions +const ToolCall = require('../../models/ToolCall'); +// eslint-disable-next-line import/no-unresolved, import/extensions +const { TOOL_CALL_DDL } = require('../../models/ToolCall'); + +const run = process.env.INTEGRATION_TEST === 'true' ? describe : describe.skip; +const GRANT_ID = `task284-provider-ref-${Date.now()}`; + +const observerPool = () => new Pool({ + host: process.env.PG_HOST, + port: Number(process.env.PG_PORT || 5432), + database: process.env.PG_DATABASE, + user: process.env.PG_USER, + password: process.env.PG_PASSWORD, + ssl: false, +}); + +run('ToolCall provider ref audit on real PostgreSQL', () => { + let observer; + + beforeAll(async () => { + observer = observerPool(); + await ToolCall.create({ + callId: GRANT_ID, + grantId: GRANT_ID, + agentUserId: 'seat-a', + tool: 'github.push_branch', + argsDigest: 'a'.repeat(64), + outcome: 'ok', + providerRepo: 'Team-Commonly/commonly', + providerRef: 'refs/heads/seat/seat-a/work', + }); + }); + + afterAll(async () => { + if (observer) { + await observer.query('DELETE FROM tool_calls WHERE grant_id = $1', [GRANT_ID]); + await observer.end(); + } + // eslint-disable-next-line global-require, import/no-unresolved, import/extensions + const { pool } = require('../../config/db-pg'); + if (pool?.end) await pool.end(); + }); + + test('insert and list retain the exact provider repository and ref', async () => { + const query = 'SELECT provider_repo, provider_ref FROM tool_calls WHERE call_id = $1'; + const { rows } = await observer.query(query, [GRANT_ID]); + expect(rows).toEqual([{ + provider_repo: 'Team-Commonly/commonly', + provider_ref: 'refs/heads/seat/seat-a/work', + }]); + const [line] = await ToolCall.listForGrant(GRANT_ID); + expect(line).toMatchObject({ + providerRepo: 'Team-Commonly/commonly', + providerRef: 'refs/heads/seat/seat-a/work', + }); + }); + + test('DDL creates the columns and retrofits an existing table', async () => { + const schema = `task284_${Date.now()}`; + await observer.query(`CREATE SCHEMA "${schema}"`); + const client = await observer.connect(); + try { + await client.query(`SET search_path TO "${schema}"`); + await client.query(TOOL_CALL_DDL); + const fresh = await client.query( + "SELECT column_name FROM information_schema.columns WHERE table_schema = $1 AND table_name = 'tool_calls'", + [schema], + ); + expect(fresh.rows.map((row) => row.column_name)).toEqual(expect.arrayContaining([ + 'provider_repo', 'provider_ref', + ])); + + await client.query('DROP TABLE tool_calls'); + await client.query(`CREATE TABLE tool_calls ( + call_id VARCHAR(255) PRIMARY KEY, grant_id VARCHAR(255) NOT NULL, + pod_id VARCHAR(255), installation_id VARCHAR(255), agent_user_id VARCHAR(255) NOT NULL, + tool VARCHAR(255) NOT NULL, args_digest CHAR(64) NOT NULL, + occurred_at TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT CURRENT_TIMESTAMP, + outcome VARCHAR(32) NOT NULL, reason VARCHAR(255), approval_id VARCHAR(255), + duration_ms INTEGER, credential_owner_id VARCHAR(255) + )`); + await client.query(TOOL_CALL_DDL); + await client.query(`INSERT INTO tool_calls + (call_id, grant_id, agent_user_id, tool, args_digest, outcome, provider_repo, provider_ref) + VALUES ($1, $2, $3, $4, $5, $6, $7, $8)`, [ + 'retrofit-row', GRANT_ID, 'seat-a', 'github.push_branch', 'b'.repeat(64), + 'ok', 'Team-Commonly/commonly', 'refs/heads/seat/seat-a/work', + ]); + const { rows } = await client.query( + 'SELECT provider_repo, provider_ref FROM tool_calls WHERE call_id = $1', + ['retrofit-row'], + ); + expect(rows[0]).toMatchObject({ + provider_repo: 'Team-Commonly/commonly', + provider_ref: 'refs/heads/seat/seat-a/work', + }); + } finally { + await client.query('RESET search_path'); + client.release(); + await observer.query(`DROP SCHEMA "${schema}" CASCADE`); + } + }); +}); diff --git a/backend/__tests__/unit/routes/mcpGrants.hostGh.test.js b/backend/__tests__/unit/routes/mcpGrants.hostGh.test.js index 7bd257e0ef..8f94115339 100644 --- a/backend/__tests__/unit/routes/mcpGrants.hostGh.test.js +++ b/backend/__tests__/unit/routes/mcpGrants.hostGh.test.js @@ -50,7 +50,8 @@ describe('host GitHub broker command boundary', () => { expect(response.status).toBe(200); expect(response.body).toMatchObject({ callId: 'call-1', outcome: 'ok' }); expect(mockCallTool).toHaveBeenCalledWith({ - grantId: 'grant-1', agentUserId: AGENT_ID, agentName: 'kai', instanceId: 'default', tool, args, + grantId: 'grant-1', agentUserId: AGENT_ID, agentTokenHash: 'a'.repeat(64), + agentName: 'kai', instanceId: 'default', tool, args, }); expect(mockReserveBudgetLineage).toHaveBeenCalledWith(expect.arrayContaining([ expect.objectContaining({ calls: operation === 'pr.view' ? 30 : 10, windowMs: 60000 }), @@ -127,16 +128,15 @@ describe('host GitHub push boundary', () => { newOid: 'b'.repeat(40), bundle: 'YnVuZGxl', }; - it('requires the exact push tool and reserves two token/grant slots', async () => { + it('requires the exact push tool and passes token identity to the shared broker budget', async () => { mockListToolsForGrant.mockResolvedValue([{ name: 'github.push_branch' }]); const response = await request(app).post('/api/mcp/grants/grant-1/push').send(push); expect(response.status).toBe(200); expect(mockCallTool).toHaveBeenCalledWith(expect.objectContaining({ - grantId: 'grant-1', agentUserId: AGENT_ID, tool: 'github.push_branch', args: push, + grantId: 'grant-1', agentUserId: AGENT_ID, agentTokenHash: 'a'.repeat(64), + tool: 'github.push_branch', args: push, })); - expect(mockReserveBudgetLineage).toHaveBeenCalledWith(expect.arrayContaining([ - expect.objectContaining({ calls: 2, windowMs: 60000 }), - ])); + expect(mockReserveBudgetLineage).not.toHaveBeenCalled(); }); it('refuses a read-only or omitted push tool before budget use', async () => { @@ -147,12 +147,14 @@ describe('host GitHub push boundary', () => { expect(mockCallTool).not.toHaveBeenCalled(); }); - it('refuses exhausted push budget before any provider call', async () => { + it('returns the shared broker budget refusal', async () => { mockListToolsForGrant.mockResolvedValue([{ name: 'github.push_branch' }]); - mockReserveBudgetLineage.mockResolvedValue(false); + mockCallTool.mockRejectedValue(Object.assign(new Error('push budget exhausted'), { + code: 'host_github_push_rate_limited', statusCode: 429, + })); const response = await request(app).post('/api/mcp/grants/grant-1/push').send(push); expect(response.status).toBe(429); - expect(mockCallTool).not.toHaveBeenCalled(); + expect(mockCallTool).toHaveBeenCalledTimes(1); }); it('preserves the larger push parser before the global JSON parser in the real server', async () => { diff --git a/backend/__tests__/unit/services/seatGithubPush.test.js b/backend/__tests__/unit/services/seatGithubPush.test.js index 78c6eed16e..266d5c5836 100644 --- a/backend/__tests__/unit/services/seatGithubPush.test.js +++ b/backend/__tests__/unit/services/seatGithubPush.test.js @@ -3,7 +3,7 @@ const { mkdtemp, rm, writeFile } = require('fs/promises'); const { tmpdir } = require('os'); const { join } = require('path'); const { promisify } = require('util'); -const { parseSeatPushArgs, pushWithLease, validateSeatBundle } = require('../../../services/seatGithubPush'); +const { parseSeatPushArgs, fetchProviderObjects, pushWithLease, validateSeatBundle } = require('../../../services/seatGithubPush'); const runFile = promisify(execFile); const env = { ...process.env, GIT_AUTHOR_NAME: 'Test', GIT_AUTHOR_EMAIL: 'test@example.com', @@ -54,6 +54,24 @@ describe('seat GitHub push proof', () => { .resolves.toBeUndefined(); }); + it('fetches a current seat tip used as the bundle prerequisite', async () => { + const provider = join(root, 'provider.git'); + const freshReceiver = join(root, 'fresh.git'); + const ref = 'refs/heads/seat/agent123/work'; + await git(['init', '--bare', provider], root); + await git(['update-ref', 'refs/heads/main', baseOid], source); + await git(['fetch', source, 'refs/heads/main'], provider); + await git(['fetch', source, oldOid], provider); + await git(['update-ref', 'refs/heads/main', baseOid], provider); + await git(['update-ref', ref, oldOid], provider); + await git(['init', '--bare', freshReceiver], root); + await git(['bundle', 'create', bundlePath, 'HEAD', `^${oldOid}`], source); + const boundGit = (args, cwd = freshReceiver, failure) => git(args, cwd, failure); + await fetchProviderObjects({ git: boundGit, url: provider, defaultRef: 'refs/heads/main', ref, oldOid }); + await expect(validateSeatBundle({ git: boundGit, repo: freshReceiver, bundlePath, + baseOid: oldOid, oldOid, newOid })).resolves.toBeUndefined(); + }); + it('uses a zero-OID lease for a fresh branch and refuses a stale provider ref', async () => { const provider = join(root, 'provider.git'); const ref = 'refs/heads/seat/agent123/work'; diff --git a/backend/__tests__/unit/services/toolBrokerService.test.js b/backend/__tests__/unit/services/toolBrokerService.test.js index 946aabb190..3361b76819 100644 --- a/backend/__tests__/unit/services/toolBrokerService.test.js +++ b/backend/__tests__/unit/services/toolBrokerService.test.js @@ -42,6 +42,9 @@ jest.mock('../../../services/seatGrantConfinement', () => ({ })); jest.mock('../../../services/githubAppService', () => mockGithub); jest.mock('../../../services/seatGithubPush', () => ({ + SeatPushError: class SeatPushError extends Error { + constructor(code, statusCode = 403) { super(code); this.code = code; this.statusCode = statusCode; } + }, parseSeatPushArgs: jest.fn(), pushSeatBranch: (...args) => mockPushSeatBranch(...args), })); @@ -79,7 +82,7 @@ jest.mock('../../../services/roomGrantService', () => { }); // eslint-disable-next-line import/no-unresolved, import/extensions -const { callTool, executeApprovedToolCall } = require('../../../services/toolBrokerService'); +const { callTool, executeApprovedToolCall, executionErrorOutcome } = require('../../../services/toolBrokerService'); // eslint-disable-next-line import/no-unresolved, import/extensions const { assertGrantUsable, getGrantLineage, RoomGrantError } = require('../../../services/roomGrantService'); @@ -585,6 +588,21 @@ describe('host GitHub command tools', () => { })).rejects.toMatchObject({ code: 'tool_not_allowed' }); expect(mockGithub.createPullRequest).not.toHaveBeenCalled(); }); + + it('refuses a sibling PR branch before proposing and again at approved execution', async () => { + const args = { title: 'Change', head: 'seat/agent-b/work', base: 'main' }; + mockRoomGrant.findOne.mockResolvedValue(seatGrant({ + tools: ['github.create_pull_request'], writeMode: 'write', + })); + await expect(callTool({ grantId: 'grant-1', agentUserId: 'agent-a', + tool: 'github.create_pull_request', args })).rejects.toMatchObject({ code: 'branch_not_owned_by_seat' }); + const approvedArgs = { ...args, owner: 'Team-Commonly', repo: 'commonly' }; + await expect(executeApprovedToolCall({ grantId: 'grant-1', agentUserId: 'agent-a', + tool: 'github.create_pull_request', args: approvedArgs, + expectedArgsDigest: digest(approvedArgs), approvalId: 'approval-test' })) + .rejects.toMatchObject({ code: 'branch_not_owned_by_seat' }); + expect(mockGithub.createPullRequest).not.toHaveBeenCalled(); + }); }); describe('host GitHub push authority', () => { @@ -623,4 +641,32 @@ describe('host GitHub push authority', () => { providerRef: args.ref, outcome: 'ok', })); }); + + it('shares a mandatory two-call budget across token-bearing and tokenless broker calls', async () => { + mockRoomGrant.findOne.mockResolvedValue(seatGrant({ tools: ['github.push_branch'], writeMode: 'write' })); + mockPushSeatBranch.mockResolvedValue({ ref: args.ref, oldOid: '0'.repeat(40), newOid: args.newOid }); + const spent = new Map(); + mockReserveBudgetLineage.mockImplementation(async (entries) => { + if (entries.some(({ grantId, calls }) => (spent.get(grantId) || 0) >= calls)) return false; + entries.forEach(({ grantId }) => spent.set(grantId, (spent.get(grantId) || 0) + 1)); + return true; + }); + const input = { grantId: 'grant-1', agentUserId: 'agent-a', tool: 'github.push_branch', args }; + await expect(callTool({ ...input, agentTokenHash: 'a'.repeat(64) })).resolves.toMatchObject({ outcome: 'ok' }); + await expect(callTool(input)).resolves.toMatchObject({ outcome: 'ok' }); + await expect(callTool({ ...input, agentTokenHash: 'b'.repeat(64) })) + .rejects.toMatchObject({ code: 'host_github_push_rate_limited', statusCode: 429 }); + expect(mockPushSeatBranch).toHaveBeenCalledTimes(2); + expect(mockReserveBudgetLineage.mock.calls[0][0]).toEqual(expect.arrayContaining([ + expect.objectContaining({ grantId: expect.stringMatching(/^host-gh:push:seat:/), calls: 2 }), + expect.objectContaining({ grantId: expect.stringMatching(/^host-gh:push:grant:/), calls: 2 }), + expect.objectContaining({ grantId: `host-gh:push:token:${'a'.repeat(64)}`, calls: 2 }), + ])); + }); + + it('records push policy errors as refusals and provider errors as failures', () => { + const { SeatPushError } = require('../../../services/seatGithubPush'); + expect(executionErrorOutcome(new SeatPushError('non_fast_forward', 409))).toBe('refused'); + expect(executionErrorOutcome(new SeatPushError('provider_git_failed', 502))).toBe('failed'); + }); }); diff --git a/backend/routes/mcpGrants.ts b/backend/routes/mcpGrants.ts index c9e6d2d2f8..acc8ad2d4e 100644 --- a/backend/routes/mcpGrants.ts +++ b/backend/routes/mcpGrants.ts @@ -111,12 +111,8 @@ router.post('/:grantId/push', brokerRateLimit, agentRuntimeAuth, async ( if (!(await listToolsForGrant(identity)).some((tool) => tool.name === 'github.push_branch')) { return res.status(403).json({ error: 'operation_not_granted' }); } - const digest = createHash('sha256').update(identity.grantId).digest('hex'); - if (!await reserveBudgetLineage([ - { grantId: `host-gh:push:token:${req.agentTokenHash}`, calls: 2, windowMs: 60_000 }, - { grantId: `host-gh:push:grant:${digest}`, calls: 2, windowMs: 60_000 }, - ])) return res.status(429).json({ error: 'host_github_push_rate_limited' }); - const result = await callTool({ ...identity, tool: 'github.push_branch', args }); + const result = await callTool({ ...identity, agentTokenHash: req.agentTokenHash, + tool: 'github.push_branch', args }); return res.json({ callId: result.callId, outcome: result.outcome, result: result.result }); } catch (error) { const e = error as { statusCode?: number }; @@ -161,6 +157,7 @@ router.post('/:grantId/cli', brokerRateLimit, agentRuntimeAuth, async (req: expr } const result = await callTool({ ...identity, + agentTokenHash: req.agentTokenHash, tool: operation.tool, args, }); @@ -239,6 +236,7 @@ router.post('/:grantId', brokerRateLimit, agentRuntimeAuth, async (req: express. const result = await callTool({ grantId: String(req.params.grantId), agentUserId, + agentTokenHash: req.agentTokenHash, agentName, instanceId, tool, diff --git a/backend/services/seatGithubPush.ts b/backend/services/seatGithubPush.ts index b9c9c50f67..6ab2ab389b 100644 --- a/backend/services/seatGithubPush.ts +++ b/backend/services/seatGithubPush.ts @@ -97,6 +97,21 @@ export const pushWithLease = async (options: { } }; +/** Fetch both lines of provider history needed for bundle and ancestry proof. */ +export const fetchProviderObjects = async (options: { + git: (args: string[], cwd?: string, failure?: SeatPushError) => Promise; + url: string; + defaultRef: string; + ref: string; + oldOid: string; +}): Promise => { + const { git, url, defaultRef, ref, oldOid } = options; + await git(['fetch', '--no-tags', url, defaultRef]); + // A bundle may name the current seat tip as its prerequisite. Fetch that + // object even if its OID equals baseOid; it need not be on the default branch. + if (oldOid !== ZERO_OID) await git(['fetch', '--no-tags', url, ref]); +}; + /** All subprocess settings are backend-owned. The bundle controls no Git config, hook or remote. */ export const pushSeatBranch = async ( raw: Record, connection: GithubToolConnection, agentUserId: string, @@ -142,8 +157,7 @@ export const pushSeatBranch = async ( const current = await git(['ls-remote', url, ref]); const oldOid = current ? current.split(/\s+/, 1)[0] : ZERO_OID; if (!OID.test(oldOid)) throw new SeatPushError('provider_ref_invalid', 502); - await git(['fetch', '--no-tags', url, defaultRef]); - if (oldOid !== ZERO_OID && oldOid !== baseOid) await git(['fetch', '--no-tags', url, ref]); + await fetchProviderObjects({ git, url, defaultRef, ref, oldOid }); await writeFile(bundlePath, bytes, { mode: 0o600 }); await validateSeatBundle({ git, repo, bundlePath, baseOid, newOid, oldOid }); // The server, not the child, chooses the compare-and-swap lease. A ref diff --git a/backend/services/toolBrokerService.ts b/backend/services/toolBrokerService.ts index 346fa543e9..78b6041961 100644 --- a/backend/services/toolBrokerService.ts +++ b/backend/services/toolBrokerService.ts @@ -1,4 +1,4 @@ -import { randomUUID } from 'crypto'; +import { createHash, randomUUID } from 'crypto'; import Pod from '../models/Pod'; import Integration from '../models/Integration'; import RoomGrant, { IRoomGrant, RoomGrantWriteMode } from '../models/RoomGrant'; @@ -15,7 +15,7 @@ import { judgeSeatConfinement } from './seatGrantConfinement'; import { findHostedToolDefinition, hostedToolDefinitions } from './hostedMcpToolDefinitions'; import { HostedMcpCredentialError } from './hostedMcpCredentialError'; import { outsiderGrantPodRefusal, outsiderGrantRefusalMessage } from './outsiderGrantPodEligibility'; -import { parseSeatPushArgs, pushSeatBranch } from './seatGithubPush'; +import { parseSeatPushArgs, pushSeatBranch, SeatPushError } from './seatGithubPush'; // eslint-disable-next-line @typescript-eslint/no-require-imports const GitHubAppService = require('./githubAppService'); @@ -61,6 +61,7 @@ export interface GithubToolDefinition extends ToolDefinitionBase { prepareApproval?: ( args: Record, connection: GithubToolConnection, + agentUserId: string, ) => Promise>; call: (args: Record, connection: GithubToolConnection, agentUserId?: string) => Promise; } @@ -86,6 +87,8 @@ export type ToolDefinition = GithubToolDefinition | HostedToolDefinition; export interface BrokerCallInput { grantId: string; agentUserId: string; + /** Middleware-derived token hash; never a request-body field. */ + agentTokenHash?: string; agentName?: string; instanceId?: string; tool: string; @@ -139,6 +142,15 @@ const pullView = (pull: Record): Record => ({ ? String((pull.head as Record).ref || '') : undefined, }); +const assertOwnedSeatBranch = (head: unknown, agentUserId: string): void => { + const prefix = `seat/${agentUserId}/`; + if (typeof head !== 'string' || !agentUserId || !head.startsWith(prefix) + || head.length > 200 || head.split('/').some((part) => !/^[A-Za-z0-9][A-Za-z0-9._-]*$/.test(part) + || part.includes('..') || part.endsWith('.lock') || part.endsWith('.'))) { + throw new RoomGrantError('branch_not_owned_by_seat', 'PR head must belong to the authenticated seat', 403); + } +}; + const objectArgs = (args: unknown): Record => { if (!args || typeof args !== 'object' || Array.isArray(args)) { throw new RoomGrantError('invalid_tool_args', 'tool arguments must be an object', 400); @@ -327,13 +339,15 @@ const createPullRequest: GithubToolDefinition = { required: ['title', 'head', 'base'], additionalProperties: false, }, - async prepareApproval(rawArgs, connection) { + async prepareApproval(rawArgs, connection, agentUserId) { + assertOwnedSeatBranch(rawArgs.head, agentUserId); return pinConnectionRepository(rawArgs, connection); }, - async call(rawArgs, connection) { + async call(rawArgs, connection, agentUserId) { assertNoUnknown(rawArgs, ['title', 'head', 'base', 'body', 'draft']); const title = nonEmptyString(rawArgs.title, 'title'); const head = nonEmptyString(rawArgs.head, 'head'); + assertOwnedSeatBranch(head, agentUserId || ''); const base = nonEmptyString(rawArgs.base, 'base'); if (title.length > 256 || !/^[A-Za-z0-9][A-Za-z0-9._/-]{0,199}$/.test(head) || !/^[A-Za-z0-9][A-Za-z0-9._/-]{0,199}$/.test(base)) { @@ -1161,10 +1175,11 @@ const prepareApprovalFor = ( definition: ToolDefinition, args: Record, connection: ToolConnection, + agentUserId: string, ): Promise> => { if (definition.connectionType === 'hosted-mcp') return Promise.resolve(args); if (!definition.prepareApproval) return Promise.resolve(args); - return definition.prepareApproval(args, asGithubConnection(providerConnection(connection))); + return definition.prepareApproval(args, asGithubConnection(providerConnection(connection)), agentUserId); }; /** The definition a tool name resolves to: the seeded GitHub record, or a hosted catalogue entry. */ @@ -1190,6 +1205,7 @@ const stripVendorDiagnostics = (error: unknown): void => { }; export const executionErrorOutcome = (error: unknown): 'refused' | 'failed' | 'pending_approval' => { + if (error instanceof SeatPushError) return error.statusCode < 500 ? 'refused' : 'failed'; if (error instanceof HostedMcpCredentialError) { return error.code === 'reconnect_required' || error.code === 'connection_mismatch' ? 'refused' : 'failed'; } @@ -1341,6 +1357,18 @@ const budgetEntriesFor = async ( }); }; +/** Mandatory push budget shared by HTTP, MCP, and approved execution. */ +const seatPushBudgetEntries = (input: Pick) => { + const digest = (value: string) => createHash('sha256').update(value).digest('hex'); + const entry = (key: string) => ({ grantId: `host-gh:push:${key}`, calls: 2, windowMs: 60_000 }); + return [ + entry(`seat:${digest(input.agentUserId)}`), + entry(`grant:${digest(input.grantId)}`), + ...(input.agentTokenHash && /^[a-f0-9]{64}$/.test(input.agentTokenHash) + ? [entry(`token:${input.agentTokenHash}`)] : []), + ]; +}; + /** * Execute one brokered tool call. The only identity accepted here is the * middleware-derived agentUserId; a request argument with that name is never @@ -1385,6 +1413,7 @@ export const callTool = async (input: BrokerCallInput): Promise 0) { const reserved = await reserveBudgetLineage(budgetEntries); - if (!reserved) throw new RoomGrantError('budget_exhausted', 'grant call budget is exhausted', 403); + if (!reserved) throw new RoomGrantError( + input.tool === 'github.push_branch' ? 'host_github_push_rate_limited' : 'budget_exhausted', + 'grant call budget is exhausted', input.tool === 'github.push_branch' ? 429 : 403, + ); } const result = await runDefinition(definition, parsedArgs, connection, input.agentUserId); @@ -1600,9 +1633,14 @@ export const executeApprovedToolCall = async ( const executionArgs = definition.connectionType === 'github-app' ? providerArgsFromApprovedEnvelope(input.args, asGithubConnection(connection)) : input.args; + if (input.tool === 'github.push_branch') parseSeatPushArgs(executionArgs, input.agentUserId); const budgetEntries = await budgetEntriesFor(grant); + if (input.tool === 'github.push_branch') budgetEntries.push(...seatPushBudgetEntries(input)); if (budgetEntries.length > 0 && !await reserveBudgetLineage(budgetEntries)) { - throw new RoomGrantError('budget_exhausted', 'grant call budget is exhausted', 403); + throw new RoomGrantError( + input.tool === 'github.push_branch' ? 'host_github_push_rate_limited' : 'budget_exhausted', + 'grant call budget is exhausted', input.tool === 'github.push_branch' ? 429 : 403, + ); } const result = await runDefinition(definition, executionArgs, connection, input.agentUserId); const recorded = await recordExecutionResult( From c6c8bf61812687f18c518ba24243c0f0036b7aeb Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Fri, 23 Oct 2026 14:47:53 -0700 Subject: [PATCH 11/12] Trim provider ref service test whitespace Co-authored-by: kai Agent-Model: codex/gpt-6-sol/medium Agent-Model: codex/gpt-6.1-sol/medium --- backend/__tests__/service/toolCallProviderRef.test.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/backend/__tests__/service/toolCallProviderRef.test.js b/backend/__tests__/service/toolCallProviderRef.test.js index cb864f1651..07bda38275 100644 --- a/backend/__tests__/service/toolCallProviderRef.test.js +++ b/backend/__tests__/service/toolCallProviderRef.test.js @@ -49,12 +49,12 @@ run('ToolCall provider ref audit on real PostgreSQL', () => { const { rows } = await observer.query(query, [GRANT_ID]); expect(rows).toEqual([{ provider_repo: 'Team-Commonly/commonly', - provider_ref: 'refs/heads/seat/seat-a/work', + provider_ref: 'refs/heads/seat/seat-a/work', }]); const [line] = await ToolCall.listForGrant(GRANT_ID); expect(line).toMatchObject({ providerRepo: 'Team-Commonly/commonly', - providerRef: 'refs/heads/seat/seat-a/work', + providerRef: 'refs/heads/seat/seat-a/work', }); }); @@ -95,7 +95,7 @@ run('ToolCall provider ref audit on real PostgreSQL', () => { ); expect(rows[0]).toMatchObject({ provider_repo: 'Team-Commonly/commonly', - provider_ref: 'refs/heads/seat/seat-a/work', + provider_ref: 'refs/heads/seat/seat-a/work', }); } finally { await client.query('RESET search_path'); From f22c765cdbbb6bfd381906dc5e5a1ef39e62423d Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sat, 24 Oct 2026 05:39:24 -0700 Subject: [PATCH 12/12] fix: refuse brokered pushes to automation policy paths Co-authored-by: kai Agent-Model: codex/gpt-6.1-sol/medium --- .../unit/services/seatGithubPush.test.js | 55 ++++++++++++++++++- backend/services/seatGithubPush.ts | 15 +++++ 2 files changed, 68 insertions(+), 2 deletions(-) diff --git a/backend/__tests__/unit/services/seatGithubPush.test.js b/backend/__tests__/unit/services/seatGithubPush.test.js index 266d5c5836..1a53e756e3 100644 --- a/backend/__tests__/unit/services/seatGithubPush.test.js +++ b/backend/__tests__/unit/services/seatGithubPush.test.js @@ -1,7 +1,7 @@ const { execFile } = require('child_process'); -const { mkdtemp, rm, writeFile } = require('fs/promises'); +const { mkdtemp, mkdir, rename, rm, writeFile } = require('fs/promises'); const { tmpdir } = require('os'); -const { join } = require('path'); +const { dirname, join } = require('path'); const { promisify } = require('util'); const { parseSeatPushArgs, fetchProviderObjects, pushWithLease, validateSeatBundle } = require('../../../services/seatGithubPush'); @@ -126,4 +126,55 @@ describe('seat GitHub push proof', () => { expect(() => parseSeatPushArgs({ ...args, repository: 'other/repo' }, 'agent123')) .toThrow('push_args_invalid'); }); + + it.each([ + ['.github/workflows/seat.yml', false], + ['.github/actions/seat/action.yml', true], + ['CODEOWNERS', false], + ['.github/CODEOWNERS', true], + ['docs/CODEOWNERS', false], + ])('refuses protected path %s before a provider ref update (fresh=%s)', async (file, fresh) => { + await mkdir(dirname(join(source, file)), { recursive: true }); + await writeFile(join(source, file), 'unapproved policy\n'); + await git(['add', '.'], source); + await git(['commit', '-m', 'policy change'], source); + const tip = await git(['rev-parse', 'HEAD'], source); + await git(['bundle', 'create', bundlePath, 'HEAD', `^${baseOid}`], source); + const ref = 'refs/heads/seat/agent123/work'; + if (!fresh) await git(['update-ref', ref, oldOid], receiver); + const providerGit = jest.fn((args, cwd, failure) => git(args, cwd, failure)); + await expect((async () => { + await validateSeatBundle({ git, repo: receiver, bundlePath, baseOid, + oldOid: fresh ? '0'.repeat(40) : oldOid, newOid: tip }); + await pushWithLease({ git: providerGit, repo: receiver, url: receiver, ref, + oldOid: fresh ? '0'.repeat(40) : oldOid, newOid: tip }); + })()).rejects.toMatchObject({ code: 'push_protected_path', statusCode: 403 }); + expect(providerGit).not.toHaveBeenCalled(); + expect(await git(['for-each-ref', '--format=%(objectname)', ref], receiver)) + .toBe(fresh ? '' : oldOid); + }); + + it.each(['rename', 'delete', 'revert'])('refuses a protected path %s even when the final name is harmless', async (operation) => { + const file = join(source, '.github/workflows/seat.yml'); + await mkdir(dirname(file), { recursive: true }); + await writeFile(file, 'workflow\n'); + await git(['add', '.'], source); + await git(['commit', '-m', 'workflow baseline'], source); + const policyBase = await git(['rev-parse', 'HEAD'], source); + await git(['fetch', source, policyBase], receiver); + if (operation === 'rename') await rename(file, join(source, 'ordinary.yml')); + else if (operation === 'delete') await rm(file); + else await writeFile(file, 'changed workflow\n'); + await git(['add', '-A'], source); + await git(['commit', '-m', operation], source); + if (operation === 'revert') { + await writeFile(file, 'workflow\n'); + await git(['commit', '-am', 'restore workflow'], source); + } + const tip = await git(['rev-parse', 'HEAD'], source); + await git(['bundle', 'create', bundlePath, 'HEAD', `^${policyBase}`], source); + await expect(validateSeatBundle({ git, repo: receiver, bundlePath, + baseOid: policyBase, oldOid: policyBase, newOid: tip })) + .rejects.toMatchObject({ code: 'push_protected_path', statusCode: 403 }); + }); }); diff --git a/backend/services/seatGithubPush.ts b/backend/services/seatGithubPush.ts index 6ab2ab389b..fceb04e86e 100644 --- a/backend/services/seatGithubPush.ts +++ b/backend/services/seatGithubPush.ts @@ -74,6 +74,21 @@ export const validateSeatBundle = async (options: { new SeatPushError('non_fast_forward', 409)); if (oldOid !== ZERO_OID) await git(['merge-base', '--is-ancestor', oldOid, newOid], repo, new SeatPushError('non_fast_forward', 409)); + // Inspect names without rename detection and with NUL separators: a rename, + // deletion or newline in a filename must not hide an automation-policy edit. + const baseline = oldOid === ZERO_OID ? baseOid : oldOid; + const changed = await git(['diff', '--name-only', '-z', '--no-renames', baseline, newOid], repo); + // Also refuse an edit reverted later in the same bundle, including merge + // resolutions. Every newly introduced commit is part of the pushed history. + const history = await git(['log', '--format=', '--name-only', '-z', '--no-renames', + '--diff-merges=first-parent', `${baseline}..${newOid}`], repo); + if (`${changed}\0${history}`.split('\0').some((name) => { + const file = name.replace(/^\n+/, ''); + return /^\.github(?:\/(?:workflows|actions)(?:\/|$)|$)/i.test(file) + || /(?:^|\/)CODEOWNERS$/i.test(file); + })) { + throw new SeatPushError('push_protected_path', 403); + } }; /** Provider CAS is independent of client push flags or advertised remote state. */