fix(identity): resolve front-end origin per-request for auth e-mail links - #1377
marcelo-maciel wants to merge 20 commits into
Conversation
…inks 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.
…-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.
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).
…meout 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.
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.
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.
Address review on fullstackhero#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 (fullstackhero#1324 CORS allow-list).
…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.
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.
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.
…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.
…figured 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.
…ning 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.
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.
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.
The index line is how an agent decides whether to open security.md at all.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 124f182e8a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0ed861dcb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # from one of them would otherwise be rejected with a 400 once this list is non-empty. | ||
| frontend_allowed_origins = compact(concat( | ||
| [local.admin_url, local.dashboard_url], | ||
| var.api_extra_cors_origins, |
There was a problem hiding this comment.
Keep extra CORS origins out of the email-link allow-list
When api_extra_cors_origins contains a web origin that is allowed only to call the API (its documented contract), this copies it into FrontendOptions:AllowedOrigins. A caller can then use that origin on the anonymous forgot-password or self-registration flows and cause a reset/confirmation URL containing the token to target that external origin, turning CORS permission into an unintended credential-link trust grant. Use a separate explicitly configured frontend-link list instead of copying arbitrary extra CORS entries. .agents/rules/security.mdL12-L12
Useful? React with 👍 / 👎.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 similar comment
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
d974f26 to
c9a7f0f
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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).
…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 fullstackhero#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.
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`.
c9a7f0f to
84aae7a
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Problem
The kit ships two front-ends (admin on
:5173, dashboard on:5174), but the back-end had no way to build a user-facing link that targets the front-end a request actually came from:OriginOptions.OriginUrl. Inappsettings.jsonthat value is the API URL (https://localhost:7030), and inappsettings.Production.jsonit is empty, so the handler threw"Origin URL is not configured.".api/v1/identity/confirm-email(which returns JSON) rather than a front-end page.Originheader was never consulted, so with more than one SPA there was no way to send the link to the correct one.This is the structural follow-up to #1302, which fixed only the reset-link string format (trailing slash,
tenantparam, URL-encoding).Solution
A framework-level
IFrontendOriginResolverwith two notions of origin, matched to who receives the link:ResolveForCurrentRequest()(self-service: forgot-password, self-register) reads the requestOriginheader, validates it against an allow-list, and returns the canonical configured entry (never the client's raw casing). A present-but-unlisted origin is a forged or misconfigured client, so it throws a 400-mapped exception. When the request carries noOriginheader (non-browser callers: curl, the Scalar try-it UI, mobile, server-to-server), it falls back to a configured default rather than failing an otherwise valid flow.ResolveDefault()(operator-driven: an admin registering or re-inviting a tenant user, whose confirmation link must land on the tenant's app rather than the operator's; and background jobs with no HTTP request) returns the configured default front-end origin.Matching is component-wise
Uricomparison (scheme + host + port, port exact), normalized once at startup, so an entry like:443or an IDN form does not silently fail a raw string compare.The confirmation e-mail now points at the SPA
/confirm-emailpage (which already exists in bothclients/adminandclients/dashboardand calls the API) instead of the API route directly.Changes to
src/BuildingBlocks(Golden Rule #4, requesting sign-off)The first revision of this PR kept the resolver inside the Identity module and coupled it to
CorsOptions. Per your review (coupling the e-mail-link trust list to the CORS list breaks same-origin / reverse-proxy topologies), the resolver is now framework-level so any module that sends user-facing links (Identity today, Notifications / Billing / Tickets tomorrow) resolves the origin the same way. That places it in protected code, and the PR description must say so plainly:src/BuildingBlocks/Web/Frontend/—IFrontendOriginResolver,FrontendOriginResolver(internal),FrontendOptions.src/BuildingBlocks/Web/Extensions.cs— bindsFrontendOptions, registers the resolver andIHttpContextAccessor, and logs the one startupWarningwhenDefaultOriginis unset.src/BuildingBlocks/Web/Web.csproj—InternalsVisibleTo("Framework.Tests")so the internal resolver is unit-testable.Flagging explicitly for approval under Golden Rule #4; the earlier "no changes to BuildingBlocks" line was wrong and is corrected here.
Config and upgrade note (Golden Rule #10)
A dedicated
FrontendOptions, deliberately separate fromCorsOptions:FrontendOptions:AllowedOrigins— SPA origins trusted to appear in e-mail links.FrontendOptions:DefaultOrigin— fallback SPA for non-browser and operator-driven flows.appsettings.jsonlists the dev SPA origins (http://localhost:5173,http://localhost:5174) plus aDefaultOrigin, so a local run and the Aspire stack work unchanged.appsettings.Production.jsonships both empty, but the two shipped deployment paths populate them from the SPA URLs they already know:deploy/docker/docker-compose.ymlfromFSH_ADMIN_URL/FSH_DASHBOARD_URL, and the AWS Terraform stack from the resolvedadmin_url/dashboard_url(plusapi_extra_cors_origins, so an extra trusted SPA origin does not start getting a 400 once the list is non-empty). The API domain is deliberately not carried over from the CORS list: allow-listing the API origin is what puts the link back on the API.An existing deployment keeps booting after the upgrade. There is no
ValidateOnStarton these settings: loud at first use of the feature is right, loud at process start for a feature the deployment may never exercise is not. WithDefaultOriginunset the host starts, logs a single startupWarningnaming the setting, the config file and what degrades, andResolveDefault()walks a fallback chain:FrontendOptions:DefaultOriginOriginOptions:OriginUrl(the API's own configured public base)appsettings.Production.jsonshipsOriginUrlempty too, so a deployment that touched neither setting must still produce a linkTiers 2–3 put the link on the API rather than the SPA: serviceable, and the same place register / self-register / resend derived their links from before this PR. Nothing goes dark, and the operator is told.
Tier 3 is the API's own request host, never the caller's
Originheader — that distinction is the whole reasonResolveDefault()exists apart fromResolveForCurrentRequest(), so an operator-driven confirmation link still cannot point back at the admin SPA the request came from. Forged-origin rejection is untouched: a presentOriginthat misses a configured allow-list is still a400, never swapped for a fallback.AllowedOriginsis purely additive: it only widens which request origins may be echoed into self-service links. An empty list means there is nothing to validate against, so the header is discarded and the link usesDefaultOrigin— browsers attachOriginto these POSTs even same-origin, so matching an empty list would 400 every legitimate reset on the shipped Production config and on any single-SPA or reverse-proxy topology. The client's value is never echoed either way, so this is not a relaxation: a forged origin against a configured list is still a400. The startupWarningnames the empty list separately from a missingDefaultOrigin, since a deployment can get one right and the other wrong.OriginOptions:OriginUrlkeeps its meaning as the API public base (avatars /IRequestContext.Origin); it is no longer overloaded as the reset-link base, but it is now also the fallback for link building.Known limitation:
DefaultOriginis a single global, not per-tenant / custom-domain aware, so operator-driven register / resend point every tenant's link at that one SPA. That fits the kit's single-dashboard model; a per-tenant-custom-domain deployment would resolve the recipient tenant's own origin instead. Documented on the option.Security
The allow-list check is the security boundary: because forgot-password is anonymous, a forged
Originheader must never be turned into a link inside an e-mail. The resolver validates againstFrontendOptions:AllowedOriginsindependently ofCorsOptions.AllowAll, returns only the canonical listed entry, and rejects anything else with a 400. Rejections log atDebug(anonymous endpoints, so bot traffic would flood the aggregator atWarning); a genuine deployer misconfig still surfaces as a 400 to the affected SPA's own users.Tests
FrontendOriginResolverTests(Framework.Tests) — allow-listed origin returns the canonical entry; trailing-slash / case match; differing port does not match; forged origin throws 400; missing header falls back toDefaultOrigin. Boot-safety tiers:DefaultOriginunset (and the empty stringappsettings.Production.jsonships) falls back to the API origin; a configuredDefaultOriginwins over it; a non-absoluteOriginUrl(also shipped as"") is skipped in favour of the request host; nothing configured and no request throws; and a forged header is still400even when a fallback is available.ForgotPasswordCommandHandlerTestsupdated to the resolver.ForgotPassword_Should_Reject_When_OriginNotAlloweddrives a forgedOriginend-to-end (rejected, no reset link); the harness sends anOriginheader like a browser.Docs
Docs + changelog land in the separate
fullstackhero/docssite: docs#232.