Don't include default port in upstream url to proxy #498
Conversation
|
don't just throw an uncommented commit and a link at us, describe what problem you experience and what the commit is meant to fix. the commit message has unlimited space for explanations. |
|
Hello @rofl0r, since the issue is very similar to the one in curl library, I thought linking them was better. The problem is that Example (tinyproxy listening on This issue is only present on default ports, as using other port yields the same for both So this PR makes url sent to proxy to not include the port if it's the default for its protocol (so both |
are you talking about an actual upstream (see Upstream directive in config) or just the http server one connects to ? also, what upstream server fails to process this correctly ? it seems odd nobody ever reported this as an issue before if this was an actual practical problem, not just theoretical. apart from that, HTTPS connections are made via CONNECT method, so whatever the browser puts into Host: field is forwarded unchanged to the target. btw do you use latest master, or some outdated version for your testing ? |
Yes, I'm talking about the "actual upstream" (connected using the
I found out this issue because I'm chaining
The problem is not the
Tested compiling latest master |
|
i've taken the time to read the mentioned RFC, and when it comes to the "optional port number", it refers to https://www.rfc-editor.org/rfc/rfc3986#section-3.2.3, which reads:
emphasis mine. so the port should be omitted, but it's not a must. in which case the text would use the terms this basically means that the culprit isn't tinyproxy (it conforms to RFC), but apt-cache-ng, and therefore apt-cache-ng should be fixed. |
The link you provided is for RFC3986, but the issue is not with that one but RFC7230, specifically section 5.4:
As seen in the examples, the client correctly set both target URI and Host to be identical, but it's
Even though upstream proxies could fix this on their end the same way |
|
good find. can you try whether the attached patch fixes the issue (apply on master without your PR) diff --git a/src/reqs.c b/src/reqs.c
index 45db118..5179505 100644
--- a/src/reqs.c
+++ b/src/reqs.c
@@ -258,11 +258,7 @@ establish_http_connection (struct conn_s *connptr, struct request_s *request)
char portbuff[7];
char dst[sizeof(struct in6_addr)];
- /* Build a port string if it's not a standard port */
- if (request->port != HTTP_PORT && request->port != HTTP_PORT_SSL)
- snprintf (portbuff, 7, ":%u", request->port);
- else
- portbuff[0] = '\0';
+ snprintf (portbuff, 7, ":%u", request->port);
if (inet_pton(AF_INET6, request->host, dst) > 0) {
/* host is an IPv6 address literal, so surround it withthe patch makes it so the port field is unconditionally added in both uri and host field, and should therefore comply with the requirements of RFC7230. |
That doesn't work for https as you mentioned
So this would still cause issues, and there's also a concern for leaving the port unconditionally as already talked in the upstream issue, quoting the relevant parts: issue request poster:
curl member reply:
Given |
|
happy with this patch ? please test. diff --git a/src/reqs.c b/src/reqs.c
index 45db118..8ce0a44 100644
--- a/src/reqs.c
+++ b/src/reqs.c
@@ -249,6 +249,19 @@ ERROR_EXIT:
return -1;
}
+/*
+ * fill into buf either :port or empty string,
+ * according to wether the port is a default port.
+ * buf must be of size >= 7
+ */
+static void port_or_not(char *buf, int port) {
+ /* Build a port string if it's not a standard port */
+ if (port != HTTP_PORT && port != HTTP_PORT_SSL)
+ snprintf (buf, 7, ":%u", port);
+ else
+ buf[0] = '\0';
+}
+
/*
* Create a connection for HTTP connections.
*/
@@ -258,11 +271,7 @@ establish_http_connection (struct conn_s *connptr, struct request_s *request)
char portbuff[7];
char dst[sizeof(struct in6_addr)];
- /* Build a port string if it's not a standard port */
- if (request->port != HTTP_PORT && request->port != HTTP_PORT_SSL)
- snprintf (portbuff, 7, ":%u", request->port);
- else
- portbuff[0] = '\0';
+ port_or_not(portbuff, request->port);
if (inet_pton(AF_INET6, request->host, dst) > 0) {
/* host is an IPv6 address literal, so surround it with
@@ -1413,6 +1422,7 @@ connect_to_upstream (struct conn_s *connptr, struct request_s *request)
*/
return -1;
#else
+ char portbuff[7];
char *combined_string;
int len;
@@ -1451,6 +1461,8 @@ connect_to_upstream (struct conn_s *connptr, struct request_s *request)
"using file descriptor %d.",
cur_upstream->host, connptr->server_fd);
+ port_or_not(portbuff, request->port);
+
/*
* We need to re-write the "path" part of the request so that we
* can reuse the establish_http_connection() function. It expects a
@@ -1464,8 +1476,8 @@ connect_to_upstream (struct conn_s *connptr, struct request_s *request)
return -1;
}
- snprintf (combined_string, len, "%s:%d", request->host,
- request->port);
+ snprintf (combined_string, len, "%s%s", request->host,
+ portbuff);
} else {
len = strlen (request->host) + strlen (request->path) + 14;
combined_string = (char *) safemalloc (len);
@@ -1473,8 +1485,8 @@ connect_to_upstream (struct conn_s *connptr, struct request_s *request)
return -1;
}
- snprintf (combined_string, len, "http://%s:%d%s", request->host,
- request->port, request->path);
+ snprintf (combined_string, len, "http://%s%s%s", request->host,
+ portbuff, request->path);
}
if (request->path) |
I got diff --git a/src/reqs.c b/src/reqs.c
index 45db118..8ce0a44 100644
--- a/src/reqs.c
+++ b/src/reqs.c
@@ -249,6 +249,19 @@ ERROR_EXIT:
return -1;
}
+/*
+ * fill into buf either :port or empty string,
+ * according to wether the port is a default port.
+ * buf must be of size >= 7
+ */
+static void port_or_not(char *buf, int port) {
+ /* Build a port string if it's not a standard port */
+ if (port != HTTP_PORT && port != HTTP_PORT_SSL)
+ snprintf (buf, 7, ":%u", port);
+ else
+ buf[0] = '\0';
+}
+
/*
* Create a connection for HTTP connections.
*/
@@ -258,11 +271,7 @@ establish_http_connection (struct conn_s *connptr, struct request_s *request)
char portbuff[7];
char dst[sizeof(struct in6_addr)];
- /* Build a port string if it's not a standard port */
- if (request->port != HTTP_PORT && request->port != HTTP_PORT_SSL)
- snprintf (portbuff, 7, ":%u", request->port);
- else
- portbuff[0] = '\0';
+ port_or_not(portbuff, request->port);
if (inet_pton(AF_INET6, request->host, dst) > 0) {
/* host is an IPv6 address literal, so surround it with
@@ -1413,6 +1413,7 @@ connect_to_upstream (struct conn_s *connptr, struct request_s *request)
*/
return -1;
#else
+ char portbuff[7];
char *combined_string;
int len;
@@ -1451,6 +1451,8 @@ connect_to_upstream (struct conn_s *connptr, struct request_s *request)
"using file descriptor %d.",
cur_upstream->host, connptr->server_fd);
+ port_or_not(portbuff, request->port);
+
/*
* We need to re-write the "path" part of the request so that we
* can reuse the establish_http_connection() function. It expects a
@@ -1464,8 +1464,8 @@ connect_to_upstream (struct conn_s *connptr, struct request_s *request)
return -1;
}
- snprintf (combined_string, len, "%s:%d", request->host,
- request->port);
+ snprintf (combined_string, len, "%s%s", request->host,
+ portbuff);
} else {
len = strlen (request->host) + strlen (request->path) + 14;
combined_string = (char *) safemalloc (len);
@@ -1473,8 +1473,8 @@ connect_to_upstream (struct conn_s *connptr, struct request_s *request)
return -1;
}
- snprintf (combined_string, len, "http://%s:%d%s", request->host,
- request->port, request->path);
+ snprintf (combined_string, len, "http://%s%s%s", request->host,
+ portbuff, request->path);
}
if (request->path)Also ran some tests and couldn't find any inconsistent target uri & host, so it's working as expected. |
|
i can see a potential issue though, someone running a plain http server on port 443... probably quite uncommon, yet possible |
We can use the diff --git a/src/reqs.c b/src/reqs.c
index 45db118..729ddf4 100644
--- a/src/reqs.c
+++ b/src/reqs.c
@@ -249,6 +249,22 @@ ERROR_EXIT:
return -1;
}
+/*
+ * fill into buf either :port or empty string,
+ * according to wether the port is a default port.
+ * buf must be of size >= 7
+ */
+static void port_or_not(char *buf, int port, unsigned int connect_method) {
+ /* Build a port string if it's not a standard port */
+ if (
+ (!connect_method && port != HTTP_PORT)
+ || (connect_method && port != HTTP_PORT_SSL)
+ )
+ snprintf (buf, 7, ":%u", port);
+ else
+ buf[0] = '\0';
+}
+
/*
* Create a connection for HTTP connections.
*/
@@ -258,11 +274,7 @@ establish_http_connection (struct conn_s *connptr, struct request_s *request)
char portbuff[7];
char dst[sizeof(struct in6_addr)];
- /* Build a port string if it's not a standard port */
- if (request->port != HTTP_PORT && request->port != HTTP_PORT_SSL)
- snprintf (portbuff, 7, ":%u", request->port);
- else
- portbuff[0] = '\0';
+ port_or_not(portbuff, request->port, connptr->connect_method);
if (inet_pton(AF_INET6, request->host, dst) > 0) {
/* host is an IPv6 address literal, so surround it with
@@ -1413,6 +1425,7 @@ connect_to_upstream (struct conn_s *connptr, struct request_s *request)
*/
return -1;
#else
+ char portbuff[7];
char *combined_string;
int len;
@@ -1451,6 +1464,8 @@ connect_to_upstream (struct conn_s *connptr, struct request_s *request)
"using file descriptor %d.",
cur_upstream->host, connptr->server_fd);
+ port_or_not(portbuff, request->port, connptr->connect_method);
+
/*
* We need to re-write the "path" part of the request so that we
* can reuse the establish_http_connection() function. It expects a
@@ -1464,8 +1479,8 @@ connect_to_upstream (struct conn_s *connptr, struct request_s *request)
return -1;
}
- snprintf (combined_string, len, "%s:%d", request->host,
- request->port);
+ snprintf (combined_string, len, "%s%s", request->host,
+ portbuff);
} else {
len = strlen (request->host) + strlen (request->path) + 14;
combined_string = (char *) safemalloc (len);
@@ -1473,8 +1488,8 @@ connect_to_upstream (struct conn_s *connptr, struct request_s *request)
return -1;
}
- snprintf (combined_string, len, "http://%s:%d%s", request->host,
- request->port, request->path);
+ snprintf (combined_string, len, "http://%s%s%s", request->host,
+ portbuff, request->path);
}
if (request->path)Should I edit the original PR? |
|
@ejose19 I personally would have the following thoughts / opinions to make it clear:
Examples for configurations:
Could you provide your configuration examples you had used when you faced the issue ? For me this would make the logic much more understandable, and if a port is configured, then it will be used. If not then not. It can be, that this comment decribes what was in the last code diff proposed ... not got it fully. But it really would be good i think if we know how you had configured the tinyproxy.conf, to avoid to do assumptions |
Related:
curl/curl#6769
curl/curl@3bbf62b