Changeset 228036 in webkit

Timestamp:

Feb 2, 2018 4:49:56 PM (6 years ago)

Author:

david_quesada@apple.com

Message:

WebAppManifest scope should default to the containing directory of start_url if 'scope' is not specified
​https://bugs.webkit.org/show_bug.cgi?id=182363
rdar://problem/37093498

Reviewed by Ryosuke Niwa.

Source/WebCore:

If an app manifest doesn't specify a scope, we should default to the "parent directory" of
the start URL, rather than leaving the app unbounded. This is more reasonable than using the
entire internet as the app scope.

No new tests, updates to the existing tests verify the new behavior.

• Modules/applicationmanifest/ApplicationManifestParser.cpp:

(WebCore::ApplicationManifestParser::parseScope):

Tools:

Updated ApplicationManifestParserTests to account for the new default scope behavior.

• TestWebKitAPI/Tests/WebCore/ApplicationManifestParser.cpp:

(assertManifestHasDefaultValues):
(TEST_F):

Location:

trunk

Files:

• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp (modified) (2 diffs)
• Tools/ChangeLog (modified) (1 diff)
• Tools/TestWebKitAPI/Tests/WebCore/ApplicationManifestParser.cpp (modified) (2 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2018-02-02  David Quesada  <david_quesada@apple.com>
+
+        WebAppManifest scope should default to the containing directory of start_url if 'scope' is not specified
+        https://bugs.webkit.org/show_bug.cgi?id=182363
+        rdar://problem/37093498
+
+        Reviewed by Ryosuke Niwa.
+
+        If an app manifest doesn't specify a scope, we should default to the "parent directory" of
+        the start URL, rather than leaving the app unbounded. This is more reasonable than using the
+        entire internet as the app scope.
+
+        No new tests, updates to the existing tests verify the new behavior.
+
+        * Modules/applicationmanifest/ApplicationManifestParser.cpp:
+        (WebCore::ApplicationManifestParser::parseScope):
+
 2018-02-02  Youenn Fablet  <youenn@apple.com>
 

trunk/Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp

@@ -194 +194 @@
 URL ApplicationManifestParser::parseScope(const JSON::Object& manifest, const URL& documentURL, const URL& startURL)
 {
+    URL defaultScope { startURL, "./" };
+
     RefPtr<JSON::Value> value;
     if (!manifest.getValue("scope", value))
-        return { };
+        return defaultScope;
 
     String stringValue;
     if (!value->asString(stringValue)) {
         logManifestPropertyNotAString(ASCIILiteral("scope"));
-        return { };
+        return defaultScope;
     }
 
     if (stringValue.isEmpty())
-        return { };
+        return defaultScope;
 
     URL scopeURL(m_manifestURL, stringValue);
     if (!scopeURL.isValid()) {
         logManifestPropertyInvalidURL(ASCIILiteral("scope"));
-        return { };
+        return defaultScope;
     }
 
@@ -217 +219 @@
         auto documentOrigin = SecurityOrigin::create(documentURL);
         logDeveloperWarning(ASCIILiteral("The scope's origin of \"") + scopeURLOrigin->toString() + ASCIILiteral("\" is different from the document's origin of \"") + documentOrigin->toString() + ASCIILiteral("\"."));
-        return { };
+        return defaultScope;
     }
 
     if (!isInScope(scopeURL, startURL)) {
         logDeveloperWarning(ASCIILiteral("The start URL is not within scope of the provided scope URL."));
-        return { };
+        return defaultScope;
     }
 

trunk/Tools/ChangeLog

@@ +1 @@
+2018-02-02  David Quesada  <david_quesada@apple.com>
+
+        WebAppManifest scope should default to the containing directory of start_url if 'scope' is not specified
+        https://bugs.webkit.org/show_bug.cgi?id=182363
+        rdar://problem/37093498
+
+        Reviewed by Ryosuke Niwa.
+
+        Updated ApplicationManifestParserTests to account for the new default scope behavior.
+
+        * TestWebKitAPI/Tests/WebCore/ApplicationManifestParser.cpp:
+        (assertManifestHasDefaultValues):
+        (TEST_F):
+
 2018-02-02  Youenn Fablet  <youenn@apple.com>
 

trunk/Tools/TestWebKitAPI/Tests/WebCore/ApplicationManifestParser.cpp

@@ -136 +136 @@
     EXPECT_TRUE(manifest.shortName.isNull());
     EXPECT_TRUE(manifest.description.isNull());
-    EXPECT_TRUE(manifest.scope.isEmpty());
-    EXPECT_STREQ(manifest.startURL.string().utf8().data(), documentURL.string().utf8().data());
+    EXPECT_STREQ("https://example.com/", manifest.scope.string().utf8().data());
+    EXPECT_STREQ(documentURL.string().utf8().data(), manifest.startURL.string().utf8().data());
 }
 
@@ -248 +248 @@
 TEST_F(ApplicationManifestParserTest, Scope)
 {
-    testScope("123", String());
-    testScope("null", String());
-    testScope("true", String());
-    testScope("{ }", String());
-    testScope("[ ]", String());
-    testScope("\"\"", String());
-
-    testScope("\"http:?\"", String());
-
-    // If scope URL is not same origin as document URL, return undefined.
+    // If the scope is not a string or not a valid URL, return the default scope (the parent path of the start URL).
+    m_documentURL = { { }, "https://example.com/a/page?queryParam=value#fragment" };
+    m_manifestURL = { { }, "https://example.com/manifest.json" };
+    testScope("123", "https://example.com/a/");
+    testScope("null", "https://example.com/a/");
+    testScope("true", "https://example.com/a/");
+    testScope("{ }", "https://example.com/a/");
+    testScope("[ ]", "https://example.com/a/");
+    testScope("\"\"", "https://example.com/a/");
+    testScope("\"http:?\"", "https://example.com/a/");
+
+    m_documentURL = { { }, "https://example.com/a/pageEndingWithSlash/" };
+    testScope("null", "https://example.com/a/pageEndingWithSlash/");
+
+    // If scope URL is not same origin as document URL, return the default scope.
     m_documentURL = { { }, "https://example.com/home" };
     m_manifestURL = { { }, "https://other-site.com/manifest.json" };
-    testScope("\"https://other-site.com/some-scope\"", String());
+    testScope("\"https://other-site.com/some-scope\"", "https://example.com/");
 
     m_documentURL = { { }, "https://example.com/app/home" };
     m_manifestURL = { { }, "https://example.com/app/manifest.json" };
 
-    // If start URL is not within scope of scope URL, return undefined
-    testScope("\"https://example.com/subdirectory\"", String());
+    // If start URL is not within scope of scope URL, return the default scope.
+    testScope("\"https://example.com/subdirectory\"", "https://example.com/app/");
     testScope("\"https://example.com/app\"", "https://example.com/app");
-    testScope("\"https://example.com/APP\"", String());
+    testScope("\"https://example.com/APP\"", "https://example.com/app/");
     testScope("\"https://example.com/a\"", "https://example.com/a");