launchpad-reviewers team mailing list archive

[Goulin Khoge <mp+476203@xxxxxxxxxxxxxxxxxx>]

Goulin Khoge has proposed merging ~goulinkh/launchpad:goulin-escape-query-params into launchpad:master.

Commit message:
Escape query param values before rendering the HTML templates.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~goulinkh/launchpad/+git/launchpad/+merge/476203
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~goulinkh/launchpad:goulin-escape-query-params into launchpad:master.

diff --git a/lib/lp/services/webapp/publisher.py b/lib/lp/services/webapp/publisher.py
index 18aca49..64892eb 100644
--- a/lib/lp/services/webapp/publisher.py
+++ b/lib/lp/services/webapp/publisher.py
@@ -306,6 +306,7 @@ class LaunchpadView(UserAttributeCache):
         cache = self._get_json_cache()
         if cache is None:
             return
+        self.form = request.form_ng
         related_features = cache.setdefault("related_features", {})
         related_features.update(self.related_feature_info)
 
diff --git a/lib/lp/services/webapp/servers.py b/lib/lp/services/webapp/servers.py
index 1bed9ee..d520286 100644
--- a/lib/lp/services/webapp/servers.py
+++ b/lib/lp/services/webapp/servers.py
@@ -69,6 +69,7 @@ from lp.services.webapp.authorization import (
 )
 from lp.services.webapp.errorlog import ErrorReportRequest
 from lp.services.webapp.interaction import get_interaction_extras
+from lp.services.webapp.escaping import html_escape
 from lp.services.webapp.interfaces import (
     IBasicLaunchpadRequest,
     IBrowserFormNG,
@@ -748,6 +749,10 @@ class BrowserFormNG:
     def __init__(self, form):
         """Create a new BrowserFormNG that wraps a dict of form data."""
         self.form = form
+        # sanitize the form data
+        for key, value in list(self.form.items()):
+            if isinstance(value, str):
+                self.form[key] = html_escape(value)
 
     def __contains__(self, name):
         """See IBrowserFormNG."""
diff --git a/lib/lp/services/webapp/tests/test_publisher.py b/lib/lp/services/webapp/tests/test_publisher.py
index 8e6cc93..0f1051d 100644
--- a/lib/lp/services/webapp/tests/test_publisher.py
+++ b/lib/lp/services/webapp/tests/test_publisher.py
@@ -526,6 +526,63 @@ class TestLaunchpadView(TestCaseWithFactory):
         self.assertEqual(expected_beta_features, view.beta_features)
 
 
+    def test_request_form_sanitizes_html(self):
+        """Test that HTML in form parameters is properly escaped."""
+        request = LaunchpadTestRequest(
+            form={'resize_frame': '<script>alert(1)</script>'})
+        view = LaunchpadView(object(), request)
+        self.assertEqual(
+            request.form['resize_frame'],
+            '&lt;script&gt;alert(1)&lt;/script&gt;'
+        )
+
+    def test_request_form_preserves_safe_values(self):
+        """Test that safe form values are not modified."""
+        request = LaunchpadTestRequest(
+            form={'resize_frame': 'normal123-value.text'})
+        view = LaunchpadView(object(), request)
+        self.assertEqual(
+            request.form['resize_frame'],
+            'normal123-value.text'
+        )
+
+    def test_request_form_sanitizes_multiple_values(self):
+        """Test that multiple form values containing HTML are escaped."""
+        request = LaunchpadTestRequest(form={
+            'field1': '<p>test</p>',
+            'field2': '<script>alert(2)</script>'
+        })
+        view = LaunchpadView(object(), request)
+        self.assertEqual(
+            request.form['field1'],
+            '&lt;p&gt;test&lt;/p&gt;'
+        )
+        self.assertEqual(
+            request.form['field2'],
+            '&lt;script&gt;alert(2)&lt;/script&gt;'
+        )
+
+    def test_request_form_handles_non_string_values(self):
+        """Test that non-string form values are not modified."""
+        request = LaunchpadTestRequest(form={
+            'number': 123,
+            'boolean': True
+        })
+        view = LaunchpadView(object(), request)
+        self.assertEqual(request.form['number'], 123)
+        self.assertEqual(request.form['boolean'], True)
+
+    def test_request_form_handles_empty_values(self):
+        """Test that empty or None form values are handled properly."""
+        request = LaunchpadTestRequest(form={
+            'empty': '',
+            'none': None
+        })
+        view = LaunchpadView(object(), request)
+        self.assertEqual(request.form['empty'], '')
+        self.assertIsNone(request.form['none'])
+
+
 class TestRedirectionView(TestCase):
     layer = DatabaseFunctionalLayer