From f4ae978f902fa64ee01173fb0dd6b6c5d8ae6039 Mon Sep 17 00:00:00 2001 From: Sev7eNup <79143581+Sev7eNup@users.noreply.github.com> Date: Sat, 24 Oct 2026 17:36:45 +0200 Subject: [PATCH] fix: reject delayed shutdown without explicit force Reject a positive shutdown or restart delay when force=false before opening a remote session. Windows implicitly enables force for a positive shutdown timer. Document the separate Delay-step alternative in both UI languages, the activity reference and both documentation languages. Cover safe commands and rejected combinations without executing shutdown. Fixes #613 --- .../Embedded/activity-config-reference.json | 4 +-- .../Activities/PowerManagementActivity.cs | 9 +++++- .../content/de/activities-reference.md | 2 ++ .../content/en/activities-reference.md | 2 ++ .../propertyConfigs.activities.test.tsx | 4 +-- .../src/i18n/locales/de/properties.json | 2 +- .../src/i18n/locales/en/properties.json | 2 +- .../Activities/BuildScriptTests.cs | 7 +++-- .../PowerManagementActivityTests.cs | 28 +++++++++++++++++-- 9 files changed, 47 insertions(+), 13 deletions(-) diff --git a/src/NodePilot.Core/Activities/Embedded/activity-config-reference.json b/src/NodePilot.Core/Activities/Embedded/activity-config-reference.json index eabacdba..5f77f34f 100644 --- a/src/NodePilot.Core/Activities/Embedded/activity-config-reference.json +++ b/src/NodePilot.Core/Activities/Embedded/activity-config-reference.json @@ -265,8 +265,8 @@ "description": "Shut down, restart or otherwise power-control a machine (remote).", "configKeys": [ { "key": "action", "type": "string", "required": true, "description": "shutdown | restart | logoff | abort | hibernate." }, - { "key": "delaySeconds", "type": "number", "required": false, "description": "Delay before the action." }, - { "key": "force", "type": "boolean", "required": false, "description": "Force running applications to close." }, + { "key": "delaySeconds", "type": "number", "required": false, "description": "Shutdown/restart countdown in seconds, default 0. Positive values require force=true. To wait with force=false, use a separate Delay step and set delaySeconds=0." }, + { "key": "force", "type": "boolean", "required": false, "description": "Force running applications to close, default true. When false, delaySeconds must be 0." }, { "key": "message", "type": "string", "required": false, "description": "Message shown to logged-on users." } ] }, diff --git a/src/NodePilot.Engine/Activities/PowerManagementActivity.cs b/src/NodePilot.Engine/Activities/PowerManagementActivity.cs index 4cb3b0f3..2c399ed0 100644 --- a/src/NodePilot.Engine/Activities/PowerManagementActivity.cs +++ b/src/NodePilot.Engine/Activities/PowerManagementActivity.cs @@ -13,7 +13,7 @@ namespace NodePilot.Engine.Activities; /// /// Config: /// action string, required — "shutdown" | "restart" | "logoff" | "abort" | "hibernate" -/// delaySeconds int, default 0 — /t N countdown before execution (shutdown/restart only) +/// delaySeconds int, default 0 — /t N countdown (shutdown/restart only; positive values require force) /// force bool, default true — /f, closes running apps without prompting /// message optional shutdown or restart dialog comment /// @@ -112,6 +112,13 @@ private static string GuardNativeExit(string invocation) => private static string BuildShutdownInvocation(string opFlag, int delay, bool force, string? message) { + // Windows implicitly enables /f whenever /t is positive. + if (delay > 0 && !force) + throw new InvalidOperationException( + "Power Management: delaySeconds must be 0 when force is false. " + + "Windows forces applications closed with a positive shutdown delay. " + + "Use a separate Delay step before this activity to wait without forcing applications closed."); + // shutdown.exe flags are bare tokens in PowerShell — the PS parser does not try to // resolve them as cmdlets because of the `&` call operator. Only the user-supplied // message needs PowerShellQuoter because it can contain apostrophes. diff --git a/src/nodepilot-docs-ui/content/de/activities-reference.md b/src/nodepilot-docs-ui/content/de/activities-reference.md index 1394724e..2c8da616 100644 --- a/src/nodepilot-docs-ui/content/de/activities-reference.md +++ b/src/nodepilot-docs-ui/content/de/activities-reference.md @@ -172,6 +172,8 @@ Das Timeout begrenzt das Warten auf den laufenden Prozess. Nach seinem Ende wird - **Config:** `action` (shutdown/restart/logoff/abort/hibernate), `delaySeconds`, `force`, `message` - **Outputs:** — +Bei Shutdown und Restart ist `force` standardmäßig `true`. Mit `force=false` muss `delaySeconds` auf `0` stehen; ein positiver Wert wird vor der Ausführung abgelehnt. [Windows erzwingt bei einem positiven Shutdown-Timer das Schließen von Apps](https://learn.microsoft.com/en-us/windows-server/administration/windows-commands/shutdown). Zum Warten ohne erzwungenes Schließen eine separate `delay`-Aktivität vor diesen Schritt setzen und `delaySeconds=0` beibehalten. + ## `scheduledTask` **Remote.** Erfordert meist Admin auf dem Ziel. diff --git a/src/nodepilot-docs-ui/content/en/activities-reference.md b/src/nodepilot-docs-ui/content/en/activities-reference.md index b836ae80..11ca9850 100644 --- a/src/nodepilot-docs-ui/content/en/activities-reference.md +++ b/src/nodepilot-docs-ui/content/en/activities-reference.md @@ -167,6 +167,8 @@ The timeout bounds the wait on the running process. After it exits, reading cont - **Config:** `action` (shutdown/restart/logoff/abort/hibernate), `delaySeconds`, `force`, `message` - **Outputs:** — +For shutdown and restart, `force` defaults to `true`. With `force=false`, `delaySeconds` must be `0`; a positive value is rejected before execution. [Windows implicitly forces apps closed when its shutdown timer is positive](https://learn.microsoft.com/en-us/windows-server/administration/windows-commands/shutdown). To wait without forcing apps closed, add a separate `delay` activity before this step and keep `delaySeconds=0`. + ## `scheduledTask` **Remote.** Usually requires admin rights on the target. diff --git a/src/nodepilot-ui/src/__tests__/components/properties/propertyConfigs.activities.test.tsx b/src/nodepilot-ui/src/__tests__/components/properties/propertyConfigs.activities.test.tsx index 6a924817..7bcf5e11 100644 --- a/src/nodepilot-ui/src/__tests__/components/properties/propertyConfigs.activities.test.tsx +++ b/src/nodepilot-ui/src/__tests__/components/properties/propertyConfigs.activities.test.tsx @@ -745,14 +745,14 @@ describe('PowerManagementConfig', () => { it('defaultsToShutdown_showsDelayAndForce', () => { wrap(); expect(screen.getByText('Delay (seconds)')).toBeInTheDocument(); - expect(screen.getByText(/Force close running apps/i)).toBeInTheDocument(); + expect(screen.getByText('Force close running apps')).toBeInTheDocument(); }); it('logoffAction_hidesDelayAndForce', () => { // logoff, abort and hibernate ignore delay and force, so those fields stay hidden. wrap(); expect(screen.queryByText('Delay (seconds)')).not.toBeInTheDocument(); - expect(screen.queryByText(/Force close running apps/i)).not.toBeInTheDocument(); + expect(screen.queryByText('Force close running apps')).not.toBeInTheDocument(); }); it('actionChange_emitsPatch', () => { diff --git a/src/nodepilot-ui/src/i18n/locales/de/properties.json b/src/nodepilot-ui/src/i18n/locales/de/properties.json index fda9afbd..06bbb468 100644 --- a/src/nodepilot-ui/src/i18n/locales/de/properties.json +++ b/src/nodepilot-ui/src/i18n/locales/de/properties.json @@ -350,7 +350,7 @@ "forceClose": "Laufende Apps zwangsweise schließen", "message": "Nachricht (optional)", "messagePlaceholder": "Geplante Wartung — bitte speichern Sie Ihre Arbeit", - "delayHint": "Bei Targets, auf denen NodePilot selbst läuft, empfiehlt sich ein Delay größer 0, damit der Step sauber zurückkehren kann, bevor das OS herunterfährt.", + "delayHint": "Eine Verzögerung größer 0 erfordert „Laufende Apps zwangsweise schließen“. Zum Warten ohne erzwungenes Schließen einen separaten Delay-Schritt vor diese Aktivität setzen und diese Verzögerung auf 0 stellen.", "forceCloseHint": "Offene Anwendungen ohne Rückfrage schließen (/f)" }, "waitForCondition": { diff --git a/src/nodepilot-ui/src/i18n/locales/en/properties.json b/src/nodepilot-ui/src/i18n/locales/en/properties.json index ea994261..fc5ed1d6 100644 --- a/src/nodepilot-ui/src/i18n/locales/en/properties.json +++ b/src/nodepilot-ui/src/i18n/locales/en/properties.json @@ -350,7 +350,7 @@ "forceClose": "Force close running apps", "message": "Message (optional)", "messagePlaceholder": "Scheduled maintenance — please save your work", - "delayHint": "On targets running NodePilot itself, a delay greater than 0 is advisable so the step can return cleanly before the OS shuts down.", + "delayHint": "A delay greater than 0 requires ‘Force close running apps’. To wait without forcing apps closed, add a separate Delay step before this activity and set this delay to 0.", "forceCloseHint": "Close open applications without asking (/f)" }, "waitForCondition": { diff --git a/tests/NodePilot.Engine.Tests/Activities/BuildScriptTests.cs b/tests/NodePilot.Engine.Tests/Activities/BuildScriptTests.cs index 943f3047..0fd59e11 100644 --- a/tests/NodePilot.Engine.Tests/Activities/BuildScriptTests.cs +++ b/tests/NodePilot.Engine.Tests/Activities/BuildScriptTests.cs @@ -537,11 +537,12 @@ public async Task PowerManagement_DelayAndMessage_EmittedAsArgs() } [Fact] - public async Task PowerManagement_ForceFalse_OmitsForceFlag() + public async Task PowerManagement_ForceFalseWithDelay_RejectsImplicitForce() { var config = ParseConfig("{\"action\":\"shutdown\",\"force\":false,\"delaySeconds\":30}"); - var script = await ExecuteAndCaptureScript(CreatePowerMgmt(), config); - ShutdownInvocation(script).Should().Be("& shutdown.exe /s /t 30"); + var act = () => ExecuteAndCaptureScript(CreatePowerMgmt(), config); + await act.Should().ThrowAsync() + .WithMessage("*delaySeconds must be 0 when force is false*"); } [Fact] diff --git a/tests/NodePilot.Engine.Tests/Activities/PowerManagementActivityTests.cs b/tests/NodePilot.Engine.Tests/Activities/PowerManagementActivityTests.cs index 0e8e3c74..c72bde91 100644 --- a/tests/NodePilot.Engine.Tests/Activities/PowerManagementActivityTests.cs +++ b/tests/NodePilot.Engine.Tests/Activities/PowerManagementActivityTests.cs @@ -93,6 +93,24 @@ await CreateActivity().ExecuteAsync(Ctx(), // ---- Error cases ---- + [Theory] + [InlineData("shutdown", 1)] + [InlineData("shutdown", 30)] + [InlineData("restart", 1)] + [InlineData("restart", 30)] + [InlineData("Restart", 30)] + public async Task PositiveDelayWithoutForce_RejectsBeforeExecutingCommand(string action, int delaySeconds) + { + var act = () => CreateActivity().ExecuteAsync(Ctx(), + JsonSerializer.SerializeToElement(new { action, delaySeconds, force = false }), CancellationToken.None); + + await act.Should().ThrowAsync() + .WithMessage("*delaySeconds must be 0 when force is false*"); + _capturedScript.Should().BeNull(); + _sessionFactory.Verify(f => f.CreateSessionAsync( + It.IsAny(), It.IsAny(), It.IsAny()), Times.Never); + } + // Defence-in-depth: PowerManagement is all-destructive (shutdown/restart/logoff/ // hibernate), so the backend must NOT silently default a missing `action`. The UI // persists the visual default on first render to heal the common case; this throw @@ -193,14 +211,18 @@ await CreateActivity().ExecuteAsync(Ctx(), _capturedScript.Should().Contain("/f"); } - [Fact] - public async Task Shutdown_ForceFalse_NoSlashF() + [Theory] + [InlineData("shutdown", "/s", 0)] + [InlineData("restart", "/r", 0)] + [InlineData("shutdown", "/s", -5)] + public async Task NonPositiveDelayWithoutForce_UsesZeroTimerAndNoSlashF(string action, string flag, int delaySeconds) { _capturedScript = null; await CreateActivity().ExecuteAsync(Ctx(), - Cfg("{\"action\": \"shutdown\", \"force\": false}"), + JsonSerializer.SerializeToElement(new { action, delaySeconds, force = false }), CancellationToken.None); _capturedScript.Should().NotContain("/f"); + _capturedScript.Should().Contain($"& shutdown.exe {flag} /t 0"); } [Fact]