fix(cli): restrict pre-existing connector key files and pin API/CLI helper parity

[Chris0Jeky]

Summary

Closes the three implementable gaps left by PR #2665 on the CLI connector key file.

1. Forward remediation. CliFirstRunBootstrapper.EnsureKeyOnDisk returned as soon as it found a persisted key, so a key file created before fix(cli): create the connector key file atomically with owner-only permissions #2665 kept whatever permissions it was born with. The CLI now re-restricts that file to the current user before returning the key. RestrictedFileWriter gains RestrictFileToCurrentUser, a verbatim copy of the API helper of the same name, and a new best-effort wrapper CliFirstRunBootstrapper.RestrictExistingKeyFileAt calls it. The wrapper does nothing when the file is absent, never reads, rewrites or deletes content, and turns any IOException or UnauthorizedAccessException into one [CliFirstRun] WARNING: line on stderr that names the path, after which the run continues with the key it already has.

2. Parity guard. CliRestrictedFileWriterParityTests reads backend/src/Taskdeck.Api/FirstRun/FirstRunBootstrapper.cs and backend/src/Taskdeck.Cli/RestrictedFileWriter.cs, extracts the bodies of the six shared methods by signature, strips //, /// and /* */ comments, drops blank lines and collapses whitespace, and asserts each pair is identical. A mismatch names the method and prints the first differing normalized line from each side. A source file that cannot be located raises FileNotFoundException, so the test fails rather than passing quietly. Paths are built with Path.Combine only.

3. Doc. One paragraph in docs/platform/CONFIGURATION_REFERENCE.md, at the end of the "Generated local configuration file" section, on the filesystem requirement for the CLI key file.

Decisions taken inside the task's scope:

• The API's RestrictFileToCurrentUser has no "already restricted" fast path, so there was none to mirror. The call runs on every run; it is one SetAccessControl or one chmod.
• The existing.PreserveFile branch is treated the same way. That branch is reached when the file exists but could not be read, so the file survives the run and deserves the same lockdown; changing the DACL or mode does not read or rewrite content, so it is safe on a file we could not read. The one path left uncovered is "bootstrap lock unavailable and the file exists but is unreadable", which returns before the PreserveFile branch is reached.
• The operator-key-via-env path is NOT remediated. EnsureConnectorEncryptionKey returns before it resolves any path when Connectors:EncryptionKey is already in configuration, and that no-file-IO early return is a documented contract with an existing test behind it (EnsureConnectorEncryptionKey_WhenAlreadyConfigured_IsNoOp). Remediating there would mean resolving the data directory and touching a file the run never uses.

Root cause

EnsureKeyOnDisk had a single early return for the already-provisioned case:

var existing = ReadExisting(localConfigPath);
if (!string.IsNullOrWhiteSpace(existing.Key))
{
    return existing.Key!;
}

PR #2665 changed only the creation path (PersistKey now stages the key through RestrictedFileWriter.WriteRestrictedFile). Nothing on the read path touched permissions, so an install that already had appsettings.local.json from an older build kept the directory's inherited Windows DACL, typically including BUILTIN\Users read, or the umask-derived Unix mode, typically 0644, on every subsequent run.

Verification

All commands run from the worktree root with -m:1, one test process at a time.

Red first, against the pre-fix source. The remediation call was removed from the existing-key path and one character was changed in the CLI copy of RestrictFileToCurrentUser ("current user" to "current usEr"), then:

dotnet test backend/tests/Taskdeck.Cli.Tests/Taskdeck.Cli.Tests.csproj -c Release -m:1 --filter "FullyQualifiedName~CliKeyFileRemediationTests|FullyQualifiedName~CliRestrictedFileWriterParityTests"
Failed!  - Failed: 3, Passed: 8, Skipped: 0, Total: 11

The three failures:

• EnsureKeyOnDisk_ExistingUnrestrictedKeyFile_IsRestrictedToCurrentUser: "inheritance should be disabled so the directory's default ACEs (e.g. BUILTIN\Users read) do not apply" at CliRestrictedFileWriterTests.AssertOwnerOnly.
• EnsureKeyOnDisk_ExistingKeyPath_CallsTheRemediation_StructuralCheck: "Assert.Contains() Failure: Sub-string not found. Not found: RestrictExistingKeyFileAt(localConfigPath...".
• SharedLockdownHelper_HasIdenticalBodiesOnBothSides(method: "RestrictFileToCurrentUser", ...): "RestrictFileToCurrentUser has drifted between the API original and the CLI copy. First difference at normalized line 18: API ... to the current user; ... CLI ... to the current usEr; ...". The other five parity cases passed in the same run, which is what shows the guard discriminates rather than failing on everything.

Both temporary changes were reverted, then:

dotnet test backend/tests/Taskdeck.Cli.Tests/Taskdeck.Cli.Tests.csproj -c Release -m:1
Passed!  - Failed: 0, Passed: 239, Skipped: 0, Total: 239

dotnet test backend/tests/Taskdeck.Architecture.Tests/Taskdeck.Architecture.Tests.csproj -c Release -m:1
Passed!  - Failed: 0, Passed: 28, Skipped: 1, Total: 29

dotnet build backend/Taskdeck.sln -c Release
Build succeeded. 0 Error(s), 14 Warning(s)

The 14 warnings are all pre-existing and all in files this PR does not touch (Taskdeck.Api.Tests, Taskdeck.Application.Tests, and CliStartupTraceTests.cs). No new warning.

dotnet test backend/Taskdeck.sln -c Release -m:1
Domain 1603 passed, Application 4177 passed, Api 2843 passed / 4 skipped, Cli 239 passed, Architecture 28 passed / 1 skipped, Integration 7 passed / 29 skipped. 0 failed in every project.

node scripts/check-docs-governance.mjs
Docs governance check passed.

git diff --check
clean, no output.

Not verified

• Every command above ran on Windows 11 only. The Unix branches of RestrictFileToCurrentUser and of AssertOwnerOnly (0600 via File.SetUnixFileMode) were not executed locally; the Ubuntu CI leg covers them. The parity test itself is platform independent and passed locally.
• The remediation-failure path was proved at the helper level only, by injecting a throwing restrictFile delegate into RestrictExistingKeyFileAt and asserting the stderr warning and that nothing is thrown. There is no test that drives a real permission failure through EnsureKeyOnDisk end to end, because no deterministic way to force SetAccessControl or chmod to fail was available in-process. That EnsureKeyOnDisk still returns the key follows from the wrapper never throwing.
• Real FAT32, exFAT, and SMB behavior described in the doc paragraph is not exercised by any test here. It follows from the existing fail-closed guards in RestrictedFileWriter (AreAccessRulesProtected read-back and the SetUnixFileMode handle pin), which have their own structural coverage.
• Issue [Security][CLI] Connector key file follow-ups from PR #2665: remediate pre-existing files, guard API/CLI helper parity, document the non-ACL filesystem fail-closed #2667 item 4 is untouched. It is maintainer wording in docs/COURSE_CORRECTION.md and stays open.

Risk notes

• The remediation runs on every CLI invocation that finds a persisted key. It is one ACL write on Windows and one chmod on Unix, on a file the process is about to read anyway.
• It cannot lose a key. The helper never opens the file for writing, never deletes it, and never throws; the worst case is one stderr line and unchanged permissions.
• On Windows the new call replaces the file's DACL with a single owner-only ACE and disables inheritance, matching what fix(cli): create the connector key file atomically with owner-only permissions #2665 already does for newly created key files. An operator who had deliberately widened permissions on that file, for example to share the key with a service account on the same host, will see them narrowed on the next CLI run. Sharing a key that way was never supported; the documented option is an explicit Connectors__EncryptionKey on every host.
• The parity test compares source text, so a purely cosmetic re-wrap of a string literal across lines on one side only will fail it. The fix in that case is to apply the same wrapping to both copies, which is the intended behavior for a deliberate duplicate.
• The doc change is additive prose in one section. No canonical doc was touched.

Refs #2667 (items 1 to 3; item 4 stays open for maintainer wording). Refs #1262, PR #2665, #1241, #1264, PR #1267.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[Chris0Jeky]

Fix round

Head after the round: 79a8d42eeddd7a0352d54715aaa49526c03ae94f. No executable behaviour changed — the only source file touched is a test file; CliFirstRunBootstrapper.cs is byte-identical to the reviewed head.

1. MEDIUM — second RestrictExistingKeyFileAt call site (PreserveFile branch) untested — fixed

backend/tests/Taskdeck.Cli.Tests/CliKeyFileRemediationTests.cs gains two tests:

• EnsureKeyOnDisk_PreserveFilePath_CallsTheRemediation_StructuralCheck — same style as the existing-key pin: anchors on if (existing.PreserveFile), takes the slice up to the branch's return generated;, asserts RestrictExistingKeyFileAt(localConfigPath); is inside it.
• EnsureKeyOnDisk_UnreadableExistingKeyFile_KeepsTheFileAndWarns — the suggested behavioural test. It holds the key file open with FileShare.None, so File.ReadAllText inside ReadExisting throws and the run takes the PreserveFile branch; asserts the returned key is a fresh transient key (not the persisted one), the file still exists with its bytes unchanged, and stderr carries [CliFirstRun] WARNING: Could not read.

Two limits stated in the test comments rather than papered over:

• The behavioural test is Windows-only (early return elsewhere). FileShare.None is mandatory only on Windows; an open handle blocks nothing on POSIX and a 0000 mode is ignored when the suite runs as root, so there is no deterministic Unix equivalent. The Unix runner is covered by the structural pin.
• It cannot observe the remediation's effect: while the test holds the file with FileShare.None, the SetAccessControl open fails, so the DACL cannot be asserted. It pins the preserve-and-warn contract; the structural test pins the call site. Red-first proof of exactly that split below.

2. MEDIUM — unconditional lockdown claim in docs/platform/CONFIGURATION_REFERENCE.md — fixed

The claim is now scoped to runs where the CLI resolves the key file itself (no Connectors__EncryptionKey configured), and the paragraph states that a configured Connectors__EncryptionKey returns from the bootstrap before any path is resolved, so no lockdown is re-applied — an operator who sets the variable on an install predating the owner-only key file must delete or restrict the legacy appsettings.local.json themselves.

3. LOW — FAT32/exFAT/SMB sentence covered only the create path — fixed

Same paragraph now says the refusal applies to persisting a new key, and that an existing key file on such a volume keeps being used: the CLI returns the persisted key and warns on every run that it could not restrict the file to the current user. The remedy paragraph is stated as common to both cases (move the data directory, or set Connectors__EncryptionKey and remove the key file).

4. LOW — stale offsets + missing new site in docs/security/UNKNOWN_EXCEPTION_SURFACE_INVENTORY.md — fixed

Row re-read against this head:

• ex.Message print sites :183,249,298,370; type-filtered catches :114,169,244,294,354,366,439; bare cleanup catch :422-424.
• New site recorded: :294-301, catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) in RestrictExistingKeyFileAt, classified bounded — only those two exception types, one stderr warning, stdout stays clean JSON, the run continues — explicitly the same shape as the lock-unavailable warning at :174,183.
• Tests column now cites CliKeyFileRemediationTests.cs:91 (RestrictExistingKeyFileAt_WhenRestrictionFails_WarnsOnStderrAndDoesNotThrow) instead of none.
• The file's scope note (which records which rows carry which commit's numbers) gains one sentence saying this row was re-read at #2667.

node scripts/check-unknown-exception-boundary.mjs passes — it did not flag the new site, both before and after the doc edit. Nothing was suppressed or allowlisted.

Verification (all run from the PR worktree at this head)

Command	Result
dotnet test backend/tests/Taskdeck.Cli.Tests/Taskdeck.Cli.Tests.csproj -c Release -m:1 --filter "FullyQualifiedName~CliKeyFileRemediationTests"	Passed — 7/7, 0 failed (was 5 before this round)
dotnet test backend/tests/Taskdeck.Cli.Tests/Taskdeck.Cli.Tests.csproj -c Release -m:1	Passed — 241/241, 0 failed, 3 m 20 s
node scripts/check-docs-governance.mjs	Docs governance check passed. (exit 0)
node scripts/check-unknown-exception-boundary.mjs	Unknown-exception boundary check passed. (exit 0)
git diff --check	exit 0 (only a CRLF-normalization notice)

Red-first proof for finding 1: deleting the RestrictExistingKeyFileAt(localConfigPath); line from the PreserveFile branch turns the run red — EnsureKeyOnDisk_PreserveFilePath_CallsTheRemediation_StructuralCheck fails with Assert.Contains() Failure: Sub-string not found, 1 failed / 6 passed. The behavioural test stays green under that mutation, which is the limit named above. The line was restored (git checkout --) and the green run above is from the restored source. Note the structural pin, like the existing one it mirrors, matches source text and would still pass against a commented-out call.

No backend solution rerun: no non-test source line changed in this round, so the previously-run solution evidence still applies to the shipped code. NOT verified: the Unix branch of the new behavioural test (skipped by design on non-Windows) and the CI runners' own results for this head.

[Chris0Jeky]

Review disposition (beta-platform-integrity lane, T3 gate)

Round 1 (fresh-context, read-only reviewer at 7d73011): SHIP, no CRITICAL or HIGH; two MEDIUM, four LOW. The reviewer diffed the CLI copy of RestrictFileToCurrentUser against the API original by hand (identical), confirmed the remediation runs before the persisted key is returned, never reads, truncates or deletes the file, cannot widen access (one owner-only ACE with inheritance disabled on Windows, exactly 0600 on Unix), and cannot fail the run (every escape is normalized to IOException and caught into one stderr line).
Round 2 (scoped re-review of the fix diff at 79a8d42): SHIP, all four requested fixes closed, two LOW residuals. Two rounds is the ceiling; both residuals were closed by a docs-only commit and nothing reopens.

Fixed in the fix round (79a8d42)

• MEDIUM, the PreserveFile call site was untested: a structural pin (red when the call is deleted, proven and reverted) plus a Windows-only behavioural test that holds the key file open with FileShare.None, drives the unreadable-file branch, and asserts preserve-and-warn with byte-identical content. The Linux leg covers that branch structurally only, as the test says.
• MEDIUM, the configuration reference claimed the lockdown is re-applied unconditionally: now scoped to runs where the CLI resolves the key file itself, with the Connectors__EncryptionKey early return stated and the legacy-file cleanup put on the operator.
• LOW, the non-ACL volume sentence covered only the create path: the existing-file case is stated with the shared remedy.
• LOW, the exception-surface inventory row had stale offsets and missed the new stderr ex.Message site: every offset re-derived at the head, the new site classified (two exception types, one stderr warning, stdout untouched, run continues), and the boundary guard confirmed to pass without any allowlist change (it does not scan this file; the row is the right record).

Fixed after round 2 (afbd16b, docs only)

• LOW, the doc promised a guaranteed warning on volumes that ignore permissions, but the remediation helper has no read-back (unlike creation): reworded to "warns where the filesystem reports the failure; a volume that silently ignores permission changes produces no warning, so treat any such directory as unsupported".
• LOW, the inventory's type-filtered catch list omitted three real catches (:140, :174, :264): the list is now exhaustive, verified against the head file.

Declined, with reasoning

• LOW, parity guard scope: it compares six named method bodies for parity, not the lockdown contract, so a weakening applied to both copies passes and the [SupportedOSPlatform] attributes sit outside the compared region. That is what a parity guard is; the contract is pinned by the behavioural DACL and mode tests on both sides.
• LOW, two uncovered shapes: bootstrap lock unavailable plus an unreadable existing file returns a transient key before remediation (leaves the file exactly as it was; remediated on the next run that reads it), and File.SetUnixFileMode follows a symlink planted at the key path (requires write access to the CLI's own data directory, where the key could simply be replaced). Both pre-existing; recorded here, no issue.

Verification

Round 1 head: red first on the unmodified source (DACL assertion, structural pin, and one-character parity mutation all red; the other five parity cases green in the same run), Cli.Tests 239/239, Architecture.Tests 28/28 (one pre-existing skip), solution build 0 errors, full serialized backend solution 0 failed, docs governance, diff check. Fix round: Cli.Tests 241/241, boundary guard and docs governance green; no non-test source line changed after 7d73011 (the bootstrapper is byte-identical to the round-1 head), so no solution rerun. Final commit is docs only. Merge once exact-head ci-required is green at afbd16b and the three-minute floor has passed.