This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit e5815e2312088c092bfa4a866332a650aee8f142
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Thu Feb 23 16:37:30 2023 +0000
Handle CONNECT with deliberate 501 rather than accidental 400
---
.../apache/catalina/connector/CoyoteAdapter.java | 92 ++++++++++++----------
.../catalina/connector/LocalStrings.properties | 1 +
java/org/apache/coyote/http2/Stream.java | 3 +-
.../apache/coyote/http11/TestHttp11Processor.java | 18 +++++
.../apache/coyote/http2/TestStreamProcessor.java | 23 ++++++
webapps/docs/changelog.xml | 4 +
6 files changed, 97 insertions(+), 44 deletions(-)
diff --git a/java/org/apache/catalina/connector/CoyoteAdapter.java
b/java/org/apache/catalina/connector/CoyoteAdapter.java
index 9efcce07e6..0e0526a878 100644
--- a/java/org/apache/catalina/connector/CoyoteAdapter.java
+++ b/java/org/apache/catalina/connector/CoyoteAdapter.java
@@ -635,53 +635,59 @@ public class CoyoteAdapter implements Adapter {
MessageBytes decodedURI = req.decodedURI();
- if (undecodedURI.getType() == MessageBytes.T_BYTES) {
- if (connector.getRejectSuspiciousURIs()) {
- if (checkSuspiciousURIs(undecodedURI.getByteChunk())) {
- response.sendError(400, "Invalid URI");
+ // Filter CONNECT method
+ if (req.method().equalsIgnoreCase("CONNECT")) {
+ response.sendError(HttpServletResponse.SC_NOT_IMPLEMENTED,
sm.getString("coyoteAdapter.connect"));
+ } else {
+ // No URI for CONNECT requests
+ if (undecodedURI.getType() == MessageBytes.T_BYTES) {
+ if (connector.getRejectSuspiciousURIs()) {
+ if (checkSuspiciousURIs(undecodedURI.getByteChunk())) {
+ response.sendError(400, "Invalid URI");
+ }
}
- }
- // Copy the raw URI to the decodedURI
- decodedURI.duplicate(undecodedURI);
+ // Copy the raw URI to the decodedURI
+ decodedURI.duplicate(undecodedURI);
- // Parse (and strip out) the path parameters
- parsePathParameters(req, request);
+ // Parse (and strip out) the path parameters
+ parsePathParameters(req, request);
- // URI decoding
- // %xx decoding of the URL
- try {
- req.getURLDecoder().convert(decodedURI.getByteChunk(),
connector.getEncodedSolidusHandlingInternal());
- } catch (IOException ioe) {
- response.sendError(400, "Invalid URI: " + ioe.getMessage());
- }
- // Normalization
- if (normalize(req.decodedURI(), connector.getAllowBackslash())) {
- // Character decoding
- convertURI(decodedURI, request);
- // URIEncoding values are limited to US-ASCII supersets.
- // Therefore it is not necessary to check that the URI remains
- // normalized after character decoding
+ // URI decoding
+ // %xx decoding of the URL
+ try {
+ req.getURLDecoder().convert(decodedURI.getByteChunk(),
connector.getEncodedSolidusHandlingInternal());
+ } catch (IOException ioe) {
+ response.sendError(400, "Invalid URI: " +
ioe.getMessage());
+ }
+ // Normalization
+ if (normalize(req.decodedURI(),
connector.getAllowBackslash())) {
+ // Character decoding
+ convertURI(decodedURI, request);
+ // URIEncoding values are limited to US-ASCII supersets.
+ // Therefore it is not necessary to check that the URI
remains
+ // normalized after character decoding
+ } else {
+ response.sendError(400, "Invalid URI");
+ }
} else {
- response.sendError(400, "Invalid URI");
- }
- } else {
- /* The URI is chars or String, and has been sent using an in-memory
- * protocol handler. The following assumptions are made:
- * - req.requestURI() has been set to the 'original' non-decoded,
- * non-normalized URI
- * - req.decodedURI() has been set to the decoded, normalized form
- * of req.requestURI()
- * - 'suspicious' URI filtering - if required - has already been
- * performed
- */
- decodedURI.toChars();
- // Remove all path parameters; any needed path parameter should be
set
- // using the request object rather than passing it in the URL
- CharChunk uriCC = decodedURI.getCharChunk();
- int semicolon = uriCC.indexOf(';');
- if (semicolon > 0) {
- decodedURI.setChars(uriCC.getBuffer(), uriCC.getStart(),
semicolon);
+ /* The URI is chars or String, and has been sent using an
in-memory
+ * protocol handler. The following assumptions are made:
+ * - req.requestURI() has been set to the 'original'
non-decoded,
+ * non-normalized URI
+ * - req.decodedURI() has been set to the decoded, normalized
form
+ * of req.requestURI()
+ * - 'suspicious' URI filtering - if required - has already
been
+ * performed
+ */
+ decodedURI.toChars();
+ // Remove all path parameters; any needed path parameter
should be set
+ // using the request object rather than passing it in the URL
+ CharChunk uriCC = decodedURI.getCharChunk();
+ int semicolon = uriCC.indexOf(';');
+ if (semicolon > 0) {
+ decodedURI.setChars(uriCC.getBuffer(), uriCC.getStart(),
semicolon);
+ }
}
}
@@ -837,7 +843,7 @@ public class CoyoteAdapter implements Adapter {
return false;
}
- // Filter trace method
+ // Filter TRACE method
if (!connector.getAllowTrace()
&& req.method().equalsIgnoreCase("TRACE")) {
Wrapper wrapper = request.getWrapper();
diff --git a/java/org/apache/catalina/connector/LocalStrings.properties
b/java/org/apache/catalina/connector/LocalStrings.properties
index 09a1ed58f1..4f77e0eb9b 100644
--- a/java/org/apache/catalina/connector/LocalStrings.properties
+++ b/java/org/apache/catalina/connector/LocalStrings.properties
@@ -19,6 +19,7 @@ coyoteAdapter.authenticate=Authenticated user [{0}] provided
by connector
coyoteAdapter.authorize=Authorizing user [{0}] using Tomcat''s Realm
coyoteAdapter.checkRecycled.request=Encountered a non-recycled request and
recycled it forcedly.
coyoteAdapter.checkRecycled.response=Encountered a non-recycled response and
recycled it forcedly.
+coyoteAdapter.connect=HTTP requests using the CONNECT method are not supported
coyoteAdapter.debug=The variable [{0}] has value [{1}]
coyoteAdapter.nullRequest=An asynchronous dispatch may only happen on an
existing request
diff --git a/java/org/apache/coyote/http2/Stream.java
b/java/org/apache/coyote/http2/Stream.java
index c5cc5f5f2f..dc72e1e9db 100644
--- a/java/org/apache/coyote/http2/Stream.java
+++ b/java/org/apache/coyote/http2/Stream.java
@@ -517,7 +517,8 @@ class Stream extends AbstractNonZeroStream implements
HeaderEmitter {
final boolean receivedEndOfHeaders() throws ConnectionException {
- if (coyoteRequest.method().isNull() || coyoteRequest.scheme().isNull()
|| coyoteRequest.requestURI().isNull()) {
+ if (coyoteRequest.method().isNull() || coyoteRequest.scheme().isNull()
||
+ !coyoteRequest.method().equalsIgnoreCase("CONNECT") &&
coyoteRequest.requestURI().isNull()) {
throw new
ConnectionException(sm.getString("stream.header.required", getConnectionId(),
getIdAsString()),
Http2Error.PROTOCOL_ERROR);
}
diff --git a/test/org/apache/coyote/http11/TestHttp11Processor.java
b/test/org/apache/coyote/http11/TestHttp11Processor.java
index dfda55b892..9c0c02582c 100644
--- a/test/org/apache/coyote/http11/TestHttp11Processor.java
+++ b/test/org/apache/coyote/http11/TestHttp11Processor.java
@@ -1998,6 +1998,24 @@ public class TestHttp11Processor extends TomcatBaseTest {
}
+ @Test
+ public void testConnect() throws Exception {
+ getTomcatInstanceTestWebapp(false, true);
+
+ String request =
+ "CONNECT example.local HTTP/1.1" + SimpleHttpClient.CRLF +
+ "Host: example.local" + SimpleHttpClient.CRLF +
+ SimpleHttpClient.CRLF;
+
+ Client client = new Client(getPort());
+ client.setRequest(new String[] {request});
+
+ client.connect();
+ client.processRequest();
+ Assert.assertTrue(client.isResponse501());
+ }
+
+
private static class TestPostNoReadServlet extends HttpServlet {
private static final long serialVersionUID = 1L;
diff --git a/test/org/apache/coyote/http2/TestStreamProcessor.java
b/test/org/apache/coyote/http2/TestStreamProcessor.java
index 16fa106567..78e84bb57f 100644
--- a/test/org/apache/coyote/http2/TestStreamProcessor.java
+++ b/test/org/apache/coyote/http2/TestStreamProcessor.java
@@ -575,4 +575,27 @@ public class TestStreamProcessor extends Http2TestBase {
}
}
}
+
+
+ @Test
+ public void testConnect() throws Exception {
+ http2Connect();
+
+ List<Header> headers = new ArrayList<>(4);
+ headers.add(new Header(":method", "CONNECT"));
+ headers.add(new Header(":scheme", "http"));
+ headers.add(new Header(":authority", "example.local"));
+
+ byte[] headersFrameHeader = new byte[9];
+ ByteBuffer headersPayload = ByteBuffer.allocate(128);
+
+ buildGetRequest(headersFrameHeader, headersPayload, null, headers , 3);
+
+ writeFrame(headersFrameHeader, headersPayload);
+
+ parser.readFrame();
+
+ String trace = output.getTrace();
+ Assert.assertTrue(trace, trace.contains("3-Header-[:status]-[501]"));
+ }
}
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 19034a62ba..8a09747055 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -112,6 +112,10 @@
types from mod_rewrite. Based on a pull request <pr>591</pr>
provided by Dimitrios Soumis. (markt)
</update>
+ <update>
+ Provide a more appropriate response (501 rather than 400) when
rejecting
+ an HTTP request using the CONNECT method. (markt)
+ </update>
</changelog>
</subsection>
<subsection name="Coyote">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
