Skip to content

[DependencyInjection][HttpClient][Routing] Reject vertical tab in URIs #59511

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

Closed

Conversation

alexandre-daubois
Copy link
Member

Q A
Branch? 7.3
Bug fix? no
New feature? no
Deprecations? no
Issues -
License MIT

Follows:

Also, \v is not supported in the @testWith annotation, thus converting them to data providers.

@nicolas-grekas
Copy link
Member

Why exclude vertical tabs?

@alexandre-daubois
Copy link
Member Author

Like horizontal tabs and other control characters, this char should not appear in a URL. We already have a check for control chars, this one can be added for completeness?

@nicolas-grekas
Copy link
Member

Vertical tabs are not mentioned on the URL spec: https://url.spec.whatwg.org/
We have to follow the standards.
👎

@alexandre-daubois
Copy link
Member Author

alexandre-daubois commented Jan 17, 2025

The URL spec mentions that C0 controls are forbidden, and \v is in the range of C0 control chars. Maybe I misunderstand a bit, C0 seem forbidden in hosts and domains only. Maybe this check should be done only on the host, not the whole URL

@nicolas-grekas
Copy link
Member

I can't see where the vertical tab needs this special handling. Please link to the exact sentences you have in mind?

@alexandre-daubois
Copy link
Member Author

alexandre-daubois commented Jan 17, 2025

I don't think the vertical tab needs a special check, the proposed patch is not a good idea as explained by your comments. Reading the spec again, I'm wondering if we should still ensure the host doesn't contain C0 control chars, as stated in https://url.spec.whatwg.org/#forbidden-host-code-point ? There are check that ensure there are no leading/trailing C0 chars in the URL, but it does not check the host if I understand correctly RequestContext and HttpClientTrait

@nicolas-grekas
Copy link
Member

I'm not sure. Which problem would that solve?
BTW parse_url already does something with C0 chars: https://3v4l.org/SZMC5

@alexandre-daubois
Copy link
Member Author

The code would closer follow the living standard. Indeed parse_url seems to handle it but the output seems "wrong" as the spec says it should be invalid.
If you're not convinced, let's close this PR and wait for a real world use case to appear, I'm ok with that 🙂

@nicolas-grekas
Copy link
Member

Let's close yes. We don't need a spec-compliant parser here. There are third party libs for that.

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

Successfully merging this pull request may close these issues.

3 participants