From 0b3d10c3da916e41084bf6e8f7a3762fe242f0b5 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 10 Jan 2016 10:55:22 -0800 Subject: [PATCH] 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 --- .../sql/autopatches/20160110.repo.01.slug.sql | 5 ++ .../sql/autopatches/20160110.repo.02.slug.php | 49 ++++++++++ ...DiffusionRepositoryEditBasicController.php | 21 +++-- .../DiffusionRepositoryEditMainController.php | 2 +- .../editor/PhabricatorRepositoryEditor.php | 71 ++++++++++++++- .../query/PhabricatorRepositoryQuery.php | 13 +++ .../storage/PhabricatorRepository.php | 89 +++++++++++++++++-- .../PhabricatorRepositoryTestCase.php | 64 +++++++++++++ 8 files changed, 299 insertions(+), 15 deletions(-) create mode 100644 resources/sql/autopatches/20160110.repo.01.slug.sql create mode 100644 resources/sql/autopatches/20160110.repo.02.slug.php diff --git a/resources/sql/autopatches/20160110.repo.01.slug.sql b/resources/sql/autopatches/20160110.repo.01.slug.sql new file mode 100644 index 0000000000..af755ed78e --- /dev/null +++ b/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); diff --git a/resources/sql/autopatches/20160110.repo.02.slug.php b/resources/sql/autopatches/20160110.repo.02.slug.php new file mode 100644 index 0000000000..8655311df9 --- /dev/null +++ b/resources/sql/autopatches/20160110.repo.02.slug.php @@ -0,0 +1,49 @@ +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)); + } + +} diff --git a/src/applications/diffusion/controller/DiffusionRepositoryEditBasicController.php b/src/applications/diffusion/controller/DiffusionRepositoryEditBasicController.php index 5b4429b61a..1e4de51cff 100644 --- a/src/applications/diffusion/controller/DiffusionRepositoryEditBasicController.php +++ b/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); - return id(new AphrontRedirectResponse())->setURI($edit_uri); + 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); diff --git a/src/applications/diffusion/controller/DiffusionRepositoryEditMainController.php b/src/applications/diffusion/controller/DiffusionRepositoryEditMainController.php index bbf7152bf3..4a48f02bf9 100644 --- a/src/applications/diffusion/controller/DiffusionRepositoryEditMainController.php +++ b/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( diff --git a/src/applications/repository/editor/PhabricatorRepositoryEditor.php b/src/applications/repository/editor/PhabricatorRepositoryEditor.php index af4226a4f6..9e04a5552f 100644 --- a/src/applications/repository/editor/PhabricatorRepositoryEditor.php +++ b/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); + } + } diff --git a/src/applications/repository/query/PhabricatorRepositoryQuery.php b/src/applications/repository/query/PhabricatorRepositoryQuery.php index bd9765130a..d87fdaa9ce 100644 --- a/src/applications/repository/query/PhabricatorRepositoryQuery.php +++ b/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; } diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php index e34c4980c0..ee0e15c879 100644 --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/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 )------------------------------------------- */ diff --git a/src/applications/repository/storage/__tests__/PhabricatorRepositoryTestCase.php b/src/applications/repository/storage/__tests__/PhabricatorRepositoryTestCase.php index 7e71efe755..84379feb49 100644 --- a/src/applications/repository/storage/__tests__/PhabricatorRepositoryTestCase.php +++ b/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)); + } + } + }