Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2709543002:
Network traffic annotation added to chrome_omnibox_navigation_observer. (Closed)
|
Created:
3 years, 10 months ago by Ramin Halavati Modified:
3 years, 9 months ago CC:
chromium-reviews, jdonnelly+watch_chromium.org, battre, msramek Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNetwork traffic annotation added to chrome_omnibox_navigation_observer.
Network traffic annotation is added to network request of
chrome/browser/ui/omnibox/chrome_omnibox_navigation_observer.cc.
BUG=656607
Review-Url: https://codereview.chromium.org/2709543002
Cr-Commit-Position: refs/heads/master@{#453080}
Committed: https://chromium.googlesource.com/chromium/src/+/1bae62a2d64bd64a6bcd8700af33d192488aa652
Patch Set 1 #
Total comments: 18
Patch Set 2 : Annotation updated. #Patch Set 3 : Minor correction. #Messages
Total messages: 14 (5 generated)
rhalavati@chromium.org changed reviewers: + pkasting@chromium.org
As you have seen before, we are annotating all network requests in Chromium with a new NetworkTrafficAnnotation scheme. This allows enterprise users of Chrome to audit the requests and configure Chrome in a way that meets their security policies and expectations. Furthermore, it allows us to generate better debugging data in chrome://net-internals and measure bandwidth consumption on a per-request-type basis. I've modified another omnibox file and added annotation template to it. Please review it and suggest the required answers for the empty parts. Please note that the claims should be thorough and covering all cases. In case you believe that annotation should be passed to the modified function instead of originating from it, please tell me to change the CL accordingly. Please take a look at the protobuf scheme in: https://cs.chromium.org/chromium/src/tools/traffic_annotation/traffic_annotat... for the definition of the annotations. You can find a sample annotation in: https://cs.chromium.org/chromium/src/components/spellcheck/browser/spelling_s... Entriprise policy options are here: https://cs.chromium.org/chromium/src/out/Debug/gen/components/policy/proto/cl... And more description on enterprise policy settings is here: http://dev.chromium.org/administrators/policy-list-3 Please tell me if you need any clarifications from my side. If you want to learn more, see: https://docs.google.com/document/d/1FWEFTHzdqIA1wHUo_DuSpJjppW22otCMPCOA5NumhJU.
I will try to get to this tomorrow.
On 2017/02/23 02:07:02, Peter Kasting wrote: > I will try to get to this tomorrow. Thanks Peter.
https://codereview.chromium.org/2709543002/diff/1/chrome/browser/ui/omnibox/c... File chrome/browser/ui/omnibox/chrome_omnibox_navigation_observer.cc (right): https://codereview.chromium.org/2709543002/diff/1/chrome/browser/ui/omnibox/c... chrome/browser/ui/omnibox/chrome_omnibox_navigation_observer.cc:71: sender: "..." omnibox https://codereview.chromium.org/2709543002/diff/1/chrome/browser/ui/omnibox/c... chrome/browser/ui/omnibox/chrome_omnibox_navigation_observer.cc:72: description: "..." Certain omnibox inputs, e.g. single words, may either be search queries or attempts to navigate to intranet hostnames. When such a hostname is not in the user's history, a background request is made to see if it is navigable. If so, the browser will display a prompt on the search results page asking if the user wished to navigate instead of searching. https://codereview.chromium.org/2709543002/diff/1/chrome/browser/ui/omnibox/c... chrome/browser/ui/omnibox/chrome_omnibox_navigation_observer.cc:73: trigger: "..." User attempts to search for a string that is plausibly a navigable hostname but is not in the local history. https://codereview.chromium.org/2709543002/diff/1/chrome/browser/ui/omnibox/c... chrome/browser/ui/omnibox/chrome_omnibox_navigation_observer.cc:74: data: "..." None. However, the hostname itself is a string the user searched for, and thus can expose data about the user's searches. https://codereview.chromium.org/2709543002/diff/1/chrome/browser/ui/omnibox/c... chrome/browser/ui/omnibox/chrome_omnibox_navigation_observer.cc:75: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER WEBSITE https://codereview.chromium.org/2709543002/diff/1/chrome/browser/ui/omnibox/c... chrome/browser/ui/omnibox/chrome_omnibox_navigation_observer.cc:78: cookies_allowed: false/true true https://codereview.chromium.org/2709543002/diff/1/chrome/browser/ui/omnibox/c... chrome/browser/ui/omnibox/chrome_omnibox_navigation_observer.cc:79: cookies_store: "..." I'm not sure. The actual request context is on line 145-6: content::BrowserContext::GetDefaultStoragePartition(controller->GetBrowserContext())->GetURLRequestContext()); What context is that? The user context? That's what I'd expect. https://codereview.chromium.org/2709543002/diff/1/chrome/browser/ui/omnibox/c... chrome/browser/ui/omnibox/chrome_omnibox_navigation_observer.cc:80: setting: "..." NA https://codereview.chromium.org/2709543002/diff/1/chrome/browser/ui/omnibox/c... chrome/browser/ui/omnibox/chrome_omnibox_navigation_observer.cc:86: } The closest policy we have for this is DefaultSearchProviderEnabled -- if you disable default search, users can't search, so they can't hit this. This is a very large hammer for admins to use. https://bugs.chromium.org/p/chromium/issues/detail?id=81226 is about providing a more fine-grained policy and seems to have enough requests from the public that I think we should do it. I don't know what that means we should do fin this CL.
Annotation updated, please review. https://codereview.chromium.org/2709543002/diff/1/chrome/browser/ui/omnibox/c... File chrome/browser/ui/omnibox/chrome_omnibox_navigation_observer.cc (right): https://codereview.chromium.org/2709543002/diff/1/chrome/browser/ui/omnibox/c... chrome/browser/ui/omnibox/chrome_omnibox_navigation_observer.cc:71: sender: "..." On 2017/02/24 03:06:33, Peter Kasting wrote: > omnibox Done. https://codereview.chromium.org/2709543002/diff/1/chrome/browser/ui/omnibox/c... chrome/browser/ui/omnibox/chrome_omnibox_navigation_observer.cc:72: description: "..." On 2017/02/24 03:06:33, Peter Kasting wrote: > Certain omnibox inputs, e.g. single words, may either be search queries or > attempts to navigate to intranet hostnames. When such a hostname is not in the > user's history, a background request is made to see if it is navigable. If so, > the browser will display a prompt on the search results page asking if the user > wished to navigate instead of searching. Done. https://codereview.chromium.org/2709543002/diff/1/chrome/browser/ui/omnibox/c... chrome/browser/ui/omnibox/chrome_omnibox_navigation_observer.cc:73: trigger: "..." On 2017/02/24 03:06:33, Peter Kasting wrote: > User attempts to search for a string that is plausibly a navigable hostname but > is not in the local history. Done. https://codereview.chromium.org/2709543002/diff/1/chrome/browser/ui/omnibox/c... chrome/browser/ui/omnibox/chrome_omnibox_navigation_observer.cc:74: data: "..." On 2017/02/24 03:06:33, Peter Kasting wrote: > None. However, the hostname itself is a string the user searched for, and thus > can expose data about the user's searches. Done. https://codereview.chromium.org/2709543002/diff/1/chrome/browser/ui/omnibox/c... chrome/browser/ui/omnibox/chrome_omnibox_navigation_observer.cc:75: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER On 2017/02/24 03:06:34, Peter Kasting wrote: > WEBSITE Done. https://codereview.chromium.org/2709543002/diff/1/chrome/browser/ui/omnibox/c... chrome/browser/ui/omnibox/chrome_omnibox_navigation_observer.cc:78: cookies_allowed: false/true On 2017/02/24 03:06:33, Peter Kasting wrote: > true Done. https://codereview.chromium.org/2709543002/diff/1/chrome/browser/ui/omnibox/c... chrome/browser/ui/omnibox/chrome_omnibox_navigation_observer.cc:79: cookies_store: "..." On 2017/02/24 03:06:33, Peter Kasting wrote: > I'm not sure. The actual request context is on line 145-6: > > content::BrowserContext::GetDefaultStoragePartition(controller->GetBrowserContext())->GetURLRequestContext()); > > What context is that? The user context? That's what I'd expect. Done. https://codereview.chromium.org/2709543002/diff/1/chrome/browser/ui/omnibox/c... chrome/browser/ui/omnibox/chrome_omnibox_navigation_observer.cc:80: setting: "..." On 2017/02/24 03:06:33, Peter Kasting wrote: > NA Done. https://codereview.chromium.org/2709543002/diff/1/chrome/browser/ui/omnibox/c... chrome/browser/ui/omnibox/chrome_omnibox_navigation_observer.cc:86: } On 2017/02/24 03:06:33, Peter Kasting wrote: > The closest policy we have for this is DefaultSearchProviderEnabled -- if you > disable default search, users can't search, so they can't hit this. > > This is a very large hammer for admins to use. > https://bugs.chromium.org/p/chromium/issues/detail?id=81226 is about providing a > more fine-grained policy and seems to have enough requests from the public that > I think we should do it. > > I don't know what that means we should do fin this CL. Done.
LGTM
The CQ bit was checked by rhalavati@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": 40001, "attempt_start_ts": 1488007763705690,
"parent_rev": "5f1caa2b0b6d50b592b8415ce1ccb7b6bb2c5398", "commit_rev":
"1bae62a2d64bd64a6bcd8700af33d192488aa652"}
Message was sent while issue was closed.
Description was changed from ========== Network traffic annotation added to chrome_omnibox_navigation_observer. Network traffic annotation is added to network request of chrome/browser/ui/omnibox/chrome_omnibox_navigation_observer.cc. BUG=656607 ========== to ========== Network traffic annotation added to chrome_omnibox_navigation_observer. Network traffic annotation is added to network request of chrome/browser/ui/omnibox/chrome_omnibox_navigation_observer.cc. BUG=656607 Review-Url: https://codereview.chromium.org/2709543002 Cr-Commit-Position: refs/heads/master@{#453080} Committed: https://chromium.googlesource.com/chromium/src/+/1bae62a2d64bd64a6bcd8700af33... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/1bae62a2d64bd64a6bcd8700af33...
Message was sent while issue was closed.
battre@chromium.org changed reviewers: + battre@chromium.org
Message was sent while issue was closed.
lgtm |
