fix: change how 'cookie' header is represented in trans to avoid possible mapping conflict (#4007)

Refs: #3322
Fixes: #4006

Author: trentm

CHANGELOG.asciidoc

@@ -33,6 +33,47 @@ Notes:
 
 See the <<upgrade-to-v4>> guide.
 
+==== Unreleased
+
+[float]
+===== Breaking changes
+
+[float]
+===== Features
+
+[float]
+===== Bug fixes
+
+- Change how the "cookie" HTTP request header is represented in APM transaction
+  data to avoid a rare, but possible, intake bug where the transaction could be
+  rejected due to a mapping conflict.
+
+  Before this change a `Cookie: foo=bar; sessionid=42` HTTP request header
+  would be represented in the transaction document in Elasticsearch with these
+  document fields (the example assumes <<sanitize-field-names>> matches
+  "sessionid", as it does by default):
+
+  ```
+  http.request.headers.cookie: "[REDACTED]"
+  ...
+  http.request.cookies.foo: "bar"
+  http.request.cookies.sessionid: "[REDACTED]"
+  ```
+
+  After this change it is represented as:
+
+  ```
+  http.request.headers.cookie: "foo=bar; sessionid=REDACTED"
+  ```
+
+  In other words, `http.request.cookies` are no longer separated out.
+  ({issues}4006[#4006])
+
+
+[float]
+===== Chores
+
+
 [[release-notes-4.5.3]]
 ==== 4.5.3 - 2024/04/23
 

lib/filters/sanitize-field-names.js

@@ -48,16 +48,17 @@ function redactKeysFromPostedFormVariables(body, requestHeaders, regexes) {
  *
  * @param {Object} obj The source object be copied with redacted fields
  * @param {Array<RegExp>} regexes RegExps to check if the entry value needd to be redacted
+ * @param {String} redactedStr The string to use for redacted values. Defaults to '[REDACTED]'.
  * @returns {Object} Copy of the source object with REDACTED entries or the original if falsy or regexes is not an array
  */
-function redactKeysFromObject(obj, regexes) {
+function redactKeysFromObject(obj, regexes, redactedStr = REDACTED) {
   if (!obj || !Array.isArray(regexes)) {
     return obj;
   }
   const result = {};
   for (const key of Object.keys(obj)) {
     const shouldRedact = regexes.some((regex) => regex.test(key));
-    result[key] = shouldRedact ? REDACTED : obj[key];
+    result[key] = shouldRedact ? redactedStr : obj[key];
   }
   return result;
 }

lib/parsers.js

@@ -20,6 +20,15 @@ const {
   redactKeysFromPostedFormVariables,
 } = require('./filters/sanitize-field-names');
 
+// When redacting individual cookie field values, this string is used instead
+// of `[REDACTED]`. The APM spec says:
+//    > The replacement string SHOULD be `[REDACTED]`.
+// We diverge from spec here because, for better or worse, the `cookie` module
+// does `encodeURIComponent/decodeURIComponent` encoding on cookie fields. If we
+// used the brackets, then the reconstructed cookie would look like
+// `foo=bar; session-id=%5BREDACTED%5D`, which isn't helpful.
+const COOKIE_VAL_REDACTED = 'REDACTED';
+
 /**
  * Extract appropriate `{transaction,error}.context.request` from an HTTP
  * request object. This handles header and body capture and redaction
@@ -61,14 +70,21 @@ function getContextFromRequest(req, conf, type) {
       conf.sanitizeFieldNamesRegExp,
     );
 
-    if (context.headers.cookie) {
-      context.cookies = cookie.parse(req.headers.cookie);
-      context.cookies = redactKeysFromObject(
-        context.cookies,
+    if (context.headers.cookie && context.headers.cookie !== REDACTED) {
+      let cookies = cookie.parse(req.headers.cookie);
+      cookies = redactKeysFromObject(
+        cookies,
         conf.sanitizeFieldNamesRegExp,
+        COOKIE_VAL_REDACTED,
       );
-      // Redact the cookie to avoid data duplication
-      context.headers.cookie = REDACTED;
+      try {
+        context.headers.cookie = Object.keys(cookies)
+          .map((k) => cookie.serialize(k, cookies[k]))
+          .join('; ');
+      } catch (_err) {
+        // Fallback to full redaction if there is an issue re-serializing.
+        context.headers.cookie = REDACTED;
+      }
     }
   }
 

package.json

@@ -9,7 +9,7 @@
     "docs:open": "PREVIEW=1 npm run docs:build",
     "docs:build": "./docs/scripts/build_docs.sh apm-agent-nodejs ./docs ./build",
     "lint": "npm run lint:eslint && npm run lint:license-files && npm run lint:yaml-files && npm run lint:tav",
-    "lint:eslint": "eslint # requires node >=18.18.0",
+    "lint:eslint": "eslint . # requires node >=18.18.0",
     "lint:eslint-nostyle": "eslint --rule 'prettier/prettier: off' . # lint without checking style, not normally used; requires node>=18.18.0",
     "lint:fix": "eslint --fix .  # requires node >=18.18.0",
     "lint:license-files": "./dev-utils/gen-notice.sh --lint .  # requires node >=16",

test/instrumentation/transaction.test.js

@@ -553,15 +553,10 @@ test('#_encode() - http request meta data', function (t) {
         host: 'example.com',
         'user-agent': 'user-agent-header',
         'content-length': 42,
-        cookie: '[REDACTED]',
+        cookie: 'cookie1=foo; cookie2=bar; session-id=REDACTED',
         'x-foo': 'bar',
         'x-bar': 'baz',
       },
-      cookies: {
-        cookie1: 'foo',
-        cookie2: 'bar',
-        'session-id': '[REDACTED]',
-      },
       body: '[REDACTED]',
     },
   });

test/parsers.test.js

@@ -11,6 +11,7 @@ var http = require('http');
 var test = require('tape');
 
 var parsers = require('../lib/parsers');
+const { normalizeSanitizeFieldNames } = require('../lib/config/normalizers');
 
 test('#getContextFromResponse()', function (t) {
   t.test('for error (before headers)', function (t) {
@@ -279,6 +280,68 @@ test('#getContextFromRequest()', function (t) {
     t.end();
   });
 
+  t.test('cookie header fields are sanitized', function (t) {
+    const conf = { captureHeaders: true, sanitizeFieldNames: ['*session*'] };
+    normalizeSanitizeFieldNames(conf);
+    const req = {
+      httpVersion: '1.1',
+      method: 'GET',
+      url: '/',
+      headers: {
+        host: 'example.com',
+        cookie: 'foo=bar%3Bbaz; spam=eggs; sessionid=42',
+      },
+    };
+    const parsed = parsers.getContextFromRequest(req, conf);
+    t.deepEqual(parsed, {
+      http_version: '1.1',
+      method: 'GET',
+      url: {
+        raw: '/',
+        protocol: 'http:',
+        hostname: 'example.com',
+        pathname: '/',
+        full: 'http://example.com/',
+      },
+      headers: {
+        host: 'example.com',
+        cookie: 'foo=bar%3Bbaz; spam=eggs; sessionid=REDACTED',
+      },
+    });
+    t.end();
+  });
+
+  t.test('cookie header is in sanitizeFieldNames', function (t) {
+    const conf = {
+      captureHeaders: true,
+      sanitizeFieldNames: ['*session*', 'cookie'],
+    };
+    normalizeSanitizeFieldNames(conf);
+    const req = {
+      httpVersion: '1.1',
+      method: 'GET',
+      url: '/',
+      headers: {
+        host: 'example.com',
+        cookie: 'foo=bar%3Bbaz; spam=eggs; sessionid=42',
+      },
+    };
+    const parsed = parsers.getContextFromRequest(req, conf);
+    t.deepEqual(parsed, {
+      http_version: '1.1',
+      method: 'GET',
+      url: {
+        raw: '/',
+        protocol: 'http:',
+        hostname: 'example.com',
+        pathname: '/',
+        full: 'http://example.com/',
+      },
+      headers: { host: 'example.com', cookie: '[REDACTED]' },
+    });
+    t.end();
+  });
+
   function getMockReq() {
     return {
       httpVersion: '1.1',