8000 test: default to HTTP/1.1 protocol version for all tests instead of HTTP/1.0 by daum3ns · Pull Request #4043 · coreruleset/coreruleset · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

test: default to HTTP/1.1 protocol version for all tests instead of HTTP/1.0 #4043

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

Merged

Conversation

daum3ns
Copy link
Contributor
@daum3ns daum3ns commented Mar 21, 2025

tests which explicitly test wrong or not allowed versions are not changed. (in REQUEST-920-PROTOCOL-ENFORCEMENT/920430.yaml and REQUEST-920-PROTOCOL-ENFORCEMENT/920280.yaml).

…TTP/1.0

tests which explicitly test wrong or not allowed versions are not changed.
(in REQUEST-920-PROTOCOL-ENFORCEMENT/920430.yaml and
REQUEST-920-PROTOCOL-ENFORCEMENT/920280.yaml).
@airween
Copy link
Contributor
airween commented Mar 21, 2025

Hi @daum3ns,

thanks for this PR - unfortunately there is failed test in case of both engines' regression test. Have you tested these modifications with Apache2 and Nginx before you sent this PR?

@daum3ns
Copy link
Contributor Author
daum3ns commented Mar 21, 2025

i am currently looking into it, its the same test for both engines that fails..

@daum3ns
Copy link
Contributor Author
daum3ns commented Mar 21, 2025

Ok, I figured it out. In HTTP/1.0 an empty host header is allowed, and therefore the rule is expected to block the request in the test..
in HTTP/1.1 an empty host header is not allowed, so apache as well as nginx directly block it with an http 400 response and the rule does not get evaluated, therefore the test fails.
As the tested rule explicitly tests for the empty host header, i would suggest to go back to HTTP/1.0 for this test.

@airween What do you think?

@airween
Copy link
Contributor
airween commented Mar 21, 2025

What do you think?

Yes, changing the protocol version in that case is a good idea. Probably you might add some extra comment here.

@theseion
Copy link
Contributor

I would rather change the way we deal with versions than simply changing the default. For most tests we expect them to work for any HTTP version and we should really test that that assumption is true. I propose to update go-ftw, and possibly the test schema, to run each test against multiple HTTP versions, unless a test defines a restriction. I also think we should transform the HTTP version restrictions into ranges, such that we can say:

  • run for HTTP/1.0 only
  • run for > HTTP/1.0
  • run for < HTTP/2
  • run for HTTP/1.0 - HTTP/1.1
  • ...

@dune73
Copy link
Member
dune73 commented Mar 23, 2025

I do not get the reasoning behind your proposal @theseion. Could you elaborate?

@theseion
Copy link
Contributor

Updating from HTTP/1.0 to HTTP/1.1 is fine. However:

  • all upgraded tests will be run against 1.1 only
  • we basically lose the information that all of those tests work for 1.0
  • the tests that actually target 1.0 could potentially also be valid for other versions but we have no way of declaring that, except for duplicating such tests
  • the internet is running on HTTP/2.0 and HTTP/2 (quic) and we should run our tests against those protocols
  • we can update all tests now but will have to do the same thing again for HTTP/2.0 at some point, with the same implications as above

Does that answer your question @dune73?

@dune73
Copy link
Member
dune73 commented Mar 24, 2025

Got you.

As of this writing we do not know if the tests work with HTTP/1.1 and HTTP/2 etc. Your proposal expands the coverage across various protocols. That sounds good.

Yet, I wonder if this is really necessary for every test. I see a lot of tests with application/x-www-form-urlencoded and no special HTTP headers or behavior. So I can't see how they might fail. And running these tests against multiple protocols is just a futile exercise that slows down testing.

@fzipi
Copy link
Member
fzipi commented Mar 24, 2025

I see two different discussions here. One related to this PR, which I think we will be able to merge without further changes first.

Then we should open a new issue in go-ftw and handle the discussion here around http versions.

@fzipi fzipi changed the title test: Default to HTTP/1.1 protocol version for all tests instead of HTTP/1.0 test: default to HTTP/1.1 protocol version for all tests instead of HTTP/1.0 Mar 24, 2025
Copy link
Member
@fzipi fzipi left a comment

Choose a reason for hiding this comment

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

LGTM.

@theseion theseion added this pull request to the merge queue Mar 24, 2025
Merged via the queue into coreruleset:main with commit fd992ac Mar 24, 2025
7 checks passed
@theseion
Copy link
Contributor

Created issue here: coreruleset/go-ftw#480

@daum3ns daum3ns deleted the default-to-http-1.1-in-regression-tests branch March 26, 2025 08:20
@fzipi fzipi added the release:ignore Ignore for changelog release label Mar 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:ignore Ignore for changelog release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants
0