Enforce sensible, unique clone/checkout names for repositories

Summary:
Fixes T7938.

  - Primarily, users can currently shoot themselves in the foot by putting `../../etc/passwd` and other similar nonsense in these fields (this is not dangerous, but also does not work). Require sensible names.
  - Enforce uniqueness so these names can be used in URIs and as identifiers in the future.
  - (This doesn't start actually using them for anything fancy yet.)

Test Plan:
  - Gave several repositories clone names: a valid name, two duplicate names, an invalid, name, some with no names.
  - Ran migrations.
  - Got clean conversion for valid names, appropriate errors for invalid/duplicate names.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T7938

Differential Revision: https://secure.phabricator.com/D14986

resources/sql/autopatches/20160110.repo.01.slug.sql

@@ -0,0 +1,5 @@
+ALTER TABLE {$NAMESPACE}_repository.repository
+  ADD repositorySlug VARCHAR(64) COLLATE {$COLLATE_SORT};
+
+ALTER TABLE {$NAMESPACE}_repository.repository
+  ADD UNIQUE KEY `key_slug` (repositorySlug);

resources/sql/autopatches/20160110.repo.02.slug.php

@@ -0,0 +1,49 @@
+<?php
+
+$table = new PhabricatorRepository();
+$conn_w = $table->establishConnection('w');
+
+foreach (new LiskMigrationIterator($table) as $repository) {
+  $slug = $repository->getRepositorySlug();
+
+  if ($slug !== null) {
+    continue;
+  }
+
+  $clone_name = $repository->getDetail('clone-name');
+
+  if (!strlen($clone_name)) {
+    continue;
+  }
+
+  if (!PhabricatorRepository::isValidRepositorySlug($clone_name)) {
+    echo tsprintf(
+      "%s\n",
+      pht(
+        'Repository "%s" has a "Clone/Checkout As" name which is no longer '.
+        'valid ("%s"). You can edit the repository to give it a new, valid '.
+        'short name.',
+        $repository->getDisplayName(),
+        $clone_name));
+    continue;
+  }
+
+  try {
+    queryfx(
+      $conn_w,
+      'UPDATE %T SET repositorySlug = %s WHERE id = %d',
+      $table->getTableName(),
+      $clone_name,
+      $repository->getID());
+  } catch (AphrontDuplicateKeyQueryException $ex) {
+    echo tsprintf(
+      "%s\n",
+      pht(
+        'Repository "%s" has a duplicate "Clone/Checkout As" name ("%s"). '.
+        'Each name must now be unique. You can edit the repository to give '.
+        'it a new, unique short name.',
+        $repository->getDisplayName(),
+        $clone_name));
+  }
+
+}

src/applications/diffusion/controller/DiffusionRepositoryEditBasicController.php

@@ -17,13 +17,15 @@ final class DiffusionRepositoryEditBasicController
 
     $v_name = $repository->getName();
     $v_desc = $repository->getDetail('description');
-    $v_clone_name = $repository->getDetail('clone-name');
+    $v_clone_name = $repository->getRepositorySlug();
     $v_projects = PhabricatorEdgeQuery::loadDestinationPHIDs(
       $repository->getPHID(),
       PhabricatorProjectObjectHasProjectEdgeType::EDGECONST);
     $e_name = true;
+    $e_slug = null;
     $errors = array();
 
+    $validation_exception = null;
     if ($request->isFormPost()) {
       $v_name = $request->getStr('name');
       $v_desc = $request->getStr('description');
@@ -71,13 +73,20 @@ final class DiffusionRepositoryEditBasicController
               '=' => array_fuse($v_projects),
             ));
 
-        id(new PhabricatorRepositoryEditor())
+        $editor = id(new PhabricatorRepositoryEditor())
           ->setContinueOnNoEffect(true)
           ->setContentSourceFromRequest($request)
-          ->setActor($viewer)
-          ->applyTransactions($repository, $xactions);
+          ->setActor($viewer);
+
+        try {
+          $editor->applyTransactions($repository, $xactions);
 
           return id(new AphrontRedirectResponse())->setURI($edit_uri);
+        } catch (PhabricatorApplicationTransactionValidationException $ex) {
+          $validation_exception = $ex;
+
+          $e_slug = $ex->getShortMessage($type_clone_name);
+        }
       }
     }
 
@@ -102,6 +111,7 @@ final class DiffusionRepositoryEditBasicController
             ->setName('cloneName')
             ->setLabel(pht('Clone/Checkout As'))
             ->setValue($v_clone_name)
+            ->setError($e_slug)
             ->setCaption(
               pht(
                 'Optional directory name to use when cloning or checking out '.
@@ -130,6 +140,7 @@ final class DiffusionRepositoryEditBasicController
 
     $object_box = id(new PHUIObjectBoxView())
       ->setHeaderText($title)
+      ->setValidationException($validation_exception)
       ->setForm($form)
       ->setFormErrors($errors);
 

src/applications/diffusion/controller/DiffusionRepositoryEditMainController.php

@@ -286,7 +286,7 @@ final class DiffusionRepositoryEditMainController
     $view->addProperty(pht('Type'), $type);
     $view->addProperty(pht('Callsign'), $repository->getCallsign());
 
-    $clone_name = $repository->getDetail('clone-name');
+    $clone_name = $repository->getRepositorySlug();
 
     if ($repository->isHosted()) {
       $view->addProperty(

src/applications/repository/editor/PhabricatorRepositoryEditor.php

@@ -99,7 +99,7 @@ final class PhabricatorRepositoryEditor
       case PhabricatorRepositoryTransaction::TYPE_DANGEROUS:
         return $object->shouldAllowDangerousChanges();
       case PhabricatorRepositoryTransaction::TYPE_CLONE_NAME:
-        return $object->getDetail('clone-name');
+        return $object->getRepositorySlug();
       case PhabricatorRepositoryTransaction::TYPE_SERVICE:
         return $object->getAlmanacServicePHID();
       case PhabricatorRepositoryTransaction::TYPE_SYMBOLS_LANGUAGE:
@@ -141,13 +141,18 @@ final class PhabricatorRepositoryEditor
       case PhabricatorRepositoryTransaction::TYPE_PUSH_POLICY:
       case PhabricatorRepositoryTransaction::TYPE_CREDENTIAL:
       case PhabricatorRepositoryTransaction::TYPE_DANGEROUS:
-      case PhabricatorRepositoryTransaction::TYPE_CLONE_NAME:
       case PhabricatorRepositoryTransaction::TYPE_SERVICE:
       case PhabricatorRepositoryTransaction::TYPE_SYMBOLS_LANGUAGE:
       case PhabricatorRepositoryTransaction::TYPE_SYMBOLS_SOURCES:
       case PhabricatorRepositoryTransaction::TYPE_STAGING_URI:
       case PhabricatorRepositoryTransaction::TYPE_AUTOMATION_BLUEPRINTS:
         return $xaction->getNewValue();
+      case PhabricatorRepositoryTransaction::TYPE_CLONE_NAME:
+        $name = $xaction->getNewValue();
+        if (strlen($name)) {
+          return $name;
+        }
+        return null;
       case PhabricatorRepositoryTransaction::TYPE_NOTIFY:
       case PhabricatorRepositoryTransaction::TYPE_AUTOCLOSE:
         return (int)$xaction->getNewValue();
@@ -216,7 +221,7 @@ final class PhabricatorRepositoryEditor
         $object->setDetail('allow-dangerous-changes', $xaction->getNewValue());
         return;
       case PhabricatorRepositoryTransaction::TYPE_CLONE_NAME:
-        $object->setDetail('clone-name', $xaction->getNewValue());
+        $object->setRepositorySlug($xaction->getNewValue());
         return;
       case PhabricatorRepositoryTransaction::TYPE_SERVICE:
         $object->setAlmanacServicePHID($xaction->getNewValue());
@@ -448,9 +453,69 @@ final class PhabricatorRepositoryEditor
           }
         }
         break;
+
+      case PhabricatorRepositoryTransaction::TYPE_CLONE_NAME:
+        foreach ($xactions as $xaction) {
+          $old = $xaction->getOldValue();
+          $new = $xaction->getNewValue();
+
+          if (!strlen($new)) {
+            continue;
+          }
+
+          if ($new === $old) {
+            continue;
+          }
+
+          try {
+            PhabricatorRepository::asssertValidRepositorySlug($new);
+          } catch (Exception $ex) {
+            $errors[] = new PhabricatorApplicationTransactionValidationError(
+              $type,
+              pht('Invalid'),
+              $ex->getMessage(),
+              $xaction);
+            continue;
+          }
+
+          $other = id(new PhabricatorRepositoryQuery())
+            ->setViewer(PhabricatorUser::getOmnipotentUser())
+            ->withSlugs(array($new))
+            ->executeOne();
+          if ($other && ($other->getID() !== $object->getID())) {
+            $errors[] = new PhabricatorApplicationTransactionValidationError(
+              $type,
+              pht('Duplicate'),
+              pht(
+                'The selected repository short name is already in use by '.
+                'another repository. Choose a unique short name.'),
+              $xaction);
+            continue;
+          }
+        }
+        break;
+
     }
 
     return $errors;
   }
 
+  protected function didCatchDuplicateKeyException(
+    PhabricatorLiskDAO $object,
+    array $xactions,
+    Exception $ex) {
+
+    $errors = array();
+
+    $errors[] = new PhabricatorApplicationTransactionValidationError(
+      null,
+      pht('Invalid'),
+      pht(
+        'The chosen callsign or repository short name is already in '.
+        'use by another repository.'),
+      null);
+
+    throw new PhabricatorApplicationTransactionValidationException($errors);
+  }
+
 }

src/applications/repository/query/PhabricatorRepositoryQuery.php

@@ -11,6 +11,7 @@ final class PhabricatorRepositoryQuery
   private $nameContains;
   private $remoteURIs;
   private $datasourceQuery;
+  private $slugs;
 
   private $numericIdentifiers;
   private $callsignIdentifiers;
@@ -114,6 +115,11 @@ final class PhabricatorRepositoryQuery
     return $this;
   }
 
+  public function withSlugs(array $slugs) {
+    $this->slugs = $slugs;
+    return $this;
+  }
+
   public function needCommitCounts($need_counts) {
     $this->needCommitCounts = $need_counts;
     return $this;
@@ -564,6 +570,13 @@ final class PhabricatorRepositoryQuery
         $callsign);
     }
 
+    if ($this->slugs !== null) {
+      $where[] = qsprintf(
+        $conn,
+        'r.repositorySlug IN (%Ls)',
+        $this->slugs);
+    }
+
     return $where;
   }
 

src/applications/repository/storage/PhabricatorRepository.php

@@ -46,6 +46,7 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
 
   protected $name;
   protected $callsign;
+  protected $repositorySlug;
   protected $uuid;
   protected $viewPolicy;
   protected $editPolicy;
@@ -93,6 +94,7 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
       self::CONFIG_COLUMN_SCHEMA => array(
         'name' => 'sort255',
         'callsign' => 'sort32',
+        'repositorySlug' => 'sort64?',
         'versionControlSystem' => 'text32',
         'uuid' => 'text64?',
         'pushPolicy' => 'policy',
@@ -100,11 +102,6 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
         'almanacServicePHID' => 'phid?',
       ),
       self::CONFIG_KEY_SCHEMA => array(
-        'key_phid' => null,
-        'phid' => array(
-          'columns' => array('phid'),
-          'unique' => true,
-        ),
         'callsign' => array(
           'columns' => array('callsign'),
           'unique' => true,
@@ -115,6 +112,10 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
         'key_vcs' => array(
           'columns' => array('versionControlSystem'),
         ),
+        'key_slug' => array(
+          'columns' => array('repositorySlug'),
+          'unique' => true,
+        ),
       ),
     ) + parent::getConfiguration();
   }
@@ -297,7 +298,7 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
    * @return string
    */
   public function getCloneName() {
-    $name = $this->getDetail('clone-name');
+    $name = $this->getRepositorySlug();
 
     // Make some reasonable effort to produce reasonable default directory
     // names from repository names.
@@ -314,6 +315,82 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
     return $name;
   }
 
+  public static function isValidRepositorySlug($slug) {
+    try {
+      self::asssertValidRepositorySlug($slug);
+      return true;
+    } catch (Exception $ex) {
+      return false;
+    }
+  }
+
+  public static function asssertValidRepositorySlug($slug) {
+    if (!strlen($slug)) {
+      throw new Exception(
+        pht(
+          'The empty string is not a valid repository short name. '.
+          'Repository short names must be at least one character long.'));
+    }
+
+    if (strlen($slug) > 64) {
+      throw new Exception(
+        pht(
+          'The name "%s" is not a valid repository short name. Repository '.
+          'short names must not be longer than 64 characters.',
+          $slug));
+    }
+
+    if (preg_match('/[^a-zA-Z0-9._-]/', $slug)) {
+      throw new Exception(
+        pht(
+          'The name "%s" is not a valid repository short name. Repository '.
+          'short names may only contain letters, numbers, periods, hyphens '.
+          'and underscores.',
+          $slug));
+    }
+
+    if (!preg_match('/^[a-zA-Z0-9]/', $slug)) {
+      throw new Exception(
+        pht(
+          'The name "%s" is not a valid repository short name. Repository '.
+          'short names must begin with a letter or number.',
+          $slug));
+    }
+
+    if (!preg_match('/[a-zA-Z0-9]\z/', $slug)) {
+      throw new Exception(
+        pht(
+          'The name "%s" is not a valid repository short name. Repository '.
+          'short names must end with a letter or number.',
+          $slug));
+    }
+
+    if (preg_match('/__|--|\\.\\./', $slug)) {
+      throw new Exception(
+        pht(
+          'The name "%s" is not a valid repository short name. Repository '.
+          'short names must not contain multiple consecutive underscores, '.
+          'hyphens, or periods.',
+          $slug));
+    }
+
+    if (preg_match('/^[A-Z]+\z/', $slug)) {
+      throw new Exception(
+        pht(
+          'The name "%s" is not a valid repository short name. Repository '.
+          'short names may not contain only uppercase letters.',
+          $slug));
+    }
+
+    if (preg_match('/^\d+\z/', $slug)) {
+      throw new Exception(
+        pht(
+          'The name "%s" is not a valid repository short name. Repository '.
+          'short names may not contain only numbers.',
+          $slug));
+    }
+  }
+
 
 /* -(  Remote Command Execution  )------------------------------------------- */
 

src/applications/repository/storage/__tests__/PhabricatorRepositoryTestCase.php

@@ -152,4 +152,68 @@ final class PhabricatorRepositoryTestCase
     }
   }
 
+  public function testRepositoryShortNameValidation() {
+    $good = array(
+      'sensible-repository',
+      'AReasonableName',
+      'ACRONYM-project',
+      'sol-123',
+      '46-helixes',
+      'node.io',
+      'internet.com',
+      'www.internet-site.com.repository',
+      'with_under-scores',
+
+      // Can't win them all.
+      'A-_._-_._-_._-_._-_._-_._-1',
+
+      // 64-character names are fine.
+      str_repeat('a', 64),
+    );
+
+    $poor = array(
+      '',
+      '1',
+      '.',
+      '-_-',
+      'AAAA',
+      '..',
+      'a/b',
+      '../../etc/passwd',
+      '/',
+      '!',
+      '@',
+      'ca$hmoney',
+      'repo with spaces',
+      'hyphen-',
+      '-ated',
+      '_underscores_',
+      'yes!',
+
+      // 65-character names are no good.
+      str_repeat('a', 65),
+    );
+
+    foreach ($good as $nice_name) {
+      $actual = PhabricatorRepository::isValidRepositorySlug($nice_name);
+      $this->assertEqual(
+        true,
+        $actual,
+        pht(
+          'Expected "%s" to be a valid repository short name.',
+          $nice_name));
+    }
+
+    foreach ($poor as $poor_name) {
+      $actual = PhabricatorRepository::isValidRepositorySlug($poor_name);
+      $this->assertEqual(
+        false,
+        $actual,
+        pht(
+          'Expected "%s" to be rejected as an invalid repository '.
+          'short name.',
+          $poor_name));
+    }
+  }
+
 }