-
Notifications
You must be signed in to change notification settings - Fork 33
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
- Fixes case issue when comparing domain name in Zone10 #737
Conversation
@@ -699,14 +699,14 @@ sub zone10 { | |||
} | |||
); | |||
} | |||
elsif ( $soa[0]->owner ne $name->fqdn ) { | |||
elsif ( lc( $soa[0]->owner ) ne lc( $name->fqdn ) ) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
@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? |
@vlevigneron, please merge. |
Fixes #734