feat(portal): same-origin /apps/<slug>/* proxy so federated remotes can load, + split the two PROVIDER_UNAVAILABLE conditions in the log - #812
Open
github-actions[bot] wants to merge 6 commits into
Conversation
…nd logged neither
`POST /api/v1/security/session` answers 503 PROVIDER_UNAVAILABLE for two
errors that call for OPPOSITE responses from whoever is paged:
AuthentikUnavailableError the identity provider is unreachable or too slow.
A platform incident. Every password sign-in is
affected. Go look at the provider.
UnsupportedFlowStageError THIS account's login flow presented a stage a
server cannot drive -- MFA, consent, a prompt.
Nothing is down. Browser/SSO sign-in is
unaffected. Only the programmatic password grant
fails, and only for that account.
The response cannot distinguish them, and that is correct and deliberate:
security-routes.test.ts pins that the provider's raw message is never echoed to
this unauthenticated endpoint, because it can name internal hosts and flow
slugs. The contract also fixes the code at PROVIDER_UNAVAILABLE for both.
So the log is the only place the distinction can live -- and this branch logged
NOTHING. The stage component was carried in the thrown error
(`new UnsupportedFlowStageError(component)`, authentikPassword.ts) and thrown
away one line from where it was needed.
MEASURED COST, today. A synthetic monitoring account's flow gained an
undriveable stage after a Helm upgrade re-ran the seed-admin post-upgrade hook.
The portal census probe began returning:
503 {"error":"Authentication unavailable","code":"PROVIDER_UNAVAILABLE"}
with nothing in the log to qualify it. Read literally, against an endpoint
named "authentication", that says the platform is down -- so it was escalated
as a production auth outage. It was not: browser sign-in worked throughout, as
the owner confirmed by simply logging in. Two hours and four probe runs went
into re-deriving from source what `err.message` already knew.
The fix is only logging. Status, code, headers and body are byte-identical, so
no client behaviour and no contract clause changes:
UnsupportedFlowStageError -> console.warn, names the stage, and states
explicitly that it is NOT an outage and that
browser/SSO sign-in is unaffected.
AuthentikUnavailableError -> console.error, names it a platform incident.
Newlines are stripped from the provider-derived message before logging so it
cannot forge additional log lines.
This is PARITY, not invention. The sibling legacy route (routes/auth.ts) has
distinguished these two since it was written -- warn with the stage vs. error
for the outage. `/api/v1/security/session`, the route the login form actually
calls, was the sole outlier. The two are now consistent.
Three tests pin it, and they were verified to FAIL against the unpatched
source before being kept: the stage name appears, the outage line reads
"platform incident" and never "NOT an outage", and an embedded newline does not
produce a forged log line.
Verified: npx tsc --noEmit clean; jest tests/security-routes.test.ts
114 passed, 114 total (111 pre-existing + 3 new).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GaPa3JgrVNtWrGvqQEAEqv
Owner
Blocked: CI never started — 17 workflows are awaiting manual approvalNot a failure. On head For contrast, the previous head on this same branch ( One click unblocks it: the PR's Checks tab → Approve and run workflows. I can't do it from here — the tooling available to this session exposes run / re-run / cancel / delete-logs but no approve, and re-running a gated run would not bypass the gate anyway. I'm not going to try to route around an approval control. Nothing is red and there is nothing to fix. For what it's worth, local verification before the push was: and the three new tests were each confirmed to fail against the unpatched source before being kept — with the logging block removed, all three go red. That is evidence the tests are real, not a substitute for CI. I'll pick this up as soon as the workflows are approved. Generated by Claude Code |
…nt-registration-hymmqk
…an load
The portal census has never scored more than ONE app as genuinely federated
(run 33009185082, 2026-08-26 20:12Z: 13 registered, 3 PASS, 10 FAIL, and only
`fuzeservice` loads remoteEntry + chunks as JavaScript). The cause is not the
registered manifests, which is where several rounds of fixes went. It is that
nothing ever served /apps/<slug>/* on the portal origin.
frontend/src/utils/loadFederatedApp.ts:71 is the entire resolution mechanism:
const resolved = new URL(remoteEntry, origin)
so a registered `/apps/finance/remoteEntry.js` is fetched from
app.fuzefront.com. No ingress rule matched that path, so it fell through to the
`/` rule, reached the frontend, and the SPA fallback answered with 200 +
index.html. That is exactly what the census reports for fuzequality --
"remoteEntry returned 200 but is HTML" -- and the browser refuses a module
served as text/html. Blank panel, green healthcheck.
WHY NOT THE MECHANISM THAT ALREADY EXISTED. `.Values.federatedApps` renders one
Ingress per remote and has been `[]` in every values file since it was written.
It cannot work: an Ingress may only name a Service in its OWN namespace, and
every family product deploys to its own. The documented escape hatch, an
ExternalName Service, is refused by Traefik unless allowExternalNameServices is
set, and it defaults to false. `clock` is reachable only because it has a
hand-written same-namespace block. The mechanism was not misconfigured; it was
inapplicable, which is why populating it would not have helped.
A reverse proxy has no namespace restriction -- cross-namespace Service DNS is
an ordinary HTTP call -- and it puts every remote on the portal's own origin,
which the same-origin/no-mixed-content rule wants anyway.
backend/src/routes/federatedProxy.ts the proxy
ingress.yaml /apps -> fuzefront-backend, ABOVE /
frontend/nginx.conf location /apps/ (in-pod path)
values.yaml federatedProxy.enabled + upstreams
_helpers.tpl upstreams -> JSON env
TRUST MODEL, and the reason this is an allowlist rather than a registry lookup.
The registry knows every remote's URL, so deriving the target from it is the
obvious shortcut. It is also an SSRF: any product can edit its own manifest via
PUT /apps/{slug}, and a `<script src>` carries no credentials, so a registered
app could aim an unauthenticated proxy at an arbitrary in-cluster address and
read the response. The upstream set is therefore exactly what an operator wrote
in values. Consequences, all deliberate: GET/HEAD only; inbound Authorization
and Cookie are never forwarded upstream; outbound Set-Cookie is dropped; no
redirects followed; `..` and %2e%2e rejected rather than normalised.
Two things the proxy must NOT do, both pinned by tests:
- Never launder a remote's 404 into a 200. Its status and Content-Type are
relayed verbatim, because Content-Type is the whole ballgame for Module
Federation and improving on it is how the current failure hides.
- Never relay Content-Encoding. axios has already decompressed the body, so
echoing `gzip` hands the browser plaintext labelled as compressed.
Defaults are off and empty: `federatedProxy.enabled: false`, `upstreams: []`.
With nothing configured every /apps/<slug>/* answers 404 -- a truthful "not
configured", which is strictly better than the 200-that-is-HTML it replaces.
A malformed FEDERATED_PROXY_UPSTREAMS drops entries with a loud error rather
than crashing, because this router shares a process with auth and billing; but
a missing `slug`/`url` FAILS THE HELM RENDER, since a silently dropped remote is
indistinguishable from a working one until someone opens the portal.
No prod Service names are guessed. Populating `upstreams` is a separate,
operator-owned change; this commit ships the mechanism and leaves it disabled.
Verified:
jest tests/federated-proxy.test.ts 21 passed, 21 total
jest --runInBand apps auth appHealth federated-proxy \
notification-proxy app-registry-delegation 111 passed, 111 total
tsc --noEmit clean for these files (2 pre-existing errors in
src/custom-domains/* for an unbuilt workspace package, untouched here)
apps.test.ts showed 6 failures when several suites ran in PARALLEL against the
shared test database, and 41/41 both alone and under --runInBand, which is how
CI runs it. Checked against a reverted index.ts rather than assumed.
NOT verified locally: `helm template`. get.helm.sh is blocked by this
environment's egress policy (403 on CONNECT), so the chart changes rest on CI's
Helm lint & kubeconform gate.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GaPa3JgrVNtWrGvqQEAEqv
izzywdev
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| location /apps/ { | ||
| add_header X-Frame-Options "SAMEORIGIN" always; | ||
| add_header X-Content-Type-Options "nosniff" always; | ||
| add_header Referrer-Policy "strict-origin-when-cross-origin" always; |
| # proxy. MUST be declared, and must sit above `location /`: nginx picks | ||
| # the longest matching prefix, so without this block these requests fall | ||
| # into the SPA fallback and remoteEntry.js comes back as 200 + index.html. | ||
| # A module served as text/html is refused by the browser, which is the |
| location /apps/ { | ||
| add_header X-Frame-Options "SAMEORIGIN" always; | ||
| add_header X-Content-Type-Options "nosniff" always; | ||
| add_header Referrer-Policy "strict-origin-when-cross-origin" always; |
…flow + rate limit CodeQL flagged 3 new alerts (1 high, 2 medium) against the proxy added in 86cf905. Both underlying concerns are real for a new UNAUTHENTICATED endpoint that makes an outbound request, so they are fixed rather than dismissed. 1. Uncontrolled data in a network request (the high one). The upstream host was already constrained by the operator allowlist, but the URL was assembled by string concatenation from `req.url`, so caller-supplied bytes reached the string that becomes an authority. That is a taint flow whether or not it is exploitable today, and "the allowlist makes it fine" is the kind of reasoning that stops being true after one refactor. Replaced with two independent controls, either of which alone would stop it: - buildUpstreamUrl() attaches the path and query through the WHATWG URL API (`target.pathname = ...`), which cannot alter protocol, host or port, and then asserts `target.origin === baseUrl.origin` byte-for-byte, refusing the request if it ever does not hold. - splitRequest() now applies a character WHITELIST per path segment (unreserved + the sub-delims real bundler output uses + percent-escapes) and to the query. ':', '@', '\', '/', '#', whitespace and control characters are refused rather than escaped, because those are precisely the bytes that change how a URL parses. A percent-escape like %5C is deliberately ALLOWED: it stays escaped in the path, cannot affect the authority, and refusing it would break legitimate asset names for no gain. The raw character is refused. Both cases are pinned. 2. Missing rate limiting. `express-rate-limit` was already a dependency of this workspace and used in exactly zero places. This endpoint cannot be authenticated -- a `<script src>` carries no credentials -- and every request costs an outbound in-cluster fetch, so without a ceiling one client can turn the portal into a load generator aimed at another team's service. Applied to this router only. The window is deliberately generous (600/min per IP, both env-overridable): mounting a remote fetches remoteEntry plus every chunk it imports, so a normal page load is a burst of dozens of requests and a limit tuned for a JSON API would break legitimate use. One test expectation changed, and it is a genuine behaviour change worth naming: the request URL is now normalised by the URL API, so a configured `http://host:80` produces `http://host/...`. Those are the same URL. The STORED allowlist value still keeps the operator's `:80` verbatim, so config and logs still read the same -- only the derived request URL is normalised. Also corrected a test that was checking the wrong thing: the "raw backslash" case was asserted over HTTP, but superagent percent-encodes a literal backslash before it leaves the client, so it was exercising the client's encoder rather than this guard. Moved to a direct splitRequest() assertion. Verified: jest tests/federated-proxy.test.ts 29 passed, 29 total jest --runInBand federated-proxy apps auth appHealth \ notification-proxy app-registry-delegation 119 passed, 119 total tsc --noEmit clean (the 2 pre-existing src/custom-domains errors for an unbuilt workspace package are untouched and unrelated) No alert was suppressed, no rule disabled, no baseline edited. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GaPa3JgrVNtWrGvqQEAEqv
…g, nginx headers CORRECTION to the previous commit (48cc7e7). I fixed SSRF and rate limiting before the annotations arrived, reasoning about which rules a proxy would trip. Both changes are worth keeping, but neither was what CodeQL reported. The real alerts, all on ONE line, are: CodeQL 1805 Log injection federatedProxy.ts:278 CodeQL 1804 Externally-controlled format string federatedProxy.ts:278 Semgrep 1827 unsafe-formatstring federatedProxy.ts:278 Line 278 was the upstream-error log: console.error( `[federated-proxy] upstream error for ${parts.slug} (${req.method} ${url}):`, ax.code || ax.message ) Two genuine defects in one statement, and the second is the one I would have missed by eye: 1. Log forging. slug, method and url all originate in the request. A CR/LF in any of them writes what reads as a second, fabricated log entry. 2. Format-string injection. That template literal is console's FORMAT argument. A '%s' inside caller data silently consumes `ax.code` -- so an attacker can not only add a line, they can eat the real diagnostic off the end of it. Fixed everywhere in the file, not just at line 278: every console.* now takes a CONSTANT format string, and every caller-derived value goes through a new logSafe() (collapses \\u0000-\\u001f and \\u007f to a space, caps at 200 chars) and is passed as an ARGUMENT. Three tests pin it, including one asserting the format argument is the fixed literal and contains no slug. Also fixed, from Semgrep on frontend/nginx.conf: 1828/1829 header-redefinition `add_header` in a location block REPLACES the server-level set rather than adding to it. My /apps/ block listed three headers where its siblings list four -- so it would have served every federated asset without X-XSS-Protection. This is the failure mode the rule exists for: the block looked fine, and the omission is invisible unless you diff it against a neighbour. All four are now listed. 481 request-host-used `proxy_set_header Host $host` forwards an attacker-controllable Host to the backend. Removed rather than pinned to a literal: with no Host directive nginx sends the upstream's own name, and this backend routes these requests by path only, so it needs nothing from the original Host. The identical line in the pre-existing /api/ block is NOT touched here -- it is not this PR's, and changing request routing for the API surface is not a change to make as a drive-by. Verified: jest tests/federated-proxy.test.ts 32 passed, 32 total jest --runInBand federated-proxy apps auth appHealth \ notification-proxy app-registry-delegation 122 passed, 122 total tsc --noEmit clean (2 pre-existing src/custom-domains errors untouched) No alert suppressed, no rule disabled, no baseline edited. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GaPa3JgrVNtWrGvqQEAEqv
…d of suppressing it `Lint & Test (24.x)` failed on 8bafbfd with exactly one error, and it is mine: src/routes/federatedProxy.ts 94:14 error Unexpected control character(s) in regular expression: \x00, \x1f no-control-regex logSafe() used a character-class regex to collapse the control characters that would otherwise let a caller forge a log line. Those control characters ARE the point of that function, so this is the case eslint's own docs describe as a legitimate inline disable. Not taken. An `eslint-disable-next-line` is a suppression, and the rule exists because a control character in a regex is nearly always a typo -- so the next person to read a disabled rule has to re-derive why it was safe. Rewritten as a code-point scan, which needs no exemption at all and says what it does: - a run of control characters still collapses to ONE space, so a CR+LF pair cannot become two spaces. The regex's `+` did this; a naive per-character map would not, and would have broken the log-forging test. - still capped at 200 characters - both existing logSafe tests pass unchanged, which is the check that this is behaviour-preserving rather than merely lint-clean The other 10 findings in that job are pre-existing WARNINGS in files this branch does not touch (unused vars in migrations/024, adminPortals, apps, organizations, machine-identity, organizationProvisioning, portalProvisioning). They do not fail the job and are not this PR's to fix. Verified: npx eslint src/routes/federatedProxy.ts clean npm run lint 0 errors, 10 warnings (all pre-existing) jest tests/federated-proxy.test.ts 32 passed, 32 total tsc --noEmit clean Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GaPa3JgrVNtWrGvqQEAEqv
| # the longest matching prefix, so without this block these requests fall | ||
| # into the SPA fallback and remoteEntry.js comes back as 200 + index.html. | ||
| # A module served as text/html is refused by the browser, which is the | ||
| # blank-panel-behind-a-green-healthcheck failure the portal census |
| # into the SPA fallback and remoteEntry.js comes back as 200 + index.html. | ||
| # A module served as text/html is refused by the browser, which is the | ||
| # blank-panel-behind-a-green-healthcheck failure the portal census | ||
| # reports. No Upgrade forwarding — these are static asset fetches. |
Owner
|
On the four unaddressed Two Semgrep findings on
The remaining four annotations — 676, 677, 678 (
Deliberately not fixed here: the same Generated by Claude Code |
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
3 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.
Two independent changes on one branch. Flagging that up front: this PR was opened as the logging fix alone and was green and review-ready before the proxy was added on top. The session is pinned to this branch, so the second change landed here rather than on a branch of its own. Happy to split it out if you'd rather review them separately — say the word.
1. Same-origin
/apps/<slug>/*proxy (the new part)The measurement this exists to move
Portal census run 33009185082, 2026-08-26 20:12Z against
app.fuzefront.com:Only
fuzeserviceis a genuine module-federation pass (remoteEntry + 2 chunk(s) all load as JavaScript).executiveandfuzesocialPASS as bare-URL 200s — iframe-style, not MF.Root cause, and why several rounds of manifest fixes did not move it
frontend/src/utils/loadFederatedApp.ts:71is the entire resolution mechanism:So a registered
/apps/finance/remoteEntry.jsis fetched from the portal's own origin. Nothing ever served that path. No ingress rule matched/apps/*, so it fell through the/catch-all to the frontend, whose SPA fallback answers any unmatched path with200 + index.html.That is exactly what the census reports for
fuzequality:A module served as
text/htmlis refused by the browser. Blank panel, green healthcheck.Why not the mechanism that already existed
.Values.federatedAppsrenders one Ingress per remote and has been[]in every values file since it was written. It cannot work:fuzemarket,fuzequality, …).allowExternalNameServicesis set, and it defaults to false.clockis reachable only because it has a hand-written same-namespace block.It was not misconfigured, it was inapplicable — populating it would not have helped. A reverse proxy has no namespace restriction, since cross-namespace Service DNS is an ordinary HTTP call, and it puts every remote on the portal's origin, which the same-origin / no-mixed-content rule wants anyway.
What landed
backend/src/routes/federatedProxy.tstemplates/ingress.yaml/apps→fuzefront-backend, above the/catch-allfrontend/nginx.conflocation /apps/for the in-pod / port-forward pathvalues.yamlfederatedProxy.enabled+upstreams_helpers.tplTrust model — why an allowlist and not a registry lookup
The registry knows every remote's URL, so deriving the target from it is the obvious shortcut. It is also an SSRF: any product can edit its own manifest via
PUT /apps/{slug}, and a<script src>carries no credentials — so a registered app could aim an unauthenticated proxy at an arbitrary in-cluster address and read the response. The upstream set is therefore exactly what an operator wrote in values.Consequences, all deliberate and all test-pinned:
AuthorizationandCookieare never forwarded upstream.Set-Cookieis dropped — a remote cannot set cookies on the portal origin.maxRedirects: 0)...and%2e%2eare rejected, not normalised.Two things the proxy must not do
Content-Typeare relayed verbatim —Content-Typeis the whole ballgame for Module Federation, and "improving" on it is how the current failure hides.Content-Encoding. axios has already decompressed the body; echoinggziphands the browser plaintext labelled as compressed and breaks every chunk.Defaults, and what is deliberately NOT in this PR
federatedProxy.enabled: false,upstreams: []. With nothing configured every/apps/<slug>/*answers 404 — a truthful "not configured", strictly better than the 200-that-is-HTML it replaces.No prod Service names are guessed. Populating
upstreamsis a separate, operator-owned change; this ships the mechanism with it disabled. Turning it on is what will actually move the census number, and it needs the real Service name/port/namespace for each remote.Error handling is split on purpose: a malformed
FEDERATED_PROXY_UPSTREAMSdrops entries with a loud error rather than crashing, because this router shares a process with auth and billing — but a missingslug/urlfails the Helm render, since a silently dropped remote is indistinguishable from a working one until someone opens the portal.2.
PROVIDER_UNAVAILABLEcovered two opposite conditions and logged neitherPOST /api/v1/security/sessionanswers 503PROVIDER_UNAVAILABLEfor two errors that call for opposite responses:AuthentikUnavailableErrorUnsupportedFlowStageErrorThe response cannot tell them apart, and that is correct —
security-routes.test.tspins that the provider's raw message is never echoed to this unauthenticated endpoint, since it can name internal hosts and flow slugs. So the log is the only place the distinction can live, and this branch logged nothing.What that cost, measured: a monitoring account's flow gained an undriveable stage, the census probe started returning
503 PROVIDER_UNAVAILABLEwith nothing to qualify it, and it was escalated as a production auth outage. It was not one — browser sign-in worked throughout, which the owner established in about thirty seconds by logging in.Logging only: status, code,
Retry-Afterand body are byte-identical.UnsupportedFlowStageError→console.warnnaming the stage and stating it is not an outage;AuthentikUnavailableError→console.errornaming it a platform incident. Newlines are stripped from the provider-derived message so it cannot forge log lines. This is parity with the sibling legacy routeroutes/auth.ts, which has distinguished the two since it was written.🧪 Testing
The three logging tests were each verified to fail against the unpatched source before being kept.
apps.test.tsshowed 6 failures when several suites ran in parallel against the shared test database, and 41/41 both alone and under--runInBand, which is how CI runs it. Checked against a revertedindex.tsrather than assumed.The 2 remaining
tscerrors are pre-existing, insrc/custom-domains/*, for an unbuilt workspace package — untouched here.🚨 Not verified locally
helm templatewas not run.get.helm.shis blocked by this environment's egress policy (403 on CONNECT), so the chart changes rest on CI'sHelm lint & kubeconformgate rather than a local render. Worth a reviewer's eye on_helpers.tpland the new/appsingress path in particular.Breaking changes
None. Both changes are default-off or byte-identical: the proxy ships disabled, and the logging change alters no response byte. No test was weakened or skipped.