Skip to content

Conversation

@lulhum
Copy link
Contributor

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

@WyriHaximusWyriHaximus 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.

@WyriHaximusWyriHaximus 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.

@lulhumlulhumforce-pushed the lulhum-uri-allow-underscore branch from fc14263 to a65d400CompareAugust 18, 2024 09:41
@lulhum
Copy link
ContributorAuthor

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

Copy link
Member

@WyriHaximusWyriHaximus 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
@clueclueforce-pushed the lulhum-uri-allow-underscore branch from a65d400 to 888e3c0CompareNovember 19, 2024 22:11
@clueclue changed the title Allow undescore character in Uri hostAllow underscore character in Uri hostNov 19, 2024
@clueclue 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! 👍

@clueclue requested review from WyriHaximus and removed request for SimonFringsNovember 19, 2024 22:14
@WyriHaximusWyriHaximus merged commit 71c0e5c into reactphp:1.xNov 19, 2024
14 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@lulhum@SimonFrings@robertragas@clue@WyriHaximus