From eb5edbcfde4930a5c4e1802f2a9d88933fbb9ce5 Mon Sep 17 00:00:00 2001 From: "maruel@chromium.org" Date: Mon, 16 Jan 2012 17:03:28 +0000 Subject: [PATCH] Add UpgradeToHttps() to reliably and forcibly upgrade all urls to https. Enable it for git-cl and gcl. R=nsylvain@chromium.org BUG=107838 TEST=New connections go through https:// Review URL: http://codereview.chromium.org/9214004 git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@117857 0039d316-1c4b-4281-b951-d872f2087c98 --- gcl.py | 8 ++---- gclient_utils.py | 31 +++++++++++++++++++- git_cl.py | 49 +++++++++++++++----------------- tests/gcl_unittest.py | 38 +++++++++++++------------ tests/gclient_utils_test.py | 56 +++++++++++++++++++++++++++---------- tests/git_cl_test.py | 6 ++-- 6 files changed, 120 insertions(+), 68 deletions(-) diff --git a/gcl.py b/gcl.py index 3a17fc820..8a0dfc64e 100755 --- a/gcl.py +++ b/gcl.py @@ -294,10 +294,8 @@ class ChangeInfo(object): self.patch = None self._local_root = local_root self.needs_upload = needs_upload - self.rietveld = rietveld_url - if not self.rietveld: - # Set the default value. - self.rietveld = GetCodeReviewSetting('CODE_REVIEW_SERVER') + self.rietveld = gclient_utils.UpgradeToHttps( + rietveld_url or GetCodeReviewSetting('CODE_REVIEW_SERVER')) self._rpc_server = None def _get_description(self): @@ -1039,7 +1037,7 @@ def CMDcommit(change_info, args): if change_info.issue: revision = re.compile(".*?\nCommitted revision (\d+)", re.DOTALL).match(output).group(1) - viewvc_url = GetCodeReviewSetting("VIEW_VC") + viewvc_url = gclient_utils.UpgradeToHttps(GetCodeReviewSetting('VIEW_VC')) change_info.description += '\n' if viewvc_url: change_info.description += "\nCommitted: " + viewvc_url + revision diff --git a/gclient_utils.py b/gclient_utils.py index e5784ac47..35f8b3bf4 100644 --- a/gclient_utils.py +++ b/gclient_utils.py @@ -14,6 +14,7 @@ import sys import tempfile import threading import time +import urlparse import subprocess2 @@ -734,6 +735,30 @@ def RunEditor(content, git): os.remove(filename) +def UpgradeToHttps(url): + """Upgrades random urls to https://. + + Do not touch unknown urls like ssh:// or git://. + Do not touch http:// urls with a port number, + Fixes invalid GAE url. + """ + if not url: + return url + if not re.match(r'[a-z\-]+\://.*', url): + # Make sure it is a valid uri. Otherwise, urlparse() will consider it a + # relative url and will use http:///foo. Note that it defaults to http:// + # for compatibility with naked url like "localhost:8080". + url = 'http://%s' % url + parsed = list(urlparse.urlparse(url)) + # Do not automatically upgrade http to https if a port number is provided. + if parsed[0] == 'http' and not re.match(r'^.+?\:\d+$', parsed[1]): + parsed[0] = 'https' + # Until GAE supports SNI, manually convert the url. + if parsed[1] == 'codereview.chromium.org': + parsed[1] = 'chromiumcodereview.appspot.com' + return urlparse.urlunparse(parsed) + + def ParseCodereviewSettingsContent(content): """Process a codereview.settings file properly.""" lines = (l for l in content.splitlines() if not l.strip().startswith("#")) @@ -742,5 +767,9 @@ def ParseCodereviewSettingsContent(content): except ValueError: raise Error( 'Failed to process settings, please fix. Content:\n\n%s' % content) - # TODO(maruel): Post-process + def fix_url(key): + if keyvals.get(key): + keyvals[key] = UpgradeToHttps(keyvals[key]) + fix_url('CODE_REVIEW_SERVER') + fix_url('VIEW_VC') return keyvals diff --git a/git_cl.py b/git_cl.py index 6af8bcbe5..2922345af 100755 --- a/git_cl.py +++ b/git_cl.py @@ -43,7 +43,7 @@ import subprocess2 import watchlists -DEFAULT_SERVER = 'http://codereview.appspot.com' +DEFAULT_SERVER = 'https://codereview.appspot.com' POSTUPSTREAM_HOOK_PATTERN = '.git/hooks/post-cl-%s' DESCRIPTION_BACKUP_FILE = '~/.git_cl_description_backup' @@ -94,15 +94,6 @@ def ask_for_data(prompt): sys.exit(1) -def FixUrl(server): - """Fix a server url to defaults protocol to http:// if none is specified.""" - if not server: - return server - if not re.match(r'[a-z]+\://.*', server): - return 'http://' + server - return server - - def MatchSvnGlob(url, base_url, glob_spec, allow_wildcards): """Return the corresponding git ref if |base_url| together with |glob_spec| matches the full |url|. @@ -168,15 +159,15 @@ class Settings(object): def GetDefaultServerUrl(self, error_ok=False): if not self.default_server: self.LazyUpdateIfNeeded() - self.default_server = FixUrl(self._GetConfig('rietveld.server', - error_ok=True)) + self.default_server = gclient_utils.UpgradeToHttps( + self._GetConfig('rietveld.server', error_ok=True)) if error_ok: return self.default_server if not self.default_server: error_message = ('Could not find settings file. You must configure ' 'your review setup by running "git cl config".') - self.default_server = FixUrl(self._GetConfig( - 'rietveld.server', error_message=error_message)) + self.default_server = gclient_utils.UpgradeToHttps( + self._GetConfig('rietveld.server', error_message=error_message)) return self.default_server def GetRoot(self): @@ -266,7 +257,8 @@ class Settings(object): def GetViewVCUrl(self): if not self.viewvc_url: - self.viewvc_url = self._GetConfig('rietveld.viewvc-url', error_ok=True) + self.viewvc_url = gclient_utils.UpgradeToHttps( + self._GetConfig('rietveld.viewvc-url', error_ok=True)) return self.viewvc_url def GetDefaultCCList(self): @@ -426,7 +418,7 @@ or verify this branch is set up to track another (via the --track argument to issue = RunGit(['config', self._IssueSetting()], error_ok=True).strip() if issue: self.issue = issue - self.rietveld_server = FixUrl(RunGit( + self.rietveld_server = gclient_utils.UpgradeToHttps(RunGit( ['config', self._RietveldServer()], error_ok=True).strip()) else: self.issue = None @@ -625,23 +617,28 @@ def GetCodereviewSettingsInteractively(): newserver = ask_for_data(prompt + ':') if not server and not newserver: newserver = DEFAULT_SERVER - if newserver and newserver != server: - RunGit(['config', 'rietveld.server', newserver]) + if newserver: + newserver = gclient_utils.UpgradeToHttps(newserver) + if newserver != server: + RunGit(['config', 'rietveld.server', newserver]) - def SetProperty(initial, caption, name): + def SetProperty(initial, caption, name, is_url): prompt = caption if initial: prompt += ' ("x" to clear) [%s]' % initial new_val = ask_for_data(prompt + ':') if new_val == 'x': RunGit(['config', '--unset-all', 'rietveld.' + name], error_ok=True) - elif new_val and new_val != initial: - RunGit(['config', 'rietveld.' + name, new_val]) + elif new_val: + if is_url: + new_val = gclient_utils.UpgradeToHttps(new_val) + if new_val != initial: + RunGit(['config', 'rietveld.' + name, new_val]) - SetProperty(settings.GetDefaultCCList(), 'CC list', 'cc') + SetProperty(settings.GetDefaultCCList(), 'CC list', 'cc', False) SetProperty(settings.GetTreeStatusUrl(error_ok=True), 'Tree status URL', - 'tree-status-url') - SetProperty(settings.GetViewVCUrl(), 'ViewVC URL', 'viewvc-url') + 'tree-status-url', False) + SetProperty(settings.GetViewVCUrl(), 'ViewVC URL', 'viewvc-url', True) # TODO: configure a default branch to diff against, rather than this # svn-based hackery. @@ -1238,8 +1235,8 @@ def CMDpatch(parser, args): issue = issue_arg patch_data = Changelist().GetPatchSetDiff(issue) else: - # Assume it's a URL to the patch. Default to http. - issue_url = FixUrl(issue_arg) + # Assume it's a URL to the patch. Default to https. + issue_url = gclient_utils.UpgradeToHttps(issue_arg) match = re.match(r'.*?/issue(\d+)_\d+.diff', issue_url) if not match: DieWithError('Must pass an issue ID or full URL for ' diff --git a/tests/gcl_unittest.py b/tests/gcl_unittest.py index 3b1a18e7c..4ea69b446 100755 --- a/tests/gcl_unittest.py +++ b/tests/gcl_unittest.py @@ -1,5 +1,5 @@ #!/usr/bin/env python -# Copyright (c) 2011 The Chromium Authors. All rights reserved. +# Copyright (c) 2012 The Chromium Authors. All rights reserved. # Use of this source code is governed by a BSD-style license that can be # found in the LICENSE file. @@ -52,7 +52,7 @@ class GclTestsBase(SuperMoxTestBase): change_info.GetFileNames = lambda : [f[1] for f in change_info.files] change_info.GetLocalRoot = lambda : 'proout' change_info.patch = None - change_info.rietveld = 'my_server' + change_info.rietveld = 'https://my_server' change_info.reviewers = None change_info._closed = False change_info._deleted = False @@ -259,7 +259,7 @@ class ChangeInfoUnittest(GclTestsBase): gcl.GetChangelistInfoFile('').AndReturn('foo') values = { 'description': '', 'patchset': 2, 'issue': 1, - 'files': [], 'needs_upload': False, 'rietveld': 'foo'} + 'files': [], 'needs_upload': False, 'rietveld': 'https://foo'} gcl.gclient_utils.FileWrite( 'foo', gcl.json.dumps(values, sort_keys=True, indent=2)) self.mox.ReplayAll() @@ -272,7 +272,7 @@ class ChangeInfoUnittest(GclTestsBase): gcl.GetChangelistInfoFile('n').AndReturn('foo') values = { 'description': 'des', 'patchset': 0, 'issue': 0, - 'files': [], 'needs_upload': True, 'rietveld': 'foo'} + 'files': [], 'needs_upload': True, 'rietveld': 'https://foo'} gcl.gclient_utils.FileWrite( 'foo', gcl.json.dumps(values, sort_keys=True, indent=2)) self.mox.ReplayAll() @@ -302,7 +302,7 @@ class CMDuploadUnittest(GclTestsBase): change_info.description = 'deescription\n\nR=foo@bar.com', change_info.files = [('A', 'aa'), ('M', 'bb')] change_info.patch = None - change_info.rietveld = 'my_server' + change_info.rietveld = 'https://my_server' files = [item[1] for item in change_info.files] output = presubmit_support.PresubmitOutput() gcl.DoPresubmitChecks(change_info, False, True).AndReturn(output) @@ -312,7 +312,7 @@ class CMDuploadUnittest(GclTestsBase): gcl.os.chdir('proout') change_info.GetFileNames().AndReturn(files) gcl.GenerateDiff(files) - gcl.upload.RealMain(['upload.py', '-y', '--server=my_server', + gcl.upload.RealMain(['upload.py', '-y', '--server=https://my_server', '-r', 'georges@example.com', '--message=\'\'', '--issue=1'], change_info.patch).AndReturn(("1", @@ -356,9 +356,10 @@ class CMDuploadUnittest(GclTestsBase): gcl.os.getcwd().AndReturn('somewhere') gcl.os.chdir(change_info.GetLocalRoot()) gcl.GenerateDiff(change_info.GetFileNames()) - gcl.upload.RealMain(['upload.py', '-y', '--server=my_server', '--server=a', - "--description_file=descfile", - "--message=deescription"], change_info.patch).AndReturn(("1", "2")) + gcl.upload.RealMain( + [ 'upload.py', '-y', '--server=https://my_server', '--server=a', + '--description_file=descfile', '--message=deescription'], + change_info.patch).AndReturn(("1", "2")) gcl.os.remove('descfile') change_info.SendToRietveld("/lint/issue%s_%s" % ('1', '2'), timeout=1) gcl.os.chdir('somewhere') @@ -397,7 +398,7 @@ class CMDuploadUnittest(GclTestsBase): gcl.os.getcwd().AndReturn('somewhere') gcl.os.chdir(change_info.GetLocalRoot()) gcl.GenerateDiff(change_info.GetFileNames()) - gcl.upload.RealMain(['upload.py', '-y', '--server=my_server', + gcl.upload.RealMain(['upload.py', '-y', '--server=https://my_server', "--description_file=descfile", "--message=deescription"], change_info.patch).AndReturn(("1", "2")) gcl.os.remove('descfile') @@ -442,7 +443,7 @@ class CMDuploadUnittest(GclTestsBase): change_info.description = 'deescription\n\nR=georges@example.com', change_info.files = [('A', 'aa'), ('M', 'bb')] change_info.patch = None - change_info.rietveld = 'my_server' + change_info.rietveld = 'https://my_server' change_info.reviewers = ['georges@example.com'] files = [item[1] for item in change_info.files] output = presubmit_support.PresubmitOutput() @@ -454,7 +455,7 @@ class CMDuploadUnittest(GclTestsBase): change_info.GetLocalRoot().AndReturn('proout') gcl.os.chdir('proout') gcl.GenerateDiff(files) - gcl.upload.RealMain(['upload.py', '-y', '--server=my_server', + gcl.upload.RealMain(['upload.py', '-y', '--server=https://my_server', '--reviewers=georges@example.com', '--message=\'\'', '--issue=1'], change_info.patch).AndReturn(("1", "2")) @@ -483,7 +484,7 @@ class CMDuploadUnittest(GclTestsBase): gcl.os.getcwd().AndReturn('somewhere') gcl.os.chdir('proout') gcl.GenerateDiff(change_info.GetFileNames()) - gcl.upload.RealMain(['upload.py', '-y', '--server=my_server', + gcl.upload.RealMain(['upload.py', '-y', '--server=https://my_server', '--reviewers=foo@example.com,bar@example.com', '--message=\'\'', '--issue=1'], change_info.patch).AndReturn(("1", "2")) @@ -558,8 +559,8 @@ class CMDCommitUnittest(GclTestsBase): def testPresubmitSucceeds(self): change_info = self.mockLoad() self.mockPresubmit(change_info, fail=False) - self.mockCommit(change_info, 'deescription\nReview URL: http://my_server/1', - '') + self.mockCommit( + change_info, 'deescription\nReview URL: https://my_server/1', '') self.mox.ReplayAll() retval = gcl.CMDcommit(['naame']) @@ -573,15 +574,16 @@ class CMDCommitUnittest(GclTestsBase): def testPresubmitSucceedsWithCommittedMessage(self): change_info = self.mockLoad() self.mockPresubmit(change_info, fail=False) - self.mockCommit(change_info, 'deescription\nReview URL: http://my_server/1', - '\nCommitted revision 12345') + self.mockCommit( + change_info, 'deescription\nReview URL: https://my_server/1', + '\nCommitted revision 12345') self.mox.ReplayAll() retval = gcl.CMDcommit(['naame']) self.assertEquals(retval, 0) self.assertEquals(change_info.description, - 'deescription\n\nCommitted: http://view/12345') + 'deescription\n\nCommitted: https://view/12345') # pylint: disable=W0212 self.assertTrue(change_info._deleted) self.assertTrue(change_info._closed) diff --git a/tests/gclient_utils_test.py b/tests/gclient_utils_test.py index 937e43a7c..9f01c5d1e 100755 --- a/tests/gclient_utils_test.py +++ b/tests/gclient_utils_test.py @@ -35,10 +35,11 @@ class GclientUtilsUnittest(GclientUtilBase): 'MakeDateRevision', 'MakeFileAutoFlush', 'MakeFileAnnotated', 'PathDifference', 'ParseCodereviewSettingsContent', 'PrintableObject', 'RemoveDirectory', 'RunEditor', - 'SplitUrlRevision', 'SyntaxErrorToError', 'Wrapper', 'WorkItem', + 'SplitUrlRevision', 'SyntaxErrorToError', + 'UpgradeToHttps', 'Wrapper', 'WorkItem', 'errno', 'lockedmethod', 'logging', 'os', 'Queue', 're', 'rmtree', 'safe_makedirs', 'stat', 'subprocess2', 'sys', 'tempfile', 'threading', - 'time', + 'time', 'urlparse', ] # If this test fails, you should add the relevant test. self.compareMembers(gclient_utils, members) @@ -171,20 +172,45 @@ class GClientUtilsTest(trial_dir.TestCase): os.chmod(l2, 0) os.chmod(l1, 0) + def testUpgradeToHttps(self): + values = [ + ['', ''], + [None, None], + ['foo', 'https://foo'], + ['http://foo', 'https://foo'], + ['foo/', 'https://foo/'], + ['ssh-svn://foo', 'ssh-svn://foo'], + ['ssh-svn://foo/bar/', 'ssh-svn://foo/bar/'], + ['codereview.chromium.org', 'https://chromiumcodereview.appspot.com'], + ['codereview.chromium.org/', 'https://chromiumcodereview.appspot.com/'], + ['http://foo:8080', 'http://foo:8080'], + ['http://foo:8080/bar', 'http://foo:8080/bar'], + ['foo:8080', 'http://foo:8080'], + ['foo:', 'https://foo:'], + ] + for content, expected in values: + self.assertEquals( + expected, gclient_utils.UpgradeToHttps(content)) + def testParseCodereviewSettingsContent(self): - expected = { - 'Foo': 'bar:baz', - 'Second': 'value', - } - content = ( - '# bleh\n' - '\t# foo : bar\n' - 'Foo:bar:baz\n' - ' Second : value \n\r' - '#inconsistency' - ) - self.assertEquals( - expected, gclient_utils.ParseCodereviewSettingsContent(content)) + values = [ + ['# bleh\n', {}], + ['\t# foo : bar\n', {}], + ['Foo:bar', {'Foo': 'bar'}], + ['Foo:bar:baz\n', {'Foo': 'bar:baz'}], + [' Foo : bar ', {'Foo': 'bar'}], + [' Foo : bar \n', {'Foo': 'bar'}], + ['a:b\n\rc:d\re:f', {'a': 'b', 'c': 'd', 'e': 'f'}], + ['an_url:http://value/', {'an_url': 'http://value/'}], + [ + 'CODE_REVIEW_SERVER : http://r/s', + {'CODE_REVIEW_SERVER': 'https://r/s'} + ], + ['VIEW_VC:http://r/s', {'VIEW_VC': 'https://r/s'}], + ] + for content, expected in values: + self.assertEquals( + expected, gclient_utils.ParseCodereviewSettingsContent(content)) if __name__ == '__main__': diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index b2f3e1965..7d3264860 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -1,5 +1,5 @@ #!/usr/bin/env python -# Copyright (c) 2011 The Chromium Authors. All rights reserved. +# Copyright (c) 2012 The Chromium Authors. All rights reserved. # Use of this source code is governed by a BSD-style license that can be # found in the LICENSE file. @@ -105,7 +105,7 @@ class TestGitCl(TestCase): (['git', 'svn', 'info'], ''), (['git', 'config', 'branch.master.rietveldissue', '1'], ''), (['git', 'config', 'branch.master.rietveldserver', - 'http://codereview.example.com'], ''), + 'https://codereview.example.com'], ''), (['git', 'config', 'branch.master.rietveldpatchset', '2'], ''), ] @@ -115,7 +115,7 @@ class TestGitCl(TestCase): msg = description.split('\n', 1)[0] return [ 'upload', '--assume_yes', '--server', - 'http://codereview.example.com', + 'https://codereview.example.com', '--message', msg, '--description', description ] + args + [