Suspicious request blocking - Express Path Parameters #4769

CarlesDD · 2024-10-11T06:45:27Z

What does this PR do?

Subscribe to express process_param in order to report express path parameters to the WAF and block malicious incoming requests.

Motivation

To extend suspicious request blocking feature, blocking malicious payloads coming in express path parameters.

Plugin Checklist

Unit tests.

Additional Notes

Based on @simon-id previous work #3666

System Test PR

APPSEC-8367

github-actions · 2024-10-11T06:46:11Z

Overall package size

Self size: 7.48 MB
Deduped: 63.25 MB
No deduping: 63.53 MB

Dependency sizes

| name | version | self size | total size | |------|---------|-----------|------------| | @datadog/native-appsec | 8.1.1 | 18.67 MB | 18.68 MB | | @datadog/native-iast-taint-tracking | 3.1.0 | 12.27 MB | 12.28 MB | | @datadog/pprof | 5.3.0 | 9.85 MB | 10.22 MB | | protobufjs | 7.2.5 | 2.77 MB | 5.16 MB | | @datadog/native-iast-rewriter | 2.5.0 | 2.51 MB | 2.59 MB | | @opentelemetry/core | 1.14.0 | 872.87 kB | 1.47 MB | | @datadog/native-metrics | 2.0.0 | 898.77 kB | 1.3 MB | | @opentelemetry/api | 1.8.0 | 1.21 MB | 1.21 MB | | jsonpath-plus | 9.0.0 | 580.4 kB | 1.03 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 | | 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 | | lru-cache | 7.14.0 | 74.95 kB | 74.95 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 | | 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}

CarlesDD · 2024-10-11T07:14:30Z

packages/dd-trace/test/appsec/index.express.plugin.spec.js

Wdyt about splitting this file into supsicious-request-blocking.express.plugin.spec.js and api-security.express.plugin.spec.js ?

Yes, this file is getting longer and longer... We could separate this in two files, but should we also split the src/appsec/index.js in different files?

Do you mean packages/dd-trace/test/appsec/index.spec.js? If so, I'd not go with it, since it contains unit test for src/appsec/index.js so, to me, it makes sense to mirror the filenames.

No, I am not talking about test file, I am talking about appsec/index.js source file.
But anyway, a refactor shouldn't be part of this PR. So not to implement here.

codecov · 2024-10-11T07:16:42Z

Codecov Report

Attention: Patch coverage is 0% with 11 lines in your changes missing coverage. Please review.

Project coverage is 70.15%. Comparing base (7f93d36) to head (2b5543c).
Report is 9 commits behind head on master.

Files with missing lines	Patch %	Lines
packages/dd-trace/src/appsec/index.js	0.00%	7 Missing ⚠️
packages/datadog-instrumentations/src/express.js	0.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4769      +/-   ##
==========================================
+ Coverage   69.19%   70.15%   +0.96%     
==========================================
  Files           1      307     +306     
  Lines         198    12992   +12794     
  Branches       33        0      -33     
==========================================
+ Hits          137     9115    +8978     
- Misses         61     3877    +3816

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pr-commenter · 2024-10-11T07:22:08Z

Benchmarks

Benchmark execution time: 2024-10-14 14:07:53

Comparing candidate commit c545063 in PR branch ccapell/path-params-blocking with baseline commit f62cbfa in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 260 metrics, 6 unstable metrics.

simon-id · 2024-10-11T08:46:09Z

packages/datadog-instrumentations/src/express.js

@@ -106,7 +106,17 @@ const wrapProcessParamsMethod = (requestPositionInArguments) => {
  return (original) => {


Can we make wrapProcessParamsMethod a non arrow function, and add names to the functions under that ? it's easier to reason about with actual function names.
Usually it goes like
wrapProcessParams()
-> wrappedProcessParams()
-> -> processParamsCallback()
or something like that

simon-id · 2024-10-11T08:48:22Z

packages/dd-trace/src/appsec/index.js

@@ -6,6 +6,7 @@ const remoteConfig = require('./remote_config')
 const {
  bodyParser,
  cookieParser,
+  expressProcessParams,


can we try to keep consistency of order between channel.js, appsec/index.js require, subscribe, handler, and unsubscribe ? just so it's not all over the place and it's easier to find a specific one.

simon-id · 2024-10-11T08:51:27Z

packages/dd-trace/src/appsec/index.js

+  const rootSpan = web.root(req)
+  if (!rootSpan) return
+
+  if (!params || typeof params !== 'object' || !Object.keys(params).length) return


is this the only place where we have Object.keys check ? why is that

In the other site - TaintTrackingPlugin- , the key length check is done (indirectly) later on, in the subscriber handler (taintObject method).

Here, this is done to avoid an unnecessary call to the WAF.

uurien · 2024-10-11T11:42:00Z

packages/dd-trace/test/appsec/index.express.plugin.spec.js

-          expect(e.response.status).to.be.equals(403)
-          expect(e.response.data).to.be.deep.equal(JSON.parse(json))
-          expect(requestBody).not.to.be.called
+          assert.equal(e.response.status, 403)


😮 you replaced all the expect by assert in this file, thanks for it :-D

uurien

minor comments, related with test spacing, optional to fix. But I like when our tests have this spacing:

it('test', () => {
  code.preparing.the.test()

  execute.the.test()

  assert(thistest).hasEmptyLinesAfterPreparationAndAfterExecution()
})

packages/dd-trace/test/appsec/index.express.plugin.spec.js

Co-authored-by: Ugaitz Urien <[email protected]>

CarlesDD self-assigned this Oct 11, 2024

CarlesDD added asm-waf semver-minor appsec labels Oct 11, 2024

CarlesDD commented Oct 11, 2024

View reviewed changes

CarlesDD marked this pull request as ready for review October 11, 2024 07:26

CarlesDD requested review from a team as code owners October 11, 2024 07:26

CarlesDD mentioned this pull request Oct 11, 2024

Path_params_blocking #3666

Closed

6 tasks

simon-id reviewed Oct 11, 2024

View reviewed changes

uurien reviewed Oct 11, 2024

View reviewed changes

packages/dd-trace/test/appsec/index.express.plugin.spec.js Show resolved Hide resolved

packages/dd-trace/test/appsec/index.express.plugin.spec.js Show resolved Hide resolved

packages/dd-trace/test/appsec/index.express.plugin.spec.js Show resolved Hide resolved

CarlesDD mentioned this pull request Oct 14, 2024

Unsubscribe NextJS body and query channels on appsec disable #4776

Merged

CarlesDD and others added 10 commits October 14, 2024 15:50

Path Parameters blocking

1cd8d76

Lint

b849955

Change expect to assert in SRB tests

bf7d4e6

Change expect to assert in API Sec tests

ac5fc5c

Improve test naming

4dc37bd

Correct spacing in tests

506a2bd

Co-authored-by: Ugaitz Urien <[email protected]>

Keep consistency of order in appsec channels

b5b549a

Better wrap fn naming in express instrumentation

0e85c94

Keep consistency of order in appsec channels handlers

80cebff

Keep consistency of order in appsec channels handlers - test

c545063

CarlesDD force-pushed the ccapell/path-params-blocking branch from 49fb545 to c545063 Compare October 14, 2024 13:54

CarlesDD requested review from uurien and simon-id October 14, 2024 14:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Suspicious request blocking - Express Path Parameters #4769

Suspicious request blocking - Express Path Parameters #4769

CarlesDD commented Oct 11, 2024 •

edited

Loading

github-actions bot commented Oct 11, 2024 •

edited

Loading

CarlesDD Oct 11, 2024

uurien Oct 11, 2024

CarlesDD Oct 11, 2024

uurien Oct 11, 2024

codecov bot commented Oct 11, 2024 •

edited

Loading

pr-commenter bot commented Oct 11, 2024 •

edited

Loading

simon-id Oct 11, 2024

CarlesDD Oct 14, 2024

simon-id Oct 11, 2024

CarlesDD Oct 14, 2024

simon-id Oct 11, 2024

CarlesDD Oct 14, 2024 •

edited

Loading

uurien Oct 11, 2024

uurien left a comment

		@@ -106,7 +106,17 @@ const wrapProcessParamsMethod = (requestPositionInArguments) => {
		return (original) => {

Suspicious request blocking - Express Path Parameters #4769

Are you sure you want to change the base?

Suspicious request blocking - Express Path Parameters #4769

Conversation

CarlesDD commented Oct 11, 2024 • edited Loading

What does this PR do?

Motivation

Plugin Checklist

Additional Notes

github-actions bot commented Oct 11, 2024 • edited Loading

Overall package size

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 11, 2024 • edited Loading

Codecov Report

pr-commenter bot commented Oct 11, 2024 • edited Loading

Benchmarks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarlesDD Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uurien left a comment

Choose a reason for hiding this comment

CarlesDD commented Oct 11, 2024 •

edited

Loading

github-actions bot commented Oct 11, 2024 •

edited

Loading

codecov bot commented Oct 11, 2024 •

edited

Loading

pr-commenter bot commented Oct 11, 2024 •

edited

Loading

CarlesDD Oct 14, 2024 •

edited

Loading