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

- Fixes case issue when comparing domain name in Zone10 #737

Merged

Conversation

vlevigneron
Copy link
Contributor

Fixes #734

@matsduf matsduf linked an issue May 20, 2020 that may be closed by this pull request
@matsduf matsduf added the A-TestCase Area: Test case specification or implementation of test case label May 20, 2020
@matsduf matsduf added this to the v2019.2.2 milestone May 20, 2020
@matsduf matsduf requested review from matsduf and mattias-p May 20, 2020 09:51
@@ -699,14 +699,14 @@ sub zone10 {
}
);
}
elsif ( $soa[0]->owner ne $name->fqdn ) {
elsif ( lc( $soa[0]->owner ) ne lc( $name->fqdn ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

I note that lc can use either ASCII rules or Unicode rules depending on context. Are we sure that either ASCII rules will always be applied here, or that LATIN SMALL LETTER I is lowercase of LATIN CAPITAL LETTER I even in Turkish locales?

Also, case insensitivity only applies to ASCII labels, right? lc operates character by character, so this will fail to catch some errors for some binary labels. But maybe we don't care about binary labels?

Copy link
Contributor

@matsduf matsduf May 20, 2020

Choose a reason for hiding this comment

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

No, in Turkish locale lower case of LATIN CAPITAL LETTER I is LATIN SMALL LETTER DOTLESS I.

It is little bit unclear, but in practice we disregard binary labels. It is also unclear about IDN labels, but if they were entered as U-labels they are already converted to A-labels when we come here and can here be treated as ASCII labels.

It is good that you raise the issue, and it should be handled, but this fix is to the better rather than the worse.

Copy link
Contributor

@matsduf matsduf left a comment

Choose a reason for hiding this comment

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

The change has been tested and solves the issue. No problems have been found.

@matsduf
Copy link
Contributor

matsduf commented May 20, 2020

@mattias-p, can you approve? We should maybe create some issues to remind us to straighten out some of the important issues that you raise?

@matsduf
Copy link
Contributor

matsduf commented May 20, 2020

@vlevigneron, please merge.

@vlevigneron vlevigneron merged commit a162c41 into zonemaster:develop May 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-TestCase Area: Test case specification or implementation of test case
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Zone10 does not ignore case when comparing domain names
3 participants