search

corazawaf / coraza-caddy

OWASP Coraza middleware for Caddy. It provides Web Application Firewall capabilities
https://www.coraza.io/
Apache License 2.0
357 stars 41 forks source link

chore: fixes short write error. #107

Closed jcchavezs closed 1 year ago

jcchavezs commented 1 year ago

Related https://github.com/caddyserver/caddy/pull/5952#issuecomment-1827462757

jcchavezs commented 1 year ago

So we have to define what to do here. The test fails as:

https://github.com/corazawaf/coraza-caddy/actions/runs/7002992650/job/19047820525#step:6:89

[Fail] could not do http request: Get "http://localhost:8080/response-headers?pass=leak": EOF

This is because before we were returning a content-length but the actual response body was empty (due to interruption) but caddy was returning nil and hence no error https://github.com/caddyserver/caddy/pull/5952/files#diff-a248c9a1ec018edea8b377d155bc1df1a642bf79d00ababb5cdacc6b86c5733dL968.

M4tteoP commented 1 year ago

What is the client receiving now that caddy is returning an error? Should we make the e2e-tester more error-tolerant?

jcchavezs commented 1 year ago

I think we should not fail because this is a legit request, we should just deny it.

sonarcloud[bot] commented 1 year ago

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

jcchavezs commented 1 year ago

PTAL @M4tteoP

jcchavezs commented 1 year ago

923 I dont know but I am tempted to say yes #925 indeed, do you mind

porting it?

On Mon, 27 Nov 2023, 23:46 Matteo Pace, @.***> wrote:

@.**** approved this pull request.

Looks good to me! And it is actually an approach that we already used #96 (comment) https://github.com/corazawaf/coraza-caddy/pull/96#discussion_r1287289408 . The interceptor was originally copied from coraza upstream ( https://github.com/corazawaf/coraza/blob/main/http/interceptor.go), can this changes also be ported upstream? Also, vice-versa, we have corazawaf/coraza#923 https://github.com/corazawaf/coraza/pull/923, corazawaf/coraza#925 https://github.com/corazawaf/coraza/pull/925

— Reply to this email directly, view it on GitHub https://github.com/corazawaf/coraza-caddy/pull/107#pullrequestreview-1751555154, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAXOYAUCSRJY7XTC3FATTPTYGUJ2XAVCNFSM6AAAAAA74EDZDKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONJRGU2TKMJVGQ . You are receiving this because you modified the open/close state.Message ID: @.***>