Merge pull request #56771 from mhilbrunner/unacceptable

Verify custom HTTP headers, fix off by one error

core/io/http_client.cpp

@@ -96,6 +96,17 @@ String HTTPClient::query_string_from_dict(const Dictionary &p_dict) {
 	return query.substr(1);
 }
 
+Error HTTPClient::verify_headers(const Vector<String> &p_headers) {
+	for (int i = 0; i < p_headers.size(); i++) {
+		String sanitized = p_headers[i].strip_edges();
+		ERR_FAIL_COND_V_MSG(sanitized.is_empty(), ERR_INVALID_PARAMETER, "Invalid HTTP header at index " + itos(i) + ": empty.");
+		ERR_FAIL_COND_V_MSG(sanitized.find(":") < 1, ERR_INVALID_PARAMETER,
+				"Invalid HTTP header at index " + itos(i) + ": String must contain header-value pair, delimited by ':', but was: " + p_headers[i]);
+	}
+
+	return OK;
+}
+
 Dictionary HTTPClient::_get_response_headers_as_dictionary() {
 	List<String> rh;
 	get_response_headers(&rh);

core/io/http_client.h

@@ -165,6 +165,7 @@ public:
 	static HTTPClient *create();
 
 	String query_string_from_dict(const Dictionary &p_dict);
+	Error verify_headers(const Vector<String> &p_headers);
 
 	virtual Error request(Method p_method, const String &p_url, const Vector<String> &p_headers, const uint8_t *p_body, int p_body_size) = 0;
 	virtual Error connect_to_host(const String &p_host, int p_port = -1, bool p_ssl = false, bool p_verify_host = true) = 0;

core/io/http_client_tcp.cpp

@@ -71,7 +71,7 @@ Error HTTPClientTCP::connect_to_host(const String &p_host, int p_port, bool p_ss
 	connection = tcp_connection;
 
 	if (ssl && https_proxy_port != -1) {
-		proxy_client.instantiate(); // Needs proxy negotiation
+		proxy_client.instantiate(); // Needs proxy negotiation.
 		server_host = https_proxy_host;
 		server_port = https_proxy_port;
 	} else if (!ssl && http_proxy_port != -1) {
@@ -83,7 +83,7 @@ Error HTTPClientTCP::connect_to_host(const String &p_host, int p_port, bool p_ss
 	}
 
 	if (server_host.is_valid_ip_address()) {
-		// Host contains valid IP
+		// Host contains valid IP.
 		Error err = tcp_connection->connect_to_host(IPAddress(server_host), server_port);
 		if (err) {
 			status = STATUS_CANT_CONNECT;
@@ -92,7 +92,7 @@ Error HTTPClientTCP::connect_to_host(const String &p_host, int p_port, bool p_ss
 
 		status = STATUS_CONNECTING;
 	} else {
-		// Host contains hostname and needs to be resolved to IP
+		// Host contains hostname and needs to be resolved to IP.
 		resolving = IP::get_singleton()->resolve_hostname_queue_item(server_host);
 		status = STATUS_RESOLVING;
 	}
@@ -124,7 +124,7 @@ Ref<StreamPeer> HTTPClientTCP::get_connection() const {
 static bool _check_request_url(HTTPClientTCP::Method p_method, const String &p_url) {
 	switch (p_method) {
 		case HTTPClientTCP::METHOD_CONNECT: {
-			// Authority in host:port format, as in RFC7231
+			// Authority in host:port format, as in RFC7231.
 			int pos = p_url.find_char(':');
 			return 0 < pos && pos < p_url.length() - 1;
 		}
@@ -135,7 +135,7 @@ static bool _check_request_url(HTTPClientTCP::Method p_method, const String &p_u
 			[[fallthrough]];
 		}
 		default:
-			// Absolute path or absolute URL
+			// Absolute path or absolute URL.
 			return p_url.begins_with("/") || p_url.begins_with("http://") || p_url.begins_with("https://");
 	}
 }
@@ -146,6 +146,11 @@ Error HTTPClientTCP::request(Method p_method, const String &p_url, const Vector<
 	ERR_FAIL_COND_V(status != STATUS_CONNECTED, ERR_INVALID_PARAMETER);
 	ERR_FAIL_COND_V(connection.is_null(), ERR_INVALID_DATA);
 
+	Error err = verify_headers(p_headers);
+	if (err) {
+		return err;
+	}
+
 	String uri = p_url;
 	if (!ssl && http_proxy_port != -1) {
 		uri = vformat("http://%s:%d%s", conn_host, conn_port, p_url);
@@ -173,7 +178,7 @@ Error HTTPClientTCP::request(Method p_method, const String &p_url, const Vector<
 	}
 	if (add_host) {
 		if ((ssl && conn_port == PORT_HTTPS) || (!ssl && conn_port == PORT_HTTP)) {
-			// Don't append the standard ports
+			// Don't append the standard ports.
 			request += "Host: " + conn_host + "\r\n";
 		} else {
 			request += "Host: " + conn_host + ":" + itos(conn_port) + "\r\n";
@@ -199,8 +204,7 @@ Error HTTPClientTCP::request(Method p_method, const String &p_url, const Vector<
 		memcpy(data.ptrw() + cs.length(), p_body, p_body_size);
 	}
 
-	// TODO Implement non-blocking requests.
-	Error err = connection->put_data(data.ptr(), data.size());
+	err = connection->put_data(data.ptr(), data.size());
 
 	if (err) {
 		close();
@@ -274,7 +278,7 @@ Error HTTPClientTCP::poll() {
 			IP::ResolverStatus rstatus = IP::get_singleton()->get_resolve_item_status(resolving);
 			switch (rstatus) {
 				case IP::RESOLVER_STATUS_WAITING:
-					return OK; // Still resolving
+					return OK; // Still resolving.
 
 				case IP::RESOLVER_STATUS_DONE: {
 					ip_candidates = IP::get_singleton()->get_resolve_item_addresses(resolving);
@@ -356,7 +360,7 @@ Error HTTPClientTCP::poll() {
 					} else if (ssl) {
 						Ref<StreamPeerSSL> ssl;
 						if (!handshaking) {
-							// Connect the StreamPeerSSL and start handshaking
+							// Connect the StreamPeerSSL and start handshaking.
 							ssl = Ref<StreamPeerSSL>(StreamPeerSSL::create());
 							ssl->set_blocking_handshake_enabled(false);
 							Error err = ssl->connect_to_stream(tcp_connection, ssl_verify_host, conn_host);
@@ -368,7 +372,7 @@ Error HTTPClientTCP::poll() {
 							connection = ssl;
 							handshaking = true;
 						} else {
-							// We are already handshaking, which means we can use your already active SSL connection
+							// We are already handshaking, which means we can use your already active SSL connection.
 							ssl = static_cast<Ref<StreamPeerSSL>>(connection);
 							if (ssl.is_null()) {
 								close();
@@ -376,22 +380,22 @@ Error HTTPClientTCP::poll() {
 								return ERR_CANT_CONNECT;
 							}
 
-							ssl->poll(); // Try to finish the handshake
+							ssl->poll(); // Try to finish the handshake.
 						}
 
 						if (ssl->get_status() == StreamPeerSSL::STATUS_CONNECTED) {
-							// Handshake has been successful
+							// Handshake has been successful.
 							handshaking = false;
 							ip_candidates.clear();
 							status = STATUS_CONNECTED;
 							return OK;
 						} else if (ssl->get_status() != StreamPeerSSL::STATUS_HANDSHAKING) {
-							// Handshake has failed
+							// Handshake has failed.
 							close();
 							status = STATUS_SSL_HANDSHAKE_ERROR;
 							return ERR_CANT_CONNECT;
 						}
-						// ... we will need to poll more for handshake to finish
+						// ... we will need to poll more for handshake to finish.
 					} else {
 						ip_candidates.clear();
 						status = STATUS_CONNECTED;
@@ -416,7 +420,7 @@ Error HTTPClientTCP::poll() {
 		} break;
 		case STATUS_BODY:
 		case STATUS_CONNECTED: {
-			// Check if we are still connected
+			// Check if we are still connected.
 			if (ssl) {
 				Ref<StreamPeerSSL> tmp = connection;
 				tmp->poll();
@@ -428,7 +432,7 @@ Error HTTPClientTCP::poll() {
 				status = STATUS_CONNECTION_ERROR;
 				return ERR_CONNECTION_ERROR;
 			}
-			// Connection established, requests can now be made
+			// Connection established, requests can now be made.
 			return OK;
 		} break;
 		case STATUS_REQUESTING: {
@@ -547,7 +551,7 @@ PackedByteArray HTTPClientTCP::read_response_body_chunk() {
 	if (chunked) {
 		while (true) {
 			if (chunk_trailer_part) {
-				// We need to consume the trailer part too or keep-alive will break
+				// We need to consume the trailer part too or keep-alive will break.
 				uint8_t b;
 				int rec = 0;
 				err = _get_http_data(&b, 1, rec);
@@ -560,18 +564,18 @@ PackedByteArray HTTPClientTCP::read_response_body_chunk() {
 				int cs = chunk.size();
 				if ((cs >= 2 && chunk[cs - 2] == '\r' && chunk[cs - 1] == '\n')) {
 					if (cs == 2) {
-						// Finally over
+						// Finally over.
 						chunk_trailer_part = false;
 						status = STATUS_CONNECTED;
 						chunk.clear();
 						break;
 					} else {
-						// We do not process nor return the trailer data
+						// We do not process nor return the trailer data.
 						chunk.clear();
 					}
 				}
 			} else if (chunk_left == 0) {
-				// Reading length
+				// Reading length.
 				uint8_t b;
 				int rec = 0;
 				err = _get_http_data(&b, 1, rec);
@@ -658,7 +662,7 @@ PackedByteArray HTTPClientTCP::read_response_body_chunk() {
 				uint8_t *w = ret.ptrw();
 				err = _get_http_data(w + _offset, to_read, rec);
 			}
-			if (rec <= 0) { // Ended up reading less
+			if (rec <= 0) { // Ended up reading less.
 				ret.resize(_offset);
 				break;
 			} else {
@@ -679,7 +683,7 @@ PackedByteArray HTTPClientTCP::read_response_body_chunk() {
 		close();
 
 		if (err == ERR_FILE_EOF) {
-			status = STATUS_DISCONNECTED; // Server disconnected
+			status = STATUS_DISCONNECTED; // Server disconnected.
 		} else {
 			status = STATUS_CONNECTION_ERROR;
 		}

core/io/http_client_tcp.h

@@ -38,13 +38,13 @@ private:
 	Status status = STATUS_DISCONNECTED;
 	IP::ResolverID resolving = IP::RESOLVER_INVALID_ID;
 	Array ip_candidates;
-	int conn_port = -1; // Server to make requests to
+	int conn_port = -1; // Server to make requests to.
 	String conn_host;
-	int server_port = -1; // Server to connect to (might be a proxy server)
+	int server_port = -1; // Server to connect to (might be a proxy server).
 	String server_host;
-	int http_proxy_port = -1; // Proxy server for http requests
+	int http_proxy_port = -1; // Proxy server for http requests.
 	String http_proxy_host;
-	int https_proxy_port = -1; // Proxy server for https requests
+	int https_proxy_port = -1; // Proxy server for https requests.
 	String https_proxy_host;
 	bool ssl = false;
 	bool ssl_verify_host = false;
@@ -64,7 +64,7 @@ private:
 
 	Ref<StreamPeerTCP> tcp_connection;
 	Ref<StreamPeer> connection;
-	Ref<HTTPClientTCP> proxy_client; // Negotiate with proxy server
+	Ref<HTTPClientTCP> proxy_client; // Negotiate with proxy server.
 
 	int response_num = 0;
 	Vector<String> response_headers;

platform/javascript/http_client_javascript.cpp

@@ -87,6 +87,11 @@ Error HTTPClientJavaScript::request(Method p_method, const String &p_url, const
 	ERR_FAIL_COND_V(port < 0, ERR_UNCONFIGURED);
 	ERR_FAIL_COND_V(!p_url.begins_with("/"), ERR_INVALID_PARAMETER);
 
+	Error err = verify_headers(p_headers);
+	if (err) {
+		return err;
+	}
+
 	String url = (use_tls ? "https://" : "http://") + host + ":" + itos(port) + p_url;
 	Vector<CharString> keeper;
 	Vector<const char *> c_strings;

scene/main/http_request.cpp

@@ -86,9 +86,9 @@ String HTTPRequest::get_header_value(const PackedStringArray &p_headers, const S
 
 	String lowwer_case_header_name = p_header_name.to_lower();
 	for (int i = 0; i < p_headers.size(); i++) {
-		if (p_headers[i].find(":", 0) >= 0) {
+		if (p_headers[i].find(":") > 0) {
 			Vector<String> parts = p_headers[i].split(":", false, 1);
-			if (parts[0].strip_edges().to_lower() == lowwer_case_header_name) {
+			if (parts.size() > 1 && parts[0].strip_edges().to_lower() == lowwer_case_header_name) {
 				value = parts[1].strip_edges();
 				break;
 			}
@@ -99,7 +99,7 @@ String HTTPRequest::get_header_value(const PackedStringArray &p_headers, const S
 }
 
 Error HTTPRequest::request(const String &p_url, const Vector<String> &p_custom_headers, bool p_ssl_validate_domain, HTTPClient::Method p_method, const String &p_request_data) {
-	// Copy the string into a raw buffer
+	// Copy the string into a raw buffer.
 	Vector<uint8_t> raw_data;
 
 	CharString charstr = p_request_data.utf8();
@@ -134,7 +134,7 @@ Error HTTPRequest::request_raw(const String &p_url, const Vector<String> &p_cust
 	headers = p_custom_headers;
 
 	if (accept_gzip) {
-		// If the user has specified a different Accept-Encoding, don't overwrite it
+		// If the user has specified an Accept-Encoding header, don't overwrite it.
 		if (!has_header(headers, "Accept-Encoding")) {
 			headers.push_back("Accept-Encoding: gzip, deflate");
 		}
@@ -227,7 +227,7 @@ bool HTTPRequest::_handle_response(bool *ret_value) {
 	}
 
 	if (response_code == 301 || response_code == 302) {
-		// Handle redirect
+		// Handle redirect.
 
 		if (max_redirects >= 0 && redirections >= max_redirects) {
 			call_deferred(SNAME("_request_done"), RESULT_REDIRECT_LIMIT_REACHED, response_code, response_headers, PackedByteArray());
@@ -244,12 +244,12 @@ bool HTTPRequest::_handle_response(bool *ret_value) {
 		}
 
 		if (!new_request.is_empty()) {
-			// Process redirect
+			// Process redirect.
 			client->close();
-			int new_redirs = redirections + 1; // Because _request() will clear it
+			int new_redirs = redirections + 1; // Because _request() will clear it.
 			Error err;
 			if (new_request.begins_with("http")) {
-				// New url, request all again
+				// New url, new request.
 				_parse_url(new_request);
 			} else {
 				request_string = new_request;
@@ -276,11 +276,11 @@ bool HTTPRequest::_update_connection() {
 	switch (client->get_status()) {
 		case HTTPClient::STATUS_DISCONNECTED: {
 			call_deferred(SNAME("_request_done"), RESULT_CANT_CONNECT, 0, PackedStringArray(), PackedByteArray());
-			return true; // End it, since it's doing something
+			return true; // End it, since it's disconnected.
 		} break;
 		case HTTPClient::STATUS_RESOLVING: {
 			client->poll();
-			// Must wait
+			// Must wait.
 			return false;
 		} break;
 		case HTTPClient::STATUS_CANT_RESOLVE: {
@@ -290,9 +290,9 @@ bool HTTPRequest::_update_connection() {
 		} break;
 		case HTTPClient::STATUS_CONNECTING: {
 			client->poll();
-			// Must wait
+			// Must wait.
 			return false;
-		} break; // Connecting to IP
+		} break; // Connecting to IP.
 		case HTTPClient::STATUS_CANT_CONNECT: {
 			call_deferred(SNAME("_request_done"), RESULT_CANT_CONNECT, 0, PackedStringArray(), PackedByteArray());
 			return true;
@@ -301,7 +301,7 @@ bool HTTPRequest::_update_connection() {
 		case HTTPClient::STATUS_CONNECTED: {
 			if (request_sent) {
 				if (!got_response) {
-					// No body
+					// No body.
 
 					bool ret_value;
 
@@ -313,16 +313,16 @@ bool HTTPRequest::_update_connection() {
 					return true;
 				}
 				if (body_len < 0) {
-					// Chunked transfer is done
+					// Chunked transfer is done.
 					call_deferred(SNAME("_request_done"), RESULT_SUCCESS, response_code, response_headers, body);
 					return true;
 				}
 
 				call_deferred(SNAME("_request_done"), RESULT_CHUNKED_BODY_SIZE_MISMATCH, response_code, response_headers, PackedByteArray());
 				return true;
-				// Request might have been done
+				// Request might have been done.
 			} else {
-				// Did not request yet, do request
+				// Did not request yet, do request.
 
 				int size = request_data.size();
 				Error err = client->request(method, request_string, headers, size > 0 ? request_data.ptr() : nullptr, size);
@@ -334,13 +334,13 @@ bool HTTPRequest::_update_connection() {
 				request_sent = true;
 				return false;
 			}
-		} break; // Connected: break requests only accepted here
+		} break; // Connected: break requests only accepted here.
 		case HTTPClient::STATUS_REQUESTING: {
-			// Must wait, still requesting
+			// Must wait, still requesting.
 			client->poll();
 			return false;
 
-		} break; // Request in progress
+		} break; // Request in progress.
 		case HTTPClient::STATUS_BODY: {
 			if (!got_response) {
 				bool ret_value;
@@ -411,7 +411,7 @@ bool HTTPRequest::_update_connection() {
 
 			return false;
 
-		} break; // Request resulted in body: break which must be read
+		} break; // Request resulted in body: break which must be read.
 		case HTTPClient::STATUS_CONNECTION_ERROR: {
 			call_deferred(SNAME("_request_done"), RESULT_CONNECTION_ERROR, 0, PackedStringArray(), PackedByteArray());
 			return true;
@@ -428,7 +428,7 @@ bool HTTPRequest::_update_connection() {
 void HTTPRequest::_request_done(int p_status, int p_code, const PackedStringArray &p_headers, const PackedByteArray &p_data) {
 	cancel_request();
 
-	// Determine if the request body is compressed
+	// Determine if the request body is compressed.
 	bool is_compressed;
 	String content_encoding = get_header_value(p_headers, "Content-Encoding").to_lower();
 	Compression::Mode mode;
@@ -452,11 +452,11 @@ void HTTPRequest::_request_done(int p_status, int p_code, const PackedStringArra
 		} else if (result == -5) {
 			WARN_PRINT("Decompressed size of HTTP response body exceeded body_size_limit");
 			p_status = RESULT_BODY_SIZE_LIMIT_EXCEEDED;
-			// Just return the raw data if we failed to decompress it
+			// Just return the raw data if we failed to decompress it.
 		} else {
 			WARN_PRINT("Failed to decompress HTTP response body");
 			p_status = RESULT_BODY_DECOMPRESS_FAILED;
-			// Just return the raw data if we failed to decompress it
+			// Just return the raw data if we failed to decompress it.
 		}
 	}
 
@@ -471,7 +471,6 @@ void HTTPRequest::_notification(int p_what) {
 		bool done = _update_connection();
 		if (done) {
 			set_process_internal(false);
-			// cancel_request(); called from _request done now
 		}
 	}
 
@@ -619,7 +618,6 @@ void HTTPRequest::_bind_methods() {
 	ADD_SIGNAL(MethodInfo("request_completed", PropertyInfo(Variant::INT, "result"), PropertyInfo(Variant::INT, "response_code"), PropertyInfo(Variant::PACKED_STRING_ARRAY, "headers"), PropertyInfo(Variant::PACKED_BYTE_ARRAY, "body")));
 
 	BIND_ENUM_CONSTANT(RESULT_SUCCESS);
-	//BIND_ENUM_CONSTANT( RESULT_NO_BODY );
 	BIND_ENUM_CONSTANT(RESULT_CHUNKED_BODY_SIZE_MISMATCH);
 	BIND_ENUM_CONSTANT(RESULT_CANT_CONNECT);
 	BIND_ENUM_CONSTANT(RESULT_CANT_RESOLVE);