Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: fixes short write error. #107

Merged
merged 2 commits into from
Nov 27, 2023
Merged

chore: fixes short write error. #107

merged 2 commits into from
Nov 27, 2023

Conversation

jcchavezs
Copy link
Member

@jcchavezs jcchavezs commented Nov 27, 2023

@jcchavezs
Copy link
Member Author

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
Copy link
Member

M4tteoP commented Nov 27, 2023

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

@jcchavezs
Copy link
Member Author

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

Copy link

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 jcchavezs enabled auto-merge (squash) November 27, 2023 22:31
@jcchavezs
Copy link
Member Author

PTAL @M4tteoP

Copy link
Member

@M4tteoP M4tteoP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jcchavezs jcchavezs merged commit 8e628a0 into main Nov 27, 2023
7 checks passed
@jcchavezs jcchavezs deleted the fixes_short_write branch November 27, 2023 23:03
@jcchavezs
Copy link
Member Author

jcchavezs commented Nov 27, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants