diff --git a/plugins/kindle-dedrm/.claude-plugin/plugin.json b/plugins/kindle-dedrm/.claude-plugin/plugin.json index 078bb41c3a..dedaf6305a 100644 --- a/plugins/kindle-dedrm/.claude-plugin/plugin.json +++ b/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", diff --git a/plugins/kindle-dedrm/CHANGELOG.md b/plugins/kindle-dedrm/CHANGELOG.md index f5627312a2..d8df4cee9f 100644 --- a/plugins/kindle-dedrm/CHANGELOG.md +++ b/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 diff --git a/plugins/kindle-dedrm/skills/manage/scripts/firewall.ps1 b/plugins/kindle-dedrm/skills/manage/scripts/firewall.ps1 index 1b169963d3..0cd65efe18 100644 --- a/plugins/kindle-dedrm/skills/manage/scripts/firewall.ps1 +++ b/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 { diff --git a/plugins/kindle-dedrm/skills/manage/tests/firewall.Tests.ps1 b/plugins/kindle-dedrm/skills/manage/tests/firewall.Tests.ps1 new file mode 100644 index 0000000000..8adc7056a0 --- /dev/null +++ b/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 + } + } +}