launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Fixing



Diff comments:

> 
> === modified file 'lib/lp/code/model/githosting.py'
> --- lib/lp/code/model/githosting.py	2016-04-29 22:06:11 +0000
> +++ lib/lp/code/model/githosting.py	2016-05-03 15:27:15 +0000
> @@ -155,7 +155,8 @@
>                  logger.info(
>                      "Fetching file %s from repository %s" % (filename, path))
>              url = "/repo/%s/blob/%s" % (path, quote(filename))
> -            return self._get(url, params={"rev": rev})
> +            response = self._get(url, params={"rev": rev})
> +            return response["data"].decode("base64")

I'd suggest checking response["size"] against the length of the decoded data, and raising a GitRepositoryScanFault if they don't match.  That would be a cheap integrity check.

>          except Exception as e:
>              raise GitRepositoryScanFault(
>                  "Failed to get file from Git repository: %s" % unicode(e))
> 
> === modified file 'lib/lp/code/model/tests/test_githosting.py'
> --- lib/lp/code/model/tests/test_githosting.py	2016-04-29 22:06:11 +0000
> +++ lib/lp/code/model/tests/test_githosting.py	2016-05-03 15:27:15 +0000
> @@ -185,20 +185,22 @@
>                  self.client.delete, "123")
>  
>      def test_getBlob(self):
> -        blob = b''.join(chr(i) for i in range(256)).encode("base64")
> -        content = {"data": blob, "size": len(blob)}
> +        blob = b''.join(chr(i) for i in range(256))
> +        encoded_blob = blob.encode("base64")
> +        content = {"data": encoded_blob, "size": len(encoded_blob)}

The size element should be the length of the raw blob, not of its base64 encoding.  (Same below.)  This will of course start mattering if you check it in GitHostingClient.

>          with self.mockRequests(content=json.dumps(content)):
>              response = self.client.getBlob("123", "dir/path/file/name")
> -        self.assertEqual(content, response)
> +        self.assertEqual(blob, response)
>          self.assertRequest(
>              "repo/123/blob/dir/path/file/name", method="GET")
>  
>      def test_getBlob_revision(self):
> -        blob = b''.join(chr(i) for i in range(256)).encode("base64")
> -        content = {"data": blob, "size": len(blob)}
> +        blob = b''.join(chr(i) for i in range(256))
> +        encoded_blob = blob.encode("base64")
> +        content = {"data": encoded_blob, "size": len(encoded_blob)}
>          with self.mockRequests(content=json.dumps(content)):
>              response = self.client.getBlob("123", "dir/path/file/name", "dev")
> -        self.assertEqual(content, response)
> +        self.assertEqual(blob, response)
>          self.assertRequest(
>              "repo/123/blob/dir/path/file/name?rev=dev", method="GET")
>  


-- 
https://code.launchpad.net/~maxiberta/launchpad/git-get-blob-unexport-api/+merge/293637
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.