GH-1271: [Flight] Reject port 0 in NettyClientBuilder - #1282
Conversation
… omitted port Tighten the port validation to 1–65535 (port 0 is reserved and invalid for client connections). Four builder-based tests were not calling withPort() at all, so the port field defaulted to 0 and slipped through unnoticed before the validation was added. Each test now passes the actual test-server port via FLIGHT_SERVER_TEST_EXTENSION.getPort(). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Converts testUnencryptedConnectionProvidingInvalidPort to a @ParameterizedTest covering 0, -1, 65536, and 65537, explicitly exercising the port-0 rejection added to NettyClientBuilder.build(). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ennuite
changed the title
|
No need from my side to add this one to release 20.0.0 It needs the enhancement label. |
This comment has been minimized.
This comment has been minimized.
| { | ||
| final int port = location.getUri().getPort(); | ||
| if (port < 0 || port > 65535) { | ||
| if (port < 1 || port > 65535) { |
There was a problem hiding this comment.
URI.getPort() returns -1 to mean "no port present in the URI", not "the user asked for port -1".
Folding it into this range check produces Invalid port -1: must be between 1 and 65535 (a number that the user never defined).
That path is reachable: a Location like grpc+tls://myhost with no port, and also ArrowFlightSqlClientHandler.getStreams(), which does withPort(endpointUri.getPort()) on an endpoint URI that may carry no port.
Since this PR is rewriting the message anyway, this seems like the moment to split
the two cases:
if (port == -1) {
throw new IllegalArgumentException(
"No port specified in location URI: " + location.getUri());
}
if (port < 1 || port > 65535) {
throw new IllegalArgumentException(
"Invalid port " + port + ": must be between 1 and 65535.");
}| if (port < 1 || port > 65535) { | ||
| throw new IllegalArgumentException( | ||
| "Invalid port " + port + ": must be between 0 and 65535."); | ||
| "Invalid port " + port + ": must be between 1 and 65535."); |
There was a problem hiding this comment.
This narrows the accepted input range of published public API: NettyClientBuilder,
and transitively FlightClient.Builder. Third-party code that today calls FlightClient.builder(allocator, someLocation).build() with a port-0 location
(a server location captured before start(), or a placeholder resolved later) gets a lazily-failing channel today and an immediate IllegalArgumentException after this.
The change itself is right, but neither NettyClientBuilder.build() nor
FlightClient.Builder.build() documents the precondition.
Could we add an @throws IllegalArgumentException javadoc stating the accepted range, so callers can
discover it without hitting it in production?
2 participants
What's changed
Port validation in
NettyClientBuilder.build()is tightened to explicitly disallow port 0, as it is not a valid destination port number for a client.Tests that made use of port 0 were updated to use a valid port under the new validation.
Are these changes tested?
Yes.
ConnectionTest.testUnencryptedConnectionProvidingInvalidPortwas expanded to cover more invalid ports, including port 0.This change was created with AI assistance (Claude Code). All lines were manually reviewed by a human. The output is not copyrightable subject matter.
Closes #1271.