Bug 1737854 - macOS 'Share' menu modifies URL such that shared page doesn't load r=necko-reviewers,mstange,valentin

Revert some of the fix for 1722758 so that only the URL ref component is re-encoded for NSURL compatibility. Other URL fields need additional work to be addressed in a follow up. Update the set of characters re-encoded to be as minimal as possible and include missing characters. Add tests to ensure encoding works as expected, not just that it is accepted by NSURL. Differential Revision: https://phabricator.services.mozilla.com/D130445
2021-11-10 19:32:53 +00:00 · 2021-11-10 19:32:53 +00:00 · 3cf27e9320
--- a/netwerk/base/nsNetUtil.cpp
+++ b/netwerk/base/nsNetUtil.cpp
@ -1749,39 +1749,21 @@ nsresult NS_NewURIWithNSURLEncoding(nsIURI** aResult, const nsACString& aSpec) {
  nsresult rv = NS_NewURI(getter_AddRefs(uri), aSpec);
  NS_ENSURE_SUCCESS(rv, rv);

-  // Escape the ref portion of the URL. An unescaped '#' to indicate
-  // the beginning of the ref component is accepted by NSURL, but '#'
-  // characters in the ref must be escaped. The ref returned from
-  // GetRef() does not include the leading '#'.
+  // Escape the ref portion of the URL. NSURL is more strict about which
+  // characters in the URL must be % encoded. For example, an unescaped '#'
+  // to indicate the beginning of the ref component is accepted by NSURL, but
+  // '#' characters in the ref must be escaped. Also adds encoding for other
+  // characters not accepted by NSURL in the ref such as '{', '|', '}', and '^'.
+  // The ref returned from GetRef() does not include the leading '#'.
  nsAutoCString ref, escapedRef;
  if (NS_SUCCEEDED(uri->GetRef(ref)) && !ref.IsEmpty()) {
-    if (!NS_Escape(ref, escapedRef, url_AppleExtra)) {
+    if (!NS_Escape(ref, escapedRef, url_NSURLRef)) {
      return NS_ERROR_INVALID_ARG;
    }
    rv = NS_MutateURI(uri).SetRef(escapedRef).Finalize(uri);
    NS_ENSURE_SUCCESS(rv, rv);
  }

-  // Escape the file path
-  nsAutoCString filePath, escapedFilePath;
-  if (NS_SUCCEEDED(uri->GetFilePath(filePath)) && !filePath.IsEmpty()) {
-    if (!NS_Escape(filePath, escapedFilePath, url_AppleExtra)) {
-      return NS_ERROR_INVALID_ARG;
-    }
-    rv = NS_MutateURI(uri).SetFilePath(escapedFilePath).Finalize(uri);
-    NS_ENSURE_SUCCESS(rv, rv);
-  }
-
-  // Escape the query
-  nsAutoCString query, escapedQuery;
-  if (NS_SUCCEEDED(uri->GetQuery(query)) && !query.IsEmpty()) {
-    if (!NS_Escape(query, escapedQuery, url_AppleExtra)) {
-      return NS_ERROR_INVALID_ARG;
-    }
-    rv = NS_MutateURI(uri).SetQuery(escapedQuery).Finalize(uri);
-    NS_ENSURE_SUCCESS(rv, rv);
-  }
-
  uri.forget(aResult);
  return NS_OK;
 }
--- a/widget/tests/unit/test_macsharingservice.js
+++ b/widget/tests/unit/test_macsharingservice.js
@ -26,7 +26,9 @@ function test_getSharingProviders() {
    "http://example.org/#/",
    "http://example.org/#/#",
    "http://example.org/#/#/",
-    "http://example.org/foo/bar/x|page.html#this_is_a_fragment",
+    // This test fails due to the '|' in the path which needs additional
+    // encoding before conversion to NSURL. See bug 1740565.
+    // "http://example.org/foo/bar/x|page.html#this_is_a_fragment",
    "http://example.org/page.html#this_is_a_fragment",
    "http://example.org/page.html#this_is_a_fragment#and_another",
    "http://example.org/foo/bar;#foo",
--- a/xpcom/io/nsEscape.cpp
+++ b/xpcom/io/nsEscape.cpp
@ -23,8 +23,8 @@ static const unsigned char netCharType[256] =
 **  Bit 1       xpalpha     -- as xalpha but
 **                             converts spaces to plus and plus to %2B
 **  Bit 3 ...   path        -- as xalphas but doesn't escape '/'
-**  Bit 4 ...   Apple-NSURL -- extra encoding for Apple NSURL compatibility.
-**                             This encoding set is used on encoded URL
+**  Bit 4 ...   NSURL-ref   -- extra encoding for Apple NSURL compatibility.
+**                             This encoding set is used on encoded URL ref
 **                             components before converting a URL to an NSURL
 **                             so we don't include '%' to avoid double encoding.
 */
@ -32,19 +32,19 @@ static const unsigned char netCharType[256] =
  {  0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0, /* 0x */
     0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0, /* 1x */
  /*       !   "   #   $   %   &   '   (   )   *   +   ,   -   .   /        */
-     0x0,0x0,0x0,0x0,0x0,0x8,0x0,0x0,0x0,0x0,0xf,0xc,0x0,0xf,0xf,0xc, /* 2x */
+     0x0,0x8,0x0,0x0,0x8,0x8,0x8,0x8,0x8,0x8,0xf,0xc,0x8,0xf,0xf,0xc, /* 2x */
  /*   0   1   2   3   4   5   6   7   8   9   :   ;   <   =   >   ?        */
-     0xf,0xf,0xf,0xf,0xf,0xf,0xf,0xf,0xf,0xf,0x0,0x0,0x0,0x0,0x0,0x0, /* 3x */
+     0xf,0xf,0xf,0xf,0xf,0xf,0xf,0xf,0xf,0xf,0x8,0x8,0x0,0x8,0x0,0x8, /* 3x */
  /*   @   A   B   C   D   E   F   G   H   I   J   K   L   M   N   O        */
-     0x0,0xf,0xf,0xf,0xf,0xf,0xf,0xf,0xf,0xf,0xf,0xf,0xf,0xf,0xf,0xf, /* 4x */
+     0x8,0xf,0xf,0xf,0xf,0xf,0xf,0xf,0xf,0xf,0xf,0xf,0xf,0xf,0xf,0xf, /* 4x */
     /* bits for '@' changed from 7 to 0 so '@' can be escaped   */
     /* in usernames and passwords in publishing.                */
  /*   P   Q   R   S   T   U   V   W   X   Y   Z   [   \   ]   ^   _        */
     0xf,0xf,0xf,0xf,0xf,0xf,0xf,0xf,0xf,0xf,0xf,0x0,0x0,0x0,0x0,0xf, /* 5x */
  /*   `   a   b   c   d   e   f   g   h   i   j   k   l   m   n   o        */
     0x0,0xf,0xf,0xf,0xf,0xf,0xf,0xf,0xf,0xf,0xf,0xf,0xf,0xf,0xf,0xf, /* 6x */
-  /*   p   q   r   s   t   u   v   w   x   y   z   {   \   }   ~ DEL        */
-     0xf,0xf,0xf,0xf,0xf,0xf,0xf,0xf,0xf,0xf,0xf,0x0,0x0,0x0,0x0,0x0, /* 7x */
+  /*   p   q   r   s   t   u   v   w   x   y   z   {   |   }   ~ DEL        */
+     0xf,0xf,0xf,0xf,0xf,0xf,0xf,0xf,0xf,0xf,0xf,0x0,0x0,0x0,0x8,0x0, /* 7x */
     0x0,
  };

--- a/xpcom/io/nsEscape.h
+++ b/xpcom/io/nsEscape.h
@ -27,9 +27,9 @@ typedef enum {
  url_XPAlphas =
      1u
      << 1,  // As url_XAlphas, but convert spaces (0x20) to '+' and plus to %2B
-  url_Path = 1u << 2,       // As url_XAlphas, but don't escape slash ('/')
-  url_AppleExtra = 1u << 3  // Extra encoding required for Apple's
-                            // NSURL compatibility
+  url_Path = 1u << 2,     // As url_XAlphas, but don't escape slash ('/')
+  url_NSURLRef = 1u << 3  // Extra URL ref encoding required for Apple's
+                          // NSURL compatibility
 } nsEscapeMask;

 #ifdef __cplusplus
--- a/xpcom/tests/gtest/TestEscape.cpp
+++ b/xpcom/tests/gtest/TestEscape.cpp
@ -148,7 +148,7 @@ TEST(Escape, AppleNSURLEscapeHash)
 {
  nsCString toEscape("#");
  nsCString escaped;
-  bool isEscapedOK = NS_Escape(toEscape, escaped, url_AppleExtra);
+  bool isEscapedOK = NS_Escape(toEscape, escaped, url_NSURLRef);
  EXPECT_EQ(isEscapedOK, true);
  EXPECT_STREQ(escaped.BeginReading(), "%23");
 }
@ -158,31 +158,46 @@ TEST(Escape, AppleNSURLEscapeNoDouble)
  // The '%' in "%23" shouldn't be encoded again.
  nsCString toEscape("%23");
  nsCString escaped;
-  bool isEscapedOK = NS_Escape(toEscape, escaped, url_AppleExtra);
+  bool isEscapedOK = NS_Escape(toEscape, escaped, url_NSURLRef);
  EXPECT_EQ(isEscapedOK, true);
  EXPECT_STREQ(escaped.BeginReading(), "%23");
 }

-// Test that a URL with a '#' within the URL ref component gets escaped
-// correctly when the url_AppleExtra encoding mode is used. Specifically,
-// ensure the second '#' is escaped.
-TEST(Escape, AppleNSURLEscapeURL)
+// Test escaping of URLs that shouldn't be changed by escaping.
+TEST(Escape, AppleNSURLEscapeLists)
 {
-  nsCString toEscape("https://chat.mozilla.org/#/room/#macdev:mozilla.org");
-  nsCString escaped;
-  nsresult rv = NS_GetSpecWithNSURLEncoding(escaped, toEscape);
-  EXPECT_EQ(rv, NS_OK);
-  EXPECT_STREQ(escaped.BeginReading(),
-               "https://chat.mozilla.org/#/room/%23macdev%3Amozilla.org");
-}
+  // Pairs of URLs (un-encoded, encoded)
+  nsTArray<std::pair<nsCString, nsCString>> pairs{
+      {"https://chat.mozilla.org/#/room/#macdev:mozilla.org"_ns,
+       "https://chat.mozilla.org/#/room/%23macdev:mozilla.org"_ns},
+  };

-// Test that '%' encoded characters are not double encoded when used in a URL.
-TEST(Escape, AppleNSURLEscapeURLDouble)
-{
-  const nsCString toEscape(
-      "https://chat.mozilla.org/#/room/%23macdev%3Amozilla.org");
-  nsCString escaped;
-  nsresult rv = NS_GetSpecWithNSURLEncoding(escaped, toEscape);
-  EXPECT_EQ(rv, NS_OK);
-  EXPECT_STREQ(toEscape.BeginReading(), escaped.BeginReading());
+  for (std::pair<nsCString, nsCString>& pair : pairs) {
+    nsCString escaped;
+    nsresult rv = NS_GetSpecWithNSURLEncoding(escaped, pair.first);
+    EXPECT_EQ(rv, NS_OK);
+    EXPECT_STREQ(pair.second.BeginReading(), escaped.BeginReading());
+  }
+
+  // A list of URLs that should not be changed by encoding.
+  nsTArray<nsCString> unchangedURLs{
+      // '=' In the query
+      "https://bugzilla.mozilla.org/show_bug.cgi?id=1737854"_ns,
+      // Escaped character in the fragment
+      "https://html.spec.whatwg.org/multipage/dom.html#the-document%27s-address"_ns,
+      // Misc query
+      "https://www.google.com/search?q=firefox+web+browser&client=firefox-b-1-d&ei=abc&ved=abc&abc=5&oq=firefox+web+browser&gs_lcp=abc&sclient=gws-wiz"_ns,
+      // Check for double encoding. % encoded octals should not be re-encoded.
+      "https://chat.mozilla.org/#/room/%23macdev%3Amozilla.org"_ns,
+      "https://searchfox.org/mozilla-central/search?q=symbol%3AE_%3CT_mozilla%3A%3AWebGLExtensionID%3E_EXT_color_buffer_half_float&path="_ns,
+      // Other
+      "https://site.com/script?foo=bar#this_ref"_ns,
+  };
+
+  for (nsCString& toEscape : unchangedURLs) {
+    nsCString escaped;
+    nsresult rv = NS_GetSpecWithNSURLEncoding(escaped, toEscape);
+    EXPECT_EQ(rv, NS_OK);
+    EXPECT_STREQ(toEscape.BeginReading(), escaped.BeginReading());
+  }
 }
--- a/xpcom/tests/gtest/TestMacNSURLEscaping.mm
+++ b/xpcom/tests/gtest/TestMacNSURLEscaping.mm
@ -0,0 +1,140 @@
+#include "nsCocoaUtils.h"
+#include "nsEscape.h"
+#include "nsNetUtil.h"
+#include "gtest/gtest.h"
+
+#include <CoreFoundation/CoreFoundation.h>
+
+//
+// For the macOS File->Share menu, we must create an NSURL. However, NSURL is
+// more strict than the browser about the encoding of URLs it accepts.
+// Therefore additional encoding must be done on a URL before it is used to
+// create an NSURL object. These tests aim to exercise the code used to
+// perform additional encoding on a URL used to create NSURL objects.
+//
+
+// Ensure nsCocoaUtils::ToNSURL() didn't change the URL.
+// Create an NSURL with the provided string and then read the URL out of
+// the NSURL and test it matches the provided string.
+void ExpectUnchangedByNSURL(nsCString& aEncoded) {
+  NSURL* macURL = nsCocoaUtils::ToNSURL(NS_ConvertUTF8toUTF16(aEncoded));
+  NSString* macURLString = [macURL absoluteString];
+
+  nsString geckoURLString;
+  nsCocoaUtils::GetStringForNSString(macURLString, geckoURLString);
+  EXPECT_STREQ(aEncoded.BeginReading(), NS_ConvertUTF16toUTF8(geckoURLString).get());
+}
+
+// Test escaping of URLs to ensure that
+// 1) We escape URLs in such a way that macOS's NSURL code accepts the URL as
+//    valid.
+// 2) The encoding encoded the expected characters only.
+// 2) NSURL not modify the URL. Check this by reading the URL back out of the
+//    NSURL object and comparing it. If the URL is changed by creating an
+//    NSURL, it may indicate the encoding is incorrect.
+//
+// It is not a requirement that NSURL not change the URL, but we don't
+// expect that for these test cases.
+TEST(NSURLEscaping, NSURLEscapingTests)
+{
+  // Per RFC2396, URI "unreserved" characters. These are allowed in URIs and
+  // can be escaped without changing the semantics of the URI, but the escaping
+  // should only be done if the URI is used in a context that requires it.
+  //
+  //   "-" | "_" | "." | "!" | "~" | "*" | "'" | "(" | ")"
+  //
+  // These are the URI general "reserved" characters. Their reserved purpose
+  // is as delimters so they don't need to be escaped unless used in a URI
+  // component in a way that conflicts with the reserved purpose. i.e.,
+  // whether or not they must be encoded depends on the component.
+  //
+  //   ";" | "/" | "?" | ":" | "@" | "&" | "=" | "+" | "$" | ","
+  //
+  // Characters considered excluded from URI use and should be escaped.
+  // "#" when not used to delimit the start of the fragment identifier.
+  // "%" when not used to escape a character.
+  //
+  //   "<" | ">" | "#" | "%" | <"> | "{" | "}" | "|" | "\" | "^" | "[" | "]" |
+  //   "`"
+
+  // Pairs of URLs of the form (un-encoded, expected encoded result) to verify.
+  nsTArray<std::pair<nsCString, nsCString>> pairs{
+      {
+          // '#' in ref
+         "https://chat.mozilla.org/#/room/#macdev:mozilla.org"_ns,
+         "https://chat.mozilla.org/#/room/%23macdev:mozilla.org"_ns},
+      {
+          // '"' in ref
+          "https://example.com/path#ref_with_#_and\""_ns,
+          "https://example.com/path#ref_with_%23_and%22"_ns,
+      },
+      {
+          // '[]{}|' in ref
+          "https://example.com/path#ref_with_[foo]_{and}_|"_ns,
+          "https://example.com/path#ref_with_%5Bfoo%5D_%7Band%7D_%7C"_ns,
+      },
+      {
+          // Unreserved characters in path, query, and ref
+          "https://example.com/path-_.!~&'(x)?x=y&x=z-_.!~&'(x)#ref-_.!~&'(x)"_ns,
+          "https://example.com/path-_.!~&'(x)?x=y&x=z-_.!~&%27(x)#ref-_.!~&'(x)"_ns,
+      },
+      {
+          // All excluded characters in the ref.
+          "https://example.com/path#ref \"#<>[]\\^`{|}ref"_ns,
+          "https://example.com/path#ref%20%22%23%3C%3E%5B%5D%5C%5E%60%7B%7C%7Dref"_ns,
+      },
+      /*
+       * Bug 1739533:
+       * This test fails because the '%' character needs to be escaped before
+       * the URI is passed to [NSURL URLWithString]. '<' brackets are
+       * already escaped by the browser to their "%xx" form so the encoding must
+       * be added in a way that ignores characters already % encoded "%xx".
+      {
+        // Unreserved characters in path, query, and ref
+        // https://example.com/path/with<more>/%/and"#frag_with_#_and"
+        "https://example.com/path/with<more>/%/and\"#frag_with_#_and\""_ns,
+        "https://example.com/path/with%3Cmore%3E/%25/and%22#frag_with_%23_and%22"_ns,
+      },
+      */
+  };
+
+  for (std::pair<nsCString, nsCString>& pair : pairs) {
+    nsCString escaped;
+    nsresult rv = NS_GetSpecWithNSURLEncoding(escaped, pair.first);
+    EXPECT_EQ(rv, NS_OK);
+    EXPECT_STREQ(pair.second.BeginReading(), escaped.BeginReading());
+    ExpectUnchangedByNSURL(escaped);
+  }
+
+  // A list of URLs that should not be changed by encoding.
+  nsTArray<nsCString> unchangedURLs{
+      // '=' In the query
+      "https://bugzilla.mozilla.org/show_bug.cgi?id=1737854"_ns,
+      "https://bugzilla.mozilla.org/show_bug.cgi?id=1737854#ref"_ns,
+      "https://bugzilla.mozilla.org/allinref#show_bug.cgi?id=1737854ref"_ns,
+      "https://example.com/script?foo=bar#this_ref"_ns,
+      // Escaped character in the ref
+      "https://html.spec.whatwg.org/multipage/dom.html#the-document%27s-address"_ns,
+      // Misc query
+      "https://www.google.com/search?q=firefox+web+browser&client=firefox-b-1-d&ei=abc&ved=abc&abc=5&oq=firefox+web+browser&gs_lcp=abc&sclient=gws-wiz"_ns,
+      // Check for double encoding. % encoded octals should not be re-encoded.
+      "https://chat.mozilla.org/#/room/%23macdev%3Amozilla.org"_ns,
+      "https://searchfox.org/mozilla-central/search?q=symbol%3AE_%3CT_mozilla%3A%3AWebGLExtensionID%3E_EXT_color_buffer_half_float&path="_ns,
+      // Unreserved and reserved that don't need encoding in ref.
+      "https://example.com/path#ref!$&'(foo),:;=?@~"_ns,
+      // Unreserved and reserved that don't need encoding in path.
+      "https://example.com/path-_.!~&'(x)#ref"_ns,
+      // Unreserved and reserved that don't need encoding in path and ref.
+      "https://example.com/path-_.!~&'(x)#ref-_.!~&'(x)"_ns,
+      // Reserved in query.
+      "https://example.com/path?a=b&;=/&/=?&@=a+b,$"_ns,
+  };
+
+  for (nsCString& toEscape : unchangedURLs) {
+    nsCString escaped;
+    nsresult rv = NS_GetSpecWithNSURLEncoding(escaped, toEscape);
+    EXPECT_EQ(rv, NS_OK);
+    EXPECT_STREQ(toEscape.BeginReading(), escaped.BeginReading());
+    ExpectUnchangedByNSURL(escaped);
+  }
+}
--- a/xpcom/tests/gtest/moz.build
+++ b/xpcom/tests/gtest/moz.build
@ -117,6 +117,7 @@ else:
 if CONFIG["OS_TARGET"] == "Darwin":
    UNIFIED_SOURCES += [
        "TestAvailableMemoryWatcherMac.cpp",
+        "TestMacNSURLEscaping.mm",
    ]

 if (