fix: harden Shell Target recovery and storage

2026-07-17 03:11:33 +08:00
parent 12e857932b
commit 5e0de9b766
9 changed files with 527 additions and 150 deletions
@@ -56,19 +56,73 @@ func TestShellAdapterRunsReusableTargetContract(t *testing.T) {
 						return failed.SetDesired(context.Background(), true, activeProfile())
 					},
 					PartialFailure: func(t *testing.T) target.OperationResult {
-						failed := newAdapterWithFS(t, shellName, nil, &failingFS{FileSystem: shell.OSFileSystem{}, failState: true})
+						failed := newAdapterWithFS(t, shellName, nil, &failingFS{FileSystem: shell.OSFileSystem{}, failEnvironment: true})
 						return failed.SetDesired(context.Background(), true, activeProfile())
 					},
 					RedactionFailure: func(t *testing.T) target.OperationResult {
 						failed := newAdapterWithFS(t, shellName, nil, &failingFS{FileSystem: shell.OSFileSystem{}, failWrite: true})
 						return failed.SetDesired(context.Background(), true, activeProfile())
 					},
+					RecoveryRetry: func(t *testing.T) (target.OperationResult, target.OperationResult, target.OperationResult) {
+						files := &failingFS{FileSystem: shell.OSFileSystem{}, failRecoveryOnce: true}
+						failed := newAdapterWithFS(t, shellName, map[string]string{"HTTP_PROXY": "http://prior.example:3128"}, files)
+						first := failed.SetDesired(context.Background(), true, activeProfile())
+						retry := failed.SetDesired(context.Background(), true, activeProfile())
+						disabled := failed.SetDesired(context.Background(), false, nil)
+						return first, retry, disabled
+					},
 				}
 			})
 		})
 	}
 }

+func TestRecoveryFailureRetryPreservesOriginalShellState(t *testing.T) {
+	files := &failingFS{FileSystem: shell.OSFileSystem{}, failRecoveryOnce: true}
+	a := newAdapterWithFS(t, "/bin/bash", map[string]string{"HTTP_PROXY": "http://prior.example:3128"}, files)
+	first := a.SetDesired(context.Background(), true, activeProfile())
+	if first.Err == nil || first.Inspection.State != target.External {
+		t.Fatalf("first enable = %#v", first)
+	}
+	if _, err := os.Stat(a.EnvironmentPath()); !errors.Is(err, os.ErrNotExist) {
+		t.Fatalf("managed environment was visible before recovery persisted: %v", err)
+	}
+	if retry := a.SetDesired(context.Background(), true, activeProfile()); retry.Err != nil || retry.Inspection.State != target.Enabled {
+		t.Fatalf("retry = %#v", retry)
+	}
+	if disabled := a.SetDesired(context.Background(), false, nil); disabled.Err != nil || disabled.Inspection.State != target.External {
+		t.Fatalf("disable = %#v", disabled)
+	}
+	if restored := readFile(t, a.EnvironmentPath()); !strings.Contains(restored, "http://prior.example:3128") || strings.Contains(restored, "proxy.example:8080") {
+		t.Fatalf("original was not restored: %s", restored)
+	}
+}
+
+func TestRecognizesOnlyExactSupportedShellProxySyntax(t *testing.T) {
+	for _, tc := range []struct {
+		name, contents string
+		state          target.State
+	}{
+		{"comment", "# export HTTP_PROXY='http://comment.example:8080'\n", target.Disabled},
+		{"prefixed variable", "export NOT_HTTP_PROXY='http://wrong.example:8080'\n", target.Disabled},
+		{"prose", "echo export HTTP_PROXY=http://wrong.example:8080\n", target.Disabled},
+		{"unset", "unset HTTP_PROXY http_proxy\n", target.Disabled},
+		{"assignment", "HTTP_PROXY='http://external.example:8080'\n", target.External},
+		{"export assignment", "export http_proxy='http://external.example:8080'\n", target.External},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			a := newAdapter(t, "/bin/bash", nil)
+			if err := os.MkdirAll(filepath.Dir(a.EnvironmentPath()), 0o700); err != nil {
+				t.Fatal(err)
+			}
+			if err := os.WriteFile(a.EnvironmentPath(), []byte(tc.contents), 0o600); err != nil {
+				t.Fatal(err)
+			}
+			assertState(t, a, activeProfile(), tc.state)
+		})
+	}
+}
+
 func TestShellTargetContract(t *testing.T) {
 	contract := func(t *testing.T, shellName string) {
 		t.Helper()
@@ -146,14 +200,14 @@ func TestShellTargetContract(t *testing.T) {
 			})

 			t.Run("permission-and-partial-failure-reinspect", func(t *testing.T) {
-				fs := &failingFS{FileSystem: shell.OSFileSystem{}, failState: true}
+				fs := &failingFS{FileSystem: shell.OSFileSystem{}, failEnvironment: true}
 				a := newAdapterWithFS(t, shellName, nil, fs)
 				got := a.SetDesired(context.Background(), true, activeProfile())
 				if got.Err == nil || !errors.Is(got.Err, os.ErrPermission) {
 					t.Fatalf("permission error = %#v", got)
 				}
-				if got.Inspection.State != target.External {
-					t.Fatalf("partial write state = %s, want External", got.Inspection.State)
+				if got.Inspection.State != target.Drifted {
+					t.Fatalf("partial write state = %s, want Drifted", got.Inspection.State)
 				}
 			})
 		})
@@ -200,6 +254,80 @@ func TestIntegrationInstallUninstallPreservesStartupFileAndLoadsAfterCommand(t *
 	}
 }

+func TestDescriptorAnchoredWritesRejectSymlinkedAncestorVictims(t *testing.T) {
+	t.Run("configuration", func(t *testing.T) {
+		root, victim := t.TempDir(), t.TempDir()
+		if err := os.Symlink(victim, filepath.Join(root, "config")); err != nil {
+			t.Fatal(err)
+		}
+		a := shell.New(shell.Configuration{Shell: "/bin/bash", StartupFile: filepath.Join(root, "home", ".bashrc"), EnvironmentFile: filepath.Join(root, "config", "proxyctl", "shell.env"), RecoveryFile: filepath.Join(root, "state", "proxyctl", "shell.json"), TrustedRoots: []string{root}}, nil, shell.MapEnvironment{})
+		if got := a.SetDesired(context.Background(), true, activeProfile()); got.Err == nil {
+			t.Fatal("enable followed symlinked configuration ancestor")
+		}
+		assertVictimEmpty(t, victim)
+	})
+	t.Run("configuration final file", func(t *testing.T) {
+		root := t.TempDir()
+		victim := filepath.Join(t.TempDir(), "victim")
+		if err := os.WriteFile(victim, []byte("unchanged\n"), 0o600); err != nil {
+			t.Fatal(err)
+		}
+		if err := os.MkdirAll(filepath.Join(root, "config", "proxyctl"), 0o700); err != nil {
+			t.Fatal(err)
+		}
+		environmentFile := filepath.Join(root, "config", "proxyctl", "shell.env")
+		if err := os.Symlink(victim, environmentFile); err != nil {
+			t.Fatal(err)
+		}
+		a := shell.New(shell.Configuration{Shell: "/bin/bash", StartupFile: filepath.Join(root, ".bashrc"), EnvironmentFile: environmentFile, RecoveryFile: filepath.Join(root, "state", "proxyctl", "shell.json"), TrustedRoots: []string{root}}, nil, shell.MapEnvironment{})
+		if got := a.SetDesired(context.Background(), true, activeProfile()); got.Err == nil {
+			t.Fatal("enable replaced symlinked final configuration file")
+		}
+		if got := readFile(t, victim); got != "unchanged\n" {
+			t.Fatalf("final symlink victim changed to %q", got)
+		}
+	})
+	t.Run("state", func(t *testing.T) {
+		root, victim := t.TempDir(), t.TempDir()
+		if err := os.Mkdir(filepath.Join(root, ".local"), 0o700); err != nil {
+			t.Fatal(err)
+		}
+		if err := os.Symlink(victim, filepath.Join(root, ".local", "state")); err != nil {
+			t.Fatal(err)
+		}
+		a := shell.New(shell.Configuration{Shell: "/bin/bash", StartupFile: filepath.Join(root, ".bashrc"), EnvironmentFile: filepath.Join(root, ".config", "proxyctl", "shell.env"), RecoveryFile: filepath.Join(root, ".local", "state", "proxyctl", "shell.json"), TrustedRoots: []string{root}}, nil, shell.MapEnvironment{})
+		if got := a.SetDesired(context.Background(), true, activeProfile()); got.Err == nil {
+			t.Fatal("enable followed symlinked state ancestor")
+		}
+		assertVictimEmpty(t, victim)
+		if _, err := os.Stat(a.EnvironmentPath()); !errors.Is(err, os.ErrNotExist) {
+			t.Fatalf("environment changed before secure recovery: %v", err)
+		}
+	})
+	t.Run("integration", func(t *testing.T) {
+		root, victim := t.TempDir(), t.TempDir()
+		if err := os.Symlink(victim, filepath.Join(root, "home")); err != nil {
+			t.Fatal(err)
+		}
+		a := shell.New(shell.Configuration{Shell: "/bin/bash", StartupFile: filepath.Join(root, "home", ".bashrc"), EnvironmentFile: filepath.Join(root, "config", "shell.env"), RecoveryFile: filepath.Join(root, "state", "shell.json"), TrustedRoots: []string{root}}, nil, shell.MapEnvironment{})
+		if err := a.Install(context.Background(), "/bin/proxyctl"); err == nil {
+			t.Fatal("install followed symlinked startup ancestor")
+		}
+		assertVictimEmpty(t, victim)
+	})
+}
+
+func assertVictimEmpty(t *testing.T, path string) {
+	t.Helper()
+	entries, err := os.ReadDir(path)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(entries) != 0 {
+		t.Fatalf("symlink victim changed: %v", entries)
+	}
+}
+
 func TestIntegrationRefusesStartupSymlinkWithoutChangingVictim(t *testing.T) {
 	a := newAdapter(t, "/bin/bash", nil)
 	if err := os.MkdirAll(filepath.Dir(a.StartupPath()), 0o700); err != nil {
@@ -237,6 +365,38 @@ func TestMalformedIntegrationMarkersNeverCauseStartupContentDeletion(t *testing.
 	}
 }

+func TestFreshIntegratedShellSourcesExistingEnvironmentAtStartup(t *testing.T) {
+	for _, shellName := range []string{"bash", "zsh"} {
+		t.Run(shellName, func(t *testing.T) {
+			path, err := exec.LookPath(shellName)
+			if err != nil {
+				t.Skip(shellName + " is not installed")
+			}
+			a := newAdapter(t, filepath.Join("/bin", shellName), nil)
+			if enabled := a.SetDesired(context.Background(), true, activeProfile()); enabled.Err != nil {
+				t.Fatal(enabled.Err)
+			}
+			if err := a.Install(context.Background(), "/bin/true"); err != nil {
+				t.Fatal(err)
+			}
+			var command *exec.Cmd
+			if shellName == "bash" {
+				command = exec.Command(path, "--noprofile", "--rcfile", a.StartupPath(), "-i", "-c", `printf %s "$HTTP_PROXY"`)
+			} else {
+				command = exec.Command(path, "-d", "-i", "-c", `printf %s "$HTTP_PROXY"`)
+				command.Env = append(os.Environ(), "ZDOTDIR="+filepath.Dir(a.StartupPath()))
+			}
+			output, err := command.Output()
+			if err != nil {
+				t.Fatalf("start fresh %s: %v", shellName, err)
+			}
+			if got := string(output); got != "http://alice:secret@proxy.example:8080" {
+				t.Fatalf("fresh %s HTTP_PROXY = %q", shellName, got)
+			}
+		})
+	}
+}
+
 func TestInstalledBashWrapperReloadsParentEnvironmentAfterProxyctlExits(t *testing.T) {
 	if _, err := exec.LookPath("bash"); err != nil {
 		t.Skip("bash is not installed")
@@ -322,7 +482,11 @@ func newAdapter(t *testing.T, shellName string, env map[string]string) *shell.Ad
 func newAdapterWithFS(t *testing.T, shellName string, env map[string]string, fs shell.FileSystem) *shell.Adapter {
 	t.Helper()
 	root := t.TempDir()
-	return shell.New(shell.Configuration{Shell: shellName, StartupFile: filepath.Join(root, "home", ".bashrc"), EnvironmentFile: filepath.Join(root, "config", "shell.env"), RecoveryFile: filepath.Join(root, "state", "shell.json")}, fs, shell.MapEnvironment(env))
+	startup := ".bashrc"
+	if filepath.Base(shellName) == "zsh" {
+		startup = ".zshrc"
+	}
+	return shell.New(shell.Configuration{Shell: shellName, StartupFile: filepath.Join(root, "home", startup), EnvironmentFile: filepath.Join(root, "config", "shell.env"), RecoveryFile: filepath.Join(root, "state", "shell.json")}, fs, shell.MapEnvironment(env))
 }
 func assertState(t *testing.T, a *shell.Adapter, p *profile.Profile, want target.State) {
 	t.Helper()
@@ -341,7 +505,7 @@ func readFile(t *testing.T, path string) string {

 type failingFS struct {
 	shell.FileSystem
-	failWrite, failState, failRestore, permission bool
+	failWrite, failState, failEnvironment, failRestore, permission, failRecoveryOnce bool
 }

 func (f *failingFS) AtomicWrite(path string, data []byte, mode os.FileMode, owner *shell.Ownership) error {
@@ -351,9 +515,16 @@ func (f *failingFS) AtomicWrite(path string, data []byte, mode os.FileMode, owne
 	if f.failWrite {
 		return errors.New("write http://alice:secret@proxy.example:8080 failed")
 	}
+	if f.failEnvironment && strings.HasSuffix(path, "shell.env") {
+		return os.ErrPermission
+	}
 	if f.failRestore && strings.HasSuffix(path, "shell.env") {
 		return errors.New("restore http://alice:restore-secret@prior.example:8080 failed")
 	}
+	if f.failRecoveryOnce && strings.HasSuffix(path, "shell.json") {
+		f.failRecoveryOnce = false
+		return os.ErrPermission
+	}
 	if f.failState && strings.HasSuffix(path, "shell.json") {
 		return os.ErrPermission
 	}