From 0683e348ba060c9bd45af23f3f1a1518b04b135c Mon Sep 17 00:00:00 2001 From: "alexei.dolgolyov" Date: Fri, 29 May 2026 14:14:12 +0300 Subject: [PATCH] fix(security): bound bet-notes length + harden EventId against path/control chars Two defense-in-depth findings from the I-series security review (both safe today, neither currently exploitable): - AddBetForm.Notes was unbounded free-text into SQLite; add a 2000-char sanity cap in IsValid (covers both the add and edit paths), alongside the existing stake/rate caps. - EventId only rejected empty/whitespace; now also reject path separators, '..' traversal, control/newline chars and over-length input so no current-or-future consumer that builds a path/filename/log line from an id can be tricked. The charset stays open for forward-compat with non-numeric bookmaker ids. --- src/Marathon.Domain/ValueObjects/EventId.cs | 22 ++++++++ .../Services/BetJournalViewModels.cs | 4 ++ .../ValueObjects/EventIdTests.cs | 35 ++++++++++++ .../Services/AddBetFormTests.cs | 55 +++++++++++++++++++ 4 files changed, 116 insertions(+) create mode 100644 tests/Marathon.UI.Tests/Services/AddBetFormTests.cs diff --git a/src/Marathon.Domain/ValueObjects/EventId.cs b/src/Marathon.Domain/ValueObjects/EventId.cs index af5ac99..b05c492 100644 --- a/src/Marathon.Domain/ValueObjects/EventId.cs +++ b/src/Marathon.Domain/ValueObjects/EventId.cs @@ -7,6 +7,9 @@ namespace Marathon.Domain.ValueObjects; /// public sealed record EventId { + /// Sane upper bound — real bookmaker ids are short (≈8 chars). + private const int MaxLength = 128; + public string Value { get; } public EventId(string value) @@ -14,6 +17,25 @@ public sealed record EventId if (string.IsNullOrWhiteSpace(value)) throw new ArgumentException("EventId must not be empty or whitespace.", nameof(value)); + // Defense-in-depth. The id is deliberately string-typed for forward-compat with other + // bookmakers' id formats, so we do NOT pin a charset whitelist. We only reject the few + // characters that would be dangerous if any current or future consumer ever builds a + // file path, filename, or log line from the id — path separators, parent-dir traversal + // ("..") and control/newline characters — plus a length cap against pathological input. + if (value.Length > MaxLength) + throw new ArgumentException($"EventId must be at most {MaxLength} characters.", nameof(value)); + + if (value.Contains('/') || value.Contains('\\') || value.Contains("..")) + throw new ArgumentException( + "EventId must not contain path separators or '..'.", nameof(value)); + + foreach (var ch in value) + { + if (char.IsControl(ch)) + throw new ArgumentException( + "EventId must not contain control characters.", nameof(value)); + } + Value = value; } diff --git a/src/Marathon.UI/Services/BetJournalViewModels.cs b/src/Marathon.UI/Services/BetJournalViewModels.cs index 1e623f3..6e83422 100644 --- a/src/Marathon.UI/Services/BetJournalViewModels.cs +++ b/src/Marathon.UI/Services/BetJournalViewModels.cs @@ -52,6 +52,9 @@ public sealed class AddBetForm /// Upper sanity cap on a single wager. public const decimal MaxStake = 10_000_000m; + /// Upper bound on the free-text note so a paste can't bloat the row unbounded. + public const int MaxNotesLength = 2000; + public bool IsValid(out string? error) { if (string.IsNullOrWhiteSpace(EventId)) { error = "EventId is required."; return false; } @@ -59,6 +62,7 @@ public sealed class AddBetForm if (Stake > MaxStake) { error = $"Stake must be at most {MaxStake:N0}."; return false; } if (Rate < 1.01m) { error = "Rate must be at least 1.01."; return false; } if (Rate > MaxRate) { error = $"Rate must be at most {MaxRate:N0}."; return false; } + if (Notes is { Length: > MaxNotesLength }) { error = $"Notes must be at most {MaxNotesLength:N0} characters."; return false; } // Mirror Bet invariants — surface a friendly message instead of throwing // ArgumentException deep in the use case. diff --git a/tests/Marathon.Domain.Tests/ValueObjects/EventIdTests.cs b/tests/Marathon.Domain.Tests/ValueObjects/EventIdTests.cs index 2bb364c..c61c412 100644 --- a/tests/Marathon.Domain.Tests/ValueObjects/EventIdTests.cs +++ b/tests/Marathon.Domain.Tests/ValueObjects/EventIdTests.cs @@ -31,6 +31,41 @@ public sealed class EventIdTests .WithParameterName("value"); } + [Theory] + [InlineData("a/b")] // forward slash (path separator) + [InlineData("a\\b")] // back slash (path separator) + [InlineData("..")] // parent-dir traversal + [InlineData("../etc/passwd")] + [InlineData("evt\n1")] // control char (newline) + [InlineData("evt\r1")] // control char (CR) + [InlineData("evt\0id")] // control char (NUL) + public void Constructor_ThrowsArgumentException_WhenValueHasDangerousCharacters(string value) + { + var act = () => new EventId(value); + act.Should().Throw() + .WithParameterName("value"); + } + + [Fact] + public void Constructor_ThrowsArgumentException_WhenValueExceedsMaxLength() + { + var act = () => new EventId(new string('1', 129)); + act.Should().Throw() + .WithParameterName("value"); + } + + [Theory] + [InlineData("26456117")] // numeric (marathonbet.by) + [InlineData("evt-1")] // hyphenated + [InlineData("event_1")] // underscore + [InlineData("evt.1")] // single dot is fine — only ".." is rejected + [InlineData("AB12cd34")] // mixed-case alphanumeric (forward-compat) + public void Constructor_Accepts_ValidAndForwardCompatIds(string value) + { + var id = new EventId(value); + id.Value.Should().Be(value); + } + [Fact] public void ToString_ReturnsValue() { diff --git a/tests/Marathon.UI.Tests/Services/AddBetFormTests.cs b/tests/Marathon.UI.Tests/Services/AddBetFormTests.cs new file mode 100644 index 0000000..126cc20 --- /dev/null +++ b/tests/Marathon.UI.Tests/Services/AddBetFormTests.cs @@ -0,0 +1,55 @@ +using FluentAssertions; +using Marathon.Domain.Enums; +using Marathon.UI.Services; + +namespace Marathon.UI.Tests.Services; + +public sealed class AddBetFormTests +{ + private static AddBetForm Valid() => new() + { + EventId = "26456117", + Type = BetType.Win, + Side = Side.Side1, + Rate = 1.90m, + Stake = 100m, + Notes = "ok", + }; + + [Fact] + public void IsValid_ReturnsTrue_ForAWellFormedForm() + { + Valid().IsValid(out var error).Should().BeTrue(); + error.Should().BeNull(); + } + + [Fact] + public void IsValid_AllowsNullNotes() + { + var form = Valid(); + form.Notes = null; + + form.IsValid(out var error).Should().BeTrue(); + error.Should().BeNull(); + } + + [Fact] + public void IsValid_AllowsNotesAtTheMaxLength() + { + var form = Valid(); + form.Notes = new string('x', AddBetForm.MaxNotesLength); + + form.IsValid(out var error).Should().BeTrue(); + error.Should().BeNull(); + } + + [Fact] + public void IsValid_RejectsNotesOverTheMaxLength() + { + var form = Valid(); + form.Notes = new string('x', AddBetForm.MaxNotesLength + 1); + + form.IsValid(out var error).Should().BeFalse(); + error.Should().Contain("Notes"); + } +}