Skip to content

Commit c36a2d8

Browse files
committed
Address CodeRabbit review feedback
- Guard the window conversion against TimeSpan overflow. FromHours(int.MaxValue) throws, and PapercutServiceModule only handles a failed ExecutionResult, so "1/2147483647h" in config took down service startup instead of falling back to no limit. Now reported as a parse failure, with tests. - Read the clock inside the rate limiter's lock. A timestamp captured outside it can predate a _windowStart another thread already advanced, giving a negative elapsed and an inflated RetryAfter. - Move the rate limit env vars out of the IP allowlist section in DOCKERHUB.md; they had orphaned the indented allowlist examples below them. - Tag the SMTP response fence in docs/service.md as text.
1 parent a433576 commit c36a2d8

5 files changed

Lines changed: 53 additions & 10 deletions

File tree

DOCKERHUB.md

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
![Papercut Logo](https://raw.githubusercontent.com/ChangemakerStudios/Papercut/develop/graphics/PapercutLogo.png)<br>
1+
![Papercut Logo](https://raw.githubusercontent.com/ChangemakerStudios/Papercut/develop/graphics/PapercutLogo.png)<br>
22
*The Simple Desktop Email Helper*
33

44
## The problem
@@ -90,13 +90,13 @@ docker run -d \
9090
- `SmtpServer__MessagePath` - Path where emails are stored (default: /app/Incoming)
9191
- `SmtpServer__LoggingPath` - Path for log files (default: /app/logs)
9292
- `SmtpServer__AllowedIps` - IP allowlist for SMTP connections (default: "*" = all IPs allowed)
93+
- `SmtpServer__RateLimit` - Message reception rate limit as `<count>/<window>`, e.g. `500/1h` (default: "*" = no limit)
94+
- `SmtpServer__RateLimitReplyCode` - SMTP reply code returned once the rate limit is hit (default: 451)
9395
- `Urls` - HTTP server URLs (default: http://0.0.0.0:8080)
9496
- `HttpPathPrefix` - Serve the web UI and API under a path prefix, e.g. `/webmail` (default: empty = serve at root)
9597

9698
**Security - SMTP IP Allowlist Configuration:**
9799
- `SmtpServer__AllowedIps` - Comma-separated list of allowed IP addresses or CIDR ranges for SMTP connections
98-
- `SmtpServer__RateLimit` - Message reception rate limit as `<count>/<window>`, e.g. `500/1h` (default: "*" = no limit)
99-
- `SmtpServer__RateLimitReplyCode` - SMTP reply code returned once the rate limit is hit (default: 451)
100100
- `*` - Allow all IPs (default, backward compatible)
101101
- `192.168.1.0/24` - Allow single CIDR range
102102
- `192.168.1.0/24,10.0.0.0/8` - Allow multiple CIDR ranges

docs/service.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
# Service & Web UI
1+
# Service & Web UI
22

33
The **Papercut SMTP Service** is an optional background component that receives email even when the desktop app isn't running — as a Windows Service or a [Docker container](docker.md) — and includes a browser-based UI for viewing messages.
44

@@ -82,7 +82,7 @@ for example `500/1h`, `5/10m`, or `100/30s`. Use `*` (the default) for no limit.
8282

8383
Once the limit is reached, Papercut rejects at `MAIL FROM` until the window resets:
8484

85-
```
85+
```text
8686
451 4.7.1 Message rate limit exceeded (500 per hour)
8787
```
8888

src/Papercut.Infrastructure.Smtp/RateLimiting/SmtpRateLimit.cs

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -140,14 +140,31 @@ private static ExecutionResult<TimeSpan> ParseWindow(string window)
140140

141141
return char.ToLowerInvariant(unit) switch
142142
{
143-
's' => ExecutionResult.Success(TimeSpan.FromSeconds(amount)),
144-
'm' => ExecutionResult.Success(TimeSpan.FromMinutes(amount)),
145-
'h' => ExecutionResult.Success(TimeSpan.FromHours(amount)),
143+
's' => FromUnits(amount, TimeSpan.TicksPerSecond, window),
144+
'm' => FromUnits(amount, TimeSpan.TicksPerMinute, window),
145+
'h' => FromUnits(amount, TimeSpan.TicksPerHour, window),
146146
_ => ExecutionResult.Failure<TimeSpan>(
147147
$"Invalid rate limit window unit '{unit}'. Expected s (seconds), m (minutes), or h (hours).")
148148
};
149149
}
150150

151+
/// <summary>
152+
/// Converts a whole number of units into a TimeSpan, reporting a failure rather than
153+
/// throwing when the result would not fit. TimeSpan.FromHours(int.MaxValue) overflows,
154+
/// and an exception escaping here would take down service startup instead of falling
155+
/// back to no limit.
156+
/// </summary>
157+
private static ExecutionResult<TimeSpan> FromUnits(int amount, long ticksPerUnit, string window)
158+
{
159+
if (amount > TimeSpan.MaxValue.Ticks / ticksPerUnit)
160+
{
161+
return ExecutionResult.Failure<TimeSpan>(
162+
$"Rate limit window '{window}' is too large.");
163+
}
164+
165+
return ExecutionResult.Success(new TimeSpan(amount * ticksPerUnit));
166+
}
167+
151168
private static string DescribeWindow(TimeSpan window)
152169
{
153170
if (window.TotalHours >= 1 && window.TotalHours % 1 == 0)

src/Papercut.Infrastructure.Smtp/RateLimiting/SmtpRateLimiter.cs

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,10 +54,12 @@ public RateLimitDecision TryAcquire()
5454
return new RateLimitDecision(true, 0, TimeSpan.Zero);
5555
}
5656

57-
var now = timeProvider.GetUtcNow();
58-
5957
lock (this._sync)
6058
{
59+
// Read the clock inside the lock: a timestamp captured outside it can be
60+
// older than a _windowStart another thread has already advanced, which
61+
// yields a negative elapsed and an inflated RetryAfter.
62+
var now = timeProvider.GetUtcNow();
6163
var elapsed = now - this._windowStart;
6264

6365
if (this._count == 0 || elapsed >= this.Limit.Window)

test/Papercut.Infrastructure.Smtp.Tests/RateLimiting/SmtpRateLimitTests.cs

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,30 @@ public void Create_WithInvalidSpec_ReturnsFailureWithError(string spec)
119119
result.Errors.Should().NotBeEmpty();
120120
}
121121

122+
[TestCase("1/2147483647h")]
123+
[TestCase("1/999999999h")]
124+
public void Create_WithWindowTooLargeForTimeSpan_ReturnsFailureRatherThanThrowing(string spec)
125+
{
126+
// Act -- TimeSpan.FromHours(int.MaxValue) overflows; an exception escaping here
127+
// would take down service startup instead of falling back to no limit
128+
var result = SmtpRateLimit.Create(spec);
129+
130+
// Assert
131+
result.IsFailed.Should().BeTrue();
132+
result.Errors.Should().NotBeEmpty();
133+
}
134+
135+
[Test]
136+
public void Create_WithLargestWindowThatStillFits_ReturnsSuccess()
137+
{
138+
// Act -- int.MaxValue minutes is within TimeSpan's range and must keep working
139+
var result = SmtpRateLimit.Create("1/2147483647m");
140+
141+
// Assert
142+
result.IsSuccess.Should().BeTrue();
143+
result.Value.Window.Should().Be(TimeSpan.FromMinutes(2147483647L));
144+
}
145+
122146
#endregion
123147

124148
#region Reply Code Tests

0 commit comments

Comments
 (0)