diff --git a/backend/src/Taskdeck.Cli/CliFailureSink.cs b/backend/src/Taskdeck.Cli/CliFailureSink.cs index 7c83f65b9..27e78e884 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 — 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 @@ -42,15 +45,40 @@ 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 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 = CliStartupTrace.CorrelationLength; + + /// + /// 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); 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; @@ -59,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); } } @@ -172,7 +210,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 +222,6 @@ internal bool TryRecord( try { Directory.CreateDirectory(_diagnosticsDirectory); - EvictOldestRecords(_diagnosticsDirectory); var path = Path.Combine(_diagnosticsDirectory, BuildFileName(reference, timestamp)); @@ -204,9 +245,30 @@ 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. + // + // 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) @@ -271,22 +333,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 void EvictOldestRecords(string diagnosticsDirectory, string writtenPath) { - var existing = Directory.GetFiles(diagnosticsDirectory, FileNameSearchPattern); - if (existing.Length < MaximumRecordCount) + var existing = _listRecords(diagnosticsDirectory, FileNameSearchPattern); + 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 +367,19 @@ 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 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(Uri.IsHexDigit); + private static string DescribeVersion() { try @@ -311,6 +393,20 @@ 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) 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, + /// 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 +414,88 @@ 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 (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 + // --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. + 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; } + /// + /// 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 + /// 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) 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 680b8007e..4b1f10287 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); @@ -281,6 +282,285 @@ 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 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("0a1b2c3d4e5")] + [InlineData("0a1b2c3d4e5f0")] + [InlineData("zzzzzzzzzzzz")] + [InlineData("")] + [InlineData(" ")] + public void TryRecord_WithAReferenceThatIsNotAnAcceptedCorrelation_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"); + } + + /// + /// 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_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() + { + 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() diff --git a/docs/security/SECURITY_LOGGING_REDACTION.md b/docs/security/SECURITY_LOGGING_REDACTION.md index 460164dc9..da569b355 100644 --- a/docs/security/SECURITY_LOGGING_REDACTION.md +++ b/docs/security/SECURITY_LOGGING_REDACTION.md @@ -70,13 +70,38 @@ 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. 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 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. 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. + 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 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