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.

Sign up for GitHub

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

Jump to bottom

Fix handling of consecutive and final dots #983

Merged
merged 4 commits into from
May 5, 2022
Merged

Fix handling of consecutive and final dots #983

merged 4 commits into from
May 5, 2022

Conversation

hannaeko
Copy link
Member

@hannaeko hannaeko commented Apr 28, 2022 •
edited
Loading

Purpose

  • NS with final dot should not make the test crash
  • Having a final dot should not result in a different hash
  • Domain with consecutive dots should not be accepted as valid

Context

zonemaster/zonemaster-engine#1055
#854

Changes

  • Remove final dot from domain if not root
  • Remove final dot from NS
  • Return an error if a domain contains consecutive dots

How to test this PR

  • ./script/zmb start_domain_test --domain zonemaster.net --nameserver ns2.nic.fr.:192.93.0.4 should not crash
  • ./script/zmb start_domain_test --domain zonemaster.net. --nameserver ns2.nic.fr:192.93.0.4 should return the same hash id
  • ./script/zmb start_domain_test --domain zonemaster..net. --nameserver ns2.nic.fr...:192.93.0.4 should return an validation error

@hannaeko hannaeko added this to the v2022.1 milestone Apr 28, 2022
@hannaeko hannaeko requested review from mattias-p, matsduf and a user April 28, 2022 08:46
ghost
ghost previously approved these changes Apr 28, 2022
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

lgtm

@ghost
Copy link

ghost commented Apr 29, 2022

Addresses #854 as well

matsduf
matsduf reviewed Apr 29, 2022
View reviewed changes
lib/Zonemaster/Backend/DB.pm Show resolved Hide resolved
lib/Zonemaster/Backend/DB.pm Outdated
@@ -557,6 +558,7 @@ sub _project_params {
delete $$nameserver{ip};
}
$$nameserver{ns} = lc $$nameserver{ns};
$$nameserver{ns} =~ s/\.$//;
Copy link
Contributor

Choose a reason for hiding this comment

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

$$nameserver{ns} =~ s/\.+$//;

Copy link
Member Author

Choose a reason for hiding this comment

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

I disagree for the same reasons.

Copy link
Contributor

Choose a reason for hiding this comment

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

See above.

@hannaeko hannaeko linked an issue May 2, 2022 that may be closed by this pull request
NS with final dot makes the test die #854
Closed
@hannaeko hannaeko dismissed ghost ’s stale review via 81e87cc May 2, 2022 08:46
ghost
ghost previously approved these changes May 2, 2022
View reviewed changes
@matsduf matsduf mentioned this pull request May 2, 2022
Merged
matsduf
matsduf requested changes May 2, 2022
View reviewed changes
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.

Make this PR resolve zonemaster/zonemaster-engine#1055 even if it is a temporary fix.

@hannaeko hannaeko dismissed ghost ’s stale review via 225e1c1 May 2, 2022 12:30
@hannaeko hannaeko force-pushed the remove-final-dot branch from d78d257 to 61b7511 Compare May 2, 2022 12:41
@hannaeko hannaeko changed the title remove final dot from domains before saving the params Fix handling or consecutive and final dots May 2, 2022
@hannaeko hannaeko changed the title Fix handling or consecutive and final dots Fix handling of consecutive and final dots May 2, 2022
@hannaeko
Copy link
Member Author

hannaeko commented May 2, 2022

I rebased on top of develop and added the check for consecutive dots in check_domain

@hannaeko hannaeko force-pushed the remove-final-dot branch from 61b7511 to 87dd920 Compare May 2, 2022 12:50
matsduf
matsduf reviewed May 2, 2022
View reviewed changes
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.

It looks fine, but the MO files should not be included.

@hannaeko hannaeko force-pushed the remove-final-dot branch from 87dd920 to 630b014 Compare May 2, 2022 12:56
@hannaeko
Copy link
Member Author

hannaeko commented May 2, 2022

It looks fine, but the MO files should not be included.

Oops, those should not have been included. Fixed.

matsduf
matsduf approved these changes May 2, 2022
View reviewed changes
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.

This is fine. I approve.

@hannaeko hannaeko merged commit 9000f9f into zonemaster:develop May 5, 2022
@ghost ghost mentioned this pull request May 12, 2022
Merged
@matsduf matsduf mentioned this pull request May 18, 2022
Closed
@tgreenx tgreenx added the S-ReleaseTested Status: The PR has been successfully tested in release testing label May 31, 2022
@ghost ghost mentioned this pull request Jun 3, 2022
Merged
@ghost ghost mentioned this pull request Jul 26, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matsduf matsduf matsduf approved these changes

@mattias-p mattias-p Awaiting requested review from mattias-p

Assignees
No one assigned
Labels
S-ReleaseTested Status: The PR has been successfully tested in release testing
Projects
None yet
Milestone
v2022.1
Development

Successfully merging this pull request may close these issues.

NS with final dot makes the test die
3 participants
@hannaeko @matsduf @tgreenx