From 81f6ab5be22cb129e97caaa0e636f7bdaa91443d Mon Sep 17 00:00:00 2001 From: simon-id Date: Wed, 11 Sep 2024 13:46:53 +0200 Subject: [PATCH 001/126] instrument passport --- .../src/helpers/hooks.js | 1 + .../datadog-instrumentations/src/passport.js | 79 +++++++++++++++++++ 2 files changed, 80 insertions(+) create mode 100644 packages/datadog-instrumentations/src/passport.js diff --git a/packages/datadog-instrumentations/src/helpers/hooks.js b/packages/datadog-instrumentations/src/helpers/hooks.js index 284e4ed5950..892517ed72d 100644 --- a/packages/datadog-instrumentations/src/helpers/hooks.js +++ b/packages/datadog-instrumentations/src/helpers/hooks.js @@ -92,6 +92,7 @@ module.exports = { oracledb: () => require('../oracledb'), openai: () => require('../openai'), paperplane: () => require('../paperplane'), + passport: () => require('../passport'), 'passport-http': () => require('../passport-http'), 'passport-local': () => require('../passport-local'), pg: () => require('../pg'), diff --git a/packages/datadog-instrumentations/src/passport.js b/packages/datadog-instrumentations/src/passport.js new file mode 100644 index 00000000000..c3c8be99b4a --- /dev/null +++ b/packages/datadog-instrumentations/src/passport.js @@ -0,0 +1,79 @@ +'use strict' + +const shimmer = require('../../datadog-shimmer') +const { addHook } = require('./helpers/instrument') + +/* TODO: test with: +passport-jwt +@nestjs/passport +pasport-local +passport-oauth2 +passport-google-oauth20 +passport-custom +passport-http +passport-http-bearer +koa-passport +*/ + +function wrapDone (done) { + // eslint-disable-next-line n/handle-callback-err + return function wrappedDone (err, user) { + console.log('USER DESERIALIZE', user) + if (user) { + // channel.publish({ req, user }) + } + + return done.apply(this, arguments) + } +} + +function wrapDeserializeUser (deserializeUser) { + return function wrappedDeserializeUser (fn, req, done) { + if (typeof req === 'function') { + done = req + // req = storage.getStore().get('req') + arguments[1] = wrapDone(done) + } else { + arguments[2] = wrapDone(done) + } + + return deserializeUser.apply(this, arguments) + } +} + + +const { block } = require('../../dd-trace/src/appsec/blocking') +const { getRootSpan } = require('../../dd-trace/src/appsec/sdk/utils') + +addHook({ + name: 'passport', + file: 'lib/authenticator.js', + versions: ['>=0.3.0'] // TODO +}, Authenticator => { + shimmer.wrap(Authenticator.prototype, 'deserializeUser', wrapDeserializeUser) + + shimmer.wrap(Authenticator.prototype, 'authenticate', function wrapAuthenticate (authenticate) { + return function wrappedAuthenticate (name) { + const middleware = authenticate.apply(this, arguments) + + const strategy = this._strategy(name) + + strategy._verify + + return function wrappedMiddleware (req, res, next) { + return middleware(req, res, function wrappedNext (err) { + strategy + console.log('NEW', req.user) + if (req.user?.name === 'bitch') { + + return block(req, res, getRootSpan(global._ddtrace)) + } + + return next.apply(this, arguments) + }) + } + } + }) + + return Authenticator +}) From 023828ca1278ee2ad633a11b55acf8cf2270654b Mon Sep 17 00:00:00 2001 From: simon-id Date: Wed, 11 Sep 2024 14:10:20 +0200 Subject: [PATCH 002/126] lint --- packages/dd-trace/src/config.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/dd-trace/src/config.js b/packages/dd-trace/src/config.js index 5c9c1372160..49127b44ad9 100644 --- a/packages/dd-trace/src/config.js +++ b/packages/dd-trace/src/config.js @@ -60,7 +60,7 @@ function checkIfBothOtelAndDdEnvVarSet () { log.warn(`both ${ddVar} and ${otelVar} environment variables are set`) getCounter('otel.env.hiding', ddVar, otelVar, otelVar === 'OTEL_TRACES_SAMPLER' && - process.env.OTEL_TRACES_SAMPLER_ARG + process.env.OTEL_TRACES_SAMPLER_ARG ? 'OTEL_TRACES_SAMPLER_ARG' : undefined).inc() } @@ -714,7 +714,7 @@ class Config { ? false : undefined this._setBoolean(env, 'runtimeMetrics', DD_RUNTIME_METRICS_ENABLED || - otelSetRuntimeMetrics) + otelSetRuntimeMetrics) const OTEL_TRACES_SAMPLER_MAPPING = { always_on: '1.0', always_off: '0.0', From 98090fbf209b4f397934fb85915fa7404da22ecf Mon Sep 17 00:00:00 2001 From: simon-id Date: Wed, 11 Sep 2024 17:54:21 +0200 Subject: [PATCH 003/126] update config --- packages/dd-trace/src/config.js | 35 +++++++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 6 deletions(-) diff --git a/packages/dd-trace/src/config.js b/packages/dd-trace/src/config.js index 49127b44ad9..018234ae53a 100644 --- a/packages/dd-trace/src/config.js +++ b/packages/dd-trace/src/config.js @@ -252,11 +252,33 @@ class Config { maybeFile(options.appsec.blockedTemplateGraphql), maybeFile(process.env.DD_APPSEC_GRAPHQL_BLOCKED_TEMPLATE_JSON) ) - const DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING = coalesce( - options.appsec.eventTracking && options.appsec.eventTracking.mode, - process.env.DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING, - 'safe' + + const DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING_ENABLED = coalesce( + options.appsec.eventTracking?.enabled, + process.env.DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING_ENABLED && + isTrue(process.env.DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING_ENABLED), + true + ) + + // little util for retrocompatibility, delete in next major + function convertEventTrackingMode (mode) { + if (typeof mode !== 'string') return mode + + mode = mode.toLowerCase() + + if (mode === 'extended') return 'ident' + if (mode === 'safe') return 'anon' + + return mode + } + + const DD_APPSEC_AUTO_USER_INSTRUMENTATION_MODE = coalesce( + convertEventTrackingMode(options.appsec.eventTracking?.mode), + process.env.DD_APPSEC_AUTO_USER_INSTRUMENTATION_MODE, + convertEventTrackingMode(process.env.DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING), // TODO: make it deprecated + 'ident' ).toLowerCase() + const DD_API_SECURITY_ENABLED = coalesce( options.appsec?.apiSecurity?.enabled, process.env.DD_API_SECURITY_ENABLED && isTrue(process.env.DD_API_SECURITY_ENABLED), @@ -319,8 +341,9 @@ class Config { this.appsec = { blockedTemplateGraphql: DD_APPSEC_GRAPHQL_BLOCKED_TEMPLATE_JSON, eventTracking: { - enabled: ['extended', 'safe'].includes(DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING), - mode: DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING + enabled: DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING_ENABLED && + ['anon', 'ident'].includes(DD_APPSEC_AUTO_USER_INSTRUMENTATION_MODE), + mode: DD_APPSEC_AUTO_USER_INSTRUMENTATION_MODE }, apiSecurity: { enabled: DD_API_SECURITY_ENABLED, From 5cc727472f63848cb57154a0bb27ee99d5a7345d Mon Sep 17 00:00:00 2001 From: simon-id Date: Wed, 11 Sep 2024 17:54:47 +0200 Subject: [PATCH 004/126] add new RC capability --- packages/dd-trace/src/appsec/remote_config/capabilities.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/dd-trace/src/appsec/remote_config/capabilities.js b/packages/dd-trace/src/appsec/remote_config/capabilities.js index 6e320493336..97c887460ea 100644 --- a/packages/dd-trace/src/appsec/remote_config/capabilities.js +++ b/packages/dd-trace/src/appsec/remote_config/capabilities.js @@ -17,5 +17,6 @@ module.exports = { APM_TRACING_HTTP_HEADER_TAGS: 1n << 14n, APM_TRACING_CUSTOM_TAGS: 1n << 15n, APM_TRACING_ENABLED: 1n << 19n, - APM_TRACING_SAMPLE_RULES: 1n << 29n + APM_TRACING_SAMPLE_RULES: 1n << 29n, + ASM_AUTO_USER_INSTRUM_MODE: 1n << 31n } From 4e127de991099605a9e75470045ed312e3234453 Mon Sep 17 00:00:00 2001 From: simon-id Date: Thu, 12 Sep 2024 15:07:16 +0200 Subject: [PATCH 005/126] add RC handler --- packages/dd-trace/src/appsec/remote_config/index.js | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/packages/dd-trace/src/appsec/remote_config/index.js b/packages/dd-trace/src/appsec/remote_config/index.js index b63b3690102..240d101e6a6 100644 --- a/packages/dd-trace/src/appsec/remote_config/index.js +++ b/packages/dd-trace/src/appsec/remote_config/index.js @@ -5,6 +5,7 @@ const Activation = require('../activation') const RemoteConfigManager = require('./manager') const RemoteConfigCapabilities = require('./capabilities') const apiSecuritySampler = require('../api_security_sampler') +const { setCollectionMode } = require('./passport') let rc @@ -28,6 +29,8 @@ function enable (config, appsec) { rc.updateCapabilities(RemoteConfigCapabilities.ASM_API_SECURITY_SAMPLE_RATE, true) } + rc.updateCapabilities(RemoteConfigCapabilities.ASM_AUTO_USER_INSTRUM_MODE, true) + rc.setProductHandler('ASM_FEATURES', (action, rcConfig) => { if (!rcConfig) return @@ -36,6 +39,14 @@ function enable (config, appsec) { } apiSecuritySampler.setRequestSampling(rcConfig.api_security?.request_sample_rate) + + if (typeof rcConfig.auto_user_instrum?.mode === 'string') { + if (action === 'apply' || action === 'modify') { + setCollectionMode(rcConfig.auto_user_instrum.mode) + } else { + setCollectionMode(config.appsec.eventTracking.mode) + } + } }) } From b4daf8fa3172b4ca2807abedd9acaa5653f8cc14 Mon Sep 17 00:00:00 2001 From: simon-id Date: Thu, 12 Sep 2024 15:07:53 +0200 Subject: [PATCH 006/126] fix require path --- packages/dd-trace/src/appsec/remote_config/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/dd-trace/src/appsec/remote_config/index.js b/packages/dd-trace/src/appsec/remote_config/index.js index 240d101e6a6..658d5f33a79 100644 --- a/packages/dd-trace/src/appsec/remote_config/index.js +++ b/packages/dd-trace/src/appsec/remote_config/index.js @@ -5,7 +5,7 @@ const Activation = require('../activation') const RemoteConfigManager = require('./manager') const RemoteConfigCapabilities = require('./capabilities') const apiSecuritySampler = require('../api_security_sampler') -const { setCollectionMode } = require('./passport') +const { setCollectionMode } = require('../passport') let rc From 5f85b8e9c23a3ef84baebfb224628f1874f67098 Mon Sep 17 00:00:00 2001 From: simon-id Date: Thu, 12 Sep 2024 15:09:56 +0200 Subject: [PATCH 007/126] add setCollectionMode() --- packages/dd-trace/src/appsec/passport.js | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/packages/dd-trace/src/appsec/passport.js b/packages/dd-trace/src/appsec/passport.js index 2093b7b1fdc..b1eeb26205d 100644 --- a/packages/dd-trace/src/appsec/passport.js +++ b/packages/dd-trace/src/appsec/passport.js @@ -10,6 +10,23 @@ const regexUsername = new RegExp(UUID_PATTERN, 'i') const SDK_USER_EVENT_PATTERN = '^_dd\\.appsec\\.events\\.users\\.[\\W\\w+]+\\.sdk$' const regexSdkEvent = new RegExp(SDK_USER_EVENT_PATTERN, 'i') +let collectionMode + +function setCollectionMode (mode) { + switch (mode) { + case 'ident': + case 'identification': + collectionMode = 'ident' + break + case 'anon': + case 'anonymization': + collectionMode = 'anon' + break + default: + collectionMode = null // disabled + } +} + function isSdkCalled (tags) { let called = false @@ -106,5 +123,6 @@ function passportTrackEvent (credentials, passportUser, rootSpan, mode) { } module.exports = { - passportTrackEvent + passportTrackEvent, + setCollectionMode } From 1b074adc0abf53c1397889ec3ea94877c1946a1d Mon Sep 17 00:00:00 2001 From: simon-id Date: Thu, 12 Sep 2024 15:10:24 +0200 Subject: [PATCH 008/126] use setCollectionMode() in appsec index --- packages/dd-trace/src/appsec/index.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/dd-trace/src/appsec/index.js b/packages/dd-trace/src/appsec/index.js index 10e63ebd2de..cc82d061a9d 100644 --- a/packages/dd-trace/src/appsec/index.js +++ b/packages/dd-trace/src/appsec/index.js @@ -25,7 +25,7 @@ const web = require('../plugins/util/web') const { extractIp } = require('../plugins/util/ip_extractor') const { HTTP_CLIENT_IP } = require('../../../../ext/tags') const { isBlocked, block, setTemplates, getBlockingAction } = require('./blocking') -const { passportTrackEvent } = require('./passport') +const { setCollectionMode, passportTrackEvent } = require('./passport') const { storage } = require('../../../datadog-core') const graphql = require('./graphql') const rasp = require('./rasp') @@ -66,6 +66,7 @@ function enable (_config) { responseSetHeader.subscribe(onResponseSetHeader) if (_config.appsec.eventTracking.enabled) { + setCollectionMode(_config.appsec.eventTracking.mode) passportVerify.subscribe(onPassportVerify) } From cbc8cf3153f68e2946d0da716efea55eca3982ae Mon Sep 17 00:00:00 2001 From: simon-id Date: Tue, 17 Sep 2024 12:06:50 +0200 Subject: [PATCH 009/126] DRY up passport strategies instrumentation --- .../src/passport-http.js | 16 ++------------ .../src/passport-local.js | 16 ++------------ .../src/passport-utils.js | 21 ++++++++++++++++++- 3 files changed, 24 insertions(+), 29 deletions(-) diff --git a/packages/datadog-instrumentations/src/passport-http.js b/packages/datadog-instrumentations/src/passport-http.js index 0969d2d3fc9..7f9eff4bd87 100644 --- a/packages/datadog-instrumentations/src/passport-http.js +++ b/packages/datadog-instrumentations/src/passport-http.js @@ -1,22 +1,10 @@ 'use strict' -const shimmer = require('../../datadog-shimmer') const { addHook } = require('./helpers/instrument') -const { wrapVerify } = require('./passport-utils') +const { createStrategyHook } = require('./passport-utils') addHook({ name: 'passport-http', file: 'lib/passport-http/strategies/basic.js', versions: ['>=0.3.0'] -}, BasicStrategy => { - return shimmer.wrapFunction(BasicStrategy, BasicStrategy => function () { - const type = 'http' - - if (typeof arguments[0] === 'function') { - arguments[0] = wrapVerify(arguments[0], false, type) - } else { - arguments[1] = wrapVerify(arguments[1], (arguments[0] && arguments[0].passReqToCallback), type) - } - return BasicStrategy.apply(this, arguments) - }) -}) +}, createStrategyHook('http')) diff --git a/packages/datadog-instrumentations/src/passport-local.js b/packages/datadog-instrumentations/src/passport-local.js index dab74eb470e..11013f71eb0 100644 --- a/packages/datadog-instrumentations/src/passport-local.js +++ b/packages/datadog-instrumentations/src/passport-local.js @@ -1,22 +1,10 @@ 'use strict' -const shimmer = require('../../datadog-shimmer') const { addHook } = require('./helpers/instrument') -const { wrapVerify } = require('./passport-utils') +const { createStrategyHook } = require('./passport-utils') addHook({ name: 'passport-local', file: 'lib/strategy.js', versions: ['>=1.0.0'] -}, Strategy => { - return shimmer.wrapFunction(Strategy, Strategy => function () { - const type = 'local' - - if (typeof arguments[0] === 'function') { - arguments[0] = wrapVerify(arguments[0], false, type) - } else { - arguments[1] = wrapVerify(arguments[1], (arguments[0] && arguments[0].passReqToCallback), type) - } - return Strategy.apply(this, arguments) - }) -}) +}, createStrategyHook('local')) diff --git a/packages/datadog-instrumentations/src/passport-utils.js b/packages/datadog-instrumentations/src/passport-utils.js index 7969ab486b4..acf45f53f0c 100644 --- a/packages/datadog-instrumentations/src/passport-utils.js +++ b/packages/datadog-instrumentations/src/passport-utils.js @@ -32,6 +32,25 @@ function wrapVerify (verify, passReq, type) { } } +function createWrapStrategy (type) { + return function wrapStrategy (Strategy) { + return function wrappedStrategy () { + if (typeof arguments[0] === 'function') { + arguments[0] = wrapVerify(arguments[0], false, type) + } else { + arguments[1] = wrapVerify(arguments[1], (arguments[0] && arguments[0].passReqToCallback), type) + } + return Strategy.apply(this, arguments) + } + } +} + +function createStrategyHook (type) { + return function strategyHook (Strategy) { + return shimmer.wrapFunction(Strategy, createWrapStrategy(type)) + } +} + module.exports = { - wrapVerify + createStrategyHook } From 729241d746bb56adb570993e600d637eafbb589c Mon Sep 17 00:00:00 2001 From: simon-id Date: Tue, 17 Sep 2024 13:50:23 +0200 Subject: [PATCH 010/126] simplify passport strategies instrumentations --- .../src/passport-http.js | 4 +-- .../src/passport-local.js | 4 +-- .../src/passport-utils.js | 30 ++++++++----------- 3 files changed, 17 insertions(+), 21 deletions(-) diff --git a/packages/datadog-instrumentations/src/passport-http.js b/packages/datadog-instrumentations/src/passport-http.js index 7f9eff4bd87..3b930a1a1cc 100644 --- a/packages/datadog-instrumentations/src/passport-http.js +++ b/packages/datadog-instrumentations/src/passport-http.js @@ -1,10 +1,10 @@ 'use strict' const { addHook } = require('./helpers/instrument') -const { createStrategyHook } = require('./passport-utils') +const { strategyHook } = require('./passport-utils') addHook({ name: 'passport-http', file: 'lib/passport-http/strategies/basic.js', versions: ['>=0.3.0'] -}, createStrategyHook('http')) +}, strategyHook) diff --git a/packages/datadog-instrumentations/src/passport-local.js b/packages/datadog-instrumentations/src/passport-local.js index 11013f71eb0..c6dcec9a48d 100644 --- a/packages/datadog-instrumentations/src/passport-local.js +++ b/packages/datadog-instrumentations/src/passport-local.js @@ -1,10 +1,10 @@ 'use strict' const { addHook } = require('./helpers/instrument') -const { createStrategyHook } = require('./passport-utils') +const { strategyHook } = require('./passport-utils') addHook({ name: 'passport-local', file: 'lib/strategy.js', versions: ['>=1.0.0'] -}, createStrategyHook('local')) +}, strategyHook) diff --git a/packages/datadog-instrumentations/src/passport-utils.js b/packages/datadog-instrumentations/src/passport-utils.js index acf45f53f0c..18c44f6987a 100644 --- a/packages/datadog-instrumentations/src/passport-utils.js +++ b/packages/datadog-instrumentations/src/passport-utils.js @@ -18,39 +18,35 @@ function wrapVerifiedAndPublish (username, password, verified, type) { }) } -function wrapVerify (verify, passReq, type) { +function wrapVerify (verify, passReq) { if (passReq) { return function (req, username, password, verified) { - arguments[3] = wrapVerifiedAndPublish(username, password, verified, type) + arguments[3] = wrapVerifiedAndPublish(username, password, verified, this.name) return verify.apply(this, arguments) } } else { return function (username, password, verified) { - arguments[2] = wrapVerifiedAndPublish(username, password, verified, type) + arguments[2] = wrapVerifiedAndPublish(username, password, verified, this.name) return verify.apply(this, arguments) } } } -function createWrapStrategy (type) { - return function wrapStrategy (Strategy) { - return function wrappedStrategy () { - if (typeof arguments[0] === 'function') { - arguments[0] = wrapVerify(arguments[0], false, type) - } else { - arguments[1] = wrapVerify(arguments[1], (arguments[0] && arguments[0].passReqToCallback), type) - } - return Strategy.apply(this, arguments) +function wrapStrategy (Strategy) { + return function wrappedStrategy () { + if (typeof arguments[0] === 'function') { + arguments[0] = wrapVerify(arguments[0], false) + } else { + arguments[1] = wrapVerify(arguments[1], (arguments[0] && arguments[0].passReqToCallback)) } + return Strategy.apply(this, arguments) } } -function createStrategyHook (type) { - return function strategyHook (Strategy) { - return shimmer.wrapFunction(Strategy, createWrapStrategy(type)) - } +return function strategyHook (Strategy) { + return shimmer.wrapFunction(Strategy, wrapStrategy) } module.exports = { - createStrategyHook + strategyHook } From 6ffc21f6728771e72cb88af595f47c6e7240dede Mon Sep 17 00:00:00 2001 From: simon-id Date: Tue, 17 Sep 2024 14:13:57 +0200 Subject: [PATCH 011/126] simplify instrumentation --- .../src/passport-utils.js | 27 ++++++++++--------- 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/packages/datadog-instrumentations/src/passport-utils.js b/packages/datadog-instrumentations/src/passport-utils.js index 18c44f6987a..242d2d9c92a 100644 --- a/packages/datadog-instrumentations/src/passport-utils.js +++ b/packages/datadog-instrumentations/src/passport-utils.js @@ -18,26 +18,29 @@ function wrapVerifiedAndPublish (username, password, verified, type) { }) } -function wrapVerify (verify, passReq) { - if (passReq) { - return function (req, username, password, verified) { - arguments[3] = wrapVerifiedAndPublish(username, password, verified, this.name) - return verify.apply(this, arguments) - } - } else { - return function (username, password, verified) { - arguments[2] = wrapVerifiedAndPublish(username, password, verified, this.name) - return verify.apply(this, arguments) +function wrapVerify (verify) { + return function wrappedVerify (req, username, password, verified) { + let index = 3 + + if (!this._passReqToCallback) { + index = 2 + username = req + password = username + verified = password } + + arguments[index] = wrapVerifiedAndPublish(username, password, verified, this.name) + + return verify.apply(this, arguments) } } function wrapStrategy (Strategy) { return function wrappedStrategy () { if (typeof arguments[0] === 'function') { - arguments[0] = wrapVerify(arguments[0], false) + arguments[0] = wrapVerify(arguments[0]) } else { - arguments[1] = wrapVerify(arguments[1], (arguments[0] && arguments[0].passReqToCallback)) + arguments[1] = wrapVerify(arguments[1]) } return Strategy.apply(this, arguments) } From 4433baafaba6be41bc28f2238edeea1d8474d7b5 Mon Sep 17 00:00:00 2001 From: simon-id Date: Tue, 17 Sep 2024 14:16:57 +0200 Subject: [PATCH 012/126] note for later --- packages/datadog-instrumentations/src/passport-utils.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/datadog-instrumentations/src/passport-utils.js b/packages/datadog-instrumentations/src/passport-utils.js index 242d2d9c92a..004493e55cb 100644 --- a/packages/datadog-instrumentations/src/passport-utils.js +++ b/packages/datadog-instrumentations/src/passport-utils.js @@ -12,6 +12,7 @@ function wrapVerifiedAndPublish (username, password, verified, type) { // eslint-disable-next-line n/handle-callback-err return shimmer.wrapFunction(verified, verified => function (err, user, info) { + if (err) return // ???? const credentials = { type, username } passportVerifyChannel.publish({ credentials, user }) return verified.apply(this, arguments) From cf39858b278262eaf436b9037db5392548fa78d7 Mon Sep 17 00:00:00 2001 From: simon-id Date: Tue, 17 Sep 2024 14:19:19 +0200 Subject: [PATCH 013/126] add blocking to login --- packages/datadog-instrumentations/src/passport-utils.js | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/packages/datadog-instrumentations/src/passport-utils.js b/packages/datadog-instrumentations/src/passport-utils.js index 004493e55cb..6a237306605 100644 --- a/packages/datadog-instrumentations/src/passport-utils.js +++ b/packages/datadog-instrumentations/src/passport-utils.js @@ -14,7 +14,14 @@ function wrapVerifiedAndPublish (username, password, verified, type) { return shimmer.wrapFunction(verified, verified => function (err, user, info) { if (err) return // ???? const credentials = { type, username } - passportVerifyChannel.publish({ credentials, user }) + const abortController = new AbortController() + + passportVerifyChannel.publish({ credentials, user, abortController }) + + if (abortController.signal.aborted) { + arguments[0] = new Error('Blocked') + } + return verified.apply(this, arguments) }) } From d8bed471fbc3ce58ea75cb18ef9cd2b53e740e2e Mon Sep 17 00:00:00 2001 From: simon-id Date: Tue, 17 Sep 2024 15:56:38 +0200 Subject: [PATCH 014/126] add abortController in listener --- 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 10e63ebd2de..89e09d3c3ed 100644 --- a/packages/dd-trace/src/appsec/index.js +++ b/packages/dd-trace/src/appsec/index.js @@ -212,7 +212,7 @@ function onResponseBody ({ req, body }) { }, req) } -function onPassportVerify ({ credentials, user }) { +function onPassportVerify ({ credentials, user, abortController }) { const store = storage.getStore() const rootSpan = store?.req && web.root(store.req) From 5105006d5780823962e87bd2d1d3174b3a29aabf Mon Sep 17 00:00:00 2001 From: simon-id Date: Sun, 20 Oct 2024 22:25:48 +0200 Subject: [PATCH 015/126] cleanup --- 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 cc82d061a9d..42021d151e3 100644 --- a/packages/dd-trace/src/appsec/index.js +++ b/packages/dd-trace/src/appsec/index.js @@ -222,7 +222,7 @@ function onPassportVerify ({ credentials, user }) { return } - passportTrackEvent(credentials, user, rootSpan, config.appsec.eventTracking.mode) + passportTrackEvent(credentials, user, rootSpan) } const responseAnalyzedSet = new WeakSet() From 65c680b80c5a7480b2abaa6e42ec9ba69c962828 Mon Sep 17 00:00:00 2001 From: simon-id Date: Sun, 20 Oct 2024 22:27:02 +0200 Subject: [PATCH 016/126] update typings --- index.d.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/index.d.ts b/index.d.ts index 1d0e2473d01..f16dc639af4 100644 --- a/index.d.ts +++ b/index.d.ts @@ -651,7 +651,7 @@ declare namespace tracer { * On extended mode, no redaction will take place. * @default 'safe' */ - mode?: 'safe' | 'extended' | 'disabled' + mode?: 'anonymous' | 'identification' | 'safe' | 'extended' | 'disabled' }, /** * Configuration for Api Security sampling From 23f965d566000794c3bbdcf0e9e64aef6bfeee21 Mon Sep 17 00:00:00 2001 From: simon-id Date: Sun, 20 Oct 2024 22:28:06 +0200 Subject: [PATCH 017/126] cleanup --- packages/dd-trace/src/appsec/sdk/track_event.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/dd-trace/src/appsec/sdk/track_event.js b/packages/dd-trace/src/appsec/sdk/track_event.js index 61500e2cfbe..a80ddd2455c 100644 --- a/packages/dd-trace/src/appsec/sdk/track_event.js +++ b/packages/dd-trace/src/appsec/sdk/track_event.js @@ -63,7 +63,7 @@ function trackEvent (eventName, fields, sdkMethodName, rootSpan, mode) { tags[`_dd.appsec.events.${eventName}.sdk`] = 'true' } - if (mode === 'safe' || mode === 'extended') { + if (mode && mode !== 'sdk') { tags[`_dd.appsec.events.${eventName}.auto.mode`] = mode } From b5b336afb780c6c2517a81e8049a1c8930d2d9bc Mon Sep 17 00:00:00 2001 From: simon-id Date: Sun, 20 Oct 2024 22:32:02 +0200 Subject: [PATCH 018/126] RC config update --- packages/dd-trace/src/appsec/remote_config/index.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/dd-trace/src/appsec/remote_config/index.js b/packages/dd-trace/src/appsec/remote_config/index.js index 658d5f33a79..3dd5422e571 100644 --- a/packages/dd-trace/src/appsec/remote_config/index.js +++ b/packages/dd-trace/src/appsec/remote_config/index.js @@ -29,7 +29,9 @@ function enable (config, appsec) { rc.updateCapabilities(RemoteConfigCapabilities.ASM_API_SECURITY_SAMPLE_RATE, true) } - rc.updateCapabilities(RemoteConfigCapabilities.ASM_AUTO_USER_INSTRUM_MODE, true) + if (config.appsec.eventTracking?.enabled) { + rc.updateCapabilities(RemoteConfigCapabilities.ASM_AUTO_USER_INSTRUM_MODE, true) + } rc.setProductHandler('ASM_FEATURES', (action, rcConfig) => { if (!rcConfig) return @@ -40,7 +42,8 @@ function enable (config, appsec) { apiSecuritySampler.setRequestSampling(rcConfig.api_security?.request_sample_rate) - if (typeof rcConfig.auto_user_instrum?.mode === 'string') { + // TODO: does local config take precedence? + if (config.appsec.eventTracking?.enabled && typeof rcConfig.auto_user_instrum?.mode === 'string') { if (action === 'apply' || action === 'modify') { setCollectionMode(rcConfig.auto_user_instrum.mode) } else { From 53a7791abdfe5af383ce087bd3e10c23c57be2f0 Mon Sep 17 00:00:00 2001 From: simon-id Date: Wed, 30 Oct 2024 11:52:28 +0100 Subject: [PATCH 019/126] push everything --- packages/dd-trace/src/appsec/passport.js | 138 +++++++++--------- .../src/appsec/remote_config/index.js | 1 - packages/dd-trace/src/appsec/sdk/set_user.js | 11 ++ .../src/appsec/user_tracking/index.js | 22 +++ packages/dd-trace/src/config.js | 27 ++-- 5 files changed, 113 insertions(+), 86 deletions(-) create mode 100644 packages/dd-trace/src/appsec/user_tracking/index.js diff --git a/packages/dd-trace/src/appsec/passport.js b/packages/dd-trace/src/appsec/passport.js index b1eeb26205d..9aee9bc4ad5 100644 --- a/packages/dd-trace/src/appsec/passport.js +++ b/packages/dd-trace/src/appsec/passport.js @@ -3,29 +3,15 @@ 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 crypto = require('crypto') const SDK_USER_EVENT_PATTERN = '^_dd\\.appsec\\.events\\.users\\.[\\W\\w+]+\\.sdk$' const regexSdkEvent = new RegExp(SDK_USER_EVENT_PATTERN, 'i') -let collectionMode - -function setCollectionMode (mode) { - switch (mode) { - case 'ident': - case 'identification': - collectionMode = 'ident' - break - case 'anon': - case 'anonymization': - collectionMode = 'anon' - break - default: - collectionMode = null // disabled - } -} +// The user ID generated must be consistent and repeatable meaning that, for a given framework, the same field must always be used. +const USER_ID_FIELDS = ['id', '_id', 'email', 'username', 'login', 'user'] + + function isSdkCalled (tags) { let called = false @@ -37,91 +23,99 @@ function isSdkCalled (tags) { return called } +function obfuscateId (id) { + return 'anon_' + crypto.createHash('sha256').update(id).digest().toString('hex', 0, 16).toLowerCase() +} + // delete this function later if we know it's always credential.username function getLogin (credentials) { const type = credentials && credentials.type let login if (type === 'local' || type === 'http') { login = credentials.username + + if (collectionMode === 'anon') { + login = obfuscateId(login) + } } return login } -function parseUser (login, passportUser, mode) { - const user = { - 'usr.id': login - } +function getUserId (passportUser) { + for (const field of USER_ID_FIELDS) { + let id = passportUser[field] + if (id) { + if (collectionMode === 'anon') { + id = obfuscateId(id) + } - if (!user['usr.id']) { - return user + return id + } } +} - if (passportUser) { - // Guess id - if (passportUser.id) { - user['usr.id'] = passportUser.id - } else if (passportUser._id) { - user['usr.id'] = passportUser._id - } +// TODO passpoort-jwt ? - if (mode === 'extended') { - if (login) { - user['usr.login'] = login - } +// TODO: SDK always has precendence ? +// Whenever relevant, the user ID must be collected by the libraries as part of the root span, using the tag usr.id. +// Whenever relevant, the user login must be collected by the libraries as part of the root span, using the tag usr.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 - } - } - } +/* +These modes only impact automated user ID and login collection, either for business logic events or for authenticated user tracking, and should be disregarded when the collection is performed through the various SDKs. - if (mode === 'safe') { - // Remove PII in safe mode - if (!regexUsername.test(user['usr.id'])) { - user['usr.id'] = '' - } - } - return user -} +In the disabled mode, as the name suggests, libraries should not collect user ID or user login. Effectively, this means that libraries shouldn’t send automated business logic events, specifically login and signup events, nor should they automatically track authenticated requests. +*/ + +function passportTrackEvent (credentials, passportUser, rootSpan) { + if (!collectionMode) return -function passportTrackEvent (credentials, passportUser, rootSpan, mode) { const tags = rootSpan && rootSpan.context() && rootSpan.context()._tags + // TODO: what if sdk is called after automated user + 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 user ID found in authentication instrumentation') - return - } + // If a passportUser object is published then the login succeded if (passportUser) { - // If a passportUser object is published then the login succeded - const userTags = {} - Object.entries(user).forEach(([k, v]) => { - const attr = k.split('.', 2)[1] - userTags[attr] = v - }) - - setUserTags(userTags, rootSpan) - trackEvent('users.login.success', null, 'passportTrackEvent', rootSpan, mode) + const userId = getUserId(passportUser) + + if (userId === undefined) { + log.warn('No user ID found in authentication instrumentation') + // telemetry counter: 'appsec.instrum.user_auth.missing_user_id' + return + } + + setUserTags({ id: userId }, rootSpan) + + trackEvent('users.login.success', null, 'passportTrackEvent', rootSpan, collectionMode) + + // call WAF ephemeral } else { - trackEvent('users.login.failure', user, 'passportTrackEvent', rootSpan, mode) + const login = getLogin(credentials) + + if (!login) { + return // idk + } + + trackEvent('users.login.failure', { 'usr.id': login, login }, 'passportTrackEvent', rootSpan, collectionMode) } } +function passportTrackUser (session) { + if (!collectionMode) return + + const userId = getUserId(session.passport.user) + + // call WAF ephemeral + +} + module.exports = { passportTrackEvent, setCollectionMode diff --git a/packages/dd-trace/src/appsec/remote_config/index.js b/packages/dd-trace/src/appsec/remote_config/index.js index 3dd5422e571..13b8d04ad4f 100644 --- a/packages/dd-trace/src/appsec/remote_config/index.js +++ b/packages/dd-trace/src/appsec/remote_config/index.js @@ -42,7 +42,6 @@ function enable (config, appsec) { apiSecuritySampler.setRequestSampling(rcConfig.api_security?.request_sample_rate) - // TODO: does local config take precedence? if (config.appsec.eventTracking?.enabled && typeof rcConfig.auto_user_instrum?.mode === 'string') { if (action === 'apply' || action === 'modify') { setCollectionMode(rcConfig.auto_user_instrum.mode) diff --git a/packages/dd-trace/src/appsec/sdk/set_user.js b/packages/dd-trace/src/appsec/sdk/set_user.js index 81b0e3ec7ad..07a075bfea1 100644 --- a/packages/dd-trace/src/appsec/sdk/set_user.js +++ b/packages/dd-trace/src/appsec/sdk/set_user.js @@ -21,7 +21,18 @@ function setUser (tracer, user) { return } + // must get user ID with USER_ID_FIELDS + + // _dd.appsec.user.collection_mode: collectionMode // sdk/ident/anon + setUserTags(user, rootSpan) + + /* + User IDs generated through the SDK must now be provided to libddwaf as a persistent addresses. + If the user monitoring SDK has already resulted in a call to libddwaf before any automated instrumentation or collection method has been executed, no extra call should be made. + If the automated instrumentation or collection method has resulted in a call to libddwaf before the user monitoring SDK has been executed, a second call must be performed with the user ID obtained through the SDK. + */ + // will the second call trigger tho ? make some edge case tests } module.exports = { diff --git a/packages/dd-trace/src/appsec/user_tracking/index.js b/packages/dd-trace/src/appsec/user_tracking/index.js new file mode 100644 index 00000000000..c627fbdb6cc --- /dev/null +++ b/packages/dd-trace/src/appsec/user_tracking/index.js @@ -0,0 +1,22 @@ +'use strict' + +let collectionMode + +function setCollectionMode (mode) { + switch (mode) { + case 'ident': + case 'identification': + collectionMode = 'ident' + break + case 'anon': + case 'anonymization': + collectionMode = 'anon' + break + default: + collectionMode = null // disabled + } +} + +module.exports = { + setCollectionMode +} diff --git a/packages/dd-trace/src/config.js b/packages/dd-trace/src/config.js index cb990b2c243..210dbd79fc7 100644 --- a/packages/dd-trace/src/config.js +++ b/packages/dd-trace/src/config.js @@ -284,6 +284,7 @@ class Config { options.appsec = {} } + // TODO: remove in next major const DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING_ENABLED = coalesce( options.appsec.eventTracking?.enabled, process.env.DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING_ENABLED && @@ -482,8 +483,8 @@ class Config { this._setValue(defaults, 'appsec.blockedTemplateHtml', undefined) this._setValue(defaults, 'appsec.blockedTemplateJson', undefined) this._setValue(defaults, 'appsec.enabled', undefined) - this._setValue(defaults, 'appsec.eventTracking.enabled', true) - this._setValue(defaults, 'appsec.eventTracking.mode', 'safe') + //this._setValue(defaults, 'appsec.eventTracking.enabled', true) + //this._setValue(defaults, 'appsec.eventTracking.mode', 'safe') this._setValue(defaults, 'appsec.obfuscatorKeyRegex', defaultWafObfuscatorKeyRegex) this._setValue(defaults, 'appsec.obfuscatorValueRegex', defaultWafObfuscatorValueRegex) this._setValue(defaults, 'appsec.rasp.enabled', true) @@ -711,11 +712,11 @@ class Config { this._setValue(env, 'appsec.blockedTemplateJson', maybeFile(DD_APPSEC_HTTP_BLOCKED_TEMPLATE_JSON)) this._envUnprocessed['appsec.blockedTemplateJson'] = DD_APPSEC_HTTP_BLOCKED_TEMPLATE_JSON this._setBoolean(env, 'appsec.enabled', DD_APPSEC_ENABLED) - if (DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING) { - this._setValue(env, 'appsec.eventTracking.enabled', - ['extended', 'safe'].includes(DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING.toLowerCase())) - this._setValue(env, 'appsec.eventTracking.mode', DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING.toLowerCase()) - } + // if (DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING) { + // this._setValue(env, 'appsec.eventTracking.enabled', + // ['extended', 'safe'].includes(DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING.toLowerCase())) + // this._setValue(env, 'appsec.eventTracking.mode', DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING.toLowerCase()) + // } this._setString(env, 'appsec.obfuscatorKeyRegex', DD_APPSEC_OBFUSCATION_PARAMETER_KEY_REGEXP) this._setString(env, 'appsec.obfuscatorValueRegex', DD_APPSEC_OBFUSCATION_PARAMETER_VALUE_REGEXP) this._setBoolean(env, 'appsec.rasp.enabled', DD_APPSEC_RASP_ENABLED) @@ -878,12 +879,12 @@ class Config { this._setValue(opts, 'appsec.blockedTemplateJson', maybeFile(options.appsec.blockedTemplateJson)) this._optsUnprocessed['appsec.blockedTemplateJson'] = options.appsec.blockedTemplateJson this._setBoolean(opts, 'appsec.enabled', options.appsec.enabled) - let eventTracking = options.appsec.eventTracking?.mode - if (eventTracking) { - eventTracking = eventTracking.toLowerCase() - this._setValue(opts, 'appsec.eventTracking.enabled', ['extended', 'safe'].includes(eventTracking)) - this._setValue(opts, 'appsec.eventTracking.mode', eventTracking) - } + // let eventTracking = options.appsec.eventTracking?.mode + // if (eventTracking) { + // eventTracking = eventTracking.toLowerCase() + // this._setValue(opts, 'appsec.eventTracking.enabled', ['extended', 'safe'].includes(eventTracking)) + // this._setValue(opts, 'appsec.eventTracking.mode', eventTracking) + // } this._setString(opts, 'appsec.obfuscatorKeyRegex', options.appsec.obfuscatorKeyRegex) this._setString(opts, 'appsec.obfuscatorValueRegex', options.appsec.obfuscatorValueRegex) this._setBoolean(opts, 'appsec.rasp.enabled', options.appsec.rasp?.enabled) From 2b7d2e6bd410f84095aa0ac8732627b9a6d1919d Mon Sep 17 00:00:00 2001 From: simon-id Date: Wed, 30 Oct 2024 12:56:33 +0100 Subject: [PATCH 020/126] simplify passport strategies instrumentation --- .../src/passport-http.js | 16 +------ .../src/passport-local.js | 16 +------ .../src/passport-utils.js | 46 ++++++++++++++----- 3 files changed, 38 insertions(+), 40 deletions(-) diff --git a/packages/datadog-instrumentations/src/passport-http.js b/packages/datadog-instrumentations/src/passport-http.js index 0969d2d3fc9..3b930a1a1cc 100644 --- a/packages/datadog-instrumentations/src/passport-http.js +++ b/packages/datadog-instrumentations/src/passport-http.js @@ -1,22 +1,10 @@ 'use strict' -const shimmer = require('../../datadog-shimmer') const { addHook } = require('./helpers/instrument') -const { wrapVerify } = require('./passport-utils') +const { strategyHook } = require('./passport-utils') addHook({ name: 'passport-http', file: 'lib/passport-http/strategies/basic.js', versions: ['>=0.3.0'] -}, BasicStrategy => { - return shimmer.wrapFunction(BasicStrategy, BasicStrategy => function () { - const type = 'http' - - if (typeof arguments[0] === 'function') { - arguments[0] = wrapVerify(arguments[0], false, type) - } else { - arguments[1] = wrapVerify(arguments[1], (arguments[0] && arguments[0].passReqToCallback), type) - } - return BasicStrategy.apply(this, arguments) - }) -}) +}, strategyHook) diff --git a/packages/datadog-instrumentations/src/passport-local.js b/packages/datadog-instrumentations/src/passport-local.js index dab74eb470e..c6dcec9a48d 100644 --- a/packages/datadog-instrumentations/src/passport-local.js +++ b/packages/datadog-instrumentations/src/passport-local.js @@ -1,22 +1,10 @@ 'use strict' -const shimmer = require('../../datadog-shimmer') const { addHook } = require('./helpers/instrument') -const { wrapVerify } = require('./passport-utils') +const { strategyHook } = require('./passport-utils') addHook({ name: 'passport-local', file: 'lib/strategy.js', versions: ['>=1.0.0'] -}, Strategy => { - return shimmer.wrapFunction(Strategy, Strategy => function () { - const type = 'local' - - if (typeof arguments[0] === 'function') { - arguments[0] = wrapVerify(arguments[0], false, type) - } else { - arguments[1] = wrapVerify(arguments[1], (arguments[0] && arguments[0].passReqToCallback), type) - } - return Strategy.apply(this, arguments) - }) -}) +}, strategyHook) diff --git a/packages/datadog-instrumentations/src/passport-utils.js b/packages/datadog-instrumentations/src/passport-utils.js index 7969ab486b4..677328141b3 100644 --- a/packages/datadog-instrumentations/src/passport-utils.js +++ b/packages/datadog-instrumentations/src/passport-utils.js @@ -12,26 +12,48 @@ function wrapVerifiedAndPublish (username, password, verified, type) { // eslint-disable-next-line n/handle-callback-err return shimmer.wrapFunction(verified, verified => function (err, user, info) { - const credentials = { type, username } - passportVerifyChannel.publish({ credentials, user }) + if (err) return // an error from the database doesn't mean either success or failure + + passportVerifyChannel.publish({ login: username, user, abortController }) + return verified.apply(this, arguments) }) } -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) +function wrapVerify (verify) { + return function wrappedVerify (req, username, password, verified) { + let index = 3 + + if (!this._passReqToCallback) { + index = 2 + username = req + password = username + verified = password } - } else { - return function (username, password, verified) { - arguments[2] = wrapVerifiedAndPublish(username, password, verified, type) - return verify.apply(this, arguments) + + arguments[index] = wrapVerifiedAndPublish(username, password, verified, this.name) + + return verify.apply(this, arguments) + } +} + +function wrapStrategy (Strategy) { + return function wrappedStrategy () { + // verify function can be either the first or second argument + if (typeof arguments[0] === 'function') { + arguments[0] = wrapVerify(arguments[0]) + } else { + arguments[1] = wrapVerify(arguments[1]) } + + return Strategy.apply(this, arguments) } } +function strategyHook (Strategy) { + return shimmer.wrapFunction(Strategy, wrapStrategy) +} + module.exports = { - wrapVerify + strategyHook } From 24c2828cbfbf3c8aca83d1623392b8c59e6cce24 Mon Sep 17 00:00:00 2001 From: simon-id Date: Wed, 30 Oct 2024 13:25:42 +0100 Subject: [PATCH 021/126] fixes --- .../src/passport-utils.js | 43 ++++++++++--------- 1 file changed, 22 insertions(+), 21 deletions(-) diff --git a/packages/datadog-instrumentations/src/passport-utils.js b/packages/datadog-instrumentations/src/passport-utils.js index 677328141b3..dc86b5fff1c 100644 --- a/packages/datadog-instrumentations/src/passport-utils.js +++ b/packages/datadog-instrumentations/src/passport-utils.js @@ -5,34 +5,35 @@ const { channel } = require('./helpers/instrument') const passportVerifyChannel = channel('datadog:passport:verify:finish') -function wrapVerifiedAndPublish (username, password, verified, type) { - if (!passportVerifyChannel.hasSubscribers) { - return verified - } - - // eslint-disable-next-line n/handle-callback-err - return shimmer.wrapFunction(verified, verified => function (err, user, info) { - if (err) return // an error from the database doesn't mean either success or failure - - passportVerifyChannel.publish({ login: username, user, abortController }) - - return verified.apply(this, arguments) +function wrapVerifiedAndPublish (username, verified) { + return shimmer.wrapFunction(verified, function wrapVerify (verified) { + return function wrappedVerified (err, user) { + // if there is an error, it's neither an auth success nor a failure + if (!err) { + passportVerifyChannel.publish({ success: !!user, login: username, user }) + } + + return verified.apply(this, arguments) + } }) } function wrapVerify (verify) { return function wrappedVerify (req, username, password, verified) { - let index = 3 - - if (!this._passReqToCallback) { - index = 2 - username = req - password = username - verified = password + if (passportVerifyChannel.hasSubscribers) { + // verify can be called with or without req + let verifiedIndex = 3 + if (!this._passReqToCallback) { + verifiedIndex = 2 + username = req + verified = password + } + + // replace the callback with our own wrapper to get the result + arguments[verifiedIndex] = wrapVerifiedAndPublish(username, verified) + // if we ever need the type of strategy, we can get it from this.name } - arguments[index] = wrapVerifiedAndPublish(username, password, verified, this.name) - return verify.apply(this, arguments) } } From cdb89f6424d762dff3c12740b63b4a772ca8f381 Mon Sep 17 00:00:00 2001 From: simon-id Date: Wed, 30 Oct 2024 13:36:18 +0100 Subject: [PATCH 022/126] simplify code --- .../datadog-instrumentations/src/passport-utils.js | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/packages/datadog-instrumentations/src/passport-utils.js b/packages/datadog-instrumentations/src/passport-utils.js index dc86b5fff1c..5999a7b94ba 100644 --- a/packages/datadog-instrumentations/src/passport-utils.js +++ b/packages/datadog-instrumentations/src/passport-utils.js @@ -21,17 +21,13 @@ function wrapVerifiedAndPublish (username, verified) { function wrapVerify (verify) { return function wrappedVerify (req, username, password, verified) { if (passportVerifyChannel.hasSubscribers) { - // verify can be called with or without req - let verifiedIndex = 3 - if (!this._passReqToCallback) { - verifiedIndex = 2 - username = req - verified = password - } - // replace the callback with our own wrapper to get the result - arguments[verifiedIndex] = wrapVerifiedAndPublish(username, verified) // if we ever need the type of strategy, we can get it from this.name + if (this._passReqToCallback) { + arguments[3] = wrapVerifiedAndPublish(username, verified) + } else { + arguments[2] = wrapVerifiedAndPublish(req, password) // shifted args + } } return verify.apply(this, arguments) From 32edfaeb79cce55941ecaafdebd03f763141e985 Mon Sep 17 00:00:00 2001 From: simon-id Date: Wed, 30 Oct 2024 13:41:47 +0100 Subject: [PATCH 023/126] cleanup --- 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 5999a7b94ba..b31a3c7e0f4 100644 --- a/packages/datadog-instrumentations/src/passport-utils.js +++ b/packages/datadog-instrumentations/src/passport-utils.js @@ -10,7 +10,7 @@ function wrapVerifiedAndPublish (username, verified) { return function wrappedVerified (err, user) { // if there is an error, it's neither an auth success nor a failure if (!err) { - passportVerifyChannel.publish({ success: !!user, login: username, user }) + passportVerifyChannel.publish({ login: username, user, success: !!user }) } return verified.apply(this, arguments) From 50b3605ac970f772f45f59f6b6c232ede20f3daf Mon Sep 17 00:00:00 2001 From: simon-id Date: Wed, 30 Oct 2024 14:53:45 +0100 Subject: [PATCH 024/126] revert some changes --- .../src/passport-http.js | 16 ++++++- .../src/passport-local.js | 16 ++++++- .../src/passport-utils.js | 48 +++++-------------- 3 files changed, 39 insertions(+), 41 deletions(-) diff --git a/packages/datadog-instrumentations/src/passport-http.js b/packages/datadog-instrumentations/src/passport-http.js index 3b930a1a1cc..0969d2d3fc9 100644 --- a/packages/datadog-instrumentations/src/passport-http.js +++ b/packages/datadog-instrumentations/src/passport-http.js @@ -1,10 +1,22 @@ 'use strict' +const shimmer = require('../../datadog-shimmer') const { addHook } = require('./helpers/instrument') -const { strategyHook } = require('./passport-utils') +const { wrapVerify } = require('./passport-utils') addHook({ name: 'passport-http', file: 'lib/passport-http/strategies/basic.js', versions: ['>=0.3.0'] -}, strategyHook) +}, BasicStrategy => { + return shimmer.wrapFunction(BasicStrategy, BasicStrategy => function () { + const type = 'http' + + if (typeof arguments[0] === 'function') { + arguments[0] = wrapVerify(arguments[0], false, type) + } else { + 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 c6dcec9a48d..dab74eb470e 100644 --- a/packages/datadog-instrumentations/src/passport-local.js +++ b/packages/datadog-instrumentations/src/passport-local.js @@ -1,10 +1,22 @@ 'use strict' +const shimmer = require('../../datadog-shimmer') const { addHook } = require('./helpers/instrument') -const { strategyHook } = require('./passport-utils') +const { wrapVerify } = require('./passport-utils') addHook({ name: 'passport-local', file: 'lib/strategy.js', versions: ['>=1.0.0'] -}, strategyHook) +}, Strategy => { + return shimmer.wrapFunction(Strategy, Strategy => function () { + const type = 'local' + + if (typeof arguments[0] === 'function') { + arguments[0] = wrapVerify(arguments[0], false, type) + } else { + 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 index 6a237306605..7969ab486b4 100644 --- a/packages/datadog-instrumentations/src/passport-utils.js +++ b/packages/datadog-instrumentations/src/passport-utils.js @@ -12,52 +12,26 @@ function wrapVerifiedAndPublish (username, password, verified, type) { // eslint-disable-next-line n/handle-callback-err return shimmer.wrapFunction(verified, verified => function (err, user, info) { - if (err) return // ???? const credentials = { type, username } - const abortController = new AbortController() - - passportVerifyChannel.publish({ credentials, user, abortController }) - - if (abortController.signal.aborted) { - arguments[0] = new Error('Blocked') - } - + passportVerifyChannel.publish({ credentials, user }) return verified.apply(this, arguments) }) } -function wrapVerify (verify) { - return function wrappedVerify (req, username, password, verified) { - let index = 3 - - if (!this._passReqToCallback) { - index = 2 - username = req - password = username - verified = password +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) } - - arguments[index] = wrapVerifiedAndPublish(username, password, verified, this.name) - - return verify.apply(this, arguments) - } -} - -function wrapStrategy (Strategy) { - return function wrappedStrategy () { - if (typeof arguments[0] === 'function') { - arguments[0] = wrapVerify(arguments[0]) - } else { - arguments[1] = wrapVerify(arguments[1]) + } else { + return function (username, password, verified) { + arguments[2] = wrapVerifiedAndPublish(username, password, verified, type) + return verify.apply(this, arguments) } - return Strategy.apply(this, arguments) } } -return function strategyHook (Strategy) { - return shimmer.wrapFunction(Strategy, wrapStrategy) -} - module.exports = { - strategyHook + wrapVerify } From b5aa0d4201b67701a7a9a4df17dd88f7ac4b99b3 Mon Sep 17 00:00:00 2001 From: simon-id Date: Wed, 30 Oct 2024 14:56:24 +0100 Subject: [PATCH 025/126] delete passport.js --- packages/dd-trace/src/appsec/passport.js | 122 ----------------------- 1 file changed, 122 deletions(-) delete mode 100644 packages/dd-trace/src/appsec/passport.js diff --git a/packages/dd-trace/src/appsec/passport.js b/packages/dd-trace/src/appsec/passport.js deleted file mode 100644 index 9aee9bc4ad5..00000000000 --- a/packages/dd-trace/src/appsec/passport.js +++ /dev/null @@ -1,122 +0,0 @@ -'use strict' - -const log = require('../log') -const { trackEvent } = require('./sdk/track_event') -const { setUserTags } = require('./sdk/set_user') -const crypto = require('crypto') - -const SDK_USER_EVENT_PATTERN = '^_dd\\.appsec\\.events\\.users\\.[\\W\\w+]+\\.sdk$' -const regexSdkEvent = new RegExp(SDK_USER_EVENT_PATTERN, 'i') - -// The user ID generated must be consistent and repeatable meaning that, for a given framework, the same field must always be used. -const USER_ID_FIELDS = ['id', '_id', 'email', 'username', 'login', 'user'] - - - -function isSdkCalled (tags) { - let called = false - - if (tags !== null && typeof tags === 'object') { - called = Object.entries(tags).some(([key, value]) => regexSdkEvent.test(key) && value === 'true') - } - - return called -} - -function obfuscateId (id) { - return 'anon_' + crypto.createHash('sha256').update(id).digest().toString('hex', 0, 16).toLowerCase() -} - -// delete this function later if we know it's always credential.username -function getLogin (credentials) { - const type = credentials && credentials.type - let login - if (type === 'local' || type === 'http') { - login = credentials.username - - if (collectionMode === 'anon') { - login = obfuscateId(login) - } - } - - return login -} - -function getUserId (passportUser) { - for (const field of USER_ID_FIELDS) { - let id = passportUser[field] - if (id) { - if (collectionMode === 'anon') { - id = obfuscateId(id) - } - - return id - } - } -} - -// TODO passpoort-jwt ? - -// TODO: SDK always has precendence ? -// Whenever relevant, the user ID must be collected by the libraries as part of the root span, using the tag usr.id. -// Whenever relevant, the user login must be collected by the libraries as part of the root span, using the tag usr.login. - - -/* -These modes only impact automated user ID and login collection, either for business logic events or for authenticated user tracking, and should be disregarded when the collection is performed through the various SDKs. - - -In the disabled mode, as the name suggests, libraries should not collect user ID or user login. Effectively, this means that libraries shouldn’t send automated business logic events, specifically login and signup events, nor should they automatically track authenticated requests. -*/ - -function passportTrackEvent (credentials, passportUser, rootSpan) { - if (!collectionMode) return - - const tags = rootSpan && rootSpan.context() && rootSpan.context()._tags - - // TODO: what if sdk is called after automated user - - if (isSdkCalled(tags)) { - // Don't overwrite tags set by SDK callings - return - } - - // If a passportUser object is published then the login succeded - if (passportUser) { - const userId = getUserId(passportUser) - - if (userId === undefined) { - log.warn('No user ID found in authentication instrumentation') - // telemetry counter: 'appsec.instrum.user_auth.missing_user_id' - return - } - - setUserTags({ id: userId }, rootSpan) - - trackEvent('users.login.success', null, 'passportTrackEvent', rootSpan, collectionMode) - - // call WAF ephemeral - } else { - const login = getLogin(credentials) - - if (!login) { - return // idk - } - - trackEvent('users.login.failure', { 'usr.id': login, login }, 'passportTrackEvent', rootSpan, collectionMode) - } -} - -function passportTrackUser (session) { - if (!collectionMode) return - - const userId = getUserId(session.passport.user) - - // call WAF ephemeral - -} - -module.exports = { - passportTrackEvent, - setCollectionMode -} From 1f30a3c82eb24f8d7326f1f61884c2033a73e19f Mon Sep 17 00:00:00 2001 From: simon-id Date: Wed, 30 Oct 2024 17:08:08 +0100 Subject: [PATCH 026/126] Update packages/datadog-instrumentations/src/passport-utils.js --- packages/datadog-instrumentations/src/passport-utils.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/datadog-instrumentations/src/passport-utils.js b/packages/datadog-instrumentations/src/passport-utils.js index b31a3c7e0f4..3ca2f326583 100644 --- a/packages/datadog-instrumentations/src/passport-utils.js +++ b/packages/datadog-instrumentations/src/passport-utils.js @@ -24,9 +24,9 @@ function wrapVerify (verify) { // replace the callback with our own wrapper to get the result // if we ever need the type of strategy, we can get it from this.name if (this._passReqToCallback) { - arguments[3] = wrapVerifiedAndPublish(username, verified) + arguments[3] = wrapVerifiedAndPublish(arguments[1], arguments[3]) } else { - arguments[2] = wrapVerifiedAndPublish(req, password) // shifted args + arguments[2] = wrapVerifiedAndPublish(arguments[0], arguments[2]) } } From 9fee2bc097434d46c86bda1055249393fcccb91a Mon Sep 17 00:00:00 2001 From: simon-id Date: Wed, 30 Oct 2024 17:14:37 +0100 Subject: [PATCH 027/126] update verify subscriber --- packages/dd-trace/src/appsec/index.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/dd-trace/src/appsec/index.js b/packages/dd-trace/src/appsec/index.js index 42021d151e3..4ee6dc5fdfb 100644 --- a/packages/dd-trace/src/appsec/index.js +++ b/packages/dd-trace/src/appsec/index.js @@ -25,7 +25,7 @@ const web = require('../plugins/util/web') const { extractIp } = require('../plugins/util/ip_extractor') const { HTTP_CLIENT_IP } = require('../../../../ext/tags') const { isBlocked, block, setTemplates, getBlockingAction } = require('./blocking') -const { setCollectionMode, passportTrackEvent } = require('./passport') +const { setCollectionMode, trackLogin } = require('./user_tracking') const { storage } = require('../../../datadog-core') const graphql = require('./graphql') const rasp = require('./rasp') @@ -213,7 +213,7 @@ function onResponseBody ({ req, body }) { }, req) } -function onPassportVerify ({ credentials, user }) { +function onPassportVerify ({ login, user, success }) { const store = storage.getStore() const rootSpan = store?.req && web.root(store.req) @@ -222,7 +222,7 @@ function onPassportVerify ({ credentials, user }) { return } - passportTrackEvent(credentials, user, rootSpan) + trackLogin(login, user, success, rootSpan) } const responseAnalyzedSet = new WeakSet() From be3115a43305508f2f2d8ac5131b9564ebd5cac0 Mon Sep 17 00:00:00 2001 From: simon-id Date: Wed, 30 Oct 2024 17:52:56 +0100 Subject: [PATCH 028/126] rollback config changes --- packages/dd-trace/src/config.js | 49 +++++++++------------------------ 1 file changed, 13 insertions(+), 36 deletions(-) diff --git a/packages/dd-trace/src/config.js b/packages/dd-trace/src/config.js index 210dbd79fc7..aca7b55de1a 100644 --- a/packages/dd-trace/src/config.js +++ b/packages/dd-trace/src/config.js @@ -284,33 +284,6 @@ class Config { options.appsec = {} } - // TODO: remove in next major - const DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING_ENABLED = coalesce( - options.appsec.eventTracking?.enabled, - process.env.DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING_ENABLED && - isTrue(process.env.DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING_ENABLED), - true - ) - - // little util for retrocompatibility, delete in next major - function convertEventTrackingMode (mode) { - if (typeof mode !== 'string') return mode - - mode = mode.toLowerCase() - - if (mode === 'extended') return 'ident' - if (mode === 'safe') return 'anon' - - return mode - } - - const DD_APPSEC_AUTO_USER_INSTRUMENTATION_MODE = coalesce( - convertEventTrackingMode(options.appsec.eventTracking?.mode), - process.env.DD_APPSEC_AUTO_USER_INSTRUMENTATION_MODE, - convertEventTrackingMode(process.env.DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING), // TODO: make it deprecated - 'ident' - ).toLowerCase() - const DD_INSTRUMENTATION_INSTALL_ID = coalesce( process.env.DD_INSTRUMENTATION_INSTALL_ID, null @@ -346,13 +319,6 @@ class Config { // TODO: refactor this.apiKey = DD_API_KEY - this.appsec = { - eventTracking: { - enabled: DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING_ENABLED && - ['anon', 'ident'].includes(DD_APPSEC_AUTO_USER_INSTRUMENTATION_MODE), - mode: DD_APPSEC_AUTO_USER_INSTRUMENTATION_MODE - } - } // sent in telemetry event app-started this.installSignature = { @@ -483,8 +449,7 @@ class Config { this._setValue(defaults, 'appsec.blockedTemplateHtml', undefined) this._setValue(defaults, 'appsec.blockedTemplateJson', undefined) this._setValue(defaults, 'appsec.enabled', undefined) - //this._setValue(defaults, 'appsec.eventTracking.enabled', true) - //this._setValue(defaults, 'appsec.eventTracking.mode', 'safe') + this._setValue(defaults, 'appsec.eventTracking.mode', 'ident') this._setValue(defaults, 'appsec.obfuscatorKeyRegex', defaultWafObfuscatorKeyRegex) this._setValue(defaults, 'appsec.obfuscatorValueRegex', defaultWafObfuscatorValueRegex) this._setValue(defaults, 'appsec.rasp.enabled', true) @@ -1288,4 +1253,16 @@ function setHiddenProperty (obj, name, value) { return obj[name] } +// little util for retrocompatibility, delete in next major +function convertEventsTrackingMode (mode) { + if (typeof mode !== 'string') return mode + + mode = mode.toLowerCase() + + if (mode === 'extended') return 'ident' + if (mode === 'safe') return 'anon' + + return mode +} + module.exports = Config From 38b44195be84cb718a501922db72ae2b2a93e26c Mon Sep 17 00:00:00 2001 From: simon-id Date: Wed, 30 Oct 2024 21:47:33 +0100 Subject: [PATCH 029/126] fix config --- packages/dd-trace/src/config.js | 29 ++++++----------------------- 1 file changed, 6 insertions(+), 23 deletions(-) diff --git a/packages/dd-trace/src/config.js b/packages/dd-trace/src/config.js index aca7b55de1a..821bfbd4457 100644 --- a/packages/dd-trace/src/config.js +++ b/packages/dd-trace/src/config.js @@ -557,6 +557,7 @@ class Config { DD_API_SECURITY_REQUEST_SAMPLE_RATE, DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING, DD_APPSEC_ENABLED, + DD_APPSEC_AUTO_USER_INSTRUMENTATION_MODE, DD_APPSEC_GRAPHQL_BLOCKED_TEMPLATE_JSON, DD_APPSEC_HTTP_BLOCKED_TEMPLATE_HTML, DD_APPSEC_HTTP_BLOCKED_TEMPLATE_JSON, @@ -677,11 +678,10 @@ class Config { this._setValue(env, 'appsec.blockedTemplateJson', maybeFile(DD_APPSEC_HTTP_BLOCKED_TEMPLATE_JSON)) this._envUnprocessed['appsec.blockedTemplateJson'] = DD_APPSEC_HTTP_BLOCKED_TEMPLATE_JSON this._setBoolean(env, 'appsec.enabled', DD_APPSEC_ENABLED) - // if (DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING) { - // this._setValue(env, 'appsec.eventTracking.enabled', - // ['extended', 'safe'].includes(DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING.toLowerCase())) - // this._setValue(env, 'appsec.eventTracking.mode', DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING.toLowerCase()) - // } + this._setString(env, 'appsec.eventTracking.mode', coalesce( + DD_APPSEC_AUTO_USER_INSTRUMENTATION_MODE, + DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING // TODO: remove in next major + )) this._setString(env, 'appsec.obfuscatorKeyRegex', DD_APPSEC_OBFUSCATION_PARAMETER_KEY_REGEXP) this._setString(env, 'appsec.obfuscatorValueRegex', DD_APPSEC_OBFUSCATION_PARAMETER_VALUE_REGEXP) this._setBoolean(env, 'appsec.rasp.enabled', DD_APPSEC_RASP_ENABLED) @@ -844,12 +844,7 @@ class Config { this._setValue(opts, 'appsec.blockedTemplateJson', maybeFile(options.appsec.blockedTemplateJson)) this._optsUnprocessed['appsec.blockedTemplateJson'] = options.appsec.blockedTemplateJson this._setBoolean(opts, 'appsec.enabled', options.appsec.enabled) - // let eventTracking = options.appsec.eventTracking?.mode - // if (eventTracking) { - // eventTracking = eventTracking.toLowerCase() - // this._setValue(opts, 'appsec.eventTracking.enabled', ['extended', 'safe'].includes(eventTracking)) - // this._setValue(opts, 'appsec.eventTracking.mode', eventTracking) - // } + this._setString(opts, 'appsec.eventTracking.mode', options.appsec.eventTracking?.mode) this._setString(opts, 'appsec.obfuscatorKeyRegex', options.appsec.obfuscatorKeyRegex) this._setString(opts, 'appsec.obfuscatorValueRegex', options.appsec.obfuscatorValueRegex) this._setBoolean(opts, 'appsec.rasp.enabled', options.appsec.rasp?.enabled) @@ -1253,16 +1248,4 @@ function setHiddenProperty (obj, name, value) { return obj[name] } -// little util for retrocompatibility, delete in next major -function convertEventsTrackingMode (mode) { - if (typeof mode !== 'string') return mode - - mode = mode.toLowerCase() - - if (mode === 'extended') return 'ident' - if (mode === 'safe') return 'anon' - - return mode -} - module.exports = Config From 40c9df157d0c3c8d423427a9f12ad9479085d719 Mon Sep 17 00:00:00 2001 From: simon-id Date: Wed, 30 Oct 2024 21:49:49 +0100 Subject: [PATCH 030/126] add blocking for passport strategies --- packages/datadog-instrumentations/src/passport-utils.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/datadog-instrumentations/src/passport-utils.js b/packages/datadog-instrumentations/src/passport-utils.js index 3ca2f326583..df42fdeb542 100644 --- a/packages/datadog-instrumentations/src/passport-utils.js +++ b/packages/datadog-instrumentations/src/passport-utils.js @@ -10,7 +10,11 @@ function wrapVerifiedAndPublish (username, verified) { return function wrappedVerified (err, user) { // if there is an error, it's neither an auth success nor a failure if (!err) { - passportVerifyChannel.publish({ login: username, user, success: !!user }) + const abortController = new AbortController() + + passportVerifyChannel.publish({ login: username, user, success: !!user, abortController }) + + if (abortController.signal.aborted) return } return verified.apply(this, arguments) From e34bb72610a2a9ca5c1a136b53f6fc56ac96d479 Mon Sep 17 00:00:00 2001 From: simon-id Date: Wed, 30 Oct 2024 22:08:05 +0100 Subject: [PATCH 031/126] update typings and docs --- index.d.ts | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/index.d.ts b/index.d.ts index f16dc639af4..b0132638519 100644 --- a/index.d.ts +++ b/index.d.ts @@ -646,12 +646,24 @@ declare namespace tracer { */ eventTracking?: { /** - * Controls the automated user event tracking mode. Possible values are disabled, safe and extended. - * 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' + * Controls the automated user tracking mode for user IDs and logins collections. Possible values: + * * 'anonymous': will hash user IDs and user logins before collecting them + * * 'anon': alias for 'anonymous' + * * 'safe': deprecated alias for 'anonymous' + * + * * 'identification': will collect user IDs and logins without redaction + * * 'ident': alias for 'identification' + * * 'extended': deprecated alias for 'identification' + * + * * 'disabled': will not collect user IDs and logins + * + * Unknown values will be considered as 'disabled' + * @default 'ident' */ - mode?: 'anonymous' | 'identification' | 'safe' | 'extended' | 'disabled' + mode?: + 'anonymous' | 'anon' | 'safe' | + 'identification' | 'ident' | 'extended' | + 'disabled' }, /** * Configuration for Api Security sampling From d255702453fa1e721f83d57dd0b7b30a1601b75d Mon Sep 17 00:00:00 2001 From: simon-id Date: Wed, 30 Oct 2024 22:37:24 +0100 Subject: [PATCH 032/126] update appsec index --- packages/dd-trace/src/appsec/index.js | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/packages/dd-trace/src/appsec/index.js b/packages/dd-trace/src/appsec/index.js index 4ee6dc5fdfb..71efd3df89a 100644 --- a/packages/dd-trace/src/appsec/index.js +++ b/packages/dd-trace/src/appsec/index.js @@ -25,7 +25,7 @@ const web = require('../plugins/util/web') const { extractIp } = require('../plugins/util/ip_extractor') const { HTTP_CLIENT_IP } = require('../../../../ext/tags') const { isBlocked, block, setTemplates, getBlockingAction } = require('./blocking') -const { setCollectionMode, trackLogin } = require('./user_tracking') +const UserTracking = require('./user_tracking') const { storage } = require('../../../datadog-core') const graphql = require('./graphql') const rasp = require('./rasp') @@ -54,8 +54,11 @@ function enable (_config) { apiSecuritySampler.configure(_config.appsec) + UserTracking.setCollectionMode(_config.appsec.eventTracking.mode, false) + incomingHttpRequestStart.subscribe(incomingHttpStartTranslator) incomingHttpRequestEnd.subscribe(incomingHttpEndTranslator) + passportVerify.subscribe(onPassportVerify) // possible optimization: only subscribe if collection mode is enabled bodyParser.subscribe(onRequestBodyParsed) nextBodyParsed.subscribe(onRequestBodyParsed) nextQueryParsed.subscribe(onRequestQueryParsed) @@ -65,11 +68,6 @@ function enable (_config) { responseWriteHead.subscribe(onResponseWriteHead) responseSetHeader.subscribe(onResponseSetHeader) - if (_config.appsec.eventTracking.enabled) { - setCollectionMode(_config.appsec.eventTracking.mode) - passportVerify.subscribe(onPassportVerify) - } - isEnabled = true config = _config } catch (err) { @@ -222,7 +220,7 @@ function onPassportVerify ({ login, user, success }) { return } - trackLogin(login, user, success, rootSpan) + UserTracking.trackLogin(login, user, success, rootSpan) } const responseAnalyzedSet = new WeakSet() From b6d458404324e82a6fc574f4cfd4dd946d2df158 Mon Sep 17 00:00:00 2001 From: simon-id Date: Wed, 30 Oct 2024 22:51:44 +0100 Subject: [PATCH 033/126] update RC --- packages/dd-trace/src/appsec/remote_config/index.js | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/packages/dd-trace/src/appsec/remote_config/index.js b/packages/dd-trace/src/appsec/remote_config/index.js index 13b8d04ad4f..8a475994ee0 100644 --- a/packages/dd-trace/src/appsec/remote_config/index.js +++ b/packages/dd-trace/src/appsec/remote_config/index.js @@ -5,7 +5,7 @@ const Activation = require('../activation') const RemoteConfigManager = require('./manager') const RemoteConfigCapabilities = require('./capabilities') const apiSecuritySampler = require('../api_security_sampler') -const { setCollectionMode } = require('../passport') +const { setCollectionMode } = require('../user_tracking') let rc @@ -29,9 +29,7 @@ function enable (config, appsec) { rc.updateCapabilities(RemoteConfigCapabilities.ASM_API_SECURITY_SAMPLE_RATE, true) } - if (config.appsec.eventTracking?.enabled) { - rc.updateCapabilities(RemoteConfigCapabilities.ASM_AUTO_USER_INSTRUM_MODE, true) - } + rc.updateCapabilities(RemoteConfigCapabilities.ASM_AUTO_USER_INSTRUM_MODE, true) rc.setProductHandler('ASM_FEATURES', (action, rcConfig) => { if (!rcConfig) return @@ -42,7 +40,7 @@ function enable (config, appsec) { apiSecuritySampler.setRequestSampling(rcConfig.api_security?.request_sample_rate) - if (config.appsec.eventTracking?.enabled && typeof rcConfig.auto_user_instrum?.mode === 'string') { + if (typeof rcConfig.auto_user_instrum?.mode === 'string') { if (action === 'apply' || action === 'modify') { setCollectionMode(rcConfig.auto_user_instrum.mode) } else { From 4ff837920eee1857222c937a527d89c2c7494ceb Mon Sep 17 00:00:00 2001 From: simon-id Date: Wed, 30 Oct 2024 23:01:23 +0100 Subject: [PATCH 034/126] push some stuff --- .../dd-trace/src/appsec/sdk/track_event.js | 21 +++- .../src/appsec/user_tracking/index.js | 119 +++++++++++++++++- 2 files changed, 136 insertions(+), 4 deletions(-) diff --git a/packages/dd-trace/src/appsec/sdk/track_event.js b/packages/dd-trace/src/appsec/sdk/track_event.js index 429e2d74974..876dd0a8f15 100644 --- a/packages/dd-trace/src/appsec/sdk/track_event.js +++ b/packages/dd-trace/src/appsec/sdk/track_event.js @@ -50,7 +50,7 @@ function trackCustomEvent (tracer, eventName, metadata) { trackEvent(eventName, metadata, 'trackCustomEvent', getRootSpan(tracer), 'sdk') } -function trackEvent (eventName, fields, sdkMethodName, rootSpan, mode) { +function trackEvent (eventName, fields, sdkMethodName, rootSpan, mode, abortController) { if (!rootSpan) { log.warn(`Root span not available in ${sdkMethodName}`) return @@ -76,6 +76,25 @@ function trackEvent (eventName, fields, sdkMethodName, rootSpan, mode) { } } + const persistent = { + + } + + if (user.id) { + persistent[addresses.USER_ID] = user.id + } + + if (user.login) { + persistent[addresses.USER_LOGIN] = user.login + } + + // call WAF + const results = waf.run({ persistent }) + + if (abortController) { + Blocking.handleResults(results) + } + rootSpan.addTags(tags) standalone.sample(rootSpan) diff --git a/packages/dd-trace/src/appsec/user_tracking/index.js b/packages/dd-trace/src/appsec/user_tracking/index.js index c627fbdb6cc..2aef8547482 100644 --- a/packages/dd-trace/src/appsec/user_tracking/index.js +++ b/packages/dd-trace/src/appsec/user_tracking/index.js @@ -1,22 +1,135 @@ 'use strict' +const crypto = require('crypto') +const log = require('../../log') + +const SDK_USER_EVENT_PATTERN = '^_dd\\.appsec\\.events\\.users\\.[\\W\\w+]+\\.sdk$' +const regexSdkEvent = new RegExp(SDK_USER_EVENT_PATTERN, 'i') + +// The user ID generated must be consistent and repeatable meaning that, for a given framework, the same field must always be used. +const USER_ID_FIELDS = ['id', '_id', 'email', 'username', 'login', 'user'] + let collectionMode -function setCollectionMode (mode) { +function setCollectionMode (mode, overwrite = true) { + // don't overwrite if already set, only used in appsec/index.js to not overwrite RC values + if (!overwrite && collectionMode) return + switch (mode) { + case 'extended': + log.warn('Using deprecated value') // eslint-ignore-line no-fallthrough case 'ident': case 'identification': collectionMode = 'ident' break + case 'safe': + log.warn('Using deprecated value') // eslint-ignore-line no-fallthrough case 'anon': case 'anonymization': collectionMode = 'anon' break default: - collectionMode = null // disabled + collectionMode = 'disabled' + } +} + +function isSdkCalled (rootSpan) { + const tags = rootSpan && rootSpan.context() && rootSpan.context()._tags + + let called = false + + if (tags !== null && typeof tags === 'object') { + called = Object.entries(tags).some(([key, value]) => regexSdkEvent.test(key) && value === 'true') + } + + return called +} + +function getUserId (user) { + for (const field of USER_ID_FIELDS) { + let id = user[field] + if (id) { + if (collectionMode === 'anon') { + id = obfuscateId(id) + } + + return id + } + } +} ù + +function obfuscateId (id) { + return 'anon_' + crypto.createHash('sha256').update(id).digest().toString('hex', 0, 16).toLowerCase() +} + +function trackLogin (login, user, success, rootSpan) { + if (!collectionMode) return + + // TODO: what if sdk is called after automated user + + if (isSdkCalled(tags)) { + // Don't overwrite tags set by SDK callings + return + } + + if (collectionMode === 'anon') { + login = obfuscateId(login) + } + + if (success) { + // getID + sdk.trackEvent('users.login.success', null, 'passportTrackEvent', rootSpan, collectionMode) + } else { + + } +} + + + +// TODO passpoort-jwt ? + +// TODO: SDK always has precendence ? +// Whenever relevant, the user ID must be collected by the libraries as part of the root span, using the tag usr.id. +// Whenever relevant, the user login must be collected by the libraries as part of the root span, using the tag usr.login. + + +/* +These modes only impact automated user ID and login collection, either for business logic events or for authenticated user tracking, and should be disregarded when the collection is performed through the various SDKs. + + +In the disabled mode, as the name suggests, libraries should not collect user ID or user login. Effectively, this means that libraries shouldn’t send automated business logic events, specifically login and signup events, nor should they automatically track authenticated requests. +*/ + +function passportTrackEvent (credentials, passportUser, rootSpan) { + if (!collectionMode) return + + // If a passportUser object is published then the login succeded + if (passportUser) { + const userId = getUserId(passportUser) + + if (userId === undefined) { + log.warn('No user ID found in authentication instrumentation') + // telemetry counter: 'appsec.instrum.user_auth.missing_user_id' + return + } + + setUserTags({ id: userId }, rootSpan) + + trackEvent('users.login.success', null, 'passportTrackEvent', rootSpan, collectionMode) + + // call WAF ephemeral + } else { + const login = getLogin(credentials) + + if (!login) { + return // idk + } + + trackEvent('users.login.failure', { 'usr.id': login, login }, 'passportTrackEvent', rootSpan, collectionMode) } } module.exports = { - setCollectionMode + setCollectionMode, + trackLogin } From be473e6dc6ca2aa7a4a3e4529e7705e6db13b71c Mon Sep 17 00:00:00 2001 From: simon-id Date: Wed, 30 Oct 2024 23:25:32 +0100 Subject: [PATCH 035/126] push some stuff --- packages/dd-trace/src/appsec/index.js | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/packages/dd-trace/src/appsec/index.js b/packages/dd-trace/src/appsec/index.js index 89e09d3c3ed..cf05d457168 100644 --- a/packages/dd-trace/src/appsec/index.js +++ b/packages/dd-trace/src/appsec/index.js @@ -212,6 +212,20 @@ function onResponseBody ({ req, body }) { }, req) } +function onPassportDeserializeUser ({ user, abordController }) { + // if (!collectionMode) return + + // if (collectionMode === 'anon') user.id = hashUser(user.id) + + // don't override if already set by "sdk" but still add the _dd.appsec.usr.id + + rootSpan.addTags({ + 'usr.id': user.id, + '_dd.appsec.usr.id': user.id, // always AND only send when automated + '_dd.appsec.user.collection_mode': collectionMode + }) +} + function onPassportVerify ({ credentials, user, abortController }) { const store = storage.getStore() const rootSpan = store?.req && web.root(store.req) From 4a566a8862d28b4d1dec0a1ed310acdbe661c0a0 Mon Sep 17 00:00:00 2001 From: simon-id Date: Thu, 31 Oct 2024 00:04:55 +0100 Subject: [PATCH 036/126] cleanup --- .../src/appsec/user_tracking/index.js | 31 +++++++++---------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/packages/dd-trace/src/appsec/user_tracking/index.js b/packages/dd-trace/src/appsec/user_tracking/index.js index 2aef8547482..f0b147aaeae 100644 --- a/packages/dd-trace/src/appsec/user_tracking/index.js +++ b/packages/dd-trace/src/appsec/user_tracking/index.js @@ -3,8 +3,7 @@ const crypto = require('crypto') const log = require('../../log') -const SDK_USER_EVENT_PATTERN = '^_dd\\.appsec\\.events\\.users\\.[\\W\\w+]+\\.sdk$' -const regexSdkEvent = new RegExp(SDK_USER_EVENT_PATTERN, 'i') +const SDK_EVENT_REGEX = /^_dd\.appsec\.events\.users\.[\W\w+]+\.sdk$/i // The user ID generated must be consistent and repeatable meaning that, for a given framework, the same field must always be used. const USER_ID_FIELDS = ['id', '_id', 'email', 'username', 'login', 'user'] @@ -15,34 +14,32 @@ function setCollectionMode (mode, overwrite = true) { // don't overwrite if already set, only used in appsec/index.js to not overwrite RC values if (!overwrite && collectionMode) return + /* eslint-disable no-fallthrough */ switch (mode) { - case 'extended': - log.warn('Using deprecated value') // eslint-ignore-line no-fallthrough - case 'ident': - case 'identification': - collectionMode = 'ident' - break case 'safe': - log.warn('Using deprecated value') // eslint-ignore-line no-fallthrough + log.warn('Using deprecated value "safe" in config.appsec.eventTracking.mode') case 'anon': case 'anonymization': collectionMode = 'anon' break + case 'extended': + log.warn('Using deprecated value "extended" in config.appsec.eventTracking.mode') + case 'ident': + case 'identification': + collectionMode = 'ident' + break default: collectionMode = 'disabled' } } +/* eslint-enable no-fallthrough */ function isSdkCalled (rootSpan) { - const tags = rootSpan && rootSpan.context() && rootSpan.context()._tags + const tags = rootSpan?.context()?._tags - let called = false + if (tags === null || typeof tags !== 'object') return false - if (tags !== null && typeof tags === 'object') { - called = Object.entries(tags).some(([key, value]) => regexSdkEvent.test(key) && value === 'true') - } - - return called + return Object.entries(tags).some(([key, value]) => SDK_EVENT_REGEX.test(key) && value === 'true') } function getUserId (user) { @@ -56,7 +53,7 @@ function getUserId (user) { return id } } -} ù +} function obfuscateId (id) { return 'anon_' + crypto.createHash('sha256').update(id).digest().toString('hex', 0, 16).toLowerCase() From 882bc8bf9daa08da747bda79da73bf16593475f5 Mon Sep 17 00:00:00 2001 From: simon-id Date: Thu, 31 Oct 2024 00:08:12 +0100 Subject: [PATCH 037/126] cleanup --- packages/dd-trace/src/appsec/user_tracking/index.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/dd-trace/src/appsec/user_tracking/index.js b/packages/dd-trace/src/appsec/user_tracking/index.js index f0b147aaeae..f4588a6dd9a 100644 --- a/packages/dd-trace/src/appsec/user_tracking/index.js +++ b/packages/dd-trace/src/appsec/user_tracking/index.js @@ -5,8 +5,7 @@ const log = require('../../log') const SDK_EVENT_REGEX = /^_dd\.appsec\.events\.users\.[\W\w+]+\.sdk$/i -// The user ID generated must be consistent and repeatable meaning that, for a given framework, the same field must always be used. -const USER_ID_FIELDS = ['id', '_id', 'email', 'username', 'login', 'user'] +const USER_ID_FIELDS = ['id', '_id', 'email', 'username', 'login', 'user'] // The user ID generated must be consistent and repeatable meaning that, for a given framework, the same field must always be used. let collectionMode @@ -31,8 +30,8 @@ function setCollectionMode (mode, overwrite = true) { default: collectionMode = 'disabled' } + /* eslint-enable no-fallthrough */ } -/* eslint-enable no-fallthrough */ function isSdkCalled (rootSpan) { const tags = rootSpan?.context()?._tags @@ -46,6 +45,8 @@ function getUserId (user) { for (const field of USER_ID_FIELDS) { let id = user[field] if (id) { + id = id.toString() + if (collectionMode === 'anon') { id = obfuscateId(id) } From d4345ef735481be41dfb341e797d10253341a520 Mon Sep 17 00:00:00 2001 From: simon-id Date: Thu, 31 Oct 2024 13:29:11 +0100 Subject: [PATCH 038/126] add new usr.login waf address --- packages/dd-trace/src/appsec/addresses.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/dd-trace/src/appsec/addresses.js b/packages/dd-trace/src/appsec/addresses.js index 40c643012ef..e02f1a5018d 100644 --- a/packages/dd-trace/src/appsec/addresses.js +++ b/packages/dd-trace/src/appsec/addresses.js @@ -1,5 +1,6 @@ 'use strict' +// TODO: reorder all this, it's a mess module.exports = { HTTP_INCOMING_BODY: 'server.request.body', HTTP_INCOMING_QUERY: 'server.request.query', @@ -20,6 +21,8 @@ module.exports = { HTTP_CLIENT_IP: 'http.client_ip', USER_ID: 'usr.id', + USER_LOGIN: 'usr.login', + WAF_CONTEXT_PROCESSOR: 'waf.context.processor', HTTP_OUTGOING_URL: 'server.io.net.url', From be4a06c8dfddf3f8da14c3ca3b624595fc86aa3a Mon Sep 17 00:00:00 2001 From: simon-id Date: Thu, 31 Oct 2024 15:28:53 +0100 Subject: [PATCH 039/126] commit some stuff --- .../src/appsec/user_tracking/index.js | 34 +++++++++++-------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/packages/dd-trace/src/appsec/user_tracking/index.js b/packages/dd-trace/src/appsec/user_tracking/index.js index f4588a6dd9a..65fc3deddb2 100644 --- a/packages/dd-trace/src/appsec/user_tracking/index.js +++ b/packages/dd-trace/src/appsec/user_tracking/index.js @@ -3,9 +3,8 @@ const crypto = require('crypto') const log = require('../../log') -const SDK_EVENT_REGEX = /^_dd\.appsec\.events\.users\.[\W\w+]+\.sdk$/i - -const USER_ID_FIELDS = ['id', '_id', 'email', 'username', 'login', 'user'] // The user ID generated must be consistent and repeatable meaning that, for a given framework, the same field must always be used. +// the official list doesn't include '_id', but it's common in MongoDB +const USER_ID_FIELDS = ['id', '_id', 'email', 'username', 'login', 'user'] let collectionMode @@ -19,39 +18,44 @@ function setCollectionMode (mode, overwrite = true) { log.warn('Using deprecated value "safe" in config.appsec.eventTracking.mode') case 'anon': case 'anonymization': - collectionMode = 'anon' + collectionMode = 'anonymization' break + case 'extended': log.warn('Using deprecated value "extended" in config.appsec.eventTracking.mode') case 'ident': case 'identification': - collectionMode = 'ident' + collectionMode = 'identification' break + default: collectionMode = 'disabled' } /* eslint-enable no-fallthrough */ } -function isSdkCalled (rootSpan) { - const tags = rootSpan?.context()?._tags - - if (tags === null || typeof tags !== 'object') return false - - return Object.entries(tags).some(([key, value]) => SDK_EVENT_REGEX.test(key) && value === 'true') +function obfuscateIfNeeded (str) { + if (collectionMode === 'anonymization') { + return 'anon_' + crypto.createHash('sha256').update(str).digest().toString('hex', 0, 16).toLowerCase() + } else { + return str + } } function getUserId (user) { + if (!user) return + for (const field of USER_ID_FIELDS) { let id = user[field] - if (id) { + if (id && typeof id.toString === 'function') { id = id.toString() - if (collectionMode === 'anon') { - id = obfuscateId(id) + if (id.startsWith('[object ')) { + // probably not a usable ID ? + continue } - return id + return obfuscateIfNeeded(id) } } } From f129890459df351cb86b28afef85818c1049c8f0 Mon Sep 17 00:00:00 2001 From: simon-id Date: Thu, 31 Oct 2024 23:59:27 +0100 Subject: [PATCH 040/126] cleanup --- packages/datadog-instrumentations/src/passport.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/datadog-instrumentations/src/passport.js b/packages/datadog-instrumentations/src/passport.js index c3c8be99b4a..92a0bcf1636 100644 --- a/packages/datadog-instrumentations/src/passport.js +++ b/packages/datadog-instrumentations/src/passport.js @@ -18,9 +18,12 @@ koa-passport function wrapDone (done) { // eslint-disable-next-line n/handle-callback-err return function wrappedDone (err, user) { - console.log('USER DESERIALIZE', user) if (user) { - // channel.publish({ req, user }) + const abortController = new AbortController() + + channel.publish({ req, user, abortController }) + + if (abortController.signal.aborted) return } return done.apply(this, arguments) @@ -62,7 +65,6 @@ addHook({ return function wrappedMiddleware (req, res, next) { return middleware(req, res, function wrappedNext (err) { - strategy console.log('NEW', req.user) if (req.user?.name === 'bitch') { From cceac23b180fa9c43c447de76f60075e24de079e Mon Sep 17 00:00:00 2001 From: simon-id Date: Fri, 1 Nov 2024 00:04:58 +0100 Subject: [PATCH 041/126] cleanup --- packages/dd-trace/src/appsec/index.js | 12 +----------- .../dd-trace/src/appsec/user_tracking/index.js | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/packages/dd-trace/src/appsec/index.js b/packages/dd-trace/src/appsec/index.js index c307e2f1fe5..82341e56d97 100644 --- a/packages/dd-trace/src/appsec/index.js +++ b/packages/dd-trace/src/appsec/index.js @@ -181,17 +181,7 @@ function incomingHttpEndTranslator ({ req, res }) { } function onPassportDeserializeUser ({ user, abordController }) { - // if (!collectionMode) return - - // if (collectionMode === 'anon') user.id = hashUser(user.id) - - // don't override if already set by "sdk" but still add the _dd.appsec.usr.id - - rootSpan.addTags({ - 'usr.id': user.id, - '_dd.appsec.usr.id': user.id, // always AND only send when automated - '_dd.appsec.user.collection_mode': collectionMode - }) + UserTracking.trackUser(user, abortController) } function onPassportVerify ({ login, user, success }) { diff --git a/packages/dd-trace/src/appsec/user_tracking/index.js b/packages/dd-trace/src/appsec/user_tracking/index.js index f4588a6dd9a..84fcbb96bfa 100644 --- a/packages/dd-trace/src/appsec/user_tracking/index.js +++ b/packages/dd-trace/src/appsec/user_tracking/index.js @@ -127,6 +127,21 @@ function passportTrackEvent (credentials, passportUser, rootSpan) { } } + +function trackUser (user, abortController) { + // if (!collectionMode) return + + // if (collectionMode === 'anon') user.id = hashUser(user.id) + + // don't override if already set by "sdk" but still add the _dd.appsec.usr.id + + rootSpan.addTags({ + 'usr.id': user.id, + '_dd.appsec.usr.id': user.id, // always AND only send when automated + '_dd.appsec.user.collection_mode': collectionMode + }) +} + module.exports = { setCollectionMode, trackLogin From 654d260b742188139bef84003f1aaf54e7b41a1f Mon Sep 17 00:00:00 2001 From: simon-id Date: Fri, 1 Nov 2024 00:15:35 +0100 Subject: [PATCH 042/126] add comment for later --- packages/datadog-instrumentations/src/passport.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/datadog-instrumentations/src/passport.js b/packages/datadog-instrumentations/src/passport.js index 92a0bcf1636..5e5e7d39770 100644 --- a/packages/datadog-instrumentations/src/passport.js +++ b/packages/datadog-instrumentations/src/passport.js @@ -20,6 +20,8 @@ function wrapDone (done) { return function wrappedDone (err, user) { if (user) { const abortController = new AbortController() + + // express-session middleware sets req.sessionID, it's required to use passport sessions anyway so might as well use it ? channel.publish({ req, user, abortController }) From 066da87c134c0e1be06d1dc41a9bb0ee4ead5860 Mon Sep 17 00:00:00 2001 From: simon-id Date: Fri, 1 Nov 2024 00:18:39 +0100 Subject: [PATCH 043/126] add some ideas --- packages/dd-trace/src/appsec/user_tracking/index.js | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/dd-trace/src/appsec/user_tracking/index.js b/packages/dd-trace/src/appsec/user_tracking/index.js index 84fcbb96bfa..c958547f1a8 100644 --- a/packages/dd-trace/src/appsec/user_tracking/index.js +++ b/packages/dd-trace/src/appsec/user_tracking/index.js @@ -129,12 +129,15 @@ function passportTrackEvent (credentials, passportUser, rootSpan) { function trackUser (user, abortController) { - // if (!collectionMode) return + if (!collectionMode || collectionMode === 'disabled') return - // if (collectionMode === 'anon') user.id = hashUser(user.id) + const userId = getUserId(user) - // don't override if already set by "sdk" but still add the _dd.appsec.usr.id + if (!userId) { + + } + // don't override if already set by "sdk" but still add the _dd.appsec.usr.id rootSpan.addTags({ 'usr.id': user.id, '_dd.appsec.usr.id': user.id, // always AND only send when automated From da9b27032969e13e6fa9179814aa621107fe2839 Mon Sep 17 00:00:00 2001 From: simon-id Date: Fri, 1 Nov 2024 01:17:15 +0100 Subject: [PATCH 044/126] cleanup --- packages/dd-trace/src/appsec/sdk/set_user.js | 26 ++++++++++++++----- .../src/appsec/user_tracking/index.js | 15 ++++++++--- 2 files changed, 31 insertions(+), 10 deletions(-) diff --git a/packages/dd-trace/src/appsec/sdk/set_user.js b/packages/dd-trace/src/appsec/sdk/set_user.js index 07a075bfea1..4ba3c88eb95 100644 --- a/packages/dd-trace/src/appsec/sdk/set_user.js +++ b/packages/dd-trace/src/appsec/sdk/set_user.js @@ -2,6 +2,7 @@ const { getRootSpan } = require('./utils') const log = require('../../log') +const waf = require('../waf') function setUserTags (user, rootSpan) { for (const k of Object.keys(user)) { @@ -23,16 +24,29 @@ function setUser (tracer, user) { // must get user ID with USER_ID_FIELDS - // _dd.appsec.user.collection_mode: collectionMode // sdk/ident/anon - - setUserTags(user, rootSpan) + setTags({ + 'usr.id': userId, + '_dd.appsec.user.collection_mode': 'sdk/ident/anon' + }) /* - User IDs generated through the SDK must now be provided to libddwaf as a persistent addresses. - If the user monitoring SDK has already resulted in a call to libddwaf before any automated instrumentation or collection method has been executed, no extra call should be made. - If the automated instrumentation or collection method has resulted in a call to libddwaf before the user monitoring SDK has been executed, a second call must be performed with the user ID obtained through the SDK. + When the user monitoring SDK is available and in use, the following applies: + The usr.id must be set to the value provided through the user monitoring SDK. + The span tag _dd.appsec.user.collection_mode must be set to sdk. + This effectively means that the automated user ID collection mechanism must not overwrite the aforementioned span tags, while the user monitoring SDK must overwrite them if present. */ + + + setUserTags(user, rootSpan) + + // If the automated instrumentation or collection method has resulted in a call to libddwaf before the user monitoring SDK has been executed, a second call must be performed with the user ID obtained through the SDK. // will the second call trigger tho ? make some edge case tests + waf.run({ + persistent: { + [USER_ID]: userId + } + }) + } module.exports = { diff --git a/packages/dd-trace/src/appsec/user_tracking/index.js b/packages/dd-trace/src/appsec/user_tracking/index.js index c958547f1a8..52b96475b1f 100644 --- a/packages/dd-trace/src/appsec/user_tracking/index.js +++ b/packages/dd-trace/src/appsec/user_tracking/index.js @@ -137,11 +137,18 @@ function trackUser (user, abortController) { } - // don't override if already set by "sdk" but still add the _dd.appsec.usr.id + // don't override tags if already set by "sdk" but still add the _dd.appsec.usr.id rootSpan.addTags({ - 'usr.id': user.id, - '_dd.appsec.usr.id': user.id, // always AND only send when automated - '_dd.appsec.user.collection_mode': collectionMode + 'usr.id': userId, + '_dd.appsec.usr.id': userId, // always AND only send when automated + '_dd.appsec.user.collection_mode': collectionMode // short or long form ? + }) + + // If the user monitoring SDK has already resulted in a call to libddwaf before any automated instrumentation or collection method has been executed, no extra call should be made. + const results = waf.run({ + persistent: { + [USER_ID]: userID + } }) } From 01c824022bd4504c790ee4c92566f7e277de8f9c Mon Sep 17 00:00:00 2001 From: simon-id Date: Fri, 1 Nov 2024 02:02:55 +0100 Subject: [PATCH 045/126] aaaaa --- packages/datadog-instrumentations/src/passport.js | 8 +++++--- packages/dd-trace/src/appsec/index.js | 10 +++++++++- packages/dd-trace/src/appsec/sdk/set_user.js | 13 ++++++++++++- 3 files changed, 26 insertions(+), 5 deletions(-) diff --git a/packages/datadog-instrumentations/src/passport.js b/packages/datadog-instrumentations/src/passport.js index 5e5e7d39770..6429bb67b0a 100644 --- a/packages/datadog-instrumentations/src/passport.js +++ b/packages/datadog-instrumentations/src/passport.js @@ -4,7 +4,9 @@ const shimmer = require('../../datadog-shimmer') const { addHook } = require('./helpers/instrument') /* TODO: test with: -passport-jwt +passport-jwt JWTs + can be used both for login events, or as a session, that complicates things it think + maybe instrument this lib directly, and ofc only send the events after it was verified @nestjs/passport pasport-local passport-oauth2 @@ -22,8 +24,8 @@ function wrapDone (done) { const abortController = new AbortController() // express-session middleware sets req.sessionID, it's required to use passport sessions anyway so might as well use it ? - - channel.publish({ req, user, abortController }) + // what if session IDs are using rolling sessions or always changing or something idk ? + channel.publish({ req, user, sessionId: req.sessionID, abortController }) if (abortController.signal.aborted) return } diff --git a/packages/dd-trace/src/appsec/index.js b/packages/dd-trace/src/appsec/index.js index 82341e56d97..e78f1f42a35 100644 --- a/packages/dd-trace/src/appsec/index.js +++ b/packages/dd-trace/src/appsec/index.js @@ -180,8 +180,16 @@ function incomingHttpEndTranslator ({ req, res }) { Reporter.finishRequest(req, res) } -function onPassportDeserializeUser ({ user, abordController }) { +function onPassportDeserializeUser ({ req, user, sessionId, abordController }) { UserTracking.trackUser(user, abortController) + + if (sessionId && typeof sessionId === 'string') { + const results = waf.run({ + persistent: { + 'usr.session_id': sessionId + } + }) + } } function onPassportVerify ({ login, user, success }) { diff --git a/packages/dd-trace/src/appsec/sdk/set_user.js b/packages/dd-trace/src/appsec/sdk/set_user.js index 4ba3c88eb95..d1293878d7c 100644 --- a/packages/dd-trace/src/appsec/sdk/set_user.js +++ b/packages/dd-trace/src/appsec/sdk/set_user.js @@ -35,13 +35,24 @@ function setUser (tracer, user) { The span tag _dd.appsec.user.collection_mode must be set to sdk. This effectively means that the automated user ID collection mechanism must not overwrite the aforementioned span tags, while the user monitoring SDK must overwrite them if present. */ + + + + /* + When a user provides their own session ID through the use of the SDK using the session_id key: + If libddwaf hasn’t already been called with the usr.session_id address, it should be called with the provided session_id and further calls through the automated collection method should be inhibited. + If libddwaf has already been called with the usr.session_id address, it should be called again. + */ + if (user.session_id && typeof user.session_id === 'string') { + persistent['usr.session_id'] = user.session_id + } setUserTags(user, rootSpan) // If the automated instrumentation or collection method has resulted in a call to libddwaf before the user monitoring SDK has been executed, a second call must be performed with the user ID obtained through the SDK. // will the second call trigger tho ? make some edge case tests - waf.run({ + const results = waf.run({ persistent: { [USER_ID]: userId } From cc86e3a347c1eb345bde0e775e548a3331734a14 Mon Sep 17 00:00:00 2001 From: simon-id Date: Fri, 1 Nov 2024 02:24:42 +0100 Subject: [PATCH 046/126] pass abort controller for blocking --- packages/dd-trace/src/appsec/index.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/dd-trace/src/appsec/index.js b/packages/dd-trace/src/appsec/index.js index 02f9f008379..bd0e50f645a 100644 --- a/packages/dd-trace/src/appsec/index.js +++ b/packages/dd-trace/src/appsec/index.js @@ -180,7 +180,7 @@ function incomingHttpEndTranslator ({ req, res }) { Reporter.finishRequest(req, res) } -function onPassportVerify ({ login, user, success }) { +function onPassportVerify ({ login, user, success, abortController }) { const store = storage.getStore() const rootSpan = store?.req && web.root(store.req) @@ -189,7 +189,7 @@ function onPassportVerify ({ login, user, success }) { return } - UserTracking.trackLogin(login, user, success, rootSpan) + UserTracking.trackLogin(login, user, success, rootSpan, abortController) } function onRequestQueryParsed ({ req, res, query, abortController }) { From 69d12a97f4ac3e0376a737b78e4437ddb2a041e7 Mon Sep 17 00:00:00 2001 From: simon-id Date: Thu, 14 Nov 2024 21:51:07 +0100 Subject: [PATCH 047/126] push some notes --- packages/dd-trace/src/appsec/sdk/set_user.js | 2 +- packages/dd-trace/src/appsec/user_tracking/index.js | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/dd-trace/src/appsec/sdk/set_user.js b/packages/dd-trace/src/appsec/sdk/set_user.js index d1293878d7c..7dc556fc346 100644 --- a/packages/dd-trace/src/appsec/sdk/set_user.js +++ b/packages/dd-trace/src/appsec/sdk/set_user.js @@ -26,7 +26,7 @@ function setUser (tracer, user) { setTags({ 'usr.id': userId, - '_dd.appsec.user.collection_mode': 'sdk/ident/anon' + '_dd.appsec.user.collection_mode': 'sdk' }) /* diff --git a/packages/dd-trace/src/appsec/user_tracking/index.js b/packages/dd-trace/src/appsec/user_tracking/index.js index 52b96475b1f..e48b5d39aa3 100644 --- a/packages/dd-trace/src/appsec/user_tracking/index.js +++ b/packages/dd-trace/src/appsec/user_tracking/index.js @@ -137,6 +137,8 @@ function trackUser (user, abortController) { } + // ANONYMISE user id if needed + // don't override tags if already set by "sdk" but still add the _dd.appsec.usr.id rootSpan.addTags({ 'usr.id': userId, From ce604c1d5927ac7f53166f5eaa6528841a1dd282 Mon Sep 17 00:00:00 2001 From: simon-id Date: Fri, 15 Nov 2024 11:58:28 +0100 Subject: [PATCH 048/126] change config default --- packages/dd-trace/src/config.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/dd-trace/src/config.js b/packages/dd-trace/src/config.js index 4454bb5e66c..509bdb2457e 100644 --- a/packages/dd-trace/src/config.js +++ b/packages/dd-trace/src/config.js @@ -449,7 +449,7 @@ class Config { this._setValue(defaults, 'appsec.blockedTemplateHtml', undefined) this._setValue(defaults, 'appsec.blockedTemplateJson', undefined) this._setValue(defaults, 'appsec.enabled', undefined) - this._setValue(defaults, 'appsec.eventTracking.mode', 'ident') + this._setValue(defaults, 'appsec.eventTracking.mode', 'identification') this._setValue(defaults, 'appsec.obfuscatorKeyRegex', defaultWafObfuscatorKeyRegex) this._setValue(defaults, 'appsec.obfuscatorValueRegex', defaultWafObfuscatorValueRegex) this._setValue(defaults, 'appsec.rasp.enabled', true) From 56c30e88245f8a17610f6128dcd83ee29a497952 Mon Sep 17 00:00:00 2001 From: simon-id Date: Fri, 15 Nov 2024 12:18:26 +0100 Subject: [PATCH 049/126] handle duplicate RC confs for auto_user_instrum.mode --- .../src/appsec/remote_config/index.js | 29 ++++++++++++++----- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/packages/dd-trace/src/appsec/remote_config/index.js b/packages/dd-trace/src/appsec/remote_config/index.js index 044c330083d..f2a5bd6fd45 100644 --- a/packages/dd-trace/src/appsec/remote_config/index.js +++ b/packages/dd-trace/src/appsec/remote_config/index.js @@ -31,22 +31,35 @@ function enable (config, appsec) { rc.updateCapabilities(RemoteConfigCapabilities.ASM_AUTO_USER_INSTRUM_MODE, true) - rc.setProductHandler('ASM_FEATURES', (action, rcConfig) => { - if (!rcConfig) return + let autoUserInstrumModeId - if (activation === Activation.ONECLICK) { - enableOrDisableAppsec(action, rcConfig, config, appsec) - } - - apiSecuritySampler.setRequestSampling(rcConfig.api_security?.request_sample_rate) + rc.setProductHandler('ASM_FEATURES', (action, rcConfig, configId) => { + if (!rcConfig) return + // this is put before other handlers because it can reject the config if (typeof rcConfig.auto_user_instrum?.mode === 'string') { if (action === 'apply' || action === 'modify') { + // check if there is already a config applied with this field + if (autoUserInstrumModeId && configId !== autoUserInstrumModeId) { + // eslint-disable-next-line no-throw-literal + throw 'Multiple auto_user_instrum.mode received in ASM_FEATURES' + } + setCollectionMode(rcConfig.auto_user_instrum.mode) + autoUserInstrumModeId = configId } else { - setCollectionMode(config.appsec.eventTracking.mode) + if (configId === autoUserInstrumModeId) { + setCollectionMode(config.appsec.eventTracking.mode) + autoUserInstrumModeId = null + } } } + + if (activation === Activation.ONECLICK) { + enableOrDisableAppsec(action, rcConfig, config, appsec) + } + + apiSecuritySampler.setRequestSampling(rcConfig.api_security?.request_sample_rate) }) } From 1d3773a9c7448619d1bb3c0c3469626dfae47be8 Mon Sep 17 00:00:00 2001 From: simon-id Date: Fri, 15 Nov 2024 12:27:09 +0100 Subject: [PATCH 050/126] refactor sdk/track_event.js to only be used by the SDK --- .../dd-trace/src/appsec/sdk/track_event.js | 51 ++++++++----------- 1 file changed, 21 insertions(+), 30 deletions(-) diff --git a/packages/dd-trace/src/appsec/sdk/track_event.js b/packages/dd-trace/src/appsec/sdk/track_event.js index 876dd0a8f15..3f3d8a66719 100644 --- a/packages/dd-trace/src/appsec/sdk/track_event.js +++ b/packages/dd-trace/src/appsec/sdk/track_event.js @@ -7,6 +7,7 @@ const standalone = require('../standalone') const waf = require('../waf') const { SAMPLING_MECHANISM_APPSEC } = require('../../constants') const { keepTrace } = require('../../priority_sampler') +const addresses = require('../addresses') function trackUserLoginSuccessEvent (tracer, user, metadata) { // TODO: better user check here and in _setUser() ? @@ -23,7 +24,9 @@ function trackUserLoginSuccessEvent (tracer, user, metadata) { setUserTags(user, rootSpan) - trackEvent('users.login.success', metadata, 'trackUserLoginSuccessEvent', rootSpan, 'sdk') + trackEvent('users.login.success', metadata, 'trackUserLoginSuccessEvent', rootSpan) + + runWaf('users.login.success', user) } function trackUserLoginFailureEvent (tracer, userId, exists, metadata) { @@ -38,7 +41,9 @@ function trackUserLoginFailureEvent (tracer, userId, exists, metadata) { ...metadata } - trackEvent('users.login.failure', fields, 'trackUserLoginFailureEvent', getRootSpan(tracer), 'sdk') + trackEvent('users.login.failure', fields, 'trackUserLoginFailureEvent', getRootSpan(tracer)) + + runWaf('users.login.failure', { login: userId }) } function trackCustomEvent (tracer, eventName, metadata) { @@ -47,10 +52,10 @@ function trackCustomEvent (tracer, eventName, metadata) { return } - trackEvent(eventName, metadata, 'trackCustomEvent', getRootSpan(tracer), 'sdk') + trackEvent(eventName, metadata, 'trackCustomEvent', getRootSpan(tracer)) } -function trackEvent (eventName, fields, sdkMethodName, rootSpan, mode, abortController) { +function trackEvent (eventName, fields, sdkMethodName, rootSpan) { if (!rootSpan) { log.warn(`Root span not available in ${sdkMethodName}`) return @@ -59,15 +64,8 @@ function trackEvent (eventName, fields, sdkMethodName, rootSpan, mode, abortCont keepTrace(rootSpan, SAMPLING_MECHANISM_APPSEC) const tags = { - [`appsec.events.${eventName}.track`]: 'true' - } - - if (mode === 'sdk') { - tags[`_dd.appsec.events.${eventName}.sdk`] = 'true' - } - - if (mode && mode !== 'sdk') { - tags[`_dd.appsec.events.${eventName}.auto.mode`] = mode + [`appsec.events.${eventName}.track`]: 'true', + [`_dd.appsec.events.${eventName}.sdk`]: 'true' } if (fields) { @@ -76,32 +74,25 @@ function trackEvent (eventName, fields, sdkMethodName, rootSpan, mode, abortCont } } - const persistent = { + rootSpan.addTags(tags) + + standalone.sample(rootSpan) +} +function runWaf (eventName, user) { + const persistent = { + [`server.business_logic.${eventName}`]: null } if (user.id) { - persistent[addresses.USER_ID] = user.id + persistent[addresses.USER_ID] = '' + user.id } if (user.login) { - persistent[addresses.USER_LOGIN] = user.login - } - - // call WAF - const results = waf.run({ persistent }) - - if (abortController) { - Blocking.handleResults(results) + persistent[addresses.USER_LOGIN] = '' + user.login } - rootSpan.addTags(tags) - - standalone.sample(rootSpan) - - if (['users.login.success', 'users.login.failure'].includes(eventName)) { - waf.run({ persistent: { [`server.business_logic.${eventName}`]: null } }) - } + waf.run({ persistent }) } module.exports = { From a72695f869901daa3394a213d0d0d4dc7d18ba6d Mon Sep 17 00:00:00 2001 From: simon-id Date: Tue, 19 Nov 2024 13:28:50 +0100 Subject: [PATCH 051/126] push some stuff --- packages/dd-trace/src/appsec/index.js | 1 + packages/dd-trace/src/appsec/sdk/set_user.js | 20 +++++++++++++++++++ .../src/appsec/user_tracking/index.js | 4 ++++ 3 files changed, 25 insertions(+) diff --git a/packages/dd-trace/src/appsec/index.js b/packages/dd-trace/src/appsec/index.js index 1b7e88fb59b..5ac4a47b31e 100644 --- a/packages/dd-trace/src/appsec/index.js +++ b/packages/dd-trace/src/appsec/index.js @@ -66,6 +66,7 @@ function enable (_config) { incomingHttpRequestStart.subscribe(incomingHttpStartTranslator) incomingHttpRequestEnd.subscribe(incomingHttpEndTranslator) passportVerify.subscribe(onPassportVerify) // possible optimization: only subscribe if collection mode is enabled + passportUser.subscribe(onPassportDeserializeUser) queryParser.subscribe(onRequestQueryParsed) nextBodyParsed.subscribe(onRequestBodyParsed) nextQueryParsed.subscribe(onRequestQueryParsed) diff --git a/packages/dd-trace/src/appsec/sdk/set_user.js b/packages/dd-trace/src/appsec/sdk/set_user.js index 7dc556fc346..571ec7a9433 100644 --- a/packages/dd-trace/src/appsec/sdk/set_user.js +++ b/packages/dd-trace/src/appsec/sdk/set_user.js @@ -58,6 +58,26 @@ function setUser (tracer, user) { } }) + + + const persistent = {} + + if (user.id) { + persistent[addresses.USER_ID] = '' + user.id + } + + if (user.login) { + persistent[addresses.USER_LOGIN] = '' + user.login + } + + waf.run({ persistent }) + + /* + User IDs generated through the SDK must now be provided to libddwaf as a persistent addresses. + If the user monitoring SDK has already resulted in a call to libddwaf before any automated instrumentation or collection method has been executed, no extra call should be made. + If the automated instrumentation or collection method has resulted in a call to libddwaf before the user monitoring SDK has been executed, a second call must be performed with the user ID obtained through the SDK. + */ + // will the second call trigger tho ? make some edge case tests } module.exports = { diff --git a/packages/dd-trace/src/appsec/user_tracking/index.js b/packages/dd-trace/src/appsec/user_tracking/index.js index f87795494e1..5e2e0c9c99c 100644 --- a/packages/dd-trace/src/appsec/user_tracking/index.js +++ b/packages/dd-trace/src/appsec/user_tracking/index.js @@ -137,6 +137,8 @@ function trackUser (user, abortController) { const userId = getUserId(user) + // must get user ID with USER_ID_FIELDS + if (!userId) { } @@ -150,6 +152,8 @@ function trackUser (user, abortController) { '_dd.appsec.user.collection_mode': collectionMode // short or long form ? }) + // _dd.appsec.user.collection_mode: collectionMode // sdk/ident/anon + // If the user monitoring SDK has already resulted in a call to libddwaf before any automated instrumentation or collection method has been executed, no extra call should be made. const results = waf.run({ persistent: { From 91a59dbd08ef7cfc6d182493f9b0fc84d9c613b3 Mon Sep 17 00:00:00 2001 From: simon-id Date: Tue, 19 Nov 2024 13:29:04 +0100 Subject: [PATCH 052/126] remove some comments --- packages/dd-trace/src/appsec/sdk/set_user.js | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/packages/dd-trace/src/appsec/sdk/set_user.js b/packages/dd-trace/src/appsec/sdk/set_user.js index 07a075bfea1..81b0e3ec7ad 100644 --- a/packages/dd-trace/src/appsec/sdk/set_user.js +++ b/packages/dd-trace/src/appsec/sdk/set_user.js @@ -21,18 +21,7 @@ function setUser (tracer, user) { return } - // must get user ID with USER_ID_FIELDS - - // _dd.appsec.user.collection_mode: collectionMode // sdk/ident/anon - setUserTags(user, rootSpan) - - /* - User IDs generated through the SDK must now be provided to libddwaf as a persistent addresses. - If the user monitoring SDK has already resulted in a call to libddwaf before any automated instrumentation or collection method has been executed, no extra call should be made. - If the automated instrumentation or collection method has resulted in a call to libddwaf before the user monitoring SDK has been executed, a second call must be performed with the user ID obtained through the SDK. - */ - // will the second call trigger tho ? make some edge case tests } module.exports = { From faea3aa54dbd02d722a81e296c691d250370ed66 Mon Sep 17 00:00:00 2001 From: simon-id Date: Tue, 19 Nov 2024 15:40:48 +0100 Subject: [PATCH 053/126] pass login to WAF in SDK login success event --- packages/dd-trace/src/appsec/sdk/track_event.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/dd-trace/src/appsec/sdk/track_event.js b/packages/dd-trace/src/appsec/sdk/track_event.js index 3f3d8a66719..61f8de04576 100644 --- a/packages/dd-trace/src/appsec/sdk/track_event.js +++ b/packages/dd-trace/src/appsec/sdk/track_event.js @@ -26,7 +26,7 @@ function trackUserLoginSuccessEvent (tracer, user, metadata) { trackEvent('users.login.success', metadata, 'trackUserLoginSuccessEvent', rootSpan) - runWaf('users.login.success', user) + runWaf('users.login.success', { id: user.id, login: user.login ?? user.id }) } function trackUserLoginFailureEvent (tracer, userId, exists, metadata) { From fe70c84d5f126af739369fac3f0a14ac058d4b5e Mon Sep 17 00:00:00 2001 From: simon-id Date: Tue, 19 Nov 2024 16:29:01 +0100 Subject: [PATCH 054/126] add comments --- packages/dd-trace/src/appsec/user_tracking/index.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/dd-trace/src/appsec/user_tracking/index.js b/packages/dd-trace/src/appsec/user_tracking/index.js index 65fc3deddb2..99224b73b8e 100644 --- a/packages/dd-trace/src/appsec/user_tracking/index.js +++ b/packages/dd-trace/src/appsec/user_tracking/index.js @@ -3,7 +3,7 @@ const crypto = require('crypto') const log = require('../../log') -// the official list doesn't include '_id', but it's common in MongoDB +// the RFC doesn't include '_id', but it's common in MongoDB const USER_ID_FIELDS = ['id', '_id', 'email', 'username', 'login', 'user'] let collectionMode @@ -36,17 +36,21 @@ function setCollectionMode (mode, overwrite = true) { function obfuscateIfNeeded (str) { if (collectionMode === 'anonymization') { + // get first 16 bytes of sha256 hash in lowercase hex return 'anon_' + crypto.createHash('sha256').update(str).digest().toString('hex', 0, 16).toLowerCase() } else { return str } } +// TODO: should we find other ways to get the user ID ? function getUserId (user) { if (!user) return for (const field of USER_ID_FIELDS) { let id = user[field] + + // try to find a field that can be stringified if (id && typeof id.toString === 'function') { id = id.toString() From e32f78e323aca22a7e41101ca204472e2c5be131 Mon Sep 17 00:00:00 2001 From: simon-id Date: Wed, 20 Nov 2024 12:07:22 +0100 Subject: [PATCH 055/126] add framework name to passport strategy instrum --- .../datadog-instrumentations/src/passport-utils.js | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/packages/datadog-instrumentations/src/passport-utils.js b/packages/datadog-instrumentations/src/passport-utils.js index df42fdeb542..cb0ed7ba389 100644 --- a/packages/datadog-instrumentations/src/passport-utils.js +++ b/packages/datadog-instrumentations/src/passport-utils.js @@ -5,14 +5,14 @@ const { channel } = require('./helpers/instrument') const passportVerifyChannel = channel('datadog:passport:verify:finish') -function wrapVerifiedAndPublish (username, verified) { +function wrapVerifiedAndPublish (framework, username, verified) { return shimmer.wrapFunction(verified, function wrapVerify (verified) { return function wrappedVerified (err, user) { // if there is an error, it's neither an auth success nor a failure if (!err) { const abortController = new AbortController() - passportVerifyChannel.publish({ login: username, user, success: !!user, abortController }) + passportVerifyChannel.publish({ framework, login: username, user, success: !!user, abortController }) if (abortController.signal.aborted) return } @@ -25,12 +25,13 @@ function wrapVerifiedAndPublish (username, verified) { function wrapVerify (verify) { return function wrappedVerify (req, username, password, verified) { if (passportVerifyChannel.hasSubscribers) { + const framework = `passport-${this.name}` + // replace the callback with our own wrapper to get the result - // if we ever need the type of strategy, we can get it from this.name if (this._passReqToCallback) { - arguments[3] = wrapVerifiedAndPublish(arguments[1], arguments[3]) + arguments[3] = wrapVerifiedAndPublish(framework, arguments[1], arguments[3]) } else { - arguments[2] = wrapVerifiedAndPublish(arguments[0], arguments[2]) + arguments[2] = wrapVerifiedAndPublish(framework, arguments[0], arguments[2]) } } From 806ab237623fdb9f5cbc21fc78ef24197f859da5 Mon Sep 17 00:00:00 2001 From: simon-id Date: Wed, 20 Nov 2024 12:11:53 +0100 Subject: [PATCH 056/126] add framework name and waf handleResults() to onPassportVerify() --- packages/dd-trace/src/appsec/index.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/dd-trace/src/appsec/index.js b/packages/dd-trace/src/appsec/index.js index bd0e50f645a..0d2fa1ffb14 100644 --- a/packages/dd-trace/src/appsec/index.js +++ b/packages/dd-trace/src/appsec/index.js @@ -180,7 +180,7 @@ function incomingHttpEndTranslator ({ req, res }) { Reporter.finishRequest(req, res) } -function onPassportVerify ({ login, user, success, abortController }) { +function onPassportVerify ({ framework, login, user, success, abortController }) { const store = storage.getStore() const rootSpan = store?.req && web.root(store.req) @@ -189,7 +189,9 @@ function onPassportVerify ({ login, user, success, abortController }) { return } - UserTracking.trackLogin(login, user, success, rootSpan, abortController) + const results = UserTracking.trackLogin(framework, login, user, success, rootSpan) + + handleResults(results, store.req, store.req.res, rootSpan, abortController) } function onRequestQueryParsed ({ req, res, query, abortController }) { From 1e1bb47a44a8069ee78c9c16e2e3f481dace7ebe Mon Sep 17 00:00:00 2001 From: simon-id Date: Thu, 21 Nov 2024 18:36:32 +0100 Subject: [PATCH 057/126] finally commit trackLogin() --- .../src/appsec/user_tracking/index.js | 117 +++++++++++------- 1 file changed, 74 insertions(+), 43 deletions(-) diff --git a/packages/dd-trace/src/appsec/user_tracking/index.js b/packages/dd-trace/src/appsec/user_tracking/index.js index 99224b73b8e..e58a1d3a7ef 100644 --- a/packages/dd-trace/src/appsec/user_tracking/index.js +++ b/packages/dd-trace/src/appsec/user_tracking/index.js @@ -2,6 +2,12 @@ const crypto = require('crypto') const log = require('../../log') +const telemetry = require('../telemetry') +const addresses = require('../addresses') +const { keepTrace } = require('../../priority_sampler') +const { SAMPLING_MECHANISM_APPSEC } = require('../../constants') +const standalone = require('../standalone') +const waf = require('../waf') // the RFC doesn't include '_id', but it's common in MongoDB const USER_ID_FIELDS = ['id', '_id', 'email', 'username', 'login', 'user'] @@ -64,75 +70,100 @@ function getUserId (user) { } } -function obfuscateId (id) { - return 'anon_' + crypto.createHash('sha256').update(id).digest().toString('hex', 0, 16).toLowerCase() -} +function trackLogin (framework, login, user, success, rootSpan) { + if (!collectionMode || collectionMode === 'disabled') return -function trackLogin (login, user, success, rootSpan) { - if (!collectionMode) return + if (!rootSpan) { + log.error('No rootSpan found in AppSec trackLogin') + return + } - // TODO: what if sdk is called after automated user + if (!login || typeof login !== 'string') { + log.error('Invalid login provided to AppSec trackLogin') - if (isSdkCalled(tags)) { - // Don't overwrite tags set by SDK callings + telemetry.incrementMissingUserLogin(framework, success ? 'login_success' : 'login_failure') + // note: + // if we start supporting using userId if login is missing, we need to only give up if both are missing, and + // implement 'appsec.instrum.user_auth.missing_user_id' telemetry too return } - if (collectionMode === 'anon') { - login = obfuscateId(login) - } + const currentTags = rootSpan.context()._tags + let isSdkCalled - if (success) { - // getID - sdk.trackEvent('users.login.success', null, 'passportTrackEvent', rootSpan, collectionMode) - } else { + login = obfuscateIfNeeded(login) + const userId = getUserId(user) + let newTags + + const persistent = { + [addresses.USER_LOGIN]: login } -} + // used to not overwrite tags set by SDK + function shouldSetTag (tag) { + return !isSdkCalled && !currentTags[tag] + } + if (success) { + isSdkCalled = currentTags['_dd.appsec.events.users.login.success.sdk'] === 'true' -// TODO passpoort-jwt ? + newTags = { + 'appsec.events.users.login.success.track': 'true', + '_dd.appsec.events.users.login.success.auto.mode': collectionMode, + '_dd.appsec.usr.login': login + } -// TODO: SDK always has precendence ? -// Whenever relevant, the user ID must be collected by the libraries as part of the root span, using the tag usr.id. -// Whenever relevant, the user login must be collected by the libraries as part of the root span, using the tag usr.login. + if (shouldSetTag('appsec.events.users.login.success.usr.login')) { + newTags['appsec.events.users.login.success.usr.login'] = login + } + if (userId) { + newTags['_dd.appsec.usr.id'] = userId -/* -These modes only impact automated user ID and login collection, either for business logic events or for authenticated user tracking, and should be disregarded when the collection is performed through the various SDKs. + if (shouldSetTag('usr.id')) { + newTags['usr.id'] = userId + persistent[addresses.USER_ID] = userId + } + } + persistent['server.business_logic.users.login.success'] = null + } else { + isSdkCalled = currentTags['_dd.appsec.events.users.login.failure.sdk'] === 'true' -In the disabled mode, as the name suggests, libraries should not collect user ID or user login. Effectively, this means that libraries shouldn’t send automated business logic events, specifically login and signup events, nor should they automatically track authenticated requests. -*/ + newTags = { + 'appsec.events.users.login.failure.track': 'true', + '_dd.appsec.events.users.login.failure.auto.mode': collectionMode, + '_dd.appsec.usr.login': login + } -function passportTrackEvent (credentials, passportUser, rootSpan) { - if (!collectionMode) return + if (shouldSetTag('appsec.events.users.login.failure.usr.login')) { + newTags['appsec.events.users.login.failure.usr.login'] = login + } - // If a passportUser object is published then the login succeded - if (passportUser) { - const userId = getUserId(passportUser) + if (userId) { + newTags['_dd.appsec.usr.id'] = userId - if (userId === undefined) { - log.warn('No user ID found in authentication instrumentation') - // telemetry counter: 'appsec.instrum.user_auth.missing_user_id' - return + if (shouldSetTag('appsec.events.users.login.failure.usr.id')) { + newTags['appsec.events.users.login.failure.usr.id'] = userId + } } - setUserTags({ id: userId }, rootSpan) + /* TODO: if one day we have this info + if (shouldSetTag('appsec.events.users.login.failure.usr.exists')) { + newTags['appsec.events.users.login.failure.usr.exists'] = 'true' + } + */ - trackEvent('users.login.success', null, 'passportTrackEvent', rootSpan, collectionMode) + persistent['server.business_logic.users.login.failure'] = null + } - // call WAF ephemeral - } else { - const login = getLogin(credentials) + keepTrace(rootSpan, SAMPLING_MECHANISM_APPSEC) + standalone.sample(rootSpan) - if (!login) { - return // idk - } + rootSpan.addTags(newTags) - trackEvent('users.login.failure', { 'usr.id': login, login }, 'passportTrackEvent', rootSpan, collectionMode) - } + return waf.run({ persistent }) } module.exports = { From 8eac0551a788fa023f49dfdcfbabe7f4c349106f Mon Sep 17 00:00:00 2001 From: simon-id Date: Thu, 21 Nov 2024 20:01:56 +0100 Subject: [PATCH 058/126] fix tag override condition --- .../dd-trace/src/appsec/user_tracking/index.js | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/packages/dd-trace/src/appsec/user_tracking/index.js b/packages/dd-trace/src/appsec/user_tracking/index.js index e58a1d3a7ef..b89c9c019e4 100644 --- a/packages/dd-trace/src/appsec/user_tracking/index.js +++ b/packages/dd-trace/src/appsec/user_tracking/index.js @@ -88,9 +88,6 @@ function trackLogin (framework, login, user, success, rootSpan) { return } - const currentTags = rootSpan.context()._tags - let isSdkCalled - login = obfuscateIfNeeded(login) const userId = getUserId(user) @@ -100,14 +97,15 @@ function trackLogin (framework, login, user, success, rootSpan) { [addresses.USER_LOGIN]: login } + const currentTags = rootSpan.context()._tags + const isSdkCalled = currentTags[`_dd.appsec.events.users.login.${success ? 'success' : 'failure'}.sdk`] === 'true' + // used to not overwrite tags set by SDK function shouldSetTag (tag) { - return !isSdkCalled && !currentTags[tag] + return !(isSdkCalled && currentTags[tag]) } if (success) { - isSdkCalled = currentTags['_dd.appsec.events.users.login.success.sdk'] === 'true' - newTags = { 'appsec.events.users.login.success.track': 'true', '_dd.appsec.events.users.login.success.auto.mode': collectionMode, @@ -129,8 +127,6 @@ function trackLogin (framework, login, user, success, rootSpan) { persistent['server.business_logic.users.login.success'] = null } else { - isSdkCalled = currentTags['_dd.appsec.events.users.login.failure.sdk'] === 'true' - newTags = { 'appsec.events.users.login.failure.track': 'true', '_dd.appsec.events.users.login.failure.auto.mode': collectionMode, @@ -150,8 +146,8 @@ function trackLogin (framework, login, user, success, rootSpan) { } /* TODO: if one day we have this info - if (shouldSetTag('appsec.events.users.login.failure.usr.exists')) { - newTags['appsec.events.users.login.failure.usr.exists'] = 'true' + if (exists != null && shouldSetTag('appsec.events.users.login.failure.usr.exists')) { + newTags['appsec.events.users.login.failure.usr.exists'] = exists } */ From a444cfe1551126595fa3c7ecbcac87c2a11b8aba Mon Sep 17 00:00:00 2001 From: simon-id Date: Thu, 21 Nov 2024 20:03:07 +0100 Subject: [PATCH 059/126] add telemetry function --- packages/dd-trace/src/appsec/telemetry.js | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/packages/dd-trace/src/appsec/telemetry.js b/packages/dd-trace/src/appsec/telemetry.js index d96ca77601f..4bf557d4230 100644 --- a/packages/dd-trace/src/appsec/telemetry.js +++ b/packages/dd-trace/src/appsec/telemetry.js @@ -172,6 +172,15 @@ function addRaspRequestMetrics (store, { duration, durationExt }) { store[DD_TELEMETRY_REQUEST_METRICS].raspEvalCount++ } +function incrementMissingUserLogin (framework, eventType) { + if (!enabled) return + + appsecMetrics.count('instrum.user_auth.missing_user_login', { + framework, + event_type: eventType + }).inc() +} + function getRequestMetrics (req) { if (req) { const store = getStore(req) @@ -188,6 +197,7 @@ module.exports = { incrementWafInitMetric, incrementWafUpdatesMetric, incrementWafRequestsMetric, + incrementMissingUserLogin, getRequestMetrics } From 9fa94f6d9b8582e1b08abe5a3a660f69b26e881a Mon Sep 17 00:00:00 2001 From: simon-id Date: Fri, 22 Nov 2024 12:04:43 +0100 Subject: [PATCH 060/126] move user_tracking into a file instead of a subfolder --- .../{user_tracking/index.js => user_tracking.js} | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) rename packages/dd-trace/src/appsec/{user_tracking/index.js => user_tracking.js} (93%) diff --git a/packages/dd-trace/src/appsec/user_tracking/index.js b/packages/dd-trace/src/appsec/user_tracking.js similarity index 93% rename from packages/dd-trace/src/appsec/user_tracking/index.js rename to packages/dd-trace/src/appsec/user_tracking.js index b89c9c019e4..a50f0dbba1a 100644 --- a/packages/dd-trace/src/appsec/user_tracking/index.js +++ b/packages/dd-trace/src/appsec/user_tracking.js @@ -1,13 +1,13 @@ 'use strict' const crypto = require('crypto') -const log = require('../../log') -const telemetry = require('../telemetry') -const addresses = require('../addresses') -const { keepTrace } = require('../../priority_sampler') -const { SAMPLING_MECHANISM_APPSEC } = require('../../constants') -const standalone = require('../standalone') -const waf = require('../waf') +const log = require('../log') +const telemetry = require('./telemetry') +const addresses = require('./addresses') +const { keepTrace } = require('../priority_sampler') +const { SAMPLING_MECHANISM_APPSEC } = require('../constants') +const standalone = require('./standalone') +const waf = require('./waf') // the RFC doesn't include '_id', but it's common in MongoDB const USER_ID_FIELDS = ['id', '_id', 'email', 'username', 'login', 'user'] From 8b22248750fba79d1869d48ed0f7e69d461789c0 Mon Sep 17 00:00:00 2001 From: simon-id Date: Fri, 22 Nov 2024 15:12:59 +0100 Subject: [PATCH 061/126] remove changes --- .../src/appsec/user_tracking/index.js | 31 ------------------- 1 file changed, 31 deletions(-) diff --git a/packages/dd-trace/src/appsec/user_tracking/index.js b/packages/dd-trace/src/appsec/user_tracking/index.js index 5e2e0c9c99c..65fc3deddb2 100644 --- a/packages/dd-trace/src/appsec/user_tracking/index.js +++ b/packages/dd-trace/src/appsec/user_tracking/index.js @@ -131,37 +131,6 @@ function passportTrackEvent (credentials, passportUser, rootSpan) { } } - -function trackUser (user, abortController) { - if (!collectionMode || collectionMode === 'disabled') return - - const userId = getUserId(user) - - // must get user ID with USER_ID_FIELDS - - if (!userId) { - - } - - // ANONYMISE user id if needed - - // don't override tags if already set by "sdk" but still add the _dd.appsec.usr.id - rootSpan.addTags({ - 'usr.id': userId, - '_dd.appsec.usr.id': userId, // always AND only send when automated - '_dd.appsec.user.collection_mode': collectionMode // short or long form ? - }) - - // _dd.appsec.user.collection_mode: collectionMode // sdk/ident/anon - - // If the user monitoring SDK has already resulted in a call to libddwaf before any automated instrumentation or collection method has been executed, no extra call should be made. - const results = waf.run({ - persistent: { - [USER_ID]: userID - } - }) -} - module.exports = { setCollectionMode, trackLogin From 25abd503458491ebeb10088157beb50f2ad6966d Mon Sep 17 00:00:00 2001 From: simon-id Date: Fri, 22 Nov 2024 15:14:27 +0100 Subject: [PATCH 062/126] temp revert changes --- packages/dd-trace/src/appsec/sdk/set_user.js | 47 +------------------- 1 file changed, 1 insertion(+), 46 deletions(-) diff --git a/packages/dd-trace/src/appsec/sdk/set_user.js b/packages/dd-trace/src/appsec/sdk/set_user.js index 571ec7a9433..07a075bfea1 100644 --- a/packages/dd-trace/src/appsec/sdk/set_user.js +++ b/packages/dd-trace/src/appsec/sdk/set_user.js @@ -2,7 +2,6 @@ const { getRootSpan } = require('./utils') const log = require('../../log') -const waf = require('../waf') function setUserTags (user, rootSpan) { for (const k of Object.keys(user)) { @@ -24,54 +23,10 @@ function setUser (tracer, user) { // must get user ID with USER_ID_FIELDS - setTags({ - 'usr.id': userId, - '_dd.appsec.user.collection_mode': 'sdk' - }) - - /* - When the user monitoring SDK is available and in use, the following applies: - The usr.id must be set to the value provided through the user monitoring SDK. - The span tag _dd.appsec.user.collection_mode must be set to sdk. - This effectively means that the automated user ID collection mechanism must not overwrite the aforementioned span tags, while the user monitoring SDK must overwrite them if present. - */ - - - - /* - When a user provides their own session ID through the use of the SDK using the session_id key: - If libddwaf hasn’t already been called with the usr.session_id address, it should be called with the provided session_id and further calls through the automated collection method should be inhibited. - If libddwaf has already been called with the usr.session_id address, it should be called again. - */ - if (user.session_id && typeof user.session_id === 'string') { - persistent['usr.session_id'] = user.session_id - } - + // _dd.appsec.user.collection_mode: collectionMode // sdk/ident/anon setUserTags(user, rootSpan) - // If the automated instrumentation or collection method has resulted in a call to libddwaf before the user monitoring SDK has been executed, a second call must be performed with the user ID obtained through the SDK. - // will the second call trigger tho ? make some edge case tests - const results = waf.run({ - persistent: { - [USER_ID]: userId - } - }) - - - - const persistent = {} - - if (user.id) { - persistent[addresses.USER_ID] = '' + user.id - } - - if (user.login) { - persistent[addresses.USER_LOGIN] = '' + user.login - } - - waf.run({ persistent }) - /* User IDs generated through the SDK must now be provided to libddwaf as a persistent addresses. If the user monitoring SDK has already resulted in a call to libddwaf before any automated instrumentation or collection method has been executed, no extra call should be made. From 52a0601ed1123f32f0c54e5e2ac1883521ae71fd Mon Sep 17 00:00:00 2001 From: simon-id Date: Fri, 22 Nov 2024 15:15:13 +0100 Subject: [PATCH 063/126] temp revert changes --- packages/dd-trace/src/appsec/index.js | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/packages/dd-trace/src/appsec/index.js b/packages/dd-trace/src/appsec/index.js index 5ac4a47b31e..bd0e50f645a 100644 --- a/packages/dd-trace/src/appsec/index.js +++ b/packages/dd-trace/src/appsec/index.js @@ -66,7 +66,6 @@ function enable (_config) { incomingHttpRequestStart.subscribe(incomingHttpStartTranslator) incomingHttpRequestEnd.subscribe(incomingHttpEndTranslator) passportVerify.subscribe(onPassportVerify) // possible optimization: only subscribe if collection mode is enabled - passportUser.subscribe(onPassportDeserializeUser) queryParser.subscribe(onRequestQueryParsed) nextBodyParsed.subscribe(onRequestBodyParsed) nextQueryParsed.subscribe(onRequestQueryParsed) @@ -181,18 +180,6 @@ function incomingHttpEndTranslator ({ req, res }) { Reporter.finishRequest(req, res) } -function onPassportDeserializeUser ({ req, user, sessionId, abordController }) { - UserTracking.trackUser(user, abortController) - - if (sessionId && typeof sessionId === 'string') { - const results = waf.run({ - persistent: { - 'usr.session_id': sessionId - } - }) - } -} - function onPassportVerify ({ login, user, success, abortController }) { const store = storage.getStore() const rootSpan = store?.req && web.root(store.req) From 5a10fc0cec6fdb49a7332eca5be846b073404b48 Mon Sep 17 00:00:00 2001 From: simon-id Date: Fri, 22 Nov 2024 15:22:06 +0100 Subject: [PATCH 064/126] fix conflict --- packages/dd-trace/src/appsec/index.js | 13 +++++ packages/dd-trace/src/appsec/sdk/set_user.js | 57 +++++++++++++++++++ .../dd-trace/src/appsec/sdk/track_event.js | 30 ++++++++++ 3 files changed, 100 insertions(+) diff --git a/packages/dd-trace/src/appsec/index.js b/packages/dd-trace/src/appsec/index.js index 3c267b5edb5..1730e315bf5 100644 --- a/packages/dd-trace/src/appsec/index.js +++ b/packages/dd-trace/src/appsec/index.js @@ -66,6 +66,7 @@ function enable (_config) { incomingHttpRequestStart.subscribe(incomingHttpStartTranslator) incomingHttpRequestEnd.subscribe(incomingHttpEndTranslator) passportVerify.subscribe(onPassportVerify) // possible optimization: only subscribe if collection mode is enabled + passportUser.subscribe(onPassportDeserializeUser) queryParser.subscribe(onRequestQueryParsed) nextBodyParsed.subscribe(onRequestBodyParsed) nextQueryParsed.subscribe(onRequestQueryParsed) @@ -179,6 +180,18 @@ function incomingHttpEndTranslator ({ req, res }) { Reporter.finishRequest(req, res) } +function onPassportDeserializeUser ({ req, user, sessionId, abordController }) { + UserTracking.trackUser(user, abortController) + + if (sessionId && typeof sessionId === 'string') { + const results = waf.run({ + persistent: { + 'usr.session_id': sessionId + } + }) + } +} + function onPassportVerify ({ framework, login, user, success, abortController }) { const store = storage.getStore() const rootSpan = store?.req && web.root(store.req) diff --git a/packages/dd-trace/src/appsec/sdk/set_user.js b/packages/dd-trace/src/appsec/sdk/set_user.js index 6efe44ebd41..810caabd5b4 100644 --- a/packages/dd-trace/src/appsec/sdk/set_user.js +++ b/packages/dd-trace/src/appsec/sdk/set_user.js @@ -2,6 +2,7 @@ const { getRootSpan } = require('./utils') const log = require('../../log') +const waf = require('../waf') function setUserTags (user, rootSpan) { for (const k of Object.keys(user)) { @@ -21,7 +22,63 @@ function setUser (tracer, user) { return } + // must get user ID with USER_ID_FIELDS + + setTags({ + 'usr.id': userId, + '_dd.appsec.user.collection_mode': 'sdk' + }) + + /* + When the user monitoring SDK is available and in use, the following applies: + The usr.id must be set to the value provided through the user monitoring SDK. + The span tag _dd.appsec.user.collection_mode must be set to sdk. + This effectively means that the automated user ID collection mechanism must not overwrite the aforementioned span tags, while the user monitoring SDK must overwrite them if present. + */ + + + + /* + When a user provides their own session ID through the use of the SDK using the session_id key: + If libddwaf hasn’t already been called with the usr.session_id address, it should be called with the provided session_id and further calls through the automated collection method should be inhibited. + If libddwaf has already been called with the usr.session_id address, it should be called again. + */ + if (user.session_id && typeof user.session_id === 'string') { + persistent['usr.session_id'] = user.session_id + } + + + setUserTags(user, rootSpan) + + // If the automated instrumentation or collection method has resulted in a call to libddwaf before the user monitoring SDK has been executed, a second call must be performed with the user ID obtained through the SDK. + // will the second call trigger tho ? make some edge case tests + const results = waf.run({ + persistent: { + [USER_ID]: userId + } + }) + + + + const persistent = {} + + if (user.id) { + persistent[addresses.USER_ID] = '' + user.id + } + + if (user.login) { + persistent[addresses.USER_LOGIN] = '' + user.login + } + + waf.run({ persistent }) + + /* + User IDs generated through the SDK must now be provided to libddwaf as a persistent addresses. + If the user monitoring SDK has already resulted in a call to libddwaf before any automated instrumentation or collection method has been executed, no extra call should be made. + If the automated instrumentation or collection method has resulted in a call to libddwaf before the user monitoring SDK has been executed, a second call must be performed with the user ID obtained through the SDK. + */ + // will the second call trigger tho ? make some edge case tests } module.exports = { diff --git a/packages/dd-trace/src/appsec/sdk/track_event.js b/packages/dd-trace/src/appsec/sdk/track_event.js index d2c1c392790..19597c31650 100644 --- a/packages/dd-trace/src/appsec/sdk/track_event.js +++ b/packages/dd-trace/src/appsec/sdk/track_event.js @@ -95,6 +95,36 @@ function runWaf (eventName, user) { waf.run({ persistent }) } +function trackUser (user, abortController) { + if (!collectionMode || collectionMode === 'disabled') return + + const userId = getUserId(user) + + // must get user ID with USER_ID_FIELDS + + if (!userId) { + + } + + // ANONYMISE user id if needed + + // don't override tags if already set by "sdk" but still add the _dd.appsec.usr.id + rootSpan.addTags({ + 'usr.id': userId, + '_dd.appsec.usr.id': userId, // always AND only send when automated + '_dd.appsec.user.collection_mode': collectionMode // short or long form ? + }) + + // _dd.appsec.user.collection_mode: collectionMode // sdk/ident/anon + + // If the user monitoring SDK has already resulted in a call to libddwaf before any automated instrumentation or collection method has been executed, no extra call should be made. + const results = waf.run({ + persistent: { + [USER_ID]: userID + } + }) +} + module.exports = { trackUserLoginSuccessEvent, trackUserLoginFailureEvent, From 9869c4b927db3a400c9546fbaf97ed994bb646fb Mon Sep 17 00:00:00 2001 From: simon-id Date: Fri, 22 Nov 2024 15:24:07 +0100 Subject: [PATCH 065/126] wrong file --- .../dd-trace/src/appsec/sdk/track_event.js | 30 ------------------- packages/dd-trace/src/appsec/user_tracking.js | 30 +++++++++++++++++++ 2 files changed, 30 insertions(+), 30 deletions(-) diff --git a/packages/dd-trace/src/appsec/sdk/track_event.js b/packages/dd-trace/src/appsec/sdk/track_event.js index 19597c31650..d2c1c392790 100644 --- a/packages/dd-trace/src/appsec/sdk/track_event.js +++ b/packages/dd-trace/src/appsec/sdk/track_event.js @@ -95,36 +95,6 @@ function runWaf (eventName, user) { waf.run({ persistent }) } -function trackUser (user, abortController) { - if (!collectionMode || collectionMode === 'disabled') return - - const userId = getUserId(user) - - // must get user ID with USER_ID_FIELDS - - if (!userId) { - - } - - // ANONYMISE user id if needed - - // don't override tags if already set by "sdk" but still add the _dd.appsec.usr.id - rootSpan.addTags({ - 'usr.id': userId, - '_dd.appsec.usr.id': userId, // always AND only send when automated - '_dd.appsec.user.collection_mode': collectionMode // short or long form ? - }) - - // _dd.appsec.user.collection_mode: collectionMode // sdk/ident/anon - - // If the user monitoring SDK has already resulted in a call to libddwaf before any automated instrumentation or collection method has been executed, no extra call should be made. - const results = waf.run({ - persistent: { - [USER_ID]: userID - } - }) -} - module.exports = { trackUserLoginSuccessEvent, trackUserLoginFailureEvent, diff --git a/packages/dd-trace/src/appsec/user_tracking.js b/packages/dd-trace/src/appsec/user_tracking.js index a50f0dbba1a..1d173730381 100644 --- a/packages/dd-trace/src/appsec/user_tracking.js +++ b/packages/dd-trace/src/appsec/user_tracking.js @@ -162,6 +162,36 @@ function trackLogin (framework, login, user, success, rootSpan) { return waf.run({ persistent }) } +function trackUser (user, abortController) { + if (!collectionMode || collectionMode === 'disabled') return + + const userId = getUserId(user) + + // must get user ID with USER_ID_FIELDS + + if (!userId) { + + } + + // ANONYMISE user id if needed + + // don't override tags if already set by "sdk" but still add the _dd.appsec.usr.id + rootSpan.addTags({ + 'usr.id': userId, + '_dd.appsec.usr.id': userId, // always AND only send when automated + '_dd.appsec.user.collection_mode': collectionMode // short or long form ? + }) + + // _dd.appsec.user.collection_mode: collectionMode // sdk/ident/anon + + // If the user monitoring SDK has already resulted in a call to libddwaf before any automated instrumentation or collection method has been executed, no extra call should be made. + const results = waf.run({ + persistent: { + [USER_ID]: userID + } + }) +} + module.exports = { setCollectionMode, trackLogin From 76e2e655e576af5ee36ba264047c78e3ee16530d Mon Sep 17 00:00:00 2001 From: simon-id Date: Fri, 22 Nov 2024 16:00:17 +0100 Subject: [PATCH 066/126] add some stuff --- packages/datadog-instrumentations/src/http/server.js | 1 + packages/dd-trace/src/appsec/blocking.js | 2 +- packages/dd-trace/src/appsec/channels.js | 1 + packages/dd-trace/src/appsec/index.js | 5 +++-- 4 files changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/datadog-instrumentations/src/http/server.js b/packages/datadog-instrumentations/src/http/server.js index 0624c886787..f3245c24a8d 100644 --- a/packages/datadog-instrumentations/src/http/server.js +++ b/packages/datadog-instrumentations/src/http/server.js @@ -24,6 +24,7 @@ addHook({ name: httpNames }, http => { shimmer.wrap(http.Server.prototype, 'emit', wrapEmit) shimmer.wrap(http.ServerResponse.prototype, 'writeHead', wrapWriteHead) shimmer.wrap(http.ServerResponse.prototype, 'write', wrapWrite) + http.ServerResponse.prototype._originalEnd = http.ServerResponse.prototype.end shimmer.wrap(http.ServerResponse.prototype, 'end', wrapEnd) shimmer.wrap(http.ServerResponse.prototype, 'setHeader', wrapSetHeader) shimmer.wrap(http.ServerResponse.prototype, 'removeHeader', wrapAppendOrRemoveHeader) diff --git a/packages/dd-trace/src/appsec/blocking.js b/packages/dd-trace/src/appsec/blocking.js index d831b310eb3..906dfbc0128 100644 --- a/packages/dd-trace/src/appsec/blocking.js +++ b/packages/dd-trace/src/appsec/blocking.js @@ -115,7 +115,7 @@ function block (req, res, rootSpan, abortController, actionParameters = defaultB res.removeHeader(headerName) } - res.writeHead(statusCode, headers).end(body) + res.writeHead(statusCode, headers)._originalEnd(body) responseBlockedSet.add(res) diff --git a/packages/dd-trace/src/appsec/channels.js b/packages/dd-trace/src/appsec/channels.js index 8e7f27211c6..dd59e80014f 100644 --- a/packages/dd-trace/src/appsec/channels.js +++ b/packages/dd-trace/src/appsec/channels.js @@ -13,6 +13,7 @@ module.exports = { apolloServerCoreChannel: dc.tracingChannel('datadog:apollo-server-core:request'), incomingHttpRequestStart: dc.channel('dd-trace:incomingHttpRequestStart'), incomingHttpRequestEnd: dc.channel('dd-trace:incomingHttpRequestEnd'), + passportUser: dc.channel('datadog:passport:deserializeUser:finish'), passportVerify: dc.channel('datadog:passport:verify:finish'), queryParser: dc.channel('datadog:query:read:finish'), setCookieChannel: dc.channel('datadog:iast:set-cookie'), diff --git a/packages/dd-trace/src/appsec/index.js b/packages/dd-trace/src/appsec/index.js index 1730e315bf5..9b9cbbbb62f 100644 --- a/packages/dd-trace/src/appsec/index.js +++ b/packages/dd-trace/src/appsec/index.js @@ -9,6 +9,7 @@ const { multerParser, incomingHttpRequestStart, incomingHttpRequestEnd, + passportUser, passportVerify, queryParser, nextBodyParsed, @@ -65,8 +66,8 @@ function enable (_config) { cookieParser.subscribe(onRequestCookieParser) incomingHttpRequestStart.subscribe(incomingHttpStartTranslator) incomingHttpRequestEnd.subscribe(incomingHttpEndTranslator) - passportVerify.subscribe(onPassportVerify) // possible optimization: only subscribe if collection mode is enabled passportUser.subscribe(onPassportDeserializeUser) + passportVerify.subscribe(onPassportVerify) // possible optimization: only subscribe if collection mode is enabled queryParser.subscribe(onRequestQueryParsed) nextBodyParsed.subscribe(onRequestBodyParsed) nextQueryParsed.subscribe(onRequestQueryParsed) @@ -181,7 +182,7 @@ function incomingHttpEndTranslator ({ req, res }) { } function onPassportDeserializeUser ({ req, user, sessionId, abordController }) { - UserTracking.trackUser(user, abortController) + UserTracking.trackUser(user) if (sessionId && typeof sessionId === 'string') { const results = waf.run({ From 80ae6930bd2895e27203b61390ffc28e0e5269d4 Mon Sep 17 00:00:00 2001 From: simon-id Date: Mon, 25 Nov 2024 15:22:41 +0100 Subject: [PATCH 067/126] push some stuff --- packages/datadog-instrumentations/src/passport.js | 8 +++++--- packages/dd-trace/src/appsec/user_tracking.js | 3 ++- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/packages/datadog-instrumentations/src/passport.js b/packages/datadog-instrumentations/src/passport.js index 6429bb67b0a..b48b5e32432 100644 --- a/packages/datadog-instrumentations/src/passport.js +++ b/packages/datadog-instrumentations/src/passport.js @@ -1,7 +1,7 @@ 'use strict' const shimmer = require('../../datadog-shimmer') -const { addHook } = require('./helpers/instrument') +const { channel, addHook } = require('./helpers/instrument') /* TODO: test with: passport-jwt JWTs @@ -17,7 +17,9 @@ passport-http-bearer koa-passport */ -function wrapDone (done) { +const onPassportDeserializeUserChannel = channel('datadog:passport:deserializeUser:finish') + +function wrapDone (req, done) { // eslint-disable-next-line n/handle-callback-err return function wrappedDone (err, user) { if (user) { @@ -25,7 +27,7 @@ function wrapDone (done) { // express-session middleware sets req.sessionID, it's required to use passport sessions anyway so might as well use it ? // what if session IDs are using rolling sessions or always changing or something idk ? - channel.publish({ req, user, sessionId: req.sessionID, abortController }) + onPassportDeserializeUserChannel.publish({ req, user, sessionId: req.sessionID, abortController }) if (abortController.signal.aborted) return } diff --git a/packages/dd-trace/src/appsec/user_tracking.js b/packages/dd-trace/src/appsec/user_tracking.js index 1d173730381..993c552c6b1 100644 --- a/packages/dd-trace/src/appsec/user_tracking.js +++ b/packages/dd-trace/src/appsec/user_tracking.js @@ -194,5 +194,6 @@ function trackUser (user, abortController) { module.exports = { setCollectionMode, - trackLogin + trackLogin, + trackUser } From 7e3374da5c9423083820113c9f9eee167a0a8e04 Mon Sep 17 00:00:00 2001 From: simon-id Date: Mon, 25 Nov 2024 15:25:36 +0100 Subject: [PATCH 068/126] Use addresses enum instead of hardcoded business logic events Co-authored-by: Carles Capell <107924659+CarlesDD@users.noreply.github.com> --- packages/dd-trace/src/appsec/user_tracking.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/dd-trace/src/appsec/user_tracking.js b/packages/dd-trace/src/appsec/user_tracking.js index a50f0dbba1a..cc542646a76 100644 --- a/packages/dd-trace/src/appsec/user_tracking.js +++ b/packages/dd-trace/src/appsec/user_tracking.js @@ -125,7 +125,7 @@ function trackLogin (framework, login, user, success, rootSpan) { } } - persistent['server.business_logic.users.login.success'] = null + persistent[addresses.LOGIN_SUCCESS] = null } else { newTags = { 'appsec.events.users.login.failure.track': 'true', @@ -151,7 +151,7 @@ function trackLogin (framework, login, user, success, rootSpan) { } */ - persistent['server.business_logic.users.login.failure'] = null + persistent[addresses.LOGIN_FAILURE] = null } keepTrace(rootSpan, SAMPLING_MECHANISM_APPSEC) From c099a2ae23af7e32a5c2da0c1a8415e451c6f03c Mon Sep 17 00:00:00 2001 From: simon-id Date: Mon, 25 Nov 2024 15:26:39 +0100 Subject: [PATCH 069/126] Update default value of collection mode in typings --- index.d.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/index.d.ts b/index.d.ts index da4c8dc109c..a41b4aee410 100644 --- a/index.d.ts +++ b/index.d.ts @@ -667,7 +667,7 @@ declare namespace tracer { * * 'disabled': will not collect user IDs and logins * * Unknown values will be considered as 'disabled' - * @default 'ident' + * @default 'identification' */ mode?: 'anonymous' | 'anon' | 'safe' | From 96cb72249610b80fe8dd41e422f886c56df8366b Mon Sep 17 00:00:00 2001 From: simon-id Date: Mon, 25 Nov 2024 16:42:35 +0100 Subject: [PATCH 070/126] Add string check in getUserId() Co-authored-by: Ugaitz Urien --- packages/dd-trace/src/appsec/user_tracking.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/dd-trace/src/appsec/user_tracking.js b/packages/dd-trace/src/appsec/user_tracking.js index cc542646a76..e55db31d389 100644 --- a/packages/dd-trace/src/appsec/user_tracking.js +++ b/packages/dd-trace/src/appsec/user_tracking.js @@ -60,7 +60,7 @@ function getUserId (user) { if (id && typeof id.toString === 'function') { id = id.toString() - if (id.startsWith('[object ')) { + if (typeof id !== 'string' || id.startsWith('[object ')) { // probably not a usable ID ? continue } From 85ad6f2d2fa6588e302788f40e58f07891e3b457 Mon Sep 17 00:00:00 2001 From: simon-id Date: Tue, 26 Nov 2024 13:29:06 +0100 Subject: [PATCH 071/126] fix existing RC tests --- .../dd-trace/test/appsec/remote_config/index.spec.js | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/packages/dd-trace/test/appsec/remote_config/index.spec.js b/packages/dd-trace/test/appsec/remote_config/index.spec.js index 67447cf7a69..ecead9f56bd 100644 --- a/packages/dd-trace/test/appsec/remote_config/index.spec.js +++ b/packages/dd-trace/test/appsec/remote_config/index.spec.js @@ -46,7 +46,7 @@ describe('Remote Config index', () => { describe('enable', () => { it('should listen to remote config when appsec is not explicitly configured', () => { - config.appsec = { enabled: undefined } + config.appsec.enabled = undefined remoteConfig.enable(config) @@ -57,18 +57,18 @@ describe('Remote Config index', () => { }) it('should listen to remote config when appsec is explicitly configured as enabled=true', () => { - config.appsec = { enabled: true } + config.appsec.enabled = true remoteConfig.enable(config) expect(RemoteConfigManager).to.have.been.calledOnceWithExactly(config) - expect(rc.updateCapabilities).to.not.have.been.calledWith('ASM_ACTIVATION') + expect(rc.updateCapabilities).to.not.have.been.calledWith(RemoteConfigCapabilities.ASM_ACTIVATION) expect(rc.setProductHandler).to.have.been.calledOnceWith('ASM_FEATURES') expect(rc.setProductHandler.firstCall.args[1]).to.be.a('function') }) it('should not listen to remote config when appsec is explicitly configured as enabled=false', () => { - config.appsec = { enabled: false } + config.appsec.enabled = false remoteConfig.enable(config) @@ -81,8 +81,6 @@ describe('Remote Config index', () => { let listener beforeEach(() => { - config.appsec = { enabled: undefined } - remoteConfig.enable(config, appsec) listener = rc.setProductHandler.firstCall.args[1] From 70dcbab64663db4d386785da3c8f067ebed2f290 Mon Sep 17 00:00:00 2001 From: simon-id Date: Tue, 26 Nov 2024 13:31:05 +0100 Subject: [PATCH 072/126] add tests for RC collection mode --- .../test/appsec/remote_config/index.spec.js | 68 ++++++++++++++++++- 1 file changed, 67 insertions(+), 1 deletion(-) diff --git a/packages/dd-trace/test/appsec/remote_config/index.spec.js b/packages/dd-trace/test/appsec/remote_config/index.spec.js index ecead9f56bd..73078bb6715 100644 --- a/packages/dd-trace/test/appsec/remote_config/index.spec.js +++ b/packages/dd-trace/test/appsec/remote_config/index.spec.js @@ -7,6 +7,7 @@ let config let rc let RemoteConfigManager let RuleManager +let UserTracking let appsec let remoteConfig @@ -14,7 +15,10 @@ describe('Remote Config index', () => { beforeEach(() => { config = { appsec: { - enabled: undefined + enabled: undefined, + eventTracking: { + mode: 'identification' + } } } @@ -32,6 +36,10 @@ describe('Remote Config index', () => { updateWafFromRC: sinon.stub() } + UserTracking = { + setCollectionMode: sinon.stub() + } + appsec = { enable: sinon.spy(), disable: sinon.spy() @@ -40,6 +48,7 @@ describe('Remote Config index', () => { remoteConfig = proxyquire('../src/appsec/remote_config', { './manager': RemoteConfigManager, '../rule_manager': RuleManager, + '../user_tracking': UserTracking, '..': appsec }) }) @@ -52,6 +61,8 @@ describe('Remote Config index', () => { expect(RemoteConfigManager).to.have.been.calledOnceWithExactly(config) expect(rc.updateCapabilities).to.have.been.calledWithExactly(RemoteConfigCapabilities.ASM_ACTIVATION, true) + expect(rc.updateCapabilities) + .to.have.been.calledWithExactly(RemoteConfigCapabilities.ASM_AUTO_USER_INSTRUM_MODE, true) expect(rc.setProductHandler).to.have.been.calledWith('ASM_FEATURES') expect(rc.setProductHandler.firstCall.args[1]).to.be.a('function') }) @@ -63,6 +74,8 @@ describe('Remote Config index', () => { expect(RemoteConfigManager).to.have.been.calledOnceWithExactly(config) expect(rc.updateCapabilities).to.not.have.been.calledWith(RemoteConfigCapabilities.ASM_ACTIVATION) + expect(rc.updateCapabilities) + .to.have.been.calledWithExactly(RemoteConfigCapabilities.ASM_AUTO_USER_INSTRUM_MODE, true) expect(rc.setProductHandler).to.have.been.calledOnceWith('ASM_FEATURES') expect(rc.setProductHandler.firstCall.args[1]).to.be.a('function') }) @@ -74,6 +87,8 @@ describe('Remote Config index', () => { expect(RemoteConfigManager).to.have.been.calledOnceWithExactly(config) expect(rc.updateCapabilities).to.not.have.been.calledWith(RemoteConfigCapabilities.ASM_ACTIVATION, true) + expect(rc.updateCapabilities) + .to.not.have.been.calledWith(RemoteConfigCapabilities.ASM_AUTO_USER_INSTRUM_MODE, true) expect(rc.setProductHandler).to.not.have.been.called }) @@ -110,6 +125,57 @@ describe('Remote Config index', () => { expect(appsec.enable).to.not.have.been.called expect(appsec.disable).to.not.have.been.called }) + + describe('auto_user_instrum', () => { + const rcConfig = { auto_user_instrum: { mode: 'anonymous' } } + const configId = 'collectionModeId' + + afterEach(() => { + listener('unnaply', rcConfig, configId) + }) + + it('should not update collection mode when not a string', () => { + listener('apply', { auto_user_instrum: { mode: 123 } }, configId) + + expect(UserTracking.setCollectionMode).to.not.have.been.called + }) + + it('should throw when called two times with different config ids', () => { + listener('apply', rcConfig, configId) + + expect(() => listener('apply', rcConfig, 'anotherId')).to.throw() + }) + + it('should update collection mode when called with apply', () => { + listener('apply', rcConfig, configId) + + expect(UserTracking.setCollectionMode).to.have.been.calledOnceWithExactly(rcConfig.auto_user_instrum.mode) + }) + + it('should update collection mode when called with modify', () => { + listener('modify', rcConfig, configId) + + expect(UserTracking.setCollectionMode).to.have.been.calledOnceWithExactly(rcConfig.auto_user_instrum.mode) + }) + + it('should revert collection mode when called with unnaply', () => { + listener('apply', rcConfig, configId) + UserTracking.setCollectionMode.resetHistory() + + listener('unnaply', rcConfig, configId) + + expect(UserTracking.setCollectionMode).to.have.been.calledOnceWithExactly(config.appsec.eventTracking.mode) + }) + + it('should not revert collection mode when called with unnaply and unknown id', () => { + listener('apply', rcConfig, configId) + UserTracking.setCollectionMode.resetHistory() + + listener('unnaply', rcConfig, 'unknownId') + + expect(UserTracking.setCollectionMode).to.not.have.been.called + }) + }) }) }) From ff722719872330f74fdfcdf968cfb36818f29d0c Mon Sep 17 00:00:00 2001 From: simon-id Date: Tue, 26 Nov 2024 18:28:15 +0100 Subject: [PATCH 073/126] fix env var ordering --- packages/dd-trace/src/config.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/dd-trace/src/config.js b/packages/dd-trace/src/config.js index 2e82133c88f..6cf342452f5 100644 --- a/packages/dd-trace/src/config.js +++ b/packages/dd-trace/src/config.js @@ -573,9 +573,9 @@ class Config { DD_AGENT_HOST, DD_API_SECURITY_ENABLED, DD_API_SECURITY_SAMPLE_DELAY, + DD_APPSEC_AUTO_USER_INSTRUMENTATION_MODE, DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING, DD_APPSEC_ENABLED, - DD_APPSEC_AUTO_USER_INSTRUMENTATION_MODE, DD_APPSEC_GRAPHQL_BLOCKED_TEMPLATE_JSON, DD_APPSEC_HTTP_BLOCKED_TEMPLATE_HTML, DD_APPSEC_HTTP_BLOCKED_TEMPLATE_JSON, From 7338bd677a77b1051b63601f277ffeca768e8305 Mon Sep 17 00:00:00 2001 From: simon-id Date: Fri, 29 Nov 2024 15:01:37 +0100 Subject: [PATCH 074/126] cleanup track_event.spec.js --- packages/dd-trace/test/appsec/sdk/track_event.spec.js | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) 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 97f1ac07bd7..e55ba51986c 100644 --- a/packages/dd-trace/test/appsec/sdk/track_event.spec.js +++ b/packages/dd-trace/test/appsec/sdk/track_event.spec.js @@ -12,13 +12,13 @@ describe('track_event', () => { describe('Internal API', () => { const tracer = {} let log + let prioritySampler let rootSpan let getRootSpan let setUserTags - let trackUserLoginSuccessEvent, trackUserLoginFailureEvent, trackCustomEvent, trackEvent let sample let waf - let prioritySampler + let trackUserLoginSuccessEvent, trackUserLoginFailureEvent, trackCustomEvent beforeEach(() => { log = { @@ -65,10 +65,6 @@ describe('track_event', () => { trackEvent = trackEvents.trackEvent }) - afterEach(() => { - sinon.restore() - }) - describe('trackUserLoginSuccessEvent', () => { it('should log warning when passed invalid user', () => { trackUserLoginSuccessEvent(tracer, null, { key: 'value' }) From b71ed06ae0f3bd92952544fdbdeeb01cba194f1c Mon Sep 17 00:00:00 2001 From: simon-id Date: Fri, 29 Nov 2024 15:32:25 +0100 Subject: [PATCH 075/126] do not export or test trackEvent() --- .../dd-trace/src/appsec/sdk/track_event.js | 3 +- .../test/appsec/sdk/track_event.spec.js | 38 ------------------- 2 files changed, 1 insertion(+), 40 deletions(-) diff --git a/packages/dd-trace/src/appsec/sdk/track_event.js b/packages/dd-trace/src/appsec/sdk/track_event.js index d2c1c392790..6d4d37162ee 100644 --- a/packages/dd-trace/src/appsec/sdk/track_event.js +++ b/packages/dd-trace/src/appsec/sdk/track_event.js @@ -98,6 +98,5 @@ function runWaf (eventName, user) { module.exports = { trackUserLoginSuccessEvent, trackUserLoginFailureEvent, - trackCustomEvent, - trackEvent + trackCustomEvent } 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 e55ba51986c..9a8a9791c08 100644 --- a/packages/dd-trace/test/appsec/sdk/track_event.spec.js +++ b/packages/dd-trace/test/appsec/sdk/track_event.spec.js @@ -62,7 +62,6 @@ describe('track_event', () => { trackUserLoginSuccessEvent = trackEvents.trackUserLoginSuccessEvent trackUserLoginFailureEvent = trackEvents.trackUserLoginFailureEvent trackCustomEvent = trackEvents.trackCustomEvent - trackEvent = trackEvents.trackEvent }) describe('trackUserLoginSuccessEvent', () => { @@ -278,43 +277,6 @@ describe('track_event', () => { .to.have.been.calledOnceWithExactly(rootSpan, USER_KEEP, SAMPLING_MECHANISM_APPSEC) }) }) - - 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', - '_dd.appsec.events.event.auto.mode': 'safe', - 'appsec.events.event.metaKey1': 'metaValue1', - 'appsec.events.event.metakey2': 'metaValue2' - }) - expect(prioritySampler.setPriority) - .to.have.been.calledOnceWithExactly(rootSpan, USER_KEEP, SAMPLING_MECHANISM_APPSEC) - }) - - 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', - '_dd.appsec.events.event.auto.mode': 'extended', - 'appsec.events.event.metaKey1': 'metaValue1', - 'appsec.events.event.metakey2': 'metaValue2' - }) - expect(prioritySampler.setPriority) - .to.have.been.calledOnceWithExactly(rootSpan, USER_KEEP, SAMPLING_MECHANISM_APPSEC) - }) - - it('should call standalone sample', () => { - trackEvent('event', undefined, 'trackEvent', rootSpan, undefined) - - expect(rootSpan.addTags).to.have.been.calledOnceWithExactly({ - 'appsec.events.event.track': 'true' - }) - expect(prioritySampler.setPriority) - .to.have.been.calledOnceWithExactly(rootSpan, USER_KEEP, SAMPLING_MECHANISM_APPSEC) - expect(sample).to.have.been.calledOnceWithExactly(rootSpan) - }) - }) }) describe('Integration with the tracer', () => { From 166bc2387c8507f6c471a9cddd33ab30b76e3305 Mon Sep 17 00:00:00 2001 From: simon-id Date: Fri, 29 Nov 2024 15:45:33 +0100 Subject: [PATCH 076/126] update test, cleanup, and add missing coverage --- .../test/appsec/sdk/track_event.spec.js | 93 ++++++++++++++----- 1 file changed, 72 insertions(+), 21 deletions(-) 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 9a8a9791c08..611eb97436b 100644 --- a/packages/dd-trace/test/appsec/sdk/track_event.spec.js +++ b/packages/dd-trace/test/appsec/sdk/track_event.spec.js @@ -4,7 +4,7 @@ const proxyquire = require('proxyquire') const agent = require('../../plugins/agent') const axios = require('axios') const tracer = require('../../../../../index') -const { LOGIN_SUCCESS, LOGIN_FAILURE } = require('../../../src/appsec/addresses') +const { LOGIN_SUCCESS, LOGIN_FAILURE, USER_ID, USER_LOGIN } = require('../../../src/appsec/addresses') const { SAMPLING_MECHANISM_APPSEC } = require('../../../src/constants') const { USER_KEEP } = require('../../../../../ext/priority') @@ -109,6 +109,14 @@ describe('track_event', () => { }) expect(prioritySampler.setPriority) .to.have.been.calledOnceWithExactly(rootSpan, USER_KEEP, SAMPLING_MECHANISM_APPSEC) + expect(sample).to.have.been.calledOnceWithExactly(rootSpan) + expect(waf.run).to.have.been.calledOnceWithExactly({ + persistent: { + [LOGIN_SUCCESS]: null, + [USER_ID]: 'user_id', + [USER_LOGIN]: 'user_id' + } + }) }) it('should call setUser and addTags without metadata', () => { @@ -124,23 +132,44 @@ describe('track_event', () => { }) expect(prioritySampler.setPriority) .to.have.been.calledOnceWithExactly(rootSpan, USER_KEEP, SAMPLING_MECHANISM_APPSEC) + expect(sample).to.have.been.calledOnceWithExactly(rootSpan) + expect(waf.run).to.have.been.calledOnceWithExactly({ + persistent: { + [LOGIN_SUCCESS]: null, + [USER_ID]: 'user_id', + [USER_LOGIN]: 'user_id' + } + }) }) - it('should call waf run with login success address', () => { - const user = { id: 'user_id' } + it('should call waf with user login', () => { + const user = { id: 'user_id', login: 'user_login' } trackUserLoginSuccessEvent(tracer, user) - sinon.assert.calledOnceWithExactly( - waf.run, - { persistent: { [LOGIN_SUCCESS]: null } } - ) + + 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', + '_dd.appsec.events.users.login.success.sdk': 'true' + }) + expect(prioritySampler.setPriority) + .to.have.been.calledOnceWithExactly(rootSpan, USER_KEEP, SAMPLING_MECHANISM_APPSEC) + expect(sample).to.have.been.calledOnceWithExactly(rootSpan) + expect(waf.run).to.have.been.calledOnceWithExactly({ + persistent: { + [LOGIN_SUCCESS]: null, + [USER_ID]: 'user_id', + [USER_LOGIN]: 'user_login' + } + }) }) }) describe('trackUserLoginFailureEvent', () => { it('should log warning when passed invalid userId', () => { - trackUserLoginFailureEvent(tracer, null, false) - trackUserLoginFailureEvent(tracer, [], false) + trackUserLoginFailureEvent(tracer, null, false, { key: 'value' }) + trackUserLoginFailureEvent(tracer, [], false, { key: 'value' }) expect(log.warn).to.have.been.calledTwice expect(log.warn.firstCall) @@ -154,7 +183,7 @@ describe('track_event', () => { it('should log warning when root span is not available', () => { rootSpan = undefined - trackUserLoginFailureEvent(tracer, 'user_id', false) + trackUserLoginFailureEvent(tracer, 'user_id', false, { key: 'value' }) expect(log.warn) .to.have.been.calledOnceWithExactly('[ASM] Root span not available in %s', 'trackUserLoginFailureEvent') @@ -163,7 +192,9 @@ describe('track_event', () => { it('should call addTags with metadata', () => { trackUserLoginFailureEvent(tracer, 'user_id', true, { - metakey1: 'metaValue1', metakey2: 'metaValue2', metakey3: 'metaValue3' + metakey1: 'metaValue1', + metakey2: 'metaValue2', + metakey3: 'metaValue3' }) expect(log.warn).to.not.have.been.called @@ -179,11 +210,20 @@ describe('track_event', () => { }) expect(prioritySampler.setPriority) .to.have.been.calledOnceWithExactly(rootSpan, USER_KEEP, SAMPLING_MECHANISM_APPSEC) + expect(sample).to.have.been.calledOnceWithExactly(rootSpan) + expect(waf.run).to.have.been.calledOnceWithExactly({ + persistent: { + [LOGIN_FAILURE]: null, + [USER_LOGIN]: 'user_id' + } + }) }) it('should send false `usr.exists` property when the user does not exist', () => { trackUserLoginFailureEvent(tracer, 'user_id', false, { - metakey1: 'metaValue1', metakey2: 'metaValue2', metakey3: 'metaValue3' + metakey1: 'metaValue1', + metakey2: 'metaValue2', + metakey3: 'metaValue3' }) expect(log.warn).to.not.have.been.called @@ -199,6 +239,13 @@ describe('track_event', () => { }) expect(prioritySampler.setPriority) .to.have.been.calledOnceWithExactly(rootSpan, USER_KEEP, SAMPLING_MECHANISM_APPSEC) + expect(sample).to.have.been.calledOnceWithExactly(rootSpan) + expect(waf.run).to.have.been.calledOnceWithExactly({ + persistent: { + [LOGIN_FAILURE]: null, + [USER_LOGIN]: 'user_id' + } + }) }) it('should call addTags without metadata', () => { @@ -214,14 +261,13 @@ describe('track_event', () => { }) expect(prioritySampler.setPriority) .to.have.been.calledOnceWithExactly(rootSpan, USER_KEEP, SAMPLING_MECHANISM_APPSEC) - }) - - it('should call waf run with login failure address', () => { - trackUserLoginFailureEvent(tracer, 'user_id') - sinon.assert.calledOnceWithExactly( - waf.run, - { persistent: { [LOGIN_FAILURE]: null } } - ) + expect(sample).to.have.been.calledOnceWithExactly(rootSpan) + expect(waf.run).to.have.been.calledOnceWithExactly({ + persistent: { + [LOGIN_FAILURE]: null, + [USER_LOGIN]: 'user_id' + } + }) }) }) @@ -250,7 +296,10 @@ describe('track_event', () => { }) it('should call addTags with metadata', () => { - trackCustomEvent(tracer, 'custom_event', { metaKey1: 'metaValue1', metakey2: 'metaValue2' }) + trackCustomEvent(tracer, 'custom_event', { + metaKey1: 'metaValue1', + metakey2: 'metaValue2' + }) expect(log.warn).to.not.have.been.called expect(setUserTags).to.not.have.been.called @@ -262,6 +311,7 @@ describe('track_event', () => { }) expect(prioritySampler.setPriority) .to.have.been.calledOnceWithExactly(rootSpan, USER_KEEP, SAMPLING_MECHANISM_APPSEC) + expect(sample).to.have.been.calledOnceWithExactly(rootSpan) }) it('should call addTags without metadata', () => { @@ -275,6 +325,7 @@ describe('track_event', () => { }) expect(prioritySampler.setPriority) .to.have.been.calledOnceWithExactly(rootSpan, USER_KEEP, SAMPLING_MECHANISM_APPSEC) + expect(sample).to.have.been.calledOnceWithExactly(rootSpan) }) }) }) From a93b79a15067305f176e5a187dbbe7cc98ded6ff Mon Sep 17 00:00:00 2001 From: simon-id Date: Fri, 29 Nov 2024 15:45:50 +0100 Subject: [PATCH 077/126] change ordering of code to match tests --- packages/dd-trace/src/appsec/sdk/track_event.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/dd-trace/src/appsec/sdk/track_event.js b/packages/dd-trace/src/appsec/sdk/track_event.js index 6d4d37162ee..06f10e05393 100644 --- a/packages/dd-trace/src/appsec/sdk/track_event.js +++ b/packages/dd-trace/src/appsec/sdk/track_event.js @@ -61,8 +61,6 @@ function trackEvent (eventName, fields, sdkMethodName, rootSpan) { return } - keepTrace(rootSpan, SAMPLING_MECHANISM_APPSEC) - const tags = { [`appsec.events.${eventName}.track`]: 'true', [`_dd.appsec.events.${eventName}.sdk`]: 'true' @@ -76,6 +74,7 @@ function trackEvent (eventName, fields, sdkMethodName, rootSpan) { rootSpan.addTags(tags) + keepTrace(rootSpan, SAMPLING_MECHANISM_APPSEC) standalone.sample(rootSpan) } From a83c4fbe56e70e101005d6bffc5cd9a72b221175 Mon Sep 17 00:00:00 2001 From: simon-id Date: Fri, 29 Nov 2024 18:32:12 +0100 Subject: [PATCH 078/126] delete passport-utils tests because it's useless --- .../test/passport-utils.spec.js | 36 ------------------- 1 file changed, 36 deletions(-) delete mode 100644 packages/datadog-instrumentations/test/passport-utils.spec.js diff --git a/packages/datadog-instrumentations/test/passport-utils.spec.js b/packages/datadog-instrumentations/test/passport-utils.spec.js deleted file mode 100644 index 3cf6a64a60a..00000000000 --- a/packages/datadog-instrumentations/test/passport-utils.spec.js +++ /dev/null @@ -1,36 +0,0 @@ -'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 not 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 6cf7ef264e1ae4fc06a99d0147056a8c7a40418e Mon Sep 17 00:00:00 2001 From: simon-id Date: Fri, 29 Nov 2024 18:48:26 +0100 Subject: [PATCH 079/126] fix test to correctly use passReqToCallback --- .../test/passport-local.spec.js | 58 ++++++++++++------- 1 file changed, 38 insertions(+), 20 deletions(-) diff --git a/packages/datadog-instrumentations/test/passport-local.spec.js b/packages/datadog-instrumentations/test/passport-local.spec.js index d54f02b289f..bc882bfc0e5 100644 --- a/packages/datadog-instrumentations/test/passport-local.spec.js +++ b/packages/datadog-instrumentations/test/passport-local.spec.js @@ -19,24 +19,44 @@ withVersions('passport-local', 'passport-local', version => { 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) - } + function validateUser (req, username, password, done) { + // support with or without passReqToCallback + if (typeof done !== 'function') { + done = password + password = username + username = req } - )) + + // simulate db error + if (username === 'error') return done('error') + + 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) + } + } + + passport.use('local', new LocalStrategy({ + usernameField: 'username', + passwordField: 'password', + passReqToCallback: false + }, validateUser)) + + passport.use('local-withreq', new LocalStrategy({ + usernameField: 'username', + passwordField: 'password', + passReqToCallback: true + }, validateUser)) app.use(passport.initialize()) app.use(express.json()) @@ -45,16 +65,14 @@ withVersions('passport-local', 'passport-local', version => { passport.authenticate('local', { successRedirect: '/grant', failureRedirect: '/deny', - passReqToCallback: false, session: false }) ) app.post('/req', - passport.authenticate('local', { + passport.authenticate('local-withreq', { successRedirect: '/grant', failureRedirect: '/deny', - passReqToCallback: true, session: false }) ) From 29c0d62bc75e6885cde7518e07dead50235d663d Mon Sep 17 00:00:00 2001 From: simon-id Date: Fri, 29 Nov 2024 18:52:12 +0100 Subject: [PATCH 080/126] update passport-local tests --- .../test/passport-local.spec.js | 74 +++++++++++++------ 1 file changed, 51 insertions(+), 23 deletions(-) diff --git a/packages/datadog-instrumentations/test/passport-local.spec.js b/packages/datadog-instrumentations/test/passport-local.spec.js index bc882bfc0e5..bcfc2e56dc9 100644 --- a/packages/datadog-instrumentations/test/passport-local.spec.js +++ b/packages/datadog-instrumentations/test/passport-local.spec.js @@ -1,8 +1,9 @@ 'use strict' const agent = require('../../dd-trace/test/plugins/agent') -const axios = require('axios') +const axios = require('axios').create({ validateStatus: null }) const dc = require('dc-polyfill') +const { storage } = require('../../datadog-core') withVersions('passport-local', 'passport-local', version => { describe('passport-local instrumentation', () => { @@ -10,7 +11,7 @@ withVersions('passport-local', 'passport-local', version => { let port, server, subscriberStub before(() => { - return agent.load(['express', 'passport', 'passport-local'], { client: false }) + return agent.load(['http', 'express', 'passport', 'passport-local'], { client: false }) }) before((done) => { @@ -85,9 +86,7 @@ withVersions('passport-local', 'passport-local', version => { res.send('Denied') }) - passportVerifyChannel.subscribe(function ({ credentials, user, err, info }) { - subscriberStub(arguments[0]) - }) + passportVerifyChannel.subscribe((data) => subscriberStub(data)) server = app.listen(0, () => { port = server.address().port @@ -104,17 +103,25 @@ withVersions('passport-local', 'passport-local', version => { return agent.close({ ritmReset: false }) }) + it('should not call subscriber when an error occurs', async () => { + const res = await axios.post(`http://localhost:${port}/`, { username: 'error', password: '1234' }) + + expect(res.status).to.equal(500) + expect(subscriberStub).to.not.be.called + }) + 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' }, - user: { _id: 1, username: 'test', password: '1234', email: 'testuser@ddog.com' } - } - ) + expect(subscriberStub).to.be.calledOnceWithExactly({ + framework: 'passport-local', + login: 'test', + user: { _id: 1, username: 'test', password: '1234', email: 'testuser@ddog.com' }, + success: true, + abortController: new AbortController() + }) }) it('should call subscriber with proper arguments on success with passReqToCallback set to true', async () => { @@ -122,12 +129,13 @@ withVersions('passport-local', 'passport-local', version => { 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' } - } - ) + expect(subscriberStub).to.be.calledOnceWithExactly({ + framework: 'passport-local', + login: 'test', + user: { _id: 1, username: 'test', password: '1234', email: 'testuser@ddog.com' }, + success: true, + abortController: new AbortController() + }) }) it('should call subscriber with proper arguments on failure', async () => { @@ -135,12 +143,32 @@ withVersions('passport-local', 'passport-local', version => { expect(res.status).to.equal(200) expect(res.data).to.equal('Denied') - expect(subscriberStub).to.be.calledOnceWithExactly( - { - credentials: { type: 'local', username: 'test' }, - user: false - } - ) + expect(subscriberStub).to.be.calledOnceWithExactly({ + framework: 'passport-local', + login: 'test', + user: false, + success: false, + abortController: new AbortController() + }) + }) + + it('should block when subscriber aborts', async () => { + subscriberStub = sinon.spy(({ abortController }) => { + storage.getStore().req.res.writeHead(403).end('Blocked') + abortController.abort() + }) + + const res = await axios.post(`http://localhost:${port}/`, { username: 'test', password: '1234' }) + + expect(res.status).to.equal(403) + expect(res.data).to.equal('Blocked') + expect(subscriberStub).to.be.calledOnceWithExactly({ + framework: 'passport-local', + login: 'test', + user: { _id: 1, username: 'test', password: '1234', email: 'testuser@ddog.com' }, + success: true, + abortController: new AbortController() + }) }) }) }) From afafb8aedafed9c66dd6ee3c9f3b2533bb6e2db2 Mon Sep 17 00:00:00 2001 From: simon-id Date: Mon, 2 Dec 2024 16:19:53 +0100 Subject: [PATCH 081/126] update tests for passpot-http --- .../test/passport-http.spec.js | 115 +++++++++++++----- 1 file changed, 86 insertions(+), 29 deletions(-) diff --git a/packages/datadog-instrumentations/test/passport-http.spec.js b/packages/datadog-instrumentations/test/passport-http.spec.js index 2918c935e20..08d2a9849df 100644 --- a/packages/datadog-instrumentations/test/passport-http.spec.js +++ b/packages/datadog-instrumentations/test/passport-http.spec.js @@ -1,8 +1,9 @@ 'use strict' const agent = require('../../dd-trace/test/plugins/agent') -const axios = require('axios') +const axios = require('axios').create({ validateStatus: null }) const dc = require('dc-polyfill') +const { storage } = require('../../datadog-core') withVersions('passport-http', 'passport-http', version => { describe('passport-http instrumentation', () => { @@ -10,7 +11,7 @@ withVersions('passport-http', 'passport-http', version => { let port, server, subscriberStub before(() => { - return agent.load(['express', 'passport', 'passport-http'], { client: false }) + return agent.load(['http', 'express', 'passport', 'passport-http'], { client: false }) }) before((done) => { @@ -19,7 +20,17 @@ withVersions('passport-http', 'passport-http', version => { const BasicStrategy = require(`../../../versions/passport-http@${version}`).get().BasicStrategy const app = express() - passport.use(new BasicStrategy((username, password, done) => { + function validateUser (req, username, password, done) { + // support with or without passReqToCallback + if (typeof done !== 'function') { + done = password + password = username + username = req + } + + // simulate db error + if (username === 'error') return done('error') + const users = [{ _id: 1, username: 'test', @@ -35,7 +46,18 @@ withVersions('passport-http', 'passport-http', version => { return done(null, user) } } - )) + + passport.use('basic', new BasicStrategy({ + usernameField: 'username', + passwordField: 'password', + passReqToCallback: false + }, validateUser)) + + passport.use('basic-withreq', new BasicStrategy({ + usernameField: 'username', + passwordField: 'password', + passReqToCallback: true + }, validateUser)) app.use(passport.initialize()) app.use(express.json()) @@ -44,16 +66,14 @@ withVersions('passport-http', 'passport-http', version => { passport.authenticate('basic', { successRedirect: '/grant', failureRedirect: '/deny', - passReqToCallback: false, session: false }) ) - app.post('/req', - passport.authenticate('basic', { + app.get('/req', + passport.authenticate('basic-withreq', { successRedirect: '/grant', failureRedirect: '/deny', - passReqToCallback: true, session: false }) ) @@ -66,9 +86,7 @@ withVersions('passport-http', 'passport-http', version => { res.send('Denied') }) - passportVerifyChannel.subscribe(function ({ credentials, user, err, info }) { - subscriberStub(arguments[0]) - }) + passportVerifyChannel.subscribe((data) => subscriberStub(data)) server = app.listen(0, () => { port = server.address().port @@ -85,6 +103,18 @@ withVersions('passport-http', 'passport-http', version => { return agent.close({ ritmReset: false }) }) + it('should not call subscriber when an error occurs', async () => { + const res = await axios.get(`http://localhost:${port}/`, { + headers: { + // error:1234 + Authorization: 'Basic ZXJyb3I6MTIzNAo=' + } + }) + + expect(res.status).to.equal(500) + expect(subscriberStub).to.not.be.called + }) + it('should call subscriber with proper arguments on success', async () => { const res = await axios.get(`http://localhost:${port}/`, { headers: { @@ -95,16 +125,17 @@ withVersions('passport-http', 'passport-http', version => { 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' } - } - ) + expect(subscriberStub).to.be.calledOnceWithExactly({ + framework: 'passport-basic', + login: 'test', + user: { _id: 1, username: 'test', password: '1234', email: 'testuser@ddog.com' }, + success: true, + abortController: new AbortController() + }) }) it('should call subscriber with proper arguments on success with passReqToCallback set to true', async () => { - const res = await axios.get(`http://localhost:${port}/`, { + const res = await axios.get(`http://localhost:${port}/req`, { headers: { // test:1234 Authorization: 'Basic dGVzdDoxMjM0' @@ -113,12 +144,13 @@ withVersions('passport-http', 'passport-http', version => { 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' } - } - ) + expect(subscriberStub).to.be.calledOnceWithExactly({ + framework: 'passport-basic', + login: 'test', + user: { _id: 1, username: 'test', password: '1234', email: 'testuser@ddog.com' }, + success: true, + abortController: new AbortController() + }) }) it('should call subscriber with proper arguments on failure', async () => { @@ -131,12 +163,37 @@ withVersions('passport-http', 'passport-http', version => { expect(res.status).to.equal(200) expect(res.data).to.equal('Denied') - expect(subscriberStub).to.be.calledOnceWithExactly( - { - credentials: { type: 'http', username: 'test' }, - user: false + expect(subscriberStub).to.be.calledOnceWithExactly({ + framework: 'passport-basic', + login: 'test', + user: false, + success: false, + abortController: new AbortController() + }) + }) + + it('should block when subscriber aborts', async () => { + subscriberStub = sinon.spy(({ abortController }) => { + storage.getStore().req.res.writeHead(403).end('Blocked') + abortController.abort() + }) + + const res = await axios.get(`http://localhost:${port}/`, { + headers: { + // test:1234 + Authorization: 'Basic dGVzdDoxMjM0' } - ) + }) + + expect(res.status).to.equal(403) + expect(res.data).to.equal('Blocked') + expect(subscriberStub).to.be.calledOnceWithExactly({ + framework: 'passport-basic', + login: 'test', + user: { _id: 1, username: 'test', password: '1234', email: 'testuser@ddog.com' }, + success: true, + abortController: new AbortController() + }) }) }) }) From 956cb9b45e863272e73ff7cfab27a5b4492ad3ac Mon Sep 17 00:00:00 2001 From: simon-id Date: Mon, 2 Dec 2024 17:01:13 +0100 Subject: [PATCH 082/126] allow empty login strings --- packages/dd-trace/src/appsec/user_tracking.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/dd-trace/src/appsec/user_tracking.js b/packages/dd-trace/src/appsec/user_tracking.js index e55db31d389..b8dfde08b2b 100644 --- a/packages/dd-trace/src/appsec/user_tracking.js +++ b/packages/dd-trace/src/appsec/user_tracking.js @@ -78,7 +78,7 @@ function trackLogin (framework, login, user, success, rootSpan) { return } - if (!login || typeof login !== 'string') { + if (typeof login !== 'string') { log.error('Invalid login provided to AppSec trackLogin') telemetry.incrementMissingUserLogin(framework, success ? 'login_success' : 'login_failure') From 62ae42b07d679d04d4fca994e6ff028c3e080e37 Mon Sep 17 00:00:00 2001 From: simon-id Date: Mon, 2 Dec 2024 17:05:05 +0100 Subject: [PATCH 083/126] update TS tests --- docs/test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/test.ts b/docs/test.ts index 479b4620b4d..ce34a23d62b 100644 --- a/docs/test.ts +++ b/docs/test.ts @@ -111,7 +111,7 @@ tracer.init({ blockedTemplateJson: './blocked.json', blockedTemplateGraphql: './blockedgraphql.json', eventTracking: { - mode: 'safe' + mode: 'anon' }, apiSecurity: { enabled: true, From 375f688f9ec3e50118e18a2a5e3d667900c32728 Mon Sep 17 00:00:00 2001 From: simon-id Date: Mon, 2 Dec 2024 17:12:08 +0100 Subject: [PATCH 084/126] update telemetry tests --- packages/dd-trace/test/appsec/telemetry.spec.js | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/packages/dd-trace/test/appsec/telemetry.spec.js b/packages/dd-trace/test/appsec/telemetry.spec.js index a297ede3280..7b21ec7a26c 100644 --- a/packages/dd-trace/test/appsec/telemetry.spec.js +++ b/packages/dd-trace/test/appsec/telemetry.spec.js @@ -339,6 +339,17 @@ describe('Appsec Telemetry metrics', () => { expect(count).to.not.have.been.called }) }) + + describe('incrementMissingUserLogin', () => { + it('should increment instrum.user_auth.missing_user_login metric', () => { + appsecTelemetry.incrementMissingUserLogin('passport-local', 'login_success') + + expect(count).to.have.been.calledOnceWithExactly('instrum.user_auth.missing_user_login', { + framework: 'passport-local', + event_type: 'login_success' + }) + }) + }) }) describe('if disabled', () => { From 5a0ee2e7ede8c07e7836afa1cfbc3f7c243bec2d Mon Sep 17 00:00:00 2001 From: simon-id Date: Thu, 26 Dec 2024 17:11:46 +0100 Subject: [PATCH 085/126] push stuff --- packages/dd-trace/src/appsec/channels.js | 2 +- packages/dd-trace/src/appsec/index.js | 31 +++++++------ packages/dd-trace/src/appsec/user_tracking.js | 43 +++++++++++-------- 3 files changed, 44 insertions(+), 32 deletions(-) diff --git a/packages/dd-trace/src/appsec/channels.js b/packages/dd-trace/src/appsec/channels.js index aa701f975dc..1fe8d632041 100644 --- a/packages/dd-trace/src/appsec/channels.js +++ b/packages/dd-trace/src/appsec/channels.js @@ -13,8 +13,8 @@ module.exports = { apolloServerCoreChannel: dc.tracingChannel('datadog:apollo-server-core:request'), incomingHttpRequestStart: dc.channel('dd-trace:incomingHttpRequestStart'), incomingHttpRequestEnd: dc.channel('dd-trace:incomingHttpRequestEnd'), - passportUser: dc.channel('datadog:passport:deserializeUser:finish'), passportVerify: dc.channel('datadog:passport:verify:finish'), + passportUser: dc.channel('datadog:passport:deserializeUser:finish'), queryParser: dc.channel('datadog:query:read:finish'), setCookieChannel: dc.channel('datadog:iast:set-cookie'), nextBodyParsed: dc.channel('apm:next:body-parsed'), diff --git a/packages/dd-trace/src/appsec/index.js b/packages/dd-trace/src/appsec/index.js index 736be7bb20b..bcbf0de138e 100644 --- a/packages/dd-trace/src/appsec/index.js +++ b/packages/dd-trace/src/appsec/index.js @@ -9,8 +9,8 @@ const { multerParser, incomingHttpRequestStart, incomingHttpRequestEnd, - passportUser, passportVerify, + passportUser, queryParser, nextBodyParsed, nextQueryParsed, @@ -67,8 +67,8 @@ function enable (_config) { cookieParser.subscribe(onRequestCookieParser) incomingHttpRequestStart.subscribe(incomingHttpStartTranslator) incomingHttpRequestEnd.subscribe(incomingHttpEndTranslator) - passportUser.subscribe(onPassportDeserializeUser) passportVerify.subscribe(onPassportVerify) // possible optimization: only subscribe if collection mode is enabled + passportUser.subscribe(onPassportDeserializeUser) queryParser.subscribe(onRequestQueryParsed) nextBodyParsed.subscribe(onRequestBodyParsed) nextQueryParsed.subscribe(onRequestQueryParsed) @@ -185,28 +185,32 @@ function incomingHttpEndTranslator ({ req, res }) { Reporter.finishRequest(req, res) } -function onPassportDeserializeUser ({ req, user, sessionId, abordController }) { - UserTracking.trackUser(user) +function onPassportVerify ({ framework, login, user, success, abortController }) { + const store = storage.getStore() + const rootSpan = store?.req && web.root(store.req) - if (sessionId && typeof sessionId === 'string') { - const results = waf.run({ - persistent: { - 'usr.session_id': sessionId - } - }) + if (!rootSpan) { + log.warn('[ASM] No rootSpan found in onPassportVerify') + return } + + const results = UserTracking.trackLogin(framework, login, user, success, rootSpan) + + handleResults(results, store.req, store.req.res, rootSpan, abortController) } -function onPassportVerify ({ framework, login, user, success, abortController }) { +function onPassportDeserializeUser ({ user, abortController }) { const store = storage.getStore() const rootSpan = store?.req && web.root(store.req) if (!rootSpan) { - log.warn('[ASM] No rootSpan found in onPassportVerify') + log.warn('[ASM] No rootSpan found in onPassportDeserializeUser') return } - const results = UserTracking.trackLogin(framework, login, user, success, rootSpan) + const results = UserTracking.trackUser(user, rootSpan) + + console.log(results) handleResults(results, store.req, store.req.res, rootSpan, abortController) } @@ -324,6 +328,7 @@ function disable () { if (incomingHttpRequestStart.hasSubscribers) incomingHttpRequestStart.unsubscribe(incomingHttpStartTranslator) if (incomingHttpRequestEnd.hasSubscribers) incomingHttpRequestEnd.unsubscribe(incomingHttpEndTranslator) if (passportVerify.hasSubscribers) passportVerify.unsubscribe(onPassportVerify) + if (passportUser.hasSubscribers) passportUser.unsubscribe(onPassportDeserializeUser) if (queryParser.hasSubscribers) queryParser.unsubscribe(onRequestQueryParsed) if (nextBodyParsed.hasSubscribers) nextBodyParsed.unsubscribe(onRequestBodyParsed) if (nextQueryParsed.hasSubscribers) nextQueryParsed.unsubscribe(onRequestQueryParsed) diff --git a/packages/dd-trace/src/appsec/user_tracking.js b/packages/dd-trace/src/appsec/user_tracking.js index 04522d97cba..7865ee78d26 100644 --- a/packages/dd-trace/src/appsec/user_tracking.js +++ b/packages/dd-trace/src/appsec/user_tracking.js @@ -49,10 +49,16 @@ function obfuscateIfNeeded (str) { } } +// what if we have a stack of auth strategies and only last one is sucessful ? we shouldn't send a million failures +// what if sdk is called after automated user, will the waf detect it ? will the tags be overriden ? + // TODO: should we find other ways to get the user ID ? function getUserId (user) { if (!user) return + // should we iterate on user keyss instead to be case independent + // but if we iterate over user then we're missing the inherited props ? + for (const field of USER_ID_FIELDS) { let id = user[field] @@ -73,11 +79,6 @@ function getUserId (user) { function trackLogin (framework, login, user, success, rootSpan) { if (!collectionMode || collectionMode === 'disabled') return - if (!rootSpan) { - log.error('[ASM] No rootSpan found in AppSec trackLogin') - return - } - if (typeof login !== 'string') { log.error('[ASM] Invalid login provided to AppSec trackLogin') @@ -162,32 +163,38 @@ function trackLogin (framework, login, user, success, rootSpan) { return waf.run({ persistent }) } -function trackUser (user, abortController) { +function trackUser (user, rootSpan) { if (!collectionMode || collectionMode === 'disabled') return const userId = getUserId(user) - // must get user ID with USER_ID_FIELDS - if (!userId) { - + log.error('[ASM] Invalid login provided to AppSec trackLogin') + return } - // ANONYMISE user id if needed + const currentTags = rootSpan.context()._tags + + // used to not overwrite tags set by SDK + function shouldSetTag (tag) { + return !(isSdkCalled && currentTags[tag]) + } - // don't override tags if already set by "sdk" but still add the _dd.appsec.usr.id - rootSpan.addTags({ - 'usr.id': userId, + const newTags = { '_dd.appsec.usr.id': userId, // always AND only send when automated - '_dd.appsec.user.collection_mode': collectionMode // short or long form ? - }) + '_dd.appsec.user.collection_mode': collectionMode + } - // _dd.appsec.user.collection_mode: collectionMode // sdk/ident/anon + if (!currentTags['usr.id']) { + newTags['usr.id'] = userId + } + + rootSpan.addTags(newTags) // If the user monitoring SDK has already resulted in a call to libddwaf before any automated instrumentation or collection method has been executed, no extra call should be made. - const results = waf.run({ + return waf.run({ persistent: { - [USER_ID]: userID + [addresses.USER_ID]: userId } }) } From 775bf9a1124829d47e31f87e526ee8f755887bbd Mon Sep 17 00:00:00 2001 From: simon-id Date: Fri, 27 Dec 2024 15:25:27 +0100 Subject: [PATCH 086/126] remove session id support --- .../datadog-instrumentations/src/passport.js | 37 +++---------------- 1 file changed, 5 insertions(+), 32 deletions(-) diff --git a/packages/datadog-instrumentations/src/passport.js b/packages/datadog-instrumentations/src/passport.js index b48b5e32432..27d066e513f 100644 --- a/packages/datadog-instrumentations/src/passport.js +++ b/packages/datadog-instrumentations/src/passport.js @@ -19,15 +19,13 @@ koa-passport const onPassportDeserializeUserChannel = channel('datadog:passport:deserializeUser:finish') -function wrapDone (req, done) { +function wrapDone (done) { // eslint-disable-next-line n/handle-callback-err return function wrappedDone (err, user) { if (user) { const abortController = new AbortController() - // express-session middleware sets req.sessionID, it's required to use passport sessions anyway so might as well use it ? - // what if session IDs are using rolling sessions or always changing or something idk ? - onPassportDeserializeUserChannel.publish({ req, user, sessionId: req.sessionID, abortController }) + onPassportDeserializeUserChannel.publish({ user, abortController }) if (abortController.signal.aborted) return } @@ -38,9 +36,10 @@ function wrapDone (req, done) { function wrapDeserializeUser (deserializeUser) { return function wrappedDeserializeUser (fn, req, done) { + if (typeof fn === 'function') return deserializeUser.apply(this, arguments) + if (typeof req === 'function') { done = req - // req = storage.getStore().get('req') arguments[1] = wrapDone(done) } else { arguments[2] = wrapDone(done) @@ -50,38 +49,12 @@ function wrapDeserializeUser (deserializeUser) { } } - -const { block } = require('../../dd-trace/src/appsec/blocking') -const { getRootSpan } = require('../../dd-trace/src/appsec/sdk/utils') - addHook({ name: 'passport', file: 'lib/authenticator.js', - versions: ['>=0.3.0'] // TODO + versions: ['>=0.2.0'] }, Authenticator => { shimmer.wrap(Authenticator.prototype, 'deserializeUser', wrapDeserializeUser) - shimmer.wrap(Authenticator.prototype, 'authenticate', function wrapAuthenticate (authenticate) { - return function wrappedAuthenticate (name) { - const middleware = authenticate.apply(this, arguments) - - const strategy = this._strategy(name) - - strategy._verify - - return function wrappedMiddleware (req, res, next) { - return middleware(req, res, function wrappedNext (err) { - console.log('NEW', req.user) - if (req.user?.name === 'bitch') { - - return block(req, res, getRootSpan(global._ddtrace)) - } - - return next.apply(this, arguments) - }) - } - } - }) - return Authenticator }) From cb78746318a3c635e91d64eecc42f8e64208bfb2 Mon Sep 17 00:00:00 2001 From: simon-id Date: Mon, 13 Jan 2025 19:39:13 +0100 Subject: [PATCH 087/126] add telemetry function --- packages/dd-trace/src/appsec/telemetry.js | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/packages/dd-trace/src/appsec/telemetry.js b/packages/dd-trace/src/appsec/telemetry.js index 8e9a2518f80..f7d1af0a329 100644 --- a/packages/dd-trace/src/appsec/telemetry.js +++ b/packages/dd-trace/src/appsec/telemetry.js @@ -181,6 +181,15 @@ function incrementMissingUserLoginMetric (framework, eventType) { }).inc() } +function incrementMissingUserIdMetric (framework, eventType) { + if (!enabled) return + + appsecMetrics.count('instrum.user_auth.missing_user_id', { + framework, + event_type: eventType + }).inc() +} + function getRequestMetrics (req) { if (req) { const store = getStore(req) @@ -198,6 +207,7 @@ module.exports = { incrementWafUpdatesMetric, incrementWafRequestsMetric, incrementMissingUserLoginMetric, + incrementMissingUserIdMetric, getRequestMetrics } From a259b22917108c01c816c816489e3d188e949e79 Mon Sep 17 00:00:00 2001 From: simon-id Date: Mon, 13 Jan 2025 19:39:43 +0100 Subject: [PATCH 088/126] use telemetry --- packages/dd-trace/src/appsec/user_tracking.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/dd-trace/src/appsec/user_tracking.js b/packages/dd-trace/src/appsec/user_tracking.js index 7865ee78d26..21129e71752 100644 --- a/packages/dd-trace/src/appsec/user_tracking.js +++ b/packages/dd-trace/src/appsec/user_tracking.js @@ -170,6 +170,7 @@ function trackUser (user, rootSpan) { if (!userId) { log.error('[ASM] Invalid login provided to AppSec trackLogin') + telemetry.incrementMissingUserIdMetric('passport', 'authenticated_request') return } From 96cf8e7713ff279ec116ed567704791a1cd92433 Mon Sep 17 00:00:00 2001 From: simon-id Date: Mon, 13 Jan 2025 19:48:14 +0100 Subject: [PATCH 089/126] cleanup --- packages/dd-trace/src/appsec/user_tracking.js | 47 ++++++++++++------- 1 file changed, 30 insertions(+), 17 deletions(-) diff --git a/packages/dd-trace/src/appsec/user_tracking.js b/packages/dd-trace/src/appsec/user_tracking.js index 21129e71752..27d33939085 100644 --- a/packages/dd-trace/src/appsec/user_tracking.js +++ b/packages/dd-trace/src/appsec/user_tracking.js @@ -167,37 +167,34 @@ function trackUser (user, rootSpan) { if (!collectionMode || collectionMode === 'disabled') return const userId = getUserId(user) - if (!userId) { - log.error('[ASM] Invalid login provided to AppSec trackLogin') + log.error('[ASM] No valid user ID found in AppSec trackUser') telemetry.incrementMissingUserIdMetric('passport', 'authenticated_request') return } - const currentTags = rootSpan.context()._tags - - // used to not overwrite tags set by SDK - function shouldSetTag (tag) { - return !(isSdkCalled && currentTags[tag]) - } + const isSdkCalled = rootSpan.context()._tags.currentTags['_dd.appsec.user.collection_mode'] === 'sdk' const newTags = { - '_dd.appsec.usr.id': userId, // always AND only send when automated - '_dd.appsec.user.collection_mode': collectionMode + '_dd.appsec.usr.id': userId } - if (!currentTags['usr.id']) { + // do not override SDK + if (!isSdkCalled) { newTags['usr.id'] = userId + newTags['_dd.appsec.user.collection_mode'] = collectionMode } rootSpan.addTags(newTags) - // If the user monitoring SDK has already resulted in a call to libddwaf before any automated instrumentation or collection method has been executed, no extra call should be made. - return waf.run({ - persistent: { - [addresses.USER_ID]: userId - } - }) + // do not override SDK + if (!isSdkCalled) { + return waf.run({ + persistent: { + [addresses.USER_ID]: userId + } + }) + } } module.exports = { @@ -205,3 +202,19 @@ module.exports = { trackLogin, trackUser } + + + +/* +check conflict when trackUser and trackLogin is called + + +test with: +- express-session with passport +- passport-jwt (or general jwt tokens) +- data stored in cookies +- opaque tokens that calls to third party service to get the users in each request (auth0, hydra...) +- passport-saml (Onelogin, Okta, Shibboleth, LDAP) +- passport-oauth2 + +*/ From c519d6eb4eb42683c7bdf6bbf0a9c7df6971aad5 Mon Sep 17 00:00:00 2001 From: simon-id Date: Wed, 15 Jan 2025 14:10:12 +0100 Subject: [PATCH 090/126] bad copy paste --- packages/dd-trace/src/appsec/user_tracking.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/dd-trace/src/appsec/user_tracking.js b/packages/dd-trace/src/appsec/user_tracking.js index 27d33939085..fbedd64d4e6 100644 --- a/packages/dd-trace/src/appsec/user_tracking.js +++ b/packages/dd-trace/src/appsec/user_tracking.js @@ -173,7 +173,7 @@ function trackUser (user, rootSpan) { return } - const isSdkCalled = rootSpan.context()._tags.currentTags['_dd.appsec.user.collection_mode'] === 'sdk' + const isSdkCalled = rootSpan.context()._tags['_dd.appsec.user.collection_mode'] === 'sdk' const newTags = { '_dd.appsec.usr.id': userId From 8767533cf47148091966a51836773c52fb1a406a Mon Sep 17 00:00:00 2001 From: simon-id Date: Wed, 15 Jan 2025 16:45:50 +0100 Subject: [PATCH 091/126] remove comments --- packages/dd-trace/src/appsec/sdk/set_user.js | 44 -------------------- 1 file changed, 44 deletions(-) diff --git a/packages/dd-trace/src/appsec/sdk/set_user.js b/packages/dd-trace/src/appsec/sdk/set_user.js index 810caabd5b4..26ddb313232 100644 --- a/packages/dd-trace/src/appsec/sdk/set_user.js +++ b/packages/dd-trace/src/appsec/sdk/set_user.js @@ -22,45 +22,8 @@ function setUser (tracer, user) { return } - // must get user ID with USER_ID_FIELDS - - setTags({ - 'usr.id': userId, - '_dd.appsec.user.collection_mode': 'sdk' - }) - - /* - When the user monitoring SDK is available and in use, the following applies: - The usr.id must be set to the value provided through the user monitoring SDK. - The span tag _dd.appsec.user.collection_mode must be set to sdk. - This effectively means that the automated user ID collection mechanism must not overwrite the aforementioned span tags, while the user monitoring SDK must overwrite them if present. - */ - - - - /* - When a user provides their own session ID through the use of the SDK using the session_id key: - If libddwaf hasn’t already been called with the usr.session_id address, it should be called with the provided session_id and further calls through the automated collection method should be inhibited. - If libddwaf has already been called with the usr.session_id address, it should be called again. - */ - if (user.session_id && typeof user.session_id === 'string') { - persistent['usr.session_id'] = user.session_id - } - - - setUserTags(user, rootSpan) - // If the automated instrumentation or collection method has resulted in a call to libddwaf before the user monitoring SDK has been executed, a second call must be performed with the user ID obtained through the SDK. - // will the second call trigger tho ? make some edge case tests - const results = waf.run({ - persistent: { - [USER_ID]: userId - } - }) - - - const persistent = {} if (user.id) { @@ -72,13 +35,6 @@ function setUser (tracer, user) { } waf.run({ persistent }) - - /* - User IDs generated through the SDK must now be provided to libddwaf as a persistent addresses. - If the user monitoring SDK has already resulted in a call to libddwaf before any automated instrumentation or collection method has been executed, no extra call should be made. - If the automated instrumentation or collection method has resulted in a call to libddwaf before the user monitoring SDK has been executed, a second call must be performed with the user ID obtained through the SDK. - */ - // will the second call trigger tho ? make some edge case tests } module.exports = { From a5086499094dea3486fa4983b7886b6689dba0b5 Mon Sep 17 00:00:00 2001 From: simon-id Date: Wed, 15 Jan 2025 16:46:07 +0100 Subject: [PATCH 092/126] fix sdk --- packages/dd-trace/src/appsec/sdk/set_user.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/dd-trace/src/appsec/sdk/set_user.js b/packages/dd-trace/src/appsec/sdk/set_user.js index 26ddb313232..5c8bc268ec7 100644 --- a/packages/dd-trace/src/appsec/sdk/set_user.js +++ b/packages/dd-trace/src/appsec/sdk/set_user.js @@ -3,11 +3,14 @@ const { getRootSpan } = require('./utils') const log = require('../../log') const waf = require('../waf') +const addresses = require('../addresses') function setUserTags (user, rootSpan) { for (const k of Object.keys(user)) { rootSpan.setTag(`usr.${k}`, '' + user[k]) } + + rootSpan.setTag('_dd.appsec.user.collection_mode', 'sdk') } function setUser (tracer, user) { From 42d9fdab48e0b43784d298a998a47a5c2740753d Mon Sep 17 00:00:00 2001 From: simon-id Date: Wed, 15 Jan 2025 16:59:03 +0100 Subject: [PATCH 093/126] cleanup --- packages/dd-trace/src/appsec/index.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/dd-trace/src/appsec/index.js b/packages/dd-trace/src/appsec/index.js index bcbf0de138e..9c948290525 100644 --- a/packages/dd-trace/src/appsec/index.js +++ b/packages/dd-trace/src/appsec/index.js @@ -210,8 +210,6 @@ function onPassportDeserializeUser ({ user, abortController }) { const results = UserTracking.trackUser(user, rootSpan) - console.log(results) - handleResults(results, store.req, store.req.res, rootSpan, abortController) } From d64369124714ed3ae7dc498d93cedd2a5c2af43b Mon Sep 17 00:00:00 2001 From: simon-id Date: Wed, 15 Jan 2025 17:15:39 +0100 Subject: [PATCH 094/126] cleanup --- packages/datadog-instrumentations/src/passport.js | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/packages/datadog-instrumentations/src/passport.js b/packages/datadog-instrumentations/src/passport.js index 27d066e513f..158a4f58109 100644 --- a/packages/datadog-instrumentations/src/passport.js +++ b/packages/datadog-instrumentations/src/passport.js @@ -3,20 +3,6 @@ const shimmer = require('../../datadog-shimmer') const { channel, addHook } = require('./helpers/instrument') -/* TODO: test with: -passport-jwt JWTs - can be used both for login events, or as a session, that complicates things it think - maybe instrument this lib directly, and ofc only send the events after it was verified -@nestjs/passport -pasport-local -passport-oauth2 -passport-google-oauth20 -passport-custom -passport-http -passport-http-bearer -koa-passport -*/ - const onPassportDeserializeUserChannel = channel('datadog:passport:deserializeUser:finish') function wrapDone (done) { From d9103708e82a4225d03d3b9697db6aae37cf551f Mon Sep 17 00:00:00 2001 From: simon-id Date: Wed, 15 Jan 2025 17:21:25 +0100 Subject: [PATCH 095/126] cleanup --- packages/datadog-instrumentations/src/passport.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/datadog-instrumentations/src/passport.js b/packages/datadog-instrumentations/src/passport.js index 158a4f58109..f79ac0c1773 100644 --- a/packages/datadog-instrumentations/src/passport.js +++ b/packages/datadog-instrumentations/src/passport.js @@ -6,9 +6,8 @@ const { channel, addHook } = require('./helpers/instrument') const onPassportDeserializeUserChannel = channel('datadog:passport:deserializeUser:finish') function wrapDone (done) { - // eslint-disable-next-line n/handle-callback-err return function wrappedDone (err, user) { - if (user) { + if (!err && user) { const abortController = new AbortController() onPassportDeserializeUserChannel.publish({ user, abortController }) From 28254ce1e6e3169b135cd033eaef8be157a41723 Mon Sep 17 00:00:00 2001 From: simon-id Date: Thu, 16 Jan 2025 12:03:31 +0100 Subject: [PATCH 096/126] better original res.end call --- packages/datadog-instrumentations/src/http/server.js | 1 - packages/dd-trace/src/appsec/blocking.js | 5 ++++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/datadog-instrumentations/src/http/server.js b/packages/datadog-instrumentations/src/http/server.js index f3245c24a8d..0624c886787 100644 --- a/packages/datadog-instrumentations/src/http/server.js +++ b/packages/datadog-instrumentations/src/http/server.js @@ -24,7 +24,6 @@ addHook({ name: httpNames }, http => { shimmer.wrap(http.Server.prototype, 'emit', wrapEmit) shimmer.wrap(http.ServerResponse.prototype, 'writeHead', wrapWriteHead) shimmer.wrap(http.ServerResponse.prototype, 'write', wrapWrite) - http.ServerResponse.prototype._originalEnd = http.ServerResponse.prototype.end shimmer.wrap(http.ServerResponse.prototype, 'end', wrapEnd) shimmer.wrap(http.ServerResponse.prototype, 'setHeader', wrapSetHeader) shimmer.wrap(http.ServerResponse.prototype, 'removeHeader', wrapAppendOrRemoveHeader) diff --git a/packages/dd-trace/src/appsec/blocking.js b/packages/dd-trace/src/appsec/blocking.js index 906dfbc0128..733b982a811 100644 --- a/packages/dd-trace/src/appsec/blocking.js +++ b/packages/dd-trace/src/appsec/blocking.js @@ -115,7 +115,10 @@ function block (req, res, rootSpan, abortController, actionParameters = defaultB res.removeHeader(headerName) } - res.writeHead(statusCode, headers)._originalEnd(body) + res.writeHead(statusCode, headers) + + // this is needed to call the original end method, since express-session replaces it + res.constructor.prototype.end.call(res, body) responseBlockedSet.add(res) From 2ec2e5bbd306a4ff6ac05f3faddb3db9b79f9cf4 Mon Sep 17 00:00:00 2001 From: simon-id Date: Thu, 16 Jan 2025 12:05:52 +0100 Subject: [PATCH 097/126] revert old copy paste or something --- .../dd-trace/test/appsec/remote_config/index.spec.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/dd-trace/test/appsec/remote_config/index.spec.js b/packages/dd-trace/test/appsec/remote_config/index.spec.js index dba4dc702de..f3cc6a32dac 100644 --- a/packages/dd-trace/test/appsec/remote_config/index.spec.js +++ b/packages/dd-trace/test/appsec/remote_config/index.spec.js @@ -137,7 +137,7 @@ describe('Remote Config index', () => { const configId = 'collectionModeId' afterEach(() => { - listener('unnaply', rcConfig, configId) + listener('unapply', rcConfig, configId) }) it('should not update collection mode when not a string', () => { @@ -167,20 +167,20 @@ describe('Remote Config index', () => { expect(UserTracking.setCollectionMode).to.have.been.calledOnceWithExactly(rcConfig.auto_user_instrum.mode) }) - it('should revert collection mode when called with unnaply', () => { + it('should revert collection mode when called with unapply', () => { listener('apply', rcConfig, configId) UserTracking.setCollectionMode.resetHistory() - listener('unnaply', rcConfig, configId) + listener('unapply', rcConfig, configId) expect(UserTracking.setCollectionMode).to.have.been.calledOnceWithExactly(config.appsec.eventTracking.mode) }) - it('should not revert collection mode when called with unnaply and unknown id', () => { + it('should not revert collection mode when called with unapply and unknown id', () => { listener('apply', rcConfig, configId) UserTracking.setCollectionMode.resetHistory() - listener('unnaply', rcConfig, 'unknownId') + listener('unapply', rcConfig, 'unknownId') expect(UserTracking.setCollectionMode).to.not.have.been.called }) From bd3a597f7a0c44276e7920769676935d81decd4a Mon Sep 17 00:00:00 2001 From: simon-id Date: Thu, 16 Jan 2025 16:54:39 +0100 Subject: [PATCH 098/126] cleanup --- packages/dd-trace/src/appsec/user_tracking.js | 22 +------------------ 1 file changed, 1 insertion(+), 21 deletions(-) diff --git a/packages/dd-trace/src/appsec/user_tracking.js b/packages/dd-trace/src/appsec/user_tracking.js index fbedd64d4e6..876f3fb245c 100644 --- a/packages/dd-trace/src/appsec/user_tracking.js +++ b/packages/dd-trace/src/appsec/user_tracking.js @@ -49,16 +49,12 @@ function obfuscateIfNeeded (str) { } } -// what if we have a stack of auth strategies and only last one is sucessful ? we shouldn't send a million failures -// what if sdk is called after automated user, will the waf detect it ? will the tags be overriden ? - // TODO: should we find other ways to get the user ID ? function getUserId (user) { if (!user) return - // should we iterate on user keyss instead to be case independent + // should we iterate on user keys instead to be case insensitive ? // but if we iterate over user then we're missing the inherited props ? - for (const field of USER_ID_FIELDS) { let id = user[field] @@ -202,19 +198,3 @@ module.exports = { trackLogin, trackUser } - - - -/* -check conflict when trackUser and trackLogin is called - - -test with: -- express-session with passport -- passport-jwt (or general jwt tokens) -- data stored in cookies -- opaque tokens that calls to third party service to get the users in each request (auth0, hydra...) -- passport-saml (Onelogin, Okta, Shibboleth, LDAP) -- passport-oauth2 - -*/ From bc2b2d08f5cbbb757353c94b7b6cad70c040ccaf Mon Sep 17 00:00:00 2001 From: simon-id Date: Thu, 16 Jan 2025 18:16:02 +0100 Subject: [PATCH 099/126] cleanup --- packages/dd-trace/src/appsec/user_tracking.js | 20 ++++++++----------- 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/packages/dd-trace/src/appsec/user_tracking.js b/packages/dd-trace/src/appsec/user_tracking.js index 876f3fb245c..b9f85c98fc5 100644 --- a/packages/dd-trace/src/appsec/user_tracking.js +++ b/packages/dd-trace/src/appsec/user_tracking.js @@ -171,20 +171,16 @@ function trackUser (user, rootSpan) { const isSdkCalled = rootSpan.context()._tags['_dd.appsec.user.collection_mode'] === 'sdk' - const newTags = { - '_dd.appsec.usr.id': userId - } - // do not override SDK - if (!isSdkCalled) { - newTags['usr.id'] = userId - newTags['_dd.appsec.user.collection_mode'] = collectionMode - } - - rootSpan.addTags(newTags) + if (isSdkCalled) { + rootSpan.setTag('_dd.appsec.usr.id', userId) + } else { + rootSpan.addTags({ + '_dd.appsec.usr.id': userId, + 'usr.id': userId, + '_dd.appsec.user.collection_mode': collectionMode + }) - // do not override SDK - if (!isSdkCalled) { return waf.run({ persistent: { [addresses.USER_ID]: userId From c0ae70b60904326f47f3445f055f6fbb88087f7a Mon Sep 17 00:00:00 2001 From: simon-id Date: Fri, 17 Jan 2025 16:44:34 +0100 Subject: [PATCH 100/126] cleanup --- packages/dd-trace/src/appsec/user_tracking.js | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/packages/dd-trace/src/appsec/user_tracking.js b/packages/dd-trace/src/appsec/user_tracking.js index b9f85c98fc5..e94e63f3f06 100644 --- a/packages/dd-trace/src/appsec/user_tracking.js +++ b/packages/dd-trace/src/appsec/user_tracking.js @@ -169,14 +169,12 @@ function trackUser (user, rootSpan) { return } - const isSdkCalled = rootSpan.context()._tags['_dd.appsec.user.collection_mode'] === 'sdk' + rootSpan.setTag('_dd.appsec.usr.id', userId) + const isSdkCalled = rootSpan.context()._tags['_dd.appsec.user.collection_mode'] === 'sdk' // do not override SDK - if (isSdkCalled) { - rootSpan.setTag('_dd.appsec.usr.id', userId) - } else { + if (!isSdkCalled) { rootSpan.addTags({ - '_dd.appsec.usr.id': userId, 'usr.id': userId, '_dd.appsec.user.collection_mode': collectionMode }) From 60789f4756c5e256b77d0e0c55bef989544651e6 Mon Sep 17 00:00:00 2001 From: simon-id Date: Fri, 17 Jan 2025 16:50:16 +0100 Subject: [PATCH 101/126] don't put user tracking tag when calling login tracking --- packages/dd-trace/src/appsec/sdk/set_user.js | 3 +-- packages/dd-trace/src/appsec/sdk/user_blocking.js | 1 + 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/dd-trace/src/appsec/sdk/set_user.js b/packages/dd-trace/src/appsec/sdk/set_user.js index 5c8bc268ec7..ccae859d832 100644 --- a/packages/dd-trace/src/appsec/sdk/set_user.js +++ b/packages/dd-trace/src/appsec/sdk/set_user.js @@ -9,8 +9,6 @@ function setUserTags (user, rootSpan) { for (const k of Object.keys(user)) { rootSpan.setTag(`usr.${k}`, '' + user[k]) } - - rootSpan.setTag('_dd.appsec.user.collection_mode', 'sdk') } function setUser (tracer, user) { @@ -26,6 +24,7 @@ function setUser (tracer, user) { } setUserTags(user, rootSpan) + rootSpan.setTag('_dd.appsec.user.collection_mode', 'sdk') const persistent = {} diff --git a/packages/dd-trace/src/appsec/sdk/user_blocking.js b/packages/dd-trace/src/appsec/sdk/user_blocking.js index 8af54ccbec1..162251b10c5 100644 --- a/packages/dd-trace/src/appsec/sdk/user_blocking.js +++ b/packages/dd-trace/src/appsec/sdk/user_blocking.js @@ -23,6 +23,7 @@ function checkUserAndSetUser (tracer, user) { if (rootSpan) { if (!rootSpan.context()._tags['usr.id']) { setUserTags(user, rootSpan) + rootSpan.setTag('_dd.appsec.user.collection_mode', 'sdk') } } else { log.warn('[ASM] Root span not available in isUserBlocked') From 8c3e97bf2333fdb5f85b1e9207bd1d06df859eb5 Mon Sep 17 00:00:00 2001 From: simon-id Date: Mon, 20 Jan 2025 17:25:24 +0100 Subject: [PATCH 102/126] add telemetry test --- packages/dd-trace/test/appsec/telemetry.spec.js | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/packages/dd-trace/test/appsec/telemetry.spec.js b/packages/dd-trace/test/appsec/telemetry.spec.js index 3eb3b8521b4..91ea8660d3a 100644 --- a/packages/dd-trace/test/appsec/telemetry.spec.js +++ b/packages/dd-trace/test/appsec/telemetry.spec.js @@ -350,6 +350,17 @@ describe('Appsec Telemetry metrics', () => { }) }) }) + + describe('incrementMissingUserIdMetric', () => { + it('should increment instrum.user_auth.missing_user_id metric', () => { + appsecTelemetry.incrementMissingUserIdMetric('passport', 'authenticated_request') + + expect(count).to.have.been.calledOnceWithExactly('instrum.user_auth.missing_user_id', { + framework: 'passport', + event_type: 'authenticated_request' + }) + }) + }) }) describe('if disabled', () => { From 7adb49ac61fa23de90ae585698bf9b5ad0e993e9 Mon Sep 17 00:00:00 2001 From: simon-id Date: Mon, 20 Jan 2025 17:42:39 +0100 Subject: [PATCH 103/126] fix blocking tests --- .../dd-trace/test/appsec/blocking.spec.js | 39 ++++++++++--------- 1 file changed, 21 insertions(+), 18 deletions(-) diff --git a/packages/dd-trace/test/appsec/blocking.spec.js b/packages/dd-trace/test/appsec/blocking.spec.js index 8a5496b4ecf..1a809410694 100644 --- a/packages/dd-trace/test/appsec/blocking.spec.js +++ b/packages/dd-trace/test/appsec/blocking.spec.js @@ -37,11 +37,14 @@ describe('blocking', () => { res = { setHeader: sinon.stub(), writeHead: sinon.stub(), - end: sinon.stub(), getHeaderNames: sinon.stub().returns([]), - removeHeader: sinon.stub() + removeHeader: sinon.stub(), + constructor: { + prototype: { + end: sinon.stub() + } + } } - res.writeHead.returns(res) rootSpan = { addTags: sinon.stub() @@ -61,7 +64,7 @@ describe('blocking', () => { .calledOnceWithExactly('[ASM] Cannot send blocking response when headers have already been sent') expect(rootSpan.addTags).to.not.have.been.called expect(res.setHeader).to.not.have.been.called - expect(res.end).to.not.have.been.called + expect(res.constructor.prototype.end).to.not.have.been.called }) it('should send blocking response with html type if present in the headers', () => { @@ -73,7 +76,7 @@ describe('blocking', () => { 'Content-Type': 'text/html; charset=utf-8', 'Content-Length': 12 }) - expect(res.end).to.have.been.calledOnceWithExactly('htmlBodyéé') + expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly('htmlBodyéé') }) it('should send blocking response with json type if present in the headers in priority', () => { @@ -85,7 +88,7 @@ describe('blocking', () => { 'Content-Type': 'application/json', 'Content-Length': 8 }) - expect(res.end).to.have.been.calledOnceWithExactly('jsonBody') + expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly('jsonBody') }) it('should send blocking response with json type if neither html or json is present in the headers', () => { @@ -96,7 +99,7 @@ describe('blocking', () => { 'Content-Type': 'application/json', 'Content-Length': 8 }) - expect(res.end).to.have.been.calledOnceWithExactly('jsonBody') + expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly('jsonBody') }) it('should send blocking response and call abortController if passed in arguments', () => { @@ -108,7 +111,7 @@ describe('blocking', () => { 'Content-Type': 'application/json', 'Content-Length': 8 }) - expect(res.end).to.have.been.calledOnceWithExactly('jsonBody') + expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly('jsonBody') expect(abortController.signal.aborted).to.be.true }) @@ -125,7 +128,7 @@ describe('blocking', () => { 'Content-Type': 'application/json', 'Content-Length': 8 }) - expect(res.end).to.have.been.calledOnceWithExactly('jsonBody') + expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly('jsonBody') }) }) @@ -143,7 +146,7 @@ describe('blocking', () => { block(req, res, rootSpan) - expect(res.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.html) + expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.html) }) it('should block with default json template', () => { @@ -151,7 +154,7 @@ describe('blocking', () => { block(req, res, rootSpan) - expect(res.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.json) + expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.json) }) }) @@ -174,7 +177,7 @@ describe('blocking', () => { block(req, res, rootSpan, null, actionParameters) expect(res.writeHead).to.have.been.calledOnceWith(401) - expect(res.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.html) + expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.html) }) it('should block with default json template and custom status ' + @@ -189,7 +192,7 @@ describe('blocking', () => { block(req, res, rootSpan, null, actionParameters) expect(res.writeHead).to.have.been.calledOnceWith(401) - expect(res.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.json) + expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.json) }) it('should block with default html template and custom status ' + @@ -204,7 +207,7 @@ describe('blocking', () => { block(req, res, rootSpan, null, actionParameters) expect(res.writeHead).to.have.been.calledOnceWith(401) - expect(res.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.html) + expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.html) }) it('should block with default json template and custom status', () => { @@ -217,7 +220,7 @@ describe('blocking', () => { block(req, res, rootSpan, null, actionParameters) expect(res.writeHead).to.have.been.calledOnceWith(401) - expect(res.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.json) + expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.json) }) it('should block with default json template and custom status ' + @@ -231,7 +234,7 @@ describe('blocking', () => { block(req, res, rootSpan, null, actionParameters) expect(res.writeHead).to.have.been.calledOnceWith(401) - expect(res.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.json) + expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.json) }) it('should block with default html template and custom status ' + @@ -245,7 +248,7 @@ describe('blocking', () => { block(req, res, rootSpan, null, actionParameters) expect(res.writeHead).to.have.been.calledOnceWith(401) - expect(res.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.html) + expect(res.constructor.prototype.end).to.have.been.calledOnceWithExactly(defaultBlockedTemplate.html) }) it('should block with custom redirect', () => { @@ -260,7 +263,7 @@ describe('blocking', () => { expect(res.writeHead).to.have.been.calledOnceWithExactly(301, { Location: '/you-have-been-blocked' }) - expect(res.end).to.have.been.calledOnce + expect(res.constructor.prototype.end).to.have.been.calledOnce }) }) }) From d80b41c91f14fb327022ec55f8b0c8398405fc48 Mon Sep 17 00:00:00 2001 From: simon-id Date: Mon, 20 Jan 2025 17:48:01 +0100 Subject: [PATCH 104/126] fix appsec index blocking tests --- packages/dd-trace/test/appsec/index.spec.js | 45 +++++++++++---------- 1 file changed, 24 insertions(+), 21 deletions(-) diff --git a/packages/dd-trace/test/appsec/index.spec.js b/packages/dd-trace/test/appsec/index.spec.js index 7ca54e9241b..b17f4eeceea 100644 --- a/packages/dd-trace/test/appsec/index.spec.js +++ b/packages/dd-trace/test/appsec/index.spec.js @@ -656,10 +656,13 @@ describe('AppSec Index', function () { 'content-length': 42 }), writeHead: sinon.stub(), - end: sinon.stub(), - getHeaderNames: sinon.stub().returns([]) + getHeaderNames: sinon.stub().returns([]), + constructor: { + prototype: { + end: sinon.stub() + } + } } - res.writeHead.returns(res) req = { url: '/path', @@ -687,7 +690,7 @@ describe('AppSec Index', function () { expect(waf.run).not.to.have.been.called expect(abortController.abort).not.to.have.been.called - expect(res.end).not.to.have.been.called + expect(res.constructor.prototype.end).not.to.have.been.called }) it('Should not block with body by default', () => { @@ -703,7 +706,7 @@ describe('AppSec Index', function () { } }) expect(abortController.abort).not.to.have.been.called - expect(res.end).not.to.have.been.called + expect(res.constructor.prototype.end).not.to.have.been.called }) it('Should block when it is detected as attack', () => { @@ -719,7 +722,7 @@ describe('AppSec Index', function () { } }) expect(abortController.abort).to.have.been.called - expect(res.end).to.have.been.called + expect(res.constructor.prototype.end).to.have.been.called }) }) @@ -731,7 +734,7 @@ describe('AppSec Index', function () { expect(waf.run).not.to.have.been.called expect(abortController.abort).not.to.have.been.called - expect(res.end).not.to.have.been.called + expect(res.constructor.prototype.end).not.to.have.been.called }) it('Should not block with cookie by default', () => { @@ -746,7 +749,7 @@ describe('AppSec Index', function () { } }) expect(abortController.abort).not.to.have.been.called - expect(res.end).not.to.have.been.called + expect(res.constructor.prototype.end).not.to.have.been.called }) it('Should block when it is detected as attack', () => { @@ -761,7 +764,7 @@ describe('AppSec Index', function () { } }) expect(abortController.abort).to.have.been.called - expect(res.end).to.have.been.called + expect(res.constructor.prototype.end).to.have.been.called }) }) @@ -773,7 +776,7 @@ describe('AppSec Index', function () { expect(waf.run).not.to.have.been.called expect(abortController.abort).not.to.have.been.called - expect(res.end).not.to.have.been.called + expect(res.constructor.prototype.end).not.to.have.been.called }) it('Should not block with query by default', () => { @@ -789,7 +792,7 @@ describe('AppSec Index', function () { } }) expect(abortController.abort).not.to.have.been.called - expect(res.end).not.to.have.been.called + expect(res.constructor.prototype.end).not.to.have.been.called }) it('Should block when it is detected as attack', () => { @@ -805,7 +808,7 @@ describe('AppSec Index', function () { } }) expect(abortController.abort).to.have.been.called - expect(res.end).to.have.been.called + expect(res.constructor.prototype.end).to.have.been.called }) }) @@ -839,7 +842,7 @@ describe('AppSec Index', function () { rootSpan ) expect(abortController.signal.aborted).to.be.true - expect(res.end).to.have.been.called + expect(res.constructor.prototype.end).to.have.been.called }) it('should not block when UserTracking.login() returns nothing', () => { @@ -866,7 +869,7 @@ describe('AppSec Index', function () { rootSpan ) expect(abortController.signal.aborted).to.be.false - expect(res.end).to.not.have.been.called + expect(res.constructor.prototype.end).to.not.have.been.called }) it('should not block and call log if no rootSpan is found', () => { @@ -887,7 +890,7 @@ describe('AppSec Index', function () { expect(log.warn).to.have.been.calledOnceWithExactly('[ASM] No rootSpan found in onPassportVerify') expect(UserTracking.trackLogin).to.not.have.been.called expect(abortController.signal.aborted).to.be.false - expect(res.end).to.not.have.been.called + expect(res.constructor.prototype.end).to.not.have.been.called }) }) @@ -913,7 +916,7 @@ describe('AppSec Index', function () { } }, req) expect(abortController.abort).to.have.been.calledOnce - expect(res.end).to.have.been.calledOnce + expect(res.constructor.prototype.end).to.have.been.calledOnce abortController.abort.resetHistory() @@ -921,7 +924,7 @@ describe('AppSec Index', function () { expect(waf.run).to.have.been.calledOnce expect(abortController.abort).to.have.been.calledOnce - expect(res.end).to.have.been.calledOnce + expect(res.constructor.prototype.end).to.have.been.calledOnce }) it('should not call the WAF if response was already analyzed', () => { @@ -945,13 +948,13 @@ describe('AppSec Index', function () { } }, req) expect(abortController.abort).to.have.not.been.called - expect(res.end).to.have.not.been.called + expect(res.constructor.prototype.end).to.have.not.been.called responseWriteHead.publish({ req, res, abortController, statusCode: 404, responseHeaders }) expect(waf.run).to.have.been.calledOnce expect(abortController.abort).to.have.not.been.called - expect(res.end).to.have.not.been.called + expect(res.constructor.prototype.end).to.have.not.been.called }) it('should not do anything without a root span', () => { @@ -968,7 +971,7 @@ describe('AppSec Index', function () { expect(waf.run).to.have.not.been.called expect(abortController.abort).to.have.not.been.called - expect(res.end).to.have.not.been.called + expect(res.constructor.prototype.end).to.have.not.been.called }) it('should call the WAF with responde code and headers', () => { @@ -992,7 +995,7 @@ describe('AppSec Index', function () { } }, req) expect(abortController.abort).to.have.been.calledOnce - expect(res.end).to.have.been.calledOnce + expect(res.constructor.prototype.end).to.have.been.calledOnce }) }) From 8dfbb3dff225d11adcb369f5fac0fde4f67b33df Mon Sep 17 00:00:00 2001 From: simon-id Date: Mon, 20 Jan 2025 21:28:20 +0100 Subject: [PATCH 105/126] fix setuser tests --- packages/dd-trace/test/appsec/sdk/set_user.spec.js | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/packages/dd-trace/test/appsec/sdk/set_user.spec.js b/packages/dd-trace/test/appsec/sdk/set_user.spec.js index 29eb25560a1..7aa61c13485 100644 --- a/packages/dd-trace/test/appsec/sdk/set_user.spec.js +++ b/packages/dd-trace/test/appsec/sdk/set_user.spec.js @@ -61,10 +61,11 @@ describe('set_user', () => { setUser(tracer, user) expect(log.warn).to.not.have.been.called - expect(rootSpan.setTag).to.have.been.calledThrice - expect(rootSpan.setTag.firstCall).to.have.been.calledWithExactly('usr.id', '123') - expect(rootSpan.setTag.secondCall).to.have.been.calledWithExactly('usr.email', 'a@b.c') - expect(rootSpan.setTag.thirdCall).to.have.been.calledWithExactly('usr.custom', 'hello') + expect(rootSpan.setTag.callCount).to.equal(4) + expect(rootSpan.setTag.getCall(0)).to.have.been.calledWithExactly('usr.id', '123') + expect(rootSpan.setTag.getCall(1)).to.have.been.calledWithExactly('usr.email', 'a@b.c') + expect(rootSpan.setTag.getCall(2)).to.have.been.calledWithExactly('usr.custom', 'hello') + expect(rootSpan.setTag.getCall(3)).to.have.been.calledWithExactly('_dd.appsec.user.collection_mode', 'sdk') }) }) }) From 1beff4f8e9fc3293c63203db06eadfda063c71b9 Mon Sep 17 00:00:00 2001 From: simon-id Date: Tue, 21 Jan 2025 21:14:12 +0100 Subject: [PATCH 106/126] remove outdated test --- .../dd-trace/test/appsec/user_tracking.spec.js | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/packages/dd-trace/test/appsec/user_tracking.spec.js b/packages/dd-trace/test/appsec/user_tracking.spec.js index 651048d5515..d27e77e0852 100644 --- a/packages/dd-trace/test/appsec/user_tracking.spec.js +++ b/packages/dd-trace/test/appsec/user_tracking.spec.js @@ -171,21 +171,6 @@ describe('User Tracking', () => { sinon.assert.notCalled(waf.run) }) - it('should log error when rootSpan is not found', () => { - setCollectionMode('identification') - - const results = trackLogin('passport-local', 'login', { id: '123', email: 'a@b.c' }, true) - - assert.deepStrictEqual(results, undefined) - - sinon.assert.calledOnceWithExactly(log.error, '[ASM] No rootSpan found in AppSec trackLogin') - sinon.assert.notCalled(telemetry.incrementMissingUserLoginMetric) - sinon.assert.notCalled(keepTrace) - sinon.assert.notCalled(standalone.sample) - sinon.assert.notCalled(rootSpan.addTags) - sinon.assert.notCalled(waf.run) - }) - it('should log error and send telemetry when login success is not a string', () => { setCollectionMode('identification') From 2dd9cb5cf237d442bacdb577ae3094ee6d71fe3a Mon Sep 17 00:00:00 2001 From: simon-id Date: Tue, 21 Jan 2025 21:14:40 +0100 Subject: [PATCH 107/126] fix user blocking test --- packages/dd-trace/test/appsec/sdk/user_blocking.spec.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/dd-trace/test/appsec/sdk/user_blocking.spec.js b/packages/dd-trace/test/appsec/sdk/user_blocking.spec.js index 324b70267dd..e48bd3f23d3 100644 --- a/packages/dd-trace/test/appsec/sdk/user_blocking.spec.js +++ b/packages/dd-trace/test/appsec/sdk/user_blocking.spec.js @@ -77,7 +77,8 @@ describe('user_blocking', () => { const ret = userBlocking.checkUserAndSetUser(tracer, { id: 'user' }) expect(ret).to.be.true expect(getRootSpan).to.have.been.calledOnceWithExactly(tracer) - expect(rootSpan.setTag).to.have.been.calledOnceWithExactly('usr.id', 'user') + expect(rootSpan.setTag).to.have.been.calledWithExactly('usr.id', 'user') + expect(rootSpan.setTag).to.have.been.calledWithExactly('_dd.appsec.user.collection_mode', 'sdk') }) it('should not override user when already set', () => { @@ -104,7 +105,8 @@ describe('user_blocking', () => { it('should return false when received no results', () => { const ret = userBlocking.checkUserAndSetUser(tracer, { id: 'gooduser' }) expect(ret).to.be.false - expect(rootSpan.setTag).to.have.been.calledOnceWithExactly('usr.id', 'gooduser') + expect(rootSpan.setTag).to.have.been.calledWithExactly('usr.id', 'gooduser') + expect(rootSpan.setTag).to.have.been.calledWithExactly('_dd.appsec.user.collection_mode', 'sdk') }) }) From be219e17d0ef6a8757409163363b47ab119ca61e Mon Sep 17 00:00:00 2001 From: simon-id Date: Tue, 21 Jan 2025 21:58:38 +0100 Subject: [PATCH 108/126] trackUser() tests --- .../test/appsec/user_tracking.spec.js | 126 +++++++++++++++++- 1 file changed, 125 insertions(+), 1 deletion(-) diff --git a/packages/dd-trace/test/appsec/user_tracking.spec.js b/packages/dd-trace/test/appsec/user_tracking.spec.js index d27e77e0852..4c5745eff69 100644 --- a/packages/dd-trace/test/appsec/user_tracking.spec.js +++ b/packages/dd-trace/test/appsec/user_tracking.spec.js @@ -15,11 +15,13 @@ describe('User Tracking', () => { let setCollectionMode let trackLogin + let trackUser beforeEach(() => { sinon.stub(log, 'warn') sinon.stub(log, 'error') sinon.stub(telemetry, 'incrementMissingUserLoginMetric') + sinon.stub(telemetry, 'incrementMissingUserIdMetric') sinon.stub(standalone, 'sample') sinon.stub(waf, 'run').returns(['action1']) @@ -27,7 +29,8 @@ describe('User Tracking', () => { rootSpan = { context: () => ({ _tags: currentTags }), - addTags: sinon.stub() + addTags: sinon.stub(), + setTag: sinon.stub() } keepTrace = sinon.stub() @@ -38,6 +41,7 @@ describe('User Tracking', () => { setCollectionMode = UserTracking.setCollectionMode trackLogin = UserTracking.trackLogin + trackUser = UserTracking.trackUser }) afterEach(() => { @@ -678,4 +682,124 @@ describe('User Tracking', () => { }) }) }) + + describe('trackUser', () => { + it('should not do anything if collectionMode is empty or disabled', () => { + setCollectionMode('disabled') + + const results = trackUser({ id: '123', email: 'a@b.c' }, rootSpan) + + assert.deepStrictEqual(results, undefined) + + sinon.assert.notCalled(log.error) + sinon.assert.notCalled(telemetry.incrementMissingUserIdMetric) + sinon.assert.notCalled(rootSpan.setTag) + sinon.assert.notCalled(rootSpan.addTags) + sinon.assert.notCalled(waf.run) + }) + + it('should log error and send telemetry when user ID is not found', () => { + setCollectionMode('identification') + + const results = trackUser({ notAnId: 'bonjour' }, rootSpan) + + assert.deepStrictEqual(results, undefined) + + sinon.assert.calledOnceWithExactly(log.error, '[ASM] No valid user ID found in AppSec trackUser') + sinon.assert.calledOnceWithExactly(telemetry.incrementMissingUserIdMetric, 'passport', 'authenticated_request') + sinon.assert.notCalled(rootSpan.setTag) + sinon.assert.notCalled(rootSpan.addTags) + sinon.assert.notCalled(waf.run) + }) + + describe('when collectionMode is indentification', () => { + beforeEach(() => { + setCollectionMode('identification') + }) + + it('should write tags and call waf', () => { + const results = trackUser({ id: '123', email: 'a@b.c' }, rootSpan) + + assert.deepStrictEqual(results, ['action1']) + + sinon.assert.notCalled(log.error) + sinon.assert.notCalled(telemetry.incrementMissingUserIdMetric) + + sinon.assert.calledOnceWithExactly(rootSpan.setTag, '_dd.appsec.usr.id', '123') + sinon.assert.calledOnceWithExactly(rootSpan.addTags, { + 'usr.id': '123', + '_dd.appsec.user.collection_mode': 'identification' + }) + sinon.assert.calledOnceWithExactly(waf.run, { + persistent: { + 'usr.id': '123' + } + }) + }) + + it('should not overwrite tags set by SDK', () => { + currentTags = { + 'usr.id': 'sdk_id', + '_dd.appsec.user.collection_mode': 'sdk' + } + + const results = trackUser({ id: '123', email: 'a@b.c' }, rootSpan) + + assert.deepStrictEqual(results, undefined) + + sinon.assert.notCalled(log.error) + sinon.assert.notCalled(telemetry.incrementMissingUserIdMetric) + + sinon.assert.calledOnceWithExactly(rootSpan.setTag, '_dd.appsec.usr.id', '123') + + sinon.assert.notCalled(rootSpan.addTags) + sinon.assert.notCalled(waf.run) + }) + }) + + describe('when collectionMode is anonymization', () => { + beforeEach(() => { + setCollectionMode('anonymization') + }) + + it('should write tags and call waf', () => { + const results = trackUser({ id: '123', email: 'a@b.c' }, rootSpan) + + assert.deepStrictEqual(results, ['action1']) + + sinon.assert.notCalled(log.error) + sinon.assert.notCalled(telemetry.incrementMissingUserIdMetric) + + sinon.assert.calledOnceWithExactly(rootSpan.setTag, '_dd.appsec.usr.id', 'anon_a665a45920422f9d417e4867efdc4fb8') + sinon.assert.calledOnceWithExactly(rootSpan.addTags, { + 'usr.id': 'anon_a665a45920422f9d417e4867efdc4fb8', + '_dd.appsec.user.collection_mode': 'anonymization' + }) + sinon.assert.calledOnceWithExactly(waf.run, { + persistent: { + 'usr.id': 'anon_a665a45920422f9d417e4867efdc4fb8' + } + }) + }) + + it('should not overwrite tags set by SDK', () => { + currentTags = { + 'usr.id': 'sdk_id', + '_dd.appsec.user.collection_mode': 'sdk' + } + + const results = trackUser({ id: '123', email: 'a@b.c' }, rootSpan) + + assert.deepStrictEqual(results, undefined) + + sinon.assert.notCalled(log.error) + sinon.assert.notCalled(telemetry.incrementMissingUserIdMetric) + + sinon.assert.calledOnceWithExactly(rootSpan.setTag, '_dd.appsec.usr.id', 'anon_a665a45920422f9d417e4867efdc4fb8') + + sinon.assert.notCalled(rootSpan.addTags) + sinon.assert.notCalled(waf.run) + }) + }) + }) }) From 115e808c21cb2d454ef77d17ea83543833a6a9ec Mon Sep 17 00:00:00 2001 From: simon-id Date: Tue, 21 Jan 2025 22:34:12 +0100 Subject: [PATCH 109/126] update set_user tests --- .../dd-trace/test/appsec/sdk/set_user.spec.js | 52 ++++++++++++++++--- 1 file changed, 44 insertions(+), 8 deletions(-) diff --git a/packages/dd-trace/test/appsec/sdk/set_user.spec.js b/packages/dd-trace/test/appsec/sdk/set_user.spec.js index 7aa61c13485..8d8061801f0 100644 --- a/packages/dd-trace/test/appsec/sdk/set_user.spec.js +++ b/packages/dd-trace/test/appsec/sdk/set_user.spec.js @@ -3,13 +3,16 @@ const proxyquire = require('proxyquire') const agent = require('../../plugins/agent') const tracer = require('../../../../../index') +const appsec = require('../../../src/appsec') +const Config = require('../../../src/config') const axios = require('axios') +const path = require('path') describe('set_user', () => { describe('Internal API', () => { const tracer = {} - let rootSpan, getRootSpan, log, setUser + let rootSpan, getRootSpan, log, waf, setUser beforeEach(() => { rootSpan = { @@ -21,9 +24,14 @@ describe('set_user', () => { warn: sinon.stub() } + waf = { + run: sinon.stub() + } + const setUserModule = proxyquire('../../../src/appsec/sdk/set_user', { './utils': { getRootSpan }, - '../../log': log + '../../log': log, + '../waf': waf }) setUser = setUserModule.setUser @@ -34,6 +42,7 @@ describe('set_user', () => { setUser(tracer) expect(log.warn).to.have.been.calledOnceWithExactly('[ASM] Invalid user provided to setUser') expect(rootSpan.setTag).to.not.have.been.called + expect(waf.run).to.not.have.been.called }) it('should not call setTag when user is empty', () => { @@ -41,6 +50,7 @@ describe('set_user', () => { setUser(tracer, user) expect(log.warn).to.have.been.calledOnceWithExactly('[ASM] Invalid user provided to setUser') expect(rootSpan.setTag).to.not.have.been.called + expect(waf.run).to.not.have.been.called }) it('should not call setTag when rootSpan is not available', () => { @@ -50,12 +60,13 @@ describe('set_user', () => { expect(getRootSpan).to.be.calledOnceWithExactly(tracer) expect(log.warn).to.have.been.calledOnceWithExactly('[ASM] Root span not available in setUser') expect(rootSpan.setTag).to.not.have.been.called + expect(waf.run).to.not.have.been.called }) it('should call setTag with every attribute', () => { const user = { id: '123', - email: 'a@b.c', + login: 'login', custom: 'hello' } @@ -63,14 +74,27 @@ describe('set_user', () => { expect(log.warn).to.not.have.been.called expect(rootSpan.setTag.callCount).to.equal(4) expect(rootSpan.setTag.getCall(0)).to.have.been.calledWithExactly('usr.id', '123') - expect(rootSpan.setTag.getCall(1)).to.have.been.calledWithExactly('usr.email', 'a@b.c') + expect(rootSpan.setTag.getCall(1)).to.have.been.calledWithExactly('usr.login', 'login') expect(rootSpan.setTag.getCall(2)).to.have.been.calledWithExactly('usr.custom', 'hello') expect(rootSpan.setTag.getCall(3)).to.have.been.calledWithExactly('_dd.appsec.user.collection_mode', 'sdk') + expect(waf.run).to.have.been.calledOnceWithExactly({ + persistent: { + 'usr.id': '123', + 'usr.login': 'login' + } + }) }) }) }) describe('Integration with the tracer', () => { + const config = new Config({ + appsec: { + enabled: true, + rules: path.join(__dirname, './user_blocking_rules.json') + } + }) + let http let controller let appListener @@ -94,9 +118,13 @@ describe('set_user', () => { port = appListener.address().port done() }) + + appsec.enable(config) }) after(() => { + appsec.disable() + appListener.close() return agent.close({ ritmReset: false }) }) @@ -105,16 +133,20 @@ describe('set_user', () => { it('should set a proper user', (done) => { controller = (req, res) => { tracer.appsec.setUser({ - id: 'testUser', + id: 'blockedUser', email: 'a@b.c', custom: 'hello' }) res.end() } agent.use(traces => { - expect(traces[0][0].meta).to.have.property('usr.id', 'testUser') + expect(traces[0][0].meta).to.have.property('usr.id', 'blockedUser') expect(traces[0][0].meta).to.have.property('usr.email', 'a@b.c') expect(traces[0][0].meta).to.have.property('usr.custom', 'hello') + expect(traces[0][0].meta).to.have.property('_dd.appsec.user.collection_mode', 'sdk') + expect(traces[0][0].meta).to.have.property('appsec.event', 'true') + expect(traces[0][0].meta).to.not.have.property('appsec.blocked') + expect(traces[0][0].meta).to.have.property('http.status_code', '200') }).then(done).catch(done) axios.get(`http://localhost:${port}/`) }) @@ -122,11 +154,15 @@ describe('set_user', () => { it('should override user on consecutive callings', (done) => { controller = (req, res) => { tracer.appsec.setUser({ id: 'testUser' }) - tracer.appsec.setUser({ id: 'testUser2' }) + tracer.appsec.setUser({ id: 'blockedUser' }) res.end() } agent.use(traces => { - expect(traces[0][0].meta).to.have.property('usr.id', 'testUser2') + expect(traces[0][0].meta).to.have.property('usr.id', 'blockedUser') + expect(traces[0][0].meta).to.have.property('_dd.appsec.user.collection_mode', 'sdk') + expect(traces[0][0].meta).to.have.property('appsec.event', 'true') + expect(traces[0][0].meta).to.not.have.property('appsec.blocked') + expect(traces[0][0].meta).to.have.property('http.status_code', '200') }).then(done).catch(done) axios.get(`http://localhost:${port}/`) }) From 09aadcd8486ab0863777c6a3ba3021550aa6dc1c Mon Sep 17 00:00:00 2001 From: simon-id Date: Tue, 21 Jan 2025 22:39:41 +0100 Subject: [PATCH 110/126] update user blocking tests --- packages/dd-trace/test/appsec/sdk/user_blocking.spec.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/dd-trace/test/appsec/sdk/user_blocking.spec.js b/packages/dd-trace/test/appsec/sdk/user_blocking.spec.js index e48bd3f23d3..4eba390da27 100644 --- a/packages/dd-trace/test/appsec/sdk/user_blocking.spec.js +++ b/packages/dd-trace/test/appsec/sdk/user_blocking.spec.js @@ -200,6 +200,7 @@ describe('user_blocking', () => { } agent.use(traces => { expect(traces[0][0].meta).to.have.property('usr.id', 'testUser3') + expect(traces[0][0].meta).to.have.property('_dd.appsec.user.collection_mode', 'sdk') }).then(done).catch(done) axios.get(`http://localhost:${port}/`) }) @@ -214,6 +215,7 @@ describe('user_blocking', () => { } agent.use(traces => { expect(traces[0][0].meta).to.have.property('usr.id', 'testUser') + expect(traces[0][0].meta).to.have.property('_dd.appsec.user.collection_mode', 'sdk') }).then(done).catch(done) axios.get(`http://localhost:${port}/`) }) @@ -226,6 +228,7 @@ describe('user_blocking', () => { } agent.use(traces => { expect(traces[0][0].meta).to.have.property('usr.id', 'blockedUser') + expect(traces[0][0].meta).to.have.property('_dd.appsec.user.collection_mode', 'sdk') }).then(done).catch(done) axios.get(`http://localhost:${port}/`) }) From 589271d4ba539374eb404aa7ec6859d5fcb16fff Mon Sep 17 00:00:00 2001 From: simon-id Date: Tue, 21 Jan 2025 22:54:13 +0100 Subject: [PATCH 111/126] update appsec index tests --- packages/dd-trace/test/appsec/index.spec.js | 78 ++++++++++++++++++++- 1 file changed, 75 insertions(+), 3 deletions(-) diff --git a/packages/dd-trace/test/appsec/index.spec.js b/packages/dd-trace/test/appsec/index.spec.js index b17f4eeceea..efb98b452e2 100644 --- a/packages/dd-trace/test/appsec/index.spec.js +++ b/packages/dd-trace/test/appsec/index.spec.js @@ -11,6 +11,7 @@ const { incomingHttpRequestStart, incomingHttpRequestEnd, passportVerify, + passportUser, queryParser, nextBodyParsed, nextQueryParsed, @@ -91,7 +92,8 @@ describe('AppSec Index', function () { UserTracking = { setCollectionMode: sinon.stub(), - trackLogin: sinon.stub() + trackLogin: sinon.stub(), + trackUser: sinon.stub() } log = { @@ -176,6 +178,7 @@ describe('AppSec Index', function () { expect(bodyParser.hasSubscribers).to.be.false expect(cookieParser.hasSubscribers).to.be.false expect(passportVerify.hasSubscribers).to.be.false + expect(passportUser.hasSubscribers).to.be.false expect(queryParser.hasSubscribers).to.be.false expect(nextBodyParsed.hasSubscribers).to.be.false expect(nextQueryParsed.hasSubscribers).to.be.false @@ -189,6 +192,7 @@ describe('AppSec Index', function () { expect(bodyParser.hasSubscribers).to.be.true expect(cookieParser.hasSubscribers).to.be.true expect(passportVerify.hasSubscribers).to.be.true + expect(passportUser.hasSubscribers).to.be.true expect(queryParser.hasSubscribers).to.be.true expect(nextBodyParsed.hasSubscribers).to.be.true expect(nextQueryParsed.hasSubscribers).to.be.true @@ -271,6 +275,7 @@ describe('AppSec Index', function () { expect(bodyParser.hasSubscribers).to.be.false expect(cookieParser.hasSubscribers).to.be.false expect(passportVerify.hasSubscribers).to.be.false + expect(passportUser.hasSubscribers).to.be.false expect(queryParser.hasSubscribers).to.be.false expect(nextBodyParsed.hasSubscribers).to.be.false expect(nextQueryParsed.hasSubscribers).to.be.false @@ -818,7 +823,7 @@ describe('AppSec Index', function () { sinon.stub(storage, 'getStore').returns({ req }) }) - it('should block when UserTracking.login() returns action', () => { + it('should block when UserTracking.trackLogin() returns action', () => { UserTracking.trackLogin.returns(resultActions) const abortController = new AbortController() @@ -845,7 +850,7 @@ describe('AppSec Index', function () { expect(res.constructor.prototype.end).to.have.been.called }) - it('should not block when UserTracking.login() returns nothing', () => { + it('should not block when UserTracking.trackLogin() returns nothing', () => { UserTracking.trackLogin.returns(undefined) const abortController = new AbortController() @@ -894,6 +899,73 @@ describe('AppSec Index', function () { }) }) + describe('onPassportDeserializeUser', () => { + beforeEach(() => { + web.root.resetHistory() + sinon.stub(storage, 'getStore').returns({ req }) + }) + + it('should block when UserTracking.trackUser() returns action', () => { + UserTracking.trackUser.returns(resultActions) + + const abortController = new AbortController() + const payload = { + user: { _id: 1, username: 'test', password: '1234' }, + abortController + } + + passportUser.publish(payload) + + expect(storage.getStore).to.have.been.calledOnce + expect(web.root).to.have.been.calledOnceWithExactly(req) + expect(UserTracking.trackUser).to.have.been.calledOnceWithExactly( + payload.user, + rootSpan + ) + expect(abortController.signal.aborted).to.be.true + expect(res.constructor.prototype.end).to.have.been.called + }) + + it('should not block when UserTracking.trackUser() returns nothing', () => { + UserTracking.trackUser.returns(undefined) + + const abortController = new AbortController() + const payload = { + user: { _id: 1, username: 'test', password: '1234' }, + abortController + } + + passportUser.publish(payload) + + expect(storage.getStore).to.have.been.calledOnce + expect(web.root).to.have.been.calledOnceWithExactly(req) + expect(UserTracking.trackUser).to.have.been.calledOnceWithExactly( + payload.user, + rootSpan + ) + expect(abortController.signal.aborted).to.be.false + expect(res.constructor.prototype.end).to.not.have.been.called + }) + + it('should not block and call log if no rootSpan is found', () => { + storage.getStore.returns(undefined) + + const abortController = new AbortController() + const payload = { + user: { _id: 1, username: 'test', password: '1234' }, + abortController + } + + passportUser.publish(payload) + + expect(storage.getStore).to.have.been.calledOnce + expect(log.warn).to.have.been.calledOnceWithExactly('[ASM] No rootSpan found in onPassportDeserializeUser') + expect(UserTracking.trackUser).to.not.have.been.called + expect(abortController.signal.aborted).to.be.false + expect(res.constructor.prototype.end).to.not.have.been.called + }) + }) + describe('onResponseWriteHead', () => { it('should call abortController if response was already blocked', () => { sinon.stub(waf, 'run').returns(resultActions) From 67482db69168db501132c09bfc2e8198eec1b3c0 Mon Sep 17 00:00:00 2001 From: simon-id Date: Tue, 21 Jan 2025 23:00:00 +0100 Subject: [PATCH 112/126] lint --- packages/dd-trace/test/appsec/user_tracking.spec.js | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/packages/dd-trace/test/appsec/user_tracking.spec.js b/packages/dd-trace/test/appsec/user_tracking.spec.js index 4c5745eff69..dfd70f0853c 100644 --- a/packages/dd-trace/test/appsec/user_tracking.spec.js +++ b/packages/dd-trace/test/appsec/user_tracking.spec.js @@ -770,7 +770,11 @@ describe('User Tracking', () => { sinon.assert.notCalled(log.error) sinon.assert.notCalled(telemetry.incrementMissingUserIdMetric) - sinon.assert.calledOnceWithExactly(rootSpan.setTag, '_dd.appsec.usr.id', 'anon_a665a45920422f9d417e4867efdc4fb8') + sinon.assert.calledOnceWithExactly( + rootSpan.setTag, + '_dd.appsec.usr.id', + 'anon_a665a45920422f9d417e4867efdc4fb8' + ) sinon.assert.calledOnceWithExactly(rootSpan.addTags, { 'usr.id': 'anon_a665a45920422f9d417e4867efdc4fb8', '_dd.appsec.user.collection_mode': 'anonymization' @@ -795,7 +799,11 @@ describe('User Tracking', () => { sinon.assert.notCalled(log.error) sinon.assert.notCalled(telemetry.incrementMissingUserIdMetric) - sinon.assert.calledOnceWithExactly(rootSpan.setTag, '_dd.appsec.usr.id', 'anon_a665a45920422f9d417e4867efdc4fb8') + sinon.assert.calledOnceWithExactly( + rootSpan.setTag, + '_dd.appsec.usr.id', + 'anon_a665a45920422f9d417e4867efdc4fb8' + ) sinon.assert.notCalled(rootSpan.addTags) sinon.assert.notCalled(waf.run) From 1fe6b89c11b8ffb68a99acc926d0ee494a125943 Mon Sep 17 00:00:00 2001 From: simon-id Date: Wed, 22 Jan 2025 18:00:28 +0100 Subject: [PATCH 113/126] add passport in ci tests --- .github/workflows/appsec.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/appsec.yml b/.github/workflows/appsec.yml index 2e19b3256f6..9e5799340e9 100644 --- a/.github/workflows/appsec.yml +++ b/.github/workflows/appsec.yml @@ -274,7 +274,7 @@ jobs: passport: runs-on: ubuntu-latest env: - PLUGINS: passport-local|passport-http + PLUGINS: passport-local|passport-http|passport steps: - uses: actions/checkout@v4 - uses: ./.github/actions/node/setup From e600ec0a8a55ff80cf886473d5346efc09090384 Mon Sep 17 00:00:00 2001 From: simon-id Date: Wed, 22 Jan 2025 18:04:41 +0100 Subject: [PATCH 114/126] try to break a test --- packages/datadog-instrumentations/test/passport-http.spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/datadog-instrumentations/test/passport-http.spec.js b/packages/datadog-instrumentations/test/passport-http.spec.js index 5cb0282ec2f..14dafe75a51 100644 --- a/packages/datadog-instrumentations/test/passport-http.spec.js +++ b/packages/datadog-instrumentations/test/passport-http.spec.js @@ -123,7 +123,7 @@ withVersions('passport-http', 'passport-http', version => { } }) - expect(res.status).to.equal(200) + expect(res.status).to.equal(403) // try to break the test expect(res.data).to.equal('Granted') expect(subscriberStub).to.be.calledOnceWithExactly({ framework: 'passport-basic', From f00dfbea8e0ae3328ba8499d150c63e9fd0a56c3 Mon Sep 17 00:00:00 2001 From: simon-id Date: Wed, 22 Jan 2025 18:41:26 +0100 Subject: [PATCH 115/126] try to break another test --- packages/datadog-instrumentations/test/express.spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/datadog-instrumentations/test/express.spec.js b/packages/datadog-instrumentations/test/express.spec.js index 534bfd041e8..18aabd22428 100644 --- a/packages/datadog-instrumentations/test/express.spec.js +++ b/packages/datadog-instrumentations/test/express.spec.js @@ -49,7 +49,7 @@ withVersions('express', 'express', version => { const res = await axios.get(`http://localhost:${port}/`) expect(requestBody).to.be.calledOnce - expect(res.data).to.be.equal('DONE') + expect(res.data).to.be.equal('DO') queryParserReadCh.unsubscribe(noop) }) From 0b15e9bab1ee6e98c8289aa125eb1e3bd1713d10 Mon Sep 17 00:00:00 2001 From: simon-id Date: Thu, 23 Jan 2025 10:05:14 +0100 Subject: [PATCH 116/126] Update passport-http.spec.js --- packages/datadog-instrumentations/test/passport-http.spec.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/datadog-instrumentations/test/passport-http.spec.js b/packages/datadog-instrumentations/test/passport-http.spec.js index 14dafe75a51..b1de4563360 100644 --- a/packages/datadog-instrumentations/test/passport-http.spec.js +++ b/packages/datadog-instrumentations/test/passport-http.spec.js @@ -78,6 +78,8 @@ withVersions('passport-http', 'passport-http', version => { }) ) + throw 'THIS SHOULD FAIL BUT NO' + app.get('/grant', (req, res) => { res.send('Granted') }) From 24328abb2cfd332a0cc05972913950bd8a573c89 Mon Sep 17 00:00:00 2001 From: simon-id Date: Thu, 23 Jan 2025 11:26:40 +0100 Subject: [PATCH 117/126] add express-session in the whitelist of WeakHashAnalyzer --- .../dd-trace/src/appsec/iast/analyzers/weak-hash-analyzer.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/dd-trace/src/appsec/iast/analyzers/weak-hash-analyzer.js b/packages/dd-trace/src/appsec/iast/analyzers/weak-hash-analyzer.js index b7ae6681d00..457e9f0ff74 100644 --- a/packages/dd-trace/src/appsec/iast/analyzers/weak-hash-analyzer.js +++ b/packages/dd-trace/src/appsec/iast/analyzers/weak-hash-analyzer.js @@ -22,7 +22,8 @@ const EXCLUDED_LOCATIONS = getNodeModulesPaths( 'sqreen/lib/package-reader/index.js', 'ws/lib/websocket-server.js', 'google-gax/build/src/grpc.js', - 'cookie-signature/index.js' + 'cookie-signature/index.js', + 'express-session/index.js' ) const EXCLUDED_PATHS_FROM_STACK = [ From 2ee820946000255b7828c7a75b3677a44e9787a1 Mon Sep 17 00:00:00 2001 From: simon-id Date: Fri, 24 Jan 2025 09:55:30 +0100 Subject: [PATCH 118/126] fix passport instrmentation tests not run in CI --- .github/workflows/plugins.yml | 41 +++++++++++++++++++++++++++++++---- 1 file changed, 37 insertions(+), 4 deletions(-) diff --git a/.github/workflows/plugins.yml b/.github/workflows/plugins.yml index 7cffdc3f69b..26b873cd7e7 100644 --- a/.github/workflows/plugins.yml +++ b/.github/workflows/plugins.yml @@ -13,7 +13,6 @@ concurrency: # TODO: upstream jobs - jobs: aerospike: strategy: @@ -48,7 +47,7 @@ jobs: aerospike: image: aerospike:${{ matrix.aerospike-image }} ports: - - "127.0.0.1:3000-3002:3000-3002" + - '127.0.0.1:3000-3002:3000-3002' env: PLUGINS: aerospike SERVICES: aerospike @@ -143,7 +142,7 @@ jobs: localstack-legacy: image: localstack/localstack:1.1.0 ports: - - "127.0.0.1:4567:4567" # Edge + - '127.0.0.1:4567:4567' # Edge env: LOCALSTACK_SERVICES: dynamodb,kinesis,s3,sqs,sns,redshift,route53,logs,serverless EXTRA_CORS_ALLOWED_HEADERS: x-amz-request-id,x-amzn-requestid,x-amz-id-2 @@ -752,7 +751,17 @@ jobs: version: - 18 - latest - range: ['>=10.2.0 <11', '>=11.0.0 <13', '11.1.4', '>=13.0.0 <14', '13.2.0', '>=14.0.0 <=14.2.6', '>=14.2.7 <15', '>=15.0.0'] + range: + [ + '>=10.2.0 <11', + '>=11.0.0 <13', + '11.1.4', + '>=13.0.0 <14', + '13.2.0', + '>=14.0.0 <=14.2.6', + '>=14.2.7 <15', + '>=15.0.0', + ] include: - range: '>=10.2.0 <11' range_clean: gte.10.2.0.and.lt.11 @@ -878,6 +887,30 @@ jobs: suffix: plugins-${{ github.job }} - uses: codecov/codecov-action@v3 + passport: + runs-on: ubuntu-latest + env: + PLUGINS: passport + steps: + - uses: actions/checkout@v4 + - uses: ./.github/actions/plugins/test + + passport-http: + runs-on: ubuntu-latest + env: + PLUGINS: passport-http + steps: + - uses: actions/checkout@v4 + - uses: ./.github/actions/plugins/test + + passport-local: + runs-on: ubuntu-latest + env: + PLUGINS: passport-local + steps: + - uses: actions/checkout@v4 + - uses: ./.github/actions/plugins/test + # TODO: re-enable upstream tests if it ever stops being flaky pino: runs-on: ubuntu-latest From 89b9cfee258af612b4ca5d25f018de647556d7a2 Mon Sep 17 00:00:00 2001 From: simon-id Date: Fri, 24 Jan 2025 15:01:40 +0100 Subject: [PATCH 119/126] revert debug --- packages/datadog-instrumentations/test/express.spec.js | 2 +- packages/datadog-instrumentations/test/passport-http.spec.js | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/packages/datadog-instrumentations/test/express.spec.js b/packages/datadog-instrumentations/test/express.spec.js index 18aabd22428..534bfd041e8 100644 --- a/packages/datadog-instrumentations/test/express.spec.js +++ b/packages/datadog-instrumentations/test/express.spec.js @@ -49,7 +49,7 @@ withVersions('express', 'express', version => { const res = await axios.get(`http://localhost:${port}/`) expect(requestBody).to.be.calledOnce - expect(res.data).to.be.equal('DO') + expect(res.data).to.be.equal('DONE') queryParserReadCh.unsubscribe(noop) }) diff --git a/packages/datadog-instrumentations/test/passport-http.spec.js b/packages/datadog-instrumentations/test/passport-http.spec.js index b1de4563360..5cb0282ec2f 100644 --- a/packages/datadog-instrumentations/test/passport-http.spec.js +++ b/packages/datadog-instrumentations/test/passport-http.spec.js @@ -78,8 +78,6 @@ withVersions('passport-http', 'passport-http', version => { }) ) - throw 'THIS SHOULD FAIL BUT NO' - app.get('/grant', (req, res) => { res.send('Granted') }) @@ -125,7 +123,7 @@ withVersions('passport-http', 'passport-http', version => { } }) - expect(res.status).to.equal(403) // try to break the test + expect(res.status).to.equal(200) expect(res.data).to.equal('Granted') expect(subscriberStub).to.be.calledOnceWithExactly({ framework: 'passport-basic', From 14d6760db2faf4633508986618addb898ceac4c8 Mon Sep 17 00:00:00 2001 From: simon-id Date: Fri, 24 Jan 2025 15:02:23 +0100 Subject: [PATCH 120/126] upload non finished passport test to check CI --- .../test/passport.spec.js | 118 ++++++++++++++++++ 1 file changed, 118 insertions(+) create mode 100644 packages/datadog-instrumentations/test/passport.spec.js diff --git a/packages/datadog-instrumentations/test/passport.spec.js b/packages/datadog-instrumentations/test/passport.spec.js new file mode 100644 index 00000000000..8aeddbaa343 --- /dev/null +++ b/packages/datadog-instrumentations/test/passport.spec.js @@ -0,0 +1,118 @@ +'use strict' + +const agent = require('../../dd-trace/test/plugins/agent') +const axios = require('axios').create({ validateStatus: null }) +const dc = require('dc-polyfill') +const { storage } = require('../../datadog-core') + +const users = [{ + id: 1, + username: 'test', + password: '1234', + email: 'testuser@ddog.com' +}] + +withVersions('passport', 'passport', version => { + describe('passport instrumentation', () => { + const passportDeserializeUserChannel = dc.channel('datadog:passport:deserializeUser:finish') + let port, server, subscriberStub + + before(() => { + return agent.load(['http', 'express', 'express-session', 'passport', 'passport-local'], { client: false }) + }) + + before((done) => { + const express = require('../../../versions/express').get() + const expressSession = require('../../../versions/express-session').get() + const passport = require(`../../../versions/passport@${version}`).get() + const LocalStrategy = require(`../../../versions/passport-local@${version}`).get().Strategy + + const app = express() + app.use(expressSession({ + secret: 'secret', + resave: false, + rolling: true, + saveUninitialized: true + })) + + app.use(passport.initialize()) + app.use(passport.session()) + + passport.serializeUser((user, done) => { + done(null, user.id) + }) + + passport.deserializeUser((id, done) => { + const user = users.find((user) => user.id === id) + + done(null, user) + }) + + passport.use('local', new LocalStrategy((username, password, done) => { + const user = users.find((user) => user.username === username && user.password === password) + + return done(null, user) + })) + + app.get('/', passport.authenticate('local')) + + passportDeserializeUserChannel.subscribe((data) => subscriberStub(data)) + + server = app.listen(0, () => { + port = server.address().port + done() + }) + }) + + beforeEach(async () => { + subscriberStub = sinon.stub() + + const res = await axios.post(`http://localhost:${port}/`, { username: 'test', password: '1234' }) + + console.log(res.headers['set-cookie']) + }) + + after(() => { + server.close() + return agent.close({ ritmReset: false }) + }) + + it('should not call subscriber when an error occurs', async () => { + const res = await axios.post(`http://localhost:${port}/`, { username: 'error', password: '1234' }) + + expect(res.status).to.equal(500) + expect(subscriberStub).to.not.be.called + }) + + 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({ + framework: 'passport-local', + login: 'test', + user: { _id: 1, username: 'test', password: '1234', email: 'testuser@ddog.com' }, + success: true, + abortController: new AbortController() + }) + }) + + it('should block when subscriber aborts', async () => { + subscriberStub = sinon.spy(({ abortController }) => { + storage.getStore().req.res.writeHead(403).end('Blocked') + abortController.abort() + }) + + + const res = await axios.post(`http://localhost:${port}/`, { username: 'test', password: '1234' }) + + expect(res.status).to.equal(403) + expect(res.data).to.equal('Blocked') + expect(subscriberStub).to.be.calledOnceWithExactly({ + user: { _id: 1, username: 'test', password: '1234', email: 'testuser@ddog.com' }, + abortController: new AbortController() + }) + }) + }) +}) From 0310e4039a83c6fa1ef7d1b2fe840557575c457c Mon Sep 17 00:00:00 2001 From: simon-id Date: Fri, 24 Jan 2025 15:57:31 +0100 Subject: [PATCH 121/126] revert autoformat --- .github/workflows/plugins.yml | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/.github/workflows/plugins.yml b/.github/workflows/plugins.yml index 26b873cd7e7..5f4d651ae1c 100644 --- a/.github/workflows/plugins.yml +++ b/.github/workflows/plugins.yml @@ -13,6 +13,7 @@ concurrency: # TODO: upstream jobs + jobs: aerospike: strategy: @@ -47,7 +48,7 @@ jobs: aerospike: image: aerospike:${{ matrix.aerospike-image }} ports: - - '127.0.0.1:3000-3002:3000-3002' + - "127.0.0.1:3000-3002:3000-3002" env: PLUGINS: aerospike SERVICES: aerospike @@ -142,7 +143,7 @@ jobs: localstack-legacy: image: localstack/localstack:1.1.0 ports: - - '127.0.0.1:4567:4567' # Edge + - "127.0.0.1:4567:4567" # Edge env: LOCALSTACK_SERVICES: dynamodb,kinesis,s3,sqs,sns,redshift,route53,logs,serverless EXTRA_CORS_ALLOWED_HEADERS: x-amz-request-id,x-amzn-requestid,x-amz-id-2 @@ -751,17 +752,7 @@ jobs: version: - 18 - latest - range: - [ - '>=10.2.0 <11', - '>=11.0.0 <13', - '11.1.4', - '>=13.0.0 <14', - '13.2.0', - '>=14.0.0 <=14.2.6', - '>=14.2.7 <15', - '>=15.0.0', - ] + range: ['>=10.2.0 <11', '>=11.0.0 <13', '11.1.4', '>=13.0.0 <14', '13.2.0', '>=14.0.0 <=14.2.6', '>=14.2.7 <15', '>=15.0.0'] include: - range: '>=10.2.0 <11' range_clean: gte.10.2.0.and.lt.11 From 0c51a40267040eb00bab2ce4fe8c95b5a39b56fb Mon Sep 17 00:00:00 2001 From: simon-id Date: Fri, 24 Jan 2025 15:57:50 +0100 Subject: [PATCH 122/126] cleanup --- .github/workflows/appsec.yml | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/.github/workflows/appsec.yml b/.github/workflows/appsec.yml index 9e5799340e9..70f26dc903e 100644 --- a/.github/workflows/appsec.yml +++ b/.github/workflows/appsec.yml @@ -210,7 +210,17 @@ jobs: version: - 18 - latest - range: ['>=10.2.0 <11', '>=11.0.0 <13', '11.1.4', '>=13.0.0 <14', '13.2.0', '>=14.0.0 <=14.2.6', '>=14.2.7 <15', '>=15.0.0'] + range: + [ + '>=10.2.0 <11', + '>=11.0.0 <13', + '11.1.4', + '>=13.0.0 <14', + '13.2.0', + '>=14.0.0 <=14.2.6', + '>=14.2.7 <15', + '>=15.0.0', + ] include: - range: '>=10.2.0 <11' range_clean: gte.10.2.0.and.lt.11 @@ -274,7 +284,7 @@ jobs: passport: runs-on: ubuntu-latest env: - PLUGINS: passport-local|passport-http|passport + PLUGINS: passport-local|passport-http steps: - uses: actions/checkout@v4 - uses: ./.github/actions/node/setup From 94a3d1ea1dd5ad72373b5c3e5260520ebd476e35 Mon Sep 17 00:00:00 2001 From: simon-id Date: Sun, 26 Jan 2025 19:09:29 +0100 Subject: [PATCH 123/126] cleanup --- .github/workflows/appsec.yml | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/.github/workflows/appsec.yml b/.github/workflows/appsec.yml index 70f26dc903e..2e19b3256f6 100644 --- a/.github/workflows/appsec.yml +++ b/.github/workflows/appsec.yml @@ -210,17 +210,7 @@ jobs: version: - 18 - latest - range: - [ - '>=10.2.0 <11', - '>=11.0.0 <13', - '11.1.4', - '>=13.0.0 <14', - '13.2.0', - '>=14.0.0 <=14.2.6', - '>=14.2.7 <15', - '>=15.0.0', - ] + range: ['>=10.2.0 <11', '>=11.0.0 <13', '11.1.4', '>=13.0.0 <14', '13.2.0', '>=14.0.0 <=14.2.6', '>=14.2.7 <15', '>=15.0.0'] include: - range: '>=10.2.0 <11' range_clean: gte.10.2.0.and.lt.11 From ae6d2edb5a519529220834442d06f1a5a307dd4b Mon Sep 17 00:00:00 2001 From: simon-id Date: Sun, 26 Jan 2025 23:34:15 +0100 Subject: [PATCH 124/126] fix test file --- .../test/passport.spec.js | 30 ++++++++++++++----- 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/packages/datadog-instrumentations/test/passport.spec.js b/packages/datadog-instrumentations/test/passport.spec.js index 8aeddbaa343..5f10e586fa2 100644 --- a/packages/datadog-instrumentations/test/passport.spec.js +++ b/packages/datadog-instrumentations/test/passport.spec.js @@ -1,21 +1,27 @@ 'use strict' +const { assert } = require('chai') const agent = require('../../dd-trace/test/plugins/agent') const axios = require('axios').create({ validateStatus: null }) const dc = require('dc-polyfill') const { storage } = require('../../datadog-core') const users = [{ - id: 1, + id: 'uuid_42', username: 'test', password: '1234', email: 'testuser@ddog.com' +}, { + id: 'error_user', + username: 'error', + password: '1234', + email: 'a@b.c' }] withVersions('passport', 'passport', version => { describe('passport instrumentation', () => { const passportDeserializeUserChannel = dc.channel('datadog:passport:deserializeUser:finish') - let port, server, subscriberStub + let port, server, subscriberStub, cookie before(() => { return agent.load(['http', 'express', 'express-session', 'passport', 'passport-local'], { client: false }) @@ -25,9 +31,10 @@ withVersions('passport', 'passport', version => { const express = require('../../../versions/express').get() const expressSession = require('../../../versions/express-session').get() const passport = require(`../../../versions/passport@${version}`).get() - const LocalStrategy = require(`../../../versions/passport-local@${version}`).get().Strategy + const LocalStrategy = require('../../../versions/passport-local').get().Strategy const app = express() + app.use(expressSession({ secret: 'secret', resave: false, @@ -43,18 +50,26 @@ withVersions('passport', 'passport', version => { }) passport.deserializeUser((id, done) => { + if (id === 'error_user') { + return done(new Error('*MOCK* Cannot deserialize user')) + } + const user = users.find((user) => user.id === id) done(null, user) }) - passport.use('local', new LocalStrategy((username, password, done) => { + passport.use(new LocalStrategy((username, password, done) => { const user = users.find((user) => user.username === username && user.password === password) return done(null, user) })) - app.get('/', passport.authenticate('local')) + app.get('/login', passport.authenticate('local')) + + app.get('/', (req, res) => { + res.send(req.user?.id) + }) passportDeserializeUserChannel.subscribe((data) => subscriberStub(data)) @@ -67,9 +82,8 @@ withVersions('passport', 'passport', version => { beforeEach(async () => { subscriberStub = sinon.stub() - const res = await axios.post(`http://localhost:${port}/`, { username: 'test', password: '1234' }) - - console.log(res.headers['set-cookie']) + const res = await axios.get(`http://localhost:${port}/login?username=test&password=1234`) + cookie = res.headers['set-cookie'][0] }) after(() => { From b7dedaf8b1ed0166334457c1697ed09f435b4e1b Mon Sep 17 00:00:00 2001 From: simon-id Date: Sun, 26 Jan 2025 23:59:22 +0100 Subject: [PATCH 125/126] fix passport tests --- .../test/passport.spec.js | 76 ++++++++++++------- 1 file changed, 49 insertions(+), 27 deletions(-) diff --git a/packages/datadog-instrumentations/test/passport.spec.js b/packages/datadog-instrumentations/test/passport.spec.js index 5f10e586fa2..7052f72a5fc 100644 --- a/packages/datadog-instrumentations/test/passport.spec.js +++ b/packages/datadog-instrumentations/test/passport.spec.js @@ -6,22 +6,29 @@ const axios = require('axios').create({ validateStatus: null }) const dc = require('dc-polyfill') const { storage } = require('../../datadog-core') -const users = [{ - id: 'uuid_42', - username: 'test', - password: '1234', - email: 'testuser@ddog.com' -}, { - id: 'error_user', - username: 'error', - password: '1234', - email: 'a@b.c' -}] +const users = [ + { + id: 'error_user', + username: 'error', + password: '1234', + email: 'a@b.c' + }, { + id: 'notfound_user', + username: 'notfound', + password: '1234', + email: 'a@b.c' + }, { + id: 'uuid_42', + username: 'test', + password: '1234', + email: 'testuser@ddog.com' + } +] withVersions('passport', 'passport', version => { describe('passport instrumentation', () => { const passportDeserializeUserChannel = dc.channel('datadog:passport:deserializeUser:finish') - let port, server, subscriberStub, cookie + let port, server, subscriberStub before(() => { return agent.load(['http', 'express', 'express-session', 'passport', 'passport-local'], { client: false }) @@ -54,6 +61,10 @@ withVersions('passport', 'passport', version => { return done(new Error('*MOCK* Cannot deserialize user')) } + if (id === 'notfound_user') { + return done(null, false) + } + const user = users.find((user) => user.id === id) done(null, user) @@ -81,9 +92,6 @@ withVersions('passport', 'passport', version => { beforeEach(async () => { subscriberStub = sinon.stub() - - const res = await axios.get(`http://localhost:${port}/login?username=test&password=1234`) - cookie = res.headers['set-cookie'][0] }) after(() => { @@ -92,22 +100,36 @@ withVersions('passport', 'passport', version => { }) it('should not call subscriber when an error occurs', async () => { - const res = await axios.post(`http://localhost:${port}/`, { username: 'error', password: '1234' }) + const login = await axios.get(`http://localhost:${port}/login?username=error&password=1234`) + const cookie = login.headers['set-cookie'][0] - expect(res.status).to.equal(500) - expect(subscriberStub).to.not.be.called + const res = await axios.get(`http://localhost:${port}/`, { headers: { cookie } }) + + assert.strictEqual(res.status, 500) + sinon.assert.notCalled(subscriberStub) }) - it('should call subscriber with proper arguments on success', async () => { - const res = await axios.post(`http://localhost:${port}/`, { username: 'test', password: '1234' }) + it('should not call subscriber when no user is found', async () => { + const login = await axios.get(`http://localhost:${port}/login?username=notfound&password=1234`) + const cookie = login.headers['set-cookie'][0] - expect(res.status).to.equal(200) - expect(res.data).to.equal('Granted') - expect(subscriberStub).to.be.calledOnceWithExactly({ - framework: 'passport-local', - login: 'test', - user: { _id: 1, username: 'test', password: '1234', email: 'testuser@ddog.com' }, - success: true, + const res = await axios.get(`http://localhost:${port}/`, { headers: { cookie } }) + + assert.strictEqual(res.status, 200) + assert.strictEqual(res.data, '') + sinon.assert.notCalled(subscriberStub) + }) + + it('should call subscriber with proper arguments on user deserialize', async () => { + const login = await axios.get(`http://localhost:${port}/login?username=test&password=1234`) + const cookie = login.headers['set-cookie'][0] + + const res = await axios.get(`http://localhost:${port}/`, { headers: { cookie } }) + + assert.strictEqual(res.status, 200) + assert.strictEqual(res.data, 'uuid_42') + sinon.assert.calledOnceWithExactly(subscriberStub, { + user: { id: 'uuid_42', username: 'test', password: '1234', email: 'testuser@ddog.com' }, abortController: new AbortController() }) }) From d02ed23553aa886fa5d1fd2d2778acf71d4d68c6 Mon Sep 17 00:00:00 2001 From: simon-id Date: Mon, 27 Jan 2025 00:11:50 +0100 Subject: [PATCH 126/126] no need for any plugin in passport tests --- packages/datadog-instrumentations/test/passport.spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/datadog-instrumentations/test/passport.spec.js b/packages/datadog-instrumentations/test/passport.spec.js index 7052f72a5fc..a4d4c30a56f 100644 --- a/packages/datadog-instrumentations/test/passport.spec.js +++ b/packages/datadog-instrumentations/test/passport.spec.js @@ -31,7 +31,7 @@ withVersions('passport', 'passport', version => { let port, server, subscriberStub before(() => { - return agent.load(['http', 'express', 'express-session', 'passport', 'passport-local'], { client: false }) + return agent.load([], { client: false }) }) before((done) => {