From 967ddf48fa59dfefaf80262f6e201c0f646802e9 Mon Sep 17 00:00:00 2001 From: Kamron Batman <3953314+kamronbatman@users.noreply.github.com> Date: Mon, 27 Jul 2026 23:31:37 -0700 Subject: [PATCH] fix(crowdsec): send a payload LAPI accepts (500 on alerts, 401 on auth) (#2553) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem Contributing a ban to CrowdSec failed against a real LAPI — `POST /v1/alerts` answered **500**, and depending on the shard's locale, auth answered **401**. Three independent defects, each sufficient on its own. ## Fixes **`scenario_hash` / `scenario_version` were never serialized.** LAPI dereferences both unconditionally when persisting an alert, so omitting them is a nil deref and a 500 rather than a validation error. Both are now emitted with the values a watcher without a hub scenario is expected to send (`""` and `"1.0"`). **`start_at`/`stop_at` were formatted without an `IFormatProvider`.** `:` is the time separator *specifier* in a custom .NET format string, not a literal — a shard running under a culture like `fi-FI` emitted `T15.04.05.123Z`, which Go's `time.RFC3339` rejects, producing another 500. Non-Gregorian cultures (`th-TH`, `ar-SA`) would also shift the year. Formatting is now pinned to `InvariantCulture` in `FormatTimestamp`, which additionally converts non-UTC input — the trailing `Z` is a literal and was previously an unchecked claim. **The `User-Agent` was a plain product string.** LAPI's default watcher profile matches the `crowdsec/` prefix and answers 401 without it, so the header is a protocol constraint, not cosmetic. It is now an `internal const` carrying that reason. Also fixed, same root cause as the timestamp bug: the login-expiry parse used a bare `DateTime.TryParse` on LAPI's RFC3339 `expire`. Under a mismatched culture that silently fails and falls back to a fabricated `UtcNow + 1h`, pushing re-auth past the real expiry and costing a 401-relogin round trip on every send. `capacity` now defaults to `1` instead of `0`, matching the one-decision-per-alert shape actually being sent. ## Note on scope The two 500 causes are independent. On an `en-US` shard only the missing scenario fields were biting; the date bug was latent and would have surfaced as an unexplained regression the first time someone ran a shard under a European locale. ## Verification The emitted payload is field-for-field identical to a hand-verified request that a live LAPI accepts: ```json [ { "scenario": "modernuo/rate-limit", "scenario_hash": "", "scenario_version": "1.0", "message": "ModernUO rate-limit ban for 192.0.2.123", "events_count": 1, "start_at": "2026-07-27T15:04:05.123Z", "stop_at": "2026-07-27T15:04:05.123Z", "capacity": 1, "leakspeed": "0s", "simulated": false, "events": [], "remediation": true, "source": { "scope": "Ip", "value": "192.0.2.123" }, "decisions": [ { "origin": "modernuo", "type": "ban", "scope": "Ip", "value": "192.0.2.123", "duration": "300s", "scenario": "modernuo/rate-limit" } ] } ] ``` Regression tests assert the required scenario fields on the **serialized JSON** rather than the DTO — the DTO is not what goes on the wire — and cover the timestamp as a `[Theory]` across `fi-FI`/`th-TH`/`ar-SA`. `dotnet test --filter "FullyQualifiedName~CrowdSec"` → **21/21 passed**, build clean with 0 warnings. --- .../Network/Bans/CrowdSecAlertClientTests.cs | 11 ++++ .../Network/Bans/CrowdSecReporterTests.cs | 56 +++++++++++++++++++ .../UOContent/Misc/CrowdSec/CrowdSecAlert.cs | 15 ++++- .../Misc/CrowdSec/CrowdSecAlertClient.cs | 19 ++++++- .../Misc/CrowdSec/CrowdSecReporter.cs | 14 ++++- 5 files changed, 111 insertions(+), 4 deletions(-) diff --git a/Projects/UOContent.Tests/Tests/Network/Bans/CrowdSecAlertClientTests.cs b/Projects/UOContent.Tests/Tests/Network/Bans/CrowdSecAlertClientTests.cs index f8a990f25..756e19b1d 100644 --- a/Projects/UOContent.Tests/Tests/Network/Bans/CrowdSecAlertClientTests.cs +++ b/Projects/UOContent.Tests/Tests/Network/Bans/CrowdSecAlertClientTests.cs @@ -13,6 +13,7 @@ * along with this program. If not, see . * *************************************************************************/ +using System; using System.Net; using Server.Network.Bans.CrowdSec; using Xunit; @@ -21,6 +22,16 @@ namespace Server.Tests.Network.Bans; public class CrowdSecAlertClientTests { + /// + /// LAPI's default watcher profile matches the crowdsec/ prefix and answers 401 without it, so + /// this is a protocol constraint rather than a cosmetic product string. + /// + [Fact] + public void UserAgent_IsPrefixedForTheWatcherProfile() + { + Assert.StartsWith("crowdsec/", CrowdSecAlertClient.UserAgent, StringComparison.Ordinal); + } + [Fact] public void BuildDeleteQuery_EscapesOrigin() { diff --git a/Projects/UOContent.Tests/Tests/Network/Bans/CrowdSecReporterTests.cs b/Projects/UOContent.Tests/Tests/Network/Bans/CrowdSecReporterTests.cs index a8daf3c44..6ad850f34 100644 --- a/Projects/UOContent.Tests/Tests/Network/Bans/CrowdSecReporterTests.cs +++ b/Projects/UOContent.Tests/Tests/Network/Bans/CrowdSecReporterTests.cs @@ -15,7 +15,9 @@ using System; using System.Collections.Generic; +using System.Globalization; using System.Net; +using System.Text.Json; using System.Threading; using System.Threading.Tasks; using Server.Network.Bans.CrowdSec; @@ -81,6 +83,60 @@ public class CrowdSecReporterTests Assert.Equal("modernuo/blocklist", decision.Scenario); } + /// + /// LAPI dereferences scenario_hash/scenario_version unconditionally when persisting an alert, so an + /// omitted field is a 500, not a validation error. Asserted on the serialized payload rather than the + /// DTO because that is what actually goes on the wire. + /// + [Fact] + public void BuildAlerts_SerializedPayload_CarriesRequiredScenarioFields() + { + var alerts = CrowdSecReporter.BuildAlerts( + [new(IPAddress.Parse("9.9.9.9"), TimeSpan.FromHours(1), "rate-limit", false)], + Settings(), + DateTime.UnixEpoch); + + var payload = JsonSerializer.SerializeToNode(alerts)!.AsArray()[0]!.AsObject(); + + Assert.True(payload.ContainsKey("scenario_hash")); + Assert.True(payload.ContainsKey("scenario_version")); + Assert.Equal(JsonValueKind.String, payload["scenario_hash"]!.GetValue().ValueKind); + Assert.Equal(JsonValueKind.String, payload["scenario_version"]!.GetValue().ValueKind); + Assert.Equal(1, payload["capacity"]!.GetValue()); + } + + /// + /// ':' is the time-separator specifier in a custom .NET format string, so a shard under fi-FI used to + /// emit "T00.00.00.000Z" — which Go's time.RFC3339 rejects, and LAPI answers 500 for. th-TH additionally + /// shifts the year via the Buddhist calendar. + /// + [Theory] + [InlineData("fi-FI")] + [InlineData("th-TH")] + [InlineData("ar-SA")] + public void FormatTimestamp_IsIso8601_RegardlessOfCulture(string culture) + { + var previous = CultureInfo.CurrentCulture; + try + { + CultureInfo.CurrentCulture = new CultureInfo(culture); + Assert.Equal("1970-01-01T00:00:00.000Z", CrowdSecReporter.FormatTimestamp(DateTime.UnixEpoch)); + } + finally + { + CultureInfo.CurrentCulture = previous; + } + } + + /// A non-UTC input must still be stamped as UTC — the trailing 'Z' is a literal, not a claim. + [Fact] + public void FormatTimestamp_ConvertsNonUtcInput() + { + var local = new DateTimeOffset(1970, 1, 1, 2, 0, 0, TimeSpan.FromHours(2)).LocalDateTime; + + Assert.Equal("1970-01-01T00:00:00.000Z", CrowdSecReporter.FormatTimestamp(local)); + } + [Fact] public void FormatDuration_UsesSeconds_FloorsAtOne() { diff --git a/Projects/UOContent/Misc/CrowdSec/CrowdSecAlert.cs b/Projects/UOContent/Misc/CrowdSec/CrowdSecAlert.cs index db0d8d80f..773cf50ec 100644 --- a/Projects/UOContent/Misc/CrowdSec/CrowdSecAlert.cs +++ b/Projects/UOContent/Misc/CrowdSec/CrowdSecAlert.cs @@ -21,11 +21,24 @@ namespace Server.Network.Bans.CrowdSec; public sealed class CrowdSecAlert { [JsonPropertyName("scenario")] public string Scenario { get; set; } + + /// + /// Required by LAPI. It dereferences the field unconditionally when persisting the alert, so + /// omitting it answers 500 rather than a validation error. Empty is the accepted value for a + /// watcher that isn't shipping a hub scenario. + /// + [JsonPropertyName("scenario_hash")] public string ScenarioHash { get; set; } = ""; + + /// Required alongside ; same 500-on-missing behavior. + [JsonPropertyName("scenario_version")] public string ScenarioVersion { get; set; } = "1.0"; + [JsonPropertyName("message")] public string Message { get; set; } [JsonPropertyName("events_count")] public int EventsCount { get; set; } = 1; [JsonPropertyName("start_at")] public string StartAt { get; set; } [JsonPropertyName("stop_at")] public string StopAt { get; set; } - [JsonPropertyName("capacity")] public int Capacity { get; set; } + + /// Bucket capacity. One decision per alert, so 1 — a leaky-bucket capacity of 0 is nonsense. + [JsonPropertyName("capacity")] public int Capacity { get; set; } = 1; [JsonPropertyName("leakspeed")] public string LeakSpeed { get; set; } = "0s"; [JsonPropertyName("simulated")] public bool Simulated { get; set; } [JsonPropertyName("events")] public object[] Events { get; set; } = []; diff --git a/Projects/UOContent/Misc/CrowdSec/CrowdSecAlertClient.cs b/Projects/UOContent/Misc/CrowdSec/CrowdSecAlertClient.cs index af853244a..b91d0ebe5 100644 --- a/Projects/UOContent/Misc/CrowdSec/CrowdSecAlertClient.cs +++ b/Projects/UOContent/Misc/CrowdSec/CrowdSecAlertClient.cs @@ -15,6 +15,7 @@ using System; using System.Collections.Generic; +using System.Globalization; using System.Net; using System.Net.Http; using System.Net.Http.Headers; @@ -40,6 +41,12 @@ public sealed class CrowdSecAlertClient : ICrowdSecAlertClient { private static readonly JsonSerializerOptions _jsonOptions = new() { PropertyNameCaseInsensitive = true }; + /// + /// The crowdsec/ prefix is load-bearing: LAPI's default watcher profile matches on it and + /// answers 401 for anything else, so this cannot be a plain product string. + /// + internal const string UserAgent = "crowdsec/ModernUO-watcher-1.0"; + private readonly HttpClient _http; private readonly string _machineId; private readonly string _password; @@ -51,7 +58,7 @@ public sealed class CrowdSecAlertClient : ICrowdSecAlertClient { var baseUri = new Uri(settings.LapiUrl, UriKind.Absolute); // fails loud on malformed url _http = new HttpClient { BaseAddress = baseUri, Timeout = TimeSpan.FromSeconds(30) }; - _http.DefaultRequestHeaders.Add("User-Agent", "ModernUO-watcher/1.0"); + _http.DefaultRequestHeaders.Add("User-Agent", UserAgent); _machineId = settings.MachineId; _password = settings.Password; } @@ -71,7 +78,15 @@ public sealed class CrowdSecAlertClient : ICrowdSecAlertClient var login = await response.Content.ReadFromJsonAsync(_jsonOptions, token) .ConfigureAwait(false); _token = login?.Token ?? throw new InvalidOperationException("CrowdSec login returned no token."); - _tokenExpiresUtc = DateTime.TryParse(login.Expire, out var exp) ? exp.ToUniversalTime() : DateTime.UtcNow.AddHours(1); + + // LAPI returns an RFC3339 expiry. Parse it invariantly for the same reason we format invariantly: + // the current culture must not decide whether a machine-readable timestamp is understood. + _tokenExpiresUtc = DateTime.TryParse( + login.Expire, + CultureInfo.InvariantCulture, + DateTimeStyles.AdjustToUniversal, + out var exp + ) ? exp : DateTime.UtcNow.AddHours(1); } private void Authorize(HttpRequestMessage message) => diff --git a/Projects/UOContent/Misc/CrowdSec/CrowdSecReporter.cs b/Projects/UOContent/Misc/CrowdSec/CrowdSecReporter.cs index 306d4ab53..e38aa17a6 100644 --- a/Projects/UOContent/Misc/CrowdSec/CrowdSecReporter.cs +++ b/Projects/UOContent/Misc/CrowdSec/CrowdSecReporter.cs @@ -15,6 +15,7 @@ using System; using System.Collections.Generic; +using System.Globalization; using System.Net; using System.Threading; using System.Threading.Channels; @@ -357,7 +358,7 @@ public sealed class CrowdSecReporter : IBanReporter byIp[item.Ip.ToString()] = item; } - var timestamp = nowUtc.ToString("yyyy-MM-ddTHH:mm:ss.fffZ"); + var timestamp = FormatTimestamp(nowUtc); var alerts = new List(byIp.Count); foreach (var (value, item) in byIp) @@ -390,6 +391,17 @@ public sealed class CrowdSecReporter : IBanReporter return alerts; } + /// + /// ISO8601/RFC3339 UTC timestamp for start_at/stop_at. LAPI parses these with Go's + /// time.RFC3339 and answers 500 when the parse fails, so the format must be culture-independent: + /// ':' is the *time separator* specifier in a custom .NET format string, and a shard running under a + /// culture like fi-FI would otherwise emit "T12.34.56.789Z". InvariantCulture also pins the Gregorian + /// calendar, which non-Gregorian cultures (th-TH, ar-SA) would otherwise shift the year for. + /// + internal static string FormatTimestamp(DateTime time) => + (time.Kind == DateTimeKind.Utc ? time : time.ToUniversalTime()) + .ToString("yyyy-MM-ddTHH:mm:ss.fffZ", CultureInfo.InvariantCulture); + /// CrowdSec accepts Go durations; whole seconds are unambiguous and sufficient. internal static string FormatDuration(TimeSpan ttl) {