Page MenuHomePhabricator

Add a client setup check to detect that the browser is using HTTPS but Phabricator does not recognize it
Closed, ResolvedPublic

Description

If user uses nginx(ssl) for phabricator proxy, phabricator request protocol is "http".

https://github.com/phacility/phabricator/blob/master/src/view/page/PhabricatorStandardPageView.php#L532

see this source, you can find bug. It should be changed like this.

$request->isHTTPS()
to
$client_uri->getProtocol() == "https"

Event Timeline

subicura created this task.Feb 20 2016, 7:40 AM

To clarify, you are using HTTP for Phabricator but HTTPS for the notification server? If so, why have you made this (unusual) configuration choice?

I use nginx for reverse proxy in front.

client -> nginx(https) -> phabricator/apache(http) docker container

So client connect with ssl, but phabricator sever receive not ssl.

So your client is sending an HTTPS request, but $request->isHTTPS() is false?

This is a misconfiguration.

See "Adjusting SSL" here for one approach to fixing it:

https://secure.phabricator.com/book/phabricator/article/configuring_preamble/

Oh, thanks for advise. I'll try to fix!

epriestley renamed this task from notification websockerURI should use wss protocol when use nginx to Add a client setup check to detect that the browser is using HTTPS but Phabricator does not recognize it.Feb 20 2016, 12:33 PM
epriestley edited projects, added Setup; removed Bug Report.

Some things we can do easily here:

  • Modify the "Notification Servers" panel to show which servers are connectable and make it clear when there's an HTTP vs HTTPS mismatch.
  • Examine the X-Forwarded-Proto header -- without trusting it -- and use it to raise a setup warning about HTTP vs HTTPS mismatches.

That won't cover cases where SSL is silently terminated by nginx, or by some other loadbalancer-like reverse proxy that doesn't add X-Forwarded-Proto.

We can detect all cases of HTTP/HTTPS mismatch by having the client examine the URI, then tell the server what it's using. However, we have to do this with Javascript and can't easily do it at the same time as we run other setup issues.

It's also not straightforward to figure out when to perform this check. The standard server-side setup issues may be triggered by, e.g., a loadbalancer health check request to /status/, but this is an inappropriate time to do HTTPS checks.

This might actually be more similar to T3025, basically:

  • When rendering a page, send the HTTP/HTTPS setting to the client (and, in T3025, the timezone setting).
  • If they differ, prompt the user to reconcile them.

Other vaguely-similar setup issues might be language settings and date-time preferences. Even if we can infer these from "Accept:" headers, prompting the user to reconcile them is likely the best course of action.

See also T2226, etc., for a vaguely related additional class of setup check (where we must test a server response).

@epriestley am I good to take a stab here at implementing the T3025-like behaviour?
If user is admin and clientProto != serverProto, display notification.
Approximately 5% familiar with Phabricator's JS side, but always interested in poking around more of the codebase - and I'm guessing most of this is gonna be touched in D15961.

Yeah, I think something similar to D15961 is reasonable here. It's going to a little bit one-off and weird, but this and the timezone checks are the only cases where we care about client settings, I think.

Ideally we'd do some kind of magic to just turn this into a normal client setup check somehow, but I don't really see any reasonable pathway toward that which doesn't run us into a huge fragile mess.

asherkin claimed this task.Jun 6 2016, 9:26 PM
asherkin triaged this task as Low priority.