launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03116
[Merge] lp:~henninge/launchpad/devel-745550-structured-cgi-escape into lp:launchpad
Henning Eggers has proposed merging lp:~henninge/launchpad/devel-745550-structured-cgi-escape into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #745550 in Launchpad itself: "Wrong use of structured in translations code."
https://bugs.launchpad.net/launchpad/+bug/745550
For more details, see:
https://code.launchpad.net/~henninge/launchpad/devel-745550-structured-cgi-escape/+merge/55521
= Summary =
After reading wgrant's mail about xss attack vectors, I realized that
I have seen lots of wrong uses of structured() in translations code
and that I had just recently added one. This branch fixes those.
A lot of these are in the upload views but I did not try to fix
bug 192925 now although that would have been a nice side effect.
== Implementation details ==
I could only replace one use of cgi.escape because doint the rest of
them proofed not so straight forward. It will most likely become much
easier when we start using MarkupSafe.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/translations/browser/customlanguagecode.py
lib/lp/translations/browser/translator.py
lib/lp/translations/browser/pofile.py
lib/lp/translations/browser/translationmessage.py
lib/lp/translations/browser/productseries.py
lib/lp/translations/browser/sourcepackage.py
lib/lp/translations/browser/potemplate.py
./lib/lp/translations/browser/pofile.py
788: E301 expected 1 blank line, found 2
904: E301 expected 1 blank line, found 2
./lib/lp/translations/browser/translationmessage.py
423: E301 expected 1 blank line, found 2
451: E301 expected 1 blank line, found 2
616: E301 expected 1 blank line, found 2
841: E301 expected 1 blank line, found 2
924: E301 expected 1 blank line, found 2
1451: E301 expected 1 blank line, found 2
--
https://code.launchpad.net/~henninge/launchpad/devel-745550-structured-cgi-escape/+merge/55521
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~henninge/launchpad/devel-745550-structured-cgi-escape into lp:launchpad.
=== modified file 'lib/lp/translations/browser/customlanguagecode.py'
--- lib/lp/translations/browser/customlanguagecode.py 2010-12-23 15:03:37 +0000
+++ lib/lp/translations/browser/customlanguagecode.py 2011-03-30 11:35:38 +0000
@@ -96,7 +96,7 @@
self.setFieldError(
'language_code',
structured(
- "There already is a custom language code '%s'." %
+ "There already is a custom language code '%s'.",
self.language_code))
return
else:
=== modified file 'lib/lp/translations/browser/pofile.py'
--- lib/lp/translations/browser/pofile.py 2011-03-22 10:08:40 +0000
+++ lib/lp/translations/browser/pofile.py 2011-03-30 11:35:38 +0000
@@ -725,8 +725,8 @@
'should be imported, it will be reviewed manually by an '
'administrator in the coming few days. You can track '
'your upload\'s status in the '
- '<a href="%s/+imports">Translation Import Queue</a>' %(
- canonical_url(self.context.potemplate.translationtarget))))
+ '<a href="%s/+imports">Translation Import Queue</a>',
+ canonical_url(self.context.potemplate.translationtarget)))
class POFileBatchNavigator(BatchNavigator):
=== modified file 'lib/lp/translations/browser/potemplate.py'
--- lib/lp/translations/browser/potemplate.py 2011-03-29 08:38:04 +0000
+++ lib/lp/translations/browser/potemplate.py 2011-03-30 11:35:38 +0000
@@ -450,8 +450,8 @@
'should be imported, it will be reviewed manually by an '
'administrator in the coming few days. You can track '
'your upload\'s status in the '
- '<a href="%s/+imports">Translation Import Queue</a>' %(
- canonical_url(self.context.translationtarget))))
+ '<a href="%s/+imports">Translation Import Queue</a>',
+ canonical_url(self.context.translationtarget)))
elif helpers.is_tar_filename(filename):
# Add the whole tarball to the import queue.
@@ -472,7 +472,7 @@
itthey = 'they'
self.request.response.addInfoNotification(
structured(
- 'Thank you for your upload. %d file%s from the tarball '
+ 'Thank you for your upload. %s file%s from the tarball '
'will be automatically '
'reviewed in the next few hours. If that is not enough '
'to determine whether and where your file%s should '
@@ -488,26 +488,26 @@
"A file could not be uploaded because its "
"name matched multiple existing uploads, for "
"different templates.")
- ul_conflicts = (
+ ul_conflicts = structured(
"The conflicting file name was:<br /> "
- "<ul><li>%s</li></ul>" % cgi.escape(conflicts[0]))
+ "<ul><li>%s</li></ul>", conflicts[0])
else:
- warning = (
- "%d files could not be uploaded because their "
+ warning = structured(
+ "%s files could not be uploaded because their "
"names matched multiple existing uploads, for "
- "different templates." % len(conflicts))
- ul_conflicts = (
+ "different templates.", len(conflicts))
+ ul_conflicts = structured(
"The conflicting file names were:<br /> "
"<ul><li>%s</li></ul>" % (
"</li><li>".join(map(cgi.escape, conflicts))))
self.request.response.addWarningNotification(
structured(
- warning + " This makes it "
+ "%s This makes it "
"impossible to determine which template the new "
"upload was for. Try uploading to a specific "
"template: visit the page for the template that you "
"want to upload to, and select the upload option "
- "from there.<br />"+ ul_conflicts))
+ "from there.<br />%s", warning, ul_conflicts))
else:
if len(conflicts) == 0:
self.request.response.addWarningNotification(
=== modified file 'lib/lp/translations/browser/productseries.py'
--- lib/lp/translations/browser/productseries.py 2011-03-29 10:07:59 +0000
+++ lib/lp/translations/browser/productseries.py 2011-03-30 11:35:38 +0000
@@ -263,9 +263,8 @@
'should be imported, it will be reviewed manually by an '
'administrator in the coming few days. You can track '
'your upload\'s status in the '
- '<a href="%s/+imports">Translation Import Queue</a>' %(
- canonical_url(self.context,
- rootsite='translations'))))
+ '<a href="%s/+imports">Translation Import Queue</a>',
+ canonical_url(self.context, rootsite='translations')))
elif is_tar_filename(filename):
# Add the whole tarball to the import queue.
@@ -283,43 +282,42 @@
itthey = 'they'
self.request.response.addInfoNotification(
structured(
- 'Thank you for your upload. %d file%s from the tarball '
+ 'Thank you for your upload. %s file%s from the tarball '
'will be automatically '
'reviewed in the next few hours. If that is not enough '
'to determine whether and where your file%s should '
'be imported, %s will be reviewed manually by an '
'administrator in the coming few days. You can track '
'your upload\'s status in the '
- '<a href="%s/+imports">Translation Import Queue</a>' %(
+ '<a href="%s/+imports">Translation Import Queue</a>',
num, plural_s, plural_s, itthey,
- canonical_url(self.context,
- rootsite='translations'))))
+ canonical_url(self.context, rootsite='translations')))
if len(conflicts) > 0:
if len(conflicts) == 1:
warning = (
"A file could not be uploaded because its "
"name matched multiple existing uploads, for "
"different templates.")
- ul_conflicts = (
+ ul_conflicts = structured(
"The conflicting file name was:<br /> "
- "<ul><li>%s</li></ul>" % cgi.escape(conflicts[0]))
+ "<ul><li>%s</li></ul>", conflicts[0])
else:
- warning = (
- "%d files could not be uploaded because their "
+ warning = structured(
+ "%s files could not be uploaded because their "
"names matched multiple existing uploads, for "
- "different templates." % len(conflicts))
- ul_conflicts = (
+ "different templates.", len(conflicts))
+ ul_conflicts = structured(
"The conflicting file names were:<br /> "
"<ul><li>%s</li></ul>" % (
"</li><li>".join(map(cgi.escape, conflicts))))
self.request.response.addWarningNotification(
structured(
- warning + " This makes it "
+ "%s This makes it "
"impossible to determine which template the new "
"upload was for. Try uploading to a specific "
"template: visit the page for the template that you "
"want to upload to, and select the upload option "
- "from there.<br />"+ ul_conflicts))
+ "from there.<br />%s", warning, ul_conflicts))
else:
if len(conflicts) == 0:
self.request.response.addWarningNotification(
=== modified file 'lib/lp/translations/browser/sourcepackage.py'
--- lib/lp/translations/browser/sourcepackage.py 2011-03-29 10:07:59 +0000
+++ lib/lp/translations/browser/sourcepackage.py 2011-03-30 11:35:38 +0000
@@ -122,10 +122,10 @@
'No upstream templates have been found yet. Please follow '
'the import process by going to the '
'<a href="%s">Translation Import Queue</a> of the '
- 'upstream project series.' %(
+ 'upstream project series.',
canonical_url(
self.context.productseries, rootsite='translations',
- view_name="+imports"))))
+ view_name="+imports")))
if self.is_merge_job_running:
self.request.response.addInfoNotification(
'Translations are currently being linked by a background '
=== modified file 'lib/lp/translations/browser/translationmessage.py'
--- lib/lp/translations/browser/translationmessage.py 2011-03-08 04:19:53 +0000
+++ lib/lp/translations/browser/translationmessage.py 2011-03-30 11:35:38 +0000
@@ -322,7 +322,7 @@
This only needs to be done once per language. Thanks for helping
Launchpad Translations.
</p>
- """ % self.pofile.language.englishname))
+ """, self.pofile.language.englishname))
return
self._initializeAltLanguage()
@@ -665,11 +665,10 @@
"language %(alternative)s. If you wish to see "
"suggestions from this language, "
'<a href="%(editlanguages_url)s">'
- "add it to your preferred languages</a> first."
- % dict(
+ "add it to your preferred languages</a> first.",
alternative=alternative_language.displayname,
editlanguages_url=editlanguages_url,
- )))
+ ))
alternative_language = None
second_lang_code = None
@@ -1305,8 +1304,10 @@
used_languages = [language]
if self.sec_lang is not None:
used_languages.append(self.sec_lang)
- translations = potmsgset.getExternallySuggestedOrUsedTranslationMessages(
- suggested_languages=[language], used_languages=used_languages)
+ translations = (
+ potmsgset.getExternallySuggestedOrUsedTranslationMessages(
+ suggested_languages=[language],
+ used_languages=used_languages))
alt_external = translations[self.sec_lang].used
externally_used = self._setOnePOFile(sorted(
translations[language].used,
=== modified file 'lib/lp/translations/browser/translator.py'
--- lib/lp/translations/browser/translator.py 2010-11-23 23:22:27 +0000
+++ lib/lp/translations/browser/translator.py 2011-03-30 11:35:38 +0000
@@ -51,7 +51,7 @@
self.setFieldError('language',
structured(
- '%s is already a translator for this language' %
+ '%s is already a translator for this language',
existing_translator_link))
@property
Follow ups
