From b92d4ec725ab590707be07af37f9134660760a21 Mon Sep 17 00:00:00 2001 From: Chris0Jeky Date: Sat, 5 Sep 2026 03:52:05 +0100 Subject: [PATCH 1/6] test(cli): pin the failure-sink follow-ups from the #2573 review Adds the three regressions for #2577 before the fix: a failed write at the retention cap must delete no older record, TryRecord must reject a reference that is not lowercase hex of 12 or 32 characters, and the argv line must keep only command and flag names. All three fail against the current sink (7 failed, 17 passed). --- .../Taskdeck.Cli.Tests/CliFailureSinkTests.cs | 114 ++++++++++++++++++ 1 file changed, 114 insertions(+) diff --git a/backend/tests/Taskdeck.Cli.Tests/CliFailureSinkTests.cs b/backend/tests/Taskdeck.Cli.Tests/CliFailureSinkTests.cs index 680b8007e..d0a74167b 100644 --- a/backend/tests/Taskdeck.Cli.Tests/CliFailureSinkTests.cs +++ b/backend/tests/Taskdeck.Cli.Tests/CliFailureSinkTests.cs @@ -281,6 +281,120 @@ public void ForConnectionString_WithAnUnparsableKeywordValue_FallsBackInsteadOfT Path.Combine(Directory.GetCurrentDirectory(), CliFailureSink.DirectoryName))); } + /// + /// #2577 item 1: eviction must run only after the new record is on disk. A stale file at the + /// exact target name makes the CreateNew write fail, and a failed write must delete nothing, + /// otherwise the failure mode is net diagnostic loss instead of fail-open-with-no-change. + /// + [Fact] + public void TryRecord_WhenTheWriteFailsAtTheCap_DeletesNoOlderRecord() + { + using var directory = new TemporaryDirectory(); + var diagnostics = Path.Combine(directory.Path, CliFailureSink.DirectoryName); + Directory.CreateDirectory(diagnostics); + + // Exactly the cap: one stale file planted at the target name, plus older records that a + // pre-write eviction would delete to make room for a write that then cannot happen. + var targetName = CliFailureSink.BuildFileName(Reference, FixedTimestamp); + const string planted = "planted-content"; + File.WriteAllText(Path.Combine(diagnostics, targetName), planted); + + var seeded = new List { targetName }; + for (var index = 1; index < CliFailureSink.MaximumRecordCount; index++) + { + var name = CliFailureSink.BuildFileName( + "aaaaaaaaaaaa", + FixedTimestamp.AddSeconds(index - CliFailureSink.MaximumRecordCount)); + File.WriteAllText(Path.Combine(diagnostics, name), "seed"); + seeded.Add(name); + } + + var sink = CliFailureSink.ForDataDirectory(directory.Path); + var captured = sink.TryRecord(CreateLeakyException(), Reference, arguments: null, FixedTimestamp); + + captured.Should().BeFalse(); + var remaining = Directory + .GetFiles(diagnostics, CliFailureSink.FileNameSearchPattern) + .Select(Path.GetFileName) + .ToArray(); + remaining.Should().BeEquivalentTo(seeded); + File.ReadAllText(Path.Combine(diagnostics, targetName)).Should().Be(planted); + } + + /// + /// #2577 item 2: the reference is interpolated into the record file name, so TryRecord checks + /// its shape itself instead of trusting the callers to keep the invariant. Only lowercase hex + /// of exactly 12 characters (the generated reference) or 32 (the harness trace correlation) is + /// accepted; anything else fails open, writing nothing anywhere under the data directory. + /// + [Theory] + [InlineData("../x")] + [InlineData("a/../../x")] + [InlineData("0A1B2C3D4E5F")] + [InlineData("0a1b2c3d4e5")] + [InlineData("0a1b2c3d4e5f0")] + [InlineData("zzzzzzzzzzzz")] + [InlineData("")] + [InlineData(" ")] + public void TryRecord_WithAReferenceThatIsNotLowercaseHex_FailsOpenAndWritesNothing(string reference) + { + using var directory = new TemporaryDirectory(); + var sink = CliFailureSink.ForDataDirectory(directory.Path); + + var captured = sink.TryRecord(CreateLeakyException(), reference, arguments: null, FixedTimestamp); + + captured.Should().BeFalse(); + Directory.GetFiles(directory.Path, "*", SearchOption.AllDirectories).Should().BeEmpty(); + } + + /// + /// The 32-character lowercase-hex harness trace correlation stays accepted: it is the + /// reference a harness run's record is filed under. + /// + [Fact] + public void TryRecord_AcceptsTheThirtyTwoCharacterHarnessCorrelation() + { + using var directory = new TemporaryDirectory(); + var sink = CliFailureSink.ForDataDirectory(directory.Path); + var correlation = Guid.NewGuid().ToString("N"); + + sink.TryRecord(CreateLeakyException(), correlation, arguments: null, FixedTimestamp) + .Should().BeTrue(); + + File.Exists(Path.Combine( + directory.Path, + CliFailureSink.DirectoryName, + CliFailureSink.BuildFileName(correlation, FixedTimestamp))).Should().BeTrue(); + } + + /// + /// #2577 item 3: the record keeps the command grammar (the group, the command and the flag + /// names) and replaces every other argv token with a fixed placeholder, so neither a + /// space-separated secret nor ordinary user content such as a card title reaches disk. + /// + [Fact] + public void TryRecord_KeepsCommandAndFlagNamesButNoArgumentValues() + { + using var directory = new TemporaryDirectory(); + var sink = CliFailureSink.ForDataDirectory(directory.Path); + + var captured = sink.TryRecord( + CreateLeakyException(), + Reference, + new[] { "cards", "add", "--title", "Secret plan", "--token", "abc123" }, + FixedTimestamp); + + captured.Should().BeTrue(); + var content = File.ReadAllText(Path.Combine( + directory.Path, + CliFailureSink.DirectoryName, + CliFailureSink.BuildFileName(Reference, FixedTimestamp))); + + content.Should().Contain("argv: cards add --title [value] --token [value]"); + content.Should().NotContain("Secret plan"); + content.Should().NotContain("abc123"); + } + private sealed class TemporaryDirectory : IDisposable { public TemporaryDirectory() From 4f00f1c2afa5bd7295099b1acdce2f482cbafbab Mon Sep 17 00:00:00 2001 From: Chris0Jeky Date: Sat, 5 Sep 2026 03:54:35 +0100 Subject: [PATCH 2/6] fix(cli): harden the failure sink (evict after write, hex reference, argv policy) Eviction now runs after the new record's stream is closed, skipping the record just written, so a create that fails deletes nothing. TryRecord accepts only a lowercase-hex reference of 12 or 32 characters, the two shapes the callers produce, and fails open otherwise. The argv line keeps the leading command words and the flag names and replaces every other token with [value], so a space-separated secret or a card title never reaches disk; the redactor still runs over the result for the key=value forms. --- backend/src/Taskdeck.Cli/CliFailureSink.cs | 129 ++++++++++++++++++--- 1 file changed, 115 insertions(+), 14 deletions(-) diff --git a/backend/src/Taskdeck.Cli/CliFailureSink.cs b/backend/src/Taskdeck.Cli/CliFailureSink.cs index 7c83f65b9..04ebfe1dd 100644 --- a/backend/src/Taskdeck.Cli/CliFailureSink.cs +++ b/backend/src/Taskdeck.Cli/CliFailureSink.cs @@ -17,7 +17,10 @@ namespace Taskdeck.Cli; /// /// The record deliberately never holds a raw stack trace or a raw Exception.Message: it /// carries output (redacted, bounded depth -/// and length) and the process arguments passed through . +/// and length) and the shape of the command line, never its values: only the leading command words +/// and the flag names survive, every other argument becomes +/// , and the result still goes through +/// for the attached key=value forms (#2577). /// /// Every failure mode is fail-open: an unwritable directory, a full disk, a pre-existing file at /// the target path or a permission error returns false so the caller prints the existing @@ -45,6 +48,25 @@ internal sealed class CliFailureSink /// Length, in lowercase hex characters, of a generated correlation reference. internal const int ReferenceLength = 12; + /// + /// Length, in lowercase hex characters, of the harness startup-trace correlation + /// (), the other reference shape a caller may file a record under. + /// + internal const int TraceCorrelationLength = 32; + + /// + /// Stand-in written in place of every argv token that is not a command word or a flag name + /// (#2577). The sink retains the shape of the failing command, never the operator's values. + /// + internal const string ArgumentValuePlaceholder = "[value]"; + + /// + /// Depth of the CLI's command grammar: a group and a command, as in cards add + /// (see ). Nothing past the second token is a command + /// word, so nothing past it may be retained verbatim unless it names a flag. + /// + private const int MaximumCommandWords = 2; + private static readonly UTF8Encoding StrictUtf8 = new(encoderShouldEmitUTF8Identifier: false, throwOnInvalidBytes: true); @@ -172,7 +194,11 @@ internal bool TryRecord( { ArgumentNullException.ThrowIfNull(exception); - if (_diagnosticsDirectory is null || string.IsNullOrWhiteSpace(reference)) + // The reference is interpolated into the record's file name, so its shape is checked here + // rather than trusted from the callers: anything but the two shapes the CLI produces fails + // open, which keeps a traversal-shaped or otherwise unexpected reference from steering the + // write out of the diagnostics directory even if a future caller stops validating it. + if (_diagnosticsDirectory is null || !IsAcceptedReference(reference)) { return false; } @@ -180,7 +206,6 @@ internal bool TryRecord( try { Directory.CreateDirectory(_diagnosticsDirectory); - EvictOldestRecords(_diagnosticsDirectory); var path = Path.Combine(_diagnosticsDirectory, BuildFileName(reference, timestamp)); @@ -204,9 +229,17 @@ internal bool TryRecord( options.UnixCreateMode = UnixFileMode.UserRead | UnixFileMode.UserWrite; } - using var stream = new FileStream(path, options); - stream.Write(payload); - stream.Flush(); + using (var stream = new FileStream(path, options)) + { + stream.Write(payload); + stream.Flush(); + } + + // Only now, with the new record closed and on disk, is it safe to trim the directory. + // Evicting first meant a create that then failed (a stale file at the target name, a + // full disk, a directory that permits delete but not create) destroyed older records + // and replaced none of them: net diagnostic loss instead of fail-open-with-no-change. + EvictOldestRecords(_diagnosticsDirectory, path); return true; } catch (Exception) @@ -271,22 +304,29 @@ private static byte[] Bound(string content) } /// - /// Deletes the oldest records, by name, until writing one more stays within + /// Deletes the oldest records, by name, until the directory is back within /// . The timestamp prefix makes ordinal name order the same as - /// chronological order. + /// chronological order. Runs after the write, so the record just written is already counted + /// and is skipped explicitly: a record must never evict itself. /// - private static void EvictOldestRecords(string diagnosticsDirectory) + private static void EvictOldestRecords(string diagnosticsDirectory, string writtenPath) { var existing = Directory.GetFiles(diagnosticsDirectory, FileNameSearchPattern); - if (existing.Length < MaximumRecordCount) + var surplus = existing.Length - MaximumRecordCount; + if (surplus <= 0) { return; } Array.Sort(existing, StringComparer.Ordinal); - var surplus = existing.Length - MaximumRecordCount + 1; - for (var index = 0; index < surplus; index++) + for (var index = 0; index < existing.Length && surplus > 0; index++) { + if (string.Equals(existing[index], writtenPath, StringComparison.Ordinal)) + { + continue; + } + + surplus--; try { File.Delete(existing[index]); @@ -298,6 +338,17 @@ private static void EvictOldestRecords(string diagnosticsDirectory) } } + /// + /// The two reference shapes the CLI produces: the 12-character generated reference and the + /// 32-character harness trace correlation, both lowercase hex. Lowercase hex cannot contain a + /// directory separator, a drive letter or a dot, so an accepted reference can only ever name a + /// file inside the diagnostics directory. + /// + private static bool IsAcceptedReference(string? reference) => + reference is not null && + reference.Length is ReferenceLength or TraceCorrelationLength && + reference.All(character => character is (>= '0' and <= '9') or (>= 'a' and <= 'f')); + private static string DescribeVersion() { try @@ -311,6 +362,19 @@ private static string DescribeVersion() } } + /// + /// Renders argv under the retention policy decided in #2577: keep the command grammar, drop + /// every value. A token is retained verbatim only when it starts with '-' (a flag name, whose + /// attached key=value form is still put through + /// below) or when it is one of the leading command + /// words. Everything else becomes . + /// + /// The redactor only masks the key=value and key: value forms, so a + /// space-separated secret (--token abc123) would otherwise have been retained verbatim, + /// and ordinary user content such as a card title or description would have been written to + /// disk on failure where nothing was retained before this sink existed. The command name and + /// the flag names are what makes a record actionable; the values are not worth their risk. + /// private static string DescribeArguments(IReadOnlyList? arguments) { if (arguments is null || arguments.Count == 0) @@ -318,11 +382,48 @@ private static string DescribeArguments(IReadOnlyList? arguments) return "(none)"; } - var joined = string.Join(' ', arguments); - var redacted = SensitiveDataRedactor.Redact(joined); + var builder = new StringBuilder(); + var commandWords = 0; + for (var index = 0; index < arguments.Count; index++) + { + if (index > 0) + { + builder.Append(' '); + } + + var argument = arguments[index]; + if (argument.StartsWith('-')) + { + builder.Append(argument); + + // Nothing after the first flag is a command word, so no later bare token may be + // retained on the strength of its shape alone. + commandWords = MaximumCommandWords; + } + else if (commandWords < MaximumCommandWords && IsCommandWord(argument)) + { + builder.Append(argument); + commandWords++; + } + else + { + builder.Append(ArgumentValuePlaceholder); + } + } + + var redacted = SensitiveDataRedactor.Redact(builder.ToString()); return string.IsNullOrWhiteSpace(redacted) ? "(none)" : redacted; } + /// + /// The shape every CLI command group and command has: short, lowercase, no whitespace. A + /// leading token that is not one (a positional value, a title, anything cased or spaced) is + /// replaced rather than retained. + /// + private static bool IsCommandWord(string argument) => + argument.Length is > 0 and <= 32 && + argument.All(character => character is (>= 'a' and <= 'z') or (>= '0' and <= '9') or '-'); + private static string? FirstNonEmpty(params string?[] candidates) { foreach (var candidate in candidates) From 7dcf503d0fec665891ebf1158049c6692d5f2864 Mon Sep 17 00:00:00 2001 From: Chris0Jeky Date: Sat, 5 Sep 2026 03:55:05 +0100 Subject: [PATCH 3/6] docs(security): state the CLI failure-sink argv retention policy Records the conservative option chosen for #2577 item 3 and the two hardening changes beside it: eviction only after a successful write, and the lowercase-hex reference check. --- docs/security/SECURITY_LOGGING_REDACTION.md | 30 ++++++++++++++++----- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/docs/security/SECURITY_LOGGING_REDACTION.md b/docs/security/SECURITY_LOGGING_REDACTION.md index 460164dc9..f85b7e67f 100644 --- a/docs/security/SECURITY_LOGGING_REDACTION.md +++ b/docs/security/SECURITY_LOGGING_REDACTION.md @@ -70,13 +70,29 @@ It applies to API middleware, SignalR transport request logging, queue/worker lo `/diagnostics/cli-failure--.txt`, where the data directory is the directory of the resolved SQLite data source (the same resolution the CLI first-run bootstrap uses, falling back to the working directory for a non-file data source). - The record holds the UTC timestamp, the reference, the CLI version, the process arguments - passed through `SensitiveDataRedactor.Redact`, and `SensitiveDataRedactor.SummarizeException` - output — never a raw stack trace and never a raw `Exception.Message`. Bounds: at most 8 KB per - record (truncated with an explicit marker) and at most 20 records, oldest evicted first by the - timestamp-sorted name. The file is created with `FileMode.CreateNew`, so a stale file or a - planted symlink at the target path makes the write fail rather than being appended to or - followed, and on POSIX it is created 0600 at creation time (no world-readable window). + The record holds the UTC timestamp, the reference, the CLI version, the command line under the + argv retention policy below, and `SensitiveDataRedactor.SummarizeException` output — never a + raw stack trace and never a raw `Exception.Message`. Bounds: at most 8 KB per record + (truncated with an explicit marker) and at most 20 records, oldest evicted first by the + timestamp-sorted name. Eviction runs only after the new record has been written and closed, and + never removes the record just written, so a create that fails deletes nothing. The file is + created with `FileMode.CreateNew`, so a stale file or a planted symlink at the target path + makes the write fail rather than being appended to or followed, and on POSIX it is created 0600 + at creation time (no world-readable window). `TryRecord` accepts only a reference that is + lowercase hex of exactly 12 characters (the generated reference) or 32 (the harness trace + correlation); any other reference fails open, writing nothing and printing nothing, so the + reference can never steer the record out of the diagnostics directory. + - **Argv retention policy** (#2577): the record keeps the shape of the failing command, not its + values. A token is written verbatim only when it starts with `-` (a flag name) or is one of + the at most two leading command words, which must be short lowercase words such as `cards add`; + every other token is replaced with the fixed placeholder `[value]`. The result is then still + passed through `SensitiveDataRedactor.Redact`, which masks the attached `key=value` and + `key: value` secret forms. This is the conservative option of the three the #2573 review + listed: it covers a space-separated secret flag such as `--token abc123`, which the redactor's + `key=value` rules do not match, and it keeps ordinary user content such as a card title or + description off disk, since nothing was retained on an operator run before this sink existed. + So `cards add --title "Secret plan" --token abc123` is recorded as + `cards add --title [value] --token [value]`. The reference the CLI prints alongside the generic line is the trace correlation when a trace is enabled and a freshly generated 12-hex-character reference otherwise. It is shown only when a sink actually kept the record; when every sink fails (unwritable directory, full disk, a file From 809b77194d3fcc17021f18161f53f610ed7bae31 Mon Sep 17 00:00:00 2001 From: Chris0Jeky Date: Sat, 5 Sep 2026 04:22:30 +0100 Subject: [PATCH 4/6] fix(cli): finish the failure-sink follow-ups from the #2619 review Three defects the review found in the #2577 hardening, each pinned red-first: - Eviction ran inside the write try block, so an enumeration failure made TryRecord return false for a record that was already closed on disk: the CLI then printed the "diagnostics were not captured" notice for a record that exists, and the 20-record cap was silently suspended. Eviction now has its own catch and can no longer change the reported outcome. An internal record-lister seam on ForDataDirectory lets the test make the enumeration throw. - A '-'-prefixed token was kept whole, so an attached value whose key is not in SensitiveDataRedactor's keyword list (--title=..., --description=...) was written verbatim. Only the flag name up to its first '=' is kept now; the attached value becomes the same [value] placeholder a separate one gets, and Redact still runs over the result. - IsAcceptedReference took lowercase hex only while CliStartupTrace.IsCorrelationId takes either case, so the sink could refuse a reference the CLI itself printed. Both now accept hex in either case, and the 32 length lives once, as CliStartupTrace.CorrelationLength. Also adds the missing test for the self-eviction skip branch: a record written with the oldest timestamp at the cap survives, and the next-oldest goes instead. The truncation test's argv fixture moved its bulk from attached values to flag names, since values no longer reach the record and the payload has to exceed the 8 KB bound for the test to mean anything. --- backend/src/Taskdeck.Cli/CliFailureSink.cs | 91 +++++++--- backend/src/Taskdeck.Cli/CliStartupTrace.cs | 12 +- .../Taskdeck.Cli.Tests/CliFailureSinkTests.cs | 157 +++++++++++++++++- 3 files changed, 227 insertions(+), 33 deletions(-) diff --git a/backend/src/Taskdeck.Cli/CliFailureSink.cs b/backend/src/Taskdeck.Cli/CliFailureSink.cs index 04ebfe1dd..d74c2c92f 100644 --- a/backend/src/Taskdeck.Cli/CliFailureSink.cs +++ b/backend/src/Taskdeck.Cli/CliFailureSink.cs @@ -18,9 +18,9 @@ namespace Taskdeck.Cli; /// The record deliberately never holds a raw stack trace or a raw Exception.Message: it /// carries output (redacted, bounded depth /// and length) and the shape of the command line, never its values: only the leading command words -/// and the flag names survive, every other argument becomes -/// , and the result still goes through -/// for the attached key=value forms (#2577). +/// and the flag names survive, every other argument — an attached --flag=value value +/// included — becomes , and the result still goes through +/// (#2577). /// /// Every failure mode is fail-open: an unwritable directory, a full disk, a pre-existing file at /// the target path or a permission error returns false so the caller prints the existing @@ -45,14 +45,15 @@ internal sealed class CliFailureSink /// Appended when a record hits . internal const string TruncationMarker = "\n[truncated: record exceeded the 8192-byte bound]\n"; - /// Length, in lowercase hex characters, of a generated correlation reference. + /// Length, in hex characters, of a generated correlation reference. internal const int ReferenceLength = 12; /// - /// Length, in lowercase hex characters, of the harness startup-trace correlation - /// (), the other reference shape a caller may file a record under. + /// Length, in hex characters, of the harness startup-trace correlation, the other reference + /// shape a caller may file a record under. Taken from rather + /// than repeated, so the sink cannot drift from the trace that produced the correlation. /// - internal const int TraceCorrelationLength = 32; + internal const int TraceCorrelationLength = CliStartupTrace.CorrelationLength; /// /// Stand-in written in place of every argv token that is not a command word or a flag name @@ -71,8 +72,13 @@ internal sealed class CliFailureSink new(encoderShouldEmitUTF8Identifier: false, throwOnInvalidBytes: true); private readonly string? _diagnosticsDirectory; + private readonly Func _listRecords; - private CliFailureSink(string? diagnosticsDirectory) => _diagnosticsDirectory = diagnosticsDirectory; + private CliFailureSink(string? diagnosticsDirectory, Func? listRecords = null) + { + _diagnosticsDirectory = diagnosticsDirectory; + _listRecords = listRecords ?? Directory.GetFiles; + } /// The resolved records directory, or null when it could not be resolved at all. internal string? DiagnosticsDirectory => _diagnosticsDirectory; @@ -81,21 +87,31 @@ internal sealed class CliFailureSink /// Builds a sink rooted at an explicit data directory. Used by tests and by callers that /// already know the directory. /// - internal static CliFailureSink ForDataDirectory(string? dataDirectory) + internal static CliFailureSink ForDataDirectory(string? dataDirectory) => + ForDataDirectory(dataDirectory, listRecords: null); + + /// + /// Same sink, with the retention enumeration supplied. Test seam only: it exists so a test can + /// make eviction fail after the record is already written and closed, which is the one path + /// where an exception must not turn a kept record into a reported capture failure. + /// + internal static CliFailureSink ForDataDirectory( + string? dataDirectory, + Func? listRecords) { if (string.IsNullOrWhiteSpace(dataDirectory)) { - return new CliFailureSink(diagnosticsDirectory: null); + return new CliFailureSink(diagnosticsDirectory: null, listRecords); } try { - return new CliFailureSink(Path.GetFullPath(Path.Combine(dataDirectory, DirectoryName))); + return new CliFailureSink(Path.GetFullPath(Path.Combine(dataDirectory, DirectoryName)), listRecords); } catch (Exception) { // An unresolvable path must never crash the failure boundary itself. - return new CliFailureSink(diagnosticsDirectory: null); + return new CliFailureSink(diagnosticsDirectory: null, listRecords); } } @@ -239,7 +255,20 @@ internal bool TryRecord( // Evicting first meant a create that then failed (a stale file at the target name, a // full disk, a directory that permits delete but not create) destroyed older records // and replaced none of them: net diagnostic loss instead of fail-open-with-no-change. - EvictOldestRecords(_diagnosticsDirectory, path); + // + // Past this point the write has succeeded, so eviction gets its own catch: retention + // is best effort and must never downgrade a durable record to a reported failure, or + // the caller prints the "diagnostics were not captured" notice for a record that + // exists. The directory then keeps more than the cap until a later run trims it. + try + { + EvictOldestRecords(_diagnosticsDirectory, path); + } + catch (Exception) + { + // Enumeration or sorting failed; the record itself is already on disk. + } + return true; } catch (Exception) @@ -309,9 +338,9 @@ private static byte[] Bound(string content) /// chronological order. Runs after the write, so the record just written is already counted /// and is skipped explicitly: a record must never evict itself. /// - private static void EvictOldestRecords(string diagnosticsDirectory, string writtenPath) + private void EvictOldestRecords(string diagnosticsDirectory, string writtenPath) { - var existing = Directory.GetFiles(diagnosticsDirectory, FileNameSearchPattern); + var existing = _listRecords(diagnosticsDirectory, FileNameSearchPattern); var surplus = existing.Length - MaximumRecordCount; if (surplus <= 0) { @@ -340,14 +369,16 @@ private static void EvictOldestRecords(string diagnosticsDirectory, string writt /// /// The two reference shapes the CLI produces: the 12-character generated reference and the - /// 32-character harness trace correlation, both lowercase hex. Lowercase hex cannot contain a - /// directory separator, a drive letter or a dot, so an accepted reference can only ever name a - /// file inside the diagnostics directory. + /// 32-character harness trace correlation, both hex. Case is accepted either way, because + /// does: the sink must never refuse a reference + /// the CLI itself printed to the operator. Hex cannot contain a directory separator, a drive + /// letter or a dot, so an accepted reference can only ever name a file inside the diagnostics + /// directory. /// private static bool IsAcceptedReference(string? reference) => reference is not null && reference.Length is ReferenceLength or TraceCorrelationLength && - reference.All(character => character is (>= '0' and <= '9') or (>= 'a' and <= 'f')); + reference.All(Uri.IsHexDigit); private static string DescribeVersion() { @@ -364,10 +395,11 @@ private static string DescribeVersion() /// /// Renders argv under the retention policy decided in #2577: keep the command grammar, drop - /// every value. A token is retained verbatim only when it starts with '-' (a flag name, whose - /// attached key=value form is still put through - /// below) or when it is one of the leading command - /// words. Everything else becomes . + /// every value. A token is retained verbatim only when it starts with '-' (a flag name) or + /// when it is one of the leading command words. Everything else becomes + /// , including the value attached to a flag: a + /// --flag=value token keeps only --flag=. The whole line still goes through + /// below. /// /// The redactor only masks the key=value and key: value forms, so a /// space-separated secret (--token abc123) would otherwise have been retained verbatim, @@ -394,7 +426,18 @@ private static string DescribeArguments(IReadOnlyList? arguments) var argument = arguments[index]; if (argument.StartsWith('-')) { - builder.Append(argument); + // A flag name is retained, but its attached value is a value like any other: the + // redactor only masks the key=value forms whose key it knows, so --title=... or + // --description=... would otherwise reach disk verbatim. + var separator = argument.IndexOf('='); + if (separator < 0) + { + builder.Append(argument); + } + else + { + builder.Append(argument, 0, separator + 1).Append(ArgumentValuePlaceholder); + } // Nothing after the first flag is a command word, so no later bare token may be // retained on the strength of its shape alone. diff --git a/backend/src/Taskdeck.Cli/CliStartupTrace.cs b/backend/src/Taskdeck.Cli/CliStartupTrace.cs index 69ad3b115..6d1189813 100644 --- a/backend/src/Taskdeck.Cli/CliStartupTrace.cs +++ b/backend/src/Taskdeck.Cli/CliStartupTrace.cs @@ -11,6 +11,14 @@ namespace Taskdeck.Cli; internal sealed class CliStartupTrace { internal const string CorrelationEnvironmentVariable = "TASKDECK_CLI_TEST_TRACE_CORRELATION"; + + /// + /// Length, in hex characters, of a trace correlation. Single source of truth: it is also the + /// second reference shape files a failure record under, and the + /// two acceptance checks must not drift apart. + /// + internal const int CorrelationLength = 32; + internal const int MaximumTraceBytes = 8 * 1024; internal const int MaximumTraceRecords = 32; @@ -257,8 +265,8 @@ private static bool TryParseRecord(string line, string expectedCorrelationId, ou return true; } - private static bool IsCorrelationId(string? correlationId) => - correlationId is { Length: 32 } && correlationId.All(Uri.IsHexDigit); + internal static bool IsCorrelationId(string? correlationId) => + correlationId is { Length: CorrelationLength } && correlationId.All(Uri.IsHexDigit); } internal sealed record CliStartupTraceSnapshot( diff --git a/backend/tests/Taskdeck.Cli.Tests/CliFailureSinkTests.cs b/backend/tests/Taskdeck.Cli.Tests/CliFailureSinkTests.cs index d0a74167b..d447aedc6 100644 --- a/backend/tests/Taskdeck.Cli.Tests/CliFailureSinkTests.cs +++ b/backend/tests/Taskdeck.Cli.Tests/CliFailureSinkTests.cs @@ -125,9 +125,10 @@ public void TryRecord_TruncatesAnOversizedRecordWithAMarker() var sink = CliFailureSink.ForDataDirectory(directory.Path); // argv is the only unbounded input: the exception summary is already capped by the - // redactor at five levels and 1024 characters. + // redactor at five levels and 1024 characters. Values are replaced, so the unbounded part + // is what the policy still retains — the flag names themselves. var hugeArguments = Enumerable.Range(0, 400) - .Select(index => $"--flag{index.ToString(CultureInfo.InvariantCulture)}=" + new string('x', 40)) + .Select(index => $"--flag{index.ToString(CultureInfo.InvariantCulture)}-" + new string('x', 40)) .ToArray(); var captured = sink.TryRecord(CreateLeakyException(), Reference, hugeArguments, FixedTimestamp); @@ -323,20 +324,20 @@ public void TryRecord_WhenTheWriteFailsAtTheCap_DeletesNoOlderRecord() /// /// #2577 item 2: the reference is interpolated into the record file name, so TryRecord checks - /// its shape itself instead of trusting the callers to keep the invariant. Only lowercase hex - /// of exactly 12 characters (the generated reference) or 32 (the harness trace correlation) is - /// accepted; anything else fails open, writing nothing anywhere under the data directory. + /// its shape itself instead of trusting the callers to keep the invariant. Only hex of exactly + /// 12 characters (the generated reference) or 32 (the harness trace correlation) is accepted; + /// anything else fails open, writing nothing anywhere under the data directory. /// [Theory] [InlineData("../x")] [InlineData("a/../../x")] - [InlineData("0A1B2C3D4E5F")] [InlineData("0a1b2c3d4e5")] [InlineData("0a1b2c3d4e5f0")] [InlineData("zzzzzzzzzzzz")] [InlineData("")] [InlineData(" ")] - public void TryRecord_WithAReferenceThatIsNotLowercaseHex_FailsOpenAndWritesNothing(string reference) + public void TryRecord_WithAReferenceThatIsNotAnAcceptedCorrelation_FailsOpenAndWritesNothing( + string reference) { using var directory = new TemporaryDirectory(); var sink = CliFailureSink.ForDataDirectory(directory.Path); @@ -395,6 +396,148 @@ public void TryRecord_KeepsCommandAndFlagNamesButNoArgumentValues() content.Should().NotContain("abc123"); } + /// + /// Eviction runs after the stream is closed, so the record is already durable by then: an + /// enumeration failure there must not be reported as a capture failure, or the CLI prints the + /// "diagnostics were not captured" notice for a record that exists on disk. + /// + [Fact] + public void TryRecord_WhenEvictionFails_StillReportsTheAlreadyWrittenRecordAsKept() + { + using var directory = new TemporaryDirectory(); + var sink = CliFailureSink.ForDataDirectory( + directory.Path, + listRecords: (_, _) => throw new IOException("record enumeration failed")); + + var captured = sink.TryRecord(CreateLeakyException(), Reference, arguments: null, FixedTimestamp); + + captured.Should().BeTrue(); + var path = Path.Combine( + directory.Path, + CliFailureSink.DirectoryName, + CliFailureSink.BuildFileName(Reference, FixedTimestamp)); + File.Exists(path).Should().BeTrue(); + File.ReadAllText(path).Should().Contain($"correlation: {Reference}"); + } + + /// + /// A record must never evict itself. When the record just written is the oldest name at the + /// cap, the surplus has to come out of the next-oldest instead. + /// + [Fact] + public void TryRecord_WhenTheNewRecordIsTheOldestAtTheCap_EvictsAnotherAndKeepsItself() + { + using var directory = new TemporaryDirectory(); + var diagnostics = Path.Combine(directory.Path, CliFailureSink.DirectoryName); + Directory.CreateDirectory(diagnostics); + + // Exactly the cap, all newer than the record about to be written, so ordinal name order + // puts the new record first and the skip branch is the only thing that can save it. + var seeded = new List(); + for (var index = 1; index <= CliFailureSink.MaximumRecordCount; index++) + { + var name = CliFailureSink.BuildFileName( + "aaaaaaaaaaaa", + FixedTimestamp.AddSeconds(index)); + File.WriteAllText(Path.Combine(diagnostics, name), "seed"); + seeded.Add(name); + } + + var sink = CliFailureSink.ForDataDirectory(directory.Path); + var captured = sink.TryRecord(CreateLeakyException(), Reference, arguments: null, FixedTimestamp); + + captured.Should().BeTrue(); + var remaining = Directory + .GetFiles(diagnostics, CliFailureSink.FileNameSearchPattern) + .Select(Path.GetFileName) + .ToArray(); + remaining.Should().HaveCount(CliFailureSink.MaximumRecordCount); + remaining.Should().Contain(CliFailureSink.BuildFileName(Reference, FixedTimestamp)); + // The oldest seeded record went instead of the new one. + remaining.Should().NotContain(seeded[0]); + remaining.Should().Contain(seeded[1]); + remaining.Should().Contain(seeded[^1]); + } + + /// + /// The sink must not refuse a reference the CLI itself printed: CliStartupTrace accepts + /// hex in either case, so the sink has to as well. + /// + [Fact] + public void TryRecord_AcceptsEveryCorrelationTheStartupTraceAccepts() + { + using var directory = new TemporaryDirectory(); + var sink = CliFailureSink.ForDataDirectory(directory.Path); + + var correlations = new[] + { + Guid.NewGuid().ToString("N"), + Guid.NewGuid().ToString("N").ToUpperInvariant(), + new string('a', CliStartupTrace.CorrelationLength), + new string('F', CliStartupTrace.CorrelationLength) + }; + + for (var index = 0; index < correlations.Length; index++) + { + var correlation = correlations[index]; + CliStartupTrace.IsCorrelationId(correlation).Should().BeTrue(); + + var timestamp = FixedTimestamp.AddSeconds(index); + sink.TryRecord(CreateLeakyException(), correlation, arguments: null, timestamp) + .Should().BeTrue(); + File.Exists(Path.Combine( + directory.Path, + CliFailureSink.DirectoryName, + CliFailureSink.BuildFileName(correlation, timestamp))).Should().BeTrue(); + } + } + + /// The generated 12-character reference shape is accepted in either case too. + [Theory] + [InlineData("0A1B2C3D4E5F")] + [InlineData("0a1B2c3D4e5F")] + public void TryRecord_AcceptsAGeneratedReferenceInEitherCase(string reference) + { + using var directory = new TemporaryDirectory(); + var sink = CliFailureSink.ForDataDirectory(directory.Path); + + sink.TryRecord(CreateLeakyException(), reference, arguments: null, FixedTimestamp) + .Should().BeTrue(); + + File.Exists(Path.Combine( + directory.Path, + CliFailureSink.DirectoryName, + CliFailureSink.BuildFileName(reference, FixedTimestamp))).Should().BeTrue(); + } + + /// + /// #2577 follow-up: an attached value must go the same way as a separate one, whether or not + /// its flag name is one the redactor knows. Only the flag name survives. + /// + [Fact] + public void TryRecord_ReplacesAnAttachedValueEvenWhenTheFlagIsNotASecretKeyword() + { + using var directory = new TemporaryDirectory(); + var sink = CliFailureSink.ForDataDirectory(directory.Path); + + var captured = sink.TryRecord( + CreateLeakyException(), + Reference, + new[] { "cards", "add", "--title=Secret plan", "--token=abc123" }, + FixedTimestamp); + + captured.Should().BeTrue(); + var content = File.ReadAllText(Path.Combine( + directory.Path, + CliFailureSink.DirectoryName, + CliFailureSink.BuildFileName(Reference, FixedTimestamp))); + + content.Should().Contain("argv: cards add --title=[value] "); + content.Should().Contain($"--token={SensitiveDataRedactor.RedactedValue}"); + content.Should().NotContain("Secret plan"); + content.Should().NotContain("abc123"); + } + private sealed class TemporaryDirectory : IDisposable { public TemporaryDirectory() From 1e1c600907ba8f21563375ebde3ceaa017c0d6cd Mon Sep 17 00:00:00 2001 From: Chris0Jeky Date: Sat, 5 Sep 2026 04:22:38 +0100 Subject: [PATCH 5/6] docs(security): state the sink's argv and reference rules exactly The argv paragraph overstated the guarantee: it said a flag name is kept and left the impression that an attached value was covered by Redact, which only masks the keys it knows. Say what is kept (the leading command words and the flag names, up to the first '=') and what is replaced (every value, attached or separate), and record that the reference check now takes hex in either case and that a failed eviction never changes the reported outcome. --- docs/security/SECURITY_LOGGING_REDACTION.md | 26 +++++++++++++-------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/docs/security/SECURITY_LOGGING_REDACTION.md b/docs/security/SECURITY_LOGGING_REDACTION.md index f85b7e67f..cdf78d3a7 100644 --- a/docs/security/SECURITY_LOGGING_REDACTION.md +++ b/docs/security/SECURITY_LOGGING_REDACTION.md @@ -75,19 +75,25 @@ It applies to API middleware, SignalR transport request logging, queue/worker lo raw stack trace and never a raw `Exception.Message`. Bounds: at most 8 KB per record (truncated with an explicit marker) and at most 20 records, oldest evicted first by the timestamp-sorted name. Eviction runs only after the new record has been written and closed, and - never removes the record just written, so a create that fails deletes nothing. The file is + never removes the record just written, so a create that fails deletes nothing. An eviction + that fails leaves the directory over its cap until a later run trims it, and never changes the + outcome the caller reports: the record is already closed on disk by then. The file is created with `FileMode.CreateNew`, so a stale file or a planted symlink at the target path makes the write fail rather than being appended to or followed, and on POSIX it is created 0600 - at creation time (no world-readable window). `TryRecord` accepts only a reference that is - lowercase hex of exactly 12 characters (the generated reference) or 32 (the harness trace - correlation); any other reference fails open, writing nothing and printing nothing, so the - reference can never steer the record out of the diagnostics directory. + at creation time (no world-readable window). `TryRecord` accepts only a reference that is hex + of exactly 12 characters (the generated reference) or 32 (the harness trace correlation), + in either case — the same shapes `CliStartupTrace` accepts, so the sink never refuses a + reference the CLI itself printed; any other reference fails open, writing nothing and printing + nothing, so the reference can never steer the record out of the diagnostics directory. - **Argv retention policy** (#2577): the record keeps the shape of the failing command, not its - values. A token is written verbatim only when it starts with `-` (a flag name) or is one of - the at most two leading command words, which must be short lowercase words such as `cards add`; - every other token is replaced with the fixed placeholder `[value]`. The result is then still - passed through `SensitiveDataRedactor.Redact`, which masks the attached `key=value` and - `key: value` secret forms. This is the conservative option of the three the #2573 review + values. Exactly two things are written verbatim: the at most two leading command words, which + must be short lowercase words such as `cards add`, and the flag names — a token starting with + `-`, up to but not past its first `=`. Every value is replaced with the fixed placeholder + `[value]`: every other separate token, and the value attached to a flag, so + `--title=Secret plan` is recorded as `--title=[value]` whether or not `title` is a key the + redactor knows. The result is then still passed through `SensitiveDataRedactor.Redact`, which + masks the `key=value` and `key: value` forms whose key it recognises, so `--token=` ends up + `[redacted]` rather than `[value]`. This is the conservative option of the three the #2573 review listed: it covers a space-separated secret flag such as `--token abc123`, which the redactor's `key=value` rules do not match, and it keeps ordinary user content such as a card title or description off disk, since nothing was retained on an operator run before this sink existed. From c7ca196e189429b3b2b5b5f721862214f94bdae6 Mon Sep 17 00:00:00 2001 From: Chris0Jeky Date: Sat, 5 Sep 2026 04:35:48 +0100 Subject: [PATCH 6/6] fix(cli): treat a dash-leading value with whitespace as a value, not a flag name --- backend/src/Taskdeck.Cli/CliFailureSink.cs | 31 ++++++++++++++++++- .../Taskdeck.Cli.Tests/CliFailureSinkTests.cs | 23 ++++++++++++++ docs/security/SECURITY_LOGGING_REDACTION.md | 3 ++ 3 files changed, 56 insertions(+), 1 deletion(-) diff --git a/backend/src/Taskdeck.Cli/CliFailureSink.cs b/backend/src/Taskdeck.Cli/CliFailureSink.cs index d74c2c92f..27e78e884 100644 --- a/backend/src/Taskdeck.Cli/CliFailureSink.cs +++ b/backend/src/Taskdeck.Cli/CliFailureSink.cs @@ -424,7 +424,7 @@ private static string DescribeArguments(IReadOnlyList? arguments) } var argument = arguments[index]; - if (argument.StartsWith('-')) + if (IsFlagName(argument)) { // A flag name is retained, but its attached value is a value like any other: the // redactor only masks the key=value forms whose key it knows, so --title=... or @@ -458,6 +458,35 @@ private static string DescribeArguments(IReadOnlyList? arguments) return string.IsNullOrWhiteSpace(redacted) ? "(none)" : redacted; } + /// + /// A flag name is a dash-prefixed token with no whitespace, such as --title or + /// -v. ArgParser.GetOption accepts any following token as a value, so a value + /// that starts with a dash and contains whitespace (a card title such as "- fix login") is + /// a value, not a flag name, and is replaced like any other value. A single dash-prefixed + /// word used as a value is indistinguishable from a flag name by shape and is retained. + /// + private static bool IsFlagName(string argument) + { + // Only the name part matters: "--title=Secret plan" is a flag with an attached value, + // "- fix login" is a value that happens to start with a dash. + var separator = argument.IndexOf('='); + var name = separator < 0 ? argument : argument[..separator]; + if (name.Length < 2 || name[0] != '-') + { + return false; + } + + foreach (var character in name) + { + if (char.IsWhiteSpace(character)) + { + return false; + } + } + + return true; + } + /// /// The shape every CLI command group and command has: short, lowercase, no whitespace. A /// leading token that is not one (a positional value, a title, anything cased or spaced) is diff --git a/backend/tests/Taskdeck.Cli.Tests/CliFailureSinkTests.cs b/backend/tests/Taskdeck.Cli.Tests/CliFailureSinkTests.cs index d447aedc6..4b1f10287 100644 --- a/backend/tests/Taskdeck.Cli.Tests/CliFailureSinkTests.cs +++ b/backend/tests/Taskdeck.Cli.Tests/CliFailureSinkTests.cs @@ -514,6 +514,29 @@ public void TryRecord_AcceptsAGeneratedReferenceInEitherCase(string reference) /// #2577 follow-up: an attached value must go the same way as a separate one, whether or not /// its flag name is one the redactor knows. Only the flag name survives. /// + [Fact] + public void TryRecord_ReplacesADashLeadingValueThatContainsWhitespace() + { + using var directory = new TemporaryDirectory(); + var sink = CliFailureSink.ForDataDirectory(directory.Path); + + var captured = sink.TryRecord( + CreateLeakyException(), + Reference, + new[] { "cards", "add", "--title", "- fix login for jane@acme.com", "--board", "b1" }, + FixedTimestamp); + + captured.Should().BeTrue(); + var content = File.ReadAllText(Path.Combine( + directory.Path, + CliFailureSink.DirectoryName, + CliFailureSink.BuildFileName(Reference, FixedTimestamp))); + + content.Should().Contain("argv: cards add --title [value] --board [value]"); + content.Should().NotContain("fix login"); + content.Should().NotContain("jane@acme.com"); + } + [Fact] public void TryRecord_ReplacesAnAttachedValueEvenWhenTheFlagIsNotASecretKeyword() { diff --git a/docs/security/SECURITY_LOGGING_REDACTION.md b/docs/security/SECURITY_LOGGING_REDACTION.md index cdf78d3a7..da569b355 100644 --- a/docs/security/SECURITY_LOGGING_REDACTION.md +++ b/docs/security/SECURITY_LOGGING_REDACTION.md @@ -97,6 +97,9 @@ It applies to API middleware, SignalR transport request logging, queue/worker lo listed: it covers a space-separated secret flag such as `--token abc123`, which the redactor's `key=value` rules do not match, and it keeps ordinary user content such as a card title or description off disk, since nothing was retained on an operator run before this sink existed. + A flag name is a dash-prefixed token with no whitespace; a value that starts with a dash and + contains whitespace is replaced like any other value, while a single dash-prefixed word used as + a value is indistinguishable from a flag name by shape and is retained. So `cards add --title "Secret plan" --token abc123` is recorded as `cards add --title [value] --token [value]`. The reference the CLI prints alongside the generic line is the trace correlation when a trace is