Skip to content

Security review (issue-274)

  • Mechanism: the checklist in skills/the-loop/reference/security.md (security.review.mechanism: auto → no built-in security-review skill was available in this session, so the checklist is the recorded fallback).
  • Effective risk tier: 3 (autonomy.defaultTier; no sensitivePaths touched — no schema, no .github/workflows/**, no harness-config.yaml). Below security.review.humanSignOffMinTier: 4, so no named human security sign-off is required; human approval of the pull request still applies (tier 3 = human-approves-pr).
  • Reviewed against the diff: cli/the_loop/core/sessions.py, cli/the_loop/commands/sessions_cmd.py, cli/the_loop/api/routes.py, cli/the_loop/api/mcp.py, cli/the_loop/sdk/client.py, cli/the_loop/eventlog.py, docs/api-specs/openapi/the-loop.v1.yaml and the documentation.
  • Outcome: pass, no findings.

Checklist

#ItemVerdictEvidence in the diff
1every trust boundary in design.md §Security design is enforced where the design says it ispassone mechanism each, in the order the function runs them: WorkItemRef.parse before anything is opened; the self-link refusal before the registry is touched; find_by_work_item(...) is None → return, before any write; and the write itself is the unchanged SessionRegistry.link_pull_request
2untrusted inputs validated/constrained at their ingress; injection surfaces coveredpassno webhook payload reaches this code — the dispatcher's payload-derived binding path (_record_pr_binding) is untouched. The two inputs are refs from an operator's shell or an authenticated API client, both parsed by WorkItemRef.parse (which is what bounds owner/repo/host/number shape) before the registry is opened; the file name is WorkItemRef.slug, the same sanitised derivation the registry has always used. --pull-request accepts a bare number only through ^#?(\d+)$, and a non-positive number is refused
3untrusted content cannot steer privileged behaviourpassthe operation writes one endpoint on an existing, live record. It cannot create a record, cannot arm a work item, cannot spawn or resume a session, cannot post anything remote, and cannot widen authorizedUsers. Every delivery it makes possible still passes the unchanged routing.control gate and the unchanged named-actor check on control keywords
4no secrets in code, config, logs or fixturespassthe reused session.pr_linked event carries a work-item ref and a pull-request ref — no tokens, no bodies, no hostnames beyond the ref's own host, which is the operator's own GitHub host
5AuthZ checks fail closedpassreachability is the authorization here, and it is not widened: local shell access (which already reaches sessions start/stop/register --force) or an API credential on the same authenticated surface as the sibling register/close routes. On no record, the operation fails closed — nothing written, exit 1
6least privilegepassno new file, network or process access; no gh invocation; no filesystem path accepted over HTTP (the body is two refs), matching the deliberate rule on the existing session routes
7every abuse case from the requirements has a passing negative testpass"can it invent a work item?" → test_link_pull_request_without_a_session_record_writes_nothing; "can a work item deliver itself?" → test_link_pull_request_refuses_a_work_item_delivering_itself; "does a malformed ref reach the filesystem?" → test_link_pull_request_refuses_malformed_input (six cases, each asserting nothing was written)
8new dependencies justified and from trusted sourcesn/anone added

Abuse case, stated

An attacker who can call this operation can point a pull request's events at a session they do not own. They can already do strictly more: the operation requires local shell access or an API credential, either of which also reaches sessions start, sessions stop and sessions register --force. It adds no reachability those do not already grant, and unlike them it performs no remote action and destroys nothing.

Residual risk, accepted and recorded

There is no unlink verb (design.md §Trade-offs, decision-098 D11), so a binding written in error is corrected with sessions reset, which forgets this machine's whole record for the work item. That is a heavier remedy than the mistake, and it is the existing one — adding a remover to fix a bug about a missing writer is scope this change has not earned.

Released under the MIT License.