launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing code



Diff comments:

> === modified file 'lib/lp/code/browser/diff.py'
> --- lib/lp/code/browser/diff.py	2012-01-01 02:58:52 +0000
> +++ lib/lp/code/browser/diff.py	2015-06-03 10:20:58 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
> +# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>  
>  """Display classes relating to diff objects of one sort or another."""
> @@ -64,9 +64,15 @@
>          diffstat = diff.diffstat
>          if diffstat is not None:
>              file_count = len(diffstat)
> -            file_text = get_plural_text(
> -                file_count, _(' %d file modified'), _(' %d files modified'))
> -            file_text = file_text % file_count
> +            basic_file_text = get_plural_text(
> +                file_count, _('%d file modified'), _('%d files modified'))
> +            basic_file_text = basic_file_text % file_count
> +            diffstat_text = '<br/>'.join(
> +                '%s (+%d/-%d)' % (path, added, removed)
> +                for path, (added, removed) in sorted(diffstat.items()))

That's an XSS vulnerability. This should all be using structured(), though the variable number of <br/>s is annoying.

> +            file_text = (
> +                '<div class="collapsible"><span>%s</span><div>%s</div></div>' %
> +                (basic_file_text, diffstat_text))
>  
>          args = {
>              'line_count': _('%s lines') % diff.diff_lines_count,
> @@ -84,8 +90,8 @@
>          else:
>              return (
>                  '<a href="%(url)s" class="diff-link">'
> -                '%(line_count)s%(count_text)s%(file_text)s%(conflict_text)s'
> -                '</a>' % args)
> +                '%(line_count)s%(count_text)s%(conflict_text)s'
> +                '</a>%(file_text)s' % args)
>  
>  
>  class PreviewDiffFormatterAPI(DiffFormatterAPI):
> 
> === modified file 'lib/lp/code/browser/tests/test_tales.py'
> --- lib/lp/code/browser/tests/test_tales.py	2014-07-09 02:42:47 +0000
> +++ lib/lp/code/browser/tests/test_tales.py	2015-06-03 10:20:58 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
> +# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>  
>  """Tests for the tales formatters."""
> @@ -110,7 +110,11 @@
>                  'file2': (3, 0)})
>          self.assertEqual(
>              '<a href="%s/+files/preview.diff" class="diff-link">'
> -            '10 lines (+4/-0) 2 files modified</a>'
> +            '10 lines (+4/-0)</a>'
> +            '<div class="collapsible">'
> +            '<span>2 files modified</span>'
> +            '<div>file1 (+1/-0)<br/>file2 (+3/-0)</div>'
> +            '</div>'
>              % canonical_url(preview),
>              test_tales('preview/fmt:link', preview=preview))
>  
> @@ -121,7 +125,11 @@
>                  'file': (3, 0)})
>          self.assertEqual(
>              '<a href="%s/+files/preview.diff" class="diff-link">'
> -            '10 lines (+4/-0) 1 file modified</a>'
> +            '10 lines (+4/-0)</a>'
> +            '<div class="collapsible">'
> +            '<span>1 file modified</span>'
> +            '<div>file (+3/-0)</div>'
> +            '</div>'
>              % canonical_url(preview),
>              test_tales('preview/fmt:link', preview=preview))
>  
> @@ -147,10 +155,16 @@
>              (self.factory.getUniqueString(), (2, 3)) for x in range(23))
>          preview = self._createStalePreviewDiff(
>              500, 89, 340, diffstat=diffstat)
> +        expected_diffstat = '<br/>'.join(
> +            '%s (+2/-3)' % path for path in sorted(diffstat))
>          self.assertEqual(
>              '<a href="%s/+files/preview.diff" class="diff-link">'
> -            '500 lines (+89/-340) 23 files modified</a>'
> -            % canonical_url(preview),
> +            '500 lines (+89/-340)</a>'
> +            '<div class="collapsible">'
> +            '<span>23 files modified</span>'
> +            '<div>%s</div>'
> +            '</div>'
> +            % (canonical_url(preview), expected_diffstat),
>              test_tales('preview/fmt:link', preview=preview))
>  
>      def test_fmt_stale_non_empty_diff_with_conflicts(self):
> @@ -159,10 +173,16 @@
>              (self.factory.getUniqueString(), (2, 3)) for x in range(23))
>          preview = self._createStalePreviewDiff(
>              500, 89, 340, u'conflicts', diffstat=diffstat)
> +        expected_diffstat = '<br/>'.join(
> +            '%s (+2/-3)' % path for path in sorted(diffstat))
>          self.assertEqual(
>              '<a href="%s/+files/preview.diff" class="diff-link">'
> -            '500 lines (+89/-340) 23 files modified (has conflicts)</a>'
> -            % canonical_url(preview),
> +            '500 lines (+89/-340) (has conflicts)</a>'
> +            '<div class="collapsible">'
> +            '<span>23 files modified</span>'
> +            '<div>%s</div>'
> +            '</div>'
> +            % (canonical_url(preview), expected_diffstat),
>              test_tales('preview/fmt:link', preview=preview))
>  
>  
> 
> === modified file 'lib/lp/code/stories/branches/xx-branchmergeproposals.txt'
> --- lib/lp/code/stories/branches/xx-branchmergeproposals.txt	2015-05-28 08:35:20 +0000
> +++ lib/lp/code/stories/branches/xx-branchmergeproposals.txt	2015-06-03 10:20:58 +0000
> @@ -13,7 +13,6 @@
>      ...     BranchMergeProposalStatus,
>      ...     BranchSubscriptionNotificationLevel,
>      ...     CodeReviewNotificationLevel)
> -    ...     BranchSubscriptionNotificationLevel, CodeReviewNotificationLevel)
>      >>> from lp.code.interfaces.branchlookup import IBranchLookup
>      >>> from lp.code.tests.helpers import (
>      ...     make_merge_proposal_without_reviewers)
> @@ -638,7 +637,7 @@
>      >>> from lazr.uri import URI
>      >>> print http(r"""
>      ... GET %s HTTP/1.1
> -    ... Authorization: Basic no-priv@xxxxxxxxxxxxx:test        
> +    ... Authorization: Basic no-priv@xxxxxxxxxxxxx:test
>      ... """ % URI(link['href']).path)
>      HTTP/1.1 303 See Other
>      ...
> @@ -667,7 +666,8 @@
>  Preview diff generation status
>  ------------------------------
>  
> -    >>> update = find_tag_by_id(nopriv_browser.contents, 'diff-pending-update')
> +    >>> update = find_tag_by_id(
> +    ...     nopriv_browser.contents, 'diff-pending-update')
>      >>> print extract_text(update)
>      Updating diff...
>      An updated diff will be available in a few minutes.  Reload to see the
> @@ -713,7 +713,10 @@
>      >>> print_tag_with_id(nopriv_browser.contents, 'landing-targets')
>      Ready for review for merging into lp://dev/fooix
>        Eric the Viking: Pending (code) requested ... ago
> -      Diff: 47 lines (+7/-0) 2 files modified                api
> +      Diff: 47 lines (+7/-13) 2 files modified
> +      file1 (+3/-8)
> +      file2 (+4/-5)
> +      api
>  
>  
>  Merge proposal details shown on the bug page
> @@ -734,4 +737,7 @@
>      lp://dev/~fred/fooix/proposed
>        Ready for review for merging into lp://dev/fooix
>          Eric the Viking: Pending (code) requested ... ago
> -        Diff: 47 lines (+7/-0) 2 files modified              api
> +        Diff: 47 lines (+7/-13) 2 files modified
> +        file1 (+3/-8)
> +        file2 (+4/-5)
> +	api
> 
> === modified file 'lib/lp/code/tests/helpers.py'
> --- lib/lp/code/tests/helpers.py	2015-04-21 16:53:45 +0000
> +++ lib/lp/code/tests/helpers.py	2015-06-03 10:20:58 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
> +# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>  
>  """Helper functions for code testing live here."""
> @@ -117,7 +117,7 @@
>      naked_bmp.target_branch.last_scanned_id = preview.target_revision_id
>      preview.diff_lines_count = 47
>      preview.added_lines_count = 7
> -    preview.remvoed_lines_count = 13
> +    preview.removed_lines_count = 13
>      preview.diffstat = {'file1': (3, 8), 'file2': (4, 5)}
>      return {
>          'eric': eric, 'fooix': fooix, 'trunk': trunk, 'feature': feature,
> 


-- 
https://code.launchpad.net/~cjwatson/launchpad/bmp-show-diffstat/+merge/260921
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.