Embed AppSec rules and templates files to be compatible with bundlers #2913

iunanua · 2023-03-20T10:37:10Z

What does this PR do?

Move templates/blocked.html and templates/blocked.json file contents to blocked_templates.js file and require it in order to load the appsec templates.
Now is in config.js where the template and rules are loaded, establishing the corresponding values for config.appsec.blockedTemplateHtml and config.appsec.blockedTemplateJson.

Closes #2841
Closes #2899

Motivation

Avoid errors with esbuild bundles

github-actions · 2023-03-20T10:38:15Z

Overall package size

Self size: 3.99 MB
Deduped: 59.59 MB
No deduping: 59.63 MB

Dependency sizes

name	version	self size	total size
@datadog/native-iast-taint-tracking	1.3.1	13.47 MB	13.47 MB
@datadog/pprof	2.1.0	12.31 MB	13.2 MB
@datadog/native-appsec	2.0.0	12.33 MB	12.34 MB
@datadog/native-metrics	1.5.0	7.1 MB	7.11 MB
protobufjs	7.1.2	2.76 MB	6.55 MB
@datadog/native-iast-rewriter	2.0.1	2.09 MB	2.1 MB
opentracing	0.14.7	194.81 kB	194.81 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
semver	5.7.1	61.58 kB	61.58 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.4	32.7 kB	37.17 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-03-20T10:39:48Z

Codecov Report

Merging #2913 (89288e5) into master (692048f) will decrease coverage by 15.52%.
The diff coverage is 66.66%.

@@             Coverage Diff             @@
##           master    #2913       +/-   ##
===========================================
- Coverage   87.26%   71.75%   -15.52%     
===========================================
  Files         321      167      -154     
  Lines       11453     6558     -4895     
  Branches       33       33               
===========================================
- Hits         9995     4706     -5289     
- Misses       1458     1852      +394

Impacted Files	Coverage Δ
...ackages/dd-trace/src/appsec/remote_config/index.js	`33.33% <0.00%> (-66.67%)`	⬇️
packages/dd-trace/src/plugins/ci_plugin.js	`10.00% <ø> (ø)`
packages/dd-trace/src/plugins/util/test.js	`79.84% <ø> (-0.16%)`	⬇️
packages/dd-trace/src/appsec/index.js	`17.10% <25.00%> (-82.90%)`	⬇️
packages/dd-trace/src/appsec/blocking.js	`28.00% <71.42%> (-72.00%)`	⬇️
packages/dd-trace/src/appsec/blocked_templates.js	`100.00% <100.00%> (ø)`
packages/dd-trace/src/appsec/sdk/index.js	`57.14% <100.00%> (-42.86%)`	⬇️
packages/dd-trace/src/config.js	`98.81% <100.00%> (+0.58%)`	⬆️

... and 204 files with indirect coverage changes

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

pr-commenter · 2023-03-20T10:46:50Z

Benchmarks

Comparing candidate commit 89288e5 in PR branch igor/appsec-require-template-files with baseline commit 60b4fde in branch master.

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

scenario:plugin-graphql-with-depth-off-16

🟩 max_rss_usage [-0.180MB; -0.166MB] or [-17.678%; -16.303%]

scenario:plugin-graphql-with-depth-on-max-16

🟩 max_rss_usage [-0.182MB; -0.171MB] or [-18.108%; -17.027%]

scenario:plugin-graphql-with-depth-and-collapse-on-16

🟩 max_rss_usage [-0.198MB; -0.163MB] or [-19.620%; -16.169%]

scenario:plugin-dns-with-tracer-16

🟩 max_rss_usage [-3.284KB; -2.967KB] or [-6.027%; -5.445%]

Qard

Seems to me like it would be a whole lot cleaner to just do all the file loading stuff in config.js rather than spreading this file loading stuff all over the codebase.

That's what trace is doing for DD_SPAN_SAMPLING_RULES_FILE here. 🤔

iunanua · 2023-03-22T11:51:19Z

@Qard @simon-id It is possible to change appsec rules or template paths via remote config?

simon-id · 2023-03-23T09:31:32Z

Problem of reading the files directly in config.js: telemetry sends the config object at startup, and it would also include the files we read, which is a bit dirty.

packages/dd-trace/src/appsec/templates/blocked.js

packages/dd-trace/src/config.js

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

packages/dd-trace/src/appsec/blocked-templates.js

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

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

tlhunter · 2023-03-27T21:34:05Z

I do prefer this approach compared to #2899. I could see #2899 being upstreamed into esbuild itself somehow. Maybe a community plugin that somehow interacts with fs, similar to Go's embed functionality. If it were made into an external plugin then I could see us making use of it. Otherwise the approach in #2899 seems pretty heavy for just the two files.

packages/dd-trace/test/appsec/sdk/index.spec.js

packages/dd-trace/src/config.js

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

packages/dd-trace/test/appsec/blocking.spec.js

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

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

…N.stringify blocked template json

packages/dd-trace/test/config.spec.js

simon-id · 2023-03-29T13:25:38Z

packages/dd-trace/test/config.spec.js

-    expect(config.appsec.blockedTemplateJson).to.be
-      .equal(path.join(__dirname, '..', 'src', 'appsec', 'templates', 'blocked.json'))
+
+    expect(log.error).to.be.called


This is really not good coverage, please use more precise asserts like

expect(log.error).to.have.been.calledTwice expect(log.error.firstCall).to.have.been.calledWithExactly('...') expect(log.error.secondCall).to.have.been.calledWithExactly('...')

or something like that

yes, but have we decided the error message?

I think this is adequate: dd-trace: Error reading file /not/found/file/path. Is the 'DD_APPSEC_RULES' environment variable correct?

packages/dd-trace/test/config.spec.js

…#2913) * gentle fallback if appsec template files are missing * Load appsec template files with require --------- Co-authored-by: Thomas Hunter II <tlhunter@datadog.com> Co-authored-by: simon-id <simon.id@datadoghq.com>

iunanua added the semver-patch label Mar 20, 2023

iunanua mentioned this pull request Mar 20, 2023

graceful fallback if appsec template files are missing #2841

Closed

Qard reviewed Mar 20, 2023

View reviewed changes

iunanua marked this pull request as ready for review March 21, 2023 09:21

iunanua requested review from a team as code owners March 21, 2023 09:21

iunanua changed the base branch from tlhunter/appsec-missing-file-graceful to master March 21, 2023 09:31

simon-id requested changes Mar 24, 2023

View reviewed changes

packages/dd-trace/src/appsec/templates/blocked.js Outdated Show resolved Hide resolved

packages/dd-trace/src/config.js Show resolved Hide resolved

packages/dd-trace/src/appsec/blocking.js Outdated Show resolved Hide resolved

iunanua force-pushed the igor/appsec-require-template-files branch from 8bffe79 to f370e86 Compare March 24, 2023 14:25

simon-id reviewed Mar 24, 2023

View reviewed changes

packages/dd-trace/src/appsec/blocked-templates.js Outdated Show resolved Hide resolved

iunanua force-pushed the igor/appsec-require-template-files branch from b667cd4 to 302ddc9 Compare March 27, 2023 14:04

simon-id reviewed Mar 27, 2023

View reviewed changes

packages/dd-trace/src/appsec/index.js Outdated Show resolved Hide resolved

simon-id reviewed Mar 27, 2023

View reviewed changes

packages/dd-trace/src/appsec/blocking.js Outdated Show resolved Hide resolved

tlhunter and others added 2 commits March 27, 2023 19:05

gentle fallback if appsec template files are missing

7c1be5c

Load appsec template files with require

3f8117e

iunanua force-pushed the igor/appsec-require-template-files branch from f2fc119 to 3f8117e Compare March 27, 2023 17:06

simon-id changed the title ~~require appsec templates~~ Embed AppSec rules and templates files to be compatible with bundlers Mar 28, 2023

simon-id reviewed Mar 28, 2023

View reviewed changes

packages/dd-trace/test/appsec/sdk/index.spec.js Outdated Show resolved Hide resolved

simon-id reviewed Mar 28, 2023

View reviewed changes

packages/dd-trace/src/config.js Outdated Show resolved Hide resolved

Fix test message

ac41c02

simon-id reviewed Mar 28, 2023

View reviewed changes

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

simon-id reviewed Mar 28, 2023

View reviewed changes

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

simon-id reviewed Mar 28, 2023

View reviewed changes

packages/dd-trace/test/appsec/blocking.spec.js Outdated Show resolved Hide resolved

recommended.json loading test and change test require

f2ee10b

simon-id and others added 3 commits March 28, 2023 15:48

spacing

875f4fb

cleanup

32f2535

Expect blocking.setTemplates to be called and remove empty afterEach

5913d6f

simon-id reviewed Mar 28, 2023

View reviewed changes

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

simon-id added 2 commits March 28, 2023 15:58

Update packages/dd-trace/test/appsec/index.spec.js

601a895

remove necessary path module

393b4d0

tlhunter reviewed Mar 28, 2023

View reviewed changes

packages/dd-trace/src/appsec/blocked_templates.js Show resolved Hide resolved

tlhunter previously approved these changes Mar 28, 2023

View reviewed changes

Improve trace message when error occurs when loading config file. JSO…

184d1e1

…N.stringify blocked template json

iunanua dismissed tlhunter’s stale review via 184d1e1 March 29, 2023 08:20

simon-id and others added 2 commits March 29, 2023 12:49

Add more coverage in config.spec.js

8b491b3

coverage and suggestions

67b5e04

simon-id reviewed Mar 29, 2023

View reviewed changes

packages/dd-trace/test/config.spec.js Outdated Show resolved Hide resolved

simon-id reviewed Mar 29, 2023

View reviewed changes

packages/dd-trace/test/config.spec.js Outdated Show resolved Hide resolved

iunanua and others added 5 commits March 30, 2023 12:42

log exception, increase coverage, suggestions

7299160

Update config.spec.js

110f643

Update packages/dd-trace/test/config.spec.js

deb80b7

Update config.spec.js

5366f6a

Update config.spec.js

89288e5

simon-id approved these changes Mar 31, 2023

View reviewed changes

iunanua merged commit b765f3e into master Mar 31, 2023

simon-id mentioned this pull request Mar 31, 2023

support bundling via esbuild #2763

Merged

6 tasks

tlhunter mentioned this pull request Apr 3, 2023

v2.30.0 #2964

Merged

tlhunter mentioned this pull request Apr 3, 2023

v3.17.0 #2965

Merged

iunanua deleted the igor/appsec-require-template-files branch December 19, 2023 08:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Embed AppSec rules and templates files to be compatible with bundlers #2913

Embed AppSec rules and templates files to be compatible with bundlers #2913

iunanua commented Mar 20, 2023 •

edited by simon-id

Loading

github-actions bot commented Mar 20, 2023 •

edited

Loading

codecov bot commented Mar 20, 2023 •

edited

Loading

pr-commenter bot commented Mar 20, 2023 •

edited

Loading

Qard left a comment

iunanua commented Mar 22, 2023 •

edited

Loading

simon-id commented Mar 23, 2023

tlhunter commented Mar 27, 2023

simon-id Mar 29, 2023 •

edited

Loading

iunanua Mar 30, 2023

tlhunter Mar 30, 2023

Embed AppSec rules and templates files to be compatible with bundlers #2913

Embed AppSec rules and templates files to be compatible with bundlers #2913

Conversation

iunanua commented Mar 20, 2023 • edited by simon-id Loading

What does this PR do?

Motivation

github-actions bot commented Mar 20, 2023 • edited Loading

Overall package size

Dependency sizes

codecov bot commented Mar 20, 2023 • edited Loading

Codecov Report

pr-commenter bot commented Mar 20, 2023 • edited Loading

Benchmarks

scenario:plugin-graphql-with-depth-off-16

scenario:plugin-graphql-with-depth-on-max-16

scenario:plugin-graphql-with-depth-and-collapse-on-16

scenario:plugin-dns-with-tracer-16

Qard left a comment

Choose a reason for hiding this comment

iunanua commented Mar 22, 2023 • edited Loading

simon-id commented Mar 23, 2023

tlhunter commented Mar 27, 2023

simon-id Mar 29, 2023 • edited Loading

Choose a reason for hiding this comment

iunanua Mar 30, 2023

Choose a reason for hiding this comment

tlhunter Mar 30, 2023

Choose a reason for hiding this comment

iunanua commented Mar 20, 2023 •

edited by simon-id

Loading

github-actions bot commented Mar 20, 2023 •

edited

Loading

codecov bot commented Mar 20, 2023 •

edited

Loading

pr-commenter bot commented Mar 20, 2023 •

edited

Loading

iunanua commented Mar 22, 2023 •

edited

Loading

simon-id Mar 29, 2023 •

edited

Loading