Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions SECURITY.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 <token>`) 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
Expand Down
2 changes: 1 addition & 1 deletion docs/compatibility.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
13 changes: 13 additions & 0 deletions src/LakeSpeak.Cli/Commands/CliHost.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using LakeSpeak.Genie;
using LakeSpeak.Rendering;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging;

namespace LakeSpeak.Cli.Commands;

Expand Down Expand Up @@ -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);
Expand Down
54 changes: 54 additions & 0 deletions src/LakeSpeak.Cli/Console/RedactingStderrLogger.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
using LakeSpeak.Genie;
using Microsoft.Extensions.Logging;

namespace LakeSpeak.Cli.Commands;

/// <summary>
/// Writes diagnostics to stderr with every record scrubbed.
/// </summary>
/// <remarks>
/// 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.
/// </remarks>
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>(TState state)
where TState : notnull => null;

public bool IsEnabled(LogLevel logLevel) => logLevel >= LogLevel.Debug;

public void Log<TState>(
LogLevel logLevel,
EventId eventId,
TState state,
Exception? exception,
Func<TState, Exception?, string> 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())}");
}
}
}
}
1 change: 1 addition & 0 deletions src/LakeSpeak.Cli/ExitCode.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
13 changes: 12 additions & 1 deletion src/LakeSpeak.Configuration/LakeSpeakConfig.cs
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,19 @@ public static LakeSpeakConfig Load(string? path = null)

try
{
return deserializer.Deserialize<LakeSpeakConfig>(File.ReadAllText(path))
var loaded = deserializer.Deserialize<LakeSpeakConfig>(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<string, AgentAlias>(
loaded.Agents, StringComparer.OrdinalIgnoreCase);

return loaded;
}
catch (YamlDotNet.Core.YamlException ex)
{
Expand Down
45 changes: 41 additions & 4 deletions src/LakeSpeak.Genie/GenieClient.cs
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,22 @@ private async Task<GenieResponse> 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
Expand Down Expand Up @@ -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);
/// <summary>
/// Converts a Genie timestamp, detecting its unit by magnitude.
/// </summary>
/// <remarks>
/// The field is typed <c>int64</c> 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.
/// </remarks>
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);

Expand Down
18 changes: 14 additions & 4 deletions src/LakeSpeak.Genie/GenieFailures.cs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ public enum GenieFailureKind
QueryResultExpired,
QueryExecutionFailed,
MalformedResponse,

/// <summary>A result shape this version cannot read, such as external links.</summary>
UnsupportedResult,
Network,
Unexpected,
}
Expand Down Expand Up @@ -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 <token>` 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();

/// <summary>Replaces credential-shaped substrings with <see cref="Placeholder"/>.</summary>
Expand Down
16 changes: 16 additions & 0 deletions src/LakeSpeak.Genie/Wire/GenieWireModels.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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<ExternalLinkWire>? 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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
<PackageReference Include="YamlDotNet" />
<PackageReference Include="JsonSchema.Net" />
<ProjectReference Include="..\LakeSpeak.Genie\LakeSpeak.Genie.csproj" />
<ProjectReference Include="..\LakeSpeak.Application\LakeSpeak.Application.csproj" />
<ProjectReference Include="..\LakeSpeak.Rendering\LakeSpeak.Rendering.csproj" />
</ItemGroup>

Expand Down
44 changes: 42 additions & 2 deletions src/LakeSpeak.QuestionPacks/QuestionPack.cs
Original file line number Diff line number Diff line change
Expand Up @@ -204,11 +204,51 @@ private static void ValidateOutputPath(string path, string baseDirectory, List<s
var root = Path.GetFullPath(baseDirectory);
var target = Path.GetFullPath(Path.Combine(root, path));

if (!target.StartsWith(root + Path.DirectorySeparatorChar, StringComparison.Ordinal)
&& !string.Equals(target, root, StringComparison.Ordinal))
if (!IsInside(target, root))
{
errors.Add($"spec.output.path '{path}' resolves outside the pack directory");
return;
}

// The lexical check above is not enough on its own. A pack arrives as a YAML file inside
// a directory the author controls, and that directory can contain a symlink or a Windows
// junction. `link/report.md` then passes every string comparison while the write follows
// the reparse point and lands anywhere the attacker chose — traversal without a single
// `..`. Each component between the root and the target is therefore resolved.
if (FollowsALink(target, root))
{
errors.Add(
$"spec.output.path '{path}' passes through a symbolic link or junction, so its " +
"real destination is outside the pack directory");
}
}

private static bool IsInside(string target, string root) =>
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<string> errors, string context)
Expand Down
12 changes: 9 additions & 3 deletions src/LakeSpeak.Rendering/ResultWriters.cs
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,9 @@ private sealed record ColumnRef
/// <summary>Writes a query result as RFC 4180 CSV.</summary>
public static class CsvWriter
{
// Characters a spreadsheet will skip over before deciding a cell is a formula.
private static readonly char[] FormulaLeadIn = ['\t', '\r', ' '];

/// <summary>
/// Formats the query result. Values are written exactly as Databricks returned them.
/// </summary>
Expand Down Expand Up @@ -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)
Expand Down
Loading
Loading