From 3ef03e903e21bab3db9ff191f0ce9fc7e90944f9 Mon Sep 17 00:00:00 2001
From: Phillis Tang <phillis@chromium.org>
Date: Thu, 22 Jul 2021 00:25:01 +0000
Subject: [PATCH] DPWA: use ParseURL for manifest id parsing logic

Use ParseURL for manifest id parsing logic. This allows id to be both
specified as a full url or a string as path to app's origin. This allows
a more consistent spec processing definition, see in spec pull request
https://github.com/w3c/manifest/pull/988#discussion_r671009317. The
processed manifest id is still passed as a  normalized relative url path
to WebAppProvider system to keep the manifest_id saved in sync system
the same and backward-compatible since changing the manifest_id format
in sync system will make new apps not syncable to previous versions of
Chromium.

Bug: 1182363
Change-Id: I65a7ca615bae3ee504b605abba954f60dc5d2c31
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3042714
Commit-Queue: Phillis Tang <phillis@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Cr-Commit-Position: refs/heads/master@{#904128}
---
 .../modules/manifest/manifest_parser.cc       | 30 ++++++++-------
 .../manifest/manifest_parser_unittest.cc      | 37 +++++++++++++++++++
 2 files changed, 54 insertions(+), 13 deletions(-)

diff --git a/third_party/blink/renderer/modules/manifest/manifest_parser.cc b/third_party/blink/renderer/modules/manifest/manifest_parser.cc
index 0d7115d5fdbbb2..0885423c3eb477 100644
--- a/third_party/blink/renderer/modules/manifest/manifest_parser.cc
+++ b/third_party/blink/renderer/modules/manifest/manifest_parser.cc
@@ -340,23 +340,27 @@ String ManifestParser::ParseId(const JSONObject* object,
     return String();
   }
 
-  absl::optional<String> id = ParseString(object, "id", NoTrim);
-  if (id.has_value()) {
+  if (!start_url.IsValid()) {
+    ManifestUmaUtil::ParseIdResult(
+        ManifestUmaUtil::ParseIdResultType::kInvalidStartUrl);
+    return String();
+  }
+  KURL start_url_origin = KURL(SecurityOrigin::Create(start_url)->ToString());
+
+  KURL id = ParseURL(object, "id", start_url_origin,
+                     ParseURLRestrictions::kSameOriginOnly);
+  if (id.IsValid()) {
     ManifestUmaUtil::ParseIdResult(
         ManifestUmaUtil::ParseIdResultType::kSucceed);
-    return *id;
   } else {
-    // If id is not specified, sets to start_url with origin stripped.
-    if (start_url.IsValid()) {
-      ManifestUmaUtil::ParseIdResult(
-          ManifestUmaUtil::ParseIdResultType::kDefaultToStartUrl);
-      return start_url.GetString().Substring(start_url.PathStart() + 1);
-    } else {
-      ManifestUmaUtil::ParseIdResult(
-          ManifestUmaUtil::ParseIdResultType::kInvalidStartUrl);
-      return String();
-    }
+    // If id is not specified, sets to start_url
+    ManifestUmaUtil::ParseIdResult(
+        ManifestUmaUtil::ParseIdResultType::kDefaultToStartUrl);
+    id = start_url;
   }
+  // TODO(https://crbug.com/1231765): rename the field to relative_id to reflect
+  // the actual value.
+  return id.GetString().Substring(id.PathStart() + 1);
 }
 
 KURL ManifestParser::ParseStartURL(const JSONObject* object) {
diff --git a/third_party/blink/renderer/modules/manifest/manifest_parser_unittest.cc b/third_party/blink/renderer/modules/manifest/manifest_parser_unittest.cc
index 3eb707b1b72638..5faaaf301bfe64 100644
--- a/third_party/blink/renderer/modules/manifest/manifest_parser_unittest.cc
+++ b/third_party/blink/renderer/modules/manifest/manifest_parser_unittest.cc
@@ -274,6 +274,13 @@ TEST_F(ManifestParserTest, IdParseRules) {
     ASSERT_EQ(0u, GetErrorCount());
     EXPECT_EQ("start?query=a", manifest->id);
   }
+  // Invalid type.
+  {
+    auto& manifest =
+        ParseManifest("{\"start_url\": \"/start?query=a\", \"id\": 1}");
+    ASSERT_EQ(1u, GetErrorCount());
+    EXPECT_EQ("start?query=a", manifest->id);
+  }
   // Empty string.
   {
     auto& manifest =
@@ -281,6 +288,36 @@ TEST_F(ManifestParserTest, IdParseRules) {
     ASSERT_EQ(0u, GetErrorCount());
     EXPECT_EQ("", manifest->id);
   }
+  // Full url.
+  {
+    auto& manifest = ParseManifest(
+        "{ \"start_url\": \"/start?query=a\", \"id\": \"http://foo.com/foo\" "
+        "}");
+    ASSERT_EQ(0u, GetErrorCount());
+    EXPECT_EQ("foo", manifest->id);
+  }
+  // Full url with different origin.
+  {
+    auto& manifest = ParseManifest(
+        "{ \"start_url\": \"/start?query=a\", \"id\": "
+        "\"http://another.com/foo\" }");
+    ASSERT_EQ(1u, GetErrorCount());
+    EXPECT_EQ("start?query=a", manifest->id);
+  }
+  // Relative path
+  {
+    auto& manifest =
+        ParseManifest("{ \"start_url\": \"/start?query=a\", \"id\": \".\" }");
+    ASSERT_EQ(0u, GetErrorCount());
+    EXPECT_EQ("", manifest->id);
+  }
+  // Absolute path
+  {
+    auto& manifest =
+        ParseManifest("{ \"start_url\": \"/start?query=a\", \"id\": \"/\" }");
+    ASSERT_EQ(0u, GetErrorCount());
+    EXPECT_EQ("", manifest->id);
+  }
   // Smoke test.
   {
     auto& manifest =