From b12fd3e1b763e4f0ab02ffcbde6f867f05ceb110 Mon Sep 17 00:00:00 2001 From: Jack Date: Tue, 4 Aug 2026 14:24:06 -0500 Subject: [PATCH] Feature set 130. Enforce the premium rate limit against an allowlist. The X-Api-Key header was taken at face value: any non-empty value bought 600 req/min instead of 60, because the value was never compared against anything. Worse, the limiter partitioned on the raw header, so varying it minted unlimited fresh windows -- the limit was not 10x too high, it was effectively absent for anyone who noticed. Keys now come from an allowlist (ApiKeys__Keys__0, ...). An unrecognized key falls back to the anonymous limit rather than 401, keeping the documented "no API key required" contract intact. Also removed the two named rate limit policies. No endpoint ever attached them with RequireRateLimiting, so they enforced nothing while appearing to, and the premium one carried the same flaw waiting to be wired up. Only GlobalLimiter was ever live. The request log had the same defect: IsPremium counted anyone who sent the header, valid or not, inflating the premium share in /api/stats. It now records recognized keys, captured while the request is still live rather than resolved from a disposed scope inside the fire-and-forget write. Separately, the dashboard's monster search now matches description as well as name, mirroring /api/monsters/search. Auditing the catalogue turned up ~3,200 enemy-ability pages and ~130 enemy-type pages that the wiki scrape filed as monsters, and what marks them -- "#REDIRECT ... enemy abilities", "may refer to", "is a genus of" -- lives only in the description. Searching names alone left them unreachable from the dashboard that curates them. Co-Authored-By: Claude Opus 5 --- README.md | 8 +- .../Dashboard/Browse/MonstersEndpoint.cs | 10 +- .../Middleware/RequestLoggingMiddleware.cs | 18 +++- .../RateLimiting/ApiKeyValidator.cs | 35 ++++++ .../RateLimiting/ApiRateLimiterPolicy.cs | 59 ++++------ .../RateLimiting/PremiumKeyOptions.cs | 22 ++++ src/MoogleAPI.Web/Program.cs | 6 +- tests/MoogleAPI.Tests/ApiKeyTests.cs | 102 ++++++++++++++++++ 8 files changed, 216 insertions(+), 44 deletions(-) create mode 100644 src/MoogleAPI.Web/Infrastructure/RateLimiting/ApiKeyValidator.cs create mode 100644 src/MoogleAPI.Web/Infrastructure/RateLimiting/PremiumKeyOptions.cs create mode 100644 tests/MoogleAPI.Tests/ApiKeyTests.cs diff --git a/README.md b/README.md index 1ab5509..55dd3bc 100644 --- a/README.md +++ b/README.md @@ -35,7 +35,7 @@ GET https://moogleapi.com/api/monsters?gameId=7 GET https://moogleapi.com/api/games ``` -No API key required. Pass `X-Api-Key: your-key` to get 10× the rate limit. +No API key required. Pass an issued `X-Api-Key: your-key` to get 10× the rate limit. --- @@ -153,10 +153,14 @@ Stages can be run individually with `--only=`: `games`, `characters`, `playable` | Tier | Limit | How | |------|-------|-----| | Anonymous | 60 req / min | Per IP, no setup needed | -| Premium | 600 req / min | Pass `X-Api-Key: your-key` header | +| Premium | 600 req / min | Pass an issued `X-Api-Key: your-key` header | Responses over the limit return `429 Too Many Requests`. +Premium keys have to be issued — an unrecognized key isn't rejected, it just falls back to the +anonymous limit, so the API stays usable if you send a stale one. Self-hosting? Set the +allowlist with `ApiKeys__Keys__0`, `ApiKeys__Keys__1`, … With none set, everything is anonymous. + --- ## 📜 Disclaimer diff --git a/src/MoogleAPI.Web/Features/Dashboard/Browse/MonstersEndpoint.cs b/src/MoogleAPI.Web/Features/Dashboard/Browse/MonstersEndpoint.cs index bd58820..cc7b1da 100644 --- a/src/MoogleAPI.Web/Features/Dashboard/Browse/MonstersEndpoint.cs +++ b/src/MoogleAPI.Web/Features/Dashboard/Browse/MonstersEndpoint.cs @@ -22,8 +22,16 @@ public override async Task HandleAsync(BrowseRequest req, CancellationToken ct) if (req.GameId.HasValue) query = query.Where(m => m.GameId == req.GameId.Value); + // Name *or* description, matching what /api/monsters/search already does. Curation + // needs the description: what marks a row as not-a-monster — "#REDIRECT ... enemy + // abilities", "may refer to", "is a genus of" — is only ever in the prose the scrape + // kept, never in the name. Searching names alone left those rows unreachable here. if (!string.IsNullOrWhiteSpace(req.Search)) - query = query.Where(m => EF.Functions.ILike(m.Name, $"%{req.Search.Trim()}%")); + { + var search = req.Search.Trim(); + query = query.Where(m => EF.Functions.ILike(m.Name, $"%{search}%") || + (m.Description != null && EF.Functions.ILike(m.Description, $"%{search}%"))); + } var total = await query.CountAsync(ct); var items = await query diff --git a/src/MoogleAPI.Web/Infrastructure/Middleware/RequestLoggingMiddleware.cs b/src/MoogleAPI.Web/Infrastructure/Middleware/RequestLoggingMiddleware.cs index ce3207e..562c8f1 100644 --- a/src/MoogleAPI.Web/Infrastructure/Middleware/RequestLoggingMiddleware.cs +++ b/src/MoogleAPI.Web/Infrastructure/Middleware/RequestLoggingMiddleware.cs @@ -2,10 +2,14 @@ using System.Text; using MoogleAPI.Web.Infrastructure.Data; using MoogleAPI.Web.Infrastructure.Models; +using MoogleAPI.Web.Infrastructure.RateLimiting; namespace MoogleAPI.Web.Infrastructure.Middleware; -public class RequestLoggingMiddleware(RequestDelegate next, IServiceScopeFactory scopeFactory) +public class RequestLoggingMiddleware( + RequestDelegate next, + IServiceScopeFactory scopeFactory, + ApiKeyValidator apiKeys) { public async Task InvokeAsync(HttpContext context) { @@ -32,11 +36,15 @@ public async Task InvokeAsync(HttpContext context) await next(context); var durationMs = (int)(Environment.TickCount64 - start); + // Read off the request before handing to the background write — by the time that runs + // the response is done and the context is no longer ours to read. + var isPremium = apiKeys.ResolveKey(context.Request) is not null; + // Fire-and-forget: never slow down the response for logging - _ = WriteLogAsync(context, path, durationMs); + _ = WriteLogAsync(context, path, durationMs, isPremium); } - private async Task WriteLogAsync(HttpContext context, string path, int durationMs) + private async Task WriteLogAsync(HttpContext context, string path, int durationMs, bool isPremium) { try { @@ -52,7 +60,9 @@ private async Task WriteLogAsync(HttpContext context, string path, int durationM DurationMs = durationMs, ResourceType = ExtractResourceType(path), SearchTerm = context.Request.Query["query"].FirstOrDefault(), - IsPremium = context.Request.Headers.ContainsKey("X-Api-Key"), + // Recognized key, not merely a present header — otherwise the premium share in + // /api/stats counts anyone who sent the header, valid or not. + IsPremium = isPremium, IpHash = HashIp(context.Connection.RemoteIpAddress?.ToString()), }); diff --git a/src/MoogleAPI.Web/Infrastructure/RateLimiting/ApiKeyValidator.cs b/src/MoogleAPI.Web/Infrastructure/RateLimiting/ApiKeyValidator.cs new file mode 100644 index 0000000..d65e560 --- /dev/null +++ b/src/MoogleAPI.Web/Infrastructure/RateLimiting/ApiKeyValidator.cs @@ -0,0 +1,35 @@ +using Microsoft.Extensions.Options; + +namespace MoogleAPI.Web.Infrastructure.RateLimiting; + +/// +/// Decides whether an X-Api-Key header actually entitles a caller to the premium rate +/// limit. The header used to be taken at face value — any non-empty value bought 10× the +/// anonymous limit, so the limit was opt-out rather than enforced. +/// +public class ApiKeyValidator +{ + public const string HeaderName = "X-Api-Key"; + + private readonly HashSet _keys; + + public ApiKeyValidator(IOptions options) => + _keys = options.Value.Keys + .Where(k => !string.IsNullOrWhiteSpace(k)) + .Select(k => k.Trim()) + .ToHashSet(StringComparer.Ordinal); + + public bool IsValid(string? apiKey) => + !string.IsNullOrWhiteSpace(apiKey) && _keys.Contains(apiKey.Trim()); + + /// + /// The recognized key on this request, or null for anonymous. An unrecognized key + /// returns null rather than throwing: the API is public and documented as needing no + /// key at all, so a bad one degrades to the anonymous limit instead of failing the request. + /// + public string? ResolveKey(HttpRequest request) + { + var apiKey = request.Headers[HeaderName].ToString(); + return IsValid(apiKey) ? apiKey.Trim() : null; + } +} diff --git a/src/MoogleAPI.Web/Infrastructure/RateLimiting/ApiRateLimiterPolicy.cs b/src/MoogleAPI.Web/Infrastructure/RateLimiting/ApiRateLimiterPolicy.cs index eae489a..56e42dc 100644 --- a/src/MoogleAPI.Web/Infrastructure/RateLimiting/ApiRateLimiterPolicy.cs +++ b/src/MoogleAPI.Web/Infrastructure/RateLimiting/ApiRateLimiterPolicy.cs @@ -1,12 +1,11 @@ using System.Threading.RateLimiting; -using Microsoft.AspNetCore.RateLimiting; namespace MoogleAPI.Web.Infrastructure.RateLimiting; public static class ApiRateLimiterPolicy { - public const string Anonymous = "anonymous"; - public const string Premium = "premium"; + public const int AnonymousPermitLimit = 60; + public const int PremiumPermitLimit = 600; public static IServiceCollection AddApiRateLimiting(this IServiceCollection services) { @@ -14,46 +13,34 @@ public static IServiceCollection AddApiRateLimiting(this IServiceCollection serv { options.RejectionStatusCode = StatusCodes.Status429TooManyRequests; - options.AddPolicy(Anonymous, context => - RateLimitPartition.GetFixedWindowLimiter( - partitionKey: context.Connection.RemoteIpAddress?.ToString() ?? "unknown", - factory: _ => new FixedWindowRateLimiterOptions - { - PermitLimit = 60, - Window = TimeSpan.FromMinutes(1), - QueueProcessingOrder = QueueProcessingOrder.OldestFirst, - QueueLimit = 0 - })); - - // Premium users identified by X-Api-Key header get 10x the limit - options.AddPolicy(Premium, context => + // One global limiter rather than named policies. There were two named policies here + // that no endpoint ever attached with RequireRateLimiting, so they enforced nothing + // while appearing to — and the premium one carried the same unchecked-key flaw as + // this limiter did. Anything that needs a per-endpoint limit should be added back + // deliberately, and wired up. + options.GlobalLimiter = PartitionedRateLimiter.Create(context => { - var apiKey = context.Request.Headers["X-Api-Key"].ToString(); - return RateLimitPartition.GetFixedWindowLimiter( - partitionKey: string.IsNullOrEmpty(apiKey) ? $"ip:{context.Connection.RemoteIpAddress}" : $"key:{apiKey}", - factory: _ => new FixedWindowRateLimiterOptions + // The key is validated against the configured allowlist, so the partition is + // only ever keyed on a credential we issued. Partitioning on the raw header + // would otherwise let a caller mint unlimited fresh windows just by varying it. + var validator = context.RequestServices.GetRequiredService(); + var apiKey = validator.ResolveKey(context.Request); + + var partitionKey = apiKey is null + ? $"ip:{context.Connection.RemoteIpAddress}" + : $"key:{apiKey}"; + + var permitLimit = apiKey is null ? AnonymousPermitLimit : PremiumPermitLimit; + + return RateLimitPartition.GetFixedWindowLimiter(partitionKey, _ => + new FixedWindowRateLimiterOptions { - PermitLimit = string.IsNullOrEmpty(apiKey) ? 60 : 600, + PermitLimit = permitLimit, Window = TimeSpan.FromMinutes(1), QueueProcessingOrder = QueueProcessingOrder.OldestFirst, QueueLimit = 0 }); }); - - options.GlobalLimiter = PartitionedRateLimiter.Create(context => - { - var apiKey = context.Request.Headers["X-Api-Key"].ToString(); - var limit = string.IsNullOrEmpty(apiKey) ? 60 : 600; - var key = string.IsNullOrEmpty(apiKey) - ? $"ip:{context.Connection.RemoteIpAddress}" - : $"key:{apiKey}"; - - return RateLimitPartition.GetFixedWindowLimiter(key, _ => new FixedWindowRateLimiterOptions - { - PermitLimit = limit, - Window = TimeSpan.FromMinutes(1) - }); - }); }); return services; diff --git a/src/MoogleAPI.Web/Infrastructure/RateLimiting/PremiumKeyOptions.cs b/src/MoogleAPI.Web/Infrastructure/RateLimiting/PremiumKeyOptions.cs new file mode 100644 index 0000000..b57d4ad --- /dev/null +++ b/src/MoogleAPI.Web/Infrastructure/RateLimiting/PremiumKeyOptions.cs @@ -0,0 +1,22 @@ +namespace MoogleAPI.Web.Infrastructure.RateLimiting; + +/// +/// Named for what these keys buy rather than what they are: the API itself needs no key, and +/// ApiKeyOptions would collide with Scalar's type of that name in Program.cs. +/// +public class PremiumKeyOptions +{ + public const string SectionName = "ApiKeys"; + + /// + /// The keys entitled to the premium rate limit. Anything not on this list is treated as + /// anonymous, so an empty list simply means nobody has premium — which is the correct + /// default, and the reason this isn't validated at startup the way + /// is. + /// + /// + /// Supply via ApiKeys__Keys__0, ApiKeys__Keys__1, … (env vars) or user-secrets + /// in development. These are credentials: keep them out of appsettings.json. + /// + public List Keys { get; set; } = []; +} diff --git a/src/MoogleAPI.Web/Program.cs b/src/MoogleAPI.Web/Program.cs index a7f0e25..7e33883 100644 --- a/src/MoogleAPI.Web/Program.cs +++ b/src/MoogleAPI.Web/Program.cs @@ -44,7 +44,11 @@ }; }); -// Partitioned rate limiting: 60 req/min anonymous, 600 req/min with X-Api-Key +// Partitioned rate limiting: 60 req/min anonymous, 600 req/min with a recognized X-Api-Key. +// The allowlist is what makes the premium tier mean anything — without it the header was +// self-service. No startup validation: an empty list legitimately means nobody has premium. +builder.Services.Configure(builder.Configuration.GetSection(PremiumKeyOptions.SectionName)); +builder.Services.AddSingleton(); builder.Services.AddApiRateLimiting(); // Daily puzzle seeding. Validated at startup rather than on first request: an empty secret diff --git a/tests/MoogleAPI.Tests/ApiKeyTests.cs b/tests/MoogleAPI.Tests/ApiKeyTests.cs new file mode 100644 index 0000000..ec3e290 --- /dev/null +++ b/tests/MoogleAPI.Tests/ApiKeyTests.cs @@ -0,0 +1,102 @@ +using Microsoft.AspNetCore.Http; +using Microsoft.Extensions.Options; +using MoogleAPI.Web.Infrastructure.RateLimiting; + +namespace MoogleAPI.Tests; + +public class ApiKeyTests +{ + private static ApiKeyValidator Validator(params string[] keys) => + new(Options.Create(new PremiumKeyOptions { Keys = [.. keys] })); + + private static HttpRequest RequestWith(string? apiKey) + { + var context = new DefaultHttpContext(); + if (apiKey is not null) + context.Request.Headers[ApiKeyValidator.HeaderName] = apiKey; + return context.Request; + } + + [Fact] + public void RecognizedKeyIsValid() + { + Assert.True(Validator("sponsor-key").IsValid("sponsor-key")); + } + + // The regression this whole class exists for: any non-empty header used to buy the + // premium limit, because the value was never compared against anything. + [Theory] + [InlineData("x")] + [InlineData("not-a-real-key")] + [InlineData("SPONSOR-KEY")] // keys are case-sensitive + [InlineData("sponsor-key-2")] // no prefix matching + public void UnrecognizedKeyIsNotValid(string apiKey) + { + Assert.False(Validator("sponsor-key").IsValid(apiKey)); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + public void MissingOrBlankKeyIsNotValid(string? apiKey) + { + Assert.False(Validator("sponsor-key").IsValid(apiKey)); + } + + [Fact] + public void NoConfiguredKeysMeansNobodyIsPremium() + { + var validator = Validator(); + + Assert.False(validator.IsValid("sponsor-key")); + Assert.False(validator.IsValid("anything")); + } + + [Fact] + public void SurroundingWhitespaceIsToleratedOnBothSides() + { + // Config values and curl invocations both pick up stray whitespace; a key that is + // otherwise correct shouldn't be rejected for it. + Assert.True(Validator(" sponsor-key ").IsValid("sponsor-key")); + Assert.True(Validator("sponsor-key").IsValid(" sponsor-key ")); + } + + [Fact] + public void BlankConfiguredKeysAreDiscarded() + { + // An unset env var binds as an empty string. If that were kept as a key, sending an + // empty header would match it and premium would be self-service again. + var validator = Validator("", " ", "sponsor-key"); + + Assert.False(validator.IsValid("")); + Assert.False(validator.IsValid(" ")); + Assert.True(validator.IsValid("sponsor-key")); + } + + [Fact] + public void ResolveKeyReturnsTheKeyForARecognizedHeader() + { + Assert.Equal("sponsor-key", Validator("sponsor-key").ResolveKey(RequestWith("sponsor-key"))); + } + + [Fact] + public void ResolveKeyFallsBackToAnonymousRatherThanFailing() + { + var validator = Validator("sponsor-key"); + + Assert.Null(validator.ResolveKey(RequestWith("bogus"))); + Assert.Null(validator.ResolveKey(RequestWith(null))); + } + + [Fact] + public void ResolveKeyNormalizesSoOneKeyCannotHoldSeveralRateLimitWindows() + { + // The resolved value becomes the limiter's partition key. If padding survived here, + // " key" and "key " would be separate partitions and the limit would multiply. + var validator = Validator("sponsor-key"); + + Assert.Equal("sponsor-key", validator.ResolveKey(RequestWith(" sponsor-key"))); + Assert.Equal("sponsor-key", validator.ResolveKey(RequestWith("sponsor-key "))); + } +}