From bf1cbc2499c76003295d4e0c8eccf518726015ca Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 21 Nov 2016 15:41:47 -0800
Subject: [PATCH] Don't let users pick "whatever.git" as a repository short
 name, make "." work

Summary:
Fixes T11902.

  - Periods now work in short names.
  - If you try to name something ".git", no dice.

Test Plan:
  - Tried to name something "quack.git", was politely rejected.
  - Named something "quack.notgit", and it worked fine.
  - Cloned Mercurial and Git repositories over SSH with ".git" and non-".git" variants without hitting any issues.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11902

Differential Revision: https://secure.phabricator.com/D16908
---
 .../PhabricatorDiffusionApplication.php       |  2 +-
 .../controller/DiffusionController.php        |  2 ++
 .../controller/DiffusionServeController.php   |  7 -----
 .../storage/PhabricatorRepository.php         | 28 +++++++++++--------
 .../PhabricatorRepositoryTestCase.php         |  3 ++
 5 files changed, 22 insertions(+), 20 deletions(-)

diff --git a/src/applications/diffusion/application/PhabricatorDiffusionApplication.php b/src/applications/diffusion/application/PhabricatorDiffusionApplication.php
index c320c5cc26..1ee4c0ef16 100644
--- a/src/applications/diffusion/application/PhabricatorDiffusionApplication.php
+++ b/src/applications/diffusion/application/PhabricatorDiffusionApplication.php
@@ -101,7 +101,7 @@ final class PhabricatorDiffusionApplication extends PhabricatorApplication {
       ')(?P<commit>[a-f0-9]+)'
         => 'DiffusionCommitController',
 
-      '/source/(?P<repositoryShortName>[^/.]+)(?P<dotgit>\.git)?'
+      '/source/(?P<repositoryShortName>[^/]+)'
         => $repository_routes,
 
       '/diffusion/' => array(
diff --git a/src/applications/diffusion/controller/DiffusionController.php b/src/applications/diffusion/controller/DiffusionController.php
index 16fde7ac26..19fa7def7e 100644
--- a/src/applications/diffusion/controller/DiffusionController.php
+++ b/src/applications/diffusion/controller/DiffusionController.php
@@ -92,6 +92,8 @@ abstract class DiffusionController extends PhabricatorController {
 
     $short_name = $request->getURIData('repositoryShortName');
     if (strlen($short_name)) {
+      // If the short name ends in ".git", ignore it.
+      $short_name = preg_replace('/\\.git\z/', '', $short_name);
       return $short_name;
     }
 
diff --git a/src/applications/diffusion/controller/DiffusionServeController.php b/src/applications/diffusion/controller/DiffusionServeController.php
index 65feb79ea0..02ff23b15f 100644
--- a/src/applications/diffusion/controller/DiffusionServeController.php
+++ b/src/applications/diffusion/controller/DiffusionServeController.php
@@ -88,13 +88,6 @@ final class DiffusionServeController extends DiffusionController {
       }
     }
 
-    // If the request was for a path like "/source/libphutil.git" but the
-    // repository is not a Git repository, reject the request.
-    $type_git = PhabricatorRepositoryType::REPOSITORY_TYPE_GIT;
-    if ($request->getURIData('dotgit') && ($vcs !== $type_git)) {
-      return null;
-    }
-
     return $vcs;
   }
 
diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php
index 8c32109f37..5667dc4a11 100644
--- a/src/applications/repository/storage/PhabricatorRepository.php
+++ b/src/applications/repository/storage/PhabricatorRepository.php
@@ -442,6 +442,15 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
           'short names may not contain only numbers.',
           $slug));
     }
+
+    if (preg_match('/\\.git/', $slug)) {
+      throw new Exception(
+        pht(
+          'The name "%s" is not a valid repository short name. Repository '.
+          'short names must not end in ".git". This suffix will be added '.
+          'automatically in appropriate contexts.',
+          $slug));
+    }
   }
 
   public static function assertValidCallsign($callsign) {
@@ -592,21 +601,12 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
   }
 
   public static function parseRepositoryServicePath($request_path, $vcs) {
-
-    // NOTE: In Mercurial over SSH, the path will begin without a leading "/",
-    // so we're matching it optionally.
-
-    if ($vcs == PhabricatorRepositoryType::REPOSITORY_TYPE_GIT) {
-      $maybe_git = '(?:\\.git)?';
-    } else {
-      $maybe_git = null;
-    }
+    $is_git = ($vcs == PhabricatorRepositoryType::REPOSITORY_TYPE_GIT);
 
     $patterns = array(
       '(^'.
-        '(?P<base>/?(?:diffusion|source)/(?P<identifier>[^/.]+))'.
-        $maybe_git.
-        '(?P<path>(?:/|.*)?)'.
+        '(?P<base>/?(?:diffusion|source)/(?P<identifier>[^/]+))'.
+        '(?P<path>.*)'.
       '\z)',
     );
 
@@ -618,6 +618,10 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
       }
 
       $identifier = $matches['identifier'];
+      if ($is_git) {
+        $identifier = preg_replace('/\\.git\z/', '', $identifier);
+      }
+
       $base = $matches['base'];
       $path = $matches['path'];
       break;
diff --git a/src/applications/repository/storage/__tests__/PhabricatorRepositoryTestCase.php b/src/applications/repository/storage/__tests__/PhabricatorRepositoryTestCase.php
index 63bae03d80..f78c3374ed 100644
--- a/src/applications/repository/storage/__tests__/PhabricatorRepositoryTestCase.php
+++ b/src/applications/repository/storage/__tests__/PhabricatorRepositoryTestCase.php
@@ -190,6 +190,9 @@ final class PhabricatorRepositoryTestCase
       '-ated',
       '_underscores_',
       'yes!',
+      'quack.git',
+      'git.git',
+      '.git.git.git',
 
       // 65-character names are no good.
       str_repeat('a', 65),