[AppSec] Send the real response status to the WAF and stop resending the request addresses - #9082
Conversation
![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: 98f2a72c31
ℹ️ 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".
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (9082) and master. ✅ No regressions detected |
BenchmarksBenchmark execution time: 2026-09-09 15:27:58 Comparing candidate commit 3924a44 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 72 metrics, 0 unstable metrics, 65 known flaky benchmarks, 61 flaky benchmarks without significant changes.
|
ebd367b to
612eb10
Compare
There was a problem hiding this comment.
This head is now 40 commits behind the repository's current master, and a merge simulation reports a conflict in SecurityCoordinator.Framework.cs. Please update the branch and preserve both this PR's removal of the request-phase ResponseStatus and master's newer _localRootSpan.GetHttpClientIp() ?? _localRootSpan.GetNetworkClientIp() accessors, then rerun CI.
The main behavior is otherwise coherent, but the late ASP.NET session-cookie path currently reads cookies before the session-ID access that the PR says creates that cookie.
612eb10 to
b6d35be
Compare
|
Rebased on master (b6d35be), keeping both sides of the Review feedback addressed in the same push:
Security unit tests: 1082 passed on net8.0, 1066 on net48. Replied inline on the |
There was a problem hiding this comment.
Please make these last updates before merge:
- The PR summary says
server.response.statusis sent on the last run only. For endpoint-less Core requests,Scan(lastTime: true)can supply the status andCheckReturnedHeaderscan perform a later response scan, so the status may be sent twice even though the WAF does not report the same match twice. Please describe the guarantee as omitting the status during request-phase scans and supplying it once the real response status is available. - The summary's "once per request" address claim applies to Core's one-shot collection. Framework intentionally refreshes the basic addresses on each
BeginRequest, including the second pipeline created byTransferRequest. Please qualify the claim so it does not imply the Framework path uses the same lifetime.
|
Both wording updates are in (4a8a983), and the PR description is updated for the two summary points:
Comment-only changes on top of the previous push, |
This comment has been minimized.
This comment has been minimized.
4a8a983 to
a6248ef
Compare
e-n-0
left a comment
e-n-0
left a comment
There was a problem hiding this comment.
Reviewed at the current head. The earlier correctness and documentation findings are resolved: the late session-cookie ordering, the TransferRequest handling, the contradictory test summary, and the address-lifetime wording all look fixed now.
Two things I'd still like covered, neither of them blocking:
-
The two lifecycle behaviors this PR introduces are untested end to end. ResponseStartHookTests pins the IHttpResponseFeature type-name check, but nothing covers the response-start/end fallback ownership (when CheckResponseAtRequestEnd is allowed to scan versus leaving it to the hook), and nothing covers the lazy Framework session-cookie materialization in GetEndRequestArgsForWaf.
-
The title "scan each request once" is a bit misleading. The change makes ASP.NET Core request-address collection one-shot; it does not reduce a request to one WAF invocation. Path, body, and response runs still happen, and endpoint-less requests can carry the status twice, from Scan(lastTime: true) and again from CheckReturnedHeaders. Something like "send request addresses once" would be closer to what the code does.
On benchmarks: the latest execution-time report shows no regressions. BenchmarkDotNet flags DbCommand.ExecuteNonQuery net472 at roughly -5.4% to -6.2%, but that path isn't touched here, so it looks unrelated.
dromanol
changed the title
|
Thanks for the review. Wording and test-fixture points are in (4f7fe5c), and the title is now "Send the real response status to the WAF and stop resending the request addresses" — you're right that the old one promised one WAF run per request, which isn't what this does. On the two remaining points: 1. End-to-end coverage. Agreed that both are unit-level only right now, and neither can go further in a unit test:
Neither is in this PR; I'd rather do them as a follow-up than bolt an integration host onto it. Happy to open the issue if you want it tracked. 2. Benchmarks. Same read here. |
There was a problem hiding this comment.
Just approving the tracing side, haven't reviewed the ASM side thoroughly, but just wondering why we're not using the OnStarting hook for aspnetcore? Unless we already are, and it's just not visibile in this diff 😄
|
@andrewlock good question — it's not visible in the diff because the response-start hook predates this PR: it's a CallTarget on The reason it isn't a registered That said, you're right that
Tracked in APPSEC-70098, with the detail above and the scope (one-shot guard so the callback and the hook can't both scan, HTTP.sys test host). @e-n-0 the two end-to-end coverage gaps from your review are now tracked in APPSEC-70099 (HTTP.sys vs Kestrel/IIS scan ownership, and the lazy Framework session cookie), cross-linked with APPSEC-70098 since the first one changes shape if the HTTP.sys path moves to |
…st once server.response.status came from the request-phase address set, where HttpContext.Response.StatusCode is still the default 200. It is now sent only on the last run of the request, together with the response headers. The request addresses are supplied once: the WAF keeps them for the life of its context, so a later run re-evaluates rules and processors against the stored values instead of paying for the whole set again. ASP.NET's session cookie is the exception, since it only lands in Request.Cookies once the session id is read, so the end of request run re-reads the cookies. Servers without the instrumented response start hook (HTTP.sys) never saw a real status at all; they now get a report-only scan at the end of the pipeline. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A Server.TransferRequest pipeline shares the trace context, so the one-shot flag swallowed the transferred request's addresses. EndRequest starts from an empty set instead. Also pins that a re-supplied status can't report twice. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Read the ASP.NET session id before snapshotting the request cookies, so the session cookie is in there when the fingerprint is built. Add coordinator-level tests for the one-shot request addresses and the end-of-request status, and fix the WafAddressReuseTests summary. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The one-shot address collection is the Core path only, and the session id read at end of request is a guard for hosts where SessionStateModule hasn't made the cookie visible yet. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ture The coordinator tests mock the WAF, so they don't need the native library initialised by WafLibraryRequiredTest, only the sequential collection. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#9090 changed IWaf.CreateContext to report a typed outcome through an out parameter; the tests added here still used the old overload.
4f7fe5c to
3924a44
Compare
3 participants
Summary of changes
server.response.statusreaches the WAF with the real status code: it is never sent during the request-phase scans, and it is sent once the real response status is known. On ASP.NET Core requests that reach the end of the pipeline without an endpoint, bothScan(lastTime: true)and the later response scan can carry it — the WAF doesn't report the same match twice.BeginRequest, including the second pipeline aServer.TransferRequestcreates, which is deliberate.Reason for change
The status came from the request-phase address set, where
HttpContext.Response.StatusCodeis still the default200, so status rules were fed a fabricated value — and on HTTP.sys that was the only status the WAF ever saw. Resending every request address on the later runs also re-evaluated the whole request rule set for no new information.The same gap existed on Kestrel for responses the server writes itself. On an unhandled exception the 500 is produced by
ProduceEnd(), afterapplication.ProcessRequestAsync()has returned, so the scope set by the diagnostic observer inside that call is no longer visible in theAsyncLocal:FireOnStartingfinds no active span and skips the WAF entirely. Those requests reached the WAF without a response status or response headers at all.Implementation details
Two one-shot flags on
AppSecRequestContext, both driving the Core path. Persistent addresses live for the whole WAF context, so later runs re-evaluate rules, schemas and fingerprints against the stored values.server.request.cookieson ASP.NET Framework is the one address the end-of-request run re-reads. ASP.NET putsASP.NET_SessionIdinRequest.Cookieswhen the session id is read, whichSessionStateModulenormally does atAcquireRequestState, well beforeEndRequest; the explicitSessionIDread inGetEndRequestArgsForWafis the guard for hosts where the cookie isn't visible yet, and it has to happen before the cookies are snapshotted. Without the re-read the cookie halves of_dd.appsec.fp.sessiongo empty.Test coverage
New unit tests against the real WAF: status rule matching per phase, schemas surviving a run without resupplied addresses, session fingerprint vs late cookies, server detection. Coordinator-level tests for the one-shot address collection, the omission of the status during the request phase and the real status at the end of the request, on both Core and Framework. Green locally: Security unit tests net8.0/net48, Security integration AspNetCore net8.0 + AspNetMvc5 net48, and system-tests
APPSEC_BLOCKING,APPSEC_API_SECURITY,APPSEC_WAF_TELEMETRY,APPSEC_RASP,APPSEC_AUTO_EVENTS_EXTENDED,DEFAULT.Because error responses now reach the last WAF run, API Security samples them too and error spans can carry
_dd.appsec.s.*schemas. That breaks theDEBUGGER_EXCEPTION_REPLAYspan approvals, where the schemas land on a different request every run (the sampler's 30s window against the test's 30s retry interval): DataDog/system-tests#7543 scrubs them like the_dd.appsec.fp.*fingerprints already are.Other details
Replaces #8856 (rewritten from scratch, not rebased). Left out on purpose: registering
Response.OnStarting(...)fromBlockingMiddlewarewould work on every server and let HTTP.sys block, but it means removing a calltarget and redoing the blocking and ordering story.