Skip to content

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

Merged
vlevigneron merged 1 commit into
zonemaster:developfrom
vlevigneron:fix-issue-zonemaster-engine-734
May 20, 2020
Merged

- Fixes case issue when comparing domain name in Zone10#737
vlevigneron merged 1 commit into
zonemaster:developfrom
vlevigneron:fix-issue-zonemaster-engine-734

Conversation

@vlevigneron

Copy link
Copy Markdown
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
);
}
elsif ( $soa[0]->owner ne $name->fqdn ) {
elsif ( lc( $soa[0]->owner ) ne lc( $name->fqdn ) ) {

Copy link
Copy Markdown
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?

@matsduf matsduf May 20, 2020

Copy link
Copy Markdown
Contributor

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.

@matsduf matsduf left a comment

Copy link
Copy Markdown
Contributor

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

matsduf commented May 20, 2020

Copy link
Copy Markdown
Contributor

@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

matsduf commented May 20, 2020

Copy link
Copy Markdown
Contributor

@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