diff --git a/SECURITY.md b/SECURITY.md index 471a0ec..1e101fe 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -25,8 +25,10 @@ and no token is written to its configuration file. **Tokens are never passed as process arguments.** Command lines are readable by other users on the same host on most platforms. -**Authorization headers are redacted** in all diagnostic output, including `--debug`, and in -exception messages. +**Authorization headers are redacted** in all diagnostic output, including `--verbose`, and in +exception messages. Scheme-prefixed credentials (`Authorization: Bearer `) are covered — +an earlier pattern redacted only the scheme word and left the credential, which is why the +redaction suite now tries to defeat itself rather than confirming it works on convenient shapes. **Presigned result URLs are fetched without the `Authorization` header.** Sending a Databricks bearer token to blob storage would leak it to a third party. Databricks rejects such requests with diff --git a/docs/compatibility.md b/docs/compatibility.md index 47efb3d..a41fe2d 100644 --- a/docs/compatibility.md +++ b/docs/compatibility.md @@ -75,7 +75,7 @@ contract tests only. | Credential redaction, both signature fields | Unit tests using realistic JSON payloads | | Question Pack validation, including path traversal | Unit tests, plus the CLI run by hand | | CLI parsing, help, exit codes, `config show` leaking nothing | The built binary run by hand | -| **Anything against real Databricks** | **Not done.** No live workspace has been contacted. | +| Against real Databricks | See "Live verification" above. `agents list`, `ask`, every output format, `pack run`, `export last` and `feedback last` were run against a live workspace; `chat`, chunked/external-link results and `QUERY_RESULT_EXPIRED` recovery were not. | ## How to update this file diff --git a/src/LakeSpeak.Cli/Commands/CliHost.cs b/src/LakeSpeak.Cli/Commands/CliHost.cs index 031ee59..d81c9eb 100644 --- a/src/LakeSpeak.Cli/Commands/CliHost.cs +++ b/src/LakeSpeak.Cli/Commands/CliHost.cs @@ -5,6 +5,7 @@ using LakeSpeak.Genie; using LakeSpeak.Rendering; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; namespace LakeSpeak.Cli.Commands; @@ -46,6 +47,18 @@ internal static CliHost Create(ParseResult parseResult) var profile = parseResult.GetValue(GlobalOptions.Profile) ?? config.Defaults.Profile; var services = new ServiceCollection(); + + // --verbose was declared and registered but never read, so it did nothing while the help + // text promised diagnostics "with credentials always redacted" — a guarantee about + // behaviour that did not exist. The client already logs through ILogger; it simply had no + // sink. Every record is scrubbed on its way out, which is what makes the promise true. + if (parseResult.GetValue(GlobalOptions.Verbose)) + { + services.AddLogging(builder => builder + .SetMinimumLevel(LogLevel.Debug) + .AddProvider(new RedactingStderrLoggerProvider())); + } + services.AddLakeSpeak(options => options.Profile = profile); return new CliHost(services.BuildServiceProvider(), output, config, format); diff --git a/src/LakeSpeak.Cli/Console/RedactingStderrLogger.cs b/src/LakeSpeak.Cli/Console/RedactingStderrLogger.cs new file mode 100644 index 0000000..34c0123 --- /dev/null +++ b/src/LakeSpeak.Cli/Console/RedactingStderrLogger.cs @@ -0,0 +1,54 @@ +using LakeSpeak.Genie; +using Microsoft.Extensions.Logging; + +namespace LakeSpeak.Cli.Commands; + +/// +/// Writes diagnostics to stderr with every record scrubbed. +/// +/// +/// Diagnostics go to stderr so `--verbose` never corrupts machine-readable stdout. Scrubbing +/// happens here, at the single point every record passes through, rather than at each call site: +/// a log line is exactly the kind of thing that ends up in a CI transcript or a pasted bug +/// report, and one forgotten call site would be enough to disclose a credential. +/// +internal sealed class RedactingStderrLoggerProvider : ILoggerProvider +{ + public ILogger CreateLogger(string categoryName) => new RedactingStderrLogger(categoryName); + + public void Dispose() + { + } + + private sealed class RedactingStderrLogger(string category) : ILogger + { + public IDisposable? BeginScope(TState state) + where TState : notnull => null; + + public bool IsEnabled(LogLevel logLevel) => logLevel >= LogLevel.Debug; + + public void Log( + LogLevel logLevel, + EventId eventId, + TState state, + Exception? exception, + Func formatter) + { + if (!IsEnabled(logLevel)) + { + return; + } + + var line = DiagnosticRedaction.Scrub(formatter(state, exception)); + var name = category.Split('.')[^1]; + + System.Console.Error.WriteLine($" [{logLevel.ToString().ToLowerInvariant()}] {name}: {line}"); + + if (exception is not null) + { + System.Console.Error.WriteLine( + $" [{logLevel.ToString().ToLowerInvariant()}] {name}: {DiagnosticRedaction.Scrub(exception.ToString())}"); + } + } + } +} diff --git a/src/LakeSpeak.Cli/ExitCode.cs b/src/LakeSpeak.Cli/ExitCode.cs index 7ccfef1..730b398 100644 --- a/src/LakeSpeak.Cli/ExitCode.cs +++ b/src/LakeSpeak.Cli/ExitCode.cs @@ -35,6 +35,7 @@ public static class ExitCode GenieFailureKind.PollingTimeout => Timeout, GenieFailureKind.RateLimited => GenieFailure, GenieFailureKind.MalformedResponse => MalformedResponse, + GenieFailureKind.UnsupportedResult => MalformedResponse, GenieFailureKind.Network => Unexpected, GenieFailureKind.Unexpected => Unexpected, // GenieFailureKind is this project's own closed set, so an unhandled arm is a bug to diff --git a/src/LakeSpeak.Configuration/LakeSpeakConfig.cs b/src/LakeSpeak.Configuration/LakeSpeakConfig.cs index f2cfffe..68df7f1 100644 --- a/src/LakeSpeak.Configuration/LakeSpeakConfig.cs +++ b/src/LakeSpeak.Configuration/LakeSpeakConfig.cs @@ -57,8 +57,19 @@ public static LakeSpeakConfig Load(string? path = null) try { - return deserializer.Deserialize(File.ReadAllText(path)) + var loaded = deserializer.Deserialize(File.ReadAllText(path)) ?? new LakeSpeakConfig(); + + // YamlDotNet constructs its own dictionary and assigns it over the field + // initializer, discarding the OrdinalIgnoreCase comparer. Without rebuilding it, + // an alias written `Finance:` would not match `--agent finance`, and resolution + // would silently fall through to title matching — which can select a different + // Agent entirely. That is the "answer against the wrong data" failure this + // codebase exists to avoid. + loaded.Agents = new Dictionary( + loaded.Agents, StringComparer.OrdinalIgnoreCase); + + return loaded; } catch (YamlDotNet.Core.YamlException ex) { diff --git a/src/LakeSpeak.Genie/GenieClient.cs b/src/LakeSpeak.Genie/GenieClient.cs index 03cacf3..4600343 100644 --- a/src/LakeSpeak.Genie/GenieClient.cs +++ b/src/LakeSpeak.Genie/GenieClient.cs @@ -343,6 +343,22 @@ private async Task CompleteAsync( .Select(c => new GenieColumn(c.Name ?? string.Empty, c.TypeText ?? c.TypeName, c.TypeName)) .ToList(); + // EXTERNAL_LINKS disposition puts the rows behind presigned URLs and omits data_array + // entirely. This client cannot follow those links, and returning an empty row set as a + // successful complete result would be the worst outcome available — a silently empty + // export that looks like a real answer. Fail loudly instead. + if (statement.Result?.ExternalLinks is { Count: > 0 } links + && statement.Result.DataArray is null) + { + var rowsBehindLinks = links.Sum(l => l.RowCount ?? 0); + throw new GenieException( + GenieFailureKind.UnsupportedResult, + $"Databricks returned this result as {links.Count} external link(s) covering " + + $"{rowsBehindLinks} row(s) rather than inline rows. This version cannot read that " + + "form, and will not report an empty result as a complete one. Narrow the question " + + "so the result comes back inline."); + } + var rows = statement.Result?.DataArray ?? []; // The Statement Execution contract chunks large results, and this client reads only the @@ -404,16 +420,37 @@ private static GenieResponse Normalize( new GenieResponseMetadata( duration, pollCount, - FromUnixMillis(wire.CreatedTimestamp), - FromUnixMillis(wire.LastUpdatedTimestamp), + FromUnixTimestamp(wire.CreatedTimestamp), + FromUnixTimestamp(wire.LastUpdatedTimestamp), queryAttachment?.AttachmentId)) { HasVisualization = attachments.Any(a => a.Viz is not null), }; } - private static DateTimeOffset? FromUnixMillis(long? value) - => value is null or 0 ? null : DateTimeOffset.FromUnixTimeMilliseconds(value.Value); + /// + /// Converts a Genie timestamp, detecting its unit by magnitude. + /// + /// + /// The field is typed int64 and its unit is undocumented — the SDK name suggests + /// milliseconds, while the one published example is ten digits, which is seconds. Assuming + /// milliseconds on a seconds value yields January 1970 instead of the real date: a silent + /// 55-year error on a public property. The threshold is the year 2001 in milliseconds; any + /// plausible Genie timestamp in seconds is far below it, and any in milliseconds far above. + /// + private static DateTimeOffset? FromUnixTimestamp(long? value) + { + if (value is null or <= 0) + { + return null; + } + + const long millisecondThreshold = 100_000_000_000L; + + return value.Value >= millisecondThreshold + ? DateTimeOffset.FromUnixTimeMilliseconds(value.Value) + : DateTimeOffset.FromUnixTimeSeconds(value.Value); + } private static string Esc(string segment) => Uri.EscapeDataString(segment); diff --git a/src/LakeSpeak.Genie/GenieFailures.cs b/src/LakeSpeak.Genie/GenieFailures.cs index d7405e2..55217c8 100644 --- a/src/LakeSpeak.Genie/GenieFailures.cs +++ b/src/LakeSpeak.Genie/GenieFailures.cs @@ -22,6 +22,9 @@ public enum GenieFailureKind QueryResultExpired, QueryExecutionFailed, MalformedResponse, + + /// A result shape this version cannot read, such as external links. + UnsupportedResult, Network, Unexpected, } @@ -86,10 +89,17 @@ public static partial class DiagnosticRedaction [GeneratedRegex(@"\bey[A-Za-z0-9_-]{8,}\.[A-Za-z0-9_-]{8,}\.[A-Za-z0-9_-]{8,}", RegexOptions.None, matchTimeoutMilliseconds: 1000)] private static partial Regex Jwt(); - // The optional quote after the key name matters: in JSON the key is `"name":"value"`, so a - // pattern that jumps straight from the name to the separator never matches, and the secret - // survives. A test with a realistic JSON payload caught exactly that. - [GeneratedRegex(@"(?i)\b(authorization|bearer|x-databricks-[\w-]*token|access_token|refresh_token|client_secret|download_id_signature|statement_id_signature)\b[""']?\s*[:=]?\s*[""']?([^\s""',}]+)", RegexOptions.None, matchTimeoutMilliseconds: 1000)] + // Two details carry this pattern, and both were found by a credential surviving it. + // + // The optional quote after the key name: in JSON the key is `"name":"value"`, so a pattern + // jumping straight from the name to the separator never matches and the secret survives. + // + // The optional scheme word: `Authorization: Bearer ` is the single most common way a + // credential is written down. Without consuming `Bearer`/`Basic` first, the value capture + // stops at the space and redacts only the scheme — leaving the credential itself in the + // output. That is precisely what happened, so the scheme is consumed and the token after it + // is what gets taken. + [GeneratedRegex(@"(?i)\b(authorization|bearer|x-databricks-[\w-]*token|access_token|refresh_token|client_secret|download_id_signature|statement_id_signature)\b[""']?\s*[:=]?\s*(?:(?:bearer|basic)\s+)?[""']?([^\s""',}]+)", RegexOptions.None, matchTimeoutMilliseconds: 1000)] private static partial Regex NamedSecret(); /// Replaces credential-shaped substrings with . diff --git a/src/LakeSpeak.Genie/Wire/GenieWireModels.cs b/src/LakeSpeak.Genie/Wire/GenieWireModels.cs index fbf4d49..869551a 100644 --- a/src/LakeSpeak.Genie/Wire/GenieWireModels.cs +++ b/src/LakeSpeak.Genie/Wire/GenieWireModels.cs @@ -276,6 +276,22 @@ internal sealed record ResultDataWire // chunk, so this is what stops a partial result being reported as complete. [JsonPropertyName("next_chunk_index")] public int? NextChunkIndex { get; init; } + + // Under EXTERNAL_LINKS disposition the rows are NOT inline: data_array is absent and the + // data sits behind presigned URLs. Deserialised solely so that case can be detected and + // refused — returning zero rows as a successful, complete result would be worse than any + // error this client can raise. + [JsonPropertyName("external_links")] + public IReadOnlyList? ExternalLinks { get; init; } +} + +internal sealed record ExternalLinkWire +{ + [JsonPropertyName("chunk_index")] + public int? ChunkIndex { get; init; } + + [JsonPropertyName("row_count")] + public long? RowCount { get; init; } } internal sealed record DownloadHandleWire diff --git a/src/LakeSpeak.QuestionPacks/LakeSpeak.QuestionPacks.csproj b/src/LakeSpeak.QuestionPacks/LakeSpeak.QuestionPacks.csproj index bb79a64..15a29bb 100644 --- a/src/LakeSpeak.QuestionPacks/LakeSpeak.QuestionPacks.csproj +++ b/src/LakeSpeak.QuestionPacks/LakeSpeak.QuestionPacks.csproj @@ -4,7 +4,6 @@ - diff --git a/src/LakeSpeak.QuestionPacks/QuestionPack.cs b/src/LakeSpeak.QuestionPacks/QuestionPack.cs index 8c913f2..5a4ef3f 100644 --- a/src/LakeSpeak.QuestionPacks/QuestionPack.cs +++ b/src/LakeSpeak.QuestionPacks/QuestionPack.cs @@ -204,11 +204,51 @@ private static void ValidateOutputPath(string path, string baseDirectory, List + target.StartsWith(root + Path.DirectorySeparatorChar, StringComparison.Ordinal) + || string.Equals(target, root, StringComparison.Ordinal); + + private static bool FollowsALink(string target, string root) + { + for (var current = Path.GetDirectoryName(target); + current is not null && current.Length >= root.Length; + current = Path.GetDirectoryName(current)) + { + var info = new DirectoryInfo(current); + if (!info.Exists) + { + continue; + } + + // ResolveLinkTarget returns null for an ordinary directory, so a non-null answer + // means this component redirects somewhere. Re-check where it actually lands. + if (info.ResolveLinkTarget(returnFinalTarget: true) is { } resolved + && !IsInside(Path.GetFullPath(resolved.FullName), root)) + { + return true; + } } + + return false; } private static TimeSpan? ParseDuration(string? value, List errors, string context) diff --git a/src/LakeSpeak.Rendering/ResultWriters.cs b/src/LakeSpeak.Rendering/ResultWriters.cs index 870f77e..accd26b 100644 --- a/src/LakeSpeak.Rendering/ResultWriters.cs +++ b/src/LakeSpeak.Rendering/ResultWriters.cs @@ -173,6 +173,9 @@ private sealed record ColumnRef /// Writes a query result as RFC 4180 CSV. public static class CsvWriter { + // Characters a spreadsheet will skip over before deciding a cell is a formula. + private static readonly char[] FormulaLeadIn = ['\t', '\r', ' ']; + /// /// Formats the query result. Values are written exactly as Databricks returned them. /// @@ -204,9 +207,12 @@ private static string Quote(string? value) return string.Empty; } - // A leading =, +, - or @ makes a spreadsheet treat the cell as a formula. Prefixing a - // single quote is the conventional defence and is visible rather than silent. - var needsFormulaGuard = value.Length > 0 && value[0] is '=' or '+' or '-' or '@'; + // A leading =, +, - or @ makes a spreadsheet treat the cell as a formula. Tab and + // carriage return count too: OWASP documents both as accepted prefixes before the + // marker, and some spreadsheet versions honour them. Prefixing a single quote is the + // conventional defence and is visible rather than silent. + var lead = value.AsSpan().TrimStart(FormulaLeadIn); + var needsFormulaGuard = lead.Length > 0 && lead[0] is '=' or '+' or '-' or '@'; var escaped = value.Replace("\"", "\"\"", StringComparison.Ordinal); if (needsFormulaGuard) diff --git a/tests/LakeSpeak.Genie.Tests/RedactionEvasionTests.cs b/tests/LakeSpeak.Genie.Tests/RedactionEvasionTests.cs new file mode 100644 index 0000000..3bcc1ca --- /dev/null +++ b/tests/LakeSpeak.Genie.Tests/RedactionEvasionTests.cs @@ -0,0 +1,71 @@ +using LakeSpeak.Genie; + +namespace LakeSpeak.Genie.Tests; + +/// +/// Attempts to defeat the scrubber, rather than confirming it works on values shaped to suit it. +/// +/// +/// The original tests all used single-token values with no embedded space, which is exactly the +/// shape the pattern handled. A scheme-prefixed credential — the single most common way an +/// Authorization header is written down — slipped straight through. +/// +public class RedactionEvasionTests +{ + // The token here is deliberately neither JWT-shaped nor dapi-shaped, so the only thing that + // can catch it is the named-key rule. + private const string Opaque = "sometoken_not_jwt_or_dapi_shaped_1234567890"; + + [Theory] + [InlineData("Authorization: Bearer ")] + [InlineData("authorization: bearer ")] + [InlineData("Authorization:Bearer ")] + [InlineData("Authorization: Basic ")] + [InlineData("Authorization = Bearer ")] + public void A_scheme_prefixed_credential_does_not_survive(string prefix) + { + var scrubbed = DiagnosticRedaction.Scrub(prefix + Opaque); + + // Redacting only the word "Bearer" and leaving the credential is the failure this + // whole class exists to catch. + scrubbed.ShouldNotContain(Opaque); + } + + [Fact] + public void A_bearer_token_on_its_own_does_not_survive() + { + DiagnosticRedaction.Scrub($"Bearer {Opaque}").ShouldNotContain(Opaque); + } + + [Theory] + [InlineData("access_token: Bearer ")] + [InlineData("client_secret = ")] + [InlineData("download_id_signature: ")] + [InlineData("statement_id_signature: ")] + public void Named_secrets_do_not_survive_regardless_of_separator(string prefix) + { + DiagnosticRedaction.Scrub(prefix + Opaque).ShouldNotContain(Opaque); + } + + // Realistic shape: a header dump, where the credential is followed by more headers. The + // scrubber must take the credential without eating the rest of the line's structure. + [Fact] + public void Redacts_the_credential_in_a_header_dump_without_eating_everything() + { + var scrubbed = DiagnosticRedaction.Scrub( + $"Authorization: Bearer {Opaque}\nContent-Type: application/json"); + + scrubbed.ShouldNotContain(Opaque); + scrubbed.ShouldContain("Content-Type: application/json"); + } + + [Fact] + public void Ordinary_prose_containing_the_word_bearer_is_left_readable() + { + const string prose = "The bearer of this message is not authorized."; + + // Over-scrubbing prose would make diagnostics useless; the value after the keyword is + // taken, but the sentence must stay recognisable. + DiagnosticRedaction.Scrub(prose).ShouldContain("The bearer"); + } +} diff --git a/tests/LakeSpeak.QuestionPacks.Tests/ConfigAliasTests.cs b/tests/LakeSpeak.QuestionPacks.Tests/ConfigAliasTests.cs new file mode 100644 index 0000000..3dc0156 --- /dev/null +++ b/tests/LakeSpeak.QuestionPacks.Tests/ConfigAliasTests.cs @@ -0,0 +1,74 @@ +using LakeSpeak.Configuration; + +namespace LakeSpeak.QuestionPacks.Tests; + +/// +/// Alias lookup must be case-insensitive after a YAML round trip. +/// +/// +/// The dictionary is declared with StringComparer.OrdinalIgnoreCase, but YamlDotNet builds +/// its own dictionary and assigns it over the field initializer, silently discarding the +/// comparer. The declaration therefore proves nothing on its own — only a round trip does. +/// +public sealed class ConfigAliasTests : IDisposable +{ + private readonly string _path = Path.Combine(Path.GetTempPath(), $"lakespeak-{Guid.NewGuid():N}.yaml"); + + private LakeSpeakConfig Load(string yaml) + { + File.WriteAllText(_path, yaml); + return LakeSpeakConfig.Load(_path); + } + + [Theory] + [InlineData("finance")] + [InlineData("Finance")] + [InlineData("FINANCE")] + [InlineData("FiNaNcE")] + public void An_alias_resolves_regardless_of_case(string typed) + { + var config = Load( + """ + version: 1 + agents: + Finance: + id: 01f-finance + """); + + config.Agents.TryGetValue(typed, out var alias).ShouldBeTrue(); + alias!.Id.ShouldBe("01f-finance"); + } + + // Falling through to title matching on a case mismatch is not a harmless extra round trip: + // it can select a different Agent whose title happens to match, which is the + // answer-against-the-wrong-data failure the resolver exists to prevent. + [Fact] + public void The_comparer_survives_deserialisation() + { + var config = Load( + """ + version: 1 + agents: + Sales: + id: 01f-sales + """); + + config.Agents.Comparer.ShouldBe(StringComparer.OrdinalIgnoreCase); + } + + [Fact] + public void A_config_with_no_agents_section_still_has_a_case_insensitive_dictionary() + { + var config = Load("version: 1"); + + config.Agents.Comparer.ShouldBe(StringComparer.OrdinalIgnoreCase); + } + + public void Dispose() + { + if (File.Exists(_path)) + { + File.Delete(_path); + } + } +} diff --git a/tests/LakeSpeak.QuestionPacks.Tests/LakeSpeak.QuestionPacks.Tests.csproj b/tests/LakeSpeak.QuestionPacks.Tests/LakeSpeak.QuestionPacks.Tests.csproj index 5dc4cdc..8598bc7 100644 --- a/tests/LakeSpeak.QuestionPacks.Tests/LakeSpeak.QuestionPacks.Tests.csproj +++ b/tests/LakeSpeak.QuestionPacks.Tests/LakeSpeak.QuestionPacks.Tests.csproj @@ -3,6 +3,7 @@ +