From 947d91aec35b388cd47acb9bcb01ebf839b8ab7f Mon Sep 17 00:00:00 2001 From: Julio Gonzalez Date: Thu, 1 Jun 2023 11:28:06 +0200 Subject: [PATCH 01/45] Add passport local and http instrumentations. --- .../src/helpers/hooks.js | 2 + .../src/passport-http.js | 46 +++++++ .../src/passport-local.js | 46 +++++++ .../test/passport-http.spec.js | 119 ++++++++++++++++++ .../test/passport-local.spec.js | 110 ++++++++++++++++ packages/dd-trace/test/plugins/externals.json | 10 ++ 6 files changed, 333 insertions(+) create mode 100644 packages/datadog-instrumentations/src/passport-http.js create mode 100644 packages/datadog-instrumentations/src/passport-local.js create mode 100644 packages/datadog-instrumentations/test/passport-http.spec.js create mode 100644 packages/datadog-instrumentations/test/passport-local.spec.js diff --git a/packages/datadog-instrumentations/src/helpers/hooks.js b/packages/datadog-instrumentations/src/helpers/hooks.js index 45762c94a8e..27a37698b46 100644 --- a/packages/datadog-instrumentations/src/helpers/hooks.js +++ b/packages/datadog-instrumentations/src/helpers/hooks.js @@ -68,6 +68,8 @@ module.exports = { 'next': () => require('../next'), 'oracledb': () => require('../oracledb'), 'paperplane': () => require('../paperplane'), + 'passport-http': () => require('../passport-http'), + 'passport-local': () => require('../passport-local'), 'pg': () => require('../pg'), 'pino': () => require('../pino'), 'pino-pretty': () => require('../pino'), diff --git a/packages/datadog-instrumentations/src/passport-http.js b/packages/datadog-instrumentations/src/passport-http.js new file mode 100644 index 00000000000..5fff0efeedc --- /dev/null +++ b/packages/datadog-instrumentations/src/passport-http.js @@ -0,0 +1,46 @@ +'use strict' + +const shimmer = require('../../datadog-shimmer') +const { channel, addHook } = require('./helpers/instrument') + +const passportVerifyChannel = channel('datadog:passport:verify:finish') + +function wrapVerifiedAndPublish (username, password, verified) { + if (passportVerifyChannel.hasSubscribers) { + return shimmer.wrap(verified, (err, user, info) => { + // TODO: check if err and info are really useful + const credentials = { type: 'http', username: username, password: password } + passportVerifyChannel.publish({ credentials, user, err, info }) + return verified(err, user, info) + }) + } +} + +function wrapVerify (verify, passReq) { + if (passReq) { + return function (req, username, password, verified) { + arguments[3] = wrapVerifiedAndPublish(username, password, verified) + return verify.apply(this, arguments) + } + } else { + return function (username, password, verified) { + arguments[2] = wrapVerifiedAndPublish(username, password, verified) + return verify.apply(this, arguments) + } + } +} + +addHook({ + name: 'passport-http', + file: 'lib/passport-http/strategies/basic.js', + versions: ['>=0.3.0'] +}, BasicStrategy => { + return shimmer.wrap(BasicStrategy, function () { + if (typeof arguments[0] === 'function') { + arguments[0] = wrapVerify(arguments[0], false) + } else { + arguments[1] = wrapVerify(arguments[1], arguments[0].passReqToCallback || false) + } + BasicStrategy.apply(this, arguments) + }) +}) diff --git a/packages/datadog-instrumentations/src/passport-local.js b/packages/datadog-instrumentations/src/passport-local.js new file mode 100644 index 00000000000..873db4ca692 --- /dev/null +++ b/packages/datadog-instrumentations/src/passport-local.js @@ -0,0 +1,46 @@ +'use strict' + +const shimmer = require('../../datadog-shimmer') +const { channel, addHook } = require('./helpers/instrument') + +const passportVerifyChannel = channel('datadog:passport:verify:finish') + +function wrapVerifiedAndPublish (username, password, verified) { + if (passportVerifyChannel.hasSubscribers) { + return shimmer.wrap(verified, (err, user, info) => { + // TODO: check if err and info are really useful + const credentials = { type: 'local', username: username, password: password } + passportVerifyChannel.publish({ credentials, user, err, info }) + return verified(err, user, info) + }) + } +} + +function wrapVerify (verify, passReq) { + if (passReq) { + return function (req, username, password, verified) { + arguments[3] = wrapVerifiedAndPublish(username, password, verified) + return verify.apply(this, arguments) + } + } else { + return function (username, password, verified) { + arguments[2] = wrapVerifiedAndPublish(username, password, verified) + return verify.apply(this, arguments) + } + } +} + +addHook({ + name: 'passport-local', + file: 'lib/strategy.js', + versions: ['>=1.0.0'] +}, Strategy => { + return shimmer.wrap(Strategy, function () { + if (typeof arguments[0] === 'function') { + arguments[0] = wrapVerify(arguments[0], false) + } else { + arguments[1] = wrapVerify(arguments[1], arguments[0].passReqToCallback || false) + } + Strategy.apply(this, arguments) + }) +}) diff --git a/packages/datadog-instrumentations/test/passport-http.spec.js b/packages/datadog-instrumentations/test/passport-http.spec.js new file mode 100644 index 00000000000..f2322fd5945 --- /dev/null +++ b/packages/datadog-instrumentations/test/passport-http.spec.js @@ -0,0 +1,119 @@ +'use strict' + +const agent = require('../../dd-trace/test/plugins/agent') +const getPort = require('get-port') +const axios = require('axios') +const dc = require('../../diagnostics_channel') + +withVersions('passport-http', 'passport-http', version => { + describe('passport-http instrumentation', () => { + const passportVerifyChannel = dc.channel('datadog:passport:verify:finish') + let port, server, subscriberStub + + before(() => { + return agent.load(['express', 'passport', 'passport-http'], { client: false }) + }) + before((done) => { + const express = require('../../../versions/express').get() + const passport = require('../../../versions/passport').get() + const BasicStrategy = require(`../../../versions/passport-http@${version}`).get().BasicStrategy + const app = express() + + passport.use(new BasicStrategy((username, password, done) => { + const users = [{ + _id: 1, + username: 'test', + password: '1234', + email: 'testuser@ddog.com' + }] + + const user = users.find(user => (user.username === username) && (user.password === password)) + + if (!user) { + return done(null, false) + } else { + return done(null, user) + } + } + )) + + app.use(passport.initialize()) + app.use(express.json()) + + app.get('/', + passport.authenticate('basic', { + successRedirect: '/grant', + failureRedirect: '/deny', + session: false + }) + ) + + app.get('/grant', (req, res) => { + res.send('Granted') + }) + + app.get('/deny', (req, res) => { + res.send('Denied') + }) + + passportVerifyChannel.subscribe(function ({ credentials, user, err, info }) { + subscriberStub(arguments[0]) + }) + + getPort().then(newPort => { + port = newPort + server = app.listen(port, () => { + done() + }) + }) + }) + beforeEach(async () => { + subscriberStub = sinon.stub() + }) + + after(() => { + server.close() + return agent.close({ ritmReset: false }) + }) + + it('should call subscriber with proper arguments on success', async () => { + const res = await axios.get(`http://localhost:${port}/`, { + headers: { + // test:1234 + 'Authorization': 'Basic dGVzdDoxMjM0' + } + }) + + expect(res.status).to.equal(200) + expect(res.data).to.equal('Granted') + expect(subscriberStub).to.be.calledOnceWithExactly( + { + credentials: { type: 'http', username: 'test', password: '1234' }, + user: { _id: 1, username: 'test', password: '1234', email: 'testuser@ddog.com' }, + err: null, + info: undefined + } + ) + }) + + it('should call subscriber with proper arguments on failure', async () => { + const res = await axios.get(`http://localhost:${port}/`, { + headers: { + // test:1 + 'Authorization': 'Basic dGVzdDox' + } + }) + + expect(res.status).to.equal(200) + expect(res.data).to.equal('Denied') + expect(subscriberStub).to.be.calledOnceWithExactly( + { + credentials: { type: 'http', username: 'test', password: '1' }, + user: false, + err: null, + info: undefined + } + ) + }) + }) +}) diff --git a/packages/datadog-instrumentations/test/passport-local.spec.js b/packages/datadog-instrumentations/test/passport-local.spec.js new file mode 100644 index 00000000000..f80db29dd5b --- /dev/null +++ b/packages/datadog-instrumentations/test/passport-local.spec.js @@ -0,0 +1,110 @@ +'use strict' + +const agent = require('../../dd-trace/test/plugins/agent') +const getPort = require('get-port') +const axios = require('axios') +const dc = require('../../diagnostics_channel') + +withVersions('passport-local', 'passport-local', version => { + describe('passport-local instrumentation', () => { + const passportVerifyChannel = dc.channel('datadog:passport:verify:finish') + let port, server, subscriberStub + + before(() => { + return agent.load(['express', 'passport', 'passport-local'], { client: false }) + }) + before((done) => { + const express = require('../../../versions/express').get() + const passport = require(`../../../versions/passport`).get() + const LocalStrategy = require(`../../../versions/passport-local@${version}`).get().Strategy + const app = express() + + passport.use(new LocalStrategy({ usernameField: 'username', passwordField: 'password' }, + (username, password, done) => { + const users = [{ + _id: 1, + username: 'test', + password: '1234', + email: 'testuser@ddog.com' + }] + + const user = users.find(user => (user.username === username) && (user.password === password)) + + if (!user) { + return done(null, false) + } else { + return done(null, user) + } + } + )) + + app.use(passport.initialize()) + app.use(express.json()) + + app.post('/', + passport.authenticate('local', { + successRedirect: '/grant', + failureRedirect: '/deny', + session: false + }) + ) + + app.get('/grant', (req, res) => { + res.send('Granted') + }) + + app.get('/deny', (req, res) => { + res.send('Denied') + }) + + passportVerifyChannel.subscribe(function ({ credentials, user, err, info }) { + subscriberStub(arguments[0]) + }) + + getPort().then(newPort => { + port = newPort + server = app.listen(port, () => { + done() + }) + }) + }) + beforeEach(async () => { + subscriberStub = sinon.stub() + }) + + after(() => { + server.close() + return agent.close({ ritmReset: false }) + }) + + it('should call subscriber with proper arguments on success', async () => { + const res = await axios.post(`http://localhost:${port}/`, { username: 'test', password: '1234' }) + + expect(res.status).to.equal(200) + expect(res.data).to.equal('Granted') + expect(subscriberStub).to.be.calledOnceWithExactly( + { + credentials: { type: 'local', username: 'test', password: '1234' }, + user: { _id: 1, username: 'test', password: '1234', email: 'testuser@ddog.com' }, + err: null, + info: undefined + } + ) + }) + + it('should call subscriber with proper arguments on failure', async () => { + const res = await axios.post(`http://localhost:${port}/`, { username: 'test', password: '1' }) + + expect(res.status).to.equal(200) + expect(res.data).to.equal('Denied') + expect(subscriberStub).to.be.calledOnceWithExactly( + { + credentials: { type: 'local', username: 'test', password: '1' }, + user: false, + err: null, + info: undefined + } + ) + }) + }) +}) diff --git a/packages/dd-trace/test/plugins/externals.json b/packages/dd-trace/test/plugins/externals.json index 76df8b8c879..7c3e8175558 100644 --- a/packages/dd-trace/test/plugins/externals.json +++ b/packages/dd-trace/test/plugins/externals.json @@ -166,6 +166,16 @@ "name": "react-dom", "dep": true } + ], + "passport-local": [ + { + "name": "passport-local", + "versions": ["1.0.0"] + }, + { + "name": "passport", + "versions": [">=0.4.1"] + } ], "pg": [ { From 01402c712d69c11a3646b198767109d3b763b26e Mon Sep 17 00:00:00 2001 From: Julio Gonzalez Date: Thu, 1 Jun 2023 14:23:46 +0200 Subject: [PATCH 02/45] Add automated event tracking in appsec. * Config variables. * Track event modifications. * Passport verify channel handler. --- packages/dd-trace/src/appsec/channels.js | 1 + packages/dd-trace/src/appsec/index.js | 22 +++ packages/dd-trace/src/appsec/passport.js | 99 ++++++++++++ .../dd-trace/src/appsec/sdk/track_event.js | 19 ++- packages/dd-trace/src/config.js | 10 +- packages/dd-trace/test/appsec/index.spec.js | 58 ++++++- .../dd-trace/test/appsec/passport.spec.js | 150 ++++++++++++++++++ .../test/appsec/sdk/track_event.spec.js | 42 +++-- 8 files changed, 374 insertions(+), 27 deletions(-) create mode 100644 packages/dd-trace/src/appsec/passport.js create mode 100644 packages/dd-trace/test/appsec/passport.spec.js diff --git a/packages/dd-trace/src/appsec/channels.js b/packages/dd-trace/src/appsec/channels.js index d5aa98792ca..ca3f7b19893 100644 --- a/packages/dd-trace/src/appsec/channels.js +++ b/packages/dd-trace/src/appsec/channels.js @@ -7,5 +7,6 @@ module.exports = { incomingHttpRequestStart: dc.channel('dd-trace:incomingHttpRequestStart'), incomingHttpRequestEnd: dc.channel('dd-trace:incomingHttpRequestEnd'), bodyParser: dc.channel('datadog:body-parser:read:finish'), + passportVerify: dc.channel('datadog:passport:verify:finish'), queryParser: dc.channel('datadog:query:read:finish') } diff --git a/packages/dd-trace/src/appsec/index.js b/packages/dd-trace/src/appsec/index.js index 5112a20ed93..cf85d6bc3e5 100644 --- a/packages/dd-trace/src/appsec/index.js +++ b/packages/dd-trace/src/appsec/index.js @@ -7,6 +7,7 @@ const { incomingHttpRequestStart, incomingHttpRequestEnd, bodyParser, + passportVerify, queryParser } = require('./channels') const waf = require('./waf') @@ -16,6 +17,8 @@ const web = require('../plugins/util/web') const { extractIp } = require('../plugins/util/ip_extractor') const { HTTP_CLIENT_IP } = require('../../../../ext/tags') const { block, setTemplates } = require('./blocking') +const { passportTrackEvent } = require('./passport') +const { storage } = require('../../../datadog-core') let isEnabled = false let config @@ -37,6 +40,12 @@ function enable (_config) { bodyParser.subscribe(onRequestBodyParsed) queryParser.subscribe(onRequestQueryParsed) + if (_config.appsec.eventTracking.enabled) { + passportVerify.subscribe(onPassportVerify) + } else { + log.debug('Undefined eventTracking mode') + } + isEnabled = true config = _config } catch (err) { @@ -139,6 +148,18 @@ function onRequestQueryParsed ({ req, res, abortController }) { handleResults(results, req, res, rootSpan, abortController) } +function onPassportVerify ({ credentials, user, err, info }) { + const store = storage.getStore() + const rootSpan = store && store.req && web.root(store.req) + + if (!rootSpan) { + log.warn('No rootSpan found in onPassportVerifySafe') + return + } + + passportTrackEvent(credentials, user, err, info, rootSpan, config.appsec.eventTracking.mode) +} + function handleResults (actions, req, res, rootSpan, abortController) { if (!actions || !req || !res || !rootSpan || !abortController) return @@ -160,6 +181,7 @@ function disable () { if (incomingHttpRequestEnd.hasSubscribers) incomingHttpRequestEnd.unsubscribe(incomingHttpEndTranslator) if (bodyParser.hasSubscribers) bodyParser.unsubscribe(onRequestBodyParsed) if (queryParser.hasSubscribers) queryParser.unsubscribe(onRequestQueryParsed) + if (passportVerify.hasSubscribers) passportVerify.unsubscribe(onPassportVerify) } module.exports = { diff --git a/packages/dd-trace/src/appsec/passport.js b/packages/dd-trace/src/appsec/passport.js new file mode 100644 index 00000000000..654c1ac0e6f --- /dev/null +++ b/packages/dd-trace/src/appsec/passport.js @@ -0,0 +1,99 @@ +'use strict' + +const log = require('../log') +const { trackEvent } = require('./sdk/track_event') +const { setUserTags } = require('./sdk/set_user') + +const UUID_PATTERN = '^[0-9A-F]{8}-[0-9A-F]{4}-[1-5][0-9A-F]{3}-[89AB][0-9A-F]{3}-[0-9A-F]{12}$' +const regexUsername = new RegExp(UUID_PATTERN, 'i') + +const SDK_EVENT_PATTERN = '_dd\\.appsec\\.events\\.[\\W\\w+]+\\.sdk' +const regexSdkEvent = new RegExp(SDK_EVENT_PATTERN, 'i') + +function isSdkCalled (tags) { + let called = false + + if (tags && typeof tags === 'object') { + Object.entries(tags).forEach(([key, value]) => { + if (regexSdkEvent.test(key) && value === 'true') { + called = true + } + }) + } + + return called +} + +function getLogin (credentials) { + if (credentials && credentials.type === 'local') { + return credentials.username + } +} + +function parseUser (login, passportUser, mode) { + const user = { + 'usr.id': login + } + + if (passportUser) { + // Guess id + if (passportUser.id) { + user['usr.id'] = passportUser.id + } else if (passportUser._id) { + user['usr.id'] = passportUser._id + } else { + user['usr.id'] = login + } + + if (mode === 'safe') { + if (!regexUsername.test(user['usr.id'])) { + user['usr.id'] = '' + } + } else { + if (login) { + user['usr.login'] = login + } + + if (passportUser.email) { + user['usr.email'] = passportUser.email + } + + // Guess username + if (passportUser.username) { + user['usr.username'] = passportUser.username + } else if (passportUser.name) { + user['usr.username'] = passportUser.name + } + } + } + + return user +} + +function passportTrackEvent (credentials, passportUser, passportErr, passportInfo, rootSpan, mode) { + const tags = rootSpan && rootSpan.context() && rootSpan.context()._tags + + if (isSdkCalled(tags)) { + // Don't overwrite tags set by SDK callings + return + } + const user = parseUser(getLogin(credentials), passportUser, mode) + + if (user['usr.id'] === undefined) { + log.warn('No username found in authentication instrumentation') + return + } + + if (passportUser) { + // If a passportUser object is published then the login succeded + setUserTags(user['usr.id'], rootSpan) + trackEvent('users.login.success', user, 'passportTrackEvent', rootSpan, mode) + } else { + // TODO: handle err and info? + trackEvent('users.login.failure', user, 'passportTrackEvent', rootSpan, mode) + } +} + +module.exports = { + passportTrackEvent +} diff --git a/packages/dd-trace/src/appsec/sdk/track_event.js b/packages/dd-trace/src/appsec/sdk/track_event.js index fa1b46b69c5..8debb932090 100644 --- a/packages/dd-trace/src/appsec/sdk/track_event.js +++ b/packages/dd-trace/src/appsec/sdk/track_event.js @@ -20,7 +20,7 @@ function trackUserLoginSuccessEvent (tracer, user, metadata) { setUserTags(user, rootSpan) - trackEvent(tracer, 'users.login.success', metadata, 'trackUserLoginSuccessEvent', rootSpan) + trackEvent('users.login.success', metadata, 'trackUserLoginSuccessEvent', rootSpan, 'sdk') } function trackUserLoginFailureEvent (tracer, userId, exists, metadata) { @@ -35,7 +35,7 @@ function trackUserLoginFailureEvent (tracer, userId, exists, metadata) { ...metadata } - trackEvent(tracer, 'users.login.failure', fields, 'trackUserLoginFailureEvent') + trackEvent('users.login.failure', fields, 'trackUserLoginFailureEvent', getRootSpan(tracer), 'sdk') } function trackCustomEvent (tracer, eventName, metadata) { @@ -44,10 +44,10 @@ function trackCustomEvent (tracer, eventName, metadata) { return } - trackEvent(tracer, eventName, metadata, 'trackCustomEvent') + trackEvent(eventName, metadata, 'trackCustomEvent', getRootSpan(tracer), 'sdk') } -function trackEvent (tracer, eventName, fields, sdkMethodName, rootSpan = getRootSpan(tracer)) { +function trackEvent (eventName, fields, sdkMethodName, rootSpan, mode) { if (!rootSpan) { log.warn(`Root span not available in ${sdkMethodName}`) return @@ -58,6 +58,14 @@ function trackEvent (tracer, eventName, fields, sdkMethodName, rootSpan = getRoo [MANUAL_KEEP]: 'true' } + if (mode === 'sdk') { + tags[`_dd.appsec.events.${eventName}.sdk`] = 'true' + } + + if (mode === 'safe' || mode === 'extended') { + tags[`_dd.appsec.events.${eventName}.auto.mode`] = mode + } + if (fields) { for (const metadataKey of Object.keys(fields)) { tags[`appsec.events.${eventName}.${metadataKey}`] = '' + fields[metadataKey] @@ -70,5 +78,6 @@ function trackEvent (tracer, eventName, fields, sdkMethodName, rootSpan = getRoo module.exports = { trackUserLoginSuccessEvent, trackUserLoginFailureEvent, - trackCustomEvent + trackCustomEvent, + trackEvent } diff --git a/packages/dd-trace/src/config.js b/packages/dd-trace/src/config.js index 8565fd8eaf8..f6e0f5e4652 100644 --- a/packages/dd-trace/src/config.js +++ b/packages/dd-trace/src/config.js @@ -366,6 +366,10 @@ ken|consumer_?(?:id|key|secret)|sign(?:ed|ature)?|auth(?:entication|orization)?) maybeFile(appsec.blockedTemplateJson), maybeFile(process.env.DD_APPSEC_HTTP_BLOCKED_TEMPLATE_JSON) ) + const DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING = coalesce( + process.env.DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING, + 'safe' + ) const remoteConfigOptions = options.remoteConfig || {} const DD_REMOTE_CONFIGURATION_ENABLED = coalesce( @@ -527,7 +531,11 @@ ken|consumer_?(?:id|key|secret)|sign(?:ed|ature)?|auth(?:entication|orization)?) obfuscatorKeyRegex: DD_APPSEC_OBFUSCATION_PARAMETER_KEY_REGEXP, obfuscatorValueRegex: DD_APPSEC_OBFUSCATION_PARAMETER_VALUE_REGEXP, blockedTemplateHtml: DD_APPSEC_HTTP_BLOCKED_TEMPLATE_HTML, - blockedTemplateJson: DD_APPSEC_HTTP_BLOCKED_TEMPLATE_JSON + blockedTemplateJson: DD_APPSEC_HTTP_BLOCKED_TEMPLATE_JSON, + eventTracking: { + enabled: ['extended', 'safe'].some(value => DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING.includes(value)), + mode: DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING + } } this.remoteConfig = { enabled: DD_REMOTE_CONFIGURATION_ENABLED, diff --git a/packages/dd-trace/test/appsec/index.spec.js b/packages/dd-trace/test/appsec/index.spec.js index 2dae641665a..337f9af78ee 100644 --- a/packages/dd-trace/test/appsec/index.spec.js +++ b/packages/dd-trace/test/appsec/index.spec.js @@ -9,7 +9,8 @@ const { incomingHttpRequestStart, incomingHttpRequestEnd, bodyParser, - queryParser + queryParser, + passportVerify } = require('../../src/appsec/channels') const Reporter = require('../../src/appsec/reporter') const agent = require('../plugins/agent') @@ -17,12 +18,14 @@ const Config = require('../../src/config') const axios = require('axios') const getPort = require('get-port') const blockedTemplate = require('../../src/appsec/blocked_templates') +const { storage } = require('../../../datadog-core') describe('AppSec Index', () => { let config let AppSec let web let blocking + let passport const RULES = { rules: [{ a: 1 }] } @@ -36,7 +39,11 @@ describe('AppSec Index', () => { obfuscatorKeyRegex: '.*', obfuscatorValueRegex: '.*', blockedTemplateHtml: blockedTemplate.html, - blockedTemplateJson: blockedTemplate.json + blockedTemplateJson: blockedTemplate.json, + eventTracking: { + enabled: true, + mode: 'safe' + } } } @@ -48,9 +55,14 @@ describe('AppSec Index', () => { setTemplates: sinon.stub() } + passport = { + passportTrackEvent: sinon.stub() + } + AppSec = proxyquire('../../src/appsec', { '../plugins/util/web': web, - './blocking': blocking + './blocking': blocking, + './passport': passport }) sinon.stub(waf, 'init').callThrough() @@ -97,11 +109,13 @@ describe('AppSec Index', () => { it('should subscribe to blockable channels', () => { expect(bodyParser.hasSubscribers).to.be.false expect(queryParser.hasSubscribers).to.be.false + expect(passportVerify.hasSubscribers).to.be.false AppSec.enable(config) expect(bodyParser.hasSubscribers).to.be.true expect(queryParser.hasSubscribers).to.be.true + expect(passportVerify.hasSubscribers).to.be.true }) }) @@ -142,6 +156,7 @@ describe('AppSec Index', () => { expect(bodyParser.hasSubscribers).to.be.false expect(queryParser.hasSubscribers).to.be.false + expect(passportVerify.hasSubscribers).to.be.false }) }) @@ -333,7 +348,7 @@ describe('AppSec Index', () => { }) }) - describe('checkRequestData', () => { + describe('Channel handlers', () => { let abortController, req, res, rootSpan beforeEach(() => { @@ -448,6 +463,41 @@ describe('AppSec Index', () => { expect(res.end).to.have.been.called }) }) + + describe('onPassportVerify', () => { + it('Should call passportTrackEvent', () => { + const credentials = { type: 'local', username: 'test' } + const passportUser = { id: '1234', username: 'Test' } + const passportErr = {} + const passportInfo = {} + + sinon.stub(storage, 'getStore').returns({ req: {} }) + + passportVerify.publish({ credentials, passportUser, passportErr, passportInfo }) + + expect(passport.passportTrackEvent).to.have.been.calledOnceWithExactly( + credentials, + passportUser, + passportErr, + passportInfo, + rootSpan, + config.appsec.eventTracking.mode) + }) + + it('Should call log if no rootSpan is found', () => { + const credentials = { type: 'local', username: 'test' } + const passportUser = { id: '1234', username: 'Test' } + const passportErr = {} + const passportInfo = {} + + sinon.stub(storage, 'getStore').returns(undefined) + sinon.stub(log, 'warn') + + passportVerify.publish({ credentials, passportUser, passportErr, passportInfo }) + + expect(log.warn).to.have.been.calledOnceWithExactly('No rootSpan found in onPassportVerifySafe') + }) + }) }) }) diff --git a/packages/dd-trace/test/appsec/passport.spec.js b/packages/dd-trace/test/appsec/passport.spec.js new file mode 100644 index 00000000000..f25b36016de --- /dev/null +++ b/packages/dd-trace/test/appsec/passport.spec.js @@ -0,0 +1,150 @@ +'use strict' + +const proxyquire = require('proxyquire') + +describe('Passport', () => { + const rootSpan = { + context: () => {} + } + const loginLocal = { type: 'local', username: 'test' } + const userUuid = { + id: '591dc126-8431-4d0f-9509-b23318d3dce4', + email: 'testUser@test.com', + username: 'Test User' + } + + let passportModule, log, events, setUser + beforeEach(() => { + log = { + warn: sinon.stub() + } + + events = { + trackEvent: sinon.stub() + } + + setUser = { + setUserTags: sinon.stub() + } + + passportModule = proxyquire('../../src/appsec/passport', { + '../log': log, + './sdk/track_event': events, + './sdk/set_user': setUser + }) + }) + + describe('passportTrackEvent', () => { + it('should call log when credentials is undefined', () => { + passportModule.passportTrackEvent(undefined, undefined, undefined, undefined, undefined, 'safe') + + expect(log.warn).to.have.been.calledOnceWithExactly('No username found in authentication instrumentation') + }) + + it('should call log when type is not known', () => { + const credentials = { type: 'unknown', username: 'test' } + + passportModule.passportTrackEvent(credentials, undefined, undefined, undefined, undefined, 'safe') + + expect(log.warn).to.have.been.calledOnceWithExactly('No username found in authentication instrumentation') + }) + + it('should call log when type is known but username not present', () => { + const credentials = { type: 'unknown' } + + passportModule.passportTrackEvent(credentials, undefined, undefined, undefined, undefined, 'safe') + + expect(log.warn).to.have.been.calledOnceWithExactly('No username found in authentication instrumentation') + }) + + it('should report login failure when passportUser is not present', () => { + passportModule.passportTrackEvent(loginLocal, undefined, undefined, undefined, undefined, 'safe') + + expect(setUser.setUserTags).not.to.have.been.called + expect(events.trackEvent).to.have.been.calledOnceWithExactly( + 'users.login.failure', + { 'usr.id': loginLocal.username }, + 'passportTrackEvent', + undefined, + 'safe' + ) + }) + + it('should report login success when passportUser is present', () => { + passportModule.passportTrackEvent(loginLocal, userUuid, undefined, undefined, rootSpan, 'safe') + + expect(setUser.setUserTags).to.have.been.calledOnceWithExactly(userUuid.id, rootSpan) + expect(events.trackEvent).to.have.been.calledOnceWithExactly( + 'users.login.success', + { 'usr.id': userUuid.id }, + 'passportTrackEvent', + rootSpan, + 'safe' + ) + }) + + it('should report login success and blank id in safe mode when id is not a uuid', () => { + const user = userUuid + + user.id = 'publicName' + passportModule.passportTrackEvent(loginLocal, user, undefined, undefined, rootSpan, 'safe') + + expect(setUser.setUserTags).to.have.been.calledOnceWithExactly('', rootSpan) + expect(events.trackEvent).to.have.been.calledOnceWithExactly( + 'users.login.success', + { 'usr.id': '' }, + 'passportTrackEvent', + rootSpan, + 'safe' + ) + }) + + it('should report login success and the extended fields in extended mode', () => { + const user = userUuid + + user.id = 'publicName' + passportModule.passportTrackEvent(loginLocal, user, undefined, undefined, rootSpan, 'extended') + expect(setUser.setUserTags).to.have.been.calledOnceWithExactly(user.id, rootSpan) + expect(events.trackEvent).to.have.been.calledOnceWithExactly( + 'users.login.success', + { + 'usr.id': user.id, + 'usr.email': user.email, + 'usr.username': user.username, + 'usr.login': loginLocal.username + }, + 'passportTrackEvent', + rootSpan, + 'extended' + ) + }) + + it('should not call trackEvent in safe mode if sdk track event functions are already called', () => { + rootSpan.context = () => { + return { + _tags: { + '_dd.appsec.events.users.login.success.sdk': 'true' + } + } + } + + passportModule.passportTrackEvent(loginLocal, userUuid, undefined, undefined, rootSpan, 'safe') + expect(setUser.setUserTags).not.to.have.been.called + expect(events.trackEvent).not.to.have.been.called + }) + + it('should not call trackEvent in extended mode if sdk track event functions are already called', () => { + rootSpan.context = () => { + return { + _tags: { + '_dd.appsec.events.users.login.success.sdk': 'true' + } + } + } + + passportModule.passportTrackEvent(loginLocal, userUuid, undefined, undefined, rootSpan, 'extended') + expect(setUser.setUserTags).not.to.have.been.called + expect(events.trackEvent).not.to.have.been.called + }) + }) +}) diff --git a/packages/dd-trace/test/appsec/sdk/track_event.spec.js b/packages/dd-trace/test/appsec/sdk/track_event.spec.js index d9b4059b8c1..d29ae99c8ac 100644 --- a/packages/dd-trace/test/appsec/sdk/track_event.spec.js +++ b/packages/dd-trace/test/appsec/sdk/track_event.spec.js @@ -76,13 +76,15 @@ describe('track_event', () => { expect(log.warn).to.not.have.been.called expect(setUserTags).to.have.been.calledOnceWithExactly(user, rootSpan) - expect(rootSpan.addTags).to.have.been.calledOnceWithExactly({ - 'appsec.events.users.login.success.track': 'true', - 'appsec.events.users.login.success.metakey1': 'metaValue1', - 'appsec.events.users.login.success.metakey2': 'metaValue2', - 'appsec.events.users.login.success.metakey3': 'metaValue3', - 'manual.keep': 'true' - }) + expect(rootSpan.addTags).to.have.been.calledOnceWithExactly( + { + 'appsec.events.users.login.success.track': 'true', + 'manual.keep': 'true', + '_dd.appsec.events.users.login.success.sdk': 'true', + 'appsec.events.users.login.success.metakey1': 'metaValue1', + 'appsec.events.users.login.success.metakey2': 'metaValue2', + 'appsec.events.users.login.success.metakey3': 'metaValue3' + }) }) it('should call setUser and addTags without metadata', () => { @@ -94,7 +96,8 @@ describe('track_event', () => { expect(setUserTags).to.have.been.calledOnceWithExactly(user, rootSpan) expect(rootSpan.addTags).to.have.been.calledOnceWithExactly({ 'appsec.events.users.login.success.track': 'true', - 'manual.keep': 'true' + 'manual.keep': 'true', + '_dd.appsec.events.users.login.success.sdk': 'true' }) }) }) @@ -131,12 +134,13 @@ describe('track_event', () => { expect(setUserTags).to.not.have.been.called expect(rootSpan.addTags).to.have.been.calledOnceWithExactly({ 'appsec.events.users.login.failure.track': 'true', + 'manual.keep': 'true', + '_dd.appsec.events.users.login.failure.sdk': 'true', 'appsec.events.users.login.failure.usr.id': 'user_id', 'appsec.events.users.login.failure.usr.exists': 'true', 'appsec.events.users.login.failure.metakey1': 'metaValue1', 'appsec.events.users.login.failure.metakey2': 'metaValue2', - 'appsec.events.users.login.failure.metakey3': 'metaValue3', - 'manual.keep': 'true' + 'appsec.events.users.login.failure.metakey3': 'metaValue3' }) }) @@ -149,12 +153,13 @@ describe('track_event', () => { expect(setUserTags).to.not.have.been.called expect(rootSpan.addTags).to.have.been.calledOnceWithExactly({ 'appsec.events.users.login.failure.track': 'true', + 'manual.keep': 'true', + '_dd.appsec.events.users.login.failure.sdk': 'true', 'appsec.events.users.login.failure.usr.id': 'user_id', 'appsec.events.users.login.failure.usr.exists': 'false', 'appsec.events.users.login.failure.metakey1': 'metaValue1', 'appsec.events.users.login.failure.metakey2': 'metaValue2', - 'appsec.events.users.login.failure.metakey3': 'metaValue3', - 'manual.keep': 'true' + 'appsec.events.users.login.failure.metakey3': 'metaValue3' }) }) @@ -165,9 +170,10 @@ describe('track_event', () => { expect(setUserTags).to.not.have.been.called expect(rootSpan.addTags).to.have.been.calledOnceWithExactly({ 'appsec.events.users.login.failure.track': 'true', + 'manual.keep': 'true', + '_dd.appsec.events.users.login.failure.sdk': 'true', 'appsec.events.users.login.failure.usr.id': 'user_id', - 'appsec.events.users.login.failure.usr.exists': 'true', - 'manual.keep': 'true' + 'appsec.events.users.login.failure.usr.exists': 'true' }) }) }) @@ -200,9 +206,10 @@ describe('track_event', () => { expect(setUserTags).to.not.have.been.called expect(rootSpan.addTags).to.have.been.calledOnceWithExactly({ 'appsec.events.custom_event.track': 'true', + 'manual.keep': 'true', + '_dd.appsec.events.custom_event.sdk': 'true', 'appsec.events.custom_event.metaKey1': 'metaValue1', - 'appsec.events.custom_event.metakey2': 'metaValue2', - 'manual.keep': 'true' + 'appsec.events.custom_event.metakey2': 'metaValue2' }) }) @@ -213,7 +220,8 @@ describe('track_event', () => { expect(setUserTags).to.not.have.been.called expect(rootSpan.addTags).to.have.been.calledOnceWithExactly({ 'appsec.events.custom_event.track': 'true', - 'manual.keep': 'true' + 'manual.keep': 'true', + '_dd.appsec.events.custom_event.sdk': 'true' }) }) }) From 36644b311c88e564055db31e816281b688fd1391 Mon Sep 17 00:00:00 2001 From: Julio Gonzalez Date: Thu, 1 Jun 2023 15:07:52 +0200 Subject: [PATCH 03/45] Fix tests. --- packages/dd-trace/test/appsec/index.spec.js | 26 +++++++++++++-------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/packages/dd-trace/test/appsec/index.spec.js b/packages/dd-trace/test/appsec/index.spec.js index 337f9af78ee..b6a0129de23 100644 --- a/packages/dd-trace/test/appsec/index.spec.js +++ b/packages/dd-trace/test/appsec/index.spec.js @@ -1,7 +1,6 @@ 'use strict' const proxyquire = require('proxyquire') -const log = require('../../src/log') const waf = require('../../src/appsec/waf') const RuleManager = require('../../src/appsec/rule_manager') const appsec = require('../../src/appsec') @@ -26,6 +25,7 @@ describe('AppSec Index', () => { let web let blocking let passport + let log const RULES = { rules: [{ a: 1 }] } @@ -59,7 +59,14 @@ describe('AppSec Index', () => { passportTrackEvent: sinon.stub() } + log = { + debug: sinon.stub(), + warn: sinon.stub(), + error: sinon.stub() + } + AppSec = proxyquire('../../src/appsec', { + '../log': log, '../plugins/util/web': web, './blocking': blocking, './passport': passport @@ -91,7 +98,7 @@ describe('AppSec Index', () => { }) it('should log when enable fails', () => { - sinon.stub(log, 'error') + // sinon.stub(log, 'error') RuleManager.applyRules.restore() const err = new Error('Invalid Rules') @@ -467,19 +474,19 @@ describe('AppSec Index', () => { describe('onPassportVerify', () => { it('Should call passportTrackEvent', () => { const credentials = { type: 'local', username: 'test' } - const passportUser = { id: '1234', username: 'Test' } - const passportErr = {} - const passportInfo = {} + const user = { id: '1234', username: 'Test' } + const err = {} + const info = {} sinon.stub(storage, 'getStore').returns({ req: {} }) - passportVerify.publish({ credentials, passportUser, passportErr, passportInfo }) + passportVerify.publish({ credentials, user, err, info }) expect(passport.passportTrackEvent).to.have.been.calledOnceWithExactly( credentials, - passportUser, - passportErr, - passportInfo, + user, + err, + info, rootSpan, config.appsec.eventTracking.mode) }) @@ -491,7 +498,6 @@ describe('AppSec Index', () => { const passportInfo = {} sinon.stub(storage, 'getStore').returns(undefined) - sinon.stub(log, 'warn') passportVerify.publish({ credentials, passportUser, passportErr, passportInfo }) From e2972c0afe6af5e487a20cff4b4ac44c4629f4ed Mon Sep 17 00:00:00 2001 From: Julio Gonzalez Date: Thu, 1 Jun 2023 15:38:45 +0200 Subject: [PATCH 04/45] Fix config tests. --- packages/dd-trace/test/config.spec.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/dd-trace/test/config.spec.js b/packages/dd-trace/test/config.spec.js index 1818b105d14..4e4ea3d00d3 100644 --- a/packages/dd-trace/test/config.spec.js +++ b/packages/dd-trace/test/config.spec.js @@ -709,7 +709,11 @@ describe('Config', () => { obfuscatorKeyRegex: '.*', obfuscatorValueRegex: '.*', blockedTemplateHtml: undefined, - blockedTemplateJson: undefined + blockedTemplateJson: undefined, + eventTracking: { + enabled: true, + mode: 'safe' + } }) }) From 6a581a109d07e0bf7b7f9d79ed360a9f47dae5b7 Mon Sep 17 00:00:00 2001 From: Julio Gonzalez Date: Thu, 1 Jun 2023 16:29:37 +0200 Subject: [PATCH 05/45] Improve code coverage. --- packages/dd-trace/src/appsec/index.js | 2 - .../dd-trace/test/appsec/passport.spec.js | 51 ++++++++++++++++++- .../test/appsec/sdk/track_event.spec.js | 35 +++++++++++-- 3 files changed, 80 insertions(+), 8 deletions(-) diff --git a/packages/dd-trace/src/appsec/index.js b/packages/dd-trace/src/appsec/index.js index cf85d6bc3e5..787ccdb2e81 100644 --- a/packages/dd-trace/src/appsec/index.js +++ b/packages/dd-trace/src/appsec/index.js @@ -42,8 +42,6 @@ function enable (_config) { if (_config.appsec.eventTracking.enabled) { passportVerify.subscribe(onPassportVerify) - } else { - log.debug('Undefined eventTracking mode') } isEnabled = true diff --git a/packages/dd-trace/test/appsec/passport.spec.js b/packages/dd-trace/test/appsec/passport.spec.js index f25b36016de..cad507def2a 100644 --- a/packages/dd-trace/test/appsec/passport.spec.js +++ b/packages/dd-trace/test/appsec/passport.spec.js @@ -4,7 +4,7 @@ const proxyquire = require('proxyquire') describe('Passport', () => { const rootSpan = { - context: () => {} + context: () => { return {} } } const loginLocal = { type: 'local', username: 'test' } const userUuid = { @@ -146,5 +146,54 @@ describe('Passport', () => { expect(setUser.setUserTags).not.to.have.been.called expect(events.trackEvent).not.to.have.been.called }) + + it('should report login success with the _id field', () => { + const user = { + _id: '591dc126-8431-4d0f-9509-b23318d3dce4', + email: 'testUser@test.com', + username: 'Test User' + } + + rootSpan.context = () => { return {} } + + passportModule.passportTrackEvent(loginLocal, user, undefined, undefined, rootSpan, 'extended') + expect(setUser.setUserTags).to.have.been.calledOnceWithExactly(user._id, rootSpan) + expect(events.trackEvent).to.have.been.calledOnceWithExactly( + 'users.login.success', + { + 'usr.id': user._id, + 'usr.email': user.email, + 'usr.username': user.username, + 'usr.login': loginLocal.username + }, + 'passportTrackEvent', + rootSpan, + 'extended' + ) + }) + + it('should report login success with the username field passport name', () => { + const user = { + email: 'testUser@test.com', + name: 'Test User' + } + + rootSpan.context = () => { return {} } + + passportModule.passportTrackEvent(loginLocal, user, undefined, undefined, rootSpan, 'extended') + expect(setUser.setUserTags).to.have.been.calledOnceWithExactly(loginLocal.username, rootSpan) + expect(events.trackEvent).to.have.been.calledOnceWithExactly( + 'users.login.success', + { + 'usr.id': loginLocal.username, + 'usr.email': user.email, + 'usr.username': user.name, + 'usr.login': loginLocal.username + }, + 'passportTrackEvent', + rootSpan, + 'extended' + ) + }) }) }) diff --git a/packages/dd-trace/test/appsec/sdk/track_event.spec.js b/packages/dd-trace/test/appsec/sdk/track_event.spec.js index d29ae99c8ac..93d8959783e 100644 --- a/packages/dd-trace/test/appsec/sdk/track_event.spec.js +++ b/packages/dd-trace/test/appsec/sdk/track_event.spec.js @@ -13,7 +13,7 @@ describe('track_event', () => { let rootSpan let getRootSpan let setUserTags - let trackUserLoginSuccessEvent, trackUserLoginFailureEvent, trackCustomEvent + let trackUserLoginSuccessEvent, trackUserLoginFailureEvent, trackCustomEvent, trackEvent beforeEach(() => { log = { @@ -28,7 +28,7 @@ describe('track_event', () => { setUserTags = sinon.stub() - const trackEvent = proxyquire('../../../src/appsec/sdk/track_event', { + const trackEvents = proxyquire('../../../src/appsec/sdk/track_event', { '../../log': log, './utils': { getRootSpan @@ -38,9 +38,10 @@ describe('track_event', () => { } }) - trackUserLoginSuccessEvent = trackEvent.trackUserLoginSuccessEvent - trackUserLoginFailureEvent = trackEvent.trackUserLoginFailureEvent - trackCustomEvent = trackEvent.trackCustomEvent + trackUserLoginSuccessEvent = trackEvents.trackUserLoginSuccessEvent + trackUserLoginFailureEvent = trackEvents.trackUserLoginFailureEvent + trackCustomEvent = trackEvents.trackCustomEvent + trackEvent = trackEvents.trackEvent }) describe('trackUserLoginSuccessEvent', () => { @@ -225,6 +226,30 @@ describe('track_event', () => { }) }) }) + + describe('trackEvent', () => { + it('should call addTags with safe mode', () => { + trackEvent('event', { metaKey1: 'metaValue1', metakey2: 'metaValue2' }, 'trackEvent', rootSpan, 'safe') + expect(rootSpan.addTags).to.have.been.calledOnceWithExactly({ + 'appsec.events.event.track': 'true', + 'manual.keep': 'true', + '_dd.appsec.events.event.auto.mode': 'safe', + 'appsec.events.event.metaKey1': 'metaValue1', + 'appsec.events.event.metakey2': 'metaValue2' + }) + }) + + it('should call addTags with extended mode', () => { + trackEvent('event', { metaKey1: 'metaValue1', metakey2: 'metaValue2' }, 'trackEvent', rootSpan, 'extended') + expect(rootSpan.addTags).to.have.been.calledOnceWithExactly({ + 'appsec.events.event.track': 'true', + 'manual.keep': 'true', + '_dd.appsec.events.event.auto.mode': 'extended', + 'appsec.events.event.metaKey1': 'metaValue1', + 'appsec.events.event.metakey2': 'metaValue2' + }) + }) + }) }) describe('Integration with the tracer', () => { From 1595f8b0e3904076a0536587ca048064ba3424c6 Mon Sep 17 00:00:00 2001 From: Julio Gonzalez Date: Fri, 2 Jun 2023 18:04:16 +0200 Subject: [PATCH 06/45] Fix PR comments. --- .../src/passport-http.js | 13 +-- .../src/passport-local.js | 15 ++-- .../test/passport-http.spec.js | 8 +- .../test/passport-local.spec.js | 8 +- packages/dd-trace/src/appsec/index.js | 4 +- packages/dd-trace/src/appsec/passport.js | 20 ++--- packages/dd-trace/src/config.js | 5 +- packages/dd-trace/test/appsec/index.spec.js | 3 - .../dd-trace/test/appsec/passport.spec.js | 80 +++++++++++++------ 9 files changed, 89 insertions(+), 67 deletions(-) diff --git a/packages/datadog-instrumentations/src/passport-http.js b/packages/datadog-instrumentations/src/passport-http.js index 5fff0efeedc..a1032ca9184 100644 --- a/packages/datadog-instrumentations/src/passport-http.js +++ b/packages/datadog-instrumentations/src/passport-http.js @@ -7,12 +7,13 @@ const passportVerifyChannel = channel('datadog:passport:verify:finish') function wrapVerifiedAndPublish (username, password, verified) { if (passportVerifyChannel.hasSubscribers) { - return shimmer.wrap(verified, (err, user, info) => { - // TODO: check if err and info are really useful + return shimmer.wrap(verified, function (err, user, info) { const credentials = { type: 'http', username: username, password: password } - passportVerifyChannel.publish({ credentials, user, err, info }) - return verified(err, user, info) + passportVerifyChannel.publish({ credentials, user }) + return verified.call(this, err, user, info) }) + } else { + return verified } } @@ -39,8 +40,8 @@ addHook({ if (typeof arguments[0] === 'function') { arguments[0] = wrapVerify(arguments[0], false) } else { - arguments[1] = wrapVerify(arguments[1], arguments[0].passReqToCallback || false) + arguments[1] = wrapVerify(arguments[1], (arguments[0] && arguments[0].passReqToCallback) || false) } - BasicStrategy.apply(this, arguments) + return BasicStrategy.apply(this, arguments) }) }) diff --git a/packages/datadog-instrumentations/src/passport-local.js b/packages/datadog-instrumentations/src/passport-local.js index 873db4ca692..dc67492f2ca 100644 --- a/packages/datadog-instrumentations/src/passport-local.js +++ b/packages/datadog-instrumentations/src/passport-local.js @@ -7,12 +7,13 @@ const passportVerifyChannel = channel('datadog:passport:verify:finish') function wrapVerifiedAndPublish (username, password, verified) { if (passportVerifyChannel.hasSubscribers) { - return shimmer.wrap(verified, (err, user, info) => { - // TODO: check if err and info are really useful - const credentials = { type: 'local', username: username, password: password } - passportVerifyChannel.publish({ credentials, user, err, info }) - return verified(err, user, info) + return shimmer.wrap(verified, function (err, user, info) { + const credentials = { type: 'local', username, password } + passportVerifyChannel.publish({ credentials, user }) + return verified.call(this, err, user, info) }) + } else { + return verified } } @@ -39,8 +40,8 @@ addHook({ if (typeof arguments[0] === 'function') { arguments[0] = wrapVerify(arguments[0], false) } else { - arguments[1] = wrapVerify(arguments[1], arguments[0].passReqToCallback || false) + arguments[1] = wrapVerify(arguments[1], (arguments[0] && arguments[0].passReqToCallback) || false) } - Strategy.apply(this, arguments) + return Strategy.apply(this, arguments) }) }) diff --git a/packages/datadog-instrumentations/test/passport-http.spec.js b/packages/datadog-instrumentations/test/passport-http.spec.js index f2322fd5945..53dcfbe9ecb 100644 --- a/packages/datadog-instrumentations/test/passport-http.spec.js +++ b/packages/datadog-instrumentations/test/passport-http.spec.js @@ -89,9 +89,7 @@ withVersions('passport-http', 'passport-http', version => { expect(subscriberStub).to.be.calledOnceWithExactly( { credentials: { type: 'http', username: 'test', password: '1234' }, - user: { _id: 1, username: 'test', password: '1234', email: 'testuser@ddog.com' }, - err: null, - info: undefined + user: { _id: 1, username: 'test', password: '1234', email: 'testuser@ddog.com' } } ) }) @@ -109,9 +107,7 @@ withVersions('passport-http', 'passport-http', version => { expect(subscriberStub).to.be.calledOnceWithExactly( { credentials: { type: 'http', username: 'test', password: '1' }, - user: false, - err: null, - info: undefined + user: false } ) }) diff --git a/packages/datadog-instrumentations/test/passport-local.spec.js b/packages/datadog-instrumentations/test/passport-local.spec.js index f80db29dd5b..1a5e1e8a6b3 100644 --- a/packages/datadog-instrumentations/test/passport-local.spec.js +++ b/packages/datadog-instrumentations/test/passport-local.spec.js @@ -85,9 +85,7 @@ withVersions('passport-local', 'passport-local', version => { expect(subscriberStub).to.be.calledOnceWithExactly( { credentials: { type: 'local', username: 'test', password: '1234' }, - user: { _id: 1, username: 'test', password: '1234', email: 'testuser@ddog.com' }, - err: null, - info: undefined + user: { _id: 1, username: 'test', password: '1234', email: 'testuser@ddog.com' } } ) }) @@ -100,9 +98,7 @@ withVersions('passport-local', 'passport-local', version => { expect(subscriberStub).to.be.calledOnceWithExactly( { credentials: { type: 'local', username: 'test', password: '1' }, - user: false, - err: null, - info: undefined + user: false } ) }) diff --git a/packages/dd-trace/src/appsec/index.js b/packages/dd-trace/src/appsec/index.js index 787ccdb2e81..974d4840c54 100644 --- a/packages/dd-trace/src/appsec/index.js +++ b/packages/dd-trace/src/appsec/index.js @@ -146,7 +146,7 @@ function onRequestQueryParsed ({ req, res, abortController }) { handleResults(results, req, res, rootSpan, abortController) } -function onPassportVerify ({ credentials, user, err, info }) { +function onPassportVerify ({ credentials, user }) { const store = storage.getStore() const rootSpan = store && store.req && web.root(store.req) @@ -155,7 +155,7 @@ function onPassportVerify ({ credentials, user, err, info }) { return } - passportTrackEvent(credentials, user, err, info, rootSpan, config.appsec.eventTracking.mode) + passportTrackEvent(credentials, user, rootSpan, config.appsec.eventTracking.mode) } function handleResults (actions, req, res, rootSpan, abortController) { diff --git a/packages/dd-trace/src/appsec/passport.js b/packages/dd-trace/src/appsec/passport.js index 654c1ac0e6f..f8a7fbc353c 100644 --- a/packages/dd-trace/src/appsec/passport.js +++ b/packages/dd-trace/src/appsec/passport.js @@ -7,18 +7,14 @@ const { setUserTags } = require('./sdk/set_user') const UUID_PATTERN = '^[0-9A-F]{8}-[0-9A-F]{4}-[1-5][0-9A-F]{3}-[89AB][0-9A-F]{3}-[0-9A-F]{12}$' const regexUsername = new RegExp(UUID_PATTERN, 'i') -const SDK_EVENT_PATTERN = '_dd\\.appsec\\.events\\.[\\W\\w+]+\\.sdk' -const regexSdkEvent = new RegExp(SDK_EVENT_PATTERN, 'i') +const SDK_USER_EVENT_PATTERN = '_dd\\.appsec\\.events\\.users\\.[\\W\\w+]+\\.sdk' +const regexSdkEvent = new RegExp(SDK_USER_EVENT_PATTERN, 'i') function isSdkCalled (tags) { let called = false if (tags && typeof tags === 'object') { - Object.entries(tags).forEach(([key, value]) => { - if (regexSdkEvent.test(key) && value === 'true') { - called = true - } - }) + called = Object.entries(tags).some(([key, value]) => regexSdkEvent.test(key) && value === 'true') } return called @@ -41,8 +37,6 @@ function parseUser (login, passportUser, mode) { user['usr.id'] = passportUser.id } else if (passportUser._id) { user['usr.id'] = passportUser._id - } else { - user['usr.id'] = login } if (mode === 'safe') { @@ -70,7 +64,7 @@ function parseUser (login, passportUser, mode) { return user } -function passportTrackEvent (credentials, passportUser, passportErr, passportInfo, rootSpan, mode) { +function passportTrackEvent (credentials, passportUser, rootSpan, mode) { const tags = rootSpan && rootSpan.context() && rootSpan.context()._tags if (isSdkCalled(tags)) { @@ -86,10 +80,12 @@ function passportTrackEvent (credentials, passportUser, passportErr, passportInf if (passportUser) { // If a passportUser object is published then the login succeded - setUserTags(user['usr.id'], rootSpan) + // TODO : test + setUserTags({ id: user['usr.id'] }, rootSpan) + // Prevent 'usr.id' from being reported again in the metadata + delete user['usr.id'] trackEvent('users.login.success', user, 'passportTrackEvent', rootSpan, mode) } else { - // TODO: handle err and info? trackEvent('users.login.failure', user, 'passportTrackEvent', rootSpan, mode) } } diff --git a/packages/dd-trace/src/config.js b/packages/dd-trace/src/config.js index f6e0f5e4652..358dd4c69c7 100644 --- a/packages/dd-trace/src/config.js +++ b/packages/dd-trace/src/config.js @@ -367,9 +367,10 @@ ken|consumer_?(?:id|key|secret)|sign(?:ed|ature)?|auth(?:entication|orization)?) maybeFile(process.env.DD_APPSEC_HTTP_BLOCKED_TEMPLATE_JSON) ) const DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING = coalesce( + appsec.eventTracking && appsec.eventTracking.mode, process.env.DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING, 'safe' - ) + ).toLowerCase() const remoteConfigOptions = options.remoteConfig || {} const DD_REMOTE_CONFIGURATION_ENABLED = coalesce( @@ -533,7 +534,7 @@ ken|consumer_?(?:id|key|secret)|sign(?:ed|ature)?|auth(?:entication|orization)?) blockedTemplateHtml: DD_APPSEC_HTTP_BLOCKED_TEMPLATE_HTML, blockedTemplateJson: DD_APPSEC_HTTP_BLOCKED_TEMPLATE_JSON, eventTracking: { - enabled: ['extended', 'safe'].some(value => DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING.includes(value)), + enabled: ['extended', 'safe'].some(value => DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING === value), mode: DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING } } diff --git a/packages/dd-trace/test/appsec/index.spec.js b/packages/dd-trace/test/appsec/index.spec.js index ebcddc3fdc2..a40ba2d9d05 100644 --- a/packages/dd-trace/test/appsec/index.spec.js +++ b/packages/dd-trace/test/appsec/index.spec.js @@ -98,7 +98,6 @@ describe('AppSec Index', () => { }) it('should log when enable fails', () => { - // sinon.stub(log, 'error') RuleManager.applyRules.restore() const err = new Error('Invalid Rules') @@ -485,8 +484,6 @@ describe('AppSec Index', () => { expect(passport.passportTrackEvent).to.have.been.calledOnceWithExactly( credentials, user, - err, - info, rootSpan, config.appsec.eventTracking.mode) }) diff --git a/packages/dd-trace/test/appsec/passport.spec.js b/packages/dd-trace/test/appsec/passport.spec.js index cad507def2a..879eff7add8 100644 --- a/packages/dd-trace/test/appsec/passport.spec.js +++ b/packages/dd-trace/test/appsec/passport.spec.js @@ -36,7 +36,7 @@ describe('Passport', () => { describe('passportTrackEvent', () => { it('should call log when credentials is undefined', () => { - passportModule.passportTrackEvent(undefined, undefined, undefined, undefined, undefined, 'safe') + passportModule.passportTrackEvent(undefined, undefined, undefined, 'safe') expect(log.warn).to.have.been.calledOnceWithExactly('No username found in authentication instrumentation') }) @@ -44,7 +44,7 @@ describe('Passport', () => { it('should call log when type is not known', () => { const credentials = { type: 'unknown', username: 'test' } - passportModule.passportTrackEvent(credentials, undefined, undefined, undefined, undefined, 'safe') + passportModule.passportTrackEvent(credentials, undefined, undefined, 'safe') expect(log.warn).to.have.been.calledOnceWithExactly('No username found in authentication instrumentation') }) @@ -52,13 +52,13 @@ describe('Passport', () => { it('should call log when type is known but username not present', () => { const credentials = { type: 'unknown' } - passportModule.passportTrackEvent(credentials, undefined, undefined, undefined, undefined, 'safe') + passportModule.passportTrackEvent(credentials, undefined, undefined, 'safe') expect(log.warn).to.have.been.calledOnceWithExactly('No username found in authentication instrumentation') }) it('should report login failure when passportUser is not present', () => { - passportModule.passportTrackEvent(loginLocal, undefined, undefined, undefined, undefined, 'safe') + passportModule.passportTrackEvent(loginLocal, undefined, undefined, 'safe') expect(setUser.setUserTags).not.to.have.been.called expect(events.trackEvent).to.have.been.calledOnceWithExactly( @@ -71,12 +71,12 @@ describe('Passport', () => { }) it('should report login success when passportUser is present', () => { - passportModule.passportTrackEvent(loginLocal, userUuid, undefined, undefined, rootSpan, 'safe') + passportModule.passportTrackEvent(loginLocal, userUuid, rootSpan, 'safe') - expect(setUser.setUserTags).to.have.been.calledOnceWithExactly(userUuid.id, rootSpan) + expect(setUser.setUserTags).to.have.been.calledOnceWithExactly({ id: userUuid.id }, rootSpan) expect(events.trackEvent).to.have.been.calledOnceWithExactly( 'users.login.success', - { 'usr.id': userUuid.id }, + {}, 'passportTrackEvent', rootSpan, 'safe' @@ -87,12 +87,12 @@ describe('Passport', () => { const user = userUuid user.id = 'publicName' - passportModule.passportTrackEvent(loginLocal, user, undefined, undefined, rootSpan, 'safe') + passportModule.passportTrackEvent(loginLocal, user, rootSpan, 'safe') - expect(setUser.setUserTags).to.have.been.calledOnceWithExactly('', rootSpan) + expect(setUser.setUserTags).to.have.been.calledOnceWithExactly({ id: '' }, rootSpan) expect(events.trackEvent).to.have.been.calledOnceWithExactly( 'users.login.success', - { 'usr.id': '' }, + {}, 'passportTrackEvent', rootSpan, 'safe' @@ -103,12 +103,11 @@ describe('Passport', () => { const user = userUuid user.id = 'publicName' - passportModule.passportTrackEvent(loginLocal, user, undefined, undefined, rootSpan, 'extended') - expect(setUser.setUserTags).to.have.been.calledOnceWithExactly(user.id, rootSpan) + passportModule.passportTrackEvent(loginLocal, user, rootSpan, 'extended') + expect(setUser.setUserTags).to.have.been.calledOnceWithExactly({ id: user.id }, rootSpan) expect(events.trackEvent).to.have.been.calledOnceWithExactly( 'users.login.success', { - 'usr.id': user.id, 'usr.email': user.email, 'usr.username': user.username, 'usr.login': loginLocal.username @@ -119,7 +118,7 @@ describe('Passport', () => { ) }) - it('should not call trackEvent in safe mode if sdk track event functions are already called', () => { + it('should not call trackEvent in safe mode if sdk user event functions are already called', () => { rootSpan.context = () => { return { _tags: { @@ -128,12 +127,12 @@ describe('Passport', () => { } } - passportModule.passportTrackEvent(loginLocal, userUuid, undefined, undefined, rootSpan, 'safe') + passportModule.passportTrackEvent(loginLocal, userUuid, rootSpan, 'safe') expect(setUser.setUserTags).not.to.have.been.called expect(events.trackEvent).not.to.have.been.called }) - it('should not call trackEvent in extended mode if sdk track event functions are already called', () => { + it('should not call trackEvent in extended mode if sdk user event functions are already called', () => { rootSpan.context = () => { return { _tags: { @@ -142,7 +141,44 @@ describe('Passport', () => { } } - passportModule.passportTrackEvent(loginLocal, userUuid, undefined, undefined, rootSpan, 'extended') + passportModule.passportTrackEvent(loginLocal, userUuid, rootSpan, 'extended') + expect(setUser.setUserTags).not.to.have.been.called + expect(events.trackEvent).not.to.have.been.called + }) + + it('should call trackEvent in extended mode if sdk custom event function is already called', () => { + rootSpan.context = () => { + return { + _tags: { + '_dd.appsec.events.custom.event.sdk': 'true' + } + } + } + + passportModule.passportTrackEvent(loginLocal, userUuid, rootSpan, 'extended') + expect(events.trackEvent).to.have.been.calledOnceWithExactly( + 'users.login.success', + { + 'usr.email': userUuid.email, + 'usr.username': userUuid.username, + 'usr.login': loginLocal.username + }, + 'passportTrackEvent', + rootSpan, + 'extended' + ) + }) + + it('should call trackEvent in extended mode if sdk dy called', () => { + rootSpan.context = () => { + return { + _tags: { + '_dd.appsec.events.users.login.success.sdk': 'true' + } + } + } + + passportModule.passportTrackEvent(loginLocal, userUuid, rootSpan, 'extended') expect(setUser.setUserTags).not.to.have.been.called expect(events.trackEvent).not.to.have.been.called }) @@ -156,12 +192,11 @@ describe('Passport', () => { rootSpan.context = () => { return {} } - passportModule.passportTrackEvent(loginLocal, user, undefined, undefined, rootSpan, 'extended') - expect(setUser.setUserTags).to.have.been.calledOnceWithExactly(user._id, rootSpan) + passportModule.passportTrackEvent(loginLocal, user, rootSpan, 'extended') + expect(setUser.setUserTags).to.have.been.calledOnceWithExactly({ id: user._id }, rootSpan) expect(events.trackEvent).to.have.been.calledOnceWithExactly( 'users.login.success', { - 'usr.id': user._id, 'usr.email': user.email, 'usr.username': user.username, 'usr.login': loginLocal.username @@ -180,12 +215,11 @@ describe('Passport', () => { rootSpan.context = () => { return {} } - passportModule.passportTrackEvent(loginLocal, user, undefined, undefined, rootSpan, 'extended') - expect(setUser.setUserTags).to.have.been.calledOnceWithExactly(loginLocal.username, rootSpan) + passportModule.passportTrackEvent(loginLocal, user, rootSpan, 'extended') + expect(setUser.setUserTags).to.have.been.calledOnceWithExactly({ id: loginLocal.username }, rootSpan) expect(events.trackEvent).to.have.been.calledOnceWithExactly( 'users.login.success', { - 'usr.id': loginLocal.username, 'usr.email': user.email, 'usr.username': user.name, 'usr.login': loginLocal.username From f7338c88ff7620f1ab7e372ddd64b37a193a14a1 Mon Sep 17 00:00:00 2001 From: Julio Gonzalez Date: Tue, 6 Jun 2023 09:39:09 +0200 Subject: [PATCH 07/45] Update index.d.ts and test.ts. --- docs/test.ts | 6 +++++- index.d.ts | 16 ++++++++++++++++ packages/dd-trace/src/appsec/passport.js | 3 +-- 3 files changed, 22 insertions(+), 3 deletions(-) diff --git a/docs/test.ts b/docs/test.ts index fee774ac14c..a01e2847095 100644 --- a/docs/test.ts +++ b/docs/test.ts @@ -105,7 +105,11 @@ tracer.init({ obfuscatorKeyRegex: '.*', obfuscatorValueRegex: '.*', blockedTemplateHtml: './blocked.html', - blockedTemplateJson: './blocked.json' + blockedTemplateJson: './blocked.json', + eventTracking: { + enabled: true, + mode: 'safe' + } } }); diff --git a/index.d.ts b/index.d.ts index b96494ec011..49945926c5d 100644 --- a/index.d.ts +++ b/index.d.ts @@ -553,6 +553,22 @@ export declare interface TracerOptions { * Specifies a path to a custom blocking template json file. */ blockedTemplateJson?: string, + + /** + * Controls the automated user event tracking configuration + */ + eventTracking?: { + /** + * Whether to enable automated user event tracking. + * @default true + */ + enabled?: boolean, + /** + * Constrols the automated user event tracking mode + * @default 'safe' + */ + mode?: string + } }; /** diff --git a/packages/dd-trace/src/appsec/passport.js b/packages/dd-trace/src/appsec/passport.js index f8a7fbc353c..d1e2f2a1bc7 100644 --- a/packages/dd-trace/src/appsec/passport.js +++ b/packages/dd-trace/src/appsec/passport.js @@ -32,7 +32,7 @@ function parseUser (login, passportUser, mode) { } if (passportUser) { - // Guess id + // Guess id if (passportUser.id) { user['usr.id'] = passportUser.id } else if (passportUser._id) { @@ -80,7 +80,6 @@ function passportTrackEvent (credentials, passportUser, rootSpan, mode) { if (passportUser) { // If a passportUser object is published then the login succeded - // TODO : test setUserTags({ id: user['usr.id'] }, rootSpan) // Prevent 'usr.id' from being reported again in the metadata delete user['usr.id'] From b2420c85aae6f4c2ec2a25df0b4c586963e96fd2 Mon Sep 17 00:00:00 2001 From: Julio Gonzalez <107922352+hoolioh@users.noreply.github.com> Date: Tue, 6 Jun 2023 15:42:49 +0200 Subject: [PATCH 08/45] Update index.d.ts Co-authored-by: Carles Capell <107924659+CarlesDD@users.noreply.github.com> --- index.d.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/index.d.ts b/index.d.ts index 49945926c5d..f5111343630 100644 --- a/index.d.ts +++ b/index.d.ts @@ -564,7 +564,7 @@ export declare interface TracerOptions { */ enabled?: boolean, /** - * Constrols the automated user event tracking mode + * Controls the automated user event tracking mode * @default 'safe' */ mode?: string From 0c45328094a8910e00c97586126355b2770980d1 Mon Sep 17 00:00:00 2001 From: Julio Gonzalez Date: Tue, 6 Jun 2023 16:23:57 +0200 Subject: [PATCH 09/45] Fix PR comments. --- docs/test.ts | 1 - index.d.ts | 7 +------ 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/docs/test.ts b/docs/test.ts index a01e2847095..5cc2236e97f 100644 --- a/docs/test.ts +++ b/docs/test.ts @@ -107,7 +107,6 @@ tracer.init({ blockedTemplateHtml: './blocked.html', blockedTemplateJson: './blocked.json', eventTracking: { - enabled: true, mode: 'safe' } } diff --git a/index.d.ts b/index.d.ts index 49945926c5d..5138eedb1ef 100644 --- a/index.d.ts +++ b/index.d.ts @@ -558,16 +558,11 @@ export declare interface TracerOptions { * Controls the automated user event tracking configuration */ eventTracking?: { - /** - * Whether to enable automated user event tracking. - * @default true - */ - enabled?: boolean, /** * Constrols the automated user event tracking mode * @default 'safe' */ - mode?: string + mode?: 'safe' | 'extended' | 'disabled' } }; From ec3dc06e89de954776c9add8d74a1f1899a7ab91 Mon Sep 17 00:00:00 2001 From: Julio Gonzalez <107922352+hoolioh@users.noreply.github.com> Date: Wed, 21 Jun 2023 15:53:20 +0200 Subject: [PATCH 10/45] Update packages/datadog-instrumentations/src/passport-local.js Co-authored-by: simon-id --- packages/datadog-instrumentations/src/passport-local.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/datadog-instrumentations/src/passport-local.js b/packages/datadog-instrumentations/src/passport-local.js index dc67492f2ca..12a80c7690a 100644 --- a/packages/datadog-instrumentations/src/passport-local.js +++ b/packages/datadog-instrumentations/src/passport-local.js @@ -10,7 +10,7 @@ function wrapVerifiedAndPublish (username, password, verified) { return shimmer.wrap(verified, function (err, user, info) { const credentials = { type: 'local', username, password } passportVerifyChannel.publish({ credentials, user }) - return verified.call(this, err, user, info) + return verified.apply(this, arguments) }) } else { return verified From 503fcf4134ac40c01fb3630e014850740de96413 Mon Sep 17 00:00:00 2001 From: Julio Gonzalez <107922352+hoolioh@users.noreply.github.com> Date: Wed, 21 Jun 2023 16:02:58 +0200 Subject: [PATCH 11/45] Update packages/datadog-instrumentations/src/passport-http.js Co-authored-by: simon-id --- packages/datadog-instrumentations/src/passport-http.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/datadog-instrumentations/src/passport-http.js b/packages/datadog-instrumentations/src/passport-http.js index a1032ca9184..0f1638d5338 100644 --- a/packages/datadog-instrumentations/src/passport-http.js +++ b/packages/datadog-instrumentations/src/passport-http.js @@ -8,7 +8,7 @@ const passportVerifyChannel = channel('datadog:passport:verify:finish') function wrapVerifiedAndPublish (username, password, verified) { if (passportVerifyChannel.hasSubscribers) { return shimmer.wrap(verified, function (err, user, info) { - const credentials = { type: 'http', username: username, password: password } + const credentials = { type: 'http', username, password } passportVerifyChannel.publish({ credentials, user }) return verified.call(this, err, user, info) }) From 1fbd3fc666b44eac70f8784a12f914432c749d70 Mon Sep 17 00:00:00 2001 From: Julio Gonzalez <107922352+hoolioh@users.noreply.github.com> Date: Wed, 21 Jun 2023 16:05:45 +0200 Subject: [PATCH 12/45] Update packages/datadog-instrumentations/src/passport-local.js Co-authored-by: simon-id --- packages/datadog-instrumentations/src/passport-local.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/datadog-instrumentations/src/passport-local.js b/packages/datadog-instrumentations/src/passport-local.js index 12a80c7690a..c8798490ebe 100644 --- a/packages/datadog-instrumentations/src/passport-local.js +++ b/packages/datadog-instrumentations/src/passport-local.js @@ -12,9 +12,9 @@ function wrapVerifiedAndPublish (username, password, verified) { passportVerifyChannel.publish({ credentials, user }) return verified.apply(this, arguments) }) - } else { - return verified } + + return verified } function wrapVerify (verify, passReq) { From 2449f5099435483e3ce633d41979b1b2af04ef43 Mon Sep 17 00:00:00 2001 From: Julio Gonzalez Date: Wed, 21 Jun 2023 15:40:33 +0200 Subject: [PATCH 13/45] Add support for getting the credentials from passport-http instrumentation. --- packages/dd-trace/src/appsec/passport.js | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/packages/dd-trace/src/appsec/passport.js b/packages/dd-trace/src/appsec/passport.js index d1e2f2a1bc7..a22983f2f5a 100644 --- a/packages/dd-trace/src/appsec/passport.js +++ b/packages/dd-trace/src/appsec/passport.js @@ -21,9 +21,17 @@ function isSdkCalled (tags) { } function getLogin (credentials) { - if (credentials && credentials.type === 'local') { - return credentials.username + const type = credentials && credentials.type + let login + if (type === 'local') { + login = credentials.username + } else if (type === 'http') { + login = credentials.username + } else { + login = undefined } + + return login } function parseUser (login, passportUser, mode) { @@ -41,7 +49,7 @@ function parseUser (login, passportUser, mode) { if (mode === 'safe') { if (!regexUsername.test(user['usr.id'])) { - user['usr.id'] = '' + user['usr.id'] = ' ' } } else { if (login) { From 4a95b7699936c7a72e9a8cda8637dfa876025ef2 Mon Sep 17 00:00:00 2001 From: Julio Gonzalez Date: Fri, 23 Jun 2023 17:26:17 +0200 Subject: [PATCH 14/45] Fix systems tests: * Fix bug where a login failure was leaking PII information in 'usr.id' field. * Move 'usr' fields from 'login.success' tags to common ones. --- packages/dd-trace/src/appsec/passport.js | 25 ++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/packages/dd-trace/src/appsec/passport.js b/packages/dd-trace/src/appsec/passport.js index a22983f2f5a..bb6cc746a26 100644 --- a/packages/dd-trace/src/appsec/passport.js +++ b/packages/dd-trace/src/appsec/passport.js @@ -47,11 +47,7 @@ function parseUser (login, passportUser, mode) { user['usr.id'] = passportUser._id } - if (mode === 'safe') { - if (!regexUsername.test(user['usr.id'])) { - user['usr.id'] = ' ' - } - } else { + if (mode === 'extended') { if (login) { user['usr.login'] = login } @@ -69,6 +65,13 @@ function parseUser (login, passportUser, mode) { } } + if (mode === 'safe') { + // Remove PII in safe mode + if (!regexUsername.test(user['usr.id'])) { + user['usr.id'] = ' ' + } + } + return user } @@ -88,9 +91,15 @@ function passportTrackEvent (credentials, passportUser, rootSpan, mode) { if (passportUser) { // If a passportUser object is published then the login succeded - setUserTags({ id: user['usr.id'] }, rootSpan) - // Prevent 'usr.id' from being reported again in the metadata - delete user['usr.id'] + let userTags = {} + Object.entries(user).some((entry) => { + let attr = entry[0].split('.')[1] + userTags[attr] = entry[1] + // Prevent attributes from beint sent under 'users.login.success' prefix + delete user[entry[0]] + }) + + setUserTags(userTags, rootSpan) trackEvent('users.login.success', user, 'passportTrackEvent', rootSpan, mode) } else { trackEvent('users.login.failure', user, 'passportTrackEvent', rootSpan, mode) From ec45d9ae6fd148748198f26e534df112929c3608 Mon Sep 17 00:00:00 2001 From: Julio Gonzalez Date: Fri, 23 Jun 2023 18:04:43 +0200 Subject: [PATCH 15/45] Fix PR comments. --- packages/datadog-instrumentations/src/passport-http.js | 4 ++-- .../datadog-instrumentations/src/passport-local.js | 4 ++-- packages/dd-trace/src/config.js | 2 +- packages/dd-trace/test/plugins/externals.json | 10 ++++++++++ 4 files changed, 15 insertions(+), 5 deletions(-) diff --git a/packages/datadog-instrumentations/src/passport-http.js b/packages/datadog-instrumentations/src/passport-http.js index 0f1638d5338..fe17cf202f1 100644 --- a/packages/datadog-instrumentations/src/passport-http.js +++ b/packages/datadog-instrumentations/src/passport-http.js @@ -8,7 +8,7 @@ const passportVerifyChannel = channel('datadog:passport:verify:finish') function wrapVerifiedAndPublish (username, password, verified) { if (passportVerifyChannel.hasSubscribers) { return shimmer.wrap(verified, function (err, user, info) { - const credentials = { type: 'http', username, password } + const credentials = { type: 'http', username } passportVerifyChannel.publish({ credentials, user }) return verified.call(this, err, user, info) }) @@ -40,7 +40,7 @@ addHook({ if (typeof arguments[0] === 'function') { arguments[0] = wrapVerify(arguments[0], false) } else { - arguments[1] = wrapVerify(arguments[1], (arguments[0] && arguments[0].passReqToCallback) || false) + arguments[1] = wrapVerify(arguments[1], (arguments[0] && arguments[0].passReqToCallback)) } return BasicStrategy.apply(this, arguments) }) diff --git a/packages/datadog-instrumentations/src/passport-local.js b/packages/datadog-instrumentations/src/passport-local.js index c8798490ebe..d1f2adc658b 100644 --- a/packages/datadog-instrumentations/src/passport-local.js +++ b/packages/datadog-instrumentations/src/passport-local.js @@ -8,7 +8,7 @@ const passportVerifyChannel = channel('datadog:passport:verify:finish') function wrapVerifiedAndPublish (username, password, verified) { if (passportVerifyChannel.hasSubscribers) { return shimmer.wrap(verified, function (err, user, info) { - const credentials = { type: 'local', username, password } + const credentials = { type: 'local', username } passportVerifyChannel.publish({ credentials, user }) return verified.apply(this, arguments) }) @@ -40,7 +40,7 @@ addHook({ if (typeof arguments[0] === 'function') { arguments[0] = wrapVerify(arguments[0], false) } else { - arguments[1] = wrapVerify(arguments[1], (arguments[0] && arguments[0].passReqToCallback) || false) + arguments[1] = wrapVerify(arguments[1], (arguments[0] && arguments[0].passReqToCallback)) } return Strategy.apply(this, arguments) }) diff --git a/packages/dd-trace/src/config.js b/packages/dd-trace/src/config.js index 88a46074185..d1cca5d5652 100644 --- a/packages/dd-trace/src/config.js +++ b/packages/dd-trace/src/config.js @@ -538,7 +538,7 @@ ken|consumer_?(?:id|key|secret)|sign(?:ed|ature)?|auth(?:entication|orization)?) blockedTemplateHtml: DD_APPSEC_HTTP_BLOCKED_TEMPLATE_HTML, blockedTemplateJson: DD_APPSEC_HTTP_BLOCKED_TEMPLATE_JSON, eventTracking: { - enabled: ['extended', 'safe'].some(value => DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING === value), + enabled: ['extended', 'safe'].includes(DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING), mode: DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING } } diff --git a/packages/dd-trace/test/plugins/externals.json b/packages/dd-trace/test/plugins/externals.json index 7c3e8175558..b77592ad2b7 100644 --- a/packages/dd-trace/test/plugins/externals.json +++ b/packages/dd-trace/test/plugins/externals.json @@ -167,6 +167,16 @@ "dep": true } ], + "passport-http": [ + { + "name": "passport-http", + "versions": ["0.3.0"] + }, + { + "name": "passport", + "versions": [">=0.4.1"] + } + ], "passport-local": [ { "name": "passport-local", From 9ae13d2e1fea8750e60dca2c43b411452c62a6d7 Mon Sep 17 00:00:00 2001 From: Julio Gonzalez Date: Mon, 26 Jun 2023 11:51:26 +0200 Subject: [PATCH 16/45] Move common functions to a single utils file. --- .../src/passport-http.js | 37 +++---------------- .../src/passport-local.js | 37 +++---------------- .../src/passport-utils.js | 35 ++++++++++++++++++ .../test/passport-http.spec.js | 4 +- .../test/passport-local.spec.js | 4 +- 5 files changed, 51 insertions(+), 66 deletions(-) create mode 100644 packages/datadog-instrumentations/src/passport-utils.js diff --git a/packages/datadog-instrumentations/src/passport-http.js b/packages/datadog-instrumentations/src/passport-http.js index fe17cf202f1..3ffc369a395 100644 --- a/packages/datadog-instrumentations/src/passport-http.js +++ b/packages/datadog-instrumentations/src/passport-http.js @@ -1,35 +1,8 @@ 'use strict' const shimmer = require('../../datadog-shimmer') -const { channel, addHook } = require('./helpers/instrument') - -const passportVerifyChannel = channel('datadog:passport:verify:finish') - -function wrapVerifiedAndPublish (username, password, verified) { - if (passportVerifyChannel.hasSubscribers) { - return shimmer.wrap(verified, function (err, user, info) { - const credentials = { type: 'http', username } - passportVerifyChannel.publish({ credentials, user }) - return verified.call(this, err, user, info) - }) - } else { - return verified - } -} - -function wrapVerify (verify, passReq) { - if (passReq) { - return function (req, username, password, verified) { - arguments[3] = wrapVerifiedAndPublish(username, password, verified) - return verify.apply(this, arguments) - } - } else { - return function (username, password, verified) { - arguments[2] = wrapVerifiedAndPublish(username, password, verified) - return verify.apply(this, arguments) - } - } -} +const { addHook } = require('./helpers/instrument') +const { wrapVerify } = require('./passport-utils') addHook({ name: 'passport-http', @@ -37,10 +10,12 @@ addHook({ versions: ['>=0.3.0'] }, BasicStrategy => { return shimmer.wrap(BasicStrategy, function () { + const type = 'http' + if (typeof arguments[0] === 'function') { - arguments[0] = wrapVerify(arguments[0], false) + arguments[0] = wrapVerify(arguments[0], false, type) } else { - arguments[1] = wrapVerify(arguments[1], (arguments[0] && arguments[0].passReqToCallback)) + arguments[1] = wrapVerify(arguments[1], (arguments[0] && arguments[0].passReqToCallback), type) } return BasicStrategy.apply(this, arguments) }) diff --git a/packages/datadog-instrumentations/src/passport-local.js b/packages/datadog-instrumentations/src/passport-local.js index d1f2adc658b..d0c48c56ccb 100644 --- a/packages/datadog-instrumentations/src/passport-local.js +++ b/packages/datadog-instrumentations/src/passport-local.js @@ -1,35 +1,8 @@ 'use strict' const shimmer = require('../../datadog-shimmer') -const { channel, addHook } = require('./helpers/instrument') - -const passportVerifyChannel = channel('datadog:passport:verify:finish') - -function wrapVerifiedAndPublish (username, password, verified) { - if (passportVerifyChannel.hasSubscribers) { - return shimmer.wrap(verified, function (err, user, info) { - const credentials = { type: 'local', username } - passportVerifyChannel.publish({ credentials, user }) - return verified.apply(this, arguments) - }) - } - - return verified -} - -function wrapVerify (verify, passReq) { - if (passReq) { - return function (req, username, password, verified) { - arguments[3] = wrapVerifiedAndPublish(username, password, verified) - return verify.apply(this, arguments) - } - } else { - return function (username, password, verified) { - arguments[2] = wrapVerifiedAndPublish(username, password, verified) - return verify.apply(this, arguments) - } - } -} +const { addHook } = require('./helpers/instrument') +const { wrapVerify } = require('./passport-utils') addHook({ name: 'passport-local', @@ -37,10 +10,12 @@ addHook({ versions: ['>=1.0.0'] }, Strategy => { return shimmer.wrap(Strategy, function () { + const type = 'local' + if (typeof arguments[0] === 'function') { - arguments[0] = wrapVerify(arguments[0], false) + arguments[0] = wrapVerify(arguments[0], false, type) } else { - arguments[1] = wrapVerify(arguments[1], (arguments[0] && arguments[0].passReqToCallback)) + arguments[1] = wrapVerify(arguments[1], (arguments[0] && arguments[0].passReqToCallback), type) } return Strategy.apply(this, arguments) }) diff --git a/packages/datadog-instrumentations/src/passport-utils.js b/packages/datadog-instrumentations/src/passport-utils.js new file mode 100644 index 00000000000..8d74f540f6f --- /dev/null +++ b/packages/datadog-instrumentations/src/passport-utils.js @@ -0,0 +1,35 @@ +'use strict' + +const shimmer = require('../../datadog-shimmer') +const { channel } = require('./helpers/instrument') +const passportVerifyChannel = channel('datadog:passport:verify:finish') + +function wrapVerifiedAndPublish (username, password, verified, strategy) { + if (passportVerifyChannel.hasSubscribers) { + return shimmer.wrap(verified, function (err, user, info) { + const credentials = { type: strategy, username } + passportVerifyChannel.publish({ credentials, user }) + return verified.call(this, err, user, info) + }) + } else { + return verified + } +} + +function wrapVerify (verify, passReq, type) { + if (passReq) { + return function (req, username, password, verified) { + arguments[3] = wrapVerifiedAndPublish(username, password, verified, type) + return verify.apply(this, arguments) + } + } else { + return function (username, password, verified) { + arguments[2] = wrapVerifiedAndPublish(username, password, verified, type) + return verify.apply(this, arguments) + } + } +} + +module.exports = { + wrapVerify +} diff --git a/packages/datadog-instrumentations/test/passport-http.spec.js b/packages/datadog-instrumentations/test/passport-http.spec.js index 53dcfbe9ecb..f741cf95d87 100644 --- a/packages/datadog-instrumentations/test/passport-http.spec.js +++ b/packages/datadog-instrumentations/test/passport-http.spec.js @@ -88,7 +88,7 @@ withVersions('passport-http', 'passport-http', version => { expect(res.data).to.equal('Granted') expect(subscriberStub).to.be.calledOnceWithExactly( { - credentials: { type: 'http', username: 'test', password: '1234' }, + credentials: { type: 'http', username: 'test' }, user: { _id: 1, username: 'test', password: '1234', email: 'testuser@ddog.com' } } ) @@ -106,7 +106,7 @@ withVersions('passport-http', 'passport-http', version => { expect(res.data).to.equal('Denied') expect(subscriberStub).to.be.calledOnceWithExactly( { - credentials: { type: 'http', username: 'test', password: '1' }, + credentials: { type: 'http', username: 'test' }, user: false } ) diff --git a/packages/datadog-instrumentations/test/passport-local.spec.js b/packages/datadog-instrumentations/test/passport-local.spec.js index 1a5e1e8a6b3..5133048c680 100644 --- a/packages/datadog-instrumentations/test/passport-local.spec.js +++ b/packages/datadog-instrumentations/test/passport-local.spec.js @@ -84,7 +84,7 @@ withVersions('passport-local', 'passport-local', version => { expect(res.data).to.equal('Granted') expect(subscriberStub).to.be.calledOnceWithExactly( { - credentials: { type: 'local', username: 'test', password: '1234' }, + credentials: { type: 'local', username: 'test' }, user: { _id: 1, username: 'test', password: '1234', email: 'testuser@ddog.com' } } ) @@ -97,7 +97,7 @@ withVersions('passport-local', 'passport-local', version => { expect(res.data).to.equal('Denied') expect(subscriberStub).to.be.calledOnceWithExactly( { - credentials: { type: 'local', username: 'test', password: '1' }, + credentials: { type: 'local', username: 'test' }, user: false } ) From 966a0686309f6c206d5e8e742c4b028e7d07d7a7 Mon Sep 17 00:00:00 2001 From: Julio Gonzalez Date: Mon, 26 Jun 2023 12:23:26 +0200 Subject: [PATCH 17/45] Describe safe and extended mode. --- index.d.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/index.d.ts b/index.d.ts index a9f32912bbc..b327cccc1ad 100644 --- a/index.d.ts +++ b/index.d.ts @@ -559,7 +559,9 @@ export declare interface TracerOptions { */ eventTracking?: { /** - * Controls the automated user event tracking mode + * Controls the automated user event tracking mode. Possible values are disabled, safe and extended. + * On safe mode any possible Personal Identifable Information (PII) about the user will be removed from the event. + * On extended mode any PII or non-PII information about the user could be added to the event. * @default 'safe' */ mode?: 'safe' | 'extended' | 'disabled' From 0039d24bea7b795a35ec8ccba71d2faabcf8c32a Mon Sep 17 00:00:00 2001 From: Julio Gonzalez Date: Mon, 26 Jun 2023 12:51:57 +0200 Subject: [PATCH 18/45] Fix passport unit tests. --- packages/dd-trace/src/appsec/passport.js | 8 ++- .../dd-trace/test/appsec/passport.spec.js | 56 ++++++++++--------- 2 files changed, 37 insertions(+), 27 deletions(-) diff --git a/packages/dd-trace/src/appsec/passport.js b/packages/dd-trace/src/appsec/passport.js index bb6cc746a26..4ba40bf24ca 100644 --- a/packages/dd-trace/src/appsec/passport.js +++ b/packages/dd-trace/src/appsec/passport.js @@ -39,6 +39,10 @@ function parseUser (login, passportUser, mode) { 'usr.id': login } + if (!user['usr.id']) { + return user + } + if (passportUser) { // Guess id if (passportUser.id) { @@ -91,9 +95,9 @@ function passportTrackEvent (credentials, passportUser, rootSpan, mode) { if (passportUser) { // If a passportUser object is published then the login succeded - let userTags = {} + const userTags = {} Object.entries(user).some((entry) => { - let attr = entry[0].split('.')[1] + const attr = entry[0].split('.')[1] userTags[attr] = entry[1] // Prevent attributes from beint sent under 'users.login.success' prefix delete user[entry[0]] diff --git a/packages/dd-trace/test/appsec/passport.spec.js b/packages/dd-trace/test/appsec/passport.spec.js index 879eff7add8..9d3e59d07fc 100644 --- a/packages/dd-trace/test/appsec/passport.spec.js +++ b/packages/dd-trace/test/appsec/passport.spec.js @@ -63,7 +63,7 @@ describe('Passport', () => { expect(setUser.setUserTags).not.to.have.been.called expect(events.trackEvent).to.have.been.calledOnceWithExactly( 'users.login.failure', - { 'usr.id': loginLocal.username }, + { 'usr.id': ' ' }, 'passportTrackEvent', undefined, 'safe' @@ -89,7 +89,7 @@ describe('Passport', () => { user.id = 'publicName' passportModule.passportTrackEvent(loginLocal, user, rootSpan, 'safe') - expect(setUser.setUserTags).to.have.been.calledOnceWithExactly({ id: '' }, rootSpan) + expect(setUser.setUserTags).to.have.been.calledOnceWithExactly({ id: ' ' }, rootSpan) expect(events.trackEvent).to.have.been.calledOnceWithExactly( 'users.login.success', {}, @@ -104,14 +104,18 @@ describe('Passport', () => { user.id = 'publicName' passportModule.passportTrackEvent(loginLocal, user, rootSpan, 'extended') - expect(setUser.setUserTags).to.have.been.calledOnceWithExactly({ id: user.id }, rootSpan) - expect(events.trackEvent).to.have.been.calledOnceWithExactly( - 'users.login.success', + expect(setUser.setUserTags).to.have.been.calledOnceWithExactly( { - 'usr.email': user.email, - 'usr.username': user.username, - 'usr.login': loginLocal.username + id: user.id, + login: 'test', + email: 'testUser@test.com', + username: 'Test User' }, + rootSpan + ) + expect(events.trackEvent).to.have.been.calledOnceWithExactly( + 'users.login.success', + {}, 'passportTrackEvent', rootSpan, 'extended' @@ -158,11 +162,7 @@ describe('Passport', () => { passportModule.passportTrackEvent(loginLocal, userUuid, rootSpan, 'extended') expect(events.trackEvent).to.have.been.calledOnceWithExactly( 'users.login.success', - { - 'usr.email': userUuid.email, - 'usr.username': userUuid.username, - 'usr.login': loginLocal.username - }, + {}, 'passportTrackEvent', rootSpan, 'extended' @@ -193,14 +193,18 @@ describe('Passport', () => { rootSpan.context = () => { return {} } passportModule.passportTrackEvent(loginLocal, user, rootSpan, 'extended') - expect(setUser.setUserTags).to.have.been.calledOnceWithExactly({ id: user._id }, rootSpan) - expect(events.trackEvent).to.have.been.calledOnceWithExactly( - 'users.login.success', + expect(setUser.setUserTags).to.have.been.calledOnceWithExactly( { - 'usr.email': user.email, - 'usr.username': user.username, - 'usr.login': loginLocal.username + id: user._id, + login: 'test', + email: 'testUser@test.com', + username: 'Test User' }, + rootSpan + ) + expect(events.trackEvent).to.have.been.calledOnceWithExactly( + 'users.login.success', + {}, 'passportTrackEvent', rootSpan, 'extended' @@ -216,14 +220,16 @@ describe('Passport', () => { rootSpan.context = () => { return {} } passportModule.passportTrackEvent(loginLocal, user, rootSpan, 'extended') - expect(setUser.setUserTags).to.have.been.calledOnceWithExactly({ id: loginLocal.username }, rootSpan) + expect(setUser.setUserTags).to.have.been.calledOnceWithExactly( + { + id: loginLocal.username, + login: 'test', + email: 'testUser@test.com', + username: 'Test User' + }, rootSpan) expect(events.trackEvent).to.have.been.calledOnceWithExactly( 'users.login.success', - { - 'usr.email': user.email, - 'usr.username': user.name, - 'usr.login': loginLocal.username - }, + {}, 'passportTrackEvent', rootSpan, 'extended' From c69d016e2577cb35cec7de70093de3c3268c6397 Mon Sep 17 00:00:00 2001 From: Julio Gonzalez Date: Tue, 27 Jun 2023 09:50:20 +0200 Subject: [PATCH 19/45] Change 'call' method for 'apply'. --- packages/datadog-instrumentations/src/passport-utils.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/datadog-instrumentations/src/passport-utils.js b/packages/datadog-instrumentations/src/passport-utils.js index 8d74f540f6f..8d99762c7cd 100644 --- a/packages/datadog-instrumentations/src/passport-utils.js +++ b/packages/datadog-instrumentations/src/passport-utils.js @@ -9,7 +9,7 @@ function wrapVerifiedAndPublish (username, password, verified, strategy) { return shimmer.wrap(verified, function (err, user, info) { const credentials = { type: strategy, username } passportVerifyChannel.publish({ credentials, user }) - return verified.call(this, err, user, info) + return verified.apply(this, arguments) }) } else { return verified From 48158f14d27249e56d09f2fcf9be22baf79a2e8c Mon Sep 17 00:00:00 2001 From: Julio Gonzalez Date: Tue, 27 Jun 2023 10:35:32 +0200 Subject: [PATCH 20/45] Fix log. --- packages/dd-trace/src/appsec/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/dd-trace/src/appsec/index.js b/packages/dd-trace/src/appsec/index.js index 974d4840c54..82d0b6d2d48 100644 --- a/packages/dd-trace/src/appsec/index.js +++ b/packages/dd-trace/src/appsec/index.js @@ -151,7 +151,7 @@ function onPassportVerify ({ credentials, user }) { const rootSpan = store && store.req && web.root(store.req) if (!rootSpan) { - log.warn('No rootSpan found in onPassportVerifySafe') + log.warn('No rootSpan found in onPassportVerify') return } From e65edb044207da9d0afef68efdab3dd27e3614e2 Mon Sep 17 00:00:00 2001 From: Julio Gonzalez Date: Tue, 27 Jun 2023 11:52:17 +0200 Subject: [PATCH 21/45] Fix PR comments regarding config tests. --- packages/dd-trace/test/config.spec.js | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/packages/dd-trace/test/config.spec.js b/packages/dd-trace/test/config.spec.js index 9d6db7104fc..7ee7c069342 100644 --- a/packages/dd-trace/test/config.spec.js +++ b/packages/dd-trace/test/config.spec.js @@ -108,6 +108,8 @@ describe('Config', () => { expect(config).to.have.nested.property('appsec.obfuscatorValueRegex').with.length(443) expect(config).to.have.nested.property('appsec.blockedTemplateHtml', undefined) expect(config).to.have.nested.property('appsec.blockedTemplateJson', undefined) + expect(config).to.have.nested.property('appsec.eventTracking.enabled', true) + expect(config).to.have.nested.property('appsec.eventTracking.mode', 'safe') expect(config).to.have.nested.property('remoteConfig.enabled', true) expect(config).to.have.nested.property('remoteConfig.pollInterval', 5) expect(config).to.have.nested.property('iast.enabled', false) @@ -198,6 +200,7 @@ describe('Config', () => { process.env.DD_APPSEC_OBFUSCATION_PARAMETER_VALUE_REGEXP = '.*' process.env.DD_APPSEC_HTTP_BLOCKED_TEMPLATE_HTML = BLOCKED_TEMPLATE_HTML_PATH process.env.DD_APPSEC_HTTP_BLOCKED_TEMPLATE_JSON = BLOCKED_TEMPLATE_JSON_PATH + process.env.DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING = 'extended' process.env.DD_REMOTE_CONFIGURATION_ENABLED = 'false' process.env.DD_REMOTE_CONFIG_POLL_INTERVAL_SECONDS = '42' process.env.DD_IAST_ENABLED = 'true' @@ -265,6 +268,8 @@ describe('Config', () => { expect(config).to.have.nested.property('appsec.obfuscatorValueRegex', '.*') expect(config).to.have.nested.property('appsec.blockedTemplateHtml', BLOCKED_TEMPLATE_HTML) expect(config).to.have.nested.property('appsec.blockedTemplateJson', BLOCKED_TEMPLATE_JSON) + expect(config).to.have.nested.property('appsec.eventTracking.enabled', true) + expect(config).to.have.nested.property('appsec.eventTracking.mode', 'extended') expect(config).to.have.nested.property('remoteConfig.enabled', false) expect(config).to.have.nested.property('remoteConfig.pollInterval', 42) expect(config).to.have.nested.property('iast.enabled', true) @@ -573,6 +578,7 @@ describe('Config', () => { process.env.DD_APPSEC_OBFUSCATION_PARAMETER_VALUE_REGEXP = '^$' process.env.DD_APPSEC_HTTP_BLOCKED_TEMPLATE_HTML = BLOCKED_TEMPLATE_JSON // note the inversion between process.env.DD_APPSEC_HTTP_BLOCKED_TEMPLATE_JSON = BLOCKED_TEMPLATE_HTML // json and html here + process.env.DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING = 'disabled' process.env.DD_REMOTE_CONFIG_POLL_INTERVAL_SECONDS = 11 process.env.DD_IAST_ENABLED = 'false' process.env.DD_TRACE_128_BIT_TRACEID_GENERATION_ENABLED = 'true' @@ -623,7 +629,11 @@ describe('Config', () => { obfuscatorKeyRegex: '.*', obfuscatorValueRegex: '.*', blockedTemplateHtml: BLOCKED_TEMPLATE_HTML_PATH, - blockedTemplateJson: BLOCKED_TEMPLATE_JSON_PATH + blockedTemplateJson: BLOCKED_TEMPLATE_JSON_PATH, + eventTracking: { + enabled: true, + mode: 'safe' + } }, remoteConfig: { pollInterval: 42 @@ -666,6 +676,8 @@ describe('Config', () => { expect(config).to.have.nested.property('appsec.obfuscatorValueRegex', '.*') expect(config).to.have.nested.property('appsec.blockedTemplateHtml', BLOCKED_TEMPLATE_HTML) expect(config).to.have.nested.property('appsec.blockedTemplateJson', BLOCKED_TEMPLATE_JSON) + expect(config).to.have.nested.property('appsec.eventTracking.enabled', true) + expect(config).to.have.nested.property('appsec.eventTracking.mode', 'safe') expect(config).to.have.nested.property('remoteConfig.pollInterval', 42) expect(config).to.have.nested.property('iast.enabled', true) expect(config).to.have.nested.property('iast.requestSampling', 30) @@ -685,7 +697,11 @@ describe('Config', () => { obfuscatorKeyRegex: '.*', obfuscatorValueRegex: '.*', blockedTemplateHtml: undefined, - blockedTemplateJson: undefined + blockedTemplateJson: undefined, + eventTracking: { + enabled: true, + mode: 'safe' + } }, experimental: { appsec: { @@ -696,7 +712,11 @@ describe('Config', () => { obfuscatorKeyRegex: '^$', obfuscatorValueRegex: '^$', blockedTemplateHtml: BLOCKED_TEMPLATE_HTML_PATH, - blockedTemplateJson: BLOCKED_TEMPLATE_JSON_PATH + blockedTemplateJson: BLOCKED_TEMPLATE_JSON_PATH, + eventTracking: { + enabled: false, + mode: 'disabled' + } } } }) From 201a0dd00c9acbe9045ca7377552cc7bfe296c63 Mon Sep 17 00:00:00 2001 From: Julio Gonzalez Date: Tue, 27 Jun 2023 12:21:13 +0200 Subject: [PATCH 22/45] Fix version pinning. --- packages/dd-trace/test/plugins/externals.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/dd-trace/test/plugins/externals.json b/packages/dd-trace/test/plugins/externals.json index 25446ef3129..c3df94712de 100644 --- a/packages/dd-trace/test/plugins/externals.json +++ b/packages/dd-trace/test/plugins/externals.json @@ -174,7 +174,7 @@ "passport-http": [ { "name": "passport-http", - "versions": ["0.3.0"] + "versions": [">=0.3.0"] }, { "name": "passport", @@ -184,7 +184,7 @@ "passport-local": [ { "name": "passport-local", - "versions": ["1.0.0"] + "versions": [">=1.0.0"] }, { "name": "passport", From 379ce1002f7af7eb8618e17463f12ed33a7f103c Mon Sep 17 00:00:00 2001 From: Julio Gonzalez Date: Tue, 27 Jun 2023 14:00:57 +0200 Subject: [PATCH 23/45] Fix PR suggestion. --- .../datadog-instrumentations/src/passport-utils.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/datadog-instrumentations/src/passport-utils.js b/packages/datadog-instrumentations/src/passport-utils.js index 8d99762c7cd..08bce18a99b 100644 --- a/packages/datadog-instrumentations/src/passport-utils.js +++ b/packages/datadog-instrumentations/src/passport-utils.js @@ -5,15 +5,15 @@ const { channel } = require('./helpers/instrument') const passportVerifyChannel = channel('datadog:passport:verify:finish') function wrapVerifiedAndPublish (username, password, verified, strategy) { - if (passportVerifyChannel.hasSubscribers) { - return shimmer.wrap(verified, function (err, user, info) { - const credentials = { type: strategy, username } - passportVerifyChannel.publish({ credentials, user }) - return verified.apply(this, arguments) - }) - } else { + if (!passportVerifyChannel.hasSubscribers) { return verified } + + return shimmer.wrap(verified, function (err, user, info) { + const credentials = { type: strategy, username } + passportVerifyChannel.publish({ credentials, user }) + return verified.apply(this, arguments) + }) } function wrapVerify (verify, passReq, type) { From 272c490fde49798d297759a3f1ec496a9e3eff87 Mon Sep 17 00:00:00 2001 From: Julio Gonzalez <107922352+hoolioh@users.noreply.github.com> Date: Tue, 27 Jun 2023 16:17:52 +0200 Subject: [PATCH 24/45] Apply suggestions from code review Co-authored-by: simon-id --- index.d.ts | 4 ++-- packages/datadog-instrumentations/src/passport-utils.js | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/index.d.ts b/index.d.ts index a2ca9914edb..43bda7352bc 100644 --- a/index.d.ts +++ b/index.d.ts @@ -563,8 +563,8 @@ export declare interface TracerOptions { eventTracking?: { /** * Controls the automated user event tracking mode. Possible values are disabled, safe and extended. - * On safe mode any possible Personal Identifable Information (PII) about the user will be removed from the event. - * On extended mode any PII or non-PII information about the user could be added to the event. + * On safe mode, any detected Personally Identifiable Information (PII) about the user will be redacted from the event. + * On extended mode, no redaction will take place. * @default 'safe' */ mode?: 'safe' | 'extended' | 'disabled' diff --git a/packages/datadog-instrumentations/src/passport-utils.js b/packages/datadog-instrumentations/src/passport-utils.js index 08bce18a99b..c1d842b64a6 100644 --- a/packages/datadog-instrumentations/src/passport-utils.js +++ b/packages/datadog-instrumentations/src/passport-utils.js @@ -2,9 +2,10 @@ const shimmer = require('../../datadog-shimmer') const { channel } = require('./helpers/instrument') + const passportVerifyChannel = channel('datadog:passport:verify:finish') -function wrapVerifiedAndPublish (username, password, verified, strategy) { +function wrapVerifiedAndPublish (username, password, verified, type) { if (!passportVerifyChannel.hasSubscribers) { return verified } From d630a1e8b55b56aa6cebcc2cab7c4a9505b964dc Mon Sep 17 00:00:00 2001 From: Julio Gonzalez Date: Tue, 27 Jun 2023 17:41:24 +0200 Subject: [PATCH 25/45] Remove enabled from test since it's not in the public API. --- packages/dd-trace/test/config.spec.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/packages/dd-trace/test/config.spec.js b/packages/dd-trace/test/config.spec.js index 7ee7c069342..1eb5131003f 100644 --- a/packages/dd-trace/test/config.spec.js +++ b/packages/dd-trace/test/config.spec.js @@ -631,7 +631,6 @@ describe('Config', () => { blockedTemplateHtml: BLOCKED_TEMPLATE_HTML_PATH, blockedTemplateJson: BLOCKED_TEMPLATE_JSON_PATH, eventTracking: { - enabled: true, mode: 'safe' } }, @@ -699,7 +698,6 @@ describe('Config', () => { blockedTemplateHtml: undefined, blockedTemplateJson: undefined, eventTracking: { - enabled: true, mode: 'safe' } }, @@ -714,7 +712,6 @@ describe('Config', () => { blockedTemplateHtml: BLOCKED_TEMPLATE_HTML_PATH, blockedTemplateJson: BLOCKED_TEMPLATE_JSON_PATH, eventTracking: { - enabled: false, mode: 'disabled' } } From 9b396dcebd628fda3ad36353e92603be8981d98e Mon Sep 17 00:00:00 2001 From: Julio Gonzalez Date: Tue, 27 Jun 2023 17:42:20 +0200 Subject: [PATCH 26/45] Remove versions from main package. --- packages/dd-trace/test/plugins/externals.json | 8 -------- 1 file changed, 8 deletions(-) diff --git a/packages/dd-trace/test/plugins/externals.json b/packages/dd-trace/test/plugins/externals.json index c3df94712de..9df957678b2 100644 --- a/packages/dd-trace/test/plugins/externals.json +++ b/packages/dd-trace/test/plugins/externals.json @@ -172,20 +172,12 @@ } ], "passport-http": [ - { - "name": "passport-http", - "versions": [">=0.3.0"] - }, { "name": "passport", "versions": [">=0.4.1"] } ], "passport-local": [ - { - "name": "passport-local", - "versions": [">=1.0.0"] - }, { "name": "passport", "versions": [">=0.4.1"] From 7ba4d6f0168135560ad73ec67ac8b64445332ad3 Mon Sep 17 00:00:00 2001 From: Julio Gonzalez Date: Tue, 27 Jun 2023 17:50:44 +0200 Subject: [PATCH 27/45] Fix PR commit suggestion. --- packages/datadog-instrumentations/src/passport-utils.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/datadog-instrumentations/src/passport-utils.js b/packages/datadog-instrumentations/src/passport-utils.js index c1d842b64a6..02e64665b21 100644 --- a/packages/datadog-instrumentations/src/passport-utils.js +++ b/packages/datadog-instrumentations/src/passport-utils.js @@ -11,7 +11,7 @@ function wrapVerifiedAndPublish (username, password, verified, type) { } return shimmer.wrap(verified, function (err, user, info) { - const credentials = { type: strategy, username } + const credentials = { type: type, username } passportVerifyChannel.publish({ credentials, user }) return verified.apply(this, arguments) }) From b11a4fb36643e66aa16f9ac46cbc9352124f5f11 Mon Sep 17 00:00:00 2001 From: simon-id Date: Tue, 27 Jun 2023 18:10:02 +0200 Subject: [PATCH 28/45] Update packages/datadog-instrumentations/src/passport-utils.js --- packages/datadog-instrumentations/src/passport-utils.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/datadog-instrumentations/src/passport-utils.js b/packages/datadog-instrumentations/src/passport-utils.js index 02e64665b21..5af55ca94c0 100644 --- a/packages/datadog-instrumentations/src/passport-utils.js +++ b/packages/datadog-instrumentations/src/passport-utils.js @@ -11,7 +11,7 @@ function wrapVerifiedAndPublish (username, password, verified, type) { } return shimmer.wrap(verified, function (err, user, info) { - const credentials = { type: type, username } + const credentials = { type, username } passportVerifyChannel.publish({ credentials, user }) return verified.apply(this, arguments) }) From dea65799ec5433115a313a6468277c4ef74adfb8 Mon Sep 17 00:00:00 2001 From: simon-id Date: Tue, 27 Jun 2023 18:21:16 +0200 Subject: [PATCH 29/45] increase coverage in config test --- packages/dd-trace/test/config.spec.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/dd-trace/test/config.spec.js b/packages/dd-trace/test/config.spec.js index 1eb5131003f..18bcfdcb9a4 100644 --- a/packages/dd-trace/test/config.spec.js +++ b/packages/dd-trace/test/config.spec.js @@ -698,7 +698,7 @@ describe('Config', () => { blockedTemplateHtml: undefined, blockedTemplateJson: undefined, eventTracking: { - mode: 'safe' + mode: 'disabled' } }, experimental: { @@ -712,7 +712,7 @@ describe('Config', () => { blockedTemplateHtml: BLOCKED_TEMPLATE_HTML_PATH, blockedTemplateJson: BLOCKED_TEMPLATE_JSON_PATH, eventTracking: { - mode: 'disabled' + mode: 'safe' } } } @@ -729,8 +729,8 @@ describe('Config', () => { blockedTemplateHtml: undefined, blockedTemplateJson: undefined, eventTracking: { - enabled: true, - mode: 'safe' + enabled: false, + mode: 'disabled' } }) }) From df1e4ebbc6f4221451faf744759bee0962131d83 Mon Sep 17 00:00:00 2001 From: Julio Gonzalez Date: Wed, 28 Jun 2023 12:39:14 +0200 Subject: [PATCH 30/45] Add PR suggestions. --- packages/dd-trace/test/appsec/index.spec.js | 26 +++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/packages/dd-trace/test/appsec/index.spec.js b/packages/dd-trace/test/appsec/index.spec.js index 0f51f2b5976..d35188f3405 100644 --- a/packages/dd-trace/test/appsec/index.spec.js +++ b/packages/dd-trace/test/appsec/index.spec.js @@ -471,6 +471,23 @@ describe('AppSec Index', () => { }) describe('onPassportVerify', () => { + it('Should not subscribe to the channel if disabled', () => { + const config = { + appsec: { + enabled: true, + eventTracking: { + enabled: false, + mode: 'disabled' + } + } + } + + AppSec.disable() + AppSec.enable(config) + + expect(passportVerify.hasSubscribers).to.be.false + }) + it('Should call passportTrackEvent', () => { const credentials = { type: 'local', username: 'test' } const user = { id: '1234', username: 'Test' } @@ -490,15 +507,16 @@ describe('AppSec Index', () => { it('Should call log if no rootSpan is found', () => { const credentials = { type: 'local', username: 'test' } - const passportUser = { id: '1234', username: 'Test' } - const passportErr = {} - const passportInfo = {} + const user = { id: '1234', username: 'Test' } + const err = {} + const info = {} sinon.stub(storage, 'getStore').returns(undefined) - passportVerify.publish({ credentials, passportUser, passportErr, passportInfo }) + passportVerify.publish({ credentials, user, err, info }) expect(log.warn).to.have.been.calledOnceWithExactly('No rootSpan found in onPassportVerify') + expect(passport.passportTrackEvent).not.to.have.been.called }) }) }) From d0070afe530c7ad4b8446ccee57f5bdcedb6b680 Mon Sep 17 00:00:00 2001 From: Julio Gonzalez Date: Wed, 28 Jun 2023 12:43:47 +0200 Subject: [PATCH 31/45] Add PR suggestions. --- packages/datadog-instrumentations/test/passport-http.spec.js | 2 +- packages/datadog-instrumentations/test/passport-local.spec.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/datadog-instrumentations/test/passport-http.spec.js b/packages/datadog-instrumentations/test/passport-http.spec.js index f741cf95d87..14a67becff9 100644 --- a/packages/datadog-instrumentations/test/passport-http.spec.js +++ b/packages/datadog-instrumentations/test/passport-http.spec.js @@ -67,7 +67,7 @@ withVersions('passport-http', 'passport-http', version => { }) }) }) - beforeEach(async () => { + beforeEach(() => { subscriberStub = sinon.stub() }) diff --git a/packages/datadog-instrumentations/test/passport-local.spec.js b/packages/datadog-instrumentations/test/passport-local.spec.js index 5133048c680..fcdf5457d63 100644 --- a/packages/datadog-instrumentations/test/passport-local.spec.js +++ b/packages/datadog-instrumentations/test/passport-local.spec.js @@ -68,7 +68,7 @@ withVersions('passport-local', 'passport-local', version => { }) }) }) - beforeEach(async () => { + beforeEach(() => { subscriberStub = sinon.stub() }) From 74f99c215b903e3a0ff953bc813f02742d065f03 Mon Sep 17 00:00:00 2001 From: Julio Gonzalez Date: Wed, 28 Jun 2023 13:57:59 +0200 Subject: [PATCH 32/45] Improve test coverage. --- .../test/passport-http.spec.js | 28 +++++++++++++++ .../test/passport-local.spec.js | 23 ++++++++++++ .../test/passport-utils.spec.js | 36 +++++++++++++++++++ 3 files changed, 87 insertions(+) create mode 100644 packages/datadog-instrumentations/test/passport-utils.spec.js diff --git a/packages/datadog-instrumentations/test/passport-http.spec.js b/packages/datadog-instrumentations/test/passport-http.spec.js index 14a67becff9..e772a6680b3 100644 --- a/packages/datadog-instrumentations/test/passport-http.spec.js +++ b/packages/datadog-instrumentations/test/passport-http.spec.js @@ -44,6 +44,16 @@ withVersions('passport-http', 'passport-http', version => { passport.authenticate('basic', { successRedirect: '/grant', failureRedirect: '/deny', + passReqToCallback: false, + session: false + }) + ) + + app.post('/req', + passport.authenticate('basic', { + successRedirect: '/grant', + failureRedirect: '/deny', + passReqToCallback: true, session: false }) ) @@ -94,6 +104,24 @@ withVersions('passport-http', 'passport-http', version => { ) }) + it('should call subscriber with proper arguments on success with passReqToCallback set to true', async () => { + const res = await axios.get(`http://localhost:${port}/`, { + headers: { + // test:1234 + 'Authorization': 'Basic dGVzdDoxMjM0' + } + }) + + expect(res.status).to.equal(200) + expect(res.data).to.equal('Granted') + expect(subscriberStub).to.be.calledOnceWithExactly( + { + credentials: { type: 'http', username: 'test' }, + user: { _id: 1, username: 'test', password: '1234', email: 'testuser@ddog.com' } + } + ) + }) + it('should call subscriber with proper arguments on failure', async () => { const res = await axios.get(`http://localhost:${port}/`, { headers: { diff --git a/packages/datadog-instrumentations/test/passport-local.spec.js b/packages/datadog-instrumentations/test/passport-local.spec.js index fcdf5457d63..78c9521ab20 100644 --- a/packages/datadog-instrumentations/test/passport-local.spec.js +++ b/packages/datadog-instrumentations/test/passport-local.spec.js @@ -45,6 +45,16 @@ withVersions('passport-local', 'passport-local', version => { passport.authenticate('local', { successRedirect: '/grant', failureRedirect: '/deny', + passReqToCallback: false, + session: false + }) + ) + + app.post('/req', + passport.authenticate('local', { + successRedirect: '/grant', + failureRedirect: '/deny', + passReqToCallback: true, session: false }) ) @@ -90,6 +100,19 @@ withVersions('passport-local', 'passport-local', version => { ) }) + it('should call subscriber with proper arguments on success with passReqToCallback set to true', async () => { + const res = await axios.post(`http://localhost:${port}/req`, { username: 'test', password: '1234' }) + + expect(res.status).to.equal(200) + expect(res.data).to.equal('Granted') + expect(subscriberStub).to.be.calledOnceWithExactly( + { + credentials: { type: 'local', username: 'test' }, + user: { _id: 1, username: 'test', password: '1234', email: 'testuser@ddog.com' } + } + ) + }) + it('should call subscriber with proper arguments on failure', async () => { const res = await axios.post(`http://localhost:${port}/`, { username: 'test', password: '1' }) diff --git a/packages/datadog-instrumentations/test/passport-utils.spec.js b/packages/datadog-instrumentations/test/passport-utils.spec.js new file mode 100644 index 00000000000..ddd1e6c4f2d --- /dev/null +++ b/packages/datadog-instrumentations/test/passport-utils.spec.js @@ -0,0 +1,36 @@ +'use strict' + +const proxyquire = require('proxyquire') +const { channel } = require('../src/helpers/instrument') + +const passportVerifyChannel = channel('datadog:passport:verify:finish') + +describe('passport-utils', () => { + const shimmer = { + wrap: sinon.stub() + } + + let passportUtils + + beforeEach(() => { + passportUtils = proxyquire('../src/passport-utils', { + '../../datadog-shimmer': shimmer + }) + }) + + it('should call wrap when there is no subscribers', () => { + const wrap = passportUtils.wrapVerify(() => {}, false, 'type') + + wrap() + expect(shimmer.wrap).not.to.have.been.called + }) + + it('should call wrap when there is subscribers', () => { + const wrap = passportUtils.wrapVerify(() => {}, false, 'type') + + passportVerifyChannel.subscribe(() => {}) + + wrap() + expect(shimmer.wrap).to.have.been.called + }) +}) From 195f61c59faaa90f2e919008ce6bc015f0319246 Mon Sep 17 00:00:00 2001 From: Julio Gonzalez Date: Wed, 28 Jun 2023 15:13:15 +0200 Subject: [PATCH 33/45] Add express as a dependency in passport tests. --- packages/dd-trace/test/plugins/externals.json | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/packages/dd-trace/test/plugins/externals.json b/packages/dd-trace/test/plugins/externals.json index 9df957678b2..9fce297df9d 100644 --- a/packages/dd-trace/test/plugins/externals.json +++ b/packages/dd-trace/test/plugins/externals.json @@ -175,13 +175,22 @@ { "name": "passport", "versions": [">=0.4.1"] + }, + { + "name": "express", + "versions": [">=4.16.2"] } ], "passport-local": [ { "name": "passport", "versions": [">=0.4.1"] - } + }, + { + "name": "express", + "versions": [">=4.16.2"] + } + ], "pg": [ { From 69afe51d3c2c7ac3d8e0bdbea7853d7bc587e8dd Mon Sep 17 00:00:00 2001 From: simon-id Date: Wed, 28 Jun 2023 15:16:45 +0200 Subject: [PATCH 34/45] move and simplfiy a test --- packages/dd-trace/test/appsec/index.spec.js | 26 +++++++-------------- 1 file changed, 9 insertions(+), 17 deletions(-) diff --git a/packages/dd-trace/test/appsec/index.spec.js b/packages/dd-trace/test/appsec/index.spec.js index d35188f3405..05e3d528c31 100644 --- a/packages/dd-trace/test/appsec/index.spec.js +++ b/packages/dd-trace/test/appsec/index.spec.js @@ -123,6 +123,15 @@ describe('AppSec Index', () => { expect(queryParser.hasSubscribers).to.be.true expect(passportVerify.hasSubscribers).to.be.true }) + + it('should not subscribe to passportVerify if eventTracking is disabled', () => { + config.eventTracking.enabled = false + + AppSec.disable() + AppSec.enable(config) + + expect(passportVerify.hasSubscribers).to.be.false + }) }) describe('disable', () => { @@ -471,23 +480,6 @@ describe('AppSec Index', () => { }) describe('onPassportVerify', () => { - it('Should not subscribe to the channel if disabled', () => { - const config = { - appsec: { - enabled: true, - eventTracking: { - enabled: false, - mode: 'disabled' - } - } - } - - AppSec.disable() - AppSec.enable(config) - - expect(passportVerify.hasSubscribers).to.be.false - }) - it('Should call passportTrackEvent', () => { const credentials = { type: 'local', username: 'test' } const user = { id: '1234', username: 'Test' } From e5bd6b44a8912ee853dc4669946f5598f8b1caac Mon Sep 17 00:00:00 2001 From: simon-id Date: Wed, 28 Jun 2023 15:21:05 +0200 Subject: [PATCH 35/45] fix typo --- packages/datadog-instrumentations/test/passport-utils.spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/datadog-instrumentations/test/passport-utils.spec.js b/packages/datadog-instrumentations/test/passport-utils.spec.js index ddd1e6c4f2d..3cf6a64a60a 100644 --- a/packages/datadog-instrumentations/test/passport-utils.spec.js +++ b/packages/datadog-instrumentations/test/passport-utils.spec.js @@ -18,7 +18,7 @@ describe('passport-utils', () => { }) }) - it('should call wrap when there is no subscribers', () => { + it('should not call wrap when there is no subscribers', () => { const wrap = passportUtils.wrapVerify(() => {}, false, 'type') wrap() From 94236c95de4e30b9db90d58d00cb4bcbd7bf72fb Mon Sep 17 00:00:00 2001 From: Julio Gonzalez Date: Wed, 28 Jun 2023 15:23:13 +0200 Subject: [PATCH 36/45] Remove unused variables. --- packages/dd-trace/test/appsec/index.spec.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/dd-trace/test/appsec/index.spec.js b/packages/dd-trace/test/appsec/index.spec.js index d35188f3405..3708ca8b69a 100644 --- a/packages/dd-trace/test/appsec/index.spec.js +++ b/packages/dd-trace/test/appsec/index.spec.js @@ -491,12 +491,10 @@ describe('AppSec Index', () => { it('Should call passportTrackEvent', () => { const credentials = { type: 'local', username: 'test' } const user = { id: '1234', username: 'Test' } - const err = {} - const info = {} sinon.stub(storage, 'getStore').returns({ req: {} }) - passportVerify.publish({ credentials, user, err, info }) + passportVerify.publish({ credentials, user }) expect(passport.passportTrackEvent).to.have.been.calledOnceWithExactly( credentials, From 7c6968ffc6d5f3b5738a2dfaef2996c2142227ea Mon Sep 17 00:00:00 2001 From: simon-id Date: Wed, 28 Jun 2023 15:38:22 +0200 Subject: [PATCH 37/45] fix test and cleanup --- packages/dd-trace/test/appsec/index.spec.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/dd-trace/test/appsec/index.spec.js b/packages/dd-trace/test/appsec/index.spec.js index a9e9dfd9abf..e901e27e51b 100644 --- a/packages/dd-trace/test/appsec/index.spec.js +++ b/packages/dd-trace/test/appsec/index.spec.js @@ -125,7 +125,7 @@ describe('AppSec Index', () => { }) it('should not subscribe to passportVerify if eventTracking is disabled', () => { - config.eventTracking.enabled = false + config.appsec.eventTracking.enabled = false AppSec.disable() AppSec.enable(config) @@ -498,8 +498,6 @@ describe('AppSec Index', () => { it('Should call log if no rootSpan is found', () => { const credentials = { type: 'local', username: 'test' } const user = { id: '1234', username: 'Test' } - const err = {} - const info = {} sinon.stub(storage, 'getStore').returns(undefined) From 261f57cad2723bddf3e9662f7cd105192d144785 Mon Sep 17 00:00:00 2001 From: simon-id Date: Wed, 28 Jun 2023 15:39:41 +0200 Subject: [PATCH 38/45] cleanup --- packages/dd-trace/test/appsec/index.spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/dd-trace/test/appsec/index.spec.js b/packages/dd-trace/test/appsec/index.spec.js index e901e27e51b..d186d5e21cb 100644 --- a/packages/dd-trace/test/appsec/index.spec.js +++ b/packages/dd-trace/test/appsec/index.spec.js @@ -501,7 +501,7 @@ describe('AppSec Index', () => { sinon.stub(storage, 'getStore').returns(undefined) - passportVerify.publish({ credentials, user, err, info }) + passportVerify.publish({ credentials, user }) expect(log.warn).to.have.been.calledOnceWithExactly('No rootSpan found in onPassportVerify') expect(passport.passportTrackEvent).not.to.have.been.called From a752a19e571772d4b5c09a3713e89e77478c1a3e Mon Sep 17 00:00:00 2001 From: simon-id Date: Wed, 28 Jun 2023 17:12:05 +0200 Subject: [PATCH 39/45] optimize setting user tags --- packages/dd-trace/src/appsec/passport.js | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/packages/dd-trace/src/appsec/passport.js b/packages/dd-trace/src/appsec/passport.js index 4ba40bf24ca..ab031c75415 100644 --- a/packages/dd-trace/src/appsec/passport.js +++ b/packages/dd-trace/src/appsec/passport.js @@ -96,15 +96,13 @@ function passportTrackEvent (credentials, passportUser, rootSpan, mode) { if (passportUser) { // If a passportUser object is published then the login succeded const userTags = {} - Object.entries(user).some((entry) => { - const attr = entry[0].split('.')[1] - userTags[attr] = entry[1] - // Prevent attributes from beint sent under 'users.login.success' prefix - delete user[entry[0]] + Object.entries(user).forEach(([k, v]) => { + const attr = k.split('.', 2)[1] + userTags[attr] = v }) setUserTags(userTags, rootSpan) - trackEvent('users.login.success', user, 'passportTrackEvent', rootSpan, mode) + trackEvent('users.login.success', null, 'passportTrackEvent', rootSpan, mode) } else { trackEvent('users.login.failure', user, 'passportTrackEvent', rootSpan, mode) } From b98d94a4b57dd171759ad3b4847056eed2f71101 Mon Sep 17 00:00:00 2001 From: simon-id Date: Wed, 28 Jun 2023 17:12:41 +0200 Subject: [PATCH 40/45] Apply comment suggestion --- packages/dd-trace/src/appsec/passport.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/dd-trace/src/appsec/passport.js b/packages/dd-trace/src/appsec/passport.js index ab031c75415..ce01981c873 100644 --- a/packages/dd-trace/src/appsec/passport.js +++ b/packages/dd-trace/src/appsec/passport.js @@ -7,7 +7,7 @@ const { setUserTags } = require('./sdk/set_user') const UUID_PATTERN = '^[0-9A-F]{8}-[0-9A-F]{4}-[1-5][0-9A-F]{3}-[89AB][0-9A-F]{3}-[0-9A-F]{12}$' const regexUsername = new RegExp(UUID_PATTERN, 'i') -const SDK_USER_EVENT_PATTERN = '_dd\\.appsec\\.events\\.users\\.[\\W\\w+]+\\.sdk' +const SDK_USER_EVENT_PATTERN = '^_dd\\.appsec\\.events\\.users\\.[\\W\\w+]+\\.sdk$' const regexSdkEvent = new RegExp(SDK_USER_EVENT_PATTERN, 'i') function isSdkCalled (tags) { From 95e8a0453533c2fa01454786501cf83bfe701f02 Mon Sep 17 00:00:00 2001 From: simon-id Date: Wed, 28 Jun 2023 17:13:09 +0200 Subject: [PATCH 41/45] Apply comment suggestion --- packages/dd-trace/src/appsec/passport.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/dd-trace/src/appsec/passport.js b/packages/dd-trace/src/appsec/passport.js index ce01981c873..e3da51029d9 100644 --- a/packages/dd-trace/src/appsec/passport.js +++ b/packages/dd-trace/src/appsec/passport.js @@ -89,7 +89,7 @@ function passportTrackEvent (credentials, passportUser, rootSpan, mode) { const user = parseUser(getLogin(credentials), passportUser, mode) if (user['usr.id'] === undefined) { - log.warn('No username found in authentication instrumentation') + log.warn('No user ID found in authentication instrumentation') return } From 4a6527aac36c32e36f3c9646a92c976b5616f7d6 Mon Sep 17 00:00:00 2001 From: simon-id Date: Wed, 28 Jun 2023 17:15:45 +0200 Subject: [PATCH 42/45] Apply comment suggestion --- packages/dd-trace/src/appsec/passport.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/dd-trace/src/appsec/passport.js b/packages/dd-trace/src/appsec/passport.js index e3da51029d9..3fe0f0428df 100644 --- a/packages/dd-trace/src/appsec/passport.js +++ b/packages/dd-trace/src/appsec/passport.js @@ -20,6 +20,7 @@ function isSdkCalled (tags) { return called } +// delete this function later if we know it's always credential.username function getLogin (credentials) { const type = credentials && credentials.type let login From e30ae8d42756b701a6a3ca8c1bc489989bc36acb Mon Sep 17 00:00:00 2001 From: simon-id Date: Wed, 28 Jun 2023 17:16:01 +0200 Subject: [PATCH 43/45] Apply comment suggestion --- packages/dd-trace/src/appsec/passport.js | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/packages/dd-trace/src/appsec/passport.js b/packages/dd-trace/src/appsec/passport.js index 3fe0f0428df..47b7ab446ea 100644 --- a/packages/dd-trace/src/appsec/passport.js +++ b/packages/dd-trace/src/appsec/passport.js @@ -24,12 +24,8 @@ function isSdkCalled (tags) { function getLogin (credentials) { const type = credentials && credentials.type let login - if (type === 'local') { + if (type === 'local' || type === 'http) { login = credentials.username - } else if (type === 'http') { - login = credentials.username - } else { - login = undefined } return login From c55380ed7fbd4d5ea8cfa162be6c9e3434ef9817 Mon Sep 17 00:00:00 2001 From: simon-id Date: Wed, 28 Jun 2023 17:22:45 +0200 Subject: [PATCH 44/45] missing quote --- packages/dd-trace/src/appsec/passport.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/dd-trace/src/appsec/passport.js b/packages/dd-trace/src/appsec/passport.js index 47b7ab446ea..0e745288bd9 100644 --- a/packages/dd-trace/src/appsec/passport.js +++ b/packages/dd-trace/src/appsec/passport.js @@ -24,7 +24,7 @@ function isSdkCalled (tags) { function getLogin (credentials) { const type = credentials && credentials.type let login - if (type === 'local' || type === 'http) { + if (type === 'local' || type === 'http') { login = credentials.username } From 4eed545e92f2b9266b86674039e25edf95a4e0d4 Mon Sep 17 00:00:00 2001 From: Julio Gonzalez Date: Wed, 28 Jun 2023 18:38:19 +0200 Subject: [PATCH 45/45] Fix PR comments. --- .../dd-trace/test/appsec/passport.spec.js | 53 ++++++++++--------- 1 file changed, 29 insertions(+), 24 deletions(-) diff --git a/packages/dd-trace/test/appsec/passport.spec.js b/packages/dd-trace/test/appsec/passport.spec.js index 9d3e59d07fc..9d1ad8a0e00 100644 --- a/packages/dd-trace/test/appsec/passport.spec.js +++ b/packages/dd-trace/test/appsec/passport.spec.js @@ -15,6 +15,8 @@ describe('Passport', () => { let passportModule, log, events, setUser beforeEach(() => { + rootSpan.context = () => { return {} } + log = { warn: sinon.stub() } @@ -38,7 +40,7 @@ describe('Passport', () => { it('should call log when credentials is undefined', () => { passportModule.passportTrackEvent(undefined, undefined, undefined, 'safe') - expect(log.warn).to.have.been.calledOnceWithExactly('No username found in authentication instrumentation') + expect(log.warn).to.have.been.calledOnceWithExactly('No user ID found in authentication instrumentation') }) it('should call log when type is not known', () => { @@ -46,15 +48,15 @@ describe('Passport', () => { passportModule.passportTrackEvent(credentials, undefined, undefined, 'safe') - expect(log.warn).to.have.been.calledOnceWithExactly('No username found in authentication instrumentation') + expect(log.warn).to.have.been.calledOnceWithExactly('No user ID found in authentication instrumentation') }) it('should call log when type is known but username not present', () => { - const credentials = { type: 'unknown' } + const credentials = { type: 'http' } passportModule.passportTrackEvent(credentials, undefined, undefined, 'safe') - expect(log.warn).to.have.been.calledOnceWithExactly('No username found in authentication instrumentation') + expect(log.warn).to.have.been.calledOnceWithExactly('No user ID found in authentication instrumentation') }) it('should report login failure when passportUser is not present', () => { @@ -76,7 +78,7 @@ describe('Passport', () => { expect(setUser.setUserTags).to.have.been.calledOnceWithExactly({ id: userUuid.id }, rootSpan) expect(events.trackEvent).to.have.been.calledOnceWithExactly( 'users.login.success', - {}, + null, 'passportTrackEvent', rootSpan, 'safe' @@ -84,15 +86,17 @@ describe('Passport', () => { }) it('should report login success and blank id in safe mode when id is not a uuid', () => { - const user = userUuid - - user.id = 'publicName' + const user = { + id: 'publicName', + email: 'testUser@test.com', + username: 'Test User' + } passportModule.passportTrackEvent(loginLocal, user, rootSpan, 'safe') expect(setUser.setUserTags).to.have.been.calledOnceWithExactly({ id: ' ' }, rootSpan) expect(events.trackEvent).to.have.been.calledOnceWithExactly( 'users.login.success', - {}, + null, 'passportTrackEvent', rootSpan, 'safe' @@ -100,13 +104,16 @@ describe('Passport', () => { }) it('should report login success and the extended fields in extended mode', () => { - const user = userUuid + const user = { + id: 'publicName', + email: 'testUser@test.com', + username: 'Test User' + } - user.id = 'publicName' passportModule.passportTrackEvent(loginLocal, user, rootSpan, 'extended') expect(setUser.setUserTags).to.have.been.calledOnceWithExactly( { - id: user.id, + id: 'publicName', login: 'test', email: 'testUser@test.com', username: 'Test User' @@ -115,7 +122,7 @@ describe('Passport', () => { ) expect(events.trackEvent).to.have.been.calledOnceWithExactly( 'users.login.success', - {}, + null, 'passportTrackEvent', rootSpan, 'extended' @@ -136,7 +143,7 @@ describe('Passport', () => { expect(events.trackEvent).not.to.have.been.called }) - it('should not call trackEvent in extended mode if sdk user event functions are already called', () => { + it('should not call trackEvent in extended mode if trackUserLoginSuccessEvent is already called', () => { rootSpan.context = () => { return { _tags: { @@ -150,7 +157,7 @@ describe('Passport', () => { expect(events.trackEvent).not.to.have.been.called }) - it('should call trackEvent in extended mode if sdk custom event function is already called', () => { + it('should call trackEvent in extended mode if trackCustomEvent function is already called', () => { rootSpan.context = () => { return { _tags: { @@ -162,18 +169,18 @@ describe('Passport', () => { passportModule.passportTrackEvent(loginLocal, userUuid, rootSpan, 'extended') expect(events.trackEvent).to.have.been.calledOnceWithExactly( 'users.login.success', - {}, + null, 'passportTrackEvent', rootSpan, 'extended' ) }) - it('should call trackEvent in extended mode if sdk dy called', () => { + it('should not call trackEvent in extended mode if trackUserLoginFailureEvent is already called', () => { rootSpan.context = () => { return { _tags: { - '_dd.appsec.events.users.login.success.sdk': 'true' + '_dd.appsec.events.users.login.failure.sdk': 'true' } } } @@ -190,12 +197,10 @@ describe('Passport', () => { username: 'Test User' } - rootSpan.context = () => { return {} } - passportModule.passportTrackEvent(loginLocal, user, rootSpan, 'extended') expect(setUser.setUserTags).to.have.been.calledOnceWithExactly( { - id: user._id, + id: '591dc126-8431-4d0f-9509-b23318d3dce4', login: 'test', email: 'testUser@test.com', username: 'Test User' @@ -204,7 +209,7 @@ describe('Passport', () => { ) expect(events.trackEvent).to.have.been.calledOnceWithExactly( 'users.login.success', - {}, + null, 'passportTrackEvent', rootSpan, 'extended' @@ -222,14 +227,14 @@ describe('Passport', () => { passportModule.passportTrackEvent(loginLocal, user, rootSpan, 'extended') expect(setUser.setUserTags).to.have.been.calledOnceWithExactly( { - id: loginLocal.username, + id: 'test', login: 'test', email: 'testUser@test.com', username: 'Test User' }, rootSpan) expect(events.trackEvent).to.have.been.calledOnceWithExactly( 'users.login.success', - {}, + null, 'passportTrackEvent', rootSpan, 'extended'