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

Allow underscore character in Uri host #524

Merged
merged 1 commit into from
Nov 19, 2024

Conversation

lulhum
Copy link
Contributor

@lulhum lulhum commented Apr 22, 2024

I don't understand why this validity check has been added. Has per rfc 2181 (https://datatracker.ietf.org/doc/html/rfc2181#section-11), underscore are valid character to use in an uri host.

For my specific usage, it broke for requests using docker internal hostnames.

Copy link
Member

@WyriHaximus WyriHaximus left a comment

Choose a reason for hiding this comment

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

Hey, welcome and thank you for taking the time to report and provide a fix for it. I don't think underscores where intentionally blocked and this somehow slipped through the cracks.

src/Message/Uri.php Show resolved Hide resolved
@WyriHaximus WyriHaximus added this to the v1.11.0 milestone Apr 22, 2024
@SimonFrings
Copy link
Member

@lulhum Thanks for applying these changes 👍

Looks good to me so far. As a last step, can you squash the two commits into one? I don't think we need two separate ones as they're related to each other.

@robertragas
Copy link

@SimonFrings If @lulhum is not responding on the squash in her fork is there anything we can do to speed the issue up?
I also ran into this issue where our production containers have underscores in the host name where I needed to patch it.

As the fix is not that complex, but can be quite impactful I believe we should not wait for the reply for this long.

@lulhum lulhum force-pushed the lulhum-uri-allow-underscore branch from fc14263 to a65d400 Compare August 18, 2024 09:41
@lulhum
Copy link
Contributor Author

lulhum commented Aug 18, 2024

@SimonFrings
Sorry, I missed the squash request, thanks for the reminder @robertragas

Copy link
Member

@WyriHaximus WyriHaximus left a comment

Choose a reason for hiding this comment

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

Thanks for the squash and adding the test 👍

@clue
Copy link
Member

clue commented Aug 29, 2024

@lulhum Thanks for the feedback on the URI host validation check. It appears there may have been some confusion around where underscores are allowed in URIs. As the maintainer who introduced that change in #521 recently, let me try to add some context.

You're correct that per RFC 2181, underscores are valid characters to use in DNS labels, which form the hostname component of a URI. However, the hostname component of a URI is subject to stricter requirements, as outlined in RFC 1123 and RFC 952. These RFCs state that hostnames must consist of alphanumeric characters and hyphens, and underscores are not permitted (see e.g. https://en.wikipedia.org/wiki/Hostname#Syntax).

I can certainly understand the frustration if this broke functionality for your specific use case with Docker internal hostnames. Early versions of Docker Compose did indeed use underscores in container names, but the more recent v2.0 release (released 2021) switched to using hyphens instead, following the hostname restrictions (docker/compose#472).

While some other projects may be more lenient and accept underscores in hostnames, the RFCs are clear that this is not the correct behavior. That said, I'm open to further discussion on the potential impact and whether a more flexible approach could be warranted. Perhaps there are ways to strike a balance between strict RFC compliance and practical usability while keeping security in mind (https://claroty.com/team82/research/exploiting-url-parsing-confusion).

What do you think would be the best path forward here? I'm open to suggestions on how to balance strict adherence to the standards with practical considerations for users. I'm happy to discuss further, so please feel free to share any additional thoughts or suggestions you may have.

I don't understand why this validity check has be added.
Has per rfc 2181
(https://datatracker.ietf.org/doc/html/rfc2181#section-11), underscore
are valid character to use in an uri host.

For my specific usage, it broke for requests using docker internal
hostnames.

added test to prevent regression on URI containing underscore in host
@clue clue force-pushed the lulhum-uri-allow-underscore branch from a65d400 to 888e3c0 Compare November 19, 2024 22:11
@clue clue changed the title Allow undescore character in Uri host Allow underscore character in Uri host Nov 19, 2024
@clue clue added new feature and removed bug labels Nov 19, 2024
@clue
Copy link
Member

clue commented Nov 19, 2024

@lulhum Let's move forward with this one! :shipit: I've only fixed a minor typo, rebased your changes and added another test, changes look good to me otherwise as discussed above, so let's get this shipped! Keep it up! 👍

@clue clue requested review from WyriHaximus and removed request for SimonFrings November 19, 2024 22:14
@WyriHaximus WyriHaximus merged commit 71c0e5c into reactphp:1.x Nov 19, 2024
14 checks passed
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.

5 participants