From 5c26a53ff6f6dd7be3855522a33eeef56ead280a Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 8 Sep 2026 15:18:08 +0000 Subject: [PATCH] A CORS preflight is answered wherever a CORS header is written MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `OPTIONS` matched no route on this server. It fell through to whatever the host had, and a host with a deny-everything fallback policy answered 401 - on the discovery documents, on the JWKS, and on the RFC 9728 metadata the resource server serves. Measured against a running deployment, 2026-09-08. WHY IT IS WORTH A RELEASE RATHER THAN A NOTE, given nothing was broken today: a browser preflights only once a request stops being simple, so the client that finds this is the first one to add a request header of its own. What that client is shown is not a 401. It is the browser's generic "no Access-Control-Allow-Origin header is present", pointing at CORS configuration that is correct - and the cost is the hour spent looking there. Authenticating a preflight cannot be right in any case: the browser sends it with no credentials by specification, so there is nothing in it to authenticate. The rule is one sentence: every route that writes Access-Control-Allow-Origin answers OPTIONS, and no other route does. That is the two discovery documents, the JWKS, the three /.well-known catch-alls, /token, and both forms of the protected-resource metadata. The boundary is held from both sides by tests, because the permissive half is the half somebody would later read as a licence: /authorize MUST have no CORS at all (OAuth 2.1 §3.2, RFC 9700 §2.6), and a change that made every OPTIONS succeed would have taken that with it. Three decisions in it: - The requested headers are ECHOED rather than published as a fixed list. These endpoints are read without credentials and already answer `*` with no Access-Control-Allow-Credentials, so the browser sends no ambient authority and naming back what was asked grants nothing they do not already grant to anyone. A fixed list would make the next header a client adds the next incident, which is how this was found. - The /.well-known catch-alls answer the preflight too, and still 404 after it. That reads backwards until you see the alternative: a refused preflight tells a browser client only "CORS", while an answered one lets the real request reach the bare 404 the route exists to give - the answer that lets a client try its next probe. - 204 and not 200: there is nothing to send, and a preflight body is a body every client discards. DiscoveryEndpoints' remarks said "these are simple cross-origin GETs, so no preflight is involved". That was true of the clients we had rather than of the endpoint, and it is corrected in place with what it cost. Red-first: the three document routes and /token fail without the change, and the control - a host route that never asked for CORS - passes before and after. 0.5.2 and the baseline to 0.5.1, which the architecture test holds to CHANGELOG's second heading. Nothing a consumer compiles against moved; the version turns because the behaviour did. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HwL9CdWHruodcVHeQAKtGR --- CHANGELOG.md | 30 +++++++ Directory.Build.props | 4 +- .../Endpoints/DiscoveryEndpoints.cs | 85 ++++++++++++++++++- .../Endpoints/TokenEndpoint.cs | 19 ++++- .../ProtectedResourceMetadataEndpoints.cs | 56 ++++++++++++ .../DiscoveryEndpointTests.cs | 81 ++++++++++++++++++ .../ProtectedResourceMetadataEndpointTests.cs | 43 ++++++++++ 7 files changed, 314 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a6338d3..4584bce 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,36 @@ Three conventions, because a changelog nobody can rely on is worse than none: method announces itself at the consumer's next build; a renamed class in the rendered markup and a changed default in the container never do, so they carry the same marker. +## [0.5.2] + +### Fixed + +- **A CORS preflight is answered wherever a CORS header is written.** `OPTIONS` matched no route on + the authorization server or on the resource server's metadata, so a preflight fell through to + whatever the host had; a host with a deny-everything fallback policy answered `401`. Measured + against a running deployment, 2026-09-08, on all three public documents. + + It bites later than it looks, and that is the reason it is worth a release rather than a note. A + browser preflights only once a request stops being simple, so the client that finds this is the + first one to add a request header of its own, and what that client is shown is the browser's + generic "no `Access-Control-Allow-Origin` header is present" pointing at CORS configuration that + is in fact correct. Authenticating a preflight cannot be right in any case: the browser sends it + with no credentials by specification, so there is nothing in it to authenticate. + + `OPTIONS` is now mapped on the two discovery documents, the JWKS, the `/.well-known` catch-alls, + `/token`, and both forms of the RFC 9728 protected-resource metadata. That is every route that + already wrote `Access-Control-Allow-Origin` and no other: `/authorize` MUST have none (OAuth 2.1 + §3.2, RFC 9700 §2.6), and a change that made every `OPTIONS` succeed would have taken that with + it, so a test holds the boundary from both sides. + + The requested headers are echoed rather than published as a fixed list. These endpoints are read + without credentials and already answer `Access-Control-Allow-Origin: *` with no + `Access-Control-Allow-Credentials`, so naming back what was asked grants nothing they do not + already grant to anyone who asks, and a fixed list would make the next header a client adds the + next incident. + + Nothing a consumer compiles against moved; the version turns because the behaviour did. + ## [0.5.1] - 2026-09-02 ### Fixed diff --git a/Directory.Build.props b/Directory.Build.props index bad28a5..1123c37 100644 --- a/Directory.Build.props +++ b/Directory.Build.props @@ -60,7 +60,7 @@ cost twenty minutes instead of a rollback. --> - 0.5.1 + 0.5.2 @@ -202,7 +202,7 @@ --> true - 0.5.0 + 0.5.1 "contains CORS metadata, but a middleware was not found" - a 500 on every discovery /// document, while the 404 catch-all keeps working. Measured, and invisible to a test fixture /// that happens to call UseCors(). Writing the one header the documents need removes the -/// dependency: these are simple cross-origin GETs, so no preflight is involved. +/// dependency. +/// +/// +/// This paragraph used to end "these are simple cross-origin GETs, so no preflight is involved", +/// and that was true of the clients we had rather than of the endpoint. A browser preflights as +/// soon as a request stops being simple - one non-safelisted request header is enough - and +/// OPTIONS matched no route here, so it fell through to the host and a deny-everything +/// fallback policy answered 401. Measured against a running deployment, 2026-09-08. +/// +/// +/// So OPTIONS is mapped wherever this server writes Access-Control-Allow-Origin, and +/// only there. Authenticating a preflight cannot be right in any case - the browser sends it +/// without credentials by specification, so there is nothing in it to authenticate - and the cost +/// of leaving it was never the 401 itself: the browser reports a missing +/// Access-Control-Allow-Origin, which sends the reader to CORS configuration that is +/// correct. /// /// public static class DiscoveryEndpoints @@ -59,6 +74,11 @@ public static IEndpointRouteBuilder MapOAuthDiscovery( .MapMethods(path, ProbeMethods, () => Document(document)) .AllowAnonymous() .WithName("boltway-discovery-" + path.Replace('/', '_')); + + endpoints + .MapMethods(path, PreflightMethods, Preflight) + .AllowAnonymous() + .WithName("boltway-discovery-preflight-" + path.Replace('/', '_')); } endpoints @@ -66,6 +86,11 @@ public static IEndpointRouteBuilder MapOAuthDiscovery( .AllowAnonymous() .WithName("boltway-jwks"); + endpoints + .MapMethods(AuthorizationServerPaths.Jwks, PreflightMethods, Preflight) + .AllowAnonymous() + .WithName("boltway-jwks-preflight"); + // Both shapes of well-known path that this server does not serve. // // RFC 8414 §3.1 *inserts* the well-known segment before an issuer path @@ -81,12 +106,22 @@ public static IEndpointRouteBuilder MapOAuthDiscovery( // string was inserted" - and a conforming client is then required to reject what it just // fetched. A 404 lets it try the next probe instead of failing on a document it must not // trust. + // The preflight is answered here as well, and a 404 still follows it. That reads backwards + // until you see what the alternative costs: a refused preflight tells a browser client + // nothing except "CORS", while an answered one lets the real request through to the bare + // 404 this route exists to give - which is the answer that lets a client try its next + // probe, the whole reason the route is here. foreach (var (template, name) in NotFoundRoutes) { endpoints .MapMethods(template, ProbeMethods, NotFound) .AllowAnonymous() .WithName(name); + + endpoints + .MapMethods(template, PreflightMethods, Preflight) + .AllowAnonymous() + .WithName(name + "-preflight"); } return endpoints; @@ -94,6 +129,13 @@ public static IEndpointRouteBuilder MapOAuthDiscovery( private static readonly string[] ProbeMethods = ["GET", "HEAD"]; + private static readonly string[] PreflightMethods = ["OPTIONS"]; + + private static PreflightResult Preflight() => new(DocumentMethods); + + /// What a preflight on a read-only public document is told it may do. + private const string DocumentMethods = "GET, HEAD, OPTIONS"; + /// /// The well-known paths that get a bare 404. /// @@ -192,6 +234,47 @@ internal static void AllowAnyOrigin(HttpResponse response) } } +/// +/// The answer to a CORS preflight: allowed, with no body. +/// +/// +/// +/// The requested headers are echoed rather than published as a list. Every endpoint that +/// uses this serves something public and reads no credential, and the response already carries +/// Access-Control-Allow-Origin: * with no Access-Control-Allow-Credentials - so the +/// browser sends no cookie and no ambient authority, and naming back what was asked for grants +/// nothing the document does not already grant to anyone who asks. A fixed list would instead make +/// the next header a client adds the next incident, which is exactly how this defect was found. +/// +/// +/// 204 rather than 200: there is nothing to send, and a preflight with a body is a +/// body every client throws away. +/// +/// +internal sealed class PreflightResult(string allowedMethods) : IResult +{ + public Task ExecuteAsync(HttpContext httpContext) + { + ArgumentNullException.ThrowIfNull(httpContext); + + var response = httpContext.Response; + DiscoveryHeaders.AllowAnyOrigin(response); + response.Headers[HeaderNames.AccessControlAllowMethods] = allowedMethods; + + var asked = httpContext.Request.Headers[HeaderNames.AccessControlRequestHeaders]; + if (!StringValues.IsNullOrEmpty(asked)) + { + response.Headers[HeaderNames.AccessControlAllowHeaders] = asked; + } + + // Ten minutes, the same order as the documents' own five: a preflight the browser has to + // repeat on every call is a round trip per request, and these answers do not change. + response.Headers[HeaderNames.AccessControlMaxAge] = "600"; + response.StatusCode = StatusCodes.Status204NoContent; + return Task.CompletedTask; + } +} + /// A JSON body with a strong ETag and a conditional-GET short circuit. internal sealed class CachedJsonResult(ImmutableArray json, string etag, int maxAgeSeconds) : IResult { diff --git a/src/Boltway.AuthorizationServer/Endpoints/TokenEndpoint.cs b/src/Boltway.AuthorizationServer/Endpoints/TokenEndpoint.cs index 704d10e..0b74d53 100644 --- a/src/Boltway.AuthorizationServer/Endpoints/TokenEndpoint.cs +++ b/src/Boltway.AuthorizationServer/Endpoints/TokenEndpoint.cs @@ -29,11 +29,21 @@ namespace Boltway.AuthorizationServer.Endpoints; /// public static class TokenEndpoint { - /// Map POST /token. + /// Map POST /token, and the preflight a browser client may send first. /// + /// /// MapPost rather than MapMethods, so routing answers 405 for every other /// method by itself. MapGet would additionally serve HEAD, and a HEAD that reaches a /// grant handler is a token exchange whose response the client never sees. + /// + /// + /// OPTIONS is the exception, and it is mapped for the same reason this endpoint writes a + /// CORS header at all: a browser-based client calls it directly. A public client posting + /// form-encoded fields sends a simple request and never preflights, which is why this was not + /// noticed - but a client that authenticates with an Authorization header, or adds any + /// header of its own, preflights, and the preflight matched no route and fell through to the + /// host. See for what that cost. + /// /// public static IEndpointRouteBuilder MapToken(this IEndpointRouteBuilder endpoints) { @@ -44,9 +54,16 @@ public static IEndpointRouteBuilder MapToken(this IEndpointRouteBuilder endpoint .AllowAnonymous() .WithName("boltway-token"); + endpoints + .MapMethods(AuthorizationServerPaths.Token, PreflightMethods, () => new PreflightResult("POST, OPTIONS")) + .AllowAnonymous() + .WithName("boltway-token-preflight"); + return endpoints; } + private static readonly string[] PreflightMethods = ["OPTIONS"]; + /// /// Run the exchange, and turn a store that cannot be reached into a load-shed rather than a crash. /// diff --git a/src/Boltway.ResourceServer/Endpoints/ProtectedResourceMetadataEndpoints.cs b/src/Boltway.ResourceServer/Endpoints/ProtectedResourceMetadataEndpoints.cs index cf3f6dd..0b04c84 100644 --- a/src/Boltway.ResourceServer/Endpoints/ProtectedResourceMetadataEndpoints.cs +++ b/src/Boltway.ResourceServer/Endpoints/ProtectedResourceMetadataEndpoints.cs @@ -78,6 +78,10 @@ public static class ProtectedResourceMetadataEndpoints { private static readonly string[] ProbeMethods = ["GET", "HEAD"]; + private static readonly string[] PreflightMethods = ["OPTIONS"]; + + private static PreflightResult Preflight() => new(); + /// /// One hour. /// @@ -103,6 +107,21 @@ public static IEndpointRouteBuilder MapProtectedResourceMetadata(this IEndpointR .AllowAnonymous() .WithName("boltway-prm-root"); + // The CORS preflight, on both routes below as well. This document already answers + // `Access-Control-Allow-Origin: *` because browser-based clients read it, and OPTIONS + // matched no route - so a preflight fell through to the host, which on a deployment with a + // deny-everything fallback answers 401. Measured on the authorization server's identical + // routes, 2026-09-08; the same shape, found here by looking rather than by an incident. + // + // It bites later than it looks: a browser preflights only once a request stops being + // simple, so the client that finds it is the first one to add a header, and what it sees is + // the browser's "no Access-Control-Allow-Origin header is present" pointing at CORS + // configuration that is correct. + endpoints + .MapMethods(WellKnownResourceUri.Suffix, PreflightMethods, Preflight) + .AllowAnonymous() + .WithName("boltway-prm-root-preflight"); + // E-23, the path-inserted form. A catch-all is mandatory: a plain route for the suffix // alone 404s the "/mcp" variant, which is the URL a conformant client constructs first and // the one this server's own challenges point at. @@ -122,6 +141,11 @@ public static IEndpointRouteBuilder MapProtectedResourceMetadata(this IEndpointR .AllowAnonymous() .WithName("boltway-prm-inserted"); + endpoints + .MapMethods(WellKnownResourceUri.Suffix + "/{*rest}", PreflightMethods, Preflight) + .AllowAnonymous() + .WithName("boltway-prm-inserted-preflight"); + return endpoints; } @@ -166,6 +190,38 @@ internal static void AllowAnyOrigin(HttpResponse response) } /// A JSON body with a strong ETag and a conditional-GET short circuit. +/// +/// The answer to a CORS preflight: allowed, with no body. +/// +/// +/// The requested headers are echoed rather than published as a list. This document is public and +/// is read with no credential, and the response carries Access-Control-Allow-Origin: * with +/// no Access-Control-Allow-Credentials, so the browser sends no ambient authority and naming +/// back what was asked grants nothing the document does not already grant to anyone. A fixed list +/// would make the next header a client adds the next incident. +/// +internal sealed class PreflightResult : IResult +{ + public Task ExecuteAsync(HttpContext httpContext) + { + ArgumentNullException.ThrowIfNull(httpContext); + + var response = httpContext.Response; + MetadataHeaders.AllowAnyOrigin(response); + response.Headers[HeaderNames.AccessControlAllowMethods] = "GET, HEAD, OPTIONS"; + + var asked = httpContext.Request.Headers[HeaderNames.AccessControlRequestHeaders]; + if (!StringValues.IsNullOrEmpty(asked)) + { + response.Headers[HeaderNames.AccessControlAllowHeaders] = asked; + } + + response.Headers[HeaderNames.AccessControlMaxAge] = "600"; + response.StatusCode = StatusCodes.Status204NoContent; + return Task.CompletedTask; + } +} + internal sealed class CachedJsonResult(ImmutableArray json, string etag, int maxAgeSeconds) : IResult { public async Task ExecuteAsync(HttpContext httpContext) diff --git a/tests/Boltway.AuthorizationServer.Tests/DiscoveryEndpointTests.cs b/tests/Boltway.AuthorizationServer.Tests/DiscoveryEndpointTests.cs index 94dc326..6012466 100644 --- a/tests/Boltway.AuthorizationServer.Tests/DiscoveryEndpointTests.cs +++ b/tests/Boltway.AuthorizationServer.Tests/DiscoveryEndpointTests.cs @@ -333,6 +333,87 @@ public async Task Discovery_allows_cross_origin_reads() Assert.Equal("*", Assert.Single(response.Headers.GetValues("Access-Control-Allow-Origin"))); } + /// + /// A CORS preflight on a public document is answered, not authenticated. + /// + /// + /// + /// OPTIONS matched no route here, so it fell through to whatever the host had - and a + /// host with a deny-everything fallback policy answered 401. Measured against a running + /// deployment on 2026-09-08, on all three public documents. + /// + /// + /// It had not bitten yet, and the reason it had not is the reason it is worth fixing rather + /// than noting: a browser preflights only when the request is not simple, and the OIDC client + /// libraries fetch these with CORS-safelisted headers alone. So the defect waits for the first + /// client that adds one header - and what that client sees is not a 401, it is the browser's + /// generic "no Access-Control-Allow-Origin header is present", pointing at CORS configuration + /// that is in fact correct. The cost is the hour spent looking in the wrong place. + /// + /// + /// Authenticating a preflight cannot be right in any case: the browser sends it with no + /// credentials by specification, so there is nothing there to authenticate. + /// + /// + [Theory] + [InlineData("/.well-known/oauth-authorization-server")] + [InlineData("/.well-known/openid-configuration")] + [InlineData("/.well-known/jwks.json")] + public async Task A_preflight_on_a_public_document_is_answered(string url) + { + using var request = new HttpRequestMessage(HttpMethod.Options, url); + request.Headers.Add("Origin", "https://client.example"); + request.Headers.Add("Access-Control-Request-Method", "GET"); + request.Headers.Add("Access-Control-Request-Headers", "x-request-id"); + + var response = await _client.SendAsync(request); + + Assert.Equal(HttpStatusCode.NoContent, response.StatusCode); + Assert.Equal("*", Assert.Single(response.Headers.GetValues("Access-Control-Allow-Origin"))); + Assert.Contains("GET", Assert.Single(response.Headers.GetValues("Access-Control-Allow-Methods")), StringComparison.Ordinal); + // Echoed rather than published as a list. These documents are public and read without + // credentials, so naming what was asked for grants nothing that the document itself does + // not already grant, and it is what makes an unknown client work rather than the next + // header being the next incident. + Assert.Equal("x-request-id", Assert.Single(response.Headers.GetValues("Access-Control-Allow-Headers"))); + } + + /// The token endpoint too, which a browser-based client posts to. + [Fact] + public async Task A_preflight_on_the_token_endpoint_is_answered() + { + using var request = new HttpRequestMessage(HttpMethod.Options, "/token"); + request.Headers.Add("Origin", "https://client.example"); + request.Headers.Add("Access-Control-Request-Method", "POST"); + + var response = await _client.SendAsync(request); + + Assert.Equal(HttpStatusCode.NoContent, response.StatusCode); + Assert.Equal("*", Assert.Single(response.Headers.GetValues("Access-Control-Allow-Origin"))); + Assert.Contains("POST", Assert.Single(response.Headers.GetValues("Access-Control-Allow-Methods")), StringComparison.Ordinal); + } + + /// + /// The control, and it is the half that keeps the fix from being a CORS server. + /// + /// + /// A preflight is answered where this server already writes + /// Access-Control-Allow-Origin, and nowhere else. The host's own routes are the host's, + /// and /authorize in particular MUST have no CORS at all (OAuth 2.1 §3.2, RFC 9700 + /// §2.6) - a change that made every OPTIONS succeed would have taken that with it. + /// + [Fact] + public async Task A_preflight_on_a_route_that_did_not_ask_for_cors_is_left_alone() + { + using var request = new HttpRequestMessage(HttpMethod.Options, "/some/app/route"); + request.Headers.Add("Origin", "https://client.example"); + request.Headers.Add("Access-Control-Request-Method", "GET"); + + var response = await _client.SendAsync(request); + + Assert.False(response.Headers.Contains("Access-Control-Allow-Origin")); + } + /// /// The authorization endpoint has no CORS headers. /// diff --git a/tests/Boltway.ResourceServer.Tests/ProtectedResourceMetadataEndpointTests.cs b/tests/Boltway.ResourceServer.Tests/ProtectedResourceMetadataEndpointTests.cs index 4fc0583..85adad1 100644 --- a/tests/Boltway.ResourceServer.Tests/ProtectedResourceMetadataEndpointTests.cs +++ b/tests/Boltway.ResourceServer.Tests/ProtectedResourceMetadataEndpointTests.cs @@ -265,6 +265,49 @@ public async Task The_document_is_readable_cross_origin(string path) Assert.Equal("*", Assert.Single(response.Headers.GetValues("Access-Control-Allow-Origin"))); } + /// + /// A preflight on the metadata document is answered rather than authenticated. + /// + /// + /// The document already answers Access-Control-Allow-Origin: * because browser clients + /// read it, and OPTIONS matched no route - so a preflight fell through to the host, and + /// a host with a deny-everything fallback answered 401. It bites later than it looks: a + /// browser preflights only once a request stops being simple, so the client that finds it is + /// the first to add a header of its own, and what it is shown is the browser's generic + /// "no Access-Control-Allow-Origin header is present" pointing at configuration that is right. + /// + [Theory] + [InlineData(Build.MetadataPath)] + [InlineData(Build.RootMetadataPath)] + public async Task A_preflight_on_the_document_is_answered(string path) + { + await using var fixture = await ResourceServerFixture.StartAsync(); + + using var request = new HttpRequestMessage(HttpMethod.Options, new Uri(path, UriKind.Relative)); + request.Headers.Add("Origin", "https://client.example"); + request.Headers.Add("Access-Control-Request-Method", "GET"); + request.Headers.Add("Access-Control-Request-Headers", "x-request-id"); + + using var response = await fixture.Client.SendAsync(request); + + Assert.Equal(HttpStatusCode.NoContent, response.StatusCode); + Assert.Equal("*", Assert.Single(response.Headers.GetValues("Access-Control-Allow-Origin"))); + Assert.Contains("GET", Assert.Single(response.Headers.GetValues("Access-Control-Allow-Methods")), StringComparison.Ordinal); + Assert.Equal("x-request-id", Assert.Single(response.Headers.GetValues("Access-Control-Allow-Headers"))); + } + + /// The control: the GET this preflight clears still answers the document. + [Fact] + public async Task The_request_a_preflight_clears_still_answers() + { + await using var fixture = await ResourceServerFixture.StartAsync(); + + using var response = await fixture.Client.GetAsync(new Uri(Build.MetadataPath, UriKind.Relative)); + + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + Assert.Equal("application/json", response.Content.Headers.ContentType?.MediaType); + } + [Fact] public async Task A_host_that_already_set_the_origin_header_does_not_get_it_twice() {