From f48f2dae9f39bccd61ea4868c6d5d95dc0ec68e0 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 31 Jul 2017 12:20:58 -0700 Subject: [PATCH] Move Phabricator to use PhutilBinaryAnalyzer and show binary versions Summary: Fixes T12942. - Adds binary version and path information to {nav Config > Version Information}. - Replaces old code all over the place with new consolidated code. Test Plan: {F5073531} Also faked some cases of missing binaries, bad versions, etc. Reviewers: chad Reviewed By: chad Maniphest Tasks: T12942 Differential Revision: https://secure.phabricator.com/D18306 --- src/__phutil_library_map__.php | 4 -- .../check/PhabricatorBinariesSetupCheck.php | 9 ++--- .../PhabricatorConfigVersionController.php | 23 +++++++++++ .../DiffusionLowLevelMercurialPathsQuery.php | 9 +++-- ...fusionLowLevelMercurialPathsQueryTests.php | 31 -------------- .../PhabricatorRepositoryVersion.php | 40 ------------------- .../PhabricatorRepositoryPullEngine.php | 6 +-- 7 files changed, 34 insertions(+), 88 deletions(-) delete mode 100644 src/applications/diffusion/query/lowlevel/__tests__/DiffusionLowLevelMercurialPathsQueryTests.php delete mode 100644 src/applications/repository/constants/PhabricatorRepositoryVersion.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 4488bacd9a..6427e95432 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -747,7 +747,6 @@ phutil_register_library_map(array( 'DiffusionLowLevelGitRefQuery' => 'applications/diffusion/query/lowlevel/DiffusionLowLevelGitRefQuery.php', 'DiffusionLowLevelMercurialBranchesQuery' => 'applications/diffusion/query/lowlevel/DiffusionLowLevelMercurialBranchesQuery.php', 'DiffusionLowLevelMercurialPathsQuery' => 'applications/diffusion/query/lowlevel/DiffusionLowLevelMercurialPathsQuery.php', - 'DiffusionLowLevelMercurialPathsQueryTests' => 'applications/diffusion/query/lowlevel/__tests__/DiffusionLowLevelMercurialPathsQueryTests.php', 'DiffusionLowLevelParentsQuery' => 'applications/diffusion/query/lowlevel/DiffusionLowLevelParentsQuery.php', 'DiffusionLowLevelQuery' => 'applications/diffusion/query/lowlevel/DiffusionLowLevelQuery.php', 'DiffusionLowLevelResolveRefsQuery' => 'applications/diffusion/query/lowlevel/DiffusionLowLevelResolveRefsQuery.php', @@ -3857,7 +3856,6 @@ phutil_register_library_map(array( 'PhabricatorRepositoryURITransaction' => 'applications/repository/storage/PhabricatorRepositoryURITransaction.php', 'PhabricatorRepositoryURITransactionQuery' => 'applications/repository/query/PhabricatorRepositoryURITransactionQuery.php', 'PhabricatorRepositoryVCSPassword' => 'applications/repository/storage/PhabricatorRepositoryVCSPassword.php', - 'PhabricatorRepositoryVersion' => 'applications/repository/constants/PhabricatorRepositoryVersion.php', 'PhabricatorRepositoryWorkingCopyVersion' => 'applications/repository/storage/PhabricatorRepositoryWorkingCopyVersion.php', 'PhabricatorRequestExceptionHandler' => 'aphront/handler/PhabricatorRequestExceptionHandler.php', 'PhabricatorResourceSite' => 'aphront/site/PhabricatorResourceSite.php', @@ -5741,7 +5739,6 @@ phutil_register_library_map(array( 'DiffusionLowLevelGitRefQuery' => 'DiffusionLowLevelQuery', 'DiffusionLowLevelMercurialBranchesQuery' => 'DiffusionLowLevelQuery', 'DiffusionLowLevelMercurialPathsQuery' => 'DiffusionLowLevelQuery', - 'DiffusionLowLevelMercurialPathsQueryTests' => 'PhabricatorTestCase', 'DiffusionLowLevelParentsQuery' => 'DiffusionLowLevelQuery', 'DiffusionLowLevelQuery' => 'Phobject', 'DiffusionLowLevelResolveRefsQuery' => 'DiffusionLowLevelQuery', @@ -9387,7 +9384,6 @@ phutil_register_library_map(array( 'PhabricatorRepositoryURITransaction' => 'PhabricatorApplicationTransaction', 'PhabricatorRepositoryURITransactionQuery' => 'PhabricatorApplicationTransactionQuery', 'PhabricatorRepositoryVCSPassword' => 'PhabricatorRepositoryDAO', - 'PhabricatorRepositoryVersion' => 'Phobject', 'PhabricatorRepositoryWorkingCopyVersion' => 'PhabricatorRepositoryDAO', 'PhabricatorRequestExceptionHandler' => 'AphrontRequestExceptionHandler', 'PhabricatorResourceSite' => 'PhabricatorSite', diff --git a/src/applications/config/check/PhabricatorBinariesSetupCheck.php b/src/applications/config/check/PhabricatorBinariesSetupCheck.php index 0d577b5297..cf3c87f480 100644 --- a/src/applications/config/check/PhabricatorBinariesSetupCheck.php +++ b/src/applications/config/check/PhabricatorBinariesSetupCheck.php @@ -99,12 +99,12 @@ final class PhabricatorBinariesSetupCheck extends PhabricatorSetupCheck { continue; } - $version = null; + $version = PhutilBinaryAnalyzer::getForBinary($binary) + ->getBinaryVersion(); + switch ($vcs['versionControlSystem']) { case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT: $bad_versions = array(); - list($err, $stdout, $stderr) = exec_manual('git --version'); - $version = trim(substr($stdout, strlen('git version '))); break; case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN: $bad_versions = array( @@ -117,8 +117,6 @@ final class PhabricatorBinariesSetupCheck extends PhabricatorSetupCheck { 'for files added in rN (Subversion issue #2873), fixed in 1.7.2.', 'svn diff -c N'), ); - list($err, $stdout, $stderr) = exec_manual('svn --version --quiet'); - $version = trim($stdout); break; case PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL: $bad_versions = array( @@ -134,7 +132,6 @@ final class PhabricatorBinariesSetupCheck extends PhabricatorSetupCheck { 'in 2.2.1. Pushing fails with this version as well; see %s.', 'T3046#54922'), ); - $version = PhabricatorRepositoryVersion::getMercurialVersion(); break; } diff --git a/src/applications/config/controller/PhabricatorConfigVersionController.php b/src/applications/config/controller/PhabricatorConfigVersionController.php index 8f43192b3b..ca638051e4 100644 --- a/src/applications/config/controller/PhabricatorConfigVersionController.php +++ b/src/applications/config/controller/PhabricatorConfigVersionController.php @@ -64,6 +64,29 @@ final class PhabricatorConfigVersionController $version_from_file); } + $binaries = PhutilBinaryAnalyzer::getAllBinaries(); + foreach ($binaries as $binary) { + if (!$binary->isBinaryAvailable()) { + $binary_info = pht('Not Available'); + } else { + $version = $binary->getBinaryVersion(); + $path = $binary->getBinaryPath(); + if ($path === null && $version === null) { + $binary_info = pht('-'); + } else if ($path === null) { + $binary_info = $version; + } else if ($version === null) { + $binary_info = pht('- at %s', $path); + } else { + $binary_info = pht('%s at %s', $version, $path); + } + } + + $version_property_list->addProperty( + $binary->getBinaryName(), + $binary_info); + } + return $version_property_list; } diff --git a/src/applications/diffusion/query/lowlevel/DiffusionLowLevelMercurialPathsQuery.php b/src/applications/diffusion/query/lowlevel/DiffusionLowLevelMercurialPathsQuery.php index 12b9e661d7..f4e27db670 100644 --- a/src/applications/diffusion/query/lowlevel/DiffusionLowLevelMercurialPathsQuery.php +++ b/src/applications/diffusion/query/lowlevel/DiffusionLowLevelMercurialPathsQuery.php @@ -24,11 +24,12 @@ final class DiffusionLowLevelMercurialPathsQuery $path = $this->path; $commit = $this->commit; - $hg_paths_command = 'locate --print0 --rev %s -I %s'; - $hg_version = PhabricatorRepositoryVersion::getMercurialVersion(); - if (PhabricatorRepositoryVersion::isMercurialFilesCommandAvailable( - $hg_version)) { + $has_files = PhutilBinaryAnalyzer::getForBinary('hg') + ->isMercurialFilesCommandAvailable(); + if ($has_files) { $hg_paths_command = 'files --print0 --rev %s -I %s'; + } else { + $hg_paths_command = 'locate --print0 --rev %s -I %s'; } $match_against = trim($path, '/'); diff --git a/src/applications/diffusion/query/lowlevel/__tests__/DiffusionLowLevelMercurialPathsQueryTests.php b/src/applications/diffusion/query/lowlevel/__tests__/DiffusionLowLevelMercurialPathsQueryTests.php deleted file mode 100644 index 075ca2f1a5..0000000000 --- a/src/applications/diffusion/query/lowlevel/__tests__/DiffusionLowLevelMercurialPathsQueryTests.php +++ /dev/null @@ -1,31 +0,0 @@ - pht('Versions which should not use `files`'), - 'versions' => array('2.6.2', '2.9', '3.1'), - 'match' => false, - ), - - array( - 'name' => pht('Versions which should use `files`'), - 'versions' => array('3.2', '3.3', '3.5.2'), - 'match' => true, - ), - ); - - foreach ($cases as $case) { - foreach ($case['versions'] as $version) { - $actual = PhabricatorRepositoryVersion - ::isMercurialFilesCommandAvailable($version); - $expect = $case['match']; - $this->assertEqual($expect, $actual, $case['name']); - } - } - } - -} diff --git a/src/applications/repository/constants/PhabricatorRepositoryVersion.php b/src/applications/repository/constants/PhabricatorRepositoryVersion.php deleted file mode 100644 index 5f722fa40a..0000000000 --- a/src/applications/repository/constants/PhabricatorRepositoryVersion.php +++ /dev/null @@ -1,40 +0,0 @@ -='); - } - -} diff --git a/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php b/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php index b144ecfd96..32e99e619d 100644 --- a/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php +++ b/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php @@ -485,8 +485,8 @@ final class PhabricatorRepositoryPullEngine // On vulnerable versions of Mercurial, we refuse to clone remotes which // contain characters which may be interpreted by the shell. - $hg_version = PhabricatorRepositoryVersion::getMercurialVersion(); - $is_vulnerable = version_compare($hg_version, '3.2.4', '<'); + $hg_binary = PhutilBinaryAnalyzer::getForBinary('hg'); + $is_vulnerable = $hg_binary->isMercurialVulnerableToInjection(); if ($is_vulnerable) { $cleartext = $remote->openEnvelope(); // The use of "%R" here is an attempt to limit collateral damage @@ -501,7 +501,7 @@ final class PhabricatorRepositoryPullEngine 'command injection security vulnerability. The remote URI for '. 'this repository (%s) is potentially unsafe. Upgrade Mercurial '. 'to at least 3.2.4 to clone it.', - $hg_version, + $hg_binary->getBinaryVersion(), $repository->getMonogram())); } }