Web Share: Validate URL template at manifest parse time, not share time |
|||
Issue descriptionCurrently there are two conditions for an invalid share_target.url_template field of the manifest: 1. The placeholder syntax is invalid (e.g., mismatched braces). 2. The URL itself is invalid (e.g., contains NUL characters or invalid UTF-8 characters). That is, GURL::is_valid() returns false on the final target URL. Currently both of these are validated at SHARE time which means we can store an invalid url_template from a manifest and when you choose it as a share target, the navigator.share promise fails. It should be validated when the manifest is parsed, with a warning logged to the console, and the target not being added to the list of share targets at all. (Note: Condition #2 is very hard, if not impossible, to trigger in practice. NUL characters (i.e., "\u0000") seem to disappear somewhere along the line, and don't appear in the URL as seen by ShareServiceImpl. Invalid UTF-8 characters aren't even possible in JSON. We could just convert that runtime check into a DCHECK since I don't think it's possible to trigger.)
,
Mar 2 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b1903b6f84d63b24d1522ca836f53557d06704f1 commit b1903b6f84d63b24d1522ca836f53557d06704f1 Author: Peter Kotwicz <pkotwicz@chromium.org> Date: Fri Mar 02 02:36:19 2018 [Android WebAPK] Share target - Replace placeholders only in query and fragment This CL changes the WebAPK's processing of share intents. The CL restricts replacing placeholders in the share target template URL to the query and fragment portions of the URL only The new behavior matches the web share target spec https://wicg.github.io/web-share-target/ Bug= 694380 Change-Id: Ib1d96fea0c322b1a5d0c6e0aa6a8d83b8a0143cd Reviewed-on: https://chromium-review.googlesource.com/916502 Commit-Queue: Peter Kotwicz <pkotwicz@chromium.org> Reviewed-by: Matt Giuca <mgiuca@chromium.org> Reviewed-by: Yaron Friedman <yfriedman@chromium.org> Cr-Commit-Position: refs/heads/master@{#540421} [modify] https://crrev.com/b1903b6f84d63b24d1522ca836f53557d06704f1/chrome/android/webapk/shell_apk/shell_apk_version.gni [modify] https://crrev.com/b1903b6f84d63b24d1522ca836f53557d06704f1/chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ShareActivity.java
,
Mar 4 2018
,
Mar 7 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3e8cb129c0f1d0bfe1e5f432c13ebb72561ad180 commit 3e8cb129c0f1d0bfe1e5f432c13ebb72561ad180 Author: Peter Kotwicz <pkotwicz@chromium.org> Date: Wed Mar 07 17:12:28 2018 [WebShare] Only parse placeholders in the query and fragment part 1 This CL parses the url_template as a GURL prior to substituting the placeholders. Parsing the url_template a GURL causes '{' and '}' in the url_template path to be escaped, and thus stops the ReplacePlaceholders() algorithm from replacing placeholders in the url_template path. Parsing the url_template as a GURL does not affect how placeholders in the URL query and URL fragment are treated. This change is in line with share target spec https://wicg.github.io/web-share-target/ A follow up CL (part 2) will make the fact that ReplacePlaceholders() only substitutes placeholders in the query and fragment more explicit. URL spec reference: https://url.spec.whatwg.org/#query-state '{' and '}' - are in the path-percent-encode set https://url.spec.whatwg.org/#path-percent-encode-set - are not in the fragment-percent-encode set https://url.spec.whatwg.org/#fragment-percent-encode-set - should not be percent encoded in the URL query https://url.spec.whatwg.org/#query-state BUG= 694380 Change-Id: I0e3bcefa6cd9bc3997caf54ad824b6bfc87b9e88 Reviewed-on: https://chromium-review.googlesource.com/923539 Reviewed-by: Robert Sesek <rsesek@chromium.org> Reviewed-by: Mounir Lamouri <mlamouri@chromium.org> Reviewed-by: Dominick Ng <dominickn@chromium.org> Reviewed-by: Matt Giuca <mgiuca@chromium.org> Reviewed-by: John Abd-El-Malek <jam@chromium.org> Commit-Queue: Peter Kotwicz <pkotwicz@chromium.org> Cr-Commit-Position: refs/heads/master@{#541478} [modify] https://crrev.com/3e8cb129c0f1d0bfe1e5f432c13ebb72561ad180/chrome/browser/android/shortcut_info.cc [modify] https://crrev.com/3e8cb129c0f1d0bfe1e5f432c13ebb72561ad180/chrome/browser/android/shortcut_info.h [modify] https://crrev.com/3e8cb129c0f1d0bfe1e5f432c13ebb72561ad180/chrome/browser/android/webapk/webapk_installer.cc [modify] https://crrev.com/3e8cb129c0f1d0bfe1e5f432c13ebb72561ad180/chrome/browser/ui/views/webshare/webshare_target_picker_view_unittest.cc [modify] https://crrev.com/3e8cb129c0f1d0bfe1e5f432c13ebb72561ad180/chrome/browser/webshare/share_service_impl.cc [modify] https://crrev.com/3e8cb129c0f1d0bfe1e5f432c13ebb72561ad180/chrome/browser/webshare/share_service_impl_unittest.cc [modify] https://crrev.com/3e8cb129c0f1d0bfe1e5f432c13ebb72561ad180/chrome/browser/webshare/share_target_pref_helper.cc [modify] https://crrev.com/3e8cb129c0f1d0bfe1e5f432c13ebb72561ad180/chrome/browser/webshare/share_target_pref_helper_unittest.cc [modify] https://crrev.com/3e8cb129c0f1d0bfe1e5f432c13ebb72561ad180/chrome/browser/webshare/webshare_target.cc [modify] https://crrev.com/3e8cb129c0f1d0bfe1e5f432c13ebb72561ad180/chrome/browser/webshare/webshare_target.h [modify] https://crrev.com/3e8cb129c0f1d0bfe1e5f432c13ebb72561ad180/content/public/common/manifest.h [modify] https://crrev.com/3e8cb129c0f1d0bfe1e5f432c13ebb72561ad180/content/public/common/manifest_struct_traits.cc [modify] https://crrev.com/3e8cb129c0f1d0bfe1e5f432c13ebb72561ad180/content/public/common/manifest_struct_traits.h [modify] https://crrev.com/3e8cb129c0f1d0bfe1e5f432c13ebb72561ad180/content/renderer/manifest/manifest_parser.cc [modify] https://crrev.com/3e8cb129c0f1d0bfe1e5f432c13ebb72561ad180/content/renderer/manifest/manifest_parser.h [modify] https://crrev.com/3e8cb129c0f1d0bfe1e5f432c13ebb72561ad180/content/renderer/manifest/manifest_parser_unittest.cc [modify] https://crrev.com/3e8cb129c0f1d0bfe1e5f432c13ebb72561ad180/third_party/WebKit/public/platform/modules/manifest/manifest.mojom
,
Mar 8 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/20f515ccc63480fe265b1560b80b701d43fe006b commit 20f515ccc63480fe265b1560b80b701d43fe006b Author: Peter Kotwicz <pkotwicz@chromium.org> Date: Thu Mar 08 01:15:02 2018 [WebShare] Only parse placeholders in the query and fragment part 2 This CL refactors ReplacePlaceholders() to make it clearer that only placeholders in the url_template query and fragment are replaced. BUG= 694380 , 788191 Change-Id: I62672ae9e8386ef53ff532cf20a0d312fcba600f Reviewed-on: https://chromium-review.googlesource.com/925648 Commit-Queue: Peter Kotwicz <pkotwicz@chromium.org> Reviewed-by: Matt Giuca <mgiuca@chromium.org> Cr-Commit-Position: refs/heads/master@{#541673} [modify] https://crrev.com/20f515ccc63480fe265b1560b80b701d43fe006b/chrome/browser/webshare/share_service_impl.cc [modify] https://crrev.com/20f515ccc63480fe265b1560b80b701d43fe006b/chrome/browser/webshare/share_service_impl.h [modify] https://crrev.com/20f515ccc63480fe265b1560b80b701d43fe006b/chrome/browser/webshare/share_service_impl_unittest.cc
,
Mar 16 2018
,
Mar 16 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/89d5601fd746b9db3f4847aae24aa09b65b48caf commit 89d5601fd746b9db3f4847aae24aa09b65b48caf Author: Peter Kotwicz <pkotwicz@chromium.org> Date: Fri Mar 16 18:15:21 2018 [WebShare] Check share target url_template syntax at parse time Invalid share targets will now be ignored, rather than showing up in the picker and being rejected at launch time. This brings the implementation in line with the spec: https://wicg.github.io/web-share-target/ This CL: - Makes manifest_parser.cc check the share target url_template placeholder syntax. - Refactors ReplacePlaceholders() so that it does no string copying when called with url_template_filled==nullptr BUG= 694380 , 788191 Change-Id: Icd2a5af5e3100fd93a3fa3cd9ba00a23905355b7 Reviewed-on: https://chromium-review.googlesource.com/927581 Commit-Queue: Peter Kotwicz <pkotwicz@chromium.org> Reviewed-by: John Abd-El-Malek <jam@chromium.org> Reviewed-by: Matt Giuca <mgiuca@chromium.org> Reviewed-by: Mounir Lamouri <mlamouri@chromium.org> Cr-Commit-Position: refs/heads/master@{#543763} [modify] https://crrev.com/89d5601fd746b9db3f4847aae24aa09b65b48caf/chrome/browser/webshare/share_service_impl.cc [modify] https://crrev.com/89d5601fd746b9db3f4847aae24aa09b65b48caf/content/common/manifest_share_target_util_unittest.cc [modify] https://crrev.com/89d5601fd746b9db3f4847aae24aa09b65b48caf/content/public/common/manifest_share_target_util.cc [modify] https://crrev.com/89d5601fd746b9db3f4847aae24aa09b65b48caf/content/public/common/manifest_share_target_util.h [modify] https://crrev.com/89d5601fd746b9db3f4847aae24aa09b65b48caf/content/renderer/manifest/manifest_parser.cc [modify] https://crrev.com/89d5601fd746b9db3f4847aae24aa09b65b48caf/content/renderer/manifest/manifest_parser_unittest.cc |
|||
►
Sign in to add a comment |
|||
Comment 1 by bugdroid1@chromium.org
, Feb 21 2017