graceful fallback if appsec template files are missing #2841

tlhunter · 2023-02-24T22:49:56Z

What does this PR do?

if the template files used by appsec are missing on disk then gently fallback to basic inlined version

Motivation

this fixes an upcoming issue with bundler support where template files are not bundled

github-actions · 2023-02-24T22:50:48Z

Overall package size

Self size: 3.84 MB
Deduped: 57.5 MB
No deduping: 57.55 MB

Dependency sizes

name	version	self size	total size
@datadog/native-iast-taint-tracking	1.1.1	13.38 MB	13.39 MB
@datadog/native-appsec	2.0.0	12.33 MB	12.34 MB
@datadog/pprof	2.0.0	10.47 MB	11.35 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-02-24T22:53:11Z

Codecov Report

Merging #2841 (2016762) into master (09d605f) will increase coverage by 2.21%.
The diff coverage is 55.55%.

@@            Coverage Diff             @@
##           master    #2841      +/-   ##
==========================================
+ Coverage   89.26%   91.47%   +2.21%     
==========================================
  Files         317      269      -48     
  Lines       11282     9459    -1823     
  Branches       33       33              
==========================================
- Hits        10071     8653    -1418     
+ Misses       1211      806     -405

Impacted Files	Coverage Δ
packages/dd-trace/src/appsec/blocking.js	`82.22% <55.55%> (-17.78%)`	⬇️

... and 48 files with indirect coverage changes

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

Qard

Perhaps it would be better to just inline the current files with template strings or something?

Qard · 2023-02-24T23:00:53Z

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

@@ -8,6 +8,9 @@ let templateLoaded = false
 let templateHtml = ''
 let templateJson = ''

+const ERROR_HTML = '<html>blocked</html>'
+const ERROR_JSON = '{"error": "blocked"}'


You could just set these to the template* variables directly and skip needing these extra vars.

Change has been made

pr-commenter · 2023-02-24T23:04:45Z

Benchmarks

Comparing candidate commit 4d5a1a9 in PR branch tlhunter/appsec-missing-file-graceful with baseline commit 09d605f in branch master.

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

simon-id · 2023-03-11T20:12:53Z

There is another file which is read at runtime, it's dd-trace/src/appsec/recommended.json, it is read by dd-trace/src/appsec/index.js.
Shouldn't we do something about that too ?

simon-id · 2023-03-11T20:45:44Z

Shouldn't we have some tests for this ?

simon-id · 2023-03-13T10:05:28Z

@iunanua

simon-id · 2023-03-14T15:25:49Z

After discussion with the guild, the best solution would I think be to embed the html and json templates directly in js files as a string and require those files. As for the rules files I mentionned above, basically just require it. I'm not sure if we should require those 3 files at startup time or we can keep it during runtime like it is currently.

iunanua · 2023-03-15T09:30:16Z

to embed the html and json templates directly in js files

this is the simplest solution but templates and rules file are configurable by the user.
In the case of blockedTemplateHtml you would need to wrap the html content inside a string and export it and then you could require it.
Something like this:

module.exports = '
<!DOCTYPE html>
<html lang="en">
...
'

And blockedTemplateHtml property is a path so yo will have to differentiate when a user has configured it to not require and readFile it like before because user will continue to use an html file.

iunanua · 2023-03-15T10:21:43Z

Another detail, esbuild can bundle requires with packages that it can resolve during build time but it cannot with things like:

const rules = require(_config.appsec.rules || path.join(__dirname, 'recommended.json'))

I'm trying to extend Thomas esbuild plugin to detect the load of certain files like appsec/index.js or appsec/blocking.js and make some expression replacements in order to embed the template and json files content inside them

iunanua · 2023-03-20T11:19:58Z

one solution using require #2913

tlhunter requested a review from a team as a code owner February 24, 2023 22:49

tlhunter added the semver-patch label Feb 24, 2023

tlhunter mentioned this pull request Feb 24, 2023

support bundling via esbuild #2763

Merged

6 tasks

Qard reviewed Feb 24, 2023

View reviewed changes

tlhunter force-pushed the tlhunter/appsec-missing-file-graceful branch from 345f888 to 2016762 Compare March 10, 2023 20:59

gentle fallback if appsec template files are missing

4d5a1a9

tlhunter force-pushed the tlhunter/appsec-missing-file-graceful branch from 2016762 to 4d5a1a9 Compare March 10, 2023 21:06

This was referenced Mar 16, 2023

ddTrace esbuild plugin /* @dd-bundle: */ comment expressions PoC #2899

Closed

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

Merged

iunanua closed this in #2913 Mar 31, 2023

tlhunter deleted the tlhunter/appsec-missing-file-graceful branch December 19, 2023 16:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

graceful fallback if appsec template files are missing #2841

graceful fallback if appsec template files are missing #2841

tlhunter commented Feb 24, 2023

github-actions bot commented Feb 24, 2023 •

edited

Loading

codecov bot commented Feb 24, 2023 •

edited

Loading

Qard left a comment

Qard Feb 24, 2023

tlhunter Mar 10, 2023

pr-commenter bot commented Feb 24, 2023 •

edited

Loading

simon-id commented Mar 11, 2023

simon-id commented Mar 11, 2023

simon-id commented Mar 13, 2023

simon-id commented Mar 14, 2023

iunanua commented Mar 15, 2023

iunanua commented Mar 15, 2023 •

edited

Loading

iunanua commented Mar 20, 2023

graceful fallback if appsec template files are missing #2841

graceful fallback if appsec template files are missing #2841

Conversation

tlhunter commented Feb 24, 2023

What does this PR do?

Motivation

github-actions bot commented Feb 24, 2023 • edited Loading

Overall package size

Dependency sizes

codecov bot commented Feb 24, 2023 • edited Loading

Codecov Report

Qard left a comment

Choose a reason for hiding this comment

Qard Feb 24, 2023

Choose a reason for hiding this comment

tlhunter Mar 10, 2023

Choose a reason for hiding this comment

pr-commenter bot commented Feb 24, 2023 • edited Loading

Benchmarks

simon-id commented Mar 11, 2023

simon-id commented Mar 11, 2023

simon-id commented Mar 13, 2023

simon-id commented Mar 14, 2023

iunanua commented Mar 15, 2023

iunanua commented Mar 15, 2023 • edited Loading

iunanua commented Mar 20, 2023

github-actions bot commented Feb 24, 2023 •

edited

Loading

codecov bot commented Feb 24, 2023 •

edited

Loading

pr-commenter bot commented Feb 24, 2023 •

edited

Loading

iunanua commented Mar 15, 2023 •

edited

Loading