Skip to content

Support legacy auth env config in HTTP server#691

Open
Tejasvp24 wants to merge 3 commits into
czlonkowski:mainfrom
Tejasvp24:codex/auth-env-compatibility
Open

Support legacy auth env config in HTTP server#691
Tejasvp24 wants to merge 3 commits into
czlonkowski:mainfrom
Tejasvp24:codex/auth-env-compatibility

Conversation

@Tejasvp24

Copy link
Copy Markdown

This fixes #509.

Summary:

  • honor AUTH_ENABLED=false in the single-session HTTP server and skip bearer auth when explicitly disabled
  • honor RATE_LIMIT_ENABLED=false for auth rate limiting
  • accept legacy MAX_SESSIONS and AUTH_MAX_ATTEMPTS aliases alongside the current env names
  • document the compatibility behavior in .env.example
  • add unit coverage for the env compatibility paths

Verification:

  • npx tsc --noEmit --pretty false
  • npx vitest run tests/unit/http-server-n8n-mode.test.ts --coverage.enabled=false

@czlonkowski

Copy link
Copy Markdown
Owner

Thanks @Tejasvp24 — honoring AUTH_ENABLED, RATE_LIMIT_ENABLED, and the legacy MAX_SESSIONS / AUTH_MAX_ATTEMPTS aliases (issue #509) is worth doing, and the parseBooleanEnv/parseNumberEnv helpers and instance-field refactor are clean in isolation. But this is a security-sensitive change and it was written against a stale base, so it needs rework before merge:

Blocking

  1. Auth bypass is wired to the wrong place (security). The PR gates auth on this.authEnabled inside an inline auth block in POST /mcp. Current main no longer has that inline block — it calls a shared authenticateRequest() from five endpoints (POST/GET/DELETE /mcp, GET /sse, POST /messages). After rebase your edit either won't apply or, worse, would disable auth only for POST /mcp while the other four keep enforcing it — an inconsistent, partially-open server. Please move the bypass into authenticateRequest() itself: if (!this.authEnabled) return true; (with a log), so it applies uniformly.
  2. No NODE_ENV guard on disabling auth. AUTH_ENABLED=false unconditionally nulls the token with no production stop. The repo already throws in production for a default token; disabling auth entirely is strictly more dangerous. Please throw when NODE_ENV==='production' (and logger.warn otherwise).
  3. Rate-limiter regression (SSE reconnection loop: POST /mcp handler doesn't route to SSEServerTransport #617). The rewritten authLimiter drops skipSuccessfulRequests: true (added in SSE reconnection loop: POST /mcp handler doesn't route to SSEServerTransport #617) and re-types the old options. Gate at the single shared declaration instead — const authLimiter = this.rateLimitEnabled ? rateLimit({...keep all main options...}) : (req,res,next)=>next(); — keyed only on rateLimitEnabled (decouple it from authEnabled), kept before all route registrations so all five endpoints honor it.

Tests
Add: RATE_LIMIT_ENABLED=false; that AUTH_ENABLED=false bypasses all five endpoints (not just POST /mcp); that it's refused when NODE_ENV=production; and a regression test that auth is still enforced by default.

Please rebase onto current main and re-implement against the authenticateRequest() / shared-limiter architecture. Happy to re-review.

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.

AUTH_MAX_ATTEMPTS and AUTH_ENABLED=false environment variables are ignored

2 participants