Skip to content

Commit e0f92e6

Browse files
avivsinaiavivsinai
authored
fix: resolve wake inject-via symlinks before validation (#200)
1 parent 97e801c commit e0f92e6

7 files changed

Lines changed: 180 additions & 50 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
3434
install atomically with fsync (#187).
3535
- Wake identity mismatches now stay `unverified` unless AMQ can prove the
3636
recorded PID is not an `amq wake` process (#187).
37+
- `amq wake --inject-via` now accepts symlinked executables, such as
38+
Homebrew-installed injectors, by resolving them before validation; validation,
39+
persistence, and execution all use the resolved physical path (closes #197).
3740
- On Windows, atomic file writes now replace existing files atomically instead
3841
of temporarily deleting the destination during rename retries (#188).
3942
- Atomic queue-file writes now fail with `io.ErrShortWrite` if the filesystem

‎internal/cli/coop_exec_unix.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,9 +69,11 @@ func runCoopExec(args []string) error {
6969
return UsageError("--wake-inject-arg requires --wake-inject-via")
7070
}
7171
if wakeInjectVia != "" {
72-
if err := validateWakeInjectViaPath(wakeInjectVia); err != nil {
72+
resolvedWakeInjectVia, err := validateWakeInjectViaPath(wakeInjectVia)
73+
if err != nil {
7374
return UsageError("--wake-inject-via: %v", err)
7475
}
76+
wakeInjectVia = resolvedWakeInjectVia
7577
}
7678

7779
remaining := fs.Args()

‎internal/cli/wake.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -450,7 +450,7 @@ func injectVia(cfg *wakeConfig, text string) error {
450450
if executable == "" {
451451
return fmt.Errorf("inject-via command is blank")
452452
}
453-
if err := validateWakeInjectViaPath(executable); err != nil {
453+
if err := validateResolvedWakeInjectViaPath(executable); err != nil {
454454
return err
455455
}
456456

‎internal/cli/wake_repair_unix_test.go‎

Lines changed: 73 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ func TestWakeTargetRejectsWorldWritableInjectVia(t *testing.T) {
119119
if err := os.Chmod(injector, 0o777); err != nil {
120120
t.Fatalf("chmod injector: %v", err)
121121
}
122-
err := validateWakeInjectViaPath(injector)
122+
_, err := validateWakeInjectViaPath(injector)
123123
if err == nil || !strings.Contains(err.Error(), "group/world-writable") {
124124
t.Fatalf("expected world-writable rejection, got %v", err)
125125
}
@@ -143,22 +143,26 @@ func TestWakeTargetRejectsWorldWritableInjectViaAncestor(t *testing.T) {
143143
t.Fatalf("write injector: %v", err)
144144
}
145145

146-
err := validateWakeInjectViaPath(injector)
146+
_, err := validateWakeInjectViaPath(injector)
147147
if err == nil || !strings.Contains(err.Error(), "group/world-writable") {
148148
t.Fatalf("expected writable ancestor rejection, got %v", err)
149149
}
150150
}
151151

152-
func TestWakeTargetRejectsSymlinkInjectVia(t *testing.T) {
152+
func TestWakeTargetResolvesLeafSymlinkInjectVia(t *testing.T) {
153+
root := secureTempDirForTest(t)
153154
injector := writeExecutableForTest(t, "injector")
154155
link := filepath.Join(t.TempDir(), "injector-link")
155156
if err := os.Symlink(injector, link); err != nil {
156157
t.Fatalf("symlink injector: %v", err)
157158
}
158159

159-
err := validateWakeInjectViaPath(link)
160-
if err == nil || !strings.Contains(err.Error(), "must not be a symlink") {
161-
t.Fatalf("expected symlink rejection, got %v", err)
160+
target := mustNewWakeTargetForTest(t, root, "codex", link, nil)
161+
if target.InjectVia != injector {
162+
t.Fatalf("target inject_via = %q, want resolved %q", target.InjectVia, injector)
163+
}
164+
if err := validateWakeTarget(target, root, "codex"); err != nil {
165+
t.Fatalf("validateWakeTarget: %v", err)
162166
}
163167
}
164168

@@ -207,7 +211,7 @@ func TestWakeTargetResolvesSymlinkedInjectViaParent(t *testing.T) {
207211
}
208212
}
209213

210-
func TestWakeTargetKeepsSymlinkInjectViaForValidationRejection(t *testing.T) {
214+
func TestWakeTargetResolvesLeafSymlinkInjectViaForValidation(t *testing.T) {
211215
base := secureTempDirForTest(t)
212216
injector := writeExecutableForTest(t, "injector")
213217
link := filepath.Join(base, "injector-link")
@@ -216,11 +220,64 @@ func TestWakeTargetKeepsSymlinkInjectViaForValidationRejection(t *testing.T) {
216220
}
217221

218222
target := mustNewWakeTargetForTest(t, base, "codex", link, nil)
219-
if target.InjectVia != link {
220-
t.Fatalf("target inject_via = %q, want unresolved symlink %q", target.InjectVia, link)
223+
if target.InjectVia != injector {
224+
t.Fatalf("target inject_via = %q, want resolved %q", target.InjectVia, injector)
225+
}
226+
if err := validateWakeTarget(target, base, "codex"); err != nil {
227+
t.Fatalf("validateWakeTarget: %v", err)
228+
}
229+
}
230+
231+
func TestWakeTargetRejectsDanglingSymlinkInjectVia(t *testing.T) {
232+
root := secureTempDirForTest(t)
233+
link := filepath.Join(root, "injector-link")
234+
if err := os.Symlink(filepath.Join(root, "missing-injector"), link); err != nil {
235+
t.Fatalf("symlink injector: %v", err)
236+
}
237+
238+
_, err := newWakeTarget(root, "codex", link, nil)
239+
if err == nil || !strings.Contains(err.Error(), "resolve inject_via") {
240+
t.Fatalf("expected dangling symlink rejection, got %v", err)
241+
}
242+
}
243+
244+
func TestWakeTargetRejectsSymlinkToNonExecutableInjectVia(t *testing.T) {
245+
root := secureTempDirForTest(t)
246+
injector := filepath.Join(root, "injector")
247+
if err := os.WriteFile(injector, []byte("#!/bin/sh\nexit 0\n"), 0o644); err != nil {
248+
t.Fatalf("write injector: %v", err)
221249
}
222-
if err := validateWakeTarget(target, base, "codex"); err == nil || !strings.Contains(err.Error(), "must not be a symlink") {
223-
t.Fatalf("expected symlink validation rejection, got %v", err)
250+
link := filepath.Join(root, "injector-link")
251+
if err := os.Symlink(injector, link); err != nil {
252+
t.Fatalf("symlink injector: %v", err)
253+
}
254+
255+
_, err := newWakeTarget(root, "codex", link, nil)
256+
if err == nil || !strings.Contains(err.Error(), "not executable") {
257+
t.Fatalf("expected non-executable resolved target rejection, got %v", err)
258+
}
259+
}
260+
261+
func TestWakeTargetRejectsSymlinkToNonOwnedInjectVia(t *testing.T) {
262+
oldCurrent := wakeTargetCurrentUID
263+
oldOwner := wakeTargetFileOwnerUID
264+
wakeTargetCurrentUID = func() (int, bool) { return 1000, true }
265+
wakeTargetFileOwnerUID = func(info os.FileInfo) (int, bool) { return 2000, true }
266+
t.Cleanup(func() {
267+
wakeTargetCurrentUID = oldCurrent
268+
wakeTargetFileOwnerUID = oldOwner
269+
})
270+
271+
root := secureTempDirForTest(t)
272+
injector := writeExecutableForTest(t, "injector")
273+
link := filepath.Join(root, "injector-link")
274+
if err := os.Symlink(injector, link); err != nil {
275+
t.Fatalf("symlink injector: %v", err)
276+
}
277+
278+
_, err := newWakeTarget(root, "codex", link, nil)
279+
if err == nil || !strings.Contains(err.Error(), "owned by uid 2000") {
280+
t.Fatalf("expected owner rejection, got %v", err)
224281
}
225282
}
226283

@@ -229,8 +286,8 @@ func TestWakeTargetCreationRejectsMissingInjectVia(t *testing.T) {
229286
missing := filepath.Join(root, "missing-injector")
230287

231288
_, err := newWakeTarget(root, "codex", missing, nil)
232-
if err == nil || !strings.Contains(err.Error(), "stat inject_via") {
233-
t.Fatalf("expected inject_via stat failure, got %v", err)
289+
if err == nil || !strings.Contains(err.Error(), "resolve inject_via") {
290+
t.Fatalf("expected inject_via resolve failure, got %v", err)
234291
}
235292
}
236293

@@ -239,7 +296,7 @@ func TestWakeTargetRejectsGroupWritableInjectVia(t *testing.T) {
239296
if err := os.Chmod(injector, 0o775); err != nil {
240297
t.Fatalf("chmod injector: %v", err)
241298
}
242-
err := validateWakeInjectViaPath(injector)
299+
_, err := validateWakeInjectViaPath(injector)
243300
if err == nil || !strings.Contains(err.Error(), "group/world-writable") {
244301
t.Fatalf("expected group-writable rejection, got %v", err)
245302
}
@@ -248,7 +305,7 @@ func TestWakeTargetRejectsGroupWritableInjectVia(t *testing.T) {
248305
func TestWakeTargetRejectsNonRegularInjectVia(t *testing.T) {
249306
path := t.TempDir()
250307

251-
err := validateWakeInjectViaPath(path)
308+
_, err := validateWakeInjectViaPath(path)
252309
if err == nil || !strings.Contains(err.Error(), "must be a regular file") {
253310
t.Fatalf("expected non-regular rejection, got %v", err)
254311
}
@@ -265,7 +322,7 @@ func TestWakeTargetRejectsNonOwnedInjectVia(t *testing.T) {
265322
})
266323

267324
injector := writeExecutableForTest(t, "injector")
268-
err := validateWakeInjectViaPath(injector)
325+
_, err := validateWakeInjectViaPath(injector)
269326
if err == nil || !strings.Contains(err.Error(), "owned by uid 2000") {
270327
t.Fatalf("expected owner rejection, got %v", err)
271328
}

‎internal/cli/wake_target.go‎

Lines changed: 43 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -59,19 +59,7 @@ func newWakeTarget(root, me, injectVia string, injectArgs []string) (wakeTarget,
5959
}
6060

6161
func wakeTargetInjectViaPath(path string) (string, error) {
62-
trimmed := strings.TrimSpace(path)
63-
info, err := os.Lstat(trimmed)
64-
if err != nil {
65-
return "", fmt.Errorf("stat inject_via: %w", err)
66-
}
67-
if info.Mode()&os.ModeSymlink != 0 {
68-
return trimmed, nil
69-
}
70-
if resolved, err := filepath.EvalSymlinks(trimmed); err == nil {
71-
return resolved, nil
72-
} else {
73-
return "", fmt.Errorf("resolve inject_via: %w", err)
74-
}
62+
return validateWakeInjectViaPath(path)
7563
}
7664

7765
func wakeTargetDigest(target wakeTarget) string {
@@ -190,7 +178,7 @@ func validateWakeTarget(target wakeTarget, root, me string) error {
190178
if target.Agent != me {
191179
return fmt.Errorf("wake target agent mismatch")
192180
}
193-
if err := validateWakeInjectViaPath(target.InjectVia); err != nil {
181+
if err := validateResolvedWakeInjectViaPath(target.InjectVia); err != nil {
194182
return err
195183
}
196184
for _, arg := range target.InjectArgs {
@@ -248,37 +236,64 @@ func validateWakeOwner(owner wakeOwner) error {
248236
return nil
249237
}
250238

251-
func validateWakeInjectViaPath(path string) error {
252-
if strings.TrimSpace(path) == "" {
253-
return fmt.Errorf("inject_via must not be blank")
239+
func validateWakeInjectViaPath(path string) (string, error) {
240+
resolvedPath, err := resolveWakeInjectViaPath(path)
241+
if err != nil {
242+
return "", err
243+
}
244+
if err := validateResolvedWakeInjectViaTarget(resolvedPath); err != nil {
245+
return "", err
246+
}
247+
return resolvedPath, nil
248+
}
249+
250+
func resolveWakeInjectViaPath(path string) (string, error) {
251+
trimmed := strings.TrimSpace(path)
252+
if trimmed == "" {
253+
return "", fmt.Errorf("inject_via must not be blank")
254254
}
255255
if strings.ContainsRune(path, 0) {
256-
return fmt.Errorf("inject_via contains NUL")
256+
return "", fmt.Errorf("inject_via contains NUL")
257257
}
258-
if !filepath.IsAbs(path) {
259-
return fmt.Errorf("inject_via must be an absolute path")
258+
if !filepath.IsAbs(trimmed) {
259+
return "", fmt.Errorf("inject_via must be an absolute path")
260260
}
261-
info, err := os.Lstat(path)
261+
resolvedPath, err := filepath.EvalSymlinks(trimmed)
262262
if err != nil {
263-
return fmt.Errorf("stat inject_via: %w", err)
263+
return "", fmt.Errorf("resolve inject_via: %w", err)
264264
}
265-
if info.Mode()&os.ModeSymlink != 0 {
266-
return fmt.Errorf("inject_via must not be a symlink")
265+
return resolvedPath, nil
266+
}
267+
268+
func validateResolvedWakeInjectViaTarget(resolvedPath string) error {
269+
info, err := os.Stat(resolvedPath)
270+
if err != nil {
271+
return fmt.Errorf("stat inject_via: %w", err)
267272
}
268273
if !info.Mode().IsRegular() {
269274
return fmt.Errorf("inject_via must be a regular file")
270275
}
271276
if info.Mode().Perm()&0o111 == 0 {
272277
return fmt.Errorf("inject_via is not executable")
273278
}
274-
if err := validateWakeTargetPathOwnership("inject_via", path, info); err != nil {
279+
if err := validateWakeTargetPathOwnership("inject_via", resolvedPath, info); err != nil {
275280
return err
276281
}
277-
resolvedPath, err := filepath.EvalSymlinks(path)
282+
if err := validateWakeTargetParentDirs("inject_via", resolvedPath); err != nil {
283+
return err
284+
}
285+
return nil
286+
}
287+
288+
func validateResolvedWakeInjectViaPath(path string) error {
289+
resolvedPath, err := resolveWakeInjectViaPath(path)
278290
if err != nil {
279-
return fmt.Errorf("resolve inject_via: %w", err)
291+
return err
280292
}
281-
if err := validateWakeTargetParentDirs("inject_via", resolvedPath); err != nil {
293+
if resolvedPath != strings.TrimSpace(path) {
294+
return fmt.Errorf("inject_via must be a resolved path")
295+
}
296+
if err := validateResolvedWakeInjectViaTarget(resolvedPath); err != nil {
282297
return err
283298
}
284299
return nil

‎internal/cli/wake_unix.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -787,7 +787,7 @@ func runWakeWithLoop(args []string, loop wakeLoopFunc) error {
787787
return fmt.Errorf("clear stale wake target after persist failure: %w", removeErr)
788788
}
789789
}
790-
if err := validateWakeInjectViaPath(injectVia); err != nil {
790+
if err := validateResolvedWakeInjectViaPath(injectVia); err != nil {
791791
return err
792792
}
793793
} else if err := removeWakeTarget(root, me); err != nil {

‎internal/cli/wake_unix_test.go‎

Lines changed: 56 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -263,6 +263,55 @@ func TestRunWakeWithLoopExecutesResolvedInjectViaPath(t *testing.T) {
263263
}
264264
}
265265

266+
func TestRunWakeWithLoopPersistsResolvedLeafSymlinkInjectViaPath(t *testing.T) {
267+
root := secureTempDirForTest(t)
268+
if err := fsq.EnsureRootDirs(root); err != nil {
269+
t.Fatalf("EnsureRootDirs: %v", err)
270+
}
271+
if err := fsq.EnsureAgentDirs(root, "orchestrator"); err != nil {
272+
t.Fatalf("EnsureAgentDirs: %v", err)
273+
}
274+
base := secureTempDirForTest(t)
275+
cellarDir := filepath.Join(base, "Cellar", "injector", "1.0.0", "bin")
276+
if err := os.MkdirAll(cellarDir, 0o700); err != nil {
277+
t.Fatalf("mkdir cellar bin: %v", err)
278+
}
279+
injector := filepath.Join(cellarDir, "injector")
280+
if err := os.WriteFile(injector, []byte("#!/bin/sh\nexit 0\n"), 0o755); err != nil {
281+
t.Fatalf("write injector: %v", err)
282+
}
283+
binDir := filepath.Join(base, "bin")
284+
if err := os.MkdirAll(binDir, 0o700); err != nil {
285+
t.Fatalf("mkdir bin: %v", err)
286+
}
287+
link := filepath.Join(binDir, "injector")
288+
if err := os.Symlink(injector, link); err != nil {
289+
t.Fatalf("symlink injector: %v", err)
290+
}
291+
292+
errDone := errors.New("done")
293+
err := runWakeWithLoop([]string{
294+
"--root", root,
295+
"--me", "orchestrator",
296+
"--inject-via", link,
297+
}, func(cfg wakeConfig) error {
298+
if cfg.injectVia != injector {
299+
t.Fatalf("cfg.injectVia = %q, want resolved %q", cfg.injectVia, injector)
300+
}
301+
target, exists, err := readWakeTarget(root, "orchestrator")
302+
if err != nil || !exists {
303+
t.Fatalf("readWakeTarget exists=%v err=%v", exists, err)
304+
}
305+
if target.InjectVia != injector {
306+
t.Fatalf("target inject_via = %q, want resolved %q", target.InjectVia, injector)
307+
}
308+
return errDone
309+
})
310+
if !errors.Is(err, errDone) {
311+
t.Fatalf("expected loop sentinel error, got %v", err)
312+
}
313+
}
314+
266315
func TestRunWakeWithLoopRejectsUnsafeInjectViaBeforeLoop(t *testing.T) {
267316
root := secureTempDirForTest(t)
268317
if err := fsq.EnsureRootDirs(root); err != nil {
@@ -314,7 +363,7 @@ func TestInjectViaRevalidatesExecutableBeforeExec(t *testing.T) {
314363
t.Fatalf("symlink injector: %v", err)
315364
}
316365
},
317-
wantText: "must not be a symlink",
366+
wantText: "must be a resolved path",
318367
},
319368
{
320369
name: "nonregular",
@@ -411,7 +460,11 @@ func TestRunWakeWithLoopRejectsInjectorSwappedAfterTargetWrite(t *testing.T) {
411460
if err := os.Remove(injector); err != nil {
412461
t.Fatalf("remove injector: %v", err)
413462
}
414-
if err := os.Symlink("/bin/sh", injector); err != nil {
463+
nonExecutable := filepath.Join(secureTempDirForTest(t), "non-executable-injector")
464+
if err := os.WriteFile(nonExecutable, []byte("#!/bin/sh\nexit 0\n"), 0o644); err != nil {
465+
t.Fatalf("write non-executable injector: %v", err)
466+
}
467+
if err := os.Symlink(nonExecutable, injector); err != nil {
415468
t.Fatalf("swap injector to symlink: %v", err)
416469
}
417470

@@ -424,7 +477,7 @@ func TestRunWakeWithLoopRejectsInjectorSwappedAfterTargetWrite(t *testing.T) {
424477
t.Fatalf("loop should not run after injector swap: %#v", cfg)
425478
return nil
426479
})
427-
if err == nil || !strings.Contains(err.Error(), "must not be a symlink") {
480+
if err == nil || !strings.Contains(err.Error(), "not executable") {
428481
t.Fatalf("expected swapped injector rejection, got %v", err)
429482
}
430483
}

0 commit comments

Comments
 (0)