Follow-ups from post-merge review of #4

.env.example

@@ -24,14 +24,17 @@ NETBIRD_ENABLE_OAUTH=true
 # Verify the pasted NetBird PAT with a live read during the OAuth login. Set false for offline dev.
 NETBIRD_VERIFY_PAT_ON_LOGIN=true
 # Behind a reverse proxy / load balancer, set this so per-IP rate limits key on the
-# real client IP, not the proxy's. Hop count (e.g. 1), boolean, or a preset like "loopback".
+# real client IP, not the proxy's. Hop count (e.g. 1) or a preset like "loopback".
+# Bare booleans (true/yes/on) are rejected — trusting every proxy allows IP spoofing.
 NETBIRD_TRUST_PROXY=
 
 # 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
+# Leave unset to keep the automatic default; an explicit false blocks the
+# auto-enable when OAuth is off.
+NETBIRD_ENABLE_DIRECT_PAT=
 # Header the hosted server reads the caller's NetBird PAT from (per-request, never stored).
 NETBIRD_TOKEN_HEADER=x-netbird-token
 # Optional header for per-tenant API base URL override.

README.md

@@ -138,7 +138,7 @@ steered at an arbitrary host or driven with an unverified token.
 | `NETBIRD_ENABLE_OAUTH` | cloud | `true` | Enable the OAuth 2.1 authorization server |
 | `NETBIRD_ENABLE_DIRECT_PAT` | cloud | off when OAuth on | Allow the direct-PAT header path (auto-on when OAuth is off) |
 | `NETBIRD_VERIFY_PAT_ON_LOGIN` | cloud | `true` | Live-check the PAT during OAuth login |
-| `NETBIRD_TRUST_PROXY` | cloud | `false` | Express `trust proxy` for real client IPs behind a proxy: hop count (e.g. `1`), boolean, or preset (`loopback`) |
+| `NETBIRD_TRUST_PROXY` | cloud | `false` | Express `trust proxy` for real client IPs behind a proxy: hop count (e.g. `1`) or preset (`loopback`). Bare booleans are rejected — trusting every proxy allows IP spoofing |
 | `NETBIRD_TOKEN_HEADER` | cloud | `x-netbird-token` | Header carrying the caller's PAT (direct-PAT mode) |
 | `NETBIRD_URL_HEADER` | cloud | `x-netbird-api-url` | Optional per-tenant base URL header |
 

src/config.ts

@@ -43,8 +43,9 @@ export interface HttpConfig {
    * Express `trust proxy` setting. Behind a reverse proxy / load balancer the
    * per-IP rate limits (OAuth routes, the login form, and per-source login
    * verification) must key on the real client IP, not the proxy's — otherwise
-   * every client collapses into one bucket. Set to the number of proxy hops, a
-   * boolean, or a preset (e.g. "loopback"); defaults to false (direct connections).
+   * every client collapses into one bucket. Set to the number of proxy hops or
+   * a preset (e.g. "loopback"); defaults to false (direct connections). Bare
+   * booleans are rejected — trusting every proxy allows IP spoofing.
    */
   trustProxy: boolean | number | string;
 }
@@ -79,16 +80,28 @@ function intEnv(value: string | undefined, fallback: number): number {
 
 /**
  * Parse the Express `trust proxy` value from env. A bare integer is a hop count
- * (the common "one proxy in front" = 1); true/false toggle it; anything else is
- * passed through so operators can use Express presets or subnet lists
- * ("loopback", "10.0.0.0/8", …). Unset means false — safe for direct connections.
+ * (the common "one proxy in front" = 1); anything else is passed through so
+ * operators can use Express presets or subnet lists ("loopback", "10.0.0.0/8", …).
+ * Unset means false — safe for direct connections.
+ *
+ * Permissive booleans (true/yes/on) are rejected: `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. A hop count or preset is
+ * always the safer way to express the same intent.
  */
 function parseTrustProxy(value: string | undefined): boolean | number | string {
   const v = value?.trim();
   if (!v) return false;
   if (/^\d+$/.test(v)) return Number.parseInt(v, 10);
   const lower = v.toLowerCase();
-  if (["true", "yes", "on"].includes(lower)) return true;
+  if (["true", "yes", "on"].includes(lower)) {
+    throw new Error(
+      `NETBIRD_TRUST_PROXY="${v}" is not allowed: trusting every proxy lets clients ` +
+        `spoof their IP via X-Forwarded-For and bypass per-IP rate limits. ` +
+        `Set the number of proxy hops (e.g. 1) or an Express preset/subnet ` +
+        `(e.g. "loopback", "10.0.0.0/8") instead.`,
+    );
+  }
   if (["false", "no", "off"].includes(lower)) return false;
   return v;
 }

test/config.test.ts

@@ -99,11 +99,24 @@ describe("loadServerConfig — http sub-object explicit values", () => {
       loadServerConfig({ NETBIRD_TRUST_PROXY: v } as NodeJS.ProcessEnv).http.trustProxy;
     expect(trust(undefined)).toBe(false);
     expect(trust("1")).toBe(1); // one proxy hop in front
-    expect(trust("true")).toBe(true);
     expect(trust("false")).toBe(false);
+    expect(trust("no")).toBe(false);
+    expect(trust("off")).toBe(false);
     expect(trust("loopback")).toBe("loopback"); // Express preset, passed through
   });
 
+  it("rejects permissive NETBIRD_TRUST_PROXY booleans 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. Safe alternatives (a hop count or preset) always exist, so a bare
+    // boolean fails fast instead of silently enabling the unsafe mode.
+    for (const v of ["true", "yes", "on", "TRUE"]) {
+      expect(() => loadServerConfig({ NETBIRD_TRUST_PROXY: v } as NodeJS.ProcessEnv)).toThrow(
+        /NETBIRD_TRUST_PROXY/,
+      );
+    }
+  });
+
   it("strips a trailing slash from an explicit PUBLIC_BASE_URL", () => {
     const config = loadServerConfig({
       PUBLIC_BASE_URL: "https://mcp.example.com///",

test/limiterPool.test.ts

@@ -81,7 +81,7 @@ describe("LimiterPool eviction (memory bound / DoS defense)", () => {
 
     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");
@@ -91,7 +91,7 @@ describe("LimiterPool eviction (memory bound / DoS defense)", () => {
 
     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
   });
 
   it("evicts a limiter left idle past the TTL", () => {