Let hostname() accept a bare single-label name with the RFC 1034 dot - #479
Open
afonsojanu wants to merge 1 commit into
Open
Conversation
hostname("yu", rfc_1034=True) passes today through the maybe_simple
fast path, since _simple_hostname_regex has no problem with a lone
alphanumeric label. hostname("yu.", rfc_1034=True) doesn't, because
that regex has no notion of a trailing dot at all, and the fallback to
domain() requires at least a second label before the TLD. So the exact
same name is accepted or rejected purely based on whether it carries
the dot RFC 1034 is supposed to permit, which is backwards from what
that flag promises.
Added a small wrapper around the simple-hostname match that strips a
lone trailing dot first when rfc_1034 is set, mirroring what domain()
already does for its own regex. Left everything else about the simple
path untouched, so this doesn't change hostname() for the vast
majority of callers who never pass rfc_1034.
Fixes python-validatorsGH-442.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #442, at least the hostname() half of it.
hostname("yu", rfc_1034=True) already passes today, through the maybe_simple fast path, since _simple_hostname_regex doesn't care whether a lone alphanumeric label has a dot after it or not. hostname("yu.", rfc_1034=True) doesn't pass, though, because that regex has no notion of a trailing dot at all, and falling back to domain() needs at least a second label before the TLD. So the exact same name gets accepted or rejected depending only on whether it carries the dot that rfc_1034 is supposed to permit, which is the wrong way around.
Fix: strip a lone trailing dot before running the simple-hostname match when rfc_1034 is set, the same thing domain() already does for its own regex a bit further down. Everything else about the simple path is untouched, so this shouldn't move the needle for the vast majority of callers who never pass rfc_1034.
I looked at domain()'s own half of the issue too (a bare TLD with no second label at all, e.g. "com" or "com."), but that's a bigger question about whether a single label should count as a "domain" on its own, separate from the hostname() inconsistency this PR closes. Left that alone rather than guess at the intended scope.
Added two new cases to the existing parametrized true/false tests in test_hostname.py rather than a separate test function, since they slot into the same shape already there. Confirmed via git stash that both new true-cases fail on unpatched code with the same ValidationError from the issue, and pass with the fix. Full local suite: 882 passing (17 unrelated failures from crypto_addresses/test_eth_address.py needing an optional extra that isn't installed here, same before and after this change).