diff --git a/internal/engine/boundary.go b/internal/engine/boundary.go index 259e009..3c6c3ec 100644 --- a/internal/engine/boundary.go +++ b/internal/engine/boundary.go @@ -169,8 +169,10 @@ func splitNUL(output string) []string { // gitMetaFiles are the metadata names a write to which changes what jig's own // git commands DO: `config` carries core.hooksPath / core.fsmonitor, the -// hooks directory carries the scripts themselves. -var gitMetaFiles = []string{"config", "config.worktree"} +// hooks directory carries the scripts themselves, and a linked worktree's +// `commondir` re-points it at a different shared repository — config, hooks +// and all — exactly as a rewritten `.git` pointer does. +var gitMetaFiles = []string{"config", "config.worktree", "commondir"} // snapshotGitMeta fingerprints the repository metadata reachable from dir. // Deliberately narrow: `.git` as a whole churns on every command (index, diff --git a/internal/engine/boundary_test.go b/internal/engine/boundary_test.go index c69cda9..832f045 100644 --- a/internal/engine/boundary_test.go +++ b/internal/engine/boundary_test.go @@ -538,6 +538,33 @@ func TestRewritingGitConfigIsABreachThatCannotBeRestored(t *testing.T) { } } +// A linked worktree's commondir names the shared repository every later git +// command resolves config and hooks through. A rewrite is a breach by name, +// even one that still resolves somewhere git accepts. +func TestRewritingALinkedWorktreesCommondirIsABreach(t *testing.T) { + ctx := context.Background() + repo := initBoundaryRepo(t) + linked := filepath.Join(t.TempDir(), "linked") + gitIn(t, repo, "worktree", "add", "--quiet", "-b", "linked", linked) + common, err := filepath.EvalSymlinks(filepath.Join(repo, ".git")) + if err != nil { + t.Fatal(err) + } + before := snapshotOrFail(t, ctx, linked) + + commondir := filepath.Join(common, "worktrees", "linked", "commondir") + if err := os.WriteFile(commondir, []byte(common+"\n"), 0o644); err != nil { + t.Fatal(err) + } + _, _, breaches, err := enforceBoundary(ctx, linked, before, nil, nil) + if err != nil { + t.Fatal(err) + } + if !contains(breachPaths(breaches), commondir) { + t.Fatalf("breaches = %+v, want the rewritten commondir", breaches) + } +} + // ---- build outputs --------------------------------------------------------- // initOutputRepo is the boundary repo with `bin/` optionally gitignored — diff --git a/internal/engine/crash_boundary_test.go b/internal/engine/crash_boundary_test.go new file mode 100644 index 0000000..038b808 --- /dev/null +++ b/internal/engine/crash_boundary_test.go @@ -0,0 +1,221 @@ +// crash_boundary_test.go — a dead phase is retried only after its writes +// pass the write boundary (R10/R11). Rollback alone cannot be trusted with +// this: it deliberately leaves a path that was dirty before the phase in its +// post-agent state, so an edit to a builder's uncommitted file — or a write +// the retry quietly reverts — would otherwise ride the retry unseen. +package engine_test + +import ( + "context" + "os" + "path/filepath" + "reflect" + "strings" + "testing" + "time" + + "github.com/StructuPath/jig/internal/engine/enginetest" + "github.com/StructuPath/jig/internal/protocol" +) + +// builderReaderSnapshot is a builder that may write src/ followed by a +// read-only reviewer. +const builderReaderSnapshot = `name: crash +roster: + builder: {model: test, system_prompt: Build., user_prompt: Build., writes: ["src/"]} + reader: {model: test, system_prompt: Read., user_prompt: Read., writes: []} +phases: + - {name: build, kind: agent, owner: builder} + - {name: review, kind: agent, owner: reader} +acceptance: [all_phases_passed] +` + +func readFile(t *testing.T, path string) string { + t.Helper() + content, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + return string(content) +} + +// A read-only reviewer that edits the builder's uncommitted work and dies is +// a breach, not a death to retry: the retry would start from the edit, and a +// retry that puts the content back would erase every trace of it. +func TestAReadOnlyRoleThatEditsBuilderWIPAndCrashesIsNeverRetried(t *testing.T) { + repo := initRepo(t) + fake := enginetest.New( + writes(map[string]string{"src/app.txt": "app"}, "built"), + enginetest.Step{Crash: true, Files: map[string]string{"src/app.txt": "reviewer edit"}}, + // The retry would restore the builder's content and pass. + writes(map[string]string{"src/app.txt": "app"}, "looks right"), + ) + sink := &recordingSink{} + runner := newTestRunner(t, fake, sink, nil) + + outcome := runner.Execute(context.Background(), testAttempt(builderReaderSnapshot, nil, repo)) + if outcome.State != protocol.AttemptFailed || !strings.Contains(outcome.Error, "src/app.txt") { + t.Fatalf("state = %s (%s), want failed on a breach naming src/app.txt", outcome.State, outcome.Error) + } + if calls := len(fake.Calls()); calls != 2 { + t.Fatalf("calls = %d, want build plus one review: the dead reviewer must not be retried", calls) + } + if !sink.has(protocol.EventError, "write_boundary_breach") { + t.Fatal("no write_boundary_breach event") + } + // The death is still in the trace, and the entry closes exactly once. + if got := sink.count(protocol.EventPhaseDeath, ""); got != 1 { + t.Fatalf("phase_death events = %d, want 1", got) + } + if got := sink.count(protocol.EventAgentEnd, "reader"); got != 1 { + t.Fatalf("reviewer agent_end events = %d, want 1", got) + } + if got := phaseStatuses(t, outcome.Result)["review"]; !reflect.DeepEqual(got, []string{protocol.EnvelopeFail}) { + t.Fatalf("review results = %v, want one failed breach result", got) + } + // The edit to pre-existing work is reported, never discarded. + if got := readFile(t, filepath.Join(repo, "src", "app.txt")); got != "reviewer edit" { + t.Fatalf("src/app.txt = %q, want the reviewer's edit left for inspection", got) + } +} + +// The builder's own death over its own uncommitted work stays what it was: +// rolled back as far as rollback goes, then retried. +func TestAnAuthorizedRoleThatCrashesOverItsWIPIsStillRetried(t *testing.T) { + repo := initRepo(t) + if err := os.MkdirAll(filepath.Join(repo, "src"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(repo, "src", "app.txt"), []byte("wip"), 0o644); err != nil { + t.Fatal(err) + } + fake := enginetest.New( + enginetest.Step{Crash: true, Files: map[string]string{"src/app.txt": "half done", "src/new.txt": "half"}}, + writes(map[string]string{"src/app.txt": "done"}, "built"), + ) + sink := &recordingSink{} + runner := newTestRunner(t, fake, sink, nil) + + snapshot := "name: t\n" + repairRoster + ` +phases: + - {name: build, kind: agent, owner: builder} +acceptance: [all_phases_passed] +` + outcome := runner.Execute(context.Background(), testAttempt(snapshot, nil, repo)) + if outcome.State != protocol.AttemptAcceptedUnpublished { + t.Fatalf("state = %s (%s), want accepted after the retry", outcome.State, outcome.Error) + } + if calls := len(fake.Calls()); calls != 2 { + t.Fatalf("calls = %d, want the death and its retry", calls) + } + if sink.has(protocol.EventError, "write_boundary_breach") { + t.Fatal("an authorized write raised a breach") + } + if _, err := os.Stat(filepath.Join(repo, "src", "new.txt")); !os.IsNotExist(err) { + t.Fatalf("the dead entry's new file survived the rollback: %v", err) + } +} + +// A declared build output a dead read-only phase wrote is neither a breach +// nor carried into the retry: rollback removes it and the phase re-enters. +func TestAFreshBuildOutputFromADeadReadOnlyPhaseIsRemovedAndRetried(t *testing.T) { + repo := initRepo(t) + ignoreBin(t, repo) + fake := enginetest.New( + enginetest.Step{Crash: true, Files: map[string]string{"bin/x": "half a build"}}, + success("read", nil), + ) + sink := &recordingSink{} + runner := newTestRunner(t, fake, sink, nil) + + snapshot := `name: readonly +roster: + reader: {model: test, system_prompt: Read., user_prompt: Read., writes: []} +phases: + - {name: read, kind: agent, owner: reader} +acceptance: [all_phases_passed] +` + buildOutputsLine + outcome := runner.Execute(context.Background(), testAttempt(snapshot, nil, repo)) + if outcome.State != protocol.AttemptAcceptedUnpublished { + t.Fatalf("state = %s (%s), want accepted after the retry", outcome.State, outcome.Error) + } + if calls := len(fake.Calls()); calls != 2 { + t.Fatalf("calls = %d, want the death and its retry", calls) + } + if sink.has(protocol.EventError, "write_boundary_breach") { + t.Fatal("a declared build output raised a breach") + } + if _, err := os.Stat(filepath.Join(repo, "bin", "x")); !os.IsNotExist(err) { + t.Fatalf("the dead entry's build output survived the rollback: %v", err) + } +} + +// hideEdit and withDo script a retry that would erase the dead entry's trace. +func hideEdit(step enginetest.Step, files map[string]string) enginetest.Step { + step.Files = files + return step +} + +func withDo(step enginetest.Step, do func(enginetest.Call)) enginetest.Step { + step.Do = do + return step +} + +// A dead member's edit to the builder's uncommitted work must not become the +// next wave's baseline: a retry that reverts it leaves the group's final +// enforcement an unchanged tree. +func TestADeadMembersEditToBuilderWIPEndsTheGroupBeforeARetry(t *testing.T) { + f := newRepairFixture(t, panelSnapshot(nil, panelGroup), nil) + app := filepath.Join(f.repo, "src", "app.txt") + f.fake.Route(buildRole, writes(map[string]string{"src/app.txt": "app"}, "built the app")) + f.fake.Route(correctnessRole, + enginetest.Step{Crash: true, Do: func(enginetest.Call) { + if err := os.WriteFile(app, []byte("reviewer edit"), 0o644); err != nil { + t.Error(err) + } + }}, + hideEdit(approve("would hide the edit", nil), map[string]string{"src/app.txt": "app"})) + f.fake.Route(securityRole, approve("secure", nil)) + f.fake.Route(maintainabilityRole, approve("maintainable", nil)) + f.fake.Route(closerRole, success("wrapped up", nil)) + + outcome := executeWithin(t, f, 20*time.Second) + if outcome.State != protocol.AttemptFailed || !strings.Contains(outcome.Error, "src/app.txt") { + t.Fatalf("outcome = %s (%s), want failed on a breach naming src/app.txt", outcome.State, outcome.Error) + } + if got := len(callsFor(f.fake, correctnessRole)); got != 1 { + t.Fatalf("correctness invoked %d times, want one: no wave may run over a breach", got) + } + if got := readFile(t, app); got != "reviewer edit" { + t.Fatalf("src/app.txt = %q, want the reviewer's edit left for inspection", got) + } +} + +// The same for repository metadata: a hook a dead member planted is a breach +// before the next wave, not something a retry can delete unseen. +func TestADeadMembersPlantedHookEndsTheGroupBeforeARetry(t *testing.T) { + f := newRepairFixture(t, panelSnapshot(nil, panelGroup), nil) + hook := filepath.Join(f.repo, ".git", "hooks", "post-checkout") + f.fake.Route(buildRole, writes(map[string]string{"src/app.txt": "app"}, "built the app")) + f.fake.Route(correctnessRole, + enginetest.Step{Crash: true, Do: func(enginetest.Call) { + if err := os.WriteFile(hook, []byte("#!/bin/sh\nexit 0\n"), 0o755); err != nil { + t.Error(err) + } + }}, + withDo(approve("would hide the hook", nil), func(enginetest.Call) { _ = os.Remove(hook) })) + f.fake.Route(securityRole, approve("secure", nil)) + f.fake.Route(maintainabilityRole, approve("maintainable", nil)) + f.fake.Route(closerRole, success("wrapped up", nil)) + + outcome := executeWithin(t, f, 20*time.Second) + if outcome.State != protocol.AttemptFailed || !strings.Contains(outcome.Error, "post-checkout") { + t.Fatalf("outcome = %s (%s), want failed on a breach naming the hook", outcome.State, outcome.Error) + } + if got := len(callsFor(f.fake, correctnessRole)); got != 1 { + t.Fatalf("correctness invoked %d times, want one: no wave may run over a breach", got) + } + if _, err := os.Stat(hook); !os.IsNotExist(err) { + t.Fatalf("the planted hook survived enforcement: %v", err) + } +} diff --git a/internal/engine/parallel.go b/internal/engine/parallel.go index e6493ab..55979a7 100644 --- a/internal/engine/parallel.go +++ b/internal/engine/parallel.go @@ -224,7 +224,8 @@ func (e *execution) runGroupOnce( // concurrently; a member that died re-enters alone in the next wave, after // its siblings finished, until its death budget is spent. The first result // that ends the attempt stops every sibling — their sends are killed — and -// the runner waits for all of them before it enforces the boundary once. +// the runner waits for all of them before it enforces the boundary: ahead of +// every retry wave, and once at the end. func (e *execution) runMembers( ctx context.Context, members []protocol.PhaseSpec, active []int, runs []memberRun, previous *parsedEnvelope, names []string, before treeSnapshot, @@ -295,13 +296,20 @@ func (e *execution) runMembers( break } if len(next) > 0 { - // A read-only group cannot retry against agent-moved history (R10/R11). - head, err := runGit(context.WithoutCancel(ctx), e.attempt.WorktreePath, "rev-parse", "--verify", "HEAD^{commit}") + // Every member has joined, so the tree is quiet: enforce the group + // boundary before another wave runs (R10/R11). A dead member's + // write is otherwise the retry's baseline — an edit to a file + // that was already dirty, or a planted hook, that the retry + // reverts leaves the final enforcement nothing to see. Declared + // build outputs pass, so a member that died mid-build still + // re-enters. + _, outputs, breaches, err := enforceBoundary( + context.WithoutCancel(ctx), e.attempt.WorktreePath, before, []string{}, e.spec.BuildOutputs) if err != nil { - return e.groupInfra(names, "check HEAD before member retry: "+err.Error()) + return e.groupInfra(names, "write-boundary enforcement before member retry: "+err.Error()) } - if strings.TrimSpace(head) != before.head { - break // the group's enforcement below reports and rolls back the breach + if len(breaches) > 0 { + return e.groupBreach(members, active, runs, names, breaches, outputs) } } pending = next diff --git a/internal/engine/phase.go b/internal/engine/phase.go index feb2911..57e8ffb 100644 --- a/internal/engine/phase.go +++ b/internal/engine/phase.go @@ -1248,7 +1248,26 @@ func (e *execution) runAgentPhaseAttempt( // death wraps up one dead entry: roll the worktree back to the pre-phase // snapshot and emit the envelope-less terminal trace event (R11). + // + // The boundary comes first. restoreSnapshot leaves a path that was dirty + // before the phase in its post-agent state, so a read-only role that + // edits a builder's uncommitted file and then crashes would otherwise + // hand that edit to the retry as its baseline — and git metadata it + // changed would be "restored" only as far as deleting what is new. A + // breach ends the attempt here, unretried (R10). Detached for the same + // reason as terminalExit: a death can coincide with cancellation. death := func(detail string) (phaseRun, bool) { + if !e.grouped { + if _, terminal := e.enforceWriteBoundary( + context.WithoutCancel(ctx), phase, entry, started, before, role.Writes); terminal != nil { + terminal.failure = detail + " — and " + terminal.failure + agentEnd(terminal.agentOutcome()) + e.emit.emit(protocol.EventPhaseDeath, phase.Name, phase.Owner, map[string]any{ + "phase_attempt": entry, "error": detail, + }) + return *terminal, false + } + } agentEnd(protocol.AgentDied) e.emit.emit(protocol.EventPhaseDeath, phase.Name, phase.Owner, map[string]any{ "phase_attempt": entry, "error": detail, diff --git a/internal/worker/metadata.go b/internal/worker/metadata.go new file mode 100644 index 0000000..87c1de8 --- /dev/null +++ b/internal/worker/metadata.go @@ -0,0 +1,296 @@ +// metadata.go — refusal of repository cache entries and attempt worktrees +// whose git metadata jig did not write. Hardened flags switch off hooks, +// fsmonitor, and the ext transport, but not what else a repository's own +// config can make git execute or redirect — a credential.helper, a remote +// pushurl / uploadpack / receivepack, a core.sshCommand, an include of a +// file nobody checked. Agents run git in worktrees whose shared metadata IS +// the cache entry, and the engine can only REPORT a changed config it cannot +// restore (engine boundary.go), so the worker checks before it trusts: +// anything outside what jig itself writes is refused, never repaired or +// deleted, and stays on disk for the operator — the same discipline as an +// origin that stops matching its registered identity. +// +// This is a check at the moment of use, not a sandbox: an agent still +// running concurrently could rewrite metadata between check and use. +package worker + +import ( + "context" + "errors" + "fmt" + "io" + "os" + "path/filepath" + "slices" + "strings" +) + +// inertCacheConfig is every repository-local key jig's own clone writes +// (`init --bare`, `remote add origin`, the fixed fetch refspec) or git adds +// on its own for the platform or the operator's worktree settings, mapped to +// the values it may hold. A nil set admits any single value: the origin URL +// is checked against the registered identity separately. +var inertCacheConfig = map[string][]string{ + "core.repositoryformatversion": {"0", "1"}, + "core.filemode": {"true", "false"}, + "core.bare": {"true"}, + "core.ignorecase": {"true", "false"}, + "core.precomposeunicode": {"true", "false"}, + "core.symlinks": {"true", "false"}, + "extensions.objectformat": {"sha1", "sha256"}, + "extensions.relativeworktrees": {"true"}, + "remote.origin.url": nil, + "remote.origin.fetch": {"+refs/heads/*:refs/remotes/origin/*"}, +} + +// maxMetadataFileBytes bounds the plain reads of pointer and config files. +// Anything git or jig writes there is tiny. +const maxMetadataFileBytes = 64 << 10 + +// validateMetadata refuses a cache entry whose metadata could make a worker +// git command execute or redirect anything: config keys outside +// inertCacheConfig (includes are never followed, so an include is itself a +// refused key), a worktree-scoped config file, or an active hook. Config is +// listed with `git config --file`, which reads only the named file. +func (e *repoEntry) validateMetadata(ctx context.Context) error { + refuse := func(reason string) error { + return fmt.Errorf("repository cache entry %s refused: %s; "+ + "it is left untouched for inspection — remove the directory to let jig re-clone it", e.dir, reason) + } + if err := requireRealDirectory(e.dir); err != nil { + return refuse(err.Error()) + } + // A bare entry has neither. Run in the entry, git follows a `.git` there + // or a commondir to another repository, whose config this check never + // reads — so either one is refused before anything below is trusted. + for _, pointer := range []string{".git", "commondir"} { + if _, err := os.Lstat(filepath.Join(e.dir, pointer)); err == nil { + return refuse(fmt.Sprintf("a %s is present at the bare entry's root", pointer)) + } else if !errors.Is(err, os.ErrNotExist) { + return refuse(fmt.Sprintf("inspect %s: %v", pointer, err)) + } + } + config := filepath.Join(e.dir, "config") + if _, err := readMetadataFile(config); err != nil { + return refuse(err.Error()) + } + listing, err := runGit(ctx, "", "config", "--file", config, "--no-includes", "--null", "--list") + if err != nil { + return refuse("config unreadable: " + err.Error()) + } + seen := make(map[string]bool) + for _, item := range splitNUL(listing) { + key, value, _ := strings.Cut(item, "\n") + allowed, known := inertCacheConfig[key] + if !known { + // Only a subsection is free text — credential..helper can + // carry credentials — so a key with one is never quoted. + if strings.Count(key, ".") != 1 { + return refuse("a config key with a subsection is not one jig writes") + } + return refuse(fmt.Sprintf("config key %q is not one jig writes", key)) + } + if seen[key] { + return refuse(fmt.Sprintf("config key %q is set more than once", key)) + } + seen[key] = true + if allowed != nil && !slices.Contains(allowed, value) { + return refuse(fmt.Sprintf("config key %q has an unexpected value", key)) + } + } + if !seen["remote.origin.url"] { + return refuse("config has no origin URL") + } + if _, err := os.Lstat(filepath.Join(e.dir, "config.worktree")); !errors.Is(err, os.ErrNotExist) { + return refuse("a config.worktree is present") + } + hooks := filepath.Join(e.dir, "hooks") + if _, err := os.Lstat(hooks); errors.Is(err, os.ErrNotExist) { + return nil + } + if err := requireRealDirectory(hooks); err != nil { + return refuse(err.Error()) + } + entries, err := os.ReadDir(hooks) + if err != nil { + return refuse("hooks unreadable: " + err.Error()) + } + for _, entry := range entries { + // git's own templates install inactive *.sample scripts; anything + // else in hooks/ is a hook git would run. + if !entry.Type().IsRegular() || !strings.HasSuffix(entry.Name(), ".sample") { + return refuse(fmt.Sprintf("active hook %q is present", entry.Name())) + } + } + return nil +} + +// validateWorktree refuses an attempt worktree whose git indirection does not +// lead straight back to this entry: the `.git` file must be a plain pointer +// into /worktrees/, that admin directory's gitdir must lead back to +// this worktree, its commondir must be the `../..` git writes, and its HEAD +// must be the attempt's own branch. All are read as plain files rather than +// trusted to git, because git follows them: a re-pointed `.git` or commondir +// hands every later command — status, add, commit, push — an attacker-chosen +// repository and its config. Paths are compared resolved, since git records +// resolved paths and macOS temporary roots are not (/var -> /private/var). +// The caller holds the entry mutex. +func (e *repoEntry) validateWorktree(path, branch string) error { + refuse := func(reason string) error { + return fmt.Errorf("worktree %s refused: %s; it is left untouched for inspection", path, reason) + } + pointer, err := readMetadataFile(filepath.Join(path, ".git")) + if err != nil { + return refuse(err.Error()) + } + value, ok := metadataValue(pointer) + gitDir, found := strings.CutPrefix(value, "gitdir: ") + if !ok || !found || !plainMetadataValue(gitDir) { + return refuse(fmt.Sprintf(".git is not a plain gitdir pointer: %q", pointer)) + } + if !filepath.IsAbs(gitDir) { + // Preserve symlink/.. traversal until resolution, as git does. + gitDir = path + string(os.PathSeparator) + gitDir + } + admin, err := filepath.EvalSymlinks(gitDir) + if err != nil { + return refuse("resolve .git pointer: " + err.Error()) + } + common, err := filepath.EvalSymlinks(e.dir) + if err != nil { + return refuse("resolve repository cache entry: " + err.Error()) + } + if filepath.Dir(admin) != filepath.Join(common, "worktrees") { + return refuse(fmt.Sprintf(".git points at %s, outside the repository cache entry %s", admin, common)) + } + if err := requireRealDirectory(admin); err != nil { + return refuse(err.Error()) + } + // Any admin directory under worktrees/ is not enough: one belonging to a + // sibling attempt hands this worktree that attempt's HEAD, branch, and + // index. The admin's gitdir backlink — absolute, or relative to the admin + // when git writes relative paths — must name this worktree's own `.git`. + content, err := readMetadataFile(filepath.Join(admin, "gitdir")) + if err != nil { + return refuse(err.Error()) + } + backlink, ok := metadataValue(content) + if !ok { + return refuse(fmt.Sprintf("the admin directory's gitdir is not a plain path: %q", content)) + } + if !filepath.IsAbs(backlink) { + backlink = admin + string(os.PathSeparator) + backlink + } + owner, err := filepath.EvalSymlinks(backlink) + if err != nil { + return refuse("resolve gitdir backlink: " + err.Error()) + } + own, err := filepath.EvalSymlinks(path + string(os.PathSeparator) + ".git") + if err != nil { + return refuse("resolve worktree: " + err.Error()) + } + if owner != own { + return refuse(fmt.Sprintf(".git points at admin directory %s, whose gitdir names %s, not this worktree", admin, owner)) + } + commondir, err := readMetadataFile(filepath.Join(admin, "commondir")) + if err != nil { + return refuse(err.Error()) + } + if value, ok := metadataValue(commondir); !ok || value != "../.." { + return refuse(fmt.Sprintf("commondir is %q, not the repository cache entry", commondir)) + } + if _, err := os.Lstat(filepath.Join(admin, "config.worktree")); !errors.Is(err, os.ErrNotExist) { + return refuse("a config.worktree is present") + } + // The pointer pair can be forged consistently across two attempts — A's + // `.git` on B's admin and B's gitdir rewritten to name A — and A's files + // would then commit on B's branch and index. The admin's HEAD must be this + // attempt's own branch; attempts never detach, since publication pushes + // the branch, not HEAD. + if branch == "" { + return refuse("no attempt branch to check HEAD against") + } + head, err := readMetadataFile(filepath.Join(admin, "HEAD")) + if err != nil { + return refuse(err.Error()) + } + if value, ok := metadataValue(head); !ok || value != "ref: refs/heads/"+branch { + return refuse(fmt.Sprintf("HEAD is %q, not the attempt branch %s", head, branch)) + } + return nil +} + +// metadataValue reads a one-line pointer file the way git does. Git strips +// only the trailing line terminators (CR and LF) from `.git` and commondir +// and keeps every other byte, spaces included, as part of the path — so +// trimming spaces here would validate `` while git opens ` `. +// Git trims more from gitdir and HEAD, which is why anything still edged +// with whitespace, or carrying a terminator or NUL inside, is refused rather +// than interpreted: nothing jig or git writes looks like that, and refusing +// it leaves exactly one reading of every accepted value, git's and ours. +func metadataValue(content string) (string, bool) { + value := strings.TrimRight(content, "\r\n") + return value, plainMetadataValue(value) +} + +func plainMetadataValue(value string) bool { + return value != "" && strings.TrimSpace(value) == value && !strings.ContainsAny(value, "\r\n\x00") +} + +// checkWorktreeMetadata validates an attempt worktree on its attempt branch, +// and the cache entry it shares, before the worker runs git inside it. +func (w *Worker) checkWorktreeMetadata(ctx context.Context, repository, path, branch string) error { + entry, err := w.cache.entry(ctx, repository) + if err != nil { + return err + } + entry.mutex.Lock() + defer entry.mutex.Unlock() + return entry.validateWorktree(path, branch) +} + +// readMetadataFile reads one pointer or config file that must be a regular +// file: a symlink would let the check read one file while git reads another. +func readMetadataFile(path string) (string, error) { + info, err := os.Lstat(path) + if err != nil { + return "", fmt.Errorf("inspect %s: %w", filepath.Base(path), err) + } + if !info.Mode().IsRegular() { + return "", fmt.Errorf("%s is not a regular file", filepath.Base(path)) + } + file, err := os.Open(path) + if err != nil { + return "", fmt.Errorf("read %s: %w", filepath.Base(path), err) + } + defer file.Close() + content, err := io.ReadAll(io.LimitReader(file, maxMetadataFileBytes+1)) + if err != nil { + return "", fmt.Errorf("read %s: %w", filepath.Base(path), err) + } + if len(content) > maxMetadataFileBytes { + return "", fmt.Errorf("%s is larger than any file git writes there", filepath.Base(path)) + } + return string(content), nil +} + +func requireRealDirectory(path string) error { + info, err := os.Lstat(path) + if err != nil { + return fmt.Errorf("inspect %s: %w", filepath.Base(path), err) + } + if !info.IsDir() { + return fmt.Errorf("%s is not a real directory", filepath.Base(path)) + } + return nil +} + +func splitNUL(output string) []string { + var items []string + for _, item := range strings.Split(output, "\x00") { + if item != "" { + items = append(items, item) + } + } + return items +} diff --git a/internal/worker/metadata_test.go b/internal/worker/metadata_test.go new file mode 100644 index 0000000..e8fef9c --- /dev/null +++ b/internal/worker/metadata_test.go @@ -0,0 +1,967 @@ +// metadata_test.go — the worker's own git never executes metadata an agent +// could have left in a shared cache entry or an attempt worktree, and +// refuses — without repairing or deleting — anything jig did not write. +// Every fixture is a temporary local repository; the "executables" are +// marker scripts that only record that they ran. +package worker + +import ( + "context" + "fmt" + "net/http" + "net/http/httptest" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +// markerScript writes a shell script that records it ran and fails, and +// returns the script's path and the marker it would create. +func markerScript(t *testing.T) (script, marker string) { + t.Helper() + dir := t.TempDir() + script = filepath.Join(dir, "marker.sh") + marker = filepath.Join(dir, "ran") + body := "#!/bin/sh\necho ran >> '" + marker + "'\nexit 1\n" + if err := os.WriteFile(script, []byte(body), 0o755); err != nil { + t.Fatal(err) + } + return script, marker +} + +func requireNoMarker(t *testing.T, marker string) { + t.Helper() + if _, err := os.Stat(marker); !os.IsNotExist(err) { + t.Fatalf("a repository-supplied command ran worker-side (marker %s: %v)", marker, err) + } +} + +func appendConfig(t *testing.T, path, text string) { + t.Helper() + file, err := os.OpenFile(path, os.O_APPEND|os.O_WRONLY, 0) + if err != nil { + t.Fatal(err) + } + defer file.Close() + if _, err := file.WriteString(text); err != nil { + t.Fatal(err) + } +} + +// cachedWorktree materializes a real cache entry for a fresh local origin +// and adds one attempt worktree at its head, the way prepareAttempt does. +func cachedWorktree(t *testing.T) (*Worker, attemptManifest, *repoEntry, string) { + t.Helper() + originDir, head, identity := newOriginRepo(t) + dataDir := t.TempDir() + w := &Worker{config: Config{DataDir: dataDir}, cache: newRepoCache(filepath.Join(dataDir, "repos"))} + entry, err := w.cache.entry(context.Background(), identity) + if err != nil { + t.Fatalf("materialize cache entry: %v", err) + } + path := filepath.Join(dataDir, "worktrees", "attempt-1") + if err := w.createWorktree(context.Background(), entry, head, path, "jig/job-1/1"); err != nil { + t.Fatalf("create worktree: %v", err) + } + return w, attemptManifest{ + AttemptID: "attempt-1", Repository: identity, RepositoryDir: entry.dir, + BaseSHA: head, WorktreePath: path, Branch: "jig/job-1/1", + }, entry, originDir +} + +// A jig-built entry and worktree pass, again on reuse, even when the +// operator's global config carries a template with an active hook. +func TestAJigBuiltCacheEntryAndWorktreePassValidation(t *testing.T) { + templateHooks := filepath.Join(t.TempDir(), "template", "hooks") + if err := os.MkdirAll(templateHooks, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(templateHooks, "pre-commit"), []byte("#!/bin/sh\n"), 0o755); err != nil { + t.Fatal(err) + } + global := filepath.Join(t.TempDir(), "gitconfig") + if err := os.WriteFile(global, []byte("[init]\n\ttemplateDir = "+filepath.Dir(templateHooks)+"\n"), 0o644); err != nil { + t.Fatal(err) + } + t.Setenv("GIT_CONFIG_GLOBAL", global) + + w, manifest, _, _ := cachedWorktree(t) + if err := w.checkWorktreeMetadata(context.Background(), manifest.Repository, manifest.WorktreePath, manifest.Branch); err != nil { + t.Fatalf("a jig-built worktree was refused: %v", err) + } + if err := w.cleanupEligible(context.Background(), manifest); err != nil { + t.Fatalf("a clean worktree at base is not cleanup-eligible: %v", err) + } +} + +// Finding 1 itself: a core.fsmonitor command left in the shared entry's +// config — the change the engine can report but not restore — must not run +// during cleanup, and the evidence stays where it is. +func TestCleanupNeverRunsAnFsmonitorLeftInTheSharedConfig(t *testing.T) { + w, manifest, entry, _ := cachedWorktree(t) + script, marker := markerScript(t) + config := filepath.Join(entry.dir, "config") + appendConfig(t, config, "[core]\n\tfsmonitor = "+script+"\n") + + err := w.cleanupEligible(context.Background(), manifest) + requireNoMarker(t, marker) + if err == nil || !strings.Contains(err.Error(), "refused") || !strings.Contains(err.Error(), "core.fsmonitor") { + t.Fatalf("cleanup eligibility = %v, want a refusal naming core.fsmonitor", err) + } + if body, _ := os.ReadFile(config); !strings.Contains(string(body), script) { + t.Fatal("the contaminated config was repaired; it must stay for inspection") + } + if _, err := os.Stat(manifest.WorktreePath); err != nil { + t.Fatalf("the worktree was removed: %v", err) + } +} + +// An active hook in the entry is refused before any fetch could fire it. +func TestAnActiveHookInTheCacheEntryIsRefusedBeforeAFetch(t *testing.T) { + _, manifest, entry, originDir := cachedWorktree(t) + script, marker := markerScript(t) + hooks := filepath.Join(entry.dir, "hooks") + if err := os.MkdirAll(hooks, 0o755); err != nil { + t.Fatal(err) + } + if err := os.Symlink(script, filepath.Join(hooks, "reference-transaction")); err != nil { + t.Fatal(err) + } + advanceOrigin(t, originDir) + + cache := newRepoCache(filepath.Dir(entry.dir)) + reused, err := cache.entry(context.Background(), manifest.Repository) + if err == nil { + // Pre-fix behaviour: the entry is trusted and the fetch runs the hook. + _ = reused.fetch(context.Background()) + } + requireNoMarker(t, marker) + if err == nil || !strings.Contains(err.Error(), "active hook") { + t.Fatalf("entry = %v, want a refusal naming the active hook", err) + } +} + +// Repository-local config that would execute or redirect during a fetch, +// a push, or credential lookup is refused on reuse before it can take effect. +func TestUnsafeCacheConfigIsRefusedBeforeItTakesEffect(t *testing.T) { + for name, text := range map[string]string{ + "uploadpack": "[remote \"origin\"]\n\tuploadpack = %s\n", + "pushurl": "[remote \"origin\"]\n\tpushurl = /elsewhere.git\n", + "credential": "[credential]\n\thelper = !%s\n", + "include": "[include]\n\tpath = %s\n", + "second fetch": "[remote \"origin\"]\n\tfetch = +refs/*:refs/*\n", + "worktree config": "[extensions]\n\tworktreeConfig = true\n", + "ssh command": "[core]\n\tsshCommand = %s\n", + "origin url twice": "[remote \"origin\"]\n\turl = /elsewhere.git\n", + } { + t.Run(name, func(t *testing.T) { + _, manifest, entry, originDir := cachedWorktree(t) + script, marker := markerScript(t) + appendConfig(t, filepath.Join(entry.dir, "config"), strings.ReplaceAll(text, "%s", script)) + advanceOrigin(t, originDir) + + cache := newRepoCache(filepath.Dir(entry.dir)) + reused, err := cache.entry(context.Background(), manifest.Repository) + if err == nil { + _ = reused.fetch(context.Background()) + } + requireNoMarker(t, marker) + if err == nil || !strings.Contains(err.Error(), "refused") { + t.Fatalf("entry = %v, want a refusal", err) + } + if !strings.Contains(err.Error(), entry.dir) { + t.Fatalf("refusal %q does not name the entry directory", err) + } + }) + } +} + +// A re-pointed `.git` or commondir hands every later command another +// repository's config. Cleanup and publication both refuse before running +// git there, and nothing is deleted. +func TestARepointedWorktreeIsRefusedByCleanupAndPublication(t *testing.T) { + repoint := map[string]func(t *testing.T, manifest attemptManifest, entry *repoEntry, evil string){ + ".git": func(t *testing.T, manifest attemptManifest, _ *repoEntry, evil string) { + pointer := filepath.Join(manifest.WorktreePath, ".git") + if err := os.WriteFile(pointer, []byte("gitdir: "+evil+"\n"), 0o644); err != nil { + t.Fatal(err) + } + }, + "commondir": func(t *testing.T, manifest attemptManifest, entry *repoEntry, evil string) { + commondir := filepath.Join(entry.dir, "worktrees", filepath.Base(manifest.WorktreePath), "commondir") + if err := os.WriteFile(commondir, []byte(evil+"\n"), 0o644); err != nil { + t.Fatal(err) + } + }, + } + for name, apply := range repoint { + t.Run(name, func(t *testing.T) { + w, manifest, entry, _ := cachedWorktree(t) + script, marker := markerScript(t) + evilRoot := t.TempDir() + gitRun(t, "", "init", "--quiet", evilRoot) + evil := filepath.Join(evilRoot, ".git") + appendConfig(t, filepath.Join(evil, "config"), "[core]\n\tfsmonitor = "+script+"\n") + apply(t, manifest, entry, evil) + if err := os.WriteFile(filepath.Join(manifest.WorktreePath, "README.md"), []byte("edit\n"), 0o644); err != nil { + t.Fatal(err) + } + + err := w.cleanupEligible(context.Background(), manifest) + if err == nil || !strings.Contains(err.Error(), "refused") { + t.Fatalf("cleanup eligibility = %v, want a refusal", err) + } + target := publishTarget{ + attemptID: manifest.AttemptID, repository: manifest.Repository, branch: manifest.Branch, + worktreePath: manifest.WorktreePath, baseSHA: manifest.BaseSHA, + } + _, err = w.pushAttemptBranch(context.Background(), PublishOptions{}, target, []string{"README.md"}) + if publishCode(err) != "publish_worktree_unsafe" { + t.Fatalf("publish = %v, want publish_worktree_unsafe", err) + } + err = w.requireRoundCommitsDeclared(context.Background(), target, manifest.BaseSHA, nil) + if publishCode(err) != "publish_worktree_unsafe" { + t.Fatalf("repair round check = %v, want publish_worktree_unsafe", err) + } + requireNoMarker(t, marker) + if _, err := os.Stat(filepath.Join(manifest.WorktreePath, "README.md")); err != nil { + t.Fatalf("the refused worktree lost its content: %v", err) + } + }) + } +} + +// A `.git` or commondir planted at the bare entry's root redirects git run +// there to a repository whose config the check never read. The entry is +// refused on its next acquisition — before a fetch or a proof read can reach +// the planted config — and the planted pointer stays for inspection. +func TestARedirectedCacheEntryRootIsRefusedBeforeGitFollowsIt(t *testing.T) { + plant := map[string]func(entry, evil string) (string, string){ + ".git": func(entry, evil string) (string, string) { + return filepath.Join(entry, ".git"), "gitdir: " + evil + "\n" + }, + "commondir": func(entry, evil string) (string, string) { return filepath.Join(entry, "commondir"), evil + "\n" }, + } + for name, pointer := range plant { + t.Run(name, func(t *testing.T) { + w, manifest, entry, originDir := cachedWorktree(t) + script, marker := markerScript(t) + evil := filepath.Join(t.TempDir(), "evil.git") + gitRun(t, "", "init", "--bare", "--quiet", evil) + appendConfig(t, filepath.Join(evil, "config"), + "[remote \"origin\"]\n\turl = "+originDir+"\n\tuploadpack = "+script+"\n") + path, body := pointer(entry.dir, evil) + if err := os.WriteFile(path, []byte(body), 0o644); err != nil { + t.Fatal(err) + } + advanceOrigin(t, originDir) + + reused, err := w.cache.entry(context.Background(), manifest.Repository) + if err == nil { + _ = reused.fetch(context.Background()) + } + target := publishTarget{repository: manifest.Repository} + _, proofErr := w.remoteHead(context.Background(), target, manifest.Branch) + requireNoMarker(t, marker) + if err == nil || !strings.Contains(err.Error(), "refused") || !strings.Contains(err.Error(), name) { + t.Fatalf("entry = %v, want a refusal naming %s", err, name) + } + if publishCode(proofErr) != "publish_repository_unavailable" { + t.Fatalf("proof read = %v, want publish_repository_unavailable", proofErr) + } + if kept, _ := os.ReadFile(path); string(kept) != body { + t.Fatalf("the planted %s was changed or removed; it must stay for inspection", name) + } + }) + } +} + +// An entry this process already materialized is still re-checked against its +// registered identity on every acquisition: an origin URL replaced since must +// not receive the push, nor answer the proof read in place of the registered +// remote. +func TestAReadyEntryWithAReplacedOriginIsRefusedForPushAndProof(t *testing.T) { + w, manifest, entry, originDir := cachedWorktree(t) + substitute, _, _ := newOriginRepo(t) + gitRun(t, "", "config", "--file", filepath.Join(entry.dir, "config"), "remote.origin.url", substitute) + if err := os.WriteFile(filepath.Join(manifest.WorktreePath, "README.md"), []byte("edit\n"), 0o644); err != nil { + t.Fatal(err) + } + target := publishTarget{ + attemptID: manifest.AttemptID, repository: manifest.Repository, branch: manifest.Branch, + worktreePath: manifest.WorktreePath, baseSHA: manifest.BaseSHA, + } + + _, pushErr := w.pushAttemptBranch(context.Background(), PublishOptions{}, target, []string{"README.md"}) + _, proofErr := w.remoteHead(context.Background(), target, manifest.Branch) + for _, remote := range []string{substitute, originDir} { + if refs := gitRun(t, "", "ls-remote", remote, "refs/heads/"+manifest.Branch); refs != "" { + t.Fatalf("the attempt branch reached %s: %s", remote, refs) + } + } + if publishCode(pushErr) != "publish_worktree_unsafe" || !strings.Contains(pushErr.Error(), "origin") { + t.Fatalf("publish = %v, want publish_worktree_unsafe naming the origin", pushErr) + } + if publishCode(proofErr) != "publish_repository_unavailable" { + t.Fatalf("proof read = %v, want publish_repository_unavailable", proofErr) + } + if url := gitRun(t, "", "config", "--file", filepath.Join(entry.dir, "config"), "remote.origin.url"); url != substitute { + t.Fatalf("the replaced origin URL was repaired to %q; it must stay for inspection", url) + } +} + +// A `.git` pointing at a SIBLING attempt's admin directory still lands under +// /worktrees, but git would then stage, commit, and push on the +// sibling's branch and index. The admin's gitdir backlink must name this +// worktree, so the attempt is refused before staging and the sibling is +// untouched. +func TestAWorktreePointedAtASiblingAttemptIsRefusedBeforeStaging(t *testing.T) { + w, manifest, entry, _ := cachedWorktree(t) + sibling := filepath.Join(filepath.Dir(manifest.WorktreePath), "attempt-2") + if err := w.createWorktree(context.Background(), entry, manifest.BaseSHA, sibling, "jig/job-2/1"); err != nil { + t.Fatalf("create sibling worktree: %v", err) + } + siblingAdmin := strings.TrimPrefix(strings.TrimSpace(string(mustRead(t, filepath.Join(sibling, ".git")))), "gitdir: ") + if !filepath.IsAbs(siblingAdmin) { + siblingAdmin = filepath.Join(sibling, siblingAdmin) + } + siblingHead := gitRun(t, sibling, "rev-parse", "refs/heads/jig/job-2/1") + siblingIndex := mustRead(t, filepath.Join(siblingAdmin, "index")) + if err := os.WriteFile(filepath.Join(manifest.WorktreePath, ".git"), []byte("gitdir: "+siblingAdmin+"\n"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(manifest.WorktreePath, "README.md"), []byte("edit\n"), 0o644); err != nil { + t.Fatal(err) + } + target := publishTarget{ + attemptID: manifest.AttemptID, repository: manifest.Repository, branch: manifest.Branch, + worktreePath: manifest.WorktreePath, baseSHA: manifest.BaseSHA, + } + + _, pushErr := w.pushAttemptBranch(context.Background(), PublishOptions{}, target, []string{"README.md"}) + cleanupErr := w.cleanupEligible(context.Background(), manifest) + if head := gitRun(t, sibling, "rev-parse", "refs/heads/jig/job-2/1"); head != siblingHead { + t.Fatalf("the sibling's branch moved from %s to %s", siblingHead, head) + } + if index := mustRead(t, filepath.Join(siblingAdmin, "index")); string(index) != string(siblingIndex) { + t.Fatal("the sibling's index was rewritten") + } + if status := gitRun(t, sibling, "status", "--porcelain=v1"); status != "" { + t.Fatalf("the sibling worktree is dirty:\n%s", status) + } + if publishCode(pushErr) != "publish_worktree_unsafe" || !strings.Contains(pushErr.Error(), "gitdir") { + t.Fatalf("publish = %v, want publish_worktree_unsafe naming the gitdir backlink", pushErr) + } + if cleanupErr == nil || !strings.Contains(cleanupErr.Error(), "refused") { + t.Fatalf("cleanup eligibility = %v, want a refusal", cleanupErr) + } + if err := w.checkWorktreeMetadata(context.Background(), manifest.Repository, sibling, "jig/job-2/1"); err != nil { + t.Fatalf("the untouched sibling is refused: %v", err) + } +} + +// The backlink alone can be forged too: with A's `.git` on B's admin AND B's +// gitdir rewritten to name A, the pointer chain is consistent, but A's files +// would still run on B's HEAD, branch, and index. The admin's HEAD must name +// A's own attempt branch, so publication and cleanup refuse before staging, +// and both attempts and the forged pointers stay as they were. +func TestAMutuallySwappedWorktreeIsRefusedByItsBranch(t *testing.T) { + w, manifest, entry, originDir := cachedWorktree(t) + sibling := filepath.Join(filepath.Dir(manifest.WorktreePath), "attempt-2") + siblingBranch := "jig/job-2/1" + if err := w.createWorktree(context.Background(), entry, manifest.BaseSHA, sibling, siblingBranch); err != nil { + t.Fatalf("create sibling worktree: %v", err) + } + adminOf := func(worktree string) string { + admin := strings.TrimPrefix(strings.TrimSpace(string(mustRead(t, filepath.Join(worktree, ".git")))), "gitdir: ") + if !filepath.IsAbs(admin) { + admin = filepath.Join(worktree, admin) + } + return admin + } + ownAdmin, siblingAdmin := adminOf(manifest.WorktreePath), adminOf(sibling) + branchHeads := func() string { + return gitRun(t, entry.dir, "rev-parse", "refs/heads/"+manifest.Branch, "refs/heads/"+siblingBranch) + } + headsBefore := branchHeads() + ownIndex := mustRead(t, filepath.Join(ownAdmin, "index")) + siblingIndex := mustRead(t, filepath.Join(siblingAdmin, "index")) + forged := map[string]string{ + filepath.Join(manifest.WorktreePath, ".git"): "gitdir: " + siblingAdmin + "\n", + filepath.Join(siblingAdmin, "gitdir"): filepath.Join(manifest.WorktreePath, ".git") + "\n", + } + for path, body := range forged { + if err := os.WriteFile(path, []byte(body), 0o644); err != nil { + t.Fatal(err) + } + } + if err := os.WriteFile(filepath.Join(manifest.WorktreePath, "README.md"), []byte("edit\n"), 0o644); err != nil { + t.Fatal(err) + } + target := publishTarget{ + attemptID: manifest.AttemptID, repository: manifest.Repository, branch: manifest.Branch, + worktreePath: manifest.WorktreePath, baseSHA: manifest.BaseSHA, + } + + _, pushErr := w.pushAttemptBranch(context.Background(), PublishOptions{}, target, []string{"README.md"}) + roundErr := w.requireRoundCommitsDeclared(context.Background(), target, manifest.BaseSHA, nil) + cleanupErr := w.cleanupEligible(context.Background(), manifest) + if heads := branchHeads(); heads != headsBefore { + t.Fatalf("attempt branches moved:\nbefore %s\nafter %s", headsBefore, heads) + } + if string(mustRead(t, filepath.Join(ownAdmin, "index"))) != string(ownIndex) || + string(mustRead(t, filepath.Join(siblingAdmin, "index"))) != string(siblingIndex) { + t.Fatal("an attempt's index was rewritten") + } + for _, branch := range []string{manifest.Branch, siblingBranch} { + if refs := gitRun(t, "", "ls-remote", originDir, "refs/heads/"+branch); refs != "" { + t.Fatalf("%s reached the origin: %s", branch, refs) + } + } + if publishCode(pushErr) != "publish_worktree_unsafe" || !strings.Contains(pushErr.Error(), "HEAD") { + t.Fatalf("publish = %v, want publish_worktree_unsafe naming HEAD", pushErr) + } + if publishCode(roundErr) != "publish_worktree_unsafe" { + t.Fatalf("repair round check = %v, want publish_worktree_unsafe", roundErr) + } + if cleanupErr == nil || !strings.Contains(cleanupErr.Error(), "refused") { + t.Fatalf("cleanup eligibility = %v, want a refusal", cleanupErr) + } + for path, body := range forged { + if string(mustRead(t, path)) != body { + t.Fatalf("forged pointer %s was repaired; it must stay for inspection", path) + } + } + if string(mustRead(t, filepath.Join(manifest.WorktreePath, "README.md"))) != "edit\n" { + t.Fatal("the refused worktree lost its content") + } +} + +// Valid layouts still pass the backlink check: a worktree reached through a +// symlinked alias of its root, and one git wrote with relative paths. +func TestOrdinaryWorktreeLayoutsPassTheBacklinkCheck(t *testing.T) { + t.Run("symlinked alias", func(t *testing.T) { + w, manifest, _, _ := cachedWorktree(t) + alias := filepath.Join(t.TempDir(), "alias") + if err := os.Symlink(filepath.Dir(manifest.WorktreePath), alias); err != nil { + t.Fatal(err) + } + aliased := filepath.Join(alias, filepath.Base(manifest.WorktreePath)) + if err := w.checkWorktreeMetadata(context.Background(), manifest.Repository, aliased, manifest.Branch); err != nil { + t.Fatalf("a worktree reached through an alias was refused: %v", err) + } + }) + t.Run("relative paths", func(t *testing.T) { + w, manifest, entry, _ := cachedWorktree(t) + path := filepath.Join(filepath.Dir(manifest.WorktreePath), "attempt-relative") + if _, err := runGit(context.Background(), entry.dir, "-c", "worktree.useRelativePaths=true", + "worktree", "add", "-b", "jig/job-3/1", path, manifest.BaseSHA); err != nil { + t.Skipf("this git cannot write relative worktree paths: %v", err) + } + if pointer := string(mustRead(t, filepath.Join(path, ".git"))); strings.Contains(pointer, "gitdir: /") { + t.Skipf("this git wrote an absolute pointer: %q", pointer) + } + if err := w.checkWorktreeMetadata(context.Background(), manifest.Repository, path, "jig/job-3/1"); err != nil { + t.Fatalf("a relative-path worktree was refused: %v", err) + } + }) +} + +// Git keeps spaces in `.git` and commondir paths — it strips only the line +// terminators — so a pointer that differs from a valid one by a trailing space +// names a different admin or common directory. Each fixture plants that +// directory as a copy of the entry whose config runs a marker as receivepack: +// the push would execute it if the check read the pointer more loosely than +// git does. Both are refused before staging, and the planted metadata stays. +func TestAPointerShiftedByATrailingSpaceIsRefusedWhereGitWouldFollowIt(t *testing.T) { + plant := map[string]func(t *testing.T, manifest attemptManifest, admin, evil string) (string, string){ + ".git": func(t *testing.T, manifest attemptManifest, admin, evil string) (string, string) { + shifted := admin + " " + copyTree(t, admin, shifted) + if err := os.WriteFile(filepath.Join(shifted, "commondir"), []byte(evil+"\n"), 0o644); err != nil { + t.Fatal(err) + } + return filepath.Join(manifest.WorktreePath, ".git"), "gitdir: " + shifted + "\n" + }, + "commondir": func(t *testing.T, _ attemptManifest, admin, evil string) (string, string) { + // "../.. " from the admin directory is /worktrees/".. ". + if err := os.Rename(evil, filepath.Join(filepath.Dir(admin), ".. ")); err != nil { + t.Fatal(err) + } + return filepath.Join(admin, "commondir"), "../.. \n" + }, + } + for name, apply := range plant { + t.Run(name, func(t *testing.T) { + w, manifest, entry, originDir := cachedWorktree(t) + script, marker := markerScript(t) + evil := filepath.Join(t.TempDir(), "evil.git") + copyTree(t, entry.dir, evil) + appendConfig(t, filepath.Join(evil, "config"), "[remote \"origin\"]\n\treceivepack = "+script+"\n") + admin := worktreeAdmin(t, manifest.WorktreePath) + path, body := apply(t, manifest, admin, evil) + if err := os.WriteFile(path, []byte(body), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(manifest.WorktreePath, "README.md"), []byte("edit\n"), 0o644); err != nil { + t.Fatal(err) + } + target := publishTarget{ + attemptID: manifest.AttemptID, repository: manifest.Repository, branch: manifest.Branch, + worktreePath: manifest.WorktreePath, baseSHA: manifest.BaseSHA, + } + + _, pushErr := w.pushAttemptBranch(context.Background(), PublishOptions{}, target, []string{"README.md"}) + cleanupErr := w.cleanupEligible(context.Background(), manifest) + requireNoMarker(t, marker) + if publishCode(pushErr) != "publish_worktree_unsafe" || !strings.Contains(pushErr.Error(), name) { + t.Fatalf("publish = %v, want publish_worktree_unsafe naming %s", pushErr, name) + } + if cleanupErr == nil || !strings.Contains(cleanupErr.Error(), "refused") { + t.Fatalf("cleanup eligibility = %v, want a refusal", cleanupErr) + } + if refs := gitRun(t, "", "ls-remote", originDir, "refs/heads/"+manifest.Branch); refs != "" { + t.Fatalf("the attempt branch reached the origin: %s", refs) + } + if string(mustRead(t, path)) != body { + t.Fatalf("the planted %s was repaired; it must stay for inspection", name) + } + if string(mustRead(t, filepath.Join(manifest.WorktreePath, "README.md"))) != "edit\n" { + t.Fatal("the refused worktree lost its content") + } + }) + } +} + +// Git keeps a quoted origin URL's trailing space too, so `url = " "` +// pushes to a different local repository than the registered it +// would normalize to. It is refused before normalization, neither repository +// receives the branch, and the URL stays for inspection. +func TestAnOriginURLShiftedByATrailingSpaceIsRefused(t *testing.T) { + w, manifest, entry, originDir := cachedWorktree(t) + shifted := originDir + " " + copyTree(t, originDir, shifted) + config := filepath.Join(entry.dir, "config") + gitRun(t, "", "config", "--file", config, "remote.origin.url", shifted) + if err := os.WriteFile(filepath.Join(manifest.WorktreePath, "README.md"), []byte("edit\n"), 0o644); err != nil { + t.Fatal(err) + } + target := publishTarget{ + attemptID: manifest.AttemptID, repository: manifest.Repository, branch: manifest.Branch, + worktreePath: manifest.WorktreePath, baseSHA: manifest.BaseSHA, + } + + _, pushErr := w.pushAttemptBranch(context.Background(), PublishOptions{}, target, []string{"README.md"}) + _, proofErr := w.remoteHead(context.Background(), target, manifest.Branch) + for _, remote := range []string{shifted, originDir} { + if refs := gitRun(t, "", "ls-remote", remote, "refs/heads/"+manifest.Branch); refs != "" { + t.Fatalf("the attempt branch reached %q: %s", remote, refs) + } + } + if publishCode(pushErr) != "publish_worktree_unsafe" || !strings.Contains(pushErr.Error(), "origin") { + t.Fatalf("publish = %v, want publish_worktree_unsafe naming the origin", pushErr) + } + if publishCode(proofErr) != "publish_repository_unavailable" { + t.Fatalf("proof read = %v, want publish_repository_unavailable", proofErr) + } + if raw, _ := runGit(context.Background(), "", "config", "--file", config, "remote.origin.url"); raw != shifted+"\n" { + t.Fatalf("the shifted origin URL was repaired to %q; it must stay for inspection", raw) + } +} + +// Spaces INSIDE paths are ordinary: a data directory, worktree, and origin +// whose paths all contain spaces still pass validation and publish. +func TestPathsWithInternalSpacesPassValidationAndPublish(t *testing.T) { + plainOrigin, head, _ := newOriginRepo(t) + originDir := filepath.Join(t.TempDir(), "steel origin", "repo.git") + gitRun(t, "", "clone", "--quiet", "--bare", plainOrigin, originDir) + identity, err := normalizeRepositoryIdentity(originDir) + if err != nil { + t.Fatal(err) + } + dataDir := filepath.Join(t.TempDir(), "jig data") + w := &Worker{config: Config{DataDir: dataDir}, cache: newRepoCache(filepath.Join(dataDir, "repos"))} + entry, err := w.cache.entry(context.Background(), identity) + if err != nil { + t.Fatalf("materialize cache entry: %v", err) + } + path := filepath.Join(dataDir, "worktrees", "attempt 1") + if err := w.createWorktree(context.Background(), entry, head, path, "jig/job-1/1"); err != nil { + t.Fatalf("create worktree: %v", err) + } + if err := w.checkWorktreeMetadata(context.Background(), identity, path, "jig/job-1/1"); err != nil { + t.Fatalf("a worktree with spaces in its paths was refused: %v", err) + } + if err := os.WriteFile(filepath.Join(path, "README.md"), []byte("edit\n"), 0o644); err != nil { + t.Fatal(err) + } + target := publishTarget{ + attemptID: "attempt-1", repository: identity, branch: "jig/job-1/1", worktreePath: path, baseSHA: head, + } + if _, err := w.pushAttemptBranch(context.Background(), PublishOptions{}, target, []string{"README.md"}); err != nil { + t.Fatalf("publish from paths with spaces: %v", err) + } + if refs := gitRun(t, "", "ls-remote", originDir, "refs/heads/jig/job-1/1"); refs == "" { + t.Fatal("the attempt branch did not reach the origin") + } +} + +// Line terminators are the one thing git strips from a pointer, so a CRLF +// pointer git follows to the same admin directory still passes. +func TestACRLFTerminatedPointerStillPasses(t *testing.T) { + w, manifest, _, _ := cachedWorktree(t) + admin := worktreeAdmin(t, manifest.WorktreePath) + if err := os.WriteFile(filepath.Join(manifest.WorktreePath, ".git"), []byte("gitdir: "+admin+"\r\n"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(admin, "commondir"), []byte("../..\r\n"), 0o644); err != nil { + t.Fatal(err) + } + if err := w.checkWorktreeMetadata(context.Background(), manifest.Repository, manifest.WorktreePath, manifest.Branch); err != nil { + t.Fatalf("a CRLF-terminated pointer was refused: %v", err) + } +} + +// worktreeAdmin is the absolute admin directory a jig-built worktree's `.git` +// names. +func worktreeAdmin(t *testing.T, worktree string) string { + t.Helper() + admin := strings.TrimPrefix(strings.TrimRight(string(mustRead(t, filepath.Join(worktree, ".git"))), "\n"), "gitdir: ") + if !filepath.IsAbs(admin) { + admin = filepath.Join(worktree, admin) + } + return admin +} + +func copyTree(t *testing.T, source, destination string) { + t.Helper() + if output, err := exec.Command("cp", "-R", source, destination).CombinedOutput(); err != nil { + t.Fatalf("copy %s: %v: %s", source, err, output) + } +} + +func mustRead(t *testing.T, path string) []byte { + t.Helper() + body, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + return body +} + +// The worker's own git ignores fsmonitor and hooks wherever it runs, even +// in a repository no validation has looked at. +func TestWorkerGitDoesNotRunRepositoryFsmonitorOrHooks(t *testing.T) { + repo := t.TempDir() + gitRun(t, "", "init", "--quiet", repo) + script, marker := markerScript(t) + appendConfig(t, filepath.Join(repo, ".git", "config"), "[core]\n\tfsmonitor = "+script+"\n") + if err := os.Symlink(script, filepath.Join(repo, ".git", "hooks", "pre-commit")); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(repo, "a.txt"), []byte("a\n"), 0o644); err != nil { + t.Fatal(err) + } + gitRun(t, repo, "status", "--porcelain=v1") + gitRun(t, repo, "add", "a.txt") + gitRun(t, repo, "-c", "user.name=jig-test", "-c", "user.email=jig@test", "commit", "--quiet", "-m", "a") + requireNoMarker(t, marker) +} + +// Hardening must not cost publication its credentials: the operator's +// GLOBAL credential helper is still consulted. Isolated fixture — a temporary +// global config, no system config, a local server that only ever answers +// 401, and a helper that hands out a dummy credential. +func TestWorkerGitStillUsesTheGlobalCredentialHelper(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(response http.ResponseWriter, _ *http.Request) { + response.Header().Set("WWW-Authenticate", `Basic realm="fixture"`) + response.WriteHeader(http.StatusUnauthorized) + })) + defer server.Close() + marker := filepath.Join(t.TempDir(), "helper-called") + global := filepath.Join(t.TempDir(), "gitconfig") + helper := "!f() { echo called >> '" + marker + "'; echo username=fixture; echo password=dummy; }; f" + if err := os.WriteFile(global, []byte("[credential]\n\thelper = \""+helper+"\"\n"), 0o644); err != nil { + t.Fatal(err) + } + t.Setenv("HOME", t.TempDir()) + t.Setenv("GIT_CONFIG_GLOBAL", global) + t.Setenv("GIT_CONFIG_NOSYSTEM", "1") + + if _, err := runGit(context.Background(), "", "ls-remote", server.URL+"/repo.git"); err == nil { + t.Fatal("ls-remote against an always-401 server succeeded") + } + if _, err := os.Stat(marker); err != nil { + t.Fatalf("the global credential helper was not consulted: %v", err) + } +} + +// advanceOrigin pushes one more commit to the origin, so the entry's next +// fetch updates a ref. +func advanceOrigin(t *testing.T, originDir string) { + t.Helper() + clone := filepath.Join(t.TempDir(), "clone") + gitRun(t, "", "clone", "--quiet", originDir, clone) + gitRun(t, clone, "-c", "user.name=jig-test", "-c", "user.email=jig@test", + "commit", "--quiet", "--allow-empty", "-m", "advance") + gitRun(t, clone, "push", "--quiet", "origin", "HEAD:main") +} + +func TestMetadataValuesKeepGitPathWhitespaceUnambiguous(t *testing.T) { + for _, test := range []struct { + name, content, value string + valid bool + }{ + {"no terminator", "path", "path", true}, + {"LF", "path\n", "path", true}, + {"CRLF", "path\r\n", "path", true}, + {"repeated terminators", "path\r\n\r\n", "path", true}, + {"internal space", "one two\n", "one two", true}, + {"trailing space", "path \n", "path ", false}, + {"trailing tab", "path\t\n", "path\t", false}, + {"leading space", " path\n", " path", false}, + {"embedded LF", "one\ntwo\n", "one\ntwo", false}, + {"embedded CR", "one\rtwo\n", "one\rtwo", false}, + {"NUL", "path\x00\n", "path\x00", false}, + {"empty", "\r\n", "", false}, + } { + t.Run(test.name, func(t *testing.T) { + value, valid := metadataValue(test.content) + if value != test.value || valid != test.valid { + t.Fatalf("metadata value = %q, %t; want %q, %t", value, valid, test.value, test.valid) + } + }) + } +} + +func TestOriginRefusalDoesNotEchoEmbeddedCredentials(t *testing.T) { + _, _, entry, _ := cachedWorktree(t) + for _, suffix := range []string{" ", "\t", "\n", "\x01path"} { + t.Run(fmt.Sprintf("suffix %q", suffix), func(t *testing.T) { + raw := "https://fixture-user:fixture-password@github.com/owner/repo" + suffix + if _, err := runGit(context.Background(), entry.dir, "config", "remote.origin.url", raw); err != nil { + t.Fatal(err) + } + err := entry.validateOrigin(context.Background()) + if err == nil || !strings.Contains(err.Error(), "padding or control characters") { + t.Fatalf("origin validation error = %v, want unsafe URL refusal", err) + } + if strings.Contains(err.Error(), "fixture-password") || strings.Contains(err.Error(), "fixture-user") { + t.Fatal("origin refusal exposed URL credentials") + } + }) + } +} + +func TestSymlinkParentTraversalCannotRedirectWorkerGit(t *testing.T) { + for _, relative := range []bool{false, true} { + t.Run(fmt.Sprintf("relative %t", relative), func(t *testing.T) { + w, manifest, entry, origin := cachedWorktree(t) + script, marker := markerScript(t) + evil := filepath.Join(t.TempDir(), "evil.git") + copyTree(t, entry.dir, evil) + appendConfig(t, filepath.Join(evil, "config"), "[remote \"origin\"]\n\treceivepack = "+script+"\n") + admin := worktreeAdmin(t, manifest.WorktreePath) + hopTarget := filepath.Join(evil, "worktrees", "hop-target") + if err := os.MkdirAll(hopTarget, 0o755); err != nil { + t.Fatal(err) + } + hop := filepath.Join(filepath.Dir(admin), "hop") + if err := os.Symlink(hopTarget, hop); err != nil { + t.Fatal(err) + } + if relative { + var err error + hop, err = filepath.Rel(manifest.WorktreePath, hop) + if err != nil { + t.Fatal(err) + } + } + // Git follows the symlink before ..; cleaning first hides evil.git. + pointer := "gitdir: " + hop + "/../" + filepath.Base(admin) + "\n" + pointerPath := filepath.Join(manifest.WorktreePath, ".git") + if err := os.WriteFile(pointerPath, []byte(pointer), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(manifest.WorktreePath, "README.md"), []byte("fixture edit\n"), 0o644); err != nil { + t.Fatal(err) + } + target := publishTarget{ + attemptID: manifest.AttemptID, repository: manifest.Repository, branch: manifest.Branch, + worktreePath: manifest.WorktreePath, baseSHA: manifest.BaseSHA, + } + _, err := w.pushAttemptBranch(context.Background(), PublishOptions{}, target, []string{"README.md"}) + requireNoMarker(t, marker) + if publishCode(err) != "publish_worktree_unsafe" { + t.Fatalf("publish = %v, want unsafe worktree refusal", err) + } + if err := w.cleanupEligible(context.Background(), manifest); err == nil { + t.Fatal("redirected worktree was eligible for cleanup") + } + if got := string(mustRead(t, pointerPath)); got != pointer { + t.Fatal("refusal changed the planted pointer") + } + if refs := gitRun(t, "", "ls-remote", origin, "refs/heads/"+manifest.Branch); refs != "" { + t.Fatalf("attempt reached origin: %s", refs) + } + }) + } +} + +// Git hands an origin in a scheme it does not implement itself to +// git-remote- from PATH, and protocol.ext.allow=never does not cover +// that: `jig-boundary-probe://github.com/owner/repo.git` normalizes to the +// registered github.com/owner/repo yet runs a helper an agent could plant. +// The origin is refused by transport before the identity comparison, the +// helper never runs, and the URL stays for inspection. +func TestAnOriginInACustomHelperTransportIsRefusedBeforeGitDispatchesIt(t *testing.T) { + for _, origin := range []string{ + "jig-boundary-probe://github.com/owner/repo.git", + "jig-boundary-probe://fixture-user:fixture-password@github.com/owner/repo.git", + "jig-boundary-probe::https://github.com/owner/repo.git", + } { + t.Run(origin, func(t *testing.T) { + helperDir := t.TempDir() + marker := filepath.Join(t.TempDir(), "ran") + helper := "#!/bin/sh\necho ran >> '" + marker + "'\nexit 1\n" + if err := os.WriteFile(filepath.Join(helperDir, "git-remote-jig-boundary-probe"), []byte(helper), 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", helperDir+string(os.PathListSeparator)+os.Getenv("PATH")) + dir := filepath.Join(t.TempDir(), "entry") + gitRun(t, "", "init", "--quiet", "--bare", "--template=", dir) + gitRun(t, dir, "remote", "add", "origin", origin) + entry := &repoEntry{identity: "github.com/owner/repo", dir: dir} + if err := entry.validateMetadata(context.Background()); err != nil { + t.Fatalf("metadata refused, so the transport check is not what is tested: %v", err) + } + + err := entry.fetch(context.Background()) + requireNoMarker(t, marker) + if err == nil || !strings.Contains(err.Error(), "transport") { + t.Fatalf("fetch = %v, want a transport refusal", err) + } + if strings.Contains(err.Error(), "fixture-user") || strings.Contains(err.Error(), "fixture-password") { + t.Fatal("transport refusal exposed URL credentials") + } + if url := gitRun(t, dir, "config", "remote.origin.url"); url != origin { + t.Fatalf("the origin URL was repaired to %q; it must stay for inspection", url) + } + }) + } +} + +// The origins jig's clone writes, and the standard transports git implements +// itself, still pass origin validation. Nothing is fetched. +func TestStandardOriginTransportsPassOriginValidation(t *testing.T) { + local, _, _ := newOriginRepo(t) + for _, origin := range []string{ + "https://github.com/owner/repo.git", + "http://github.com/owner/repo.git", + "ssh://git@github.com/owner/repo.git", + "git://github.com/owner/repo.git", + "git@github.com:owner/repo.git", + "https://github.com/owner/re::po.git", + "ssh://git@[::1]/owner/repo.git", + "file://" + local, + local, + } { + t.Run(origin, func(t *testing.T) { + identity, err := normalizeRepositoryIdentity(origin) + if err != nil { + t.Fatal(err) + } + dir := filepath.Join(t.TempDir(), "entry") + gitRun(t, "", "init", "--quiet", "--bare", "--template=", dir) + gitRun(t, dir, "remote", "add", "origin", origin) + entry := &repoEntry{identity: identity, dir: dir} + if err := entry.validateOrigin(context.Background()); err != nil { + t.Fatalf("a standard origin was refused: %v", err) + } + }) + } +} + +// A local origin is a path, whatever bytes it holds: a directory named with +// `::`, or a `:` directory followed by a doubled separator, is neither a +// helper nor a scheme to the transport check. The `::` origin fetches; git +// itself reads any `://` as a URL and refuses that one ("protocol not +// supported") without a helper, so it is checked by validation alone — jig's +// own clone never writes it, since canonical paths have no `//`. +func TestLocalOriginPathsThatLookLikeTransportSyntaxPassValidation(t *testing.T) { + plainOrigin, head, _ := newOriginRepo(t) + base := t.TempDir() + for _, test := range []struct { + name, origin string + fetch bool + }{ + {"double colon", filepath.Join(base, "a::b", "origin.git"), true}, + {"colon slash slash", filepath.Join(base, "c:") + "//d/origin.git", false}, + } { + t.Run(test.name, func(t *testing.T) { + gitRun(t, "", "clone", "--quiet", "--bare", plainOrigin, test.origin) + identity, err := normalizeRepositoryIdentity(test.origin) + if err != nil { + t.Fatal(err) + } + dir := filepath.Join(t.TempDir(), "entry") + gitRun(t, "", "init", "--quiet", "--bare", "--template=", dir) + gitRun(t, dir, "remote", "add", "origin", test.origin) + gitRun(t, dir, "config", "remote.origin.fetch", "+refs/heads/*:refs/remotes/origin/*") + entry := &repoEntry{identity: identity, dir: dir} + if err := entry.validateOrigin(context.Background()); err != nil { + t.Fatalf("a local origin at %q was refused: %v", test.origin, err) + } + if !test.fetch { + return + } + if err := entry.fetch(context.Background()); err != nil { + t.Fatalf("a local origin at %q did not fetch: %v", test.origin, err) + } + if !entry.hasCommit(context.Background(), head) { + t.Fatal("the fetch did not bring the origin's head") + } + }) + } +} + +// Refusals of config keys and origin URLs never quote the raw text: a URL +// subsection or an origin can carry credentials. +func TestMetadataAndOriginRefusalsDoNotEchoCredentials(t *testing.T) { + for name, text := range map[string]string{ + "unknown key subsection": "[credential \"https://fixture-user:fixture-password@github.com\"]\n\tusername = x\n", + "known key value": "[core]\n\tsymlinks = https://fixture-user:fixture-password@github.com\n", + } { + t.Run(name, func(t *testing.T) { + _, manifest, entry, _ := cachedWorktree(t) + appendConfig(t, filepath.Join(entry.dir, "config"), text) + _, err := newRepoCache(filepath.Dir(entry.dir)).entry(context.Background(), manifest.Repository) + if err == nil || !strings.Contains(err.Error(), "refused") { + t.Fatalf("entry = %v, want a refusal", err) + } + if strings.Contains(err.Error(), "fixture-user") || strings.Contains(err.Error(), "fixture-password") { + t.Fatal("config refusal exposed URL credentials") + } + }) + } + _, _, entry, _ := cachedWorktree(t) + for _, origin := range []string{ + "https://fixture-user:fixture-password@github.com:badport/owner/repo.git", + filepath.Join(t.TempDir(), "fixture-user:fixture-password@missing"), + } { + t.Run("malformed origin", func(t *testing.T) { + if _, err := runGit(context.Background(), entry.dir, "config", "remote.origin.url", origin); err != nil { + t.Fatal(err) + } + err := entry.validateOrigin(context.Background()) + if err == nil { + t.Fatal("a malformed origin passed validation") + } + if strings.Contains(err.Error(), "fixture-user") || strings.Contains(err.Error(), "fixture-password") { + t.Fatal("origin refusal exposed URL credentials") + } + }) + } +} diff --git a/internal/worker/publish.go b/internal/worker/publish.go index e3c81fa..ba9e72a 100644 --- a/internal/worker/publish.go +++ b/internal/worker/publish.go @@ -603,6 +603,9 @@ func (w *Worker) pushAttemptBranch( return "", publishFailure("publish_worktree_missing", "the attempt worktree at %s is unavailable: %s", target.worktreePath, err) } + if err := w.checkWorktreeMetadata(ctx, target.repository, target.worktreePath, target.branch); err != nil { + return "", publishFailure("publish_worktree_unsafe", "%s", err.Error()) + } head, err := w.commitChangedPaths(ctx, options, target, changedPaths) if err != nil { return "", err diff --git a/internal/worker/publish_repair.go b/internal/worker/publish_repair.go index bb57a72..2fd858f 100644 --- a/internal/worker/publish_repair.go +++ b/internal/worker/publish_repair.go @@ -203,6 +203,9 @@ func (w *Worker) authorizeRound(ctx context.Context, target publishTarget, round // paths get the same check from commitChangedPaths; this is its twin for // what the round committed itself. func (w *Worker) requireRoundCommitsDeclared(ctx context.Context, target publishTarget, red string, declared []string) error { + if err := w.checkWorktreeMetadata(ctx, target.repository, target.worktreePath, target.branch); err != nil { + return publishFailure("publish_worktree_unsafe", "%s", err.Error()) + } committed, err := runGit(ctx, target.worktreePath, "diff", "--name-only", "-z", red, "HEAD") if err != nil { return publishFailure("publish_staging_unreadable", "%s", err.Error()) diff --git a/internal/worker/repocache.go b/internal/worker/repocache.go index 902a646..7ad76d0 100644 --- a/internal/worker/repocache.go +++ b/internal/worker/repocache.go @@ -16,8 +16,10 @@ import ( "os" "os/exec" "path/filepath" + "slices" "strings" "sync" + "unicode" "github.com/StructuPath/jig/internal/protocol" ) @@ -77,13 +79,21 @@ func (c *repoCache) entry(ctx context.Context, repository string) (*repoEntry, e entry.mutex.Lock() defer entry.mutex.Unlock() - if entry.ready { - return entry, nil + if !entry.ready { + if err := c.materializeLocked(ctx, entry, repository); err != nil { + return nil, err + } + entry.ready = true + } + // Every use, not only the first: agents run git in worktrees whose + // metadata IS this entry, so a ready entry can have been rewritten since — + // including its origin URL, which the push and the proof read both use. + if err := entry.validateMetadata(ctx); err != nil { + return nil, err } - if err := c.materializeLocked(ctx, entry, repository); err != nil { + if err := entry.validateOrigin(ctx); err != nil { return nil, err } - entry.ready = true return entry, nil } @@ -95,8 +105,12 @@ func (c *repoCache) materializeLocked(ctx context.Context, entry *repoEntry, rep if !info.IsDir() || info.Mode()&os.ModeSymlink != 0 { return errors.New("repository cache entry must be a real directory") } - // Reused entry from an earlier process: the origin must still match - // the registered identity before anything trusts it. + // Reused entry from an earlier process: its metadata must be inert and + // its origin must still match the registered identity before anything + // trusts it. + if err := entry.validateMetadata(ctx); err != nil { + return err + } return entry.validateOrigin(ctx) } else if !errors.Is(err, os.ErrNotExist) { return fmt.Errorf("inspect repository cache entry: %w", err) @@ -120,8 +134,10 @@ func (c *repoCache) materializeLocked(ctx context.Context, entry *repoEntry, rep _ = os.RemoveAll(temporary) }() buildDir := filepath.Join(temporary, "repository") + // No template: an operator's init.templateDir hooks would be active hooks + // in a jig-managed entry, which validateMetadata refuses. steps := [][]string{ - {"init", "--bare", buildDir}, + {"init", "--bare", "--template=", buildDir}, {"-C", buildDir, "remote", "add", "origin", source}, {"-C", buildDir, "config", "remote.origin.fetch", "+refs/heads/*:refs/remotes/origin/*"}, {"-C", buildDir, "fetch", "--no-tags", "origin"}, @@ -200,9 +216,27 @@ func (e *repoEntry) validateOrigin(ctx context.Context) error { if err != nil { return fmt.Errorf("revalidate repository origin: %w", err) } - identity, err := normalizeRepositoryIdentity(strings.TrimSpace(stdout)) + // get-url prints the URL and one newline. Normalization trims the rest, + // but git does not: a quoted `url = "/path/origin "` names a different + // local repository — whose hooks a push runs — than the registered + // /path/origin it would normalize to. A URL edged with whitespace is + // nothing jig's clone writes, so it is refused before normalization. + raw := strings.TrimSuffix(stdout, "\n") + if !plainMetadataValue(raw) || strings.ContainsFunc(raw, unicode.IsControl) { + return errors.New("revalidate repository origin: origin URL contains padding or control characters") + } + // Normalization reads any scheme as host/path, but git hands a scheme it + // does not implement — and any `::
` — to git-remote- + // from PATH, which protocol.ext.allow does not cover. Only the transports + // git implements itself are admitted, before the identity comparison. + if !nativeOriginTransport(raw) { + return errors.New("revalidate repository origin: origin URL uses an unsupported transport") + } + // Neither the URL nor the normalization error is quoted: an origin can + // carry credentials. + identity, err := normalizeRepositoryIdentity(raw) if err != nil { - return fmt.Errorf("revalidate repository origin: %w", err) + return errors.New("revalidate repository origin: origin URL is not a recognized repository form") } if !sameRemoteIdentity(identity, e.identity) { return errors.New("repository origin no longer matches the registered identity") @@ -210,6 +244,34 @@ func (e *repoEntry) validateOrigin(ctx context.Context) error { return nil } +// nativeOriginTransports are the URL schemes git implements itself (remote +// curl ships as git-remote-http/https in git's own exec path). +var nativeOriginTransports = []string{"https", "http", "ssh", "git", "file"} + +// nativeOriginTransport reports whether an origin URL is one git transports +// without dispatching a remote helper: an absolute local path, a URL in a +// native scheme, or scp-style user@host:path. An absolute path is checked +// first, so `::` or `://` in its bytes is not read as helper or scheme syntax +// (git itself refuses a path holding `://`, with no helper), and a native +// URL may hold `::` in an IPv6 host or its path, so only what is left +// is refused for helper syntax; `helper::https://` fails as an unknown +// scheme. Scheme case is not folded — git would look up git-remote-HTTPS — +// and anything else, relative paths included, is refused rather than +// interpreted. +func nativeOriginTransport(raw string) bool { + if filepath.IsAbs(raw) { + return true + } + if scheme, _, found := strings.Cut(raw, "://"); found { + return slices.Contains(nativeOriginTransports, scheme) + } + if strings.Contains(raw, "::") { + return false + } + prefix, _, found := strings.Cut(raw, ":") + return found && strings.Contains(prefix, "@") && !strings.Contains(prefix, "/") +} + // fetch updates remote-tracking refs, with origin revalidation before and // after: a fetch from a swapped origin must fail, and an origin swapped // during the fetch must be detected before anything trusts the refs. @@ -286,12 +348,30 @@ func cacheDirectoryName(identityKey string) string { // ---- git command runner ---------------------------------------------------- -// runGit runs one git command with a bounded timeout and no terminal -// prompting, returning stdout. Failures carry a stderr excerpt. +// workerGitHardening is prepended to every worker git command. Cleanup, +// staging, and publish run git inside worktrees agents wrote, against cache +// entries those worktrees share: a `core.fsmonitor` command, a hook, or an +// `ext::` remote left there would otherwise run worker-side. It is the +// engine's switch set minus core.quotepath, whose output change no worker +// caller expects. The environment is deliberately NOT the engine's +// sanitized one: push and fetch need the operator's global credential +// helpers, and repository-local config that could subvert those is refused +// by validateMetadata before git ever reads it. +var workerGitHardening = []string{ + "-c", "core.hooksPath=/dev/null", + "-c", "core.fsmonitor=", + "-c", "protocol.ext.allow=never", + "-c", "gc.auto=0", + "-c", "maintenance.auto=false", +} + +// runGit runs one hardened git command with a bounded timeout and no +// terminal prompting, returning stdout. Failures carry a stderr excerpt. func runGit(ctx context.Context, dir string, arguments ...string) (string, error) { ctx, cancel := context.WithTimeout(ctx, protocol.GitCommandTimeout) defer cancel() - command := exec.CommandContext(ctx, "git", arguments...) + command := exec.CommandContext(ctx, "git", + append(append([]string(nil), workerGitHardening...), arguments...)...) if dir != "" { command.Dir = dir } diff --git a/internal/worker/worktree.go b/internal/worker/worktree.go index 0169475..0371163 100644 --- a/internal/worker/worktree.go +++ b/internal/worker/worktree.go @@ -154,6 +154,9 @@ func (w *Worker) cleanupEligible(ctx context.Context, manifest attemptManifest) } entry.mutex.Lock() defer entry.mutex.Unlock() + if err := entry.validateWorktree(manifest.WorktreePath, manifest.Branch); err != nil { + return err + } status, err := runGit(ctx, manifest.WorktreePath, "--no-optional-locks", "status", "--porcelain=v1") if err != nil { return fmt.Errorf("worktree status unavailable: %w", err)