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

Let header validator find host header field when :authority pseudo-header field is missing #324

Merged
merged 2 commits into from
Feb 5, 2025

Conversation

naokiiwakami
Copy link
Contributor

Motivation:

Some proxy server sends target host name and port number in host header field instead of
using :authority pseudo-header field. According to the HTTP/3 spec, this is a valid
way to send the target endpoint, but current header validator checks only :authority header.
It causes false-positive request validation errors.

Modifications:

Class Http3HeadersSink checks if host header field exists when :authority pseudo-header is
missing in the request.

Result:

No false-positive request validation errors.

…eader field is missing

    Motivation:

    Some proxy server sends target host name and port number in host header field instead of
    using :authority pseudo-header field. According to the HTTP/3 spec, this is a valid
    way to send the target endpoint, but current header validator checks only :authority header.
    It causes false-positive request validation errors.

    Modifications:

    Class Http3HeadersSink checks if host header field exists when :authority pseudo-header is
    missing in the request.

    Result:

    No false-positive request validation errors.
@normanmaurer
Copy link
Member

@naokiiwakami did you sign our icla yet ? https://netty.io/s/icla

@normanmaurer normanmaurer added this to the 0.0.29.Final milestone Feb 4, 2025
* https://www.rfc-editor.org/rfc/rfc9110#section-7.2
*/
private boolean authorityOrHostHeaderReceived() {
return (receivedPseudoHeaders & AUTHORITY.getFlag()) == AUTHORITY.getFlag() || headers.contains("host");
Copy link
Member

Choose a reason for hiding this comment

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

Use HttpHeaderNames.HOST

@@ -16,6 +16,8 @@
package io.netty.incubator.codec.http3;


import com.google.common.net.HttpHeaders;
Copy link
Member

Choose a reason for hiding this comment

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

Replace by our own HttpHeaderNames class

        Motivation:

        To replace hard-coded string and to remove unnecessary dependency.

        Modifications:

        Used netty's HttpHeaderNames class for host header field name.

        Result:

        Cleaner code.
@naokiiwakami
Copy link
Contributor Author

Hi Norman, thanks for looking into the PR. I have addressed your comments.

@naokiiwakami did you sign our icla yet ? https://netty.io/s/icla

I have signed it.

@naokiiwakami
Copy link
Contributor Author

naokiiwakami commented Feb 4, 2025

The automated test failed https://github.com/netty/netty-incubator-codec-http3/actions/runs/13137920730/job/36657661053

It seems that the test does not allow a PR from a fork. But I only have a fork. Did I do anything wrong?

Run dawidd6/action-download-artifact@v6
==> Repository: netty/netty-incubator-codec-http3
==> Artifact name: test-results-build
==> Local path: ./
==> Workflow name: 4461213
==> Workflow conclusion: completed
==> Commit: da31eec
==> Allow forks: false
==> Skipping run from fork: naokiiwakami/netty-incubator-codec-http3
Error: no matching workflow run found with any artifacts?

@normanmaurer normanmaurer merged commit 8ee0949 into netty:main Feb 5, 2025
4 checks passed
@normanmaurer
Copy link
Member

@naokiiwakami Thanks a lot!

@naokiiwakami naokiiwakami deleted the fix-323 branch February 10, 2025 23:33
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