fix: Prevent URLSearchParams.toString() from returning unencoded params when not modified (#292)

* fix: prevent URLSearchParams.toString() from returning unencoded params when not modified

* refactor: address pr review feedback

Author: mattermoran

crates/rust-url/src/lib.rs

@@ -1,6 +1,7 @@
 #![allow(clippy::missing_safety_doc)]
 
 /// Wrapper for the Url crate and a URLSearchParams implementation that enables use from C++.
+use std::cell::RefCell;
 use std::marker::PhantomData;
 use std::slice;
 use url::{form_urlencoded, quirks, Url};
@@ -14,12 +15,22 @@ impl JSUrl {
     fn update_params(&self) {
         if let Some(params) = unsafe { self.params.as_mut() } {
             params.list = self.url.query_pairs().into_owned().collect();
+            if let UrlOrString::Url {
+                ref serialized_query_cache,
+                ..
+            } = params.url_or_str
+            {
+                *serialized_query_cache.borrow_mut() = None;
+            }
         }
     }
 }
 
 enum UrlOrString {
-    Url(*mut JSUrl),
+    Url {
+        url: *mut JSUrl,
+        serialized_query_cache: RefCell<Option<String>>,
+    },
     Str(String),
 }
 
@@ -36,7 +47,10 @@ impl JSUrlSearchParams {
     /// This is used in `params_to_string` to hand out a stable reference.
     fn update_url_or_str(&mut self) {
         match self.url_or_str {
-            UrlOrString::Url(url) => {
+            UrlOrString::Url {
+                url,
+                ref serialized_query_cache,
+            } => {
                 let url = unsafe { url.as_mut().unwrap() };
                 if self.list.is_empty() {
                     url.url.set_query(None);
@@ -45,6 +59,7 @@ impl JSUrlSearchParams {
                     pairs.clear();
                     pairs.extend_pairs(self.list.iter());
                 }
+                *serialized_query_cache.borrow_mut() = None;
             }
             UrlOrString::Str(_) => {
                 let str = if self.list.is_empty() {
@@ -222,7 +237,10 @@ pub unsafe extern "C" fn url_search_params(url: *mut JSUrl) -> *mut JSUrlSearchP
     if url.params.is_null() {
         url.params = Box::into_raw(Box::new(JSUrlSearchParams {
             list: url.url.query_pairs().into_owned().collect(),
-            url_or_str: UrlOrString::Url(url),
+            url_or_str: UrlOrString::Url {
+                url,
+                serialized_query_cache: RefCell::new(None),
+            },
         }));
     }
     url.params
@@ -401,12 +419,17 @@ pub extern "C" fn params_sort(params: &mut JSUrlSearchParams) {
 #[no_mangle]
 pub extern "C" fn params_to_string(params: &JSUrlSearchParams) -> SpecSlice<'_> {
     match &params.url_or_str {
-        UrlOrString::Url(url) => {
-            let url = unsafe { url.as_mut().unwrap() };
-            match url.url.query() {
-                Some(query) => query.into(),
-                None => "".into(),
-            }
+        UrlOrString::Url {
+            serialized_query_cache,
+            ..
+        } => {
+            let mut cache = serialized_query_cache.borrow_mut();
+            let query = cache.get_or_insert_with(|| {
+                form_urlencoded::Serializer::new(String::new())
+                    .extend_pairs(&params.list)
+                    .finish()
+            });
+            SpecSlice::new(query.as_ptr(), query.len())
         }
         UrlOrString::Str(str) => str.as_str().into(),
     }

tests/wpt-harness/expectations/url/url-constructor.any.js.json

@@ -1461,7 +1461,7 @@
     "status": "PASS"
   },
   "Parsing: <??a=b&c=d> against <http://example.org/foo/bar>": {
-    "status": "FAIL"
+    "status": "PASS"
   },
   "Parsing: <http:> against <http://example.org/foo/bar>": {
     "status": "PASS"
@@ -1473,19 +1473,19 @@
     "status": "PASS"
   },
   "Parsing: <http://foo.bar/baz?qux#foo\bbar> without base": {
-    "status": "FAIL"
+    "status": "PASS"
   },
   "Parsing: <http://foo.bar/baz?qux#foo\"bar> without base": {
-    "status": "FAIL"
+    "status": "PASS"
   },
   "Parsing: <http://foo.bar/baz?qux#foo<bar> without base": {
-    "status": "FAIL"
+    "status": "PASS"
   },
   "Parsing: <http://foo.bar/baz?qux#foo>bar> without base": {
-    "status": "FAIL"
+    "status": "PASS"
   },
   "Parsing: <http://foo.bar/baz?qux#foo`bar> without base": {
-    "status": "FAIL"
+    "status": "PASS"
   },
   "Parsing: <http://1.2.3.4/> against <http://other.com/>": {
     "status": "PASS"

tests/wpt-harness/expectations/url/url-searchparams.any.js.json

@@ -9,6 +9,6 @@
     "status": "PASS"
   },
   "URL.searchParams and URL.search setters, update propagation": {
-    "status": "FAIL"
+    "status": "PASS"
   }
 }

tests/wpt-harness/expectations/url/urlsearchparams-stringifier.any.js.json

@@ -36,7 +36,7 @@
     "status": "PASS"
   },
   "URLSearchParams connected to URL": {
-    "status": "FAIL"
+    "status": "PASS"
   },
   "URLSearchParams must not do newline normalization": {
     "status": "PASS"