fix: streamed responses #1449

daum3ns · 2025-11-25T08:30:11Z

This pull request fixes #1120 by preventing Coraza from buffering responses when SecResponseBodyAccess off is enabled.

Added streaming_test.go for tests regarding streamed responses.
Introduced a new flag allowFlushing in the interceptor.go

My code includes positive and negative tests.
I have an appropriate description with correct grammar.
I have read Contribution guidelines, maintainers note and Quality standard.
My code is properly linted and passes pre-commit tests.

Thanks for your contribution ❤️

Signed-off-by: daum3ns <[email protected]>

…racks whether the Flush() was invoked. like this we can really check whether the flush was correctly propagated. Signed-off-by: daum3ns <[email protected]>

…on basic functionality, add a test with a rule match in phase 3 Signed-off-by: daum3ns <[email protected]>

case responsebody is not accessable or processable

codecov · 2025-11-25T08:31:07Z

Codecov Report

❌ Patch coverage is 95.58824% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.27%. Comparing base (38f1ed5) to head (d66be96).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
http/e2e/e2e.go	95.08%	1 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1449      +/-   ##
==========================================
+ Coverage   85.12%   85.27%   +0.14%     
==========================================
  Files         173      173              
  Lines        8336     8400      +64     
==========================================
+ Hits         7096     7163      +67     
+ Misses        993      991       -2     
+ Partials      247      246       -1

Flag	Coverage Δ
coraza.rule.case_sensitive_args_keys	`85.23% <95.58%> (+0.14%)`	⬆️
coraza.rule.mandatory_rule_id_check	`85.26% <95.58%> (+0.14%)`	⬆️
coraza.rule.multiphase_evaluation	`84.98% <95.58%> (+0.15%)`	⬆️
coraza.rule.no_regex_multiline	`85.26% <95.58%> (+0.14%)`	⬆️
default	`85.27% <95.58%> (+0.14%)`	⬆️
examples+	`16.51% <ø> (ø)`
examples+coraza.rule.case_sensitive_args_keys	`85.16% <95.58%> (+0.14%)`	⬆️
examples+coraza.rule.mandatory_rule_id_check	`85.26% <95.58%> (+0.14%)`	⬆️
examples+coraza.rule.multiphase_evaluation	`84.78% <95.58%> (+0.15%)`	⬆️
examples+coraza.rule.no_regex_multiline	`85.10% <95.58%> (+0.15%)`	⬆️
examples+memoize_builders	`85.21% <95.58%> (+0.15%)`	⬆️
examples+no_fs_access	`82.84% <95.58%> (+0.16%)`	⬆️
ftw	`85.27% <95.58%> (+0.14%)`	⬆️
memoize_builders	`85.39% <95.58%> (+0.14%)`	⬆️
no_fs_access	`84.79% <95.58%> (+0.15%)`	⬆️
tinygo	`85.24% <95.58%> (+0.14%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: daum3ns <[email protected]>

jcchavezs · 2025-11-25T20:39:08Z

GREAT WORK! Any chance we get a test in e2e? https://github.com/corazawaf/coraza/blob/main/http/e2e/e2e.go

daum3ns · 2025-11-27T08:47:55Z

are the e2e tests intended to be run manually? at least it looks like it expects something running on localhost 8080 and 8081?

Signed-off-by: daum3ns <[email protected]>

jptosso

LGTM, looking to resolve @jcchavezs question

daum3ns · 2025-12-05T08:39:32Z

i think the current e2e setup doesn't allow to easily do that...

TestCase types Signed-off-by: daum3ns <[email protected]>

Signed-off-by: daum3ns <[email protected]>

daum3ns · 2025-12-11T17:05:22Z

I have added the basis for the e2e test, however at the moment there is no verification on client side that the response is streamed. in other words the test also succeeds if we put it in the Run() method, (which shouldn't because the body is buffered)..

Also do you think its better to have an own set of directives, or should I integrate it in the existing directives using this Rule:
SecRule RESPONSE_CONTENT_TYPE "text/event-stream" "id:1001,phase:3,log,allow,ctl:responseBodyAccess=Off"
Or even both maybe its worth having this rule tested too?

streaming callback to testconfiguration Signed-off-by: daum3ns <[email protected]>

Signed-off-by: daum3ns <[email protected]>

runnner". Signed-off-by: daum3ns <[email protected]>

Signed-off-by: daum3ns <[email protected]>

daum3ns · 2025-12-12T15:04:09Z

okay I think this should be fine for an e2e scenario.. I figured out that we can easily do it in the main Run because of the MimeType settings. Response body access is on, but the mime type is not defined (i.e. tx.IsProcessable == false) to be inspected so the body gets streamed correctly. I have additionally added such a setup in the streaming_test.go.
The only reason to maybe keep a separate runner would be if we, for some reason, want an isolated streaming test setup with different directives ... but at the moment I don't think its needed.

however, when I was playing around with the directives, I fount out that removing the SecResponseBodyMimeType directive entirely, it causes a panic in the test "Denied request with a malicious response header"

 [5/9] Running test: Denied request with a malicious response header
2025/12/12 15:56:33 http: panic serving 127.0.0.1:57320: http: wrote more than the declared Content-Length
goroutine 5 [running]:
net/http.(*conn).serve.func1()
	/usr/lib/go/src/net/http/server.go:1943 +0xd3
panic({0xba8960?, 0xc0004125c0?})
	/usr/lib/go/src/runtime/panic.go:783 +0x132
github.com/mccutchen/go-httpbin/v2/httpbin.mustMarshalJSON({0x7f50d2f5c440?, 0xc000438360?}, {0xc8fb20?, 0xc000406a80?})
	/home/doi/go/pkg/mod/github.com/mccutchen/go-httpbin/[email protected]/httpbin/helpers.go:120 +0xbc
github.com/mccutchen/go-httpbin/v2/httpbin.(*HTTPBin).ResponseHeaders(0xc0002ce1e0, {0xe3c1f0, 0xc000438360}, 0xc000404200?)
	/home/doi/go/pkg/mod/github.com/mccutchen/go-httpbin/[email protected]/httpbin/handlers.go:374 +0x2f5
net/http.HandlerFunc.ServeHTTP(0xc00015c180?, {0xe3c1f0?, 0xc000438360?}, 0xc000404200?)
	/usr/lib/go/src/net/http/server.go:2322 +0x29
net/http.(*ServeMux).ServeHTTP(0xe3c1f0?, {0xe3c1f0, 0xc000438360}, 0xc000436280)
	/usr/lib/go/src/net/http/server.go:2861 +0x1c7
github.com/mccutchen/go-httpbin/v2/httpbin.(*HTTPBin).Handler.limitRequestSize.func1({0xe3c1f0?, 0xc000438360?}, 0xc000436280?)
	/home/doi/go/pkg/mod/github.com/mccutchen/go-httpbin/[email protected]/httpbin/middleware.go:42 +0xa5
net/http.HandlerFunc.ServeHTTP(0xc000406360?, {0xe3c1f0?, 0xc000438360?}, 0xcede56?)
	/usr/lib/go/src/net/http/server.go:2322 +0x29
github.com/mccutchen/go-httpbin/v2/httpbin.(*HTTPBin).Handler.preflight.func2({0xe3c1f0, 0xc000438360}, 0xc000436280)
	/home/doi/go/pkg/mod/github.com/mccutchen/go-httpbin/[email protected]/httpbin/middleware.go:33 +0x1eb
net/http.HandlerFunc.ServeHTTP(0x7f50d59c1c00?, {0xe3c1f0?, 0xc000438360?}, 0x0?)
	/usr/lib/go/src/net/http/server.go:2322 +0x29
github.com/mccutchen/go-httpbin/v2/httpbin.(*HTTPBin).Handler.autohead.func3({0xe3c1f0?, 0xc000438360?}, 0xc0004041c0?)
	/home/doi/go/pkg/mod/github.com/mccutchen/go-httpbin/[email protected]/httpbin/middleware.go:62 +0xe9
net/http.HandlerFunc.ServeHTTP(0x8?, {0xe3c1f0?, 0xc000438360?}, 0xc000404170?)
	/usr/lib/go/src/net/http/server.go:2322 +0x29
github.com/mccutchen/go-httpbin/v2/httpbin.(*HTTPBin).ServeHTTP(0xe3c0a0?, {0xe3c1f0?, 0xc000438360?}, 0xe4ae40?)
	/home/doi/go/pkg/mod/github.com/mccutchen/go-httpbin/[email protected]/httpbin/httpbin.go:131 +0x2c
github.com/corazawaf/coraza/v3/testing/e2e_test.TestE2e.WrapHandler.func3({0xe3c0a0, 0xc0004140f0}, 0xc000436280)
	/home/doi/01_workspace/core-waap/coraza/http/middleware.go:148 +0x122
net/http.HandlerFunc.ServeHTTP(0xc00015c540?, {0xe3c0a0?, 0xc0004140f0?}, 0x7b1b56?)
	/usr/lib/go/src/net/http/server.go:2322 +0x29
net/http.(*ServeMux).ServeHTTP(0x47f199?, {0xe3c0a0, 0xc0004140f0}, 0xc000436280)
	/usr/lib/go/src/net/http/server.go:2861 +0x1c7
net/http.serverHandler.ServeHTTP({0xc0003a60c0?}, {0xe3c0a0?, 0xc0004140f0?}, 0x1?)
	/usr/lib/go/src/net/http/server.go:3340 +0x8e
net/http.(*conn).serve(0xc0000b6000, {0xe3e348, 0xc000382120})
	/usr/lib/go/src/net/http/server.go:2109 +0x665
created by net/http.(*Server).Serve in goroutine 21
	/usr/lib/go/src/net/http/server.go:3493 +0x485

this happens in the main branch as well, so it's not introduced by this PR.. But I don't know, do you guys think that it is a problem?

Signed-off-by: daum3ns <[email protected]>

jcchavezs · 2025-12-18T09:20:06Z

Amazing work! I am inclined to merge it. Any thoughts @M4tteoP ?

jcchavezs · 2025-12-18T09:52:25Z

I took the freedom to cherry-pick some of the great improvements in here that are not tied to the streaming support into another PR so we can review this easier #1459

jcchavezs · 2025-12-22T12:01:50Z

Testing the e2e in

coraza-proxy-wasm tests: add-testcase-for-streamed-responses branch coraza-proxy-wasm#312
coraza-caddy tests: add-testcase-for-streamed-responses branch coraza-caddy#253 (I suspect it will fail)

@fionera wanna test it in coraza-spoa?

jcchavezs · 2025-12-22T12:43:59Z

Apparently it works as expected in envoy https://github.com/corazawaf/coraza-proxy-wasm/actions/runs/20431263460/job/58705278664?pr=312#step:9:438

jcchavezs · 2025-12-22T13:21:33Z

It failed as expected in caddy :) https://github.com/corazawaf/coraza-caddy/actions/runs/20432299095/job/58705536937?pr=253#step:7:206

jcchavezs · 2025-12-22T15:34:34Z

Looks like it works, thanks! but I'd like a second review from @M4tteoP

http/e2e/e2e.go

jcchavezs · 2025-12-23T21:14:34Z

http/e2e/e2e.go

+	if totalDeadline <= 0 {
+		totalDeadline = 30 * time.Second
+	}
+	if firstChunkDeadline < 0 {


in which case would this be <0?

http/e2e/e2e.go

Copilot

Pull request overview

This pull request fixes issue #1120 by enabling HTTP response streaming when SecResponseBodyAccess is off or when response body is not processable. The fix prevents Coraza from buffering responses that don't need body inspection, allowing frameworks like Next.js to properly stream content to clients.

Key Changes

Added allowFlushing flag to the response writer interceptor that enables flush propagation when response body inspection is disabled
Implemented comprehensive streaming tests covering multiple scenarios (engine off, MIME type mismatch, response body access off, header blocking, HTTP/1.0)
Extended E2E test framework with SSE streaming verification capabilities

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 9 comments.

File	Description
http/interceptor.go	Added `allowFlushing` flag that is set when response body isn't accessible/processable; modified `Flush()` to propagate flush calls when flag is true
http/streaming_test.go	New comprehensive test file with 5 test cases validating streaming behavior across different WAF configurations and scenarios
http/e2e/e2e.go	Added `StreamCheck` type and `VerifySSEStreamResponse` function for validating progressive SSE streaming; refactored body length checking to use actual body length instead of Content-Length header; added SSE test case
http/e2e/e2e_test.go	Added unit tests for new SSE verification functions and other e2e helper functions; updated status code expectations to use http constants

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

http/interceptor.go

http/streaming_test.go

http/e2e/e2e.go

Co-authored-by: Copilot <[email protected]>

M4tteoP

LGTM

daum3ns added 7 commits November 19, 2025 17:22

add testcases for streamed responses

b878b43

Signed-off-by: daum3ns <[email protected]>

add a flush "spy": it simply wraps existing http.ResponseWriter and t…

5108129

…racks whether the Flush() was invoked. like this we can really check whether the flush was correctly propagated. Signed-off-by: daum3ns <[email protected]>

improve testcases: remove "stream and inspect" tests and concentrate …

10b011b

…on basic functionality, add a test with a rule match in phase 3 Signed-off-by: daum3ns <[email protected]>

add boolean to track whether Flush should be propagated, set it early in

1c4dca6

case responsebody is not accessable or processable

remove debugging outputs

1f96279

Merge branch 'corazawaf:main' into add-testcase-for-streamed-responses

0f57e57

Merge branch 'corazawaf:main' into add-testcase-for-streamed-responses

9b11c8c

daum3ns requested a review from a team as a code owner November 25, 2025 08:30

remove depcrecated build tag

8c4089d

Signed-off-by: daum3ns <[email protected]>

fzipi changed the title ~~Fix streamed responses~~ fix: streamed responses Nov 26, 2025

add testcase for HTTP/1.0

20112c0

Signed-off-by: daum3ns <[email protected]>

jptosso reviewed Nov 27, 2025

View reviewed changes

daum3ns added 3 commits December 11, 2025 16:13

test e2e: refacored Run to use helper functions, added HealthCheck and

42ab7d0

TestCase types Signed-off-by: daum3ns <[email protected]>

add directives and a first SSE test

9a591a8

Signed-off-by: daum3ns <[email protected]>

add basic response body verification

b36988c

Signed-off-by: daum3ns <[email protected]>

daum3ns added 5 commits December 11, 2025 18:21

e2e.go: refactor runTests so that streaming can be checked, add

468642f

streaming callback to testconfiguration Signed-off-by: daum3ns <[email protected]>

add SSE verification

7d7ddb5

Signed-off-by: daum3ns <[email protected]>

add a testcase with SecResponseBodyMimeType

63dcc68

Signed-off-by: daum3ns <[email protected]>

Move streaming test to the main e2e Run and remove dedicated "streaming

b322617

runnner". Signed-off-by: daum3ns <[email protected]>

fix linter complaining

8d59772

Signed-off-by: daum3ns <[email protected]>

daum3ns added 4 commits December 16, 2025 10:22

adding unit tests for helper functions

b4a8a18

Signed-off-by: daum3ns <[email protected]>

space -> tab

d78f640

Signed-off-by: daum3ns <[email protected]>

Merge branch 'main' into add-testcase-for-streamed-responses

1a794f7

add unit tests for verifySSEStreamResponse, runTests and runHealthchecks

6939906

Signed-off-by: daum3ns <[email protected]>

jcchavezs mentioned this pull request Dec 18, 2025

Pre add testcase for streamed responses #1459

Merged

4 tasks

Merge branch 'main' into add-testcase-for-streamed-responses

668723c

Merge branch 'main' into add-testcase-for-streamed-responses

43ebc6f

jcchavezs reviewed Dec 23, 2025

View reviewed changes

http/e2e/e2e.go Outdated Show resolved Hide resolved

jcchavezs reviewed Dec 23, 2025

View reviewed changes

http/e2e/e2e.go Outdated Show resolved Hide resolved

jcchavezs requested a review from Copilot December 23, 2025 21:17

Copilot started reviewing on behalf of jcchavezs December 23, 2025 21:18 View session

Copilot AI reviewed Dec 23, 2025

View reviewed changes

M4tteoP and others added 4 commits December 24, 2025 17:02

Merge branch 'main' into add-testcase-for-streamed-responses

f0b1906

chore: makes lint happy

3511755

tests: adds more tests for SSE

fd97734

fix: wording

d66be96

Co-authored-by: Copilot <[email protected]>

M4tteoP approved these changes Dec 27, 2025

View reviewed changes

jcchavezs merged commit 73368f4 into corazawaf:main Dec 27, 2025
136 checks passed

Uh oh!

fix: streamed responses #1449

fix: streamed responses #1449

Uh oh!

Conversation

daum3ns commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jcchavezs commented Nov 25, 2025

Uh oh!

daum3ns commented Nov 27, 2025

Uh oh!

jptosso left a comment

Choose a reason for hiding this comment

Uh oh!

daum3ns commented Dec 5, 2025

Uh oh!

daum3ns commented Dec 11, 2025

Uh oh!

daum3ns commented Dec 12, 2025

Uh oh!

jcchavezs commented Dec 18, 2025

Uh oh!

jcchavezs commented Dec 18, 2025

Uh oh!

jcchavezs commented Dec 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcchavezs commented Dec 22, 2025

Uh oh!

jcchavezs commented Dec 22, 2025

Uh oh!

jcchavezs commented Dec 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jcchavezs Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

M4tteoP left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

daum3ns commented Nov 25, 2025 •

edited

Loading

codecov bot commented Nov 25, 2025 •

edited

Loading

jcchavezs commented Dec 22, 2025 •

edited

Loading

jcchavezs commented Dec 22, 2025 •

edited

Loading