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.
This commit is contained in:
@@ -7,6 +7,9 @@ namespace Marathon.Domain.ValueObjects;
|
||||
/// </summary>
|
||||
public sealed record EventId
|
||||
{
|
||||
/// <summary>Sane upper bound — real bookmaker ids are short (≈8 chars).</summary>
|
||||
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;
|
||||
}
|
||||
|
||||
|
||||
@@ -52,6 +52,9 @@ public sealed class AddBetForm
|
||||
/// <summary>Upper sanity cap on a single wager.</summary>
|
||||
public const decimal MaxStake = 10_000_000m;
|
||||
|
||||
/// <summary>Upper bound on the free-text note so a paste can't bloat the row unbounded.</summary>
|
||||
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.
|
||||
|
||||
@@ -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<ArgumentException>()
|
||||
.WithParameterName("value");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Constructor_ThrowsArgumentException_WhenValueExceedsMaxLength()
|
||||
{
|
||||
var act = () => new EventId(new string('1', 129));
|
||||
act.Should().Throw<ArgumentException>()
|
||||
.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()
|
||||
{
|
||||
|
||||
@@ -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");
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user