fix(utils): Streamline IP capturing on incoming requests

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/utils/src/requestdata.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/remix/src/utils/vendor/getIpAddress.ts renamed to packages/utils/src/vendor/getIpAddress.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
+ *
+ * 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 =
+    /^((?:[0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])[.]){3}(?:[0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])$/;
+
+  return regex.test(str);
+}