amadan

carlos / platform Public

activator: StartCmd refuses an unsafe run id before any disk work; longer backlog grace

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude 0d91bf5d8fa6362cc824f00b1799d6614821eccd parent 056fe55
2 files changed, +48 −7
  • internal/activator/cmdunit.go +21 −4
  • internal/activator/cmdunit_test.go +27 −3

CI cancelled — superseded by a newer push — run details

diff --git a/internal/activator/cmdunit.go b/internal/activator/cmdunit.go
index da39ce66b..bf84cf9e8 100644
--- a/internal/activator/cmdunit.go
+++ b/internal/activator/cmdunit.go
@@ -8,6 +8,7 @@ import (
"log"
"net"
"path/filepath"
+ "strings"
"sync"
"sync/atomic"
"time"
@@ -83,6 +84,22 @@ func (b *ExecBackend) cmdVars(rt registry.Route, fileEnv map[string]string, dbPa
return withVar(v, "CARLOS_RUN_DIR", runDir)
}
+// checkCmdRunDir refuses a run id that is not one plain path element and a
+// run directory other than <instance dir>/.carlos-run/<RunID>. The run
+// directory is deleted after the run, so it must never name anything
+// outside the instance's own .carlos-run. It does no disk work.
+func checkCmdRunDir(dataDir string, spec CmdSpec) error {
+ id := spec.RunID
+ if id == "" || id == "." || id == ".." || filepath.Base(id) != id || strings.ContainsAny(id, `/\`) {
+ return fmt.Errorf("run id %q is not a single path element", id)
+ }
+ root := filepath.Join(dataDir, CmdRunDirName)
+ if want := filepath.Join(root, id); spec.RunDir != want || filepath.Dir(spec.RunDir) != root {
+ return fmt.Errorf("run directory %q is not %q", spec.RunDir, want)
+ }
+ return nil
+}
+
func (b *ExecBackend) cmdLayout(rt registry.Route) (Layout, string) {
if b.layoutFn != nil {
return b.layoutFn(rt)
@@ -98,6 +115,9 @@ func (b *ExecBackend) StartCmd(ctx context.Context, rt registry.Route, spec CmdS
if l, why := b.cmdLayout(rt); l != LayoutNested {
return nil, fmt.Errorf("%w: %s is %s (%s)", ErrCmdNotNested, rt.Host, l, why)
}
+ if err := checkCmdRunDir(b.pathsFor(rt).dataDir, spec); err != nil {
+ return nil, fmt.Errorf("%s: %w", rt.Host, err)
+ }
if err := b.nestedGate(rt); err != nil {
return nil, err
}
@@ -108,9 +128,6 @@ func (b *ExecBackend) StartCmd(ctx context.Context, rt registry.Route, spec CmdS
if err != nil {
return nil, err
}
- if want := filepath.Join(p.dataDir, CmdRunDirName, spec.RunID); spec.RunDir != want {
- return nil, fmt.Errorf("%s: run directory %q is not %q", rt.Host, spec.RunDir, want)
- }
out, err := listenCmdStream(b.logSocketNamed(rt.Host, string(instancectl.RoleCmd)), spec.Stdout)
if err != nil {
return nil, fmt.Errorf("stdout listener for %s: %w", rt.Host, err)
@@ -235,7 +252,7 @@ func (s *cmdStream) copy(c net.Conn) {
// cmdAcceptGrace is how long Drain waits for a connection still queued in
// the listener's backlog.
-const cmdAcceptGrace = 100 * time.Millisecond
+const cmdAcceptGrace = time.Second
// Drain waits up to bound for the stream to reach EOF (its unit has
// stopped, so every writer has closed), then closes the listener. The unit
diff --git a/internal/activator/cmdunit_test.go b/internal/activator/cmdunit_test.go
index 942c2b9fe..a8a4a585f 100644
--- a/internal/activator/cmdunit_test.go
+++ b/internal/activator/cmdunit_test.go
@@ -75,6 +75,7 @@ func startTestCmd(t *testing.T) (*ExecBackend, registry.Route, *fakeControl, *fa
func TestStartCmdWritesTheLaunchFileAndStartsTheUnit(t *testing.T) {
b, rt, ctl, _, u, _, _ := startTestCmd(t)
+ t.Cleanup(func() { u.StopAndSettle(context.Background()) })
if u.Invocation() != "inv1" || !strings.Contains(ctl.joined(), "cmd "+rt.Host) {
t.Fatalf("invocation %q, calls\n%s", u.Invocation(), ctl.joined())
}
@@ -116,6 +117,27 @@ func TestStartCmdRefusesAFlatInstanceAndAForeignRunDir(t *testing.T) {
}
}
+func TestStartCmdRefusesAnUnsafeRunID(t *testing.T) {
+ for _, id := range []string{"../x", "", "a/b", ".", "..", "../../other.example.com", `a\b`} {
+ ctl := &fakeControl{cmdInv: "inv1"}
+ b, rt := nestedBackend(t, ctl, &fakeUnit{})
+ b.layoutFn = func(registry.Route) (Layout, string) { return LayoutNested, "" }
+ dir := provision.InstanceDir(b.DataDir, rt.Host)
+ // the RunDir that matches the id after cleaning: the caller is
+ // "consistent", so only the id itself can be refused
+ runDir := filepath.Join(dir, CmdRunDirName, id)
+ if _, err := b.StartCmd(context.Background(), rt, CmdSpec{RunID: id, RunDir: runDir}); err == nil {
+ t.Errorf("run id %q was accepted", id)
+ }
+ if strings.Contains(ctl.joined(), "cmd ") {
+ t.Errorf("run id %q reached the control service", id)
+ }
+ if _, err := os.Stat(dir); !os.IsNotExist(err) {
+ t.Errorf("run id %q: the instance directory was made (%v)", id, err)
+ }
+ }
+}
+
func TestStopAndSettleDrainsBothStreams(t *testing.T) {
b, rt, ctl, unit, u, out, errs := startTestCmd(t)
unit.set("active", "inv1", true, true)
@@ -168,7 +190,9 @@ func TestCmdStreamDrainTakesAConnectionStillInTheBacklog(t *testing.T) {
if err != nil {
t.Fatal(err)
}
- c.Write([]byte("last words"))
+ if _, err := c.Write([]byte("last words")); err != nil {
+ t.Fatal(err)
+ }
c.Close()
if err := s.Drain(time.Second); err != nil {
t.Fatalf("round %d: Drain: %v", i, err)
@@ -179,7 +203,7 @@ func TestCmdStreamDrainTakesAConnectionStillInTheBacklog(t *testing.T) {
}
}
-func TestCmdStreamDrainOfAnUnconnectedStreamIsQuick(t *testing.T) {
+func TestCmdStreamDrainOfAnUnconnectedStreamEndsAfterTheGrace(t *testing.T) {
dir, err := os.MkdirTemp("", "cs")
if err != nil {
t.Fatal(err)
@@ -193,7 +217,7 @@ func TestCmdStreamDrainOfAnUnconnectedStreamIsQuick(t *testing.T) {
if err := s.Drain(time.Second); err != nil {
t.Fatal(err)
}
- if d := time.Since(start); d > time.Second {
+ if d := time.Since(start); d > 3*cmdAcceptGrace {
t.Errorf("took %s", d)
}
}