Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2802643002:
Network traffic annotation added to password_protection_request (Closed)
|
Created:
3 years, 8 months ago by Ramin Halavati Modified:
3 years, 7 months ago CC:
battre, chromium-reviews, grt+watch_chromium.org, timvolodine, vakh+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionNetwork traffic annotation added to password_protection_request
Network traffic annotation is added to network request of
components/safe_browsing/password_protection/password_protection_request.cc
BUG=656607
Review-Url: https://codereview.chromium.org/2802643002
Cr-Commit-Position: refs/heads/master@{#473650}
Committed: https://chromium.googlesource.com/chromium/src/+/e51cd18b640fe393dafefffdd24ecf40b2d54cd7
Patch Set 1 #
Total comments: 4
Patch Set 2 : Comments addressed. #Patch Set 3 : Extended reporting added. #
Total comments: 10
Patch Set 4 : Annotation updated. #Patch Set 5 : Extra policy removed. #Messages
Total messages: 32 (13 generated)
rhalavati@chromium.org changed reviewers: + shess@chromium.org
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 password_protection_request and added annotation template to it. Please review it and suggest the required answers for the empty parts (marked with '...'). 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/ch... 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.
shess@chromium.org changed reviewers: + nparker@chromium.org
Bouncing to Nathan, I think it's fine, but really have no idea.
On 2017/04/10 16:38:06, Scott Hess wrote: > Bouncing to Nathan, I think it's fine, but really have no idea. Thank you, I just note that we need suggested values for all missing data.
https://codereview.chromium.org/2802643002/diff/1/components/safe_browsing/pa... File components/safe_browsing/password_protection/password_protection_request.cc (right): https://codereview.chromium.org/2802643002/diff/1/components/safe_browsing/pa... components/safe_browsing/password_protection/password_protection_request.cc:116: sender: "..." Sender: Safe Browsing desc: When the user is about to log in to a new, uncommon site, Chrome will send a request to Safe Browsing to determine if the page is phishing. It'll then show a warning if the page is bad. trigger: When the user focuses on a password field on a page that isn't popular or known to be safe, and they haven't visited. data: URL and referrer of the current page, password form action, and iframe structure cookies: yes, SB cookie store https://codereview.chromium.org/2802643002/diff/1/components/safe_browsing/pa... components/safe_browsing/password_protection/password_protection_request.cc:132: policy_exception_justification: "..." User can turn off Safe Browsing.
Annotation updated, please review. https://codereview.chromium.org/2802643002/diff/1/components/safe_browsing/pa... File components/safe_browsing/password_protection/password_protection_request.cc (right): https://codereview.chromium.org/2802643002/diff/1/components/safe_browsing/pa... components/safe_browsing/password_protection/password_protection_request.cc:116: sender: "..." On 2017/04/14 21:39:38, Nathan Parker wrote: > Sender: Safe Browsing > desc: When the user is about to log in to a new, uncommon site, Chrome will send > a request to Safe Browsing to determine if the page is phishing. It'll then > show a warning if the page is bad. > > trigger: When the user focuses on a password field on a page that isn't popular > or known to be safe, and they haven't visited. > > data: URL and referrer of the current page, password form action, and iframe > structure > > cookies: yes, SB cookie store Done. https://codereview.chromium.org/2802643002/diff/1/components/safe_browsing/pa... components/safe_browsing/password_protection/password_protection_request.cc:132: policy_exception_justification: "..." On 2017/04/14 21:39:38, Nathan Parker wrote: > User can turn off Safe Browsing. Done.
lgtm
rhalavati@chromium.org changed reviewers: + msramek@chromium.org
Thank you very much. Martin, Any comments?
Sorry for my long silence on this CL - I was waiting for a closure on all the discussions we had about this feature. Can we update this to reflect the first step in our rollout plan, which is Safe Browsing Extended Reporting users?
nparker@chromium.org changed reviewers: + jialiul@chromium.org
Sure, let's just add that the trigger is gated by Extended Reporting. We'll switch that to be "gated by Sync || SBER" when we switch the finch config to enable that. That way the code stays in sync with the finch flags.
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Annotation updated, please review.
Sorry, a few more tweaking suggestions. https://codereview.chromium.org/2802643002/diff/80001/components/safe_browsin... File components/safe_browsing/password_protection/password_protection_request.cc (right): https://codereview.chromium.org/2802643002/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.cc:137: "phishing. It'll then show a warning if the page is bad." nit: "bad" is a subjective term. Maybe "...if the page poses a risk of phishing."? https://codereview.chromium.org/2802643002/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.cc:140: "focuses on a password field on a page that isn't popular or known " nit: I would rephrase a bit for readability. "When a user focuses on a password field on a page that they haven't visited before and that isn't popular or known to be safe." We can leave the SBER part for "setting" to make this shorter. https://codereview.chromium.org/2802643002/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.cc:153: "possible security incidents to Google' setting under 'Privacy'. " Do we normally list both SB and SBER as the setting for SBER features? I think we normally only list SBER. But I also vaguely remember that we used to explain the difference between SBER1 and SBER2.
https://codereview.chromium.org/2802643002/diff/80001/components/safe_browsin... File components/safe_browsing/password_protection/password_protection_request.cc (right): https://codereview.chromium.org/2802643002/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.cc:137: "phishing. It'll then show a warning if the page is bad." On 2017/05/12 at 19:10:19, msramek wrote: > nit: "bad" is a subjective term. > > Maybe "...if the page poses a risk of phishing."? Agree, "...if the page poses a risk of phishing." sounds better. https://codereview.chromium.org/2802643002/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.cc:140: "focuses on a password field on a page that isn't popular or known " On 2017/05/12 at 19:10:19, msramek wrote: > nit: I would rephrase a bit for readability. > > "When a user focuses on a password field on a page that they haven't visited before and that isn't popular or known to be safe." > > We can leave the SBER part for "setting" to make this shorter. SGTM https://codereview.chromium.org/2802643002/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.cc:153: "possible security incidents to Google' setting under 'Privacy'. " On 2017/05/12 at 19:10:19, msramek wrote: > Do we normally list both SB and SBER as the setting for SBER features? I think we normally only list SBER. > > But I also vaguely remember that we used to explain the difference between SBER1 and SBER2. SBER condition is temporary, eventually this feature will be extended to all SB users. The current setting text looks fine for me. And this feature applies to both SBER1 and SBER2. Do you think we still need to explain the differences?
Thank very much, annotation updated. Martin? https://codereview.chromium.org/2802643002/diff/80001/components/safe_browsin... File components/safe_browsing/password_protection/password_protection_request.cc (right): https://codereview.chromium.org/2802643002/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.cc:137: "phishing. It'll then show a warning if the page is bad." On 2017/05/12 19:59:33, Jialiu Lin wrote: > On 2017/05/12 at 19:10:19, msramek wrote: > > nit: "bad" is a subjective term. > > > > Maybe "...if the page poses a risk of phishing."? > > Agree, "...if the page poses a risk of phishing." sounds better. Done. https://codereview.chromium.org/2802643002/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.cc:140: "focuses on a password field on a page that isn't popular or known " On 2017/05/12 19:59:33, Jialiu Lin wrote: > On 2017/05/12 at 19:10:19, msramek wrote: > > nit: I would rephrase a bit for readability. > > > > "When a user focuses on a password field on a page that they haven't visited > before and that isn't popular or known to be safe." > > > > We can leave the SBER part for "setting" to make this shorter. > > SGTM Done.
LGTM (+non-actionable answer to Jialiu's comment) https://codereview.chromium.org/2802643002/diff/80001/components/safe_browsin... File components/safe_browsing/password_protection/password_protection_request.cc (right): https://codereview.chromium.org/2802643002/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.cc:153: "possible security incidents to Google' setting under 'Privacy'. " On 2017/05/12 19:59:33, Jialiu Lin wrote: > On 2017/05/12 at 19:10:19, msramek wrote: > > Do we normally list both SB and SBER as the setting for SBER features? I think > we normally only list SBER. > > > > But I also vaguely remember that we used to explain the difference between > SBER1 and SBER2. > > SBER condition is temporary, eventually this feature will be extended to all SB > users. The current setting text looks fine for me. I'm just saying that all SBER features can be disabled by switching off either SBER or SB, but we don't describe that in other annotations. I'm not sure that future plans play a role here. First of all, the annotation should be correct for the current implemention. Secondly, the next step in the rollout plan includes history sync users, and we don't mention that either. And finally, it's not yet clear whether this will be launched for SB as a whole - that depends on your metrics. That said, the current wording is technically correct, so I'm fine with it if you think it should stay this way. > > And this feature applies to both SBER1 and SBER2. Do you think we still need to > explain the differences? No. But I think we do that in some annotations, so I'm aiming for consistency.
https://codereview.chromium.org/2802643002/diff/80001/components/safe_browsin... File components/safe_browsing/password_protection/password_protection_request.cc (right): https://codereview.chromium.org/2802643002/diff/80001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.cc:153: "possible security incidents to Google' setting under 'Privacy'. " On 2017/05/15 12:57:28, msramek wrote: > On 2017/05/12 19:59:33, Jialiu Lin wrote: > > On 2017/05/12 at 19:10:19, msramek wrote: > > > Do we normally list both SB and SBER as the setting for SBER features? I > think > > we normally only list SBER. > > > > > > But I also vaguely remember that we used to explain the difference between > > SBER1 and SBER2. > > > > SBER condition is temporary, eventually this feature will be extended to all > SB > > users. The current setting text looks fine for me. > > I'm just saying that all SBER features can be disabled by switching off either > SBER or SB, but we don't describe that in other annotations. > > I'm not sure that future plans play a role here. First of all, the annotation > should be correct for the current implemention. Secondly, the next step in the > rollout plan includes history sync users, and we don't mention that either. And > finally, it's not yet clear whether this will be launched for SB as a whole - > that depends on your metrics. > > That said, the current wording is technically correct, so I'm fine with it if > you think it should stay this way. > > > > > And this feature applies to both SBER1 and SBER2. Do you think we still need > to > > explain the differences? > > No. But I think we do that in some annotations, so I'm aiming for consistency. We usually set the most fine grained policy that disables the feature. So I think we can just have 'SafeBrowsingExtendedReportingOptInAllowed'. What do you suggest Jialio?
The CQ bit was checked by rhalavati@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nparker@chromium.org Link to the patchset: https://codereview.chromium.org/2802643002/#ps100001 (title: "Annotation updated.")
The CQ bit was unchecked by rhalavati@chromium.org
Jialiu, Do you agree with removing the extra policy?
lgtm
The CQ bit was checked by rhalavati@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msramek@chromium.org, nparker@chromium.org Link to the patchset: https://codereview.chromium.org/2802643002/#ps120001 (title: "Extra policy removed.")
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": 120001, "attempt_start_ts": 1495477632116070,
"parent_rev": "99d8aeb59f72b051b8f4b4af8a55b54c22dedfda", "commit_rev":
"e51cd18b640fe393dafefffdd24ecf40b2d54cd7"}
Message was sent while issue was closed.
Description was changed from ========== Network traffic annotation added to password_protection_request Network traffic annotation is added to network request of components/safe_browsing/password_protection/password_protection_request.cc BUG=656607 ========== to ========== Network traffic annotation added to password_protection_request Network traffic annotation is added to network request of components/safe_browsing/password_protection/password_protection_request.cc BUG=656607 Review-Url: https://codereview.chromium.org/2802643002 Cr-Commit-Position: refs/heads/master@{#473650} Committed: https://chromium.googlesource.com/chromium/src/+/e51cd18b640fe393dafefffdd24e... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001) as https://chromium.googlesource.com/chromium/src/+/e51cd18b640fe393dafefffdd24e... |
