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

feat: add underscore support #43

Merged
merged 2 commits into from
Feb 18, 2023
Merged

Conversation

patrykcieszkowski
Copy link
Contributor

Adds support for underscore character (_) in domain names. While registers don't allow it for top-level domains, it should be supported based on RFC specification, and is widely used for subdomanis (e.g. letsencrypt dns). To not break backwards compatibility - I've set it to false, by default.

https://stackoverflow.com/a/2183140/2225918

PS I'm aware of the test coverage issue. I can't manage to get it right.

@Marsup
Copy link
Collaborator

Marsup commented Oct 11, 2022

Hi,

Thanks for the PR!
Correct me if I'm wrong, but I don't think domains should be allowed to have underscores, only subdomains, right?
As for the coverage issue, you're never triggering the error case, you'll need another case with that new option and something such as _abc.{example}.com

@MartinKolarik
Copy link

@Marsup as per the RFC the underscore could appear anywhere. In practice, I'm not aware of second-level domains that use it, but it isn't forbidden (at least not directly in the spec but each TLD can impose additional restrictions):

The DNS itself places only one restriction on the particular labels
that can be used to identify resource records. That one restriction
relates to the length of the label and the full name. The length of
any one label is limited to between 1 and 63 octets. A full domain
name is limited to 255 octets (including the separators).

@Marsup
Copy link
Collaborator

Marsup commented Oct 11, 2022

Trying to register a domain with an underscore doesn't seem to work in any of the registrar I've tried. Some even mention that underscore is not an allowed character.

@patrykcieszkowski
Copy link
Contributor Author

Well, that's true, but that doesn't mean much:

  1. The RFC document defining the syntax doesn't disallow underscores, and therefore - there are DNSes operating with underscore present. The letsencrypt DNS is the best example I could think of.
  2. Most (if not all) DNS servers allow for subdomains with underscore to be registered.

Copy link
Member

@Nargonath Nargonath left a comment

Choose a reason for hiding this comment

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

LGTM considering some DNS providers accept the underscore in sub-domains and domains as other mentioned. At least the feature is disabled by default, it's up to the user to enable it if they want to be opened to such domains causing no harm to all users that don't care about it.

@patrykcieszkowski
Copy link
Contributor Author

As for the coverage issue, you're never triggering the error case, you'll need another case with that new option and something such as _abc.{example}.com

I tried that, and it didn't work for me. Maybe somebody else could play around and try to figure it out.

@Marsup
Copy link
Collaborator

Marsup commented Oct 13, 2022

Weird, adding ['_abc.{example}.com', false, { allowUnderscore: true }] does it for me.

@patrykcieszkowski
Copy link
Contributor Author

Weird, adding ['_abc.{example}.com', false, { allowUnderscore: true }] does it for me.

You're right, It passed. Thanks!

@Marsup
Copy link
Collaborator

Marsup commented Nov 22, 2022

FYI I haven't forgotten about this PR, but since I know you're probably expecting it in joi afterwards, I need to figure out (which is mostly done) why the new address adds so much to the browser bundle, and how to come back to a better place, but this PR is valid as is.

@Marsup Marsup added this to the 5.0.1 milestone Feb 18, 2023
@Marsup Marsup merged commit 13e39e4 into hapijs:master Feb 18, 2023
@Marsup Marsup modified the milestones: 5.0.1, 5.1.0 Feb 18, 2023
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.

5 participants