Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement RFC-7230 section 5.3.3 #66

Merged
merged 7 commits into from
May 10, 2023
Merged

Implement RFC-7230 section 5.3.3 #66

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
56c19ad
Implement RFC-7230 section 5.3.3
brian-mcnamara May 5, 2023
b1ec83f
More validations
brian-mcnamara May 5, 2023
d7312dd
More comments
brian-mcnamara May 5, 2023
3d779a3
Simplify Request line based on review
brian-mcnamara May 8, 2023
05d1759
Update comments
brian-mcnamara May 8, 2023
0ce68f0
Remove quote from javadoc
brian-mcnamara May 9, 2023
f0a770d
Test fixes
brian-mcnamara May 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 22 additions & 9 deletions rawhttp-core/src/main/java/rawhttp/core/RequestLine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,16 +88,29 @@ private void writeTo(OutputStream outputStream, boolean newLine) throws IOExcept
outputStream.write(method.getBytes(StandardCharsets.US_ASCII));
outputStream.write(' ');

String path = uri.getRawPath();
if (path == null || path.isEmpty()) {
outputStream.write('/');
//RFC-7230 section 5.3.3
if ("CONNECT".equalsIgnoreCase(method)) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@brian-mcnamara brian-mcnamara May 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

String host = uri.getHost();
int port = uri.getPort();
assert host != null : "Host is missing from RequestLine uri";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion! I'll implement this.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@brian-mcnamara brian-mcnamara May 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😊)


outputStream.write(host.getBytes(StandardCharsets.US_ASCII));
if (port != -1) {
outputStream.write(':');
outputStream.write(Integer.toString(port).getBytes(StandardCharsets.US_ASCII));
}
} else {
outputStream.write(path.getBytes(StandardCharsets.US_ASCII));
}
String query = uri.getRawQuery();
if (query != null && !query.isEmpty()) {
outputStream.write('?');
outputStream.write(query.getBytes(StandardCharsets.US_ASCII));
String path = uri.getRawPath();
if (path == null || path.isEmpty()) {
outputStream.write('/');
} else {
outputStream.write(path.getBytes(StandardCharsets.US_ASCII));
}
String query = uri.getRawQuery();
if (query != null && !query.isEmpty()) {
outputStream.write('?');
outputStream.write(query.getBytes(StandardCharsets.US_ASCII));
}
}

outputStream.write(' ');
Expand Down
18 changes: 18 additions & 0 deletions rawhttp-core/src/test/kotlin/rawhttp/core/RequestLineTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import io.kotest.data.table
import io.kotest.matchers.shouldBe
import org.junit.jupiter.api.Test
import rawhttp.core.errors.InvalidHttpRequest
import java.net.URI

class RequestLineTest {

Expand Down Expand Up @@ -193,4 +194,21 @@ class RequestLineTest {
}
}

@Test
fun testConnectSetsAuthorityFormTarget() {
val table = table(headers("method", "uri", "version", "expected request line"),
row("CONNECT", URI("http://example.com"), HttpVersion.HTTP_1_1, "CONNECT example.com HTTP/1.1"),
row("CONNECT", URI("http://example.com:80"), HttpVersion.HTTP_1_1, "CONNECT example.com:80 HTTP/1.1"),
row("CONNECT", URI("http://user:pass@example.com:80"), HttpVersion.HTTP_1_1, "CONNECT example.com:80 HTTP/1.1"),
row("CONNECT", URI("http://example.com/somePath"), HttpVersion.HTTP_1_1, "CONNECT example.com HTTP/1.1"),
row("CONNECT", URI("http://example.com:80"), HttpVersion.HTTP_1_1, "CONNECT example.com:80 HTTP/1.1"),
row("CONNECT", URI("http://www.example.com:80"), HttpVersion.HTTP_1_1, "CONNECT www.example.com:80 HTTP/1.1"),
)
forAll(table) { method, uri, version, expectedRequest ->
RequestLine(method, uri, version).run {
toString() shouldBe expectedRequest
}
}
}

}