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

Add validation for :path pseudo-header in HTTP/2 requests RFC7540 #428

Merged
merged 1 commit into from
Sep 20, 2023

Conversation

arturobernalg
Copy link
Member

@arturobernalg arturobernalg commented Sep 6, 2023

This PR introduces validations for the :path pseudo-header field in accordance with the HTTP/2 specifications:

  1. Ensures the :path pseudo-header field correctly represents the path and query parts of the target URI. Adheres to the "path-absolute" production. Optionally allows for a '?' character followed by the "query" production (as per Sections 3.3 and 3.4 of [RFC3986]).

  2. Validates that requests in asterisk form have the value '*' for the :path pseudo-header field.

  3. Enforces that the :path pseudo-header field is not left empty for "http" or "https" URIs.

  4. Guarantees that "http" or "https" URIs without a path component include a '/' value for the :path pseudo-header field.

  5. Implements an exception for OPTIONS requests:
    If the request targets an "http" or "https" URI and lacks a path component, it must set the :path pseudo-header field to '*'.

Copy link
Member

@ok2c ok2c left a comment

Choose a reason for hiding this comment

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

@arturobernalg What RFC have you been using as a reference for change?

@arturobernalg
Copy link
Member Author

arturobernalg commented Sep 18, 2023

@arturobernalg What RFC have you been using as a reference for change?

@ok2c
this one RFC7540

This specific section:
This pseudo-header field MUST NOT be empty for "http" or "https" URIs; "http" or "https" URIs that do not contain a path component MUST include a value of '/'. The exception to this rule is an OPTIONS request for an "http" or "https" URI that does not include a path component; these MUST include a ":path" pseudo-header field with a value of '*' (see [[RFC7230], Section 5.3.4](https://datatracker.ietf.org/doc/html/rfc7230#section-5.3.4)).

@arturobernalg arturobernalg requested a review from ok2c September 18, 2023 18:31
} else {
if (Method.OPTIONS.isSame(method)) {
if (!"*".equals(path)) {
throw new ProtocolException("OPTIONS request for an 'http' or 'https' URI without a path component must have a ':path' pseudo-header field with a value of '*'");
Copy link
Member

Choose a reason for hiding this comment

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

@arturobernalg This still looks wrong. Why should OPTIONS with the / path should be rejected as invalid? What you want to do it reject the path if it does not start with '/' but make an exception for OPTIONS if the path is *.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ok2c please check again.

This commit adds robust validation for the :path pseudo-header field in HTTP/2 requests, as specified in RFC 7540. It ensures that:

- The :path field is not empty for 'http' or 'https' URIs.
- For OPTIONS requests without a path component, the :path field must be '*'.
- For 'http' or 'https' URIs, the :path field must either start with '/' or be '*'.
@arturobernalg arturobernalg merged commit dc17d58 into apache:5.3.x Sep 20, 2023
7 checks passed
ok2c pushed a commit that referenced this pull request Dec 13, 2023
…#428)

This commit adds robust validation for the :path pseudo-header field in HTTP/2 requests, as specified in RFC 7540. It ensures that:

- The :path field is not empty for 'http' or 'https' URIs.
- For OPTIONS requests without a path component, the :path field must be '*'.
- For 'http' or 'https' URIs, the :path field must either start with '/' or be '*'.
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