fix: trust one proxy hop by default instead of every X-Forwarded-* value

README.md

@@ -183,6 +183,7 @@ ignored.
 | `--source`         | `FUNCTION_SOURCE`         | The path to the directory of your function. Default: `cwd` (the current working directory)                                                                                                                       |
 | `--log-execution-id`| `LOG_EXECUTION_ID`        | Enables execution IDs in logs, either `true` or `false`. When not specified, default to disable. Requires Node.js 13.0.0 or later.                                                                                |
 | `--ignored-routes`| `IGNORED_ROUTES`        | A route expression for requests that should not be routed the function. An empty 404 response will be returned. This is set to `/favicon.ico|/robots.txt` by default for `http` functions.                                                     |
+| `--trust-proxy`   | `TRUST_PROXY`           | The Express [`trust proxy`](https://expressjs.com/en/guide/behind-proxies.html) setting used to derive `req.ip`, `req.hostname` and `req.protocol` from the `X-Forwarded-*` headers: a hop count, `true`, `false`, or a list of trusted addresses. Default: `1`, the one proxy Cloud Run places in front of a function. Raise it when another proxy or load balancer sits in front of Cloud Run. |
 
 You can set command-line flags in your `package.json` via the `start` script.
 For example:

src/options.ts

@@ -59,6 +59,12 @@ export interface FrameworkOptions {
    * Routes that should return a 404 without invoking the function.
    */
   ignoredRoutes: string | null;
+  /**
+   * The Express "trust proxy" setting: how many proxy hops in front of the
+   * function are trusted when deriving req.ip, req.hostname and req.protocol
+   * from the X-Forwarded-* headers.
+   */
+  trustProxy: boolean | number | string;
 }
 
 /**
@@ -139,6 +145,43 @@ const IgnoredRoutesOption = new ConfigurableOption<string | null>(
   null, // null by default so we can detect if it is explicitly set to ""
 );
 
+/**
+ * Express "trust proxy" setting. Cloud Run places one proxy in front of the
+ * function; it appends the address it accepted the connection from as the
+ * last X-Forwarded-For entry, and anything before that entry is whatever the
+ * client sent. Trusting every hop (true) makes req.ip, req.hostname and
+ * req.protocol follow client-supplied X-Forwarded-For, X-Forwarded-Host and
+ * X-Forwarded-Proto values instead of the platform's.
+ */
+const TrustProxyOption = new ConfigurableOption<boolean | number | string>(
+  'trust-proxy',
+  'TRUST_PROXY',
+  1,
+  (x: boolean | number | string) => {
+    if (typeof x === 'boolean' || typeof x === 'number') {
+      return x;
+    }
+    const value = x.trim();
+    if (value.toLowerCase() === 'true') {
+      return true;
+    }
+    if (value.toLowerCase() === 'false') {
+      return false;
+    }
+    if (/^\d+$/.test(value)) {
+      return parseInt(value, 10);
+    }
+    if (value === '') {
+      throw new OptionsError(
+        'Trust proxy must be a hop count, true, false, or a list of trusted addresses',
+      );
+    }
+    // Anything else is passed to Express as is: a comma-separated list of
+    // addresses or subnets, or one of its named presets.
+    return value;
+  },
+);
+
 export const requiredNodeJsVersionForLogExecutionID = '13.0.0';
 
 export const logExecutionIdSupported =
@@ -189,6 +232,7 @@ export const parseOptions = (
       SourceLocationOption.cliOption,
       TimeoutOption.cliOption,
       IgnoredRoutesOption.cliOption,
+      TrustProxyOption.cliOption,
     ],
   });
   return {
@@ -200,5 +244,6 @@ export const parseOptions = (
     printHelp: cliArgs[2] === '-h' || cliArgs[2] === '--help',
     enableExecutionId: ExecutionIdOption.parse(argv, envVars),
     ignoredRoutes: IgnoredRoutesOption.parse(argv, envVars),
+    trustProxy: TrustProxyOption.parse(argv, envVars),
   };
 };

src/server.ts

@@ -95,7 +95,10 @@ export function getServer(
   // The parser will process ALL content types so MUST come last.
   // Subsequent parsers will be skipped when one is matched.
   app.use(bodyParser.raw(rawBodySavingOptions));
-  app.enable('trust proxy'); // To respect X-Forwarded-For header.
+  // Derive req.ip, req.hostname and req.protocol from the X-Forwarded-*
+  // headers, trusting only the configured number of proxy hops (one on Cloud
+  // Run) so that values a client put in those headers are not used.
+  app.set('trust proxy', options.trustProxy);
 
   // Disable Express 'x-powered-by' header:
   // http://expressjs.com/en/advanced/best-practice-security.html#at-a-minimum-disable-x-powered-by-header

src/testing.ts

@@ -57,5 +57,6 @@ export const getTestServer = (functionName: string): Server => {
     sourceLocation: '',
     printHelp: false,
     ignoredRoutes: null,
+    trustProxy: 1,
   });
 };

test/integration/http.ts

@@ -18,6 +18,7 @@ import * as supertest from 'supertest';
 
 import * as functions from '../../src/index';
 import {getTestServer} from '../../src/testing';
+import {getServer} from '../../src/server';
 
 describe('HTTP Function', () => {
   let callCount = 0;
@@ -117,4 +118,58 @@ describe('HTTP Function', () => {
       assert.strictEqual(callCount, test.expectedCallCount);
     });
   });
+
+  describe('proxy headers', () => {
+    before(() => {
+      functions.http('testProxyHeaders', (req, res) => {
+        res.send({
+          ip: req.ip,
+          ips: req.ips,
+          hostname: req.hostname,
+          protocol: req.protocol,
+        });
+      });
+    });
+
+    it('derives the client address from the hop Cloud Run adds, not from a value the client sent', async () => {
+      // Cloud Run appends the connecting client's address to whatever the
+      // client already put in X-Forwarded-For, so the last entry is the one the
+      // platform vouches for.
+      const response = await supertest(getTestServer('testProxyHeaders'))
+        .get('/')
+        .set('X-Forwarded-For', '198.51.100.77, 203.0.113.9')
+        .set('X-Forwarded-Proto', 'https');
+      assert.strictEqual(response.status, 200);
+      assert.strictEqual(response.body.ip, '203.0.113.9');
+      assert.deepStrictEqual(response.body.ips, ['203.0.113.9']);
+      assert.strictEqual(response.body.protocol, 'https');
+    });
+
+    it('only trusts every hop when trust proxy is set to true explicitly', async () => {
+      const handler: functions.HttpFunction = (req, res) => {
+        res.send({ip: req.ip, ips: req.ips});
+      };
+      const server = getServer(handler, {
+        signatureType: 'http',
+        enableExecutionId: false,
+        timeoutMilliseconds: 0,
+        port: '0',
+        target: '',
+        sourceLocation: '',
+        printHelp: false,
+        ignoredRoutes: null,
+        trustProxy: true,
+      });
+      const response = await supertest(server)
+        .get('/')
+        .set('X-Forwarded-For', '198.51.100.77, 203.0.113.9');
+      assert.strictEqual(response.status, 200);
+      // The previous default: the first, client-supplied entry wins.
+      assert.strictEqual(response.body.ip, '198.51.100.77');
+      assert.deepStrictEqual(response.body.ips, [
+        '198.51.100.77',
+        '203.0.113.9',
+      ]);
+    });
+  });
 });

test/integration/legacy_event.ts

@@ -41,6 +41,7 @@ const testOptions = {
   sourceLocation: '',
   printHelp: false,
   ignoredRoutes: null,
+  trustProxy: 1,
 };
 
 describe('Event Function', () => {

test/options.ts

@@ -60,6 +60,7 @@ describe('parseOptions', () => {
         enableExecutionId: false,
         timeoutMilliseconds: 0,
         ignoredRoutes: null,
+        trustProxy: 1,
       },
     },
     {
@@ -124,6 +125,7 @@ describe('parseOptions', () => {
         '--source=/source',
         '--timeout=3',
         '--ignored-routes=avocado',
+        '--trust-proxy=2',
       ],
       envVars: {
         PORT: '4567',
@@ -132,6 +134,7 @@ describe('parseOptions', () => {
         FUNCTION_SOURCE: '/somewhere/else',
         CLOUD_RUN_TIMEOUT_SECONDS: '5',
         IGNORED_ROUTES: 'banana',
+        TRUST_PROXY: 'true',
       },
       expectedOptions: {
         port: '1234',
@@ -142,6 +145,23 @@ describe('parseOptions', () => {
         enableExecutionId: false,
         timeoutMilliseconds: 3000,
         ignoredRoutes: 'avocado',
+        trustProxy: 2,
+      },
+    },
+    {
+      name: 'parses the trust proxy option as a hop count, a boolean, or an address list',
+      cliOpts: ['bin/node', '/index.js'],
+      envVars: {TRUST_PROXY: 'loopback, 10.0.0.0/8'},
+      expectedOptions: {
+        trustProxy: 'loopback, 10.0.0.0/8',
+      },
+    },
+    {
+      name: 'parses a boolean trust proxy option',
+      cliOpts: ['bin/node', '/index.js', '--trust-proxy=false'],
+      envVars: {},
+      expectedOptions: {
+        trustProxy: false,
       },
     },
   ];