Conversation
marekdano
left a comment
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
npm run build fails on this line
| return reply.from(upstreamPath, { | ||
| rewriteRequestHeaders: (_req, headers) => ({ | ||
| ...headers, | ||
| authorization: `Bearer ${bearerToken}`, |
There was a problem hiding this comment.
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]; |
There was a problem hiding this comment.
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.
223df70 to
96e7af6
Compare
|
@vishu-bh @marekdano Addressed! :D |
marekdano
left a comment
There was a problem hiding this comment.
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 startwithout an SPA build serves this shell and every asset 404s → blank page. vite.config.ts'soutDir: "server/public"+emptyOutDir: truedeletes 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
left a comment
There was a problem hiding this comment.
Code looks clean just few gaps needs addressing
| } | ||
|
|
||
| // No CSRF token yet at login, so check Sec-Fetch-Site instead to block cross-site login CSRF. | ||
| function isCrossSiteRequest(request: FastifyRequest): boolean { |
There was a problem hiding this comment.
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.
| 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; |
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
@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
left a comment
There was a problem hiding this comment.
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=falseThe 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
fetchhas no timeout orAbortSignal, so an upstream that accepts the connection and never answers (hung worker, dropped packets — distinct from the connection-refused case covered byauth.test.ts:237) holds the request open until the OS socket timeout; - it is
awaited at line 60 beforedeleteSessionat 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
left a comment
There was a problem hiding this comment.
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 🚀
|
@vishu-bh - please review it too when you have time. |
vishu-bh
left a comment
There was a problem hiding this comment.
LGTM 🚀
Tested out locally runs as expected.
Thanks @gcgoncalves
Running the BFF locally
1. FastAPI (terminal A, repo root)
REDIS_URL=memory:// by default — no Redis process needed for local dev (in-process session store; state resets on restart).
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 unlessPASSWORD_CHANGE_ENFORCEMENT_ENABLED=falseis set in the root .env).Troubleshooting