Implement RFC-7230 section 5.3.3 #66

brian-mcnamara · 2023-05-05T14:55:47Z

https://datatracker.ietf.org/doc/html/rfc7230#section-5.3.3

brian-mcnamara · 2023-05-05T14:57:19Z

rawhttp-core/src/main/java/rawhttp/core/RequestLine.java

+        if ("CONNECT".equalsIgnoreCase(method)) {
+            String host = uri.getHost();
+            int port = uri.getPort();
+            assert host != null : "Host is missing from RequestLine uri";


I noticed this library does not use asserts or throws, so curious to hear what the suggestion is here. https://datatracker.ietf.org/doc/html/rfc7230#section-5.3.3 seems to require host to be provided in a CONNECT case

host can be null here. The Host header may be used for defining that.

Oh, you're about this:

A client sending a CONNECT request MUST send the authority form of request-target ([Section 5.3 of [RFC7230]](https://datatracker.ietf.org/doc/html/rfc7230#section-5.3))

In this case, this shouldn't be an assert, it should be straight out an Exception, because the caller should be notified of the problem.
But because I want RawHTTP to be usable for testing "bad" requests, I normally make this kind of thing optional, so you would need to add an option to the RawHttpOptions to allow for requests missing the host.

Thanks for the suggestion! I'll implement this.

Looks like the server side also has some "obligations" to make CONNECT work.
There's also some other small requirements on the client, for example:

A server MUST NOT send any Transfer-Encoding or Content-Length header fields in a 2xx (Successful) response to CONNECT. A client MUST ignore any Content-Length or Transfer-Encoding header fields received in a successful response to CONNECT. A payload within a CONNECT request message has no defined semantics; sending a payload body on a CONNECT request might cause some existing implementations to reject the request.

To support CONNECT correctly, we would need to do this as well :) . But I can finish the job, just let me know if you can help and how you're using this, so I can understand the use cases.

I can take a look. I'm implementing a tunnel solution in java which connects to pomerium. As part of their tcp tunnel implementation, I create a socket and send a CONNECT request to which it replies with a response followed by keeping the socket open and switches over to proxying the streams. So I'm using this library to construct the initial message and parse the response.

Went through and added move validations and some changes to requestHasBody (responseHasBody is already handling connect). Let me know if there were other areas of the code you can think of that may need this (new to the project 😊)

rawhttp-core/src/main/java/rawhttp/core/RawHttp.java

rawhttp-core/src/main/java/rawhttp/core/RequestLine.java

renatoathaydes · 2023-05-07T08:21:54Z

rawhttp-core/src/main/java/rawhttp/core/RequestLine.java

-        if (path == null || path.isEmpty()) {
-            outputStream.write('/');
+        //RFC-7230 section 5.3.3
+        if ("CONNECT".equalsIgnoreCase(method)) {


I think the easier way to do this logic would be to already check the options here, like this:

if (!options.allowIllegalConnectAuthority() && "CONNECT.equalsIgnoreCase(method)) ...

There's no need to enter this branch in any other case. Also notice the code below is checking the wrong option.

Jeez, good catch, starts with allowIllegal and they all looked the same 🙈

Ok so the thinking is if allowIllegalConnectAuthority is set, the logic will fall back into the original implementation.

renatoathaydes · 2023-05-09T15:15:53Z

I'll merge pending the build results.
Is it correct to assume CONNECT is fully supported by the server now, given you have it working with a real implementation?

brian-mcnamara · 2023-05-09T15:28:07Z

Looking at the failure.

I have only tested HttpResponse via RawHttp().parseResponse(inputStream) which works. I have not tried using the TcpRawHttpServer

renatoathaydes · 2023-05-09T16:14:32Z

Errors are on the Javadocs:

/home/runner/work/rawhttp/rawhttp/rawhttp-core/src/main/java/rawhttp/core/RawHttp.java:229: error: reference not found
     * {@link RawHttp#requestHasBody(RawHttpHeaders)} or {@link RawHttp#responseHasBody(StatusLine, RequestLine)}.
              ^
/home/runner/work/rawhttp/rawhttp/rawhttp-core/src/main/java/rawhttp/core/RawHttp.java:279: warning: no @param for requestLine
    public static boolean requestHasBody(RawHttpHeaders headers, RequestLine requestLine) {
                          ^
/home/runner/work/rawhttp/rawhttp/rawhttp-core/src/main/java/rawhttp/core/RawHttpHeaders.java:47: warning: no comment
    public static final RawHttpHeaders CONTENT_LENGTH_ZERO = newBuilderSkippingValidation()
                                       ^
/home/runner/work/rawhttp/rawhttp/rawhttp-core/src/main/java/rawhttp/core/RawHttpHeaders.java:315: warning: no comment
    public static RawHttpHeaders empty() {
                                 ^
/home/runner/work/rawhttp/rawhttp/rawhttp-core/src/main/java/rawhttp/core/RawHttpOptions.java:317: error: unknown tag: quote
         * <quote>
           ^
/home/runner/work/rawhttp/rawhttp/rawhttp-core/src/main/java/rawhttp/core/RawHttpOptions.java:320: error: unknown tag: quote
         * </quote>
           ^

renatoathaydes · 2023-05-09T16:17:13Z

rawhttp-core/src/main/java/rawhttp/core/RawHttp.java

@@ -276,7 +276,7 @@ public FramedBody getFramedBody(StartLine startLine, RawHttpHeaders headers) {
     * @param headers HTTP request's headers
     * @return true if the headers indicate the request should have a body, false otherwise
     */
-    public static boolean requestHasBody(RawHttpHeaders headers) {
+    public static boolean requestHasBody(RawHttpHeaders headers, RequestLine requestLine) {


This is a breaking change. We need an overload without the RequestLine so old code continues working.

You can add @Deprecated to the method without RequestLine so we can remove it in the future.

renatoathaydes · 2023-05-09T16:18:19Z

rawhttp-core/src/main/java/rawhttp/core/RawHttpOptions.java

+         * Allows bypassing requirements for CONNECT requests which require host and port to be sent
+         * <p>
+         * RFC-7230 section 5.3.3 defines CONNECT requests must:
+         * <quote>


Javadoc does not support the full HTML spec.
I think you can do {@code } here.

Or use <pre>, that should work.

When you're done, run gradle javadoc from the root folder. If that passes, everything should be fine.
Run gradle check to run all tests. Also should be fine as I ran locally and it seems fine.

Implement RFC-7230 section 5.3.3

56c19ad

brian-mcnamara commented May 5, 2023

View reviewed changes

brian-mcnamara marked this pull request as draft May 5, 2023 16:27

brian-mcnamara added 2 commits May 5, 2023 12:47

More validations

b1ec83f

More comments

d7312dd

renatoathaydes reviewed May 7, 2023

View reviewed changes

rawhttp-core/src/main/java/rawhttp/core/RawHttp.java Outdated Show resolved Hide resolved

renatoathaydes reviewed May 7, 2023

View reviewed changes

rawhttp-core/src/main/java/rawhttp/core/RequestLine.java Outdated Show resolved Hide resolved

renatoathaydes reviewed May 7, 2023

View reviewed changes

brian-mcnamara added 2 commits May 8, 2023 16:52

Simplify Request line based on review

3d779a3

Update comments

05d1759

brian-mcnamara marked this pull request as ready for review May 8, 2023 23:58

renatoathaydes approved these changes May 9, 2023

View reviewed changes

renatoathaydes reviewed May 9, 2023

View reviewed changes

brian-mcnamara added 2 commits May 9, 2023 09:41

Remove quote from javadoc

0ce68f0

Test fixes

f0a770d

renatoathaydes merged commit ff052e4 into renatoathaydes:master May 10, 2023
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement RFC-7230 section 5.3.3 #66

Implement RFC-7230 section 5.3.3 #66

brian-mcnamara commented May 5, 2023 •

edited

brian-mcnamara May 5, 2023

renatoathaydes May 5, 2023

renatoathaydes May 5, 2023

brian-mcnamara May 5, 2023

renatoathaydes May 5, 2023

brian-mcnamara May 5, 2023

brian-mcnamara May 5, 2023 •

edited

renatoathaydes May 7, 2023

brian-mcnamara May 8, 2023 •

edited

renatoathaydes commented May 9, 2023

brian-mcnamara commented May 9, 2023

renatoathaydes commented May 9, 2023

renatoathaydes May 9, 2023

renatoathaydes May 9, 2023

renatoathaydes May 9, 2023

renatoathaydes May 9, 2023

renatoathaydes May 9, 2023

Implement RFC-7230 section 5.3.3 #66

Implement RFC-7230 section 5.3.3 #66

Conversation

brian-mcnamara commented May 5, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brian-mcnamara May 5, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brian-mcnamara May 8, 2023 • edited

Choose a reason for hiding this comment

renatoathaydes commented May 9, 2023

brian-mcnamara commented May 9, 2023

renatoathaydes commented May 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brian-mcnamara commented May 5, 2023 •

edited

brian-mcnamara May 5, 2023 •

edited

brian-mcnamara May 8, 2023 •

edited