[AAP] API10 downstream request analysis#8232
Merged
Merged
Conversation
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (8232) and master. ✅ No regressions detected - check the details below Full Metrics ComparisonFakeDbCommand
HttpMessageHandler
Comparison explanationExecution-time benchmarks measure the whole time it takes to execute a program, and are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are highlighted in **red**. The following thresholds were used for comparing the execution times:
Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard. Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph). Duration chartsFakeDbCommand (.NET Framework 4.8)gantt
title Execution time (ms) FakeDbCommand (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8232) - mean (72ms) : 70, 75
master - mean (72ms) : 69, 74
section Bailout
This PR (8232) - mean (76ms) : 73, 79
master - mean (76ms) : 75, 78
section CallTarget+Inlining+NGEN
This PR (8232) - mean (1,073ms) : 1016, 1130
master - mean (1,066ms) : 1023, 1110
FakeDbCommand (.NET Core 3.1)gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8232) - mean (112ms) : 108, 115
master - mean (111ms) : 108, 115
section Bailout
This PR (8232) - mean (113ms) : 110, 115
master - mean (113ms) : 110, 116
section CallTarget+Inlining+NGEN
This PR (8232) - mean (784ms) : 762, 806
master - mean (783ms) : 763, 803
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8232) - mean (99ms) : 96, 102
master - mean (99ms) : 95, 103
section Bailout
This PR (8232) - mean (100ms) : 97, 102
master - mean (99ms) : 97, 102
section CallTarget+Inlining+NGEN
This PR (8232) - mean (936ms) : 906, 966
master - mean (931ms) : 903, 960
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8232) - mean (99ms) : 96, 102
master - mean (98ms) : 95, 101
section Bailout
This PR (8232) - mean (100ms) : 97, 102
master - mean (98ms) : 96, 100
section CallTarget+Inlining+NGEN
This PR (8232) - mean (813ms) : 780, 847
master - mean (811ms) : 778, 844
HttpMessageHandler (.NET Framework 4.8)gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8232) - mean (201ms) : 198, 205
master - mean (191ms) : 188, 195
section Bailout
This PR (8232) - mean (205ms) : 202, 208
master - mean (195ms) : 193, 196
section CallTarget+Inlining+NGEN
This PR (8232) - mean (1,186ms) : 1134, 1238
master - mean (1,141ms) : 1097, 1185
HttpMessageHandler (.NET Core 3.1)gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8232) - mean (289ms) : 285, 294
master - mean (273ms) : 269, 277
section Bailout
This PR (8232) - mean (290ms) : crit, 286, 293
master - mean (274ms) : 271, 277
section CallTarget+Inlining+NGEN
This PR (8232) - mean (962ms) : 940, 984
master - mean (925ms) : 905, 944
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8232) - mean (283ms) : 278, 289
master - mean (268ms) : 265, 272
section Bailout
This PR (8232) - mean (282ms) : 278, 285
master - mean (268ms) : 265, 270
section CallTarget+Inlining+NGEN
This PR (8232) - mean (1,162ms) : 1129, 1194
master - mean (1,136ms) : 1093, 1179
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8232) - mean (281ms) : 276, 286
master - mean (267ms) : 262, 271
section Bailout
This PR (8232) - mean (282ms) : crit, 277, 286
master - mean (266ms) : 263, 270
section CallTarget+Inlining+NGEN
This PR (8232) - mean (1,042ms) : 991, 1093
master - mean (1,018ms) : 979, 1058
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
dromanol
changed the title
andrewlock
reviewed
Mar 6, 2026
| { | ||
| State state = new(MaxElements); | ||
| return ReadValue(jsonReader, ref state, 0); | ||
| } |
Member
There was a problem hiding this comment.
It's very likely we're only going to load partial data given how we're loading the stream, which means we're very likely to throw, and this is very likely to always return null after doing a bunch of expensive work... not sure there's a good solution to that currently, other than you buffering the whole request
Member
There was a problem hiding this comment.
Discussed offline - because we bail if we have a "full" buffer, then this shouldn't be the case
Contributor
Snapshots difference summaryThe following differences have been observed in committed snapshots. It is meant to help the reviewer. 2 occurrences of : - _dd.appsec.json: {"triggers":[{"rule":{"id":"rasp-932-110","name":"OS command injection exploit","tags":{"category":"vulnerability_trigger","type":"command_injection"}},"rule_matches":[{"operator":"cmdi_detector","operator_value":"","parameters":[{"address":null,"highlight":["/bin/rebootCommand"],"key_path":null,"value":null}]}],"span_id": XXX}]},
+ _dd.appsec.json: {"triggers":[{"rule":{"id":"rasp-932-110","name":"OS command injection exploit","tags":{"category":"vulnerability_trigger","type":"command_injection"}},"rule_matches":[{"operator":"cmdi_detector","operator_value":"","parameters":[{"address":null,"highlight":["/bin/rebootCommand"],"key_path":null,"value":null}]}],"security_response_id":"Guid_2","span_id": XXX}]},
2 occurrences of : - _dd.appsec.json: {"triggers":[{"rule":{"id":"rasp-932-100","name":"Shell command injection exploit","tags":{"category":"vulnerability_trigger","type":"command_injection"}},"rule_matches":[{"operator":"shi_detector","operator_value":"","parameters":[{"address":null,"highlight":[";evilCommand"],"key_path":null,"value":null}]}],"span_id": XXX}]},
+ _dd.appsec.json: {"triggers":[{"rule":{"id":"rasp-932-100","name":"Shell command injection exploit","tags":{"category":"vulnerability_trigger","type":"command_injection"}},"rule_matches":[{"operator":"shi_detector","operator_value":"","parameters":[{"address":null,"highlight":[";evilCommand"],"key_path":null,"value":null}]}],"security_response_id":"Guid_2","span_id": XXX}]},
2 occurrences of : - _dd.appsec.json: {"triggers":[{"rule":{"id":"rasp-001-001","name":"Path traversal attack","tags":{"category":"vulnerability_trigger","type":"lfi"}},"rule_matches":[{"operator":"lfi_detector","operator_value":"","parameters":[{"address":null,"highlight":["/etc/password"],"key_path":null,"value":null}]}],"span_id": XXX}]},
+ _dd.appsec.json: {"triggers":[{"rule":{"id":"rasp-001-001","name":"Path traversal attack","tags":{"category":"vulnerability_trigger","type":"lfi"}},"rule_matches":[{"operator":"lfi_detector","operator_value":"","parameters":[{"address":null,"highlight":["/etc/password"],"key_path":null,"value":null}]}],"security_response_id":"Guid_2","span_id": XXX}]},
2 occurrences of : - _dd.appsec.json: {"triggers":[{"rule":{"id":"rasp-002-001","name":"Server-side request forgery","tags":{"category":"vulnerability_trigger","type":"ssrf"}},"rule_matches":[{"operator":"ssrf_detector","operator_value":"","parameters":[{"address":null,"highlight":["127.0.0.1"],"key_path":null,"value":null}]}],"span_id": XXX}]},
+ _dd.appsec.json: {"triggers":[{"rule":{"id":"rasp-002-001","name":"Server-side request forgery","tags":{"category":"vulnerability_trigger","type":"ssrf"}},"rule_matches":[{"operator":"ssrf_detector","operator_value":"","parameters":[{"address":null,"highlight":["127.0.0.1"],"key_path":null,"value":null}]}],"security_response_id":"Guid_2","span_id": XXX}]},
2 occurrences of : - _dd.appsec.json: {"triggers":[{"rule":{"id":"rasp-942-100","name":"SQL injection exploit","tags":{"category":"vulnerability_trigger","type":"sql_injection"}},"rule_matches":[{"operator":"sqli_detector","operator_value":"","parameters":[{"address":null,"highlight":["' or '1'='1"],"key_path":null,"value":null}]}],"span_id": XXX}]},
+ _dd.appsec.json: {"triggers":[{"rule":{"id":"rasp-942-100","name":"SQL injection exploit","tags":{"category":"vulnerability_trigger","type":"sql_injection"}},"rule_matches":[{"operator":"sqli_detector","operator_value":"","parameters":[{"address":null,"highlight":["' or '1'='1"],"key_path":null,"value":null}]}],"security_response_id":"Guid_2","span_id": XXX}]},
5 occurrences of : - },
- MetaStruct: {
- appsec: ,
- iast: ,
- _dd.stack:
1 occurrences of : - },
- MetaStruct: {
- iast: ,
- _dd.stack:
|
dromanol
force-pushed
the
dani/asm/API10
branch
from
March 10, 2026 10:38
ab4bba4 to
0e0c400
Compare
BenchmarksBenchmark execution time: 2026-04-13 17:30:49 Comparing candidate commit 5ddcd72 in PR branch Found 27 performance improvements and 44 performance regressions! Performance is the same for 209 metrics, 8 unstable metrics.
|
andrewlock
reviewed
Mar 10, 2026
| var contentType = content.Headers?.ContentType?.MediaType; | ||
| if (contentType is "application/json") | ||
| { | ||
| await content.LoadIntoBufferAsync().ConfigureAwait(false); |
Member
There was a problem hiding this comment.
Looking at the framework code, this scares me. By default, it looks like LoadIntoBufferAsync() loads int.MaxValue bytes when you don't provide a max buffer size. Would need to check properly, but if so, that's a hell of an allocation. We should probably pass in the bodySizeLimit here at the very least (which is still huge, but still).
dromanol
marked this pull request as ready for review
March 11, 2026 12:44
dromanol
marked this pull request as draft
March 11, 2026 12:48
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb5b8093c3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
dromanol
force-pushed
the
dani/asm/API10
branch
from
March 11, 2026 12:51
bb5b809 to
502c622
Compare
dromanol
marked this pull request as ready for review
March 12, 2026 09:38
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7bb14d101f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
e-n-0
reviewed
Mar 12, 2026
|
|
||
| internal bool RaspEnabled => _settings.RaspEnabled && AppsecEnabled; | ||
|
|
||
| internal long ApiSecurityMaxDownstreamRequestBodyAnalysis => _settings.ApiSecurityMaxDownstreamRequestBodyAnalysis; |
Member
There was a problem hiding this comment.
Not sure to understand why is it declared as a long but _settings.ApiSecurityMaxDownstreamRequestBodyAnalysis is an int.
| _waf = waf; | ||
| _configurationState = configurationState ?? new ConfigurationState(_settings, telemetry, _waf is null); | ||
| LifetimeManager.Instance.AddShutdownTask(RunShutdown); | ||
| _downstreamSampler = downstreamSampler ?? new DownstreamSampler(_settings.ApiSecurityMaxDownstreamRequestBodyAnalysis); |
Member
There was a problem hiding this comment.
DownstreamSampler is taking a double and clamps the value for the sample rate. Is it expected that _settings.ApiSecurityMaxDownstreamRequestBodyAnalysis (an int) is passed as argument here?
Contributor
Author
There was a problem hiding this comment.
Well seen. It's ApiSecurityDownstreamBodyAnalysisSampleRate
e-n-0
approved these changes
Mar 12, 2026
andrewlock
reviewed
Mar 13, 2026
Comment on lines
+31
to
+38
| if (rate < 0.5) | ||
| { | ||
| return (long)(rate * max) + long.MinValue; | ||
| } | ||
| else if (rate < 1.0) | ||
| { | ||
| return (long)((rate * max) + long.MinValue); | ||
| } |
Member
There was a problem hiding this comment.
AFAICT, both of these branches are the same? 🤔
Also, are we considering overflow correctly here? 🤔 Should this be checked or unchecked?
Contributor
Author
There was a problem hiding this comment.
Same thing. Fixing it
andrewlock
reviewed
Mar 13, 2026
| } | ||
|
|
||
| var rootSpan = Tracer.Instance.InternalActiveScope?.Root?.Span; | ||
| rootSpan??= Tracer.Instance.InternalActiveScope?.Root?.Span; |
Member
There was a problem hiding this comment.
I guess we can/should fix it in a follow up PR?
| var value = enumerator.Current.Value; | ||
| if (!headersDic.TryAdd(currentKey, value)) | ||
| { | ||
| Log.Warning("Header {Key} couldn't be added as argument to the waf", currentKey); |
Member
There was a problem hiding this comment.
We should probably remove it before merging
| { | ||
| try | ||
| { | ||
| _processDownstreamRequest = false; |
Member
There was a problem hiding this comment.
My point is, you're always running this method, even if we're not in a callsite scenario, right? Because in those cases _processDownstreamRequest==false
| public void Parse_PrimitiveNull_ReturnsNull() | ||
| { | ||
| var json = @"null"; | ||
| var result = BodyParser.Parse(json); |
Member
There was a problem hiding this comment.
AFACIT these tests all test the "string json" path, whereas AFAICT we only use the stream version. Maybe we should just delete the string API entirely, and always call the stream version here?
Contributor
Author
There was a problem hiding this comment.
Fixed. Removed the overload and migrated tests to use the Stream version
Comment on lines
+17
to
+18
| [Fact] | ||
| public void Parse_NullString_ReturnsNull() |
Member
There was a problem hiding this comment.
I think you should have some tests for invalid cases, e.g. invalid JSON of various forms
Member
There was a problem hiding this comment.
You're still not parsing any invalid JSON though right? 🤔 I think you could do something as simple as
Suggested change
| [Fact] | |
| public void Parse_NullString_ReturnsNull() | |
| [Theory] | |
| [InlineData("""{"name":"test","age":30""")] | |
| [InlineData("""{"name":"test","age":""")] | |
| [InlineData("""{"name":"test","age" """)] | |
| [InlineData("""{"name":"test",""")] | |
| public void Parse_TruncatedString_ReturnsNull(string json) | |
| { | |
| BodyParser.Parse(json).Should.BeNull(); | |
| ParseBody(json).Should().BeNull(); | |
| } | |
| [Fact] | |
| public void Parse_NullString_ReturnsNull() |
| var wafArgs = new Dictionary<string, object>(); | ||
|
|
||
| // Use reflection to call the private AddBody method | ||
| var method = typeof(RaspModule).GetMethod("AddBody", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Static); |
Member
There was a problem hiding this comment.
Just make the method public and mark it with [TestingOnly] - this reflection is horribly fragile 😅
Contributor
Author
There was a problem hiding this comment.
Oh, did not know that trick
| } | ||
|
|
||
| [Fact] | ||
| public void AddBody_InvalidJson_DoesNotAddBody() |
Member
There was a problem hiding this comment.
I think there are a couple of key cases missing here, namely proper "streaming" responses, e.g. chunked encoding. I think we really should add those test cases to be confident it's always behaving as expected (where the content length can't be detected up front)
Contributor
Author
There was a problem hiding this comment.
Added some chunked HttpContent tests
|
|
||
| if (body is not null) | ||
| { | ||
| request.Content = new StringContent(body, Encoding.UTF8, contentType ?? "application/json"); |
Member
There was a problem hiding this comment.
Using StringContent is unfortunately not a good test for real life - it knows the content size up front, which will not be the case for many responses.
dromanol
force-pushed
the
dani/asm/API10
branch
2 times, most recently
from
March 17, 2026 15:24
823f2f0 to
7cbf4b4
Compare
andrewlock
reviewed
Mar 17, 2026
Comment on lines
+17
to
+18
| [Fact] | ||
| public void Parse_NullString_ReturnsNull() |
Member
There was a problem hiding this comment.
You're still not parsing any invalid JSON though right? 🤔 I think you could do something as simple as
Suggested change
| [Fact] | |
| public void Parse_NullString_ReturnsNull() | |
| [Theory] | |
| [InlineData("""{"name":"test","age":30""")] | |
| [InlineData("""{"name":"test","age":""")] | |
| [InlineData("""{"name":"test","age" """)] | |
| [InlineData("""{"name":"test",""")] | |
| public void Parse_TruncatedString_ReturnsNull(string json) | |
| { | |
| BodyParser.Parse(json).Should.BeNull(); | |
| ParseBody(json).Should().BeNull(); | |
| } | |
| [Fact] | |
| public void Parse_NullString_ReturnsNull() |
andrewlock
reviewed
Apr 10, 2026
andrewlock
approved these changes
Apr 10, 2026
andrewlock
reviewed
Apr 10, 2026
andrewlock
reviewed
Apr 10, 2026
Co-authored-by: Andrew Lock <[email protected]>
andrewlock
force-pushed
the
dani/asm/API10
branch
from
April 13, 2026 14:17
5e2f8c7 to
5ddcd72
Compare
andrewlock
enabled auto-merge (squash)
April 13, 2026 16:26
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of changes
Implement needed changes to handle API10 (downstream request) vulnerability in RASP
Reason for change
Implementation details
Test coverage
Other details