Skip to content

Merge and issue write handlers run blocking git subprocesses with no deadline while holding a write guard (write-side sibling of #204) #424

Description

@PierrunoYT

Sibling of #204, which deliberately scoped itself to the read and fork handlers. The same defect is present on the merge and issue write paths, and there it holds a repository write resource while it blocks.

Merge

api/pulls.rs:222 is an async handler that calls store::merge_branch directly. That function (git/store.rs:795) runs several std::process::Command::output() calls in sequence. output() blocks the worker thread, and none of the calls carries a deadline — git_service_timeout_secs is not consulted on this path, exactly as #204 describes for read and fork.

Issue writes

api/issues.rs calls the synchronous helpers in git/issues.rscreate_issue, close_issue, get_issue, resolve_issue_id — each of which is a bare Command::output(). These run while a write guard is held.

Why the write side is worse than #204's read side

Three differences, and the third is the one that matters:

  1. A merge is a heavier Git operation than a log or ls-tree, so the blocking interval is longer for the same disk.
  2. The write guard is retained for the duration, so a stuck subprocess denies the repo to every other writer, not just to itself.
  3. These routes carry no rate limit. server.rs:114 says so directly — "Write routes — require HTTP Signature (no rate limit)" — and api/issues.rs's routes are in the equally unbraked issue_write_routes group at server.rs:284. A signature is free to obtain, and server.rs:307 already states the consequence in another context: "a signature alone does not cap cost — a did:key farm self-registers (INV-10)".

So the ceiling that makes #204's read-path exposure bounded in practice is absent here. This overlaps the "State damage, and unthrottled" list in #253, which reaches the same routes from the replay direction.

Fix direction

spawn_blocking alone is not sufficient and is worth saying explicitly, because it is the obvious first move: it frees the worker but leaves the subprocess unbounded and the write guard held. The subprocess needs a deadline and descendant cleanup as well, and the guard must be retained until teardown completes rather than released when the future is dropped.

The node already has the machinery — the bounded execution path in git/smart_http.rs with its process-group handling, and the admission permits in AppState. The fix is routing these handlers through it rather than building something new. Consolidating on one bounded subprocess mechanism would close #204 and this together.

A rate limit on the affected routers is a separate, smaller change that is worth doing independently of the execution work, since it bounds the exposure without touching the Git paths.

Validation status

Verified by reading the handlers and tracing the call chain to the Command sites. Not verified by executing a hung merge against a running node. The cost claims follow from the blocking calls being on the async path, not from measurement.

Disclosure note

Originally submitted through private vulnerability reporting and filed publicly at the maintainer's direction, on the assessment that the exposure is availability-only and reachable to actors the threat model already treats as untrusted — the same reasoning #253 gives for filing in the open.


Found during an external audit pass.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    kind:securityVulnerability fix or hardeningsev:highMajor break or real security/trust risk, no easy workaroundsubsystem:apiNode REST API request/response surfacesubsystem:storageBlob/object store, Arweave, IPFS, archives

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions