diff --git a/.github/workflows/plugins.yml b/.github/workflows/plugins.yml index 423948aa557..a2e3d02c50b 100644 --- a/.github/workflows/plugins.yml +++ b/.github/workflows/plugins.yml @@ -1245,6 +1245,23 @@ jobs: uses: ./.github/actions/testagent/logs - uses: codecov/codecov-action@v3 + undici: + runs-on: ubuntu-latest + env: + PLUGINS: undici + steps: + - uses: actions/checkout@v4 + - uses: ./.github/actions/testagent/start + - uses: ./.github/actions/node/setup + - run: yarn install + - uses: ./.github/actions/node/oldest + - run: yarn test:plugins:ci + - uses: ./.github/actions/node/latest + - run: yarn test:plugins:ci + - if: always() + uses: ./.github/actions/testagent/logs + - uses: codecov/codecov-action@v3 + when: runs-on: ubuntu-latest env: diff --git a/CODEOWNERS b/CODEOWNERS index 29d5693b54f..fc3236e1320 100644 --- a/CODEOWNERS +++ b/CODEOWNERS @@ -2,8 +2,8 @@ /packages/dd-trace/src/appsec/ @DataDog/asm-js /packages/dd-trace/test/appsec/ @DataDog/asm-js -/packages/dd-trace/src/lambda/ @DataDog/serverless -/packages/dd-trace/test/lambda/ @DataDog/serverless +/packages/dd-trace/src/lambda/ @DataDog/serverless-aws +/packages/dd-trace/test/lambda/ @DataDog/serverless-aws /packages/datadog-plugin-*/ @Datadog/dd-trace-js @Datadog/apm-idm-js /packages/datadog-instrumentations/ @Datadog/dd-trace-js @Datadog/apm-idm-js @@ -42,7 +42,7 @@ /.github/workflows/ci-visibility-performance.yml @DataDog/ci-app-libraries /.github/workflows/codeql-analysis.yml @DataDog/dd-trace-js /.github/workflows/core.yml @DataDog/dd-trace-js -/.github/workflows/lambda.yml @DataDog/serverless-apm +/.github/workflows/lambda.yml @DataDog/serverless-aws /.github/workflows/package-size.yml @DataDog/dd-trace-js /.github/workflows/plugins.yml @DataDog/dd-trace-js /.github/workflows/pr-labels.yml @DataDog/dd-trace-js @@ -52,8 +52,7 @@ /.github/workflows/release-dev.yml @DataDog/dd-trace-js /.github/workflows/release-latest.yml @DataDog/dd-trace-js /.github/workflows/release-proposal.yml @DataDog/dd-trace-js -/.github/workflows/serverless-integration-test.yml @DataDog/serverless -/.github/workflows/serverless-performance.yml @DataDog/serverless-apm @DataDog/serverless +/.github/workflows/serverless-integration-test.yml @DataDog/serverless-aws @DataDog/serverless /.github/workflows/system-tests.yml @DataDog/asm-js /.github/workflows/test-k8s-lib-injection.yaml @DataDog/dd-trace-js /.github/workflows/tracing.yml @DataDog/dd-trace-js diff --git a/docs/API.md b/docs/API.md index a43507f9437..68cdc3747cb 100644 --- a/docs/API.md +++ b/docs/API.md @@ -94,6 +94,7 @@ tracer.use('pg', {
+

Available Plugins

@@ -146,6 +147,7 @@ tracer.use('pg', { * [restify](./interfaces/export_.plugins.restify.html) * [router](./interfaces/export_.plugins.router.html) * [tedious](./interfaces/export_.plugins.tedious.html) +* [undici](./interfaces/export_.plugins.undici.html) * [when](./interfaces/export_.plugins.when.html) * [winston](./interfaces/export_.plugins.winston.html) diff --git a/docs/add-redirects.sh b/docs/add-redirects.sh index b738562979c..fd0590a934a 100755 --- a/docs/add-redirects.sh +++ b/docs/add-redirects.sh @@ -60,6 +60,7 @@ declare -a plugins=( "restify" "router" "tedious" + "undici" "when" "winston" ) diff --git a/docs/test.ts b/docs/test.ts index 91fafd48734..7734dad4098 100644 --- a/docs/test.ts +++ b/docs/test.ts @@ -352,6 +352,7 @@ tracer.use('selenium'); tracer.use('sharedb'); tracer.use('sharedb', sharedbOptions); tracer.use('tedious'); +tracer.use('undici'); tracer.use('winston'); tracer.use('express', false) diff --git a/index.d.ts b/index.d.ts index 51d87993ab4..4184a015fda 100644 --- a/index.d.ts +++ b/index.d.ts @@ -197,6 +197,7 @@ interface Plugins { "selenium": tracer.plugins.selenium; "sharedb": tracer.plugins.sharedb; "tedious": tracer.plugins.tedious; + "undici": tracer.plugins.undici; "winston": tracer.plugins.winston; } @@ -1800,6 +1801,12 @@ declare namespace tracer { */ interface tedious extends Instrumentation {} + /** + * This plugin automatically instruments the + * [undici](https://github.com/nodejs/undici) module. + */ + interface undici extends HttpClient {} + /** * This plugin patches the [winston](https://github.com/winstonjs/winston) * to automatically inject trace identifiers in log records when the diff --git a/packages/datadog-instrumentations/src/helpers/hooks.js b/packages/datadog-instrumentations/src/helpers/hooks.js index 34654182ddd..0723ceabd84 100644 --- a/packages/datadog-instrumentations/src/helpers/hooks.js +++ b/packages/datadog-instrumentations/src/helpers/hooks.js @@ -109,6 +109,7 @@ module.exports = { sequelize: () => require('../sequelize'), sharedb: () => require('../sharedb'), tedious: () => require('../tedious'), + undici: () => require('../undici'), when: () => require('../when'), winston: () => require('../winston') } diff --git a/packages/datadog-instrumentations/src/http/server.js b/packages/datadog-instrumentations/src/http/server.js index 14bccd88994..18a00ac2789 100644 --- a/packages/datadog-instrumentations/src/http/server.js +++ b/packages/datadog-instrumentations/src/http/server.js @@ -10,6 +10,7 @@ const startServerCh = channel('apm:http:server:request:start') const exitServerCh = channel('apm:http:server:request:exit') 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 requestFinishedSet = new WeakSet() @@ -20,6 +21,9 @@ const httpsNames = ['https', 'node:https'] addHook({ name: httpNames }, http => { shimmer.wrap(http.ServerResponse.prototype, 'emit', wrapResponseEmit) shimmer.wrap(http.Server.prototype, 'emit', wrapEmit) + shimmer.wrap(http.ServerResponse.prototype, 'writeHead', wrapWriteHead) + shimmer.wrap(http.ServerResponse.prototype, 'write', wrapWrite) + shimmer.wrap(http.ServerResponse.prototype, 'end', wrapEnd) return http }) @@ -86,3 +90,97 @@ function wrapSetHeader (res) { } }) } + +function wrapWriteHead (writeHead) { + return function wrappedWriteHead (statusCode, reason, obj) { + if (!startWriteHeadCh.hasSubscribers) { + return writeHead.apply(this, arguments) + } + + const abortController = new AbortController() + + if (typeof reason !== 'string') { + obj ??= reason + } + + // support writeHead(200, ['key1', 'val1', 'key2', 'val2']) + if (Array.isArray(obj)) { + const headers = {} + + for (let i = 0; i < obj.length; i += 2) { + headers[obj[i]] = obj[i + 1] + } + + obj = headers + } + + // this doesn't support explicit duplicate headers, but it's an edge case + const responseHeaders = Object.assign(this.getHeaders(), obj) + + startWriteHeadCh.publish({ + req: this.req, + res: this, + abortController, + statusCode, + responseHeaders + }) + + if (abortController.signal.aborted) { + return this + } + + return writeHead.apply(this, arguments) + } +} + +function wrapWrite (write) { + return function wrappedWrite () { + if (!startWriteHeadCh.hasSubscribers) { + return write.apply(this, arguments) + } + + const abortController = new AbortController() + + const responseHeaders = this.getHeaders() + + startWriteHeadCh.publish({ + req: this.req, + res: this, + abortController, + statusCode: this.statusCode, + responseHeaders + }) + + if (abortController.signal.aborted) { + return true + } + + return write.apply(this, arguments) + } +} + +function wrapEnd (end) { + return function wrappedEnd () { + if (!startWriteHeadCh.hasSubscribers) { + return end.apply(this, arguments) + } + + const abortController = new AbortController() + + const responseHeaders = this.getHeaders() + + startWriteHeadCh.publish({ + req: this.req, + res: this, + abortController, + statusCode: this.statusCode, + responseHeaders + }) + + if (abortController.signal.aborted) { + return this + } + + return end.apply(this, arguments) + } +} diff --git a/packages/datadog-instrumentations/src/undici.js b/packages/datadog-instrumentations/src/undici.js new file mode 100644 index 00000000000..cd3207ea9c3 --- /dev/null +++ b/packages/datadog-instrumentations/src/undici.js @@ -0,0 +1,18 @@ +'use strict' + +const { + addHook +} = require('./helpers/instrument') +const shimmer = require('../../datadog-shimmer') + +const tracingChannel = require('dc-polyfill').tracingChannel +const ch = tracingChannel('apm:undici:fetch') + +const { createWrapFetch } = require('./helpers/fetch') + +addHook({ + name: 'undici', + versions: ['^4.4.1', '5', '>=6.0.0'] +}, undici => { + return shimmer.wrap(undici, 'fetch', createWrapFetch(undici.Request, ch)) +}) diff --git a/packages/datadog-plugin-undici/src/index.js b/packages/datadog-plugin-undici/src/index.js new file mode 100644 index 00000000000..c436aceb882 --- /dev/null +++ b/packages/datadog-plugin-undici/src/index.js @@ -0,0 +1,12 @@ +'use strict' + +const FetchPlugin = require('../../datadog-plugin-fetch/src/index.js') + +class UndiciPlugin extends FetchPlugin { + static get id () { return 'undici' } + static get prefix () { + return 'tracing:apm:undici:fetch' + } +} + +module.exports = UndiciPlugin diff --git a/packages/datadog-plugin-undici/test/index.spec.js b/packages/datadog-plugin-undici/test/index.spec.js new file mode 100644 index 00000000000..734e8f6c9a9 --- /dev/null +++ b/packages/datadog-plugin-undici/test/index.spec.js @@ -0,0 +1,525 @@ +'use strict' + +const getPort = require('get-port') +const agent = require('../../dd-trace/test/plugins/agent') +const tags = require('../../../ext/tags') +const { expect } = require('chai') +const { rawExpectedSchema } = require('./naming') +const { DD_MAJOR } = require('../../../version') +const { ERROR_MESSAGE, ERROR_TYPE, ERROR_STACK } = require('../../dd-trace/src/constants') + +const HTTP_REQUEST_HEADERS = tags.HTTP_REQUEST_HEADERS +const HTTP_RESPONSE_HEADERS = tags.HTTP_RESPONSE_HEADERS + +const SERVICE_NAME = DD_MAJOR < 3 ? 'test-http-client' : 'test' + +describe('Plugin', () => { + let express + let fetch + let appListener + + describe('undici-fetch', () => { + withVersions('undici', 'undici', version => { + function server (app, port, listener) { + const server = require('http').createServer(app) + server.listen(port, 'localhost', listener) + return server + } + + beforeEach(() => { + appListener = null + }) + + afterEach(() => { + if (appListener) { + appListener.close() + } + return agent.close({ ritmReset: false }) + }) + + describe('without configuration', () => { + beforeEach(() => { + return agent.load('undici', { + service: 'test' + }) + .then(() => { + express = require('express') + fetch = require(`../../../versions/undici@${version}`, {}).get() + }) + }) + + afterEach(() => { + express = null + }) + + withNamingSchema( + () => { + const app = express() + app.get('/user', (req, res) => { + res.status(200).send() + }) + + getPort().then(port => { + appListener = server(app, port, () => { + fetch.fetch(`http://localhost:${port}/user`, { method: 'GET' }) + }) + }) + }, + rawExpectedSchema.client + ) + + it('should do automatic instrumentation', function (done) { + const app = express() + app.get('/user', (req, res) => { + res.status(200).send() + }) + getPort().then(port => { + agent + .use(traces => { + expect(traces[0][0]).to.have.property('service', 'test') + expect(traces[0][0]).to.have.property('type', 'http') + expect(traces[0][0]).to.have.property('resource', 'GET') + expect(traces[0][0].meta).to.have.property('span.kind', 'client') + expect(traces[0][0].meta).to.have.property('http.url', `http://localhost:${port}/user`) + expect(traces[0][0].meta).to.have.property('http.method', 'GET') + expect(traces[0][0].meta).to.have.property('http.status_code', '200') + expect(traces[0][0].meta).to.have.property('component', 'undici') + expect(traces[0][0].meta).to.have.property('out.host', 'localhost') + }) + .then(done) + .catch(done) + + appListener = server(app, port, () => { + fetch.fetch(`http://localhost:${port}/user`, { method: 'GET' }) + }) + }) + }) + + it('should support URL input', done => { + const app = express() + app.post('/user', (req, res) => { + res.status(200).send() + }) + getPort().then(port => { + agent + .use(traces => { + expect(traces[0][0]).to.have.property('service', SERVICE_NAME) + expect(traces[0][0]).to.have.property('type', 'http') + expect(traces[0][0]).to.have.property('resource', 'POST') + expect(traces[0][0].meta).to.have.property('span.kind', 'client') + expect(traces[0][0].meta).to.have.property('http.url', `http://localhost:${port}/user`) + expect(traces[0][0].meta).to.have.property('http.method', 'POST') + expect(traces[0][0].meta).to.have.property('http.status_code', '200') + expect(traces[0][0].meta).to.have.property('component', 'undici') + expect(traces[0][0].meta).to.have.property('out.host', 'localhost') + }) + .then(done) + .catch(done) + + appListener = server(app, port, () => { + fetch.fetch(new URL(`http://localhost:${port}/user`), { method: 'POST' }) + }) + }) + }) + + it('should return the response', done => { + const app = express() + app.get('/user', (req, res) => { + res.status(200).send() + }) + getPort().then(port => { + appListener = server(app, port, () => { + fetch.fetch((`http://localhost:${port}/user`)) + .then(res => { + expect(res).to.have.property('status', 200) + done() + }) + .catch(done) + }) + }) + }) + + it('should remove the query string from the URL', done => { + const app = express() + + app.get('/user', (req, res) => { + res.status(200).send() + }) + + getPort().then(port => { + agent + .use(traces => { + expect(traces[0][0].meta).to.have.property('http.status_code', '200') + expect(traces[0][0].meta).to.have.property('http.url', `http://localhost:${port}/user`) + }) + .then(done) + .catch(done) + + appListener = server(app, port, () => { + fetch.fetch(`http://localhost:${port}/user?foo=bar`) + }) + }) + }) + + it('should inject its parent span in the headers', done => { + const app = express() + + app.get('/user', (req, res) => { + expect(req.get('x-datadog-trace-id')).to.be.a('string') + expect(req.get('x-datadog-parent-id')).to.be.a('string') + + res.status(200).send() + }) + + getPort().then(port => { + agent + .use(traces => { + expect(traces[0][0].meta).to.have.property('http.status_code', '200') + }) + .then(done) + .catch(done) + + appListener = server(app, port, () => { + fetch.fetch(`http://localhost:${port}/user?foo=bar`) + }) + }) + }) + + it('should inject its parent span in the existing headers', done => { + const app = express() + + app.get('/user', (req, res) => { + expect(req.get('foo')).to.be.a('string') + expect(req.get('x-datadog-trace-id')).to.be.a('string') + expect(req.get('x-datadog-parent-id')).to.be.a('string') + + res.status(200).send() + }) + + getPort().then(port => { + agent + .use(traces => { + expect(traces[0][0].meta).to.have.property('http.status_code', '200') + }) + .then(done) + .catch(done) + + appListener = server(app, port, () => { + fetch.fetch(`http://localhost:${port}/user?foo=bar`, { headers: { foo: 'bar' } }) + }) + }) + }) + it('should handle connection errors', done => { + getPort().then(port => { + let error + + agent + .use(traces => { + expect(traces[0][0].meta).to.have.property(ERROR_TYPE, error.name) + expect(traces[0][0].meta).to.have.property(ERROR_MESSAGE, error.message || error.code) + expect(traces[0][0].meta).to.have.property(ERROR_STACK, error.stack) + expect(traces[0][0].meta).to.have.property('component', 'undici') + }) + .then(done) + .catch(done) + + fetch.fetch(`http://localhost:${port}/user`).catch(err => { + error = err + }) + }) + }) + it('should not record HTTP 5XX responses as errors by default', done => { + const app = express() + + app.get('/user', (req, res) => { + res.status(500).send() + }) + + getPort().then(port => { + agent + .use(traces => { + expect(traces[0][0]).to.have.property('error', 0) + }) + .then(done) + .catch(done) + + appListener = server(app, port, () => { + fetch.fetch(`http://localhost:${port}/user`) + }) + }) + }) + + it('should record HTTP 4XX responses as errors by default', done => { + const app = express() + + app.get('/user', (req, res) => { + res.status(400).send() + }) + + getPort().then(port => { + agent + .use(traces => { + expect(traces[0][0]).to.have.property('error', 1) + }) + .then(done) + .catch(done) + + appListener = server(app, port, () => { + fetch.fetch(`http://localhost:${port}/user`) + }) + }) + }) + + it('should not record aborted requests as errors', done => { + const app = express() + + app.get('/user', (req, res) => {}) + + getPort().then(port => { + agent + .use(traces => { + expect(traces[0][0]).to.have.property('error', 0) + expect(traces[0][0].meta).to.not.have.property('http.status_code') + }) + .then(done) + .catch(done) + + appListener = server(app, port, () => { + const controller = new AbortController() + + fetch.fetch(`http://localhost:${port}/user`, { + signal: controller.signal + }).catch(() => {}) + + controller.abort() + }) + }) + }) + + it('should record when the request was aborted', done => { + const app = express() + + app.get('/abort', (req, res) => { + res.status(200).send() + }) + + getPort().then(port => { + agent + .use(traces => { + expect(traces[0][0]).to.have.property('service', SERVICE_NAME) + }) + .then(done) + .catch(done) + + appListener = server(app, port, () => { + const controller = new AbortController() + + fetch.fetch(`http://localhost:${port}/user`, { + signal: controller.signal + }).catch(() => {}) + + controller.abort() + }) + }) + }) + }) + describe('with service configuration', () => { + let config + + beforeEach(() => { + config = { + service: 'custom' + } + + return agent.load('undici', config) + .then(() => { + express = require('express') + fetch = require(`../../../versions/undici@${version}`, {}).get() + }) + }) + + it('should be configured with the correct values', done => { + const app = express() + + app.get('/user', (req, res) => { + res.status(200).send() + }) + + getPort().then(port => { + agent + .use(traces => { + expect(traces[0][0]).to.have.property('service', 'custom') + }) + .then(done) + .catch(done) + + appListener = server(app, port, () => { + fetch.fetch(`http://localhost:${port}/user`).catch(() => {}) + }) + }) + }) + }) + describe('with headers configuration', () => { + let config + + beforeEach(() => { + config = { + headers: ['x-baz', 'x-foo'] + } + + return agent.load('undici', config) + .then(() => { + express = require('express') + fetch = require(`../../../versions/undici@${version}`, {}).get() + }) + }) + + it('should add tags for the configured headers', done => { + const app = express() + + app.get('/user', (req, res) => { + res.setHeader('x-foo', 'bar') + res.status(200).send() + }) + + getPort().then(port => { + agent + .use(traces => { + const meta = traces[0][0].meta + expect(meta).to.have.property(`${HTTP_REQUEST_HEADERS}.x-baz`, 'qux') + expect(meta).to.have.property(`${HTTP_RESPONSE_HEADERS}.x-foo`, 'bar') + }) + .then(done) + .catch(done) + + appListener = server(app, port, () => { + fetch.fetch(`http://localhost:${port}/user`, { + headers: { + 'x-baz': 'qux' + } + }).catch(() => {}) + }) + }) + }) + }) + describe('with hooks configuration', () => { + let config + + beforeEach(() => { + config = { + hooks: { + request: (span, req, res) => { + span.setTag('foo', '/foo') + } + } + } + + return agent.load('undici', config) + .then(() => { + express = require('express') + fetch = require(`../../../versions/undici@${version}`, {}).get() + }) + }) + + it('should run the request hook before the span is finished', done => { + const app = express() + + app.get('/user', (req, res) => { + res.status(200).send() + }) + + getPort().then(port => { + agent + .use(traces => { + expect(traces[0][0].meta).to.have.property('foo', '/foo') + }) + .then(done) + .catch(done) + + appListener = server(app, port, () => { + fetch.fetch(`http://localhost:${port}/user`).catch(() => {}) + }) + }) + }) + }) + + describe('with propagationBlocklist configuration', () => { + let config + + beforeEach(() => { + config = { + propagationBlocklist: [/\/users/] + } + + return agent.load('undici', config) + .then(() => { + express = require('express') + fetch = require(`../../../versions/undici@${version}`, {}).get() + }) + }) + + it('should skip injecting if the url matches an item in the propagationBlacklist', done => { + const app = express() + + app.get('/users', (req, res) => { + try { + expect(req.get('x-datadog-trace-id')).to.be.undefined + expect(req.get('x-datadog-parent-id')).to.be.undefined + + res.status(200).send() + + done() + } catch (e) { + done(e) + } + }) + + getPort().then(port => { + appListener = server(app, port, () => { + fetch.fetch(`http://localhost:${port}/users`).catch(() => {}) + }) + }) + }) + }) + + describe('with blocklist configuration', () => { + let config + + beforeEach(() => { + config = { + blocklist: [/\/user/] + } + + return agent.load('undici', config) + .then(() => { + express = require('express') + fetch = require(`../../../versions/undici@${version}`, {}).get() + }) + }) + + it('should skip recording if the url matches an item in the blocklist', done => { + const app = express() + + app.get('/user', (req, res) => { + res.status(200).send() + }) + + getPort().then(port => { + const timer = setTimeout(done, 100) + + agent + .use(() => { + clearTimeout(timer) + done(new Error('Blocklisted requests should not be recorded.')) + }) + .catch(done) + + appListener = server(app, port, () => { + fetch.fetch(`http://localhost:${port}/users`).catch(() => {}) + }) + }) + }) + }) + }) + }) +}) diff --git a/packages/datadog-plugin-undici/test/naming.js b/packages/datadog-plugin-undici/test/naming.js new file mode 100644 index 00000000000..5bf2be387c3 --- /dev/null +++ b/packages/datadog-plugin-undici/test/naming.js @@ -0,0 +1,19 @@ +const { resolveNaming } = require('../../dd-trace/test/plugins/helpers') + +const rawExpectedSchema = { + client: { + v0: { + serviceName: 'test', + opName: 'undici.request' + }, + v1: { + serviceName: 'test', + opName: 'undici.request' + } + } +} + +module.exports = { + rawExpectedSchema, + expectedSchema: resolveNaming(rawExpectedSchema) +} diff --git a/packages/dd-trace/src/appsec/blocking.js b/packages/dd-trace/src/appsec/blocking.js index 141667b9d57..f3d2103b59e 100644 --- a/packages/dd-trace/src/appsec/blocking.js +++ b/packages/dd-trace/src/appsec/blocking.js @@ -111,6 +111,10 @@ function block (req, res, rootSpan, abortController, actionParameters) { const { body, headers, statusCode } = getBlockingData(req, null, rootSpan, actionParameters) + for (const headerName of res.getHeaderNames()) { + res.removeHeader(headerName) + } + res.writeHead(statusCode, headers).end(body) abortController?.abort() diff --git a/packages/dd-trace/src/appsec/channels.js b/packages/dd-trace/src/appsec/channels.js index 57a3c29676c..82e7eb5f6f7 100644 --- a/packages/dd-trace/src/appsec/channels.js +++ b/packages/dd-trace/src/appsec/channels.js @@ -18,5 +18,6 @@ module.exports = { nextBodyParsed: dc.channel('apm:next:body-parsed'), 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') } diff --git a/packages/dd-trace/src/appsec/index.js b/packages/dd-trace/src/appsec/index.js index 76e67a0ef72..af0ffc934f3 100644 --- a/packages/dd-trace/src/appsec/index.js +++ b/packages/dd-trace/src/appsec/index.js @@ -12,7 +12,8 @@ const { queryParser, nextBodyParsed, nextQueryParsed, - responseBody + responseBody, + responseWriteHead } = require('./channels') const waf = require('./waf') const addresses = require('./addresses') @@ -60,6 +61,7 @@ function enable (_config) { queryParser.subscribe(onRequestQueryParsed) cookieParser.subscribe(onRequestCookieParser) responseBody.subscribe(onResponseBody) + responseWriteHead.subscribe(onResponseWriteHead) if (_config.appsec.eventTracking.enabled) { passportVerify.subscribe(onPassportVerify) @@ -110,14 +112,7 @@ function incomingHttpStartTranslator ({ req, res, abortController }) { } function incomingHttpEndTranslator ({ req, res }) { - // TODO: this doesn't support headers sent with res.writeHead() - const responseHeaders = Object.assign({}, res.getHeaders()) - delete responseHeaders['set-cookie'] - - const persistent = { - [addresses.HTTP_INCOMING_RESPONSE_CODE]: '' + res.statusCode, - [addresses.HTTP_INCOMING_RESPONSE_HEADERS]: responseHeaders - } + const persistent = {} // we need to keep this to support other body parsers // TODO: no need to analyze it if it was already done by the body-parser hook @@ -139,7 +134,9 @@ function incomingHttpEndTranslator ({ req, res }) { persistent[addresses.HTTP_INCOMING_QUERY] = req.query } - waf.run({ persistent }, req) + if (Object.keys(persistent).length) { + waf.run({ persistent }, req) + } waf.disposeContext(req) @@ -225,12 +222,48 @@ function onPassportVerify ({ credentials, user }) { passportTrackEvent(credentials, user, rootSpan, config.appsec.eventTracking.mode) } +const responseAnalyzedSet = new WeakSet() +const responseBlockedSet = new WeakSet() + +function onResponseWriteHead ({ req, res, abortController, statusCode, responseHeaders }) { + // avoid "write after end" error + if (responseBlockedSet.has(res)) { + abortController?.abort() + return + } + + // avoid double waf call + if (responseAnalyzedSet.has(res)) { + return + } + + const rootSpan = web.root(req) + if (!rootSpan) return + + responseHeaders = Object.assign({}, responseHeaders) + delete responseHeaders['set-cookie'] + + const results = waf.run({ + persistent: { + [addresses.HTTP_INCOMING_RESPONSE_CODE]: '' + statusCode, + [addresses.HTTP_INCOMING_RESPONSE_HEADERS]: responseHeaders + } + }, req) + + responseAnalyzedSet.add(res) + + handleResults(results, req, res, rootSpan, abortController) +} + function handleResults (actions, req, res, rootSpan, abortController) { if (!actions || !req || !res || !rootSpan || !abortController) return const blockingAction = getBlockingAction(actions) if (blockingAction) { block(req, res, rootSpan, abortController, blockingAction) + if (!abortController.signal || abortController.signal.aborted) { + responseBlockedSet.add(res) + } } } @@ -256,6 +289,7 @@ function disable () { if (cookieParser.hasSubscribers) cookieParser.unsubscribe(onRequestCookieParser) if (responseBody.hasSubscribers) responseBody.unsubscribe(onResponseBody) if (passportVerify.hasSubscribers) passportVerify.unsubscribe(onPassportVerify) + if (responseWriteHead.hasSubscribers) responseWriteHead.unsubscribe(onResponseWriteHead) } module.exports = { diff --git a/packages/dd-trace/src/appsec/remote_config/capabilities.js b/packages/dd-trace/src/appsec/remote_config/capabilities.js index 61684e171f0..6e320493336 100644 --- a/packages/dd-trace/src/appsec/remote_config/capabilities.js +++ b/packages/dd-trace/src/appsec/remote_config/capabilities.js @@ -6,6 +6,7 @@ module.exports = { ASM_DD_RULES: 1n << 3n, ASM_EXCLUSIONS: 1n << 4n, ASM_REQUEST_BLOCKING: 1n << 5n, + ASM_RESPONSE_BLOCKING: 1n << 6n, ASM_USER_BLOCKING: 1n << 7n, ASM_CUSTOM_RULES: 1n << 8n, ASM_CUSTOM_BLOCKING_RESPONSE: 1n << 9n, diff --git a/packages/dd-trace/src/appsec/remote_config/index.js b/packages/dd-trace/src/appsec/remote_config/index.js index f39d02347eb..169e5c2dff7 100644 --- a/packages/dd-trace/src/appsec/remote_config/index.js +++ b/packages/dd-trace/src/appsec/remote_config/index.js @@ -71,6 +71,7 @@ function enableWafUpdate (appsecConfig) { rc.updateCapabilities(RemoteConfigCapabilities.ASM_DD_RULES, true) rc.updateCapabilities(RemoteConfigCapabilities.ASM_EXCLUSIONS, true) rc.updateCapabilities(RemoteConfigCapabilities.ASM_REQUEST_BLOCKING, true) + rc.updateCapabilities(RemoteConfigCapabilities.ASM_RESPONSE_BLOCKING, true) rc.updateCapabilities(RemoteConfigCapabilities.ASM_CUSTOM_RULES, true) rc.updateCapabilities(RemoteConfigCapabilities.ASM_CUSTOM_BLOCKING_RESPONSE, true) rc.updateCapabilities(RemoteConfigCapabilities.ASM_TRUSTED_IPS, true) @@ -92,6 +93,7 @@ function disableWafUpdate () { rc.updateCapabilities(RemoteConfigCapabilities.ASM_DD_RULES, false) rc.updateCapabilities(RemoteConfigCapabilities.ASM_EXCLUSIONS, false) rc.updateCapabilities(RemoteConfigCapabilities.ASM_REQUEST_BLOCKING, false) + rc.updateCapabilities(RemoteConfigCapabilities.ASM_RESPONSE_BLOCKING, false) rc.updateCapabilities(RemoteConfigCapabilities.ASM_CUSTOM_RULES, false) rc.updateCapabilities(RemoteConfigCapabilities.ASM_CUSTOM_BLOCKING_RESPONSE, false) rc.updateCapabilities(RemoteConfigCapabilities.ASM_TRUSTED_IPS, false) diff --git a/packages/dd-trace/src/format.js b/packages/dd-trace/src/format.js index 6d7c85ce039..fcb2a07d01d 100644 --- a/packages/dd-trace/src/format.js +++ b/packages/dd-trace/src/format.js @@ -34,6 +34,7 @@ function format (span) { const formatted = formatSpan(span) extractSpanLinks(formatted, span) + extractSpanEvents(formatted, span) extractRootTags(formatted, span) extractChunkTags(formatted, span) extractTags(formatted, span) @@ -88,6 +89,22 @@ function extractSpanLinks (trace, span) { if (links.length > 0) { trace.meta['_dd.span_links'] = JSON.stringify(links) } } +function extractSpanEvents (trace, span) { + const events = [] + if (span._events) { + for (const event of span._events) { + const formattedEvent = { + name: event.name, + time_unix_nano: Math.round(event.startTime * 1e6), + attributes: event.attributes && Object.keys(event.attributes).length > 0 ? event.attributes : undefined + } + + events.push(formattedEvent) + } + } + if (events.length > 0) { trace.meta.events = JSON.stringify(events) } +} + function extractTags (trace, span) { const context = span.context() const origin = context._trace.origin @@ -134,7 +151,10 @@ function extractTags (trace, span) { case ERROR_STACK: // HACK: remove when implemented in the backend if (context._name !== 'fs.operation') { - trace.error = 1 + // HACK: to ensure otel.recordException does not influence trace.error + if (tags.setTraceError) { + trace.error = 1 + } } else { break } @@ -142,7 +162,6 @@ function extractTags (trace, span) { addTag(trace.meta, trace.metrics, tag, tags[tag]) } } - setSingleSpanIngestionTags(trace, context._spanSampling) addTag(trace.meta, trace.metrics, 'language', 'javascript') diff --git a/packages/dd-trace/src/opentelemetry/span.js b/packages/dd-trace/src/opentelemetry/span.js index 5c1be3f1e60..5a8fb29c693 100644 --- a/packages/dd-trace/src/opentelemetry/span.js +++ b/packages/dd-trace/src/opentelemetry/span.js @@ -20,6 +20,20 @@ function hrTimeToMilliseconds (time) { return time[0] * 1e3 + time[1] / 1e6 } +function isTimeInput (startTime) { + if (typeof startTime === 'number') { + return true + } + if (startTime instanceof Date) { + return true + } + if (Array.isArray(startTime) && startTime.length === 2 && + typeof startTime[0] === 'number' && typeof startTime[1] === 'number') { + return true + } + return false +} + const spanKindNames = { [api.SpanKind.INTERNAL]: kinds.INTERNAL, [api.SpanKind.SERVER]: kinds.SERVER, @@ -196,11 +210,6 @@ class Span { return this } - addEvent (name, attributesOrStartTime, startTime) { - api.diag.warn('Events not supported') - return this - } - addLink (context, attributes) { // extract dd context const ddSpanContext = context._ddContext @@ -244,12 +253,29 @@ class Span { return this.ended === false } - recordException (exception) { + addEvent (name, attributesOrStartTime, startTime) { + startTime = attributesOrStartTime && isTimeInput(attributesOrStartTime) ? attributesOrStartTime : startTime + const hrStartTime = timeInputToHrTime(startTime || (performance.now() + timeOrigin)) + startTime = hrTimeToMilliseconds(hrStartTime) + + this._ddSpan.addEvent(name, attributesOrStartTime, startTime) + return this + } + + recordException (exception, timeInput) { + // HACK: identifier is added so that trace.error remains unchanged after a call to otel.recordException this._ddSpan.addTags({ [ERROR_TYPE]: exception.name, [ERROR_MESSAGE]: exception.message, - [ERROR_STACK]: exception.stack + [ERROR_STACK]: exception.stack, + doNotSetTraceError: true }) + const attributes = {} + if (exception.message) attributes['exception.message'] = exception.message + if (exception.type) attributes['exception.type'] = exception.type + if (exception.escaped) attributes['exception.escaped'] = exception.escaped + if (exception.stack) attributes['exception.stacktrace'] = exception.stack + this.addEvent(exception.name, attributes, timeInput) } get duration () { diff --git a/packages/dd-trace/src/opentracing/span.js b/packages/dd-trace/src/opentracing/span.js index 6cb9cb77b1b..f71cf329c02 100644 --- a/packages/dd-trace/src/opentracing/span.js +++ b/packages/dd-trace/src/opentracing/span.js @@ -67,6 +67,8 @@ class DatadogSpan { this._store = storage.getStore() this._duration = undefined + this._events = [] + // For internal use only. You probably want `context()._name`. // This name property is not updated when the span name changes. // This is necessary for span count metrics. @@ -163,6 +165,19 @@ class DatadogSpan { }) } + addEvent (name, attributesOrStartTime, startTime) { + const event = { name } + if (attributesOrStartTime) { + if (typeof attributesOrStartTime === 'object') { + event.attributes = this._sanitizeEventAttributes(attributesOrStartTime) + } else { + startTime = attributesOrStartTime + } + } + event.startTime = startTime || this._getTime() + this._events.push(event) + } + finish (finishTime) { if (this._duration !== undefined) { return @@ -221,7 +236,30 @@ class DatadogSpan { const [key, value] = entry addArrayOrScalarAttributes(key, value) }) + return sanitizedAttributes + } + + _sanitizeEventAttributes (attributes = {}) { + const sanitizedAttributes = {} + for (const key in attributes) { + const value = attributes[key] + if (Array.isArray(value)) { + const newArray = [] + for (const subkey in value) { + if (ALLOWED.includes(typeof value[subkey])) { + newArray.push(value[subkey]) + } else { + log.warn('Dropping span event attribute. It is not of an allowed type') + } + } + sanitizedAttributes[key] = newArray + } else if (ALLOWED.includes(typeof value)) { + sanitizedAttributes[key] = value + } else { + log.warn('Dropping span event attribute. It is not of an allowed type') + } + } return sanitizedAttributes } diff --git a/packages/dd-trace/src/plugins/index.js b/packages/dd-trace/src/plugins/index.js index d7193917b05..0b98cd9c076 100644 --- a/packages/dd-trace/src/plugins/index.js +++ b/packages/dd-trace/src/plugins/index.js @@ -81,5 +81,6 @@ module.exports = { get 'selenium-webdriver' () { return require('../../../datadog-plugin-selenium/src') }, get sharedb () { return require('../../../datadog-plugin-sharedb/src') }, get tedious () { return require('../../../datadog-plugin-tedious/src') }, + get undici () { return require('../../../datadog-plugin-undici/src') }, get winston () { return require('../../../datadog-plugin-winston/src') } } diff --git a/packages/dd-trace/src/service-naming/schemas/v0/web.js b/packages/dd-trace/src/service-naming/schemas/v0/web.js index c63f83fac52..0c2228a563b 100644 --- a/packages/dd-trace/src/service-naming/schemas/v0/web.js +++ b/packages/dd-trace/src/service-naming/schemas/v0/web.js @@ -30,6 +30,10 @@ const web = { lambda: { opName: () => 'aws.request', serviceName: awsServiceV0 + }, + undici: { + opName: () => 'undici.request', + serviceName: httpPluginClientService } }, server: { diff --git a/packages/dd-trace/src/service-naming/schemas/v1/web.js b/packages/dd-trace/src/service-naming/schemas/v1/web.js index dfe3e6594e9..333ccae51c3 100644 --- a/packages/dd-trace/src/service-naming/schemas/v1/web.js +++ b/packages/dd-trace/src/service-naming/schemas/v1/web.js @@ -29,6 +29,10 @@ const web = { lambda: { opName: () => 'aws.lambda.invoke', serviceName: identityService + }, + undici: { + opName: () => 'undici.request', + serviceName: httpPluginClientService } }, server: { diff --git a/packages/dd-trace/src/tagger.js b/packages/dd-trace/src/tagger.js index aa3dd8efea4..41c8616a086 100644 --- a/packages/dd-trace/src/tagger.js +++ b/packages/dd-trace/src/tagger.js @@ -1,6 +1,10 @@ 'use strict' +const constants = require('./constants') const log = require('./log') +const ERROR_MESSAGE = constants.ERROR_MESSAGE +const ERROR_STACK = constants.ERROR_STACK +const ERROR_TYPE = constants.ERROR_TYPE const otelTagMap = { 'deployment.environment': 'env', @@ -14,7 +18,6 @@ function add (carrier, keyValuePairs, parseOtelTags = false) { if (Array.isArray(keyValuePairs)) { return keyValuePairs.forEach(tags => add(carrier, tags)) } - try { if (typeof keyValuePairs === 'string') { const segments = keyValuePairs.split(',') @@ -32,6 +35,12 @@ function add (carrier, keyValuePairs, parseOtelTags = false) { carrier[key.trim()] = value.trim() } } else { + // HACK: to ensure otel.recordException does not influence trace.error + if (ERROR_MESSAGE in keyValuePairs || ERROR_STACK in keyValuePairs || ERROR_TYPE in keyValuePairs) { + if (!('doNotSetTraceError' in keyValuePairs)) { + carrier.setTraceError = true + } + } Object.assign(carrier, keyValuePairs) } } catch (e) { diff --git a/packages/dd-trace/test/appsec/blocking.spec.js b/packages/dd-trace/test/appsec/blocking.spec.js index a0d454a77c7..04a3c496b46 100644 --- a/packages/dd-trace/test/appsec/blocking.spec.js +++ b/packages/dd-trace/test/appsec/blocking.spec.js @@ -37,7 +37,9 @@ describe('blocking', () => { res = { setHeader: sinon.stub(), writeHead: sinon.stub(), - end: sinon.stub() + end: sinon.stub(), + getHeaderNames: sinon.stub().returns([]), + removeHeader: sinon.stub() } res.writeHead.returns(res) @@ -109,6 +111,22 @@ describe('blocking', () => { expect(res.end).to.have.been.calledOnceWithExactly('jsonBody') expect(abortController.signal.aborted).to.be.true }) + + it('should remove all headers before sending blocking response', () => { + res.getHeaderNames.returns(['header1', 'header2']) + + block(req, res, rootSpan) + + expect(rootSpan.addTags).to.have.been.calledOnceWithExactly({ 'appsec.blocked': 'true' }) + expect(res.removeHeader).to.have.been.calledTwice + expect(res.removeHeader.firstCall).to.have.been.calledWithExactly('header1') + expect(res.removeHeader.secondCall).to.have.been.calledWithExactly('header2') + expect(res.writeHead).to.have.been.calledOnceWithExactly(403, { + 'Content-Type': 'application/json', + 'Content-Length': 8 + }) + expect(res.end).to.have.been.calledOnceWithExactly('jsonBody') + }) }) describe('block with default templates', () => { diff --git a/packages/dd-trace/test/appsec/index.spec.js b/packages/dd-trace/test/appsec/index.spec.js index febac128f83..652aae028ec 100644 --- a/packages/dd-trace/test/appsec/index.spec.js +++ b/packages/dd-trace/test/appsec/index.spec.js @@ -12,7 +12,8 @@ const { incomingHttpRequestEnd, queryParser, passportVerify, - responseBody + responseBody, + responseWriteHead } = require('../../src/appsec/channels') const Reporter = require('../../src/appsec/reporter') const agent = require('../plugins/agent') @@ -166,6 +167,7 @@ describe('AppSec Index', () => { expect(cookieParser.hasSubscribers).to.be.false expect(queryParser.hasSubscribers).to.be.false expect(passportVerify.hasSubscribers).to.be.false + expect(responseWriteHead.hasSubscribers).to.be.false AppSec.enable(config) @@ -173,6 +175,7 @@ describe('AppSec Index', () => { expect(cookieParser.hasSubscribers).to.be.true expect(queryParser.hasSubscribers).to.be.true expect(passportVerify.hasSubscribers).to.be.true + expect(responseWriteHead.hasSubscribers).to.be.true }) it('should not subscribe to passportVerify if eventTracking is disabled', () => { @@ -249,6 +252,7 @@ describe('AppSec Index', () => { expect(cookieParser.hasSubscribers).to.be.false expect(queryParser.hasSubscribers).to.be.false expect(passportVerify.hasSubscribers).to.be.false + expect(responseWriteHead.hasSubscribers).to.be.false }) it('should call appsec telemetry disable', () => { @@ -320,7 +324,7 @@ describe('AppSec Index', () => { web.root.returns(rootSpan) }) - it('should propagate incoming http end data', () => { + it('should not propagate incoming http end data without express', () => { const req = { url: '/path', headers: { @@ -348,17 +352,12 @@ describe('AppSec Index', () => { AppSec.incomingHttpEndTranslator({ req, res }) - expect(waf.run).to.have.been.calledOnceWithExactly({ - persistent: { - 'server.response.status': '201', - 'server.response.headers.no_cookies': { 'content-type': 'application/json', 'content-lenght': 42 } - } - }, req) + expect(waf.run).to.have.not.been.called expect(Reporter.finishRequest).to.have.been.calledOnceWithExactly(req, res) }) - it('should propagate incoming http end data with invalid framework properties', () => { + it('should not propagate incoming http end data with invalid framework properties', () => { const req = { url: '/path', headers: { @@ -391,12 +390,7 @@ describe('AppSec Index', () => { AppSec.incomingHttpEndTranslator({ req, res }) - expect(waf.run).to.have.been.calledOnceWithExactly({ - persistent: { - 'server.response.status': '201', - 'server.response.headers.no_cookies': { 'content-type': 'application/json', 'content-lenght': 42 } - } - }, req) + expect(waf.run).to.have.not.been.called expect(Reporter.finishRequest).to.have.been.calledOnceWithExactly(req, res) }) @@ -446,8 +440,6 @@ describe('AppSec Index', () => { expect(waf.run).to.have.been.calledOnceWithExactly({ persistent: { - 'server.response.status': '201', - 'server.response.headers.no_cookies': { 'content-type': 'application/json', 'content-lenght': 42 }, 'server.request.body': { a: '1' }, 'server.request.path_params': { c: '3' }, 'server.request.cookies': { d: '4', e: '5' }, @@ -652,7 +644,8 @@ describe('AppSec Index', () => { 'content-lenght': 42 }), writeHead: sinon.stub(), - end: sinon.stub() + end: sinon.stub(), + getHeaderNames: sinon.stub().returns([]) } res.writeHead.returns(res) @@ -660,10 +653,6 @@ describe('AppSec Index', () => { AppSec.incomingHttpStartTranslator({ req, res }) }) - afterEach(() => { - AppSec.disable() - }) - describe('onRequestBodyParsed', () => { it('Should not block without body', () => { sinon.stub(waf, 'run') @@ -822,6 +811,111 @@ describe('AppSec Index', () => { expect(passport.passportTrackEvent).not.to.have.been.called }) }) + + describe('onResponseWriteHead', () => { + it('should call abortController if response was already blocked', () => { + 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(waf.run).to.have.been.calledOnceWithExactly({ + persistent: { + 'server.response.status': '404', + 'server.response.headers.no_cookies': { + 'content-type': 'application/json', + 'content-lenght': 42 + } + } + }, req) + expect(abortController.abort).to.have.been.calledOnce + expect(res.end).to.have.been.calledOnce + + abortController.abort.resetHistory() + + responseWriteHead.publish({ req, res, abortController, statusCode: 404, responseHeaders }) + + expect(waf.run).to.have.been.calledOnce + expect(abortController.abort).to.have.been.calledOnce + expect(res.end).to.have.been.calledOnce + }) + + it('should not call the WAF if response was already analyzed', () => { + sinon.stub(waf, 'run').returns(null) + + const responseHeaders = { + 'content-type': 'application/json', + 'content-lenght': 42, + 'set-cookie': 'a=1;b=2' + } + + responseWriteHead.publish({ req, res, abortController, statusCode: 404, responseHeaders }) + + expect(waf.run).to.have.been.calledOnceWithExactly({ + persistent: { + 'server.response.status': '404', + 'server.response.headers.no_cookies': { + 'content-type': 'application/json', + 'content-lenght': 42 + } + } + }, req) + expect(abortController.abort).to.have.not.been.called + expect(res.end).to.have.not.been.called + + responseWriteHead.publish({ req, res, abortController, statusCode: 404, responseHeaders }) + + expect(waf.run).to.have.been.calledOnce + expect(abortController.abort).to.have.not.been.called + expect(res.end).to.have.not.been.called + }) + + it('should not do anything without a root span', () => { + web.root.returns(null) + sinon.stub(waf, 'run').returns(null) + + const responseHeaders = { + 'content-type': 'application/json', + 'content-lenght': 42, + 'set-cookie': 'a=1;b=2' + } + + responseWriteHead.publish({ req, res, abortController, statusCode: 404, responseHeaders }) + + expect(waf.run).to.have.not.been.called + expect(abortController.abort).to.have.not.been.called + expect(res.end).to.have.not.been.called + }) + + it('should call the WAF with responde code and headers', () => { + 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(waf.run).to.have.been.calledOnceWithExactly({ + persistent: { + 'server.response.status': '404', + 'server.response.headers.no_cookies': { + 'content-type': 'application/json', + 'content-lenght': 42 + } + } + }, req) + expect(abortController.abort).to.have.been.calledOnce + expect(res.end).to.have.been.calledOnce + }) + }) }) describe('Metrics', () => { diff --git a/packages/dd-trace/test/appsec/remote_config/index.spec.js b/packages/dd-trace/test/appsec/remote_config/index.spec.js index 1307fbed444..d954e41e15b 100644 --- a/packages/dd-trace/test/appsec/remote_config/index.spec.js +++ b/packages/dd-trace/test/appsec/remote_config/index.spec.js @@ -276,6 +276,8 @@ describe('Remote Config index', () => { .to.have.been.calledWithExactly(RemoteConfigCapabilities.ASM_EXCLUSIONS, true) expect(rc.updateCapabilities) .to.have.been.calledWithExactly(RemoteConfigCapabilities.ASM_REQUEST_BLOCKING, true) + expect(rc.updateCapabilities) + .to.have.been.calledWithExactly(RemoteConfigCapabilities.ASM_RESPONSE_BLOCKING, true) expect(rc.updateCapabilities) .to.have.been.calledWithExactly(RemoteConfigCapabilities.ASM_CUSTOM_RULES, true) expect(rc.updateCapabilities) @@ -304,6 +306,8 @@ describe('Remote Config index', () => { .to.have.been.calledWithExactly(RemoteConfigCapabilities.ASM_EXCLUSIONS, true) expect(rc.updateCapabilities) .to.have.been.calledWithExactly(RemoteConfigCapabilities.ASM_REQUEST_BLOCKING, true) + expect(rc.updateCapabilities) + .to.have.been.calledWithExactly(RemoteConfigCapabilities.ASM_RESPONSE_BLOCKING, true) expect(rc.updateCapabilities) .to.have.been.calledWithExactly(RemoteConfigCapabilities.ASM_CUSTOM_RULES, true) expect(rc.updateCapabilities) @@ -334,6 +338,8 @@ describe('Remote Config index', () => { .to.have.been.calledWithExactly(RemoteConfigCapabilities.ASM_EXCLUSIONS, true) expect(rc.updateCapabilities) .to.have.been.calledWithExactly(RemoteConfigCapabilities.ASM_REQUEST_BLOCKING, true) + expect(rc.updateCapabilities) + .to.have.been.calledWithExactly(RemoteConfigCapabilities.ASM_RESPONSE_BLOCKING, true) expect(rc.updateCapabilities) .to.have.been.calledWithExactly(RemoteConfigCapabilities.ASM_CUSTOM_RULES, true) expect(rc.updateCapabilities) @@ -359,6 +365,8 @@ describe('Remote Config index', () => { .to.have.been.calledWithExactly(RemoteConfigCapabilities.ASM_EXCLUSIONS, false) expect(rc.updateCapabilities) .to.have.been.calledWithExactly(RemoteConfigCapabilities.ASM_REQUEST_BLOCKING, false) + expect(rc.updateCapabilities) + .to.have.been.calledWithExactly(RemoteConfigCapabilities.ASM_RESPONSE_BLOCKING, false) expect(rc.updateCapabilities) .to.have.been.calledWithExactly(RemoteConfigCapabilities.ASM_CUSTOM_RULES, false) expect(rc.updateCapabilities) diff --git a/packages/dd-trace/test/appsec/response_blocking.spec.js b/packages/dd-trace/test/appsec/response_blocking.spec.js new file mode 100644 index 00000000000..672933784e7 --- /dev/null +++ b/packages/dd-trace/test/appsec/response_blocking.spec.js @@ -0,0 +1,271 @@ +'use strict' + +const { assert } = require('chai') +const getPort = require('get-port') +const agent = require('../plugins/agent') +const Axios = require('axios') +const appsec = require('../../src/appsec') +const Config = require('../../src/config') +const path = require('path') +const WafContext = require('../../src/appsec/waf/waf_context_wrapper') +const blockingResponse = JSON.parse(require('../../src/appsec/blocked_templates').json) +const fs = require('fs') + +describe('HTTP Response Blocking', () => { + let server + let responseHandler + let axios + + before(async () => { + const port = await getPort() + + await agent.load('http') + + const http = require('http') + + server = new http.Server((req, res) => { + // little polyfill, older versions of node don't have setHeaders() + if (typeof res.setHeaders !== 'function') { + res.setHeaders = headers => headers.forEach((v, k) => res.setHeader(k, v)) + } + + if (responseHandler) { + responseHandler(req, res) + } else { + res.writeHead(200) + res.end('OK') + } + }) + + await new Promise((resolve, reject) => { + server.listen(port, 'localhost') + .once('listening', resolve) + .once('error', reject) + }) + + axios = Axios.create(({ + baseURL: `http://localhost:${port}`, + validateStatus: null + })) + + appsec.enable(new Config({ + appsec: { + enabled: true, + rules: path.join(__dirname, 'response_blocking_rules.json') + } + })) + }) + + beforeEach(() => { + sinon.spy(WafContext.prototype, 'run') + }) + + afterEach(() => { + sinon.restore() + responseHandler = null + }) + + after(() => { + appsec.disable() + server?.close() + return agent.close({ ritmReset: false }) + }) + + it('should block with implicit statusCode + setHeader() + end()', async () => { + responseHandler = (req, res) => { + res.statusCode = 404 + res.setHeader('k', '404') + res.end('end') + } + + const res = await axios.get('/') + + assertBlocked(res) + }) + + it('should block with setHeader() + setHeaders() + writeHead() headers', async () => { + responseHandler = (req, res) => { + res.setHeaders(new Map(Object.entries({ a: 'bad1', b: 'good' }))) + res.setHeader('c', 'bad2') + res.writeHead(200, { d: 'bad3' }) + res.end('end') + } + + const res = await axios.get('/') + + assertBlocked(res) + }) + + it('should block with setHeader() + array writeHead() ', async () => { + responseHandler = (req, res) => { + res.setHeader('a', 'bad1') + res.writeHead(200, 'OK', ['b', 'bad2', 'c', 'bad3']) + res.end('end') + } + + const res = await axios.get('/') + + assertBlocked(res) + }) + + it('should not block with array writeHead() when attack is in the header name and not in header value', async () => { + responseHandler = (req, res) => { + res.writeHead(200, 'OK', ['a', 'bad1', 'b', 'bad2', 'bad3', 'c']) + res.end('end') + } + + const res = await axios.get('/') + + assert.equal(res.status, 200) + assert.hasAllKeys(cloneHeaders(res.headers), [ + 'a', + 'b', + 'bad3', + 'date', + 'connection', + 'transfer-encoding' + ]) + assert.deepEqual(res.data, 'end') + }) + + it('should block with implicit statusCode + setHeader() + flushHeaders()', async () => { + responseHandler = (req, res) => { + res.statusCode = 404 + res.setHeader('k', '404') + res.flushHeaders() + res.end('end') + } + + const res = await axios.get('/') + + assertBlocked(res) + }) + + it('should block with implicit statusCode + setHeader() + write()', async () => { + responseHandler = (req, res) => { + res.statusCode = 404 + res.setHeader('k', '404') + res.write('write') + res.end('end') + } + + const res = await axios.get('/') + + assertBlocked(res) + }) + + it('should block with implicit statusCode + setHeader() + stream pipe', async () => { + responseHandler = (req, res) => { + res.statusCode = 404 + res.setHeader('k', '404') + streamFile(res) + } + + const res = await axios.get('/') + + assertBlocked(res) + }) + + it('should block with writeHead() + write()', async () => { + responseHandler = (req, res) => { + res.writeHead(404, { k: '404' }) + res.write('write') + res.end('end') + } + + const res = await axios.get('/') + + assertBlocked(res) + }) + + it('should block with every methods combined', async () => { + responseHandler = (req, res) => { + res.setHeaders(new Map(Object.entries({ a: 'bad1', b: 'good' }))) + res.setHeader('c', 'bad2') + res.setHeader('d', 'good') + res.writeHead(200, 'OK', { d: 'good', e: 'bad3' }) + res.flushHeaders() + res.write('write') + res.addTrailers({ k: 'v' }) + streamFile(res) + } + + const res = await axios.get('/') + + assertBlocked(res) + }) + + it('should not block with every methods combined but no attack', async () => { + responseHandler = (req, res) => { + res.setHeaders(new Map(Object.entries({ a: 'good', b: 'good' }))) + res.setHeader('c', 'good') + res.setHeader('d', 'good') + res.writeHead(201, 'OK', { d: 'good', e: 'good' }) + res.flushHeaders() + res.write('write') + res.addTrailers({ k: 'v' }) + streamFile(res) + } + + const res = await axios.get('/') + + assert.equal(res.status, 201) + assert.hasAllKeys(cloneHeaders(res.headers), [ + 'a', + 'b', + 'c', + 'd', + 'e', + 'date', + 'connection', + 'transfer-encoding' + ]) + assert.deepEqual(res.data, 'writefileend') + }) + + it('should ignore subsequent response writes after blocking', async () => { + responseHandler = (req, res) => { + res.statusCode = 404 + res.setHeader('k', '404') + res.flushHeaders() + res.writeHead(200, { k: '200' }) + res.write('write1') + setTimeout(() => { + res.write('write2') + res.end('end') + }, 1000) + } + + const res = await axios.get('/') + + assertBlocked(res) + }) +}) + +function cloneHeaders (headers) { + // clone the headers accessor to a flat object + // and delete the keep-alive header as it's not always present + headers = Object.fromEntries(Object.entries(headers)) + delete headers['keep-alive'] + + return headers +} + +function assertBlocked (res) { + assert.equal(res.status, 403) + assert.hasAllKeys(cloneHeaders(res.headers), [ + 'content-type', + 'content-length', + 'date', + 'connection' + ]) + assert.deepEqual(res.data, blockingResponse) + + sinon.assert.callCount(WafContext.prototype.run, 2) +} + +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')) +} diff --git a/packages/dd-trace/test/appsec/response_blocking_rules.json b/packages/dd-trace/test/appsec/response_blocking_rules.json new file mode 100644 index 00000000000..bd7d1279892 --- /dev/null +++ b/packages/dd-trace/test/appsec/response_blocking_rules.json @@ -0,0 +1,110 @@ +{ + "version": "2.2", + "metadata": { + "rules_version": "1.5.0" + }, + "rules": [ + { + "id": "test-rule-id-1", + "name": "test-rule-name-1", + "tags": { + "type": "security_scanner1", + "category": "attack_attempt1" + }, + "conditions": [ + { + "operator": "match_regex", + "parameters": { + "inputs": [ + { + "address": "server.response.status" + } + ], + "regex": "^404$", + "options": { + "case_sensitive": true + } + } + }, + { + "operator": "match_regex", + "parameters": { + "inputs": [ + { + "address": "server.response.headers.no_cookies" + } + ], + "regex": "^404$", + "options": { + "case_sensitive": false + } + } + } + ], + "transformers": [ + "lowercase" + ], + "on_match": [ + "block" + ] + }, + { + "id": "test-rule-id-2", + "name": "test-rule-name-2", + "tags": { + "type": "security_scanner2", + "category": "attack_attempt2" + }, + "conditions": [ + { + "operator": "match_regex", + "parameters": { + "inputs": [ + { + "address": "server.response.headers.no_cookies" + } + ], + "regex": "^bad1$", + "options": { + "case_sensitive": false + } + } + }, + { + "operator": "match_regex", + "parameters": { + "inputs": [ + { + "address": "server.response.headers.no_cookies" + } + ], + "regex": "^bad2$", + "options": { + "case_sensitive": false + } + } + }, + { + "operator": "match_regex", + "parameters": { + "inputs": [ + { + "address": "server.response.headers.no_cookies" + } + ], + "regex": "^bad3$", + "options": { + "case_sensitive": false + } + } + } + ], + "transformers": [ + "lowercase" + ], + "on_match": [ + "block" + ] + } + ] +} diff --git a/packages/dd-trace/test/appsec/streamtest.txt b/packages/dd-trace/test/appsec/streamtest.txt new file mode 100644 index 00000000000..1a010b1c0f0 --- /dev/null +++ b/packages/dd-trace/test/appsec/streamtest.txt @@ -0,0 +1 @@ +file \ No newline at end of file diff --git a/packages/dd-trace/test/encode/0.4.spec.js b/packages/dd-trace/test/encode/0.4.spec.js index 13d20250109..564daf8e92e 100644 --- a/packages/dd-trace/test/encode/0.4.spec.js +++ b/packages/dd-trace/test/encode/0.4.spec.js @@ -185,6 +185,21 @@ describe('encode', () => { }) }) + it('should encode span events', () => { + const encodedLink = '[{"name":"Something went so wrong","time_unix_nano":1000000},' + + '{"name":"I can sing!!! acbdefggnmdfsdv k 2e2ev;!|=xxx","time_unix_nano":1633023102000000,' + + '"attributes":{"emotion":"happy","rating":9.8,"other":[1,9.5,1],"idol":false}}]' + + data[0].meta.events = encodedLink + + encoder.encode(data) + + const buffer = encoder.makePayload() + const decoded = msgpack.decode(buffer, { codec }) + const trace = decoded[0] + expect(trace[0].meta.events).to.deep.equal(encodedLink) + }) + it('should encode spanLinks', () => { const traceIdHigh = id('10') const traceId = id('1234abcd1234abcd') diff --git a/packages/dd-trace/test/encode/0.5.spec.js b/packages/dd-trace/test/encode/0.5.spec.js index 2ef925c60c3..ec7b36af08b 100644 --- a/packages/dd-trace/test/encode/0.5.spec.js +++ b/packages/dd-trace/test/encode/0.5.spec.js @@ -65,6 +65,27 @@ describe('encode 0.5', () => { expect(stringMap[trace[0][11]]).to.equal('') // unset }) + it('should encode span events', () => { + const encodedLink = '[{"name":"Something went so wrong","time_unix_nano":1000000},' + + '{"name":"I can sing!!! acbdefggnmdfsdv k 2e2ev;!|=xxx","time_unix_nano":1633023102000000,' + + '"attributes":{"emotion":"happy","rating":9.8,"other":[1,9.5,1],"idol":false}}]' + + data[0].meta.events = encodedLink + + encoder.encode(data) + + const buffer = encoder.makePayload() + const decoded = msgpack.decode(buffer, { codec }) + const stringMap = decoded[0] + const trace = decoded[1][0] + expect(stringMap).to.include('events') + expect(stringMap).to.include(encodedLink) + expect(trace[0][9]).to.include({ + [stringMap.indexOf('bar')]: stringMap.indexOf('baz'), + [stringMap.indexOf('events')]: stringMap.indexOf(encodedLink) + }) + }) + it('should encode span links', () => { const traceIdHigh = id('10') const traceId = id('1234abcd1234abcd') diff --git a/packages/dd-trace/test/format.spec.js b/packages/dd-trace/test/format.spec.js index b75c73d99a7..65cf8a4aa3e 100644 --- a/packages/dd-trace/test/format.spec.js +++ b/packages/dd-trace/test/format.spec.js @@ -87,6 +87,27 @@ describe('format', () => { }) describe('format', () => { + it('should format span events', () => { + span._events = [ + { name: 'Something went so wrong', startTime: 1 }, + { + name: 'I can sing!!! acbdefggnmdfsdv k 2e2ev;!|=xxx', + attributes: { emotion: 'happy', rating: 9.8, other: [1, 9.5, 1], idol: false }, + startTime: 1633023102 + } + ] + + trace = format(span) + const spanEvents = JSON.parse(trace.meta.events) + expect(spanEvents).to.deep.equal([{ + name: 'Something went so wrong', + time_unix_nano: 1000000 + }, { + name: 'I can sing!!! acbdefggnmdfsdv k 2e2ev;!|=xxx', + time_unix_nano: 1633023102000000, + attributes: { emotion: 'happy', rating: 9.8, other: [1, 9.5, 1], idol: false } + }]) + }) it('should convert a span to the correct trace format', () => { trace = format(span) @@ -403,14 +424,31 @@ describe('format', () => { }) }) - it('should set the error flag when there is an error-related tag', () => { + it('should not set the error flag when there is an error-related tag without a set trace tag', () => { + spanContext._tags[ERROR_TYPE] = 'Error' + spanContext._tags[ERROR_MESSAGE] = 'boom' + spanContext._tags[ERROR_STACK] = '' + + trace = format(span) + + expect(trace.error).to.equal(0) + }) + + it('should set the error flag when there is an error-related tag with should setTrace', () => { spanContext._tags[ERROR_TYPE] = 'Error' spanContext._tags[ERROR_MESSAGE] = 'boom' spanContext._tags[ERROR_STACK] = '' + spanContext._tags.setTraceError = 1 trace = format(span) expect(trace.error).to.equal(1) + + spanContext._tags[ERROR_TYPE] = 'foo' + spanContext._tags[ERROR_MESSAGE] = 'foo' + spanContext._tags[ERROR_STACK] = 'foo' + + expect(trace.error).to.equal(1) }) it('should not set the error flag for internal spans with error tags', () => { diff --git a/packages/dd-trace/test/opentelemetry/span.spec.js b/packages/dd-trace/test/opentelemetry/span.spec.js index 378067ae541..ecdea99a1fa 100644 --- a/packages/dd-trace/test/opentelemetry/span.spec.js +++ b/packages/dd-trace/test/opentelemetry/span.spec.js @@ -2,8 +2,12 @@ require('../setup/tap') -const { expect } = require('chai') +const sinon = require('sinon') +const { performance } = require('perf_hooks') +const { timeOrigin } = performance +const { timeInputToHrTime } = require('@opentelemetry/core') +const { expect } = require('chai') const tracer = require('../../').init() const api = require('@opentelemetry/api') @@ -347,6 +351,40 @@ describe('OTel Span', () => { } } + const error = new TestError() + const datenow = Date.now() + span.recordException(error, datenow) + + const { _tags } = span._ddSpan.context() + expect(_tags).to.have.property(ERROR_TYPE, error.name) + expect(_tags).to.have.property(ERROR_MESSAGE, error.message) + expect(_tags).to.have.property(ERROR_STACK, error.stack) + + const events = span._ddSpan._events + expect(events).to.have.lengthOf(1) + expect(events).to.deep.equal([{ + name: error.name, + attributes: { + 'exception.message': error.message, + 'exception.stacktrace': error.stack + }, + startTime: datenow + }]) + }) + + it('should record exception without passing in time', () => { + const stub = sinon.stub(performance, 'now').returns(60000) + const span = makeSpan('name') + + class TestError extends Error { + constructor () { + super('test message') + } + } + + const time = timeInputToHrTime(60000 + timeOrigin) + const timeInMilliseconds = time[0] * 1e3 + time[1] / 1e6 + const error = new TestError() span.recordException(error) @@ -354,6 +392,18 @@ describe('OTel Span', () => { expect(_tags).to.have.property(ERROR_TYPE, error.name) expect(_tags).to.have.property(ERROR_MESSAGE, error.message) expect(_tags).to.have.property(ERROR_STACK, error.stack) + + const events = span._ddSpan._events + expect(events).to.have.lengthOf(1) + expect(events).to.deep.equal([{ + name: error.name, + attributes: { + 'exception.message': error.message, + 'exception.stacktrace': error.stack + }, + startTime: timeInMilliseconds + }]) + stub.restore() }) it('should not set status on already ended spans', () => { @@ -402,4 +452,25 @@ describe('OTel Span', () => { expect(processor.onStart).to.have.been.calledWith(span, span._context) expect(processor.onEnd).to.have.been.calledWith(span) }) + it('should add span events', () => { + const span1 = makeSpan('span1') + const span2 = makeSpan('span2') + const datenow = Date.now() + span1.addEvent('Web page unresponsive', + { 'error.code': '403', 'unknown values': [1, ['h', 'a', [false]]] }, datenow) + span2.addEvent('Web page loaded') + span2.addEvent('Button changed color', { colors: [112, 215, 70], 'response.time': 134.3, success: true }) + const events1 = span1._ddSpan._events + const events2 = span2._ddSpan._events + expect(events1).to.have.lengthOf(1) + expect(events1).to.deep.equal([{ + name: 'Web page unresponsive', + startTime: datenow, + attributes: { + 'error.code': '403', + 'unknown values': [1] + } + }]) + expect(events2).to.have.lengthOf(2) + }) }) diff --git a/packages/dd-trace/test/opentracing/span.spec.js b/packages/dd-trace/test/opentracing/span.spec.js index 754d0a67f37..326796cae26 100644 --- a/packages/dd-trace/test/opentracing/span.spec.js +++ b/packages/dd-trace/test/opentracing/span.spec.js @@ -279,6 +279,43 @@ describe('Span', () => { }) }) + describe('events', () => { + it('should add span events', () => { + span = new Span(tracer, processor, prioritySampler, { operationName: 'operation' }) + + span.addEvent('Web page unresponsive', + { 'error.code': '403', 'unknown values': [1, ['h', 'a', [false]]] }, 1714536311886) + span.addEvent('Web page loaded') + span.addEvent('Button changed color', { colors: [112, 215, 70], 'response.time': 134.3, success: true }) + + const events = span._events + const expectedEvents = [ + { + name: 'Web page unresponsive', + startTime: 1714536311886, + attributes: { + 'error.code': '403', + 'unknown values': [1] + } + }, + { + name: 'Web page loaded', + startTime: 1500000000000 + }, + { + name: 'Button changed color', + attributes: { + colors: [112, 215, 70], + 'response.time': 134.3, + success: true + }, + startTime: 1500000000000 + } + ] + expect(events).to.deep.equal(expectedEvents) + }) + }) + describe('getBaggageItem', () => { it('should get a baggage item', () => { span = new Span(tracer, processor, prioritySampler, { operationName: 'operation' }) diff --git a/packages/dd-trace/test/tagger.spec.js b/packages/dd-trace/test/tagger.spec.js index 4f0426ea179..161136819d2 100644 --- a/packages/dd-trace/test/tagger.spec.js +++ b/packages/dd-trace/test/tagger.spec.js @@ -1,6 +1,10 @@ 'use strict' +const constants = require('../src/constants') require('./setup/tap') +const ERROR_MESSAGE = constants.ERROR_MESSAGE +const ERROR_STACK = constants.ERROR_STACK +const ERROR_TYPE = constants.ERROR_TYPE describe('tagger', () => { let carrier @@ -45,4 +49,29 @@ describe('tagger', () => { it('should handle missing carrier', () => { expect(() => tagger.add()).not.to.throw() }) + + it('should set trace error', () => { + tagger.add(carrier, { + [ERROR_TYPE]: 'foo', + [ERROR_MESSAGE]: 'foo', + [ERROR_STACK]: 'foo', + doNotSetTraceError: true + }) + + expect(carrier).to.have.property(ERROR_TYPE, 'foo') + expect(carrier).to.have.property(ERROR_MESSAGE, 'foo') + expect(carrier).to.have.property(ERROR_STACK, 'foo') + expect(carrier).to.not.have.property('setTraceError') + + tagger.add(carrier, { + [ERROR_TYPE]: 'foo', + [ERROR_MESSAGE]: 'foo', + [ERROR_STACK]: 'foo' + }) + + expect(carrier).to.have.property(ERROR_TYPE, 'foo') + expect(carrier).to.have.property(ERROR_MESSAGE, 'foo') + expect(carrier).to.have.property(ERROR_STACK, 'foo') + expect(carrier).to.have.property('setTraceError', true) + }) })