diff --git a/cmd/odek/project_sandbox_approval.go b/cmd/odek/project_sandbox_approval.go index b93eca69..3d825d2c 100644 --- a/cmd/odek/project_sandbox_approval.go +++ b/cmd/odek/project_sandbox_approval.go @@ -62,6 +62,13 @@ func approveProjectSandboxWithTTY(resolved config.ResolvedConfig, stdin io.Reade o.HasUser || o.HasMemory || o.HasCPUs || o.HasPidsLimit || o.HasNetworks || o.HasComposeProject dfHash, dfRequired := dockerfileBuildRequirement(resolved) if !hasOverride && !dfRequired { + // Nothing to approve: the project requests no networks, so no + // earlier network grant for this directory may linger. + if wd, err := os.Getwd(); err == nil { + if dir, err := filepath.Abs(wd); err == nil { + recordProjectNetworkGrant(dir, o, nil) + } + } return nil } @@ -185,8 +192,8 @@ func approveProjectSandboxWithTTY(resolved config.ResolvedConfig, stdin io.Reade } if hasOverride { recordOverrideGrant(projectDir, overrideKey) - recordProjectNetworkGrant(projectDir, o, nets) } + recordProjectNetworkGrant(projectDir, o, nets) return nil case "t", "trust": if hasOverride { @@ -199,9 +206,7 @@ func approveProjectSandboxWithTTY(resolved config.ResolvedConfig, stdin io.Reade if err := saveProjectSandboxApprovals(approved); err != nil { return fmt.Errorf("project sandbox approval: save approvals: %w", err) } - if hasOverride { - recordProjectNetworkGrant(projectDir, o, nets) - } + recordProjectNetworkGrant(projectDir, o, nets) return nil default: return fmt.Errorf("project sandbox config was not approved") @@ -420,18 +425,20 @@ var ( projectNetworkGrants = map[string]map[string]bool{} ) -// recordProjectNetworkGrant remembers the networks behind an approval of a -// project override that requests networks. +// recordProjectNetworkGrant remembers the networks behind the latest +// approval for projectDir. An approved override that requests no networks +// drops any earlier grant, so the record always matches the latest approval. func recordProjectNetworkGrant(projectDir string, o config.ProjectSandboxOverride, nets []sandbox.NetworkInfo) { + projectNetworkGrantsMu.Lock() + defer projectNetworkGrantsMu.Unlock() if !o.HasNetworks && !o.HasComposeProject { + delete(projectNetworkGrants, projectDir) return } ids := map[string]bool{} for _, n := range nets { ids[n.ID] = true } - projectNetworkGrantsMu.Lock() - defer projectNetworkGrantsMu.Unlock() projectNetworkGrants[projectDir] = ids } diff --git a/cmd/odek/red_fd_handoff_test.go b/cmd/odek/red_fd_handoff_test.go index 86b94370..23840e34 100644 --- a/cmd/odek/red_fd_handoff_test.go +++ b/cmd/odek/red_fd_handoff_test.go @@ -1,9 +1,11 @@ package main import ( - "os" + "fmt" + "go/ast" + "go/parser" + "go/token" "path/filepath" - "regexp" "strings" "testing" ) @@ -13,22 +15,136 @@ import ( // descriptor still owned by an *os.File gets it closed twice: the file's // finalizer later closes whatever the number has been reused for — in a // parallel test often the directory fd os.RemoveAll is walking, which then -// fails with "unlinkat: bad file descriptor". Tests must hand over a dup. +// fails with "unlinkat: bad file descriptor". So in every test function that +// sets one of those env vars, each .Fd() call must sit inside syscall.Dup or +// dupFD: the reader is only ever handed a descriptor of its own, however the +// number reaches the env var. func TestRED_FDHandoffTestsPassADuplicatedDescriptor(t *testing.T) { files, err := filepath.Glob("*_test.go") if err != nil { t.Fatal(err) } - raw := regexp.MustCompile(`t\.Setenv\((keyFDEnvVar|frameFDEnvVar)[^\n]*\.Fd\(\)`) for _, f := range files { - data, err := os.ReadFile(f) - if err != nil { - t.Fatal(err) + for _, v := range fdHandoffViolations(t, f, nil) { + t.Error(v) } - for _, m := range raw.FindAllString(string(data), -1) { - if !strings.Contains(m, "Dup") && !strings.Contains(m, "dupFD") { - t.Errorf("%s hands an owned descriptor to the fd reader: %s (use dupFD)", f, m) + } +} + +// The checker itself must flag a raw handoff, however the number reaches +// the env var, and accept a dup. +func TestRED_FDHandoffChecker_FlagsRawAndAcceptsDup(t *testing.T) { + const src = `package main + +func rawDirect(t *testing.T, r *os.File) { t.Setenv(keyFDEnvVar, fmt.Sprint(r.Fd())) } + +func rawViaVariable(t *testing.T, r *os.File) { + fd := int(r.Fd()) + t.Setenv(frameFDEnvVar, strconv.Itoa(fd)) +} + +func rawInClosure(t *testing.T, r *os.File) { + f := func() string { return fmt.Sprint(r.Fd()) } + os.Setenv(keyFDEnvVar, f()) +} + +func viaDupFD(t *testing.T, r *os.File) { t.Setenv(keyFDEnvVar, dupFD(t, r)) } + +func viaSyscallDup(t *testing.T, r *os.File) { + fd, _ := syscall.Dup(int(r.Fd())) + t.Setenv(keyFDEnvVar, strconv.Itoa(fd)) +} + +func unrelated(r *os.File) uintptr { return r.Fd() } +` + got := fdHandoffViolations(t, "fixture_test.go", src) + want := []string{"rawDirect", "rawViaVariable", "rawInClosure"} + if len(got) != len(want) { + t.Fatalf("violations = %q, want one each for %v", got, want) + } + for i, name := range want { + if !strings.Contains(got[i], " "+name+" ") { + t.Errorf("violation %d = %q, want it to name %s", i, got[i], name) + } + } +} + +// fdHandoffViolations parses one test file (or src when non-nil) and +// describes every .Fd() call outside syscall.Dup/dupFD in a function that +// sets the key or frame fd env var. +func fdHandoffViolations(t *testing.T, name string, src any) []string { + t.Helper() + fset := token.NewFileSet() + file, err := parser.ParseFile(fset, name, src, 0) + if err != nil { + t.Fatal(err) + } + var out []string + for _, decl := range file.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok || fn.Body == nil || fn.Name.Name == "dupFD" || !setsFDEnvVar(fn.Body) { + continue + } + for _, pos := range rawFdCalls(fn.Body) { + out = append(out, fmt.Sprintf("%s: %s reads an owned descriptor with .Fd() outside syscall.Dup/dupFD while handing fds to the key/frame reader", + fset.Position(pos), fn.Name.Name)) + } + } + return out +} + +// setsFDEnvVar reports whether body calls t.Setenv(keyFDEnvVar|frameFDEnvVar, …). +func setsFDEnvVar(body *ast.BlockStmt) bool { + found := false + ast.Inspect(body, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok || len(call.Args) == 0 { + return true + } + if sel, ok := call.Fun.(*ast.SelectorExpr); ok && sel.Sel.Name == "Setenv" { + if id, ok := call.Args[0].(*ast.Ident); ok && (id.Name == "keyFDEnvVar" || id.Name == "frameFDEnvVar") { + found = true } } + return !found + }) + return found +} + +// rawFdCalls returns the positions of .Fd() calls in body that are not +// nested inside a syscall.Dup(…) or dupFD(…) call. +func rawFdCalls(body *ast.BlockStmt) []token.Pos { + var out []token.Pos + var walk func(n ast.Node, dup bool) + walk = func(n ast.Node, dup bool) { + ast.Inspect(n, func(c ast.Node) bool { + call, ok := c.(*ast.CallExpr) + if !ok { + return true + } + if isDupCall(call) && !dup { + for _, a := range call.Args { + walk(a, true) + } + return false + } + if sel, ok := call.Fun.(*ast.SelectorExpr); ok && sel.Sel.Name == "Fd" && len(call.Args) == 0 && !dup { + out = append(out, call.Pos()) + } + return true + }) + } + walk(body, false) + return out +} + +func isDupCall(call *ast.CallExpr) bool { + switch fun := call.Fun.(type) { + case *ast.Ident: + return fun.Name == "dupFD" + case *ast.SelectorExpr: + pkg, ok := fun.X.(*ast.Ident) + return ok && pkg.Name == "syscall" && fun.Sel.Name == "Dup" } + return false } diff --git a/cmd/odek/red_stale_network_grant_test.go b/cmd/odek/red_stale_network_grant_test.go new file mode 100644 index 00000000..6e28caab --- /dev/null +++ b/cmd/odek/red_stale_network_grant_test.go @@ -0,0 +1,84 @@ +package main + +import ( + "io" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/BackendStack21/odek/internal/config" +) + +// The recorded network grant always reflects the latest approval for the +// directory: an approval whose override requests no networks drops an +// earlier network grant instead of leaving it in place. +func TestRED_ProjectNetworkGrant_LaterApprovalWithoutNetworksClearsIt(t *testing.T) { + approvalEnv(t) + resetProjectNetworkGrants() + d := &mutableComposeDocker{nets: map[string]fakeNet{"shared": {id: id("a"), internal: true}}} + d.install(t) + if err := approveProjectSandboxWithTTY(netOverride("shared"), strings.NewReader("y\n"), io.Discard, true); err != nil { + t.Fatal(err) + } + wd, _ := os.Getwd() + dir, _ := filepath.Abs(wd) + projectNetworkGrantsMu.Lock() + _, ok := projectNetworkGrants[dir] + projectNetworkGrantsMu.Unlock() + if !ok { + t.Fatal("the first approval must record a network grant") + } + memOnly := config.ResolvedConfig{ProjectSandboxOverride: config.ProjectSandboxOverride{HasMemory: true, Memory: "512m"}} + if err := approveProjectSandboxWithTTY(memOnly, strings.NewReader("y\n"), io.Discard, true); err != nil { + t.Fatal(err) + } + projectNetworkGrantsMu.Lock() + _, stale := projectNetworkGrants[dir] + projectNetworkGrantsMu.Unlock() + if stale { + t.Error("an approval without network fields must drop the earlier network grant") + } +} + +// The same holds for approvals that carry no override at all: a Dockerfile.odek +// build approval, or a run with nothing to approve. +func TestRED_ProjectNetworkGrant_ApprovalWithoutOverrideClearsIt(t *testing.T) { + approvalEnv(t) + d := &mutableComposeDocker{nets: map[string]fakeNet{"shared": {id: id("a"), internal: true}}} + d.install(t) + wd, _ := os.Getwd() + dir, _ := filepath.Abs(wd) + granted := func() bool { + projectNetworkGrantsMu.Lock() + defer projectNetworkGrantsMu.Unlock() + _, ok := projectNetworkGrants[dir] + return ok + } + grant := func() { + t.Helper() + resetProjectNetworkGrants() + if err := approveProjectSandboxWithTTY(netOverride("shared"), strings.NewReader("y\n"), io.Discard, true); err != nil || !granted() { + t.Fatalf("setup: network approval must record a grant (err=%v)", err) + } + } + + grant() + if err := approveProjectSandboxWithTTY(config.ResolvedConfig{}, strings.NewReader(""), io.Discard, false); err != nil { + t.Fatal(err) + } + if granted() { + t.Error("a run with nothing to approve must drop the earlier network grant") + } + + grant() + if err := os.WriteFile(filepath.Join(dir, "Dockerfile.odek"), []byte("FROM alpine:3\n"), 0o644); err != nil { + t.Fatal(err) + } + if err := approveProjectSandboxWithTTY(config.ResolvedConfig{Sandbox: true}, strings.NewReader("y\n"), io.Discard, true); err != nil { + t.Fatal(err) + } + if granted() { + t.Error("a Dockerfile-only approval must drop the earlier network grant") + } +} diff --git a/cmd/odek/redres_2026_10_14_runtime_ops_test.go b/cmd/odek/redres_2026_10_14_runtime_ops_test.go index a0072b80..70b172eb 100644 --- a/cmd/odek/redres_2026_10_14_runtime_ops_test.go +++ b/cmd/odek/redres_2026_10_14_runtime_ops_test.go @@ -9,6 +9,7 @@ import ( "net/http/httptest" "os" "path/filepath" + "strconv" "strings" "testing" "time" @@ -36,8 +37,11 @@ func TestRED_RtOps_KeyFDStrictParse(t *testing.T) { t.Fatal(err) } w.Close() - fd := int(r.Fd()) - val := fmt.Sprintf("%d", fd) + // A dup of r: if a malformed value were ever accepted, the reader + // would close its own descriptor, never one r still owns. + dup := dupFD(t, r) + fd, _ := strconv.Atoi(dup) + val := dup if bad == "3x" || bad == "03x" { val += "x" } else if bad == "+3" { @@ -54,6 +58,9 @@ func TestRED_RtOps_KeyFDStrictParse(t *testing.T) { t.Setenv(keyFDEnvVar, val) if got := readKeyFromInheritedFD(); got != "" { t.Errorf("env %q accepted, key %q", val, got) + } else { + // Rejected before it was opened: the dup is still ours to close. + os.NewFile(uintptr(fd), "rejected-key-fd").Close() } r.Close() }