[appsec] Stripe business logic events#7138
Conversation
Overall package sizeSelf size: 4.62 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 2.0.6 | 81.92 kB | 813.08 kB | | dc-polyfill | 0.1.10 | 26.73 kB | 26.73 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🔗 Commit SHA: 0a6d94c | Docs | Datadog PR Page | Was this helpful? Give us feedback! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7138 +/- ##
==========================================
+ Coverage 80.15% 80.19% +0.04%
==========================================
Files 730 731 +1
Lines 31217 31281 +64
==========================================
+ Hits 25023 25087 +64
Misses 6194 6194
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
BenchmarksBenchmark execution time: 2026-02-17 10:34:58 Comparing candidate commit 0a6d94c in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 229 metrics, 31 unstable metrics. |
|
|
||
| addHook({ | ||
| name: 'stripe', | ||
| versions: ['9', '10', '11', '12', '13', '14', '15', '16', '17', '18', '19', '>=20.0.0'], |
There was a problem hiding this comment.
Is this long list (instead of >=9) used here only for testing purpose? To force the the execution in the versions 9, 10...?
I think that we shold add a comment explainig why, or else we can set this as >=9 and in externals.json add these version in stripe section. I think that it is the main purpose of having an array for versions in externals.json:
"stripe": [
{
"name": "express",
"versions": ["^4"]
},
{
"name": "body-parser",
"versions": ["1.20.1"]
},
{
"name": "stripe",
"versions": ['9', '10', '11', '12', '13', '14', '15', '16', '17', '18', '19']
}
],There was a problem hiding this comment.
yes, it's to make sure we test all the majors, as it seems like stripe sdk loves majors. I don't think a comment is super useful here as this syntax is used in other instrumentations too. If you want a comment, please make a github suggestion, it's easy and handy!
And about externals.json I am not as sure as oyu this is why we have it. What's wrong with having it in the instrmentation anyway ?
There was a problem hiding this comment.
What's wrong with having it in the instrumentation anyway ?
I am afraid that someone could see that and change it to <=9.
There was a problem hiding this comment.
I believe we should just improve our install script to always run tests for all majors in a range
|
I found an edge case worth documenting.
There's not much we can do here since the span is already closed at that point anyway. Even if In the real world this is rare (requires client timeout < Stripe latency), and the existing warning |
|
@CarlesDD Good point but as you said it's a limitation of everything for appsec. I don't know if it's worth documenting this especially since i wouldn't know where to put this explanation. It's kind of obvious ? can't add tags when there is no span ? |
4 participants
What does this PR do?
This PR adds instrumentation for the Stripe SDK, and sends payloads from a couple of methods to the AppSec WAF, that will in turn convert them into span tags for the backend to receive.
Motivation
These tags will be used by the WAF to detect endpoints that handle payments in the Endpoint Catalog, and will be useful for fraud detection and general security observability.
ST: DataDog/system-tests#6219