-
Notifications
You must be signed in to change notification settings - Fork 0
fix(api): fail loudly when Cors:Origins is empty outside Development #511
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
2fe154d
5c2066e
324a118
6e05ce2
af8d53d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,29 @@ | ||||||
| namespace TrueMain.Options; | ||||||
|
|
||||||
| /// <summary> | ||||||
| /// Cross-origin policy for the public API, bound from <c>Cors:*</c>. The single | ||||||
| /// <see cref="Origins"/> list drives the <c>FrontendCors</c> policy: the browser | ||||||
| /// receives <c>Access-Control-Allow-Origin</c> only for the hosts listed here. | ||||||
| /// Named <c>FrontendCorsOptions</c> (not <c>CorsOptions</c>) to avoid colliding | ||||||
| /// with ASP.NET Core's own <c>Microsoft.AspNetCore.Cors.Infrastructure.CorsOptions</c>. | ||||||
| /// </summary> | ||||||
| /// <remarks> | ||||||
| /// An empty list is a silent-failure trap — the policy still builds, but with no | ||||||
| /// allowed origins the frontend's cross-origin requests are rejected by the | ||||||
| /// browser even though the API itself answers. It reads as "works locally, | ||||||
| /// broken in prod" because Development ships real origins while | ||||||
| /// <c>appsettings.json</c> ships an empty array. Startup therefore validates the | ||||||
| /// list: empty fails the boot in every non-Development environment (see | ||||||
| /// <c>Program.cs</c>) and only logs a warning under Development. | ||||||
| /// </remarks> | ||||||
| public sealed class FrontendCorsOptions | ||||||
| { | ||||||
| public const string SectionName = "Cors"; | ||||||
|
|
||||||
| /// <summary> | ||||||
| /// Exact origins (scheme + host + port) allowed to make cross-origin browser | ||||||
| /// requests, e.g. <c>https://truemain.app</c>. Must be non-empty outside | ||||||
| /// Development. | ||||||
| /// </summary> | ||||||
| public string[] Origins { get; set; } = []; | ||||||
| } | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NON BLOQUANT —
Suggested change
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,107 @@ | ||
| using System.Net; | ||
| using AwesomeAssertions; | ||
| using Microsoft.AspNetCore.Hosting; | ||
| using Microsoft.AspNetCore.Mvc.Testing; | ||
| using Microsoft.Extensions.Configuration; | ||
| using Microsoft.Extensions.Options; | ||
|
|
||
| namespace TrueMain.IntegrationTests; | ||
|
|
||
| /// <summary> | ||
| /// Covers the startup CORS guard added for issue #209: outside Development an | ||
| /// empty <c>Cors:Origins</c> must fail the boot loudly (it would otherwise ship a | ||
| /// no-op CORS policy that silently rejects the frontend in production), while a | ||
| /// populated list boots and actually allows the configured origin. | ||
| /// </summary> | ||
| [Collection(IntegrationCollection.Name)] | ||
| public sealed class CorsStartupIntegrationTests | ||
| { | ||
| private readonly PostgresFixture _fixture; | ||
|
|
||
| public CorsStartupIntegrationTests(PostgresFixture fixture) | ||
| { | ||
| _fixture = fixture; | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task Startup_InNonDevelopment_FailsWhenCorsOriginsEmpty() | ||
| { | ||
| // appsettings.json ships "Cors:Origins": [], so withholding the override | ||
| // leaves the list empty. Testing is non-Development, so ValidateOnStart | ||
| // must reject the boot rather than silently run with a no-op policy. | ||
| // (Testing, not Production: the readiness-health-check guard fails fast on | ||
| // a missing connection string only under Production, and the test host | ||
| // injects that string after Program reads it.) | ||
| await using var factory = new CorsStartupFactory(_fixture, environment: "Testing", origin: null); | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NON BLOQUANT — |
||
| var startup = () => factory.CreateClient(); | ||
|
|
||
| startup.Should().Throw<OptionsValidationException>( | ||
| "an empty Cors:Origins outside Development must fail the boot, not run a no-op policy") | ||
| .WithMessage("*Cors:Origins*"); | ||
| } | ||
|
|
||
| [Fact] | ||
|
ilyanfraimbault marked this conversation as resolved.
|
||
| public async Task Startup_InNonDevelopment_AllowsConfiguredOriginWhenPresent() | ||
| { | ||
| const string allowedOrigin = "https://app.truemain.test"; | ||
| await using var factory = new CorsStartupFactory(_fixture, environment: "Testing", origin: allowedOrigin); | ||
| using var client = factory.CreateClient(new WebApplicationFactoryClientOptions | ||
| { | ||
| BaseAddress = new Uri("https://localhost") | ||
| }); | ||
|
|
||
| // A CORS preflight exercises the policy directly through the CORS | ||
| // middleware, so the assertion stays on the header that matters | ||
| // (Access-Control-Allow-Origin) without coupling to any endpoint's | ||
| // business logic or status code. | ||
| using var preflight = new HttpRequestMessage(HttpMethod.Options, "/champions"); | ||
| preflight.Headers.Add("Origin", allowedOrigin); | ||
| preflight.Headers.Add("Access-Control-Request-Method", "GET"); | ||
|
|
||
| var response = await client.SendAsync(preflight); | ||
|
ilyanfraimbault marked this conversation as resolved.
|
||
|
|
||
| response.StatusCode.Should().Be(HttpStatusCode.NoContent, | ||
| "a handled CORS preflight is short-circuited by the middleware with 204 No Content"); | ||
| response.Headers.GetValues("Access-Control-Allow-Origin").Should().ContainSingle() | ||
| .Which.Should().Be(allowedOrigin, | ||
| "the configured origin must be echoed back so the browser accepts the cross-origin response"); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// A <see cref="WebApplicationFactory{TEntryPoint}"/> that — unlike | ||
| /// <see cref="TrueMainWebApplicationFactory{TEntryPoint}"/> — does not inject a | ||
| /// default <c>Cors:Origins</c>, so a test can drive the host with the origin | ||
| /// list either empty (a null origin argument) or populated, and pick the | ||
| /// environment that decides whether the guard fails or warns. | ||
| /// </summary> | ||
| private sealed class CorsStartupFactory : WebApplicationFactory<Program> | ||
| { | ||
| private readonly string _environment; | ||
| private readonly List<KeyValuePair<string, string?>> _configuration; | ||
|
|
||
| public CorsStartupFactory(PostgresFixture fixture, string environment, string? origin) | ||
| { | ||
| _environment = environment; | ||
| _configuration = | ||
| [ | ||
| new KeyValuePair<string, string?>("ConnectionStrings:TrueMain", fixture.ConnectionString), | ||
| new KeyValuePair<string, string?>( | ||
| "Ops:ApiKey", | ||
| TrueMainWebApplicationFactory<Program>.DefaultOpsApiKey) | ||
| ]; | ||
|
|
||
| if (origin is not null) | ||
| { | ||
| _configuration.Add(new KeyValuePair<string, string?>("Cors:Origins:0", origin)); | ||
| } | ||
| } | ||
|
|
||
| protected override void ConfigureWebHost(IWebHostBuilder builder) | ||
| { | ||
| builder.UseEnvironment(_environment); | ||
| builder.ConfigureAppConfiguration((_, configurationBuilder) => | ||
| configurationBuilder.AddInMemoryCollection(_configuration)); | ||
| } | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.