Code review comment for lp:~dpb/landscape-charm/strip-url-newline-1411353

Thanks, I've addressed all feedback.

On Thu, Jan 15, 2015 at 1:12 PM, Andreas Hasenack <email address hidden>
wrote:

> Review: Approve
>
> +1, just a minor comment about the test.
>
>
> Diff comments:
>
> > === modified file 'hooks/hooks.py'
> > --- hooks/hooks.py 2015-01-12 10:40:19 +0000
> > +++ hooks/hooks.py 2015-01-15 19:11:45 +0000
> > @@ -501,11 +501,13 @@
> > notify_website_relation()
> >
> >
> > -def _download_file(url):
> > +def _download_file(url, Curl=pycurl.Curl):
> > """ Download from a url and save to the filename given """
> > + # Fix for CVE-2014-8150, urls cannot end with newline
> > + url = url.rstrip()
> > buf = cStringIO.StringIO()
> > juju.juju_log("Fetching License: %s" % url)
> > - curl = pycurl.Curl()
> > + curl = Curl()
> > curl.setopt(pycurl.URL, str(url))
> > curl.setopt(pycurl.WRITEFUNCTION, buf.write)
> > curl.perform()
> >
> > === modified file 'hooks/test_hooks.py'
> > --- hooks/test_hooks.py 2014-12-10 22:08:57 +0000
> > +++ hooks/test_hooks.py 2015-01-15 19:11:45 +0000
> > @@ -12,6 +12,23 @@
> > import yaml
> >
> >
> > +class CurlStub(object):
> > +
> > + def __init__(self, result=None, infos=None, error=None):
> > + pass
> > +
> > + def setopt(self, option, value):
> > + if option == pycurl.URL:
> > + if "\n" in value:
>
> The curl patch (http://curl.haxx.se/CVE-2014-8150.patch) blocks both \n
> and \r, can we do the same in the test just to be consistent?
>
> > + raise AssertionError("URL cannot contain newline")
> > +
> > + def perform(self):
> > + pass
> > +
> > + def close(self):
> > + pass
> > +
> > +
> > class TestJuju(object):
> > """
> > Testing object to intercept juju calls and inject data, or make sure
> > @@ -908,13 +925,19 @@
> > filename = self.makeFile()
> > with open(filename, "w") as fp:
> > fp.write("foobar")
> > - output = hooks._download_file("file://%s" % filename)
> > + output = hooks._download_file("file:///%s" % filename)
> > self.assertIn("foobar", output)
> >
> > + def test__download_file_success_trailing_newline(self):
> > + """Test that newlines are stripped before passing to curl.
> CVE-2014-8150."""
> > + # put two newlines, since that could be a common pattern in a
> text
> > + # file when using an editor
> > + hooks._download_file("http://example.com/\n\n", Curl=CurlStub)
> > +
> > def test__download_file_failure(self):
> > """The fail path of download file raises an exception."""
> > self.assertRaises(
> > - pycurl.error, hooks._download_file, "file://FOO/NO/EXIST")
> > + pycurl.error, hooks._download_file, "file:///FOO/NO/EXIST")
> >
> > def test__replace_in_file(self):
> > """
> > @@ -962,7 +985,7 @@
> > source = self.makeFile()
> > with open(source, "w") as fp:
> > fp.write("LICENSE_FILE_TEXT from curl")
> > - hooks.juju.config["license-file"] = "file://%s" % source
> > + hooks.juju.config["license-file"] = "file:///%s" % source
> > hooks._install_license()
> > self.assertFileContains(
> > hooks.LANDSCAPE_LICENSE_DEST, "LICENSE_FILE_TEXT from curl")
> >
>
>
> --
>
> https://code.launchpad.net/~davidpbritton/landscape-charm/strip-url-newline-1411353/+merge/246628
> You are the owner of
> lp:~davidpbritton/landscape-charm/strip-url-newline-1411353.
>

--
David Britton <email address hidden>