fix(kindle-dedrm): compare firewall rule state against 'True', not truthiness

plugins/kindle-dedrm/.claude-plugin/plugin.json

@@ -1,7 +1,7 @@
 {
   "$schema": "https://json.schemastore.org/claude-code-plugin-manifest.json",
   "name": "kindle-dedrm",
-  "version": "0.7.6",
+  "version": "0.7.7",
   "description": "Manage the Kindle for PC 2.8.0 + Calibre DeDRM workflow for personal-use ebook DRM removal on books you own (Windows only). Action router with setup, sync, update, cleanup, and status, each state mutation paired with a documented compensating reversal.",
   "author": {
     "name": "Melodic Software",

plugins/kindle-dedrm/CHANGELOG.md

@@ -3,6 +3,32 @@
 All notable changes to the `kindle-dedrm` plugin are documented here. Format follows
 [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); this plugin uses semantic versioning.
 
+## [0.7.7]
+
+### Fixed
+
+- **`firewall.ps1` stops reading every rule as enabled.** `Get-NetFirewallRule`'s `Enabled`
+  property is a NetSecurity enum whose members are `True = 1` and `False = 2`, so both values are
+  non-zero and both coerce to boolean `$true`. The `enable` action's `if (-not $rule.Enabled)` was
+  therefore always false: a disabled Kindle-blocking rule was reported "already enabled, no
+  change" and never re-enabled, defeating the point of the action. The `disable` action's
+  `if ($rule.Enabled)` had the mirror fault and called `Disable-NetFirewallRule` against rules
+  that were already disabled. Both now compare against `'True'`, which also holds when the
+  property arrives already stringified. Confirmed on Windows 11 against a live disabled rule:
+  `[int]$rule.Enabled` is `2` and `[bool]$rule.Enabled` is `True`. The state display at
+  `Show-State` was audited under the same suspicion and left alone: it interpolates the property
+  for output, which renders the member name (`Enabled=False`) correctly.
+
+### Added
+
+- **A Pester suite for `firewall.ps1`** at `skills/manage/tests/firewall.Tests.ps1`, the plugin's
+  first. It lifts the two real guard expressions out of the script's AST and evaluates them
+  against enum-valued and string-valued rule objects, because the branches themselves sit behind
+  an elevation gate that a test cannot pass and behind a `Test-IsElevated` defined inside the
+  script that shadows any injected stand-in. The `check` action is additionally driven end to end
+  in a child process. Run with
+  `Invoke-Pester -Path plugins/kindle-dedrm/skills/manage/tests`.
+
 ## [0.7.6]
 
 ### Changed

plugins/kindle-dedrm/skills/manage/scripts/firewall.ps1

@@ -69,7 +69,12 @@ switch ($Action) {
         }
         $rule = Get-Rule
         if ($rule) {
-            if (-not $rule.Enabled) {
+            # Compared against 'True', never tested for truthiness. Get-NetFirewallRule's
+            # Enabled is a NetSecurity enum whose members are True = 1 and False = 2, so
+            # BOTH are non-zero and both coerce to boolean $true. `-not $rule.Enabled` was
+            # therefore always false and a disabled rule was never re-enabled. The string
+            # comparison also holds when the property arrives already stringified.
+            if ($rule.Enabled -ne 'True') {
                 Enable-NetFirewallRule -DisplayName $RuleName | Out-Null
                 Write-Output '[firewall] re-enabled existing rule'
             } else {
@@ -97,7 +102,9 @@ switch ($Action) {
             Write-Output '[firewall] not present — nothing to disable'
             exit 0
         }
-        if ($rule.Enabled) {
+        # Same enum hazard as the enable branch above: a bare truthiness test took the
+        # disable path even for a rule that was already disabled.
+        if ($rule.Enabled -eq 'True') {
             Disable-NetFirewallRule -DisplayName $RuleName | Out-Null
             Write-Output '[firewall] disabled (rule retained — re-enable when sync is done)'
         } else {

plugins/kindle-dedrm/skills/manage/tests/firewall.Tests.ps1

@@ -0,0 +1,213 @@
+#Requires -Version 7.4
+#Requires -Modules @{ ModuleName = 'Pester'; ModuleVersion = '5.7.0' }
+<#
+.SYNOPSIS
+Tests for skills/manage/scripts/firewall.ps1.
+
+.DESCRIPTION
+Pins the fix for the NetSecurity enum hazard (#3368). `Get-NetFirewallRule`'s
+`Enabled` property is an enum whose members are True = 1 and False = 2, so BOTH
+values are non-zero and BOTH coerce to boolean $true. The script used to test it
+for truthiness, which meant `enable` reported "already enabled" against a
+disabled rule and never re-enabled it, and `disable` called
+`Disable-NetFirewallRule` against a rule that was already disabled.
+
+.NOTES
+Run from the repository root:
+
+    Invoke-Pester -Path plugins/kindle-dedrm/skills/manage/tests -Output Detailed
+
+The `enable` and `disable` branches sit behind an elevation gate that exits 2
+before reaching the guard, and `Test-IsElevated` is defined inside the script,
+so it shadows anything a test could inject. Those guards are therefore exercised
+by lifting the REAL condition expressions out of the script's AST and evaluating
+them against a rule object, rather than by asserting on a copy of the source
+text. The `check` action needs no elevation and is driven end to end.
+#>
+
+# Mirrors Microsoft.PowerShell.Cmdletization.GeneratedTypes.NetSecurity.Enabled,
+# which only exists once the Windows-only NetSecurity module is loaded. Declared
+# locally so the suite runs anywhere; the first test cross-checks the two where
+# the real type is available.
+enum FakeNetSecurityEnabled {
+    True = 1
+    False = 2
+}
+
+BeforeAll {
+    # Separate child segments, not one 'scripts\firewall.ps1' string: Join-Path
+    # does not split a child argument on `\`, so on non-Windows pwsh the
+    # backslash would survive as an ordinary filename character and the path
+    # would not resolve.
+    $script:ScriptPath = Join-Path (Split-Path -Parent $PSScriptRoot) 'scripts' 'firewall.ps1'
+
+    # Returns the literal `if` condition guarding the enum test inside one switch
+    # clause of firewall.ps1, as a runnable scriptblock plus its source text.
+    function Get-EnabledGuard {
+        param(
+            [Parameter(Mandatory)] [string] $Path,
+            [Parameter(Mandatory)] [string] $Action
+        )
+        $ast = [System.Management.Automation.Language.Parser]::ParseFile(
+            $Path, [ref]$null, [ref]$null)
+        $switchAst = $ast.Find(
+            { param($n) $n -is [System.Management.Automation.Language.SwitchStatementAst] },
+            $true)
+        $clause = $switchAst.Clauses | Where-Object { $_.Item1.Extent.Text -eq "'$Action'" }
+        if (-not $clause) { throw "No '$Action' clause in $Path." }
+        $ifAst = $clause.Item2.Find({
+                param($n)
+                $n -is [System.Management.Automation.Language.IfStatementAst] -and
+                $n.Clauses[0].Item1.Extent.Text -match '\$rule\.Enabled'
+            }, $true)
+        if (-not $ifAst) { throw "No `$rule.Enabled guard in the '$Action' clause of $Path." }
+        $text = $ifAst.Clauses[0].Item1.Extent.Text
+        return [pscustomobject]@{
+            Text  = $text
+            Guard = [scriptblock]::Create($text)
+        }
+    }
+}
+
+Describe 'firewall.ps1' {
+    Context 'the Enabled enum this suite stands in for' {
+        It 'matches the real NetSecurity enum: False is 2, non-zero, and renders "False"' {
+            Import-Module NetSecurity -ErrorAction SilentlyContinue
+            $real = 'Microsoft.PowerShell.Cmdletization.GeneratedTypes.NetSecurity.Enabled' -as [type]
+            if (-not $real) {
+                Set-ItResult -Skipped -Because 'the Windows-only NetSecurity module is unavailable'
+                return
+            }
+            [int]$real::False | Should -Be ([int][FakeNetSecurityEnabled]::False)
+            [int]$real::True | Should -Be ([int][FakeNetSecurityEnabled]::True)
+            "$($real::False)" | Should -Be 'False'
+            # The whole defect in one line: a DISABLED rule is boolean true.
+            [bool]$real::False | Should -BeTrue
+        }
+    }
+
+    Context 'enable guard' {
+        It 'fires for a disabled rule so the rule is re-enabled' {
+            $guard = Get-EnabledGuard -Path $script:ScriptPath -Action 'enable'
+            $rule = [pscustomobject]@{ Enabled = [FakeNetSecurityEnabled]::False }
+            [bool](& $guard.Guard) | Should -BeTrue -Because `
+                "'$($guard.Text)' must treat a disabled rule as needing re-enabling"
+        }
+
+        It 'does not fire for an already-enabled rule' {
+            $guard = Get-EnabledGuard -Path $script:ScriptPath -Action 'enable'
+            $rule = [pscustomobject]@{ Enabled = [FakeNetSecurityEnabled]::True }
+            [bool](& $guard.Guard) | Should -BeFalse
+        }
+
+        It 'is not a bare truthiness test' {
+            $guard = Get-EnabledGuard -Path $script:ScriptPath -Action 'enable'
+            $guard.Text | Should -BeLike '*True*'
+        }
+    }
+
+    Context 'disable guard' {
+        It 'does not fire for an already-disabled rule' {
+            $guard = Get-EnabledGuard -Path $script:ScriptPath -Action 'disable'
+            $rule = [pscustomobject]@{ Enabled = [FakeNetSecurityEnabled]::False }
+            [bool](& $guard.Guard) | Should -BeFalse -Because `
+                "'$($guard.Text)' must not call Disable-NetFirewallRule on a disabled rule"
+        }
+
+        It 'fires for an enabled rule' {
+            $guard = Get-EnabledGuard -Path $script:ScriptPath -Action 'disable'
+            $rule = [pscustomobject]@{ Enabled = [FakeNetSecurityEnabled]::True }
+            [bool](& $guard.Guard) | Should -BeTrue
+        }
+
+        It 'is not a bare truthiness test' {
+            $guard = Get-EnabledGuard -Path $script:ScriptPath -Action 'disable'
+            $guard.Text | Should -BeLike '*True*'
+        }
+    }
+
+    Context 'both guards tolerate the other shapes Enabled arrives in' {
+        # Why the comparison is against the STRING 'True' rather than the
+        # fully-qualified NetSecurity type literal: the literal only resolves
+        # once the Windows-only NetSecurity module is loaded, while `-eq 'True'`
+        # holds for the enum, for a CIM path that hands the property back
+        # already stringified, and for a plain boolean.
+        It 'reads plain "False"/"True" strings the same way' {
+            $enable = Get-EnabledGuard -Path $script:ScriptPath -Action 'enable'
+            $disable = Get-EnabledGuard -Path $script:ScriptPath -Action 'disable'
+
+            $rule = [pscustomobject]@{ Enabled = 'False' }
+            [bool](& $enable.Guard) | Should -BeTrue
+            [bool](& $disable.Guard) | Should -BeFalse
+
+            $rule = [pscustomobject]@{ Enabled = 'True' }
+            [bool](& $enable.Guard) | Should -BeFalse
+            [bool](& $disable.Guard) | Should -BeTrue
+        }
+
+        # Deliberately weaker than its siblings, and labelled so no reader
+        # mistakes it for a regression pin. With a [bool] on the LEFT, PowerShell
+        # converts the RIGHT operand to [bool], and every non-empty string
+        # converts to $true -- so 'True', 'False', and any other literal behave
+        # identically here, and the guard collapses to the truthiness test this
+        # PR removed. The case therefore proves only that the fix did not BREAK
+        # a boolean-valued property; it cannot discriminate the fix from the
+        # defect, because a plain boolean never had the enum's True = 1 /
+        # False = 2 aliasing the defect depended on.
+        It 'does not break when Enabled arrives as a plain boolean' {
+            $enable = Get-EnabledGuard -Path $script:ScriptPath -Action 'enable'
+            $disable = Get-EnabledGuard -Path $script:ScriptPath -Action 'disable'
+
+            $rule = [pscustomobject]@{ Enabled = $false }
+            [bool](& $enable.Guard) | Should -BeTrue
+            [bool](& $disable.Guard) | Should -BeFalse
+
+            $rule = [pscustomobject]@{ Enabled = $true }
+            [bool](& $enable.Guard) | Should -BeFalse
+            [bool](& $disable.Guard) | Should -BeTrue
+        }
+    }
+
+    Context 'check action, end to end' {
+        # Run in a child pwsh: firewall.ps1 calls `exit`, which escapes an
+        # in-process `& $ScriptPath` and surfaces as
+        # "A 'break' or 'continue' statement ... escaped from your code",
+        # aborting the WHOLE container with no results rather than failing one
+        # test. The child gets a Get-NetFirewallRule stub in its global scope,
+        # which the script picks up because it defines no function of that name
+        # itself.
+        BeforeAll {
+            function Invoke-FirewallCheck {
+                param([Parameter(Mandatory)] [string] $EnabledMember)
+                $child = @"
+enum FakeNetSecurityEnabled { True = 1; False = 2 }
+function Get-NetFirewallRule {
+    param([string] `$DisplayName, `$ErrorAction)
+    [pscustomobject]@{
+        DisplayName = `$DisplayName
+        Enabled     = [FakeNetSecurityEnabled]::$EnabledMember
+        Action      = 'Block'
+        Direction   = 'Outbound'
+    }
+}
+& '$($script:ScriptPath)' -Action check
+exit `$LASTEXITCODE
+"@
+                $out = pwsh -NoProfile -Command $child 2>&1 | Out-String
+                return [pscustomobject]@{ Output = $out; ExitCode = $LASTEXITCODE }
+            }
+        }
+
+        It 'reports a disabled rule as Enabled=False rather than as enabled' {
+            $r = Invoke-FirewallCheck -EnabledMember 'False'
+            $r.Output | Should -Match 'present, Enabled=False'
+            $r.ExitCode | Should -Be 0
+        }
+
+        It 'reports an enabled rule as Enabled=True' {
+            $r = Invoke-FirewallCheck -EnabledMember 'True'
+            $r.Output | Should -Match 'present, Enabled=True'
+            $r.ExitCode | Should -Be 0
+        }
+    }
+}