feat: Fallback to X-Forwarded-* headers

* Fallback to X-Forwarded-* headers This uses the first value from X-Forwarded-Host and X-Forwarded-Proto if they're present and the standard Forwarded header is not. * Update parseForwarded to handle X-Forwarded-* This updates the signature for parseForwarded to take in the headers and handle the logic of falling back to X-Forwarded-* headers. * Update src/util/HeaderUtil.ts Co-authored-by: Ruben Verborgh <[email protected]> * Inline parseXForwarded helper Additionally fixes a typo, updates a unit test, and removes a typing that is no longer necessary. * Tweak handling of X-Forwarded value checking and assignment * Fix: terminology & consistency suggestions from review Co-authored-by: Ruben Verborgh <[email protected]> Co-authored-by: Ruben Verborgh <[email protected]> Co-authored-by: Wouter Termont <[email protected]>
CommunitySolidServer · Mar 23, 2021 · de51a23 · de51a23
1 parent e2284c4
commit de51a23
Show file tree

Hide file tree

Showing 6 changed files with 131 additions and 22 deletions.
diff --git a/src/ldp/UnsecureWebSocketsProtocol.ts b/src/ldp/UnsecureWebSocketsProtocol.ts
@@ -44,7 +44,7 @@ class WebSocketListener extends EventEmitter {
     }
 
     // Store the HTTP host and protocol
-    const forwarded = parseForwarded(headers.forwarded);
+    const forwarded = parseForwarded(headers);
     this.host = forwarded.host ?? headers.host ?? 'localhost';
     this.protocol = forwarded.proto === 'https' || (socket as any).secure ? 'https:' : 'http:';
   }

diff --git a/src/ldp/http/OriginalUrlExtractor.ts b/src/ldp/http/OriginalUrlExtractor.ts
@@ -21,17 +21,17 @@ export class OriginalUrlExtractor extends TargetExtractor {
       throw new Error('Missing URL');
     }
 
-    // Extract host and protocol (possibly overridden by the Forwarded header)
+    // Extract host and protocol (possibly overridden by the Forwarded/X-Forwarded-* header)
     let { host } = headers;
     let protocol = (connection as TLSSocket)?.encrypted ? 'https' : 'http';
-    if (headers.forwarded) {
-      const forwarded = parseForwarded(headers.forwarded);
-      if (forwarded.host) {
-        ({ host } = forwarded);
-      }
-      if (forwarded.proto) {
-        ({ proto: protocol } = forwarded);
-      }
+
+    // Check Forwarded/X-Forwarded-* headers
+    const forwarded = parseForwarded(headers);
+    if (forwarded.host) {
+      ({ host } = forwarded);
+    }
+    if (forwarded.proto) {
+      ({ proto: protocol } = forwarded);
     }
 
     // Perform a sanity check on the host

diff --git a/src/util/HeaderUtil.ts b/src/util/HeaderUtil.ts
@@ -1,3 +1,4 @@
+import type { IncomingHttpHeaders } from 'http';
 import { getLoggerFor } from '../logging/LogUtil';
 import type { HttpResponse } from '../server/HttpResponse';
 import { BadRequestHttpError } from './errors/BadRequestHttpError';
@@ -429,21 +430,29 @@ export interface Forwarded {
 }
 
 /**
- * Parses a Forwarded header value.
+ * Parses a Forwarded header value and will fall back to X-Forwarded-* headers.
  *
- * @param value - The Forwarded header value.
+ * @param headers - The incoming HTTP headers.
  *
  * @returns The parsed Forwarded header.
  */
-export function parseForwarded(value = ''): Forwarded {
+export function parseForwarded(headers: IncomingHttpHeaders): Forwarded {
   const forwarded: Record<string, string> = {};
-  if (value) {
-    for (const pair of value.replace(/\s*,.*$/u, '').split(';')) {
+  if (headers.forwarded) {
+    for (const pair of headers.forwarded.replace(/\s*,.*/u, '').split(';')) {
       const components = /^(by|for|host|proto)=(.+)$/u.exec(pair);
       if (components) {
         forwarded[components[1]] = components[2];
       }
     }
+  } else {
+    const suffixes = [ 'host', 'proto' ];
+    for (const suffix of suffixes) {
+      const value = headers[`x-forwarded-${suffix}`] as string;
+      if (value) {
+        forwarded[suffix] = value.trim().replace(/\s*,.*/u, '');
+      }
+    }
   }
   return forwarded;
 }
diff --git a/test/unit/ldp/UnsecureWebSocketsProtocol.test.ts b/test/unit/ldp/UnsecureWebSocketsProtocol.test.ts
@@ -182,4 +182,20 @@ describe('An UnsecureWebSocketsProtocol', (): void => {
     expect(webSocket.messages).toHaveLength(2);
     expect(webSocket.messages.pop()).toBe('ack https://other.example/protocol/foo');
   });
+
+  it('respects the X-Forwarded-* headers if Forwarded header is not present.', async(): Promise<void> => {
+    const webSocket = new DummySocket();
+    const upgradeRequest = {
+      headers: {
+        'x-forwarded-host': 'other.example',
+        'x-forwarded-proto': 'https',
+        'sec-websocket-protocol': 'solid-0.1',
+      },
+      socket: {},
+    } as any as HttpRequest;
+    await protocol.handle({ webSocket, upgradeRequest } as any);
+    webSocket.emit('message', 'sub https://other.example/protocol/foo');
+    expect(webSocket.messages).toHaveLength(2);
+    expect(webSocket.messages.pop()).toBe('ack https://other.example/protocol/foo');
+  });
 });
diff --git a/test/unit/ldp/http/OriginalUrlExtractor.test.ts b/test/unit/ldp/http/OriginalUrlExtractor.test.ts
@@ -81,4 +81,53 @@ describe('A OriginalUrlExtractor', (): void => {
     await expect(extractor.handle({ request: { url: '/foo/bar', headers } as any }))
       .resolves.toEqual({ path: 'https://pod.example/foo/bar' });
   });
+
+  it('should fallback to x-fowarded-* headers.', async(): Promise<void> => {
+    const headers = {
+      host: 'test.com',
+      'x-forwarded-host': 'pod.example',
+      'x-forwarded-proto': 'https',
+    };
+    await expect(extractor.handle({ request: { url: '/foo/bar', headers } as any }))
+      .resolves.toEqual({ path: 'https://pod.example/foo/bar' });
+  });
+
+  it('should just take x-forwarded-host if provided.', async(): Promise<void> => {
+    const headers = {
+      host: 'test.com',
+      'x-forwarded-host': 'pod.example',
+    };
+    await expect(extractor.handle({ request: { url: '/foo/bar', headers } as any }))
+      .resolves.toEqual({ path: 'http://pod.example/foo/bar' });
+  });
+
+  it('should just take x-forwarded-protocol if provided.', async(): Promise<void> => {
+    const headers = {
+      host: 'test.com',
+      'x-forwarded-proto': 'https',
+    };
+    await expect(extractor.handle({ request: { url: '/foo/bar', headers } as any }))
+      .resolves.toEqual({ path: 'https://test.com/foo/bar' });
+  });
+
+  it('should prefer forwarded header to x-forwarded-* headers.', async(): Promise<void> => {
+    const headers = {
+      host: 'test.com',
+      forwarded: 'proto=http;host=pod.example',
+      'x-forwarded-proto': 'https',
+      'x-forwarded-host': 'anotherpod.example',
+    };
+    await expect(extractor.handle({ request: { url: '/foo/bar', headers } as any }))
+      .resolves.toEqual({ path: 'http://pod.example/foo/bar' });
+  });
+
+  it('should just take the first x-forwarded-* value.', async(): Promise<void> => {
+    const headers = {
+      host: 'test.com',
+      'x-forwarded-host': 'pod.example, another.domain',
+      'x-forwarded-proto': 'http,https',
+    };
+    await expect(extractor.handle({ request: { url: '/foo/bar', headers } as any }))
+      .resolves.toEqual({ path: 'http://pod.example/foo/bar' });
+  });
 });
diff --git a/test/unit/util/HeaderUtil.test.ts b/test/unit/util/HeaderUtil.test.ts
@@ -188,16 +188,18 @@ describe('HeaderUtil', (): void => {
   });
 
   describe('#parseForwarded', (): void => {
-    it('parses an undefined value.', (): void => {
-      expect(parseForwarded()).toEqual({});
+    it('handles an empty set of headers.', (): void => {
+      expect(parseForwarded({})).toEqual({});
     });
 
-    it('parses an empty string.', (): void => {
-      expect(parseForwarded('')).toEqual({});
+    it('handles empty string values.', (): void => {
+      const headers = { forwarded: '', 'x-forwarded-host': '', 'x-forwarded-proto': '' };
+      expect(parseForwarded(headers)).toEqual({});
     });
 
     it('parses a Forwarded header value.', (): void => {
-      expect(parseForwarded('for=192.0.2.60;proto=http;by=203.0.113.43;host=example.org')).toEqual({
+      const headers = { forwarded: 'for=192.0.2.60;proto=http;by=203.0.113.43;host=example.org' };
+      expect(parseForwarded(headers)).toEqual({
         by: '203.0.113.43',
         for: '192.0.2.60',
         host: 'example.org',
@@ -206,15 +208,48 @@ describe('HeaderUtil', (): void => {
     });
 
     it('skips empty fields.', (): void => {
-      expect(parseForwarded('for=192.0.2.60;proto=;by=;host=')).toEqual({
+      const headers = { forwarded: 'for=192.0.2.60;proto=;by=;host=' };
+      expect(parseForwarded(headers)).toEqual({
         for: '192.0.2.60',
       });
     });
 
     it('takes only the first value into account.', (): void => {
-      expect(parseForwarded('host=pod.example, for=192.0.2.43, host=other')).toEqual({
+      const headers = { forwarded: 'host=pod.example, for=192.0.2.43, host=other' };
+      expect(parseForwarded(headers)).toEqual({
         host: 'pod.example',
       });
     });
+
+    it('should fall back to X-Forwarded-Host and X-Forwarded-Proto without Forward header.', (): void => {
+      const headers = { 'x-forwarded-host': 'pod.example', 'x-forwarded-proto': 'https' };
+      expect(parseForwarded(headers)).toEqual({
+        host: 'pod.example',
+        proto: 'https',
+      });
+    });
+
+    it('should prefer Forwarded to X-Forwarded-Host and X-Forwarded-Proto with Forward header.', (): void => {
+      const headers = {
+        forwarded: 'proto=http;host=pod.example',
+        'x-forwarded-host': 'anotherpod.example',
+        'x-forwarded-proto': 'https',
+      };
+      expect(parseForwarded(headers)).toEqual({
+        host: 'pod.example',
+        proto: 'http',
+      });
+    });
+
+    it('should properly handle multiple values with varying spaces for X-Forwarded-*.', (): void => {
+      const headers = {
+        'x-forwarded-host': ' pod.example ,192.0.2.60, 192.0.2.43',
+        'x-forwarded-proto': ' https ,http',
+      };
+      expect(parseForwarded(headers)).toEqual({
+        host: 'pod.example',
+        proto: 'https',
+      });
+    });
   });
 });