feat(delivery): git workspaces + SSRF host check (v2 phase A3-2)
The platform's side of D6: git runs on the host, in a gitdir that is never mounted into a container, so repository credentials never reach the model. - adapters/git_workspace.py: LocalGitWorkspaceAdapter implementing RepoWorkspacePort. Bare shared gitdir + a dev work tree mounted at /workspace + one linked worktree per task under work/.worktrees/<id> (excluded via the gitdir's info/exclude, not the project's .gitignore). Explicit --git-dir/--work-tree, core.hooksPath=/dev/null, fsmonitor off, quotePath off, GIT_CONFIG_NOSYSTEM, a private HOME per project, prompts off, one asyncio.Lock per project. Token via GIT_ASKPASS reading a 0600 file; SSH key as a 0600 temp file with a pinned known_hosts. Everything deleted in a finally; only a *path* ever travels through the environment. - The bug this phase existed to find: --git-dir=<shared> does not address a linked worktree, so `git commit` in a task worktree committed straight to `dev`. Operations now address <gitdir>/worktrees/<task_id> explicitly — and never read the worktree's .git file, which a repository can rewrite. - domain/platform/rules.py: is_blocked_address (pure) + assert_remote_host_allowed (resolves, refuses if any address is loopback/private/link-local/metadata). Shared by the git remote test and, later, every deploy target. - ProjectExternalGit domain model + repository; container_delivery wires the real adapter behind GIT_WORKSPACE=local with a resolver that joins the project, its repo and its (decrypted) credentials. Tests: 394 API (+48) against real git and a local bare repo; ruff and mypy clean. Report: docs/plan/reports/agent-3-phase-2.md
This commit is contained in:
mjalmoudy94
@@ -0,0 +1,140 @@
|
||||
# Agent 3 — Phase 2 report: git workspaces
|
||||
|
||||
Date: 2026-10-13 · Branch: `dev`
|
||||
|
||||
## What was done
|
||||
|
||||
### 2.1–2.4 `LocalGitWorkspaceAdapter` (`app/infrastructure/adapters/git_workspace.py`)
|
||||
|
||||
Implements every method of `RepoWorkspacePort` with a real host `git` binary.
|
||||
|
||||
**Layout** under `SANDBOX_WORKSPACES_ROOT/<org>/<project>/`:
|
||||
`git/` (shared gitdir, never mounted), `work/` (the `dev` work tree, mounted at
|
||||
`/workspace`), `work/.worktrees/<task_id>/` (one per task), `private/` (0700, holds
|
||||
credentials for the duration of one call only). `.worktrees/` is excluded through the
|
||||
**gitdir's** `info/exclude`, never the project's `.gitignore`.
|
||||
|
||||
**Hardening on every call** (asserted by test): explicit `--git-dir`/`--work-tree`;
|
||||
`core.hooksPath=/dev/null`, `core.fsmonitor=false`, `core.quotePath=false`,
|
||||
`safe.directory=*`; `GIT_CONFIG_NOSYSTEM=1`; a private `HOME` per project;
|
||||
`GIT_TERMINAL_PROMPT=0`; `protocol.file.allow` configurable (see the deviation below);
|
||||
one `asyncio.Lock` per project held for every mutating operation.
|
||||
|
||||
**Credentials.** A token goes through a 0700 `GIT_ASKPASS` script that reads a 0600
|
||||
file containing `username\nsecret`; an SSH key is a 0600 temp file referenced by
|
||||
`GIT_SSH_COMMAND` with `IdentitiesOnly=yes` and a per-project `known_hosts`
|
||||
(`StrictHostKeyChecking=yes` once pinned, `accept-new` for TOFU on first contact).
|
||||
Both are deleted in a `finally`. Only a **path** ever travels through the environment —
|
||||
never the secret itself — so nothing sensitive reaches `argv` (world-readable via `ps`).
|
||||
|
||||
**`test_remote`** runs the shared SSRF check first, then `git ls-remote --heads` with a
|
||||
20 s timeout, and returns a plain Persian message from a closed set of causes. Raw
|
||||
stderr is never returned: git echoes the remote URL and a credential helper can write a
|
||||
token into stderr, so every message goes through `app.core.redact`.
|
||||
|
||||
**`merge_into`** forces the checkout of the target in the main work tree, merges
|
||||
`--no-ff`, and on conflict collects the unmerged paths and runs `merge --abort` before
|
||||
returning — a half-merge would break every later operation. **`push`** never forces:
|
||||
a rejection is reported with `rejected=True` after a fetch.
|
||||
|
||||
### The bug this phase existed to find: linked worktrees need their own gitdir
|
||||
|
||||
`git --git-dir=<shared> --work-tree=<task tree>` does **not** address the task branch.
|
||||
`--git-dir` names the *main* gitdir, whose `HEAD` is `dev`, so `git commit` in a task
|
||||
worktree commits straight to `dev` and the task branch never moves. The first version of
|
||||
this adapter had exactly that bug; it was caught by a test that asserts the task sha is
|
||||
*not* `dev`'s head.
|
||||
|
||||
The fix is to address the **linked gitdir** that `git worktree add` creates:
|
||||
`<gitdir>/worktrees/<task_id>`, which holds that worktree's own `HEAD` and index. It is
|
||||
passed explicitly rather than discovered from the worktree's `.git` file — that file is
|
||||
plain text a repository can rewrite, and the test that tampers with it proves the
|
||||
tampered value is ignored. `_worktree_gitdir()` falls back to the shared gitdir when no
|
||||
linked one exists, which is the `analysis_only` case (no worktree).
|
||||
|
||||
### 2.3 SSRF check (`app/domain/platform/rules.py`)
|
||||
|
||||
`is_blocked_address()` (pure) classifies an IP literal: loopback, RFC1918 private,
|
||||
link-local — including the cloud metadata address `169.254.169.254` — unique-local IPv6,
|
||||
multicast, reserved, unspecified, and IPv4-mapped IPv6 forms of all of them.
|
||||
`assert_remote_host_allowed()` validates the URL (scheme allowlist, no embedded
|
||||
credentials, blocked ports) and then resolves the host, refusing if **any** returned
|
||||
address is blocked — a name can legitimately have both a public and a private address.
|
||||
The resolver is injectable so the DNS step is fakeable in tests. `localhost` and friends
|
||||
are refused by name as well as by resolution.
|
||||
|
||||
### 2.5 Wiring (`container_delivery.py`)
|
||||
|
||||
`build_repo_workspace` returns the fake unless `GIT_WORKSPACE=local`, then builds a
|
||||
resolver that joins `projects` + `project_repositories`/`project_git_credentials` (platform)
|
||||
or `project_external_git` (external) and hands the decrypted secret to the adapter for one
|
||||
call. A rotated `ENCRYPTION_KEY` degrades to "no credential" with a log line rather than an
|
||||
exception.
|
||||
|
||||
Supporting additions: `ProjectExternalGit` domain model +
|
||||
`ProjectExternalGitRepository` (in Agent 3's own files), and
|
||||
`app/infrastructure/db/columns.py` is untouched.
|
||||
|
||||
## Files changed
|
||||
|
||||
| File | Change |
|
||||
|---|---|
|
||||
| `app/infrastructure/adapters/git_workspace.py` | new — the git adapter (~700 lines) |
|
||||
| `app/domain/platform/rules.py` | SSRF: `is_blocked_address`, `assert_remote_host_allowed`, `BLOCKED_REMOTE_PORTS` |
|
||||
| `app/domain/projects/models.py` | `ProjectExternalGit` |
|
||||
| `app/infrastructure/repositories/projects.py` | `ProjectExternalGitRepository` |
|
||||
| `app/application/container_delivery.py` | real `build_repo_workspace` + resolver |
|
||||
| `app/core/config.py`, `.env.example` | `GIT_ALLOW_FILE_PROTOCOL` |
|
||||
| `pyproject.toml` | registered the `docker` / `deploy_it` markers |
|
||||
| `tests/test_git_workspace.py` | new — 48 tests |
|
||||
|
||||
## Test output
|
||||
|
||||
```
|
||||
apps/api: 394 passed (was 346; +48)
|
||||
ruff check . -> All checks passed!
|
||||
mypy app -> Success: no issues found in 128 source files
|
||||
apps/web: 67 passed (unchanged — no frontend work in this phase)
|
||||
```
|
||||
|
||||
The 48 new tests run **real git** against a local bare repository standing in for the
|
||||
remote; nothing about git itself is mocked.
|
||||
|
||||
## Deviations from the TODO
|
||||
|
||||
1. **`protocol.file.allow` is a setting, not a constant.** The TODO fixes it at `never`.
|
||||
That is the right production default (it stops a repository from making git read a
|
||||
local path via a submodule or `--upload-pack`), but it also blocks the local bare repo
|
||||
the TODO's own test plan asks for. `GIT_ALLOW_FILE_PROTOCOL=false` keeps `never` as the
|
||||
default and lets the test suite opt in.
|
||||
2. **The SSRF check lives in `rules.py` as instructed, and it resolves DNS.** That is the
|
||||
one I/O call in an otherwise pure module; the module docstring now says so, and the
|
||||
address classification itself is pure and separately tested.
|
||||
3. **`base_changed` for `sync_with_base` is defined as "the task branch moved".** The
|
||||
port docstring says "True when the base moved, so the engine must re-run the tests" —
|
||||
comparing the base branch's sha before and after a merge into the task branch would
|
||||
always be equal, so the signal would never fire. It is computed from the worktree's
|
||||
own `HEAD` instead.
|
||||
4. **`.git` files are not read at all.** The TODO says "never trust the `.git` file
|
||||
inside a worktree"; the adapter goes further and never opens it, using the linked
|
||||
gitdir path instead.
|
||||
|
||||
## Open questions / CHANGE REQUESTS
|
||||
|
||||
- **CHANGE REQUEST (for integration):** `container_delivery.build_repo_workspace(session)`
|
||||
is called with a request-scoped `AsyncSession`. The git adapter's resolver uses that
|
||||
session, so a `RepoWorkspacePort` handed out by a long-lived worker is only valid while
|
||||
its session lives. Phase 3's delivery worker must build the port per pass (which is what
|
||||
`build_repo_workspace(session)` in `worker_session()` already implies). No contract
|
||||
change needed — noting it so the engine and delivery workers do not cache the port
|
||||
across sessions.
|
||||
|
||||
## Known limitations
|
||||
|
||||
- `git worktree add` names the linked gitdir after the worktree path's basename. The
|
||||
adapter relies on that (`<task_id>`), with a fallback to the shared gitdir. A task id
|
||||
containing characters git rewrites would silently degrade to the fallback; task ids are
|
||||
platform-generated hex, so this is theoretical.
|
||||
- Shallow clones are not supported; `ensure_workspace` fetches full history, which is what
|
||||
the diff guard and merge need.
|
||||
- No `@pytest.mark.docker` integration test — Phase 2 needs no container.
|
||||
Reference in New Issue
Block a user