diff --git a/cmd/proxyctl/main.go b/cmd/proxyctl/main.go index 7e02d4d..4d78ec7 100644 --- a/cmd/proxyctl/main.go +++ b/cmd/proxyctl/main.go @@ -103,7 +103,16 @@ func runWithEnvironmentAndTargets(args []string, input io.Reader, output io.Writ } targets := append([]target.Target{shellTarget}, systemTargets...) stateDirectory := filepath.Dir(shellConfiguration.RecoveryFile) - mutationLock := operationlock.NewOwned(filepath.Join(stateDirectory, "operation.lock"), profileConfiguration.Owner.UID, profileConfiguration.Owner.GID) + userMutationLock := operationlock.NewOwned(filepath.Join(stateDirectory, "operation.lock"), profileConfiguration.Owner.UID, profileConfiguration.Owner.GID) + var mutationLock app.MutationLock = userMutationLock + if os.Geteuid() == 0 { + // Every elevated process also takes a root-owned lock that the invoking + // user cannot replace while a system Target mutation is in flight. + mutationLock = operationlock.NewGroup( + operationlock.New("/run/lock/proxyctl/operation.lock"), + userMutationLock, + ) + } logger := oplog.NewOwned(filepath.Join(stateDirectory, "operations.log"), 64*1024, 3, profileConfiguration.Owner.UID, profileConfiguration.Owner.GID) store := profile.NewOwnedFileStoreAt(profileConfiguration.Path, profileConfiguration.Owner, profileConfiguration.TrustedRoot) @@ -130,9 +139,12 @@ func runWithEnvironmentAndTargets(args []string, input io.Reader, output io.Writ if operationErr != nil { result, detail = "failure", operationErr.Error() } - _ = logger.Write(app.LogEntry{Operation: operation, Target: shellTarget.ID(), Result: result, Detail: detail}) + logErr := logger.Write(app.LogEntry{Operation: operation, Target: shellTarget.ID(), Result: result, Detail: detail}) if operationErr != nil { - return operationErr + return errors.Join(operationErr, logErr) + } + if logErr != nil { + return fmt.Errorf("Shell integration changed but the operation log could not be written: %w", logErr) } if args[1] == "install" { fmt.Fprintln(output, "Shell integration installed; restart or source your Shell startup file once.") diff --git a/internal/app/app.go b/internal/app/app.go index 0ab1083..fc2fa3a 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -2,9 +2,10 @@ package app import ( "context" + "crypto/sha256" + "encoding/hex" "errors" "fmt" - "strings" "sync" "time" @@ -28,11 +29,21 @@ const ( ) type Intent struct { - Kind IntentKind - Name string - Profile profile.Profile - Confirm bool - TargetID string + Kind IntentKind + Name string + Profile profile.Profile + Confirm bool + ConfirmationToken string + TargetID string +} + +func confirmationToken(parts ...string) string { + hash := sha256.New() + for _, part := range parts { + _, _ = hash.Write([]byte(part)) + _, _ = hash.Write([]byte{0}) + } + return hex.EncodeToString(hash.Sum(nil)) } // TargetActionKind identifies a Target action independently of display labels. @@ -299,7 +310,7 @@ type targetDetectionUpdate struct { complete bool } -func (a *Application) beginTargetDetection() (Snapshot, <-chan targetDetectionUpdate) { +func (a *Application) beginTargetDetection(ctx context.Context) (Snapshot, <-chan targetDetectionUpdate) { a.stateMu.Lock() a.detectionGeneration++ generation := a.detectionGeneration @@ -316,7 +327,7 @@ func (a *Application) beginTargetDetection() (Snapshot, <-chan targetDetectionUp for i, adapter := range a.targets { adapter, id := adapter, a.targetIDs[i] go func() { - outcomes <- targetDetectionOutcome{targetID: id, detection: detectTarget(adapter, active)} + outcomes <- targetDetectionOutcome{targetID: id, detection: detectTarget(ctx, adapter, active)} }() } go func() { @@ -351,28 +362,30 @@ func (a *Application) beginTargetDetection() (Snapshot, <-chan targetDetectionUp return checking, updates } -func detectTarget(adapter target.Target, active *profile.Profile) target.Detection { +func detectTarget(ctx context.Context, adapter target.Target, active *profile.Profile) target.Detection { if detector, ok := adapter.(target.Detector); ok { - return detector.Detect(context.Background(), active) + return detector.Detect(ctx, active) } return target.Detection{ - Availability: adapter.Availability(context.Background()), - Inspection: adapter.Inspect(context.Background(), active), + Availability: adapter.Availability(ctx), + Inspection: adapter.Inspect(ctx, active), } } func (a *Application) Dispatch(intent Intent) Result { - return a.dispatch(intent, nil) + ctx, cancel := context.WithTimeout(context.Background(), operationTimeout) + defer cancel() + return a.dispatch(ctx, intent, nil) } -func (a *Application) dispatch(intent Intent, publish func(Result)) Result { +func (a *Application) dispatch(ctx context.Context, intent Intent, publish func(Result)) Result { // Serialize local mutation workflows even when tests intentionally omit the // cross-process MutationLock. a.dispatchMu.Lock() defer a.dispatchMu.Unlock() if a.mutationLock != nil { - unlock, err := a.mutationLock.Lock(context.Background()) + unlock, err := a.mutationLock.Lock(ctx) if err != nil { return Result{Snapshot: a.Snapshot(), Err: fmt.Errorf("lock proxyctl mutation: %w", err)} } @@ -388,16 +401,16 @@ func (a *Application) dispatch(intent Intent, publish func(Result)) Result { } a.replaceState(fresh) if intent.Kind == ActivateProfile { - a.refreshInspections() + a.refreshInspections(ctx) } if isTargetMutationIntent(intent.Kind) { - return a.setTargetDesired(intent, publish) + return a.setTargetDesired(ctx, intent, publish) } if intent.Kind == RefreshTargets { - return a.refreshTargets(publish) + return a.refreshTargets(ctx, publish) } if intent.Kind == ActivateProfile { - return a.activateProfile(intent, publish) + return a.activateProfile(ctx, intent, publish) } var notice string next, err := a.store.Mutate(func(current profile.State) (profile.State, error) { @@ -416,7 +429,7 @@ func (a *Application) dispatch(intent Intent, publish func(Result)) Result { }) if err == nil { a.replaceState(next) - a.refreshInspections() + a.refreshInspections(ctx) } else { var committed *profile.CommitError if errors.As(err, &committed) { @@ -434,7 +447,7 @@ func (a *Application) dispatch(intent Intent, publish func(Result)) Result { return Result{Snapshot: a.Snapshot(), Notice: notice, Err: err} } -func (a *Application) activateProfile(intent Intent, publish func(Result)) Result { +func (a *Application) activateProfile(ctx context.Context, intent Intent, publish func(Result)) Result { state := a.stateCopy() if index(state, intent.Name) < 0 { return Result{Snapshot: a.Snapshot(), Err: fmt.Errorf("Profile %q does not exist", intent.Name)} @@ -446,23 +459,28 @@ func (a *Application) activateProfile(intent Intent, publish func(Result)) Resul inspections := a.inspectionsCopy() var enabled []int var warnings []string + confirmationScope := []string{"activate-profile", intent.Name} for i, adapter := range a.targets { id := a.targetIDs[i] if inspections[id].State != target.Enabled { continue } enabled = append(enabled, i) + confirmationScope = append(confirmationScope, id) if warning := adapter.Confirmation(true); warning != "" { - warnings = append(warnings, a.targetDescriptions[i].Name+": "+warning) + detail := a.targetDescriptions[i].Name + ": " + warning + warnings = append(warnings, detail) + confirmationScope = append(confirmationScope, detail) + } else { + confirmationScope = append(confirmationScope, "") } } - if len(warnings) > 0 && !intent.Confirm { - message := "Changing the Active Profile requires confirmation:\n" + strings.Join(warnings, "\n") + token := confirmationToken(confirmationScope...) + if len(warnings) > 0 && (!intent.Confirm || intent.ConfirmationToken != token) { confirmation := &Confirmation{ Title: "Confirm Active Profile change", Summary: "Changing the Active Profile has high-impact Target effects.", - Details: warnings, ConfirmLabel: "Change Profile", CancelLabel: "Cancel", HighImpact: true, + Details: warnings, ConfirmLabel: "Change Profile", CancelLabel: "Cancel", HighImpact: true, Token: token, } - _ = message return Result{Snapshot: a.Snapshot(), ConfirmationRequest: confirmation, Status: OperationStatus{Summary: confirmation.Title}} } @@ -489,7 +507,7 @@ func (a *Application) activateProfile(intent Intent, publish func(Result)) Resul // without the state lock, then publish all observations atomically. observed := make(map[string]target.Inspection, len(a.targets)) for i, adapter := range a.targets { - observed[a.targetIDs[i]] = adapter.Inspect(context.Background(), active) + observed[a.targetIDs[i]] = adapter.Inspect(ctx, active) } a.replaceInspections(observed) @@ -505,16 +523,25 @@ func (a *Application) activateProfile(intent Intent, publish func(Result)) Resul publish(Result{Snapshot: a.Snapshot(), Progress: []TargetProgress{{TargetID: id, Phase: TargetProgressRunning, State: a.inspection(id).State}}, Notice: "Updating " + name + " Target", Status: OperationStatus{Summary: "Updating " + name + " Target", TargetID: id}}) } - operation := adapter.SetDesired(context.Background(), true, active) + operation := adapter.SetDesired(ctx, true, active) inspection := operation.Inspection var safeErr error if operation.Err != nil { // A failure-returned inspection may predate a partial write. Always // inspect explicitly against the new Active Profile before publishing. - inspection = adapter.Inspect(context.Background(), active) + inspection = adapter.Inspect(ctx, active) safeErr = fmt.Errorf("%s (fresh inspection: %s — %s)", profile.RedactText(operation.Err.Error()), inspection.State, profile.RedactText(inspection.Evidence)) } a.replaceInspection(id, inspection) + if a.logger != nil { + outcome, detail := "success", inspection.Evidence + if safeErr != nil { + outcome, detail = "failure", safeErr.Error() + } + if logErr := a.logger.Write(LogEntry{Time: time.Now(), Operation: "switch-Active-Profile", Target: id, Result: outcome, Detail: RedactLogDetail(detail)}); logErr != nil { + safeErr = errors.Join(safeErr, fmt.Errorf("write %s Target operation log: %w", name, logErr)) + } + } progressPhase := TargetProgressSucceeded if safeErr != nil { progressPhase = TargetProgressFailed @@ -528,13 +555,6 @@ func (a *Application) activateProfile(intent Intent, publish func(Result)) Resul } publish(Result{Snapshot: a.Snapshot(), Progress: []TargetProgress{item}, Notice: name + " Target update completed", Status: status, Err: safeErr}) } - if a.logger != nil { - outcome, detail := "success", inspection.Evidence - if safeErr != nil { - outcome, detail = "failure", safeErr.Error() - } - _ = a.logger.Write(LogEntry{Time: time.Now(), Operation: "switch-Active-Profile", Target: id, Result: outcome, Detail: RedactLogDetail(detail)}) - } if safeErr != nil { failures = append(failures, fmt.Errorf("%s Target: %w", name, safeErr)) } @@ -545,7 +565,7 @@ func (a *Application) activateProfile(intent Intent, publish func(Result)) Resul for i, adapter := range a.targets { id := a.targetIDs[i] if !updated[id] { - remaining[id] = adapter.Inspect(context.Background(), active) + remaining[id] = adapter.Inspect(ctx, active) } } a.mergeInspections(remaining) @@ -670,8 +690,8 @@ func targetByID(snapshot Snapshot, id string) (TargetSnapshot, string) { return TargetSnapshot{ID: id}, id } -func (a *Application) refreshTargets(publish func(Result)) Result { - checking, updates := a.beginTargetDetection() +func (a *Application) refreshTargets(ctx context.Context, publish func(Result)) Result { + checking, updates := a.beginTargetDetection(ctx) if publish != nil { progress := make([]TargetProgress, 0, len(checking.Targets)) for _, item := range checking.Targets { @@ -714,11 +734,11 @@ func (a *Application) refreshTargets(publish func(Result)) Result { return result } -func (a *Application) refreshInspections() { +func (a *Application) refreshInspections(ctx context.Context) { active := a.activeProfile() inspections := make(map[string]target.Inspection, len(a.targets)) for i, adapter := range a.targets { - inspections[a.targetIDs[i]] = adapter.Inspect(context.Background(), active) + inspections[a.targetIDs[i]] = adapter.Inspect(ctx, active) } a.replaceInspections(inspections) } @@ -769,7 +789,7 @@ func validateTargetAction(intent Intent, selected TargetSnapshot) (targetActionP return policy, nil } -func (a *Application) setTargetDesired(intent Intent, publish func(Result)) Result { +func (a *Application) setTargetDesired(ctx context.Context, intent Intent, publish func(Result)) Result { selectedIndex := -1 for i, id := range a.targetIDs { if id == intent.TargetID { @@ -785,27 +805,30 @@ func (a *Application) setTargetDesired(intent Intent, publish func(Result)) Resu // This targeted check runs inside dispatch serialization and immediately // before action validation. Adapter calls happen without stateMu; the full // outcome is then published under one lock before policy is derived. - detection := detectTarget(selected, active) + detection := detectTarget(ctx, selected, active) snapshot := a.publishTargetDetection(intent.TargetID, detection) selectedSnapshot := snapshot.Targets[selectedIndex] policy, err := validateTargetAction(intent, selectedSnapshot) if err != nil { return Result{Snapshot: snapshot, Err: err} } - if warning := selected.Confirmation(policy.desired); warning != "" && !intent.Confirm { - confirmation := &Confirmation{Title: "Confirm " + a.targetDescriptions[selectedIndex].Name + " Target change", Summary: warning, - ConfirmLabel: "Continue", CancelLabel: "Cancel", HighImpact: true} - return Result{Snapshot: a.Snapshot(), ConfirmationRequest: confirmation, - Status: OperationStatus{Summary: confirmation.Title, TargetID: intent.TargetID}} + if warning := selected.Confirmation(policy.desired); warning != "" { + token := confirmationToken("target", intent.TargetID, fmt.Sprint(intent.Kind), fmt.Sprint(selectedSnapshot.State), fmt.Sprint(policy.desired), warning) + if !intent.Confirm || intent.ConfirmationToken != token { + confirmation := &Confirmation{Title: "Confirm " + a.targetDescriptions[selectedIndex].Name + " Target change", Summary: warning, + ConfirmLabel: "Continue", CancelLabel: "Cancel", HighImpact: true, Token: token} + return Result{Snapshot: a.Snapshot(), ConfirmationRequest: confirmation, + Status: OperationStatus{Summary: confirmation.Title, TargetID: intent.TargetID}} + } } if publish != nil { publish(Result{Snapshot: snapshot, Progress: []TargetProgress{{TargetID: intent.TargetID, Phase: TargetProgressRunning, State: selectedSnapshot.State}}, Status: OperationStatus{Summary: "Updating " + a.targetDescriptions[selectedIndex].Name + " Target", TargetID: intent.TargetID}}) } - result := selected.SetDesired(context.Background(), policy.desired, active) + result := selected.SetDesired(ctx, policy.desired, active) inspection := result.Inspection if result.Err != nil { - inspection = selected.Inspect(context.Background(), active) + inspection = selected.Inspect(ctx, active) } a.replaceInspection(intent.TargetID, inspection) outcome, detail := "success", inspection.Evidence @@ -813,7 +836,9 @@ func (a *Application) setTargetDesired(intent Intent, publish func(Result)) Resu outcome, detail = "failure", result.Err.Error() } if a.logger != nil { - _ = a.logger.Write(LogEntry{Time: time.Now(), Operation: policy.operation, Target: intent.TargetID, Result: outcome, Detail: RedactLogDetail(detail)}) + if logErr := a.logger.Write(LogEntry{Time: time.Now(), Operation: policy.operation, Target: intent.TargetID, Result: outcome, Detail: RedactLogDetail(detail)}); logErr != nil { + result.Err = errors.Join(result.Err, fmt.Errorf("write Target operation log: %w", logErr)) + } } notice := "" progressPhase := TargetProgressSucceeded diff --git a/internal/app/operation.go b/internal/app/operation.go index 89a0409..70ab8ea 100644 --- a/internal/app/operation.go +++ b/internal/app/operation.go @@ -1,5 +1,12 @@ package app +import ( + "context" + "time" +) + +const operationTimeout = 5 * time.Minute + // StatusKind gives presentation layers semantics without requiring them to // inspect user-facing status text. type StatusKind uint8 @@ -27,6 +34,9 @@ type Confirmation struct { ConfirmLabel string CancelLabel string HighImpact bool + // Token binds acceptance to the exact state and operation scope that was + // presented. Presentation layers must return it with the confirmed Intent. + Token string } // TargetProgressPhase describes one Target's part in an operation stream. @@ -189,6 +199,7 @@ func (u OperationUpdate) IsTerminal() bool { return u.Decode().IsTerm type OperationStream struct { Snapshot Snapshot Updates <-chan OperationUpdate + Cancel context.CancelFunc } // InteractiveModule is the presentation-independent Application seam used by @@ -216,16 +227,20 @@ func (a *Application) StartOperation(intent Intent) OperationStream { a.interactiveOperation = true a.interactiveMu.Unlock() + ctx, cancel := context.WithTimeout(context.Background(), operationTimeout) updates := make(chan OperationUpdate, 2*len(a.targets)+2) go func() { defer close(updates) - defer a.finishInteractiveOperation() - final := a.dispatch(intent, func(result Result) { + defer cancel() + final := a.dispatch(ctx, intent, func(result Result) { updates <- runningOperationUpdate(result) }) + // Release the Application gate before publishing the terminal update so + // an immediate confirmation retry cannot observe a stale busy state. + a.finishInteractiveOperation() updates <- terminalOperationUpdate(final) }() - return OperationStream{Snapshot: initial, Updates: updates} + return OperationStream{Snapshot: initial, Updates: updates, Cancel: cancel} } func (a *Application) finishInteractiveOperation() { diff --git a/internal/app/operation_stream_test.go b/internal/app/operation_stream_test.go index 471cb21..7a18588 100644 --- a/internal/app/operation_stream_test.go +++ b/internal/app/operation_stream_test.go @@ -83,6 +83,45 @@ func (t *coordinatedOperationTarget) SetDesired(_ context.Context, enabled bool, return target.OperationResult{Inspection: target.Inspection{State: t.state, Evidence: "completed"}} } +type cancellableOperationTarget struct{ started chan struct{} } + +func (t *cancellableOperationTarget) ID() string { return "cancellable" } +func (t *cancellableOperationTarget) Description() target.Description { + return target.Description{Name: "Cancellable"} +} +func (t *cancellableOperationTarget) Availability(context.Context) target.Availability { + return target.Availability{Available: true} +} +func (t *cancellableOperationTarget) Inspect(context.Context, *profile.Profile) target.Inspection { + return target.Inspection{State: target.Disabled, Evidence: "not changed"} +} +func (t *cancellableOperationTarget) Confirmation(bool) string { return "" } +func (t *cancellableOperationTarget) SetDesired(ctx context.Context, _ bool, _ *profile.Profile) target.OperationResult { + close(t.started) + <-ctx.Done() + return target.OperationResult{Inspection: target.Inspection{State: target.Disabled}, Err: ctx.Err()} +} + +func TestInteractiveOperationCancellationReachesTarget(t *testing.T) { + adapter := &cancellableOperationTarget{started: make(chan struct{})} + application, err := app.NewWithTargets(profile.NewFileStore(filepath.Join(t.TempDir(), "profiles.json")), []target.Target{adapter}) + if err != nil { + t.Fatal(err) + } + createAndActivate(t, application) + refreshApplicationTargets(t, application) + operation := application.StartOperation(app.Intent{Kind: app.EnableTarget, TargetID: adapter.ID()}) + <-adapter.started + operation.Cancel() + var final app.OperationUpdate + for update := range operation.Updates { + final = update + } + if !final.IsFailed() || !errors.Is(final.Err(), context.Canceled) { + t.Fatalf("cancelled operation final update = %#v", final) + } +} + func TestInteractiveOperationStreamSerializesMutationAndPublishesStructuredProgress(t *testing.T) { adapter := &coordinatedOperationTarget{id: "shell", state: target.Disabled, started: make(chan struct{}), release: make(chan struct{})} application, err := app.NewWithTargets(profile.NewFileStore(filepath.Join(t.TempDir(), "profiles.json")), []target.Target{adapter}) @@ -224,9 +263,22 @@ func TestInteractiveOperationStreamUsesStructuredConfirmation(t *testing.T) { operation := application.StartOperation(app.Intent{Kind: app.EnableTarget, TargetID: adapter.ID()}) update := <-operation.Updates confirmation := update.Confirmation() - if !update.IsConfirmation() || confirmation == nil || confirmation.Title == "" || confirmation.CancelLabel == "" { + if !update.IsConfirmation() || confirmation == nil || confirmation.Title == "" || confirmation.CancelLabel == "" || confirmation.Token == "" { t.Fatalf("confirmation update = %#v", update) } + // Retry immediately after the terminal confirmation, before observing the + // first stream close. The Application gate must already be released. + retry := application.StartOperation(app.Intent{Kind: app.EnableTarget, TargetID: adapter.ID(), Confirm: true, ConfirmationToken: confirmation.Token}) + var final app.OperationUpdate + for next := range retry.Updates { + if next.Decode().Kind == app.OperationClosed { + t.Fatalf("immediate confirmed retry observed stale operation gate: %#v", next) + } + final = next + } + if !final.IsSucceeded() { + t.Fatalf("confirmed retry final update = %#v", final) + } if _, open := <-operation.Updates; open { t.Fatal("confirmation operation stream did not close") } diff --git a/internal/app/target_test.go b/internal/app/target_test.go index 2a322a3..733bca6 100644 --- a/internal/app/target_test.go +++ b/internal/app/target_test.go @@ -92,7 +92,12 @@ func TestApplicationRequiresTargetConfirmationBeforeMutation(t *testing.T) { if warning.ConfirmationRequest == nil || warning.Err != nil || len(adapter.calls) != 0 || warning.Snapshot.Targets[0].State != target.Disabled { t.Fatalf("unconfirmed result = %#v, calls=%v", warning, adapter.calls) } - confirmed := application.Dispatch(app.Intent{Kind: app.EnableTarget, TargetID: "docker-daemon", Confirm: true}) + adapter.confirmation = "Updated restart warning after state changed." + stale := application.Dispatch(app.Intent{Kind: app.EnableTarget, TargetID: "docker-daemon", Confirm: true, ConfirmationToken: warning.ConfirmationRequest.Token}) + if stale.ConfirmationRequest == nil || stale.ConfirmationRequest.Token == warning.ConfirmationRequest.Token || len(adapter.calls) != 0 { + t.Fatalf("stale confirmation was accepted = %#v, calls=%v", stale, adapter.calls) + } + confirmed := application.Dispatch(app.Intent{Kind: app.EnableTarget, TargetID: "docker-daemon", Confirm: true, ConfirmationToken: stale.ConfirmationRequest.Token}) if confirmed.Err != nil || len(adapter.calls) != 1 || confirmed.Snapshot.Targets[0].State != target.Enabled { t.Fatalf("confirmed result = %#v, calls=%v", confirmed, adapter.calls) } @@ -105,6 +110,25 @@ type targetWithID struct { func (t targetWithID) ID() string { return t.id } +func TestTargetMutationReportsLoggingFailureAfterKeepingAppliedState(t *testing.T) { + adapter := &fakeTarget{available: true, state: target.Disabled} + logger := &recordingLog{err: errors.New("log device full")} + application, err := app.NewWithTargetsAndRuntime(profile.NewFileStore(filepath.Join(t.TempDir(), "profiles.json")), []target.Target{adapter}, nil, logger) + if err != nil { + t.Fatal(err) + } + createAndActivate(t, application) + refreshApplicationTargets(t, application) + + result := application.Dispatch(app.Intent{Kind: app.EnableTarget, TargetID: adapter.ID()}) + if result.Err == nil || !strings.Contains(result.Err.Error(), "operation log") || result.Snapshot.Targets[0].State != target.Enabled { + t.Fatalf("logging failure result = %#v", result) + } + if len(logger.entries) != 1 { + t.Fatalf("log attempts = %#v", logger.entries) + } +} + func TestApplicationHidesUnavailableTargetButKeepsExplanation(t *testing.T) { adapter := &fakeTarget{available: false, state: target.Disabled} application, err := app.NewWithTargets(profile.NewFileStore(filepath.Join(t.TempDir(), "profiles.json")), []target.Target{adapter}) diff --git a/internal/apt/apt.go b/internal/apt/apt.go index cb68af5..54c8ae5 100644 --- a/internal/apt/apt.go +++ b/internal/apt/apt.go @@ -2,7 +2,6 @@ package apt import ( - "bufio" "context" "errors" "fmt" @@ -15,6 +14,7 @@ import ( "sort" "strings" + "proxyctl/internal/execsafe" "proxyctl/internal/profile" "proxyctl/internal/securefs" "proxyctl/internal/target" @@ -77,7 +77,7 @@ type Runner interface { type systemRunner struct{} -func (systemRunner) LookPath(name string) (string, error) { return exec.LookPath(name) } +func (systemRunner) LookPath(name string) (string, error) { return execsafe.LookPath(name) } func (systemRunner) Output(ctx context.Context, name string, args ...string) ([]byte, error) { return exec.CommandContext(ctx, name, args...).Output() } @@ -435,9 +435,8 @@ func aptQuote(value string) string { func parseConfiguration(data []byte) map[string]string { settings := make(map[string]string) - scanner := bufio.NewScanner(strings.NewReader(string(data))) - for scanner.Scan() { - line := strings.TrimSpace(scanner.Text()) + for _, raw := range strings.Split(string(data), "\n") { + line := strings.TrimSpace(raw) if line == "" || strings.HasPrefix(line, "//") || strings.HasPrefix(line, "#") { continue } diff --git a/internal/dockerclient/dockerclient.go b/internal/dockerclient/dockerclient.go index 11560ce..e183b97 100644 --- a/internal/dockerclient/dockerclient.go +++ b/internal/dockerclient/dockerclient.go @@ -329,6 +329,7 @@ func (a *Adapter) enable(active *profile.Profile) error { if err != nil { return fmt.Errorf("read Docker client configuration for safe JSON merge: %w", err) } + observedConfig := fingerprintDocument(config, configPresent) metadata, managed, recoveryErr := a.readRecovery() if recoveryErr != nil && !errors.Is(recoveryErr, fs.ErrNotExist) { return fmt.Errorf("read Docker client recovery metadata: %w", recoveryErr) @@ -362,6 +363,9 @@ func (a *Adapter) enable(active *profile.Profile) error { applyDesired(defaults, desired) setChildObject(proxies, "default", defaults) setChildObject(config, "proxies", proxies) + if err := a.requireConfigUnchanged(observedConfig); err != nil { + return err + } if err := a.writeConfig(config); err != nil { return fmt.Errorf("write Docker client proxy defaults after recovery metadata persisted: %w", err) } @@ -380,6 +384,7 @@ func (a *Adapter) disable() error { if err != nil && !errors.Is(err, fs.ErrNotExist) { return fmt.Errorf("read Docker client configuration for restoration: %w", err) } + observedConfig := fingerprintDocument(config, configPresent) if !configPresent { config = make(map[string]json.RawMessage) } @@ -422,6 +427,9 @@ func (a *Adapter) disable() error { } else { setChildObject(config, "proxies", proxies) } + if err := a.requireConfigUnchanged(observedConfig); err != nil { + return err + } if len(config) == 0 && !metadata.ConfigPresent { if err := a.files.Remove(a.configuration.ConfigFile); err != nil && !errors.Is(err, fs.ErrNotExist) { return fmt.Errorf("remove newly created empty Docker client configuration: %w", err) @@ -543,23 +551,6 @@ func applyDesired(defaults map[string]json.RawMessage, desired map[string]string } } } -func recognizesProxyDefaults(config map[string]json.RawMessage) bool { - proxies, _, err := childObject(config, "proxies") - if err != nil { - return false - } - defaults, _, err := childObject(proxies, "default") - if err != nil { - return false - } - for _, key := range append(append([]string(nil), managedKeys...), "ftpProxy") { - var value string - if raw, ok := defaults[key]; ok && json.Unmarshal(raw, &value) == nil && value != "" { - return true - } - } - return false -} type managedValue struct { Present bool `json:"present"` @@ -612,6 +603,27 @@ func fingerprintSnapshot(snapshot map[string]managedValue) string { sum := sha256.Sum256(data) return hex.EncodeToString(sum[:]) } + +func fingerprintDocument(config map[string]json.RawMessage, present bool) string { + data, _ := json.Marshal(struct { + Present bool `json:"present"` + Config map[string]json.RawMessage `json:"config"` + }{Present: present, Config: config}) + sum := sha256.Sum256(data) + return hex.EncodeToString(sum[:]) +} + +func (a *Adapter) requireConfigUnchanged(expected string) error { + current, present, err := a.readConfig() + if err != nil { + return fmt.Errorf("re-read Docker client configuration before atomic replacement: %w", err) + } + if fingerprintDocument(current, present) != expected { + return errors.New("Docker client configuration changed concurrently; refusing to overwrite external updates") + } + return nil +} + func cloneRaw(value json.RawMessage) json.RawMessage { return append(json.RawMessage(nil), value...) } func (a *Adapter) ownership() *Ownership { if !a.configuration.OwnFiles { diff --git a/internal/dockerclient/dockerclient_test.go b/internal/dockerclient/dockerclient_test.go index 5e92284..94f0d4b 100644 --- a/internal/dockerclient/dockerclient_test.go +++ b/internal/dockerclient/dockerclient_test.go @@ -656,6 +656,39 @@ func (f *failingFS) AtomicWrite(path string, data []byte, mode os.FileMode, owne return f.FileSystem.AtomicWrite(path, data, mode, owner) } +type concurrentChangeFS struct { + dockerclient.FileSystem + configPath string + reads int +} + +func (f *concurrentChangeFS) ReadFile(path string) ([]byte, error) { + if path == f.configPath { + f.reads++ + if f.reads == 3 { + if err := os.WriteFile(path, []byte(`{"auths":{"registry.example":{"auth":"external-update"}}}`), 0o600); err != nil { + return nil, err + } + } + } + return f.FileSystem.ReadFile(path) +} + +func TestConcurrentDockerConfigChangeIsNotOverwritten(t *testing.T) { + files := &concurrentChangeFS{FileSystem: dockerclient.OSFileSystem{}} + a := newAdapter(t, files) + files.configPath = a.ConfigPath() + writeConfig(t, a.ConfigPath(), `{"auths":{"registry.example":{"auth":"original"}}}`) + + result := a.SetDesired(context.Background(), true, activeProfile()) + if result.Err == nil || !strings.Contains(result.Err.Error(), "changed concurrently") { + t.Fatalf("concurrent update result = %#v", result) + } + if got := readFile(t, a.ConfigPath()); !strings.Contains(got, "external-update") || strings.Contains(got, "httpProxy") { + t.Fatalf("external Docker config update was overwritten: %s", got) + } +} + type countingFS struct { dockerclient.FileSystem writes int diff --git a/internal/dockerdaemon/dockerdaemon.go b/internal/dockerdaemon/dockerdaemon.go index 3d22478..540dde7 100644 --- a/internal/dockerdaemon/dockerdaemon.go +++ b/internal/dockerdaemon/dockerdaemon.go @@ -15,6 +15,7 @@ import ( "path/filepath" "strings" + "proxyctl/internal/execsafe" "proxyctl/internal/profile" "proxyctl/internal/securefs" "proxyctl/internal/target" @@ -64,7 +65,11 @@ type CommandRunner interface { type ExecRunner struct{} func (ExecRunner) Run(ctx context.Context, name string, args ...string) ([]byte, error) { - output, err := exec.CommandContext(ctx, name, args...).CombinedOutput() + command, err := execsafe.LookPath(name) + if err != nil { + return nil, err + } + output, err := exec.CommandContext(ctx, command, args...).CombinedOutput() if err != nil { return output, fmt.Errorf("%s: %s: %w", name, strings.TrimSpace(string(output)), err) } @@ -336,11 +341,9 @@ func (a *Adapter) enable(ctx context.Context, p profile.Profile) error { return fmt.Errorf("persist Docker daemon recovery metadata before changing configuration: %w", err) } if fingerprint(current) != desiredFingerprint { - mode := os.FileMode(0o600) - if metadata.OriginalExists { - mode = os.FileMode(metadata.OriginalMode) - } - if err := a.files.AtomicWrite(a.configuration.DaemonConfigPath, desired, mode); err != nil { + // Managed proxy URLs may contain credentials. Keep the live configuration + // owner-only; disable restores the original mode with the original data. + if err := a.files.AtomicWrite(a.configuration.DaemonConfigPath, desired, 0o600); err != nil { return fmt.Errorf("write Docker daemon configuration after recovery metadata persisted: %w", err) } } diff --git a/internal/dockerdaemon/dockerdaemon_test.go b/internal/dockerdaemon/dockerdaemon_test.go index 039a45d..8697d62 100644 --- a/internal/dockerdaemon/dockerdaemon_test.go +++ b/internal/dockerdaemon/dockerdaemon_test.go @@ -546,6 +546,9 @@ func TestDockerDaemonPreservesAndRestoresExistingConfiguration(t *testing.T) { if got := a.SetDesired(context.Background(), true, activeProfile()); got.Err != nil { t.Fatal(got.Err) } + if info, err := os.Stat(daemonPath); err != nil || info.Mode().Perm() != 0o600 { + t.Fatalf("managed mode = %v, %v; want owner-only", info, err) + } if got := a.SetDesired(context.Background(), false, nil); got.Err != nil || got.Inspection.State != target.External { t.Fatalf("disable = %#v", got) } diff --git a/internal/execsafe/execsafe.go b/internal/execsafe/execsafe.go new file mode 100644 index 0000000..9c5dba2 --- /dev/null +++ b/internal/execsafe/execsafe.go @@ -0,0 +1,35 @@ +// Package execsafe resolves privileged system commands without consulting the +// caller-controlled PATH. +package execsafe + +import ( + "errors" + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" +) + +var systemDirectories = []string{"/usr/sbin", "/usr/bin", "/sbin", "/bin"} + +// LookPath resolves a bare command name only from standard system directories. +func LookPath(name string) (string, error) { + if name == "" || filepath.Base(name) != name || strings.ContainsRune(name, filepath.Separator) { + return "", fmt.Errorf("unsafe system command name %q", name) + } + for _, directory := range systemDirectories { + candidate := filepath.Join(directory, name) + info, err := os.Stat(candidate) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + continue + } + return "", fmt.Errorf("inspect trusted system command %s: %w", candidate, err) + } + if info.Mode().IsRegular() && info.Mode().Perm()&0o111 != 0 { + return candidate, nil + } + } + return "", fmt.Errorf("%w: trusted system command %s", exec.ErrNotFound, name) +} diff --git a/internal/execsafe/execsafe_test.go b/internal/execsafe/execsafe_test.go new file mode 100644 index 0000000..09827fc --- /dev/null +++ b/internal/execsafe/execsafe_test.go @@ -0,0 +1,31 @@ +package execsafe_test + +import ( + "errors" + "os/exec" + "path/filepath" + "testing" + + "proxyctl/internal/execsafe" +) + +func TestLookPathUsesOnlyTrustedSystemDirectories(t *testing.T) { + path, err := execsafe.LookPath("sh") + if err != nil { + t.Fatal(err) + } + if !filepath.IsAbs(path) || filepath.Base(path) != "sh" { + t.Fatalf("resolved path = %q", path) + } +} + +func TestLookPathRejectsPathsAndMissingCommands(t *testing.T) { + for _, name := range []string{"../systemctl", "/tmp/systemctl", "dir/systemctl", ""} { + if _, err := execsafe.LookPath(name); err == nil { + t.Fatalf("unsafe name %q was accepted", name) + } + } + if _, err := execsafe.LookPath("proxyctl-command-that-does-not-exist"); !errors.Is(err, exec.ErrNotFound) { + t.Fatalf("missing command error = %v", err) + } +} diff --git a/internal/operationlock/lock.go b/internal/operationlock/lock.go index c7c952a..fa838fb 100644 --- a/internal/operationlock/lock.go +++ b/internal/operationlock/lock.go @@ -13,6 +13,10 @@ import ( "proxyctl/internal/securefs" ) +type Locker interface { + Lock(context.Context) (func(), error) +} + type Lock struct { path string uid int @@ -20,6 +24,29 @@ type Lock struct { owned bool } +type Group struct{ locks []Locker } + +func NewGroup(locks ...Locker) *Group { return &Group{locks: append([]Locker(nil), locks...)} } + +func (g *Group) Lock(ctx context.Context) (func(), error) { + var unlocks []func() + for _, lock := range g.locks { + unlock, err := lock.Lock(ctx) + if err != nil { + for i := len(unlocks) - 1; i >= 0; i-- { + unlocks[i]() + } + return nil, err + } + unlocks = append(unlocks, unlock) + } + return func() { + for i := len(unlocks) - 1; i >= 0; i-- { + unlocks[i]() + } + }, nil +} + func New(path string) *Lock { return &Lock{path: path} } func NewOwned(path string, uid, gid int) *Lock { return &Lock{path: path, uid: uid, gid: gid, owned: true} diff --git a/internal/operationlock/lock_test.go b/internal/operationlock/lock_test.go index 06aa54b..777e689 100644 --- a/internal/operationlock/lock_test.go +++ b/internal/operationlock/lock_test.go @@ -10,6 +10,42 @@ import ( "proxyctl/internal/operationlock" ) +type recordingLocker struct { + name string + events *[]string + err error +} + +func (l recordingLocker) Lock(context.Context) (func(), error) { + *l.events = append(*l.events, "lock "+l.name) + if l.err != nil { + return nil, l.err + } + return func() { *l.events = append(*l.events, "unlock "+l.name) }, nil +} + +func TestGroupAcquiresInOrderAndReleasesInReverse(t *testing.T) { + var events []string + group := operationlock.NewGroup( + recordingLocker{name: "system", events: &events}, + recordingLocker{name: "user", events: &events}, + ) + unlock, err := group.Lock(context.Background()) + if err != nil { + t.Fatal(err) + } + unlock() + want := []string{"lock system", "lock user", "unlock user", "unlock system"} + if len(events) != len(want) { + t.Fatalf("events = %v, want %v", events, want) + } + for i := range want { + if events[i] != want[i] { + t.Fatalf("events = %v, want %v", events, want) + } + } +} + func TestLockRejectsSymlinkedStateAncestor(t *testing.T) { root := t.TempDir() victim := filepath.Join(root, "victim") diff --git a/internal/shell/shell.go b/internal/shell/shell.go index 88ed7cd..1b81fe9 100644 --- a/internal/shell/shell.go +++ b/internal/shell/shell.go @@ -323,8 +323,19 @@ func (a *Adapter) disable() error { if metadata.OriginalScript != nil { restored = []byte(*metadata.OriginalScript) } - if err := a.writeOwned(a.configuration.EnvironmentFile, restored); err != nil { - return fmt.Errorf("restore Shell environment: %w", err) + current, err := a.files.ReadFile(a.configuration.EnvironmentFile) + if err != nil { + return fmt.Errorf("read managed Shell environment before restore: %w", err) + } + switch currentFingerprint := fingerprint(current); currentFingerprint { + case fingerprint(restored): + // A previous recovery completed the file write but not metadata cleanup. + case metadata.Fingerprint: + if err := a.writeOwned(a.configuration.EnvironmentFile, restored); err != nil { + return fmt.Errorf("restore Shell environment: %w", err) + } + default: + return errors.New("Shell environment changed after proxyctl applied it; refusing to overwrite current configuration or discard recovery metadata") } if err := a.files.Remove(a.configuration.RecoveryFile); err != nil && !errors.Is(err, fs.ErrNotExist) { return fmt.Errorf("remove Shell recovery metadata after environment changed: %w", err) diff --git a/internal/shell/shell_test.go b/internal/shell/shell_test.go index b44be98..08609a5 100644 --- a/internal/shell/shell_test.go +++ b/internal/shell/shell_test.go @@ -296,6 +296,27 @@ func TestShellTargetContract(t *testing.T) { assertState(t, a, activeProfile(), target.Drifted) }) + t.Run("drifted-environment-refuses-destructive-restore", func(t *testing.T) { + a := newAdapter(t, shellName, nil) + if result := a.SetDesired(context.Background(), true, activeProfile()); result.Err != nil { + t.Fatal(result.Err) + } + changed := "export HTTP_PROXY='http://user-change.example:8080'\n" + if err := os.WriteFile(a.EnvironmentPath(), []byte(changed), 0o600); err != nil { + t.Fatal(err) + } + result := a.SetDesired(context.Background(), false, nil) + if result.Err == nil || result.Inspection.State != target.Drifted { + t.Fatalf("unsafe restore = %#v", result) + } + if got := readFile(t, a.EnvironmentPath()); got != changed { + t.Fatalf("drifted environment overwritten:\n%s", got) + } + if _, err := os.Stat(a.RecoveryPath()); err != nil { + t.Fatalf("recovery metadata removed after refused restore: %v", err) + } + }) + t.Run("validation-and-redaction", func(t *testing.T) { fs := &failingFS{FileSystem: shell.OSFileSystem{}, failWrite: true} a := newAdapterWithFS(t, shellName, nil, fs) diff --git a/internal/target/diagnostic.go b/internal/target/diagnostic.go index 4f8e622..6476e1a 100644 --- a/internal/target/diagnostic.go +++ b/internal/target/diagnostic.go @@ -69,6 +69,14 @@ func readOnlyWritablePath(path string) (bool, string) { } if index == len(components)-1 { _ = unix.Close(child) + if kind != unix.S_IFREG { + return false, componentPath + " is not a regular file" + } + readable, readErr := unix.Openat(current, component, unix.O_RDONLY|unix.O_NONBLOCK|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0) + if readErr != nil { + return false, componentPath + " cannot be read for safe atomic replacement: " + readErr.Error() + } + _ = unix.Close(readable) return writableDirectory(current, currentPath, absolute) } if kind != unix.S_IFDIR { diff --git a/internal/target/diagnostic_test.go b/internal/target/diagnostic_test.go index 4a93cd5..c00dd60 100644 --- a/internal/target/diagnostic_test.go +++ b/internal/target/diagnostic_test.go @@ -10,6 +10,20 @@ import ( "proxyctl/internal/target" ) +func TestReadOnlyWriteAuthorityRejectsUnreadableExistingFile(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root can read files regardless of mode bits") + } + path := filepath.Join(t.TempDir(), "managed.json") + if err := os.WriteFile(path, []byte("{}\n"), 0o200); err != nil { + t.Fatal(err) + } + check := target.ReadOnlyWriteAuthority(path) + if check.OK || !strings.Contains(check.Evidence, "cannot be read") { + t.Fatalf("unreadable destination reported writable: %#v", check) + } +} + func TestReadOnlyWriteAuthorityRejectsSymlinkedFinalAndAncestorWithoutTouchingVictim(t *testing.T) { for _, test := range []struct { name string diff --git a/internal/tui/help_dialog_test.go b/internal/tui/help_dialog_test.go index 52f497a..3971695 100644 --- a/internal/tui/help_dialog_test.go +++ b/internal/tui/help_dialog_test.go @@ -139,7 +139,7 @@ func TestCompleteModelDialogResizeAndHelpPreserveMeaningfulFocus(t *testing.T) { updated, command := model.Update(tea.KeyMsg{Type: tea.KeyEnter}) model = updated.(Model) - if command != nil || len(module.intents) != 1 || module.intents[0].Kind != app.DeleteProfile { + if command == nil || len(module.intents) != 1 || module.intents[0].Kind != app.DeleteProfile { t.Fatalf("resize and Help changed confirmation focus: intents=%#v", module.intents) } } diff --git a/internal/tui/model.go b/internal/tui/model.go index 67ceddc..c7d5ea8 100644 --- a/internal/tui/model.go +++ b/internal/tui/model.go @@ -145,13 +145,24 @@ func (m Model) Update(message tea.Msg) (tea.Model, tea.Cmd) { m.operation.unexpectedClose() return m, m.operation.deferredQuit() } + completedIntent := m.operation.intent m.snapshot = progress.update.Snapshot m.retainTargetSelection() m.retainProfileSelection(m.selectedProfileName, m.selected) + decoded := progress.update.Decode() if firstFailed := m.operation.apply(progress.update); firstFailed != "" { m.selectedTargetID = firstFailed m.detailsOffset = 0 } + if decoded.IsTerminal() && isProfileMutation(completedIntent.Kind) { + if decoded.Kind == app.OperationSucceeded { + m.page = profilesPage + m.form = formState{} + m.status = decoded.Status.Summary + } else if decoded.Err != nil { + m.status = "Error: " + decoded.Err.Error() + } + } if m.operation.confirmation != nil { m.page = mainPage if m.help.open { @@ -399,7 +410,7 @@ func (m Model) updateProfiles(key tea.KeyMsg) (tea.Model, tea.Cmd) { case confirmationAccepted: name := m.profileDelete.name m.profileDelete = nil - m.applyProfileMutation(app.Intent{Kind: app.DeleteProfile, Name: name, Confirm: true}, "") + return m, m.startOperation(app.Intent{Kind: app.DeleteProfile, Name: name, Confirm: true}) case confirmationCancelled: m.profileDelete = nil } @@ -519,13 +530,8 @@ func (m Model) updateForm(key tea.KeyMsg) (tea.Model, tea.Cmd) { } normalized := m.form.validation.Normalized intent := app.Intent{Kind: kind, Name: m.form.originalName, Profile: normalized} - result := m.applyProfileMutation(intent, normalized.Name) - if result.Err != nil { - return m, nil - } - m.page = profilesPage - m.form = formState{} - return m, nil + m.selectedProfileName = normalized.Name + return m, m.startOperation(intent) case tea.KeyCtrlD: address, token, started := m.form.startProbe() if !started { @@ -556,20 +562,13 @@ func (m Model) beginProfileActivation(intent app.Intent) (tea.Model, tea.Cmd) { return m, m.startOperation(intent) } -func (m *Model) applyProfileMutation(intent app.Intent, preferredName string) app.Result { - formerName, formerIndex := m.selectedProfileName, m.selected - result := m.application.Dispatch(intent) - m.snapshot = result.Snapshot - if preferredName == "" { - preferredName = formerName +func isProfileMutation(kind app.IntentKind) bool { + switch kind { + case app.CreateProfile, app.UpdateProfile, app.DeleteProfile: + return true + default: + return false } - m.retainProfileSelection(preferredName, formerIndex) - if result.Err != nil { - m.status = "Error: " + result.Err.Error() - } else { - m.status = result.Notice - } - return result } func (m Model) selectedProfile() (profile.Profile, bool) { diff --git a/internal/tui/model_test.go b/internal/tui/model_test.go index 41135d1..c1a44dd 100644 --- a/internal/tui/model_test.go +++ b/internal/tui/model_test.go @@ -149,7 +149,20 @@ func updateKey(model Model, key string) Model { msg = tea.KeyMsg{Type: keyType} } updated, _ := model.Update(msg) - return updated.(Model) + model = updated.(Model) + // In-memory test modules publish terminal operation updates synchronously. + // Drain only updates already available; real in-flight operation tests remain + // under their explicit channel control. + for model.operation.active && isProfileMutation(model.operation.intent.Kind) { + select { + case update, open := <-model.operation.updates: + updated, _ = model.Update(operationProgress{updates: model.operation.updates, update: update, closed: !open}) + model = updated.(Model) + default: + return model + } + } + return model } func updateRunes(model Model, value string) Model { diff --git a/internal/tui/operation_coordinator.go b/internal/tui/operation_coordinator.go index b1c5517..86f48af 100644 --- a/internal/tui/operation_coordinator.go +++ b/internal/tui/operation_coordinator.go @@ -22,6 +22,7 @@ type operationCoordinator struct { active bool intent app.Intent updates <-chan app.OperationUpdate + cancel func() progress map[string]app.TargetProgressPhase status app.OperationStatus currentError *operationError @@ -85,6 +86,7 @@ func (o *operationCoordinator) start(application app.InteractiveModule, intent a o.active = true o.intent = intent o.updates = stream.Updates + o.cancel = stream.Cancel o.progress = make(map[string]app.TargetProgressPhase) o.spinner = spinner.New(spinner.WithSpinner(applyingSpinner)) if intent.TargetID != "" { @@ -100,6 +102,8 @@ func operationStartingSummary(kind app.IntentKind) string { return "Checking Target availability and state…" case app.ActivateProfile: return "Changing Active Profile…" + case app.CreateProfile, app.UpdateProfile, app.DeleteProfile: + return "Saving Proxy Profile changes…" default: return "Starting Target mutation…" } @@ -171,6 +175,8 @@ func relatedResolution(failed, success app.Intent) bool { return success.Kind == app.RefreshTargets case failed.Kind == app.ActivateProfile: return success.Kind == app.ActivateProfile && failed.Name == success.Name + case isProfileMutation(failed.Kind): + return isProfileMutation(success.Kind) && failed.Name == success.Name case isTargetMutation(failed.Kind): return isTargetMutation(success.Kind) && failed.TargetID != "" && failed.TargetID == success.TargetID default: @@ -188,9 +194,19 @@ func isTargetMutation(kind app.IntentKind) bool { } func (o *operationCoordinator) finish() { + // start seeds speculative Running progress before the stream publishes its + // first update. A terminal update may contain no replacement progress (most + // notably a confirmation request), so never let that transient phase outlive + // the operation that created it. + for targetID, phase := range o.progress { + if phase == app.TargetProgressRunning { + delete(o.progress, targetID) + } + } o.active = false o.intent = app.Intent{} o.updates = nil + o.cancel = nil } func (o *operationCoordinator) unexpectedClose() { @@ -225,6 +241,7 @@ func (o *operationCoordinator) confirmedIntent() (app.Intent, bool) { } intent := *o.confirmationIntent intent.Confirm = true + intent.ConfirmationToken = o.confirmation.Token o.confirmation = nil o.confirmationDialog = confirmationDialog{} o.confirmationIntent = nil @@ -245,6 +262,9 @@ func (o operationCoordinator) deferredQuit() tea.Cmd { func (o *operationCoordinator) requestQuit() tea.Cmd { o.quitRequested = true if o.active { + if o.cancel != nil { + o.cancel() + } return nil } return tea.Quit diff --git a/internal/tui/profile_form.go b/internal/tui/profile_form.go index 9383e5c..9094beb 100644 --- a/internal/tui/profile_form.go +++ b/internal/tui/profile_form.go @@ -199,8 +199,6 @@ func (f *formState) toggleMode() tea.Cmd { return f.rebuildInputs(0) } -func (f formState) fieldCount() int { return len(f.inputs) } - func (f formState) currentField() profile.Field { if f.field < 0 || f.field >= len(f.inputs) { return "" diff --git a/internal/tui/profile_management_page_test.go b/internal/tui/profile_management_page_test.go index 932ffbf..a381be8 100644 --- a/internal/tui/profile_management_page_test.go +++ b/internal/tui/profile_management_page_test.go @@ -146,8 +146,8 @@ func TestCompleteModelProfileDeletionDialogIsSafeByDefaultAndSupportsExplicitSel model = updateKey(model, "right") updated, command = model.Update(tea.KeyMsg{Type: tea.KeyEnter}) model = updated.(Model) - if command != nil || len(module.intents) != 1 || module.intents[0].Kind != app.DeleteProfile || module.intents[0].Name != "office" || !module.intents[0].Confirm { - t.Fatalf("focused Delete did not dispatch the confirmed mutation: intents=%#v", module.intents) + if command == nil || len(module.intents) != 1 || module.intents[0].Kind != app.DeleteProfile || module.intents[0].Name != "office" || !module.intents[0].Confirm { + t.Fatalf("focused Delete did not start the confirmed mutation: intents=%#v", module.intents) } assertUsableFrame(t, model.View(), 80, 24, "> remote", "Proxy: http://remote.example:8080") } diff --git a/internal/tui/target_test.go b/internal/tui/target_test.go index b58612d..6aaf6a2 100644 --- a/internal/tui/target_test.go +++ b/internal/tui/target_test.go @@ -76,8 +76,8 @@ func TestTargetRestartConfirmationCancelsWithoutConfirmedMutation(t *testing.T) } updated, command = model.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'n'}}) model = updated.(Model) - if command != nil || len(module.intents) != 1 || strings.Contains(model.View(), "restart immediately") { - t.Fatalf("cancel mutated = intents %#v\n%s", module.intents, model.View()) + if command != nil || len(module.intents) != 1 || strings.Contains(model.View(), "restart immediately") || strings.Contains(model.View(), "Applying") { + t.Fatalf("cancel mutated or left stale progress = intents %#v\n%s", module.intents, model.View()) } updated, command = model.Update(tea.KeyMsg{Type: tea.KeySpace}) diff --git a/scripts/test-hermetic.sh b/scripts/test-hermetic.sh index ec443cd..c0738a6 100755 --- a/scripts/test-hermetic.sh +++ b/scripts/test-hermetic.sh @@ -76,7 +76,7 @@ snapshot() { snapshot >"$work/before" ( cd "$repo" - go test ./... + go test -count=1 ./... ) snapshot >"$work/after" cmp "$work/before" "$work/after" diff --git a/scripts/verify-clean-machine.sh b/scripts/verify-clean-machine.sh index 998a5ba..7de03b1 100755 --- a/scripts/verify-clean-machine.sh +++ b/scripts/verify-clean-machine.sh @@ -17,15 +17,41 @@ snapshot_host_path() { return fi - # Avoid pipelines: traversal, stat, hash, and sort failures must stop the - # verification rather than being mistaken for an unchanged host snapshot. - find -H "$path" -xdev \ + stat --printf='top-metadata\t%F\t%a\t%u\t%g\t%N\n' -- "$path" + if { [ -d "$path" ] && { [ ! -r "$path" ] || [ ! -x "$path" ]; }; } || \ + { [ ! -d "$path" ] && [ ! -r "$path" ]; }; + then + printf 'unreadable\t%s\n' "$path" + return + fi + + traversal_complete=true + if ! find -H "$path" -xdev \ -exec stat --printf='metadata\t%F\t%a\t%u\t%g\t%N\n' -- {} + \ - >"$work/host-snapshot-metadata" + >"$work/host-snapshot-metadata" 2>/dev/null + then + traversal_complete=false + fi LC_ALL=C sort "$work/host-snapshot-metadata" - find -H "$path" -xdev -type f -exec sha256sum -- {} + \ - >"$work/host-snapshot-content" + + if ! find -H "$path" -xdev \ + \( \( -type d \( ! -readable -o ! -executable \) \) -o \( -type f ! -readable \) \) \ + -printf='unreadable\t%p\n' >"$work/host-snapshot-unreadable" 2>/dev/null + then + traversal_complete=false + fi + LC_ALL=C sort "$work/host-snapshot-unreadable" + + if ! find -H "$path" -xdev -type f -readable -exec sha256sum -- {} + \ + >"$work/host-snapshot-content" 2>/dev/null + then + traversal_complete=false + fi LC_ALL=C sort "$work/host-snapshot-content" + + if [ "$traversal_complete" = false ]; then + printf 'unreadable-descendant\t%s\n' "$path" + fi } snapshot_host() {