fix(phase6): make gh CLI actually usable inside the VM (graphql, utility paths, safe.directory)

Two related fixes from real end-to-end testing of Phase 6:

**Fix 1: gh CLI was effectively broken inside the VM.**
The original github_path_allowed accepted only /user/* and
/repos/<owner>/<repo>/..., which blocked the GraphQL endpoint gh CLI
needs for the most common workflows:

- `gh pr create` → POST /graphql (rejected with 403)
- `gh issue create` → POST /graphql (rejected)
- `gh repo view --json` → POST /graphql (rejected)
- `gh issue list --search ...` → /search/issues (rejected)
- `gh repo view org/x` → /orgs/<org> (rejected)
- gh ambient probes → /rate_limit, /meta, /markdown (rejected)

Extend the allow-list:

- /graphql is allowed unconditionally. Per-repo scoping doesn't reach
  into GraphQL bodies in v1; this is a documented trade-off.
- /search/*, /rate_limit, /meta, /markdown are allowed (read-only
  utility, no per-repo state exposed).
- /orgs/<org> (bare) is allowed for `gh repo view org/...` resolution;
  /orgs by itself (listing) is denied to avoid leaking org membership.
- /user surface narrowed: only /user (auth probe) and /user/orgs are
  allowed. /user/repos, /user/keys, /user/emails, /user/gpg_keys are
  now BLOCKED — the previous catch-all leaked the full inventory of
  private repos and SSH keys / verified emails (see code-review
  finding #4 from this session).
- Reject any path segment equal to ".." anywhere to close the
  /repos/<allowed>/<repo>/../../<victim> path-traversal that GitHub
  would otherwise normalise upstream (code-review finding #2).

**Fix 2: `fatal: detected dubious ownership in repository`.**
The project is bind-mounted into the guest as the host user (UID 1000)
but the in-guest agent runs as root (UID 0). git refuses operations on
repos whose .git is owned by a different UID. Symptom: every `git
status` / `git log` / `git show` etc. inside the guest aborts.

write_guest_gh_config now ALWAYS writes a base /root/.gitconfig with
`[safe] directory = *` regardless of whether gh auth was captured.
The credential helper + url.insteadOf stanzas are only included
when has_gh_token=true; the gh hosts.yml is still gated on having
a token. Launcher always calls write_guest_gh_config (was gated on
gh_token_file before) so safe.directory is in place even with
--no-git or no host gh auth.

Verified inside the guest:
- `git status` returns "On branch master / nothing to commit" — no
  dubious-ownership abort.
- `git log` reads the bind-mounted history.
- `gh api graphql -f query="query { viewer { login } }"` reaches
  GitHub (response is Bad credentials, same nested-host limit as
  documented for the other providers — not our allow-list).
- `gh api /rate_limit` reaches GitHub.
- `gh api repos/octocat/Hello-World` still returns the clean 403
  naming the allow-listed repos.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Evgeny Boger 2026-05-24 13:11:27 +03:00
parent 68b293a364
commit 4479b1f2d1
3 changed files with 113 additions and 47 deletions

View file

@ -235,17 +235,61 @@ fn parse_http_request(req: &[u8]) -> Result<(String, String, Vec<(String, String
/// Path-based allow-list check for api.github.com requests.
///
/// Accepts:
/// Accepts (case-insensitive on owner/repo):
/// - `/repos/<owner>/<repo>[/...]` where `<owner>/<repo>` is in the
/// allow-list (case-insensitive).
/// - `/user` and `/user/...` (gh CLI uses these to confirm auth +
/// list accessible repos; doesn't expose specific-repo state).
/// - `/repos/<owner>/<repo>` for the parent-fetch of a fork (we
/// accept any owner here since the parent is read-only metadata —
/// reconsider if this proves too permissive).
/// allow-list, after rejecting any `..` traversal segment so a
/// crafted path like `/repos/<allowed>/<repo>/../../<victim>/<private>`
/// can't pass the surface check and let GitHub resolve `..`
/// upstream.
/// - `/user` and `/user/...` — gh CLI auth-status, current-user info.
/// Limited to the small set gh actually needs: /user (auth probe),
/// /user/orgs (org membership). Excludes /user/repos, /user/keys,
/// /user/emails, /user/gpg_keys to avoid leaking full inventory /
/// PII.
/// - **`/graphql`** — gh CLI's `gh pr create`, `gh issue create`,
/// `gh repo view --json` all use GraphQL mutations. We DO NOT body-
/// filter the GraphQL request: per-repo scoping doesn't reach into
/// the JSON body in v1. Accept that an agent that crafts arbitrary
/// GraphQL queries can read anything the token can; the practical
/// trade-off is "gh CLI works" vs. "no GraphQL".
/// - `/search/...`, `/rate_limit`, `/meta`, `/markdown` — small
/// utility endpoints gh CLI uses ambiently.
/// - `/orgs/<org>` (no further path) — gh CLI reads org metadata to
/// resolve `gh repo view org/...`.
fn github_path_allowed(path: &str, allowed: &[String]) -> bool {
let p = path.split_once('?').map(|(p, _)| p).unwrap_or(path);
if p == "/user" || p.starts_with("/user/") {
// Reject traversal anywhere in the path. GitHub normalises `..`
// upstream and would otherwise serve a different (blocked) repo.
for seg in p.split('/') {
if seg == ".." {
return false;
}
}
// Narrow /user/* surface to what gh actually needs for auth +
// org-membership checks. Excludes /user/repos, /user/keys,
// /user/emails, /user/gpg_keys (PII / full-inventory leak).
if p == "/user" || p == "/user/orgs" || p.starts_with("/user/orgs/") {
return true;
}
// Utility endpoints — read-only, no per-repo state.
if matches!(p, "/rate_limit" | "/meta" | "/markdown") {
return true;
}
if p == "/search" || p.starts_with("/search/") {
return true;
}
if p == "/orgs" {
return false; // listing orgs isn't useful and exposes membership
}
// gh resolves `gh repo view org/x` via /orgs/<org> first; allow
// bare org-info reads (no /repos sub-path which would be a list).
if let Some(rest) = p.strip_prefix("/orgs/") {
return !rest.is_empty() && !rest.contains('/');
}
// GraphQL: gh PR/issue creation. No body-filter — see fn doc.
if p == "/graphql" {
return true;
}
if let Some(rest) = p.strip_prefix("/repos/") {

View file

@ -230,13 +230,13 @@ pub async fn launch(agent: Agent, args: Args) -> Result<i32> {
let creds = crate::secrets::refresh(&session.state_dir, &project_guest_path, use_github)
.context("snapshotting host credentials")?;
// Phase 6: write guest-side gh/git config that reaches for the
// placeholder bearer the proxy will substitute on outbound. Only
// wired when we actually captured a gh token.
if creds.gh_token_file.is_some() {
crate::secrets::write_guest_gh_config(&session.state_dir)
// Phase 6/9: always write the guest gitconfig (carries the
// unconditional `safe.directory = *` so git inside the guest
// accepts the host-bind-mounted project despite the UID
// mismatch). The credential-helper / gh hosts.yml stanzas are
// gated on having actually captured a host gh token.
crate::secrets::write_guest_gh_config(&session.state_dir, creds.gh_token_file.is_some())
.context("writing guest gh/git config")?;
}
// Phase 7: parse `--mount HOST[:GUEST]` extras. The libkrun IRQ
// pool is *tight* — empirically the project bind + state bind +

View file

@ -627,36 +627,57 @@ fn write_default_claude_root_state(path: &Path, project_guest_path: &str) -> Res
Ok(())
}
/// Phase 6: write the gh/git config the guest agent uses to talk to
/// GitHub. Files land under `state_dir` so they're available inside
/// the guest via the existing bind mount + symlinks (see run.rs
/// patch builder).
/// Phase 6/9: write the guest's gitconfig and (if `has_gh_token`)
/// gh/git credential plumbing. Always called so the
/// `safe.directory = *` line is in place — without it, git inside the
/// guest fails with "fatal: detected dubious ownership in repository"
/// because the host-owned bind-mounted project is read by the guest's
/// root user (different UID).
///
/// Files land under `state_dir` so they're available inside the guest
/// via the existing bind mount + symlinks (see run.rs patch builder).
///
/// - `<state>/gitconfig` → symlinked to `/root/.gitconfig` in the
/// guest. A `credential.helper` echoes `username=x-access-token`
/// and `password=<placeholder>` so `git push` to GitHub goes out
/// as `Authorization: Basic base64(x-access-token:placeholder)`,
/// which the proxy substitutes on the wire.
/// guest. Always contains `safe.directory = *`. When the host has
/// gh auth, also contains a `credential.helper` that echoes
/// `username=x-access-token` / `password=<placeholder>` so
/// `git push` to GitHub goes out as
/// `Authorization: Basic base64(x-access-token:placeholder)`, which
/// the proxy substitutes on the wire.
/// - `<state>/gh-config/hosts.yml` → symlinked to `/root/.config/gh`
/// in the guest. The placeholder is what gh CLI sends; the proxy
/// substitutes outbound to api.github.com.
pub fn write_guest_gh_config(state_dir: &Path) -> Result<()> {
let gitconfig = format!(
// The credential helper is a shell snippet; git invokes it
// with `get` and reads `username=`/`password=` lines.
/// in the guest. Only written when `has_gh_token`; the placeholder
/// is what gh CLI sends and the proxy substitutes outbound to
/// api.github.com.
pub fn write_guest_gh_config(state_dir: &Path, has_gh_token: bool) -> Result<()> {
// safe.directory = * is unconditional: the guest IS the security
// boundary (microVM), so trusting every path is fine and git
// operating on the host-bind-mounted project requires it. Use
// wildcard form so any --mount extra also works without
// listing every path.
let mut gitconfig = String::from(
"[safe]\n\
\tdirectory = *\n\
[user]\n\
\tname = agent-vm\n\
\temail = agent-vm@msb.local\n",
);
if has_gh_token {
// git's credential helper for github.com pushes/clones. The
// helper is a shell snippet; git invokes it with `get` and
// reads `username=`/`password=` lines from stdout.
gitconfig.push_str(&format!(
"[credential \"https://github.com\"]\n\
\thelper = \"!f() {{ test \\\"$1\\\" = get && echo username=x-access-token && echo password={tok}; }}; f\"\n\
[credential \"https://gist.github.com\"]\n\
\thelper = \"!f() {{ test \\\"$1\\\" = get && echo username=x-access-token && echo password={tok}; }}; f\"\n\
[url \"https://github.com/\"]\n\
\tinsteadOf = git@github.com:\n\
[user]\n\
\tname = agent-vm\n\
\temail = agent-vm@msb.local\n",
\tinsteadOf = git@github.com:\n",
tok = GH_TOKEN_PLACEHOLDER,
);
));
}
atomic_write(&state_dir.join("gitconfig"), gitconfig.as_bytes(), 0o600)?;
if has_gh_token {
let gh_dir = state_dir.join("gh-config");
std::fs::create_dir_all(&gh_dir)?;
let hosts_yml = format!(
@ -668,6 +689,7 @@ pub fn write_guest_gh_config(state_dir: &Path) -> Result<()> {
)
.replace("\\x20", " ");
atomic_write(&gh_dir.join("hosts.yml"), hosts_yml.as_bytes(), 0o600)?;
}
Ok(())
}