From 66d1cecb9674ccea8fb4889e299b3d5b2e2c918d Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Wed, 27 May 2026 10:58:59 +0200 Subject: [PATCH 1/7] fix(appsec): Implement limits for analyzed downstream requests --- .../src/http/client.js | 76 +++- .../test/http.spec.js | 352 +++++++++++++----- .../src/appsec/downstream_requests.js | 46 +++ packages/dd-trace/src/appsec/rasp/ssrf.js | 16 +- .../src/config/generated-config-types.d.ts | 1 + .../src/config/supported-configurations.json | 8 + .../test/appsec/downstream_requests.spec.js | 53 +++ .../dd-trace/test/appsec/rasp/ssrf.spec.js | 48 ++- packages/dd-trace/test/config/index.spec.js | 6 + 9 files changed, 497 insertions(+), 109 deletions(-) diff --git a/packages/datadog-instrumentations/src/http/client.js b/packages/datadog-instrumentations/src/http/client.js index 14edc2c867..6b2c35b9ba 100644 --- a/packages/datadog-instrumentations/src/http/client.js +++ b/packages/datadog-instrumentations/src/http/client.js @@ -56,6 +56,62 @@ function normalizeCallback (inputOptions, callback, inputURL) { return typeof inputOptions === 'function' ? [inputOptions, inputURL || {}] : [callback, inputOptions] } +/** + * @param {string|null|undefined} contentType raw content-type header value. + * @returns {string|null} lowercase mime type without parameters. + */ +function extractMimeType (contentType) { + if (typeof contentType !== 'string') { + return null + } + + return contentType.split(';', 1)[0].trim().toLowerCase() +} + +/** + * @param {string|string[]|undefined} contentLength raw content-length header value. + * @returns {number|null} parsed content length or null when invalid. + */ +function parseContentLength (contentLength) { + if (contentLength == null) { + return null + } + + const value = Array.isArray(contentLength) ? contentLength[0] : contentLength + const parsed = Number.parseInt(String(value), 10) + + if (!Number.isFinite(parsed) || parsed < 0) { + return null + } + + return parsed +} + +/** + * @param {import('http').IncomingMessage} res downstream response. + * @param {{ maxBytes: number, supportedMimeTypes: Set }} responseBodyCollection collection limits. + * @returns {{ collect: boolean, reason?: string }} + */ +function evaluateResponseBodyCollection (res, responseBodyCollection) { + const { maxBytes, supportedMimeTypes } = responseBodyCollection + + const mime = extractMimeType(res.headers?.['content-type']) + if (!mime || !supportedMimeTypes.has(mime)) { + return { collect: false, reason: 'content_type_invalid' } + } + + const declaredContentLength = parseContentLength(res.headers?.['content-length']) + if (declaredContentLength == null || declaredContentLength === 0) { + return { collect: false, reason: 'content_length_missing' } + } + + if (declaredContentLength > maxBytes) { + return { collect: false, reason: 'content_length_too_big' } + } + + return { collect: true } +} + /** * Wires the downstream response so we can observe when the customer consumes * the body and when the stream finishes @@ -77,11 +133,23 @@ function setupResponseInstrumentation (ctx, res) { let dataListenerAdded = false let dataReadStarted = false - const { shouldCollectBody } = ctx - const bodyChunks = shouldCollectBody ? [] : null + const { shouldCollectBody, responseBodyCollection } = ctx + + let bodyChunks = null + let responseBodyIgnoredReason + + if (shouldCollectBody && responseBodyCollection) { + const evaluation = evaluateResponseBodyCollection(res, responseBodyCollection) + + if (evaluation.collect) { + bodyChunks = [] + } else { + responseBodyIgnoredReason = evaluation.reason + } + } const collectChunk = chunk => { - if (!shouldCollectBody || !chunk) return + if (!bodyChunks || !chunk) return if (typeof chunk === 'string') { bodyChunks.push(chunk) @@ -145,7 +213,7 @@ function setupResponseInstrumentation (ctx, res) { : Buffer.concat(bodyChunks) } - responseFinishChannel.publish({ ctx, res, body }) + responseFinishChannel.publish({ ctx, res, body, responseBodyIgnoredReason }) cleanup() } diff --git a/packages/datadog-instrumentations/test/http.spec.js b/packages/datadog-instrumentations/test/http.spec.js index 8cc194ceba..45e04ade50 100644 --- a/packages/datadog-instrumentations/test/http.spec.js +++ b/packages/datadog-instrumentations/test/http.spec.js @@ -1,6 +1,8 @@ 'use strict' const assert = require('node:assert') +const nodeHttp = require('node:http') +const { URL } = require('node:url') const { inspect } = require('node:util') const dc = require('dc-polyfill') const { describe, it, beforeEach, afterEach } = require('mocha') @@ -48,10 +50,19 @@ describe('client', () => { * Necessary because the tracer makes extra requests to the agent * and the same stub could be called multiple times */ + // originalUrl is the raw http.request first arg (string, URL, or options); uri is always a string. + function getRequestUrlString (args) { + if (!args) return undefined + const { originalUrl, uri } = args + if (typeof originalUrl === 'string') return originalUrl + if (originalUrl instanceof URL) return originalUrl.href + return uri + } + function getContextFromStubByUrl (url, stub) { for (const args of stub.args) { const arg = args[0] - if (arg.args?.originalUrl === url) { + if (getRequestUrlString(arg.args) === url) { return arg } } @@ -60,9 +71,7 @@ describe('client', () => { function stubHasResponseForUrl (url, stub) { return stub.args.some(([payload]) => { - const ctx = payload?.ctx - const originalUrl = ctx?.args?.originalUrl || ctx?.args?.uri - return originalUrl === url + return getRequestUrlString(payload?.ctx?.args) === url }) } @@ -194,11 +203,19 @@ describe('client', () => { }) describe('response finish channel', () => { - let responseFinishChannelCb + let responseFinishChannelCb, responseBodyCollection before(() => { http = require(httpSchema) url = `${httpSchema}://www.datadoghq.com` + responseBodyCollection = { + maxBytes: 1024 * 1024, + supportedMimeTypes: new Set([ + 'application/json', + 'text/json', + 'application/x-www-form-urlencoded', + ]), + } }) beforeEach(() => { @@ -210,17 +227,24 @@ describe('client', () => { responseFinishChannel.unsubscribe(responseFinishChannelCb) }) + function isLocalServerRequest (args) { + const requestUrl = getRequestUrlString(args) + return typeof requestUrl === 'string' && requestUrl.startsWith('http://127.0.0.1:') + } + function setCollectBody (ctx) { - if (ctx.args.originalUrl === url) { + const requestUrl = getRequestUrlString(ctx.args) + // External tests use `url` (datadoghq); local server tests use 127.0.0.1 and never match it. + if (requestUrl === url || isLocalServerRequest(ctx.args)) { ctx.shouldCollectBody = true + ctx.responseBodyCollection = responseBodyCollection } } - function getResponseFinishPayload (url, stub) { + function getResponseFinishPayload (requestUrl, stub) { for (const args of stub.args) { const payload = args[0] - const originalUrl = payload?.ctx?.args?.originalUrl || payload?.ctx?.args?.uri - if (originalUrl === url) { + if (getRequestUrlString(payload?.ctx?.args) === requestUrl) { return payload } } @@ -252,121 +276,247 @@ describe('client', () => { }) }) - it('collects and concatenates all chunks when ctx.shouldCollectBody is true', (done) => { - startChannelCb.callsFake(setCollectBody) - - const chunks = [] - http.get(url, (res) => { - res.on('data', (chunk) => { - chunks.push(chunk) + // Local server tests use plain http:// URLs; skip under the https schema loop. + const describeIfHttp = httpSchema === 'http' ? describe : describe.skip + + describeIfHttp('with local http server', () => { + function requestWithLocalServer ({ responseHeaders, responseBody, onResponse }, done) { + const server = nodeHttp.createServer((req, res) => { + res.writeHead(200, responseHeaders) + if (responseBody != null) { + res.end(responseBody) + } else { + res.end() + } }) - res.on('end', () => { - try { - const payload = getResponseFinishPayload(url, responseFinishChannelCb) - assert(Buffer.isBuffer(payload.body), `Expected Buffer, got ${inspect(payload.body)}`) - const expectedBody = Buffer.concat(chunks) - assert(payload.body.equals(expectedBody), `Got: ${inspect(payload.body)}`) + server.listen(0, () => { + const localUrl = `http://127.0.0.1:${server.address().port}/` + startChannelCb.callsFake(setCollectBody) - done() - } catch (e) { - done(e) - } + http.get(localUrl, (res) => { + onResponse(res, server, localUrl, done) + }).on('error', (err) => { + server.close(() => done(err)) + }) }) - }) - }) + } - it('collects and concatenates string chunks when using setEncoding', (done) => { - startChannelCb.callsFake(setCollectBody) + function requestWithLocalJsonBody (onResponse, done) { + const body = JSON.stringify({ ok: true }) + requestWithLocalServer({ + responseHeaders: { + 'content-type': 'application/json', + 'content-length': String(Buffer.byteLength(body)), + }, + responseBody: body, + onResponse, + }, done) + } - const chunks = [] - http.get(url, (res) => { - res.setEncoding('utf8') - const consume = () => { - let chunk - while ((chunk = res.read()) !== null) { + it('collects and concatenates all chunks when ctx.shouldCollectBody is true', (done) => { + const chunks = [] + requestWithLocalJsonBody((res, server, localUrl, done) => { + res.on('data', (chunk) => { chunks.push(chunk) + }) + res.on('end', () => { + try { + const payload = getResponseFinishPayload(localUrl, responseFinishChannelCb) + assert(Buffer.isBuffer(payload.body), `Expected Buffer, got ${inspect(payload.body)}`) + + const expectedBody = Buffer.concat(chunks) + assert(payload.body.equals(expectedBody), `Got: ${inspect(payload.body)}`) + + server.close(() => done()) + } catch (e) { + server.close(() => done(e)) + } + }) + }, done) + }) + + it('collects and concatenates string chunks when using setEncoding', (done) => { + const chunks = [] + requestWithLocalJsonBody((res, server, localUrl, done) => { + res.setEncoding('utf8') + const consume = () => { + let chunk + while ((chunk = res.read()) !== null) { + chunks.push(chunk) + } } - } - res.on('readable', consume) - res.on('end', () => { - try { - const payload = getResponseFinishPayload(url, responseFinishChannelCb) - assert.strictEqual(typeof payload.body, 'string') + res.on('readable', consume) + res.on('end', () => { + try { + const payload = getResponseFinishPayload(localUrl, responseFinishChannelCb) + assert.strictEqual(typeof payload.body, 'string') - const expectedBody = chunks.join('') - assert.strictEqual(payload.body, expectedBody) + const expectedBody = chunks.join('') + assert.strictEqual(payload.body, expectedBody) - done() - } catch (e) { - done(e) - } - }) + server.close(() => done()) + } catch (e) { + server.close(() => done(e)) + } + }) + }, done) }) - }) - - it('should collect data correctly when read and data are both used', (done) => { - startChannelCb.callsFake(setCollectBody) - const chunks = [] - http.get(url, (res) => { - res.setEncoding('utf8') - // eslint-disable-next-line sonarjs/no-identical-functions -- per-test chunks buffer - const consume = () => { - let chunk - while ((chunk = res.read()) !== null) { - chunks.push(chunk) + it('should collect data correctly when read and data are both used', (done) => { + const chunks = [] + requestWithLocalJsonBody((res, server, localUrl, done) => { + res.setEncoding('utf8') + // eslint-disable-next-line sonarjs/no-identical-functions -- per-test chunks buffer + const consume = () => { + let chunk + while ((chunk = res.read()) !== null) { + chunks.push(chunk) + } } - } - res.on('data', () => {}) + res.on('data', () => {}) - res.on('readable', consume) - res.on('end', () => { - try { - const payload = getResponseFinishPayload(url, responseFinishChannelCb) - assert.strictEqual(typeof payload.body, 'string') + res.on('readable', consume) + res.on('end', () => { + try { + const payload = getResponseFinishPayload(localUrl, responseFinishChannelCb) + assert.strictEqual(typeof payload.body, 'string') - const expectedBody = chunks.join('') - assert.strictEqual(payload.body, expectedBody) + const expectedBody = chunks.join('') + assert.strictEqual(payload.body, expectedBody) - done() - } catch (e) { - done(e) + server.close(() => done()) + } catch (e) { + server.close(() => done(e)) + } + }) + }, done) + }) + + it('should collect data correctly when read and data are both used in different order', (done) => { + const chunks = [] + requestWithLocalJsonBody((res, server, localUrl, done) => { + let onDataAdded = false + res.setEncoding('utf8') + const consume = () => { + let chunk + while ((chunk = res.read(100)) !== null) { + if (!onDataAdded) { + onDataAdded = true + res.on('data', () => {}) + } + chunks.push(chunk) + } } - }) + res.on('readable', consume) + res.on('end', () => { + try { + const payload = getResponseFinishPayload(localUrl, responseFinishChannelCb) + assert.strictEqual(typeof payload.body, 'string') + const expectedBody = chunks.join('') + assert.strictEqual(payload.body, expectedBody) + + server.close(() => done()) + } catch (e) { + server.close(() => done(e)) + } + }) + }, done) }) - }) - it('should collect data correctly when read and data are both used in different order', (done) => { - startChannelCb.callsFake(setCollectBody) + describe('response body limits', () => { + it('ignores body when content-type is unsupported', (done) => { + requestWithLocalServer({ + responseHeaders: { + 'content-type': 'image/png', + 'content-length': '4', + }, + responseBody: 'test', + onResponse (res, server, localUrl, done) { + res.on('data', () => {}) + res.on('end', () => { + try { + const payload = getResponseFinishPayload(localUrl, responseFinishChannelCb) + assert.strictEqual(payload.body, null) + assert.strictEqual(payload.responseBodyIgnoredReason, 'content_type_invalid') + server.close(() => done()) + } catch (e) { + server.close(() => done(e)) + } + }) + }, + }, done) + }) - const chunks = [] - http.get(url, (res) => { - let onDataAdded = false - res.setEncoding('utf8') - const consume = () => { - let chunk - while ((chunk = res.read(100)) !== null) { - if (!onDataAdded) { - onDataAdded = true + it('ignores body when content-length is missing', (done) => { + requestWithLocalServer({ + responseHeaders: { + 'content-type': 'application/json', + }, + responseBody: '{"ok":true}', + onResponse (res, server, localUrl, done) { res.on('data', () => {}) - } - chunks.push(chunk) - } - } - res.on('readable', consume) - res.on('end', () => { - try { - const payload = getResponseFinishPayload(url, responseFinishChannelCb) - assert.strictEqual(typeof payload.body, 'string') - const expectedBody = chunks.join('') - assert.strictEqual(payload.body, expectedBody) + res.on('end', () => { + try { + const payload = getResponseFinishPayload(localUrl, responseFinishChannelCb) + assert.strictEqual(payload.body, null) + assert.strictEqual(payload.responseBodyIgnoredReason, 'content_length_missing') + server.close(() => done()) + } catch (e) { + server.close(() => done(e)) + } + }) + }, + }, done) + }) - done() - } catch (e) { - done(e) - } + it('ignores body when content-length is zero', (done) => { + requestWithLocalServer({ + responseHeaders: { + 'content-type': 'application/json', + 'content-length': '0', + }, + responseBody: '', + onResponse (res, server, localUrl, done) { + res.on('data', () => {}) + res.on('end', () => { + try { + const payload = getResponseFinishPayload(localUrl, responseFinishChannelCb) + assert.strictEqual(payload.body, null) + assert.strictEqual(payload.responseBodyIgnoredReason, 'content_length_missing') + server.close(() => done()) + } catch (e) { + server.close(() => done(e)) + } + }) + }, + }, done) + }) + + it('ignores body when content-length exceeds maxBytes', (done) => { + responseBodyCollection.maxBytes = 10 + + requestWithLocalServer({ + responseHeaders: { + 'content-type': 'application/json', + 'content-length': '100', + }, + responseBody: 'x'.repeat(100), + onResponse (res, server, localUrl, done) { + res.on('data', () => {}) + res.on('end', () => { + try { + const payload = getResponseFinishPayload(localUrl, responseFinishChannelCb) + assert.strictEqual(payload.body, null) + assert.strictEqual(payload.responseBodyIgnoredReason, 'content_length_too_big') + server.close(() => done()) + } catch (e) { + server.close(() => done(e)) + } + }) + }, + }, done) }) }) }) diff --git a/packages/dd-trace/src/appsec/downstream_requests.js b/packages/dd-trace/src/appsec/downstream_requests.js index 4d6be26e6f..feac2dcf40 100644 --- a/packages/dd-trace/src/appsec/downstream_requests.js +++ b/packages/dd-trace/src/appsec/downstream_requests.js @@ -13,6 +13,20 @@ const { const KNUTH_FACTOR = 11400714819323199488n // eslint-disable-line unicorn/numeric-separators-style const UINT64_MAX = (1n << 64n) - 1n +const SUPPORTED_RESPONSE_BODY_MIME_TYPES = new Set([ + 'application/json', + 'text/json', + 'application/x-www-form-urlencoded', +]) + +const DEFAULT_MAX_DOWNSTREAM_BODY_BYTES = 10 * 1024 * 1024 + +const RESPONSE_BODY_IGNORED_METRIC_SUFFIX = { + content_type_invalid: 'content_type_invalid', + content_length_missing: 'content_length_missing', + content_length_too_big: 'content_length_too_big', +} + let config let samplingRate let globalRequestCounter @@ -209,6 +223,34 @@ function incrementDownstreamAnalysisCount (req) { } } +/** + * Returns response body collection limits for downstream instrumentation. + * @returns {{ maxBytes: number, supportedMimeTypes: Set }} + */ +function getResponseBodyCollectionConfig () { + return { + maxBytes: config?.appsec?.apiSecurity?.maxDownstreamBodyBytes ?? DEFAULT_MAX_DOWNSTREAM_BODY_BYTES, + supportedMimeTypes: SUPPORTED_RESPONSE_BODY_MIME_TYPES, + } +} + +/** + * Increments a response-body-ignored counter on the service-entry span. + * @param {import('http').IncomingMessage} req originating request. + * @param {string} reason ignore reason key. + */ +function recordResponseBodyIgnored (req, reason) { + const suffix = RESPONSE_BODY_IGNORED_METRIC_SUFFIX[reason] + if (!suffix) return + + const span = web.root(req) + if (!span) return + + const tag = `_dd.appsec.downstream_request.response_body_ignored.${suffix}` + const current = Number(span.context()._tags[tag]) || 0 + span.setTag(tag, current + 1) +} + /** * Returns the HTTP method to use for a downstream request, defaulting to GET. * @param {string} method method supplied in the outgoing request options. @@ -293,10 +335,14 @@ module.exports = { shouldSampleBody, handleRedirectResponse, incrementDownstreamAnalysisCount, + getResponseBodyCollectionConfig, + recordResponseBodyIgnored, extractRequestData, extractResponseData, // exports for tests parseBody, getMethod, storeRedirectBodyCollectionDecision, + SUPPORTED_RESPONSE_BODY_MIME_TYPES, + RESPONSE_BODY_IGNORED_METRIC_SUFFIX, } diff --git a/packages/dd-trace/src/appsec/rasp/ssrf.js b/packages/dd-trace/src/appsec/rasp/ssrf.js index 0d774635f3..181ae462d3 100644 --- a/packages/dd-trace/src/appsec/rasp/ssrf.js +++ b/packages/dd-trace/src/appsec/rasp/ssrf.js @@ -39,6 +39,10 @@ function analyzeSsrf (ctx) { // Determine if we should collect the response body based on sampling rate and redirect URL ctx.shouldCollectBody = downstream.shouldSampleBody(req, outgoingUrl) + if (ctx.shouldCollectBody) { + ctx.responseBodyCollection = downstream.getResponseBodyCollectionConfig() + } + const requestAddresses = downstream.extractRequestData(ctx) const ephemeral = { @@ -63,15 +67,21 @@ function analyzeSsrf (ctx) { * body: string|Buffer|null * }} payload event payload from the channel. */ -function handleResponseFinish ({ ctx, res, body }) { +function handleResponseFinish ({ ctx, res, body, responseBodyIgnoredReason }) { // downstream response object if (!res) return const originatingRequest = getActiveRequest() if (!originatingRequest) return - // Skip body analysis for redirect responses - const evaluateBody = ctx.shouldCollectBody && !downstream.handleRedirectResponse(originatingRequest, res) + if (responseBodyIgnoredReason) { + downstream.recordResponseBodyIgnored(originatingRequest, responseBodyIgnoredReason) + } + + // Skip body analysis for redirect responses or when guards rejected collection + const evaluateBody = ctx.shouldCollectBody && + !responseBodyIgnoredReason && + !downstream.handleRedirectResponse(originatingRequest, res) const responseBody = evaluateBody ? body : null runResponseEvaluation(res, originatingRequest, responseBody) } diff --git a/packages/dd-trace/src/config/generated-config-types.d.ts b/packages/dd-trace/src/config/generated-config-types.d.ts index 172393ba7a..574c45afa1 100644 --- a/packages/dd-trace/src/config/generated-config-types.d.ts +++ b/packages/dd-trace/src/config/generated-config-types.d.ts @@ -11,6 +11,7 @@ export interface GeneratedConfig { enabled: boolean; endpointCollectionEnabled: boolean; endpointCollectionMessageLimit: number; + maxDownstreamBodyBytes: number; maxDownstreamRequestBodyAnalysis: number; sampleDelay: number; }; diff --git a/packages/dd-trace/src/config/supported-configurations.json b/packages/dd-trace/src/config/supported-configurations.json index 772223c808..d1b21f72af 100644 --- a/packages/dd-trace/src/config/supported-configurations.json +++ b/packages/dd-trace/src/config/supported-configurations.json @@ -166,6 +166,14 @@ "default": "1" } ], + "DD_API_SECURITY_MAX_DOWNSTREAM_BODY_BYTES": [ + { + "implementation": "A", + "type": "int", + "internalPropertyName": "appsec.apiSecurity.maxDownstreamBodyBytes", + "default": "10485760" + } + ], "DD_API_SECURITY_SAMPLE_DELAY": [ { "implementation": "A", diff --git a/packages/dd-trace/test/appsec/downstream_requests.spec.js b/packages/dd-trace/test/appsec/downstream_requests.spec.js index c908de04c8..97d6dcb063 100644 --- a/packages/dd-trace/test/appsec/downstream_requests.spec.js +++ b/packages/dd-trace/test/appsec/downstream_requests.spec.js @@ -20,6 +20,7 @@ describe('appsec downstream_requests', () => { enabled: true, downstreamBodyAnalysisSampleRate: 1, maxDownstreamRequestBodyAnalysis: 1, + maxDownstreamBodyBytes: 1024, }, }, } @@ -361,6 +362,58 @@ describe('appsec downstream_requests', () => { }) }) + describe('getResponseBodyCollectionConfig', () => { + it('returns configured max bytes and supported mime types', () => { + const collectionConfig = downstream.getResponseBodyCollectionConfig() + + assert.strictEqual(collectionConfig.maxBytes, 1024) + assert.ok(collectionConfig.supportedMimeTypes.has('application/json')) + assert.ok(collectionConfig.supportedMimeTypes.has('application/x-www-form-urlencoded')) + }) + }) + + describe('recordResponseBodyIgnored', () => { + let web + let span + + beforeEach(() => { + web = require('../../src/plugins/util/web') + const tags = {} + span = { + setTag (key, value) { + tags[key] = value + }, + context () { + return { _tags: tags } + }, + } + }) + + afterEach(() => { + sinon.restore() + }) + + it('increments ignored-body metric on span', () => { + const webRootStub = sinon.stub(web, 'root').returns(span) + const tag = '_dd.appsec.downstream_request.response_body_ignored.content_type_invalid' + + downstream.recordResponseBodyIgnored(req, 'content_type_invalid') + downstream.recordResponseBodyIgnored(req, 'content_type_invalid') + + assert.strictEqual(span.context()._tags[tag], 2) + webRootStub.restore() + }) + + it('does not write tag for unknown reason', () => { + const webRootStub = sinon.stub(web, 'root').returns(span) + + downstream.recordResponseBodyIgnored(req, 'unknown_reason') + + assert.strictEqual(Object.keys(span.context()._tags).length, 0) + webRootStub.restore() + }) + }) + describe('incrementDownstreamAnalysisCount', () => { let web let span diff --git a/packages/dd-trace/test/appsec/rasp/ssrf.spec.js b/packages/dd-trace/test/appsec/rasp/ssrf.spec.js index c03d04562c..1fab139044 100644 --- a/packages/dd-trace/test/appsec/rasp/ssrf.spec.js +++ b/packages/dd-trace/test/appsec/rasp/ssrf.spec.js @@ -1,5 +1,6 @@ 'use strict' +const assert = require('node:assert/strict') const { EventEmitter } = require('events') const { describe, it, beforeEach, afterEach } = require('mocha') const sinon = require('sinon') @@ -15,7 +16,7 @@ const { withRequest } = require('../../../src/appsec/store') const DEFAULT_URL = 'http://example.com' -describe('RASP - ssrf.js', () => { +describe.only('RASP - ssrf.js', () => { let waf let downstream let ssrf @@ -61,6 +62,11 @@ describe('RASP - ssrf.js', () => { enable: sinon.stub(), disable: sinon.stub(), shouldSampleBody: sinon.stub().returns(true), + getResponseBodyCollectionConfig: sinon.stub().returns({ + maxBytes: 10485760, + supportedMimeTypes: new Set(['application/json']), + }), + recordResponseBodyIgnored: sinon.stub(), handleRedirectResponse: sinon.stub().returns(false), extractRequestData: sinon.stub().returns({}), extractResponseData: sinon.stub().returns({}), @@ -166,6 +172,20 @@ describe('RASP - ssrf.js', () => { publishRequestStart({ ctx, includeBodies: false }) sinon.assert.match(ctx.shouldCollectBody, false) + assert.strictEqual(ctx.responseBodyCollection, undefined) + }) + + it('sets responseBodyCollection when body sampling is enabled', () => { + const ctx = makeCtx() + stubStore({}, {}) + + publishRequestStart({ ctx, includeBodies: true }) + + sinon.assert.calledOnce(downstream.getResponseBodyCollectionConfig) + sinon.assert.match(ctx.responseBodyCollection, { + maxBytes: 10485760, + supportedMimeTypes: sinon.match.instanceOf(Set), + }) }) it('evaluates response and passes body through to extractResponseData', () => { @@ -305,5 +325,31 @@ describe('RASP - ssrf.js', () => { sinon.assert.calledWith(downstream.extractResponseData, response, null) }) + + it('records ignored body metric and skips WAF body when guards reject collection', () => { + const ctx = makeCtx() + const { req } = stubStore({}, {}) + + downstream.extractResponseData.returns({ + [addresses.HTTP_OUTGOING_RESPONSE_STATUS]: '200', + }) + waf.run.returns({ events: [] }) + + publishRequestStart({ ctx, includeBodies: true }) + + const response = createResponse() + const body = Buffer.from('{"ok":true}') + + httpClientResponseFinish.publish({ + ctx, + res: response, + body, + responseBodyIgnoredReason: 'content_type_invalid', + }) + + sinon.assert.calledOnceWithExactly( + downstream.recordResponseBodyIgnored, req, 'content_type_invalid') + sinon.assert.calledWith(downstream.extractResponseData, response, null) + }) }) }) diff --git a/packages/dd-trace/test/config/index.spec.js b/packages/dd-trace/test/config/index.spec.js index 14e997cc9f..607a852831 100644 --- a/packages/dd-trace/test/config/index.spec.js +++ b/packages/dd-trace/test/config/index.spec.js @@ -682,6 +682,7 @@ describe('Config', () => { endpointCollectionMessageLimit: 300, downstreamBodyAnalysisSampleRate: 0.5, maxDownstreamRequestBodyAnalysis: 1, + maxDownstreamBodyBytes: 10485760, }, blockedTemplateHtml: undefined, blockedTemplateJson: undefined, @@ -822,6 +823,7 @@ describe('Config', () => { { name: 'DD_API_SECURITY_ENDPOINT_COLLECTION_MESSAGE_LIMIT', value: 300, origin: 'default' }, { name: 'DD_API_SECURITY_DOWNSTREAM_BODY_ANALYSIS_SAMPLE_RATE', value: 0.5, origin: 'default' }, { name: 'DD_API_SECURITY_MAX_DOWNSTREAM_REQUEST_BODY_ANALYSIS', value: 1, origin: 'default' }, + { name: 'DD_API_SECURITY_MAX_DOWNSTREAM_BODY_BYTES', value: 10485760, origin: 'default' }, { name: 'DD_APPSEC_HTTP_BLOCKED_TEMPLATE_HTML', value: null, origin: 'default' }, { name: 'DD_APPSEC_HTTP_BLOCKED_TEMPLATE_JSON', value: null, origin: 'default' }, { name: 'DD_APPSEC_ENABLED', value: null, origin: 'default' }, @@ -1032,6 +1034,7 @@ describe('Config', () => { process.env.DD_API_SECURITY_ENDPOINT_COLLECTION_MESSAGE_LIMIT = '500' process.env.DD_API_SECURITY_DOWNSTREAM_BODY_ANALYSIS_SAMPLE_RATE = '0.75' process.env.DD_API_SECURITY_MAX_DOWNSTREAM_REQUEST_BODY_ANALYSIS = '2' + process.env.DD_API_SECURITY_MAX_DOWNSTREAM_BODY_BYTES = '2048' process.env.DD_APM_TRACING_ENABLED = 'false' process.env.DD_APP_KEY = 'myAppKey' process.env.DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING = 'extended' @@ -1156,6 +1159,7 @@ describe('Config', () => { endpointCollectionMessageLimit: 500, downstreamBodyAnalysisSampleRate: 0.75, maxDownstreamRequestBodyAnalysis: 2, + maxDownstreamBodyBytes: 2048, }, blockedTemplateGraphql: BLOCKED_TEMPLATE_GRAPHQL, blockedTemplateHtml: BLOCKED_TEMPLATE_HTML, @@ -1317,6 +1321,7 @@ describe('Config', () => { { name: 'DD_API_SECURITY_ENDPOINT_COLLECTION_MESSAGE_LIMIT', value: 500, origin: 'env_var' }, { name: 'DD_API_SECURITY_DOWNSTREAM_BODY_ANALYSIS_SAMPLE_RATE', value: 0.75, origin: 'env_var' }, { name: 'DD_API_SECURITY_MAX_DOWNSTREAM_REQUEST_BODY_ANALYSIS', value: 2, origin: 'env_var' }, + { name: 'DD_API_SECURITY_MAX_DOWNSTREAM_BODY_BYTES', value: 2048, origin: 'env_var' }, { name: 'DD_APPSEC_HTTP_BLOCKED_TEMPLATE_HTML', value: BLOCKED_TEMPLATE_HTML_PATH, origin: 'env_var' }, { name: 'DD_APPSEC_HTTP_BLOCKED_TEMPLATE_JSON', value: BLOCKED_TEMPLATE_JSON_PATH, origin: 'env_var' }, { name: 'DD_APPSEC_ENABLED', value: true, origin: 'env_var' }, @@ -2500,6 +2505,7 @@ describe('Config', () => { endpointCollectionMessageLimit: 500, downstreamBodyAnalysisSampleRate: 0.5, maxDownstreamRequestBodyAnalysis: 1, + maxDownstreamBodyBytes: 10485760, }, blockedTemplateGraphql: BLOCKED_TEMPLATE_GRAPHQL, blockedTemplateHtml: BLOCKED_TEMPLATE_HTML, From 2a1a590ac8b37e8a65e1fc34a035da9597566989 Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Wed, 27 May 2026 14:20:03 +0200 Subject: [PATCH 2/7] fix lint --- .../dd-trace/src/appsec/downstream_requests.js | 15 +++++++++++++-- .../test/appsec/downstream_requests.spec.js | 14 +++++--------- 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/packages/dd-trace/src/appsec/downstream_requests.js b/packages/dd-trace/src/appsec/downstream_requests.js index feac2dcf40..849398a1ef 100644 --- a/packages/dd-trace/src/appsec/downstream_requests.js +++ b/packages/dd-trace/src/appsec/downstream_requests.js @@ -32,6 +32,7 @@ let samplingRate let globalRequestCounter let bodyAnalysisCount let downstreamAnalysisCount +let responseBodyIgnoredCount let redirectBodyCollectionDecisions function enable (_config) { @@ -39,6 +40,7 @@ function enable (_config) { globalRequestCounter = 0n bodyAnalysisCount = new WeakMap() downstreamAnalysisCount = new WeakMap() + responseBodyIgnoredCount = new WeakMap() redirectBodyCollectionDecisions = new WeakMap() const bodyAnalysisSampleRate = config.appsec.apiSecurity?.downstreamBodyAnalysisSampleRate @@ -56,6 +58,7 @@ function disable () { globalRequestCounter = null bodyAnalysisCount = null downstreamAnalysisCount = null + responseBodyIgnoredCount = null redirectBodyCollectionDecisions = null } @@ -247,8 +250,16 @@ function recordResponseBodyIgnored (req, reason) { if (!span) return const tag = `_dd.appsec.downstream_request.response_body_ignored.${suffix}` - const current = Number(span.context()._tags[tag]) || 0 - span.setTag(tag, current + 1) + let counts = responseBodyIgnoredCount.get(req) + if (!counts) { + counts = {} + responseBodyIgnoredCount.set(req, counts) + } + + const current = counts[tag] || 0 + const next = current + 1 + counts[tag] = next + span.setTag(tag, next) } /** diff --git a/packages/dd-trace/test/appsec/downstream_requests.spec.js b/packages/dd-trace/test/appsec/downstream_requests.spec.js index 97d6dcb063..2ef6cd59a8 100644 --- a/packages/dd-trace/test/appsec/downstream_requests.spec.js +++ b/packages/dd-trace/test/appsec/downstream_requests.spec.js @@ -378,14 +378,8 @@ describe('appsec downstream_requests', () => { beforeEach(() => { web = require('../../src/plugins/util/web') - const tags = {} span = { - setTag (key, value) { - tags[key] = value - }, - context () { - return { _tags: tags } - }, + setTag: sinon.stub(), } }) @@ -400,7 +394,9 @@ describe('appsec downstream_requests', () => { downstream.recordResponseBodyIgnored(req, 'content_type_invalid') downstream.recordResponseBodyIgnored(req, 'content_type_invalid') - assert.strictEqual(span.context()._tags[tag], 2) + sinon.assert.calledTwice(span.setTag) + sinon.assert.calledWith(span.setTag, tag, 1) + sinon.assert.calledWith(span.setTag, tag, 2) webRootStub.restore() }) @@ -409,7 +405,7 @@ describe('appsec downstream_requests', () => { downstream.recordResponseBodyIgnored(req, 'unknown_reason') - assert.strictEqual(Object.keys(span.context()._tags).length, 0) + sinon.assert.notCalled(span.setTag) webRootStub.restore() }) }) From 416a7ef309aef0658bb87ddb149620c4874364bb Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Wed, 27 May 2026 14:51:55 +0200 Subject: [PATCH 3/7] fix comment --- packages/datadog-instrumentations/test/http.spec.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/datadog-instrumentations/test/http.spec.js b/packages/datadog-instrumentations/test/http.spec.js index 45e04ade50..e6e48a9bd0 100644 --- a/packages/datadog-instrumentations/test/http.spec.js +++ b/packages/datadog-instrumentations/test/http.spec.js @@ -46,10 +46,6 @@ describe('client', () => { errorChannel.unsubscribe(errorChannelCb) }) - /* - * Necessary because the tracer makes extra requests to the agent - * and the same stub could be called multiple times - */ // originalUrl is the raw http.request first arg (string, URL, or options); uri is always a string. function getRequestUrlString (args) { if (!args) return undefined @@ -59,6 +55,10 @@ describe('client', () => { return uri } + /* + * Necessary because the tracer makes extra requests to the agent + * and the same stub could be called multiple times + */ function getContextFromStubByUrl (url, stub) { for (const args of stub.args) { const arg = args[0] From 49149ea9884e8fa3c0124a665edd5dd57b617e00 Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Thu, 28 May 2026 16:05:46 +0200 Subject: [PATCH 4/7] Do not require extra http --- packages/datadog-instrumentations/test/http.spec.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/datadog-instrumentations/test/http.spec.js b/packages/datadog-instrumentations/test/http.spec.js index e6e48a9bd0..fa08718b92 100644 --- a/packages/datadog-instrumentations/test/http.spec.js +++ b/packages/datadog-instrumentations/test/http.spec.js @@ -1,7 +1,6 @@ 'use strict' const assert = require('node:assert') -const nodeHttp = require('node:http') const { URL } = require('node:url') const { inspect } = require('node:util') const dc = require('dc-polyfill') @@ -281,7 +280,7 @@ describe('client', () => { describeIfHttp('with local http server', () => { function requestWithLocalServer ({ responseHeaders, responseBody, onResponse }, done) { - const server = nodeHttp.createServer((req, res) => { + const server = http.createServer((req, res) => { res.writeHead(200, responseHeaders) if (responseBody != null) { res.end(responseBody) From 37afb8fceb195dffc126f6e548b7b9928479d75c Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Thu, 28 May 2026 16:45:16 +0200 Subject: [PATCH 5/7] small fixes --- packages/datadog-instrumentations/test/http.spec.js | 2 +- packages/dd-trace/test/appsec/rasp/ssrf.spec.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/datadog-instrumentations/test/http.spec.js b/packages/datadog-instrumentations/test/http.spec.js index fa08718b92..2d2e663f98 100644 --- a/packages/datadog-instrumentations/test/http.spec.js +++ b/packages/datadog-instrumentations/test/http.spec.js @@ -325,7 +325,7 @@ describe('client', () => { assert(Buffer.isBuffer(payload.body), `Expected Buffer, got ${inspect(payload.body)}`) const expectedBody = Buffer.concat(chunks) - assert(payload.body.equals(expectedBody), `Got: ${inspect(payload.body)}`) + assert.deepStrictEqual(payload.body, expectedBody) server.close(() => done()) } catch (e) { diff --git a/packages/dd-trace/test/appsec/rasp/ssrf.spec.js b/packages/dd-trace/test/appsec/rasp/ssrf.spec.js index 1fab139044..d1bc8a8f98 100644 --- a/packages/dd-trace/test/appsec/rasp/ssrf.spec.js +++ b/packages/dd-trace/test/appsec/rasp/ssrf.spec.js @@ -16,7 +16,7 @@ const { withRequest } = require('../../../src/appsec/store') const DEFAULT_URL = 'http://example.com' -describe.only('RASP - ssrf.js', () => { +describe('RASP - ssrf.js', () => { let waf let downstream let ssrf From 0ee17b349d3eff9d74b9a5e2a035c6bec404c290 Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Fri, 29 May 2026 12:38:21 +0200 Subject: [PATCH 6/7] Fix redirect body collection when response guards reject the hop Always run handleRedirectResponse before applying body guard outcomes so sampled redirect chains still store the Location decision. Skip response_body_ignored metrics on redirect hops where body analysis is deferred to the follow-up request. Co-authored-by: Cursor --- packages/dd-trace/src/appsec/rasp/ssrf.js | 10 ++++---- .../dd-trace/test/appsec/rasp/ssrf.spec.js | 25 +++++++++++++++++++ 2 files changed, 30 insertions(+), 5 deletions(-) diff --git a/packages/dd-trace/src/appsec/rasp/ssrf.js b/packages/dd-trace/src/appsec/rasp/ssrf.js index 181ae462d3..b1ece86136 100644 --- a/packages/dd-trace/src/appsec/rasp/ssrf.js +++ b/packages/dd-trace/src/appsec/rasp/ssrf.js @@ -74,14 +74,14 @@ function handleResponseFinish ({ ctx, res, body, responseBodyIgnoredReason }) { const originatingRequest = getActiveRequest() if (!originatingRequest) return - if (responseBodyIgnoredReason) { + const shouldCollectBodyAfterRedirect = ctx.shouldCollectBody && + downstream.handleRedirectResponse(originatingRequest, res) + + if (!shouldCollectBodyAfterRedirect && responseBodyIgnoredReason) { downstream.recordResponseBodyIgnored(originatingRequest, responseBodyIgnoredReason) } - // Skip body analysis for redirect responses or when guards rejected collection - const evaluateBody = ctx.shouldCollectBody && - !responseBodyIgnoredReason && - !downstream.handleRedirectResponse(originatingRequest, res) + const evaluateBody = ctx.shouldCollectBody && !shouldCollectBodyAfterRedirect && !responseBodyIgnoredReason const responseBody = evaluateBody ? body : null runResponseEvaluation(res, originatingRequest, responseBody) } diff --git a/packages/dd-trace/test/appsec/rasp/ssrf.spec.js b/packages/dd-trace/test/appsec/rasp/ssrf.spec.js index d1bc8a8f98..7d76ddd616 100644 --- a/packages/dd-trace/test/appsec/rasp/ssrf.spec.js +++ b/packages/dd-trace/test/appsec/rasp/ssrf.spec.js @@ -326,6 +326,31 @@ describe('RASP - ssrf.js', () => { sinon.assert.calledWith(downstream.extractResponseData, response, null) }) + it('stores redirect decision when guards reject collection on a redirect response', () => { + const ctx = makeCtx() + const { req } = stubStore({}, {}) + + downstream.handleRedirectResponse.returns(true) + downstream.extractResponseData.returns({ + [addresses.HTTP_OUTGOING_RESPONSE_STATUS]: '302', + }) + waf.run.returns({ events: [] }) + + publishRequestStart({ ctx, includeBodies: true }) + + const response = createResponse({ statusCode: 302, headers: { location: 'http://example.com/redirect' } }) + httpClientResponseFinish.publish({ + ctx, + res: response, + body: null, + responseBodyIgnoredReason: 'content_length_missing', + }) + + sinon.assert.calledOnceWithExactly(downstream.handleRedirectResponse, req, response) + sinon.assert.notCalled(downstream.recordResponseBodyIgnored) + sinon.assert.calledWith(downstream.extractResponseData, response, null) + }) + it('records ignored body metric and skips WAF body when guards reject collection', () => { const ctx = makeCtx() const { req } = stubStore({}, {}) From 1437d2045f46e43b65d2bfd64aac32cd43f47a17 Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Tue, 2 Jun 2026 10:06:41 +0200 Subject: [PATCH 7/7] Apply PR comments --- .../datadog-instrumentations/src/http/client.js | 15 ++++++++++++--- .../dd-trace/src/appsec/downstream_requests.js | 4 +--- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/packages/datadog-instrumentations/src/http/client.js b/packages/datadog-instrumentations/src/http/client.js index 6b2c35b9ba..2e6f6d7622 100644 --- a/packages/datadog-instrumentations/src/http/client.js +++ b/packages/datadog-instrumentations/src/http/client.js @@ -101,15 +101,24 @@ function evaluateResponseBodyCollection (res, responseBodyCollection) { } const declaredContentLength = parseContentLength(res.headers?.['content-length']) + + const evaluationResult = { collect: true, reason: undefined } + if (declaredContentLength == null || declaredContentLength === 0) { - return { collect: false, reason: 'content_length_missing' } + evaluationResult.collect = false + evaluationResult.reason = 'content_length_missing' + + return evaluationResult } if (declaredContentLength > maxBytes) { - return { collect: false, reason: 'content_length_too_big' } + evaluationResult.collect = false + evaluationResult.reason = 'content_length_too_big' + + return evaluationResult } - return { collect: true } + return evaluationResult } /** diff --git a/packages/dd-trace/src/appsec/downstream_requests.js b/packages/dd-trace/src/appsec/downstream_requests.js index 849398a1ef..666119c67d 100644 --- a/packages/dd-trace/src/appsec/downstream_requests.js +++ b/packages/dd-trace/src/appsec/downstream_requests.js @@ -19,8 +19,6 @@ const SUPPORTED_RESPONSE_BODY_MIME_TYPES = new Set([ 'application/x-www-form-urlencoded', ]) -const DEFAULT_MAX_DOWNSTREAM_BODY_BYTES = 10 * 1024 * 1024 - const RESPONSE_BODY_IGNORED_METRIC_SUFFIX = { content_type_invalid: 'content_type_invalid', content_length_missing: 'content_length_missing', @@ -232,7 +230,7 @@ function incrementDownstreamAnalysisCount (req) { */ function getResponseBodyCollectionConfig () { return { - maxBytes: config?.appsec?.apiSecurity?.maxDownstreamBodyBytes ?? DEFAULT_MAX_DOWNSTREAM_BODY_BYTES, + maxBytes: config?.appsec.apiSecurity.maxDownstreamBodyBytes, supportedMimeTypes: SUPPORTED_RESPONSE_BODY_MIME_TYPES, } }