diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 678312bfa3..2017ff5f6b 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -33,6 +33,47 @@ Notes: See the <> 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 <> 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 diff --git a/lib/filters/sanitize-field-names.js b/lib/filters/sanitize-field-names.js index 180f58524e..3911c0566f 100644 --- a/lib/filters/sanitize-field-names.js +++ b/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} 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; } diff --git a/lib/parsers.js b/lib/parsers.js index 5f4e40a418..133a17ae8a 100644 --- a/lib/parsers.js +++ b/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; + } } } diff --git a/package.json b/package.json index 44e7506630..2ecda2142b 100644 --- a/package.json +++ b/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", diff --git a/test/instrumentation/transaction.test.js b/test/instrumentation/transaction.test.js index f6ef6720ac..13a36b845b 100644 --- a/test/instrumentation/transaction.test.js +++ b/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]', }, }); diff --git a/test/parsers.test.js b/test/parsers.test.js index f1f8bcb113..71f8a85a7e 100644 --- a/test/parsers.test.js +++ b/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',