ok2c commented on code in PR #627:
URL:
https://github.com/apache/httpcomponents-client/pull/627#discussion_r2021093570
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/ProtocolSwitchStrategy.java:
##########
@@ -45,31 +49,58 @@
@Internal
public final class ProtocolSwitchStrategy {
- enum ProtocolSwitch { FAILURE, TLS }
+ private static final ProtocolVersionParser PROTOCOL_VERSION_PARSER =
ProtocolVersionParser.INSTANCE;
public ProtocolVersion switchProtocol(final HttpMessage response) throws
ProtocolException {
final Iterator<String> it = MessageSupport.iterateTokens(response,
HttpHeaders.UPGRADE);
ProtocolVersion tlsUpgrade = null;
+
while (it.hasNext()) {
final String token = it.next();
- if (token.startsWith("TLS")) {
- // TODO: Improve handling of HTTP protocol token once
HttpVersion has a #parse method
- try {
- tlsUpgrade = token.length() == 3 ? TLS.V_1_2.getVersion()
: TLS.parse(token.replace("TLS/", "TLSv"));
- } catch (final ParseException ex) {
- throw new ProtocolException("Invalid protocol: " + token);
+ final ProtocolVersion version = parseProtocolToken(token);
+ if (version != null) {
+ if ("TLS".equalsIgnoreCase(version.getProtocol())) {
+ tlsUpgrade = version;
}
- } else if (token.equals("HTTP/1.1")) {
- // TODO: Improve handling of HTTP protocol token once
HttpVersion has a #parse method
- } else {
- throw new ProtocolException("Unsupported protocol: " + token);
}
}
- if (tlsUpgrade == null) {
- throw new ProtocolException("Invalid protocol switch response");
+
+ if (tlsUpgrade != null) {
+ return tlsUpgrade;
+ } else {
+ throw new ProtocolException("Invalid protocol switch response: no
TLS version found");
}
- return tlsUpgrade;
}
-}
+ private ProtocolVersion parseProtocolToken(final String token) throws
ProtocolException {
+ if (token == null || token.trim().isEmpty()) {
+ return null;
+ }
+
+ try {
+ if ("TLS".equalsIgnoreCase(token)) {
+ return TLS.V_1_2.getVersion();
+ }
+
+ final CharArrayBuffer buffer = new CharArrayBuffer(token.length());
+ buffer.append(token);
+ final Tokenizer.Cursor cursor = new Tokenizer.Cursor(0,
token.length());
+
+ final ProtocolVersion version =
PROTOCOL_VERSION_PARSER.parse(buffer, cursor, null);
+
+ if ("TLS".equalsIgnoreCase(version.getProtocol())) {
+ if (!(TLS.V_1_0.getVersion().lessEquals(version) &&
version.lessEquals(TLS.V_1_3.getVersion()))) {
+ throw new ProtocolException("Unsupported TLS version: " +
token);
+ }
+ return version;
+ } else if
(HttpVersion.HTTP.equalsIgnoreCase(version.getProtocol()) &&
version.equals(HttpVersion.HTTP_1_1)) {
Review Comment:
@arturobernalg Should not `version.equals(HttpVersion.HTTP_1_1)` do exactly
the same?
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/ProtocolSwitchStrategy.java:
##########
@@ -45,31 +49,58 @@
@Internal
public final class ProtocolSwitchStrategy {
- enum ProtocolSwitch { FAILURE, TLS }
+ private static final ProtocolVersionParser PROTOCOL_VERSION_PARSER =
ProtocolVersionParser.INSTANCE;
public ProtocolVersion switchProtocol(final HttpMessage response) throws
ProtocolException {
final Iterator<String> it = MessageSupport.iterateTokens(response,
HttpHeaders.UPGRADE);
ProtocolVersion tlsUpgrade = null;
+
while (it.hasNext()) {
final String token = it.next();
- if (token.startsWith("TLS")) {
- // TODO: Improve handling of HTTP protocol token once
HttpVersion has a #parse method
- try {
- tlsUpgrade = token.length() == 3 ? TLS.V_1_2.getVersion()
: TLS.parse(token.replace("TLS/", "TLSv"));
- } catch (final ParseException ex) {
- throw new ProtocolException("Invalid protocol: " + token);
+ final ProtocolVersion version = parseProtocolToken(token);
+ if (version != null) {
+ if ("TLS".equalsIgnoreCase(version.getProtocol())) {
+ tlsUpgrade = version;
}
- } else if (token.equals("HTTP/1.1")) {
- // TODO: Improve handling of HTTP protocol token once
HttpVersion has a #parse method
- } else {
- throw new ProtocolException("Unsupported protocol: " + token);
}
}
- if (tlsUpgrade == null) {
- throw new ProtocolException("Invalid protocol switch response");
+
+ if (tlsUpgrade != null) {
+ return tlsUpgrade;
+ } else {
+ throw new ProtocolException("Invalid protocol switch response: no
TLS version found");
}
- return tlsUpgrade;
}
-}
+ private ProtocolVersion parseProtocolToken(final String token) throws
ProtocolException {
+ if (token == null || token.trim().isEmpty()) {
+ return null;
+ }
+
+ try {
+ if ("TLS".equalsIgnoreCase(token)) {
+ return TLS.V_1_2.getVersion();
+ }
+
+ final CharArrayBuffer buffer = new CharArrayBuffer(token.length());
+ buffer.append(token);
+ final Tokenizer.Cursor cursor = new Tokenizer.Cursor(0,
token.length());
+
+ final ProtocolVersion version =
PROTOCOL_VERSION_PARSER.parse(buffer, cursor, null);
+
+ if ("TLS".equalsIgnoreCase(version.getProtocol())) {
Review Comment:
@arturobernalg `TLS` has its own protocol version negotiation handshake. I
would not reject an upgrade response here and let TLS do its job negotiating
the exact protocol versions and rejecting those it does not understand or
support.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@hc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@hc.apache.org
For additional commands, e-mail: dev-h...@hc.apache.org
