Skip to content

feature(ui): BFF - #1

Open
gcgoncalves wants to merge 2 commits into
mainfrom
bff
Open

feature(ui): BFF#1
gcgoncalves wants to merge 2 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

Choose a reason for hiding this comment

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

worth checking

return reply.from(upstreamPath, {
rewriteRequestHeaders: (_req, headers) => ({
...headers,
authorization: `Bearer ${bearerToken}`,

Copy link
Copy Markdown

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

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
Signed-off-by: Gabriel Costa <gabrielcg@proton.me>
Signed-off-by: Gabriel Costa <gabrielcg@proton.me>
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