-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
src: fix IsIPAddress for IPv6 #53400
src: fix IsIPAddress for IPv6 #53400
Conversation
Fix the bug when copying IPv6 host to a variable to remove brackets. Fixes: nodejs#47427
@@ -192,7 +192,7 @@ static bool IsIPAddress(const std::string& host) { | |||
// Parse the IPv6 address to ensure it is syntactically valid. | |||
char ipv6_str[INET6_ADDRSTRLEN]; | |||
std::copy(host.begin() + 1, host.end() - 1, ipv6_str); | |||
ipv6_str[host.length()] = '\0'; | |||
ipv6_str[host.length() - 2] = '\0'; |
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.
A better approach is to use Ada parser and validate if it is IPv6
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 IsIPAddress
function is invoked by the IsAllowedHost
function, which accepts a host
parameter. Typically, the host
represents an IP address that may include a port number. For instance, in the test case found here, the host
is solely an IP address.
Upon reviewing the Ada parser, it appears to be designed primarily for parsing URLs. So, I can't use it here like this: ada::parse(host)
.
Is there a potential method to adapt the Ada parser for use with IP addresses?
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.
Since this is a bug fix, keeping the patch minimal is a good idea. Refactoring to use Ada or some entirely different validation instead does not need to happen in this PR.
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.
Good catch.
Landed in 73fa9ab |
Fix the bug when copying IPv6 host to a variable to remove brackets. Fixes: nodejs#47427 PR-URL: nodejs#53400 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Richard Lau <rlau@redhat.com>
Fix the bug when copying IPv6 host to a variable to remove brackets. Fixes: nodejs#47427 PR-URL: nodejs#53400 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Richard Lau <rlau@redhat.com>
I've investigated and found a fix to the related issue as follows:
host
keeps the host address which is[::]
in our case. Then, it is copied to a char array (ipv6_str
) to remove brackets. However, when adding a null-terminator, the index is written wrong. The length should have been decreased by 2 to add a null terminator.Fixes: #47427