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

remove domains final dots #253

Merged
merged 5 commits into from
May 12, 2022
Merged

Conversation

hannaeko
Copy link
Member

@hannaeko hannaeko commented Apr 28, 2022

Purpose

Do not crash when a domain is specified with a final dot.

Context

zonemaster/zonemaster-engine#1055

Changes

  • Remove final dot from domain if not root
  • Remove final dot from NS

How to test this PR

  • ../zonemaster-cli/script/zonemaster-cli --ns ns2.nic.fr. zonemaster.net. --show-testcase should run the test without crashing
  • ../zonemaster-cli/script/zonemaster-cli --ns ns2.nic.fr.. zonemaster.net. --show-testcase should show an error
  • ../zonemaster-cli/script/zonemaster-cli --ns ns2.nic.fr. zonemaster.net.. --show-testcase should show an error

lib/Zonemaster/CLI.pm Outdated Show resolved Hide resolved
ghost
ghost previously approved these changes May 2, 2022
@@ -601,6 +603,8 @@ sub add_fake_delegation {
exit( 1 );
}

$name =~ s/\.$// unless $name eq '.';;
Copy link

Choose a reason for hiding this comment

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

typo: two semi-colons

Copy link
Contributor

Choose a reason for hiding this comment

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

To resolve zonemaster/zonemaster-engine#1055 the code must remove multiple trailing dots. Cf zonemaster/zonemaster-backend#983 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

typo: two semi-colons

fixed

@@ -601,6 +603,8 @@ sub add_fake_delegation {
exit( 1 );
}

$name =~ s/\.$// unless $name eq '.';;
Copy link
Contributor

Choose a reason for hiding this comment

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

To resolve zonemaster/zonemaster-engine#1055 the code must remove multiple trailing dots. Cf zonemaster/zonemaster-backend#983 (comment)

lib/Zonemaster/CLI.pm Show resolved Hide resolved
lib/Zonemaster/CLI.pm Show resolved Hide resolved
@hannaeko hannaeko dismissed ghost ’s stale review via c5948cc May 3, 2022 08:41
@hannaeko hannaeko force-pushed the remove-final-dot branch from c5948cc to 0eea90f Compare May 3, 2022 08:44
@hannaeko
Copy link
Member Author

hannaeko commented May 3, 2022

I took the same approach as zonemaster/zonemaster-backend#983 and return an error if consecutive dots are found in names.

lib/Zonemaster/CLI.pm Outdated Show resolved Hide resolved
ghost
ghost previously approved these changes May 3, 2022
@matsduf matsduf added this to the v2022.1 milestone May 4, 2022
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.

I suggest, in general, to handle the update of the PO files in separate PRs. New PO files can easily be generated with the tools in the share directory.

exit( 1 );
}

if ( $name =~ m/\.\./i ) {
say STDERR __x( "The name of the nameserver '{ns}' contains consecutive dots.", ns => $name );
Copy link
Contributor

Choose a reason for hiding this comment

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

In a msgid in Zonemaster-Engine, an argument for this type must be "nsname". It would be good to use the same definitions here because it makes it easier for the translator. See https://github.com/zonemaster/zonemaster-engine/blob/develop/docs/logentry_args.md

Copy link
Member Author

Choose a reason for hiding this comment

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

This have been fixed

@hannaeko hannaeko dismissed ghost ’s stale review via dfe91a2 May 5, 2022 15:41
@hannaeko hannaeko force-pushed the remove-final-dot branch from dfe91a2 to a15b6f6 Compare May 5, 2022 15:53
@hannaeko hannaeko force-pushed the remove-final-dot branch from a15b6f6 to 09cd723 Compare May 5, 2022 15:55
@hannaeko
Copy link
Member Author

hannaeko commented May 9, 2022

@matsduf please re-review

1 similar comment
@hannaeko
Copy link
Member Author

hannaeko commented May 9, 2022

@matsduf please re-review

@hannaeko hannaeko requested a review from matsduf May 11, 2022 08:30
@hannaeko hannaeko merged commit 611251e into zonemaster:develop May 12, 2022
repperille pushed a commit to repperille/zonemaster-cli that referenced this pull request May 23, 2022
Missing Spanish locale in Installation document
@matsduf matsduf added the S-ReleaseTested Status: The PR has been successfully tested in release testing label Jun 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-ReleaseTested Status: The PR has been successfully tested in release testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants