On Wed, May 13, 2020 at 8:37 AM Ruediger Pluem <[email protected]> wrote:
>
> On 5/12/20 2:20 PM, [email protected] wrote:
>
> > @@ -1509,6 +1564,7 @@ int ap_proxy_http_process_response(proxy
> > ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r,
> > "HTTP: received interim %d response", r->status);
> > if (!policy
> > + || upgrade
>
> Shouldn't that be (upgrade && proxy_status == HTTP_SWITCHING_PROTOCOLS) in
> case someone sends a upgrade header
> without status HTTP_SWITCHING_PROTOCOLS?
Yes, "upgrade" was first set only for HTTP_SWITCHING_PROTOCOLS so this
test was enough, but then a later commit (on github) allowed it to be
forwarded for other status codes (as you caught below).
Fixed in r1877695.
> Furthermore don't we need to add in back the upgrade header and the
> Connection: upgrade before we send the interim
> response with HTTP_SWITCHING_PROTOCOLS?
Of course, fixed in r1877695 too.
> >
> > + /* Forward back Upgrade header if it matches the configured
> > one(s). */
> > + if (upgrade && ap_proxy_worker_can_upgrade(p, worker, upgrade)) {
> > + apr_table_setn(r->headers_out, "Connection", "Upgrade");
> > + apr_table_setn(r->headers_out, "Upgrade", apr_pstrdup(p,
> > upgrade));
> > + }
> > +
>
> Hmm. When do we hit this? When the backend sends HTTP_UPGRADE_REQUIRED? And
> if yes shouldn't we only care in this case?
Yes, HTTP_UPGRADE_REQUIRED or any status actually (unless
ProxyErrorOverride takes place).
We want to allow Upgrade negotiation between the client and the
backend (or the other way around), and I'm not sure that
HTTP_UPGRADE_REQUIRED is the only way to do that, so provided the
upgrade= configuration includes the protocol I think we should forward
it.
Regards;
Yann.
