Better error message for HostAndPort containing a protocol #983
Conversation
Generate changelog in
|
| // Additional validation for common errors. An alternative to this diff between conjure HostAndPort and | ||
| // Guava HostAndPort would be to use Guava's HostSpecifier.from() here instead. But that would require | ||
| // either re-adding a Guava dependency (removed earlier in this repo's history), or copying that class | ||
| // and its dependencies (InetAddresses, InternetDomainName, Ascii.toLowerCase, Splitter, Joiner, and more) | ||
| // into this repo like we did with HostAndPort. | ||
| Preconditions.checkArgument( | ||
| !host.contains("://"), | ||
| "hostPortString must not contain a protocol prefix like http:// or https://", | ||
| UnsafeArg.of("hostPortString", hostPortString)); |
There was a problem hiding this comment.
- Should this implement RFC 1123 hostname/IPv4/IPv6 validations for
host? - Should we allow creation of
HostAndPortfrom a valid RFC 2396 URI viajava.net.URI#create(String)with something like the following (though I hate the exception as control flow)? - Should the proxy configuration deserialization & checks actually handle this more gracefully?
@Nullable
private static HostAndPort tryParseUri(String hostPortString) {
try {
URI uri = URI.create(hostPortString);
return new HostAndPort(uri.getHost(), uri.getPort());
} catch (RuntimeException e) {
return null;
}
}
There was a problem hiding this comment.
- I think full standards-compliant host validation like RFC 1123 would be idea. This split host+port pair ends up being sent into
InetSocketAddress.createUnresolvedat https://github.com/palantir/conjure-java-runtime/blob/33525f67c7acc31106ef4311f2fd2f9ef3ac92ae/client-config/src/main/java/com/palantir/conjure/java/client/config/ClientConfigurations.java#L173
So we would preferably match whatever validation that the JDK does when opening the socket.
-
that might be nice. So instead of providing a better exception message for
hostAndPort: http://example.com:8080, accept it as equivalent toexample.com:8080 -
By handling deser and checks more gracefully, do you mean deserialize successfully but into an error state, rather than failing to deserialize?
There was a problem hiding this comment.
Making the deserialization more lenient to permit host-and-port: https://your.proxy.host.example.com:1234 seems reasonable to me, but I'm not super close to all the consumers here and would defer to @carterkozak. I think we would still want to check and throw if one specified a URI scheme not in {http, https, mesh, socks} or if non-scheme/host/port elements of URI are specified (e.g. path, authority, etc.).
For 3, I was just referring to the serDe() test above and adding similar test cases that reads raw json (or yaml) with various valid & invalid host-and-port: URIs
| @Test | ||
| void fromString_onHostAndPortWithHttpProtocolPrefix_doesNotThrow() { | ||
| assertThatLoggableExceptionThrownBy(() -> HostAndPort.fromString("http://example.com:8080")) | ||
| .hasMessageContaining("hostPortString must not contain a protocol prefix like http:// or https://") | ||
| .containsArgs(UnsafeArg.of("hostPortString", "http://example.com:8080")); | ||
| } | ||
|
|
||
| @Test | ||
| void fromString_onHostAndPortWithHttpsProtocolPrefix_doesNotThrow() { | ||
| assertThatLoggableExceptionThrownBy(() -> HostAndPort.fromString("https://example.com:8080")) | ||
| .hasMessageContaining("hostPortString must not contain a protocol prefix like http:// or https://") | ||
| .containsArgs(UnsafeArg.of("hostPortString", "https://example.com:8080")); | ||
| } | ||
|
|
||
| @Test | ||
| void fromString_onHostAndPortWithAbcProtocolPrefix_doesNotThrow() { | ||
| assertThatLoggableExceptionThrownBy(() -> HostAndPort.fromString("abc://example.com:8080")) | ||
| .hasMessageContaining("hostPortString must not contain a protocol prefix like http:// or https://") | ||
| .containsArgs(UnsafeArg.of("hostPortString", "abc://example.com:8080")); | ||
| } |
There was a problem hiding this comment.
nit: the test names *_doesNotThrow() don't seem to match expectation that this actually throws
| public void incorrectlyFormattedHostAndPort() { | ||
| assertThatLoggableExceptionThrownBy(() -> ProxyConfiguration.builder() | ||
| .hostAndPort("http://squid:3128") | ||
| .type(ProxyConfiguration.Type.HTTP) | ||
| .build()) | ||
| .hasMessageContaining("hostPortString must not contain a protocol prefix like http:// or https://") | ||
| .containsArgs(UnsafeArg.of("hostPortString", "http://squid:3128")); |
There was a problem hiding this comment.
should we add similar tests for these to serDe?
Before this PR
There was an internal report (
ptcom/public-docs/pull/1359) of a long debugging session that root caused to incorrectly specifyinghostAndPort: https://proxy-host.com:3128instead ofhostAndPort: proxy-host.com:3128in this configuration.Before this PR, that would be easy to miss in the error message:
After this PR
==COMMIT_MSG==
Better error message for HostAndPort containing a protocol
==COMMIT_MSG==
With this PR, the error message would instead be:
To see the behavior change more clearly, look at just the second commit's diff (the first commit adds tests establishing current behavior).
Possible downsides?
This PR performs more validation in the HostAndPort class, so any existing instances of values containing
://would now start failing to deserialize. The only usage of HostAndPort in this repo is in ProxyConfiguration which already fails during deserialization:conjure-java-runtime-api/service-config/src/main/java/com/palantir/conjure/java/api/config/service/ProxyConfiguration.java
Lines 95 to 97 in acf99a6
import com.palantir.conjure.java.api.config.service.HostAndPortI see only one repo doing this.