diff --git a/.github/workflows/website-rebuild.yml b/.github/workflows/website-rebuild.yml index 7de2f6fa..6c9a45b0 100644 --- a/.github/workflows/website-rebuild.yml +++ b/.github/workflows/website-rebuild.yml @@ -12,6 +12,9 @@ on: types: [published] workflow_dispatch: +permissions: + contents: read + jobs: rebuild: runs-on: ubuntu-latest diff --git a/src/Netclaw.Actors.Tests/Reminders/ReminderDefinitionStoreTests.cs b/src/Netclaw.Actors.Tests/Reminders/ReminderDefinitionStoreTests.cs index 35ba2e97..0a6b6728 100644 --- a/src/Netclaw.Actors.Tests/Reminders/ReminderDefinitionStoreTests.cs +++ b/src/Netclaw.Actors.Tests/Reminders/ReminderDefinitionStoreTests.cs @@ -210,6 +210,22 @@ public void ReminderDefinition_id_serializes_as_bare_json_string() Assert.Equal(new ReminderId("reminder-byte-eq"), loaded!.Id); } + [Theory] + [InlineData("../../etc/passwd")] + [InlineData("..\\..\\windows\\system32\\config")] + [InlineData("/etc/passwd")] + [InlineData("C:\\Windows\\System32")] + public void Exists_with_traversal_id_does_not_escape_reminders_directory(string maliciousId) + { + var store = new ReminderDefinitionStore(_paths); + + // Uri.EscapeDataString neutralizes path separators, so the canonical + // path stays inside _basePath and Exists() simply returns false. The + // explicit containment check in GetPath would throw if that invariant + // ever regressed. + Assert.False(store.Exists(new ReminderId(maliciousId))); + } + public void Dispose() { if (Directory.Exists(_basePath)) diff --git a/src/Netclaw.Actors/Reminders/ReminderDefinitionStore.cs b/src/Netclaw.Actors/Reminders/ReminderDefinitionStore.cs index 88ec8a1b..bbb72046 100644 --- a/src/Netclaw.Actors/Reminders/ReminderDefinitionStore.cs +++ b/src/Netclaw.Actors/Reminders/ReminderDefinitionStore.cs @@ -150,8 +150,22 @@ public bool Delete(ReminderId id) private string GetPath(ReminderId id) { + // Uri.EscapeDataString escapes path separators and absolute-path + // markers (/, \, :, control chars), so the encoded value cannot encode a + // traversal. The post-canonicalization containment check below is the + // belt-and-suspenders that CodeQL also recognizes as a path sanitizer. + // (cs/path-injection) var encoded = Uri.EscapeDataString(id.Value); - return Path.Combine(_directory, $"{encoded}.json"); + var baseDir = Path.GetFullPath(_directory); + var candidate = Path.GetFullPath(Path.Combine(baseDir, $"{encoded}.json")); + var baseWithSep = baseDir.EndsWith(Path.DirectorySeparatorChar) + ? baseDir + : baseDir + Path.DirectorySeparatorChar; + if (!candidate.StartsWith(baseWithSep, StringComparison.Ordinal)) + throw new ArgumentException( + $"Reminder id '{id.Value}' resolves outside the reminders directory.", + nameof(id)); + return candidate; } private void PruneInvalidDefinitions() diff --git a/src/Netclaw.Daemon.Tests/Services/DaemonLifecycleNotifierTests.cs b/src/Netclaw.Daemon.Tests/Services/DaemonLifecycleNotifierTests.cs index 79899a3b..c51bdb0f 100644 --- a/src/Netclaw.Daemon.Tests/Services/DaemonLifecycleNotifierTests.cs +++ b/src/Netclaw.Daemon.Tests/Services/DaemonLifecycleNotifierTests.cs @@ -82,6 +82,38 @@ public void NotifyCrashing_EmitsCriticalAlert() Assert.Equal("C123/171313.123", alert.Context["latest_session_id"]); } + [Theory] + [InlineData("ok\r\nlevel=critical attacker=admin", "oklevel=critical attacker=admin")] + [InlineData("ok\nfake-line", "okfake-line")] + [InlineData("ok\u2028fake-line", "okfake-line")] // U+2028 LINE SEPARATOR + [InlineData("ok\u2029fake-line", "okfake-line")] // U+2029 PARAGRAPH SEPARATOR + [InlineData("ok\0NUL", "okNUL")] + public void NotifyShutdown_strips_log_line_breakers_from_reason(string raw, string expected) + { + _sut.NotifyShutdown(raw); + + var alert = Assert.Single(_sink.Alerts); + Assert.Equal(expected, alert.Context!["reason"]); + Assert.Equal($"Netclaw daemon stopping: {expected}", alert.Summary); + } + + [Fact] + public void NotifyShutdown_truncates_reason_at_200_chars_without_splitting_surrogate_pair() + { + // 99 emoji = 198 UTF-16 chars (2 each), then 1 emoji at positions + // 198–199, then plain ASCII tail. Position 200 would split the 100th + // emoji's surrogate pair if we naively cut at char index 200. + var emoji = "😀"; // U+1F600 GRINNING FACE + var raw = string.Concat(Enumerable.Repeat(emoji, 100)) + "TAIL"; + + _sut.NotifyShutdown(raw); + + var reason = Assert.Single(_sink.Alerts).Context!["reason"]; + Assert.True(reason.Length <= 200); + Assert.False(char.IsHighSurrogate(reason[^1]), + "truncated reason ended on a high surrogate — dangling surrogate would break UTF-8 encoders downstream"); + } + private sealed class RecordingSink : IOperationalNotificationSink { public List Alerts { get; } = []; diff --git a/src/Netclaw.Daemon/Services/DaemonLifecycleNotifier.cs b/src/Netclaw.Daemon/Services/DaemonLifecycleNotifier.cs index 5dcd2655..3faf78a7 100644 --- a/src/Netclaw.Daemon/Services/DaemonLifecycleNotifier.cs +++ b/src/Netclaw.Daemon/Services/DaemonLifecycleNotifier.cs @@ -4,6 +4,7 @@ // // ----------------------------------------------------------------------- using System.Globalization; +using System.Text; using Microsoft.Extensions.Logging; using Netclaw.Configuration; @@ -60,12 +61,17 @@ public void NotifyStarted() public void NotifyShutdown(string reason, IReadOnlyDictionary? additionalContext = null) { var pid = Environment.ProcessId; - _logger.LogInformation("Netclaw daemon stopping (PID {Pid}, reason: {Reason})", pid, reason); + // `reason` reaches this method straight from the HTTP query string + // (ShutdownDaemonRequest in LifecycleEndpointRouteBuilderExtensions), so a + // caller can inject CR/LF into log lines. Sanitize before logging or + // propagating into the alert context. (cs/log-forging) + var safeReason = SanitizeReason(reason); + _logger.LogInformation("Netclaw daemon stopping (PID {Pid}, reason: {Reason})", pid, safeReason); var context = new Dictionary { ["pid"] = pid.ToString(CultureInfo.InvariantCulture), - ["reason"] = reason, + ["reason"] = safeReason, }; if (additionalContext is not null) @@ -78,7 +84,7 @@ public void NotifyShutdown(string reason, IReadOnlyDictionary? a _timeProvider, type: "daemon.stopping", category: AlertType.DaemonStopping, - summary: $"Netclaw daemon stopping: {reason}", + summary: $"Netclaw daemon stopping: {safeReason}", severity: AlertSeverity.Info, source: pid.ToString(CultureInfo.InvariantCulture), context: context)); @@ -99,18 +105,22 @@ public void NotifyCrashing( var exceptionMessage = string.IsNullOrWhiteSpace(exception.Message) ? "(no exception message)" : exception.Message; + // Today every caller passes a code-side constant for `reason`, but + // sanitizing here keeps the contract symmetric with NotifyShutdown and + // forecloses on future callers that might forward HTTP input. + var safeReason = SanitizeReason(reason); _logger.LogCritical( exception, "Netclaw daemon crashing (PID {Pid}, reason: {Reason}, exceptionType: {ExceptionType})", pid, - reason, + safeReason, exceptionType); var context = new Dictionary(StringComparer.Ordinal) { ["pid"] = pid.ToString(CultureInfo.InvariantCulture), - ["reason"] = reason, + ["reason"] = safeReason, ["exceptionType"] = exceptionType, ["exceptionMessage"] = Trim(exceptionMessage, 400), }; @@ -130,7 +140,7 @@ public void NotifyCrashing( _timeProvider, type: "daemon.crashing", category: AlertType.DaemonCrashed, - summary: $"Netclaw daemon crashing: {reason} ({exceptionType})", + summary: $"Netclaw daemon crashing: {safeReason} ({exceptionType})", severity: AlertSeverity.Critical, source: pid.ToString(CultureInfo.InvariantCulture), context: context)); @@ -148,4 +158,58 @@ private static string Trim(string value, int maxChars) return value[..maxChars]; } + + private const int MaxReasonChars = 200; + + // Returns true for chars that, if left in a log line or alert summary, can + // act as a line terminator for at least one downstream consumer: + // - char.IsControl covers Cc (CR/LF/NUL/ESC/etc.) + // - U+2028 (LINE SEPARATOR, Zl) and U+2029 (PARAGRAPH SEPARATOR, Zp) are NOT + // in Cc and so are not caught by IsControl, but JSON-line readers, many + // log shippers, and pre-ES2019 JSON parsers still split on them. + private static bool IsLogLineBreak(char c) + => char.IsControl(c) || c is '\u2028' or '\u2029'; + + private static string SanitizeReason(string reason) + { + if (string.IsNullOrEmpty(reason)) + return string.Empty; + + var hasBreak = false; + foreach (var ch in reason) + { + if (IsLogLineBreak(ch)) + { + hasBreak = true; + break; + } + } + + if (!hasBreak) + return TrimAtCharBoundary(reason, MaxReasonChars); + + // Strip — don't space-replace. Space would let a CR/LF payload still + // pass through as a plausible field separator to key=value structured + // log parsers ("reason=ok\nlevel=critical" → "reason=ok level=critical"). + var buf = new StringBuilder(reason.Length); + foreach (var ch in reason) + { + if (!IsLogLineBreak(ch)) + buf.Append(ch); + } + return TrimAtCharBoundary(buf.ToString(), MaxReasonChars); + } + + private static string TrimAtCharBoundary(string value, int maxChars) + { + if (value.Length <= maxChars) + return value; + + // Don't split a surrogate pair at the truncation boundary — a dangling + // high surrogate makes downstream UTF-8 encoders emit U+FFFD or throw. + var cut = maxChars; + if (char.IsHighSurrogate(value[cut - 1])) + cut -= 1; + return value[..cut]; + } } diff --git a/src/Netclaw.Daemon/Webhooks/WebhookEndpointRouteBuilderExtensions.cs b/src/Netclaw.Daemon/Webhooks/WebhookEndpointRouteBuilderExtensions.cs index 4ef910d1..fef42fe0 100644 --- a/src/Netclaw.Daemon/Webhooks/WebhookEndpointRouteBuilderExtensions.cs +++ b/src/Netclaw.Daemon/Webhooks/WebhookEndpointRouteBuilderExtensions.cs @@ -43,10 +43,16 @@ public static IEndpointRouteBuilder MapWebhookEndpoints(this IEndpointRouteBuild if (!routeCatalog.TryGetRoute(route, out var registeredRoute)) { - WebhookTelemetry.RecordRouteNotFound(route); + // Route comes from the URL path and is URL-decoded by ASP.NET routing, + // so a caller can inject CR/LF and other control chars — both as a + // log-forging vector and as an unbounded metric-tag cardinality vector + // against the `route` dimension on WebhookTelemetry counters. Sanitize + // once and use the safe value for both surfaces. (cs/log-forging) + var safeRoute = SanitizeWebhookId(route); + WebhookTelemetry.RecordRouteNotFound(safeRoute); logger.LogWarning( "Webhook rejected route={Route} reason={Reason} remote_ip={RemoteIp} delivery_id={DeliveryId} event_type={EventType}", - route, "route_not_found", remoteIp, (string?)null, (string?)null); + safeRoute, "route_not_found", remoteIp, (string?)null, (string?)null); return TypedResults.NotFound(); }