From 4537b8910293142ee135860037592f83c886e1cd Mon Sep 17 00:00:00 2001 From: Tyler <53561637+im-tyler@users.noreply.github.com> Date: Sat, 19 Sep 2026 01:32:33 -0700 Subject: [PATCH 1/9] =?UTF-8?q?fix(releasemeta,deploy):=20A01+A02+A03=20?= =?UTF-8?q?=E2=80=94=20app-scoped=20TLS=20attempts,=20retention-window=20p?= =?UTF-8?q?rune,=20fail-closed=20pins?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A01: attempt TLS paths move from the flat /deployments/caddy/tls/att root to /deployments/caddy/tls/att//./ (container-side /etc/caddy/tls/att//…). PruneAttempts sweeps only the pruning app's artifact root and TLS root; the legacy flat root is never swept — a flat entry's owning app cannot be proven, and sweeping it with one app's keep set deleted OTHER apps' live certificates whenever hashes differed (two apps can even share a hash string). A02: the attempt-prune protection window widens from current+previous+pins to also cover every release that still has containers on the server — keep_versions retention holds releases beyond current+previous whose records and Caddy routes reference attempt-scoped TLS/env bytes. A version leaving the keep window loses containers at step 15b of the same deploy, so its artifacts first prunable on the next. A03: an unreadable pin file now SKIPS attempt pruning entirely, matching version pruning's fail-closed policy, instead of pruning with current+previous protection only. NewAttempt also validates the app name against the config grammar (A17 boundary piece): attempt paths interpolate the app into host and container-side directories. --- internal/cli/attempt_test.go | 4 +- internal/cli/deploy.go | 5 +- internal/deploy/attempt_prune_test.go | 100 ++++++++++++++++++++++++++ internal/deploy/deploy.go | 41 +++++++---- internal/deploy/fence_test.go | 8 ++- internal/releasemeta/attempt.go | 65 +++++++++++------ internal/releasemeta/attempt_test.go | 71 +++++++++++++++--- 7 files changed, 243 insertions(+), 51 deletions(-) create mode 100644 internal/deploy/attempt_prune_test.go diff --git a/internal/cli/attempt_test.go b/internal/cli/attempt_test.go index 18a06ac..b4bbc4d 100644 --- a/internal/cli/attempt_test.go +++ b/internal/cli/attempt_test.go @@ -35,10 +35,10 @@ func TestUploadAppTLS_AttemptScoped(t *testing.T) { if err != nil { t.Fatalf("uploadAppTLS: %v", err) } - if want := "/etc/caddy/tls/att/" + att.Name() + "/myapp.crt"; cert != want { + if want := "/etc/caddy/tls/att/myapp/" + att.Name() + "/myapp.crt"; cert != want { t.Errorf("cert container path: got %s want %s", cert, want) } - if want := "/etc/caddy/tls/att/" + att.Name() + "/myapp.key"; key != want { + if want := "/etc/caddy/tls/att/myapp/" + att.Name() + "/myapp.key"; key != want { t.Errorf("key container path: got %s want %s", key, want) } hostCert := att.TLSDir() + "/myapp.crt" diff --git a/internal/cli/deploy.go b/internal/cli/deploy.go index e287c9d..7e55c04 100644 --- a/internal/cli/deploy.go +++ b/internal/cli/deploy.go @@ -1169,8 +1169,9 @@ func appTLSContainerPaths(app string) (cert, key string) { // uploadAppTLS reads the local cert + key referenced by the app's tls config // and uploads them to the server's attempt-scoped TLS directory (F08: -// /deployments/caddy/tls/att/./, key mode 0600), where the -// directory-mounted Caddy container reads them at /etc/caddy/tls/att/…. +// /deployments/caddy/tls/att//./, key mode 0600), where the +// directory-mounted Caddy container reads them at +// /etc/caddy/tls/att//…. // Attempt-scoping keeps the cert/key immutable for the release that // references it: the F14 record names these exact bytes, and a concurrent // or later attempt cannot overwrite them. It returns the container-side diff --git a/internal/deploy/attempt_prune_test.go b/internal/deploy/attempt_prune_test.go new file mode 100644 index 0000000..fb90244 --- /dev/null +++ b/internal/deploy/attempt_prune_test.go @@ -0,0 +1,100 @@ +package deploy + +import ( + "bytes" + "context" + "errors" + "strings" + "testing" + "time" + + "github.com/useteploy/teploy/internal/ssh" + "github.com/useteploy/teploy/internal/state" +) + +var errBoom = errors.New("boom") + +// deployWithAttemptPruneMocks runs a happy-path deploy of version "newhash" +// with the pin read and retained-version inventory stubbed as specified, +// returning the mock (Calls holds every issued command). +func deployWithAttemptPruneMocks(t *testing.T, pinsStub, inventoryStub ssh.MockCommand) *ssh.MockExecutor { + t.Helper() + app := "fency" + mocks := fenceHappyPathMocks(app) + mocks = append(mocks, + pinsStub, + inventoryStub, + ssh.MockCommand{Match: "ls -1 /deployments/fency/meta/att", Output: "ancient.0000000000000003"}, + ssh.MockCommand{Match: "ls -1 /deployments/caddy/tls/att/fency", Output: "ancient.0000000000000003"}, + ssh.MockCommand{Match: "rm -rf ", Output: ""}, + ) + mock := ssh.NewMockExecutor("1.2.3.4", mocks...) + lk, err := state.AcquireLockFenced(context.Background(), mock, app) + if err != nil { + t.Fatalf("AcquireLockFenced: %v", err) + } + d := NewDeployer(mock, &bytes.Buffer{}) + if err := d.DeployFenced(context.Background(), Config{ + App: app, + Domain: "fency.com", + Image: "fency:latest", + Version: "newhash", + Health: HealthConfig{Timeout: 5 * time.Second, Interval: 10 * time.Millisecond}, + }, lk); err != nil { + t.Fatalf("DeployFenced: %v", err) + } + return mock +} + +// TestDeployFenced_PinReadFailureSkipsAttemptPrune is the A03 regression: +// an unreadable pin file must fail the attempt-artifact prune closed (like +// version pruning), never prune as though no pins existed. +func TestDeployFenced_PinReadFailureSkipsAttemptPrune(t *testing.T) { + mock := deployWithAttemptPruneMocks(t, + // The framing command itself fails (transport/permission), which + // ReadRemoteFile surfaces as an error. + ssh.MockCommand{Match: "if [ ! -e '/deployments/fency/pinned' ]", Err: errBoom}, + ssh.MockCommand{Match: "docker ps --all --filter label=teploy.app='fency'", Output: ""}, + ) + for _, c := range mock.Calls { + if strings.HasPrefix(c, "rm -rf ") && strings.Contains(c, "/meta/att") { + t.Errorf("attempt artifacts must not be pruned when pins cannot be read: %s", c) + } + if strings.HasPrefix(c, "ls -1 /deployments/fency/meta/att") { + t.Errorf("the prune sweep must not even run when pins cannot be read: %s", c) + } + } +} + +// TestDeployFenced_RetainedVersionsProtectTheirAttempts is the A02 +// regression: a release that still has containers on the server (e.g. held +// by keep_versions retention or a pin) keeps its attempt artifacts — its +// record and Caddy route reference the attempt-scoped TLS/env files. +func TestDeployFenced_RetainedVersionsProtectTheirAttempts(t *testing.T) { + mock := deployWithAttemptPruneMocks(t, + ssh.MockCommand{Match: "if [ ! -e '/deployments/fency/pinned' ]", Output: "absent"}, + // The inventory reports a live container of release "ancient": a + // retained rollback target outside current+previous. + ssh.MockCommand{Match: "docker ps --all --filter label=teploy.app='fency'", Output: `{"ID":"deadbeef","Names":"fency-web-ancient","Image":"fency:latest","State":"running","Status":"up","CreatedAt":"2026-05-28 21:33:29 -0700 PDT","Labels":"teploy.app=fency,teploy.process=web,teploy.version=ancient"}`}, + ) + for _, c := range mock.Calls { + if strings.HasPrefix(c, "rm -rf ") && strings.Contains(c, "ancient.") { + t.Errorf("a release with live containers must keep its attempt artifacts: %s", c) + } + } +} + +// TestDeployFenced_InventoryFailureSkipsAttemptPrune: when the retained- +// version inventory cannot be listed, the protection window cannot be +// computed, so the prune is skipped (fail closed, A02). +func TestDeployFenced_InventoryFailureSkipsAttemptPrune(t *testing.T) { + mock := deployWithAttemptPruneMocks(t, + ssh.MockCommand{Match: "if [ ! -e '/deployments/fency/pinned' ]", Output: "absent"}, + ssh.MockCommand{Match: "docker ps --all --filter label=teploy.app='fency'", Err: errBoom}, + ) + for _, c := range mock.Calls { + if strings.HasPrefix(c, "ls -1 /deployments/fency/meta/att") { + t.Errorf("the prune sweep must not run when the version inventory is unreadable: %s", c) + } + } +} diff --git a/internal/deploy/deploy.go b/internal/deploy/deploy.go index aed8367..d9ba5d8 100644 --- a/internal/deploy/deploy.go +++ b/internal/deploy/deploy.go @@ -642,24 +642,41 @@ func (d *Deployer) DeployFenced(ctx context.Context, cfg Config, lk *state.Lock) // contexts, env files, TLS certs) are dead weight once their release // is outside the rollback window — env is baked into containers at // create and recreate uses the inspect-derived resolved env, never - // the file. Same protection window as version pruning: current, - // previous, and pinned releases keep their artifacts. + // the file. The protection window is every release that still has + // CONTAINERS on this server, plus current, previous, and pinned + // (audit A02): keep_versions retention can hold releases far beyond + // current+previous, and their records and Caddy routes reference + // attempt-scoped TLS and env files — pruning those while the release + // is retained silently invalidates its rollback target. A version + // that falls out of the keep window loses its containers at step 15b + // of THIS deploy, so its attempt dirs are first prunable on the next. { var prevHash string if current != nil { prevHash = current.CurrentHash } - protected := []string{cfg.Version, prevHash} - // Pins protect their release's artifacts like versions (F78 - // parity): an unreadable pin file skips the extra protection, and - // that is reported, never silent. - if pins, pinsErr := state.ReadPins(ctx, d.exec, cfg.App); pinsErr == nil { - protected = append(protected, pins...) + pins, pinsErr := state.ReadPins(ctx, d.exec, cfg.App) + if pinsErr != nil { + // Fail closed exactly like version pruning (15b): an + // unreadable pin file means retention obligations cannot be + // established, and pruning anyway can delete a pinned + // release's artifacts precisely when the operator cannot see + // the pin (audit A03). + fmt.Fprintf(d.out, "Warning: attempt-artifact prune skipped — pin state could not be read: %v\n", pinsErr) } else { - fmt.Fprintf(d.out, "Warning: attempt artifacts protected only as current+previous — pin state could not be read: %v\n", pinsErr) - } - if err := releasemeta.PruneAttempts(ctx, d.exec, cfg.App, protected...); err != nil { - fmt.Fprintf(d.out, "Warning: could not prune superseded attempt artifacts: %v\n", err) + protected := append([]string{cfg.Version, prevHash}, pins...) + if inv, invErr := d.docker.ListContainers(ctx, cfg.App); invErr != nil { + fmt.Fprintf(d.out, "Warning: attempt-artifact prune skipped — cannot inventory retained versions: %v\n", invErr) + } else { + for _, ct := range inv { + if v := ct.Labels["teploy.version"]; v != "" { + protected = append(protected, v) + } + } + if err := releasemeta.PruneAttempts(ctx, d.exec, cfg.App, protected...); err != nil { + fmt.Fprintf(d.out, "Warning: could not prune superseded attempt artifacts: %v\n", err) + } + } } } diff --git a/internal/deploy/fence_test.go b/internal/deploy/fence_test.go index d0b59a0..d85fcae 100644 --- a/internal/deploy/fence_test.go +++ b/internal/deploy/fence_test.go @@ -116,11 +116,13 @@ func TestDeployFenced_PrunesSupersededAttempts(t *testing.T) { app := "fency" mocks := fenceHappyPathMocks(app) // Attempt prune: the artifact roots list an ancient attempt plus an - // unparsable stray; both roots' listings and the pin read succeed. + // unparsable stray; the pin read (ReadRemoteFile framing) and the + // retained-version inventory both succeed and report nothing extra. mocks = append(mocks, - ssh.MockCommand{Match: "cat /deployments/fency/pins", Output: ""}, + ssh.MockCommand{Match: "if [ ! -e '/deployments/fency/pinned' ]", Output: "absent"}, + ssh.MockCommand{Match: "docker ps --all --filter label=teploy.app='fency'", Output: ""}, ssh.MockCommand{Match: "ls -1 /deployments/fency/meta/att", Output: "ancient.0000000000000003\nstray"}, - ssh.MockCommand{Match: "ls -1 /deployments/caddy/tls/att", Output: "ancient.0000000000000003"}, + ssh.MockCommand{Match: "ls -1 /deployments/caddy/tls/att/fency", Output: "ancient.0000000000000003"}, ssh.MockCommand{Match: "rm -rf ", Output: ""}, ) mock := ssh.NewMockExecutor("1.2.3.4", mocks...) diff --git a/internal/releasemeta/attempt.go b/internal/releasemeta/attempt.go index 4d7a0bd..a631fdb 100644 --- a/internal/releasemeta/attempt.go +++ b/internal/releasemeta/attempt.go @@ -13,7 +13,7 @@ // // - build context: /deployments//meta/att/./build // - env file: /deployments//meta/att/./env -// - TLS cert/key: /deployments/caddy/tls/att/./.{crt,key} +// - TLS cert/key: /deployments/caddy/tls/att//./.{crt,key} // // Each attempt gets a fresh random id, so its paths are written exactly once // and never rewritten by anyone — concurrent attempts cannot interleave @@ -24,19 +24,28 @@ // TLS deliberately stays under /deployments/caddy/tls rather than moving // into meta/: the caddy container mounts that directory at /etc/caddy (the // only mount a server that can do custom TLS provably has), so attempt -// scoping there changes no mount topology. Container-side path: -// /etc/caddy/tls/att/./.crt. +// scoping there changes no mount topology. The TLS namespace is scoped BY +// APP beneath that mount (audit A01): the first cut swept the flat +// /deployments/caddy/tls/att root with one app's keep set, so deploying app +// A deleted app B's live cert/key whenever B's release hash was not in A's +// protected set — two apps can even share a hash string ("release-1"). +// Container-side path: /etc/caddy/tls/att//./.crt. +// Legacy flat attempt dirs (written between F08 and A01) are never swept by +// anyone — a flat entry's owning app cannot be proven, so pruning it from +// any single app's keep set is exactly the cross-app deletion A01 fixed. // // Retention: attempt dirs are dead weight once their release is outside the // rollback window (env is baked into the container at create; recreate uses // the inspect-derived resolved env, never the file). PruneAttempts removes // attempt dirs whose release hash is not protected — the same window -// keep_versions pruning honors — and fails closed on entries it cannot -// parse, like pin pruning (F78). +// keep_versions pruning honors, which the caller must compute from every +// still-retained release, not just current+previous (audit A02) — and fails +// closed on entries it cannot parse, like pin pruning (F78). package releasemeta import ( + "context" "crypto/rand" "encoding/hex" "fmt" @@ -44,8 +53,7 @@ import ( "sort" "strings" - "context" - + "github.com/useteploy/teploy/internal/config" "github.com/useteploy/teploy/internal/ssh" ) @@ -65,10 +73,14 @@ type Attempt struct { ID string } -// NewAttempt mints an attempt for (app, hash). +// NewAttempt mints an attempt for (app, hash). The app name is validated +// against the config grammar (audit A17): attempt paths interpolate the app +// into host and container-side directories, so an app with path +// metacharacters must be rejected at construction, not discovered when a +// remote shell misparses it. func NewAttempt(app, hash string) (Attempt, error) { - if app == "" { - return Attempt{}, fmt.Errorf("attempt requires an app") + if err := config.ValidateName(app); err != nil { + return Attempt{}, fmt.Errorf("attempt requires a valid app: %w", err) } if !validHash.MatchString(hash) { return Attempt{}, fmt.Errorf("invalid release id %q for app %q", hash, app) @@ -106,19 +118,25 @@ func (a Attempt) EnvFile() string { return a.Dir() + "/env" } // TLSDir is the attempt's TLS directory on the HOST. It sits under // /deployments/caddy/tls (mounted at /etc/caddy in the caddy container) — -// see the package doc for why not under meta/. +// see the package doc for why not under meta/ — and is scoped BY APP so one +// app's pruning can never sweep another app's certificates (audit A01). func (a Attempt) TLSDir() string { - return fmt.Sprintf("/deployments/caddy/tls/att/%s", a.Name()) + return tlsAttemptRootFor(a.App) + "/" + a.Name() } // TLSCertPath / TLSKeyPath are the attempt's cert/key as seen INSIDE the // caddy container (what the Caddyfile site block references): the host's // /deployments/caddy/tls/att/... is mounted at /etc/caddy. -func (a Attempt) TLSCertPath() string { return "/etc/caddy/tls/att/" + a.Name() + "/" + a.App + ".crt" } -func (a Attempt) TLSKeyPath() string { return "/etc/caddy/tls/att/" + a.Name() + "/" + a.App + ".key" } - -// tlsAttemptRoot is the host root holding per-attempt TLS directories. -const tlsAttemptRoot = "/deployments/caddy/tls/att" +func (a Attempt) TLSCertPath() string { return "/etc/caddy/tls/att/" + a.App + "/" + a.Name() + "/" + a.App + ".crt" } +func (a Attempt) TLSKeyPath() string { return "/etc/caddy/tls/att/" + a.App + "/" + a.Name() + "/" + a.App + ".key" } + +// tlsAttemptRootFor is the host root holding ONE app's per-attempt TLS +// directories (audit A01). The legacy flat root +// /deployments/caddy/tls/att (written before A01) is deliberately NOT this +// and is never swept — see the package doc. +func tlsAttemptRootFor(app string) string { + return "/deployments/caddy/tls/att/" + app +} // attemptRoot is the host root holding per-attempt artifact directories. func attemptRoot(app string) string { @@ -141,11 +159,12 @@ func listAttempts(ctx context.Context, exec ssh.Executor, root string) ([]string return names, nil } -// PruneAttempts removes the attempt directories (artifact root and TLS -// root) of every release hash NOT in keepHashes. Entries whose names do not -// parse as . are kept — an unparsable name is not proof the -// attempt is prunable (F78's rule). Removal failures are returned; callers -// treat pruning as best-effort. +// PruneAttempts removes the attempt directories (artifact root and the +// app's TLS root) of every release hash NOT in keepHashes. Entries whose +// names do not parse as . are kept — an unparsable name is not +// proof the attempt is prunable (F78's rule). Only THIS app's roots are +// swept; the legacy flat TLS root is never touched (A01). Removal failures +// are returned; callers treat pruning as best-effort. func PruneAttempts(ctx context.Context, exec ssh.Executor, app string, keepHashes ...string) error { keep := make(map[string]bool, len(keepHashes)) for _, h := range keepHashes { @@ -154,7 +173,7 @@ func PruneAttempts(ctx context.Context, exec ssh.Executor, app string, keepHashe } } var failures []string - for _, root := range []string{attemptRoot(app), tlsAttemptRoot} { + for _, root := range []string{attemptRoot(app), tlsAttemptRootFor(app)} { names, err := listAttempts(ctx, exec, root) if err != nil { return err diff --git a/internal/releasemeta/attempt_test.go b/internal/releasemeta/attempt_test.go index c952b04..a2fa943 100644 --- a/internal/releasemeta/attempt_test.go +++ b/internal/releasemeta/attempt_test.go @@ -36,19 +36,30 @@ func TestAttemptPaths_AreAttemptScoped(t *testing.T) { if !strings.HasPrefix(a.EnvFile(), "/deployments/myapp/meta/att/abc123.") { t.Errorf("EnvFile not in the attempt namespace: %s", a.EnvFile()) } - if !strings.HasPrefix(a.TLSDir(), "/deployments/caddy/tls/att/abc123.") { - t.Errorf("TLSDir not under the caddy tls att namespace: %s", a.TLSDir()) + // A second attempt of the same release shares NO path with the first. + b := MustAttempt("myapp", "abc123") + if a.BuildDir() == b.BuildDir() || a.EnvFile() == b.EnvFile() || a.TLSDir() == b.TLSDir() { + t.Error("attempts of the same release must not share artifact paths") } - if want := "/etc/caddy/tls/att/" + a.Name() + "/myapp.crt"; a.TLSCertPath() != want { + // The TLS namespace is app-scoped (A01): the host dir sits under the + // app's own root beneath the caddy mount. + if !strings.HasPrefix(a.TLSDir(), "/deployments/caddy/tls/att/myapp/abc123.") { + t.Errorf("TLSDir not in the app-scoped caddy tls att namespace: %s", a.TLSDir()) + } + if want := "/etc/caddy/tls/att/myapp/" + a.Name() + "/myapp.crt"; a.TLSCertPath() != want { t.Errorf("TLSCertPath: got %s want %s", a.TLSCertPath(), want) } - if want := "/etc/caddy/tls/att/" + a.Name() + "/myapp.key"; a.TLSKeyPath() != want { + if want := "/etc/caddy/tls/att/myapp/" + a.Name() + "/myapp.key"; a.TLSKeyPath() != want { t.Errorf("TLSKeyPath: got %s want %s", a.TLSKeyPath(), want) } - // A second attempt of the same release shares NO path with the first. - b := MustAttempt("myapp", "abc123") - if a.BuildDir() == b.BuildDir() || a.EnvFile() == b.EnvFile() || a.TLSDir() == b.TLSDir() { - t.Error("attempts of the same release must not share artifact paths") +} + +func TestNewAttempt_RejectsInvalidAppName(t *testing.T) { + if _, err := NewAttempt("../escape", "abc123"); err == nil { + t.Error("an app name with path metacharacters must be rejected") + } + if _, err := NewAttempt("", "abc123"); err == nil { + t.Error("an empty app name must be rejected") } } @@ -63,7 +74,8 @@ func TestPruneAttempts_ProtectsKeepSetAndUnparsable(t *testing.T) { "ancient.0000000000000003", "stray-directory", }, "\n")}, - ssh.MockCommand{Match: "ls -1 /deployments/caddy/tls/att", Output: strings.Join([]string{ + // The app's OWN TLS root (A01: /deployments/caddy/tls/att/). + ssh.MockCommand{Match: "ls -1 /deployments/caddy/tls/att/myapp", Output: strings.Join([]string{ "ancient.0000000000000003", }, "\n")}, ssh.MockCommand{Match: "rm -rf", Output: ""}, @@ -87,6 +99,47 @@ func TestPruneAttempts_ProtectsKeepSetAndUnparsable(t *testing.T) { } } +// TestPruneAttempts_NeverTouchesOtherAppsOrLegacyFlatRoot is the A01 +// regression: pruning app A must not remove app B's TLS attempts (even when +// B's release hash collides with an unprotected hash of A's) and must never +// sweep the legacy flat /deployments/caddy/tls/att root, whose entries +// cannot be attributed to an owning app. +func TestPruneAttempts_NeverTouchesOtherAppsOrLegacyFlatRoot(t *testing.T) { + mock := ssh.NewMockExecutor("1.2.3.4", + ssh.MockCommand{Match: "ls -1 /deployments/app-a/meta/att", Output: strings.Join([]string{ + "shared-name.0000000000000001", // A's prunable attempt... + }, "\n")}, + ssh.MockCommand{Match: "ls -1 /deployments/caddy/tls/att/app-a", Output: strings.Join([]string{ + "shared-name.0000000000000001", // ...in both of A's roots + }, "\n")}, + ssh.MockCommand{Match: "rm -rf", Output: ""}, + ) + if err := PruneAttempts(context.Background(), mock, "app-a", "currenthash"); err != nil { + t.Fatalf("PruneAttempts: %v", err) + } + for _, c := range mock.Calls { + if !strings.HasPrefix(c, "rm -rf ") { + continue + } + if strings.Contains(c, "/deployments/caddy/tls/att/app-b") { + t.Errorf("pruned another app's TLS material: %s", c) + } + if !strings.Contains(c, "app-a") { + t.Errorf("pruned outside the pruning app's namespaces: %s", c) + } + } + // The sweep must be scoped to app-a's TLS root — a listing of the flat + // root or app-b's root proves the sweep reached beyond A's namespace. + for _, c := range mock.Calls { + if strings.HasPrefix(c, "ls -1 ") { + if c != "ls -1 /deployments/app-a/meta/att 2>/dev/null || true" && + c != "ls -1 /deployments/caddy/tls/att/app-a 2>/dev/null || true" { + t.Errorf("attempt sweep listed a namespace it must not touch: %s", c) + } + } + } +} + func TestPruneAttempts_AbsentRootsAreNoops(t *testing.T) { mock := ssh.NewMockExecutor("1.2.3.4", ssh.MockCommand{Match: "ls -1", Output: ""}, From dd8a7889b5841cacc2d14d41f0f45a96e7886e5a Mon Sep 17 00:00:00 2001 From: Tyler <53561637+im-tyler@users.noreply.github.com> Date: Sat, 19 Sep 2026 01:35:06 -0700 Subject: [PATCH 2/9] =?UTF-8?q?fix(state):=20A04+A06=20=E2=80=94=20owner-c?= =?UTF-8?q?hecked=20lock=20release,=20unique=20owner-scoped=20staging?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A04: ReleaseLockFenced runs the release under the holdership guard when a fence handle is present — grep owner info, then rm -rf, in one shell invocation. After a takeover the stale holder's deferred release is refused (ErrFenceLost treated as success: the lock belongs to someone else and leaving it alone IS the correct outcome) instead of deleting the successor's lock and admitting a third operation. A nil handle keeps the historical unfenced release (admin unlock, pre-F16 paths). A handle for a different app is refused outright (A17 lease-correspondence rule, also enforced in WriteFenced). A06: WriteFenced and the lock renewal stage to unique owner-scoped siblings (state.json.tmp--) instead of the fixed state.json.tmp-fence / .lock/info.renew names shared by every generation, so a stale holder can no longer upload into the successor's staging path and ride its guarded rename into authority. Renewal staging also moves into the app directory: a stale renewal whose .lock was already removed can no longer recreate the directory (Upload's mkdir -p) and ghost-lock the app for a full staleLockTTL. MockExecutor now models rm -rf -- as a recursive removal of recorded files, so the guarded release is provable against recorded state. --- internal/ssh/mock.go | 13 ++++- internal/state/lock.go | 94 +++++++++++++++++++++++++++---- internal/state/lock_test.go | 107 ++++++++++++++++++++++++++++++++++++ 3 files changed, 201 insertions(+), 13 deletions(-) diff --git a/internal/ssh/mock.go b/internal/ssh/mock.go index 9bd5f11..b24cf55 100644 --- a/internal/ssh/mock.go +++ b/internal/ssh/mock.go @@ -76,7 +76,7 @@ func (m *MockExecutor) Run(ctx context.Context, cmd string) (string, error) { return c.Output, c.Err } } - if strings.HasPrefix(cmd, "mv -f -- ") || strings.HasPrefix(cmd, "rm -f -- ") { + if strings.HasPrefix(cmd, "mv -f -- ") || strings.HasPrefix(cmd, "rm -f -- ") || strings.HasPrefix(cmd, "rm -rf -- ") { m.applyFileCommand(cmd) m.mu.Unlock() return "", nil @@ -155,6 +155,17 @@ func (m *MockExecutor) applyFileCommand(cmd string) { if len(fields) == 4 && fields[0] == "rm" && fields[1] == "-f" && fields[2] == "--" { delete(m.Files, fields[3]) } + // rm -rf -- : a recursive removal deletes the directory AND every + // recorded file beneath it — the guarded lock release (state package, + // audit A04) removes /deployments//.lock and its info together. + if len(fields) == 4 && fields[0] == "rm" && fields[1] == "-rf" && fields[2] == "--" { + prefix := strings.TrimSuffix(fields[3], "/") + "/" + for p := range m.Files { + if p == fields[3] || strings.HasPrefix(p, prefix) { + delete(m.Files, p) + } + } + } } func (m *MockExecutor) RunStream(ctx context.Context, cmd string, stdout, stderr io.Writer) error { diff --git a/internal/state/lock.go b/internal/state/lock.go index ff1dcfa..862de0c 100644 --- a/internal/state/lock.go +++ b/internal/state/lock.go @@ -33,9 +33,12 @@ package state import ( "context" + "crypto/rand" + "encoding/hex" "encoding/json" "errors" "fmt" + "os" "strings" "sync" "time" @@ -235,11 +238,29 @@ func (l *Lock) renewLoop(stop, done chan struct{}) { } } +// ownerTempName returns a staging sibling unique to this write: path + +// ".tmp-" + owner + "-" + random. F16's first cut staged to FIXED names +// (state.json.tmp-fence, .lock/info.renew) shared by every generation, so a +// stale holder could upload into the successor's staging path and have the +// successor's own guard pass those stale bytes into authority (audit A06). +// Staging under a random owner-scoped name makes cross-generation clobber +// impossible. +func ownerTempName(path, owner string) (string, error) { + var nonce [8]byte + if _, err := rand.Read(nonce[:]); err != nil { + return "", fmt.Errorf("generating staging name: %w", err) + } + return fmt.Sprintf("%s.tmp-%s-%s", path, owner, hex.EncodeToString(nonce[:])), nil +} + // renew refreshes renew_ts under the holdership guard — a renewal that no // longer holds must never clobber the new holder's info file. The payload -// is staged to a fixed sibling (writing it is not an effect; the lock dir -// belongs to the holder) and the rename that makes it live is the guarded -// effect, so guard and write cannot interleave. +// is staged to a unique sibling in the APP directory (not inside .lock: a +// stale renewal whose lock directory was already removed must never +// RECREATE it, which Upload's mkdir -p would do — leaving a ghost .lock +// that blocks the next acquire for a full staleLockTTL), and the rename +// that makes it live is the guarded effect, so guard and write cannot +// interleave. func (l *Lock) renew(ctx context.Context) error { l.mu.Lock() exec := l.renewer @@ -258,38 +279,87 @@ func (l *Lock) renew(ctx context.Context) error { return err } path := lockInfoPath(l.app) - tmp := path + ".renew" + tmp, err := ownerTempName(fmt.Sprintf("%s/%s/.lock-info", deploymentsDir, l.app), l.owner) + if err != nil { + return err + } if err := exec.Upload(ctx, strings.NewReader(string(payload)), tmp, "0644"); err != nil { return fmt.Errorf("renewing lock info: %w", err) } + // No mkdir here: the guard just proved .lock/info exists, so .lock + // exists; recreating it unconditionally would resurrect a released + // lock's directory. A lock removed in the grep→mv window makes mv + // fail — a transient renewal error that is retried on the next tick. _, err = l.Guarded(ctx, exec, "mv -f -- "+ssh.ShellQuote(tmp)+" "+ssh.ShellQuote(path)) - return err + if err != nil { + // The staged file is inert outside .lock; a fence-lost renewal + // must not leave litter behind it (best-effort, bounded). + if fenceLostErr(err) { + cleanupCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + exec.Run(cleanupCtx, "rm -f -- "+ssh.ShellQuote(tmp)) + cancel() + } + return err + } + return nil } -// ReleaseLockFenced stops renewal and releases the lock. The release itself -// uses the detached bounded context (see ReleaseLockDetached) so a cancelled -// deploy context cannot skip the unlock and strand the app. +// ReleaseLockFenced stops renewal and releases the lock — but only when the +// server still names THIS operation as the holder. The release effect runs +// under the holdership guard (audit A04): after a takeover or a manual +// unlock/reacquire, a stale deploy's deferred release used to execute an +// unconditional rm -rf on the lock directory, deleting the SUCCESSOR's lock +// and letting a third operation in. A refused release (ErrFenceLost) is a +// success here — the lock belongs to someone else, and leaving it alone is +// exactly the correct outcome. A nil handle keeps the historical unfenced +// release for callers that never held a fence (admin unlock, pre-F16 +// paths). func ReleaseLockFenced(exec ssh.Executor, lk *Lock, app string) { if lk != nil { lk.StopRenewal() + if lk.App() != "" && lk.App() != app { + // A handle for a different app has no authority over this + // app's lock; releasing it would be A04's defect with extra + // steps. Refuse and say so. + fmt.Fprintf(os.Stderr, "teploy: refusing to release %s's lock with a lease held for %s\n", app, lk.App()) + return + } + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + _, err := lk.Guarded(ctx, exec, "rm -rf -- "+ssh.ShellQuote(fmt.Sprintf("%s/%s/.lock", deploymentsDir, app))) + if err != nil && !fenceLostErr(err) { + // Transport-level failure: fall back to the detached + // unconditional release rather than stranding the app — + // same trade-off as the pre-A04 behavior, but only when the + // owner check itself could not be evaluated. + ReleaseLockDetached(exec, app) + } + return } ReleaseLockDetached(exec, app) } // WriteFenced is Write with the state commit under the fence: the content is -// staged to a sibling temp (no effect), and the atomic rename — the instant -// the new state becomes authoritative — runs as a guarded effect. A holder -// that lost the lock commits nothing. +// staged to a unique sibling (no effect — a stale holder cannot clobber the +// successor's staging, A06), and the atomic rename — the instant the new +// state becomes authoritative — runs as a guarded effect. A holder that +// lost the lock commits nothing. func WriteFenced(ctx context.Context, exec ssh.Executor, app string, s *AppState, lk *Lock) error { if lk == nil { return Write(ctx, exec, app, s) } + if lk.App() != app { + return fmt.Errorf("refusing to commit state for %s under a lease held for %s", app, lk.App()) + } data, err := prepareState(s) if err != nil { return err } path := fmt.Sprintf("%s/%s/state.json", deploymentsDir, app) - tmpPath := path + ".tmp-fence" + tmpPath, err := ownerTempName(path, lk.Owner()) + if err != nil { + return err + } if err := exec.Upload(ctx, strings.NewReader(string(data)), tmpPath, "0644"); err != nil { return fmt.Errorf("uploading temporary state file: %w", err) } diff --git a/internal/state/lock_test.go b/internal/state/lock_test.go index 17fc04e..8bf5808 100644 --- a/internal/state/lock_test.go +++ b/internal/state/lock_test.go @@ -217,3 +217,110 @@ func TestNilLockIsUnfencedPassthrough(t *testing.T) { lk.StartRenewal(mock) lk.StopRenewal() } + +// TestReleaseLockFenced_RefusesToReleaseSuccessorsLock is the A04 core +// regression: after a takeover (the lock was broken and re-acquired by +// another operation), the STALE holder's deferred release must leave the +// successor's lock alone. The old unconditional rm -rf deleted it, letting +// a third operation in concurrently with the successor. +func TestReleaseLockFenced_RefusesToReleaseSuccessorsLock(t *testing.T) { + lk, mock := takeFencedLock(t, "myapp") + // Takeover: the server now names a different owner. + mock.Files["/deployments/myapp/.lock/info"] = []byte(`{"type":"auto","owner":"successor"}`) + ReleaseLockFenced(mock, lk, "myapp") + if _, ok := mock.Files["/deployments/myapp/.lock/info"]; !ok { + t.Fatal("stale holder's release deleted the successor's lock info") + } +} + +// TestReleaseLockFenced_OwnerCheckRemovedOwnLock: with holdership intact, +// the guarded release does remove the lock. +func TestReleaseLockFenced_OwnerCheckRemovedOwnLock(t *testing.T) { + lk, mock := takeFencedLock(t, "myapp") + ReleaseLockFenced(mock, lk, "myapp") + if _, ok := mock.Files["/deployments/myapp/.lock/info"]; ok { + t.Fatal("expected the holder's own release to remove the lock info") + } +} + +// TestReleaseLockFenced_WrongAppHandleRefused: a lease held for one app +// must not release another app's lock (the A17 lease-correspondence rule). +func TestReleaseLockFenced_WrongAppHandleRefused(t *testing.T) { + lk, mock := takeFencedLock(t, "myapp") + ReleaseLockFenced(mock, lk, "otherapp") + if _, ok := mock.Files["/deployments/myapp/.lock/info"]; !ok { + t.Fatal("a lease for myapp must not remove otherapp's (or any unrelated) lock state") + } + for _, c := range mock.Calls { + if strings.Contains(c, "otherapp") && strings.HasPrefix(strings.TrimPrefix(c, "grep -q "), "rm") { + t.Errorf("released another app's lock: %s", c) + } + } +} + +// TestWriteFenced_UniqueStagingPerWrite is the A06 regression: two fenced +// writers staging concurrently must not share a staging path — the fixed +// state.json.tmp-fence name let a stale holder's bytes ride the successor's +// guarded rename into authority. +func TestWriteFenced_UniqueStagingPerWrite(t *testing.T) { + lk, mock := takeFencedLock(t, "myapp") + s := &AppState{SchemaVersion: SchemaVersionV2, CurrentHash: "h1"} + if err := WriteFenced(context.Background(), mock, "myapp", s, lk); err != nil { + t.Fatalf("WriteFenced: %v", err) + } + var uploads []string + for _, c := range mock.Calls { + if strings.HasPrefix(c, "UPLOAD:/deployments/myapp/state.json.tmp-") { + uploads = append(uploads, c) + } + } + if len(uploads) != 1 { + t.Fatalf("expected one staging upload, got %v", uploads) + } + if strings.Contains(uploads[0], "state.json.tmp-fence") { + t.Errorf("staging used the shared fixed name: %s", uploads[0]) + } + if !strings.Contains(uploads[0], lk.Owner()) { + t.Errorf("staging name is not owner-scoped: %s", uploads[0]) + } +} + +// TestWriteFenced_WrongAppLeaseRefused: the state commit must verify the +// lease belongs to the app whose state it renames (A17). +func TestWriteFenced_WrongAppLeaseRefused(t *testing.T) { + lk, mock := takeFencedLock(t, "myapp") + s := &AppState{SchemaVersion: SchemaVersionV2, CurrentHash: "h1"} + if err := WriteFenced(context.Background(), mock, "otherapp", s, lk); err == nil { + t.Fatal("expected a refusal to commit otherapp's state under myapp's lease") + } + if _, ok := mock.Files["/deployments/otherapp/state.json"]; ok { + t.Error("refused write must not have committed anything") + } +} + +// TestRenew_StagesOutsideLockDirAndUniqueNames: renewal staging must live +// in the app directory (a stale renewal must never recreate a removed +// .lock directory) under a unique name (A06). +func TestRenew_StagesOutsideLockDirAndUniqueNames(t *testing.T) { + lk, mock := takeFencedLock(t, "myapp") + lk.StartRenewal(mock) + defer lk.StopRenewal() + if err := lk.renew(context.Background()); err != nil { + t.Fatalf("renew: %v", err) + } + if err := lk.renew(context.Background()); err != nil { + t.Fatalf("renew: %v", err) + } + var staged []string + for _, c := range mock.Calls { + if strings.HasPrefix(c, "UPLOAD:/deployments/myapp/.lock-info.tmp-") { + staged = append(staged, c) + } + if strings.HasPrefix(c, "UPLOAD:/deployments/myapp/.lock/info.renew") { + t.Errorf("renewal staged inside the lock dir under the old fixed name: %s", c) + } + } + if len(staged) != 2 || staged[0] == staged[1] { + t.Fatalf("expected two distinct owner-scoped staging names, got %v", staged) + } +} From 89625d3785919c8963d49d64d7fad4a778715eca Mon Sep 17 00:00:00 2001 From: Tyler <53561637+im-tyler@users.noreply.github.com> Date: Sat, 19 Sep 2026 01:40:56 -0700 Subject: [PATCH 3/9] =?UTF-8?q?fix(deploy):=20A08+A10+A11+A13+A26=20?= =?UTF-8?q?=E2=80=94=20same-version=20predecessor=20guard,=20publish-branc?= =?UTF-8?q?h=20route=20restore,=20detached=20compensation,=20honest=20reco?= =?UTF-8?q?very=20reporting,=20logged=20image?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A08: the same-version path no longer force-removes a RUNNING _replaced container — after a failed attempt renamed the serving predecessor, a retry used to delete it before any healthy replacement existed, taking the app down. A running _replaced is refused with recovery direction; a stopped corpse is still cleared. Rename failures are no longer swallowed: an unclassified failure with the source container still present aborts the deploy instead of leaving snapshot and candidate names disagreed. A10: abortStateCommit's fixed-port branch (host ingress OR any publish entry) restores the Caddy route for caddy-ingress apps before removing the candidates — a caddy+publish app's commit failure used to return from the branch with Caddy still pointing at the removed candidate names. On route-restore failure the candidates are restarted instead of routing to nothing. A11: abortStateCommit runs every compensation (stops, restarts, route restore, log) on a detached bounded recovery context — a commit failure caused by a cancelled deploy context used to skip them via the dead ctx. A13: restoreDisplacedAndStarted reports every failed stop/remove/restart (itemized in output and in the returned error) — 'restored' was true whenever zero candidates had started, and one successful restart of a multi-container recovery read as full recovery. A26: logDeploy populates LogEntry.Image (the open half of TCL-19). --- internal/deploy/audit_fixes_test.go | 5 +- internal/deploy/deploy.go | 85 +++++-- internal/deploy/deploy_tcl01_tcl02_test.go | 1 + internal/deploy/deploy_test.go | 15 +- internal/deploy/recovery_a_test.go | 253 +++++++++++++++++++++ 5 files changed, 337 insertions(+), 22 deletions(-) create mode 100644 internal/deploy/recovery_a_test.go diff --git a/internal/deploy/audit_fixes_test.go b/internal/deploy/audit_fixes_test.go index a29a4bd..ea6fa4d 100644 --- a/internal/deploy/audit_fixes_test.go +++ b/internal/deploy/audit_fixes_test.go @@ -31,6 +31,7 @@ func TestDeploy_SameVersion_DedupesReplicaAndPlainNames(t *testing.T) { ssh.MockCommand{Match: "docker rm -f", Output: ""}, ssh.MockCommand{Match: "docker rename", Output: ""}, ssh.MockCommand{Match: "docker run", Output: "abc123def456\n"}, + ssh.MockCommand{Match: "docker inspect -f '{{.State.Status}}' 'myapp-web-abc123_replaced'", Output: ""}, ssh.MockCommand{Match: "docker inspect -f '{{.State.Status}}'", Output: "running"}, ssh.MockCommand{Match: "curl", Output: "200"}, ssh.MockCommand{Match: "docker ps --all", Output: ""}, @@ -64,10 +65,10 @@ func TestDeploy_SameVersion_DedupesReplicaAndPlainNames(t *testing.T) { rmCount := 0 renameIdx, runIdx := -1, -1 for i, call := range mock.Calls { - if strings.Contains(call, "docker rm -f myapp-web-abc123_replaced") { + if strings.Contains(call, "docker rm -f 'myapp-web-abc123_replaced'") { rmCount++ } - if strings.Contains(call, "docker rename myapp-web-abc123 ") && renameIdx < 0 { + if strings.Contains(call, "docker rename 'myapp-web-abc123' ") && renameIdx < 0 { renameIdx = i } if strings.HasPrefix(call, "docker run") && runIdx < 0 { diff --git a/internal/deploy/deploy.go b/internal/deploy/deploy.go index d9ba5d8..2ca9b61 100644 --- a/internal/deploy/deploy.go +++ b/internal/deploy/deploy.go @@ -334,8 +334,28 @@ func (d *Deployer) DeployFenced(ctx context.Context, cfg Config, lk *state.Lock) seen[docker.ContainerName(cfg.App, process, cfg.Version)] = true } for name := range seen { - d.exec.Run(ctx, fmt.Sprintf("docker rm -f %s 2>/dev/null", name+"_replaced")) - d.exec.Run(ctx, fmt.Sprintf("docker rename %s %s 2>/dev/null", name, name+"_replaced")) + replaced := name + "_replaced" + // A RUNNING _replaced container is the serving predecessor a + // previous failed attempt renamed (audit A08): the unconditional + // force-remove here used to delete the live workload before the + // replacement had even started, so a failed same-version retry + // took the app DOWN. Refuse and name the recovery path instead; + // a stopped corpse (interrupted deploy, completed redeploy whose + // remove failed) is still cleared as before. + if stOut, stErr := d.exec.Run(ctx, "docker inspect -f '{{.State.Status}}' "+ssh.ShellQuote(replaced)+" 2>/dev/null || true"); stErr == nil && strings.TrimSpace(stOut) == "running" { + return fmt.Errorf("container %s is still running — it is the previous same-version attempt's renamed (serving) workload; refusing to delete it. Restore it first (teploy rollback --app %s) or remove it deliberately", replaced, cfg.App) + } + d.exec.Run(ctx, "docker rm -f "+ssh.ShellQuote(replaced)+" 2>/dev/null || true") + // The rename's failure must not be swallowed (A08): with the + // predecessor still live under `name`, a silently ignored + // rename failure made the snapshot and the candidate run disagree + // about which container holds the workload. Only a confirmed + // absence of the source is a no-op. + if _, err := d.exec.Run(ctx, "docker rename "+ssh.ShellQuote(name)+" "+ssh.ShellQuote(replaced)); err != nil { + if srcOut, srcErr := d.exec.Run(ctx, "docker inspect -f '{{.State.Status}}' "+ssh.ShellQuote(name)+" 2>/dev/null || true"); srcErr != nil || strings.TrimSpace(srcOut) != "" { + return fmt.Errorf("renaming the current container %s to %s failed: %w — the workload is untouched; inspect the server before retrying", name, replaced, err) + } + } } } @@ -420,22 +440,35 @@ func (d *Deployer) DeployFenced(ctx context.Context, cfg Config, lk *state.Lock) // bounded so a hung cleanup cannot outlive the process. recoveryCtx, cancel := context.WithTimeout(context.Background(), 90*time.Second) defer cancel() + // Every intended compensation is reported (A13): a stop/remove + // failure leaves a stray container, a failed restart leaves the app + // (or one of its replicas) down — "at least one thing worked" must + // never read as "recovered". + var cleanupFailures []string for _, n := range started { - d.docker.Stop(recoveryCtx, n, 5) - d.docker.Remove(recoveryCtx, n) + if err := d.docker.Stop(recoveryCtx, n, 5); err != nil { + cleanupFailures = append(cleanupFailures, fmt.Sprintf("stop %s: %v", n, err)) + continue + } + if err := d.docker.Remove(recoveryCtx, n); err != nil { + cleanupFailures = append(cleanupFailures, fmt.Sprintf("remove %s: %v", n, err)) + } } - restored := len(started) == 0 + restored := len(displacedHostWeb) == 0 for _, old := range displacedHostWeb { if err := d.docker.Restart(recoveryCtx, old, nil); err != nil { + cleanupFailures = append(cleanupFailures, fmt.Sprintf("restore %s: %v", old, err)) fmt.Fprintf(d.out, " WARNING: could not restore displaced container %s: %v\n", old, err) } else { - restored = true fmt.Fprintf(d.out, " Restored %s\n", old) } } + if len(cleanupFailures) > 0 { + fmt.Fprintf(d.out, " WARNING: cleanup incomplete after failure — %s\n", strings.Join(cleanupFailures, "; ")) + } d.logDeploy(recoveryCtx, cfg, false, start) if !restored && len(displacedHostWeb) > 0 { - return fmt.Errorf("%w — recovery also failed: no container is serving; %s needs manual attention", reason, cfg.App) + return fmt.Errorf("%w — recovery also failed: no container is serving; %s needs manual attention (%s)", reason, cfg.App, strings.Join(cleanupFailures, "; ")) } return reason } @@ -829,22 +862,43 @@ func stopOldWorkloadsByName(ctx context.Context, dk *docker.Client, out io.Write } func (d *Deployer) abortStateCommit(ctx context.Context, cfg Config, current *state.AppState, started, displacedHostWeb []string, start time.Time, commitErr error) error { - d.logDeploy(ctx, cfg, false, start) + // Compensation runs on a DETACHED bounded context (A11): if the commit + // failed because the deploy context was cancelled, reusing that context + // would skip the very stops/restarts/route restores that undo the + // deploy — leaving the app dark while the error text claims recovery. + recoveryCtx, cancel := context.WithTimeout(context.Background(), 90*time.Second) + defer cancel() + d.logDeploy(recoveryCtx, cfg, false, start) if cfg.ingressHost() || len(cfg.Publish) > 0 { for _, name := range started { - d.docker.Stop(ctx, name, 5) + d.docker.Stop(recoveryCtx, name, 5) } for _, old := range displacedHostWeb { - if err := d.docker.Restart(ctx, old, nil); err != nil { + if err := d.docker.Restart(recoveryCtx, old, nil); err != nil { for _, name := range started { - d.docker.Start(ctx, name) + d.docker.Start(recoveryCtx, name) } return fmt.Errorf("committing authoritative applied state after replacing the fixed-port host workload: %w; restoring the original workload failed: %v; Teploy attempted to restart the new workload to avoid an outage", commitErr, err) } } + // A caddy-ingress app with publish entries entered this branch too + // (its fixed ports forced the recreate strategy) and its route WAS + // switched in step 11 — the commit failure used to return here + // without restoring the route, leaving Caddy pointed at the removed + // candidate names (A10). Restore it before removing the candidates; + // if that fails, keep the candidates running rather than routing to + // nothing. + if cfg.usesCaddy() { + if err := d.restorePreviousRoute(recoveryCtx, cfg, current); err != nil { + for _, name := range started { + d.docker.Start(recoveryCtx, name) + } + return fmt.Errorf("committing authoritative applied state after replacing the fixed-port host workload: %w; the original workload restarted but its route could not be restored: %v; the uncommitted workload was restarted to avoid an outage", commitErr, err) + } + } for _, name := range started { - d.docker.Remove(ctx, name) + d.docker.Remove(recoveryCtx, name) } if len(displacedHostWeb) == 0 { return fmt.Errorf("committing authoritative applied state after starting the first host-ingress workload: %w; the uncommitted workload was stopped and removed", commitErr) @@ -853,14 +907,14 @@ func (d *Deployer) abortStateCommit(ctx context.Context, cfg Config, current *st } if cfg.usesCaddy() { - if err := d.restorePreviousRoute(ctx, cfg, current); err != nil { + if err := d.restorePreviousRoute(recoveryCtx, cfg, current); err != nil { return fmt.Errorf("committing authoritative applied state after route switch: %w; restoring the previous route failed: %v; old and new workloads were left running to avoid routing to a stopped container", commitErr, err) } } for _, name := range started { - d.docker.Stop(ctx, name, 5) - d.docker.Remove(ctx, name) + d.docker.Stop(recoveryCtx, name, 5) + d.docker.Remove(recoveryCtx, name) } if current == nil { return fmt.Errorf("committing authoritative applied state after route switch: %w; the new route was removed and the uncommitted workload was stopped", commitErr) @@ -916,6 +970,7 @@ func (d *Deployer) logDeploy(ctx context.Context, cfg Config, success bool, star App: cfg.App, Type: "deploy", Hash: cfg.Version, + Image: cfg.Image, Success: success, DurationMs: time.Since(start).Milliseconds(), }) diff --git a/internal/deploy/deploy_tcl01_tcl02_test.go b/internal/deploy/deploy_tcl01_tcl02_test.go index daa6324..ad057a9 100644 --- a/internal/deploy/deploy_tcl01_tcl02_test.go +++ b/internal/deploy/deploy_tcl01_tcl02_test.go @@ -89,6 +89,7 @@ func TestDeploy_SameVersionCleanupNeverTouchesReplacement(t *testing.T) { ssh.MockCommand{Match: "docker rm -f", Output: ""}, ssh.MockCommand{Match: "docker rename", Output: ""}, ssh.MockCommand{Match: "docker run", Output: "abc123def456\n"}, + ssh.MockCommand{Match: "docker inspect -f '{{.State.Status}}' 'myapp-web-abc123_replaced'", Output: "exited"}, ssh.MockCommand{Match: "docker inspect -f '{{.State.Status}}'", Output: "running"}, ssh.MockCommand{Match: "curl", Output: "200"}, ssh.MockCommand{Match: "docker ps --all", Output: `{"ID":"old","Names":"myapp-web-abc123_replaced","Image":"myapp:v1","State":"running","Status":"Up","CreatedAt":"2026-01-01 00:00:00 +0000 UTC","Labels":"teploy.app=myapp,teploy.version=abc123,teploy.process=web"}`}, diff --git a/internal/deploy/deploy_test.go b/internal/deploy/deploy_test.go index b0b5482..4c6ed9e 100644 --- a/internal/deploy/deploy_test.go +++ b/internal/deploy/deploy_test.go @@ -619,7 +619,8 @@ func TestDeploy_SameVersion(t *testing.T) { ssh.MockCommand{Match: "if [ ! -e '/deployments/myapp/state' ]", Output: "present\n" + existingState}, ssh.MockCommand{Match: "ss -tln", Output: ssOutput}, // Rename existing container. - ssh.MockCommand{Match: "docker rename", Output: ""}, +ssh.MockCommand{Match: "docker inspect -f '{{.State.Status}}' 'myapp-web-abc123_replaced'", Output: ""}, + ssh.MockCommand{Match: "docker rename", Output: ""}, ssh.MockCommand{Match: "docker run", Output: "newcontainer"}, ssh.MockCommand{Match: "docker inspect -f", Output: "running"}, ssh.MockCommand{Match: "curl -s -o /dev/null", Output: "200"}, @@ -657,7 +658,7 @@ func TestDeploy_SameVersion(t *testing.T) { // Verify rename was called. renameFound := false for _, call := range mock.Calls { - if strings.Contains(call, "docker rename myapp-web-abc123 myapp-web-abc123_replaced") { + if strings.Contains(call, "docker rename 'myapp-web-abc123' 'myapp-web-abc123_replaced'") { renameFound = true } } @@ -687,6 +688,9 @@ func TestDeploy_SameVersion_StaleReplaced(t *testing.T) { ssh.MockCommand{Match: "ss -tln", Output: ssOutput}, // Pre-rename cleanup of stale _replaced container. ssh.MockCommand{Match: "docker rm -f", Output: ""}, + // The stale _replaced container exists in Exited state (A08: a + // RUNNING one must be refused, an exited corpse is cleared). + ssh.MockCommand{Match: "docker inspect -f '{{.State.Status}}' 'myapp-web-abc123_replaced'", Output: "exited"}, // Rename live container. ssh.MockCommand{Match: "docker rename", Output: ""}, ssh.MockCommand{Match: "docker run", Output: "newcontainer"}, @@ -1323,7 +1327,8 @@ func TestDeploy_SameVersionWithWorkers(t *testing.T) { ssh.MockCommand{Match: "if [ ! -e '/deployments/myapp/state.json' ]", Output: "absent"}, ssh.MockCommand{Match: "if [ ! -e '/deployments/myapp/state' ]", Output: "present\n" + stateContent}, ssh.MockCommand{Match: "ss -tln", Output: ssOutput}, - // Rename existing containers. +ssh.MockCommand{Match: "docker inspect -f '{{.State.Status}}' 'myapp-", Output: ""}, + // Rename existing containers. ssh.MockCommand{Match: "docker rename", Output: ""}, // Start new containers. ssh.MockCommand{Match: "docker run", Output: "redeploycontainer"}, @@ -1369,10 +1374,10 @@ func TestDeploy_SameVersionWithWorkers(t *testing.T) { renameWeb := false renameWorker := false for _, call := range mock.Calls { - if strings.Contains(call, "docker rename myapp-web-abc123 myapp-web-abc123_replaced") { + if strings.Contains(call, "docker rename 'myapp-web-abc123' 'myapp-web-abc123_replaced'") { renameWeb = true } - if strings.Contains(call, "docker rename myapp-worker-abc123 myapp-worker-abc123_replaced") { + if strings.Contains(call, "docker rename 'myapp-worker-abc123' 'myapp-worker-abc123_replaced'") { renameWorker = true } } diff --git a/internal/deploy/recovery_a_test.go b/internal/deploy/recovery_a_test.go new file mode 100644 index 0000000..59d0ace --- /dev/null +++ b/internal/deploy/recovery_a_test.go @@ -0,0 +1,253 @@ +package deploy + +import ( + "bytes" + "context" + "encoding/base64" + "encoding/json" + "strings" + "testing" + "time" + + "github.com/useteploy/teploy/internal/ssh" + "github.com/useteploy/teploy/internal/state" +) + +// TestSameVersion_RunningReplacedRefused is the A08 core regression: after +// a failed same-version attempt renamed the serving predecessor to +// _replaced, a retry must REFUSE to force-remove the running workload +// instead of deleting it before any healthy replacement exists. +func TestSameVersion_RunningReplacedRefused(t *testing.T) { + existingState := "current_port=49152\ncurrent_hash=abc123\nprevious_port=0\nprevious_hash=\n" + mock := ssh.NewMockExecutor("1.2.3.4", + ssh.MockCommand{Match: "mkdir -p /deployments/myapp", Output: ""}, + ssh.MockCommand{Match: "mkdir /deployments/myapp/.lock", Output: ""}, + ssh.MockCommand{Match: "if [ ! -e '/deployments/myapp/state.json' ]", Output: "absent"}, + ssh.MockCommand{Match: "if [ ! -e '/deployments/myapp/state' ]", Output: "present\n" + existingState}, + ssh.MockCommand{Match: "ss -tln", Output: ssOutput}, + // The prior failed attempt left the serving predecessor RUNNING + // under the _replaced name. + ssh.MockCommand{Match: "docker inspect -f '{{.State.Status}}' 'myapp-web-abc123_replaced'", Output: "running"}, + ssh.MockCommand{Match: "docker rm -f", Output: ""}, + ssh.MockCommand{Match: "docker rename", Output: ""}, + ) + var buf bytes.Buffer + d := NewDeployer(mock, &buf) + err := d.Deploy(context.Background(), Config{ + App: "myapp", + Domain: "myapp.com", + Image: "myapp:latest", + Version: "abc123", + Health: HealthConfig{Timeout: 5 * time.Second, Interval: 10 * time.Millisecond}, + }) + if err == nil || !strings.Contains(err.Error(), "refusing to delete") { + t.Fatalf("expected the running _replaced workload to be refused, got %v", err) + } + for _, c := range mock.Calls { + if strings.HasPrefix(c, "docker rm -f") && strings.Contains(c, "_replaced") { + t.Errorf("the running serving predecessor must not be removed, saw: %s", c) + } + if strings.HasPrefix(c, "docker run") { + t.Errorf("no candidate may start while the predecessor's fate is undecided, saw: %s", c) + } + } +} + +// TestSameVersion_RenameFailureAborts: a rename failure with the source +// container still present must abort the deploy (A08) instead of being +// swallowed and leaving the snapshot and the candidate names disagreed. +func TestSameVersion_RenameFailureAborts(t *testing.T) { + existingState := "current_port=49152\ncurrent_hash=abc123\nprevious_port=0\nprevious_hash=\n" + mock := ssh.NewMockExecutor("1.2.3.4", + ssh.MockCommand{Match: "mkdir -p /deployments/myapp", Output: ""}, + ssh.MockCommand{Match: "mkdir /deployments/myapp/.lock", Output: ""}, + ssh.MockCommand{Match: "if [ ! -e '/deployments/myapp/state.json' ]", Output: "absent"}, + ssh.MockCommand{Match: "if [ ! -e '/deployments/myapp/state' ]", Output: "present\n" + existingState}, + ssh.MockCommand{Match: "ss -tln", Output: ssOutput}, + ssh.MockCommand{Match: "docker inspect -f '{{.State.Status}}' 'myapp-web-abc123_replaced'", Output: ""}, + ssh.MockCommand{Match: "docker rm -f", Output: ""}, + // The rename fails (e.g. transport) and the source provably still + // exists — the deploy must abort rather than continue on a guess. + ssh.MockCommand{Match: "docker rename", Err: errBoom}, + ssh.MockCommand{Match: "docker inspect -f '{{.State.Status}}' 'myapp-web-abc123'", Output: "running"}, + ssh.MockCommand{Match: "docker run", Output: "x"}, + ) + var buf bytes.Buffer + d := NewDeployer(mock, &buf) + err := d.Deploy(context.Background(), Config{ + App: "myapp", + Domain: "myapp.com", + Image: "myapp:latest", + Version: "abc123", + Health: HealthConfig{Timeout: 5 * time.Second, Interval: 10 * time.Millisecond}, + }) + if err == nil || !strings.Contains(err.Error(), "renaming the current container") { + t.Fatalf("expected an abort on unclassified rename failure, got %v", err) + } + for _, c := range mock.Calls { + if strings.HasPrefix(c, "docker run") { + t.Errorf("no candidate may start after an unclassified rename failure, saw: %s", c) + } + } +} + +// TestAbortStateCommit_CancelledContextStillRunsCompensation is the A11 +// core regression: a state-commit failure caused by a CANCELLED deploy +// context must still run every compensating stop on a live, detached +// context. Host ingress keeps the flow free of Caddy so the assertion is +// exactly "compensation ran". +func TestAbortStateCommit_CancelledContextStillRunsCompensation(t *testing.T) { + app := "fency" + mock := ssh.NewMockExecutor("1.2.3.4", fenceHappyPathMocks(app)...) + // The cancelled deploy context: every exec call on it would fail. + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + var buf bytes.Buffer + d := NewDeployer(mock, &buf) + cfg := Config{ + App: app, + Image: "fency:latest", + Version: "abc123", + Ingress: "host", + ContainerPort: 8080, + } + started := []string{"fency-web-abc123"} + err := d.abortStateCommit(ctx, cfg, nil, started, nil, time.Now(), errBoom) + if err == nil { + t.Fatal("expected the commit error to be returned") + } + sawStop := false + for _, c := range mock.Calls { + if strings.HasPrefix(c, "docker stop") { + sawStop = true + } + } + if !sawStop { + t.Error("compensation must run on a detached context even when the deploy context is cancelled") + } +} + +// TestAbortStateCommit_CaddyPublishAppRestoresRoute is the A10 core +// regression: a caddy-ingress app with publish entries takes the fixed-port +// recreate branch — its commit failure must restore the Caddy route too, +// not just the workload (the old code returned from the branch without the +// route restore, leaving Caddy pointed at the removed candidate names). +func TestAbortStateCommit_CaddyPublishAppRestoresRoute(t *testing.T) { + app := "fency" + current := &state.AppState{ + SchemaVersion: 2, DeploymentType: "container", IngressMode: "caddy", + CurrentHash: "oldhash", CurrentPorts: []int{49152}, CurrentPort: 49152, + Domain: "fency.com", + } + // restorePreviousRoute inspects the old container's internal port; this + // stub must be registered BEFORE fenceHappyPathMocks' generic + // "docker inspect" (first match wins). + mocks := append([]ssh.MockCommand{ + ssh.MockCommand{Match: "docker inspect -f '{{range $p, $_ := .NetworkSettings.Ports}}", Output: "8080/tcp "}, + }, fenceHappyPathMocks(app)...) + mock := ssh.NewMockExecutor("1.2.3.4", mocks...) + + var buf bytes.Buffer + d := NewDeployer(mock, &buf) + cfg := Config{ + App: app, + Domain: "fency.com", + Image: "fency:latest", + Version: "abc123", + Publish: []string{"0.0.0.0:3001:3001"}, + Health: HealthConfig{Timeout: 5 * time.Second, Interval: 10 * time.Millisecond}, + } + started := []string{"fency-web-abc123"} + err := d.abortStateCommit(context.Background(), cfg, current, started, nil, time.Now(), errBoom) + if err == nil { + t.Fatal("expected the commit error to surface") + } + if !strings.Contains(err.Error(), "uncommitted workload was stopped and removed") { + t.Errorf("the failure must report the exact end state: %v", err) + } + sawRouteRestore := false + for _, c := range mock.Calls { + // applyManagedBlock renders the previous block into the Caddyfile + // and reloads the server — either shape proves the route restore. + if strings.Contains(c, "docker exec caddy caddy reload") || strings.Contains(c, "Caddyfile") { + sawRouteRestore = true + } + } + if !sawRouteRestore { + t.Error("a caddy-ingress app's fixed-port compensation must restore the Caddy route") + } +} + +// TestDeploy_HostIngressRunFailure_ItemizesFailedRecovery is the A13 +// regression: "at least one restart worked" must never read as +// "recovered" — every failed recovery appears in the outcome. +func TestDeploy_HostIngressRunFailure_ItemizesFailedRecovery(t *testing.T) { + app := "fency" + mock := ssh.NewMockExecutor("1.2.3.4", + ssh.MockCommand{Match: "mkdir -p /deployments/fency", Output: ""}, + ssh.MockCommand{Match: "mkdir /deployments/fency/.lock", Output: ""}, + ssh.MockCommand{Match: "if [ ! -e '/deployments/fency/state.json' ]", Output: "absent"}, + ssh.MockCommand{Match: "if [ ! -e '/deployments/fency/state' ]", Output: "absent"}, + ssh.MockCommand{Match: "ss -tln", Output: ssOutput}, + // The displaced web container stops fine... + ssh.MockCommand{Match: "docker ps --filter label=teploy.app=fency", Output: "fency-web-oldhash"}, + ssh.MockCommand{Match: "docker stop -t", Output: ""}, + // ...the candidate run fails... + ssh.MockCommand{Match: "docker run", Err: errBoom}, + // ...and EVERY recovery restart fails too (Restart = InspectRecreate + // + Recreate; the inspect fails, so the restart fails). + ssh.MockCommand{Match: "docker inspect", Err: errBoom}, + ssh.MockCommand{Match: "printf %s", Output: ""}, + ) + var buf bytes.Buffer + d := NewDeployer(mock, &buf) + err := d.Deploy(context.Background(), Config{ + App: app, + Image: "fency:latest", + Version: "abc123", + Ingress: "host", + ContainerPort: 8080, + }) + if err == nil { + t.Fatal("expected the deploy to fail") + } + if !strings.Contains(err.Error(), "no container is serving") { + t.Fatalf("a total recovery failure must say no container is serving, got: %v", err) + } + if !strings.Contains(buf.String(), "cleanup incomplete") { + t.Error("failed compensations must be itemized in the output") + } +} + +// TestLogDeploy_RecordsImage is the A26 regression (and the open half of +// TCL-19): the durable deploy log records the image identity, not just the +// version. +func TestLogDeploy_RecordsImage(t *testing.T) { + mock := ssh.NewMockExecutor("1.2.3.4", + ssh.MockCommand{Match: "printf %s", Output: ""}, + ) + d := &Deployer{exec: mock, out: &bytes.Buffer{}} + d.logDeploy(context.Background(), Config{App: "myapp", Image: "myapp:latest", Version: "abc123"}, true, time.Now()) + var line string + for _, c := range mock.Calls { + if strings.HasPrefix(c, "printf %s '") { + line = c + } + } + if line == "" { + t.Fatal("no log append issued") + } + enc := strings.TrimSuffix(strings.TrimPrefix(line, "printf %s '"), "' | base64 -d >> /deployments/teploy.log") + raw, err := base64.StdEncoding.DecodeString(enc) + if err != nil { + t.Fatalf("decoding log entry: %v", err) + } + var entry state.LogEntry + if err := json.Unmarshal(raw, &entry); err != nil { + t.Fatalf("parsing log entry: %v", err) + } + if entry.Image != "myapp:latest" { + t.Errorf("log entry image: got %q want myapp:latest", entry.Image) + } +} From 2faab7075466813a4250963fcf32b60a5df3e456 Mon Sep 17 00:00:00 2001 From: Tyler <53561637+im-tyler@users.noreply.github.com> Date: Sat, 19 Sep 2026 01:46:12 -0700 Subject: [PATCH 4/9] =?UTF-8?q?fix(deploy,docker,releasemeta,ssh):=20A15+A?= =?UTF-8?q?17+A18+A23+A52+A14=20=E2=80=94=20attempt-scoped=20assets,=20pla?= =?UTF-8?q?n=20validation,=20default=20port,=20image=20pinning,=20worker?= =?UTF-8?q?=20readiness,=20partial-run=20reconcile?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A15: asset bridging builds THIS attempt's private asset tree under meta/att/./assets (seeded from the previous attempt's tree with a real cp -a — hardlink seeding would let a later write truncate shared inodes) instead of copying into the shared /deployments//assets the running release still reads; a failed candidate can no longer mutate the live app's files. Extraction switches from 'docker run IMAGE sh -c cp' (image ENTRYPOINTs could wrap/replace it) to docker create + docker cp, and the volumes map is cloned before the mount is added, so the caller's map is no longer mutated through the Config copy. A52: DeployFenced resolves the immutable image ID once and creates every web replica and worker from it — a mutable tag re-pointed by a concurrent pull/build/tag between creates can no longer mix images within a release (the requested ref stays the recorded provenance; resolution failure warns and falls back to the requested reference). A17: Config.validate now checks identity grammar (app via config. ValidateName, version, process names), rejects unknown ingress modes, and rejects publish+replicas>1 (fixed ports cannot be load balanced). releasemeta.Path validates the app against the config grammar (was: non-empty only). SplitHostPort rejects ports outside 1..65535. A18: ContainerPort == 0 normalizes to 80 once at the top of DeployFenced and the normalized value drives host ports, Caddy upstreams, the diagnosis, and every create — a ':0' upstream can no longer be rendered. A23: workers must still be running (not exited/dead/restarting/unhealthy) one second after their detached run, or the deploy fails with full cleanup; an unreadable state inspect degrades to a warning. A14: a failed docker run reconciles the candidate name — a created-but- unstarted corpse is removed so the next deploy cannot collide; a RUNNING container under the name is never touched. --- internal/deploy/deploy.go | 246 ++++++++++++++++++++++---- internal/deploy/deploy_test.go | 27 ++- internal/deploy/hardening_test.go | 2 +- internal/deploy/plan_a_test.go | 265 ++++++++++++++++++++++++++++ internal/docker/docker.go | 17 ++ internal/releasemeta/attempt.go | 23 ++- internal/releasemeta/releasemeta.go | 10 +- internal/ssh/external.go | 6 +- 8 files changed, 545 insertions(+), 51 deletions(-) create mode 100644 internal/deploy/plan_a_test.go diff --git a/internal/deploy/deploy.go b/internal/deploy/deploy.go index 2ca9b61..193abd0 100644 --- a/internal/deploy/deploy.go +++ b/internal/deploy/deploy.go @@ -6,11 +6,13 @@ import ( "fmt" "io" "maps" + "regexp" "sort" "strings" "time" "github.com/useteploy/teploy/internal/caddy" + "github.com/useteploy/teploy/internal/config" "github.com/useteploy/teploy/internal/docker" "github.com/useteploy/teploy/internal/releasemeta" "github.com/useteploy/teploy/internal/ssh" @@ -106,13 +108,40 @@ func NewDeployer(exec ssh.Executor, out io.Writer) *Deployer { } } +// validVersion matches release ids the deploy accepts (the same grammar +// releasemeta uses for meta file names — git short hashes, tags like +// v1.2.3, sha256- image labels). +var validVersion = regexp.MustCompile(`^[A-Za-z0-9_][A-Za-z0-9._-]{0,127}$`) + +// validProcessName keeps process names safe as container-name segments. +var validProcessName = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9_.-]{0,62}$`) + // validate checks the deploy config's required fields. This is the shared // execution-plan validator: direct/ad-hoc construction (multideploy, // preview, autodeploy) does not pass through config-file parsing, so the -// bounds enforced there cannot be assumed here (TCL-18). +// bounds enforced there cannot be assumed here (TCL-18). Identity grammar +// (app, version, process names) and the ingress enum are checked too, so +// no matter how a Config was built, its values are safe to interpolate +// into container names, remote paths, and shell text (audit A17). func (c Config) validate() error { - if c.App == "" || c.Image == "" || c.Version == "" { - return fmt.Errorf("app, image, and version are required") + if err := config.ValidateName(c.App); err != nil { + return err + } + if c.Image == "" { + return fmt.Errorf("image is required") + } + if !validVersion.MatchString(c.Version) { + return fmt.Errorf("invalid version %q — must be alphanumeric with . _ - (max 128 chars)", c.Version) + } + for process := range c.Processes { + if !validProcessName.MatchString(process) { + return fmt.Errorf("invalid process name %q", process) + } + } + switch c.Ingress { + case "", "caddy", "external", "host": + default: + return fmt.Errorf("unknown ingress mode %q (expected caddy, external, or host)", c.Ingress) } // Caddy/external ingress route by domain; host ingress publishes a raw // port and needs no domain. @@ -129,9 +158,11 @@ func (c Config) validate() error { } // A fixed host port cannot be shared across containers — mirror the // config-layer rejection so a directly constructed Config cannot ask - // for a deploy that self-collides. - if c.ingressHost() && c.Replicas > 1 { - return fmt.Errorf("host ingress supports a single replica (a fixed host port can't be load-balanced across containers)") + // for a deploy that self-collides. Publish entries are fixed ports for + // the same reason host ingress is (A17): replicas>1 with publish would + // die on "port is already allocated" mid-deploy. + if (c.ingressHost() || len(c.Publish) > 0) && c.Replicas > 1 { + return fmt.Errorf("host ingress and fixed publish ports support a single replica (a fixed host port can't be load-balanced across containers)") } if c.StopTimeout < 0 { return fmt.Errorf("stop timeout cannot be negative (got %ds)", c.StopTimeout) @@ -194,6 +225,28 @@ func (d *Deployer) DeployFenced(ctx context.Context, cfg Config, lk *state.Lock) stopTimeout = 10 } + // Normalize the execution defaults ONCE and use the normalized value + // everywhere (A18): Config permits ContainerPort == 0 as "default 80", + // and the docker layer normalizes it only inside its publishing branch — + // using the raw zero for host ports or Caddy upstreams produced a ":0" + // upstream and a publish-less host deploy. + containerPort := cfg.ContainerPort + if containerPort == 0 { + containerPort = 80 + } + + // Pin every container creation this deploy makes to ONE immutable image + // identity (A52): a mutable tag can be re-pointed by a concurrent + // pull/build/tag between replica creates, mixing images within a + // release. Resolution failure warns and falls back to the requested + // reference (the create would fail against the same daemon anyway). + runImage := cfg.Image + if resolved, err := d.docker.ResolveImageID(ctx, cfg.Image); err == nil { + runImage = resolved + } else { + fmt.Fprintf(d.out, "Warning: could not resolve an immutable image ID for %s — creating from the requested reference (%v)\n", cfg.Image, err) + } + // Determine processes. Default: single web process with image CMD. processes := cfg.Processes if len(processes) == 0 { @@ -250,8 +303,8 @@ func (d *Deployer) DeployFenced(ctx context.Context, cfg Config, lk *state.Lock) // the app stays reachable at a stable bind:port. A fixed port can't be // blue/green (two containers can't bind it), so host mode recreates: // existing web containers are removed below before the new one starts. - ports = []int{cfg.ContainerPort} - fmt.Fprintf(d.out, "Publishing on %s:%d (host ingress)...\n", webBindHost, cfg.ContainerPort) + ports = []int{containerPort} + fmt.Fprintf(d.out, "Publishing on %s:%d (host ingress)...\n", webBindHost, containerPort) } else { // Allocate ephemeral ports for blue/green. Track the ports claimed so // far — containers aren't started until step 6, so `ss` can't see them @@ -272,38 +325,60 @@ func (d *Deployer) DeployFenced(ctx context.Context, cfg Config, lk *state.Lock) } port := ports[0] // primary port for health check, hooks, etc. - // 5. Asset bridging: extract assets from image before starting the container. + // 5. Asset bridging: extract assets from the image into THIS attempt's + // private tree before starting the container (audit A15). The old + // implementation copied straight into the SHARED + // /deployments//assets that the running release still reads — a + // failed candidate had already mutated the live app's files, with no + // compensation. The attempt-scoped tree is seeded from the previous + // attempt's tree with a real copy (cp -a, not hardlinks: a later + // extraction writing through a shared inode would truncate the previous + // tree's files), and the completed tree is mounted into the candidate. + // Extraction uses `docker create` + `docker cp`, so no image ENTRYPOINT + // ever executes (the old `docker run … sh -c` let an image with an + // ENTRYPOINT wrap or replace the copy command). if cfg.AssetPath != "" { - hostAssetDir := fmt.Sprintf("/deployments/%s/assets", cfg.App) + att := releasemeta.MustAttempt(cfg.App, cfg.Version) + assetDir := att.Dir() + "/assets" fmt.Fprintln(d.out, "Bridging assets...") - if _, err := d.exec.Run(ctx, "mkdir -p "+ssh.ShellQuote(hostAssetDir)); err != nil { - return fmt.Errorf("creating asset bridge directory: %w", err) + seed := "" + if prev := releasemeta.PreviousAttemptAssetsDir(ctx, d.exec, cfg.App, att.ID); prev != "" { + seed = prev + } + seedCmd := "mkdir -p " + ssh.ShellQuote(assetDir) + if seed != "" { + seedCmd += " && cp -a " + ssh.ShellQuote(seed+"/.") + " " + ssh.ShellQuote(assetDir+"/") + } + if _, err := d.exec.Run(ctx, seedCmd); err != nil { + return fmt.Errorf("creating asset tree: %w", err) } - // Extract assets from image using a one-shot container. - // --user 0 (root) so the cp can write into the host-owned - // /deployments//assets dir without permission denied, - // regardless of the image's USER directive. Drop `2>/dev/null` - // from cp so genuine failures surface in the deploy output — - // the previous silent-fail mode meant an empty host volume - // was bind-mounted over the in-image static dir, hiding all - // files and serving 404 for every static asset. - copyCmd := fmt.Sprintf("cp -r %s/. /bridge/ && echo ok-bridge", ssh.ShellQuote(cfg.AssetPath)) - extractCmd := fmt.Sprintf( - "docker run --rm --user 0 -v %s:/bridge %s sh -c %s", - ssh.ShellQuote(hostAssetDir), ssh.ShellQuote(cfg.Image), ssh.ShellQuote(copyCmd), - ) - out, err := d.exec.Run(ctx, extractCmd) - if err != nil || !strings.Contains(out, "ok-bridge") { - return fmt.Errorf("asset extraction failed: %s", strings.TrimSpace(out)) + // One-shot extraction container, removed on every exit. A stale + // corpse from an interrupted deploy is cleared first (a create with + // the same name would otherwise fail); the name carries the attempt + // id, so it can never collide with another attempt's extraction. + extractContainer := "teploy-assets-" + att.ID + extractCmd := strings.Join([]string{ + "docker rm -f " + ssh.ShellQuote(extractContainer) + " 2>/dev/null || true", + "docker create --name " + ssh.ShellQuote(extractContainer) + " " + ssh.ShellQuote(runImage), + "docker cp " + ssh.ShellQuote(extractContainer+":"+cfg.AssetPath+"/.") + " " + ssh.ShellQuote(assetDir+"/"), + "rc=$?", + "docker rm -f " + ssh.ShellQuote(extractContainer) + " >/dev/null 2>&1 || true", + "exit $rc", + }, "; ") + if _, err := d.exec.Run(ctx, extractCmd); err != nil { + return fmt.Errorf("asset extraction failed: %w", err) } - fmt.Fprintln(d.out, " Assets extracted to host") + fmt.Fprintln(d.out, " Assets extracted to the attempt's private tree") - // Mount the shared asset directory into the container. + // Mount the private tree — into a CLONED volumes map: mutating the + // caller's map through the Config value copy used to leak the mount + // into every subsequent use of that map (A15). + cfg.Volumes = maps.Clone(cfg.Volumes) if cfg.Volumes == nil { cfg.Volumes = map[string]string{} } - cfg.Volumes[hostAssetDir] = cfg.AssetPath + cfg.Volumes[assetDir] = cfg.AssetPath } // 6. Handle same-version redeploy: rename existing containers to avoid name conflicts. @@ -488,10 +563,10 @@ func (d *Deployer) DeployFenced(ctx context.Context, cfg Config, lk *state.Lock) App: cfg.App, Process: "web", Version: cfg.Version, - Image: cfg.Image, + Image: runImage, Port: ports[i], BindHost: webBindHost, - ContainerPort: cfg.ContainerPort, + ContainerPort: containerPort, Publish: cfg.Publish, EnvFiles: cfg.EnvFiles, Env: cfg.Env, @@ -503,6 +578,13 @@ func (d *Deployer) DeployFenced(ctx context.Context, cfg Config, lk *state.Lock) NoHealthcheck: cfg.NoHealthcheck["web"], }) if err != nil { + // Docker can CREATE a container and still fail the run (port + // binding, for one) — that corpse is not in `started`, so it + // would outlive this deploy and collide with the next one's + // candidate name. Reconcile it: remove the name's container + // only when it is NOT running (a running container under the + // candidate name is not provably ours — never kill it, A14). + d.reconcilePartialRun(name) return restoreDisplacedAndStarted(fmt.Errorf("starting container %s: %w", name, err)) } started = append(started, name) @@ -518,7 +600,7 @@ func (d *Deployer) DeployFenced(ctx context.Context, cfg Config, lk *state.Lock) if logs != "" { fmt.Fprintf(d.out, "\n--- Container logs ---\n%s\n--- End logs ---\n", logs) } - d.printDiagnosis(ctx, webContainerName, cfg.ContainerPort, reason, logs) + d.printDiagnosis(ctx, webContainerName, containerPort, reason, logs) return restoreDisplacedAndStarted(reason) } @@ -574,7 +656,7 @@ func (d *Deployer) DeployFenced(ctx context.Context, cfg Config, lk *state.Lock) App: cfg.App, Process: process, Version: cfg.Version, - Image: cfg.Image, + Image: runImage, Port: 0, // non-web processes don't get a port EnvFiles: cfg.EnvFiles, Env: cfg.Env, @@ -585,9 +667,19 @@ func (d *Deployer) DeployFenced(ctx context.Context, cfg Config, lk *state.Lock) NoHealthcheck: cfg.NoHealthcheck[process], }) if err != nil { + d.reconcilePartialRun(name) return fail(fmt.Errorf("starting %s: %w", name, err)) } started = append(started, name) + // A detached `docker run` proves nothing about the worker's + // viability — a bad command or an instantly-crashing process used + // to be recorded as a successful deploy while no jobs were consumed + // (A23). Require the worker to still be running one second later, + // and treat an already-exited/restarting/unhealthy state as a + // failed deploy (cleaned up with everything else via fail()). + if err := d.workerRemainsRunning(ctx, name); err != nil { + return fail(err) + } } // 11. Update Caddy route to point at new web container(s). @@ -609,7 +701,7 @@ func (d *Deployer) DeployFenced(ctx context.Context, cfg Config, lk *state.Lock) if replicas > 1 { upstreams := make([]caddy.Upstream, replicas) for i := range replicas { - upstreams[i] = caddy.Upstream{Dial: fmt.Sprintf("%s:%d", webContainerNames[i], cfg.ContainerPort)} + upstreams[i] = caddy.Upstream{Dial: fmt.Sprintf("%s:%d", webContainerNames[i], containerPort)} } // Caddy's active upstream checks probe the SAME path the deploy // readiness gate used (F47) — the block used to hardcode /up. @@ -618,7 +710,7 @@ func (d *Deployer) DeployFenced(ctx context.Context, cfg Config, lk *state.Lock) } fmt.Fprintf(d.out, " Traffic load-balanced across %d replicas\n", replicas) } else { - if err := d.caddy.SetRoute(ctx, cfg.App, cfg.Domain, webContainerName, cfg.ContainerPort, tls, cfg.CaddyExtra, cfg.Cache, cfg.Firewall, cfg.Access); err != nil { + if err := d.caddy.SetRoute(ctx, cfg.App, cfg.Domain, webContainerName, containerPort, tls, cfg.CaddyExtra, cfg.Cache, cfg.Firewall, cfg.Access); err != nil { return fail(fmt.Errorf("updating route: %w", err)) } fmt.Fprintln(d.out, " Traffic routed to new container") @@ -1091,3 +1183,83 @@ func imageDigestFromRef(image string) string { } return "" } + +// reconcilePartialRun removes the container occupying a candidate name +// after a failed `docker run`, but ONLY when it is not running (A14): +// Docker can create a container and fail the start (port binding, for one), +// leaving a corpse under the candidate name that this deploy never tracked +// and the next deploy's candidate would collide with. A RUNNING container +// under the name is not provably this operation's — it is left alone. +func (d *Deployer) reconcilePartialRun(name string) { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + out, err := d.exec.Run(ctx, "docker inspect -f '{{.State.Status}}' "+ssh.ShellQuote(name)+" 2>/dev/null || true") + if err != nil || strings.TrimSpace(out) == "" || strings.TrimSpace(out) == "running" { + return + } + if _, rmErr := d.exec.Run(ctx, "docker rm -f "+ssh.ShellQuote(name)); rmErr != nil { + fmt.Fprintf(d.out, "Warning: a partial container may remain under %s after the failed start: %v\n", name, rmErr) + } +} + +// workerRemainsRunning verifies a just-started worker process is actually +// viable: still running (not exited/dead/restarting) one second after the +// detached run, and not already flagged unhealthy by the image's +// healthcheck (A23). An inspect result that cannot be parsed degrades to a +// warning — the container itself remains subject to the normal cleanup +// paths — but a PARSED dead/restarting state fails the deploy. +func (d *Deployer) workerRemainsRunning(ctx context.Context, name string) error { + st, ok := d.inspectWorkerState(ctx, name) + if !ok { + fmt.Fprintf(d.out, "Warning: could not verify worker %s stability (inspect unreadable); proceeding\n", name) + return nil + } + if err := workerStateViable(st); err != nil { + return fmt.Errorf("worker %s is not viable: %w", name, err) + } + select { + case <-ctx.Done(): + return ctx.Err() + case <-time.After(time.Second): + } + st, ok = d.inspectWorkerState(ctx, name) + if !ok { + return nil + } + return workerStateViable(st) +} + +type workerStateJSON struct { + Status string `json:"Status"` + Running bool `json:"Running"` + Restarting bool `json:"Restarting"` + ExitCode int `json:"ExitCode"` + Health *struct { + Status string `json:"Status"` + } `json:"Health"` +} + +func (d *Deployer) inspectWorkerState(ctx context.Context, name string) (workerStateJSON, bool) { + out, err := d.exec.Run(ctx, "docker inspect -f '{{json .State}}' "+ssh.ShellQuote(name)) + if err != nil { + return workerStateJSON{}, false + } + var st workerStateJSON + if err := json.Unmarshal([]byte(strings.TrimSpace(out)), &st); err != nil { + return workerStateJSON{}, false + } + return st, true +} + +func workerStateViable(st workerStateJSON) error { + if st.Restarting || st.Status == "exited" || st.Status == "dead" { + return fmt.Errorf("container is %s (exit %d)", st.Status, st.ExitCode) + } + if st.Status != "running" { + return fmt.Errorf("container status is %q", st.Status) + } + if st.Health != nil && st.Health.Status == "unhealthy" { + return fmt.Errorf("image healthcheck reports unhealthy") + } + return nil +} diff --git a/internal/deploy/deploy_test.go b/internal/deploy/deploy_test.go index 4c6ed9e..f504a23 100644 --- a/internal/deploy/deploy_test.go +++ b/internal/deploy/deploy_test.go @@ -1183,9 +1183,12 @@ func TestDeploy_AssetBridging(t *testing.T) { ssh.MockCommand{Match: "if [ ! -e '/deployments/myapp/state' ]", Output: "absent"}, // 4. Find port. ssh.MockCommand{Match: "ss -tln", Output: ssOutput}, - // 5. Asset bridging: create dir + extract. - ssh.MockCommand{Match: "mkdir -p '/deployments/myapp/assets'", Output: ""}, - ssh.MockCommand{Match: "docker run --rm --user 0 -v '/deployments/myapp/assets':/bridge", Output: "ok-bridge\n"}, + // 5. Asset bridging: attempt-scoped tree (A15) + create/cp extraction. + ssh.MockCommand{Match: "mkdir -p '/deployments/myapp/meta/att/abc123.", Output: ""}, + ssh.MockCommand{Match: "ls -1 /deployments/myapp/meta/att", Output: ""}, + ssh.MockCommand{Match: "docker rm -f 'teploy-assets-", Output: ""}, + ssh.MockCommand{Match: "docker create --name 'teploy-assets-", Output: "extractcontainer"}, + ssh.MockCommand{Match: "docker cp 'teploy-assets-", Output: ""}, // 6. Start container. ssh.MockCommand{Match: "docker run --detach", Output: "abc123container"}, // 7. Verify running. @@ -1236,22 +1239,26 @@ func TestDeploy_AssetBridging(t *testing.T) { // Verify docker run includes asset volume mount. for _, call := range mock.Calls { if strings.HasPrefix(call, "docker run --detach") { - if !strings.Contains(call, "-v '/deployments/myapp/assets:/app/public/assets'") { + if !strings.Contains(call, "-v '/deployments/myapp/meta/att/abc123.") || !strings.Contains(call, ":/app/public/assets'") { t.Errorf("expected asset volume mount in docker run: %s", call) } break } } - // Verify one-shot extraction container was run. + // Verify one-shot extraction container was created (never run — no + // image ENTRYPOINT executes during extraction, A15). foundExtract := false for _, call := range mock.Calls { - if strings.Contains(call, "docker run --rm") && strings.Contains(call, "/bridge") { + if strings.Contains(call, "docker create --name 'teploy-assets-") && + strings.Contains(call, "'myapp:latest'") && + strings.Contains(call, "docker cp 'teploy-assets-") && + strings.Contains(call, ":/app/public/assets/.") { foundExtract = true } } if !foundExtract { - t.Error("expected one-shot asset extraction container") + t.Error("expected one-shot asset extraction via docker create + docker cp") } // Verify asset cleanup was run. @@ -1274,7 +1281,11 @@ func TestDeploy_AssetBridgingCustomKeepDays(t *testing.T) { ssh.MockCommand{Match: "if [ ! -e '/deployments/myapp/state' ]", Output: "absent"}, ssh.MockCommand{Match: "ss -tln", Output: ssOutput}, ssh.MockCommand{Match: "mkdir -p '/deployments/myapp/assets'", Output: ""}, - ssh.MockCommand{Match: "docker run --rm --user 0", Output: "ok-bridge\n"}, + ssh.MockCommand{Match: "mkdir -p '/deployments/myapp/meta/att/abc123.", Output: ""}, + ssh.MockCommand{Match: "ls -1 /deployments/myapp/meta/att", Output: ""}, + ssh.MockCommand{Match: "docker rm -f 'teploy-assets-", Output: ""}, + ssh.MockCommand{Match: "docker create --name 'teploy-assets-", Output: "extractcontainer"}, + ssh.MockCommand{Match: "docker cp 'teploy-assets-", Output: ""}, ssh.MockCommand{Match: "docker run --detach", Output: "abc123"}, ssh.MockCommand{Match: "docker inspect", Output: "running"}, ssh.MockCommand{Match: "curl -s -o /dev/null", Output: "200"}, diff --git a/internal/deploy/hardening_test.go b/internal/deploy/hardening_test.go index fbcd8d9..d4dea57 100644 --- a/internal/deploy/hardening_test.go +++ b/internal/deploy/hardening_test.go @@ -23,7 +23,7 @@ func TestDeploy_EmptyVersion(t *testing.T) { if err == nil { t.Fatal("expected error for empty version") } - if !strings.Contains(err.Error(), "required") { + if !strings.Contains(err.Error(), "invalid version") { t.Errorf("expected 'required' in error, got: %v", err) } } diff --git a/internal/deploy/plan_a_test.go b/internal/deploy/plan_a_test.go new file mode 100644 index 0000000..3129360 --- /dev/null +++ b/internal/deploy/plan_a_test.go @@ -0,0 +1,265 @@ +package deploy + +import ( + "bytes" + "context" + "strings" + "testing" + "time" + + "github.com/useteploy/teploy/internal/ssh" +) + +// TestConfigValidate_IdentityGrammar is the A17 regression: the shared +// execution-plan validator rejects shell/path-hostile identities, unknown +// ingress modes, and the publish+replicas fixed-port conflict before any +// command is issued. +func TestConfigValidate_IdentityGrammar(t *testing.T) { + base := func() Config { + return Config{App: "myapp", Domain: "myapp.com", Image: "i:latest", Version: "abc123"} + } + cases := []struct { + name string + mut func(*Config) + want string + }{ + {"app path metacharacters", func(c *Config) { c.App = "../escape" }, "app"}, + {"app uppercase", func(c *Config) { c.App = "MyApp" }, "app"}, + {"version traversal", func(c *Config) { c.Version = "../../etc" }, "version"}, + {"process name metacharacters", func(c *Config) { c.Processes = map[string]string{"w;rm": "x"} }, "process"}, + {"unknown ingress", func(c *Config) { c.Ingress = "carrier-pigeon" }, "ingress"}, + {"publish with replicas", func(c *Config) { + c.Publish = []string{"0.0.0.0:3001:3001"} + c.Replicas = 2 + }, "single replica"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + cfg := base() + tc.mut(&cfg) + err := cfg.validate() + if err == nil || !strings.Contains(err.Error(), tc.want) { + t.Fatalf("expected rejection mentioning %q, got %v", tc.want, err) + } + }) + } + // The accepted shapes still pass. + for _, ok := range []Config{ + {App: "my-app2", Domain: "d.com", Image: "i", Version: "v1.2.3"}, + {App: "a", Image: "i", Version: "sha256-abcdef", Ingress: "host"}, + {App: "a", Domain: "d.com", Image: "i", Version: "v", Ingress: "external"}, + {App: "a", Domain: "d.com", Image: "i", Version: "v", Publish: []string{"0.0.0.0:3001:3001"}, Replicas: 1}, + } { + if err := ok.validate(); err != nil { + t.Errorf("valid config rejected: %v (%+v)", err, ok) + } + } +} + +// TestDeploy_NormalizesDefaultContainerPort is the A18 regression: a +// Config built with ContainerPort == 0 must deploy with 80 everywhere — +// the Caddy upstream dial must never read "container:0". +func TestDeploy_NormalizesDefaultContainerPort(t *testing.T) { + mock := ssh.NewMockExecutor("1.2.3.4", + ssh.MockCommand{Match: "mkdir -p /deployments/myapp", Output: ""}, + ssh.MockCommand{Match: "mkdir /deployments/myapp/.lock", Output: ""}, + ssh.MockCommand{Match: "if [ ! -e '/deployments/myapp/state.json' ]", Output: "absent"}, + ssh.MockCommand{Match: "if [ ! -e '/deployments/myapp/state' ]", Output: "absent"}, + ssh.MockCommand{Match: "ss -tln", Output: ssOutput}, + ssh.MockCommand{Match: "docker run", Output: "abc123"}, + ssh.MockCommand{Match: "docker inspect", Output: "running"}, + ssh.MockCommand{Match: "curl -s -o /dev/null", Output: "200"}, + ssh.MockCommand{Match: "cat /deployments/caddy/Caddyfile", Output: "{\n\tadmin 0.0.0.0:2019\n}\n"}, + ssh.MockCommand{Match: "mkdir /deployments/caddy/.lock", Output: ""}, + ssh.MockCommand{Match: "a=$(docker exec caddy md5sum", Output: "TEPLOY_CADDY_OK"}, + ssh.MockCommand{Match: "docker exec caddy caddy reload", Output: ""}, + ssh.MockCommand{Match: "rmdir /deployments/caddy/.lock", Output: ""}, + ssh.MockCommand{Match: "printf %s", Output: ""}, + ) + var buf bytes.Buffer + d := NewDeployer(mock, &buf) + err := d.Deploy(context.Background(), Config{ + App: "myapp", + Domain: "myapp.com", + Image: "myapp:latest", + Version: "abc123", + Health: HealthConfig{Timeout: 5 * time.Second, Interval: 10 * time.Millisecond}, + }) + if err != nil { + t.Fatalf("Deploy: %v", err) + } + // The rendered route block dials the normalized container port. + caddyfile := string(mock.Files["/deployments/caddy/Caddyfile"]) + if !strings.Contains(caddyfile, "myapp-web-abc123:80") { + t.Errorf("expected the Caddy upstream to dial :80 for a default-valued ContainerPort, Caddyfile: %q", caddyfile) + } + if strings.Contains(caddyfile, "abc123:0") { + t.Errorf("a zero container port leaked into the route: %q", caddyfile) + } +} + +// TestDeploy_AllCreatesUseResolvedImageID is the A52 regression: when the +// image ID resolves, EVERY container creation (web + workers) runs the +// immutable ID, never the mutable tag — a concurrent re-tag between creates +// can no longer mix images within one release. +func TestDeploy_AllCreatesUseResolvedImageID(t *testing.T) { + imageID := "sha256:" + strings.Repeat("c0ffee", 10) + "abcd" + mock := ssh.NewMockExecutor("1.2.3.4", + ssh.MockCommand{Match: "mkdir -p /deployments/myapp", Output: ""}, + ssh.MockCommand{Match: "mkdir /deployments/myapp/.lock", Output: ""}, + ssh.MockCommand{Match: "if [ ! -e '/deployments/myapp/state.json' ]", Output: "absent"}, + ssh.MockCommand{Match: "if [ ! -e '/deployments/myapp/state' ]", Output: "absent"}, + ssh.MockCommand{Match: "ss -tln", Output: ssOutput}, + ssh.MockCommand{Match: "docker image inspect --format '{{.Id}}'", Output: imageID}, + ssh.MockCommand{Match: "docker run", Output: "abc123"}, + ssh.MockCommand{Match: "docker inspect", Output: "running"}, + ssh.MockCommand{Match: "curl -s -o /dev/null", Output: "200"}, + ssh.MockCommand{Match: "cat /deployments/caddy/Caddyfile", Output: "{\n\tadmin 0.0.0.0:2019\n}\n"}, + ssh.MockCommand{Match: "mkdir /deployments/caddy/.lock", Output: ""}, + ssh.MockCommand{Match: "a=$(docker exec caddy md5sum", Output: "TEPLOY_CADDY_OK"}, + ssh.MockCommand{Match: "docker exec caddy caddy reload", Output: ""}, + ssh.MockCommand{Match: "rmdir /deployments/caddy/.lock", Output: ""}, + ssh.MockCommand{Match: "printf %s", Output: ""}, + ) + var buf bytes.Buffer + d := NewDeployer(mock, &buf) + err := d.Deploy(context.Background(), Config{ + App: "myapp", + Domain: "myapp.com", + Image: "myapp:latest", + Version: "abc123", + Processes: map[string]string{"web": "", "worker": "npm run worker"}, + Health: HealthConfig{Timeout: 5 * time.Second, Interval: 10 * time.Millisecond}, + }) + if err != nil { + t.Fatalf("Deploy: %v", err) + } + creates := 0 + for _, c := range mock.Calls { + if !strings.HasPrefix(c, "docker run --detach") { + continue + } + creates++ + if !strings.Contains(c, "'"+imageID+"'") { + t.Errorf("container created from a non-pinned reference: %s", c) + } + } + if creates != 2 { + t.Fatalf("expected web+worker creates, got %d", creates) + } + // The requested reference remains the recorded provenance. + if s := string(mock.Files["/deployments/myapp/state.json"]); !strings.Contains(s, `"image_ref":"myapp:latest"`) { + t.Errorf("state must keep the requested image ref as provenance: %s", s) + } +} + +// TestDeploy_WorkerCrashLoopFailsDeploy is the A23 regression: a worker +// whose container exits (or restart-loops) right after the detached run +// fails the deploy — it used to be recorded as success while no jobs were +// consumed. +func TestDeploy_WorkerCrashLoopFailsDeploy(t *testing.T) { + mock := ssh.NewMockExecutor("1.2.3.4", + ssh.MockCommand{Match: "mkdir -p /deployments/myapp", Output: ""}, + ssh.MockCommand{Match: "mkdir /deployments/myapp/.lock", Output: ""}, + ssh.MockCommand{Match: "if [ ! -e '/deployments/myapp/state.json' ]", Output: "absent"}, + ssh.MockCommand{Match: "if [ ! -e '/deployments/myapp/state' ]", Output: "absent"}, + ssh.MockCommand{Match: "ss -tln", Output: ssOutput}, + ssh.MockCommand{Match: "docker run", Output: "abc123"}, + // The worker's full state shows it exited immediately (this stub + // must win over the generic running one below). + ssh.MockCommand{Match: "docker inspect -f '{{json .State}}'", Output: `{"Status":"exited","Running":false,"ExitCode":1}`}, + ssh.MockCommand{Match: "docker inspect", Output: "running"}, + ssh.MockCommand{Match: "curl -s -o /dev/null", Output: "200"}, + ssh.MockCommand{Match: "docker logs", Output: "boom"}, + ssh.MockCommand{Match: "docker stop", Output: ""}, + ssh.MockCommand{Match: "docker rm ", Output: ""}, + ssh.MockCommand{Match: "printf %s", Output: ""}, + ) + var buf bytes.Buffer + d := NewDeployer(mock, &buf) + err := d.Deploy(context.Background(), Config{ + App: "myapp", + Domain: "myapp.com", + Image: "myapp:latest", + Version: "abc123", + Processes: map[string]string{"web": "", "worker": "bad-command"}, + Health: HealthConfig{Timeout: 5 * time.Second, Interval: 10 * time.Millisecond}, + }) + if err == nil || !strings.Contains(err.Error(), "worker myapp-worker-abc123") { + t.Fatalf("expected the crashed worker to fail the deploy, got %v", err) + } + if _, ok := mock.Files["/deployments/myapp/state.json"]; ok { + t.Error("a deploy with a dead worker must not commit state") + } +} + +// TestDeploy_PartialRunCorpseReconciled is the A14 regression: when docker +// creates the container but the run fails, the untracked corpse under the +// candidate name is removed (it is not running), so the next deploy's +// candidate does not collide. +func TestDeploy_PartialRunCorpseReconciled(t *testing.T) { + mock := ssh.NewMockExecutor("1.2.3.4", + ssh.MockCommand{Match: "mkdir -p /deployments/myapp", Output: ""}, + ssh.MockCommand{Match: "mkdir /deployments/myapp/.lock", Output: ""}, + ssh.MockCommand{Match: "if [ ! -e '/deployments/myapp/state.json' ]", Output: "absent"}, + ssh.MockCommand{Match: "if [ ! -e '/deployments/myapp/state' ]", Output: "absent"}, + ssh.MockCommand{Match: "ss -tln", Output: ssOutput}, + // The run fails after creating the container... + ssh.MockCommand{Match: "docker run", Err: errBoom}, + // ...and the corpse under the candidate name is in Created state. + ssh.MockCommand{Match: "docker inspect -f '{{.State.Status}}' 'myapp-web-abc123'", Output: "created"}, + ssh.MockCommand{Match: "docker rm -f", Output: ""}, + ssh.MockCommand{Match: "printf %s", Output: ""}, + ) + var buf bytes.Buffer + d := NewDeployer(mock, &buf) + err := d.Deploy(context.Background(), Config{ + App: "myapp", + Domain: "myapp.com", + Image: "myapp:latest", + Version: "abc123", + Health: HealthConfig{Timeout: 5 * time.Second, Interval: 10 * time.Millisecond}, + }) + if err == nil { + t.Fatal("expected the deploy to fail") + } + sawCorpseRemoval := false + for _, c := range mock.Calls { + if strings.HasPrefix(c, "docker rm -f 'myapp-web-abc123'") { + sawCorpseRemoval = true + } + } + if !sawCorpseRemoval { + t.Error("the failed run's created-but-untracked container must be reconciled") + } +} + +// TestDeploy_RunningNameConflictNeverRemoved: a RUNNING container under the +// candidate name is never removed by the partial-run reconciler (A14). +func TestDeploy_RunningNameConflictNeverRemoved(t *testing.T) { + mock := ssh.NewMockExecutor("1.2.3.4", + ssh.MockCommand{Match: "mkdir -p /deployments/myapp", Output: ""}, + ssh.MockCommand{Match: "mkdir /deployments/myapp/.lock", Output: ""}, + ssh.MockCommand{Match: "if [ ! -e '/deployments/myapp/state.json' ]", Output: "absent"}, + ssh.MockCommand{Match: "if [ ! -e '/deployments/myapp/state' ]", Output: "absent"}, + ssh.MockCommand{Match: "ss -tln", Output: ssOutput}, + ssh.MockCommand{Match: "docker run", Err: errBoom}, + ssh.MockCommand{Match: "docker inspect -f '{{.State.Status}}' 'myapp-web-abc123'", Output: "running"}, + ssh.MockCommand{Match: "docker rm -f", Output: ""}, + ssh.MockCommand{Match: "printf %s", Output: ""}, + ) + var buf bytes.Buffer + d := NewDeployer(mock, &buf) + _ = d.Deploy(context.Background(), Config{ + App: "myapp", + Domain: "myapp.com", + Image: "myapp:latest", + Version: "abc123", + Health: HealthConfig{Timeout: 5 * time.Second, Interval: 10 * time.Millisecond}, + }) + for _, c := range mock.Calls { + if strings.HasPrefix(c, "docker rm -f 'myapp-web-abc123'") { + t.Errorf("a running container under the candidate name must never be force-removed: %s", c) + } + } +} diff --git a/internal/docker/docker.go b/internal/docker/docker.go index 5fd60b5..c97d2f7 100644 --- a/internal/docker/docker.go +++ b/internal/docker/docker.go @@ -266,6 +266,23 @@ func nameAlreadyInUse(output string, err error) bool { return strings.Contains(haystack, "already in use") } +// ResolveImageID returns the immutable local image ID for ref. Deploy +// resolves this ONCE and creates every replica/worker from the ID (audit +// A52): a mutable tag can be re-pointed by a concurrent pull/build/tag on +// the same host mid-deploy — an app-scoped lock does not own the global +// Docker tag namespace — silently mixing images within one release. +func (c *Client) ResolveImageID(ctx context.Context, ref string) (string, error) { + out, err := c.exec.Run(ctx, "docker image inspect --format '{{.Id}}' "+ssh.ShellQuote(ref)) + if err != nil { + return "", fmt.Errorf("resolving image identity for %s: %w", ref, err) + } + id := strings.TrimSpace(out) + if !strings.HasPrefix(id, "sha256:") || len(id) != len("sha256:")+64 { + return "", fmt.Errorf("docker returned no immutable image ID for %s (got %q)", ref, id) + } + return id, nil +} + // Stop stops a container by name. Sends SIGTERM, then SIGKILL after timeout seconds. func (c *Client) Stop(ctx context.Context, name string, timeout int) error { cmd := fmt.Sprintf("docker stop -t %d %s", timeout, ssh.ShellQuote(name)) diff --git a/internal/releasemeta/attempt.go b/internal/releasemeta/attempt.go index a631fdb..f0782f2 100644 --- a/internal/releasemeta/attempt.go +++ b/internal/releasemeta/attempt.go @@ -201,6 +201,27 @@ func PruneAttempts(ctx context.Context, exec ssh.Executor, app string, keepHashe // none (first attempt). Best-effort by contract: any failure simply means a // full transfer. func PreviousAttemptBuildDir(ctx context.Context, exec ssh.Executor, app, excludeID string) string { + return previousAttemptSubDir(ctx, exec, app, excludeID, "build") +} + +// PreviousAttemptAssetsDir returns the assets directory of the most recent +// other attempt, when one exists — the SEED for this attempt's private +// asset tree (audit A15): asset bridging must not mutate the live shared +// tree a running release still reads. Empty when there is none. +func PreviousAttemptAssetsDir(ctx context.Context, exec ssh.Executor, app, excludeID string) string { + dir := previousAttemptSubDir(ctx, exec, app, excludeID, "assets") + if dir == "" { + return "" + } + // Only a directory that provably exists is a usable seed; anything + // else means "no previous tree" (full extraction), not an error. + if out, err := exec.Run(ctx, "test -d "+ssh.ShellQuote(dir)+" && echo yes || echo no"); err != nil || strings.TrimSpace(out) != "yes" { + return "" + } + return dir +} + +func previousAttemptSubDir(ctx context.Context, exec ssh.Executor, app, excludeID, sub string) string { names, err := listAttempts(ctx, exec, attemptRoot(app)) if err != nil { return "" @@ -218,5 +239,5 @@ func PreviousAttemptBuildDir(ctx context.Context, exec ssh.Executor, app, exclud // random, so this is not chronology — it does not need to be; any // recent-ish basis gives rsync its delta. sort.Strings(filtered) - return attemptRoot(app) + "/" + filtered[len(filtered)-1] + "/build" + return attemptRoot(app) + "/" + filtered[len(filtered)-1] + "/" + sub } diff --git a/internal/releasemeta/releasemeta.go b/internal/releasemeta/releasemeta.go index 378526f..12f5da5 100644 --- a/internal/releasemeta/releasemeta.go +++ b/internal/releasemeta/releasemeta.go @@ -39,6 +39,7 @@ import ( "time" "github.com/useteploy/teploy/internal/caddy" + "github.com/useteploy/teploy/internal/config" "github.com/useteploy/teploy/internal/docker" "github.com/useteploy/teploy/internal/ssh" "github.com/useteploy/teploy/internal/state" @@ -151,9 +152,14 @@ type Record struct { // Path returns the record path for (app, hash). Both segments are grammar // checked here so no caller can interpolate an unvalidated id into a remote -// path. +// path — the app against the config name grammar (audit A17: it used to be +// checked only for non-emptiness, so a path-metacharacter app reached the +// remote shell), the hash against validHash. func Path(app, hash string) (string, error) { - if app == "" || !validHash.MatchString(hash) { + if err := config.ValidateName(app); err != nil { + return "", fmt.Errorf("release record requires a valid app: %w", err) + } + if !validHash.MatchString(hash) { return "", fmt.Errorf("invalid release id %q for app %q", hash, app) } return fmt.Sprintf("%s/%s/meta/%s.json", deploymentsDir, app, hash), nil diff --git a/internal/ssh/external.go b/internal/ssh/external.go index 6d02df5..90d1c92 100644 --- a/internal/ssh/external.go +++ b/internal/ssh/external.go @@ -75,14 +75,16 @@ func RsyncTarget(user, host, remotePath string) string { } // SplitHostPort splits an endpoint that may be "host", "host:port", a -// bracketed IPv6 "[host]:port", or a bare IPv6 literal. Returns +// bracketed IPv6 "[host]:port", or a bare IPv6 literal. The port, when +// present, must be a real TCP port number in 1..65535 — Atoi alone also +// accepted "0", negatives, and out-of-range values (audit A17). Returns // ("", "", error) for anything else. func SplitHostPort(endpoint string) (host, port string, err error) { if endpoint == "" { return "", "", fmt.Errorf("empty endpoint") } if h, p, splitErr := net.SplitHostPort(endpoint); splitErr == nil && h != "" && p != "" { - if _, convErr := strconv.Atoi(p); convErr == nil { + if n, convErr := strconv.Atoi(p); convErr == nil && n >= 1 && n <= 65535 { return h, p, nil } } From 784c955700bf372a0cf77382f1f92773276fc083 Mon Sep 17 00:00:00 2001 From: Tyler <53561637+im-tyler@users.noreply.github.com> Date: Sat, 19 Sep 2026 01:49:13 -0700 Subject: [PATCH 5/9] =?UTF-8?q?fix(docker,deploy):=20A19+A21+A25=20?= =?UTF-8?q?=E2=80=94=20bracketed=20validated=20publish=20bindings,=20ambig?= =?UTF-8?q?uity-refusing=20port=20inspectors,=20honest=20prune=20accountin?= =?UTF-8?q?g?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A19: the primary -p binding is built by publishBinding (net.JoinHostPort, IP-validated, both port-ranged) and quoted like every other interpolated argument — a bare IPv6 bind such as ::1 used to concatenate into an ambiguous '::1:49152:80' and was the run command's only unquoted value. A21: HostPort and InternalPort refuse containers with multiple DISTINCT ports instead of taking the first field (publish: entries and multi-EXPOSE images make those real; a wrong pick probed an auxiliary listener or routed Caddy at one); HostBindIP reports '' on mixed binds. The release record's TCL-14 primary designation remains the authority these fallbacks defer to. A25: PruneVersions counts a version as pruned only when every container removal succeeded and returns the joined failures; the deploy caller reports partial cleanup ('version prune incomplete') instead of printing nothing or claiming failed removals as pruned. --- internal/deploy/deploy.go | 13 +++- internal/deploy/deploy_test.go | 4 +- internal/docker/docker.go | 92 +++++++++++++++++++++++--- internal/docker/docker_test.go | 115 ++++++++++++++++++++++++++++++++- 4 files changed, 207 insertions(+), 17 deletions(-) diff --git a/internal/deploy/deploy.go b/internal/deploy/deploy.go index 193abd0..6a91635 100644 --- a/internal/deploy/deploy.go +++ b/internal/deploy/deploy.go @@ -901,9 +901,16 @@ func (d *Deployer) DeployFenced(ctx context.Context, cfg Config, lk *state.Lock) } else { protected = append(protected, pins...) pruned, err := d.docker.PruneVersions(ctx, cfg.App, cfg.KeepVersions, protected...) - if err != nil { - fmt.Fprintf(d.out, "Warning: version prune failed: %v\n", err) - } else if len(pruned) > 0 { + switch { + case err != nil: + // Partial cleanup (A25): report what was left behind — + // the old path printed nothing when any removal failed, + // or reported failed removals as pruned. + fmt.Fprintf(d.out, "Warning: version prune incomplete: %v\n", err) + if len(pruned) > 0 { + fmt.Fprintf(d.out, "Pruned %d superseded version(s): %s\n", len(pruned), strings.Join(pruned, ", ")) + } + case len(pruned) > 0: fmt.Fprintf(d.out, "Pruned %d superseded version(s): %s\n", len(pruned), strings.Join(pruned, ", ")) } } diff --git a/internal/deploy/deploy_test.go b/internal/deploy/deploy_test.go index f504a23..b981621 100644 --- a/internal/deploy/deploy_test.go +++ b/internal/deploy/deploy_test.go @@ -140,7 +140,7 @@ func TestDeploy_FirstDeploy(t *testing.T) { // Verify docker run included the right port. for _, call := range mock.Calls { if strings.HasPrefix(call, "docker run") { - if !strings.Contains(call, "-p 127.0.0.1:49152:80") { + if !strings.Contains(call, "-p '127.0.0.1:49152:80'") { t.Errorf("expected port mapping 127.0.0.1:49152:80 in docker run: %s", call) } if !strings.Contains(call, "-e PORT=80") { @@ -200,7 +200,7 @@ func TestDeploy_HostIngress(t *testing.T) { } if strings.HasPrefix(call, "docker run") { sawRun = true - if !strings.Contains(call, "-p 0.0.0.0:3000:3000") { + if !strings.Contains(call, "-p '0.0.0.0:3000:3000'") { t.Errorf("expected -p 0.0.0.0:3000:3000 in docker run: %s", call) } } diff --git a/internal/docker/docker.go b/internal/docker/docker.go index c97d2f7..846b8dd 100644 --- a/internal/docker/docker.go +++ b/internal/docker/docker.go @@ -3,8 +3,10 @@ package docker import ( "context" "encoding/json" + "errors" "fmt" "io" + "net" "sort" "strconv" "strings" @@ -57,6 +59,26 @@ type RunConfig struct { NoHealthcheck bool // pass --no-healthcheck so the container ignores the image HEALTHCHECK } +// publishBinding renders a docker -p binding "[ip:]host:container" with +// the bind IP correctly bracketed for IPv6 (A19) and both ports validated. +// The bind must be an IP literal — docker requires one for an explicit +// bind, and the health-probe builder already validates the same value, so +// a hostname here could only ever produce a deploy that fails its own +// health checks. +func publishBinding(bind string, hostPort, containerPort int) (string, error) { + normalized := strings.TrimSuffix(strings.TrimPrefix(bind, "["), "]") + if net.ParseIP(normalized) == nil { + return "", fmt.Errorf("publish bind %q must be an IP address", bind) + } + if hostPort < 1 || hostPort > 65535 { + return "", fmt.Errorf("host port %d must be in 1..65535", hostPort) + } + if containerPort < 1 || containerPort > 65535 { + return "", fmt.Errorf("container port %d must be in 1..65535", containerPort) + } + return net.JoinHostPort(normalized, strconv.Itoa(hostPort)) + ":" + strconv.Itoa(containerPort), nil +} + // ContainerName returns the standard teploy container name: {app}-{process}-{version}. func ContainerName(app, process, version string) string { return app + "-" + process + "-" + version @@ -144,12 +166,10 @@ func (c *Client) Run(ctx context.Context, cfg RunConfig) (string, error) { // Port publishing and PORT env var injection. if cfg.Port > 0 { - hostPort := strconv.Itoa(cfg.Port) containerPort := cfg.ContainerPort if containerPort == 0 { containerPort = 80 } - cPortStr := strconv.Itoa(containerPort) // Default: bind the published port to localhost only. Caddy reaches the // container over the teploy network via its network alias (see // InternalPort), so this host mapping exists solely for local health @@ -157,11 +177,20 @@ func (c *Client) Run(ctx context.Context, cfg RunConfig) (string, error) { // high port — bypassing Caddy/TLS, and Docker bypasses UFW — so we // restrict it to 127.0.0.1 unless the caller opts into a wider bind // (ingress: host sets BindHost to 0.0.0.0 for a directly-reachable port). + // + // The binding is built with net.JoinHostPort and validated as an IP + // (A19): a bare IPv6 bind such as ::1 used to concatenate into an + // ambiguous "::1:49152:80" that docker could only misparse, and the + // result is now quoted like every other interpolated argument. bindHost := cfg.BindHost if bindHost == "" { bindHost = "127.0.0.1" } - args = append(args, "-p", bindHost+":"+hostPort+":"+cPortStr, "-e", "PORT="+cPortStr) + binding, err := publishBinding(bindHost, cfg.Port, containerPort) + if err != nil { + return "", err + } + args = append(args, "-p", q(binding), "-e", "PORT="+strconv.Itoa(containerPort)) } // Extra host port mappings (AppConfig.Publish), kept separate from the @@ -425,6 +454,12 @@ func (c *Client) Remove(ctx context.Context, name string) error { // state.AppState.PreviousPort, which only ever remembers the single most // recent previous version — inspection works for --to rolling back // further than that. +// +// This is the LEGACY fallback for releases without a recorded primary port +// (TCL-14): a container publishing MULTIPLE distinct host ports (publish: +// entries) has no inspect-derived primary, and the old first-field pick +// could probe an auxiliary listener — that ambiguity is now an error, and +// callers prefer the record's designated primary (HostPortFor). func (c *Client) HostPort(ctx context.Context, name string) (int, error) { out, err := c.exec.Run(ctx, fmt.Sprintf( "docker inspect -f '{{range $p, $b := .NetworkSettings.Ports}}{{range $b}}{{.HostPort}} {{end}}{{end}}' %s", @@ -437,6 +472,13 @@ func (c *Client) HostPort(ctx context.Context, name string) (int, error) { if len(fields) == 0 { return 0, fmt.Errorf("container %s has no host-mapped ports", name) } + distinct := map[string]bool{} + for _, f := range fields { + distinct[f] = true + } + if len(distinct) > 1 { + return 0, fmt.Errorf("container %s publishes multiple host ports (%s) and has no recorded primary — its release record (TCL-14) is required to pick one", name, strings.Join(fields, ",")) + } port, err := strconv.Atoi(fields[0]) if err != nil { return 0, fmt.Errorf("parsing host port %q from container %s: %w", fields[0], name, err) @@ -464,6 +506,13 @@ func (c *Client) HostBindIP(ctx context.Context, name string) string { if len(fields) == 0 { return "" } + // A container whose ports bind DIFFERENT addresses has no single + // answer (A21) — report "cannot determine" rather than picking one. + for _, f := range fields { + if f != fields[0] { + return "" + } + } return fields[0] } @@ -485,7 +534,18 @@ func (c *Client) InternalPort(ctx context.Context, name string) (int, error) { if len(fields) == 0 { return 0, fmt.Errorf("container %s has no exposed ports", name) } - // Take the first port — teploy only publishes one per container. + distinct := map[string]bool{} + for _, f := range fields { + distinct[f] = true + } + // teploy containers publish one primary port, but publish: entries and + // multi-EXPOSE images make multi-port containers real — picking the + // first field could route Caddy at an auxiliary listener (A21). The + // record's designated primary (TCL-14) is the authority; inspection + // alone must refuse the guess. + if len(distinct) > 1 { + return 0, fmt.Errorf("container %s exposes multiple ports (%s) and has no recorded primary — its release record (TCL-14) is required to pick one", name, strings.Join(fields, ",")) + } portStr, _, _ := strings.Cut(fields[0], "/") port, err := strconv.Atoi(portStr) if err != nil { @@ -517,9 +577,10 @@ func (c *Client) ListContainers(ctx context.Context, app string) ([]Container, e // to bound the disk footprint of past deploys while keeping the current // version + a rollback window. // -// Returns the list of pruned versions and a non-nil error only when the -// initial container listing fails. Per-container removal failures are -// non-fatal — this is best-effort disk cleanup, not a deploy gate. +// Returns the list of versions whose containers were all removed (A25: a +// version with a failed container removal is NOT reported as pruned) plus +// a joined error describing every failed removal. Per-version image +// removal stays best-effort (a shared image legitimately refuses). func (c *Client) PruneVersions(ctx context.Context, app string, keep int, protectedVersions ...string) ([]string, error) { if keep < 0 { keep = 0 @@ -599,6 +660,7 @@ func (c *Client) PruneVersions(ctx context.Context, app string, keep int, protec } var pruned []string + var failures []error for _, e := range sorted { if protect[e.version] { continue @@ -606,8 +668,16 @@ func (c *Client) PruneVersions(ctx context.Context, app string, keep int, protec // Force-remove containers in case any are still running. We // already took ownership of cleanup; refusing to nuke a stray // running container from an older version defeats the point. + // A version counts as pruned ONLY when every container removal + // succeeded (A25) — the old loop counted it regardless, so + // "Pruned N" could report versions whose containers were still + // running, hiding disk exhaustion and failed cleanup. + complete := true for _, name := range e.info.containerNames { - _, _ = c.exec.Run(ctx, "docker rm -f "+ssh.ShellQuote(name)) + if _, err := c.exec.Run(ctx, "docker rm -f "+ssh.ShellQuote(name)); err != nil { + complete = false + failures = append(failures, fmt.Errorf("removing container %s (version %s): %w", name, e.version, err)) + } } // Best-effort image removal. Fails (silently) if another // container or tag still references the image, which is the @@ -615,9 +685,11 @@ func (c *Client) PruneVersions(ctx context.Context, app string, keep int, protec for img := range e.info.images { _, _ = c.exec.Run(ctx, "docker rmi "+ssh.ShellQuote(img)) } - pruned = append(pruned, e.version) + if complete { + pruned = append(pruned, e.version) + } } - return pruned, nil + return pruned, errors.Join(failures...) } // EnsureNetwork creates the "teploy" Docker network if it doesn't already exist. diff --git a/internal/docker/docker_test.go b/internal/docker/docker_test.go index ebe5037..839ced7 100644 --- a/internal/docker/docker_test.go +++ b/internal/docker/docker_test.go @@ -2,6 +2,7 @@ package docker import ( "context" + "errors" "fmt" "strings" "testing" @@ -137,7 +138,7 @@ func TestClient_Run(t *testing.T) { "--label 'teploy.app=myapp'", "--label 'teploy.process=web'", "--label 'teploy.version=abc123'", - "-p 127.0.0.1:49152:80", + "-p '127.0.0.1:49152:80'", "-e PORT=80", "--log-opt max-size=10m", "'nginx:latest'", @@ -168,7 +169,7 @@ func TestClient_Run_Publish(t *testing.T) { cmd := mock.Calls[0] for _, want := range []string{ - "-p 127.0.0.1:3000:80", + "-p '127.0.0.1:3000:80'", "-e PORT=80", "-p '0.0.0.0:3001:3001'", } { @@ -731,3 +732,113 @@ func TestClient_RestartQuotesInspectMetadata(t *testing.T) { } } } + +// TestPublishBinding_IPv6AndValidation is the A19 regression: a bare IPv6 +// bind renders bracketed, and non-IP binds / out-of-range ports are +// rejected instead of concatenated into ambiguous docker arguments. +func TestPublishBinding_IPv6AndValidation(t *testing.T) { + if got, err := publishBinding("::1", 49152, 80); err != nil || got != "[::1]:49152:80" { + t.Errorf("bare IPv6: got %q err %v", got, err) + } + if got, err := publishBinding("[::1]", 49152, 80); err != nil || got != "[::1]:49152:80" { + t.Errorf("bracketed IPv6: got %q err %v", got, err) + } + if _, err := publishBinding("", 49152, 80); err == nil { + t.Error("an empty bind must be rejected here (the caller supplies the default)") + } + for _, tc := range []struct{ bind string; hp, cp int }{ + {"host.example.com", 49152, 80}, + {"127.0.0.1", 0, 80}, + {"127.0.0.1", 65536, 80}, + {"127.0.0.1", 49152, 0}, + } { + if _, err := publishBinding(tc.bind, tc.hp, tc.cp); err == nil { + t.Errorf("expected rejection for %+v", tc) + } + } +} + +// TestClient_Run_IPv6BindRendered: the run command carries one quoted, +// bracketed -p argument for an IPv6 bind (A19). +func TestClient_Run_IPv6BindRendered(t *testing.T) { + mock := ssh.NewMockExecutor("1.2.3.4", ssh.MockCommand{Match: "docker run", Output: "abc123"}) + client := NewClient(mock) + if _, err := client.Run(context.Background(), RunConfig{ + App: "myapp", Process: "web", Version: "v1", Image: "i:latest", + Port: 49152, BindHost: "::1", ContainerPort: 80, + }); err != nil { + t.Fatalf("Run: %v", err) + } + found := false + for _, c := range mock.Calls { + if strings.Contains(c, "-p '[::1]:49152:80'") { + found = true + } + } + if !found { + t.Errorf("expected quoted bracketed -p binding, calls: %v", mock.Calls) + } +} + +// TestClient_PortInspectors_RefuseAmbiguity is the A21 regression: with +// multiple distinct exposed/host ports (publish: entries), the legacy +// first-field picks are guesses — the helpers must fail (or, for the bind +// IP, report undeterminable) instead of routing/probing an auxiliary +// listener. +func TestClient_PortInspectors_RefuseAmbiguity(t *testing.T) { + mock := ssh.NewMockExecutor("1.2.3.4", + ssh.MockCommand{Match: "docker inspect -f '{{range $p, $_ := .NetworkSettings.Ports}}", Output: "3000/tcp 8080/tcp "}, + ssh.MockCommand{Match: "docker inspect -f '{{range $p, $b := .NetworkSettings.Ports}}{{range $b}}{{.HostPort}}", Output: "49153 3001 "}, + ssh.MockCommand{Match: "docker inspect -f '{{range $p, $b := .NetworkSettings.Ports}}{{range $b}}{{.HostIp}}", Output: "127.0.0.1 0.0.0.0 "}, + ) + client := NewClient(mock) + ctx := context.Background() + if _, err := client.InternalPort(ctx, "multi"); err == nil || !strings.Contains(err.Error(), "multiple ports") { + t.Errorf("InternalPort must refuse a multi-port container: %v", err) + } + if _, err := client.HostPort(ctx, "multi"); err == nil || !strings.Contains(err.Error(), "multiple host ports") { + t.Errorf("HostPort must refuse a multi-port container: %v", err) + } + if ip := client.HostBindIP(ctx, "multi"); ip != "" { + t.Errorf("HostBindIP must report undeterminable on mixed binds, got %q", ip) + } + // Single-port answers are unchanged. + mock2 := ssh.NewMockExecutor("1.2.3.4", + ssh.MockCommand{Match: "docker inspect -f '{{range $p, $_ := .NetworkSettings.Ports}}", Output: "8080/tcp "}, + ssh.MockCommand{Match: "docker inspect -f '{{range $p, $b := .NetworkSettings.Ports}}{{range $b}}{{.HostPort}}", Output: "49153 "}, + ssh.MockCommand{Match: "docker inspect -f '{{range $p, $b := .NetworkSettings.Ports}}{{range $b}}{{.HostIp}}", Output: "0.0.0.0 0.0.0.0 "}, + ) + c2 := NewClient(mock2) + if p, err := c2.InternalPort(ctx, "single"); err != nil || p != 8080 { + t.Errorf("InternalPort single: %d %v", p, err) + } + if p, err := c2.HostPort(ctx, "single"); err != nil || p != 49153 { + t.Errorf("HostPort single: %d %v", p, err) + } + if ip := c2.HostBindIP(ctx, "single"); ip != "0.0.0.0" { + t.Errorf("HostBindIP single: %q", ip) + } +} + +// TestPruneVersions_FailedRemovalNotReportedPruned is the A25 regression: +// a version whose container removal fails is not counted as pruned, and +// the failure is returned. +func TestPruneVersions_FailedRemovalNotReportedPruned(t *testing.T) { + mock := ssh.NewMockExecutor("1.2.3.4", + ssh.MockCommand{Match: "docker ps --all", Output: strings.Join([]string{ + `{"ID":"a","Names":"myapp-web-v1","Image":"myapp:v1","State":"running","Status":"Up","CreatedAt":"2026-01-01 00:00:00 +0000 UTC","Labels":"teploy.app=myapp,teploy.version=v1"}`, + `{"ID":"b","Names":"myapp-web-v2","Image":"myapp:v2","State":"running","Status":"Up","CreatedAt":"2026-01-02 00:00:00 +0000 UTC","Labels":"teploy.app=myapp,teploy.version=v2"}`, + }, "\n")}, + ssh.MockCommand{Match: "docker rm -f 'myapp-web-v1'", Err: errors.New("device busy")}, + ssh.MockCommand{Match: "docker rm -f", Output: ""}, + ssh.MockCommand{Match: "docker rmi", Output: ""}, + ) + client := NewClient(mock) + pruned, err := client.PruneVersions(context.Background(), "myapp", 1, "v2") + if err == nil || !strings.Contains(err.Error(), "myapp-web-v1") { + t.Fatalf("expected the failed removal to be reported, got %v", err) + } + if len(pruned) != 0 { + t.Errorf("a version with a failed container removal must not be reported as pruned, got %v", pruned) + } +} From 3b6025dd3909068a70b60fd32f897892957bd27d Mon Sep 17 00:00:00 2001 From: Tyler <53561637+im-tyler@users.noreply.github.com> Date: Sat, 19 Sep 2026 01:55:45 -0700 Subject: [PATCH 6/9] =?UTF-8?q?fix(ssh,cli):=20A27+A28+A32=20=E2=80=94=20a?= =?UTF-8?q?tomic=20local=20uploads,=20process-tree=20cancellation,=20deriv?= =?UTF-8?q?ed=20provisioning=20identity?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A27: LocalExecutor.Upload mirrors RemoteExecutor.Upload's contract — the resident autodeploy path runs on this executor, so the remote hardening did not cover it. The write lands in a private sibling temp, is chmod'd and fsync'd BEFORE publication, and is renamed over the destination (replacing a leaf symlink itself, never following it); a failed or cancelled upload leaves the previous contents intact and no staging siblings behind. A28: local commands run in their own process group with Cancel killing the whole group (SIGKILL) and WaitDelay bounding the post-cancellation wait — exec.CommandContext's default killed only the shell, leaving descendants running with stdout/stderr open, which blocked CombinedOutput/Wait indefinitely on the resident engine and kept mutated state behind a released lease. Build-tagged: unix sets the group; the non-unix fallback kills the shell directly and is explicit about the weaker guarantee. A32: new PublicKeyBytes derives the provisioning public key FROM the requested private identity and verifies any existing .pub against it; setup uses it. PublicKeyPath no longer falls through to unrelated default .pub files for an explicit key (a key without .pub is an error pointing at derivation). The x/crypto/ssh import in remote.go is aliased (gossh) for the explicit references this adds. --- internal/cli/setup.go | 11 ++-- internal/ssh/hostkey_test.go | 53 ++++++++++++++++++ internal/ssh/local.go | 85 ++++++++++++++++++++-------- internal/ssh/local_other.go | 27 +++++++++ internal/ssh/local_test.go | 104 +++++++++++++++++++++++++++++++++++ internal/ssh/local_unix.go | 37 +++++++++++++ internal/ssh/remote.go | 86 +++++++++++++++++++++-------- 7 files changed, 352 insertions(+), 51 deletions(-) create mode 100644 internal/ssh/local_other.go create mode 100644 internal/ssh/local_unix.go diff --git a/internal/cli/setup.go b/internal/cli/setup.go index d798d51..1507ec6 100644 --- a/internal/cli/setup.go +++ b/internal/cli/setup.go @@ -91,13 +91,12 @@ func runSetup(flags *Flags, host string, name string, noHarden bool, networkProv // If password auth was used, inject the local SSH public key for future key-based auth. if usePassword { - pubKeyPath, err := ssh.PublicKeyPath(flags.Key) + // Derived from the private identity itself (A32): the old + // PublicKeyPath fallthrough could hand provisioning an unrelated + // default public key when --key named a key without a .pub. + pubKeyData, err := ssh.PublicKeyBytes(flags.Key) if err != nil { - return fmt.Errorf("finding SSH public key: %w", err) - } - pubKeyData, err := os.ReadFile(pubKeyPath) - if err != nil { - return fmt.Errorf("reading SSH public key: %w", err) + return fmt.Errorf("deriving SSH public key: %w", err) } pubKey := strings.TrimSpace(string(pubKeyData)) installCmd := fmt.Sprintf( diff --git a/internal/ssh/hostkey_test.go b/internal/ssh/hostkey_test.go index 50c73fe..c68419e 100644 --- a/internal/ssh/hostkey_test.go +++ b/internal/ssh/hostkey_test.go @@ -5,7 +5,9 @@ import ( "crypto/rand" "net" "os" + "os/exec" "path/filepath" + "strings" "testing" "golang.org/x/crypto/ssh" @@ -81,3 +83,54 @@ func TestAcceptNewHostKeyCallback_WriteSuccessRecordsKey(t *testing.T) { } var _ = net.Addr(fakeAddr{}) // compile-time interface check + +// TestPublicKeyBytes_DerivesFromPrivateKey is the A32 regression: with an +// explicit identity and NO .pub file, the public key is DERIVED from the +// private key instead of falling through to an unrelated default; a +// mismatched .pub is an error, never a silent identity switch. +func TestPublicKeyBytes_DerivesFromPrivateKey(t *testing.T) { + if _, err := exec.LookPath("ssh-keygen"); err != nil { + t.Skip("ssh-keygen not available") + } + dir := t.TempDir() + key := filepath.Join(dir, "id_test") + if out, err := exec.Command("ssh-keygen", "-t", "ed25519", "-N", "", "-f", key).CombinedOutput(); err != nil { + t.Fatalf("ssh-keygen: %v: %s", err, out) + } + + derived, err := PublicKeyBytes(key) + if err != nil { + t.Fatalf("PublicKeyBytes without .pub: %v", err) + } + want, err := os.ReadFile(key + ".pub") + if err != nil { + t.Fatal(err) + } + // MarshalAuthorizedKey omits the .pub's trailing comment — compare the + // key type and material only. + gotFields := strings.Fields(string(derived)) + wantFields := strings.Fields(string(want)) + if len(gotFields) < 2 || len(wantFields) < 2 || gotFields[0] != wantFields[0] || gotFields[1] != wantFields[1] { + t.Errorf("derived key does not match the generated .pub:\n got %q\nwant %q", derived, want) + } + + // A .pub that disagrees with the private key must be refused. + other := filepath.Join(dir, "id_other") + if out, err := exec.Command("ssh-keygen", "-t", "ed25519", "-N", "", "-f", other).CombinedOutput(); err != nil { + t.Fatalf("ssh-keygen: %v: %s", err, out) + } + if err := os.Rename(other+".pub", key+".pub"); err != nil { + t.Fatal(err) + } + if _, err := PublicKeyBytes(key); err == nil || !strings.Contains(err.Error(), "does not match") { + t.Fatalf("a mismatched .pub must be refused, got %v", err) + } + + // PublicKeyPath with an explicit key never falls through to defaults. + if err := os.Remove(key + ".pub"); err != nil { + t.Fatal(err) + } + if _, err := PublicKeyPath(key); err == nil { + t.Error("an explicit key without .pub must not select an unrelated default public key") + } +} diff --git a/internal/ssh/local.go b/internal/ssh/local.go index ecbc080..3cc9791 100644 --- a/internal/ssh/local.go +++ b/internal/ssh/local.go @@ -5,7 +5,6 @@ import ( "fmt" "io" "os" - "os/exec" "os/user" "path/filepath" "strconv" @@ -32,7 +31,8 @@ func NewLocalExecutor() *LocalExecutor { } func (e *LocalExecutor) Run(ctx context.Context, cmd string) (string, error) { - out, err := exec.CommandContext(ctx, "sh", "-c", cmd).CombinedOutput() + c := localCommand(ctx, cmd) + out, err := c.CombinedOutput() if err != nil { return "", fmt.Errorf("%w: %s", err, string(out)) } @@ -40,51 +40,92 @@ func (e *LocalExecutor) Run(ctx context.Context, cmd string) (string, error) { } func (e *LocalExecutor) RunStream(ctx context.Context, cmd string, stdout, stderr io.Writer) error { - c := exec.CommandContext(ctx, "sh", "-c", cmd) + c := localCommand(ctx, cmd) c.Stdout = stdout c.Stderr = stderr return c.Run() } func (e *LocalExecutor) RunInput(ctx context.Context, cmd string, stdin io.Reader) error { - c := exec.CommandContext(ctx, "sh", "-c", cmd) + c := localCommand(ctx, cmd) c.Stdin = stdin c.Stdout = io.Discard c.Stderr = io.Discard return c.Run() } -// Upload writes content to a local file, creating parent directories and -// setting mode (an octal string, e.g. "0644") — mirroring -// RemoteExecutor.Upload's semantics exactly so callers built against the -// Executor interface don't need to know which implementation they have. +// Upload writes content to a local file atomically, mirroring +// RemoteExecutor.Upload's contract (audit A27): the previous version +// buffered the whole input (ignoring cancellation), called os.WriteFile +// directly on the destination (following a leaf symlink, truncating an +// existing file on a mid-write failure), and chmod'd only AFTER the +// content was already visible at the destination's old permissions. The +// resident autodeploy path runs on this executor, so the remote +// hardening did not cover it. +// +// The write lands in a private sibling temp (0600 from creation), is +// chmod'd to the requested mode and fsync'd BEFORE publication, and is +// renamed over the destination — replacing a destination symlink itself, +// never its target. A failed or cancelled upload leaves the previous +// contents untouched. func (e *LocalExecutor) Upload(ctx context.Context, content io.Reader, path string, mode string) error { - data, err := io.ReadAll(content) + if err := ctx.Err(); err != nil { + return err + } + perm, err := strconv.ParseUint(mode, 8, 32) if err != nil { - return fmt.Errorf("reading upload content: %w", err) + return fmt.Errorf("invalid mode %q: %w", mode, err) } - - if err := os.MkdirAll(filepath.Dir(path), 0755); err != nil { + dir := filepath.Dir(path) + if err := os.MkdirAll(dir, 0755); err != nil { return fmt.Errorf("creating directory for %s: %w", path, err) } - - perm, err := strconv.ParseUint(mode, 8, 32) + f, err := os.CreateTemp(dir, ".teploy-upload-*") if err != nil { - return fmt.Errorf("invalid mode %q: %w", mode, err) + return fmt.Errorf("creating temp file beside %s: %w", path, err) } - - if err := os.WriteFile(path, data, os.FileMode(perm)); err != nil { + tmp := f.Name() + defer os.Remove(tmp) + // ctx is checked between reads; a reader that can block indefinitely + // must be closed by its owner (same contract as RemoteExecutor). + if _, err := io.Copy(f, readerWithCtx{ctx, content}); err != nil { + f.Close() return fmt.Errorf("writing %s: %w", path, err) } - // os.WriteFile only applies the mode on create — an existing file keeps - // its old permissions. Chmod explicitly so re-uploading (e.g. a - // redeployed binary) always ends up at the requested mode. - if err := os.Chmod(path, os.FileMode(perm)); err != nil { - return fmt.Errorf("chmod %s: %w", path, err) + if err := f.Chmod(os.FileMode(perm)); err != nil { + f.Close() + return fmt.Errorf("chmod %s: %w", tmp, err) + } + if err := f.Sync(); err != nil { + f.Close() + return fmt.Errorf("syncing %s: %w", tmp, err) + } + if err := f.Close(); err != nil { + return fmt.Errorf("closing %s: %w", tmp, err) + } + if err := ctx.Err(); err != nil { + return err + } + if err := os.Rename(tmp, path); err != nil { + return fmt.Errorf("publishing %s: %w", path, err) } return nil } +// readerWithCtx fails a copy once the context is done; reads themselves +// still block on the underlying reader (documented above). +type readerWithCtx struct { + ctx context.Context + r io.Reader +} + +func (r readerWithCtx) Read(p []byte) (int, error) { + if err := r.ctx.Err(); err != nil { + return 0, err + } + return r.r.Read(p) +} + func (e *LocalExecutor) Close() error { return nil } diff --git a/internal/ssh/local_other.go b/internal/ssh/local_other.go new file mode 100644 index 0000000..9927b34 --- /dev/null +++ b/internal/ssh/local_other.go @@ -0,0 +1,27 @@ +//go:build !unix + +package ssh + +import ( + "context" + "os" + "os/exec" + "time" +) + +// localCommand is the non-Unix fallback (audit A28): no process groups +// exist to kill, so cancellation kills the shell process directly and +// WaitDelay bounds the wait after cancellation. The LocalExecutor's +// resident mode targets Linux servers; on other platforms this keeps the +// build honest about what it can guarantee instead of pretending. +func localCommand(ctx context.Context, script string) *exec.Cmd { + c := exec.CommandContext(ctx, "sh", "-c", script) + c.Cancel = func() error { + if c.Process == nil { + return os.ErrProcessDone + } + return c.Process.Kill() + } + c.WaitDelay = 2 * time.Second + return c +} diff --git a/internal/ssh/local_test.go b/internal/ssh/local_test.go index 7c64197..cae820f 100644 --- a/internal/ssh/local_test.go +++ b/internal/ssh/local_test.go @@ -6,6 +6,7 @@ import ( "path/filepath" "strings" "testing" + "time" ) func TestLocalExecutor_Run(t *testing.T) { @@ -79,3 +80,106 @@ func TestLocalExecutor_HostAndUser(t *testing.T) { t.Error("User() should not be empty") } } + +// TestLocalExecutor_Upload_AtomicAndSymlinkSafe is the A27 regression: +// a destination leaf symlink is REPLACED (its target untouched), a failed +// read preserves the previous contents, the requested mode is applied +// before publication, and a cancelled context writes nothing. +func TestLocalExecutor_Upload_AtomicAndSymlinkSafe(t *testing.T) { + dir := t.TempDir() + e := NewLocalExecutor() + + victim := filepath.Join(dir, "victim") + if err := os.WriteFile(victim, []byte("old"), 0644); err != nil { + t.Fatal(err) + } + dest := filepath.Join(dir, "secret") + if err := os.Symlink(victim, dest); err != nil { + t.Fatal(err) + } + if err := e.Upload(context.Background(), strings.NewReader("new"), dest, "0600"); err != nil { + t.Fatalf("Upload over symlink: %v", err) + } + if got, _ := os.ReadFile(victim); string(got) != "old" { + t.Errorf("symlink target was modified: %q", got) + } + data, err := os.ReadFile(dest) + if err != nil || string(data) != "new" { + t.Fatalf("destination content: %q %v", data, err) + } + if fi, err := os.Lstat(dest); err != nil || !fi.Mode().IsRegular() || fi.Mode().Perm() != 0600 { + t.Errorf("destination must be a regular 0600 file: %v %v", fi, err) + } + + // Failed reader: previous contents survive. + persistent := filepath.Join(dir, "persistent") + if err := os.WriteFile(persistent, []byte("keep"), 0644); err != nil { + t.Fatal(err) + } + if err := e.Upload(context.Background(), &failingReader{}, persistent, "0600"); err == nil { + t.Fatal("expected the failed read to error") + } + if got, _ := os.ReadFile(persistent); string(got) != "keep" { + t.Errorf("failed upload corrupted the destination: %q", got) + } + + // Cancelled context: nothing is written. + ctx, cancel := context.WithCancel(context.Background()) + cancel() + cancelled := filepath.Join(dir, "cancelled") + if err := e.Upload(ctx, strings.NewReader("x"), cancelled, "0600"); err == nil { + t.Fatal("expected cancellation to error") + } + if _, err := os.Stat(cancelled); !os.IsNotExist(err) { + t.Errorf("cancelled upload wrote the destination: %v", err) + } + // No staging siblings are left behind. + entries, _ := os.ReadDir(dir) + for _, en := range entries { + if strings.HasPrefix(en.Name(), ".teploy-upload-") { + t.Errorf("staging sibling left behind: %s", en.Name()) + } + } +} + +type failingReader struct{ done bool } + +func (r *failingReader) Read(p []byte) (int, error) { + if r.done { + return 0, os.ErrClosed + } + r.done = true + copy(p, "partial") + return len("partial"), nil +} + +// TestLocalExecutor_CancellationKillsProcessGroup is the A28 regression: +// cancelling a command must terminate the whole process tree and return, +// not leave descendants running with the pipes open. The shell spawns a +// sleeping child; cancellation must let Run return promptly while the +// child is gone. +func TestLocalExecutor_CancellationKillsProcessGroup(t *testing.T) { + if testing.Short() { + t.Skip("spawns real processes") + } + marker := filepath.Join(t.TempDir(), "alive") + e := NewLocalExecutor() + ctx, cancel := context.WithTimeout(context.Background(), 500*time.Millisecond) + defer cancel() + start := time.Now() + _, err := e.Run(ctx, "sh -c 'sleep 5 && touch "+marker+"' >/dev/null 2>&1; sleep 5") + if err == nil { + t.Fatal("expected cancellation error") + } + if elapsed := time.Since(start); elapsed > 4*time.Second { + t.Fatalf("Run did not return promptly after cancellation: %s", elapsed) + } + deadline := time.Now().Add(3 * time.Second) + for time.Now().Before(deadline) { + if _, err := os.Stat(marker); os.IsNotExist(err) { + return // child died — pass + } + time.Sleep(50 * time.Millisecond) + } + t.Fatal("a descendant survived process-group cancellation") +} diff --git a/internal/ssh/local_unix.go b/internal/ssh/local_unix.go new file mode 100644 index 0000000..805fec0 --- /dev/null +++ b/internal/ssh/local_unix.go @@ -0,0 +1,37 @@ +//go:build unix + +package ssh + +import ( + "context" + "errors" + "os" + "os/exec" + "syscall" + "time" +) + +// localCommand builds the local `sh -c` invocation with whole-process-tree +// cancellation semantics (audit A28): the shell runs in its own process +// group, and cancelling the context SIGKILLs the GROUP — exec.Command's +// default kills only the shell process, so descendants kept running (and +// kept stdout/stderr open, blocking CombinedOutput/Wait indefinitely on a +// resident deployment engine whose lease was already released). WaitDelay +// bounds the wait after cancellation even when a descendant slips past the +// group kill. +func localCommand(ctx context.Context, script string) *exec.Cmd { + c := exec.CommandContext(ctx, "sh", "-c", script) + c.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + c.Cancel = func() error { + if c.Process == nil { + return os.ErrProcessDone + } + err := syscall.Kill(-c.Process.Pid, syscall.SIGKILL) + if errors.Is(err, syscall.ESRCH) { + return os.ErrProcessDone + } + return err + } + c.WaitDelay = 2 * time.Second + return c +} diff --git a/internal/ssh/remote.go b/internal/ssh/remote.go index 7387a0f..45ee166 100644 --- a/internal/ssh/remote.go +++ b/internal/ssh/remote.go @@ -14,17 +14,18 @@ import ( "strings" "time" - "golang.org/x/crypto/ssh" + gossh "golang.org/x/crypto/ssh" "golang.org/x/crypto/ssh/knownhosts" "golang.org/x/term" ) + // Compile-time check: RemoteExecutor implements Executor. var _ Executor = (*RemoteExecutor)(nil) // RemoteExecutor implements Executor using a real SSH connection. type RemoteExecutor struct { - client *ssh.Client + client *gossh.Client host string user string // acceptNewHost records the host-key policy this connection was created @@ -67,15 +68,15 @@ func Connect(ctx context.Context, cfg ConnectConfig) (*RemoteExecutor, error) { return nil, fmt.Errorf("no SSH keys found; provide --key, set TEPLOY_SSH_KEY, or place a key at ~/.ssh/id_ed25519") } - authMethods := []ssh.AuthMethod{} + authMethods := []gossh.AuthMethod{} if len(signers) > 0 { - authMethods = append(authMethods, ssh.PublicKeys(signers...)) + authMethods = append(authMethods, gossh.PublicKeys(signers...)) } if cfg.Password != "" { - authMethods = append(authMethods, ssh.Password(cfg.Password)) + authMethods = append(authMethods, gossh.Password(cfg.Password)) } - var hostKeyCallback ssh.HostKeyCallback + var hostKeyCallback gossh.HostKeyCallback if cfg.AcceptNewHost { home, err := os.UserHomeDir() if err != nil { @@ -90,7 +91,7 @@ func Connect(ctx context.Context, cfg ConnectConfig) (*RemoteExecutor, error) { } } - clientConfig := &ssh.ClientConfig{ + clientConfig := &gossh.ClientConfig{ User: cfg.User, Auth: authMethods, HostKeyCallback: hostKeyCallback, @@ -141,7 +142,7 @@ func (e *RemoteExecutor) RunStream(ctx context.Context, cmd string, stdout, stde case err := <-done: return err case <-ctx.Done(): - _ = session.Signal(ssh.SIGTERM) + _ = session.Signal(gossh.SIGTERM) _ = session.Close() // Wait for session.Run to actually return before we do — otherwise the // goroutine can keep writing to the caller's stdout/stderr after @@ -169,7 +170,7 @@ func (e *RemoteExecutor) RunInput(ctx context.Context, cmd string, stdin io.Read case err := <-done: return err case <-ctx.Done(): - _ = session.Signal(ssh.SIGTERM) + _ = session.Signal(gossh.SIGTERM) _ = session.Close() <-done return ctx.Err() @@ -225,7 +226,7 @@ func (e *RemoteExecutor) User() string { // defaultHostKeyCallback returns a known_hosts-based callback. When // known_hosts doesn't exist yet it falls back to trust-on-first-use (see // acceptNewHostKeyCallback) rather than accepting every key. -func defaultHostKeyCallback() (ssh.HostKeyCallback, error) { +func defaultHostKeyCallback() (gossh.HostKeyCallback, error) { home, err := os.UserHomeDir() if err != nil { // Previously fell through to ssh.InsecureIgnoreHostKey() here — silently @@ -262,7 +263,7 @@ func defaultHostKeyCallback() (ssh.HostKeyCallback, error) { // through to the default identities on a bad --key used to end with the // misleading "no SSH keys found" (or, worse, authenticated as a different // key than the operator named) instead of the actual key error (TCL-55). -func resolveSigners(keyPath string) ([]ssh.Signer, error) { +func resolveSigners(keyPath string) ([]gossh.Signer, error) { var paths []string if keyPath != "" { paths = []string{keyPath} @@ -277,7 +278,7 @@ func resolveSigners(keyPath string) ([]ssh.Signer, error) { } } - var signers []ssh.Signer + var signers []gossh.Signer for _, p := range paths { data, err := os.ReadFile(p) if err != nil { @@ -287,9 +288,9 @@ func resolveSigners(keyPath string) ([]ssh.Signer, error) { continue } - signer, err := ssh.ParsePrivateKey(data) + signer, err := gossh.ParsePrivateKey(data) if err != nil { - var passphraseErr *ssh.PassphraseMissingError + var passphraseErr *gossh.PassphraseMissingError if errors.As(err, &passphraseErr) { signer, err = parseEncryptedKey(data, p) if err != nil { @@ -310,14 +311,14 @@ func resolveSigners(keyPath string) ([]ssh.Signer, error) { return signers, nil } -func parseEncryptedKey(data []byte, keyPath string) (ssh.Signer, error) { +func parseEncryptedKey(data []byte, keyPath string) (gossh.Signer, error) { fmt.Fprintf(os.Stderr, "Enter passphrase for %s: ", keyPath) passphrase, err := term.ReadPassword(int(os.Stdin.Fd())) fmt.Fprintln(os.Stderr) if err != nil { return nil, fmt.Errorf("reading passphrase: %w", err) } - return ssh.ParsePrivateKeyWithPassphrase(data, passphrase) + return gossh.ParsePrivateKeyWithPassphrase(data, passphrase) } // dialWithContext bounds the SSH handshake by the context. The TCP dial is @@ -328,7 +329,7 @@ func parseEncryptedKey(data []byte, keyPath string) (ssh.Signer, error) { // context cancellation closes the underlying connection so a blocked // handshake unblocks immediately. The deadline is cleared once the // connection is established so the returned client is not time-limited. -func dialWithContext(ctx context.Context, network, addr string, config *ssh.ClientConfig) (*ssh.Client, error) { +func dialWithContext(ctx context.Context, network, addr string, config *gossh.ClientConfig) (*gossh.Client, error) { // The TCP dial itself is bounded even when the caller's context has no // deadline (Background): a black-holed address used to rely on the // OS-level connect timeout (~75s+) before the handshake deadline below @@ -347,7 +348,7 @@ func dialWithContext(ctx context.Context, network, addr string, config *ssh.Clie return nil, fmt.Errorf("setting handshake deadline: %w", err) } stopClose := context.AfterFunc(ctx, func() { _ = conn.Close() }) - c, chans, reqs, err := ssh.NewClientConn(conn, addr, config) + c, chans, reqs, err := gossh.NewClientConn(conn, addr, config) if !stopClose() { _ = conn.Close() return nil, ctx.Err() @@ -361,7 +362,7 @@ func dialWithContext(ctx context.Context, network, addr string, config *ssh.Clie _ = c.Close() return nil, fmt.Errorf("clearing handshake deadline: %w", err) } - return ssh.NewClient(c, chans, reqs), nil + return gossh.NewClient(c, chans, reqs), nil } // acceptNewHostKeyCallback returns a host key callback that accepts unknown @@ -372,7 +373,7 @@ func dialWithContext(ctx context.Context, network, addr string, config *ssh.Clie // was inconvenient" — the previous version treated a known_hosts parse // failure as "nothing is known" (accepting whatever key was presented) and // let knownhosts.RevokedError fall through the unknown-host branch. -func acceptNewHostKeyCallback(knownHostsPath string) ssh.HostKeyCallback { +func acceptNewHostKeyCallback(knownHostsPath string) gossh.HostKeyCallback { existing, existingErr := knownhosts.New(knownHostsPath) if existingErr != nil && errors.Is(existingErr, fs.ErrNotExist) { // A missing known_hosts is the fresh-box case: nothing is known, so @@ -380,7 +381,7 @@ func acceptNewHostKeyCallback(knownHostsPath string) ssh.HostKeyCallback { // but cannot be read or parsed fails closed below. existing, existingErr = nil, nil } - return func(hostname string, remote net.Addr, key ssh.PublicKey) error { + return func(hostname string, remote net.Addr, key gossh.PublicKey) error { if existingErr != nil { return fmt.Errorf("cannot verify host key: reading %s failed: %w", knownHostsPath, existingErr) } @@ -423,14 +424,53 @@ func acceptNewHostKeyCallback(knownHostsPath string) ssh.HostKeyCallback { } } -// PublicKeyPath returns the path to the SSH public key file. -// Checks KeyPath+".pub" first, then default locations. +// PublicKeyBytes returns the authorized-key line for the identity the +// caller will actually authenticate with (audit A32): for an explicit key +// path the public key is DERIVED from that private key, an existing .pub +// file is verified against it (a stale .pub used to silently provision a +// different identity), and there is no fallthrough to unrelated default +// keys. For an empty keyPath the first loadable default identity is used, +// matching resolveSigners' preference order. +func PublicKeyBytes(keyPath string) ([]byte, error) { + signers, err := resolveSigners(keyPath) + if err != nil { + return nil, err + } + if len(signers) == 0 { + return nil, fmt.Errorf("no SSH identity found for %q", keyPath) + } + derived := signers[0].PublicKey() + if keyPath != "" { + if raw, rerr := os.ReadFile(keyPath + ".pub"); rerr == nil { + pub, _, _, _, perr := gossh.ParseAuthorizedKey(raw) + if perr != nil { + return nil, fmt.Errorf("parsing %s.pub: %w", keyPath, perr) + } + if !bytes.Equal(pub.Marshal(), derived.Marshal()) { + return nil, fmt.Errorf("%s.pub does not match the private key %s — refusing to provision an unrelated identity", keyPath, keyPath) + } + } else if !errors.Is(rerr, fs.ErrNotExist) { + return nil, fmt.Errorf("reading %s.pub: %w", keyPath, rerr) + } + } + return gossh.MarshalAuthorizedKey(derived), nil +} + +// PublicKeyPath returns the path to the SSH public key file. With an +// EXPLICIT key path it returns that key's .pub only when the file exists +// and matches the private key — never an unrelated default (audit A32); +// derive one with PublicKeyBytes when the .pub is absent. func PublicKeyPath(keyPath string) (string, error) { if keyPath != "" { pub := keyPath + ".pub" if _, err := os.Stat(pub); err == nil { + // Verify the .pub against the private key before trusting it. + if _, derr := PublicKeyBytes(keyPath); derr != nil { + return "", derr + } return pub, nil } + return "", fmt.Errorf("no public key file at %s — one can be derived from the private key (see PublicKeyBytes)", pub) } home, err := os.UserHomeDir() if err != nil { From 8bd4b70c6faf807782209a7ea9e696ab9458147d Mon Sep 17 00:00:00 2001 From: Tyler <53561637+im-tyler@users.noreply.github.com> Date: Sat, 19 Sep 2026 02:01:58 -0700 Subject: [PATCH 7/9] =?UTF-8?q?fix(backup):=20A39+A40+A41+A44+A46=20?= =?UTF-8?q?=E2=80=94=20mandatory=20env=20recovery=20copy,=20proven=20AOF?= =?UTF-8?q?=20gate,=20post-stop=20snapshot=20+=20start=20compensation,=20c?= =?UTF-8?q?ollision-proof=20backup=20ids,=20unsigned=20cron=20grammar=20at?= =?UTF-8?q?=20the=20sink?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A39: the .env commit is a set -eu script staging BOTH files as private mktemp siblings on the destination filesystem — the old copy's 'cp -p' ran inside an if without chaining, so its failure was skipped and the destructive mv ran anyway, and the new env was published by a cross-filesystem mv from /tmp chmod'd only after it was live. A40: the redis restore's AOF preflight is a Go-level check requiring a proven 'appendonly no' reply — the old 'config get appendonly | tail -n 1' masked a failed docker exec (auth, transport) as empty output and fell through to the destructive RDB replacement; only a genuine substring 'yes' refused. The backup script's gate is strict for the same reason (set -eu aborts on the failed exec). A41: the redis restore snapshots the original dump AFTER the stop (a graceful shutdown writes a final RDB the old copy-before-stop could miss), the copy is mandatory when a dump exists, and a failed final docker start now invokes the same restore_original compensation as a failed install instead of exiting with redis down. A44: backup ids carry a random 16-hex suffix — two backups of one app in the same second used to target the same S3 key and silently replace each other. ValidateDate accepts both the legacy timestamp form and the new ids (with a real-date check); LatestBackupDate ordering still works since the timestamp remains the prefix. A46: cron fields must be unsigned decimals ('+1' and signed steps are rejected), and SetSchedule itself validates the schedule plus rejects line breaks/NUL in command and marker — a direct caller can no longer bypass validation or split one job into unintended crontab lines. --- internal/backup/backup.go | 159 +++++++++++++++++++----- internal/backup/backup_test.go | 148 ++++++++++++++++++++++ internal/backup/restore_staging_test.go | 24 +++- 3 files changed, 299 insertions(+), 32 deletions(-) diff --git a/internal/backup/backup.go b/internal/backup/backup.go index 9c229d3..82b212a 100644 --- a/internal/backup/backup.go +++ b/internal/backup/backup.go @@ -2,6 +2,8 @@ package backup import ( "context" + "crypto/rand" + "encoding/hex" "errors" "fmt" "io" @@ -42,10 +44,33 @@ func ValidateRegion(region string) error { return nil } -// ValidateDate checks that a date/timestamp string is safe for shell use (e.g. 20060102-150405). +// backupIDRE matches a backup identity: the historical timestamp form +// (20060102-150405) or the collision-proof form newBackupID writes +// (timestamp + 16 hex of randomness, audit A44) — two backups of the same +// app inside one second used to target the same S3 key, silently +// replacing each other. +var backupIDRE = regexp.MustCompile(`^[0-9]{8}-[0-9]{6}(-[0-9a-f]{16})?$`) + +// newBackupID mints a backup identity that is timestamp-ordered AND +// unique within the same second (A44). +func newBackupID(now time.Time) (string, error) { + var b [8]byte + if _, err := rand.Read(b[:]); err != nil { + return "", fmt.Errorf("generating backup id: %w", err) + } + return now.UTC().Format("20060102-150405") + "-" + hex.EncodeToString(b[:]), nil +} + +// ValidateDate checks that a date/timestamp backup identity is exactly the +// expected grammar (legacy timestamp or newBackupID's collision-proof +// form) and that its timestamp part parses — anything else is rejected +// before it reaches a shell or an S3 key. func ValidateDate(date string) error { - if !safeName.MatchString(date) || len(date) > 30 { - return fmt.Errorf("invalid date %q — expected format like 20060102-150405", date) + if !backupIDRE.MatchString(date) { + return fmt.Errorf("invalid backup id %q — expected format like 20060102-150405[-0123456789abcdef]", date) + } + if _, err := time.Parse("20060102-150405", date[:15]); err != nil { + return fmt.Errorf("invalid backup id %q — timestamp part is not a real date", date) } return nil } @@ -74,6 +99,9 @@ func ValidateSchedule(schedule string) error { for _, part := range strings.Split(field, ",") { bounds, step, hasStep := strings.Cut(part, "/") if hasStep { + if !unsignedDecimal(step) { + return fmt.Errorf("invalid cron schedule %q — step %q in %s is not a positive number", schedule, step, r.name) + } stepN, err := strconv.Atoi(step) if err != nil || stepN < 1 { return fmt.Errorf("invalid cron schedule %q — step %q in %s is not a positive number", schedule, step, r.name) @@ -105,12 +133,17 @@ func ValidateSchedule(schedule string) error { return nil } -// validCronValue accepts "*" or a bare number within range. Empty strings -// (e.g. from "1,,2") are rejected. +// validCronValue accepts "*" or a bare UNSIGNED number within range. +// Empty strings (e.g. from "1,,2") and signed forms like "+1" are +// rejected — strconv.Atoi accepted the plus sign, which is not the +// decimal-field grammar cron itself accepts (audit A46). func validCronValue(v string, min, max int) bool { if v == "*" { return true } + if !unsignedDecimal(v) { + return false + } n, err := strconv.Atoi(v) if err != nil { return false @@ -118,6 +151,19 @@ func validCronValue(v string, min, max int) bool { return n >= min && n <= max } +// unsignedDecimal reports whether s is one or more ASCII digits. +func unsignedDecimal(s string) bool { + if s == "" { + return false + } + for _, c := range s { + if c < '0' || c > '9' { + return false + } + } + return true +} + // S3Config holds S3 bucket and credentials info (stored on server). // // Endpoint (optional) points at an S3-compatible server instead of AWS — @@ -176,7 +222,10 @@ func (c *Client) BackupVolumes(ctx context.Context, app string, s3 S3Config) err return err } - timestamp := time.Now().UTC().Format("20060102-150405") + timestamp, err := newBackupID(time.Now()) + if err != nil { + return err + } volumesDir := fmt.Sprintf("%s/%s/volumes", deploymentsDir, app) appDir := fmt.Sprintf("%s/%s", deploymentsDir, app) @@ -305,12 +354,29 @@ func (c *Client) RestoreVolumes(ctx context.Context, app, date string, s3 S3Conf // Install the backed-up .env (if the archive carried one — older // backups and volumes-only schedules don't) while keeping the previous // file recoverable. - envCmd := fmt.Sprintf( - "if [ -f %s ]; then if [ -f %s ]; then cp -p %s %s; fi; mv %s %s && chmod 600 %s && echo 'Restored app .env'; fi", - ssh.ShellQuote(stagedEnv), - ssh.ShellQuote(envPath), ssh.ShellQuote(envPath), ssh.ShellQuote(envPath+".pre-restore"), - ssh.ShellQuote(stagedEnv), ssh.ShellQuote(envPath), ssh.ShellQuote(envPath), - ) + // The env commit runs as a set -eu script (audit A39): the old .env's + // recovery copy is MANDATORY when one exists (the old `cp -p` inside an + // if-without-chaining was skipped on failure and the destructive mv ran + // anyway), and both files are staged as private siblings ON THE + // DESTINATION FILESYSTEM (mktemp in the app dir) so publication is an + // atomic same-filesystem rename with 0600 applied BEFORE the file is + // live — not a cross-filesystem mv from /tmp chmod'd after the fact. + envCmd := strings.Join([]string{ + "set -eu", + fmt.Sprintf("if [ -f %s ]; then", ssh.ShellQuote(stagedEnv)), + fmt.Sprintf(" if [ -f %s ]; then", ssh.ShellQuote(envPath)), + ` old=$(mktemp ` + ssh.ShellQuote(appDir+"/.env-old.XXXXXXXX") + `)`, + fmt.Sprintf(` cat %s > "$old"`, ssh.ShellQuote(envPath)), + ` chmod 600 "$old"`, + fmt.Sprintf(` mv -fT -- "$old" %s`, ssh.ShellQuote(envPath+".pre-restore")), + " fi", + ` new=$(mktemp ` + ssh.ShellQuote(appDir+"/.env-new.XXXXXXXX") + `)`, + fmt.Sprintf(` cat %s > "$new"`, ssh.ShellQuote(stagedEnv)), + ` chmod 600 "$new"`, + fmt.Sprintf(` mv -fT -- "$new" %s`, ssh.ShellQuote(envPath)), + " echo 'Restored app .env'", + "fi", + }, "\n") if out, err := c.exec.Run(ctx, envCmd); err != nil { // Env commit failed: keep the recovery dir + run dir so the mixed // state is manually recoverable, and say exactly that. @@ -470,7 +536,10 @@ func (c *Client) AccessoryBackup(ctx context.Context, app, name, image string, e return err } - timestamp := time.Now().UTC().Format("20060102-150405") + timestamp, err := newBackupID(time.Now()) + if err != nil { + return err + } containerName := app + "-" + name qContainer := ssh.ShellQuote(containerName) @@ -534,8 +603,14 @@ func (c *Client) AccessoryBackup(ctx context.Context, app, name, image string, e // up only dump.rdb captures a stale or empty dataset. Fail // closed rather than uploading a wrong-point-in-time artifact // (TCL-42). - fmt.Sprintf("aof=$(docker exec %s redis-cli config get appendonly | tail -n 1)", qContainer), - `case "$aof" in *yes*) echo 'redis appendonly is enabled; teploy backup captures dump.rdb only — disable AOF or use an engine-level backup' >&2; exit 1;; esac`, + // AOF gate (A40): the old `config get appendonly | tail -n 1` + // pipeline masked a failed docker exec (empty output fell + // through as "not yes") and only refused on a substring + // match. The reply must be a proven `appendonly no` — + // anything else (auth error, empty, unexpected) refuses. + fmt.Sprintf("aof=$(docker exec %s redis-cli --raw config get appendonly)", qContainer), + `set -- $aof`, + `if [ "${1:-}" != appendonly ] || [ "${2:-}" != no ]; then echo 'cannot confirm redis appendonly=no (got: '"$aof"') — teploy backup captures dump.rdb only; refusing' >&2; exit 1; fi`, fmt.Sprintf("ls=$(docker exec %s redis-cli lastsave)", qContainer), fmt.Sprintf("bgs=$(docker exec %s redis-cli bgsave 2>&1) || true", qContainer), `case "$bgs" in *ERR*) case "$bgs" in *"in progress"*) ;; *) printf 'redis BGSAVE failed: %s\n' "$bgs" >&2; exit 1;; esac;; esac`, @@ -662,29 +737,48 @@ func (c *Client) AccessoryRestore(ctx context.Context, app, name, image, date st restorePath = tmpdir + "/restore.rdb.gz" rdbPath := tmpdir + "/restore.rdb" oldRdb := tmpdir + "/old-dump.rdb" + // A40: the AOF gate is a Go-level preflight so a failed or + // unexpected reply (auth error, empty output, anything but a + // proven `appendonly no`) refuses BEFORE any stop or copy — the + // old `config get appendonly | tail -n 1` pipeline masked a failed + // docker exec as "not yes" and fell through to the destructive + // replacement. ("Not proven yes" is not "proven no".) + aofOut, aofErr := c.exec.Run(ctx, fmt.Sprintf("docker exec %s redis-cli --raw config get appendonly", qContainer)) + if aofErr != nil { + return keepTmp(fmt.Errorf("cannot establish the Redis persistence mode for %s: %w", containerName, aofErr)) + } + if aofFields := strings.Fields(aofOut); len(aofFields) != 2 || aofFields[0] != "appendonly" || aofFields[1] != "no" { + return keepTmp(fmt.Errorf("cannot confirm appendonly=no for %s (got %q) — an AOF-enabled Redis would load the append-only file on restart and teploy's dump.rdb restore would be a no-op; an explicit restore plan is required", containerName, strings.TrimSpace(aofOut))) + } + // A41 ordering: the previous dump is snapshotted AFTER the stop — + // a graceful redis shutdown writes a final RDB, and the old + // copy-before-stop could miss data present at shutdown, making the + // "recovery copy" older than the state it claims to recover. The + // pre-stop `docker exec test` only records WHETHER a dump exists; + // the copy itself runs on the stopped container (docker cp works + // stopped) and its failure aborts before anything is modified. A + // failed final `docker start` now also puts the original dump back + // and retries the start (the old script exited without either). restoreCmd = strings.Join([]string{ "set -eu", fmt.Sprintf("gunzip -c %s > %s", ssh.ShellQuote(restorePath), ssh.ShellQuote(rdbPath)), - // AOF-enabled Redis loads the append-only file on restart, so - // replacing only dump.rdb restores nothing — refuse instead of - // reporting success over a stale dataset (TCL-42). - fmt.Sprintf("aof=$(docker exec %s redis-cli config get appendonly | tail -n 1)", qContainer), - `case "$aof" in *yes*) echo 'redis appendonly is enabled; teploy restore replaces dump.rdb only and Redis would load AOF on restart — an explicit restore plan is required' >&2; exit 1;; esac`, - // Save the current dump ONLY when one exists, and make the - // copy MANDATORY when it does: `docker cp … || true` masked a - // failed recovery copy, leaving no way back after the stop - // below (TCL-42). - fmt.Sprintf("if docker exec %s test -f /data/dump.rdb; then docker cp %s:/data/dump.rdb %s; fi", qContainer, qContainer, ssh.ShellQuote(oldRdb)), + "had=no", + fmt.Sprintf("if docker exec %s test -f /data/dump.rdb 2>/dev/null; then had=yes; fi", qContainer), fmt.Sprintf("docker stop %s", qContainer), + fmt.Sprintf(`if [ "$had" = yes ]; then docker cp %s:/data/dump.rdb %s; fi`, qContainer, ssh.ShellQuote(oldRdb)), "ok=yes", fmt.Sprintf("docker cp %s %s:/data/dump.rdb || ok=no", ssh.ShellQuote(rdbPath), qContainer), + fmt.Sprintf(`restore_original() { if [ "$had" = yes ] && [ -f %s ]; then docker cp %s %s:/data/dump.rdb || true; fi; docker start %s || true; }`, ssh.ShellQuote(oldRdb), ssh.ShellQuote(oldRdb), qContainer, qContainer), `if [ "$ok" != yes ]; then`, - fmt.Sprintf(" if [ -f %s ]; then docker cp %s %s:/data/dump.rdb || true; fi", ssh.ShellQuote(oldRdb), ssh.ShellQuote(oldRdb), qContainer), - fmt.Sprintf(" docker start %s || true", qContainer), + ` restore_original`, " echo 'redis restore failed after stopping the container; the original dump was restored when available' >&2", " exit 1", "fi", - fmt.Sprintf("docker start %s", qContainer), + fmt.Sprintf("if ! docker start %s; then", qContainer), + ` restore_original`, + " echo 'redis container failed to start after the restore; the original dump was put back — verify the accessory' >&2", + " exit 1", + "fi", fmt.Sprintf("rm -f %s", ssh.ShellQuote(rdbPath)), }, "\n") default: @@ -735,6 +829,15 @@ func (c *Client) AccessoryRestore(ctx context.Context, app, name, image, date st // a stable per-target tag (e.g. "teploy-backup:") appended as a trailing // comment so the entry can be found and replaced on reschedule. func (c *Client) SetSchedule(ctx context.Context, schedule, command, marker string) error { + // The sink enforces the grammar itself (A46): direct callers used to be + // able to bypass ValidateSchedule, and a command or marker containing a + // line break would silently install multiple unintended crontab lines. + if err := ValidateSchedule(schedule); err != nil { + return err + } + if strings.ContainsAny(command+marker, "\r\n\x00") { + return fmt.Errorf("cron command and marker must each be a single line") + } // Dedup on the marker with grep -vF (fixed string). The previous grep -v // matched the whole COMMAND as a regex — backup commands are full of regex // metacharacters (. * $ ( ) /), so the dedup matched the wrong lines or none diff --git a/internal/backup/backup_test.go b/internal/backup/backup_test.go index 9aed866..578aef4 100644 --- a/internal/backup/backup_test.go +++ b/internal/backup/backup_test.go @@ -5,11 +5,13 @@ import ( "context" "errors" "fmt" + "io" "os" "os/exec" "path/filepath" "strings" "testing" + "time" "github.com/useteploy/teploy/internal/ssh" ) @@ -763,3 +765,149 @@ esac t.Errorf("canonical no-crontab: abort=%v installed=%v (want clean first install)", err, installed) } } + +// TestNewBackupID_UniqueWithinSameSecond is the A44 regression: two ids +// minted in the same second differ, and both parse under ValidateDate. +func TestNewBackupID_UniqueWithinSameSecond(t *testing.T) { + now := time.Date(2026, 9, 19, 1, 2, 3, 0, time.UTC) + a, err := newBackupID(now) + if err != nil { + t.Fatal(err) + } + b, err := newBackupID(now) + if err != nil { + t.Fatal(err) + } + if a == b { + t.Fatalf("ids minted in the same second collided: %s", a) + } + for _, id := range []string{a, b} { + if err := ValidateDate(id); err != nil { + t.Errorf("ValidateDate(%q): %v", id, err) + } + } + // Legacy ids remain valid (restore/list of old backups). + if err := ValidateDate("20260101-000000"); err != nil { + t.Errorf("legacy id rejected: %v", err) + } + for _, bad := range []string{"20261301-000000", "20260101-000000-extra", "20260101-000000-", "20260101-000000-ZZZZZZZZZZZZZZZZ"} { + if err := ValidateDate(bad); err == nil { + t.Errorf("ValidateDate(%q) accepted", bad) + } + } +} + +// TestValidateSchedule_RejectsSignedValues is the A46 regression: cron +// fields are unsigned decimals — "+1" and negative steps are rejected. +func TestValidateSchedule_RejectsSignedValues(t *testing.T) { + for _, s := range []string{"+1 * * * *", "*/+5 * * * *", "-1 * * * *"} { + if err := ValidateSchedule(s); err == nil { + t.Errorf("ValidateSchedule(%q) accepted a signed value", s) + } + } +} + +// TestSetSchedule_EnforcesGrammarAtSink is the A46 regression: the sink +// itself validates the schedule and rejects line breaks in the command or +// marker — direct callers cannot bypass validation or split one job into +// several crontab lines. +func TestSetSchedule_EnforcesGrammarAtSink(t *testing.T) { + mock := ssh.NewMockExecutor("1.2.3.4") + client := NewClient(mock, io.Discard) + if err := client.SetSchedule(context.Background(), "99 * * * *", "cmd", "tag"); err == nil { + t.Error("an out-of-range schedule must be rejected at the sink") + } + if err := client.SetSchedule(context.Background(), "* * * * *", "echo one\necho two", "tag"); err == nil { + t.Error("a multi-line command must be rejected") + } + if err := client.SetSchedule(context.Background(), "* * * * *", "cmd", "tag\nother"); err == nil { + t.Error("a multi-line marker must be rejected") + } + for _, c := range mock.Calls { + if strings.Contains(c, "crontab -") { + t.Errorf("a rejected schedule must not touch the crontab: %s", c) + } + } +} + +// TestRedisRestore_AOFRequiresProvenNo is the A40 regression: an AOF +// preflight that errors (auth, transport) or replies unexpectedly must +// refuse BEFORE any stop/copy — "not proven yes" is not "proven no". +func TestRedisRestore_AOFRequiresProvenNo(t *testing.T) { + for _, tc := range []struct { + name, aofOut string + aofErr error + }{ + {"auth failure", "", fmt.Errorf("NOAUTH Authentication required")}, + {"empty reply", "", nil}, + {"unexpected reply", "WRONGTYPE Operation against a key", nil}, + {"yes is refused", "appendonly yes", nil}, + } { + t.Run(tc.name, func(t *testing.T) { + mock := ssh.NewMockExecutor("1.2.3.4", + ssh.MockCommand{Match: "which aws", Output: "/usr/bin/aws\n"}, + ssh.MockCommand{Match: "aws s3 cp", Output: "ok"}, + ssh.MockCommand{Match: "mktemp -d", Output: "/tmp/teploy-restore.abc\n"}, + ssh.MockCommand{Match: "gunzip -c", Output: ""}, + ssh.MockCommand{Match: "redis-cli --raw config get appendonly", Output: tc.aofOut, Err: tc.aofErr}, + ssh.MockCommand{Match: "docker stop", Output: ""}, + ssh.MockCommand{Match: "docker cp", Output: ""}, + ) + client := NewClient(mock, io.Discard) + err := client.AccessoryRestore(context.Background(), "myapp", "cache", "redis:7", "20260101-000000", nil, S3Config{Bucket: "b", Region: "us-east-1"}) + if err == nil { + t.Fatal("expected the restore to refuse") + } + for _, c := range mock.Calls { + if strings.HasPrefix(c, "docker stop") { + t.Errorf("an unproven AOF state must abort before stopping redis: %s", c) + } + } + }) + } +} + +// TestRedisRestore_SnapshotsAfterStopAndCompensatesStartFailure is the +// A41 regression: the restore script snapshots the original dump AFTER the +// stop (so the shutdown save is included) and defines a restore_original +// compensation invoked from BOTH failure branches — a failed install and a +// failed final docker start. +func TestRedisRestore_SnapshotsAfterStopAndCompensatesStartFailure(t *testing.T) { + mock := ssh.NewMockExecutor("1.2.3.4", + ssh.MockCommand{Match: "which aws", Output: "/usr/bin/aws\n"}, + ssh.MockCommand{Match: "aws s3 cp", Output: "ok"}, + ssh.MockCommand{Match: "mktemp -d", Output: "/tmp/teploy-restore.abc\n"}, + ssh.MockCommand{Match: "docker exec 'myapp-cache' redis-cli --raw config get appendonly", Output: "appendonly no"}, + ssh.MockCommand{Match: "set -eu", Output: ""}, + ) + client := NewClient(mock, io.Discard) + if err := client.AccessoryRestore(context.Background(), "myapp", "cache", "redis:7", "20260101-000000", nil, S3Config{Bucket: "b", Region: "us-east-1"}); err != nil { + t.Fatalf("AccessoryRestore: %v", err) + } + var script string + for _, c := range mock.Calls { + if strings.HasPrefix(c, "set -eu") { + script = c + } + } + if script == "" { + t.Fatal("the redis restore script never ran") + } + stopIdx := strings.Index(script, "docker stop 'myapp-cache'") + snapIdx := strings.Index(script, "docker cp 'myapp-cache':/data/dump.rdb") + if stopIdx < 0 || snapIdx < 0 || snapIdx < stopIdx { + t.Errorf("the original dump must be snapshotted AFTER the stop (stop=%d snapshot=%d)", stopIdx, snapIdx) + } + compIdx := strings.Index(script, "restore_original() {") + if compIdx < 0 { + t.Fatal("the script must define the restore_original compensation") + } + // Both failure branches invoke it. + branches := strings.Count(script, "\trestore_original") + strings.Count(script, " restore_original") + if branches < 2 { + t.Errorf("both the failed-install and failed-start branches must compensate (found %d): %q", branches, script) + } + if !strings.Contains(script, "if ! docker start 'myapp-cache'; then") { + t.Error("a failed final docker start must be a handled branch") + } +} diff --git a/internal/backup/restore_staging_test.go b/internal/backup/restore_staging_test.go index 33a40d2..11ee77e 100644 --- a/internal/backup/restore_staging_test.go +++ b/internal/backup/restore_staging_test.go @@ -26,6 +26,8 @@ func TestRestoreVolumes_StagesBeforePromoting(t *testing.T) { ssh.MockCommand{Match: "find ", Output: ""}, ssh.MockCommand{Match: "cp -a ", Output: ""}, ssh.MockCommand{Match: "if [ -f", Output: ""}, + // A39's env commit is a set -eu script. + ssh.MockCommand{Match: "set -eu", Output: "Restored app .env\n"}, ) var buf bytes.Buffer @@ -104,11 +106,23 @@ func TestRestoreVolumes_StagesBeforePromoting(t *testing.T) { if envInstall == "" { t.Fatalf("expected an .env install step, got calls: %v", mock.Calls) } - if !strings.Contains(envInstall, "mv '"+runDir+"/new.env' '/deployments/myapp/.env'") { - t.Errorf(".env must be installed beside volumes/ in the app directory, got: %s", envInstall) + // A39: the new env is staged as a private sibling on the DESTINATION + // filesystem (never a cross-filesystem mv from /tmp), secured at 0600 + // before publication, and the old file's recovery copy is mandatory. + if !strings.Contains(envInstall, "mktemp '/deployments/myapp/.env-new.") { + t.Errorf(".env must be staged beside the destination on the same filesystem, got: %s", envInstall) } - if !strings.Contains(envInstall, ".env.pre-restore") || !strings.Contains(envInstall, "chmod 600") { - t.Errorf(".env install must keep the previous file recoverable and restrict permissions, got: %s", envInstall) + if !strings.Contains(envInstall, "chmod 600 \"$new\"") || !strings.Contains(envInstall, "chmod 600 \"$old\"") { + t.Errorf("both staged files must be secured before publication, got: %s", envInstall) + } + if !strings.Contains(envInstall, "mv -fT -- \"$new\" '/deployments/myapp/.env'") { + t.Errorf(".env publication must be the atomic rename of the staged sibling, got: %s", envInstall) + } + if !strings.Contains(envInstall, ".env-old.") || !strings.Contains(envInstall, ".env.pre-restore") { + t.Errorf("the old .env's recovery copy must be staged then renamed into place, got: %s", envInstall) + } + if !strings.Contains(envInstall, "set -eu") { + t.Errorf("the env commit must abort on the first failed step, got: %s", envInstall) } } @@ -281,6 +295,8 @@ func TestAccessoryRestore_RedisRestartsAfterCopyFailure(t *testing.T) { ssh.MockCommand{Match: "which aws", Output: "/usr/bin/aws\n"}, ssh.MockCommand{Match: "aws s3 cp", Output: "download: done\n"}, ssh.MockCommand{Match: "mktemp -d '/tmp/teploy-restore.XXXXXX'", Output: "/tmp/teploy-restore.abc123\n"}, + // A40's preflight must prove appendonly=no before the script runs. + ssh.MockCommand{Match: "docker exec 'myapp-redis' redis-cli --raw config get appendonly", Output: "appendonly no"}, ssh.MockCommand{Match: "set -eu", Err: fmt.Errorf("exit status 1: docker cp failed")}, ) From d2e2d76d7bf8e7831a72bbacf9c23eacac6d45a7 Mon Sep 17 00:00:00 2001 From: Tyler <53561637+im-tyler@users.noreply.github.com> Date: Sat, 19 Sep 2026 02:04:11 -0700 Subject: [PATCH 8/9] =?UTF-8?q?fix(multideploy,cli):=20A36+A51=20=E2=80=94?= =?UTF-8?q?=20serialized=20atomic=20dedup=20persistence,=20content-authori?= =?UTF-8?q?tative=20replay=20protection,=20race-free=20bounded=20prefix=20?= =?UTF-8?q?writers,=20cancellation-aware=20fleet=20slots?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A51: PrefixWriter serializes its buffer (one writer is routinely handed to BOTH stdout and stderr of the same command — docker.ScanImage does — so concurrent stream copies raced before the shared sink lock), caps the partial-line buffer at 64 KiB (newline-free output used to buffer without bound), and surfaces write/flush failures into the deploy result instead of discarding them. parallelDeploy acquires its concurrency slot with a ctx-aware select and re-checks cancellation after (possibly) waiting, so a cancelled fleet operation no longer queues for slots or launches callbacks the caller already abandoned. A36: dedup persistence snapshots and writes under one mutex, publishes atomically (temp + rename), and reports failures to the log — concurrent requests used to race os.WriteFile on the same path, an older snapshot could overwrite a newer one, and every error was ignored. The delivery-ID header no longer suppresses anything: it is log metadata only, because every legitimate provider retry replays the same signed body (caught by content dedup), while a REUSED delivery ID carrying different authenticated content is a distinct event the old ID-only check wrongly swallowed. --- internal/cli/autodeploy_serve.go | 45 +++++++---- internal/cli/autodeploy_serve_test.go | 36 +++++++++ internal/multideploy/multideploy.go | 97 +++++++++++++++++------ internal/multideploy/multideploy_test.go | 98 ++++++++++++++++++++++++ 4 files changed, 237 insertions(+), 39 deletions(-) diff --git a/internal/cli/autodeploy_serve.go b/internal/cli/autodeploy_serve.go index 561cdf6..189bf0d 100644 --- a/internal/cli/autodeploy_serve.go +++ b/internal/cli/autodeploy_serve.go @@ -10,6 +10,7 @@ import ( "net/http" "os" "strings" + "sync" "time" "github.com/spf13/cobra" @@ -104,14 +105,31 @@ func runAutoDeployServe(app, branch string, port int, strictEnv bool) error { fmt.Fprintf(out, "%s "+format+"\n", append([]any{time.Now().UTC().Format(time.RFC3339)}, args...)...) } + // Dedup persistence is serialized AND atomic (audit A36): two + // concurrent requests used to snapshot and os.WriteFile the same file + // independently, so an older snapshot could overwrite a newer one, + // overlapping writes could truncate, and every error was ignored. + var dedupMu sync.Mutex handler := newWebhookHandler(webhookHandlerConfig{ secret: secret, branch: branch, dedup: dedup, logf: logf, onDedupChanged: func() { - if snap, err := dedup.Snapshot(); err == nil { - _ = os.WriteFile(dedupPath, snap, 0600) + dedupMu.Lock() + defer dedupMu.Unlock() + snap, err := dedup.Snapshot() + if err != nil { + logf("could not snapshot webhook dedup state: %v", err) + return + } + tmp := dedupPath + ".tmp" + if err := os.WriteFile(tmp, snap, 0600); err != nil { + logf("could not persist webhook dedup state: %v", err) + return + } + if err := os.Rename(tmp, dedupPath); err != nil { + logf("could not publish webhook dedup state: %v", err) } }, trigger: func(changedFiles []string, filesKnown bool) { @@ -235,8 +253,11 @@ func newWebhookHandler(cfg webhookHandlerConfig) http.HandlerFunc { // Replay protection is keyed on the AUTHENTICATED CONTENT, not the // unauthenticated delivery-ID header: a captured signed body could // be replayed under a fresh (or absent) delivery ID and bypass an - // ID-only dedup (audit F41). The delivery ID is kept as a second - // key so provider retries of the same delivery are also no-ops. + // ID-only dedup (audit F41). Every legitimate provider retry + // replays the SAME signed body, so content dedup covers them all; + // the delivery ID is kept as log metadata only — a REUSED delivery + // ID carrying different authenticated content used to suppress a + // distinct event (audit A36). contentSum := sha256.Sum256(body) contentID := "content:" + hex.EncodeToString(contentSum[:]) if cfg.dedup.SeenAndRecord(contentID) { @@ -246,16 +267,6 @@ func newWebhookHandler(cfg webhookHandlerConfig) http.HandlerFunc { } return } - if deliveryID != "" && cfg.dedup.SeenAndRecord(deliveryID) { - // 200, not an error status — this is a provider retry/replay - // of a delivery we already handled, an intentional no-op, not - // a failure the provider should retry harder on. - w.WriteHeader(http.StatusOK) - if cfg.logf != nil { - cfg.logf("ignored replayed delivery %s", deliveryID) - } - return - } if cfg.onDedupChanged != nil { cfg.onDedupChanged() } @@ -273,7 +284,11 @@ func newWebhookHandler(cfg webhookHandlerConfig) http.HandlerFunc { w.WriteHeader(http.StatusOK) if cfg.logf != nil { - cfg.logf("accepted webhook, triggering deploy") + if deliveryID != "" { + cfg.logf("accepted webhook (delivery %s), triggering deploy", deliveryID) + } else { + cfg.logf("accepted webhook, triggering deploy") + } } if cfg.trigger != nil { // Parse the changed-file set from the push body for monorepo diff --git a/internal/cli/autodeploy_serve_test.go b/internal/cli/autodeploy_serve_test.go index e072975..ba3d5c8 100644 --- a/internal/cli/autodeploy_serve_test.go +++ b/internal/cli/autodeploy_serve_test.go @@ -295,3 +295,39 @@ func TestWebhookHandler_ContentReplayRejected(t *testing.T) { t.Errorf("trigger called %d times for one unique signed body, want 1", triggerCount) } } + +// TestWebhookHandler_ReusedDeliveryIDDifferentContentNotSuppressed is the +// A36 regression: the (unauthenticated) delivery-ID header must never +// suppress DISTINCT authenticated content — a provider reusing an ID with +// a different signed body is a new event, and only content dedup decides +// replays. +func TestWebhookHandler_ReusedDeliveryIDDifferentContentNotSuppressed(t *testing.T) { + secret := "s3cret" + body1 := []byte(`{"ref":"refs/heads/main","after":"aaaa"}`) + body2 := []byte(`{"ref":"refs/heads/main","after":"bbbb"}`) + triggerCount := 0 + handler := newWebhookHandler(webhookHandlerConfig{ + secret: secret, + dedup: autodeploy.NewDeliveryDedup(), + logf: func(string, ...any) {}, + trigger: func(_ []string, _ bool) { + triggerCount++ + }, + }) + post := func(body []byte) { + req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(string(body))) + req.Header.Set("X-Hub-Signature-256", githubSign(secret, body)) + req.Header.Set("X-GitHub-Delivery", "same-delivery-id") + handler(httptest.NewRecorder(), req) + } + post(body1) + post(body2) + if triggerCount != 2 { + t.Errorf("distinct authenticated content under a reused delivery ID must both deploy, got %d", triggerCount) + } + // The SAME content replays to a no-op regardless of the header. + post(body1) + if triggerCount != 2 { + t.Errorf("replayed content must be a no-op, got %d", triggerCount) + } +} diff --git a/internal/multideploy/multideploy.go b/internal/multideploy/multideploy.go index cc40356..ddefe2b 100644 --- a/internal/multideploy/multideploy.go +++ b/internal/multideploy/multideploy.go @@ -26,12 +26,24 @@ type Result struct { } // PrefixWriter wraps a writer to prefix each line with a server name. +// +// A single writer can be handed to BOTH stdout and stderr of one command +// (docker.ScanImage does exactly that), so concurrent stream copies race +// on the buffer unless writes are serialized — and a newline-free write +// of unbounded length used to grow the partial-line buffer without cap +// (audit A51). The buffer is mutex-protected and capped; a fragment that +// reaches the cap is flushed as its own prefixed line (mid-"line" prefix +// tradeoff documented here rather than buffering without bound). type PrefixWriter struct { + mu sync.Mutex prefix string w io.Writer buf []byte // partial line buffer } +// fragmentLimit caps a single buffered partial line. +const fragmentLimit = 64 << 10 + // NewPrefixWriter creates a writer that prefixes each line with the given string. func NewPrefixWriter(prefix string, w io.Writer) *PrefixWriter { return &PrefixWriter{ @@ -41,30 +53,41 @@ func NewPrefixWriter(prefix string, w io.Writer) *PrefixWriter { } func (pw *PrefixWriter) Write(p []byte) (n int, err error) { - pw.buf = append(pw.buf, p...) - for { - idx := bytes.IndexByte(pw.buf, '\n') - if idx < 0 { - break + pw.mu.Lock() + defer pw.mu.Unlock() + consumed := 0 + for consumed < len(p) { + room := fragmentLimit - len(pw.buf) + n := min(room, len(p)-consumed) + piece := p[consumed : consumed+n] + if i := bytes.IndexByte(piece, '\n'); i >= 0 { + piece = piece[:i+1] + n = i + 1 } - line := pw.buf[:idx+1] - _, err = fmt.Fprintf(pw.w, "%s%s", pw.prefix, line) - if err != nil { - return len(p), err + pw.buf = append(pw.buf, piece...) + consumed += n + if pw.buf[len(pw.buf)-1] == '\n' || len(pw.buf) >= fragmentLimit { + if _, werr := pw.w.Write(append([]byte(pw.prefix), pw.buf...)); werr != nil { + // Do not blindly replay a partially written fragment. + pw.buf = nil + return consumed, werr + } + pw.buf = nil } - pw.buf = pw.buf[idx+1:] } return len(p), nil } // Flush writes any remaining partial line in the buffer. func (pw *PrefixWriter) Flush() error { - if len(pw.buf) > 0 { - _, err := fmt.Fprintf(pw.w, "%s%s\n", pw.prefix, pw.buf) - pw.buf = nil - return err + pw.mu.Lock() + defer pw.mu.Unlock() + if len(pw.buf) == 0 { + return nil } - return nil + _, err := pw.w.Write(append(append([]byte(pw.prefix), pw.buf...), '\n')) + pw.buf = nil + return err } // syncWriter wraps an io.Writer with a mutex for concurrent safety. @@ -117,17 +140,29 @@ func parallelDeploy(ctx context.Context, servers []ServerTarget, parallel int, f failed := false for i, server := range servers { - wg.Add(1) - sem <- struct{}{} // acquire semaphore (blocks until a slot is free) + // Acquire with cancellation (A51): a cancelled fleet operation used + // to keep waiting for slots and launch callbacks after the caller + // had already given up. The slot is taken BEFORE the goroutine (and + // the fail-fast skip) so bookkeeping stays balanced on every path. + select { + case sem <- struct{}{}: + case <-ctx.Done(): + results[i] = Result{ + Server: server.Name, + Success: false, + Error: fmt.Errorf("skipped: %w", ctx.Err()), + } + continue + } // Fail-fast: if a previous deploy failed, skip the rest. Disabled for // best-effort (rollback) runs, which must attempt every server. if failFast { mu.Lock() - if failed { - mu.Unlock() + f := failed + mu.Unlock() + if f { <-sem - wg.Done() results[i] = Result{ Server: server.Name, Success: false, @@ -135,15 +170,31 @@ func parallelDeploy(ctx context.Context, servers []ServerTarget, parallel int, f } continue } - mu.Unlock() } + // Re-check cancellation after (possibly) waiting for the slot. + if err := ctx.Err(); err != nil { + <-sem + results[i] = Result{ + Server: server.Name, + Success: false, + Error: fmt.Errorf("skipped: %w", err), + } + continue + } + + wg.Add(1) go func(idx int, srv ServerTarget) { defer wg.Done() + defer func() { <-sem }() pw := NewPrefixWriter(fmt.Sprintf("[%s] ", srv.Name), sw) err := deployFn(ctx, srv, pw) - pw.Flush() + // Flush failure is part of the outcome — output the fleet + // operator sees must not silently stop mid-line (A51). + if ferr := pw.Flush(); err == nil && ferr != nil { + err = ferr + } results[idx] = Result{ Server: srv.Name, @@ -156,8 +207,6 @@ func parallelDeploy(ctx context.Context, servers []ServerTarget, parallel int, f failed = true mu.Unlock() } - - <-sem // release semaphore after setting failed flag }(i, server) } diff --git a/internal/multideploy/multideploy_test.go b/internal/multideploy/multideploy_test.go index c4e6308..13b486b 100644 --- a/internal/multideploy/multideploy_test.go +++ b/internal/multideploy/multideploy_test.go @@ -3,6 +3,7 @@ package multideploy import ( "bytes" "context" + "errors" "fmt" "io" "strings" @@ -284,3 +285,100 @@ func TestFormatResults(t *testing.T) { t.Errorf("expected 'app3: skipped' in output: %s", output) } } + +// TestPrefixWriter_ConcurrentWritesAreLineSafe is the A51 regression: two +// concurrent writers into ONE PrefixWriter (stdout+stderr of the same +// command) must not interleave partial lines, and every flushed line +// carries the prefix. +func TestPrefixWriter_ConcurrentWritesAreLineSafe(t *testing.T) { + var sink lockedBuffer + pw := NewPrefixWriter("[srv] ", &sink) + var wg sync.WaitGroup + for g := 0; g < 8; g++ { + wg.Add(1) + go func() { + defer wg.Done() + for i := 0; i < 200; i++ { + if _, err := pw.Write([]byte("line-of-output\n")); err != nil { + t.Errorf("write: %v", err) + } + } + }() + } + wg.Wait() + if err := pw.Flush(); err != nil { + t.Fatalf("flush: %v", err) + } + for _, line := range strings.Split(strings.TrimSuffix(sink.String(), "\n"), "\n") { + if line != "[srv] line-of-output" { + t.Fatalf("interleaved or unprefixed line: %q", line) + } + } + if n := strings.Count(sink.String(), "line-of-output"); n != 8*200 { + t.Fatalf("lost output: got %d lines want %d", n, 8*200) + } +} + +type lockedBuffer struct { + mu sync.Mutex + buf bytes.Buffer +} + +func (b *lockedBuffer) Write(p []byte) (int, error) { + b.mu.Lock() + defer b.mu.Unlock() + return b.buf.Write(p) +} + +func (b *lockedBuffer) String() string { + b.mu.Lock() + defer b.mu.Unlock() + return b.buf.String() +} + +// TestPrefixWriter_BoundsUn newlineFreeOutput: a newline-free write larger +// than the fragment cap is flushed in capped pieces instead of buffering +// without bound (A51). +func TestPrefixWriter_BoundsNewlineFreeOutput(t *testing.T) { + var sink bytes.Buffer + pw := NewPrefixWriter("[srv] ", &sink) + big := strings.Repeat("x", 200<<10) + if _, err := pw.Write([]byte(big)); err != nil { + t.Fatalf("write: %v", err) + } + // The buffer must never hold more than the cap. + pw.mu.Lock() + buffered := len(pw.buf) + pw.mu.Unlock() + if buffered > 64<<10 { + t.Errorf("partial-line buffer exceeded the cap: %d", buffered) + } + if err := pw.Flush(); err != nil { + t.Fatalf("flush: %v", err) + } + if got := sink.Len(); got < 200<<10 { + t.Errorf("output was lost: %d bytes written of %d", got, 200<<10) + } +} + +// TestParallelDeploy_CancelledContextSkipsWaitingServers is the A51 +// regression: a cancelled fleet operation must not wait for concurrency +// slots or launch callbacks — every server reports a context skip. +func TestParallelDeploy_CancelledContextSkipsWaitingServers(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() // cancelled before the fleet even starts + var launched atomic.Int32 + targets := []ServerTarget{{Name: "a"}, {Name: "b"}, {Name: "c"}} + results := ParallelDeploy(ctx, targets, 2, func(ctx context.Context, srv ServerTarget, out io.Writer) error { + launched.Add(1) + return nil + }, io.Discard) + if got := launched.Load(); got != 0 { + t.Errorf("a cancelled fleet launched %d callbacks", got) + } + for _, r := range results { + if r.Success || !errors.Is(r.Error, context.Canceled) { + t.Errorf("server %s should report a context skip, got %+v", r.Server, r) + } + } +} From 2bfdd0bde3b81d9ca0d3d66e9acb4d896e581d60 Mon Sep 17 00:00:00 2001 From: Tyler <53561637+im-tyler@users.noreply.github.com> Date: Sat, 19 Sep 2026 02:05:17 -0700 Subject: [PATCH 9/9] =?UTF-8?q?docs(audit):=20round=203=20(A01-A52)=20reco?= =?UTF-8?q?rded=20=E2=80=94=2028=20contained=20fixes,=2024=20standing=20de?= =?UTF-8?q?ferrals=20with=20evidence=20folded=20in?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- AUDIT_OPEN.md | 114 ++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 111 insertions(+), 3 deletions(-) diff --git a/AUDIT_OPEN.md b/AUDIT_OPEN.md index c2ef0ba..495c229 100644 --- a/AUDIT_OPEN.md +++ b/AUDIT_OPEN.md @@ -18,9 +18,11 @@ bottom section: attempt-scoped artifacts, fenced locks, structured Caddy routes, strict-env — folding in round-2's TCL-04/TCL-05/TCL-24/TCL-32/ TCL-51), the round-2 residual tail itemized in that section, and the dependent designs F04 (and its TCL-10/TCL-15 dependents) that stay -deferred with their annotations. The 2 upstream/owner items are closed -(below). The two upstream items received from teploy-dash's 2026-09-17 -pass are closed below. +deferred with their annotations — plus the round-3 deferrals recorded at +the bottom (mostly the same architectural tail, with round-3 evidence +folded in). The 2 upstream/owner items are closed (below). The two +upstream items received from teploy-dash's 2026-09-17 pass are closed +below. ## Resolved from this register @@ -521,3 +523,109 @@ the fence before artifact generation) plus `DeployFenced`'s lock-handle parameter are the seam a generation handoff grows from. F45 can now build on ParseSites/ExtractPolicy. TCL-15's port allocation remains independent (the F14 record carries the resolved allocation). + +## Round 3 (2026-09-19, A01-A52, pinned at 0faf201) — record + +Report reviewed finding-by-finding against the source at the pinned HEAD +(the register's closure claims were NOT taken as proof — several findings +genuinely landed as new defects on top of the F08/F16 work, and several +restated the standing architectural tail). 28 findings closed with +contained fixes across 8 commits; the remaining 24 defer onto the +standing pass-6/round-2 tail (evidence folded in) or onto the new items +noted below. No false positives found; A21 and A17 were scoped to their +legacy-fallback/boundary remainders (the record-driven TCL-14 paths +already fixed the primary behavior). + +### Round 3 — fixed (contained) + +| ID | Sev | Where | +|---|-----|-------| +| A01 | High | 4537b89 — attempt TLS paths are app-scoped (/deployments/caddy/tls/att//./); PruneAttempts sweeps only the pruning app's artifact and TLS roots; the legacy flat TLS root is never swept (a flat entry's owner cannot be proven — the flat sweep deleted OTHER apps' live certs whenever hashes differed) | +| A02 | High | 4537b89 — the attempt-prune protection window covers every release that still has containers plus current/previous/pins (keep_versions retention holds releases whose records/routes reference attempt-scoped TLS and env); inventory failure skips the prune | +| A03 | High | 4537b89 — an unreadable pin file SKIPS attempt pruning entirely (version-prune parity) instead of pruning with current+previous only | +| A04 | High | dd8a788 — ReleaseLockFenced runs the release under the holdership guard: after a takeover the stale holder's rm -rf is refused (fence-lost = success, the successor keeps its lock); nil handles keep the admin/unfenced release | +| A06 | High | dd8a788 — WriteFenced and renewal stage to unique owner-scoped siblings (state.json.tmp--), so a stale holder cannot clobber the successor's staging and ride its guarded rename into authority; renewal stages in the app dir, never recreating a removed .lock | +| A08 | High | 89625d3 — the same-version path refuses to force-remove a RUNNING _replaced container (the failed prior attempt's renamed SERVING workload) and aborts on unclassified rename failures with the source still present | +| A10 | High | 89625d3 — abortStateCommit's fixed-port branch restores the Caddy route for caddy+publish apps (it used to return with Caddy pointing at the removed candidate names); on route-restore failure the candidates are restarted rather than routing to nothing | +| A11 | High | 89625d3 — every abortStateCommit compensation runs on a detached bounded recovery context (a cancelled-context commit failure used to skip the stops/restarts via the dead ctx) | +| A13 | Med | 89625d3 — restoreDisplacedAndStarted itemizes every failed stop/remove/restart in output and error; 'restored' is false when ANY displaced restart fails (was: true whenever zero candidates had started) | +| A14 | Med | 2faab70 — a failed docker run reconciles the candidate name (created-but-unstarted corpse removed so the next deploy cannot collide; a RUNNING container under the name is never touched) | +| A15 | High | 2faab70 — asset bridging builds the attempt's private tree (meta/att/./assets, seeded from the previous attempt with a real cp -a), extracts via docker create + docker cp (no image ENTRYPOINT runs), clones the volumes map before adding the mount — the shared live tree is never mutated pre-commit | +| A17 | Med | 2faab70 — Config.validate checks identity grammar (app/version/process), the ingress enum, and rejects publish+replicas>1; releasemeta.Path validates the app grammar; SplitHostPort rejects ports outside 1..65535; WriteFenced/ReleaseLockFenced verify the lease belongs to the app | +| A18 | Med | 2faab70 — ContainerPort==0 normalizes to 80 once at the top of DeployFenced and drives host ports, upstreams, diagnosis, and every create (no ':0' upstream) | +| A19 | Med | 784c955 — the primary -p binding is built by publishBinding (IP-validated, JoinHostPort-bracketed, port-ranged) and quoted — '::1:49152:80' concatenation is gone | +| A21 | Med | 784c955 — HostPort/InternalPort refuse containers with multiple DISTINCT ports instead of taking the first field (the release record's TCL-14 primary remains the authority); HostBindIP reports '' on mixed binds | +| A23 | Med | 2faab70 — workers must still be running (not exited/dead/restarting/unhealthy) one second after the detached run or the deploy fails with full cleanup; unreadable state inspects degrade to a warning | +| A25 | Med | 784c955 — PruneVersions counts a version pruned only when every container removal succeeded and returns the joined failures; the caller reports partial cleanup | +| A26 | Med | 89625d3 — logDeploy populates LogEntry.Image (closes TCL-19's open half) | +| A27 | High | 3b6025d — LocalExecutor.Upload is atomic (private sibling, chmod+fsync before publication, rename replacing a leaf symlink itself); the resident autodeploy path no longer runs on the un-hardened writer | +| A28 | High | 3b6025d — local commands run in their own process group with Cancel SIGKILLing the group and WaitDelay bounding the wait (descendants keeping pipes open used to block CombinedOutput indefinitely); non-unix fallback is explicit about the weaker guarantee | +| A32 | Med | 3b6025d — PublicKeyBytes derives the provisioning key from the requested private identity and verifies an existing .pub; PublicKeyPath no longer falls through to unrelated defaults for an explicit key; setup uses the derived key | +| A36 | Med | d2e2d76 — dedup persistence is serialized + atomic + error-reporting; the delivery-ID header is log metadata only (reused ID with different authenticated content no longer suppresses a distinct event) | +| A39 | High | 8bd4b70 — the .env commit is a set -eu script with a MANDATORY old-file recovery copy and both files staged as private same-filesystem siblings secured at 0600 before publication | +| A40 | High | 8bd4b70 — the redis restore AOF gate is a Go-level preflight requiring a proven 'appendonly no' reply (auth/transport/empty/unexpected all refuse before any stop or copy); the backup script's gate is strict under set -eu | +| A41 | High | 8bd4b70 — the redis restore snapshots the original dump AFTER the stop (the shutdown save is included), the copy is mandatory, and a failed final docker start invokes the same restore_original compensation as a failed install | +| A44 | Med | 8bd4b70 — backup ids carry a random 16-hex suffix (same-second S3 key collisions gone); ValidateDate accepts legacy and new ids with a real-date check; ordering preserved by the timestamp prefix | +| A46 | Med | 8bd4b70 — cron fields must be unsigned decimals; SetSchedule validates the schedule and rejects line breaks/NUL in command and marker at the sink | +| A51 | Med | d2e2d76 — PrefixWriter is mutex-protected with a 64 KiB fragment cap and surfaces write/flush failures; fleet slot acquisition is ctx-aware with a post-acquire re-check | +| A52 | Med | 2faab70 — DeployFenced resolves the immutable image ID once and creates every web/worker (and extracts assets) from it; the requested ref stays the recorded provenance; resolution failure warns and falls back | + +Gates at the closing commits: `go vet ./...` clean; `go test ./... -race` +all packages ok. No push performed. + +### Round 3 — deferred (standing tail, with round-3 evidence folded in) + +- A05 — F16's remainder: a permanent server-side flock serialization of + every lock transition and guarded effect (two contenders can still both + read a stale owner; `grep owner; effect` is atomic per command but the + multi-command phases are not). The landed owner-token fencing refuses + stale effects; the full protocol is the redesign the register defers. +- A07 — F04/F05/F35: unfenced compensation is deliberate (register); + generation/operation-scoped cleanup identities and the durable journal + are the deferred design. A10/A11/A13's honest reporting now covers the + contained half. +- A09 — F04 generation identity: same-version redeploys still rewrite the + (app, hash) record — the documented immutability exception. Attempt ids + (F08) are the keying surface for the generation-keyed store. +- A12 — F45 remainder: restorePreviousRoute still reconstructs from cfg + + live inspect (now failing closed on ambiguous ports via A21); the exact + receipt/compare-and-swap restore design remains open on + ParseSites/ExtractPolicy. +- A16 — F04 external-ingress handoff (candidates reachable via the stable + alias before readiness). +- A20 — TCL-15 port allocation redesign. +- A22 — F47/TCL-17 explicit HTTP/TCP probe modes (the 404/3xx TCP + fallback stays documented compat). +- A24 — F17 standing: Cmd remains a deliberate operator-authored shell + string at the docker-run sink. +- A29 — TCL-55 session-open bounding (needs a dedicated connection per + session). +- A30 — NEW deferral: unified structured executor output (CommandResult + with separated stdout/stderr, truncation flags). Cross-cutting contract + change over every caller; local/remote Run semantics documented as-is. +- A31 — TCL-55 TOFU enrollment serialization (cross-process known_hosts + lock); the stale-snapshot window is narrower than the fixed + fail-open-on-parse-error that F25 closed. +- A33 — F22: backup credentials in host-visible command text (AWS env + assignments, MYSQL_PWD via docker exec -e); needs the container-side + credential-file plumbing shared with the engine images. +- A34 — F42 durable webhook queue (ack-before-durable-job remains; A36 + closed the dedup-race half). +- A35 — F40 webhook build pinning to the event commit. +- A37 — F43 listener scope + operational bounds (graceful shutdown, + bounded admission) — the durable queue (A34) is the prerequisite for + honest shutdown semantics. +- A38 — TCL-40 restore under the app lease + writer quiescence. +- A42 — F37 per-engine validated cutover (SQL/Mongo in-place restore). +- A43 — TCL-44 constrained extractor/host helper (staging extraction + still runs the host tar). +- A45 — F39 crontab edit under a host-side flock. +- A47 — F62/TCL-57 bounded update extraction. +- A48 — F62 update selection policy (downgrade on string inequality). +- A49 — F63/TCL-54 owner item: goreleaser `version: latest` and + aquasec/trivy:latest need reviewed pins (real digests/versions the + report deliberately does not invent); folded into the supply-chain + owner entry with the installer-digest work. +- A50 — F65 real-filesystem/Docker integration matrix (this round's new + tests remain mock-level; PrefixWriter/cancellation tests are behavioral + with real processes).