mirror of https://github.com/raandree/NTFSSecurity
Browse Source
A probe on the published rc8 found that a piped optional enum parameter silently ignores a value that PowerShell cannot convert: a typo for Deny in AccessType creates an Allow entry, AuditFlags, the flags, and AppliesTo fall back to their defaults, and Get-FileHash2 runs SHA256. Named calls fail loudly. The maintainer decided on 2026-10-21 that rc9 fixes the family in the four access and audit cmdlets, changes the items 3 to 7 of the sheet, and gets an independent review. Decision 27 holds the probes, the options, the designs that five read-only research threads proposed, and the recommended defaults. The lab suite run of the new tests (24 runs on six machine classes, 0 failed) is recorded as the agent's flagged assumption. activeContext and progress follow. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: AI Assistant <ai@example.com>pull/132/head
Raimund Andree
1 week ago
3 changed files with 175 additions and 34 deletions
@ -0,0 +1,143 @@ |
|||
--- |
|||
status: accepted |
|||
date: 2026-10-21 |
|||
last-verified: 2026-10-21 |
|||
owner: shared |
|||
source: maintainer answers of 2026-10-21 (fix scope B, items 3 to 7 change, review on); probes of 2026-10-21 on the published rc8 |
|||
--- |
|||
|
|||
# Decision 27: The open items before 5.0.0, and the piped optional parameters |
|||
|
|||
- Context: the tests-only round left no plain test gap (#129). On 2026-10-21 |
|||
the maintainer chose to fix the silent default scope of a piped `AppliesTo` |
|||
before 5.0.0 (rc9) and to go through the other open items one by one. rc9 |
|||
is the last prerelease: a change after its acceptance needs rc10, and after |
|||
5.0.0 the repository is archived (Decision 18). So this is the last moment |
|||
to change any of the items below, and unchanged means accepted. |
|||
- Status: accepted at the level of scope (see "The maintainer's choices"). The |
|||
designs of the items 3 to 7 are still to be proposed and chosen. Every |
|||
recommendation in the tables is the agent's; the last column of the table of |
|||
the other items stays empty, because the choices are listed below. |
|||
|
|||
## New finding: the flaw is a family |
|||
|
|||
PowerShell skips a property of a piped object whose value it cannot convert to |
|||
the type of an optional parameter, and the parameter keeps its default. A named |
|||
call fails loudly; the same value on a piped object does not. Probe |
|||
`p-unconvertible-optional-params.ps1`, the published rc8, both editions (the |
|||
only difference is how `ReadData` prints): |
|||
|
|||
| Piped property | What rc8 does | |
|||
| --- | --- | |
|||
| `AccessType = 'Deny'` | A Deny entry, as intended | |
|||
| `AccessType = 'Denyy'` (typo), or a blank CSV cell | **An Allow entry**, no message | |
|||
| `AuditFlags = 'Failurre'` | Success and Failure are audited (the default), no message | |
|||
| `InheritanceFlags` or `PropagationFlags = 'Typo'` | Ignored; the default flags (`ContainerInherit`, `ObjectInherit`) | |
|||
| `AppliesTo = 'Typo'`, a blank cell, or `99` | Ignored; the broader default scope | |
|||
| `Remove-NTFSAccess` with `AccessType = 'Denyy'` | Looks for an Allow entry: the Deny entry stays, no message | |
|||
| `Get-FileHash2` with `Algorithm = 'SHA999'` | SHA256, no message | |
|||
| `-AccessType Denyy` as a named parameter | A binding error, as it should be | |
|||
|
|||
The decision of 2026-10-21 covers `AppliesTo` only. A typo for `Deny` that |
|||
creates an Allow entry is at least as serious, and it has the same cause. |
|||
|
|||
## Options for the fix |
|||
|
|||
Prototypes (`p-proto-appliesto-shapes.ps1`, `p-proto-object3.ps1`), both |
|||
editions, same attribute shape as `Add-NTFSAccess`: |
|||
|
|||
| Shape | Result | |
|||
| --- | --- | |
|||
| One parameter set with optional parameters | Does not help: the binder skips the unconvertible value there too | |
|||
| `AppliesTo` as `object` alone | Loud for a bad scope, but a CSV row with `AppliesTo` and `InheritanceFlags` is no longer `AmbiguousParameterSet`: the scope silently wins | |
|||
| `AppliesTo`, `InheritanceFlags`, `PropagationFlags`, and `AccessType` or `AuditFlags` as `object`, converted in the cmdlet with `LanguagePrimitives.ConvertTo` | Loud for every bad value (a non-terminating error for that object, the next object goes on, nothing carries over); `AppliesTo` with flags is `AmbiguousParameterSet` for every mix of strings and enums; valid values unchanged | |
|||
|
|||
| Option | What it is | Cost | |
|||
| --- | --- | --- | |
|||
| A | `AppliesTo` only (the decision as asked) | Leaves the Deny-to-Allow flip | |
|||
| B | The family in the four cmdlets (recommended): `AppliesTo`, `InheritanceFlags`, `PropagationFlags`, `AccessType` (access), `AuditFlags` (audit) | See below | |
|||
| C | B and `Get-FileHash2 -Algorithm` | B plus one cmdlet of low severity | |
|||
| D | No code change: pages and CHANGELOG say that piped values are not checked | Weak for a permissions tool | |
|||
|
|||
Consequences of B: the help shows `<Object>` for these parameters (the pages |
|||
list the values; an argument completer keeps tab completion); a named call |
|||
with a bad value is an error record of the cmdlet instead of a terminating |
|||
binding error; a blank cell for any of them is an error, not the default; a |
|||
`$null` `AccessType` stays the default (Allow); the pipeline conflict check is |
|||
now uniform. The work: tests first (the pins of the silent behavior change), |
|||
four cmdlets, four pages, the help file, the CHANGELOG, rc9, and the |
|||
acceptance (about 70 minutes with `Chain-Published.ps1`). An independent |
|||
review is recommended: public behavior and the permission scope. |
|||
|
|||
## The other open items |
|||
|
|||
| # | Item | What a user can notice | If changed | Recommended | Choice | |
|||
| --- | --- | --- | --- | --- | --- | |
|||
| 2 | The help says that a local standard user who asks about a domain account still gets "Access is denied" | Nothing changes in behavior | Docs only; rides with rc9 | Do it in rc9 | | |
|||
| 3 | Library-only items of the DLL that no cmdlet reaches: the `FileSecurity` conversions always throw; the `RemoveAll` filters ignore the account list (#113); the lazy path overloads; a `PrivilegeEnabler` that is not disposed stays enabled; the unused registry model and helper classes; `PipelineControl.IsEnd`; one unvisited catch in `Enable-Privileges` | Only .NET callers of the DLL | Code change and rc9 | Accept; list as known limits of the .NET API | | |
|||
| 4 | `Get-ChildItem2 -Filter` dot patterns differ from `Get-ChildItem` (`Report.*` misses `Report`; `Rep*.` returns nothing) | Documented, pinned by tests | New matching code and rc9 | Accept as documented | | |
|||
| 5 | The `-SecurityDescriptor` sets of the four add and remove cmdlets end with a terminating error when the change throws; the `-Path` sets write an error per item and go on (7 test rows stay unpinned) | A failing change to a descriptor in memory stops the cmdlet | Per-item handlers and rc9 | Accept | | |
|||
| 6 | Three small inconsistencies: the error category of `Get-NTFSAccess` after a failed ownership retry (`WriteError` there, `OpenError` elsewhere); `Get-NTFSOrphanedAccess` warns where its siblings write an error; `Set-NTFSSecurityDescriptor` takes `-Path` at position 2, its siblings at 1 | Scripts that check a category or pass a path by position | A breaking change for such scripts, and rc9 | Accept | | |
|||
| 7 | Seventeen owner-restore handlers do not pass on what a later command raises, in the rare case that it throws only for the `RestoreOwnerError` record | Rare | One check per handler, an elevated-only test, rc9 | Accept | | |
|||
| 8 | The ownership retry and the read denial of an audit write over SMB have no trigger on a Windows file server | None known | A seam (a design change), or the answer of a non-Windows server (#34) | Accept; #34 decides on 2026-11-02 | | |
|||
| 9 | Eight getters of piped parameters that no test binds by property name | None (tests only) | Tests only, no rc9 | Do it after the code of rc9 is final; the fix tests cover `AccessType`, `AuditFlags`, and `Algorithm` | | |
|||
| 10 | Lab and process only: the ACL of the lab stage folders under `C:\`, rollback checkpoints that report Standard, a publication recovery that never ran against the Gallery, test helpers that accept a link, the cleanup list (AppVeyor, Read the Docs, wiki, `test/transfer`, old checkpoints) | Not shipped | n/a | Accept; the cleanup before the archive is the maintainer's | | |
|||
| 11 | The deferred reviews of rc3 to rc7 (`progress.md`, open work 3) | Not new | n/a | Accept | | |
|||
|
|||
Decided before: the hard-link names under another root stay a documented known |
|||
limit (Decision 21); Decisions 22 and 24 are accepted; #34 is decided on |
|||
2026-11-02 (Decision 23). |
|||
|
|||
## The maintainer's choices, 2026-10-21 |
|||
|
|||
| Question | Answer | |
|||
| --- | --- | |
|||
| Scope of the fix of the piped optional parameters | B: the family in the four cmdlets (`AppliesTo`, `InheritanceFlags`, `PropagationFlags`, `AccessType`, `AuditFlags`) | |
|||
| Items to change in rc9 | 3 (library-only items), 4 (`Get-ChildItem2 -Filter` dot patterns), 5 (descriptor sets report per descriptor), 6 (three inconsistencies), 7 (17 owner-restore handlers) | |
|||
| Items accepted as recommended | 8 (audit retry and read denial over SMB; #34 decides on 2026-11-02), 10, 11. Items 2 and 9 need no decision: the help sentence goes into rc9, and the tests for the eight getters follow when the code of rc9 is final | |
|||
| Independent review of the rc9 change | Yes (`review: on`) | |
|||
|
|||
What follows: the agent proposes the design of each of the items 3 to 7 |
|||
(options, risk, tests, a recommended default); the maintainer chooses; the work |
|||
goes test-first into one branch, one commit per item, so that any item can be |
|||
dropped. A defect that the review finds is fixed before rc9 is tagged; a defect |
|||
that the acceptance finds needs rc10. One acceptance (about 70 minutes with |
|||
`Chain-Published.ps1`) covers rc9 as a whole. |
|||
|
|||
## Designs proposed, 2026-10-21 |
|||
|
|||
Five read-only research threads read the code, the tests, and the docs. The |
|||
sheet below is their result, verified in part by the probes above. Fix B is |
|||
decided; the rest is to be chosen. Recommended defaults are the agent's. |
|||
|
|||
| # | Design | Recommended | Alternatives and risks | |
|||
| --- | --- | --- | --- | |
|||
| B | `AppliesTo`, `InheritanceFlags`, `PropagationFlags`, `AccessType` (access cmdlets), `AuditFlags` (audit cmdlets) become `object` parameters; ProcessRecord converts them with `LanguagePrimitives.ConvertTo` into locals. One shared error: id `InvalidPipedParameterValue`, category `InvalidData`, target the piped object, message "The value '{0}' for parameter '-{1}' could not be converted to '{2}'. The piped object was skipped." (blank: "The value for parameter '-{0}' is blank. The piped object was skipped."). A null `AccessType` or `AuditFlags` stays the default. Argument completers (`IArgumentCompleter`, available in the referenced Windows PowerShell 5.0 assembly) keep tab completion. Docs: `Type: Object`, help from `New-ExternalHelp`, the CHANGELOG paragraph at lines 247 to 258 | As described | `Get-FileHash2 -Algorithm` has the same flaw (option C, not chosen). Named calls change from a terminating binding error to the error record of the cmdlet | |
|||
| 5 | In the four `-SecurityDescriptor` branches, a `try/catch (Exception)` around the change of each descriptor writes `AddAceError` or `RemoveAceError`, category `WriteError`, target the descriptor, message of the exception, and goes on; the later-command check of item 7 goes into the same catch. Lines: `AddAccess.cs` 176 to 184, `RemoveAccess.cs` 177 to 185, `AddAudit.cs` 175 to 185, `RemoveAudit.cs` 176 to 186 | As described | Tests: a failing descriptor followed by a valid one; the 7 unpinned rows | |
|||
| 7 | One `IsFromLaterCommand(ex)` check, rethrowing, in each of the 17 outer handlers (the pattern of `SetSecurityDescriptor.cs` 72 to 101): `AddAccess`, `RemoveAccess`, `ClearAccess`, `GetAccess`, `GetOrphanedAccess`, `GetEffectiveAccess`, `AddAudit`, `RemoveAudit`, `ClearAudit`, `GetFileHash2`, the six inheritance cmdlets, and `SetSecurityDescriptor` | As described | Tests: one parameterized elevated-only test (a later command that throws on the `RestoreOwnerError` record), like the existing `restoreOwnerCases` | |
|||
| 3.1 | `FileSystemSecurity2` conversions from `FileSecurity` and `DirectorySecurity` keep the given descriptor | Fix | Remove the operators: breaking public API. Medium risk | |
|||
| 3.2 | `RemoveFileSystemAccessRuleAll` and `RemoveFileSystemAuditRuleAll` honor the account list | Fix | High risk for external callers (they remove entries); the cmdlets pass no list. Tests for both rule types | |
|||
| 3.3 | The path and several accounts overloads of the two add methods write at once, like their siblings | Fix | Medium risk (the moment of an exception changes) | |
|||
| 3.4 | `PrivilegeEnabler` no longer keeps an undisposed instance reachable | Defer unless the maintainer insists | High risk: token state in every cmdlet; a finalizer test is not deterministic; every cmdlet disposes its enabler today | |
|||
| 3.6 | The registry model (about 500 lines), `FileSystemFlags`, `FileSystemEffectivePermissionEntry`, and two raw descriptor readers | Keep and document (Decisions 21 and 22) | Removal breaks unknown .NET callers of the public DLL | |
|||
| 3.9 | `PipelineControl.IsEnd` | Keep as defense in depth | A change touches the pipeline semantics of all cmdlets for no known defect | |
|||
| 3.10 | The malformed `EnablePrivileges` setting: one validation in the base class, the dead catch removed | Fix | Medium risk (base class) | |
|||
| 4 | `Get-ChildItem2 -Filter`. A: pass the pattern unchanged to AlphaFS (the Windows PowerShell 5.1 rules; brings back the 8.3 short-name matches that a past fix removed). B: one cmdlet-side matcher with the Win32 dot rules on the real names and a broad enumeration (about 40 lines; costs a larger enumeration over SMB). D: keep as documented | B | Even `Get-ChildItem` differs between the editions here, so "aligned" needs a stated policy: the Windows PowerShell 5.1 dot rules on real names, in both editions | |
|||
| 6a | `Get-NTFSAccess`, the category of `ReadSecurityError` after a failed ownership retry: `WriteError` in `Get-NTFSAudit`, `Get-NTFSEffectiveAccess`, `Get-NTFSInheritance`, and `Get-NTFSSecurityDescriptor` too, so it is the majority convention | Keep, and pin it with a test | `OpenError` would break scripts that check the category, and make the module inconsistent unless six cmdlets change | |
|||
| 6b | `Get-NTFSOrphanedAccess` warns for a read failure other than access denied | An error: `ReadSecurityError`, `OpenError`, target the path | Breaks `-WarningAction` users; `-ErrorAction Stop` newly stops | |
|||
| 6c | `Set-NTFSSecurityDescriptor` has `-SecurityDescriptor` at position 2 and no other positional parameter | Position 1 | Cosmetic: a lone positional parameter binds first whatever its number | |
|||
|
|||
## Assumption flagged for the maintainer's review |
|||
|
|||
- The lab suite: he answered "not sure, you pick". The agent took the |
|||
recommendation: run the full suite (1,615 cases per configuration) on the |
|||
lab machine classes now, against the published rc8, so that an OS-specific |
|||
finding can still go into rc9 instead of forcing rc10. It changes nothing in |
|||
the repository or the product. Evidence: `suite-newtests` in the session |
|||
folder. Result (2026-10-21, 09:10 to 09:33 UTC): 24 runs on six machine |
|||
classes (Windows Server 2019, 2022, and 2025, Windows 11 22H2 and 26H1, and |
|||
the host), 0 failures. Every run gained exactly 121 cases against the rc8 |
|||
acceptance, and the four new files give the counts of the CI runner (elevated |
|||
Desktop: Access 74/0, Audit 55/2, Inheritance 72/1, Items 90/0, executed and |
|||
skipped). No OS-specific finding. The machines are saved again, and |
|||
`OSWin11E` is off. |
|||
Loading…
Reference in new issue