Repository navigation
fix: support IPv6 addresses in the local Appium service URL - #2495
RaphaelFakhri wants to merge 4 commits into
Conversation
mykola-mokhnach
left a comment
mykola-mokhnach
left a comment
There was a problem hiding this comment.
AppiumDriverLocalService.java:129:replaceHostdoes a regexreplaceFirston the whole URL string. The new IPv6 path only works because"http://[::]:4777/".replaceFirst("::", "::1")happens to hit the host.oldHostisn't quoted, so the dots in0.0.0.0are regex wildcards, andnewHostisn't passed throughMatcher.quoteReplacement. Would building the URL fromgetProtocol()/getHost()/getPort()be safer?
Strip square brackets once in withIPAddress, so --address always gets the bare address. Treat any form of the unspecified IPv6 address as a wildcard bind when pinging, and rebuild the ping URL from its parts instead of a regex replacement.
Normalize the address once where the server arguments are built, so toUrlHost no longer strips brackets that it then adds back.
| var host = url.getHost(); | ||
| static URL fixBroadcastAddresses(URL url) { | ||
| // IPv6 hosts are returned in square brackets | ||
| var host = stripBrackets(url.getHost()); |
There was a problem hiding this comment.
why is it needed to strip brackets here?
There was a problem hiding this comment.
It isn't needed. InetAddress.getByName accepts the bracketed form ([::], [::0] and [0:0:0:0:0:0:0:0] all give isAnyLocalAddress() == true), and the 0.0.0.0 comparison never has brackets. Removed in 5d86594, fixBroadcastAddresses now passes url.getHost() through as is. The existing fixBroadcastAddresses tests still pass.
| private static URL replaceHost(URL source, String oldHost, String newHost) { | ||
| return new URL(source.toString().replaceFirst(oldHost, newHost)); | ||
| private static URL replaceHost(URL source, String newHost) { | ||
| return new URL(source.getProtocol(), toUrlHost(newHost), source.getPort(), source.getFile()); |
There was a problem hiding this comment.
does URL require IPv6 wrapped into brackets?
There was a problem hiding this comment.
Not when the host is passed separately. new URL(protocol, host, port, file) adds the brackets itself when the host contains : (new URL("http", "::1", 4723, "/") gives http://[::1]:4723/, and a zone ID like fe80::1%en0 also comes out as [fe80::1%en0]). Only the string form http://%s:%d/ needed them. In 5d86594 the constructor uses new URL("http", ipAddress, nodeJSPort, "/"), so toUrlHost and URL_MASK are gone. stripBrackets is now only used in createArgs(), so I moved it into AppiumServiceBuilder as a private method. Unit tests and checkstyle pass.
Change list
AppiumDriverLocalServiceencloses an IPv6 literal (for example::or::1) in square brackets when it builds the server URL. Before this change,build()failed withMalformedURLException.BROADCAST_IP6_ADDRESS(::) is pinged on[::1]as intended.AppiumServiceBuilderTestcover::1andBROADCAST_IP6_ADDRESS.Types of changes
Details
AppiumServiceBuilderexposesBROADCAST_IP6_ADDRESS, but passing it, or any other IPv6 address, towithIPAddressbreaksbuild():The service formats the URL as
http://%s:%d/, which produceshttp://:::4723/. With brackets the URL becomeshttp://[::]:4723/. The--addressvalue sent to the server stays unbracketed, because the server expects a plain address.fixBroadcastAddressescomparedURL.getHost()with::, butgetHost()returns[::]for IPv6 hosts, so the IPv6 branch could never match. It now strips the brackets before the comparison.