fix(utils): Streamline IP capturing on incoming requests (#13272)

This PR does three things:

1. It ensures we infer the IP (in RequestData integration) from
IP-related headers, if available.
2. It ensures we do not send these headers if IP capturing is not
enabled (which is the default)
3. It removes the custom handling we had for this in remix, as this
should now just be handled generally

Closes #13260

Author: mydea

packages/remix/src/utils/web-fetch.ts

@@ -1,4 +1,3 @@
-/* eslint-disable complexity */
 // Based on Remix's implementation of Fetch API
 // https://github.com/remix-run/web-std-io/blob/d2a003fe92096aaf97ab2a618b74875ccaadc280/packages/fetch/
 // The MIT License (MIT)
@@ -23,10 +22,6 @@
 // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
 // SOFTWARE.
 
-import { logger } from '@sentry/utils';
-
-import { DEBUG_BUILD } from './debug-build';
-import { getClientIPAddress } from './vendor/getIpAddress';
 import type { RemixRequest } from './vendor/types';
 
 /*
@@ -124,15 +119,6 @@ export const normalizeRemixRequest = (request: RemixRequest): Record<string, any
     headers.set('Connection', 'close');
   }
 
-  let ip;
-
-  // Using a try block here not to break the whole request if we can't get the IP address
-  try {
-    ip = getClientIPAddress(headers);
-  } catch (e) {
-    DEBUG_BUILD && logger.warn('Could not get client IP address', e);
-  }
-
   // HTTP-network fetch step 4.2
   // chunked encoding is handled by Node.js
   const search = getSearch(parsedURL);
@@ -156,9 +142,6 @@ export const normalizeRemixRequest = (request: RemixRequest): Record<string, any
 
     // [SENTRY] For compatibility with Sentry SDK RequestData parser, adding `originalUrl` property.
     originalUrl: parsedURL.href,
-
-    // [SENTRY] Adding `ip` property if found inside headers.
-    ip,
   };
 
   return requestOptions;

packages/remix/test/utils/getIpAddress.test.ts

@@ -83,7 +83,6 @@ describe('normalizeRemixRequest', () => {
       hostname: 'example.com',
       href: 'https://example.com/api/json?id=123',
       insecureHTTPParser: undefined,
-      ip: null,
       method: 'GET',
       originalUrl: 'https://example.com/api/json?id=123',
       path: '/api/json?id=123',

packages/remix/test/utils/normalizeRemixRequest.test.ts

@@ -13,6 +13,7 @@ import { isPlainObject, isString } from './is';
 import { logger } from './logger';
 import { normalize } from './normalize';
 import { stripUrlQueryAndFragment } from './url';
+import { getClientIPAddress, ipHeaderNames } from './vendor/getIpAddress';
 
 const DEFAULT_INCLUDES = {
   ip: false,
@@ -98,7 +99,6 @@ export function extractPathForTransaction(
   return [name, source];
 }
 
-/** JSDoc */
 function extractTransaction(req: PolymorphicRequest, type: boolean | TransactionNamingScheme): string {
   switch (type) {
     case 'path': {
@@ -116,7 +116,6 @@ function extractTransaction(req: PolymorphicRequest, type: boolean | Transaction
   }
 }
 
-/** JSDoc */
 function extractUserData(
   user: {
     [key: string]: unknown;
@@ -146,17 +145,16 @@ function extractUserData(
  */
 export function extractRequestData(
   req: PolymorphicRequest,
-  options?: {
+  options: {
     include?: string[];
-  },
+  } = {},
 ): ExtractedNodeRequestData {
-  const { include = DEFAULT_REQUEST_INCLUDES } = options || {};
-  // eslint-disable-next-line @typescript-eslint/no-explicit-any
-  const requestData: { [key: string]: any } = {};
+  const { include = DEFAULT_REQUEST_INCLUDES } = options;
+  const requestData: { [key: string]: unknown } = {};
 
   // headers:
   //   node, express, koa, nextjs: req.headers
-  const headers = (req.headers || {}) as {
+  const headers = (req.headers || {}) as typeof req.headers & {
     host?: string;
     cookie?: string;
   };
@@ -191,6 +189,14 @@ export function extractRequestData(
           delete (requestData.headers as { cookie?: string }).cookie;
         }
 
+        // Remove IP headers in case IP data should not be included in the event
+        if (!include.includes('ip')) {
+          ipHeaderNames.forEach(ipHeaderName => {
+            // eslint-disable-next-line @typescript-eslint/no-dynamic-delete
+            delete (requestData.headers as Record<string, unknown>)[ipHeaderName];
+          });
+        }
+
         break;
       }
       case 'method': {
@@ -264,9 +270,12 @@ export function addRequestDataToEvent(
   };
 
   if (include.request) {
-    const extractedRequestData = Array.isArray(include.request)
-      ? extractRequestData(req, { include: include.request })
-      : extractRequestData(req);
+    const includeRequest = Array.isArray(include.request) ? [...include.request] : [...DEFAULT_REQUEST_INCLUDES];
+    if (include.ip) {
+      includeRequest.push('ip');
+    }
+
+    const extractedRequestData = extractRequestData(req, { include: includeRequest });
 
     event.request = {
       ...event.request,
@@ -288,8 +297,9 @@ export function addRequestDataToEvent(
   // client ip:
   //   node, nextjs: req.socket.remoteAddress
   //   express, koa: req.ip
+  //   It may also be sent by proxies as specified in X-Forwarded-For or similar headers
   if (include.ip) {
-    const ip = req.ip || (req.socket && req.socket.remoteAddress);
+    const ip = (req.headers && getClientIPAddress(req.headers)) || req.ip || (req.socket && req.socket.remoteAddress);
     if (ip) {
       event.user = {
         ...event.user,

packages/utils/src/requestdata.ts

@@ -23,58 +23,46 @@
 // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
 // SOFTWARE.
 
-import { isIP } from 'net';
+// The headers to check, in priority order
+export const ipHeaderNames = [
+  'X-Client-IP',
+  'X-Forwarded-For',
+  'Fly-Client-IP',
+  'CF-Connecting-IP',
+  'Fastly-Client-Ip',
+  'True-Client-Ip',
+  'X-Real-IP',
+  'X-Cluster-Client-IP',
+  'X-Forwarded',
+  'Forwarded-For',
+  'Forwarded',
+  'X-Vercel-Forwarded-For',
+];
 
 /**
  * Get the IP address of the client sending a request.
  *
  * It receives a Request headers object and use it to get the
  * IP address from one of the following headers in order.
  *
- * - X-Client-IP
- * - X-Forwarded-For
- * - Fly-Client-IP
- * - CF-Connecting-IP
- * - Fastly-Client-Ip
- * - True-Client-Ip
- * - X-Real-IP
- * - X-Cluster-Client-IP
- * - X-Forwarded
- * - Forwarded-For
- * - Forwarded
- *
  * If the IP address is valid, it will be returned. Otherwise, null will be
  * returned.
  *
  * If the header values contains more than one IP address, the first valid one
  * will be returned.
  */
-export function getClientIPAddress(headers: Headers): string | null {
-  // The headers to check, in priority order
-  const headerNames = [
-    'X-Client-IP',
-    'X-Forwarded-For',
-    'Fly-Client-IP',
-    'CF-Connecting-IP',
-    'Fastly-Client-Ip',
-    'True-Client-Ip',
-    'X-Real-IP',
-    'X-Cluster-Client-IP',
-    'X-Forwarded',
-    'Forwarded-For',
-    'Forwarded',
-  ];
-
+export function getClientIPAddress(headers: { [key: string]: string | string[] | undefined }): string | null {
   // This will end up being Array<string | string[] | undefined | null> because of the various possible values a header
   // can take
-  const headerValues = headerNames.map((headerName: string) => {
-    const value = headers.get(headerName);
+  const headerValues = ipHeaderNames.map((headerName: string) => {
+    const rawValue = headers[headerName];
+    const value = Array.isArray(rawValue) ? rawValue.join(';') : rawValue;
 
     if (headerName === 'Forwarded') {
       return parseForwardedHeader(value);
     }
 
-    return value?.split(',').map((v: string) => v.trim());
+    return value && value.split(',').map((v: string) => v.trim());
   });
 
   // Flatten the array and filter out any falsy entries
@@ -92,7 +80,7 @@ export function getClientIPAddress(headers: Headers): string | null {
   return ipAddress || null;
 }
 
-function parseForwardedHeader(value: string | null): string | null {
+function parseForwardedHeader(value: string | null | undefined): string | null {
   if (!value) {
     return null;
   }
@@ -105,3 +93,31 @@ function parseForwardedHeader(value: string | null): string | null {
 
   return null;
 }
+
+//
+/**
+ * Custom method instead of importing this from `net` package, as this only exists in node
+ * Accepts:
+ * 127.0.0.1
+ * 192.168.1.1
+ * 192.168.1.255
+ * 255.255.255.255
+ * 10.1.1.1
+ * 0.0.0.0
+ * 2b01:cb19:8350:ed00:d0dd:fa5b:de31:8be5
+ *
+ * Rejects:
+ * 1.1.1.01
+ * 30.168.1.255.1
+ * 127.1
+ * 192.168.1.256
+ * -1.2.3.4
+ * 1.1.1.1.
+ * 3...3
+ * 192.168.1.099
+ */
+function isIP(str: string): boolean {
+  const regex =
+    /(?:^(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)){3}$)|(?:^(?:(?:[a-fA-F\d]{1,4}:){7}(?:[a-fA-F\d]{1,4}|:)|(?:[a-fA-F\d]{1,4}:){6}(?:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)(?:\\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)){3}|:[a-fA-F\d]{1,4}|:)|(?:[a-fA-F\d]{1,4}:){5}(?::(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)(?:\\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)){3}|(?::[a-fA-F\d]{1,4}){1,2}|:)|(?:[a-fA-F\d]{1,4}:){4}(?:(?::[a-fA-F\d]{1,4}){0,1}:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)(?:\\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)){3}|(?::[a-fA-F\d]{1,4}){1,3}|:)|(?:[a-fA-F\d]{1,4}:){3}(?:(?::[a-fA-F\d]{1,4}){0,2}:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)(?:\\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)){3}|(?::[a-fA-F\d]{1,4}){1,4}|:)|(?:[a-fA-F\d]{1,4}:){2}(?:(?::[a-fA-F\d]{1,4}){0,3}:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)(?:\\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)){3}|(?::[a-fA-F\d]{1,4}){1,5}|:)|(?:[a-fA-F\d]{1,4}:){1}(?:(?::[a-fA-F\d]{1,4}){0,4}:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)(?:\\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)){3}|(?::[a-fA-F\d]{1,4}){1,6}|:)|(?::(?:(?::[a-fA-F\d]{1,4}){0,5}:(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)(?:\\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)){3}|(?::[a-fA-F\d]{1,4}){1,7}|:)))(?:%[0-9a-zA-Z]{1,})?$)/;
+  return regex.test(str);
+}