launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #23716
[Merge] lp:~cjwatson/launchpad/requests-2.22.0 into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/requests-2.22.0 into lp:launchpad.
Commit message:
Upgrade to requests 2.22.0 and requests-toolbelt 0.9.1.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/requests-2.22.0/+merge/368580
The urllib3 upgrade fixes a header injection vulnerability; I understand that it isn't exploitable via requests, but better safe than sorry.
There's some test fallout due to minor differences in URL encoding and exception arguments and such, and we can simplify CleanablePoolManager because urllib3 now gives subclasses a better way to customise the mapping of schemes to pool classes.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/requests-2.22.0 into lp:launchpad.
=== modified file 'constraints.txt'
--- constraints.txt 2019-05-22 14:57:45 +0000
+++ constraints.txt 2019-06-07 22:10:19 +0000
@@ -236,6 +236,7 @@
bson==0.3.3
bzr==2.6.0.lp.3
celery==4.1.1
+certifi==2019.3.9
cffi==1.11.2
Chameleon==2.11
chardet==3.0.4
@@ -340,9 +341,9 @@
python-swiftclient==2.0.3
PyYAML==3.10
rabbitfixture==0.4.1
-requests==2.7.0
+requests==2.22.0
requests-file==1.4.3
-requests-toolbelt==0.6.2
+requests-toolbelt==0.9.1
responses==0.9.0
scandir==1.7
service-identity==17.0.0
@@ -370,6 +371,7 @@
txpkgupload==0.2
typing==3.6.2
unittest2==1.1.0
+urllib3==1.25.3
van.testing==3.0.0
vine==1.1.4
virtualenv-tools3==2.0.0
=== modified file 'lib/lp/bugs/doc/externalbugtracker-trac-lp-plugin.txt'
--- lib/lp/bugs/doc/externalbugtracker-trac-lp-plugin.txt 2019-05-22 14:57:45 +0000
+++ lib/lp/bugs/doc/externalbugtracker-trac-lp-plugin.txt 2019-06-07 22:10:19 +0000
@@ -135,7 +135,8 @@
Traceback (most recent call last):
...
BugTrackerAuthenticationError: http://example.com:
- 401 Client Error: Unauthorized
+ 401 Client Error: Unauthorized for url:
+ http://example.com/launchpad-auth/...
Current time
=== modified file 'lib/lp/bugs/doc/sourceforge-remote-products.txt'
--- lib/lp/bugs/doc/sourceforge-remote-products.txt 2018-06-29 23:10:57 +0000
+++ lib/lp/bugs/doc/sourceforge-remote-products.txt 2019-06-07 22:10:19 +0000
@@ -111,6 +111,7 @@
... requests_mock.add('GET', re.compile(r'.*'), status=500)
... finder.getRemoteProductFromSourceForge('fronobulator')
ERROR...Error fetching project...: 500 Server Error: Internal Server Error
+ for url: http://sourceforge.net/projects/fronobulator
SourceForgeRemoteProductFinder.setRemoteProductsFromSourceForge()
iterates over the list of products returned by
=== modified file 'lib/lp/bugs/externalbugtracker/tests/test_gitlab.py'
--- lib/lp/bugs/externalbugtracker/tests/test_gitlab.py 2019-03-07 15:32:48 +0000
+++ lib/lp/bugs/externalbugtracker/tests/test_gitlab.py 2019-06-07 22:10:19 +0000
@@ -131,7 +131,7 @@
tracker.getRemoteBugBatch(["1", "2"]))
self.assertEqual(
"https://gitlab.com/api/v4/projects/user%2Frepository/issues?";
- "iids[]=1&iids[]=2",
+ "iids%5B%5D=1&iids%5B%5D=2",
responses.calls[-1].request.url)
@responses.activate
@@ -144,7 +144,7 @@
tracker.getRemoteBugBatch(["1", "2"], last_accessed=since))
self.assertEqual(
"https://gitlab.com/api/v4/projects/user%2Frepository/issues?";
- "updated_after=2015-01-01T12%3A00%3A00Z&iids[]=1&iids[]=2",
+ "updated_after=2015-01-01T12%3A00%3A00Z&iids%5B%5D=1&iids%5B%5D=2",
responses.calls[-1].request.url)
@responses.activate
@@ -189,7 +189,8 @@
[str(bug["iid"]) for bug in self.sample_bugs]))
expected_urls = [
"https://gitlab.com/api/v4/projects/user%2Frepository/issues?"; +
- "&".join("iids[]=%s" % bug["iid"] for bug in self.sample_bugs),
+ "&".join(
+ "iids%%5B%%5D=%s" % bug["iid"] for bug in self.sample_bugs),
"https://gitlab.com/api/v4/projects/user%2Frepository/issues?";
"page=2",
"https://gitlab.com/api/v4/projects/user%2Frepository/issues?";
@@ -251,7 +252,7 @@
responses.add(
"GET",
"https://gitlab.com/api/v4/projects/user%2Frepository/issues?";
- "iids[]=1234",
+ "iids%5B%5D=1234",
json=remote_bug, match_querystring=True)
bug = self.factory.makeBug()
bug_tracker = self.factory.makeBugTracker(
=== modified file 'lib/lp/code/model/tests/test_branchhosting.py'
--- lib/lp/code/model/tests/test_branchhosting.py 2018-06-21 14:56:36 +0000
+++ lib/lp/code/model/tests/test_branchhosting.py 2019-06-07 22:10:19 +0000
@@ -1,4 +1,4 @@
-# Copyright 2018 Canonical Ltd. This software is licensed under the
+# Copyright 2018-2019 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Unit tests for `BranchHostingClient`.
@@ -16,6 +16,7 @@
from lazr.restful.utils import get_current_browser_request
import responses
+from six.moves.urllib.parse import urljoin
from testtools.matchers import MatchesStructure
from zope.component import getUtility
from zope.interface import implementer
@@ -26,6 +27,7 @@
BranchHostingFault,
)
from lp.code.interfaces.branchhosting import IBranchHostingClient
+from lp.services.config import config
from lp.services.job.interfaces.job import (
IRunnableJob,
JobStatus,
@@ -78,6 +80,9 @@
self.assertEqual(
"/" + url_suffix.split("?", 1)[0], action.detail.split(" ", 1)[0])
+ def makeHostingURL(self, path):
+ return urljoin(config.codehosting.internal_bzr_api_endpoint, path)
+
def test_getDiff(self):
with self.mockRequests("GET", body="---\n+++\n"):
diff = self.client.getDiff(123, "2", "1")
@@ -107,7 +112,8 @@
self.assertRaisesWithContent(
BranchHostingFault,
"Failed to get diff from Bazaar branch: "
- "400 Client Error: Bad Request",
+ "400 Client Error: Bad Request for url: " +
+ self.makeHostingURL("/+branch-id/123/diff/2/1"),
self.client.getDiff, 123, "2", "1")
def test_getInventory(self):
@@ -148,7 +154,9 @@
self.assertRaisesWithContent(
BranchHostingFault,
"Failed to get inventory from Bazaar branch: "
- "400 Client Error: Bad Request",
+ "400 Client Error: Bad Request for url: " +
+ self.makeHostingURL(
+ "/+branch-id/123/+json/files/head%3A/dir/path/file/name"),
self.client.getInventory, 123, "dir/path/file/name")
def test_getInventory_url_quoting(self):
@@ -194,7 +202,9 @@
self.assertRaisesWithContent(
BranchHostingFault,
"Failed to get file from Bazaar branch: "
- "400 Client Error: Bad Request",
+ "400 Client Error: Bad Request for url: " +
+ self.makeHostingURL(
+ "/+branch-id/123/download/head%3A/file-id"),
self.client.getBlob, 123, "file-id")
def test_getBlob_url_quoting(self):
=== modified file 'lib/lp/code/model/tests/test_githosting.py'
--- lib/lp/code/model/tests/test_githosting.py 2018-11-22 16:35:28 +0000
+++ lib/lp/code/model/tests/test_githosting.py 2019-06-07 22:10:19 +0000
@@ -2,7 +2,7 @@
# NOTE: The first line above must stay first; do not move the copyright
# notice to the top. See http://www.python.org/dev/peps/pep-0263/.
#
-# Copyright 2016-2018 Canonical Ltd. This software is licensed under the
+# Copyright 2016-2019 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Unit tests for `GitHostingClient`.
@@ -21,7 +21,17 @@
from lazr.restful.utils import get_current_browser_request
import responses
-from testtools.matchers import MatchesStructure
+from six.moves.urllib.parse import (
+ parse_qsl,
+ urljoin,
+ urlsplit,
+ )
+from testtools.matchers import (
+ AfterPreprocessing,
+ Equals,
+ MatchesListwise,
+ MatchesStructure,
+ )
from zope.component import getUtility
from zope.interface import implementer
from zope.security.proxy import removeSecurityProxy
@@ -33,6 +43,7 @@
GitRepositoryScanFault,
)
from lp.code.interfaces.githosting import IGitHostingClient
+from lp.services.config import config
from lp.services.job.interfaces.job import (
IRunnableJob,
JobStatus,
@@ -52,6 +63,24 @@
from lp.testing.layers import ZopelessDatabaseLayer
+class MatchesURL(AfterPreprocessing):
+ """Matches a URL, disregarding the order of query string parameters."""
+
+ def __init__(self, url):
+ split_url = urlsplit(url)
+ query_matcher = AfterPreprocessing(
+ lambda qs: sorted(parse_qsl(qs)),
+ MatchesListwise(
+ [Equals(pair) for pair in sorted(parse_qsl(split_url.query))]))
+ super(MatchesURL, self).__init__(
+ urlsplit, MatchesStructure(
+ scheme=Equals(split_url.scheme),
+ netloc=Equals(split_url.netloc),
+ path=Equals(split_url.path),
+ query=query_matcher,
+ fragment=Equals(split_url.fragment)))
+
+
class TestGitHostingClient(TestCase):
layer = ZopelessDatabaseLayer
@@ -77,8 +106,10 @@
def assertRequest(self, url_suffix, json_data=None, method=None, **kwargs):
[request] = self.requests
- self.assertThat(request, MatchesStructure.byEquality(
- url=urlappend(self.endpoint, url_suffix), method=method, **kwargs))
+ self.assertThat(request, MatchesStructure(
+ url=MatchesURL(urlappend(self.endpoint, url_suffix)),
+ method=Equals(method),
+ **{key: Equals(value) for key, value in kwargs.items()}))
if json_data is not None:
self.assertEqual(json_data, json.loads(request.body))
timeline = get_request_timeline(get_current_browser_request())
@@ -87,6 +118,9 @@
self.assertEqual(
"/" + url_suffix.split("?", 1)[0], action.detail.split(" ", 1)[0])
+ def makeHostingURL(self, path):
+ return urljoin(config.codehosting.internal_git_api_endpoint, path)
+
def test_create(self):
with self.mockRequests("POST"):
self.client.create("123")
@@ -105,7 +139,8 @@
self.assertRaisesWithContent(
GitRepositoryCreationFault,
"Failed to create Git repository: "
- "400 Client Error: Bad Request",
+ "400 Client Error: Bad Request for url: " +
+ self.makeHostingURL("/repo"),
self.client.create, "123")
def test_getProperties(self):
@@ -120,7 +155,8 @@
self.assertRaisesWithContent(
GitRepositoryScanFault,
"Failed to get properties of Git repository: "
- "400 Client Error: Bad Request",
+ "400 Client Error: Bad Request for url: " +
+ self.makeHostingURL("/repo/123"),
self.client.getProperties, "123")
def test_setProperties(self):
@@ -135,7 +171,8 @@
self.assertRaisesWithContent(
GitRepositoryScanFault,
"Failed to set properties of Git repository: "
- "400 Client Error: Bad Request",
+ "400 Client Error: Bad Request for url: " +
+ self.makeHostingURL("/repo/123"),
self.client.setProperties, "123",
default_branch="refs/heads/a")
@@ -160,7 +197,8 @@
self.assertRaisesWithContent(
GitRepositoryScanFault,
"Failed to get refs from Git repository: "
- "400 Client Error: Bad Request",
+ "400 Client Error: Bad Request for url: " +
+ self.makeHostingURL("/repo/123/refs"),
self.client.getRefs, "123")
def test_getCommits(self):
@@ -175,7 +213,8 @@
self.assertRaisesWithContent(
GitRepositoryScanFault,
"Failed to get commit details from Git repository: "
- "400 Client Error: Bad Request",
+ "400 Client Error: Bad Request for url: " +
+ self.makeHostingURL("/repo/123/commits"),
self.client.getCommits, "123", ["0"])
def test_getLog(self):
@@ -198,7 +237,8 @@
self.assertRaisesWithContent(
GitRepositoryScanFault,
"Failed to get commit log from Git repository: "
- "400 Client Error: Bad Request",
+ "400 Client Error: Bad Request for url: " +
+ self.makeHostingURL("/repo/123/log/refs/heads/master"),
self.client.getLog, "123", "refs/heads/master")
def test_getDiff(self):
@@ -225,7 +265,8 @@
self.assertRaisesWithContent(
GitRepositoryScanFault,
"Failed to get diff from Git repository: "
- "400 Client Error: Bad Request",
+ "400 Client Error: Bad Request for url: " +
+ self.makeHostingURL("/repo/123/compare/a..b"),
self.client.getDiff, "123", "a", "b")
def test_getMergeDiff(self):
@@ -257,7 +298,8 @@
self.assertRaisesWithContent(
GitRepositoryScanFault,
"Failed to get merge diff from Git repository: "
- "400 Client Error: Bad Request",
+ "400 Client Error: Bad Request for url: " +
+ self.makeHostingURL("/repo/123/compare-merge/a:b"),
self.client.getMergeDiff, "123", "a", "b")
def test_detectMerges(self):
@@ -273,7 +315,8 @@
self.assertRaisesWithContent(
GitRepositoryScanFault,
"Failed to detect merges in Git repository: "
- "400 Client Error: Bad Request",
+ "400 Client Error: Bad Request for url: " +
+ self.makeHostingURL("/repo/123/detect-merges/a"),
self.client.detectMerges, "123", "a", ["b", "c"])
def test_delete(self):
@@ -286,7 +329,8 @@
self.assertRaisesWithContent(
GitRepositoryDeletionFault,
"Failed to delete Git repository: "
- "400 Client Error: Bad Request",
+ "400 Client Error: Bad Request for url: " +
+ self.makeHostingURL("/repo/123"),
self.client.delete, "123")
def test_getBlob(self):
@@ -327,7 +371,8 @@
self.assertRaisesWithContent(
GitRepositoryScanFault,
"Failed to get file from Git repository: "
- "400 Client Error: Bad Request",
+ "400 Client Error: Bad Request for url: " +
+ self.makeHostingURL("/repo/123/blob/dir/path/file/name"),
self.client.getBlob, "123", "dir/path/file/name")
def test_getBlob_url_quoting(self):
=== modified file 'lib/lp/services/tests/test_timeout.py'
--- lib/lp/services/tests/test_timeout.py 2019-05-24 11:10:38 +0000
+++ lib/lp/services/tests/test_timeout.py 2019-06-07 22:10:19 +0000
@@ -418,7 +418,7 @@
'ftp://example.com/', use_proxy=True, allow_ftp=True)
self.assertThat(response, MatchesStructure(
status_code=Equals(200),
- headers=ContainsDict({'content-length': Equals('8')}),
+ headers=ContainsDict({'Content-Length': Equals('8')}),
content=Equals('Success.')))
t.join()
=== modified file 'lib/lp/services/timeout.py'
--- lib/lp/services/timeout.py 2019-03-18 11:58:18 +0000
+++ lib/lp/services/timeout.py 2019-06-07 22:10:19 +0000
@@ -34,15 +34,15 @@
DEFAULT_POOLBLOCK,
HTTPAdapter,
)
-from requests.packages.urllib3.connectionpool import (
- HTTPConnectionPool,
- HTTPSConnectionPool,
- )
-from requests.packages.urllib3.exceptions import ClosedPoolError
-from requests.packages.urllib3.poolmanager import PoolManager
from requests_file import FileAdapter
from requests_toolbelt.downloadutils import stream
from six import reraise
+from urllib3.connectionpool import (
+ HTTPConnectionPool,
+ HTTPSConnectionPool,
+ )
+from urllib3.exceptions import ClosedPoolError
+from urllib3.poolmanager import PoolManager
from lp.services.config import config
@@ -295,20 +295,9 @@
class CleanablePoolManager(PoolManager):
"""A version of urllib3's PoolManager supporting forced socket cleanup."""
- # XXX cjwatson 2015-03-11: Reimplements PoolManager._new_pool; check
- # this when upgrading requests.
- def _new_pool(self, scheme, host, port):
- if scheme not in cleanable_pool_classes_by_scheme:
- raise ValueError("Unhandled scheme: %s" % scheme)
- pool_cls = cleanable_pool_classes_by_scheme[scheme]
- kwargs = self.connection_pool_kw
- if scheme == 'http':
- kwargs = self.connection_pool_kw.copy()
- for kw in ('key_file', 'cert_file', 'cert_reqs', 'ca_certs',
- 'ssl_version'):
- kwargs.pop(kw, None)
-
- return pool_cls(host, port, **kwargs)
+ def __init__(self, *args, **kwargs):
+ super(CleanablePoolManager, self).__init__(*args, **kwargs)
+ self.pool_classes_by_scheme = cleanable_pool_classes_by_scheme
class CleanableHTTPAdapter(HTTPAdapter):
=== modified file 'lib/lp/services/webhooks/tests/test_job.py'
--- lib/lp/services/webhooks/tests/test_job.py 2019-05-22 14:57:45 +0000
+++ lib/lp/services/webhooks/tests/test_job.py 2019-06-07 22:10:19 +0000
@@ -175,7 +175,7 @@
'request': self.request_matcher,
'response': MatchesDict({
'status_code': Equals(200),
- 'headers': Equals({'content-type': 'text/plain'}),
+ 'headers': Equals({'Content-Type': 'text/plain'}),
'body': Equals('Content'),
}),
}))
@@ -188,7 +188,7 @@
'request': self.request_matcher,
'response': MatchesDict({
'status_code': Equals(404),
- 'headers': Equals({'content-type': 'text/plain'}),
+ 'headers': Equals({'Content-Type': 'text/plain'}),
'body': Equals('Content'),
}),
}))
=== modified file 'lib/lp/snappy/tests/test_snapstoreclient.py'
--- lib/lp/snappy/tests/test_snapstoreclient.py 2018-05-31 10:23:03 +0000
+++ lib/lp/snappy/tests/test_snapstoreclient.py 2019-06-07 22:10:19 +0000
@@ -1,4 +1,4 @@
-# Copyright 2016-2018 Canonical Ltd. This software is licensed under the
+# Copyright 2016-2019 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Tests for communication with the snap store."""
@@ -341,10 +341,11 @@
@responses.activate
def test_requestPackageUploadPermission_404(self):
snappy_series = self.factory.makeSnappySeries()
- responses.add("POST", "http://sca.example/dev/api/acl/";, status=404)
+ acl_url = "http://sca.example/dev/api/acl/";
+ responses.add("POST", acl_url, status=404)
self.assertRaisesWithContent(
BadRequestPackageUploadResponse,
- b"404 Client Error: Not Found",
+ b"404 Client Error: Not Found for url: " + acl_url.encode("UTF-8"),
self.client.requestPackageUploadPermission,
snappy_series, "test-snap")
@@ -499,13 +500,17 @@
store_secrets = self._make_store_secrets()
snapbuild = self.makeUploadableSnapBuild(store_secrets=store_secrets)
transaction.commit()
+ unscanned_upload_url = "http://updown.example/unscanned-upload/";
responses.add(
- "POST", "http://updown.example/unscanned-upload/";, status=502,
+ "POST", unscanned_upload_url, status=502,
body="The proxy exploded.\n")
with dbuser(config.ISnapStoreUploadJobSource.dbuser):
err = self.assertRaises(
UploadFailedResponse, self.client.upload, snapbuild)
- self.assertEqual("502 Server Error: Bad Gateway", str(err))
+ self.assertEqual(
+ "502 Server Error: Bad Gateway for url: " +
+ unscanned_upload_url,
+ str(err))
self.assertEqual(b"The proxy exploded.\n", err.detail)
self.assertTrue(err.can_retry)
@@ -592,7 +597,9 @@
status_url = "http://sca.example/dev/api/snaps/1/builds/1/status";
responses.add("GET", status_url, status=404)
self.assertRaisesWithContent(
- BadScanStatusResponse, b"404 Client Error: Not Found",
+ BadScanStatusResponse,
+ b"404 Client Error: Not Found for url: " +
+ status_url.encode("UTF-8"),
self.client.checkStatus, status_url)
@responses.activate
@@ -613,10 +620,12 @@
@responses.activate
def test_listChannels_404(self):
- responses.add(
- "GET", "http://search.example/api/v1/channels";, status=404)
+ channels_url = "http://search.example/api/v1/channels";
+ responses.add("GET", channels_url, status=404)
self.assertRaisesWithContent(
- BadSearchResponse, b"404 Client Error: Not Found",
+ BadSearchResponse,
+ b"404 Client Error: Not Found for url: " +
+ channels_url.encode("UTF-8"),
self.client.listChannels)
@responses.activate
@@ -725,8 +734,10 @@
store_name="test-snap", store_secrets=self._make_store_secrets(),
store_channels=["stable", "edge"])
snapbuild = self.factory.makeSnapBuild(snap=snap)
- responses.add(
- "POST", "http://sca.example/dev/api/snap-release/";, status=404)
+ release_url = "http://sca.example/dev/api/snap-release/";
+ responses.add("POST", release_url, status=404)
self.assertRaisesWithContent(
- ReleaseFailedResponse, b"404 Client Error: Not Found",
+ ReleaseFailedResponse,
+ b"404 Client Error: Not Found for url: " +
+ release_url.encode("UTF-8"),
self.client.release, snapbuild, 1)
Follow ups
