launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> === modified file 'lib/lp/code/browser/branch.py'
> --- lib/lp/code/browser/branch.py	2018-03-16 21:20:00 +0000
> +++ lib/lp/code/browser/branch.py	2018-06-14 17:02:33 +0000
> @@ -1280,3 +1297,28 @@
>                      'target_branch',
>                      "This branch is not mergeable into %s." %
>                      target_branch.bzr_identity)
> +
> +
> +@implementer(IBrowserPublisher)
> +class BranchDiffView:
> +
> +    def __init__(self, context, request, new, old=None):
> +        self.context = context
> +        self.request = request
> +        self.new = new
> +        self.old = old
> +
> +    def __call__(self):
> +        content = self.context.getDiff(self.new, old=self.old)

This has the potential to bring down the webapp; Loggerhead isn't very fast on large branches. We should ensure there's a sensible timeout on the external request and probably add a feature flag.

> +        if self.old is None:
> +            filename = "%s.diff" % self.new
> +        else:
> +            filename = "%s_%s.diff" % (self.old, self.new)

Could maybe include a filename-friendly version of the branch URL here, but not critical.

> +        self.request.response.setHeader("Content-Type", "text/x-patch")
> +        self.request.response.setHeader("Content-Length", str(len(content)))
> +        self.request.response.setHeader(
> +            "Content-Disposition", "attachment; filename=%s" % filename)

This sounds like it could be header injection, given old/new aren't totally validated.

> +        return content
> +
> +    def browserDefault(self, request):
> +        return self, ()
> 
> === modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
> --- lib/lp/code/browser/tests/test_branchmergeproposal.py	2018-04-25 12:01:33 +0000
> +++ lib/lp/code/browser/tests/test_branchmergeproposal.py	2018-06-14 17:02:33 +0000
> @@ -1635,14 +1635,14 @@
>  
>      def test_client_cache_bzr(self):
>          # For Bazaar, the client cache contains the branch name and a
> -        # loggerhead-based diff link.
> +        # webapp-based diff link.

"link to the Loggerhead diff via the webapp"?

>          bmp = self.factory.makeBranchMergeProposal()
>          view = create_initialized_view(bmp, '+index')
>          cache = IJSONRequestCache(view.request)
>          self.assertThat(cache.objects, ContainsDict({
>              'branch_name': Equals(bmp.source_branch.name),
>              'branch_diff_link': Equals(
> -                'https://code.launchpad.dev/+loggerhead/%s/diff/' %
> +                'http://code.launchpad.dev/%s/+diff/' %
>                  bmp.source_branch.unique_name),
>              }))
>  


-- 
https://code.launchpad.net/~cjwatson/launchpad/better-bzr-mp-diffs/+merge/345705
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.