From fde02c4b4edfb70a40cc98f28d9e173202098ef1 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 5 May 2016 16:16:09 -0700
Subject: [PATCH] Fix protocol serve detection for clustered repositories that
 terminate HTTPS

Summary:
Ref T10927. Pretty sure the issue is:

  - User makes an HTTPS request.
  - Load balancer terminates it, but with an `X-Forwarded-Proto` header.
  - `secure001` (or whatever; acting as web host) proxies it to `secure002` (or whatever; acting as a repository host). **This** connection is plain HTTP.
  - Since this proxied connection is plain HTTP, we check if the repository can serve over "http", but it can't: only "https". So we fail incorrectly, even though the original user request was HTTPS.

In the long run we should probably forward the `X-Forwarded-Proto` header, but that has some weird implications and it's broadly fine to allow either protocol to serve as long as the other one is active: configuration like `security.require-https` is already stronger than these settings.

Test Plan: This is likely only observable in production, but normal cloning still works locally.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10927

Differential Revision: https://secure.phabricator.com/D15856
---
 .../controller/DiffusionServeController.php   | 21 ++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/src/applications/diffusion/controller/DiffusionServeController.php b/src/applications/diffusion/controller/DiffusionServeController.php
index 06988ff4e3..69b8c64b17 100644
--- a/src/applications/diffusion/controller/DiffusionServeController.php
+++ b/src/applications/diffusion/controller/DiffusionServeController.php
@@ -267,20 +267,27 @@ final class DiffusionServeController extends DiffusionController {
       // token from SSH. If they're using HTTP username + password auth, they
       // have to obey the normal HTTP rules.
     } else {
-      if ($request->isHTTPS()) {
-        $protocol = PhabricatorRepositoryURI::BUILTIN_PROTOCOL_HTTPS;
-      } else {
-        $protocol = PhabricatorRepositoryURI::BUILTIN_PROTOCOL_HTTP;
-      }
+      // For now, we don't distinguish between HTTP and HTTPS-originated
+      // requests that are proxied within the cluster, so the user can connect
+      // with HTTPS but we may be on HTTP by the time we reach this part of
+      // the code. Allow things to move forward as long as either protocol
+      // can be served.
+      $proto_https = PhabricatorRepositoryURI::BUILTIN_PROTOCOL_HTTPS;
+      $proto_http = PhabricatorRepositoryURI::BUILTIN_PROTOCOL_HTTP;
 
-      if (!$repository->canServeProtocol($protocol, false)) {
+      $can_read =
+        $repository->canServeProtocol($proto_https, false) ||
+        $repository->canServeProtocol($proto_http, false);
+      if (!$can_read) {
         return new PhabricatorVCSResponse(
           403,
           pht('This repository is not available over HTTP.'));
       }
 
       if ($is_push) {
-        $can_write = $repository->canServeProtocol($protocol, true);
+        $can_write =
+          $repository->canServeProtocol($proto_https, true) ||
+          $repository->canServeProtocol($proto_http, true);
         if (!$can_write) {
           return new PhabricatorVCSResponse(
             403,