[HTTPRequest] Authority validation for CONNECT method fails on all requests

[GrammAcc]

Tested versions

Tested in: v4.3.dev.custom_build.e0349334e

System information

Godot v4.3.dev (e0349334e) - Arch Linux #1 SMP PREEMPT_DYNAMIC Wed, 17 Apr 2024 15:20:28 +0000 - Tty - Vulkan (Forward+) - dedicated AMD Radeon RX 580 Series (RADV POLARIS10) () - AMD Ryzen 5 2600 Six-Core Processor (12 Threads)

Issue description

While working on #90160, I noticed that I was unable to test CONNECT requests because the initial send will always result in an error with the current implementation.

The HTTPRequest node uses the same request building semantics for CONNECT as other methods with the exception of a check for the existence of the host and port in the target URL see:

https://github.com/godotengine/godot/blob/6118592c6d88350d01f74faff6fd49754f84a7d0/core/io/http_client_tcp.cpp#L131C1-L148C2

The bug is not in this check though. It's because the p_url that is passed to the HTTPClientTCP::request method from HTTPRequest::_update_connection only contains the path of the url that the client requested.

This is a result of the call to HTTPRequest::_parse_url at

godot/scene/main/http_request.cpp

Line 127 in 6118592

Error err = _parse_url(p_url);

The _parse_url method calls String::parse_url in order to populate the HTTPRequest::request_string property as the path of the url excluding the host and port. The HTTPRequest::request_string is then used as the p_url argument to HTTPClientTCP::request, which then fails validation for the CONNECT method due to not being a correctly formatted authority. See: https://www.rfc-editor.org/rfc/rfc9110#CONNECT

We need to special case the CONNECT method in the HTTPRequest::_parse_url method to set the HTTPRequest::request_string to {host}:{port}.

I made this change locally to test, and the request makes it past the url checks, but I'm getting TLS errors in curl:
curl -i -X CONNECT https://localhost:8000 gives curl: (35) OpenSSL/3.2.1: error:0A0000C6:SSL routines::packet length too long as an error.
In the GDScript test in the reproduction steps, I get the following when hitting https://localhost:8000 with CONNECT:

thirdparty/mbedtls/library/ssl_msg.c:3811: unknown record type 72
thirdparty/mbedtls/library/ssl_msg.c:4220: ssl_get_next_record() returned -29184 (-0x7200)
thirdparty/mbedtls/library/ssl_tls12_client.c:1197: mbedtls_ssl_read_record() returned -29184 (-0x7200)
ERROR: TLS handshake error: -29184
   at: _do_handshake (modules/mbedtls/stream_peer_mbedtls.cpp:89)
mbedtls error: returned -0x7200

Error: 5
Response Code: 0
Response Headers: []
Response Body: 

I'm not sure if this is due to my local server config or if there is a problem with the TLS handling in the request implementation. I only started using Caddy this week, so I'm not sure how it handles TSL certs. Also, I haven't run the above under gdb yet, so I need to investigate this further.

For plain http requests, I can run curl:
curl -i -X CONNECT http://localhost:8000 and it gives the correct 204 response that I expect from the server setup.

However, the GDScript test gives a 400 BAD REQUEST response. I don't know why. This is probably an issue with the http client implementation, and this leads me to believe that how we handle CONNECT requests is more flawed than simply using the path instead of the authority.

I need to do more debugging before I can submit a PR for this. I'll start working on it once I get #90160 PRd. :)

Steps to reproduce

Create a Caddyfile in the current directory with the following contents:

{
	http_port 8000
}

localhost:8000 {
	respond * 204
}

Run the server:

caddy start

Run a test project with the following script on the main scene:

extends Node

func _ready() -> void:
    var req = HTTPRequest.new()
    add_child(req)
    req.request_completed.connect(self._on_request_completed)
    req.request("http://localhost:8000", [], HTTPClient.METHOD_CONNECT)

func _on_request_completed(result, response_code, headers, body):
    print("Error: ", result)
    print("Response Code: ", response_code)
    print("Response Headers: ", headers)
    print("Response Body: ", body.get_string_from_utf8())

Minimal reproduction project (MRP)

N/A

[timothyqiu]

CONNECT is for talking to a proxy server, asking the host to establish a tunnel to a third party server.

It's fine for HTTPClient because making connection to host is a separate step/method. But for HTTPRequest, host is inferred from the target url, and there's no way to specify a host separately. I think maybe it should be an error using CONNECT method for request() and request_raw().

[GrammAcc]

It's fine for HTTPClient because making connection to host is a separate step/method. But for HTTPRequest, host is inferred from the target url, and there's no way to specify a host separately. I think maybe it should be an error using CONNECT method for request() and request_raw().

I was wondering about this. I've never had to implement CONNECT before, so I wasn't sure. If that's the case, that makes this easier. :)

@Faless Are you okay with changing the behavior to explicitly error on CONNECT requests through HTTPRequest? Probably with a message like "HTTPRequest only supports sending requests to origin servers. HTTPClient must be used to CONNECT to a proxy server."?

If so, I can get this PRd this week probably. Thanks!

[GrammAcc]

This was actually a very simple change, so I've opened a draft PR #91370 that adds the explicit error condition for method == CONNECT.

I marked it as a draft since this issue is marked as a discussion. It should be ready to go if the maintainers decide to go that route, or it can be easily closed or reworked if not. :)