Fix quoting of query parameters in list view (#5646)

* Add proper quoting of object-list filters

* Add unit test

* Change fragment

* Fix new bug caught by integration tests

* Use location.assign rather than location.replace, close modal on form submit

changes/5646.security

@@ -0,0 +1 @@
+Fixed a reflected-XSS vulnerability ([GHSA-jxgr-gcj5-cqqg](https://github.com/nautobot/nautobot/security/advisories/GHSA-jxgr-gcj5-cqqg)) in object-list view rendering of user-provided query parameters.

nautobot/core/templates/generic/object_list.html

@@ -74,7 +74,7 @@ <h1>{% block title %}{{ title }}{% endblock %}</h1>
                     class="remove-filter-param"
                     title="Remove all items"
                     data-field-type="parent"
-                    data-field-value={{ field.name }}
+                    data-field-value="{{ field.name }}"
             >×</span>
             <ul class="filter-selection-rendered">
                 {% for value in field.values %}
@@ -85,8 +85,8 @@ <h1>{% block title %}{{ title }}{% endblock %}</h1>
                     <span
                             class="filter-selection-choice-remove remove-filter-param"
                             data-field-type="child"
-                            data-field-parent={{ field.name }}
-                            data-field-value={{ value.name }}
+                            data-field-parent="{{ field.name }}"
+                            data-field-value="{{ value.name }}"
                     >×</span>{{ value.display }}
                 </li>
                 {% endfor %}

nautobot/core/tests/test_views.py

@@ -242,6 +242,25 @@ def test_filtering_on_custom_select_filter_field(self):
         self.assertInHTML(locations[0].name, response_content)
         self.assertInHTML(locations[1].name, response_content)
 
+    def test_filtering_crafted_query_params(self):
+        """Test for reflected-XSS vulnerability GHSA-jxgr-gcj5-cqqg."""
+        self.add_permissions("dcim.view_location")
+        query_param = "?location_type=1 onmouseover=alert('hi') foo=bar"
+        url = reverse("dcim:location_list") + query_param
+        response = self.client.get(url)
+        self.assertHttpStatus(response, 200)
+        response_content = response.content.decode(response.charset)
+        # The important thing here is that the data-field-parent and data-field-value are correctly quoted
+        self.assertInHTML(
+            """
+<span class="filter-selection-choice-remove remove-filter-param"
+      data-field-type="child"
+      data-field-parent="location_type"
+      data-field-value="1 onmouseover=alert(&#x27;hi&#x27;) foo=bar"
+>×</span>""",  # noqa: RUF001 - ambiguous-unicode-character-string
+            response_content,
+        )
+
 
 class ForceScriptNameTestcase(TestCase):
     """Basic test to assert that `settings.FORCE_SCRIPT_NAME` works as intended."""

nautobot/project-static/js/forms.js

@@ -632,18 +632,19 @@ function initializeDynamicFilterForm(context){
 
     // Remove applied filters
     this_context.find(".remove-filter-param").on("click", function(){
-        let query_params = location.search;
+        let query_params = new URLSearchParams(location.search);
         let type = $(this).attr("data-field-type");
         let field_value = $(this).attr("data-field-value");
-        let query_string = location.search.substr(1).split("&");
 
         if (type === "parent") {
-            query_string = query_string.filter(item => item.search(field_value) < 0);
+            // Remove all instances of this query param
+            query_params.delete(field_value);
         } else {
+            // Remove this specific instance of this query param
             let parent = $(this).attr("data-field-parent");
-            query_string = query_string.filter(item => item.search(parent + "=" + field_value) < 0)
+            query_params.delete(parent, field_value);
         }
-        location.replace("?" + query_string.join("&"))
+        location.assign("?" + query_params);
     })
 
     // On submit of filter form
@@ -657,12 +658,18 @@ function initializeDynamicFilterForm(context){
         q_field_phantom.val(q_field.val())
         dynamic_form.append(q_field_phantom);
 
-        // Get the serialize data from the forms and filter out query_params which values are empty e.g ?sam=&dan=2 becomes dan=2
-        let dynamic_filter_form_query = $("#dynamic-filter-form").serialize().split("&").filter(params => params.split("=")[1]?.length || 0 )
-        let default_filter_form_query = $("#default-filter form").serialize().split("&").filter(params => params.split("=")[1]?.length || 0 )
-        // Union Operation
-        let search_query = [...new Set([...default_filter_form_query, ...dynamic_filter_form_query])].join("&")
-        location.replace("?" + search_query)
+        // Get the serialized data from the forms and:
+        // 1) filter out query_params which values are empty e.g ?sam=&dan=2 becomes dan=2
+        // 2) combine the two forms into a single set of data without duplicate entries
+        let search_query = new URLSearchParams();
+        let dynamic_query = new URLSearchParams(new FormData(document.getElementById("dynamic-filter-form")));
+        dynamic_query.forEach((value, key) => { if (value != "") { search_query.append(key, value); }});
+        let default_query = new URLSearchParams(new FormData(document.getElementById("default-filter").firstElementChild));
+        default_query.forEach((value, key) => {
+            if (value != "" && !search_query.has(key, value)) { search_query.append(key, value); }
+        });
+        $("#FilterForm_modal").modal("hide");
+        location.assign("?" + search_query);
     })
 
     // On submit of filter search form