chore(host): point default CORS allow-list at the React client origins - #1324
Merged
iammukeshm merged 1 commit intoJul 3, 2026
Merged
iammukeshm merged 1 commit into
iammukeshm merged 1 commit into
Conversation
`CorsOptions.AllowedOrigins` still listed `https://localhost:4200` (Angular) and `https://localhost:7140`, neither of which any shipped client uses. The two front-ends this kit runs are `clients/admin` (http://localhost:5173) and `clients/dashboard` (http://localhost:5174). List those instead so the default dev config actually matches the apps.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
iammukeshm
approved these changes
Jul 3, 2026
iammukeshm
left a comment
iammukeshm
left a comment
Member
There was a problem hiding this comment.
Correct — these are the two origins the kit actually serves (admin :5173, dashboard :5174); the 4200/7140 entries were stale. Merging.
iammukeshm
added a commit
that referenced
this pull request
Sep 25, 2026
…inks (#1377) * fix(identity): resolve front-end origin per-request for auth e-mail links Password-reset and e-mail-confirmation links were built from a single configured OriginUrl (which pointed at the API and was empty in Production, throwing "Origin URL is not configured") or from the raw request host (the API), so neither could target the correct SPA when more than one front-end is served (admin :5173, dashboard :5174). Introduce IOriginResolver: - FrontendOrigin(): takes the request Origin header and validates it against CorsOptions.AllowedOrigins, so the reset/confirmation link lands on the SPA the request came from. The allow-list check is the security boundary: a forged Origin on the anonymous forgot-password flow can never be injected into an e-mail. Throws when no allow-listed origin is present. - ApiOrigin(): configured origin, else request host (unchanged behaviour) for API-served assets (avatars) and RequestContextService. The confirmation e-mail now points at the SPA `/confirm-email` page (which already exists in both clients and calls the API) instead of the API route directly. - forgot-password, register, self-register and resend-confirmation now resolve the front-end origin via the resolver. - avatar URL building and RequestContextService delegate to ApiOrigin(). - appsettings: add the dev SPA origins to CorsOptions.AllowedOrigins. Production deployments must list their SPA URLs there. - tests: OriginResolverTests (allow-list, case/slash/port, forged origin, missing header), updated ForgotPassword handler + RequestContext tests, and the integration harness now sends an Origin header like a browser. * test(identity): assert forgot-password rejects a forged Origin end-to-end Drives the failure path through the real HTTP pipeline: a forgot-password request carrying an Origin header outside CorsOptions.AllowedOrigins is rejected (500) instead of returning the uniform OK, proving a spoofed origin can never be turned into a reset link. * test(identity): assert e-mail links resolve to the requesting front-end Adds EmailLinkOriginTests: drives forgot-password and register through the real pipeline and inspects the captured MailRequest body, asserting the reset link points at the SPA origin from the request's Origin header (:5174 vs :5173, proving per-front resolution) and that the confirmation link targets the SPA /confirm-email page rather than the API route. Adds the two dev SPA origins to the integration harness allow-list so per-front resolution can be exercised. Not yet executed locally: Windows Smart App Control blocks the freshly rebuilt unsigned test DLLs (0x800711C7); runs in CI (Linux). * test(identity): match confirmation/reset e-mail by subject; richer timeout The register flow also emits a welcome e-mail (via the UserRegistered integration event), so matching only by recipient grabbed the wrong message. Match the confirmation e-mail by its subject, and likewise the reset e-mail, and include the captured messages in the timeout error to diagnose misses. * test(identity): drop e-mail-body integration test; rely on unit coverage The integration harness does not execute enqueued Hangfire mail jobs (mail-asserting tests such as TenantExpiryScanJobTests invoke the job synchronously), so the confirmation/reset e-mails never reach the capturing mail service and EmailLinkOriginTests could not observe them. The link content is already covered where it is built: UserPasswordServiceTests asserts the reset link (origin + tenant + encoding) by capturing the enqueued MailRequest, OriginResolverTests covers origin resolution, and an integration test asserts a forged Origin is rejected. Reverts the harness allow-list entries that only that test needed. * docs(identity): describe origin comments by intent, not the prior behavior * fix(identity): reword confirm-email comment to satisfy S125 The explanatory comment above the confirm-email URI build read like commented-out code to SonarAnalyzer (S125) because of its parentheses and trailing semicolon, failing the -warnaserror backend build. Reword it as plain prose; behaviour is unchanged. * refactor(identity): dedicated FrontendOptions for e-mail link origins Address review on #1323. Replace the CorsOptions-coupled, throw-on-miss OriginResolver with a framework-level front-end origin resolver, so any module that builds user-facing links (Identity today; Notifications/Billing/Tickets next) resolves them the same way. - New FSH.Framework.Web.Frontend: FrontendOptions (AllowedOrigins + DefaultOrigin) + IFrontendOriginResolver/FrontendOriginResolver. Validated at startup (ValidateOnStart) so a deployment missing both fails loud on boot instead of 500-ing on the first password-reset — resolves the silent CorsOptions.AllowAll and empty-Production-list traps. - ResolveForCurrentRequest() (self-service: forgot-password, self-register): validates the Origin header against the allow-list, returns the canonical entry (not the client's casing), falls back to DefaultOrigin when no header is present (curl / Scalar / mobile / server-to-server), and throws a 400-mapped CustomException on a present-but-forged origin (was InvalidOperationException -> 500). Matching is component-wise via Uri (port exact). - ResolveDefault() (operator-driven: register, resend-confirmation): targets the recipient's app via DefaultOrigin instead of the operator's Origin, so a tenant user provisioned from the admin app no longer gets a link into :5173. Also serves background jobs that have no HttpContext. - Dedup: ApiOrigin() folded into IRequestContext.Origin (its existing contract); RequestContextService owns the config-first/request-host logic and UserProfileService reads IRequestContextService.Origin for avatar URLs. - appsettings: FrontendOptions (dev 5173/5174 + default 5174; Production empty = deploy requirement). Rebased onto main (#1324 CORS allow-list). * refactor(web): address origin-resolver review nits (log level, docs, boot message) - Log rejected origins at Debug, not Warning: the auth endpoints are anonymous, so bot/forged traffic would flood the aggregator; a genuine deployer misconfig still surfaces as a 400 to the affected SPA's users. - Document that FrontendOptions:DefaultOrigin is a single global (not per-tenant/custom-domain aware) so operator-driven links land on one SPA. - Make the FrontendOptions startup-validation message first-run actionable, matching the JwtOptions "set it before starting the host" precedent. * fix(web): require FrontendOptions:DefaultOrigin at startup The boot validation accepted AllowedOrigins-only (DefaultOrigin empty), yet operator-driven register/resend, every non-browser caller (no Origin header) and background jobs resolve through DefaultOrigin. Such a host booted clean then 500'd on the first admin register or non-browser request - the same surprise-runtime-break the fail-loud validation was meant to prevent. Require DefaultOrigin unconditionally; AllowedOrigins stays additive (widening which request origins may be echoed into self-service links). Same-origin / reverse-proxy topologies still work with DefaultOrigin alone. Fold the redundant second AddHttpContextAccessor() call into the platform's existing one. * fix(web): keep booting when FrontendOptions:DefaultOrigin is unset DefaultOrigin was validated with ValidateOnStart, so an existing deployment that upgraded without configuring it stopped booting — a setting it may never exercise took the whole host down, and the operator's first signal was a container that would not come up. Fail loud at first use of the feature, not at process start: - drop the startup validation; the host boots with DefaultOrigin unset - ResolveDefault falls back to the API's own origin (OriginOptions:OriginUrl) so links land somewhere serviceable instead of going dark - UseHeroPlatform logs one startup Warning naming the setting, the file and what degrades without it The fallback is deliberately the configured API origin and never the current request's host: ResolveDefault exists because the caller is not the recipient, so an operator-driven confirmation link must not point at the admin app. Forged-origin rejection is unchanged — a present-but-unlisted Origin is still a 400, never swapped for the fallback. * fix(web): fall back to the request host when no origin is configured at all appsettings.Production.json ships OriginOptions:OriginUrl empty as well, so a deployment that upgraded without touching either setting still had no origin to build a link from and 500'd on the first operator-driven register/resend - the exact failure the boot-safety fallback was meant to remove. ResolveDefault now walks DefaultOrigin, then the configured API origin, then the current request's host, and only throws when there is no request either (a background job). The request host is the API's own, never the caller's Origin header, so an operator-driven link still cannot point at the admin SPA. * fix(web): resolve links against the default when no allow-list is configured appsettings.Production.json ships FrontendOptions:AllowedOrigins empty, and browsers attach an Origin header to the forgot-password and self-register POSTs even same-origin. Matching a present header against an empty list returned no canonical entry, so every legitimate password reset and self-registration came back 400 on the shipped Production config - and on any single-SPA or reverse-proxy deployment. With no allow-list there is nothing to validate against, so the header is discarded and the link resolves through the server-side default. The client's value is never echoed, so a forged origin against a configured list is still rejected with 400. The startup Warning now reports an empty AllowedOrigins independently of a missing DefaultOrigin: a deployment can configure one and not the other, and setting only the default silently sends every user to the same front-end. Also matches origins through IdnHost, so a list entry written in Unicode matches the punycode form browsers actually send instead of failing closed, and pins the handler contract on CustomException rather than the arbitrary exception type the old test stubbed. * fix(web): count the allow-list after normalization in the startup warning Unparseable entries are dropped when the resolver normalizes the list, so a list of nothing but typos matched the empty-list fallback at runtime while the warning, reading the raw config array, saw a configured list and stayed quiet. The operator got neither their allow-list nor a diagnostic. The warning now counts the normalized list, and reports separately when only some entries were dropped - those origins are rejected with 400 rather than silently ignored. * docs(web): stop claiming the Scalar try-it UI sends no Origin header Scalar.AspNetCore 2.14.14 ships no default proxy URL (the option exists but binds null, and no proxy host is baked into the assembly), so the try-it panel fetches straight from the browser and sends the API's own origin. Listing it alongside curl and server-to-server callers was wrong: those genuinely send no Origin and fall back to the default, while Scalar hits the allow-list branch and needs the API origin listed to exercise forgot-password or self-register. * docs(rules): document the front-end origin resolver in the security rule The rule file agents read before touching CORS, headers or rate limiting had no entry for FrontendOptions, so the next person to add an e-mail link had nothing telling them which resolver method matches which recipient - a choice where both options compile and both return a plausible origin. * docs(agents): list front-end link origins in the security rule index The index line is how an agent decides whether to open security.md at all. * fix(deploy): wire FrontendOptions into the docker and terraform deploys Both shipped deployment paths left `FrontendOptions` empty, so the resolver fell through to the API origin and every password-reset / e-mail-confirmation link pointed at `https://api.../reset-password` and `https://api.../confirm-email` -- SPA routes that do not exist on the API. Each path already knows the SPA URLs, so the fix is to pass them through: - `docker-compose.yml` -- `FrontendOptions__AllowedOrigins__0/1` from the existing `FSH_ADMIN_URL` / `FSH_DASHBOARD_URL`, with the dashboard as `DefaultOrigin` so an operator-driven register / resend lands on the tenant app, not on admin. - Terraform `app_stack` -- a `frontend_environment_variables` map mirroring the CORS one, built from the resolved `admin_url` / `dashboard_url` plus `api_extra_cors_origins` (extra SPA origins the deployer already trusts, which would otherwise start getting a 400 on forgot-password once the list is non-empty). The API domain is deliberately *not* carried over from the CORS list: allow-listing it reintroduces the same wrong-destination link. `DefaultOrigin` is the dashboard, falling back to admin, and stays empty when the stack hosts neither -- the pre-existing `OriginOptions__OriginUrl` behaviour. The Docker README gains the link-building meaning of those two `.env` URLs and a troubleshooting row for a link that lands on the API. Verified: `docker compose config` renders the three new keys; `terraform fmt -check -recursive` and `terraform validate` pass; the `DefaultOrigin` expression checked in `terraform console` for all three branches (dashboard, admin-only, neither). * build(deps): bump Testcontainers to 4.14.0 and SourceLink past their advisories `dotnet restore` fails for the whole solution under `TreatWarningsAsErrors`, on `main` and on every open PR alike. Advisory-database drift, not a regression from any change: a commit green on 2026-08-10 is red today with no edits. - `Testcontainers.PostgreSql` / `.Redis` / `.Minio` 4.11.0 -> 4.14.0 (NU1903, GHSA-q939-rpr3-3284). 4.11.0 depends on `SSH.NET` 2025.1.0; 4.14.0 already depends on the patched 2026.0.0, so the advisory clears with no transitive pin to remember to remove later. Same fix as #1369, so the two do not conflict. - `Microsoft.SourceLink.GitHub` 8.0.0 -> 10.0.401 (NU1902, GHSA-23fw-v26w-5fgq). 8.0.0 drags in `Microsoft.Build.Tasks.Git` 8.0.0 and the 8.x line has no patched release, so a transitive pin cannot fix it; the package itself has to move. 10.0.401 depends on `Microsoft.Build.Tasks.Git` 10.0.401, past the patched 10.0.303. Build-time only (`PrivateAssets="all"`), referenced only where `IsPackable == true`, which is the CLI alone - and `src/Tools/**` is excluded from the template, so the scaffold never sees it. Verified: `dotnet restore src/FSH.Starter.slnx` exits 0 with no NU19xx, and `dotnet build src/FSH.Starter.slnx -c Release -warnaserror` reports 0 warnings and 0 errors. * fix(infra): pull MinIO from quay.io on a pinned tag, not Docker Hub MinIO withdrew `minio/minio` from Docker Hub. Docker Hub's API now answers `object not found` for the repository, and a pull fails with: pull access denied for minio/minio, repository does not exist or may require 'docker login' That takes down every Testcontainers-backed integration test (the harness boots a MinIO container per fixture, so all 724 tests in `Integration.Tests` fail at container start), the Aspire AppHost, and the Docker Compose deployment. The image is still published at `quay.io/minio/minio`: - `Integration.Tests` and `Integration.Middleware.Tests` harnesses - `AppHost.cs`, via Aspire's `WithImageRegistry` / `WithImageTag` - `deploy/docker/docker-compose.yml` and the image table in its README The tag is pinned to `RELEASE.2025-09-07T16-13-09Z` rather than `:latest`. quay has not moved `:latest` since 2025-09-07, so the two resolve to the same digest today; pinning only removes the surprise of a silent move later, and keeps the test harness off a floating tag. Whether to track a newer release, or a different S3-compatible image, is a separate call. While in the README's image table: `postgres` and `redis` rows had drifted from what compose actually ships (`postgres:18-alpine`, `valkey/valkey:9.1.0-alpine`). Verified: `docker pull minio/minio:latest` fails with the error above; `docker pull quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z` succeeds (`sha256:14cea493d9a34af32f524e538b8346cf79f3321eff8e708c1e2960462bd8936e`, the same digest `:latest` resolves to). `dotnet test Integration.Tests -c Release` passes against the pinned image, and the Aspire manifest renders the container as `quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z`. * fix(infra): keep api_extra_cors_origins out of the e-mail-link allow-list api_extra_cors_origins grants an origin permission to CALL the API, which is what its description promises. Copying it into FrontendOptions:AllowedOrigins also let those origins receive a password-reset or e-mail-confirmation URL with the token in it, turning a CORS grant into a credential-link grant. The two lists stay separate: CORS still includes the extra origins, e-mail links only the SPAs this stack hosts, which arrive via admin_url/dashboard_url. An origin listed only for CORS now gets a 400 from the anonymous self-service endpoints, which is the intended fail-closed behaviour. * fix(web): stop deriving e-mail-link origins from the request host ResolveDefault() fell through DefaultOrigin -> OriginOptions:OriginUrl -> the request's own Host header. Both fallbacks are now gone. The Host tier is the security half. appsettings.Production.json ships FrontendOptions:AllowedOrigins [], DefaultOrigin "", OriginUrl "" and AllowedHosts "*", so on the shipped production config a forgot-password POST with a forged Host header mails the reset token to the attacker's domain. Before this resolver existed the same path threw, so this was a regression introduced by the fallback, not a pre-existing hole. The OriginUrl tier is the correctness half, and it is why the second fallback goes too. These links address SPA routes (/confirm-email, /reset-password); the API serves confirm-email under api/v{version}/identity, so a link built on the API's own origin is a 404. "Degrade to the API origin" stopped being serviceable the moment the paths changed. What is left is DefaultOrigin or a 500 naming the setting, and the startup log for a missing DefaultOrigin moves from Warning to Error to match: the consequence is no longer degradation. Both shipped deploy paths (docker compose, terraform) already set it; the gap is a bare appsettings.Production.json. Upgrade note for same-origin reverse-proxy deployments that set only OriginUrl: set FrontendOptions:DefaultOrigin to the same value. The eight tests that pinned the removed tiers are inverted, not deleted; the request-host one now asserts the throw and that the attacker's host never reaches the message. Framework.Tests 152/152, Identity.Tests 312/312, solution builds clean under TreatWarningsAsErrors. * fix(config): keep the dev SPA origins out of Production `appsettings.Production.json` shipped `"AllowedOrigins": []` for both CorsOptions and FrontendOptions, on the assumption that an empty array clears the base file. It does not: a JSON array is flattened to indexed keys, an empty one writes no indices at all, and the binder concatenates whatever the earlier provider left. A production deployment therefore trusted `http://localhost:5173` and `:5174` — as a CORS origin, and as an origin that may appear inside a password-reset link. The dev origins move to `appsettings.Development.json`, which Production never loads, so the empty arrays in the Production file are now true. `ShippedConfigurationTests` loads the shipped files the way the host does and asserts what each environment actually gets. Verified by mutation: putting one origin back in `appsettings.json` turns it red. The CorsOptions half of this is pre-existing (`main` has the same shape) and is fixed here because it is the same defect in the same file; without it the fix would read as "localhost is untrusted now", which would only be half true. * fix(infra): pull minio/mc from quay.io too, not just minio/minio The MinIO carve-out this branch carries only moved `minio/minio`. `minio/mc` is gone from Docker Hub as well (`hub.docker.com/v2/repositories/minio/mc/` answers 404), and it is what `minio-init` runs: without it `dotnet run --project src/Host/FSH.Starter.AppHost` and `docker compose up` both die on the image pull, and the `fsh` bucket is never created, so the first upload fails with NoSuchBucket. Same pinned tag as #1388, which owns the fix, so the copy stays byte-identical to it and can be dropped once that lands. * fix(frontend-origin): validate DefaultOrigin and correct three stale comments `DefaultOrigin` was only checked for emptiness while `AllowedOrigins` went through `Uri.TryCreate`. A value like `app.example.com` — no scheme, the usual `.env` slip — bound cleanly, produced no startup diagnostic, and turned every e-mail link into a relative URL no mail client makes clickable. It is now required to parse as an absolute URI, and failing that is the same Error as being unset. Three comments still described the fallback chain this PR removed: - `Web/Extensions.cs` said the resolver falls back to the API's own origin and logs a Warning. There is no fallback (it throws) and the log is an Error. That one sits in protected code, where the next maintainer would have read it as "safe degradation exists" and re-introduced the tier. - `app_stack/main.tf` said an empty list leaves the API resolving links from `OriginOptions__OriginUrl`. That tier is gone; those flows answer 500. - The rejection log wrote the caller-controlled `Origin` header verbatim. It is truncated and stripped of line breaks now, the same treatment the global exception handler gives the request path. * fix(frontend-origin): treat an unusable DefaultOrigin as an unset one at run time The startup check added in the previous commit calls a `DefaultOrigin` that is not an absolute URL "the same failure class as an unset value", but only the log line agreed: the resolver still handed the raw string back, so `app.example.com` produced a relative URL in every e-mail, which no mail client makes clickable, and nothing on the request path reported a problem. It now goes through the same normalization the allow-list gets: a value that does not parse as an absolute URI is dropped, and `ResolveDefault()` fails the way it does when nothing is configured. A base path is preserved (validating must not collapse `https://example.com/app` to its authority, or `/reset-password` 404s), and both branches are covered. Reverting the guard turns the first red. The startup message said "is not set" for a value that is set but unusable; it says "is not set to an absolute URL" now. * fix(api): fail fast in Production when FrontendOptions:DefaultOrigin is unset Without it, register / resend-confirmation / forgot-password can only return 500, so a missing or relative value now stops the API at boot, in the existing Production fail-fast block next to the connection string and signing key. Not validated in AddHeroPlatform because the DbMigrator also calls it and never sends links. Non-Production keeps the single startup Error. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: iammukeshm <iammukeshm@gmail.com> Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
CorsOptions.AllowedOriginsinappsettings.jsonlistedhttps://localhost:4200(Angular's default dev port) andhttps://localhost:7140, neither of which any client in this repo uses.The two front-ends the kit actually ships and runs (via the Aspire AppHost) are:
clients/admin→http://localhost:5173clients/dashboard→http://localhost:5174This replaces the stale entries with the real React dev origins so the default configuration matches the apps out of the box.
Note
This touches the same
AllowedOriginsblock as #1323 (per-request front-end origin resolution), which adds:5173/:5174on top of the existing entries. Whichever merges first, the other will need a trivial rebase of this one array. Happy to sequence them however you prefer.