Update blocking page and status from RC or rules file by uurien · Pull Request #3195 · DataDog/dd-trace-js

uurien · 2023-05-29T12:47:54Z

What does this PR do?

Make it possible for the customers to define the status code or the redirect url for the blocking page.

If they are defined in the custom rules file, the tracer will start to take the action defined there.

Plugin Checklist

Unit tests.

github-actions · 2023-05-29T12:48:52Z

Overall package size

Self size: 4.22 MB
Deduped: 58.42 MB
No deduping: 58.46 MB

Dependency sizes

name	version	self size	total size
@datadog/pprof	2.2.1	14.24 MB	15.12 MB
@datadog/native-iast-taint-tracking	1.4.1	14.85 MB	14.86 MB
@datadog/native-appsec	3.2.0	13.38 MB	13.39 MB
protobufjs	7.1.2	2.76 MB	6.55 MB
@datadog/native-iast-rewriter	2.0.1	2.09 MB	2.1 MB
@datadog/native-metrics	2.0.0	898.77 kB	1.3 MB
opentracing	0.14.7	194.81 kB	194.81 kB
semver	7.3.8	88.2 kB	118.6 kB
@datadog/sketches-js	2.1.0	109.9 kB	109.9 kB
lodash.sortby	4.7.0	75.76 kB	75.76 kB
lru-cache	7.14.0	74.95 kB	74.95 kB
ipaddr.js	2.0.1	59.52 kB	59.52 kB
ignore	5.2.0	48.87 kB	48.87 kB
import-in-the-middle	1.3.5	34.34 kB	38.81 kB
istanbul-lib-coverage	3.2.0	29.34 kB	29.34 kB
retry	0.10.1	27.44 kB	27.44 kB
lodash.uniq	4.5.0	25.01 kB	25.01 kB
limiter	1.1.5	23.17 kB	23.17 kB
lodash.kebabcase	4.1.1	17.75 kB	17.75 kB
lodash.pick	4.4.0	16.33 kB	16.33 kB
node-abort-controller	3.0.1	14.33 kB	14.33 kB
crypto-randomuuid	1.0.0	11.18 kB	11.18 kB
diagnostics_channel	1.1.0	7.07 kB	7.07 kB
path-to-regexp	0.1.7	6.78 kB	6.78 kB
koalas	1.0.2	6.47 kB	6.47 kB
methods	1.1.2	5.29 kB	5.29 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}

codecov · 2023-05-29T12:50:18Z

Codecov Report

Merging #3195 (c723fe8) into master (40157d3) will increase coverage by 0.09%.
The diff coverage is 97.95%.

@@            Coverage Diff             @@
##           master    #3195      +/-   ##
==========================================
+ Coverage   86.49%   86.59%   +0.09%     
==========================================
  Files         337      337              
  Lines       12020    12059      +39     
  Branches       33       33              
==========================================
+ Hits        10397    10442      +45     
+ Misses       1623     1617       -6

Impacted Files	Coverage Δ
.../dd-trace/src/appsec/remote_config/capabilities.js	`100.00% <ø> (ø)`
packages/dd-trace/src/appsec/blocking.js	`97.72% <96.42%> (-2.28%)`	⬇️
...ackages/dd-trace/src/appsec/remote_config/index.js	`100.00% <100.00%> (ø)`
packages/dd-trace/src/appsec/rule_manager.js	`95.58% <100.00%> (+4.06%)`	⬆️

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

pr-commenter · 2023-05-29T13:04:26Z

Benchmarks

Comparing candidate commit c723fe8 in PR branch ugaitz/rc-custom-blocking with baseline commit 40157d3 in branch master.

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

* Update blocking page and status from RC or rules file * Use if/else instead of return * Code styles * Split block in two methods * Fix test * Unapply after test * Fix tests * Reorder params in method * Change the signature of updateBlockingConfiguration method * Clear blocking configuration on clear rules * Update blocking response type by configuration * Fix lint

simon-id · 2023-09-15T15:13:06Z

  }
 }

+function updateBlockingConfiguration (newBlockingConfiguration) {


why is this called update... when there is a function above called set...

simon-id · 2023-09-25T09:37:14Z

+  if (blockingConfiguration && blockingConfiguration.type === 'block_request' &&
+    blockingConfiguration.parameters.status_code) {
+    res.statusCode = blockingConfiguration.parameters.status_code


all of the complicated blockingConfiguration checks should be done in updateBlockingConfiguration() that would compile the variables neccesary only when updated, not in every request. You don't need to check that blockingConfiguration.type is block_request in EVERY request do you ?

simon-id · 2023-09-25T13:18:01Z

+  if (!blockingConfiguration || blockingConfiguration.parameters.type === 'auto') {
+    if (accept && accept.includes('text/html') && !accept.includes('application/json')) {
+      type = 'text/html; charset=utf-8'
+      body = templateHtml
+    } else {
+      type = 'application/json'
+      body = templateJson
+    }
  } else {
-    type = 'application/json'
-    body = templateJson
+    if (blockingConfiguration.parameters.type === 'html') {
+      type = 'text/html; charset=utf-8'
+      body = templateHtml
+    } else {
+      type = 'application/json'
+      body = templateJson
+    }
  }


This whole block should be rewritten to be clearer tbh

simon-id · 2023-09-25T13:43:44Z

+
+  if (newActions.modified) {
+    blocking.updateBlockingConfiguration(concatArrays(newActions).find(action => action.id === 'block'))
+    appliedActions = newActions
+  }


you still need to apply the new applyState here since it's not done by the batch thing above.
Also wouldn't updating actions when the waf fails cause synchronisation problems ? waf rules are referencing an action, so if you update the actions but not the waf rules then you might end of with an inconsistent state no ?

I don't think so.
actions thing is not directly related with the WAF, the waf result would be the same, block or not to block, but the action to do with the result is different.
We don't need to say anything to the waf, we just need to change the behaviour of the blocking.
block with redirect, with custome message, with different status code etc.

you still need to apply the new applyState here since it's not done by the batch thing above.

uurien added asm-waf semver-minor labels May 29, 2023

uurien marked this pull request as ready for review May 29, 2023 14:23

uurien requested a review from a team as a code owner May 29, 2023 14:23

uurien force-pushed the ugaitz/rc-custom-blocking branch from 2a43d97 to a7eaaa9 Compare May 29, 2023 14:24

iunanua reviewed May 30, 2023

View reviewed changes

Comment thread packages/dd-trace/src/appsec/rule_manager.js Outdated

iunanua reviewed May 30, 2023

View reviewed changes

Comment thread packages/dd-trace/src/appsec/blocking.js Outdated

iunanua reviewed May 30, 2023

View reviewed changes

Comment thread packages/dd-trace/src/appsec/blocking.js Outdated

iunanua reviewed May 30, 2023

View reviewed changes

Comment thread packages/dd-trace/src/appsec/rule_manager.js Outdated

Ugaitz Urien added 12 commits June 1, 2023 08:46

Update blocking page and status from RC or rules file

157c579

Use if/else instead of return

6651a1c

Code styles

4d48e21

Split block in two methods

b1ed585

Fix test

ff7ba05

Unapply after test

ee253b1

Fix tests

90aa75a

Reorder params in method

e4fd9f5

Change the signature of updateBlockingConfiguration method

eb006ef

Clear blocking configuration on clear rules

c86f636

Update blocking response type by configuration

c5f0b00

Fix lint

c723fe8

uurien force-pushed the ugaitz/rc-custom-blocking branch from bc71599 to c723fe8 Compare June 1, 2023 06:47

hoolioh approved these changes Jun 1, 2023

View reviewed changes

iunanua approved these changes Jun 1, 2023

View reviewed changes

uurien merged commit bf2d95d into master Jun 1, 2023

uurien deleted the ugaitz/rc-custom-blocking branch June 1, 2023 07:34

This was referenced Jun 2, 2023

v2.35.0 proposal #3206

Merged

v3.22.0 proposal #3207

Merged

v4.1.0 proposal #3209

Merged

simon-id reviewed Sep 25, 2023

View reviewed changes

erikvanbrakel mentioned this pull request Jul 5, 2024

[DEVX-3565] add neo4j plugin for v4.41 mazedesignhq/dd-trace-js#15

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Update blocking page and status from RC or rules file#3195

Update blocking page and status from RC or rules file#3195
uurien merged 12 commits into
masterfrom
ugaitz/rc-custom-blocking

uurien commented May 29, 2023 •

edited

Loading

Uh oh!

github-actions Bot commented May 29, 2023 •

edited

Loading

Uh oh!

codecov Bot commented May 29, 2023 •

edited

Loading

Uh oh!

pr-commenter Bot commented May 29, 2023 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

simon-id Sep 15, 2023

Uh oh!

simon-id Sep 25, 2023

Uh oh!

simon-id Sep 25, 2023

Uh oh!

simon-id Sep 25, 2023

Uh oh!

uurien Sep 25, 2023

Uh oh!

simon-id Oct 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

uurien commented May 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Plugin Checklist

Uh oh!

github-actions Bot commented May 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overall package size

Dependency sizes

Uh oh!

codecov Bot commented May 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pr-commenter Bot commented May 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

simon-id Sep 15, 2023

Choose a reason for hiding this comment

Uh oh!

simon-id Sep 25, 2023

Choose a reason for hiding this comment

Uh oh!

simon-id Sep 25, 2023

Choose a reason for hiding this comment

Uh oh!

simon-id Sep 25, 2023

Choose a reason for hiding this comment

Uh oh!

uurien Sep 25, 2023

Choose a reason for hiding this comment

Uh oh!

simon-id Oct 2, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

uurien commented May 29, 2023 •

edited

Loading

github-actions Bot commented May 29, 2023 •

edited

Loading

codecov Bot commented May 29, 2023 •

edited

Loading

pr-commenter Bot commented May 29, 2023 •

edited

Loading