From 3ddd1074b9e43b87da5eba14e671a154f23d521b Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Tue, 4 Jun 2024 17:00:29 +0200 Subject: [PATCH 01/41] http client instrumentation for AbortController --- .../src/http/client.js | 43 +++- .../test/http.spec.js | 188 ++++++++++++++++++ 2 files changed, 229 insertions(+), 2 deletions(-) create mode 100644 packages/datadog-instrumentations/test/http.spec.js diff --git a/packages/datadog-instrumentations/src/http/client.js b/packages/datadog-instrumentations/src/http/client.js index 0f2d452f51d..cdd48d6a77d 100644 --- a/packages/datadog-instrumentations/src/http/client.js +++ b/packages/datadog-instrumentations/src/http/client.js @@ -7,6 +7,7 @@ const { channel, addHook } = require('../helpers/instrument') const shimmer = require('../../../datadog-shimmer') const log = require('../../../dd-trace/src/log') +const { storage } = require('../../../datadog-core') const startChannel = channel('apm:http:client:request:start') const finishChannel = channel('apm:http:client:request:finish') @@ -25,6 +26,30 @@ function hookFn (http) { return http } +let ClientRequest +function noop () {} + +function createAbortedClientRequest (http, args) { + if (!ClientRequest) { + const store = storage.getStore() + storage.enterWith({ noop: true }) + + const clientRequest = http.get('') + clientRequest.on('error', noop) + ClientRequest = Object.getPrototypeOf(clientRequest).constructor + + storage.enterWith(store) + } + + return new ClientRequest({ + _defaultAgent: http.globalAgent, // needed to support http and https + ...args.options, + agent: { + addRequest: noop + } + }) +} + function patch (http, methodName) { shimmer.wrap(http, methodName, instrumentRequest) @@ -43,7 +68,11 @@ function patch (http, methodName) { return request.apply(this, arguments) } - const ctx = { args, http } + const abortData = { + abortController: new AbortController() + } + + const ctx = { args, http, abortData } return startChannel.runStores(ctx, () => { let finished = false @@ -67,7 +96,17 @@ function patch (http, methodName) { } try { - const req = request.call(this, options, callback) + let req + if (abortData.abortController?.signal.aborted) { + req = createAbortedClientRequest(http, args) + + process.nextTick(() => { + req.emit('error', abortData.error || new Error('Aborted')) + }) + } else { + req = request.call(this, options, callback) + } + const emit = req.emit const setTimeout = req.setTimeout diff --git a/packages/datadog-instrumentations/test/http.spec.js b/packages/datadog-instrumentations/test/http.spec.js new file mode 100644 index 00000000000..b3a2150672b --- /dev/null +++ b/packages/datadog-instrumentations/test/http.spec.js @@ -0,0 +1,188 @@ +'use strict' + +const { assert } = require('chai') +const dc = require('dc-polyfill') + +const agent = require('../../dd-trace/test/plugins/agent') +describe('client', () => { + let url, http, startChannelCb, finishChannelCb, endChannelCb, asyncStartChannelCb, errorChannelCb + + const startChannel = dc.channel('apm:http:client:request:start') + const finishChannel = dc.channel('apm:http:client:request:finish') + const endChannel = dc.channel('apm:http:client:request:end') + const asyncStartChannel = dc.channel('apm:http:client:request:asyncStart') + const errorChannel = dc.channel('apm:http:client:request:error') + + before(async () => { + await agent.load('http') + }) + + after(() => { + return agent.close() + }) + + beforeEach(() => { + startChannelCb = sinon.stub() + startChannel.subscribe(startChannelCb) + finishChannelCb = sinon.stub() + finishChannel.subscribe(finishChannelCb) + endChannelCb = sinon.stub() + endChannel.subscribe(endChannelCb) + asyncStartChannelCb = sinon.stub() + asyncStartChannel.subscribe(asyncStartChannelCb) + errorChannelCb = sinon.stub() + errorChannel.subscribe(errorChannelCb) + }) + + afterEach(() => { + startChannel.unsubscribe(startChannelCb) + finishChannel.unsubscribe(finishChannelCb) + endChannel.unsubscribe(endChannelCb) + asyncStartChannel.unsubscribe(asyncStartChannelCb) + errorChannel.unsubscribe(errorChannelCb) + }) + + /* + * Necessary because the tracer makes extra requests to the agent + * and the same stub could be called multiple times + */ + function getContextFromStubByUrl (url, stub) { + for (let i = 0; i < stub.args.length; i++) { + const arg = stub.args[i][0] + if (arg.args?.originalUrl === url) { + return arg + } + } + + return null + } + + ['http', 'https'].forEach((httpSchema) => { + describe(`using ${httpSchema}`, () => { + describe('abort controller', () => { + function abortCallback (ctx) { + if (ctx.args.originalUrl === url) { + ctx.abortData.abortController.abort() + } + } + + before(() => { + http = require(httpSchema) + url = `${httpSchema}://www.datadoghq.com` + }) + + it('abortData is sent on startChannel', (done) => { + http.get(url, (res) => { + res.on('data', () => { + }) + res.on('end', () => { + done() + }) + }) + + sinon.assert.called(startChannelCb) + const ctx = getContextFromStubByUrl(url, startChannelCb) + assert.isNotNull(ctx) + assert.instanceOf(ctx.abortData.abortController, AbortController) + }) + + it('Request is aborted with default error', (done) => { + startChannelCb.callsFake(abortCallback) + + const cr = http.get(url, () => { + done('Request should be blocked') + }) + cr.on('error', (e) => { + try { + assert.instanceOf(e, Error) + assert.strictEqual(e.message, 'Aborted') + done() + } catch (e) { + done(e) + } + }) + }) + + it('Request is aborted with custom error', (done) => { + class CustomError extends Error { + } + + startChannelCb.callsFake((ctx) => { + if (ctx.args.originalUrl === url) { + ctx.abortData.abortController.abort() + ctx.abortData.error = new CustomError('Custom error') + } + }) + + const cr = http.get(url, () => { + done('Request should be blocked') + }) + cr.on('error', (e) => { + try { + assert.instanceOf(e, CustomError) + assert.strictEqual(e.message, 'Custom error') + done() + } catch (e) { + done(e) + } + }) + }) + + it('Error is sent on errorChannel on abort', (done) => { + startChannelCb.callsFake(abortCallback) + + const cr = http.get(url, () => { + done('Request should be blocked') + }) + cr.on('error', () => { + try { + sinon.assert.calledOnce(errorChannelCb) + assert.instanceOf(errorChannelCb.firstCall.args[0].error, Error) + done() + } catch (e) { + done(e) + } + }) + }) + + it('endChannel is called on abort', (done) => { + startChannelCb.callsFake(abortCallback) + + const cr = http.get(url, () => { + done('Request should be blocked') + }) + cr.on('error', () => { + try { + sinon.assert.called(endChannelCb) + const ctx = getContextFromStubByUrl(url, endChannelCb) + assert.strictEqual(ctx.args.originalUrl, url) + done() + } catch (e) { + done(e) + } + }) + }) + + it('asyncStartChannel is not called on abort', (done) => { + startChannelCb.callsFake(abortCallback) + + const cr = http.get(url, () => { + done('Request should be blocked') + }) + cr.on('error', () => { + try { + // Necessary because the tracer makes extra requests to the agent + if (asyncStartChannelCb.called) { + const ctx = getContextFromStubByUrl(url, asyncStartChannelCb) + assert.isNull(ctx) + } + done() + } catch (e) { + done(e.message) + } + }) + }).timeout(1000) + }) + }) + }) +}) From 9b5ea8393c0e29a30740d1a39e79cd1d4b1d9108 Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Thu, 6 Jun 2024 14:01:09 +0200 Subject: [PATCH 02/41] exploit prevention blocking - missing safeguards for writes after blocks --- integration-tests/appsec/index.spec.js | 113 ++++++++++++++++++ integration-tests/appsec/rasp/index.js | 97 +++++++++++++++ integration-tests/appsec/rasp/rasp_rules.json | 59 +++++++++ integration-tests/appsec/rasp/streamtest.txt | 1 + integration-tests/helpers.js | 5 +- packages/dd-trace/src/appsec/blocking.js | 2 + packages/dd-trace/src/appsec/rasp.js | 43 ++++++- .../test/appsec/rasp.express.plugin.spec.js | 58 +++++---- 8 files changed, 350 insertions(+), 28 deletions(-) create mode 100644 integration-tests/appsec/index.spec.js create mode 100644 integration-tests/appsec/rasp/index.js create mode 100644 integration-tests/appsec/rasp/rasp_rules.json create mode 100644 integration-tests/appsec/rasp/streamtest.txt diff --git a/integration-tests/appsec/index.spec.js b/integration-tests/appsec/index.spec.js new file mode 100644 index 00000000000..bd38eb0468d --- /dev/null +++ b/integration-tests/appsec/index.spec.js @@ -0,0 +1,113 @@ +'use strict' + +const getPort = require('get-port') +const { createSandbox, FakeAgent, spawnProc } = require('../helpers') +const path = require('path') +const Axios = require('axios') +const { assert } = require('chai') + +describe('RASP', () => { + let axios, sandbox, cwd, appPort, appFile, agent, proc, stdioHandler + + function stdOutputHandler (data) { + stdioHandler && stdioHandler(data) + } + + before(async () => { + sandbox = await createSandbox(['express']) + appPort = await getPort() + cwd = sandbox.folder + appFile = path.join(cwd, 'appsec/rasp/index.js') + axios = Axios.create({ + baseURL: `http://localhost:${appPort}` + }) + }) + + beforeEach(async () => { + agent = await new FakeAgent().start() + proc = await spawnProc(appFile, { + cwd, + env: { + AGENT_PORT: agent.port, + APP_PORT: appPort, + DD_APPSEC_ENABLED: true, + DD_APPSEC_RASP_ENABLED: true, + DD_APPSEC_RULES: path.join(cwd, 'appsec/rasp/rasp_rules.json') + } + }, stdOutputHandler, stdOutputHandler) + }) + + afterEach(async () => { + proc.kill() + await agent.stop() + }) + + after(async () => { + await sandbox.remove() + }) + + async function testNotCrashedAfterBlocking (path) { + let hasOutput = false + stdioHandler = () => { + hasOutput = true + } + try { + await axios.get(`${path}?host=ifconfig.pro`) + assert.fail('Request should have failed') + } catch (e) { + if (!e.response) { + throw e + } + assert.strictEqual(e.response.status, 403) + } + return new Promise((resolve, reject) => { + setTimeout(() => { + if (hasOutput) { + reject(new Error('Unexpected output in stdout/stderr after blocking request')) + } else { + resolve() + } + }, 50) + }) + } + + describe('ssrf', () => { + it('should block when error is unhandled', async () => { + try { + await axios.get('/ssrf/http/unhandled-error?host=ifconfig.pro') + assert.fail('Request should have failed') + } catch (e) { + assert.strictEqual(e.response.status, 403) + } + }) + + // Not implemented yet + it('should not crash the app when app send data after blocking', () => { + return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-A') + }) + + it('should not crash the app when app stream data after blocking', () => { + return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-B') + }) + + it('should not crash the app when setHeader, writeHead or end after blocking', () => { + return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-C') + }) + + it('should not crash the app when appendHeader, flushHeaders, removeHeader after blocking', () => { + return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-D') + }) + + it('should not crash the app when writeContinue after blocking', () => { + return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-E') + }) + + it('should not crash the app when writeProcessing after blocking', () => { + return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-F') + }) + + it('should not crash the app when writeEarlyHints after blocking', () => { + return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-G') + }) + }) +}) diff --git a/integration-tests/appsec/rasp/index.js b/integration-tests/appsec/rasp/index.js new file mode 100644 index 00000000000..e2912a08688 --- /dev/null +++ b/integration-tests/appsec/rasp/index.js @@ -0,0 +1,97 @@ +'use strict' + +const path = require('path') +const fs = require('fs') +require('dd-trace').init() + +const http = require('https') +const express = require('express') + +const app = express() +const port = process.env.APP_PORT || 3000 + +app.get('/ping', (req, res) => { + res.end('pong') +}) + +function makeOutgoingRequestAndCbAfterTimeout (req, res, cb) { + let finished = false + setTimeout(() => { + if (!finished && cb) { + cb() + } + }, 10) + + http.get(`https://${req.query.host}`, () => { + finished = true + res.send('end') + }) +} + +app.get('/ssrf/http/unhandled-error', (req, res) => { + makeOutgoingRequestAndCbAfterTimeout(req, res) +}) +app.get('/ssrf/http/unhandled-async-write-A', (req, res) => { + makeOutgoingRequestAndCbAfterTimeout(req, res, () => { + res.send('Late end') + }) +}) + +app.get('/ssrf/http/unhandled-async-write-B', (req, res) => { + makeOutgoingRequestAndCbAfterTimeout(req, res, () => { + streamFile(res) + }) +}) + +app.get('/ssrf/http/unhandled-async-write-C', (req, res) => { + makeOutgoingRequestAndCbAfterTimeout(req, res, () => { + res.setHeader('key', 'value') + res.writeHead(200, 'OK', ['key2', 'value2']) + res.write('test\n') + res.end('end') + }) +}) + +app.get('/ssrf/http/unhandled-async-write-D', (req, res) => { + makeOutgoingRequestAndCbAfterTimeout(req, res, () => { + res.setHeader('key', 'value') + res.appendHeader('key2', 'value2') + res.removeHeader('key') + res.flushHeaders() + res.end('end') + }) +}) + +app.get('/ssrf/http/unhandled-async-write-E', (req, res) => { + makeOutgoingRequestAndCbAfterTimeout(req, res, () => { + res.writeContinue() + res.end() + }) +}) + +app.get('/ssrf/http/unhandled-async-write-F', (req, res) => { + makeOutgoingRequestAndCbAfterTimeout(req, res, () => { + res.writeProcessing() + res.end() + }) +}) + +app.get('/ssrf/http/unhandled-async-write-G', (req, res) => { + makeOutgoingRequestAndCbAfterTimeout(req, res, () => { + const earlyHintsLink = '; rel=preload; as=style' + res.writeEarlyHints({ + link: earlyHintsLink + }) + res.end() + }) +}) + +function streamFile (res) { + const stream = fs.createReadStream(path.join(__dirname, 'streamtest.txt'), { encoding: 'utf8' }) + stream.pipe(res, { end: false }) + stream.on('end', () => res.end('end')) +} + +app.listen(port, () => { + process.send({ port }) +}) diff --git a/integration-tests/appsec/rasp/rasp_rules.json b/integration-tests/appsec/rasp/rasp_rules.json new file mode 100644 index 00000000000..6e6913b0311 --- /dev/null +++ b/integration-tests/appsec/rasp/rasp_rules.json @@ -0,0 +1,59 @@ +{ + "version": "2.2", + "metadata": { + "rules_version": "1.99.0" + }, + "rules": [ + { + "id": "test-rule-id-2", + "name": "Server-side request forgery exploit", + "enabled": true, + "tags": { + "type": "ssrf", + "category": "vulnerability_trigger", + "cwe": "918", + "capec": "1000/225/115/664", + "confidence": "0", + "module": "rasp" + }, + "conditions": [ + { + "parameters": { + "resource": [ + { + "address": "server.io.net.url" + } + ], + "params": [ + { + "address": "server.request.query" + }, + { + "address": "server.request.body" + }, + { + "address": "server.request.path_params" + }, + { + "address": "grpc.server.request.message" + }, + { + "address": "graphql.server.all_resolvers" + }, + { + "address": "graphql.server.resolver" + } + ] + }, + "operator": "ssrf_detector" + } + ], + "transformers": [], + "on_match": [ + "block", + "stack_trace" + ] + } + ] +} + diff --git a/integration-tests/appsec/rasp/streamtest.txt b/integration-tests/appsec/rasp/streamtest.txt new file mode 100644 index 00000000000..9daeafb9864 --- /dev/null +++ b/integration-tests/appsec/rasp/streamtest.txt @@ -0,0 +1 @@ +test diff --git a/integration-tests/helpers.js b/integration-tests/helpers.js index b8972540e1f..9fdb01cbf9d 100644 --- a/integration-tests/helpers.js +++ b/integration-tests/helpers.js @@ -162,7 +162,7 @@ class FakeAgent extends EventEmitter { } } -function spawnProc (filename, options = {}, stdioHandler) { +function spawnProc (filename, options = {}, stdioHandler, stderrHandler) { const proc = fork(filename, { ...options, stdio: 'pipe' }) return new Promise((resolve, reject) => { proc @@ -187,6 +187,9 @@ function spawnProc (filename, options = {}, stdioHandler) { }) proc.stderr.on('data', data => { + if (stderrHandler) { + stderrHandler(data) + } // eslint-disable-next-line no-console if (!options.silent) console.error(data.toString()) }) diff --git a/packages/dd-trace/src/appsec/blocking.js b/packages/dd-trace/src/appsec/blocking.js index f3d2103b59e..6c14a44c63c 100644 --- a/packages/dd-trace/src/appsec/blocking.js +++ b/packages/dd-trace/src/appsec/blocking.js @@ -118,6 +118,8 @@ function block (req, res, rootSpan, abortController, actionParameters) { res.writeHead(statusCode, headers).end(body) abortController?.abort() + + // TODO add res in blocked weak set when response blocking is merged } function getBlockingAction (actions) { diff --git a/packages/dd-trace/src/appsec/rasp.js b/packages/dd-trace/src/appsec/rasp.js index 268a0a5bd82..940f0aec1ad 100644 --- a/packages/dd-trace/src/appsec/rasp.js +++ b/packages/dd-trace/src/appsec/rasp.js @@ -3,7 +3,28 @@ const { storage } = require('../../../datadog-core') const addresses = require('./addresses') const { httpClientRequestStart } = require('./channels') +const web = require('../plugins/util/web') const waf = require('./waf') +const { getBlockingAction, block } = require('./blocking') + +class AbortError extends Error { + constructor (req, res, blockingAction) { + super('AbortError') + this.name = 'AbortError' + this.req = req + this.res = res + this.blockingAction = blockingAction + } +} + +function handleUncaughtException (err) { + if (err instanceof AbortError) { + const { req, res, blockingAction } = err + block(req, res, web.root(req), null, blockingAction) + } else { + throw err + } +} const RULE_TYPES = { SSRF: 'ssrf' @@ -11,10 +32,14 @@ const RULE_TYPES = { function enable () { httpClientRequestStart.subscribe(analyzeSsrf) + + process.on('uncaughtException', handleUncaughtException) } function disable () { if (httpClientRequestStart.hasSubscribers) httpClientRequestStart.unsubscribe(analyzeSsrf) + + process.off('uncaughtException', handleUncaughtException) } function analyzeSsrf (ctx) { @@ -27,10 +52,20 @@ function analyzeSsrf (ctx) { const persistent = { [addresses.HTTP_OUTGOING_URL]: url } - // TODO: Currently this is only monitoring, we should - // block the request if SSRF attempt and - // generate stack traces - waf.run({ persistent }, req, RULE_TYPES.SSRF) + // TODO: Currently this is monitoring/blocking, we should + // generate stack traces if SSRF attempts + const actions = waf.run({ persistent }, req, RULE_TYPES.SSRF) + + const res = store?.res + handleWafResults(actions, ctx.abortData, req, res) +} + +function handleWafResults (actions, abortData, req, res) { + const blockingAction = getBlockingAction(actions) + if (blockingAction && abortData) { + abortData.abortController.abort() + abortData.error = new AbortError(req, res, blockingAction) + } } module.exports = { diff --git a/packages/dd-trace/test/appsec/rasp.express.plugin.spec.js b/packages/dd-trace/test/appsec/rasp.express.plugin.spec.js index 23960db1aa3..b588f2b8916 100644 --- a/packages/dd-trace/test/appsec/rasp.express.plugin.spec.js +++ b/packages/dd-trace/test/appsec/rasp.express.plugin.spec.js @@ -61,6 +61,26 @@ withVersions('express', 'express', expressVersion => { } describe('ssrf', () => { + async function testBlockingRequest () { + try { + await axios.get('/?host=ifconfig.pro') + assert.fail('Request should be blocked') + } catch (e) { + if (!e.response) { + throw e + } + } + + await agent.use((traces) => { + const span = getWebSpan(traces) + assert.property(span.meta, '_dd.appsec.json') + assert(span.meta['_dd.appsec.json'].includes('rasp-ssrf-rule-id-1')) + assert.equal(span.metrics['_dd.appsec.rasp.rule.eval'], 1) + assert(span.metrics['_dd.appsec.rasp.duration'] > 0) + assert(span.metrics['_dd.appsec.rasp.duration_ext'] > 0) + }) + } + ['http', 'https'].forEach(protocol => { describe(`Test using ${protocol}`, () => { it('Should not detect threat', async () => { @@ -79,20 +99,16 @@ withVersions('express', 'express', expressVersion => { it('Should detect threat doing a GET request', async () => { app = (req, res) => { - require(protocol).get(`${protocol}://${req.query.host}`) - res.end('end') + const clientRequest = require(protocol).get(`${protocol}://${req.query.host}`) + clientRequest.on('error', (e) => { + if (e.message === 'AbortError') { + res.writeHead(500) + } + res.end('end') + }) } - axios.get('/?host=ifconfig.pro') - - await agent.use((traces) => { - const span = getWebSpan(traces) - assert.property(span.meta, '_dd.appsec.json') - assert(span.meta['_dd.appsec.json'].includes('rasp-ssrf-rule-id-1')) - assert.equal(span.metrics['_dd.appsec.rasp.rule.eval'], 1) - assert(span.metrics['_dd.appsec.rasp.duration'] > 0) - assert(span.metrics['_dd.appsec.rasp.duration_ext'] > 0) - }) + await testBlockingRequest() }) it('Should detect threat doing a POST request', async () => { @@ -101,19 +117,15 @@ withVersions('express', 'express', expressVersion => { .request(`${protocol}://${req.query.host}`, { method: 'POST' }) clientRequest.write('dummy_post_data') clientRequest.end() - res.end('end') + clientRequest.on('error', (e) => { + if (e.message === 'AbortError') { + res.writeHead(500) + } + res.end('end') + }) } - axios.get('/?host=ifconfig.pro') - - await agent.use((traces) => { - const span = getWebSpan(traces) - assert.property(span.meta, '_dd.appsec.json') - assert(span.meta['_dd.appsec.json'].includes('rasp-ssrf-rule-id-1')) - assert.equal(span.metrics['_dd.appsec.rasp.rule.eval'], 1) - assert(span.metrics['_dd.appsec.rasp.duration'] > 0) - assert(span.metrics['_dd.appsec.rasp.duration_ext'] > 0) - }) + await testBlockingRequest() }) }) }) From c11cb227ac733a3bfb231aec20c326b8255139bf Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Thu, 13 Jun 2024 10:24:43 +0200 Subject: [PATCH 03/41] Use responses Set to prevent crashing when tracer blocks --- .../src/http/server.js | 80 ++++++++++++++++--- packages/dd-trace/src/appsec/blocking.js | 7 +- packages/dd-trace/src/appsec/channels.js | 3 +- packages/dd-trace/src/appsec/index.js | 14 +++- packages/dd-trace/test/appsec/index.spec.js | 28 ++++++- 5 files changed, 112 insertions(+), 20 deletions(-) diff --git a/packages/datadog-instrumentations/src/http/server.js b/packages/datadog-instrumentations/src/http/server.js index 18a00ac2789..e256e2fc75f 100644 --- a/packages/datadog-instrumentations/src/http/server.js +++ b/packages/datadog-instrumentations/src/http/server.js @@ -12,6 +12,7 @@ const errorServerCh = channel('apm:http:server:request:error') const finishServerCh = channel('apm:http:server:request:finish') const startWriteHeadCh = channel('apm:http:server:response:writeHead:start') const finishSetHeaderCh = channel('datadog:http:server:response:set-header:finish') +const startSetHeaderCh = channel('datadog:http:server:response:set-header:start') const requestFinishedSet = new WeakSet() @@ -24,6 +25,12 @@ addHook({ name: httpNames }, http => { shimmer.wrap(http.ServerResponse.prototype, 'writeHead', wrapWriteHead) shimmer.wrap(http.ServerResponse.prototype, 'write', wrapWrite) shimmer.wrap(http.ServerResponse.prototype, 'end', wrapEnd) + shimmer.wrap(http.ServerResponse.prototype, 'setHeader', wrapSetHeader) + shimmer.wrap(http.ServerResponse.prototype, 'removeHeader', wrapRemoveHeader) + // Added in node v16.17.0 + if (http.ServerResponse.prototype.appendHeader) { + shimmer.wrap(http.ServerResponse.prototype, 'appendHeader', wrapAppendHeader) + } return http }) @@ -65,9 +72,7 @@ function wrapEmit (emit) { // TODO: should this always return true ? return this.listenerCount(eventName) > 0 } - if (finishSetHeaderCh.hasSubscribers) { - wrapSetHeader(res) - } + return emit.apply(this, arguments) } catch (err) { errorServerCh.publish(err) @@ -81,16 +86,6 @@ function wrapEmit (emit) { } } -function wrapSetHeader (res) { - shimmer.wrap(res, 'setHeader', setHeader => { - return function (name, value) { - const setHeaderResult = setHeader.apply(this, arguments) - finishSetHeaderCh.publish({ name, value, res }) - return setHeaderResult - } - }) -} - function wrapWriteHead (writeHead) { return function wrappedWriteHead (statusCode, reason, obj) { if (!startWriteHeadCh.hasSubscribers) { @@ -159,6 +154,65 @@ function wrapWrite (write) { } } +function wrapSetHeader (setHeader) { + return function wrappedSetHeader (name, value) { + if (!startSetHeaderCh.hasSubscribers && !finishSetHeaderCh.hasSubscribers) { + return setHeader.apply(this, arguments) + } + + if (startSetHeaderCh.hasSubscribers) { + const abortController = new AbortController() + startSetHeaderCh.publish({ res: this, abortController }) + + if (abortController.signal.aborted) { + return + } + } + + const setHeaderResult = setHeader.apply(this, arguments) + + if (finishSetHeaderCh.hasSubscribers) { + finishSetHeaderCh.publish({ name, value, res }) + } + + return setHeaderResult + } +} + +function wrapAppendHeader (appendHeader) { + return function wrappedAppendHeader () { + if (!startSetHeaderCh.hasSubscribers) { + return appendHeader.apply(this, arguments) + } + + const abortController = new AbortController() + startSetHeaderCh.publish({ res: this, abortController }) + + if (abortController.signal.aborted) { + return this + } + + return appendHeader.apply(this, arguments) + } +} + +function wrapRemoveHeader (removeHeader) { + return function wrappedRemoveHeader () { + if (!startSetHeaderCh.hasSubscribers) { + return removeHeader.apply(this, arguments) + } + + const abortController = new AbortController() + startSetHeaderCh.publish({ res: this, abortController }) + + if (abortController.signal.aborted) { + return this + } + + return removeHeader.apply(this, arguments) + } +} + function wrapEnd (end) { return function wrappedEnd () { if (!startWriteHeadCh.hasSubscribers) { diff --git a/packages/dd-trace/src/appsec/blocking.js b/packages/dd-trace/src/appsec/blocking.js index 6c14a44c63c..d917c7bc07b 100644 --- a/packages/dd-trace/src/appsec/blocking.js +++ b/packages/dd-trace/src/appsec/blocking.js @@ -9,6 +9,8 @@ let templateHtml = blockedTemplates.html let templateJson = blockedTemplates.json let templateGraphqlJson = blockedTemplates.graphqlJson +const responseBlockedSet = new WeakSet() + const specificBlockingTypes = { GRAPHQL: 'graphql' } @@ -117,6 +119,8 @@ function block (req, res, rootSpan, abortController, actionParameters) { res.writeHead(statusCode, headers).end(body) + responseBlockedSet.add(res) + abortController?.abort() // TODO add res in blocked weak set when response blocking is merged @@ -152,5 +156,6 @@ module.exports = { specificBlockingTypes, getBlockingData, getBlockingAction, - setTemplates + setTemplates, + responseBlockedSet } diff --git a/packages/dd-trace/src/appsec/channels.js b/packages/dd-trace/src/appsec/channels.js index 82e7eb5f6f7..b669668af2b 100644 --- a/packages/dd-trace/src/appsec/channels.js +++ b/packages/dd-trace/src/appsec/channels.js @@ -19,5 +19,6 @@ module.exports = { nextQueryParsed: dc.channel('apm:next:query-parsed'), responseBody: dc.channel('datadog:express:response:json:start'), responseWriteHead: dc.channel('apm:http:server:response:writeHead:start'), - httpClientRequestStart: dc.channel('apm:http:client:request:start') + httpClientRequestStart: dc.channel('apm:http:client:request:start'), + responseSetHeader: dc.channel('datadog:http:server:response:set-header:start') } diff --git a/packages/dd-trace/src/appsec/index.js b/packages/dd-trace/src/appsec/index.js index af0ffc934f3..05250523b0a 100644 --- a/packages/dd-trace/src/appsec/index.js +++ b/packages/dd-trace/src/appsec/index.js @@ -13,7 +13,8 @@ const { nextBodyParsed, nextQueryParsed, responseBody, - responseWriteHead + responseWriteHead, + responseSetHeader } = require('./channels') const waf = require('./waf') const addresses = require('./addresses') @@ -23,7 +24,7 @@ const apiSecuritySampler = require('./api_security_sampler') const web = require('../plugins/util/web') const { extractIp } = require('../plugins/util/ip_extractor') const { HTTP_CLIENT_IP } = require('../../../../ext/tags') -const { block, setTemplates, getBlockingAction } = require('./blocking') +const { responseBlockedSet, block, setTemplates, getBlockingAction } = require('./blocking') const { passportTrackEvent } = require('./passport') const { storage } = require('../../../datadog-core') const graphql = require('./graphql') @@ -62,6 +63,7 @@ function enable (_config) { cookieParser.subscribe(onRequestCookieParser) responseBody.subscribe(onResponseBody) responseWriteHead.subscribe(onResponseWriteHead) + responseSetHeader.subscribe(onResponseSetHeader) if (_config.appsec.eventTracking.enabled) { passportVerify.subscribe(onPassportVerify) @@ -223,7 +225,6 @@ function onPassportVerify ({ credentials, user }) { } const responseAnalyzedSet = new WeakSet() -const responseBlockedSet = new WeakSet() function onResponseWriteHead ({ req, res, abortController, statusCode, responseHeaders }) { // avoid "write after end" error @@ -255,6 +256,12 @@ function onResponseWriteHead ({ req, res, abortController, statusCode, responseH handleResults(results, req, res, rootSpan, abortController) } +function onResponseSetHeader ({ res, abortController }) { + if (responseBlockedSet.has(res)) { + abortController?.abort() + } +} + function handleResults (actions, req, res, rootSpan, abortController) { if (!actions || !req || !res || !rootSpan || !abortController) return @@ -290,6 +297,7 @@ function disable () { if (responseBody.hasSubscribers) responseBody.unsubscribe(onResponseBody) if (passportVerify.hasSubscribers) passportVerify.unsubscribe(onPassportVerify) if (responseWriteHead.hasSubscribers) responseWriteHead.unsubscribe(onResponseWriteHead) + if (responseSetHeader.hasSubscribers) responseSetHeader.unsubscribe(onResponseSetHeader) } module.exports = { diff --git a/packages/dd-trace/test/appsec/index.spec.js b/packages/dd-trace/test/appsec/index.spec.js index 652aae028ec..005bc7fb3e2 100644 --- a/packages/dd-trace/test/appsec/index.spec.js +++ b/packages/dd-trace/test/appsec/index.spec.js @@ -13,7 +13,8 @@ const { queryParser, passportVerify, responseBody, - responseWriteHead + responseWriteHead, + responseSetHeader } = require('../../src/appsec/channels') const Reporter = require('../../src/appsec/reporter') const agent = require('../plugins/agent') @@ -37,6 +38,7 @@ describe('AppSec Index', () => { let config let AppSec let web + let responseBlockedSet let blocking let passport let log @@ -76,8 +78,10 @@ describe('AppSec Index', () => { root: sinon.stub() } + responseBlockedSet = new WeakSet() blocking = { - setTemplates: sinon.stub() + setTemplates: sinon.stub(), + responseBlockedSet } passport = { @@ -168,6 +172,7 @@ describe('AppSec Index', () => { expect(queryParser.hasSubscribers).to.be.false expect(passportVerify.hasSubscribers).to.be.false expect(responseWriteHead.hasSubscribers).to.be.false + expect(responseSetHeader.hasSubscribers).to.be.false AppSec.enable(config) @@ -176,6 +181,7 @@ describe('AppSec Index', () => { expect(queryParser.hasSubscribers).to.be.true expect(passportVerify.hasSubscribers).to.be.true expect(responseWriteHead.hasSubscribers).to.be.true + expect(responseSetHeader.hasSubscribers).to.be.true }) it('should not subscribe to passportVerify if eventTracking is disabled', () => { @@ -253,6 +259,7 @@ describe('AppSec Index', () => { expect(queryParser.hasSubscribers).to.be.false expect(passportVerify.hasSubscribers).to.be.false expect(responseWriteHead.hasSubscribers).to.be.false + expect(responseSetHeader.hasSubscribers).to.be.false }) it('should call appsec telemetry disable', () => { @@ -916,6 +923,23 @@ describe('AppSec Index', () => { expect(res.end).to.have.been.calledOnce }) }) + + describe('onResponseSetHeader', () => { + it('should call abortController if response was already blocked', () => { + responseBlockedSet.add(res) + + responseSetHeader.publish({ res, abortController }) + + expect(abortController.abort).to.have.been.called + responseBlockedSet.delete(res) + }) + + it('should not call abortController if response was not blocked', () => { + responseSetHeader.publish({ res, abortController }) + + expect(abortController.abort).to.have.not.been.calledOnce + }) + }) }) describe('Metrics', () => { From 619273e4296ce2b24fdf33d27b92117783f068db Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Thu, 13 Jun 2024 10:28:32 +0200 Subject: [PATCH 04/41] Rename internal method name --- packages/dd-trace/src/appsec/rasp.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/dd-trace/src/appsec/rasp.js b/packages/dd-trace/src/appsec/rasp.js index 940f0aec1ad..545ae36c256 100644 --- a/packages/dd-trace/src/appsec/rasp.js +++ b/packages/dd-trace/src/appsec/rasp.js @@ -57,10 +57,10 @@ function analyzeSsrf (ctx) { const actions = waf.run({ persistent }, req, RULE_TYPES.SSRF) const res = store?.res - handleWafResults(actions, ctx.abortData, req, res) + handleResult(actions, req, res, ctx.abortData) } -function handleWafResults (actions, abortData, req, res) { +function handleResult (actions, req, res, abortData) { const blockingAction = getBlockingAction(actions) if (blockingAction && abortData) { abortData.abortController.abort() From 36e0a28db30f0f21e4493b6646e1a4d534b54639 Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Thu, 13 Jun 2024 11:01:55 +0200 Subject: [PATCH 05/41] Use abortController.signal.reason instead of abortData object --- integration-tests/appsec/index.spec.js | 8 ++++++-- integration-tests/appsec/rasp/index.js | 3 ++- packages/datadog-instrumentations/src/http/client.js | 12 +++++------- packages/datadog-instrumentations/src/http/server.js | 2 +- packages/dd-trace/src/appsec/rasp.js | 9 ++++----- 5 files changed, 18 insertions(+), 16 deletions(-) diff --git a/integration-tests/appsec/index.spec.js b/integration-tests/appsec/index.spec.js index bd38eb0468d..7b4e39ba880 100644 --- a/integration-tests/appsec/index.spec.js +++ b/integration-tests/appsec/index.spec.js @@ -1,10 +1,10 @@ 'use strict' const getPort = require('get-port') -const { createSandbox, FakeAgent, spawnProc } = require('../helpers') const path = require('path') const Axios = require('axios') const { assert } = require('chai') +const { createSandbox, FakeAgent, spawnProc } = require('../helpers') describe('RASP', () => { let axios, sandbox, cwd, appPort, appFile, agent, proc, stdioHandler @@ -51,15 +51,19 @@ describe('RASP', () => { stdioHandler = () => { hasOutput = true } + try { await axios.get(`${path}?host=ifconfig.pro`) + assert.fail('Request should have failed') } catch (e) { if (!e.response) { throw e } + assert.strictEqual(e.response.status, 403) } + return new Promise((resolve, reject) => { setTimeout(() => { if (hasOutput) { @@ -75,13 +79,13 @@ describe('RASP', () => { it('should block when error is unhandled', async () => { try { await axios.get('/ssrf/http/unhandled-error?host=ifconfig.pro') + assert.fail('Request should have failed') } catch (e) { assert.strictEqual(e.response.status, 403) } }) - // Not implemented yet it('should not crash the app when app send data after blocking', () => { return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-A') }) diff --git a/integration-tests/appsec/rasp/index.js b/integration-tests/appsec/rasp/index.js index e2912a08688..ce6fc70c7ea 100644 --- a/integration-tests/appsec/rasp/index.js +++ b/integration-tests/appsec/rasp/index.js @@ -1,8 +1,9 @@ 'use strict' +require('dd-trace').init() + const path = require('path') const fs = require('fs') -require('dd-trace').init() const http = require('https') const express = require('express') diff --git a/packages/datadog-instrumentations/src/http/client.js b/packages/datadog-instrumentations/src/http/client.js index cdd48d6a77d..856c1c0f8e2 100644 --- a/packages/datadog-instrumentations/src/http/client.js +++ b/packages/datadog-instrumentations/src/http/client.js @@ -44,7 +44,7 @@ function createAbortedClientRequest (http, args) { return new ClientRequest({ _defaultAgent: http.globalAgent, // needed to support http and https ...args.options, - agent: { + agent: { // noop agent, to prevent doing a real request addRequest: noop } }) @@ -68,11 +68,9 @@ function patch (http, methodName) { return request.apply(this, arguments) } - const abortData = { - abortController: new AbortController() - } + const abortController = new AbortController() - const ctx = { args, http, abortData } + const ctx = { args, http, abortController } return startChannel.runStores(ctx, () => { let finished = false @@ -97,11 +95,11 @@ function patch (http, methodName) { try { let req - if (abortData.abortController?.signal.aborted) { + if (abortController.signal.aborted) { req = createAbortedClientRequest(http, args) process.nextTick(() => { - req.emit('error', abortData.error || new Error('Aborted')) + req.emit('error', abortController.signal.reason || new Error('Aborted')) }) } else { req = request.call(this, options, callback) diff --git a/packages/datadog-instrumentations/src/http/server.js b/packages/datadog-instrumentations/src/http/server.js index e256e2fc75f..3ffc8640023 100644 --- a/packages/datadog-instrumentations/src/http/server.js +++ b/packages/datadog-instrumentations/src/http/server.js @@ -172,7 +172,7 @@ function wrapSetHeader (setHeader) { const setHeaderResult = setHeader.apply(this, arguments) if (finishSetHeaderCh.hasSubscribers) { - finishSetHeaderCh.publish({ name, value, res }) + finishSetHeaderCh.publish({ name, value, res: this }) } return setHeaderResult diff --git a/packages/dd-trace/src/appsec/rasp.js b/packages/dd-trace/src/appsec/rasp.js index 545ae36c256..8fc73d57091 100644 --- a/packages/dd-trace/src/appsec/rasp.js +++ b/packages/dd-trace/src/appsec/rasp.js @@ -57,14 +57,13 @@ function analyzeSsrf (ctx) { const actions = waf.run({ persistent }, req, RULE_TYPES.SSRF) const res = store?.res - handleResult(actions, req, res, ctx.abortData) + handleResult(actions, req, res, ctx.abortController) } -function handleResult (actions, req, res, abortData) { +function handleResult (actions, req, res, abortController) { const blockingAction = getBlockingAction(actions) - if (blockingAction && abortData) { - abortData.abortController.abort() - abortData.error = new AbortError(req, res, blockingAction) + if (blockingAction && abortController) { + abortController.abort(new AbortError(req, res, blockingAction)) } } From 054ca44613bb5e56a0f62e15a3a3f28e00f2f163 Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Thu, 13 Jun 2024 11:37:12 +0200 Subject: [PATCH 06/41] Fix tests --- packages/datadog-instrumentations/test/http.spec.js | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/packages/datadog-instrumentations/test/http.spec.js b/packages/datadog-instrumentations/test/http.spec.js index b3a2150672b..336d8249373 100644 --- a/packages/datadog-instrumentations/test/http.spec.js +++ b/packages/datadog-instrumentations/test/http.spec.js @@ -62,7 +62,7 @@ describe('client', () => { describe('abort controller', () => { function abortCallback (ctx) { if (ctx.args.originalUrl === url) { - ctx.abortData.abortController.abort() + ctx.abortController.abort() } } @@ -71,7 +71,7 @@ describe('client', () => { url = `${httpSchema}://www.datadoghq.com` }) - it('abortData is sent on startChannel', (done) => { + it('abortController is sent on startChannel', (done) => { http.get(url, (res) => { res.on('data', () => { }) @@ -83,7 +83,7 @@ describe('client', () => { sinon.assert.called(startChannelCb) const ctx = getContextFromStubByUrl(url, startChannelCb) assert.isNotNull(ctx) - assert.instanceOf(ctx.abortData.abortController, AbortController) + assert.instanceOf(ctx.abortController, AbortController) }) it('Request is aborted with default error', (done) => { @@ -95,7 +95,7 @@ describe('client', () => { cr.on('error', (e) => { try { assert.instanceOf(e, Error) - assert.strictEqual(e.message, 'Aborted') + assert.strictEqual(e.message, 'This operation was aborted') done() } catch (e) { done(e) @@ -109,8 +109,7 @@ describe('client', () => { startChannelCb.callsFake((ctx) => { if (ctx.args.originalUrl === url) { - ctx.abortData.abortController.abort() - ctx.abortData.error = new CustomError('Custom error') + ctx.abortController.abort(new CustomError('Custom error')) } }) From 5e5658fe48773e4ac56d447c13569aa9ef7dc351 Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Thu, 13 Jun 2024 13:12:15 +0200 Subject: [PATCH 07/41] Hacks to support node 16.0 --- integration-tests/appsec/rasp/index.js | 4 ++-- packages/datadog-instrumentations/test/http.spec.js | 1 - packages/dd-trace/src/appsec/rasp.js | 8 +++++++- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/integration-tests/appsec/rasp/index.js b/integration-tests/appsec/rasp/index.js index ce6fc70c7ea..fc85576fa39 100644 --- a/integration-tests/appsec/rasp/index.js +++ b/integration-tests/appsec/rasp/index.js @@ -56,7 +56,7 @@ app.get('/ssrf/http/unhandled-async-write-C', (req, res) => { app.get('/ssrf/http/unhandled-async-write-D', (req, res) => { makeOutgoingRequestAndCbAfterTimeout(req, res, () => { res.setHeader('key', 'value') - res.appendHeader('key2', 'value2') + res.appendHeader?.('key2', 'value2') res.removeHeader('key') res.flushHeaders() res.end('end') @@ -80,7 +80,7 @@ app.get('/ssrf/http/unhandled-async-write-F', (req, res) => { app.get('/ssrf/http/unhandled-async-write-G', (req, res) => { makeOutgoingRequestAndCbAfterTimeout(req, res, () => { const earlyHintsLink = '; rel=preload; as=style' - res.writeEarlyHints({ + res.writeEarlyHints?.({ link: earlyHintsLink }) res.end() diff --git a/packages/datadog-instrumentations/test/http.spec.js b/packages/datadog-instrumentations/test/http.spec.js index 336d8249373..b517d86c412 100644 --- a/packages/datadog-instrumentations/test/http.spec.js +++ b/packages/datadog-instrumentations/test/http.spec.js @@ -95,7 +95,6 @@ describe('client', () => { cr.on('error', (e) => { try { assert.instanceOf(e, Error) - assert.strictEqual(e.message, 'This operation was aborted') done() } catch (e) { done(e) diff --git a/packages/dd-trace/src/appsec/rasp.js b/packages/dd-trace/src/appsec/rasp.js index 8fc73d57091..6bc1c523b3e 100644 --- a/packages/dd-trace/src/appsec/rasp.js +++ b/packages/dd-trace/src/appsec/rasp.js @@ -63,7 +63,13 @@ function analyzeSsrf (ctx) { function handleResult (actions, req, res, abortController) { const blockingAction = getBlockingAction(actions) if (blockingAction && abortController) { - abortController.abort(new AbortError(req, res, blockingAction)) + const abortError = new AbortError(req, res, blockingAction) + abortController.abort(abortError) + + // TODO Delete this if when support for node 16 is removed + if (!abortController.signal.reason) { + abortController.signal.reason = abortError + } } } From a4142bf5a9f6f5833b3ee159d29b633fbd1d5542 Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Thu, 13 Jun 2024 15:57:21 +0200 Subject: [PATCH 08/41] lint --- packages/datadog-instrumentations/test/http.spec.js | 8 ++++++-- packages/dd-trace/src/appsec/blocking.js | 2 -- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/datadog-instrumentations/test/http.spec.js b/packages/datadog-instrumentations/test/http.spec.js index b517d86c412..a7ee625c067 100644 --- a/packages/datadog-instrumentations/test/http.spec.js +++ b/packages/datadog-instrumentations/test/http.spec.js @@ -73,8 +73,7 @@ describe('client', () => { it('abortController is sent on startChannel', (done) => { http.get(url, (res) => { - res.on('data', () => { - }) + res.on('data', () => {}) res.on('end', () => { done() }) @@ -92,6 +91,7 @@ describe('client', () => { const cr = http.get(url, () => { done('Request should be blocked') }) + cr.on('error', (e) => { try { assert.instanceOf(e, Error) @@ -115,6 +115,7 @@ describe('client', () => { const cr = http.get(url, () => { done('Request should be blocked') }) + cr.on('error', (e) => { try { assert.instanceOf(e, CustomError) @@ -132,6 +133,7 @@ describe('client', () => { const cr = http.get(url, () => { done('Request should be blocked') }) + cr.on('error', () => { try { sinon.assert.calledOnce(errorChannelCb) @@ -149,6 +151,7 @@ describe('client', () => { const cr = http.get(url, () => { done('Request should be blocked') }) + cr.on('error', () => { try { sinon.assert.called(endChannelCb) @@ -167,6 +170,7 @@ describe('client', () => { const cr = http.get(url, () => { done('Request should be blocked') }) + cr.on('error', () => { try { // Necessary because the tracer makes extra requests to the agent diff --git a/packages/dd-trace/src/appsec/blocking.js b/packages/dd-trace/src/appsec/blocking.js index d917c7bc07b..3c0ee72d042 100644 --- a/packages/dd-trace/src/appsec/blocking.js +++ b/packages/dd-trace/src/appsec/blocking.js @@ -122,8 +122,6 @@ function block (req, res, rootSpan, abortController, actionParameters) { responseBlockedSet.add(res) abortController?.abort() - - // TODO add res in blocked weak set when response blocking is merged } function getBlockingAction (actions) { From 96f208c5a4f2efffa6c258a1f7a6af36e39f1565 Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Fri, 14 Jun 2024 12:42:23 +0200 Subject: [PATCH 09/41] Add test case --- integration-tests/appsec/index.spec.js | 8 ++++++++ integration-tests/appsec/rasp/index.js | 6 ++++++ 2 files changed, 14 insertions(+) diff --git a/integration-tests/appsec/index.spec.js b/integration-tests/appsec/index.spec.js index 7b4e39ba880..a478bec1adb 100644 --- a/integration-tests/appsec/index.spec.js +++ b/integration-tests/appsec/index.spec.js @@ -82,6 +82,10 @@ describe('RASP', () => { assert.fail('Request should have failed') } catch (e) { + if (!e.response) { + throw e + } + assert.strictEqual(e.response.status, 403) } }) @@ -113,5 +117,9 @@ describe('RASP', () => { it('should not crash the app when writeEarlyHints after blocking', () => { return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-G') }) + + it('should not crash the app when res.json after blocking', () => { + return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-H') + }) }) }) diff --git a/integration-tests/appsec/rasp/index.js b/integration-tests/appsec/rasp/index.js index fc85576fa39..dfa83604887 100644 --- a/integration-tests/appsec/rasp/index.js +++ b/integration-tests/appsec/rasp/index.js @@ -87,6 +87,12 @@ app.get('/ssrf/http/unhandled-async-write-G', (req, res) => { }) }) +app.get('/ssrf/http/unhandled-async-write-H', (req, res) => { + makeOutgoingRequestAndCbAfterTimeout(req, res, () => { + res.json({ key: 'value' }) + }) +}) + function streamFile (res) { const stream = fs.createReadStream(path.join(__dirname, 'streamtest.txt'), { encoding: 'utf8' }) stream.pipe(res, { end: false }) From c6837a9ba87ec90c836efa757928870738480e24 Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Fri, 14 Jun 2024 15:39:22 +0200 Subject: [PATCH 10/41] Remove unused code from a test --- .../datadog-instrumentations/test/http.spec.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/datadog-instrumentations/test/http.spec.js b/packages/datadog-instrumentations/test/http.spec.js index a7ee625c067..a015465afc1 100644 --- a/packages/datadog-instrumentations/test/http.spec.js +++ b/packages/datadog-instrumentations/test/http.spec.js @@ -5,10 +5,9 @@ const dc = require('dc-polyfill') const agent = require('../../dd-trace/test/plugins/agent') describe('client', () => { - let url, http, startChannelCb, finishChannelCb, endChannelCb, asyncStartChannelCb, errorChannelCb + let url, http, startChannelCb, endChannelCb, asyncStartChannelCb, errorChannelCb const startChannel = dc.channel('apm:http:client:request:start') - const finishChannel = dc.channel('apm:http:client:request:finish') const endChannel = dc.channel('apm:http:client:request:end') const asyncStartChannel = dc.channel('apm:http:client:request:asyncStart') const errorChannel = dc.channel('apm:http:client:request:error') @@ -24,8 +23,6 @@ describe('client', () => { beforeEach(() => { startChannelCb = sinon.stub() startChannel.subscribe(startChannelCb) - finishChannelCb = sinon.stub() - finishChannel.subscribe(finishChannelCb) endChannelCb = sinon.stub() endChannel.subscribe(endChannelCb) asyncStartChannelCb = sinon.stub() @@ -36,7 +33,6 @@ describe('client', () => { afterEach(() => { startChannel.unsubscribe(startChannelCb) - finishChannel.unsubscribe(finishChannelCb) endChannel.unsubscribe(endChannelCb) asyncStartChannel.unsubscribe(asyncStartChannelCb) errorChannel.unsubscribe(errorChannelCb) @@ -95,6 +91,7 @@ describe('client', () => { cr.on('error', (e) => { try { assert.instanceOf(e, Error) + done() } catch (e) { done(e) @@ -103,8 +100,7 @@ describe('client', () => { }) it('Request is aborted with custom error', (done) => { - class CustomError extends Error { - } + class CustomError extends Error { } startChannelCb.callsFake((ctx) => { if (ctx.args.originalUrl === url) { @@ -120,6 +116,7 @@ describe('client', () => { try { assert.instanceOf(e, CustomError) assert.strictEqual(e.message, 'Custom error') + done() } catch (e) { done(e) @@ -138,6 +135,7 @@ describe('client', () => { try { sinon.assert.calledOnce(errorChannelCb) assert.instanceOf(errorChannelCb.firstCall.args[0].error, Error) + done() } catch (e) { done(e) @@ -157,6 +155,7 @@ describe('client', () => { sinon.assert.called(endChannelCb) const ctx = getContextFromStubByUrl(url, endChannelCb) assert.strictEqual(ctx.args.originalUrl, url) + done() } catch (e) { done(e) @@ -178,6 +177,7 @@ describe('client', () => { const ctx = getContextFromStubByUrl(url, asyncStartChannelCb) assert.isNull(ctx) } + done() } catch (e) { done(e.message) From 6e969e6501d18b8ba3678a42ccb1822e59b4fcfd Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Mon, 17 Jun 2024 09:21:14 +0200 Subject: [PATCH 11/41] Add tests using axios --- .../test/appsec/rasp.express.plugin.spec.js | 55 +++++++++++++++++++ packages/dd-trace/test/plugins/externals.json | 4 ++ 2 files changed, 59 insertions(+) diff --git a/packages/dd-trace/test/appsec/rasp.express.plugin.spec.js b/packages/dd-trace/test/appsec/rasp.express.plugin.spec.js index b588f2b8916..b4d16c2b0a3 100644 --- a/packages/dd-trace/test/appsec/rasp.express.plugin.spec.js +++ b/packages/dd-trace/test/appsec/rasp.express.plugin.spec.js @@ -129,6 +129,61 @@ withVersions('express', 'express', expressVersion => { }) }) }) + + describe('Test using axios', () => { + withVersions('express', 'axios', axiosVersion => { + let axiosToTest + + beforeEach(() => { + axiosToTest = require(`../../../../versions/axios@${axiosVersion}`).get() + }) + + it('Should not detect threat', async () => { + app = (req, res) => { + axios.get(`https://${req.query.host}`) + res.end('end') + } + + axios.get('/?host=www.datadoghq.com') + + await agent.use((traces) => { + const span = getWebSpan(traces) + assert.notProperty(span.meta, '_dd.appsec.json') + }) + }) + + it('Should detect threat doing a GET request', async () => { + app = async (req, res) => { + try { + await axiosToTest.get(`https://${req.query.host}`) + res.end('end') + } catch (e) { + if (e.cause.message === 'AbortError') { + res.writeHead(500) + } + res.end('end') + } + } + + await testBlockingRequest() + }) + + it('Should detect threat doing a POST request', async () => { + app = async (req, res) => { + try { + await axios.post(`https://${req.query.host}`, { key: 'value' }) + } catch (e) { + if (e.cause.message === 'AbortError') { + res.writeHead(500) + } + res.end('end') + } + } + + await testBlockingRequest() + }) + }) + }) }) }) }) diff --git a/packages/dd-trace/test/plugins/externals.json b/packages/dd-trace/test/plugins/externals.json index be17310a924..80b3b2147f2 100644 --- a/packages/dd-trace/test/plugins/externals.json +++ b/packages/dd-trace/test/plugins/externals.json @@ -54,6 +54,10 @@ } ], "express": [ + { + "name": "axios", + "versions": [">=1.0.0"] + }, { "name": "loopback", "versions": [">=2.38.1"] From 8c1612e4fa5ae663d76dbd8ea8fefa3eb0d82317 Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Mon, 17 Jun 2024 10:26:54 +0200 Subject: [PATCH 12/41] Do not block when it is not using express --- packages/dd-trace/src/appsec/rasp.js | 14 ++- .../test/appsec/rasp.express.plugin.spec.js | 105 +++++++++++++++--- packages/dd-trace/test/appsec/rasp_rules.json | 3 + 3 files changed, 103 insertions(+), 19 deletions(-) diff --git a/packages/dd-trace/src/appsec/rasp.js b/packages/dd-trace/src/appsec/rasp.js index 6bc1c523b3e..5e6f7243fd4 100644 --- a/packages/dd-trace/src/appsec/rasp.js +++ b/packages/dd-trace/src/appsec/rasp.js @@ -63,12 +63,16 @@ function analyzeSsrf (ctx) { function handleResult (actions, req, res, abortController) { const blockingAction = getBlockingAction(actions) if (blockingAction && abortController) { - const abortError = new AbortError(req, res, blockingAction) - abortController.abort(abortError) + const rootSpan = web.root(req) + // Should block only in express + if (rootSpan.context()._name === 'express.request') { + const abortError = new AbortError(req, res, blockingAction) + abortController.abort(abortError) - // TODO Delete this if when support for node 16 is removed - if (!abortController.signal.reason) { - abortController.signal.reason = abortError + // TODO Delete this if when support for node 16 is removed + if (!abortController.signal.reason) { + abortController.signal.reason = abortError + } } } } diff --git a/packages/dd-trace/test/appsec/rasp.express.plugin.spec.js b/packages/dd-trace/test/appsec/rasp.express.plugin.spec.js index b4d16c2b0a3..b6df83f6ffd 100644 --- a/packages/dd-trace/test/appsec/rasp.express.plugin.spec.js +++ b/packages/dd-trace/test/appsec/rasp.express.plugin.spec.js @@ -8,12 +8,23 @@ const Config = require('../../src/config') const path = require('path') const { assert } = require('chai') -withVersions('express', 'express', expressVersion => { - describe('RASP', () => { +describe('RASP', () => { + function getWebSpan (traces) { + for (const trace of traces) { + for (const span of trace) { + if (span.type === 'web') { + return span + } + } + } + throw new Error('web span not found') + } + + withVersions('express', 'express', expressVersion => { let app, server, port, axios before(() => { - return agent.load(['http'], { client: false }) + return agent.load(['express', 'http'], { client: false }) }) before((done) => { @@ -49,17 +60,6 @@ withVersions('express', 'express', expressVersion => { return agent.close({ ritmReset: false }) }) - function getWebSpan (traces) { - for (const trace of traces) { - for (const span of trace) { - if (span.type === 'web') { - return span - } - } - } - throw new Error('web span not found') - } - describe('ssrf', () => { async function testBlockingRequest () { try { @@ -186,4 +186,81 @@ withVersions('express', 'express', expressVersion => { }) }) }) + + describe('without express', () => { + let app, server, axios + before(() => { + return agent.load(['http'], { client: false }) + }) + + before((done) => { + const http = require('http') + server = http.createServer((req, res) => { + if (app) { + app(req, res) + } else { + res.end('end') + } + }) + + appsec.enable(new Config({ + appsec: { + enabled: true, + rules: path.join(__dirname, 'rasp_rules.json'), + rasp: { enabled: true } + } + })) + + getPort().then(newPort => { + axios = Axios.create({ + baseURL: `http://localhost:${newPort}` + }) + server.listen(newPort, () => { + done() + }) + }) + }) + + after(() => { + appsec.disable() + server.close() + return agent.close({ ritmReset: false }) + }) + + it('Should detect threat without blocking doing a GET request', async () => { + app = (req, res) => { + const clientRequest = require('http').get(`http://${req.headers.host}`, { timeout: 10 }, function () { + res.end('end') + }) + + clientRequest.on('timeout', () => { + res.writeHead(200) + res.end('timeout') + }) + + clientRequest.on('error', (e) => { + res.writeHead(500) + res.end('error') + }) + } + + const response = await axios.get('/', { + headers: { + host: 'ifconfig.pro' + } + }) + + assert.equal(response.status, 200) + assert.equal(response.data, 'timeout') + + await agent.use((traces) => { + const span = getWebSpan(traces) + assert.property(span.meta, '_dd.appsec.json') + assert(span.meta['_dd.appsec.json'].includes('rasp-ssrf-rule-id-1')) + assert.equal(span.metrics['_dd.appsec.rasp.rule.eval'], 1) + assert(span.metrics['_dd.appsec.rasp.duration'] > 0) + assert(span.metrics['_dd.appsec.rasp.duration_ext'] > 0) + }) + }) + }) }) diff --git a/packages/dd-trace/test/appsec/rasp_rules.json b/packages/dd-trace/test/appsec/rasp_rules.json index 7b01675dcaa..28930412b9a 100644 --- a/packages/dd-trace/test/appsec/rasp_rules.json +++ b/packages/dd-trace/test/appsec/rasp_rules.json @@ -34,6 +34,9 @@ { "address": "server.request.path_params" }, + { + "address": "server.request.headers.no_cookies" + }, { "address": "grpc.server.request.message" }, From 05c537173fd9ad3ee59e9e36fc68d27ed8352f45 Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Mon, 17 Jun 2024 12:20:44 +0200 Subject: [PATCH 13/41] Fix stack traces --- packages/dd-trace/src/appsec/stack_trace.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/dd-trace/src/appsec/stack_trace.js b/packages/dd-trace/src/appsec/stack_trace.js index 846366003ce..855bba63d54 100644 --- a/packages/dd-trace/src/appsec/stack_trace.js +++ b/packages/dd-trace/src/appsec/stack_trace.js @@ -27,7 +27,7 @@ function getCallSiteList (maxDepth = 100) { } function filterOutFramesFromLibrary (callSiteList) { - return callSiteList.filter(callSite => !callSite.getFileName().includes(ddBasePath)) + return callSiteList.filter(callSite => !callSite.getFileName()?.includes(ddBasePath)) } function getFramesForMetaStruct (callSiteList, maxDepth = 32) { From 5b94ef69151ef7d5c0a1c3df35a0063cbf0e1d75 Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Mon, 17 Jun 2024 12:42:25 +0200 Subject: [PATCH 14/41] Improve axios support --- integration-tests/appsec/index.spec.js | 6 +++++- integration-tests/appsec/rasp/index.js | 8 +++++++- packages/dd-trace/src/appsec/rasp.js | 3 ++- 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/integration-tests/appsec/index.spec.js b/integration-tests/appsec/index.spec.js index a478bec1adb..5eb527706db 100644 --- a/integration-tests/appsec/index.spec.js +++ b/integration-tests/appsec/index.spec.js @@ -14,7 +14,7 @@ describe('RASP', () => { } before(async () => { - sandbox = await createSandbox(['express']) + sandbox = await createSandbox(['express', 'axios']) appPort = await getPort() cwd = sandbox.folder appFile = path.join(cwd, 'appsec/rasp/index.js') @@ -121,5 +121,9 @@ describe('RASP', () => { it('should not crash the app when res.json after blocking', () => { return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-H') }) + + it('should not crash the when is blocked using axios', () => { + return testNotCrashedAfterBlocking('/ssrf/http/unhandled-axios') + }) }) }) diff --git a/integration-tests/appsec/rasp/index.js b/integration-tests/appsec/rasp/index.js index dfa83604887..ca868ce88b6 100644 --- a/integration-tests/appsec/rasp/index.js +++ b/integration-tests/appsec/rasp/index.js @@ -4,9 +4,9 @@ require('dd-trace').init() const path = require('path') const fs = require('fs') - const http = require('https') const express = require('express') +const axios = require('axios') const app = express() const port = process.env.APP_PORT || 3000 @@ -32,6 +32,7 @@ function makeOutgoingRequestAndCbAfterTimeout (req, res, cb) { app.get('/ssrf/http/unhandled-error', (req, res) => { makeOutgoingRequestAndCbAfterTimeout(req, res) }) + app.get('/ssrf/http/unhandled-async-write-A', (req, res) => { makeOutgoingRequestAndCbAfterTimeout(req, res, () => { res.send('Late end') @@ -93,6 +94,11 @@ app.get('/ssrf/http/unhandled-async-write-H', (req, res) => { }) }) +app.get('/ssrf/http/unhandled-axios', (req, res) => { + axios.get(`https://${req.query.host}`) + .then(() => res.end('end')) +}) + function streamFile (res) { const stream = fs.createReadStream(path.join(__dirname, 'streamtest.txt'), { encoding: 'utf8' }) stream.pipe(res, { end: false }) diff --git a/packages/dd-trace/src/appsec/rasp.js b/packages/dd-trace/src/appsec/rasp.js index 9d4d335355a..9267d4dbf3c 100644 --- a/packages/dd-trace/src/appsec/rasp.js +++ b/packages/dd-trace/src/appsec/rasp.js @@ -19,7 +19,8 @@ class AbortError extends Error { } function handleUncaughtException (err) { - if (err instanceof AbortError) { + // err.cause to wrap the error thrown by client request + if (err instanceof AbortError || err.cause instanceof AbortError) { const { req, res, blockingAction } = err block(req, res, web.root(req), null, blockingAction) } else { From 70fd11cf5ae4f72656b84ac9e082f2e0920b0b1a Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Mon, 17 Jun 2024 16:36:19 +0200 Subject: [PATCH 15/41] Small changes --- integration-tests/appsec/index.spec.js | 18 +++++++------- .../test/http.spec.js | 24 +++++++------------ 2 files changed, 18 insertions(+), 24 deletions(-) diff --git a/integration-tests/appsec/index.spec.js b/integration-tests/appsec/index.spec.js index 5eb527706db..3a179ff783b 100644 --- a/integration-tests/appsec/index.spec.js +++ b/integration-tests/appsec/index.spec.js @@ -90,39 +90,39 @@ describe('RASP', () => { } }) - it('should not crash the app when app send data after blocking', () => { + it('should not crash when app send data after blocking', () => { return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-A') }) - it('should not crash the app when app stream data after blocking', () => { + it('should not crash when app stream data after blocking', () => { return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-B') }) - it('should not crash the app when setHeader, writeHead or end after blocking', () => { + it('should not crash when setHeader, writeHead or end after blocking', () => { return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-C') }) - it('should not crash the app when appendHeader, flushHeaders, removeHeader after blocking', () => { + it('should not crash when appendHeader, flushHeaders, removeHeader after blocking', () => { return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-D') }) - it('should not crash the app when writeContinue after blocking', () => { + it('should not crash when writeContinue after blocking', () => { return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-E') }) - it('should not crash the app when writeProcessing after blocking', () => { + it('should not crash when writeProcessing after blocking', () => { return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-F') }) - it('should not crash the app when writeEarlyHints after blocking', () => { + it('should not crash when writeEarlyHints after blocking', () => { return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-G') }) - it('should not crash the app when res.json after blocking', () => { + it('should not crash when res.json after blocking', () => { return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-H') }) - it('should not crash the when is blocked using axios', () => { + it('should not crash when is blocked using axios', () => { return testNotCrashedAfterBlocking('/ssrf/http/unhandled-axios') }) }) diff --git a/packages/datadog-instrumentations/test/http.spec.js b/packages/datadog-instrumentations/test/http.spec.js index a015465afc1..021b85871f9 100644 --- a/packages/datadog-instrumentations/test/http.spec.js +++ b/packages/datadog-instrumentations/test/http.spec.js @@ -81,21 +81,15 @@ describe('client', () => { assert.instanceOf(ctx.abortController, AbortController) }) - it('Request is aborted with default error', (done) => { + it('Request is aborted', (done) => { startChannelCb.callsFake(abortCallback) const cr = http.get(url, () => { - done('Request should be blocked') + done(new Error('Request should be blocked')) }) - cr.on('error', (e) => { - try { - assert.instanceOf(e, Error) - - done() - } catch (e) { - done(e) - } + cr.on('error', () => { + done() }) }) @@ -109,7 +103,7 @@ describe('client', () => { }) const cr = http.get(url, () => { - done('Request should be blocked') + done(new Error('Request should be blocked')) }) cr.on('error', (e) => { @@ -128,7 +122,7 @@ describe('client', () => { startChannelCb.callsFake(abortCallback) const cr = http.get(url, () => { - done('Request should be blocked') + done(new Error('Request should be blocked')) }) cr.on('error', () => { @@ -147,7 +141,7 @@ describe('client', () => { startChannelCb.callsFake(abortCallback) const cr = http.get(url, () => { - done('Request should be blocked') + done(new Error('Request should be blocked')) }) cr.on('error', () => { @@ -167,7 +161,7 @@ describe('client', () => { startChannelCb.callsFake(abortCallback) const cr = http.get(url, () => { - done('Request should be blocked') + done(new Error('Request should be blocked')) }) cr.on('error', () => { @@ -183,7 +177,7 @@ describe('client', () => { done(e.message) } }) - }).timeout(1000) + }) }) }) }) From 2681a85992b3b7e8ce3961ee68e89fbcd8396b4f Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Thu, 20 Jun 2024 14:42:32 +0200 Subject: [PATCH 16/41] Change a bit unhandled exception approach --- integration-tests/appsec/index.spec.js | 275 ++++++++++++++++--------- integration-tests/appsec/rasp/index.js | 55 ++++- packages/dd-trace/src/appsec/rasp.js | 22 +- 3 files changed, 246 insertions(+), 106 deletions(-) diff --git a/integration-tests/appsec/index.spec.js b/integration-tests/appsec/index.spec.js index 3a179ff783b..c4eb1886e4f 100644 --- a/integration-tests/appsec/index.spec.js +++ b/integration-tests/appsec/index.spec.js @@ -13,117 +13,208 @@ describe('RASP', () => { stdioHandler && stdioHandler(data) } - before(async () => { - sandbox = await createSandbox(['express', 'axios']) - appPort = await getPort() - cwd = sandbox.folder - appFile = path.join(cwd, 'appsec/rasp/index.js') - axios = Axios.create({ - baseURL: `http://localhost:${appPort}` - }) - }) - - beforeEach(async () => { - agent = await new FakeAgent().start() - proc = await spawnProc(appFile, { - cwd, - env: { - AGENT_PORT: agent.port, - APP_PORT: appPort, - DD_APPSEC_ENABLED: true, - DD_APPSEC_RASP_ENABLED: true, - DD_APPSEC_RULES: path.join(cwd, 'appsec/rasp/rasp_rules.json') - } - }, stdOutputHandler, stdOutputHandler) - }) - - afterEach(async () => { - proc.kill() - await agent.stop() - }) + const confs = [ + // { abortOnUncaughtException: true }, + { abortOnUncaughtException: false }] + // execArgv --abort-on-uncaught-exception + + confs.forEach((conf) => { + describe(`abortOnUncaughtException: ${conf.abortOnUncaughtException}`, () => { + before(async () => { + sandbox = await createSandbox(['express', 'axios']) + appPort = await getPort() + cwd = sandbox.folder + appFile = path.join(cwd, 'appsec/rasp/index.js') + axios = Axios.create({ + baseURL: `http://localhost:${appPort}` + }) + }) + + beforeEach(async () => { + let execArgv = process.execArgv + if (conf.abortOnUncaughtException) { + execArgv = ['--abort-on-uncaught-exception', ...execArgv] + } + agent = await new FakeAgent().start() + proc = await spawnProc(appFile, { + cwd, + execArgv, + env: { + AGENT_PORT: agent.port, + APP_PORT: appPort, + DD_APPSEC_ENABLED: true, + DD_APPSEC_RASP_ENABLED: true, + DD_APPSEC_RULES: path.join(cwd, 'appsec/rasp/rasp_rules.json') + } + }, stdOutputHandler, stdOutputHandler) + }) + + afterEach(async () => { + proc.kill() + await agent.stop() + }) + + after(async () => { + await sandbox.remove() + }) + + async function testNotCrashedAfterBlocking (path) { + let hasOutput = false + stdioHandler = () => { + hasOutput = true + } - after(async () => { - await sandbox.remove() - }) + try { + await axios.get(`${path}?host=ifconfig.pro`) - async function testNotCrashedAfterBlocking (path) { - let hasOutput = false - stdioHandler = () => { - hasOutput = true - } + assert.fail('Request should have failed') + } catch (e) { + if (!e.response) { + throw e + } - try { - await axios.get(`${path}?host=ifconfig.pro`) + assert.strictEqual(e.response.status, 403) + } - assert.fail('Request should have failed') - } catch (e) { - if (!e.response) { - throw e + return new Promise((resolve, reject) => { + setTimeout(() => { + if (hasOutput) { + reject(new Error('Unexpected output in stdout/stderr after blocking request')) + } else { + resolve() + } + }, 50) + }) } - assert.strictEqual(e.response.status, 403) - } - - return new Promise((resolve, reject) => { - setTimeout(() => { - if (hasOutput) { - reject(new Error('Unexpected output in stdout/stderr after blocking request')) - } else { - resolve() + describe('ssrf', () => { + it('should crash when error is not an AbortError', async () => { + let hasOutput = false + stdioHandler = () => { + hasOutput = true + } + + try { + await axios.get('/crash') + + assert.fail('Request should have failed') + } catch (e) { + return new Promise((resolve, reject) => { + setTimeout(() => { + if (hasOutput) { + resolve() + } else { + reject(new Error('Output expected after crash')) + } + }, 50) + }) + } + }) + + it('should not crash when customer has his own setUncaughtExceptionCaptureCallback', async () => { + let hasOutput = false + stdioHandler = () => { + hasOutput = true + } + + try { + await axios.get('/crash-and-recovery-A') + + assert.fail('Request should have failed') + } catch (e) { + return new Promise((resolve, reject) => { + setTimeout(() => { + if (hasOutput) { + reject(new Error('Unexpected output in stdout/stderr after blocking request')) + } else { + resolve() + } + }, 50) + }) + } + }) + + // this test is not valid when abortOnUncaughtException is true + if (!conf.abortOnUncaughtException) { + it('should not crash when customer has his own uncaughtException', async () => { + let hasOutput = false + stdioHandler = () => { + hasOutput = true + } + + try { + await axios.get('/crash-and-recovery-B') + + assert.fail('Request should have failed') + } catch (e) { + // console.log(e) + return new Promise((resolve, reject) => { + setTimeout(() => { + if (hasOutput) { + reject(new Error('Unexpected output in stdout/stderr after blocking request')) + } else { + resolve() + } + }, 50) + }) + } + }) } - }, 50) - }) - } - describe('ssrf', () => { - it('should block when error is unhandled', async () => { - try { - await axios.get('/ssrf/http/unhandled-error?host=ifconfig.pro') + it('should block when error is unhandled', async () => { + try { + await axios.get('/ssrf/http/unhandled-error?host=ifconfig.pro') - assert.fail('Request should have failed') - } catch (e) { - if (!e.response) { - throw e - } + assert.fail('Request should have failed') + } catch (e) { + if (!e.response) { + throw e + } - assert.strictEqual(e.response.status, 403) - } - }) + assert.strictEqual(e.response.status, 403) + } + }) - it('should not crash when app send data after blocking', () => { - return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-A') - }) + it('should not crash when app send data after blocking', () => { + return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-A') + }) - it('should not crash when app stream data after blocking', () => { - return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-B') - }) + it('should not crash when app stream data after blocking', () => { + return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-B') + }) - it('should not crash when setHeader, writeHead or end after blocking', () => { - return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-C') - }) + it('should not crash when setHeader, writeHead or end after blocking', () => { + return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-C') + }) - it('should not crash when appendHeader, flushHeaders, removeHeader after blocking', () => { - return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-D') - }) + it('should not crash when appendHeader, flushHeaders, removeHeader after blocking', () => { + return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-D') + }) - it('should not crash when writeContinue after blocking', () => { - return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-E') - }) + it('should not crash when writeContinue after blocking', () => { + return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-E') + }) - it('should not crash when writeProcessing after blocking', () => { - return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-F') - }) + it('should not crash when writeProcessing after blocking', () => { + return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-F') + }) - it('should not crash when writeEarlyHints after blocking', () => { - return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-G') - }) + it('should not crash when writeEarlyHints after blocking', () => { + return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-G') + }) - it('should not crash when res.json after blocking', () => { - return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-H') - }) + it('should not crash when res.json after blocking', () => { + return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-H') + }) + + it('should not crash when is blocked using axios', () => { + return testNotCrashedAfterBlocking('/ssrf/http/unhandled-axios') + }) - it('should not crash when is blocked using axios', () => { - return testNotCrashedAfterBlocking('/ssrf/http/unhandled-axios') + it('should not crash when is blocked with unhandled rejection', () => { + return testNotCrashedAfterBlocking('/ssrf/http/unhandled-promise') + }) + }) }) }) }) diff --git a/integration-tests/appsec/rasp/index.js b/integration-tests/appsec/rasp/index.js index ca868ce88b6..abd159670ca 100644 --- a/integration-tests/appsec/rasp/index.js +++ b/integration-tests/appsec/rasp/index.js @@ -29,6 +29,52 @@ function makeOutgoingRequestAndCbAfterTimeout (req, res, cb) { }) } +function streamFile (res) { + const stream = fs.createReadStream(path.join(__dirname, 'streamtest.txt'), { encoding: 'utf8' }) + stream.pipe(res, { end: false }) + stream.on('end', () => res.end('end')) +} + +function httpGetPromise (host) { + return new Promise((resolve, reject) => { + const clientRequest = http.get(`https://${host}`, () => { + resolve() + }) + clientRequest.on('error', reject) + }) +} + +app.get('/crash', () => { + process.nextTick(() => { + throw new Error('Crash') + }) +}) + +app.get('/crash-and-recovery-A', (req, res) => { + process.setUncaughtExceptionCaptureCallback(() => { + res.writeHead(500) + res.end('error') + process.setUncaughtExceptionCaptureCallback(null) + }) + process.nextTick(() => { + throw new Error('Crash') + }) +}) + +app.get('/crash-and-recovery-B', (req, res) => { + function exceptionHandler () { + // console.log('ey - error 500') + res.writeHead(500) + res.end('error') + process.off('uncaughtException', exceptionHandler) + } + process.on('uncaughtException', exceptionHandler) + + process.nextTick(() => { + throw new Error('Crash') + }) +}) + app.get('/ssrf/http/unhandled-error', (req, res) => { makeOutgoingRequestAndCbAfterTimeout(req, res) }) @@ -99,11 +145,10 @@ app.get('/ssrf/http/unhandled-axios', (req, res) => { .then(() => res.end('end')) }) -function streamFile (res) { - const stream = fs.createReadStream(path.join(__dirname, 'streamtest.txt'), { encoding: 'utf8' }) - stream.pipe(res, { end: false }) - stream.on('end', () => res.end('end')) -} +app.get('/ssrf/http/unhandled-promise', (req, res) => { + httpGetPromise(req.query.host) + .then(() => res.end('end')) +}) app.listen(port, () => { process.send({ port }) diff --git a/packages/dd-trace/src/appsec/rasp.js b/packages/dd-trace/src/appsec/rasp.js index 9267d4dbf3c..31d1af1f08e 100644 --- a/packages/dd-trace/src/appsec/rasp.js +++ b/packages/dd-trace/src/appsec/rasp.js @@ -18,13 +18,16 @@ class AbortError extends Error { } } -function handleUncaughtException (err) { - // err.cause to wrap the error thrown by client request +function handleUncaughtExceptionMonitor (err, origin) { if (err instanceof AbortError || err.cause instanceof AbortError) { const { req, res, blockingAction } = err block(req, res, web.root(req), null, blockingAction) - } else { - throw err + + if (!process.hasUncaughtExceptionCaptureCallback()) { + process.setUncaughtExceptionCaptureCallback(() => { + process.setUncaughtExceptionCaptureCallback(null) + }) + } } } @@ -32,19 +35,20 @@ const RULE_TYPES = { SSRF: 'ssrf' } -let config +let config, abortOnUncaughtException function enable (_config) { config = _config httpClientRequestStart.subscribe(analyzeSsrf) - process.on('uncaughtException', handleUncaughtException) + process.on('uncaughtExceptionMonitor', handleUncaughtExceptionMonitor) + abortOnUncaughtException = process.execArgv?.includes('--abort-on-uncaught-exception') } function disable () { if (httpClientRequestStart.hasSubscribers) httpClientRequestStart.unsubscribe(analyzeSsrf) - process.off('uncaughtException', handleUncaughtException) + process.off('uncaughtExceptionMonitor', handleUncaughtExceptionMonitor) } function analyzeSsrf (ctx) { @@ -84,11 +88,11 @@ function handleResult (actions, req, res, abortController) { if (blockingAction && abortController) { const rootSpan = web.root(req) // Should block only in express - if (rootSpan.context()._name === 'express.request') { + if (rootSpan.context()._name === 'express.request' && !abortOnUncaughtException) { const abortError = new AbortError(req, res, blockingAction) abortController.abort(abortError) - // TODO Delete this if when support for node 16 is removed + // TODO Delete this when support for node 16 is removed if (!abortController.signal.reason) { abortController.signal.reason = abortError } From 5323576495c710f8122a3897197306395d488a0e Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Fri, 21 Jun 2024 14:15:15 +0200 Subject: [PATCH 17/41] support blocking unhandled errors with --abort-on-uncaught-exception --- integration-tests/appsec/index.spec.js | 26 ++++++++- integration-tests/appsec/rasp/index.js | 6 ++ .../src/helpers/register.js | 3 + .../datadog-instrumentations/src/process.js | 29 ++++++++++ packages/dd-trace/src/appsec/rasp.js | 58 ++++++++++++++++++- 5 files changed, 118 insertions(+), 4 deletions(-) create mode 100644 packages/datadog-instrumentations/src/process.js diff --git a/integration-tests/appsec/index.spec.js b/integration-tests/appsec/index.spec.js index c4eb1886e4f..f643ae86f80 100644 --- a/integration-tests/appsec/index.spec.js +++ b/integration-tests/appsec/index.spec.js @@ -14,7 +14,7 @@ describe('RASP', () => { } const confs = [ - // { abortOnUncaughtException: true }, + { abortOnUncaughtException: true }, { abortOnUncaughtException: false }] // execArgv --abort-on-uncaught-exception @@ -111,6 +111,29 @@ describe('RASP', () => { } }) + it('should crash when promise rejection is not an AbortError', async () => { + let hasOutput = false + stdioHandler = () => { + hasOutput = true + } + + try { + await axios.get('/crash-promise') + + assert.fail('Request should have failed') + } catch (e) { + return new Promise((resolve, reject) => { + setTimeout(() => { + if (hasOutput) { + resolve() + } else { + reject(new Error('Output expected after crash')) + } + }, 50) + }) + } + }) + it('should not crash when customer has his own setUncaughtExceptionCaptureCallback', async () => { let hasOutput = false stdioHandler = () => { @@ -147,7 +170,6 @@ describe('RASP', () => { assert.fail('Request should have failed') } catch (e) { - // console.log(e) return new Promise((resolve, reject) => { setTimeout(() => { if (hasOutput) { diff --git a/integration-tests/appsec/rasp/index.js b/integration-tests/appsec/rasp/index.js index abd159670ca..715e36b4fed 100644 --- a/integration-tests/appsec/rasp/index.js +++ b/integration-tests/appsec/rasp/index.js @@ -50,6 +50,12 @@ app.get('/crash', () => { }) }) +app.get('/crash-promise', () => { + process.nextTick(() => { + Promise.reject(new Error('Crash')) + }) +}) + app.get('/crash-and-recovery-A', (req, res) => { process.setUncaughtExceptionCaptureCallback(() => { res.writeHead(500) diff --git a/packages/datadog-instrumentations/src/helpers/register.js b/packages/datadog-instrumentations/src/helpers/register.js index eba90d6a980..a48641d52b5 100644 --- a/packages/datadog-instrumentations/src/helpers/register.js +++ b/packages/datadog-instrumentations/src/helpers/register.js @@ -27,6 +27,9 @@ const loadChannel = channel('dd-trace:instrumentation:load') if (!disabledInstrumentations.has('fetch')) { require('../fetch') } +if (!disabledInstrumentations.has('process')) { + require('../process') +} const HOOK_SYMBOL = Symbol('hookExportsMap') diff --git a/packages/datadog-instrumentations/src/process.js b/packages/datadog-instrumentations/src/process.js new file mode 100644 index 00000000000..2e0c104620a --- /dev/null +++ b/packages/datadog-instrumentations/src/process.js @@ -0,0 +1,29 @@ +'use strict' + +const shimmer = require('../../datadog-shimmer') +const { channel } = require('dc-polyfill') + +const startSetUncaughtExceptionCaptureCallback = channel('datadog:process:setUncaughtExceptionCaptureCallback:start') + +if (process.setUncaughtExceptionCaptureCallback) { + let currentCallback + + shimmer.wrap(process, 'setUncaughtExceptionCaptureCallback', + function wrapSetUncaughtExceptionCaptureCallback (originalSetUncaughtExceptionCaptureCallback) { + return function setUncaughtExceptionCaptureCallback (newCallback) { + if (startSetUncaughtExceptionCaptureCallback.hasSubscribers) { + const abortController = new AbortController() + startSetUncaughtExceptionCaptureCallback.publish({ newCallback, currentCallback, abortController }) + if (abortController.signal.aborted) { + return + } + } + + const result = originalSetUncaughtExceptionCaptureCallback.call(this, newCallback) + + currentCallback = newCallback + + return result + } + }) +} diff --git a/packages/dd-trace/src/appsec/rasp.js b/packages/dd-trace/src/appsec/rasp.js index 31d1af1f08e..983738e4e2d 100644 --- a/packages/dd-trace/src/appsec/rasp.js +++ b/packages/dd-trace/src/appsec/rasp.js @@ -7,6 +7,9 @@ const { httpClientRequestStart } = require('./channels') const { reportStackTrace } = require('./stack_trace') const waf = require('./waf') const { getBlockingAction, block } = require('./blocking') +const { channel } = require('dc-polyfill') + +const startSetUncaughtExceptionCaptureCallback = channel('datadog:process:setUncaughtExceptionCaptureCallback:start') class AbortError extends Error { constructor (req, res, blockingAction) { @@ -18,8 +21,12 @@ class AbortError extends Error { } } +function isAbortError (err) { + return err instanceof AbortError || err.cause instanceof AbortError +} + function handleUncaughtExceptionMonitor (err, origin) { - if (err instanceof AbortError || err.cause instanceof AbortError) { + if (isAbortError(err)) { const { req, res, blockingAction } = err block(req, res, web.root(req), null, blockingAction) @@ -37,12 +44,59 @@ const RULE_TYPES = { let config, abortOnUncaughtException +let previousCallback, userDefinedCallback + function enable (_config) { config = _config httpClientRequestStart.subscribe(analyzeSsrf) process.on('uncaughtExceptionMonitor', handleUncaughtExceptionMonitor) abortOnUncaughtException = process.execArgv?.includes('--abort-on-uncaught-exception') + + if (abortOnUncaughtException) { + process.on('unhandledRejection', function (err) { + if (isAbortError(err)) { + const { req, res, blockingAction } = err + block(req, res, web.root(req), null, blockingAction) + } else { + const listeners = process.listeners('unhandledRejection') + // do not force crash if there are more listeners + if (listeners.length === 1) { + // TODO check the --unhandled-rejections flag + throw err + } + } + }) + + let appsecCallbackSetted = false + startSetUncaughtExceptionCaptureCallback.subscribe(({ currentCallback, newCallback, abortController }) => { + previousCallback = currentCallback + if (appsecCallbackSetted) { + userDefinedCallback = newCallback + abortController.abort() + } + }) + + if (process.hasUncaughtExceptionCaptureCallback()) { + process.setUncaughtExceptionCaptureCallback(null) + userDefinedCallback = previousCallback + } + + const exceptionCaptureCallback = function (err) { + if (!isAbortError(err)) { + if (userDefinedCallback) { + userDefinedCallback(err) + } else { + process.setUncaughtExceptionCaptureCallback(null) + + throw err // app will crash by customer app reasons + } + } + } + + process.setUncaughtExceptionCaptureCallback(exceptionCaptureCallback) + appsecCallbackSetted = true + } } function disable () { @@ -88,7 +142,7 @@ function handleResult (actions, req, res, abortController) { if (blockingAction && abortController) { const rootSpan = web.root(req) // Should block only in express - if (rootSpan.context()._name === 'express.request' && !abortOnUncaughtException) { + if (rootSpan.context()._name === 'express.request') { const abortError = new AbortError(req, res, blockingAction) abortController.abort(abortError) From 52a7f2c72389587a674dfa85c576b6fb583e45a9 Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Tue, 25 Jun 2024 09:07:48 +0200 Subject: [PATCH 18/41] Revert "support blocking unhandled errors with --abort-on-uncaught-exception" This reverts commit 5323576495c710f8122a3897197306395d488a0e. --- integration-tests/appsec/index.spec.js | 26 +-------- integration-tests/appsec/rasp/index.js | 6 -- .../src/helpers/register.js | 3 - .../datadog-instrumentations/src/process.js | 29 ---------- packages/dd-trace/src/appsec/rasp.js | 58 +------------------ 5 files changed, 4 insertions(+), 118 deletions(-) delete mode 100644 packages/datadog-instrumentations/src/process.js diff --git a/integration-tests/appsec/index.spec.js b/integration-tests/appsec/index.spec.js index f643ae86f80..c4eb1886e4f 100644 --- a/integration-tests/appsec/index.spec.js +++ b/integration-tests/appsec/index.spec.js @@ -14,7 +14,7 @@ describe('RASP', () => { } const confs = [ - { abortOnUncaughtException: true }, + // { abortOnUncaughtException: true }, { abortOnUncaughtException: false }] // execArgv --abort-on-uncaught-exception @@ -111,29 +111,6 @@ describe('RASP', () => { } }) - it('should crash when promise rejection is not an AbortError', async () => { - let hasOutput = false - stdioHandler = () => { - hasOutput = true - } - - try { - await axios.get('/crash-promise') - - assert.fail('Request should have failed') - } catch (e) { - return new Promise((resolve, reject) => { - setTimeout(() => { - if (hasOutput) { - resolve() - } else { - reject(new Error('Output expected after crash')) - } - }, 50) - }) - } - }) - it('should not crash when customer has his own setUncaughtExceptionCaptureCallback', async () => { let hasOutput = false stdioHandler = () => { @@ -170,6 +147,7 @@ describe('RASP', () => { assert.fail('Request should have failed') } catch (e) { + // console.log(e) return new Promise((resolve, reject) => { setTimeout(() => { if (hasOutput) { diff --git a/integration-tests/appsec/rasp/index.js b/integration-tests/appsec/rasp/index.js index 715e36b4fed..abd159670ca 100644 --- a/integration-tests/appsec/rasp/index.js +++ b/integration-tests/appsec/rasp/index.js @@ -50,12 +50,6 @@ app.get('/crash', () => { }) }) -app.get('/crash-promise', () => { - process.nextTick(() => { - Promise.reject(new Error('Crash')) - }) -}) - app.get('/crash-and-recovery-A', (req, res) => { process.setUncaughtExceptionCaptureCallback(() => { res.writeHead(500) diff --git a/packages/datadog-instrumentations/src/helpers/register.js b/packages/datadog-instrumentations/src/helpers/register.js index a48641d52b5..eba90d6a980 100644 --- a/packages/datadog-instrumentations/src/helpers/register.js +++ b/packages/datadog-instrumentations/src/helpers/register.js @@ -27,9 +27,6 @@ const loadChannel = channel('dd-trace:instrumentation:load') if (!disabledInstrumentations.has('fetch')) { require('../fetch') } -if (!disabledInstrumentations.has('process')) { - require('../process') -} const HOOK_SYMBOL = Symbol('hookExportsMap') diff --git a/packages/datadog-instrumentations/src/process.js b/packages/datadog-instrumentations/src/process.js deleted file mode 100644 index 2e0c104620a..00000000000 --- a/packages/datadog-instrumentations/src/process.js +++ /dev/null @@ -1,29 +0,0 @@ -'use strict' - -const shimmer = require('../../datadog-shimmer') -const { channel } = require('dc-polyfill') - -const startSetUncaughtExceptionCaptureCallback = channel('datadog:process:setUncaughtExceptionCaptureCallback:start') - -if (process.setUncaughtExceptionCaptureCallback) { - let currentCallback - - shimmer.wrap(process, 'setUncaughtExceptionCaptureCallback', - function wrapSetUncaughtExceptionCaptureCallback (originalSetUncaughtExceptionCaptureCallback) { - return function setUncaughtExceptionCaptureCallback (newCallback) { - if (startSetUncaughtExceptionCaptureCallback.hasSubscribers) { - const abortController = new AbortController() - startSetUncaughtExceptionCaptureCallback.publish({ newCallback, currentCallback, abortController }) - if (abortController.signal.aborted) { - return - } - } - - const result = originalSetUncaughtExceptionCaptureCallback.call(this, newCallback) - - currentCallback = newCallback - - return result - } - }) -} diff --git a/packages/dd-trace/src/appsec/rasp.js b/packages/dd-trace/src/appsec/rasp.js index 983738e4e2d..31d1af1f08e 100644 --- a/packages/dd-trace/src/appsec/rasp.js +++ b/packages/dd-trace/src/appsec/rasp.js @@ -7,9 +7,6 @@ const { httpClientRequestStart } = require('./channels') const { reportStackTrace } = require('./stack_trace') const waf = require('./waf') const { getBlockingAction, block } = require('./blocking') -const { channel } = require('dc-polyfill') - -const startSetUncaughtExceptionCaptureCallback = channel('datadog:process:setUncaughtExceptionCaptureCallback:start') class AbortError extends Error { constructor (req, res, blockingAction) { @@ -21,12 +18,8 @@ class AbortError extends Error { } } -function isAbortError (err) { - return err instanceof AbortError || err.cause instanceof AbortError -} - function handleUncaughtExceptionMonitor (err, origin) { - if (isAbortError(err)) { + if (err instanceof AbortError || err.cause instanceof AbortError) { const { req, res, blockingAction } = err block(req, res, web.root(req), null, blockingAction) @@ -44,59 +37,12 @@ const RULE_TYPES = { let config, abortOnUncaughtException -let previousCallback, userDefinedCallback - function enable (_config) { config = _config httpClientRequestStart.subscribe(analyzeSsrf) process.on('uncaughtExceptionMonitor', handleUncaughtExceptionMonitor) abortOnUncaughtException = process.execArgv?.includes('--abort-on-uncaught-exception') - - if (abortOnUncaughtException) { - process.on('unhandledRejection', function (err) { - if (isAbortError(err)) { - const { req, res, blockingAction } = err - block(req, res, web.root(req), null, blockingAction) - } else { - const listeners = process.listeners('unhandledRejection') - // do not force crash if there are more listeners - if (listeners.length === 1) { - // TODO check the --unhandled-rejections flag - throw err - } - } - }) - - let appsecCallbackSetted = false - startSetUncaughtExceptionCaptureCallback.subscribe(({ currentCallback, newCallback, abortController }) => { - previousCallback = currentCallback - if (appsecCallbackSetted) { - userDefinedCallback = newCallback - abortController.abort() - } - }) - - if (process.hasUncaughtExceptionCaptureCallback()) { - process.setUncaughtExceptionCaptureCallback(null) - userDefinedCallback = previousCallback - } - - const exceptionCaptureCallback = function (err) { - if (!isAbortError(err)) { - if (userDefinedCallback) { - userDefinedCallback(err) - } else { - process.setUncaughtExceptionCaptureCallback(null) - - throw err // app will crash by customer app reasons - } - } - } - - process.setUncaughtExceptionCaptureCallback(exceptionCaptureCallback) - appsecCallbackSetted = true - } } function disable () { @@ -142,7 +88,7 @@ function handleResult (actions, req, res, abortController) { if (blockingAction && abortController) { const rootSpan = web.root(req) // Should block only in express - if (rootSpan.context()._name === 'express.request') { + if (rootSpan.context()._name === 'express.request' && !abortOnUncaughtException) { const abortError = new AbortError(req, res, blockingAction) abortController.abort(abortError) From 1c14eabffc6962ee15d9b381e022b0c8684ccf93 Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Tue, 25 Jun 2024 12:58:49 +0200 Subject: [PATCH 19/41] Improve integration tests --- integration-tests/appsec/index.spec.js | 366 ++++++++++-------- integration-tests/appsec/rasp/index.js | 26 +- .../dd-trace/src/exporters/common/writer.js | 1 - 3 files changed, 224 insertions(+), 169 deletions(-) diff --git a/integration-tests/appsec/index.spec.js b/integration-tests/appsec/index.spec.js index c4eb1886e4f..43c700fae3a 100644 --- a/integration-tests/appsec/index.spec.js +++ b/integration-tests/appsec/index.spec.js @@ -13,207 +13,249 @@ describe('RASP', () => { stdioHandler && stdioHandler(data) } - const confs = [ - // { abortOnUncaughtException: true }, - { abortOnUncaughtException: false }] - // execArgv --abort-on-uncaught-exception - - confs.forEach((conf) => { - describe(`abortOnUncaughtException: ${conf.abortOnUncaughtException}`, () => { - before(async () => { - sandbox = await createSandbox(['express', 'axios']) - appPort = await getPort() - cwd = sandbox.folder - appFile = path.join(cwd, 'appsec/rasp/index.js') - axios = Axios.create({ - baseURL: `http://localhost:${appPort}` - }) - }) - - beforeEach(async () => { - let execArgv = process.execArgv - if (conf.abortOnUncaughtException) { - execArgv = ['--abort-on-uncaught-exception', ...execArgv] + before(async () => { + sandbox = await createSandbox(['express', 'axios']) + appPort = await getPort() + cwd = sandbox.folder + appFile = path.join(cwd, 'appsec/rasp/index.js') + axios = Axios.create({ + baseURL: `http://localhost:${appPort}` + }) + }) + + after(async () => { + await sandbox.remove() + }) + + function startServer (abortOnUncaughtException) { + beforeEach(async () => { + let execArgv = process.execArgv + if (abortOnUncaughtException) { + execArgv = ['--abort-on-uncaught-exception', ...execArgv] + } + agent = await new FakeAgent().start() + proc = await spawnProc(appFile, { + cwd, + execArgv, + env: { + DD_TRACE_AGENT_PORT: agent.port, + APP_PORT: appPort, + DD_APPSEC_ENABLED: true, + DD_APPSEC_RASP_ENABLED: true, + DD_APPSEC_RULES: path.join(cwd, 'appsec/rasp/rasp_rules.json') } - agent = await new FakeAgent().start() - proc = await spawnProc(appFile, { - cwd, - execArgv, - env: { - AGENT_PORT: agent.port, - APP_PORT: appPort, - DD_APPSEC_ENABLED: true, - DD_APPSEC_RASP_ENABLED: true, - DD_APPSEC_RULES: path.join(cwd, 'appsec/rasp/rasp_rules.json') + }, stdOutputHandler, stdOutputHandler) + }) + + afterEach(async () => { + proc.kill() + await agent.stop() + }) + } + + async function assertRaspDetected () { + await agent.assertMessageReceived(({ headers, payload }) => { + assert.property(payload[0][0].meta, '_dd.appsec.json') + assert.include(payload[0][0].meta['_dd.appsec.json'], '"test-rule-id-2"') + }) + } + + describe('--abort-on-uncaught-exception is not configured', () => { + startServer(false) + + async function testNotCrashedAfterBlocking (path) { + let hasOutput = false + stdioHandler = () => { + hasOutput = true + } + + try { + await axios.get(`${path}?host=ifconfig.pro`) + + assert.fail('Request should have failed') + } catch (e) { + if (!e.response) { + throw e + } + + assert.strictEqual(e.response.status, 403) + await assertRaspDetected() + } + + return new Promise((resolve, reject) => { + setTimeout(() => { + if (hasOutput) { + reject(new Error('Unexpected output in stdout/stderr after blocking request')) + } else { + resolve() } - }, stdOutputHandler, stdOutputHandler) + }, 50) }) + } - afterEach(async () => { - proc.kill() - await agent.stop() - }) + describe('ssrf', () => { + it('should crash when error is not an AbortError', async () => { + let hasOutput = false + stdioHandler = () => { + hasOutput = true + } + + try { + await axios.get('/crash') - after(async () => { - await sandbox.remove() + assert.fail('Request should have failed') + } catch (e) { + return new Promise((resolve, reject) => { + setTimeout(() => { + if (hasOutput) { + resolve() + } else { + reject(new Error('Output expected after crash')) + } + }, 50) + }) + } }) - async function testNotCrashedAfterBlocking (path) { + it('should not crash when customer has his own setUncaughtExceptionCaptureCallback', async () => { let hasOutput = false stdioHandler = () => { hasOutput = true } try { - await axios.get(`${path}?host=ifconfig.pro`) + await axios.get('/crash-and-recovery-A') assert.fail('Request should have failed') } catch (e) { - if (!e.response) { - throw e - } + return new Promise((resolve, reject) => { + setTimeout(() => { + if (hasOutput) { + reject(new Error('Unexpected output in stdout/stderr after blocking request')) + } else { + resolve() + } + }, 50) + }) + } + }) - assert.strictEqual(e.response.status, 403) + it('should not crash when customer has his own uncaughtException', async () => { + let hasOutput = false + stdioHandler = () => { + hasOutput = true } - return new Promise((resolve, reject) => { - setTimeout(() => { - if (hasOutput) { - reject(new Error('Unexpected output in stdout/stderr after blocking request')) - } else { - resolve() - } - }, 50) - }) - } + try { + await axios.get('/crash-and-recovery-B') - describe('ssrf', () => { - it('should crash when error is not an AbortError', async () => { - let hasOutput = false - stdioHandler = () => { - hasOutput = true - } + assert.fail('Request should have failed') + } catch (e) { + // console.log(e) + return new Promise((resolve, reject) => { + setTimeout(() => { + if (hasOutput) { + reject(new Error('Unexpected output in stdout/stderr after blocking request')) + } else { + resolve() + } + }, 50) + }) + } + }) - try { - await axios.get('/crash') - - assert.fail('Request should have failed') - } catch (e) { - return new Promise((resolve, reject) => { - setTimeout(() => { - if (hasOutput) { - resolve() - } else { - reject(new Error('Output expected after crash')) - } - }, 50) - }) - } - }) + it('should block manually', async () => { + let response + const raspDetectedPromise = assertRaspDetected() + try { + response = await axios.get('/ssrf/http/manual-blocking?host=ifconfig.pro') - it('should not crash when customer has his own setUncaughtExceptionCaptureCallback', async () => { - let hasOutput = false - stdioHandler = () => { - hasOutput = true + assert.fail('Request should have failed') + } catch (e) { + if (!e.response) { + throw e } + response = e.response + } + assert.strictEqual(response.status, 403) + await raspDetectedPromise + }) - try { - await axios.get('/crash-and-recovery-A') - - assert.fail('Request should have failed') - } catch (e) { - return new Promise((resolve, reject) => { - setTimeout(() => { - if (hasOutput) { - reject(new Error('Unexpected output in stdout/stderr after blocking request')) - } else { - resolve() - } - }, 50) - }) + it('should block when error is unhandled', async () => { + try { + await axios.get('/ssrf/http/unhandled-error?host=ifconfig.pro') + + assert.fail('Request should have failed') + } catch (e) { + if (!e.response) { + throw e } - }) - - // this test is not valid when abortOnUncaughtException is true - if (!conf.abortOnUncaughtException) { - it('should not crash when customer has his own uncaughtException', async () => { - let hasOutput = false - stdioHandler = () => { - hasOutput = true - } - - try { - await axios.get('/crash-and-recovery-B') - - assert.fail('Request should have failed') - } catch (e) { - // console.log(e) - return new Promise((resolve, reject) => { - setTimeout(() => { - if (hasOutput) { - reject(new Error('Unexpected output in stdout/stderr after blocking request')) - } else { - resolve() - } - }, 50) - }) - } - }) + + assert.strictEqual(e.response.status, 403) + await assertRaspDetected() } + }) - it('should block when error is unhandled', async () => { - try { - await axios.get('/ssrf/http/unhandled-error?host=ifconfig.pro') + it('should not crash when app send data after blocking', () => { + return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-A') + }) - assert.fail('Request should have failed') - } catch (e) { - if (!e.response) { - throw e - } + it('should not crash when app stream data after blocking', () => { + return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-B') + }) - assert.strictEqual(e.response.status, 403) - } - }) + it('should not crash when setHeader, writeHead or end after blocking', () => { + return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-C') + }) - it('should not crash when app send data after blocking', () => { - return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-A') - }) + it('should not crash when appendHeader, flushHeaders, removeHeader after blocking', () => { + return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-D') + }) - it('should not crash when app stream data after blocking', () => { - return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-B') - }) + it('should not crash when writeContinue after blocking', () => { + return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-E') + }) - it('should not crash when setHeader, writeHead or end after blocking', () => { - return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-C') - }) + it('should not crash when writeProcessing after blocking', () => { + return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-F') + }) + + it('should not crash when writeEarlyHints after blocking', () => { + return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-G') + }) - it('should not crash when appendHeader, flushHeaders, removeHeader after blocking', () => { - return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-D') - }) + it('should not crash when res.json after blocking', () => { + return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-H') + }) - it('should not crash when writeContinue after blocking', () => { - return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-E') - }) + it('should not crash when is blocked using axios', () => { + return testNotCrashedAfterBlocking('/ssrf/http/unhandled-axios') + }) - it('should not crash when writeProcessing after blocking', () => { - return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-F') - }) + it('should not crash when is blocked with unhandled rejection', () => { + return testNotCrashedAfterBlocking('/ssrf/http/unhandled-promise') + }) + }) + }) - it('should not crash when writeEarlyHints after blocking', () => { - return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-G') - }) + describe('--abort-on-uncaught-exception is configured', () => { + startServer(true) - it('should not crash when res.json after blocking', () => { - return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-H') - }) + describe('ssrf', () => { + it('should not block', async () => { + let response - it('should not crash when is blocked using axios', () => { - return testNotCrashedAfterBlocking('/ssrf/http/unhandled-axios') - }) + try { + response = await axios.get('/ssrf/http/manual-blocking?host=ifconfig.pro') + } catch (e) { + if (!e.response) { + throw e + } + response = e.response + } - it('should not crash when is blocked with unhandled rejection', () => { - return testNotCrashedAfterBlocking('/ssrf/http/unhandled-promise') - }) + assert.notEqual(response.status, 403) // 200 or 500 expected + await assertRaspDetected() }) }) }) diff --git a/integration-tests/appsec/rasp/index.js b/integration-tests/appsec/rasp/index.js index abd159670ca..d4f3410f8fd 100644 --- a/integration-tests/appsec/rasp/index.js +++ b/integration-tests/appsec/rasp/index.js @@ -1,6 +1,8 @@ 'use strict' - -require('dd-trace').init() +const tracer = require('dd-trace') +tracer.init({ + flushInterval: 0 +}) const path = require('path') const fs = require('fs') @@ -11,10 +13,6 @@ const axios = require('axios') const app = express() const port = process.env.APP_PORT || 3000 -app.get('/ping', (req, res) => { - res.end('pong') -}) - function makeOutgoingRequestAndCbAfterTimeout (req, res, cb) { let finished = false setTimeout(() => { @@ -75,6 +73,22 @@ app.get('/crash-and-recovery-B', (req, res) => { }) }) +app.get('/ssrf/http/manual-blocking', (req, res) => { + const clientRequest = http.get(`https://${req.query.host}`, () => { + res.send('end') + }) + + clientRequest.on('error', (err) => { + if (err.name === 'AbortError') { + res.writeHead(403) + res.end('aborted') + } else { + res.writeHead(500) + res.end('error') + } + }) +}) + app.get('/ssrf/http/unhandled-error', (req, res) => { makeOutgoingRequestAndCbAfterTimeout(req, res) }) diff --git a/packages/dd-trace/src/exporters/common/writer.js b/packages/dd-trace/src/exporters/common/writer.js index 57f08accd32..8dbe21ca4d1 100644 --- a/packages/dd-trace/src/exporters/common/writer.js +++ b/packages/dd-trace/src/exporters/common/writer.js @@ -17,7 +17,6 @@ class Writer { done() } else if (count > 0) { const payload = this._encoder.makePayload() - this._sendPayload(payload, count, done) } else { done() From 20209f5ff4e8668a98eac3c84d9f3c01dea53e7b Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Tue, 25 Jun 2024 16:48:09 +0200 Subject: [PATCH 20/41] Small fixes --- integration-tests/appsec/index.spec.js | 10 ++++++---- integration-tests/appsec/rasp/index.js | 7 +++++-- packages/datadog-instrumentations/test/http.spec.js | 7 ++++--- packages/dd-trace/src/exporters/common/writer.js | 1 + .../dd-trace/test/appsec/rasp.express.plugin.spec.js | 11 ++++++++--- 5 files changed, 24 insertions(+), 12 deletions(-) diff --git a/integration-tests/appsec/index.spec.js b/integration-tests/appsec/index.spec.js index 43c700fae3a..36206d4fee2 100644 --- a/integration-tests/appsec/index.spec.js +++ b/integration-tests/appsec/index.spec.js @@ -166,7 +166,7 @@ describe('RASP', () => { it('should block manually', async () => { let response - const raspDetectedPromise = assertRaspDetected() + try { response = await axios.get('/ssrf/http/manual-blocking?host=ifconfig.pro') @@ -177,8 +177,8 @@ describe('RASP', () => { } response = e.response } - assert.strictEqual(response.status, 403) - await raspDetectedPromise + assert.strictEqual(response.status, 418) + await assertRaspDetected() }) it('should block when error is unhandled', async () => { @@ -254,7 +254,9 @@ describe('RASP', () => { response = e.response } - assert.notEqual(response.status, 403) // 200 or 500 expected + // not blocked + assert.notEqual(response.status, 418) + assert.notEqual(response.status, 403) await assertRaspDetected() }) }) diff --git a/integration-tests/appsec/rasp/index.js b/integration-tests/appsec/rasp/index.js index d4f3410f8fd..9ee445f7a1c 100644 --- a/integration-tests/appsec/rasp/index.js +++ b/integration-tests/appsec/rasp/index.js @@ -52,8 +52,10 @@ app.get('/crash-and-recovery-A', (req, res) => { process.setUncaughtExceptionCaptureCallback(() => { res.writeHead(500) res.end('error') + process.setUncaughtExceptionCaptureCallback(null) }) + process.nextTick(() => { throw new Error('Crash') }) @@ -61,11 +63,12 @@ app.get('/crash-and-recovery-A', (req, res) => { app.get('/crash-and-recovery-B', (req, res) => { function exceptionHandler () { - // console.log('ey - error 500') res.writeHead(500) res.end('error') + process.off('uncaughtException', exceptionHandler) } + process.on('uncaughtException', exceptionHandler) process.nextTick(() => { @@ -80,7 +83,7 @@ app.get('/ssrf/http/manual-blocking', (req, res) => { clientRequest.on('error', (err) => { if (err.name === 'AbortError') { - res.writeHead(403) + res.writeHead(418) res.end('aborted') } else { res.writeHead(500) diff --git a/packages/datadog-instrumentations/test/http.spec.js b/packages/datadog-instrumentations/test/http.spec.js index 021b85871f9..44a14d32602 100644 --- a/packages/datadog-instrumentations/test/http.spec.js +++ b/packages/datadog-instrumentations/test/http.spec.js @@ -22,12 +22,13 @@ describe('client', () => { beforeEach(() => { startChannelCb = sinon.stub() - startChannel.subscribe(startChannelCb) endChannelCb = sinon.stub() - endChannel.subscribe(endChannelCb) asyncStartChannelCb = sinon.stub() - asyncStartChannel.subscribe(asyncStartChannelCb) errorChannelCb = sinon.stub() + + startChannel.subscribe(startChannelCb) + endChannel.subscribe(endChannelCb) + asyncStartChannel.subscribe(asyncStartChannelCb) errorChannel.subscribe(errorChannelCb) }) diff --git a/packages/dd-trace/src/exporters/common/writer.js b/packages/dd-trace/src/exporters/common/writer.js index 8dbe21ca4d1..57f08accd32 100644 --- a/packages/dd-trace/src/exporters/common/writer.js +++ b/packages/dd-trace/src/exporters/common/writer.js @@ -17,6 +17,7 @@ class Writer { done() } else if (count > 0) { const payload = this._encoder.makePayload() + this._sendPayload(payload, count, done) } else { done() diff --git a/packages/dd-trace/test/appsec/rasp.express.plugin.spec.js b/packages/dd-trace/test/appsec/rasp.express.plugin.spec.js index a5537ca978c..8ebc75e6f22 100644 --- a/packages/dd-trace/test/appsec/rasp.express.plugin.spec.js +++ b/packages/dd-trace/test/appsec/rasp.express.plugin.spec.js @@ -173,7 +173,7 @@ describe('RASP', () => { it('Should detect threat doing a POST request', async () => { app = async (req, res) => { try { - await axios.post(`https://${req.query.host}`, { key: 'value' }) + await axiosToTest.post(`https://${req.query.host}`, { key: 'value' }) } catch (e) { if (e.cause.message === 'AbortError') { res.writeHead(500) @@ -241,8 +241,13 @@ describe('RASP', () => { }) clientRequest.on('error', (e) => { - res.writeHead(500) - res.end('error') + if (e.name !== 'AbortError') { + res.writeHead(200) + res.end('not-blocking-error') + } else { + res.writeHead(500) + res.end('unexpected-blocking-error') + } }) } From 3e8831256913d2c418977487ba7f932293001773 Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Thu, 27 Jun 2024 09:17:17 +0200 Subject: [PATCH 21/41] Try to handle aborterror when app has its own uncaughtExceptionCaptureCallback --- integration-tests/appsec/index.spec.js | 38 ++++++++++++++++--- integration-tests/appsec/rasp/index.js | 13 +++++++ .../src/helpers/register.js | 3 ++ .../datadog-instrumentations/src/process.js | 29 ++++++++++++++ packages/dd-trace/src/appsec/rasp.js | 29 +++++++++++++- 5 files changed, 105 insertions(+), 7 deletions(-) create mode 100644 packages/datadog-instrumentations/src/process.js diff --git a/integration-tests/appsec/index.spec.js b/integration-tests/appsec/index.spec.js index 36206d4fee2..c05ca2b5156 100644 --- a/integration-tests/appsec/index.spec.js +++ b/integration-tests/appsec/index.spec.js @@ -70,7 +70,7 @@ describe('RASP', () => { } try { - await axios.get(`${path}?host=ifconfig.pro`) + await axios.get(`${path}?host=localhost/ifconfig.pro`) assert.fail('Request should have failed') } catch (e) { @@ -151,7 +151,6 @@ describe('RASP', () => { assert.fail('Request should have failed') } catch (e) { - // console.log(e) return new Promise((resolve, reject) => { setTimeout(() => { if (hasOutput) { @@ -168,7 +167,7 @@ describe('RASP', () => { let response try { - response = await axios.get('/ssrf/http/manual-blocking?host=ifconfig.pro') + response = await axios.get('/ssrf/http/manual-blocking?host=localhost/ifconfig.pro') assert.fail('Request should have failed') } catch (e) { @@ -183,7 +182,7 @@ describe('RASP', () => { it('should block when error is unhandled', async () => { try { - await axios.get('/ssrf/http/unhandled-error?host=ifconfig.pro') + await axios.get('/ssrf/http/unhandled-error?host=localhost/ifconfig.pro') assert.fail('Request should have failed') } catch (e) { @@ -196,6 +195,35 @@ describe('RASP', () => { } }) + it('should not execute custom uncaughtExceptionCaptureCallback', async () => { + let hasOutput = false + try { + stdioHandler = () => { + hasOutput = true + } + + await axios.get('/ssrf/http/custom-uncaught-exception-capture-callback?host=localhost/ifconfig.pro') + + assert.fail('Request should have failed') + } catch (e) { + if (!e.response) { + throw e + } + + assert.strictEqual(e.response.status, 403) + await assertRaspDetected() + return new Promise((resolve, reject) => { + setTimeout(() => { + if (hasOutput) { + reject(new Error('uncaughtExceptionCaptureCallback executed')) + } else { + resolve() + } + }, 10) + }) + } + }) + it('should not crash when app send data after blocking', () => { return testNotCrashedAfterBlocking('/ssrf/http/unhandled-async-write-A') }) @@ -246,7 +274,7 @@ describe('RASP', () => { let response try { - response = await axios.get('/ssrf/http/manual-blocking?host=ifconfig.pro') + response = await axios.get('/ssrf/http/manual-blocking?host=localhost/ifconfig.pro') } catch (e) { if (!e.response) { throw e diff --git a/integration-tests/appsec/rasp/index.js b/integration-tests/appsec/rasp/index.js index 9ee445f7a1c..95c5d0fcc09 100644 --- a/integration-tests/appsec/rasp/index.js +++ b/integration-tests/appsec/rasp/index.js @@ -92,6 +92,19 @@ app.get('/ssrf/http/manual-blocking', (req, res) => { }) }) +app.get('/ssrf/http/custom-uncaught-exception-capture-callback', (req, res) => { + process.setUncaughtExceptionCaptureCallback(() => { + // wanted a log to force error when on tests + // eslint-disable-next-line no-console + console.log('Custom uncaught exception capture callback') + res.writeHead(500) + res.end('error') + }) + + http.get(`https://${req.query.host}`, () => { + res.send('end') + }) +}) app.get('/ssrf/http/unhandled-error', (req, res) => { makeOutgoingRequestAndCbAfterTimeout(req, res) }) diff --git a/packages/datadog-instrumentations/src/helpers/register.js b/packages/datadog-instrumentations/src/helpers/register.js index eba90d6a980..a48641d52b5 100644 --- a/packages/datadog-instrumentations/src/helpers/register.js +++ b/packages/datadog-instrumentations/src/helpers/register.js @@ -27,6 +27,9 @@ const loadChannel = channel('dd-trace:instrumentation:load') if (!disabledInstrumentations.has('fetch')) { require('../fetch') } +if (!disabledInstrumentations.has('process')) { + require('../process') +} const HOOK_SYMBOL = Symbol('hookExportsMap') diff --git a/packages/datadog-instrumentations/src/process.js b/packages/datadog-instrumentations/src/process.js new file mode 100644 index 00000000000..429b0d8f574 --- /dev/null +++ b/packages/datadog-instrumentations/src/process.js @@ -0,0 +1,29 @@ +'use strict' + +const shimmer = require('../../datadog-shimmer') +const { channel } = require('dc-polyfill') + +const startSetUncaughtExceptionCaptureCallback = channel('datadog:process:setUncaughtExceptionCaptureCallback:start') + +if (process.setUncaughtExceptionCaptureCallback) { + let currentCallback + + shimmer.wrap(process, 'setUncaughtExceptionCaptureCallback', + function wrapSetUncaughtExceptionCaptureCallback (originalSetUncaughtExceptionCaptureCallback) { + return function setUncaughtExceptionCaptureCallback (newCallback) { + if (startSetUncaughtExceptionCaptureCallback.hasSubscribers) { + const abortController = new AbortController() + startSetUncaughtExceptionCaptureCallback.publish({ newCallback, currentCallback, abortController }) + if (abortController.signal.aborted) { + return + } + } + + const result = originalSetUncaughtExceptionCaptureCallback.apply(this, arguments) + + currentCallback = newCallback + + return result + } + }) +} diff --git a/packages/dd-trace/src/appsec/rasp.js b/packages/dd-trace/src/appsec/rasp.js index 31d1af1f08e..b1294e82f94 100644 --- a/packages/dd-trace/src/appsec/rasp.js +++ b/packages/dd-trace/src/appsec/rasp.js @@ -7,7 +7,8 @@ const { httpClientRequestStart } = require('./channels') const { reportStackTrace } = require('./stack_trace') const waf = require('./waf') const { getBlockingAction, block } = require('./blocking') - +const { channel } = require('dc-polyfill') +const setUncaughtExceptionCaptureCallbackStart = channel('datadog:process:setUncaughtExceptionCaptureCallback:start') class AbortError extends Error { constructor (req, res, blockingAction) { super('AbortError') @@ -18,7 +19,7 @@ class AbortError extends Error { } } -function handleUncaughtExceptionMonitor (err, origin) { +function handleUncaughtExceptionMonitor (err) { if (err instanceof AbortError || err.cause instanceof AbortError) { const { req, res, blockingAction } = err block(req, res, web.root(req), null, blockingAction) @@ -27,6 +28,30 @@ function handleUncaughtExceptionMonitor (err, origin) { process.setUncaughtExceptionCaptureCallback(() => { process.setUncaughtExceptionCaptureCallback(null) }) + } else { + let previousCb + const cb = ({ currentCallback, abortController }) => { + setUncaughtExceptionCaptureCallbackStart.unsubscribe(cb) + if (!currentCallback) { + abortController.abort() + return + } + + previousCb = currentCallback + } + + setUncaughtExceptionCaptureCallbackStart.subscribe(cb) + + process.setUncaughtExceptionCaptureCallback(null) + + // For some reason, previous callback was defiend before the instrumentation + // We can not restore it, so we let the app decide + if (previousCb) { + process.setUncaughtExceptionCaptureCallback(() => { + process.setUncaughtExceptionCaptureCallback(null) + process.setUncaughtExceptionCaptureCallback(previousCb) + }) + } } } } From 693d8bee3d3c50701fc6f772e944b3a339784c61 Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Thu, 27 Jun 2024 09:41:38 +0200 Subject: [PATCH 22/41] Typo --- packages/dd-trace/src/appsec/rasp.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/dd-trace/src/appsec/rasp.js b/packages/dd-trace/src/appsec/rasp.js index b1294e82f94..d489414f8d5 100644 --- a/packages/dd-trace/src/appsec/rasp.js +++ b/packages/dd-trace/src/appsec/rasp.js @@ -44,7 +44,7 @@ function handleUncaughtExceptionMonitor (err) { process.setUncaughtExceptionCaptureCallback(null) - // For some reason, previous callback was defiend before the instrumentation + // For some reason, previous callback was defined before the instrumentation // We can not restore it, so we let the app decide if (previousCb) { process.setUncaughtExceptionCaptureCallback(() => { From 32e3627e64d25ab4d223ec3ab00255bee4d256b6 Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Thu, 27 Jun 2024 09:56:02 +0200 Subject: [PATCH 23/41] Use correct axios in the test --- packages/dd-trace/test/appsec/rasp.express.plugin.spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/dd-trace/test/appsec/rasp.express.plugin.spec.js b/packages/dd-trace/test/appsec/rasp.express.plugin.spec.js index 8ebc75e6f22..6d4abb7c3c8 100644 --- a/packages/dd-trace/test/appsec/rasp.express.plugin.spec.js +++ b/packages/dd-trace/test/appsec/rasp.express.plugin.spec.js @@ -142,7 +142,7 @@ describe('RASP', () => { it('Should not detect threat', async () => { app = (req, res) => { - axios.get(`https://${req.query.host}`) + axiosToTest.get(`https://${req.query.host}`) res.end('end') } From 14d5283978af7cefcb33b3a3c0b101d084363f72 Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Thu, 27 Jun 2024 11:32:48 +0200 Subject: [PATCH 24/41] Fix test --- packages/dd-trace/test/appsec/rasp.express.plugin.spec.js | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/dd-trace/test/appsec/rasp.express.plugin.spec.js b/packages/dd-trace/test/appsec/rasp.express.plugin.spec.js index ea870a1a8c1..6ae4e06fe56 100644 --- a/packages/dd-trace/test/appsec/rasp.express.plugin.spec.js +++ b/packages/dd-trace/test/appsec/rasp.express.plugin.spec.js @@ -258,7 +258,6 @@ describe('RASP', () => { }) assert.equal(response.status, 200) - assert.equal(response.data, 'timeout') await agent.use((traces) => { const span = getWebSpan(traces) From 519751cfe4cae81f4cd7faeb47e6be81d7aff549 Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Thu, 27 Jun 2024 17:32:25 +0200 Subject: [PATCH 25/41] Compatibility with domain --- integration-tests/appsec/index.spec.js | 145 +++++++++++++++++-------- integration-tests/appsec/rasp/index.js | 24 ++++ packages/dd-trace/src/appsec/rasp.js | 43 ++++++-- 3 files changed, 156 insertions(+), 56 deletions(-) diff --git a/integration-tests/appsec/index.spec.js b/integration-tests/appsec/index.spec.js index c05ca2b5156..dbbab918659 100644 --- a/integration-tests/appsec/index.spec.js +++ b/integration-tests/appsec/index.spec.js @@ -93,28 +93,61 @@ describe('RASP', () => { }) } - describe('ssrf', () => { - it('should crash when error is not an AbortError', async () => { - let hasOutput = false + async function testCustomErrorHandlerIsNotExecuted (path) { + let hasOutput = false + try { stdioHandler = () => { hasOutput = true } - try { - await axios.get('/crash') + await axios.get(`${path}?host=localhost/ifconfig.pro`) - assert.fail('Request should have failed') - } catch (e) { - return new Promise((resolve, reject) => { - setTimeout(() => { - if (hasOutput) { - resolve() - } else { - reject(new Error('Output expected after crash')) - } - }, 50) - }) + assert.fail('Request should have failed') + } catch (e) { + if (!e.response) { + throw e } + + assert.strictEqual(e.response.status, 403) + await assertRaspDetected() + + return new Promise((resolve, reject) => { + setTimeout(() => { + if (hasOutput) { + reject(new Error('uncaughtExceptionCaptureCallback executed')) + } else { + resolve() + } + }, 10) + }) + } + } + + async function testAppCrashesAsExpected () { + let hasOutput = false + stdioHandler = () => { + hasOutput = true + } + + try { + await axios.get('/crash') + } catch (e) { + return new Promise((resolve, reject) => { + setTimeout(() => { + if (hasOutput) { + resolve() + } else { + reject(new Error('Output expected after crash')) + } + }, 50) + }) + } + assert.fail('Request should have failed') + } + + describe('ssrf', () => { + it('should crash when error is not an AbortError', async () => { + await testAppCrashesAsExpected() }) it('should not crash when customer has his own setUncaughtExceptionCaptureCallback', async () => { @@ -125,8 +158,6 @@ describe('RASP', () => { try { await axios.get('/crash-and-recovery-A') - - assert.fail('Request should have failed') } catch (e) { return new Promise((resolve, reject) => { setTimeout(() => { @@ -138,6 +169,8 @@ describe('RASP', () => { }, 50) }) } + + assert.fail('Request should have failed') }) it('should not crash when customer has his own uncaughtException', async () => { @@ -148,8 +181,6 @@ describe('RASP', () => { try { await axios.get('/crash-and-recovery-B') - - assert.fail('Request should have failed') } catch (e) { return new Promise((resolve, reject) => { setTimeout(() => { @@ -161,6 +192,8 @@ describe('RASP', () => { }, 50) }) } + + assert.fail('Request should have failed') }) it('should block manually', async () => { @@ -168,60 +201,76 @@ describe('RASP', () => { try { response = await axios.get('/ssrf/http/manual-blocking?host=localhost/ifconfig.pro') - - assert.fail('Request should have failed') } catch (e) { if (!e.response) { throw e } response = e.response + assert.strictEqual(response.status, 418) + return await assertRaspDetected() } - assert.strictEqual(response.status, 418) - await assertRaspDetected() + + assert.fail('Request should have failed') }) - it('should block when error is unhandled', async () => { - try { - await axios.get('/ssrf/http/unhandled-error?host=localhost/ifconfig.pro') + it('should block in a domain', async () => { + let response - assert.fail('Request should have failed') + try { + response = await axios.get('/ssrf/http/should-block-in-domain?host=localhost/ifconfig.pro') } catch (e) { if (!e.response) { throw e } - - assert.strictEqual(e.response.status, 403) - await assertRaspDetected() + response = e.response + assert.strictEqual(response.status, 403) + return await assertRaspDetected() } + + assert.fail('Request should have failed') }) - it('should not execute custom uncaughtExceptionCaptureCallback', async () => { - let hasOutput = false + it('should crash as expected after block in domain request', async () => { try { - stdioHandler = () => { - hasOutput = true - } + await axios.get('/ssrf/http/should-block-in-domain?host=localhost/ifconfig.pro') + } catch (e) { + return await testAppCrashesAsExpected() + } - await axios.get('/ssrf/http/custom-uncaught-exception-capture-callback?host=localhost/ifconfig.pro') + assert.fail('Request should have failed') + }) - assert.fail('Request should have failed') + it('should block when error is unhandled', async () => { + try { + await axios.get('/ssrf/http/unhandled-error?host=localhost/ifconfig.pro') } catch (e) { if (!e.response) { throw e } assert.strictEqual(e.response.status, 403) - await assertRaspDetected() - return new Promise((resolve, reject) => { - setTimeout(() => { - if (hasOutput) { - reject(new Error('uncaughtExceptionCaptureCallback executed')) - } else { - resolve() - } - }, 10) - }) + return await assertRaspDetected() + } + + assert.fail('Request should have failed') + }) + + it('should crash as expected after a requiest block when error is unhandled', async () => { + try { + await axios.get('/ssrf/http/unhandled-error?host=localhost/ifconfig.pro') + } catch (e) { + return await testAppCrashesAsExpected() } + + assert.fail('Request should have failed') + }) + + it('should not execute custom uncaughtExceptionCaptureCallback when it is blocked', async () => { + return testCustomErrorHandlerIsNotExecuted('/ssrf/http/custom-uncaught-exception-capture-callback') + }) + + it('should not execute custom uncaughtException listener', async () => { + return testCustomErrorHandlerIsNotExecuted('/ssrf/http/custom-uncaughtException-listener') }) it('should not crash when app send data after blocking', () => { diff --git a/integration-tests/appsec/rasp/index.js b/integration-tests/appsec/rasp/index.js index 95c5d0fcc09..41ecdc04385 100644 --- a/integration-tests/appsec/rasp/index.js +++ b/integration-tests/appsec/rasp/index.js @@ -105,6 +105,30 @@ app.get('/ssrf/http/custom-uncaught-exception-capture-callback', (req, res) => { res.send('end') }) }) + +app.get('/ssrf/http/should-block-in-domain', (req, res) => { + const d = require('node:domain').create() + d.run(() => { + http.get(`https://${req.query.host}`, () => { + res.send('end') + }) + }) +}) + +app.get('/ssrf/http/custom-uncaughtException-listener', (req, res) => { + process.on('uncaughtException', () => { + // wanted a log to force error when on tests + // eslint-disable-next-line no-console + console.log('Custom uncaught exception capture callback') + res.writeHead(500) + res.end('error') + }) + + http.get(`https://${req.query.host}`, () => { + res.send('end') + }) +}) + app.get('/ssrf/http/unhandled-error', (req, res) => { makeOutgoingRequestAndCbAfterTimeout(req, res) }) diff --git a/packages/dd-trace/src/appsec/rasp.js b/packages/dd-trace/src/appsec/rasp.js index d489414f8d5..e4af3c16553 100644 --- a/packages/dd-trace/src/appsec/rasp.js +++ b/packages/dd-trace/src/appsec/rasp.js @@ -9,6 +9,7 @@ const waf = require('./waf') const { getBlockingAction, block } = require('./blocking') const { channel } = require('dc-polyfill') const setUncaughtExceptionCaptureCallbackStart = channel('datadog:process:setUncaughtExceptionCaptureCallback:start') + class AbortError extends Error { constructor (req, res, blockingAction) { super('AbortError') @@ -19,16 +20,48 @@ class AbortError extends Error { } } +const RULE_TYPES = { + SSRF: 'ssrf' +} + +let config, abortOnUncaughtException + +function removeAllListeners (emitter, event) { + const listeners = emitter.listeners(event) + emitter.removeAllListeners(event) + + let cleaned = false + return function () { + if (cleaned === true) { + return + } + cleaned = true + + for (let i = 0; i < listeners.length; ++i) { + emitter.on(event, listeners[i]) + } + } +} + function handleUncaughtExceptionMonitor (err) { if (err instanceof AbortError || err.cause instanceof AbortError) { const { req, res, blockingAction } = err block(req, res, web.root(req), null, blockingAction) if (!process.hasUncaughtExceptionCaptureCallback()) { - process.setUncaughtExceptionCaptureCallback(() => { - process.setUncaughtExceptionCaptureCallback(null) + const cleanUp = removeAllListeners(process, 'uncaughtException') + const handler = () => { + process.removeListener('uncaughtException', handler) + } + + setTimeout(() => { + process.removeListener('uncaughtException', handler) + cleanUp() }) + + process.on('uncaughtException', handler) } else { + // uncaughtException event is not executed when hasUncaughtExceptionCaptureCallback is true let previousCb const cb = ({ currentCallback, abortController }) => { setUncaughtExceptionCaptureCallbackStart.unsubscribe(cb) @@ -56,12 +89,6 @@ function handleUncaughtExceptionMonitor (err) { } } -const RULE_TYPES = { - SSRF: 'ssrf' -} - -let config, abortOnUncaughtException - function enable (_config) { config = _config httpClientRequestStart.subscribe(analyzeSsrf) From 68a000919a55a954f970e1fe8780b959aa814486 Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Thu, 27 Jun 2024 17:46:01 +0200 Subject: [PATCH 26/41] Small changes --- integration-tests/appsec/index.spec.js | 15 ++++++++------- integration-tests/appsec/rasp/index.js | 4 ++-- .../src/helpers/register.js | 1 + packages/dd-trace/src/appsec/channels.js | 5 ++++- packages/dd-trace/src/appsec/rasp.js | 3 +-- 5 files changed, 16 insertions(+), 12 deletions(-) diff --git a/integration-tests/appsec/index.spec.js b/integration-tests/appsec/index.spec.js index dbbab918659..a7c65b3932d 100644 --- a/integration-tests/appsec/index.spec.js +++ b/integration-tests/appsec/index.spec.js @@ -53,7 +53,7 @@ describe('RASP', () => { }) } - async function assertRaspDetected () { + async function assertExploitDetected () { await agent.assertMessageReceived(({ headers, payload }) => { assert.property(payload[0][0].meta, '_dd.appsec.json') assert.include(payload[0][0].meta['_dd.appsec.json'], '"test-rule-id-2"') @@ -79,7 +79,7 @@ describe('RASP', () => { } assert.strictEqual(e.response.status, 403) - await assertRaspDetected() + await assertExploitDetected() } return new Promise((resolve, reject) => { @@ -109,7 +109,7 @@ describe('RASP', () => { } assert.strictEqual(e.response.status, 403) - await assertRaspDetected() + await assertExploitDetected() return new Promise((resolve, reject) => { setTimeout(() => { @@ -142,6 +142,7 @@ describe('RASP', () => { }, 50) }) } + assert.fail('Request should have failed') } @@ -207,7 +208,7 @@ describe('RASP', () => { } response = e.response assert.strictEqual(response.status, 418) - return await assertRaspDetected() + return await assertExploitDetected() } assert.fail('Request should have failed') @@ -224,7 +225,7 @@ describe('RASP', () => { } response = e.response assert.strictEqual(response.status, 403) - return await assertRaspDetected() + return await assertExploitDetected() } assert.fail('Request should have failed') @@ -249,7 +250,7 @@ describe('RASP', () => { } assert.strictEqual(e.response.status, 403) - return await assertRaspDetected() + return await assertExploitDetected() } assert.fail('Request should have failed') @@ -334,7 +335,7 @@ describe('RASP', () => { // not blocked assert.notEqual(response.status, 418) assert.notEqual(response.status, 403) - await assertRaspDetected() + await assertExploitDetected() }) }) }) diff --git a/integration-tests/appsec/rasp/index.js b/integration-tests/appsec/rasp/index.js index 41ecdc04385..ab6c15ce2a9 100644 --- a/integration-tests/appsec/rasp/index.js +++ b/integration-tests/appsec/rasp/index.js @@ -94,7 +94,7 @@ app.get('/ssrf/http/manual-blocking', (req, res) => { app.get('/ssrf/http/custom-uncaught-exception-capture-callback', (req, res) => { process.setUncaughtExceptionCaptureCallback(() => { - // wanted a log to force error when on tests + // wanted a log to force error on tests // eslint-disable-next-line no-console console.log('Custom uncaught exception capture callback') res.writeHead(500) @@ -117,7 +117,7 @@ app.get('/ssrf/http/should-block-in-domain', (req, res) => { app.get('/ssrf/http/custom-uncaughtException-listener', (req, res) => { process.on('uncaughtException', () => { - // wanted a log to force error when on tests + // wanted a log to force error on tests // eslint-disable-next-line no-console console.log('Custom uncaught exception capture callback') res.writeHead(500) diff --git a/packages/datadog-instrumentations/src/helpers/register.js b/packages/datadog-instrumentations/src/helpers/register.js index 8703a337653..dfdb680c1ca 100644 --- a/packages/datadog-instrumentations/src/helpers/register.js +++ b/packages/datadog-instrumentations/src/helpers/register.js @@ -28,6 +28,7 @@ const loadChannel = channel('dd-trace:instrumentation:load') if (!disabledInstrumentations.has('fetch')) { require('../fetch') } + if (!disabledInstrumentations.has('process')) { require('../process') } diff --git a/packages/dd-trace/src/appsec/channels.js b/packages/dd-trace/src/appsec/channels.js index b669668af2b..77cbe5afeb4 100644 --- a/packages/dd-trace/src/appsec/channels.js +++ b/packages/dd-trace/src/appsec/channels.js @@ -1,6 +1,7 @@ 'use strict' const dc = require('dc-polyfill') +const { channel } = require('dc-polyfill') // TODO: use TBD naming convention module.exports = { @@ -20,5 +21,7 @@ module.exports = { responseBody: dc.channel('datadog:express:response:json:start'), responseWriteHead: dc.channel('apm:http:server:response:writeHead:start'), httpClientRequestStart: dc.channel('apm:http:client:request:start'), - responseSetHeader: dc.channel('datadog:http:server:response:set-header:start') + responseSetHeader: dc.channel('datadog:http:server:response:set-header:start'), + setUncaughtExceptionCaptureCallbackStart: dc.channel('datadog:process:setUncaughtExceptionCaptureCallback:start') + } diff --git a/packages/dd-trace/src/appsec/rasp.js b/packages/dd-trace/src/appsec/rasp.js index e4af3c16553..a7744d14ca6 100644 --- a/packages/dd-trace/src/appsec/rasp.js +++ b/packages/dd-trace/src/appsec/rasp.js @@ -3,12 +3,11 @@ const { storage } = require('../../../datadog-core') const web = require('./../plugins/util/web') const addresses = require('./addresses') -const { httpClientRequestStart } = require('./channels') +const { httpClientRequestStart, setUncaughtExceptionCaptureCallbackStart } = require('./channels') const { reportStackTrace } = require('./stack_trace') const waf = require('./waf') const { getBlockingAction, block } = require('./blocking') const { channel } = require('dc-polyfill') -const setUncaughtExceptionCaptureCallbackStart = channel('datadog:process:setUncaughtExceptionCaptureCallback:start') class AbortError extends Error { constructor (req, res, blockingAction) { From 268de006cae72ea4707204f3386abc0a836e16da Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Thu, 27 Jun 2024 17:52:44 +0200 Subject: [PATCH 27/41] Small change --- .../test/appsec/rasp.express.plugin.spec.js | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/packages/dd-trace/test/appsec/rasp.express.plugin.spec.js b/packages/dd-trace/test/appsec/rasp.express.plugin.spec.js index 6ae4e06fe56..9a06fe1b736 100644 --- a/packages/dd-trace/test/appsec/rasp.express.plugin.spec.js +++ b/packages/dd-trace/test/appsec/rasp.express.plugin.spec.js @@ -63,22 +63,22 @@ describe('RASP', () => { async function testBlockingRequest () { try { await axios.get('/?host=localhost/ifconfig.pro') - assert.fail('Request should be blocked') } catch (e) { if (!e.response) { throw e } + return await agent.use((traces) => { + const span = getWebSpan(traces) + assert.property(span.meta, '_dd.appsec.json') + assert(span.meta['_dd.appsec.json'].includes('rasp-ssrf-rule-id-1')) + assert.equal(span.metrics['_dd.appsec.rasp.rule.eval'], 1) + assert(span.metrics['_dd.appsec.rasp.duration'] > 0) + assert(span.metrics['_dd.appsec.rasp.duration_ext'] > 0) + assert.property(span.meta_struct, '_dd.stack') + }) } - await agent.use((traces) => { - const span = getWebSpan(traces) - assert.property(span.meta, '_dd.appsec.json') - assert(span.meta['_dd.appsec.json'].includes('rasp-ssrf-rule-id-1')) - assert.equal(span.metrics['_dd.appsec.rasp.rule.eval'], 1) - assert(span.metrics['_dd.appsec.rasp.duration'] > 0) - assert(span.metrics['_dd.appsec.rasp.duration_ext'] > 0) - assert.property(span.meta_struct, '_dd.stack') - }) + assert.fail('Request should be blocked') } ['http', 'https'].forEach(protocol => { From 328841041b3fd0c902d82d8c87a36a87b7c6a513 Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Thu, 27 Jun 2024 18:00:53 +0200 Subject: [PATCH 28/41] Lint fix --- packages/dd-trace/src/appsec/channels.js | 1 - packages/dd-trace/src/appsec/rasp.js | 1 - 2 files changed, 2 deletions(-) diff --git a/packages/dd-trace/src/appsec/channels.js b/packages/dd-trace/src/appsec/channels.js index 77cbe5afeb4..66781d88821 100644 --- a/packages/dd-trace/src/appsec/channels.js +++ b/packages/dd-trace/src/appsec/channels.js @@ -1,7 +1,6 @@ 'use strict' const dc = require('dc-polyfill') -const { channel } = require('dc-polyfill') // TODO: use TBD naming convention module.exports = { diff --git a/packages/dd-trace/src/appsec/rasp.js b/packages/dd-trace/src/appsec/rasp.js index a7744d14ca6..be38ccb1e5c 100644 --- a/packages/dd-trace/src/appsec/rasp.js +++ b/packages/dd-trace/src/appsec/rasp.js @@ -7,7 +7,6 @@ const { httpClientRequestStart, setUncaughtExceptionCaptureCallbackStart } = req const { reportStackTrace } = require('./stack_trace') const waf = require('./waf') const { getBlockingAction, block } = require('./blocking') -const { channel } = require('dc-polyfill') class AbortError extends Error { constructor (req, res, blockingAction) { From 951898d80f4176a8ca29c74440c739acfd2c3408 Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Fri, 28 Jun 2024 09:02:25 +0200 Subject: [PATCH 29/41] Enable appsec integration tests --- .github/workflows/appsec.yml | 10 ++++++++++ package.json | 1 + 2 files changed, 11 insertions(+) diff --git a/.github/workflows/appsec.yml b/.github/workflows/appsec.yml index 9e6ff9cb764..05d4007edf8 100644 --- a/.github/workflows/appsec.yml +++ b/.github/workflows/appsec.yml @@ -236,3 +236,13 @@ jobs: - uses: ./.github/actions/node/latest - run: yarn test:appsec:plugins:ci - uses: codecov/codecov-action@v3 + + integration: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - run: yarn install + - uses: ./.github/actions/node/oldest + - run: yarn test:integration:appsec + - uses: ./.github/actions/node/latest + - run: yarn test:integration:appsec diff --git a/package.json b/package.json index eb6993096ce..b98aaca358a 100644 --- a/package.json +++ b/package.json @@ -33,6 +33,7 @@ "test:profiler": "tap \"packages/dd-trace/test/profiling/**/*.spec.js\"", "test:profiler:ci": "npm run test:profiler -- --coverage --nyc-arg=--include=\"packages/dd-trace/src/profiling/**/*.js\"", "test:integration": "mocha --colors --timeout 30000 -r \"packages/dd-trace/test/setup/core.js\" \"integration-tests/*.spec.js\"", + "test:integration:appsec": "mocha --colors --timeout 30000 -r \"packages/dd-trace/test/setup/core.js\" \"integration-tests/appsec/*.spec.js\"", "test:integration:cucumber": "mocha --colors --timeout 30000 -r \"packages/dd-trace/test/setup/core.js\" \"integration-tests/cucumber/*.spec.js\"", "test:integration:cypress": "mocha --colors --timeout 30000 -r \"packages/dd-trace/test/setup/core.js\" \"integration-tests/cypress/*.spec.js\"", "test:integration:playwright": "mocha --colors --timeout 30000 -r \"packages/dd-trace/test/setup/core.js\" \"integration-tests/playwright/*.spec.js\"", From 194ec41377fe4f33f946cda8c360ebc58ca177a3 Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Fri, 28 Jun 2024 16:37:27 +0200 Subject: [PATCH 30/41] Change blocking approach --- .../src/http/client.js | 41 +++---------------- 1 file changed, 5 insertions(+), 36 deletions(-) diff --git a/packages/datadog-instrumentations/src/http/client.js b/packages/datadog-instrumentations/src/http/client.js index 856c1c0f8e2..bfd657756b9 100644 --- a/packages/datadog-instrumentations/src/http/client.js +++ b/packages/datadog-instrumentations/src/http/client.js @@ -7,7 +7,6 @@ const { channel, addHook } = require('../helpers/instrument') const shimmer = require('../../../datadog-shimmer') const log = require('../../../dd-trace/src/log') -const { storage } = require('../../../datadog-core') const startChannel = channel('apm:http:client:request:start') const finishChannel = channel('apm:http:client:request:finish') @@ -26,30 +25,6 @@ function hookFn (http) { return http } -let ClientRequest -function noop () {} - -function createAbortedClientRequest (http, args) { - if (!ClientRequest) { - const store = storage.getStore() - storage.enterWith({ noop: true }) - - const clientRequest = http.get('') - clientRequest.on('error', noop) - ClientRequest = Object.getPrototypeOf(clientRequest).constructor - - storage.enterWith(store) - } - - return new ClientRequest({ - _defaultAgent: http.globalAgent, // needed to support http and https - ...args.options, - agent: { // noop agent, to prevent doing a real request - addRequest: noop - } - }) -} - function patch (http, methodName) { shimmer.wrap(http, methodName, instrumentRequest) @@ -94,17 +69,7 @@ function patch (http, methodName) { } try { - let req - if (abortController.signal.aborted) { - req = createAbortedClientRequest(http, args) - - process.nextTick(() => { - req.emit('error', abortController.signal.reason || new Error('Aborted')) - }) - } else { - req = request.call(this, options, callback) - } - + const req = request.call(this, options, callback) const emit = req.emit const setTimeout = req.setTimeout @@ -144,6 +109,10 @@ function patch (http, methodName) { return emit.apply(this, arguments) } + if (abortController.signal.aborted) { + req.destroy(abortController.signal.reason || new Error('Aborted')) + } + return req } catch (e) { ctx.error = e From be9d2f8e4ac976d0785717fad6311454ed4b85ac Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Wed, 3 Jul 2024 12:33:31 +0200 Subject: [PATCH 31/41] Apply PR suggestions --- .../src/http/server.js | 29 ++++--------------- packages/dd-trace/src/appsec/blocking.js | 6 +++- packages/dd-trace/src/appsec/index.js | 9 ++---- packages/dd-trace/test/appsec/index.spec.js | 20 +++++++++---- 4 files changed, 29 insertions(+), 35 deletions(-) diff --git a/packages/datadog-instrumentations/src/http/server.js b/packages/datadog-instrumentations/src/http/server.js index 3ffc8640023..042d23cb9dc 100644 --- a/packages/datadog-instrumentations/src/http/server.js +++ b/packages/datadog-instrumentations/src/http/server.js @@ -26,10 +26,10 @@ addHook({ name: httpNames }, http => { shimmer.wrap(http.ServerResponse.prototype, 'write', wrapWrite) shimmer.wrap(http.ServerResponse.prototype, 'end', wrapEnd) shimmer.wrap(http.ServerResponse.prototype, 'setHeader', wrapSetHeader) - shimmer.wrap(http.ServerResponse.prototype, 'removeHeader', wrapRemoveHeader) + shimmer.wrap(http.ServerResponse.prototype, 'removeHeader', wrapAppendOrRemoveHeader) // Added in node v16.17.0 if (http.ServerResponse.prototype.appendHeader) { - shimmer.wrap(http.ServerResponse.prototype, 'appendHeader', wrapAppendHeader) + shimmer.wrap(http.ServerResponse.prototype, 'appendHeader', wrapAppendOrRemoveHeader) } return http }) @@ -179,10 +179,10 @@ function wrapSetHeader (setHeader) { } } -function wrapAppendHeader (appendHeader) { - return function wrappedAppendHeader () { +function wrapAppendOrRemoveHeader (originalMethod) { + return function wrappedAppendOrRemoveHeade () { if (!startSetHeaderCh.hasSubscribers) { - return appendHeader.apply(this, arguments) + return originalMethod.apply(this, arguments) } const abortController = new AbortController() @@ -192,24 +192,7 @@ function wrapAppendHeader (appendHeader) { return this } - return appendHeader.apply(this, arguments) - } -} - -function wrapRemoveHeader (removeHeader) { - return function wrappedRemoveHeader () { - if (!startSetHeaderCh.hasSubscribers) { - return removeHeader.apply(this, arguments) - } - - const abortController = new AbortController() - startSetHeaderCh.publish({ res: this, abortController }) - - if (abortController.signal.aborted) { - return this - } - - return removeHeader.apply(this, arguments) + return originalMethod.apply(this, arguments) } } diff --git a/packages/dd-trace/src/appsec/blocking.js b/packages/dd-trace/src/appsec/blocking.js index 3c0ee72d042..43ef9799c10 100644 --- a/packages/dd-trace/src/appsec/blocking.js +++ b/packages/dd-trace/src/appsec/blocking.js @@ -148,6 +148,10 @@ function setTemplates (config) { } } +function isBlocked (res) { + return responseBlockedSet.has(res) +} + module.exports = { addSpecificEndpoint, block, @@ -155,5 +159,5 @@ module.exports = { getBlockingData, getBlockingAction, setTemplates, - responseBlockedSet + isBlocked } diff --git a/packages/dd-trace/src/appsec/index.js b/packages/dd-trace/src/appsec/index.js index a1da5622dd8..68c07c9862e 100644 --- a/packages/dd-trace/src/appsec/index.js +++ b/packages/dd-trace/src/appsec/index.js @@ -24,7 +24,7 @@ const apiSecuritySampler = require('./api_security_sampler') const web = require('../plugins/util/web') const { extractIp } = require('../plugins/util/ip_extractor') const { HTTP_CLIENT_IP } = require('../../../../ext/tags') -const { responseBlockedSet, block, setTemplates, getBlockingAction } = require('./blocking') +const { isBlocked, block, setTemplates, getBlockingAction } = require('./blocking') const { passportTrackEvent } = require('./passport') const { storage } = require('../../../datadog-core') const graphql = require('./graphql') @@ -228,7 +228,7 @@ const responseAnalyzedSet = new WeakSet() function onResponseWriteHead ({ req, res, abortController, statusCode, responseHeaders }) { // avoid "write after end" error - if (responseBlockedSet.has(res)) { + if (isBlocked(res)) { abortController?.abort() return } @@ -257,7 +257,7 @@ function onResponseWriteHead ({ req, res, abortController, statusCode, responseH } function onResponseSetHeader ({ res, abortController }) { - if (responseBlockedSet.has(res)) { + if (isBlocked(res)) { abortController?.abort() } } @@ -268,9 +268,6 @@ function handleResults (actions, req, res, rootSpan, abortController) { const blockingAction = getBlockingAction(actions) if (blockingAction) { block(req, res, rootSpan, abortController, blockingAction) - if (!abortController.signal || abortController.signal.aborted) { - responseBlockedSet.add(res) - } } } diff --git a/packages/dd-trace/test/appsec/index.spec.js b/packages/dd-trace/test/appsec/index.spec.js index ab461837264..27c2c6fde32 100644 --- a/packages/dd-trace/test/appsec/index.spec.js +++ b/packages/dd-trace/test/appsec/index.spec.js @@ -81,8 +81,7 @@ describe('AppSec Index', function () { responseBlockedSet = new WeakSet() blocking = { - setTemplates: sinon.stub(), - responseBlockedSet + setTemplates: sinon.stub() } passport = { @@ -927,12 +926,23 @@ describe('AppSec Index', function () { describe('onResponseSetHeader', () => { it('should call abortController if response was already blocked', () => { - responseBlockedSet.add(res) + // First block the request + sinon.stub(waf, 'run').returns(resultActions) + + const responseHeaders = { + 'content-type': 'application/json', + 'content-lenght': 42, + 'set-cookie': 'a=1;b=2' + } + responseWriteHead.publish({ req, res, abortController, statusCode: 404, responseHeaders }) + + expect(abortController.abort).to.have.been.calledOnce + + abortController.abort.reset() responseSetHeader.publish({ res, abortController }) - expect(abortController.abort).to.have.been.called - responseBlockedSet.delete(res) + expect(abortController.abort).to.have.been.calledOnce }) it('should not call abortController if response was not blocked', () => { From 6274362223794607381c30140b812e823faf569c Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Wed, 3 Jul 2024 12:43:30 +0200 Subject: [PATCH 32/41] Lint fix --- packages/dd-trace/src/appsec/index.js | 2 +- packages/dd-trace/test/appsec/index.spec.js | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/dd-trace/src/appsec/index.js b/packages/dd-trace/src/appsec/index.js index 68c07c9862e..8ed57da790c 100644 --- a/packages/dd-trace/src/appsec/index.js +++ b/packages/dd-trace/src/appsec/index.js @@ -24,7 +24,7 @@ const apiSecuritySampler = require('./api_security_sampler') const web = require('../plugins/util/web') const { extractIp } = require('../plugins/util/ip_extractor') const { HTTP_CLIENT_IP } = require('../../../../ext/tags') -const { isBlocked, block, setTemplates, getBlockingAction } = require('./blocking') +const { isBlocked, block, setTemplates, getBlockingAction } = require('./blocking') const { passportTrackEvent } = require('./passport') const { storage } = require('../../../datadog-core') const graphql = require('./graphql') diff --git a/packages/dd-trace/test/appsec/index.spec.js b/packages/dd-trace/test/appsec/index.spec.js index 27c2c6fde32..caaa9cad6fb 100644 --- a/packages/dd-trace/test/appsec/index.spec.js +++ b/packages/dd-trace/test/appsec/index.spec.js @@ -39,7 +39,6 @@ describe('AppSec Index', function () { let config let AppSec let web - let responseBlockedSet let blocking let passport let log @@ -79,7 +78,6 @@ describe('AppSec Index', function () { root: sinon.stub() } - responseBlockedSet = new WeakSet() blocking = { setTemplates: sinon.stub() } From 0a462d9281f14e3d4582345eff58dfbb27a42080 Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Wed, 3 Jul 2024 16:37:07 +0200 Subject: [PATCH 33/41] Update packages/dd-trace/src/appsec/rasp.js Co-authored-by: Igor Unanua --- packages/dd-trace/src/appsec/rasp.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/dd-trace/src/appsec/rasp.js b/packages/dd-trace/src/appsec/rasp.js index be38ccb1e5c..60345a6aa28 100644 --- a/packages/dd-trace/src/appsec/rasp.js +++ b/packages/dd-trace/src/appsec/rasp.js @@ -135,10 +135,10 @@ function handleResult (actions, req, res, abortController) { } const blockingAction = getBlockingAction(actions) - if (blockingAction && abortController) { + if (blockingAction && abortController && !abortOnUncaughtException) { const rootSpan = web.root(req) // Should block only in express - if (rootSpan.context()._name === 'express.request' && !abortOnUncaughtException) { + if (rootSpan?.context()._name === 'express.request') { const abortError = new AbortError(req, res, blockingAction) abortController.abort(abortError) From f2b7253c06657ad21db030b0ce90ce5a492d7e20 Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Wed, 3 Jul 2024 16:44:13 +0200 Subject: [PATCH 34/41] Rename AbortError to DatadogRaspAbortError --- integration-tests/appsec/rasp/index.js | 2 +- packages/dd-trace/src/appsec/rasp.js | 10 +++++----- .../dd-trace/test/appsec/rasp.express.plugin.spec.js | 10 +++++----- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/integration-tests/appsec/rasp/index.js b/integration-tests/appsec/rasp/index.js index ab6c15ce2a9..f6488892c8e 100644 --- a/integration-tests/appsec/rasp/index.js +++ b/integration-tests/appsec/rasp/index.js @@ -82,7 +82,7 @@ app.get('/ssrf/http/manual-blocking', (req, res) => { }) clientRequest.on('error', (err) => { - if (err.name === 'AbortError') { + if (err.name === 'DatadogRaspAbortError') { res.writeHead(418) res.end('aborted') } else { diff --git a/packages/dd-trace/src/appsec/rasp.js b/packages/dd-trace/src/appsec/rasp.js index 60345a6aa28..d1a95a7b334 100644 --- a/packages/dd-trace/src/appsec/rasp.js +++ b/packages/dd-trace/src/appsec/rasp.js @@ -8,10 +8,10 @@ const { reportStackTrace } = require('./stack_trace') const waf = require('./waf') const { getBlockingAction, block } = require('./blocking') -class AbortError extends Error { +class DatadogRaspAbortError extends Error { constructor (req, res, blockingAction) { - super('AbortError') - this.name = 'AbortError' + super('DatadogRaspAbortError') + this.name = 'DatadogRaspAbortError' this.req = req this.res = res this.blockingAction = blockingAction @@ -42,7 +42,7 @@ function removeAllListeners (emitter, event) { } function handleUncaughtExceptionMonitor (err) { - if (err instanceof AbortError || err.cause instanceof AbortError) { + if (err instanceof DatadogRaspAbortError || err.cause instanceof DatadogRaspAbortError) { const { req, res, blockingAction } = err block(req, res, web.root(req), null, blockingAction) @@ -139,7 +139,7 @@ function handleResult (actions, req, res, abortController) { const rootSpan = web.root(req) // Should block only in express if (rootSpan?.context()._name === 'express.request') { - const abortError = new AbortError(req, res, blockingAction) + const abortError = new DatadogRaspAbortError(req, res, blockingAction) abortController.abort(abortError) // TODO Delete this when support for node 16 is removed diff --git a/packages/dd-trace/test/appsec/rasp.express.plugin.spec.js b/packages/dd-trace/test/appsec/rasp.express.plugin.spec.js index 9a06fe1b736..b16f2701e95 100644 --- a/packages/dd-trace/test/appsec/rasp.express.plugin.spec.js +++ b/packages/dd-trace/test/appsec/rasp.express.plugin.spec.js @@ -103,7 +103,7 @@ describe('RASP', () => { app = (req, res) => { const clientRequest = require(protocol).get(`${protocol}://${req.query.host}`) clientRequest.on('error', (e) => { - if (e.message === 'AbortError') { + if (e.message === 'DatadogRaspAbortError') { res.writeHead(500) } res.end('end') @@ -120,7 +120,7 @@ describe('RASP', () => { clientRequest.write('dummy_post_data') clientRequest.end() clientRequest.on('error', (e) => { - if (e.message === 'AbortError') { + if (e.message === 'DatadogRaspAbortError') { res.writeHead(500) } res.end('end') @@ -160,7 +160,7 @@ describe('RASP', () => { await axiosToTest.get(`https://${req.query.host}`) res.end('end') } catch (e) { - if (e.cause.message === 'AbortError') { + if (e.cause.message === 'DatadogRaspAbortError') { res.writeHead(500) } res.end('end') @@ -175,7 +175,7 @@ describe('RASP', () => { try { await axiosToTest.post(`https://${req.query.host}`, { key: 'value' }) } catch (e) { - if (e.cause.message === 'AbortError') { + if (e.cause.message === 'DatadogRaspAbortError') { res.writeHead(500) } res.end('end') @@ -241,7 +241,7 @@ describe('RASP', () => { }) clientRequest.on('error', (e) => { - if (e.name !== 'AbortError') { + if (e.name !== 'DatadogRaspAbortError') { res.writeHead(200) res.end('not-blocking-error') } else { From 6487c9b6b1a580d52cd6021999fe91126e9d63f0 Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Wed, 3 Jul 2024 17:49:30 +0200 Subject: [PATCH 35/41] Suggestion from PR --- packages/datadog-instrumentations/test/http.spec.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/datadog-instrumentations/test/http.spec.js b/packages/datadog-instrumentations/test/http.spec.js index 44a14d32602..ec4e989876f 100644 --- a/packages/datadog-instrumentations/test/http.spec.js +++ b/packages/datadog-instrumentations/test/http.spec.js @@ -44,13 +44,12 @@ describe('client', () => { * and the same stub could be called multiple times */ function getContextFromStubByUrl (url, stub) { - for (let i = 0; i < stub.args.length; i++) { - const arg = stub.args[i][0] + for (const args of stub.args) { + const arg = args[0] if (arg.args?.originalUrl === url) { return arg } } - return null } From c4385bc2a53bc639a84d3e4a8935277cfd75b91e Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Fri, 12 Jul 2024 13:23:33 +0200 Subject: [PATCH 36/41] Update test integration appsec timeout --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 923721e41fe..ec08e282c4b 100644 --- a/package.json +++ b/package.json @@ -33,7 +33,7 @@ "test:profiler": "tap \"packages/dd-trace/test/profiling/**/*.spec.js\"", "test:profiler:ci": "npm run test:profiler -- --coverage --nyc-arg=--include=\"packages/dd-trace/src/profiling/**/*.js\"", "test:integration": "mocha --timeout 60000 -r \"packages/dd-trace/test/setup/core.js\" \"integration-tests/*.spec.js\"", - "test:integration:appsec": "mocha --timeout 30000 -r \"packages/dd-trace/test/setup/core.js\" \"integration-tests/appsec/*.spec.js\"", + "test:integration:appsec": "mocha --timeout 60000 -r \"packages/dd-trace/test/setup/core.js\" \"integration-tests/appsec/*.spec.js\"", "test:integration:cucumber": "mocha --timeout 60000 -r \"packages/dd-trace/test/setup/core.js\" \"integration-tests/cucumber/*.spec.js\"", "test:integration:cypress": "mocha --timeout 60000 -r \"packages/dd-trace/test/setup/core.js\" \"integration-tests/cypress/*.spec.js\"", "test:integration:jest": "mocha --timeout 60000 -r \"packages/dd-trace/test/setup/core.js\" \"integration-tests/jest/*.spec.js\"", From 887064268fdbc5dc1151758adba3e6f9a736f643 Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Fri, 19 Jul 2024 10:47:31 +0200 Subject: [PATCH 37/41] Improvements from PR comments --- packages/dd-trace/src/appsec/rasp.js | 97 +++++++++++++--------- packages/dd-trace/test/appsec/rasp.spec.js | 10 +++ 2 files changed, 67 insertions(+), 40 deletions(-) diff --git a/packages/dd-trace/src/appsec/rasp.js b/packages/dd-trace/src/appsec/rasp.js index d1a95a7b334..e38fdcd40c8 100644 --- a/packages/dd-trace/src/appsec/rasp.js +++ b/packages/dd-trace/src/appsec/rasp.js @@ -7,6 +7,11 @@ const { httpClientRequestStart, setUncaughtExceptionCaptureCallbackStart } = req const { reportStackTrace } = require('./stack_trace') const waf = require('./waf') const { getBlockingAction, block } = require('./blocking') +const log = require('../log') + +const RULE_TYPES = { + SSRF: 'ssrf' +} class DatadogRaspAbortError extends Error { constructor (req, res, blockingAction) { @@ -18,10 +23,6 @@ class DatadogRaspAbortError extends Error { } } -const RULE_TYPES = { - SSRF: 'ssrf' -} - let config, abortOnUncaughtException function removeAllListeners (emitter, event) { @@ -41,48 +42,59 @@ function removeAllListeners (emitter, event) { } } +function findDatadogRaspAbortError (err, deep = 10) { + if (err instanceof DatadogRaspAbortError) { + return err + } + + if (err.cause && deep > 0) { + return findDatadogRaspAbortError(err.cause, deep - 1) + } +} + function handleUncaughtExceptionMonitor (err) { - if (err instanceof DatadogRaspAbortError || err.cause instanceof DatadogRaspAbortError) { - const { req, res, blockingAction } = err - block(req, res, web.root(req), null, blockingAction) - - if (!process.hasUncaughtExceptionCaptureCallback()) { - const cleanUp = removeAllListeners(process, 'uncaughtException') - const handler = () => { - process.removeListener('uncaughtException', handler) - } + const abortError = findDatadogRaspAbortError(err) + if (!abortError) return - setTimeout(() => { - process.removeListener('uncaughtException', handler) - cleanUp() - }) + const { req, res, blockingAction } = abortError + block(req, res, web.root(req), null, blockingAction) - process.on('uncaughtException', handler) - } else { - // uncaughtException event is not executed when hasUncaughtExceptionCaptureCallback is true - let previousCb - const cb = ({ currentCallback, abortController }) => { - setUncaughtExceptionCaptureCallbackStart.unsubscribe(cb) - if (!currentCallback) { - abortController.abort() - return - } - - previousCb = currentCallback + if (!process.hasUncaughtExceptionCaptureCallback()) { + const cleanUp = removeAllListeners(process, 'uncaughtException') + const handler = () => { + process.removeListener('uncaughtException', handler) + } + + setTimeout(() => { + process.removeListener('uncaughtException', handler) + cleanUp() + }) + + process.on('uncaughtException', handler) + } else { + // uncaughtException event is not executed when hasUncaughtExceptionCaptureCallback is true + let previousCb + const cb = ({ currentCallback, abortController }) => { + setUncaughtExceptionCaptureCallbackStart.unsubscribe(cb) + if (!currentCallback) { + abortController.abort() + return } - setUncaughtExceptionCaptureCallbackStart.subscribe(cb) + previousCb = currentCallback + } - process.setUncaughtExceptionCaptureCallback(null) + setUncaughtExceptionCaptureCallbackStart.subscribe(cb) - // For some reason, previous callback was defined before the instrumentation - // We can not restore it, so we let the app decide - if (previousCb) { - process.setUncaughtExceptionCaptureCallback(() => { - process.setUncaughtExceptionCaptureCallback(null) - process.setUncaughtExceptionCaptureCallback(previousCb) - }) - } + process.setUncaughtExceptionCaptureCallback(null) + + // For some reason, previous callback was defined before the instrumentation + // We can not restore it, so we let the app decide + if (previousCb) { + process.setUncaughtExceptionCaptureCallback(() => { + process.setUncaughtExceptionCaptureCallback(null) + process.setUncaughtExceptionCaptureCallback(previousCb) + }) } } } @@ -93,6 +105,10 @@ function enable (_config) { process.on('uncaughtExceptionMonitor', handleUncaughtExceptionMonitor) abortOnUncaughtException = process.execArgv?.includes('--abort-on-uncaught-exception') + + if (abortOnUncaughtException) { + log.warn('The --abort-on-uncaught-exception flag is enabled. The RASP module will not block operations.') + } } function disable () { @@ -153,5 +169,6 @@ function handleResult (actions, req, res, abortController) { module.exports = { enable, disable, - handleResult + handleResult, + handleUncaughtExceptionMonitor // exported only for testing purpose } diff --git a/packages/dd-trace/test/appsec/rasp.spec.js b/packages/dd-trace/test/appsec/rasp.spec.js index f4ccc7aaf30..c594a2a98c2 100644 --- a/packages/dd-trace/test/appsec/rasp.spec.js +++ b/packages/dd-trace/test/appsec/rasp.spec.js @@ -3,6 +3,7 @@ const proxyquire = require('proxyquire') const { httpClientRequestStart } = require('../../src/appsec/channels') const addresses = require('../../src/appsec/addresses') +const { handleUncaughtExceptionMonitor } = require('../../src/appsec/rasp') describe('RASP', () => { let waf, rasp, datadogCore, stackTrace, web @@ -167,4 +168,13 @@ describe('RASP', () => { sinon.assert.notCalled(waf.run) }) }) + + describe('handleUncaughtExceptionMonitor', () => { + it('should not break with infinite loop of cause', () => { + const err = new Error() + err.cause = err + + handleUncaughtExceptionMonitor(err) + }) + }) }) From 231f4ee710290bec7ab934b206963955b42273e5 Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Fri, 19 Jul 2024 10:51:11 +0200 Subject: [PATCH 38/41] Small change proposed in the PR --- packages/dd-trace/src/appsec/rasp.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/dd-trace/src/appsec/rasp.js b/packages/dd-trace/src/appsec/rasp.js index e38fdcd40c8..77a98471ff5 100644 --- a/packages/dd-trace/src/appsec/rasp.js +++ b/packages/dd-trace/src/appsec/rasp.js @@ -150,8 +150,10 @@ function handleResult (actions, req, res, abortController) { ) } + if (abortOnUncaughtException) return + const blockingAction = getBlockingAction(actions) - if (blockingAction && abortController && !abortOnUncaughtException) { + if (blockingAction && abortController) { const rootSpan = web.root(req) // Should block only in express if (rootSpan?.context()._name === 'express.request') { From afc49048963266c7f1d231e43d098774baf1038b Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Fri, 19 Jul 2024 12:09:39 +0200 Subject: [PATCH 39/41] Fix lint --- integration-tests/appsec/rasp/index.js | 1 + packages/dd-trace/test/appsec/rasp.express.plugin.spec.js | 1 + 2 files changed, 2 insertions(+) diff --git a/integration-tests/appsec/rasp/index.js b/integration-tests/appsec/rasp/index.js index f6488892c8e..a2035c6c3b4 100644 --- a/integration-tests/appsec/rasp/index.js +++ b/integration-tests/appsec/rasp/index.js @@ -107,6 +107,7 @@ app.get('/ssrf/http/custom-uncaught-exception-capture-callback', (req, res) => { }) app.get('/ssrf/http/should-block-in-domain', (req, res) => { + // eslint-disable-next-line n/no-deprecated-api const d = require('node:domain').create() d.run(() => { http.get(`https://${req.query.host}`, () => { diff --git a/packages/dd-trace/test/appsec/rasp.express.plugin.spec.js b/packages/dd-trace/test/appsec/rasp.express.plugin.spec.js index b16f2701e95..75924c88283 100644 --- a/packages/dd-trace/test/appsec/rasp.express.plugin.spec.js +++ b/packages/dd-trace/test/appsec/rasp.express.plugin.spec.js @@ -191,6 +191,7 @@ describe('RASP', () => { describe('without express', () => { let app, server, axios + before(() => { return agent.load(['http'], { client: false }) }) From fb6bf3f7d8f130adc0a3871cef32740e586a8706 Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Mon, 22 Jul 2024 09:36:07 +0200 Subject: [PATCH 40/41] Small fix --- packages/dd-trace/src/appsec/rasp.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/dd-trace/src/appsec/rasp.js b/packages/dd-trace/src/appsec/rasp.js index 77a98471ff5..de13c33e4e9 100644 --- a/packages/dd-trace/src/appsec/rasp.js +++ b/packages/dd-trace/src/appsec/rasp.js @@ -150,10 +150,10 @@ function handleResult (actions, req, res, abortController) { ) } - if (abortOnUncaughtException) return + if (!abortController || abortOnUncaughtException) return const blockingAction = getBlockingAction(actions) - if (blockingAction && abortController) { + if (blockingAction) { const rootSpan = web.root(req) // Should block only in express if (rootSpan?.context()._name === 'express.request') { From 15f1d337e5cdada9c95496ea2040d755b7c2de4f Mon Sep 17 00:00:00 2001 From: Ugaitz Urien Date: Mon, 22 Jul 2024 15:00:38 +0200 Subject: [PATCH 41/41] Fix typo Co-authored-by: simon-id --- packages/datadog-instrumentations/src/http/server.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/datadog-instrumentations/src/http/server.js b/packages/datadog-instrumentations/src/http/server.js index 042d23cb9dc..0624c886787 100644 --- a/packages/datadog-instrumentations/src/http/server.js +++ b/packages/datadog-instrumentations/src/http/server.js @@ -180,7 +180,7 @@ function wrapSetHeader (setHeader) { } function wrapAppendOrRemoveHeader (originalMethod) { - return function wrappedAppendOrRemoveHeade () { + return function wrappedAppendOrRemoveHeader () { if (!startSetHeaderCh.hasSubscribers) { return originalMethod.apply(this, arguments) }