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

Handle unbracketed ipv6 addresses #538

Merged
merged 6 commits into from
Jun 22, 2024

Conversation

jeromedalbert
Copy link
Contributor

@jeromedalbert jeromedalbert commented May 19, 2024

Fix for #537.

Solution

  • Add a new IPv6HostParser. This parser has a high priority in the parser list so IPv6 addresses are not incorrectly picked up and parsed by HostWithPortParser's simple regex.
  • Because this parser has a high priority, we need its regex to be as strict and narrow as possible to prevent it from picking up and incorrectly parsing non-IPv6 addresses. That's why it uses Resolv::IPv6::Regex from the stdlib.

Notes

My previous solution tweaked the regex of the existing IPv6HostWithPortParser, but this resulted in a regression for local hostnames, so I am using Resolv::IPv6::Regex instead, which should prove more stable. I added an extra unit test to validate the local hostname scenario.

Copy link
Member

@mattbrictson mattbrictson left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Sorry for the slow response.

I think the ambiguity in the IPv6 regex means that moving it up in the parser priority will introduce problems. For example, there a regression when the hostname is "db" and a port is specified:

# Expected behavior
Host.new("db:22")
# => hostname="db", port=22

# Regression
Host.new("db:22")
# => hostname="db:22", port=nil

Is there a way to make the regex more strict to solve this? I am not very familiar with IPv6 addresses, so I'm not sure I can offer a solution, other than to stick with the status-quo [] requirement.

Maybe using Resolv::IPv6::Regex (as you originally implemented) is the way to go?

@jeromedalbert
Copy link
Contributor Author

jeromedalbert commented Jun 22, 2024

@mattbrictson Good point. Because the IPv6 parser still needs a high priority, we need a strict IPv6 regex. Resolv::IPv6::Regex seems to be the way to go, it is pretty involved in order to catch all the different IPv6 formats, so that should be plenty strict for our needs.

I have updated the PR and description accordingly, and added an extra unit test for the db:22 scenario you mentioned.

@mattbrictson mattbrictson added the ✨ Feature Adds a new feature label Jun 22, 2024
Copy link
Member

@mattbrictson mattbrictson left a comment

Choose a reason for hiding this comment

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

Looks great!

@@ -28,6 +28,12 @@ def test_host_with_port
assert_equal 'example.com', h.hostname
end

def test_custom_host_with_port
h = Host.new 'db:22'
Copy link
Member

Choose a reason for hiding this comment

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

✨ Thanks for the additional test!

@mattbrictson mattbrictson merged commit 02198fe into capistrano:master Jun 22, 2024
18 checks passed
@jeromedalbert jeromedalbert deleted the unbracketed-ipv6 branch June 22, 2024 17:46
jeromedalbert added a commit to jeromedalbert/kamal that referenced this pull request Jun 24, 2024
Set sshkit minimum version to 1.23.0, which includes an enhancement to
support unbracketed IPv6 addresses.

See capistrano/sshkit#538
jeromedalbert added a commit to jeromedalbert/kamal that referenced this pull request Jun 25, 2024
Set sshkit minimum version to 1.23.0, which includes an enhancement to
support unbracketed IPv6 addresses.

See capistrano/sshkit#538
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ Feature Adds a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants