launchpad-reviewers team mailing list archive

[Goulin Khoge <mp+476312@xxxxxxxxxxxxxxxxxx>]

Goulin Khoge has proposed merging ~goulinkh/launchpad:fix-query-param-escaping into launchpad:master.

Commit message:
Escape query param values before using values as JavaScript code

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

diff --git a/lib/lp/registry/templates/object-timeline-graph.pt b/lib/lp/registry/templates/object-timeline-graph.pt
index da27897..776455f 100644
--- a/lib/lp/registry/templates/object-timeline-graph.pt
+++ b/lib/lp/registry/templates/object-timeline-graph.pt
@@ -140,10 +140,10 @@
   </script>
   <script
     tal:define="
-      include_inactive request/form/include_inactive | string:false;
-      resize_frame request/form/resize_frame | string:;
-      start request/form/start | string:;
-      size request/form/size | string:"
+      include_inactive request/safe_form/include_inactive | string:false;
+      resize_frame request/safe_form/resize_frame | string:;
+      start request/safe_form/start | string:;
+      size request/safe_form/size | string:"
     tal:content="
       string: show_timeline_graph(
         '${include_inactive}', '${resize_frame}', '${start}', '${size}');"/>
diff --git a/lib/lp/services/webapp/servers.py b/lib/lp/services/webapp/servers.py
index 1bed9ee..7278634 100644
--- a/lib/lp/services/webapp/servers.py
+++ b/lib/lp/services/webapp/servers.py
@@ -68,6 +68,7 @@ from lp.services.webapp.authorization import (
     LAUNCHPAD_SECURITY_POLICY_CACHE_UNAUTH_KEY,
 )
 from lp.services.webapp.errorlog import ErrorReportRequest
+from lp.services.webapp.escaping import html_escape
 from lp.services.webapp.interaction import get_interaction_extras
 from lp.services.webapp.interfaces import (
     IBasicLaunchpadRequest,
@@ -541,6 +542,22 @@ def get_query_string_params(request):
     )
 
 
+def safe_form_values(form: dict) -> dict:
+    """Return a copy of the form with values escaped for use in JavaScript."""
+    safe_form = {}
+    for key, value in form.items():
+        if isinstance(value, str):
+            value = html_escape(value)
+        elif isinstance(value, list):
+            value = []
+            for item in value:
+                if isinstance(item, str):
+                    item = html_escape(item)
+                value.append(item)
+        safe_form[key] = value
+    return safe_form
+
+
 class LaunchpadBrowserRequestMixin:
     """Provides methods used for both API and web browser requests."""
 
@@ -713,6 +730,14 @@ class LaunchpadBrowserRequest(
         """See ILaunchpadBrowserApplicationRequest."""
         return BrowserFormNG(self.form)
 
+    @cachedproperty
+    def safe_form(self):
+        """
+        use the safe_form when the value will be used in a JavaScript string,
+        to avoid injection attacks.
+        """
+        return safe_form_values(self.form)
+
     def setPrincipal(self, principal):
         self.clearSecurityPolicyCache()
         BrowserRequest.setPrincipal(self, principal)
@@ -1037,6 +1062,10 @@ class LaunchpadTestRequest(
         return BrowserFormNG(self.form)
 
     @property
+    def safe_form(self):
+        return safe_form_values(self.form)
+
+    @property
     def query_string_params(self):
         """See ILaunchpadBrowserApplicationRequest."""
         return get_query_string_params(self)
diff --git a/lib/lp/services/webapp/tests/test_publisher.py b/lib/lp/services/webapp/tests/test_publisher.py
index 8e6cc93..9351734 100644
--- a/lib/lp/services/webapp/tests/test_publisher.py
+++ b/lib/lp/services/webapp/tests/test_publisher.py
@@ -525,6 +525,55 @@ class TestLaunchpadView(TestCaseWithFactory):
         ]
         self.assertEqual(expected_beta_features, view.beta_features)
 
+    def test_request_safe_form_sanitizes_html(self):
+        """Test that HTML in form parameters is properly escaped."""
+        request = LaunchpadTestRequest(
+            form={"resize_frame": "<script>alert(1)</script>"}
+        )
+        LaunchpadView(object(), request)
+        self.assertEqual(
+            request.safe_form["resize_frame"],
+            "&lt;script&gt;alert(1)&lt;/script&gt;",
+        )
+
+    def test_request_safe_form_preserves_safe_values(self):
+        """Test that safe form values are not modified."""
+        request = LaunchpadTestRequest(
+            form={"resize_frame": "normal123-value.text"}
+        )
+        LaunchpadView(object(), request)
+        self.assertEqual(
+            request.safe_form["resize_frame"], "normal123-value.text"
+        )
+
+    def test_request_safe_form_sanitizes_multiple_values(self):
+        """Test that multiple form values containing HTML are escaped."""
+        request = LaunchpadTestRequest(
+            form={
+                "field1": "<p>test</p>",
+                "field2": "'); test //",
+            }
+        )
+        LaunchpadView(object(), request)
+        self.assertEqual(
+            request.safe_form["field1"], "&lt;p&gt;test&lt;/p&gt;"
+        )
+        self.assertEqual(request.safe_form["field2"], "&#x27;); test //")
+
+    def test_request_safe_form_handles_non_string_values(self):
+        """Test that non-string form values are not modified."""
+        request = LaunchpadTestRequest(form={"number": 123, "boolean": True})
+        LaunchpadView(object(), request)
+        self.assertEqual(request.safe_form["number"], 123)
+        self.assertEqual(request.safe_form["boolean"], True)
+
+    def test_request_safe_form_handles_empty_values(self):
+        """Test that empty or None form values are handled properly."""
+        request = LaunchpadTestRequest(form={"empty": "", "none": None})
+        LaunchpadView(object(), request)
+        self.assertEqual(request.safe_form["empty"], "")
+        self.assertIsNone(request.safe_form["none"])
+
 
 class TestRedirectionView(TestCase):
     layer = DatabaseFunctionalLayer