Skip to content

feature(ui): BFF - #1

Merged
vishu-bh merged 7 commits into
mainfrom
bff
Aug 12, 2026
Merged

feature(ui): BFF#1
vishu-bh merged 7 commits into
mainfrom
bff

Conversation

@gcgoncalves

Copy link
Copy Markdown
Contributor

Running the BFF locally

1. FastAPI (terminal A, repo root)

cp .env.example .env   # if not already done
make install-dev        # first time only
make dev                 # :8000
  1. BFF (terminal B)
cd client/server
cp .env.example .env
# edit .env: FASTAPI_URL=http://127.0.0.1:8000, COOKIE_SECURE=false for local HTTP
npm install
npm run dev                 # :3000, tsx watch

REDIS_URL=memory:// by default — no Redis process needed for local dev (in-process session store; state resets on restart).

  1. Build the SPA for the BFF to serve
cd client
npm run build:bff        # outputs to client/server/public/, base "/"
Re-run after any client/src change — the BFF serves whatever's on disk, no rebuild-on-save.
  1. Use it

Visit http://127.0.0.1:3000/ — redirects to /app/login (unauthed) or /app/ (authed). Login form posts through the BFF, which holds the FastAPI JWT server-side and hands the browser an opaque session cookie only.

Default seeded admin: admin@example.com / changeme (first login forces a password change unless PASSWORD_CHANGE_ENFORCEMENT_ENABLED=false is set in the root .env).

Troubleshooting

  • EADDRINUSE on :3000 → stale tsx watch process: lsof -ti:3000 | xargs kill, restart pnpm dev.
  • 401 mid-session → normal, token hard-expires per TOKEN_EXPIRY (default 20 min); BFF auto-revokes and redirects to login.

@marekdano marekdano left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues

Functionally-impacting

1. x-forwarded-for / x-real-ip don't carry the real client IP as the comments claim

Files: server/src/routes/proxy/catch-all.ts, server/src/routes/auth/login.ts

Both set "x-forwarded-for": request.ip with the comment "Preserve real client IP for upstream audit logging." But Fastify is constructed without trustProxy, so request.ip is the immediate socket peer. In any real deployment the BFF sits behind an ingress/LB, so request.ip is the LB's address, and any inbound X-Forwarded-For is discarded rather than appended. Upstream audit logs would record the LB IP, not the client — the stated goal isn't met.

Fix: construct Fastify with trustProxy: true (or a trusted CIDR / hop count) so request.ip reflects the parsed XFF chain, then forwarding it upstream is correct. Pair with a note that trustProxy must only be enabled when something trusted actually sits in front.

Suggestions

2. Browser cookies are forwarded upstream

File: server/src/routes/proxy/catch-all.ts

rewriteRequestHeaders spreads ...headers and adds authorization, but headers still includes the browser's Cookie (bff_sid and the HttpOnly bff_csrf secret). Those get sent to FastAPI on every proxied call. Upstream csrf_middleware.py skips CSRF for bearer-token requests (which these always are), so this won't break request handling — but it needlessly ships the BFF session id and CSRF secret to another service where they may land in logs.

Fix: strip cookie (and host) in rewriteRequestHeaders before forwarding — the upstream only needs the injected bearer.

3. /auth/login has no CSRF/origin protection (login CSRF)

File: server/src/routes/auth/login.ts

Login can't require a pre-existing CSRF token, but as-is an attacker page can POST /auth/login with attacker-controlled credentials and silently sign the victim into the attacker's account. Common BFF gap.

Fix: add a same-origin Origin / Sec-Fetch-Site check on the login route, or document the accepted risk.

4. Logout clears bff_csrf without the cookie domain

File: server/src/routes/auth/logout.ts (~line 1056)

reply.clearCookie(CSRF_COOKIE_NAME, { path: "/" }) omits domain, but the CSRF cookie is set with domain: config.cookieDomain. When COOKIE_DOMAIN is configured, this clear won't match and the CSRF cookie lingers. clearSessionCookie already handles this correctly for bff_sid — mirror it (or route the CSRF clear through a shared helper).

5. /auth/session rotates the CSRF secret on every call

File: server/src/routes/auth/session.ts

Each generateCsrf() sets a fresh secret cookie, invalidating tokens already held by other tabs → occasional 403s on concurrent tabs. Not security-relevant, just a UX papercut worth being aware of.

6. upstreamResponse.json() can throw on a 2xx non-JSON body

File: server/src/routes/auth/login.ts (~line 961)

If upstream returns 200 with a non-JSON body, this throws and surfaces as an unhandled 500. A try/catch returning a 502 would be tidier.

Comment thread package.json Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

npm run build fails on this line

@vishu-bh vishu-bh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

worth checking

Comment thread server/src/routes/proxy/catch-all.ts Outdated
return reply.from(upstreamPath, {
rewriteRequestHeaders: (_req, headers) => ({
...headers,
authorization: `Bearer ${bearerToken}`,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ContextForge supports configuring bearer authentication under a header other than Authorization, such as X-MCP-Gateway-Auth. This proxy hardcodes authorization.
When backend uses custom header, login still succeeds because /auth/email/login is unauthenticated, and BFF creates a valid local session. Every subsequent API request then receives 401 because backend cannot find token under configured header.

Knock-on impact:
Entire authenticated UI becomes unusable after apparently successful login.
SSE subscriptions fail.
BFF interprets backend 401 as dead token and deletes otherwise-valid session.
Upstream logout fails to revoke JWT, leaving it valid until expiry.
Please add validated configuration such as FASTAPI_AUTH_HEADER_NAME, and use it consistently in generic proxy, SSE proxy, and logout revocation. Header name should be validated using HTTP token syntax to prevent malformed-header/header-smuggling issues.
Alternatively, explicitly enforce AUTH_HEADER_NAME=Authorization as BFF integration requirement and fail startup when configuration differs.

Related locations needing same fix:

server/src/routes/sse/proxy-sse.ts
server/src/routes/auth/logout.ts

import { getSession, SESSION_COOKIE_NAME } from "../lib/session-store.js";

async function sessionAuth(request: FastifyRequest, reply: FastifyReply): Promise<void> {
const sessionId = request.cookies[SESSION_COOKIE_NAME];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This endpoint returns authenticated user information and current CSRF token, but does not set an explicit cache policy.
ContextForge adds Cache-Control: no-store, private to its protected routes, but /auth/session is BFF-owned and never reaches that middleware.

Knock-on impact:
Browser or intermediary may retain authenticated session metadata.
Cached authenticated response may appear after logout.
CSRF token may remain in browser cache/history-related storage.
Future CDN or reverse-proxy changes could accidentally cache user-specific responses.
Please add:

Cache-Control: no-store, private
Pragma: no-cache
Expires: 0

Same policy should cover BFF login/logout responses. Static hashed assets should remain cacheable.

@gcgoncalves
gcgoncalves marked this pull request as draft August 11, 2026 13:18
@gcgoncalves
gcgoncalves marked this pull request as ready for review August 11, 2026 13:18
@gcgoncalves
gcgoncalves force-pushed the bff branch 2 times, most recently from 223df70 to 96e7af6 Compare August 11, 2026 14:04
@gcgoncalves

Copy link
Copy Markdown
Contributor Author

@vishu-bh @marekdano Addressed! :D

@marekdano marekdano left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please comment if you think we can address it later.

Four items below fall inside the security boundary this PR sets out to establish, which is why I've marked them blocking rather than follow-ups.


Blocking

1. Upstream Set-Cookie is relayed verbatim to the browser

server/src/routes/proxy/catch-all.ts:111

reply-from does copyHeaders(rewriteHeaders(res.headers, req), reply) for every upstream header, and rewriteUpstreamLocation only touches location. The proxy carefully strips the browser's Cookie on the way up (line 102) but does nothing on the way down. mcpgateway uses a jwt_token cookie — any proxied endpoint that issues one plants it on the browser, putting the API JWT in the browser and defeating the PR's stated core guarantee ("the API JWT never reaches the browser").

Fix belongs in rewriteUpstreamLocation, which already owns response-header rewriting:

const { "set-cookie": _dropped, ...safe } = headers;

Please add a test asserting an upstream Set-Cookie never reaches the client.

2. trustProxy defaults to true

server/src/config.ts:41

The comment already says "only safe behind a trusted proxy," so the default contradicts the code's own reasoning. With the default HOST=0.0.0.0 and no LB in front, any client sets X-Forwarded-For and controls request.ip — which is forwarded upstream as x-forwarded-for/x-real-ip at login.ts:55-56 and catch-all.ts:107-108. A login brute-forcer can rotate the apparent IP to evade mcpgateway's per-IP lockout, and audit logs record forged IPs.

Should default to false, opt in via TRUST_PROXY=true.

3. SSE routes convert a browser GET into an authenticated upstream POST, with CSRF disabled

server/src/routes/sse/routes.ts:16

/api/resources/subscribe is registered as a browser GET that the BFF turns into an upstream POST (proxy-sse.ts:47). The rationale for skipping CSRF (proxy-sse.ts:13-16 — "these routes are GET/read-only") is invalidated by that conversion. The session cookie is SameSite=Lax (session-store.ts:96), which is sent on cross-site top-level GET navigations, so an attacker page doing window.open('https://bff/api/resources/subscribe') — or a plain link — produces an authenticated state-changing POST upstream.

Minimum: apply the same Sec-Fetch-Site !== "cross-site" guard that login.ts:31 already uses to all SSE routes. Better: SameSite=Strict on the session cookie, or don't expose POST upstreams behind a GET.

4. server/public/index.html is a committed build artifact that cannot work

server/public/index.html:1

It references /assets/index-CetL4HrZ.js and the other hashed bundles, but .gitignore excludes server/public/assets/, so those files aren't in the repo. It also still points at /static/favicon.ico, which this same PR moved to /favicon.ico (index.html:7, new public/favicon.ico).

  • Fresh clone → cd server && npm ci && npm start without an SPA build serves this shell and every asset 404s → blank page.
  • vite.config.ts's outDir: "server/public" + emptyOutDir: true deletes and regenerates this tracked file on every build → permanent git churn.

Add server/public/ to .gitignore instead.


Should fix

5. CSRF secret is not rotated at login

server/src/routes/auth/login.ts:96

@fastify/csrf-protection's generateCsrfCookie only mints and sets a new secret when request.cookies[cookieKey] is falsy, so an existing bff_csrf survives the authentication boundary. Anyone able to set a cookie for the site (subdomain XSS, or any plaintext hop when COOKIE_SECURE=false) plants a known secret; the victim logs in, the secret is kept, and the attacker can mint valid X-CSRF-Token values for the victim's authenticated session.

Clear the cookie before generating:

reply.clearCookie(CSRF_COOKIE_NAME, { path: "/", domain: config.cookieDomain });
const csrfToken = await reply.generateCsrf();

6. SPA-fallback extension heuristic breaks client routes containing a dot

server/src/plugins/static.ts:42

/\.[a-zA-Z0-9]+$/ matches any path whose final segment has a dot, and those get a JSON 404 instead of the app shell. /app/reset-password/:token (src/App.tsx:48) breaks if the token contains a dot (JWT-style, or any base64url token with one) — the user clicking the emailed reset link gets {"error":"Not Found"}.

Suggest inverting the test: serve the shell for everything except known asset prefixes (/assets/, /favicon.ico, …), rather than guessing from a trailing extension.

7. Unguarded upstream fetch in login

server/src/routes/auth/login.ts:50

No try/catch, unlike logout.ts's revokeUpstreamToken and unlike proxy-sse.ts, which returns 502 upstream_unavailable for the same condition. With FastAPI down, TypeError: fetch failed reaches Fastify's default handler and the login form gets an opaque 500.

8. No validation that access_token is present on a 2xx upstream response

server/src/routes/auth/login.ts:87

expires_in is defensively validated two lines above but the token itself isn't. An unexpected 2xx shape creates a session with bearerToken: undefined; JSON.stringify drops the key, every proxied call sends Bearer undefined → 401 → the catch-all's revoke-and-clear path fires. The user sees a login that appears to succeed and then immediately bounces back to login, with nothing logged server-side.

@vishu-bh vishu-bh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks clean just few gaps needs addressing

Comment thread server/src/routes/auth/login.ts Outdated
}

// No CSRF token yet at login, so check Sec-Fetch-Site instead to block cross-site login CSRF.
function isCrossSiteRequest(request: FastifyRequest): boolean {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sec-Fetch-Site === "cross-site" blocks unrelated sites, but permits sibling origins. For example, a request from evil.example.com to app.example.com is reported as same-site and passes this guard. Missing Sec-Fetch-Site also passes.
Therefore login-CSRF/session swapping remains possible if attacker controls another origin under same registrable domain.
Please validate Origin against configured public BFF origin. Sec-Fetch-Site can remain as defense in depth, but should not replace exact-origin validation.

Comment thread server/src/routes/proxy/catch-all.ts Outdated
return reply.from(upstreamPath, {
rewriteRequestHeaders: (_req, headers) => {
// Drop browser Cookie — bff_sid/bff_csrf are BFF-only secrets, upstream only needs the bearer.
const { cookie: _cookie, ...forwarded } = headers;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cookie is now removed, but remaining spread still forwards headers such as forwarded, x-forwarded-host, x-forwarded-proto, and x-forwarded-port. ContextForge trusts forwarded-host/proxy metadata for request URL construction, including OAuth-related URLs.
When FASTAPI_AUTH_HEADER_NAME is customized, browser-provided Authorization also remains alongside BFF-injected custom auth header.

Please strip all inbound infrastructure/authentication headers before adding BFF-owned values:

authorization
configured upstream auth-header name
forwarded
x-forwarded-for
x-forwarded-host
x-forwarded-proto
x-forwarded-port
x-real-ip

@marekdano marekdano left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gcgoncalves - thanks for addressing the listed issues

There are two issues should be fixed

Blocking

1. PUBLIC_ORIGIN= in .env.example breaks login with a 403

server/.env.example:36, server/src/lib/origin-guard.ts:34

origin-guard.ts reads config.publicOrigin ?? `${request.protocol}://${request.host}` .
?? falls back on null/undefined only — not on "". .env.example ships
PUBLIC_ORIGIN= (empty), and both npm run dev and npm start load .env via
--env-file-if-exists, so process.env.PUBLIC_ORIGIN === "" and expected becomes the empty string. Every request carrying an Origin header then mismatches.

Browsers always send Origin on POST, including same-origin. So following the
documented setup — cp .env.example .env, edit FASTAPI_URL and COOKIE_SECURE — makes POST /auth/login return 403 cross_site_request_forbidden. SSE routes fail the same way.

Confirmed by direct probe against the real module: isForbiddenCrossOrigin() on a
same-origin POST with PUBLIC_ORIGIN="" returns true.

Fix: normalize empty env values to undefined in config.ts (or use ||).
COOKIE_DOMAIN= on line 22 is the same latent shape — currently harmless only because the cookie serializer truthy-checks domain.

2. The BFF test suite is red — npm test fails 18 of 21

server/src/config.ts:69-74, server/vitest.config.ts

The fail-closed guard added in 96e7af60 ("Address comments") throws at module
import
whenever REDIS_URL=memory:// and COOKIE_SECURE is truthy. COOKIE_SECURE defaults to "true" and the tests never override it, so four of five test files fail to import. COOKIE_SECURE=false npx vitest run passes 33/33.

The guard itself is good — it just needs the test env to opt out.

Fix: env: { COOKIE_SECURE: "false" } in server/vitest.config.ts.

Non-blocking - can be addressed or as follow-ups

3. Nothing in CI runs the BFF

.github/workflows/, vitest.config.ts:23

client-lint-test.yml runs only root scripts (generate, format:check, lint,
test:coverage). The root vitest config includes src/** only, so server/test/**
is never swept up. client-e2e.yml stubs the API with page.route() and never
starts the BFF.

2410 lines of session/CSRF/proxy code with zero CI — which is exactly why #2 went
unnoticed. Add a server-lint-test.yml running npm ci && npm run lint && npm run test:run in server/.

4. The default production config breaks the origin guard

server/src/config.ts:38,47,52

Defaults are COOKIE_SECURE=true, TRUST_PROXY=false, PUBLIC_ORIGIN unset. Behinda TLS-terminating load balancer — the normal production shape — request.protocol is http while the browser sends Origin: https://…, so the guard mismatches and login and SSE 403. The config comments describe this hazard accurately but nothing enforces it, and the failure surfaces as an opaque cross_site_request_forbidden.

Fix: fail fast (or warn loudly) at boot when cookieSecure && !publicOrigin && !trustProxy.

5. server/public/index.html is a committed stale build artifact

server/public/index.html

It's Vite output: it references content-hashed bundles (/assets/index-CetL4HrZ.js, vendor-lucide-BscrWGY9.js) that aren't in the repo, and it still points at /static/favicon.ico — the exact path this PR corrects in the root index.html. Meanwhile the same PR gitignores server/public/ wholesale, which doesn't untrack it.

Two consequences: vite build (emptyOutDir: true) deletes it on first build, leaving a spurious deleted: in git status; and a fresh clone that starts the BFF before building the SPA serves this shell, 404ing every /assets/* for a blank page.

Fix: git rm --cached server/public/index.html.

Signed-off-by: Gabriel Costa <gabrielcg@proton.me>
Signed-off-by: Gabriel Costa <gabrielcg@proton.me>
Signed-off-by: Gabriel Costa <gabrielcg@proton.me>
Signed-off-by: Gabriel Costa <gabrielcg@proton.me>

@marekdano marekdano left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two remaining issues

1. server/.env.example — copying the example makes the server refuse to boot

REDIS_URL=memory:// (line 15) plus COOKIE_SECURE=true (line 24) trips the fail-closed guard in config.ts:76-81:

REDIS_URL=memory:// is dev-only — set a real redis:// URL in production

Even after switching REDIS_URL, the same file trips the second guard at config.ts:94 because COOKIE_SECURE=true is paired with an empty PUBLIC_ORIGIN and TRUST_PROXY=false. So the README's own quick-start — cd server && cp .env.example .env && npm run dev — exits immediately, twice in a row. Verified by loading src/config.ts with --env-file=.env.example.

The guards themselves are right and worth keeping; only the checked-in dev template is wrong.

Suggestion:

-# Set to "false" only for local HTTP development. Must be "true" (default) in prod.
-COOKIE_SECURE=true
+# "false" is the local-HTTP dev value, and is what this file ships with so a
+# fresh `cp .env.example .env` boots against the REDIS_URL=memory:// default
+# above. Set to "true" in prod — config.ts fails closed on COOKIE_SECURE=true
+# paired with either memory:// or an unset PUBLIC_ORIGIN/TRUST_PROXY, so a prod
+# deployment must set REDIS_URL and PUBLIC_ORIGIN (or TRUST_PROXY) alongside it.
+COOKIE_SECURE=false

The default in config.ts:45 stays "true", so an environment with nothing set is still fail-closed.

2. server/src/routes/auth/logout.ts:32 — a hung upstream blocks logout indefinitely

The comment above revokeUpstreamToken says the call is best-effort and "must not block the BFF-side logout the user is waiting on", but the implementation does both of the things that would block it:

  • the fetch has no timeout or AbortSignal, so an upstream that accepts the connection and never answers (hung worker, dropped packets — distinct from the connection-refused case covered by auth.test.ts:237) holds the request open until the OS socket timeout;
  • it is awaited at line 60 before deleteSession at line 62, so while it hangs the Redis session is never dropped and the cookies are never cleared. The user appears to still be logged in.

Suggestion:

+// The user is waiting on this request, so cap how long a hung (not refused)
+// upstream can hold it open.
+const UPSTREAM_REVOKE_TIMEOUT_MS = 3000;
+
 async function revokeUpstreamToken(request: FastifyRequest, bearerToken: string): Promise<void> {
   try {
     const response = await fetch(`${config.fastapiUrl}/auth/logout`, {
       method: "POST",
       headers: upstreamAuthHeader(bearerToken),
+      signal: AbortSignal.timeout(UPSTREAM_REVOKE_TIMEOUT_MS),
     });
       const sessionId = request.cookies[SESSION_COOKIE_NAME];
       if (sessionId) {
         const record = await getSession(fastify.redis, sessionId);
+        // Drop the BFF session first: the upstream revoke is best-effort and
+        // must not leave a live session behind if it stalls or throws.
+        await deleteSession(fastify.redis, sessionId);
         if (record) {
           await revokeUpstreamToken(request, record.bearerToken);
         }
-        await deleteSession(fastify.redis, sessionId);
       }

The timeout rejection lands in the existing catch and logs as upstream token revocation failed, so no new error path. AbortSignal.timeout needs Node 17.3+; CI pins Node 22. Both existing logout tests still pass, since record is read before the delete.

Signed-off-by: Gabriel Costa <gabrielcg@proton.me>
Add a 3s AbortSignal.timeout to the upstream /auth/logout fetch and
delete the BFF session before awaiting the revoke, so a hung (not
refused) upstream can no longer hold the Redis session and cookies
open indefinitely.

Also fix .env.example: COOKIE_SECURE=true paired with the default
REDIS_URL=memory:// and empty PUBLIC_ORIGIN tripped config.ts's
fail-closed guards, so `cp .env.example .env && npm run dev` exited
immediately. Ship COOKIE_SECURE=false to match the dev defaults and
document the prod requirements inline.

Signed-off-by: Gabriel Costa <gabrielcg@proton.me>
- login: warn-log when upstream expires_in is invalid, before falling
  back to the BFF default session TTL
- session-store: configurable REDIS_KEY_PREFIX instead of hardcoded
  "bff:" (also fixes revocation-subscriber's pattern to match)
- proxy-sse: jitter the SSE session-revocation recheck interval
- index: close sseUpstreamPool on shutdown
- package.json: pin engines.node >=18 (AbortSignal.timeout requirement)
- tests: CSRF secret rotation on login, config validation (memory
  Redis in prod, COOKIE_SECURE without PUBLIC_ORIGIN/TRUST_PROXY)

Signed-off-by: Gabriel Costa <gabrielcg@proton.me>

@marekdano marekdano left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No major issues. The PR looks good!

Tested locally with npm run dev and npm run start under the server folder and it works as expected.

Happy to LGTM 🚀

@marekdano

Copy link
Copy Markdown
Contributor

@vishu-bh - please review it too when you have time.

@vishu-bh vishu-bh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

Tested out locally runs as expected.
Thanks @gcgoncalves

@vishu-bh
vishu-bh merged commit e23ac0b into main Aug 12, 2026
5 checks passed
@gcgoncalves
gcgoncalves deleted the bff branch August 12, 2026 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants