Repository navigation
Security hardening: SSRF guards, bounded rate-limiter memory, OAuth token lifetime & scoped revocation - #4
Conversation
Introduce a single server-side policy (netbird/apiUrlPolicy) that decides whether a NetBird API base URL is permitted — the shared foundation the SSRF fixes for the direct-PAT path (#12) and the OAuth login form (#13) will adopt. - checkApiUrl gates a base URL against an operator host allowlist and denies private/loopback/link-local/metadata IP literals that are not explicitly allowlisted; explicit operator opt-in wins over the floor. - Config derives the allowlist from NETBIRD_API_URL plus the new NETBIRD_ALLOWED_API_HOSTS, and validates the configured URL at startup, failing fast with a clear message. - Both entrypoints refuse to boot on an unusable NETBIRD_API_URL via a shared loadConfigOrExit helper. Prefactor only: request-supplied URLs (the URL header and login form) are not yet wired to the policy — that lands in #12/#13.
#12) The direct-PAT auth path accepted any request with a token header present, took the base URL from the caller's x-netbird-api-url header with no restriction, and was reachable by fall-through even with OAuth enabled — an unauthenticated SSRF relay against internal/metadata hosts. Close it on three fronts: - resolveAuth now gates the caller-supplied base URL through the #11 host allowlist before any outbound call (forbidden_host), and verifies the PAT against NetBird before trusting it (invalid_token). "unknown" verification fails open by design (host already allowlisted; NetBird rejects a bogus token on the real call) and is logged, never silent. - The direct-PAT header path is now off by default when OAuth is enabled; opt in with NETBIRD_ENABLE_DIRECT_PAT. It stays on when OAuth is disabled (only auth path). Documented in README and .env.example. - Extract a shared netbird/client verifyPat helper so the OAuth login flow and the direct-PAT path build the verification client one way. Verified at the wire against a fake NetBird API: default config rejects a metadata-targeting request without any outbound call; with direct-PAT on, a disallowed host is refused before contact, an invalid token is rejected after verification, and a valid token passes.
…the login route (#13) The login flow validated the browser-submitted netbird_api_url with only a bare http(s) scheme check, then (with PAT verification on by default) fired a real request to that host — an SSRF whose 401-vs-other response is an internal port-scan oracle, reachable by any anonymous caller since client registration is open. And /oauth/netbird-login, an app-added route, had no rate limiting unlike the SDK's own auth routes. - completeLogin now runs the caller-submitted base URL through the #11 host allowlist (checkApiUrl) as its FIRST check — before the client lookup and before any verification fetch — so a disallowed host is refused without any outbound request. Removes the old isHttpUrl helper (checkApiUrl subsumes it). - Thread config.allowedApiHosts into OAuthCore. - Mount an express-rate-limit limiter on POST /oauth/netbird-login, matching the SDK's /authorize limiter (15 min / 100 per IP). New direct dependency. - README: note trust-proxy so per-IP limits key on the real client behind a proxy. Verified at the wire: a metadata/disallowed host is refused in ~15ms (no outbound attempt), the real route carries RateLimit headers, and a flood past the cap returns 429.
Closes #14. LimiterPool retained a RateLimiter for every distinct token hash it ever saw, with no eviction. A caller rotating the token value each request grew the map without bound — a memory-exhaustion DoS needing no valid credentials. Bound the pool two ways: - Idle TTL reclaims limiters untouched longer than idleTtlMs. Lossless for rate-limiting because the constructor forces the TTL to be at least the 60s rate-limit window (single-sourced as RATE_LIMITER_WINDOW_MS), so a reclaimed limiter has already let its budget age out. - A hard LRU size cap is the backstop against a flood that mints keys faster than the TTL reclaims. The map is kept in ascending last-access order so both eviction paths read the LRU entry from the head. The constructor fails fast on config that would silently break the pool (sub-1 cap, or a TTL below the window). Active tokens keep their limiter and count across requests (verified); a 10k-distinct-key test and a wire test driving the real HTTP entrypoint's direct-PAT path under a rotating-token flood confirm the bound holds and the server stays live.
…er-source login limiting Closes #15. Closes #16. #15 — expire OAuth refresh tokens: Refresh records carried no expiry and getRefresh did no TTL check, so a leaked refresh token could mint access tokens indefinitely. Refresh tokens now get a bounded expiresAt at issuance (30 days) and are refused-and-dropped on lookup, mirroring access tokens. Rotation re-stamps the window, so only idle tokens expire; a leaked used token is still caught by rotation-replay detection. getAccess/getRefresh now share a getLive() helper (they were byte-identical). #16 — revocation binding + per-source login rate limiting: - revokeToken now only deletes a token that belongs to the requesting client (RFC 7009); another client's or an unknown token is a silent no-op that still reports success, via a new store.revokeForClient. The unconditional primitive stays for internal refresh rotation. - Login PAT verification gained a per-source sub-limiter (a LimiterPool keyed by client IP, cap = NETBIRD_MAX_RPM/10) acquired BEFORE the shared global limiter, so a noisy source stalls on its own budget without consuming the shared one — other tenants' logins still get through. Review-driven fixes: - Per-IP limiting is only correct if req.ip is the real client IP. Added a NETBIRD_TRUST_PROXY knob wired to Express `trust proxy` (hop count / bool / preset); without it, behind a proxy every client collapsed into one bucket and the sub-limiter would throttle everyone below the global cap. This also makes the pre-existing OAuth-route and login-form per-IP limiters correct behind a proxy. Documented in README + .env.example. - LimiterPool generalized from token-only to any opaque caller key (it now serves both the per-tenant tool-call pool and the per-source login pool). Verified: 159 unit/e2e tests + tsc green; wire tests drive the real HTTP entrypoint for client-scoped /revoke (cross-client no-op, owner deletes) and for trust-proxy per-source isolation (X-Forwarded-For sources get isolated budgets).
📝 WalkthroughWalkthroughThis change adds configurable API host allowlisting, explicit Direct-PAT authentication and verification, proxy-aware login rate limiting, bounded per-source limiter pools, refresh-token expiry, and client-scoped OAuth token revocation. ChangesDirect PAT and API trust policy
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTPServer
participant resolveAuth
participant verifyDirectPat
participant NetBirdAPI
Client->>HTTPServer: POST /mcp with Direct-PAT and baseUrl
HTTPServer->>resolveAuth: resolveAuth(headers, auth options)
resolveAuth->>resolveAuth: checkApiUrl(baseUrl, allowedApiHosts)
resolveAuth->>verifyDirectPat: verifyPat(auth)
verifyDirectPat->>NetBirdAPI: verifyToken()
NetBirdAPI-->>verifyDirectPat: TokenVerification
verifyDirectPat-->>resolveAuth: verification result
resolveAuth-->>HTTPServer: AuthContext or AuthError
HTTPServer-->>Client: MCP response or authentication error
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/config.ts (1)
80-94: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
NETBIRD_TRUST_PROXY=trueis a known rate-limit-bypass footgun; nothing here warns against it.parseTrustProxypasses a bare"true"through as booleantrue, which becomes Express'strust proxysetting used by the new per-IP login/PAT rate limiters. express-rate-limit's own validation flags this exact configuration: trustProxy(request) { if (request.app.get("trust proxy") === true) { throw new ValidationError( "ERR_ERL_PERMISSIVE_TRUST_PROXY",The Express 'trust proxy' setting is true, which allows anyone to trivially bypass IP-based rate limiting.); } } — a client can spoofX-Forwarded-Forto defeat the limiter entirely. Both docs and the parser accepttruewith no caveat steering operators toward a hop count or named preset instead.
src/config.ts#L80-L94: haveparseTrustProxyreject/warn on a bare booleantrue(require a hop count or preset), or at minimum log a warning when it's chosen..env.example#L26-L28: add a line notingtruetrusts all proxies and can be spoofed — recommend a hop count (e.g.1) or preset instead.README.md#L131-L141: add the same caveat to theNETBIRD_TRUST_PROXYtable row.🤖 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 `@src/config.ts` around lines 80 - 94, Update parseTrustProxy to reject or warn when NETBIRD_TRUST_PROXY is set to bare boolean true, directing operators toward a hop count or named preset; preserve other supported values. In .env.example lines 26-28, document that true trusts all proxies and allows spoofing, recommending a hop count such as 1 or a preset. In README.md lines 131-141, add the same caveat to the NETBIRD_TRUST_PROXY documentation.src/bin/http.ts (1)
73-73: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider adding Helmet for baseline security headers.
Static analysis flags the Express app as missing Helmet. Orthogonal to this PR's SSRF/auth focus, but cheap to add for a public-facing HTTP server.
🛡️ Example
+import helmet from "helmet"; ... const app = express(); +app.use(helmet());🤖 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 `@src/bin/http.ts` at line 73, Add Helmet middleware to the Express application initialized as app in src/bin/http.ts, import the existing Helmet package or add it as a dependency if necessary, and register it early in the middleware chain so baseline security headers apply to all responses.Source: Linters/SAST tools
src/oauth/loginRateLimit.ts (1)
17-25: 🩺 Stability & Availability | 🔵 TrivialIn-memory store won't scale the login limiter across replicas.
rateLimit()here uses the default in-memoryMemoryStore, which doesn't share hit counts across processes. In a horizontally scaled HTTP/cloud deployment (this PR's target), the effective login-attempt cap becomesLOGIN_RATE_LIMIT_MAX × replica count, weakening the anti-abuse protection this middleware is meant to add. Consider documenting this constraint or supporting a shared store (e.g.,rate-limit-redis) for multi-instance deployments.🤖 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 `@src/oauth/loginRateLimit.ts` around lines 17 - 25, The loginRateLimiter implementation uses the default in-memory store, so limits are not shared across replicas. Update loginRateLimiter to support a shared production store such as rate-limit-redis, while preserving the existing in-memory behavior for local or single-instance usage; alternatively, clearly document this deployment constraint if shared-store support is outside scope.src/auth/resolve.ts (1)
83-93: 🚀 Performance & Scalability | 🔵 TrivialDirect-PAT path now costs 2x the tenant's rate budget per request.
verifyPatruns on every direct-PAT-authenticated request (not just at login), andhttp.ts'sverifyDirectPatdeliberately reuses the same per-tenantRateLimiteras the actual downstream NetBird call. That means each tool call now consumes two slots ofmaxRequestsPerMinuteinstead of one, effectively halving direct-PAT throughput versus the OAuth path. This is clearly an intentional, documented tradeoff, but operators relying on direct-PAT for "simple deploys" should be aware their effective request budget is halved — worth a callout in README/config docs if not already there.🤖 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 `@src/auth/resolve.ts` around lines 83 - 93, Document the direct-PAT rate-limit behavior near the configuration or authentication guidance: because resolve’s verifyPat invokes http.ts’s verifyDirectPat using the same per-tenant RateLimiter as the downstream request, each direct-PAT request consumes two rate-limit slots. Call out that direct-PAT throughput is effectively half the configured maxRequestsPerMinute compared with OAuth, without changing the authentication flow.src/oauth/store.ts (1)
34-61: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftConsider bounding OAuthStore's token maps the same way LimiterPool bounds rate limiters.
getLiveonly prunes an expired record when that exact key is looked up again — there's no active sweep or size cap onaccess/refresh(orcodes/clients). A token that's issued and never looked up again (abandoned session, or a client repeatedly completing dynamic-registration + login flows) sits in memory for its full TTL — up to 30 days for refresh tokens now — with no ceiling on total record count. This PR explicitly adds a size-capped, idle-sweptLimiterPoolto solve exactly this class of unbounded-growth risk for rate limiters; the same multi-tenant hardening rationale applies toOAuthStore.This isn't a regression (refresh tokens previously had no expiry at all, so this PR already improves the situation), but it's worth closing the gap given the PR's stated focus on bounding per-tenant memory in cloud/multi-tenant deployments.
Also applies to: 102-109, 111-117
🤖 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 `@src/oauth/store.ts` around lines 34 - 61, Harden OAuthStore’s access, refresh, codes, and clients maps against unbounded growth, rather than relying on getLive for key-specific expiry cleanup. Add a bounded, idle-swept storage mechanism analogous to LimiterPool, enforcing a maximum record count and removing expired or idle entries without requiring lookup. Preserve the existing TTL and token issuance/lookup behavior while applying the bounds consistently across all four maps.
🤖 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.
Inline comments:
In @.env.example:
- Around line 30-34: Update the NETBIRD_ENABLE_DIRECT_PAT entry in the
environment template so it remains unset by default, allowing config.ts to apply
the !oauthEnabled fallback when NETBIRD_ENABLE_OAUTH=false. Preserve the
surrounding comments documenting explicit opt-in and OAuth-disabled behavior.
In `@src/bin/http.ts`:
- Around line 73-76: Restrict the trust-proxy configuration used by
loadConfigOrExit and app.set("trust proxy", trustProxy) to explicit
non-permissive values, rejecting bare true, yes, or on inputs from
NETBIRD_TRUST_PROXY. Preserve valid proxy configuration behavior while ensuring
permissive aliases cannot enable trusting attacker-controlled X-Forwarded-For
values.
In `@test/limiterPool.test.ts`:
- Around line 78-95: Update the LRU eviction test around LimiterPool to store
the original limiter returned for key "b", then assert that retrieving "b" after
inserting "c" returns a different instance from that saved reference. Keep the
existing size and "a" survivor assertions unchanged.
---
Nitpick comments:
In `@src/auth/resolve.ts`:
- Around line 83-93: Document the direct-PAT rate-limit behavior near the
configuration or authentication guidance: because resolve’s verifyPat invokes
http.ts’s verifyDirectPat using the same per-tenant RateLimiter as the
downstream request, each direct-PAT request consumes two rate-limit slots. Call
out that direct-PAT throughput is effectively half the configured
maxRequestsPerMinute compared with OAuth, without changing the authentication
flow.
In `@src/bin/http.ts`:
- Line 73: Add Helmet middleware to the Express application initialized as app
in src/bin/http.ts, import the existing Helmet package or add it as a dependency
if necessary, and register it early in the middleware chain so baseline security
headers apply to all responses.
In `@src/config.ts`:
- Around line 80-94: Update parseTrustProxy to reject or warn when
NETBIRD_TRUST_PROXY is set to bare boolean true, directing operators toward a
hop count or named preset; preserve other supported values. In .env.example
lines 26-28, document that true trusts all proxies and allows spoofing,
recommending a hop count such as 1 or a preset. In README.md lines 131-141, add
the same caveat to the NETBIRD_TRUST_PROXY documentation.
In `@src/oauth/loginRateLimit.ts`:
- Around line 17-25: The loginRateLimiter implementation uses the default
in-memory store, so limits are not shared across replicas. Update
loginRateLimiter to support a shared production store such as rate-limit-redis,
while preserving the existing in-memory behavior for local or single-instance
usage; alternatively, clearly document this deployment constraint if
shared-store support is outside scope.
In `@src/oauth/store.ts`:
- Around line 34-61: Harden OAuthStore’s access, refresh, codes, and clients
maps against unbounded growth, rather than relying on getLive for key-specific
expiry cleanup. Add a bounded, idle-swept storage mechanism analogous to
LimiterPool, enforcing a maximum record count and removing expired or idle
entries without requiring lookup. Preserve the existing TTL and token
issuance/lookup behavior while applying the bounds consistently across all four
maps.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d30c0877-52ea-4688-82b2-984349b77aa4
📒 Files selected for processing (28)
.env.exampleREADME.mdpackage.jsonsrc/auth/context.tssrc/auth/resolve.tssrc/bin/http.tssrc/bin/loadConfigOrExit.tssrc/bin/stdio.tssrc/config.tssrc/netbird/apiUrlPolicy.tssrc/netbird/client.tssrc/netbird/limiterPool.tssrc/netbird/rateLimiter.tssrc/oauth/core.tssrc/oauth/loginRateLimit.tssrc/oauth/provider.tssrc/oauth/store.tstest/apiUrlPolicy.test.tstest/config.test.tstest/helpers.tstest/limiterPool.test.tstest/loginRateLimit.test.tstest/oauth.e2e.test.tstest/oauth.test.tstest/oauthCore.test.tstest/oauthStore.test.tstest/resolve.test.tstest/tools.test.ts
| # Direct-PAT fallback (handy for testing; Claude itself uses OAuth above). | ||
| # OFF by default while OAuth is on — enable it explicitly here. It turns on | ||
| # automatically when NETBIRD_ENABLE_OAUTH=false (the only auth path then). | ||
| # When on, the request base URL must be allowlisted and the PAT is verified. | ||
| NETBIRD_ENABLE_DIRECT_PAT=false |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Explicit false here defeats the auto-on-when-OAuth-off behavior it documents.
NETBIRD_ENABLE_DIRECT_PAT=false is set to a literal value, not left blank. In config.ts, boolEnv(env.NETBIRD_ENABLE_DIRECT_PAT, !oauthEnabled) only applies the !oauthEnabled fallback when the variable is undefined. Copying this template to .env unmodified and then setting NETBIRD_ENABLE_OAUTH=false will not auto-enable direct-PAT as the comment above promises — the operator is left with no HTTP auth path unless they also remember to flip this line.
🩹 Proposed fix
-NETBIRD_ENABLE_DIRECT_PAT=false
+NETBIRD_ENABLE_DIRECT_PAT=🤖 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 @.env.example around lines 30 - 34, Update the NETBIRD_ENABLE_DIRECT_PAT
entry in the environment template so it remains unset by default, allowing
config.ts to apply the !oauthEnabled fallback when NETBIRD_ENABLE_OAUTH=false.
Preserve the surrounding comments documenting explicit opt-in and OAuth-disabled
behavior.
| const app = express(); | ||
| // Behind a proxy this must be set so req.ip is the real client IP, which every | ||
| // per-IP rate limiter (OAuth routes, login form, per-source login verify) keys on. | ||
| app.set("trust proxy", trustProxy); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -B2 -A15 'trustProxy|TRUST_PROXY' src/config.ts .env.example README.md 2>/dev/nullRepository: netbirdio/netbird-mcp
Length of output: 5701
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the trust-proxy parser and its references without executing repo code.
fd -a 'config\.ts$' . | sed 's#^\./##'
rg -n -B5 -A60 'parseTrustProxy|trustProxy' src/config.ts src/bin/http.ts
sed -n '1,220p' src/config.ts
sed -n '60,85p' src/bin/http.tsRepository: netbirdio/netbird-mcp
Length of output: 21936
Restrict NETBIRD_TRUST_PROXY to non-permissive values.
parseTrustProxy() accepts true, yes, and on, and loadConfigOrExit() forwards that value directly to app.set("trust proxy", trustProxy). Any operator typo such as NETBIRD_TRUST_PROXY=on makes Express trust the leftmost X-Forwarded-For, which can let an attacker bypass the IP-keyed rate limits. Reject bare boolean/yes-on values there, or document the env as accepted risk before passing it through.
🤖 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 `@src/bin/http.ts` around lines 73 - 76, Restrict the trust-proxy configuration
used by loadConfigOrExit and app.set("trust proxy", trustProxy) to explicit
non-permissive values, rejecting bare true, yes, or on inputs from
NETBIRD_TRUST_PROXY. Preserve valid proxy configuration behavior while ensuring
permissive aliases cannot enable trusting attacker-controlled X-Forwarded-For
values.
| it("evicts the least-recently-used limiter once the size cap is exceeded", () => { | ||
| const clock = fixedClock(); | ||
| const pool = new LimiterPool(120, { maxEntries: 2, idleTtlMs: 60_000, now: clock.now }); | ||
|
|
||
| const a = pool.get("a"); | ||
| clock.tick(); | ||
| pool.get("b"); | ||
| clock.tick(); | ||
| // Touch "a" so "b" is now the least-recently-used of the two. | ||
| pool.get("a"); | ||
| clock.tick(); | ||
| // Inserting a third key overflows the cap and must evict the LRU ("b"). | ||
| pool.get("c"); | ||
|
|
||
| expect(pool.size).toBe(2); | ||
| expect(pool.get("a")).toBe(a); // survivor keeps its instance | ||
| expect(pool.get("b")).not.toBe(a); // "b" was reclaimed, re-get is a fresh limiter | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
LRU eviction test doesn't actually prove eviction happened for "b".
The final assertion only checks pool.get("b")).not.toBe(a) — trivially true whether or not "b" was evicted, since a freshly-created "b" limiter is never the same object as "a"'s limiter regardless. The test needs to capture the original b reference and compare against it, mirroring the TTL test below (lines 107-113) which does this correctly.
🧪 Proposed fix
const a = pool.get("a");
clock.tick();
- pool.get("b");
+ const b = pool.get("b");
clock.tick();
// Touch "a" so "b" is now the least-recently-used of the two.
pool.get("a");
clock.tick();
// Inserting a third key overflows the cap and must evict the LRU ("b").
pool.get("c");
expect(pool.size).toBe(2);
expect(pool.get("a")).toBe(a); // survivor keeps its instance
- expect(pool.get("b")).not.toBe(a); // "b" was reclaimed, re-get is a fresh limiter
+ expect(pool.get("b")).not.toBe(b); // "b" was reclaimed, re-get is a fresh limiter📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("evicts the least-recently-used limiter once the size cap is exceeded", () => { | |
| const clock = fixedClock(); | |
| const pool = new LimiterPool(120, { maxEntries: 2, idleTtlMs: 60_000, now: clock.now }); | |
| const a = pool.get("a"); | |
| clock.tick(); | |
| pool.get("b"); | |
| clock.tick(); | |
| // Touch "a" so "b" is now the least-recently-used of the two. | |
| pool.get("a"); | |
| clock.tick(); | |
| // Inserting a third key overflows the cap and must evict the LRU ("b"). | |
| pool.get("c"); | |
| expect(pool.size).toBe(2); | |
| expect(pool.get("a")).toBe(a); // survivor keeps its instance | |
| expect(pool.get("b")).not.toBe(a); // "b" was reclaimed, re-get is a fresh limiter | |
| }); | |
| it("evicts the least-recently-used limiter once the size cap is exceeded", () => { | |
| const clock = fixedClock(); | |
| const pool = new LimiterPool(120, { maxEntries: 2, idleTtlMs: 60_000, now: clock.now }); | |
| const a = pool.get("a"); | |
| clock.tick(); | |
| const b = pool.get("b"); | |
| clock.tick(); | |
| // Touch "a" so "b" is now the least-recently-used of the two. | |
| pool.get("a"); | |
| clock.tick(); | |
| // Inserting a third key overflows the cap and must evict the LRU ("b"). | |
| pool.get("c"); | |
| expect(pool.size).toBe(2); | |
| expect(pool.get("a")).toBe(a); // survivor keeps its instance | |
| expect(pool.get("b")).not.toBe(b); // "b" was reclaimed, re-get is a fresh limiter | |
| }); |
🤖 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 `@test/limiterPool.test.ts` around lines 78 - 95, Update the LRU eviction test
around LimiterPool to store the original limiter returned for key "b", then
assert that retrieving "b" after inserting "c" returns a different instance from
that saved reference. Keep the existing size and "a" survivor assertions
unchanged.
…values, fix LRU test assertion, unset direct-PAT in env template - NETBIRD_TRUST_PROXY now rejects bare booleans (true/yes/on) at startup: 'trust proxy: true' makes Express trust the client-controlled leftmost X-Forwarded-For entry, letting anyone spoof their IP past the per-IP rate limits. Hop counts and presets remain supported. - The LRU eviction test compared b's fresh limiter against a's instance, which is never equal regardless of eviction; it now captures and compares b's original reference. - .env.example leaves NETBIRD_ENABLE_DIRECT_PAT unset so the documented auto-enable-when-OAuth-off fallback still applies to copied templates.
Follow-ups from post-merge review of #4
Summary
A set of security-hardening changes for the HTTP (cloud, multi-tenant) deployment, from a security review. They fall into three areas: SSRF protection on the NetBird API base URL, a memory-exhaustion fix in the per-tenant rate-limiter pool, and OAuth 2.1 token-lifecycle hardening. Defaults preserve existing behaviour except where called out under Behaviour changes.
SSRF — trust the NetBird API base URL only when it is allowlisted
Both auth paths let a caller influence the NetBird API base URL (the per-tenant URL header on the direct-PAT path; the
netbird_api_urlfield on the OAuth login form). Unconstrained, that can be aimed at internal addresses — cloud metadata (169.254.169.254), RFC1918, loopback, link-local.src/netbird/apiUrlPolicy.ts): a base URL is accepted only when its host is on the allowlist; private/loopback/link-local/metadata IP literals are denied explicitly.NETBIRD_API_URLhost plus any hosts in the newNETBIRD_ALLOWED_API_HOSTS(comma-separated).NETBIRD_API_URLis validated at startup — a malformed or non-http(s)value fails fast instead of booting a broken target.netbird_api_urlis allowlist-checked before any outbound request, and the login route is rate-limited.DoS — bound the per-tenant rate-limiter pool
The multi-tenant pool created and permanently retained a rate limiter for every distinct token hash it saw, with no eviction. A caller rotating the token value on each request grew the map without bound — a memory-exhaustion vector needing no valid credentials.
OAuth — bound refresh-token lifetime
Refresh-token records carried no expiry and lookups did no TTL check, so a leaked refresh token could mint access tokens indefinitely.
OAuth — scope revocation to the requesting client (RFC 7009)
Revocation deleted any presented token regardless of which client asked.
Per-source login rate limiting
Login-time PAT verification used a single global limiter shared across all tenants, so one noisy source could exhaust it and stall everyone's logins.
NETBIRD_TRUST_PROXY(wired to Expresstrust proxy) so the per-IP limits — these and the existing OAuth-route/login-form limits — key on the real client IP behind a reverse proxy rather than the proxy's.New configuration
NETBIRD_ALLOWED_API_HOSTSNETBIRD_ENABLE_DIRECT_PATNETBIRD_TRUST_PROXYtrust proxyvalue: hop count, boolean, or preset (e.g.loopback)Behaviour changes
NETBIRD_ENABLE_DIRECT_PAT=true. It stays on automatically when OAuth is disabled, since it is then the only way to authenticate.http(s)NETBIRD_API_URLnow fails startup instead of booting with a broken target.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by CodeRabbit
New Features
Bug Fixes
Documentation