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

Ensure authority and/or host fields are not empty #232

Merged
merged 3 commits into from
Jun 1, 2024

Conversation

Pi-Cla
Copy link

@Pi-Cla Pi-Cla commented Mar 20, 2024

We could have an authority or host that consists of an empty string

@Pi-Cla Pi-Cla force-pushed the empty-fields branch 3 times, most recently from 8180959 to 9c3c83c Compare March 20, 2024 19:11
@Pi-Cla
Copy link
Author

Pi-Cla commented Mar 20, 2024

I am unsure how to correctly to create a HeaderField with an empty value for the tests. If that is even possible. If it isn't then maybe this PR is redundant and I should only update the compliance?

@Ruben2424
Copy link
Contributor

Hi, thanks for the PR!
You are using the try_from method in the test to create the header. This method is also used wen receiving a request.
try_from parses the header field to a typed header field from the http crate which allows no empty values.

I would leave the check and the test for the host field where it is.
For the authority field, i would copy the compliance comment here and assert the result of try_from instead of into_request_parts

@Pi-Cla
Copy link
Author

Pi-Cla commented Mar 20, 2024

@Ruben2424 as it turns out, once I got the tests working I discovered that the initial code already errors out when either field is empty. The host field errors out when it is built into a Uri since the Uri Builder rejects empty Uris. At least we have more tests and documentation now

@@ -202,6 +201,9 @@ impl Iterator for HeaderIter {
}
}

//= https://www.rfc-editor.org/rfc/rfc9114#section-4.3.1
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good so far. Do you mind moving that comment to the Field::parse method? The parse method will return the error when authority is empty. So one can find the location when looking through the compliance report.
iirc the error comes from here:

b":authority" => Field::Authority(try_value(name, value)?),

@Ruben2424 Ruben2424 merged commit d1ec7c4 into hyperium:master Jun 1, 2024
10 checks passed
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