> Am 04.03.2022 um 11:09 schrieb Ruediger Pluem <rpl...@apache.org>:
>
>
>
> On 3/4/22 10:50 AM, Stefan Eissing wrote:
>>
>>
>>> Am 04.03.2022 um 10:22 schrieb Ruediger Pluem <rpl...@apache.org>:
>>>
>>>
>>>
>>> On 3/4/22 9:51 AM, ic...@apache.org wrote:
>>>> Author: icing
>>>> Date: Fri Mar 4 08:51:47 2022
>>>> New Revision: 1898586
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=1898586&view=rev
>>>> Log:
>>>> merge of 1898146,1898173 from trunk:
>>>>
>>>> *) mod_http2: preserve the port number given in a HTTP/1.1
>>>> request that was Upgraded to HTTP/2. Fixes PR65881.
>>>>
>>>>
>>>> Added:
>>>> httpd/httpd/branches/2.4.x/changes-entries/pr65881.txt
>>>> - copied unchanged from r1898146,
>>>> httpd/httpd/trunk/changes-entries/pr65881.txt
>>>> httpd/httpd/branches/2.4.x/test/modules/http2/test_502_proxy_port.py
>>>> - copied unchanged from r1898146,
>>>> httpd/httpd/trunk/test/modules/http2/test_502_proxy_port.py
>>>> Modified:
>>>> httpd/httpd/branches/2.4.x/ (props changed)
>>>> httpd/httpd/branches/2.4.x/modules/http2/h2_request.c
>>>> httpd/httpd/branches/2.4.x/test/modules/http2/env.py
>>>> httpd/httpd/branches/2.4.x/test/modules/http2/htdocs/cgi/hello.py
>>>>
>>>> Propchange: httpd/httpd/branches/2.4.x/
>>>> ------------------------------------------------------------------------------
>>>> Merged /httpd/httpd/trunk:r1898146,1898173
>>>>
>>>> Modified: httpd/httpd/branches/2.4.x/modules/http2/h2_request.c
>>>> URL:
>>>> http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/modules/http2/h2_request.c?rev=1898586&r1=1898585&r2=1898586&view=diff
>>>> ==============================================================================
>>>> --- httpd/httpd/branches/2.4.x/modules/http2/h2_request.c (original)
>>>> +++ httpd/httpd/branches/2.4.x/modules/http2/h2_request.c Fri Mar 4
>>>> 08:51:47 2022
>>>> @@ -69,12 +69,25 @@ apr_status_t h2_request_rcreate(h2_reque
>>>> return APR_EINVAL;
>>>> }
>>>>
>>>> - if (!ap_strchr_c(authority, ':') && r->server && r->server->port) {
>>>> - apr_port_t defport = apr_uri_port_of_scheme(scheme);
>>>> - if (defport != r->server->port) {
>>>> - /* port info missing and port is not default for scheme:
>>>> append */
>>>> - authority = apr_psprintf(pool, "%s:%d", authority,
>>>> - (int)r->server->port);
>>>> + /* The authority we carry in h2_request is the 'authority' part of
>>>> + * the URL for the request. r->hostname has stripped any port info
>>>> that
>>>> + * might have been present. Do we need to add it?
>>>> + */
>>>> + if (!ap_strchr_c(authority, ':')) {
>>>> + if (r->parsed_uri.port_str) {
>>>> + /* Yes, it was there, add it again. */
>>>> + authority = apr_pstrcat(pool, authority, ":",
>>>> r->parsed_uri.port_str, NULL);
>>>> + }
>>>> + else if (!r->parsed_uri.hostname && r->server && r->server->port)
>>>> {
>>>> + /* If there was no hostname in the parsed URL, the URL was
>>>> relative.
>>>> + * In that case, we restore port from our server->port, if it
>>>> + * is known and not the default port for the scheme. */
>>>> + apr_port_t defport = apr_uri_port_of_scheme(scheme);
>>>> + if (defport != r->server->port) {
>>>> + /* port info missing and port is not default for scheme:
>>>> append */
>>>> + authority = apr_psprintf(pool, "%s:%d", authority,
>>>> + (int)r->server->port);
>>>> + }
>>>> }
>>>> }
>>>
>>> Sorry for my late comment, but I think the above ignores the setting of
>>> UseCanonicalPhysicalPort and UseCanonicalName.
>>>
>>> I think we should add what is returned by ap_get_server_port in case this
>>> differs from apr_uri_port_of_scheme(scheme)
>>>
>>> I think of something like the below:
>>>
>>> Index: modules/http2/h2_request.c
>>> ===================================================================
>>> --- modules/http2/h2_request.c (revision 1898509)
>>> +++ modules/http2/h2_request.c (working copy)
>>> @@ -95,21 +95,13 @@
>>> * might have been present. Do we need to add it?
>>> */
>>> if (!ap_strchr_c(authority, ':')) {
>>> - if (r->parsed_uri.port_str) {
>>> - /* Yes, it was there, add it again. */
>>> - authority = apr_pstrcat(pool, authority, ":",
>>> r->parsed_uri.port_str, NULL);
>>> + apr_port_t defport = apr_uri_port_of_scheme(scheme);
>>> + apr_port_t port = ap_get_server_port(r);
>>> +
>>> + if (defport != port) {
>>> + /* port info missing and port is not default for scheme:
>>> append */
>>> + authority = apr_psprintf(pool, "%s:%d", authority, (int)port);
>>> }
>>> - else if (!r->parsed_uri.hostname && r->server && r->server->port) {
>>> - /* If there was no hostname in the parsed URL, the URL was
>>> relative.
>>> - * In that case, we restore port from our server->port, if it
>>> - * is known and not the default port for the scheme. */
>>> - apr_port_t defport = apr_uri_port_of_scheme(scheme);
>>> - if (defport != r->server->port) {
>>> - /* port info missing and port is not default for scheme:
>>> append */
>>> - authority = apr_psprintf(pool, "%s:%d", authority,
>>> - (int)r->server->port);
>>> - }
>>> - }
>>> }
>>>
>>> req = apr_pcalloc(pool, sizeof(*req));
>>>
>>
>> Like in r1898593 now?
>
> Not completely. UseCanonicalPhysicalPort and UseCanonicalName IMHO aim to
> give the admin
> control whether he wants to accept user provided input data with regards to
> hostnames and ports.
> r1898593 uses r->parsed_uri.port_str if present no matter what
> UseCanonicalPhysicalPort and UseCanonicalName
> say. This is wrong from my point of view, but my point of view might be
> wrong. Hence let's hear what others say.
Ah, but the upgraded h1 request is converted to a h2 request which will then
enter out "normal" request
processing again afterwards. Wouldn't all the directives apply then? I hope so.
>
> Regards
>
> Rüdiger
