Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Express 5 Instrumentation #4913

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from
Draft

Express 5 Instrumentation #4913

wants to merge 10 commits into from
+551 −349

Conversation

IlyasShabi
Copy link
Contributor

@IlyasShabi IlyasShabi commented Nov 20, 2024 •
edited
Loading

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

  • Path params: Instrumente handleRequest in Layer class in router library
  • Query string: Directly instrumente query getter in express
  • Response: Added instrumentation for json, jsonp, and render in response.js
  • Route: Instrumente router prototype for use and route methods
  • Enable support for tests across different Express versions
  • Skip Path regex for Express5 since they are not supported
  • Implement request blocking exception handling (to discuss)
  • Fix router params callback tests
  • Fix RASP lfi and ssrf tests (not related to Express 🤔 )

IlyasShabi
IlyasShabi commented Nov 20, 2024
View reviewed changes
packages/datadog-instrumentations/src/express.js
return express
})

addHook({ name: 'express', file: ['lib/response.js'], versions: ['>=5.0.0'] }, response => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can also do it for express 4 with same hook I think

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 20, 2024 •
edited
Loading

Overall package size

Self size: 8.1 MB
Deduped: 94.61 MB
No deduping: 94.94 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 | | 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

IlyasShabi
IlyasShabi commented Nov 20, 2024
View reviewed changes
packages/datadog-instrumentations/src/router.js
@@ -27,6 +27,9 @@ function createWrapRouterMethod (name) {
const req = arguments[arguments.length > 3 ? 1 : 0]
const next = arguments[lastIndex]

// explicitly call getter for express5
req.query
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Express5, req.query is not parsed until it is accessed, which can delay sending it to the WAF. This might result in blocking the request too late, after headers are already sent.

To fix this, explicitly call req.query at the start of the request to ensure it is parsed and sent to the WAF early.

IlyasShabi
IlyasShabi commented Nov 20, 2024
View reviewed changes
packages/dd-trace/src/appsec/iast/taint-tracking/plugin.js
this.addSub(
{ channelName: 'datadog:query:read:finish', tag: HTTP_REQUEST_QUERY },
({ query }) => {
if (query !== null && typeof query === 'object') {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always use query directly instead of req.query to avoid an infinite loop

IlyasShabi
IlyasShabi commented Nov 20, 2024
View reviewed changes
packages/dd-trace/test/appsec/index.express.plugin.spec.js
@@ -177,6 +175,7 @@ withVersions('express', 'express', version => {
} catch (e) {
assert.equal(e.response.status, 403)
assert.deepEqual(e.response.data, JSON.parse(json))
// to be fixed
sinon.assert.notCalled(paramCallbackSpy)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is related to blocking request

@IlyasShabi IlyasShabi changed the title Express5 Express 5 Instrumentation Nov 20, 2024
CarlesDD
CarlesDD reviewed Nov 20, 2024
View reviewed changes
Copy link
Contributor

@CarlesDD CarlesDD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is worth reviewing this and optimizing it to avoid invoking the query getter 3 times.

@IlyasShabi
Copy link
Contributor Author

Yes I forgot to push this change too @CarlesDD

@codecov Codecov
Copy link

codecov bot commented Nov 21, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 33.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 50.82%. Comparing base (0b4dab7) to head (efd2f71).
Report is 38 commits behind head on master.

Files with missing lines Patch % Lines
.../dd-trace/src/appsec/iast/taint-tracking/plugin.js 33.33% 2 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

@pr-commenter
Copy link

pr-commenter bot commented Nov 21, 2024 •
edited
Loading

Benchmarks

Benchmark execution time: 2024-11-22 09:09:39

Comparing candidate commit 11d95b3 in PR branch express5 with baseline commit b81d9d8 in branch master.

Found 1 performance improvements and 0 performance regressions! Performance is the same for 261 metrics, 4 unstable metrics.

scenario:plugin-graphql-with-async-hooks-18

  • 🟩 max_rss_usage [-108.168MB; -82.492MB] or [-17.351%; -13.232%]

@IlyasShabi IlyasShabi force-pushed the express5 branch 2 times, most recently from 4f721d7 to 11d95b3 Compare November 22, 2024 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CarlesDD CarlesDD CarlesDD left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
integration-express semver-minor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@IlyasShabi @CarlesDD @wconti27