feat(auth): opt-in trusted reverse-proxy header authentication#799
Open
sntgl wants to merge 2 commits into
Open
feat(auth): opt-in trusted reverse-proxy header authentication#799sntgl wants to merge 2 commits into
sntgl wants to merge 2 commits into
Conversation
When deployed behind an authenticating reverse proxy doing forward-auth (Authelia, oauth2-proxy, Authentik, Cloudflare Access, ...), CloudCLI can trust the proxy-vouched username header instead of requiring a second login. Opt-in via TRUSTED_PROXY_AUTH; off by default => behavior is unchanged. - server/middleware/auth.js: resolveTrustedProxyUser(req) honors TRUSTED_PROXY_USER_HEADER (default Remote-User) only when the request's direct socket peer is within TRUSTED_PROXY_CIDRS (default loopback-only). Anti-spoof is anchored on socket.remoteAddress, never X-Forwarded-For. Single-user invariant preserved: match the existing user or provision the first one; never silently create additional users (mirrors /register). - server/routes/auth.js: /api/auth/status issues a JWT when a trusted identity is present so the SPA skips the login screen. - websocket-auth.service.ts + server/index.js: WS upgrade auth honors the same header via the existing dependency-injection seam (verifyClient). - AuthContext/types: frontend adopts the status-issued token. - node:test units for the CIDR / anti-spoof boundary. Mirrors the established pattern in Gitea (ENABLE_REVERSE_PROXY_AUTHENTICATION + REVERSE_PROXY_TRUSTED_PROXIES) and Grafana ([auth.proxy] + whitelist).
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds opt-in trusted reverse-proxy header authentication to both HTTP and WebSocket. Incoming requests from allowed CIDR ranges can authenticate via a configured user identity header; the first such user is auto-provisioned with a secure bcrypt hash. The ChangesTrusted Reverse-Proxy Header Authentication
Possibly Related Issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
|
Hey @sntgl, thanks for the pr. Is it ready for review or should we close this? |
Author
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/middleware/auth.trusted-proxy.test.js (1)
1-46: ⚡ Quick winConsider adding tests for
resolveTrustedProxyUser.The helper tests cover CIDR matching and anti-spoofing boundaries well. However,
resolveTrustedProxyUser(the main entry point used byauthenticateTokenand/status) lacks test coverage for key scenarios:
- User lookup when header is present but user exists/doesn't exist
- Single-user invariant enforcement (different proxy identity after first user)
- Header absent or non-string values
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/middleware/auth.trusted-proxy.test.js` around lines 1 - 46, Add unit tests for resolveTrustedProxyUser to cover: (1) when the trusted identity header is present and maps to an existing user (mock or seed the user lookup to return a user) and when it does not exist (ensure it returns null or creates/handles missing user per implementation); (2) the single-user invariant by first resolving one trusted identity then attempting to resolve a different identity and asserting the function enforces the invariant (rejects or returns the original user); and (3) header absent and non-string header values (undefined, null, numeric) to confirm it returns null/does not throw. Use the same test harness setup as the existing tests (process.env JWT_SECRET/TRUSTED_PROXY_*, import resolveTrustedProxyUser from './auth.js') and reference resolveTrustedProxyUser and authenticateToken behaviors to mirror real usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@server/middleware/auth.trusted-proxy.test.js`:
- Around line 1-46: Add unit tests for resolveTrustedProxyUser to cover: (1)
when the trusted identity header is present and maps to an existing user (mock
or seed the user lookup to return a user) and when it does not exist (ensure it
returns null or creates/handles missing user per implementation); (2) the
single-user invariant by first resolving one trusted identity then attempting to
resolve a different identity and asserting the function enforces the invariant
(rejects or returns the original user); and (3) header absent and non-string
header values (undefined, null, numeric) to confirm it returns null/does not
throw. Use the same test harness setup as the existing tests (process.env
JWT_SECRET/TRUSTED_PROXY_*, import resolveTrustedProxyUser from './auth.js') and
reference resolveTrustedProxyUser and authenticateToken behaviors to mirror real
usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1e982861-4c1b-481e-aa09-423f8a1da562
📒 Files selected for processing (7)
server/index.jsserver/middleware/auth.jsserver/middleware/auth.trusted-proxy.test.jsserver/modules/websocket/services/websocket-auth.service.tsserver/routes/auth.jssrc/components/auth/context/AuthContext.tsxsrc/components/auth/types.ts
Address CodeRabbit nitpick: the trusted-proxy tests covered the CIDR / anti-spoofing boundary but not resolveTrustedProxyUser, the entry point used by authenticateToken and /status. Add cases for first-user provisioning, existing-user lookup (no duplicate), the single-user invariant against a different proxy identity, absent / non-string identity headers, and a valid header arriving from an untrusted source. Uses a throwaway SQLite database per case (same isolation harness as the sessions.db integration test). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the RFC in #798. Opt-in, off by default — when
TRUSTED_PROXY_AUTHis unset, behavior is identical to upstream.What
Behind an authenticating reverse proxy (Authelia / oauth2-proxy / Authentik / Cloudflare Access) doing forward-auth, CloudCLI trusts the proxy-vouched username header instead of requiring a second login.
How
server/middleware/auth.js—resolveTrustedProxyUser(req)honorsTRUSTED_PROXY_USER_HEADER(defaultRemote-User) only when the request's direct socket peer is withinTRUSTED_PROXY_CIDRS(default loopback-only). Anti-spoof anchored onsocket.remoteAddress, neverX-Forwarded-For. Single-user invariant preserved (match existing user or provision the first; never silently multi-create). Wired intoauthenticateToken.server/routes/auth.js—/api/auth/statusissues a JWT when a trusted identity is present, so the SPA skips the login screen.server/modules/websocket/services/websocket-auth.service.ts+server/index.js— WS upgrade auth honors the same header via the existing DI pattern (verifyClientalready receivesinfo.req).src/components/auth/{context/AuthContext.tsx,types.ts}— frontend adopts the/status-issued token.server/middleware/auth.trusted-proxy.test.js—node:testunits for the CIDR / anti-spoof boundary.Config
TRUSTED_PROXY_AUTHfalseTRUSTED_PROXY_USER_HEADERRemote-UserTRUSTED_PROXY_CIDRS127.0.0.0/8,::1/128Test plan
node --import tsx --test server/middleware/auth.trusted-proxy.test.js:3001without the header / from outside the CIDR → rejected (falls back to normal JWT login).Opening as draft per the "discuss first" note in CONTRIBUTING (see #798) — happy to iterate on the open questions there or hold until you've weighed in.
Summary by CodeRabbit
New Features
Tests