Bff - #6114
Conversation
a-effort
left a comment
There was a problem hiding this comment.
A few things worth addressing before this merges up. Left them inline; overall this is really solid work and the security core is right. Filed as questions/options rather than blockers.
| const sessionId = request.session!.sessionId; | ||
|
|
||
| return reply.from(upstreamPath, { | ||
| rewriteRequestHeaders: (_req, headers) => ({ |
There was a problem hiding this comment.
Real client IP is lost upstream (audit accuracy). Since every request now originates server-to-server from the BFF, rewriteRequestHeaders here (and the login fetch) don't carry the caller's IP. Upstream email_auth.py derives the client IP from X-Forwarded-For, falling back to request.client.host, so audit logs will record the BFF's IP for every login and every action rather than the real client.
Worth setting x-forwarded-for / x-real-ip from request.ip here and in routes/auth/login.ts. Lockout itself isn't affected (it's keyed per-email, not per-IP), so this is about audit fidelity, not access control.
|
|
||
| subscriber.psubscribe(REVOKED_PATTERN, (err) => { | ||
| if (err) { | ||
| subscriber.emit("error", err); |
There was a problem hiding this comment.
This can crash the process on a subscribe failure. subscriber.emit("error", err) is emitted, but nothing registers an 'error' listener on this connection. A Node EventEmitter throws when 'error' is emitted with no listener, so a Redis hiccup at startup could take the whole BFF down instead of degrading gracefully to the Option-A periodic recheck.
Adding a subscriber.on("error", (err) => { /* log */ }) before psubscribe covers both this path and ordinary connection blips over the connection's lifetime.
| logLevel: optional("LOG_LEVEL", "info"), | ||
| } as const; | ||
|
|
||
| if (process.env.NODE_ENV === "production" && config.redisUrl.startsWith("memory://")) { |
There was a problem hiding this comment.
This guard depends on NODE_ENV, which the start script never sets. "start": "node --env-file-if-exists=.env dist/index.js" doesn't set NODE_ENV, so a prod deploy that forgets to export NODE_ENV=production silently falls back to the in-process memory:// store, quietly losing cross-instance sessions and revocation.
One option is to fail closed on a more reliable signal, e.g. reject memory:// whenever COOKIE_SECURE=true (which prod sets anyway), in addition to the NODE_ENV check.
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.
Potential Issues & Recommendations
Minor Issues
E2E Test Cleanup: Some tests still set sessionStorage.setItem("mcpgateway_token", ...)
- Harmless (ignored by BFF auth), but could be cleaned up for consistency
Recommendations
- Documentation: Add BFF architecture diagram to
docs/ - Monitoring: Add structured logging for session lifecycle events
- Rate Limiting: Consider adding rate limiting to
/auth/login - Session Cleanup: Add background job to clean expired Redis sessions
- CORS: Document CORS requirements if BFF and SPA are on different origins
|
Closed in favour of the new repository. |
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