fix(security): CSO 2026-08-09 — terminal-tier HIGH + access-control + supply-chain hardening - #2081
Merged
Merged
Conversation
… supply-chain hardening
Addresses the /cso full audit (docs/security-reports/cso-2026-08-09.md).
- H1 (HIGH): gate the agent terminal WebSocket at owner/admin tier
(can_user_share_agent), not accessor tier — a chat-only shared collaborator
could open /bin/bash in the container and dump CLAUDE_CODE_OAUTH_TOKEN /
TRINITY_MCP_API_KEY / .env. Mirrors the deliberately admin-only SSH endpoint.
- M1: move schedule enable/disable/trigger to OwnedAgentByName (matching
update/delete) so shared users can't flip owner-intent schedule state.
- L1: add assert_agent_access to GET /event-subscriptions/{id}, the owner
check its PUT/DELETE siblings already enforce.
- M3: npm audit fix across mcp-server/helper-mcp/frontend (clears HIGH
ip-address SSRF + undici desync advisories); raise ip-address override
floor to >=10.5.0. 0 vulnerabilities in all three; tsc + vite build pass.
- M4: npm install -> npm ci in mcp-server + frontend prod Dockerfiles,
restoring the tracked lockfile as a supply-chain control.
- M5 (partial): pin backend aiohttp==3.14.3, slackify-markdown==0.2.4.
- M9/L5: least-privilege `permissions: contents: read` on frontend-build
and deploy-dev workflows.
Held for a build/CI-verified pass (see report): base-image pip pins + Go
tarball SHA, first-party action SHA-pinning, dev frontend Dockerfile.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
vybe
approved these changes
Aug 10, 2026
vybe
left a comment
vybe
left a comment
Contributor
There was a problem hiding this comment.
Reviewed the full diff. Approving including M1.
H1 (terminal WebSocket privilege escalation) — this is the one that matters and the fix is right. Gating a /bin/bash container shell at accessor tier meant a chat-only shared collaborator could dump CLAUDE_CODE_OAUTH_TOKEN / TRINITY_MCP_API_KEY / .env — RCE as the agent plus lateral movement via the MCP key. Moving to can_user_share_agent matches the deliberately admin-only SSH endpoint, which is the correct precedent: a shell is not an accessor-tier capability.
M1 (schedule enable/disable/trigger → OwnedAgentByName) — merging as audited. This is a deliberate, user-visible tightening: shared collaborators lose the ability to flip schedule state. It's the right call because update/delete already required ownership, so accessor-tier enable/disable/trigger was an inconsistency that let a shared user override owner intent and burn LLM spend. Needs a release note — this is a behaviour change, not a pure fix.
L1 — GET /event-subscriptions/{id} gaining assert_agent_access simply matches the gate its PUT/DELETE siblings already enforce.
Queued behind #1976, which touches routers/schedules.py too.
One process note, not a blocker: this PR carries no closing keyword, so nothing auto-promotes. The committed docs/security-reports/cso-2026-08-09.{md,json} are the durable record, which is the right shape for an audit sweep spanning several unrelated findings.
4 tasks
dolho
added a commit
that referenced
this pull request
Aug 11, 2026
`POST /api/agents/{name}/schedules/{id}/enable|disable|trigger` returned 422 to
EVERY caller, owner included — breaking the REST API, the frontend Schedules tab
toggle and Run-now, and the MCP `toggle_agent_schedule` / `trigger_agent_schedule`
tools.
flip owner-intent schedule state. Right intent, wrong variant: `OwnedAgentByName`
resolves `get_owned_agent_by_name(agent_name: str = Path(...))`, and these routes
declare `{name}`. FastAPI can never satisfy that dependency, so it rejects the
request during validation, before the handler runs.
The two variants exist for exactly the two path-param spellings, so the fix keeps
M1's tier and changes only the spelling: `OwnedAgent` is the same owner/admin
check reading `name`.
VERIFIED AGAINST A RUNNING BACKEND, NOT JUST THE ANNOTATION
Two throwaway containers on the real Postgres/Redis — one on unpatched `dev`,
one on this branch — same request:
dev (unpatched) enable 422 [{"type":"missing","loc":["path","agent_name"]}]
dev (unpatched) disable 422 [{"type":"missing","loc":["path","agent_name"]}]
dev (unpatched) trigger 422 [{"type":"missing","loc":["path","agent_name"]}]
fix enable 404 'Agent not found'
fix disable 404 'Agent not found'
fix trigger 404 'Agent not found'
Byte-identical to the payload in the issue on the left; the uniform 404 of
Invariant #8 on the right. Both containers were torn down afterwards.
TWO GUARDS (AC4), because one is not enough
`test_2094_dependency_path_param_pairing.py` — static, covers all 146 uses of
the four aliases across every router. Static by design: `test_1483_route_order.py`
documents what importing `main` costs — it skipped ALWAYS, standalone included,
for months behind a message blaming the wrong cause, and a guard that can
silently skip is not a guard. It also asserts the scan found >100 pairings and
all four aliases, because a guard matching nothing certifies nothing.
`test_2094_schedule_control_routes_bind.py` — functional, drives the real router
through TestClient. A rule about annotations is a proxy for "the route works";
this is the thing itself. It also pins that the OWNER gate still resolves, so the
obvious wrong fix — drop to `AuthorizedAgent`, 422 disappears, #2081's hole
returns — fails here.
Mutation-tested: unpatched dev -> 7 failed; accessor-tier fix -> caught;
fixed -> 11 passed.
Two things the functional test had to survive, both recorded at the site: my
first draft mounted the router with a prefix it already carries, double-prefixing
every request into Starlette's generic 404 — which a loose `"not found"` substring
happily matched, so both tests passed against BROKEN code; it now asserts the
handler's exact `"Schedule not found"`. And dependency identity is compared by
qualified NAME, because `dependencies` is on conftest's #762 invariant-restore
list and a fresh import can hand back a different module object than
`routers.schedules` bound at its own import.
Not run: the 7 integration tests the issue names. They need `TEST_AGENT_NAME`
and skip rather than fail, and the isolated backend has no Docker socket so it
sees no agents. The live contrast above covers the same ground more directly.
Fixes #2094
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dolho
added a commit
that referenced
this pull request
Aug 11, 2026
`POST /api/agents/{name}/schedules/{id}/enable|disable|trigger` returned 422 to
EVERY caller, owner included — breaking the REST API, the frontend Schedules tab
toggle and Run-now, and the MCP `toggle_agent_schedule` / `trigger_agent_schedule`
tools.
flip owner-intent schedule state. Right intent, wrong variant: `OwnedAgentByName`
resolves `get_owned_agent_by_name(agent_name: str = Path(...))`, and these routes
declare `{name}`. FastAPI can never satisfy that dependency, so it rejects the
request during validation, before the handler runs.
The two variants exist for exactly the two path-param spellings, so the fix keeps
M1's tier and changes only the spelling: `OwnedAgent` is the same owner/admin
check reading `name`.
VERIFIED AGAINST A RUNNING BACKEND, NOT JUST THE ANNOTATION
Two throwaway containers on the real Postgres/Redis — one on unpatched `dev`,
one on this branch — same request:
dev (unpatched) enable 422 [{"type":"missing","loc":["path","agent_name"]}]
dev (unpatched) disable 422 [{"type":"missing","loc":["path","agent_name"]}]
dev (unpatched) trigger 422 [{"type":"missing","loc":["path","agent_name"]}]
fix enable 404 'Agent not found'
fix disable 404 'Agent not found'
fix trigger 404 'Agent not found'
Byte-identical to the payload in the issue on the left; the uniform 404 of
Invariant #8 on the right. Both containers were torn down afterwards.
TWO GUARDS (AC4), because one is not enough
`test_2094_dependency_path_param_pairing.py` — static, covers all 146 uses of
the four aliases across every router. Static by design: `test_1483_route_order.py`
documents what importing `main` costs — it skipped ALWAYS, standalone included,
for months behind a message blaming the wrong cause, and a guard that can
silently skip is not a guard. It also asserts the scan found >100 pairings and
all four aliases, because a guard matching nothing certifies nothing.
`test_2094_schedule_control_routes_bind.py` — functional, drives the real router
through TestClient. A rule about annotations is a proxy for "the route works";
this is the thing itself. It also pins that the OWNER gate still resolves, so the
obvious wrong fix — drop to `AuthorizedAgent`, 422 disappears, #2081's hole
returns — fails here.
Mutation-tested: unpatched dev -> 7 failed; accessor-tier fix -> caught;
fixed -> 11 passed.
Two things the functional test had to survive, both recorded at the site: my
first draft mounted the router with a prefix it already carries, double-prefixing
every request into Starlette's generic 404 — which a loose `"not found"` substring
happily matched, so both tests passed against BROKEN code; it now asserts the
handler's exact `"Schedule not found"`. And dependency identity is compared by
qualified NAME, because `dependencies` is on conftest's #762 invariant-restore
list and a fresh import can hand back a different module object than
`routers.schedules` bound at its own import.
Not run: the 7 integration tests the issue names. They need `TEST_AGENT_NAME`
and skip rather than fail, and the isolated backend has no Docker socket so it
sees no agents. The live contrast above covers the same ground more directly.
Fixes #2094
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dolho
added a commit
that referenced
this pull request
Aug 11, 2026
`POST /api/agents/{name}/schedules/{id}/enable|disable|trigger` returned 422 to
EVERY caller, owner included — breaking the REST API, the frontend Schedules tab
toggle and Run-now, and the MCP `toggle_agent_schedule` / `trigger_agent_schedule`
tools.
flip owner-intent schedule state. Right intent, wrong variant: `OwnedAgentByName`
resolves `get_owned_agent_by_name(agent_name: str = Path(...))`, and these routes
declare `{name}`. FastAPI can never satisfy that dependency, so it rejects the
request during validation, before the handler runs.
The two variants exist for exactly the two path-param spellings, so the fix keeps
M1's tier and changes only the spelling: `OwnedAgent` is the same owner/admin
check reading `name`.
VERIFIED AGAINST A RUNNING BACKEND, NOT JUST THE ANNOTATION
Two throwaway containers on the real Postgres/Redis — one on unpatched `dev`,
one on this branch — same request:
dev (unpatched) enable 422 [{"type":"missing","loc":["path","agent_name"]}]
dev (unpatched) disable 422 [{"type":"missing","loc":["path","agent_name"]}]
dev (unpatched) trigger 422 [{"type":"missing","loc":["path","agent_name"]}]
fix enable 404 'Agent not found'
fix disable 404 'Agent not found'
fix trigger 404 'Agent not found'
Byte-identical to the payload in the issue on the left; the uniform 404 of
Invariant #8 on the right. Both containers were torn down afterwards.
TWO GUARDS (AC4), because one is not enough
`test_2094_dependency_path_param_pairing.py` — static, covers all 146 uses of
the four aliases across every router. Static by design: `test_1483_route_order.py`
documents what importing `main` costs — it skipped ALWAYS, standalone included,
for months behind a message blaming the wrong cause, and a guard that can
silently skip is not a guard. It also asserts the scan found >100 pairings and
all four aliases, because a guard matching nothing certifies nothing.
`test_2094_schedule_control_routes_bind.py` — functional, drives the real router
through TestClient. A rule about annotations is a proxy for "the route works";
this is the thing itself. It also pins that the OWNER gate still resolves, so the
obvious wrong fix — drop to `AuthorizedAgent`, 422 disappears, #2081's hole
returns — fails here.
Mutation-tested: unpatched dev -> 7 failed; accessor-tier fix -> caught;
fixed -> 11 passed.
Two things the functional test had to survive, both recorded at the site: my
first draft mounted the router with a prefix it already carries, double-prefixing
every request into Starlette's generic 404 — which a loose `"not found"` substring
happily matched, so both tests passed against BROKEN code; it now asserts the
handler's exact `"Schedule not found"`. And dependency identity is compared by
qualified NAME, because `dependencies` is on conftest's #762 invariant-restore
list and a fresh import can hand back a different module object than
`routers.schedules` bound at its own import.
Not run: the 7 integration tests the issue names. They need `TEST_AGENT_NAME`
and skip rather than fail, and the isolated backend has no Docker socket so it
sees no agents. The live contrast above covers the same ground more directly.
Fixes #2094
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
vybe
pushed a commit
that referenced
this pull request
Aug 11, 2026
…) (#2095) * fix(schedules): pair the owner gate with the path param it reads (#2094) `POST /api/agents/{name}/schedules/{id}/enable|disable|trigger` returned 422 to EVERY caller, owner included — breaking the REST API, the frontend Schedules tab toggle and Run-now, and the MCP `toggle_agent_schedule` / `trigger_agent_schedule` tools. flip owner-intent schedule state. Right intent, wrong variant: `OwnedAgentByName` resolves `get_owned_agent_by_name(agent_name: str = Path(...))`, and these routes declare `{name}`. FastAPI can never satisfy that dependency, so it rejects the request during validation, before the handler runs. The two variants exist for exactly the two path-param spellings, so the fix keeps M1's tier and changes only the spelling: `OwnedAgent` is the same owner/admin check reading `name`. VERIFIED AGAINST A RUNNING BACKEND, NOT JUST THE ANNOTATION Two throwaway containers on the real Postgres/Redis — one on unpatched `dev`, one on this branch — same request: dev (unpatched) enable 422 [{"type":"missing","loc":["path","agent_name"]}] dev (unpatched) disable 422 [{"type":"missing","loc":["path","agent_name"]}] dev (unpatched) trigger 422 [{"type":"missing","loc":["path","agent_name"]}] fix enable 404 'Agent not found' fix disable 404 'Agent not found' fix trigger 404 'Agent not found' Byte-identical to the payload in the issue on the left; the uniform 404 of Invariant #8 on the right. Both containers were torn down afterwards. TWO GUARDS (AC4), because one is not enough `test_2094_dependency_path_param_pairing.py` — static, covers all 146 uses of the four aliases across every router. Static by design: `test_1483_route_order.py` documents what importing `main` costs — it skipped ALWAYS, standalone included, for months behind a message blaming the wrong cause, and a guard that can silently skip is not a guard. It also asserts the scan found >100 pairings and all four aliases, because a guard matching nothing certifies nothing. `test_2094_schedule_control_routes_bind.py` — functional, drives the real router through TestClient. A rule about annotations is a proxy for "the route works"; this is the thing itself. It also pins that the OWNER gate still resolves, so the obvious wrong fix — drop to `AuthorizedAgent`, 422 disappears, #2081's hole returns — fails here. Mutation-tested: unpatched dev -> 7 failed; accessor-tier fix -> caught; fixed -> 11 passed. Two things the functional test had to survive, both recorded at the site: my first draft mounted the router with a prefix it already carries, double-prefixing every request into Starlette's generic 404 — which a loose `"not found"` substring happily matched, so both tests passed against BROKEN code; it now asserts the handler's exact `"Schedule not found"`. And dependency identity is compared by qualified NAME, because `dependencies` is on conftest's #762 invariant-restore list and a fresh import can hand back a different module object than `routers.schedules` bound at its own import. Not run: the 7 integration tests the issue names. They need `TEST_AGENT_NAME` and skip rather than fail, and the isolated backend has no Docker socket so it sees no agents. The live contrast above covers the same ground more directly. Fixes #2094 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(tests): key the dependency overrides on what the ROUTES resolve (#2094) The full unit tier failed 3 of my own new tests — standalone they passed: test_the_handler_actually_ran[enable|disable|trigger] AssertionError: .../enable returned 401, expected the handler's own 404; body: {"detail":"Not authenticated"} `dependency_overrides` is an identity-keyed dict. The fixture keyed it on `dependencies.get_owned_agent` from a fresh `import dependencies` — but `dependencies` is on conftest's #762 `_SYS_MODULES_INVARIANT_KEYS` list, so its sys.modules entry is restored to a baseline between tests, and a re-import can hand back a DIFFERENT module object with different function objects than `routers.schedules` bound at its own import. The override then silently does not apply, the real auth chain runs, and every request 401s. Overrides are now keyed on the exact callables in each route's dependant graph, which are by construction the objects FastAPI looks up at request time. Module identity cannot matter because the module is never consulted. Staged the divergence deliberately rather than hunting the predecessor (the same search failed earlier today on a sibling case): OLD (import identity) -> HTTP 401 {"detail":"Not authenticated"} NEW (route identity) -> HTTP 404 {"detail":"Schedule not found"} A `>= 2` assertion on the override count now fails in the fixture if the name-matching ever stops matching, instead of surfacing as confusing 401s in every test below. Worth naming, because it is the third instance of one class today: the same conftest restore produced the ent#356 portal-test failures and the qualname comparison two commits ago. I fixed the ASSERTION identity in this very file and left the OVERRIDE identity — the docstring even called object-keying a feature ("a future swap would leave the override unapplied"), which is precisely the vulnerability. Any object-keyed registry over a module-level function is exposed; `dependency_overrides` and `monkeypatch.setattr` are the two in this repo. I also pushed the previous commit before this tier finished. It was not done. Related to #2094 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(tests): the pairing guard missed 40 routes — the Depends() form (#2094) Found reviewing my own PR, and it is the guard committing the error it exists to catch. These four gates are attached two ways, and both are in live use: name: OwnedAgent # 146 Annotated aliases name: str = Depends(get_owned_agent) # 40 Depends defaults The collector only read `ast.Name` annotations, so it scanned the first form and was blind to the second — then reported a clean pass. Forty routes were unguarded, every credential endpoint among them, while the test that exists to prove the pairing is safe said it was. None of the 40 is mispaired today, so no live bug. But #2094's AC asks that "the next OwnedAgent<->OwnedAgentByName swap can't ship silently", and in the Depends form it would have — silently, with a green guard, which is worse than no guard. Coverage 146 -> 186. Mutation-tested on the newly covered form specifically, because counting them proves only that they are counted: credentials.py:100 get_owned_agent_by_name -> get_owned_agent -> credentials.py:99 get_agent_env_drift() — route '/agents/{agent_name}/credentials/env-drift' uses Depends(get_owned_agent) (reads path param 'name') but declares ['{agent_name}']; every request 422s The completeness test now requires BOTH families to be present, not just the four aliases. Losing the Depends() scan again would fail there instead of quietly halving the coverage — the same "a guard matching nothing certifies nothing" reasoning the file already applied to the alias family, which I had applied to one family and not the other. Related to #2094 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(learnings): one-syntax guards, and identity-keyed registries (#2094) Two durable classes from the #2094 review, both of which shipped green: a static guard that scanned one of two equivalent syntaxes and reported a clean pass over the 40 routes it could not see, and an identity-keyed registry (dependency_overrides) defeated by conftest's module-object restore. Written for the /autoplan reader. Related to #2094 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(1968): read trigger_schedule via AST, not a 5000-char window `_backend_trigger_endpoint()` sliced a fixed 5000 characters from the `async def trigger_schedule(` marker — a byte count standing in for a function boundary. It broke the moment the handler grew: this branch adds a four-line comment explaining why the route uses `OwnedAgent` rather than `OwnedAgentByName` (#2094), which pushed the `execution_id` relay from 4835 to 5071 characters into the function. 71 past the cliff, and the assertion started failing against code that is completely correct — the relay is present, twice. A test that fails when a COMMENT is added is measuring the wrong thing. The property under test is "this handler relays execution_id", so read the handler: resolve it by AST and assert over its full source. The window is gone, so the guard no longer has an expiry date measured in characters. Verified load-bearing by mutation: deleting both relay lines from routers/schedules.py still fails the test. Related to #2094 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: trinity-ability <309458136+trinity-ability@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes surfaced by the
/cso fullaudit (docs/security-reports/cso-2026-08-09.md). One HIGH plus access-control and supply-chain hardening. The report + structured JSON are included in this PR.HIGH
services/agent_service/terminal.pygated a/bin/bashcontainer shell at accessor tier (can_user_access_agent), so a chat-only shared collaborator could open the terminal against a running agent and dumpCLAUDE_CODE_OAUTH_TOKEN/TRINITY_MCP_API_KEY/.env(RCE as the agent, lateral movement via the MCP key). Now gated at owner/admin tier (can_user_share_agent), matching the deliberately admin-only SSH endpoint. Confirmed prod-reachable + exploitable by two independent fresh-context refutations.Access control
enable/disable/triggermoved fromAuthorizedAgent→OwnedAgentByName(they were accessor-tier whileupdate/deleterequired ownership; a shared user could flip owner-intent schedule state and burn LLM spend).GET /event-subscriptions/{id}gainsassert_agent_access(the owner check itsPUT/DELETEsiblings already enforce; bounded IDOR by ~96-bit id, but the asymmetry is real).Supply chain
npm audit fixon mcp-server + helper-mcp + frontend clears the HIGHip-address(SSRF) andundici(response-desync) advisories in the MCP prod tree;ip-addressoverride floor raised to>=10.5.0. 0 vulnerabilities in all three trees.npm install→npm ciin the mcp-server and frontend prod Dockerfiles, restoring the tracked lockfile as a control.Dockerfilepinsaiohttp==3.14.3,slackify-markdown==0.2.4(were floating amid an otherwise exact-pinned set).CI hygiene
permissions: contents: readadded tofrontend-build.ymlanddeploy-dev.yml.Verification
py_compileon all changed backend files;test_186_enumeration_uniformity(36) andtest_subscription_bolapass.npm audit→ 0 vulnerabilities on mcp-server / helper-mcp / frontend; mcp-servertscand frontendvite buildboth succeed;npm ciexits 0 (lockfiles in sync).Full integration verification of the H1/M1 auth changes (shared-user 403 vs owner 200) runs on CI.
Deliberately held (need a build/CI-verified pass — see report)
🤖 Generated with Claude Code