Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2927123002:
Ensure that the referrer for HTTPS to HTTP downloads is dropped. (Closed)
|
Created:
3 years, 6 months ago by svaldez Modified:
3 years, 6 months ago Reviewers:
David Trainor- moved to gerrit CC:
chromium-reviews, jam, darin-cc_chromium.org, jochen (gone - plz use gerrit) Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnsure that the referrer for HTTPS to HTTP downloads is dropped.
BUG=730877
Review-Url: https://codereview.chromium.org/2927123002
Cr-Commit-Position: refs/heads/master@{#478255}
Committed: https://chromium.googlesource.com/chromium/src/+/da5f4e24e21981cc72d2cf2baf3af78eefc79279
Patch Set 1 #
Total comments: 3
Messages
Total messages: 17 (9 generated)
svaldez@chromium.org changed reviewers: + asanka@chromium.org
The CQ bit was checked by svaldez@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...
asanka@chromium.org changed reviewers: + dtrainor@chromium.org - asanka@chromium.org
-> David. (Let me know if there's someone else I can load-balance with :) I removed myself as a downloads OWNER recently. Passing the buck over to the new management. https://codereview.chromium.org/2927123002/diff/1/content/browser/download/ur... File content/browser/download/url_downloader.cc (right): https://codereview.chromium.org/2927123002/diff/1/content/browser/download/ur... content/browser/download/url_downloader.cc:71: Referrer::SanitizeForRequest(request->url(), referrer); Whoa. This step is pretty easy to miss.
lgtm with API suggestion https://codereview.chromium.org/2927123002/diff/1/content/browser/download/ur... File content/browser/download/url_downloader.cc (right): https://codereview.chromium.org/2927123002/diff/1/content/browser/download/ur... content/browser/download/url_downloader.cc:71: Referrer::SanitizeForRequest(request->url(), referrer); On 2017/06/08 16:44:30, asanka wrote: > Whoa. This step is pretty easy to miss. Should we have a bool sanitize in SetReferrerForRequest? That way callers have to explicitly declare whether or not they want the sanitization?
On 2017/06/08 16:44:30, asanka wrote: > -> David. (Let me know if there's someone else I can load-balance with :) > > I removed myself as a downloads OWNER recently. Passing the buck over to the new > management. > > https://codereview.chromium.org/2927123002/diff/1/content/browser/download/ur... > File content/browser/download/url_downloader.cc (right): > > https://codereview.chromium.org/2927123002/diff/1/content/browser/download/ur... > content/browser/download/url_downloader.cc:71: > Referrer::SanitizeForRequest(request->url(), referrer); > Whoa. This step is pretty easy to miss. Min would be a good alternative if you end up getting a bunch and want to balance. Thanks for asking! :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2927123002/diff/1/content/browser/download/ur... File content/browser/download/url_downloader.cc (right): https://codereview.chromium.org/2927123002/diff/1/content/browser/download/ur... content/browser/download/url_downloader.cc:71: Referrer::SanitizeForRequest(request->url(), referrer); On 2017/06/08 17:16:38, David Trainor-OOO to 6-8 wrote: > On 2017/06/08 16:44:30, asanka wrote: > > Whoa. This step is pretty easy to miss. > > Should we have a bool sanitize in SetReferrerForRequest? That way callers have > to explicitly declare whether or not they want the sanitization? Hmm, it looks like most other callers sanitize it early in the pipeline, so most wouldn't be calling for sanitization, but it might be useful just as a fallback in case. Should that be a separate CL?
On 2017/06/08 18:26:07, svaldez wrote: > https://codereview.chromium.org/2927123002/diff/1/content/browser/download/ur... > File content/browser/download/url_downloader.cc (right): > > https://codereview.chromium.org/2927123002/diff/1/content/browser/download/ur... > content/browser/download/url_downloader.cc:71: > Referrer::SanitizeForRequest(request->url(), referrer); > On 2017/06/08 17:16:38, David Trainor-OOO to 6-8 wrote: > > On 2017/06/08 16:44:30, asanka wrote: > > > Whoa. This step is pretty easy to miss. > > > > Should we have a bool sanitize in SetReferrerForRequest? That way callers > have > > to explicitly declare whether or not they want the sanitization? > > Hmm, it looks like most other callers sanitize it early in the pipeline, so most > wouldn't be calling for sanitization, but it might be useful just as a fallback > in case. Should that be a separate CL? Sure I'd be fine with that :)
The CQ bit was checked by svaldez@chromium.org
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": 1, "attempt_start_ts": 1497012294529780, "parent_rev":
"3eb67a8404b818203d0498faf86438681c4a6c97", "commit_rev":
"da5f4e24e21981cc72d2cf2baf3af78eefc79279"}
Message was sent while issue was closed.
Description was changed from ========== Ensure that the referrer for HTTPS to HTTP downloads is dropped. BUG=730877 ========== to ========== Ensure that the referrer for HTTPS to HTTP downloads is dropped. BUG=730877 Review-Url: https://codereview.chromium.org/2927123002 Cr-Commit-Position: refs/heads/master@{#478255} Committed: https://chromium.googlesource.com/chromium/src/+/da5f4e24e21981cc72d2cf2baf3a... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/da5f4e24e21981cc72d2cf2baf3a... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
