Express 5 Instrumentation #4913
Conversation
Overall package sizeSelf size: 8.12 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | @datadog/libdatadog | 0.2.2 | 29.27 MB | 29.27 MB | | @datadog/native-appsec | 8.3.0 | 19.37 MB | 19.38 MB | | @datadog/native-iast-taint-tracking | 3.2.0 | 13.9 MB | 13.91 MB | | @datadog/pprof | 5.4.1 | 9.76 MB | 10.13 MB | | protobufjs | 7.2.5 | 2.77 MB | 5.16 MB | | @datadog/native-iast-rewriter | 2.5.0 | 2.51 MB | 2.65 MB | | @opentelemetry/core | 1.14.0 | 872.87 kB | 1.47 MB | | @datadog/native-metrics | 3.0.1 | 1.06 MB | 1.46 MB | | @opentelemetry/api | 1.8.0 | 1.21 MB | 1.21 MB | | import-in-the-middle | 1.11.2 | 112.74 kB | 826.22 kB | | msgpack-lite | 0.1.26 | 201.16 kB | 281.59 kB | | source-map | 0.7.4 | 226 kB | 226 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | lru-cache | 7.18.3 | 133.92 kB | 133.92 kB | | pprof-format | 2.1.0 | 111.69 kB | 111.69 kB | | @datadog/sketches-js | 2.1.0 | 109.9 kB | 109.9 kB | | semver | 7.6.3 | 95.82 kB | 95.82 kB | | lodash.sortby | 4.7.0 | 75.76 kB | 75.76 kB | | ignore | 5.3.1 | 51.46 kB | 51.46 kB | | int64-buffer | 0.1.10 | 49.18 kB | 49.18 kB | | shell-quote | 1.8.1 | 44.96 kB | 44.96 kB | | istanbul-lib-coverage | 3.2.0 | 29.34 kB | 29.34 kB | | rfdc | 1.3.1 | 25.21 kB | 25.21 kB | | @isaacs/ttlcache | 1.4.1 | 25.2 kB | 25.2 kB | | tlhunter-sorted-set | 0.1.0 | 24.94 kB | 24.94 kB | | limiter | 1.1.5 | 23.17 kB | 23.17 kB | | dc-polyfill | 0.1.4 | 23.1 kB | 23.1 kB | | retry | 0.13.1 | 18.85 kB | 18.85 kB | | jest-docblock | 29.7.0 | 8.99 kB | 12.76 kB | | crypto-randomuuid | 1.0.0 | 11.18 kB | 11.18 kB | | koalas | 1.0.2 | 6.47 kB | 6.47 kB | | path-to-regexp | 0.1.10 | 6.38 kB | 6.38 kB | | module-details-from-path | 1.0.3 | 4.47 kB | 4.47 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
packages/datadog-plugin-express/test/integration-test/client.spec.js
Outdated
Show resolved
Hide resolved
|
Yes I forgot to push this change too @CarlesDD |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4913 +/- ##
===========================================
- Coverage 93.86% 50.82% -43.04%
===========================================
Files 107 94 -13
Lines 3373 2601 -772
===========================================
- Hits 3166 1322 -1844
- Misses 207 1279 +1072 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
IlyasShabi
force-pushed
the
express5
branch
4 times, most recently
from
11d95b3
to
d33e0be
Compare
IlyasShabi
force-pushed
the
express5
branch
2 times, most recently
from
0e039c8
to
22aa8c6
Compare
IlyasShabi
force-pushed
the
express5
branch
2 times, most recently
from
27566d4
to
ef71918
Compare
| @@ -98,7 +98,7 @@ | |||
| }, | |||
| { | |||
| "name": "express", | |||
| "versions": [">=4", ">=4.0.0 <4.3.0", ">=4.3.0"] | |||
| "versions": [">=4", ">=4.0.0 <4.3.0", ">=4.0.0 <5.0.0", ">=4.3.0 <5.0.0"] | |||
There was a problem hiding this comment.
| const abortController = new AbortController() | ||
|
|
||
| queryParserReadCh.publish({ req: this, res: this.res, query, abortController }) | ||
|
|
||
| if (abortController.signal.aborted) return |
There was a problem hiding this comment.
This is the right place to make iast work, because we need to taint the values each time the request is called, but is this the right place to use the abortController and call to the waf? Calling to the waf each time that req.query code is in the app, could be quite expensive.
There was a problem hiding this comment.
technically the parser can change during the middle of the request maybe ? not sure the edge case is worth the perf decrease
There was a problem hiding this comment.
yeah maybe the best thing to do would be to just remove the waf call here, and instead call it in the router handle request hook
There was a problem hiding this comment.
I agree with you about how expensive this operation could be, but just to clarify:
- In express v4, we don’t taint the query string using express’ query method but instead rely on the
qsparse function. - IMO, we should harmonize how we taint query strings between express v4 and express v5. This could be achieved either by using both
qsandquerystring(though this might lead to false positives since we can’t confirm if the source is from express) or by keeping the getter instrumentation here and using it for IAST, as you suggested.
We could also apply the same approach to express v4 and then (remove ?) theqsandquerystringinstrumentation altogether.
There was a problem hiding this comment.
Definitely, we can remove the qs instrumentation to do it in the same way in express v4 and v5.
I can't recall why are we instrumenting the qs and not reading the query in other position of the code, I think that we can move all to express instrumentation.
Not necessarily in this PR, if you see that could be done easily, go ahead, but we can change express v4 stuff in other PR.
| const shimmer = require('../../datadog-shimmer') | ||
| const names = ['querystring', 'node:querystring'] | ||
|
|
||
| const querystringParseCh = channel('datadog:querystring:parse:finish') |
There was a problem hiding this comment.
why do we need this? isn't this redundant?
We have instrumentation for req.query getter in express, why do we need to instrument this?
There was a problem hiding this comment.
I was thinking maybe for IAST, but if you're also confused then idk hahaha
There was a problem hiding this comment.
This test fails because in express v4 queries are read directly via query middleware, and IAST subscribes to this event here.
In express v5, query parsing uses both qs (same as express v4) and querystring (code here), so we need to instrument querystring to ensure the same functionality.
| req.query | ||
| // If query parsing was aborted, don't continue request handling | ||
| if (res.writableEnded) { | ||
| return | ||
| } |
There was a problem hiding this comment.
this is a bit hacky and feature related, I think that we should send a custom event in diagnostic_channels in this point, call to the waf and use the AbortController to decide if we should continue or not.
| return router | ||
| } | ||
|
|
||
| WrappedRouter.prototype = originalRouter.prototype |
There was a problem hiding this comment.
shimmer.wrapFunction should deal with these things.
| addHook({ name: 'express', file: ['lib/response.js'], versions: ['>=5.0.0'] }, response => { | ||
| shimmer.wrap(response, 'json', wrapResponseJson) | ||
| shimmer.wrap(response, 'jsonp', wrapResponseJson) | ||
| shimmer.wrap(response, 'render', wrapResponseRender) | ||
|
|
||
| return response | ||
| }) |
There was a problem hiding this comment.
why is this change needed ? express branch v5.x still has the proto exposed no ?
https://github.com/expressjs/express/blob/5.x/lib/express.js#L63-L64
and unrelated but yes i think that would work for express 4 too.
| shimmer.wrap(express.Router, 'process_params', wrapProcessParamsMethod(2)) | ||
| return express | ||
| }) | ||
|
|
||
| addHook({ name: 'express', file: ['lib/express.js'], versions: ['>=5.0.0'] }, express => { |
There was a problem hiding this comment.
a bit above you're hooking on express index, and here on lib/express.js
But the express index is just a one line that re-exports lib/express.js, so why not merge those two hooks ?
| const shimmer = require('../../datadog-shimmer') | ||
| const names = ['querystring', 'node:querystring'] | ||
|
|
||
| const querystringParseCh = channel('datadog:querystring:parse:finish') |
There was a problem hiding this comment.
I was thinking maybe for IAST, but if you're also confused then idk hahaha
| shimmer.wrapFunction(express, function (original) { | ||
| const app = original.call(this, arguments) | ||
| const requestProto = Object.getPrototypeOf(app.request) | ||
| const requestDescriptor = Object.getOwnPropertyDescriptor(requestProto, 'query') |
There was a problem hiding this comment.
this doesn't work ?
| const requestDescriptor = Object.getOwnPropertyDescriptor(requestProto, 'query') | |
| const requestDescriptor = Object.getOwnPropertyDescriptor(express.request.prototype, 'query') |
There was a problem hiding this comment.
I don't think this gonna work, we need to execute createApplication first in order to have an app object here
There was a problem hiding this comment.
we tried it together and apparently it works 🤷
| return Layer | ||
| }) | ||
|
|
||
| function wrapParam (original) { |
There was a problem hiding this comment.
why do you need two hooks for params ?
There was a problem hiding this comment.
We've tried together and apparently only one i needed 👍 make sure to double check there is no edge case
| return function wrappedMethod (options) { | ||
| const router = originalRouter.call(this, options) | ||
|
|
||
| if (!router._queryParsingWrapped) { |
There was a problem hiding this comment.
Do not modify router object, use weakSet instead
But first check if it's needed
There was a problem hiding this comment.
@uurien after checking express 5 codebase, we do call handle function only once not at each use(...)
I removed _queryParsingWrapped and seems to be good
packages/dd-trace/test/appsec/iast/taint-tracking/sources/taint-tracking.express.plugin.spec.js
Outdated
Show resolved
Hide resolved
What does this PR do?
This PR adds instrumentation for Express5 to support its new features and behavior. Some related changes were previously made by @wconti27 please check their commits for reference.
Checklist
json,jsonp, andrenderinresponse.jsuseandroutemethodsInstrumente Node.js querystring used by express to parse query params