[ASM] Consolidate ASP.NET Core WAF scans and harden request-phase cov…#8856
Draft
daniel-romano-DD wants to merge 6 commits into
Draft
[ASM] Consolidate ASP.NET Core WAF scans and harden request-phase cov…#8856daniel-romano-DD wants to merge 6 commits into
daniel-romano-DD wants to merge 6 commits into
Conversation
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (8856) 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 (8856) - mean (72ms) : 67, 76
master - mean (73ms) : 70, 77
section Bailout
This PR (8856) - mean (74ms) : 70, 77
master - mean (75ms) : 72, 79
section CallTarget+Inlining+NGEN
This PR (8856) - mean (1,083ms) : 1037, 1130
master - mean (1,080ms) : 1032, 1127
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 (8856) - mean (111ms) : 103, 118
master - mean (112ms) : 106, 117
section Bailout
This PR (8856) - mean (109ms) : 108, 110
master - mean (112ms) : 107, 117
section CallTarget+Inlining+NGEN
This PR (8856) - mean (776ms) : 751, 801
master - mean (776ms) : 749, 802
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8856) - mean (96ms) : 93, 98
master - mean (98ms) : 92, 104
section Bailout
This PR (8856) - mean (100ms) : 94, 106
master - mean (97ms) : 96, 99
section CallTarget+Inlining+NGEN
This PR (8856) - mean (934ms) : 892, 975
master - mean (934ms) : 898, 970
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8856) - mean (96ms) : 90, 101
master - mean (95ms) : 93, 98
section Bailout
This PR (8856) - mean (95ms) : 94, 97
master - mean (100ms) : 95, 104
section CallTarget+Inlining+NGEN
This PR (8856) - mean (814ms) : 775, 853
master - mean (811ms) : 770, 851
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 (8856) - mean (205ms) : 199, 211
master - mean (202ms) : 197, 206
section Bailout
This PR (8856) - mean (210ms) : 205, 214
master - mean (206ms) : 202, 209
section CallTarget+Inlining+NGEN
This PR (8856) - mean (1,217ms) : 1172, 1261
master - mean (1,210ms) : 1166, 1255
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 (8856) - mean (290ms) : 282, 298
master - mean (291ms) : 285, 298
section Bailout
This PR (8856) - mean (295ms) : 289, 300
master - mean (293ms) : 288, 298
section CallTarget+Inlining+NGEN
This PR (8856) - mean (980ms) : 960, 1000
master - mean (977ms) : 957, 998
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8856) - mean (284ms) : 277, 290
master - mean (284ms) : 278, 291
section Bailout
This PR (8856) - mean (283ms) : 278, 289
master - mean (283ms) : 276, 290
section CallTarget+Inlining+NGEN
This PR (8856) - mean (1,172ms) : 1131, 1212
master - mean (1,176ms) : 1136, 1217
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8856) - mean (285ms) : 279, 291
master - mean (285ms) : 278, 291
section Bailout
This PR (8856) - mean (286ms) : 282, 291
master - mean (286ms) : 279, 293
section CallTarget+Inlining+NGEN
This PR (8856) - mean (1,051ms) : 1010, 1093
master - mean (1,046ms) : 1004, 1088
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
BenchmarksBenchmark execution time: 2026-07-02 09:58:03 Comparing candidate commit 9b9d2cf in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 72 metrics, 0 unstable metrics, 61 known flaky benchmarks, 65 flaky benchmarks without significant changes.
|
…erage Consolidate the scattered ASP.NET Core WAF calls (basic request scan, path params, request/response body) into a single request-phase scan (RunRequestScan) plus the existing response-phase scan (CheckReturnedHeaders), keeping the WAF to two runs per request on the happy path. Hardening on top of the consolidation: - Close the short-circuit coverage gap: when the request-phase scan never ran (e.g. an MVC request short-circuited by an authorization/resource filter before ActionResponseFilter.OnActionExecuting, with no end-pipeline fallback), fold the basic request addresses into the response-phase call so request rules still evaluate and can still block. - Make RunRequestScan idempotent via RequestScanCompleted so it is safe to call from more than one hook (non-MVC route event and MVC/Razor action filter). - Resolve the local-root span in ActionResponseFilter (InternalActiveScope.Root), consistent with FireOnStartCommon, instead of the innermost active scope. - End-pipeline 404 fallback marks RequestScanCompleted to avoid the response-phase scan redundantly re-adding the basic request addresses. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The consolidated request-phase scan routed the model-bound request body through ActionResponseFilter (an IActionFilter), which ASP.NET Core does not run for Razor Pages. As a result a Razor Page request body was stashed but never scanned, so body attacks were neither detected nor blocked (returned 200 instead of 403). Distinguish the two cases at the model-binding hook using RequestScanCompleted: - MVC controller: the routing event skipped the request scan, so the body is stashed for ActionResponseFilter.OnActionExecuting (unchanged). - Razor Page: the routing event already ran the request scan (no `controller` route value), and no action filter will consume a stashed body, so scan the freshly bound body immediately. Also drop the RequestScanCompleted early-return in RunRequestScan: it prevented the second (body) scan for Razor Pages. The method is meant to run once per MVC controller but twice for a Razor Page (basic at routing, body at model binding). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
daniel-romano-DD
force-pushed
the
dani/aap/waf_call_refactor
branch
from
8d7e824 to
172f1ff
Compare
Contributor
Author
|
@codex review |
![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: 172f1ff4c9
ℹ️ 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".
| /// Stashes the extracted request body to be flushed in the next request-phase WAF call. | ||
| /// Called from the model-binding hook; consumed by ActionResponseFilter.OnActionExecuting. | ||
| /// </summary> | ||
| internal void SetPendingRequestBody(object body) => _pendingRequestBody = body; |
There was a problem hiding this comment.
When MVC binds more than one top-level body/form value before the action filter (for example multiple [FromForm] parameters), DefaultModelBindingContext_SetResult_Integration calls this setter for each value, but this assignment overwrites the previous extraction and ActionResponseFilter later scans only the last one. Before this change each extracted value was passed to CheckBody immediately, so attacks in earlier form/body-bound parameters are no longer evaluated by AppSec; please accumulate or scan every pending body value.
Useful? React with 👍 / 👎.
Comment on lines
+582
to
+587
| if (controllerName is null) | ||
| { | ||
| // Non-MVC endpoint (minimal API, Razor Pages, etc.) — run the consolidated request scan now. | ||
| // MVC controller endpoints defer to ActionResponseFilter.OnActionExecuting so the bound body | ||
| // is available and the scan runs immediately before the action executes. | ||
| _security.RunRequestScan(httpContext, rootSpan, routeValues, requestBody: null); |
There was a problem hiding this comment.
For controller endpoints this branch now skips the routing-time scan and relies on ActionResponseFilter, but that filter rebuilds server.request.path_params only from action parameters. MVC routes can contain values that are not action method parameters (for example class-level or ambient route tokens read from RouteData), and those were previously included because the routing event passed the full RouteValueDictionary; after this change those values are missing from WAF input, so path-param rules will not fire for them. The same skip exists in the single-span observer.
Useful? React with 👍 / 👎.
| // For normal matched requests, RequestScanCompleted is true and we skip to avoid a third WAF call | ||
| // (the two canonical calls are RunRequestScan in ActionResponseFilter.OnActionExecuting and | ||
| // CheckReturnedHeaders in FireOnStarting). | ||
| if (_endPipeline && security.AppsecEnabled) |
There was a problem hiding this comment.
With this guard the middleware inserted at the start of the pipeline no longer performs the request WAF scan before invoking _next; the new request scans run later from MVC/endpoint hooks and block by throwing BlockException. In apps with catch-all exception-handling middleware registered inside _next, that middleware can catch the BlockException and turn it into a normal error response before this outer catch sees it, so AppSec blocking for headers/query/cookies/path/body can be bypassed; keep the blocking decision in this outer middleware path or write the block response before entering user middleware.
Useful? React with 👍 / 👎.
Comment on lines
+57
to
+59
| var args = requestScanCompleted | ||
| ? new Dictionary<string, object>() | ||
| : securityCoordinator.GetBasicRequestArgsForWaf(); |
There was a problem hiding this comment.
In the request-not-scanned fallback this only rebuilds the basic request arguments, while the removed route-matched path called CheckPathParamsAndSessionId and RunRequestScan still passes sessionId from ISessionFeature. For MVC requests short-circuited before ActionResponseFilter.OnActionExecuting, no request-phase scan now runs and this fallback never supplies usr.session_id, so rules or blocking decisions that depend on the ASP.NET Core session id are skipped; mirror the session lookup here before calling RunWaf.
Useful? React with 👍 / 👎.
Comment on lines
+151
to
+155
| securityCoordinator.BlockAndReport(result); | ||
|
|
||
| if (appSecRequestContext is not null) | ||
| { | ||
| appSecRequestContext.RequestScanCompleted = true; |
There was a problem hiding this comment.
When this WAF call returns a blocking result, BlockAndReport throws before RequestScanCompleted is set. The outer middleware then writes the blocking response, which invokes FireOnStarting; CheckReturnedHeaders still sees the request as unscanned and re-runs the WAF with request addresses while constructing the block response. For blocking requests this adds an extra WAF execution and can throw another BlockException out of WriteResponse; set the flag before throwing or in a finally.
Useful? React with 👍 / 👎.
Comment on lines
+77
to
+80
| var extracted = ObjectExtractor.Extract(result.Value); | ||
| if (extracted is not null) | ||
| { | ||
| currentSpan.Context?.TraceContext?.AppSecRequestContext.SetPendingResponseBody(extracted); |
There was a problem hiding this comment.
This now only stashes the extracted response body and relies on CheckReturnedHeaders to consume it, but the only FireOnStarting integrations in this repo are for Kestrel and IIS (FireOnStartCommon). ASP.NET Core apps hosted on HttpSys or another server where that hook is not instrumented will never consume the pending body, so response-body AppSec rules silently stop running even though the previous CheckBody call executed directly from the MVC filter.
Useful? React with 👍 / 👎.
When a WAF redirect action (redirect_request) triggered from within the pipeline (RunRequestScan / CheckReturnedHeaders), it surfaced as a BlockException whose BlockInfo property actually holds the RedirectInfo (BlockAndReport throws `new BlockException(result, result.RedirectInfo ?? result.BlockInfo)`). The outermost BlockingMiddleware catch built the action with `GetBlockingAction(accept, blockException.BlockInfo, null)` — passing the redirect dict as blockInfo and null as redirectInfo. GetBlockingAction then took the block branch: it wrote a block body and set the status from the dict's status_code (302), with IsRedirect=false. Result: HTTP 302 with a block-page body and no Location header. Use the full result's block/redirect info (`blockException.Result.BlockInfo`, `blockException.Result.RedirectInfo`) so redirects are written as redirects (3xx + Location, no body), matching the end-pipeline path. Block actions are unaffected (RedirectInfo is null for them). Validated with AsmActionsConfiguration: redirect_request now returns 302 with content-length 0; block_request still returns its body with the configured status. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Snapshots difference summaryThe following differences have been observed in committed snapshots. It is meant to help the reviewer. 9 occurrences of : - Resource: GET /health/,
+ Resource: GET /health,
[...]
+ aspnet_core.endpoint: Samples.Security.AspNetCore5.Controllers.HealthController.Index (Samples.Security.AspNetCore5),
+ aspnet_core.route: health,
[...]
+ http.endpoint: health,
[...]
+ http.route: health,
9 occurrences of : + _dd.appsec.fp.session: ssn----<SessionFp>,
[...]
+ _dd.code_origin.frames.0.index: 0,
+ _dd.code_origin.frames.0.method: Index,
+ _dd.code_origin.frames.0.type: Samples.Security.AspNetCore5.Controllers.HealthController,
+ _dd.code_origin.type: entry,
2 occurrences of : - Resource: GET /,
+ Resource: GET /home/index,
[...]
+ aspnet_core.endpoint: Samples.Security.AspNetCore5.Controllers.HomeController.Index (Samples.Security.AspNetCore5),
+ aspnet_core.route: {controller=home}/{action=index}/{id?},
[...]
+ http.endpoint: {controller=home}/{action=index}/{id?},
[...]
+ http.route: {controller=home}/{action=index}/{id?},
2 occurrences of : + _dd.appsec.fp.session: ssn----<SessionFp>,
[...]
+ _dd.code_origin.frames.0.index: 0,
+ _dd.code_origin.frames.0.method: Index,
+ _dd.code_origin.frames.0.type: Samples.Security.AspNetCore5.Controllers.HomeController,
+ _dd.code_origin.type: entry,
|
CheckReturnedHeaders (the response-phase scan, run from FireOnStarting) already guards on !transport.IsBlocked so it can skip the response scan — and thus API-Security response schema extraction (_dd.appsec.s.res.*) — for a blocked request. But when a block was raised as a BlockException from within the pipeline (RunRequestScan / CheckReturnedHeaders via BlockAndReport/ReportAndBlock), nothing marked the transport blocked: MarkBlocked was only called on the end-pipeline 404 fallback. So IsBlocked was still false when the block response was written, CheckReturnedHeaders ran, and _dd.appsec.s.res.headers was emitted for blocked requests (res.body was already absent since the action never produced a body). Mark the request blocked at the moment we decide to block (before the BlockException unwinds and the block response is written), so the existing !IsBlocked guard skips the response-phase scan. Non-blocked requests are unaffected and still get res.body/res.headers. Matches the API Security spec (no response schemas on a blocked request) and the system-test Test_Schema_Response_on_Block. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
1 participant
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.
…erage
Consolidate the scattered ASP.NET Core WAF calls (basic request scan, path params, request/response body) into a single request-phase scan (RunRequestScan) plus the existing response-phase scan (CheckReturnedHeaders), keeping the WAF to two runs per request on the happy path.
Hardening on top of the consolidation:
Summary of changes
Reason for change
Implementation details
Test coverage
Other details