Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2811903002:
[HBD] Tighten-up which adobe.com/go links are intercepted as Flash URLs (Closed)
Description[HBD] Tighten-up which adobe.com/go links are intercepted as Flash URLs
BUG=709292
Review-Url: https://codereview.chromium.org/2811903002
Cr-Commit-Position: refs/heads/master@{#464039}
Committed: https://chromium.googlesource.com/chromium/src/+/3a3243c913d98568a9b0edb4018ad3275ae2a8f0
Patch Set 1 : fix #
Total comments: 9
Patch Set 2 : fix #
Messages
Total messages: 20 (11 generated)
Patchset #1 (id:1) has been deleted
tommycli@chromium.org changed reviewers: + raymes@chromium.org
raymes: PTAL. I verified that the master list of golinks still are all caught by the new tightened regex https://codereview.chromium.org/2811903002/diff/20001/chrome/browser/plugins/... File chrome/browser/plugins/flash_download_interception_unittest.cc (left): https://codereview.chromium.org/2811903002/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/flash_download_interception_unittest.cc:61: "https://www.example.com", git cl format wanted to collaspe these lines...
lgtm but would it make sense to add a few more test URLs? https://codereview.chromium.org/2811903002/diff/20001/chrome/browser/plugins/... File chrome/browser/plugins/flash_download_interception.cc (right): https://codereview.chromium.org/2811903002/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/flash_download_interception.cc:36: "((?i).*get[-_]?flash|getfp10android|.*fl(ash)player|.*flashpl|" is the (?i) needed here? https://codereview.chromium.org/2811903002/diff/20001/chrome/browser/plugins/... File chrome/browser/plugins/flash_download_interception_unittest.cc (right): https://codereview.chromium.org/2811903002/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/flash_download_interception_unittest.cc:53: "http://adobe.com/go/gntray_dl_getflashplayer_jp", I feel like we could add a few more URLs here to match cases that were made more specific in this CL, e.g. the h-m-a case?
Hi, A short comment on the inner workings of these download URLs - maybe it saves a headache or two: The endpoints at www.adobe.com/go/ and www.macromedia.com/go (both HTTP and HTTPS) act as nothing more than a URL shortener / redirection, with the target of the navigation depending only on the path segment after /go/ e.g. https://www.adobe.com/go/THIS_POINTS_TO_THE_REDIRECT_TARGET?while=these&have=... . My point is: the proper fix[1] would be to base the Flash interception on just the host + path part of the URL, ignoring the query & fragment. I don't know how taxing that would be performance wise, but functionally, that would be a fix that really plugs the hole instead of just decreasing a lot the likelihood of this bug popping up again in the wild. [1] Given that these "go URLs" are used throughout online documents and deployed applications, I think it's reasonable to assume the format (or the semantics) for these URLs won't change for the forseeable future. I would even go out on a limb and wager that these "go URLs" are actually used since before Macromedia was acquired by Adobe - which can be an indicator to the expected lifetime of this mechanism for the time being :) Thanks, Mihai - signed in with my personal email, but commenting on behalf of Adobe
mihai.balan@gmail.com changed reviewers: + mihai.balan@gmail.com
https://codereview.chromium.org/2811903002/diff/20001/chrome/browser/plugins/... File chrome/browser/plugins/flash_download_interception_unittest.cc (right): https://codereview.chromium.org/2811903002/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/flash_download_interception_unittest.cc:52: "http://adobe.com/go/CA-H-GET-FLASH", The code as it is now would still intercept https://www.macromedia.com/go/gnavtray_myadobe_myinformation_en_us?ua=chrome-... -- which isn't exactly the intention, as far as I can tell
Submitted a code suggestion that implements my previous free-form comment https://codereview.chromium.org/2811903002/diff/20001/chrome/browser/plugins/... File chrome/browser/plugins/flash_download_interception.cc (left): https://codereview.chromium.org/2811903002/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/flash_download_interception.cc:97: std::string target_url_str = target_url.GetContent(); Would this address the regex matching parts of the query / fragments? url::Replacements<char> replacements; replacements.ClearQuery(); replacements.ClearRef(); replacements.ClearUsername(); replacements.ClearPassword(); std::string target_url_str = target_url.ReplaceComponents(replacements).GetContent();
Thanks guys! hopefully the below addresses your feedback. https://codereview.chromium.org/2811903002/diff/20001/chrome/browser/plugins/... File chrome/browser/plugins/flash_download_interception.cc (left): https://codereview.chromium.org/2811903002/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/flash_download_interception.cc:97: std::string target_url_str = target_url.GetContent(); On 2017/04/11 06:44:14, mihai.balan wrote: > Would this address the regex matching parts of the query / fragments? > > url::Replacements<char> replacements; > replacements.ClearQuery(); > replacements.ClearRef(); > replacements.ClearUsername(); > replacements.ClearPassword(); > std::string target_url_str = > target_url.ReplaceComponents(replacements).GetContent(); Done. Awesome suggestion. I used this basically verbatim. https://codereview.chromium.org/2811903002/diff/20001/chrome/browser/plugins/... File chrome/browser/plugins/flash_download_interception.cc (right): https://codereview.chromium.org/2811903002/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/flash_download_interception.cc:36: "((?i).*get[-_]?flash|getfp10android|.*fl(ash)player|.*flashpl|" On 2017/04/10 23:54:13, raymes wrote: > is the (?i) needed here? Yes, because both 'flashdownload' and 'FlashDownload' appear in the list of golinks. It seemed better to use case-insensitive matching rather than calling out the case-variations specifically. https://codereview.chromium.org/2811903002/diff/20001/chrome/browser/plugins/... File chrome/browser/plugins/flash_download_interception_unittest.cc (right): https://codereview.chromium.org/2811903002/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/flash_download_interception_unittest.cc:52: "http://adobe.com/go/CA-H-GET-FLASH", On 2017/04/11 06:18:12, mihai.balan wrote: > The code as it is now would still intercept > https://www.macromedia.com/go/gnavtray_myadobe_myinformation_en_us?ua=chrome-... > -- which isn't exactly the intention, as far as I can tell Done. I added an explicit check to avoid intercepting this. https://codereview.chromium.org/2811903002/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/flash_download_interception_unittest.cc:53: "http://adobe.com/go/gntray_dl_getflashplayer_jp", On 2017/04/10 23:54:13, raymes wrote: > I feel like we could add a few more URLs here to match cases that were made more > specific in this CL, e.g. the h-m-a case? Done. I added the h-m-a case, but we specifically want to be cautious and not leak all of Adobe's URLs (according to laforge). I internally test with the whole list.
The CQ bit was checked by tommycli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by tommycli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org Link to the patchset: https://codereview.chromium.org/2811903002/#ps40001 (title: "fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492011528293750,
"parent_rev": "08f24841310c677a67f79f89253d971820069da4", "commit_rev":
"3a3243c913d98568a9b0edb4018ad3275ae2a8f0"}
Message was sent while issue was closed.
Description was changed from ========== [HBD] Tighten-up which adobe.com/go links are intercepted as Flash URLs BUG=709292 ========== to ========== [HBD] Tighten-up which adobe.com/go links are intercepted as Flash URLs BUG=709292 Review-Url: https://codereview.chromium.org/2811903002 Cr-Commit-Position: refs/heads/master@{#464039} Committed: https://chromium.googlesource.com/chromium/src/+/3a3243c913d98568a9b0edb4018a... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001) as https://chromium.googlesource.com/chromium/src/+/3a3243c913d98568a9b0edb4018a... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
