Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1099453005:
Switch web API/permission code to use IsOriginSecure() instead of SchemeIsSecure(). (Closed)
|
Created:
5 years, 8 months ago by lgarron Modified:
4 years ago Reviewers:
Bernhard Bauer, michaeln, benwells, tommi (sloooow) - chröme, Dan Beam, Sergey Ulanov, jww, palmer, felt CC:
browser-components-watch_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, darin-cc_chromium.org, estade+watch_chromium.org, extensions-reviews_chromium.org, feature-media-reviews_chromium.org, horo+watch_chromium.org, jam, jsbell+serviceworker_chromium.org, kinuko+watch, kinuko+serviceworker, markusheintz_, mcasas+watch_chromium.org, michaeln, nhiroki, posciak+watch_chromium.org, rouslan+autofillwatch_chromium.org, serviceworker-reviews, tzik, wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSwitch web API/permission code to use IsOriginSecure() and SchemeIsCryptographic() instead of SchemeIsSecure().
We recently introduced SchemeIsCryptographic() and IsOriginSecure(), which are meant to replace SchemeIsSecure().
IsOriginSecure() roughly means "do we trust this content not to be tampered with before it reaches the user?" [1] This is a higher-level definition that corresponds to the new "privileged contexts" spec. [2]
SchemeIsCryptographic() [3] is close to the old definition of SchemeIsSecure(), and literally just checks if the scheme is a cryptographic scheme (HTTPS or WSS as of right now). The difference is that SchemeIsCryptographic() will not consider filesystem URLs secure.
IsOriginSecure() should be correct for most Fizz code.
[1] https://code.google.com/p/chromium/codesearch#chromium/src/content/public/common/origin_util.h&sq=package:chromium&type=cs&l=19&rcl=143099866
[2] https://www.chromium.org/Home/chromium-security/prefer-secure-origins-for-powerful-new-features and https://w3c.github.io/webappsec/specs/powerfulfeatures/
[3]
https://code.google.com/p/chromium/codesearch#chromium/src/url/gurl.h&sq=package:chromium&type=cs&l=250&rcl=1430998666
BUG=362214
Patch Set 1 : #Patch Set 2 : Remove an ad-hoc definition of IsSchemeSecure() from a header file. #
Total comments: 2
Patch Set 3 : Address nhiroki's comment. #
Total comments: 11
Patch Set 4 : Address jww's comments. #
Total comments: 3
Patch Set 5 : Rebase onto the moved version of IsOriginSecure (https://codereview.chromium.org/1101033003/). #
Total comments: 12
Patch Set 6 : Change WalletServiceUrl::IsSignInContinueUrl to use SchemeIsCryptographic. #
Total comments: 1
Messages
Total messages: 44 (13 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
lgarron@chromium.org changed reviewers: + jww@chromium.org, palmer@chromium.org
I tried to separate out all the permission/Fizz code that should pretty much be aligned with the "powerful contexts" spec. I think most of these should use IsOriginSecure, except one of them (which is checking SSL connection info directly). I'm still compiling and testing to see if I introduced any syntactic errors, but how does this look?
Patchset #2 (id:80001) has been deleted
Patchset #1 (id:60001) has been deleted
nhiroki@chromium.org changed reviewers: + nhiroki@chromium.org
Drive by comment: https://codereview.chromium.org/1099453005/diff/120001/content/browser/servic... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/1099453005/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_dispatcher_host.cc:62: IsOriginSecure(script_url); IsOriginSecure() is not sufficient here because Service Worker allows only "http://localhost/" || "https://*/". Could you revive OriginCanAccessServiceWorkers() and replace "(url.SchemeIsSecure() || net::IsLocalhost(url.host()))" with IsOriginSecure()?
On 2015/04/23 01:29:58, nhiroki wrote: > Drive by comment: > > https://codereview.chromium.org/1099453005/diff/120001/content/browser/servic... > File content/browser/service_worker/service_worker_dispatcher_host.cc (right): > > https://codereview.chromium.org/1099453005/diff/120001/content/browser/servic... > content/browser/service_worker/service_worker_dispatcher_host.cc:62: > IsOriginSecure(script_url); > IsOriginSecure() is not sufficient here because Service Worker allows only > "http://localhost/" || "https://*/". Could you revive > OriginCanAccessServiceWorkers() and replace "(url.SchemeIsSecure() || > net::IsLocalhost(url.host()))" with IsOriginSecure()? Please don't include chrome/ header file from content/ files. Could you hold off on making this change until the following change lands? https://codereview.chromium.org/1101033003/
On 2015/04/23 at 01:42:31, kinuko wrote: > On 2015/04/23 01:29:58, nhiroki wrote: > > Drive by comment: > > > > https://codereview.chromium.org/1099453005/diff/120001/content/browser/servic... > > File content/browser/service_worker/service_worker_dispatcher_host.cc (right): > > > > https://codereview.chromium.org/1099453005/diff/120001/content/browser/servic... > > content/browser/service_worker/service_worker_dispatcher_host.cc:62: > > IsOriginSecure(script_url); > > IsOriginSecure() is not sufficient here because Service Worker allows only > > "http://localhost/" || "https://*/". Could you revive > > OriginCanAccessServiceWorkers() and replace "(url.SchemeIsSecure() || > > net::IsLocalhost(url.host()))" with IsOriginSecure()? > > Please don't include chrome/ header file from content/ files. > Could you hold off on making this change until the following change lands? > > https://codereview.chromium.org/1101033003/ Sure. I still need to learn the right way to do DEPS either way.
https://codereview.chromium.org/1099453005/diff/120001/content/browser/servic... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/1099453005/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_dispatcher_host.cc:62: IsOriginSecure(script_url); On 2015/04/23 01:29:58, nhiroki wrote: > IsOriginSecure() is not sufficient here because Service Worker allows only > "http://localhost/" || "https://*/". Could you revive > OriginCanAccessServiceWorkers() and replace "(url.SchemeIsSecure() || > net::IsLocalhost(url.host()))" with IsOriginSecure()? Drive-by comments are welcome! You're right, that's a functional change I did not intend. Done (even if this will probably be moot after https://codereview.chromium.org/1101033003/).
https://codereview.chromium.org/1099453005/diff/120001/content/browser/servic... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/1099453005/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_dispatcher_host.cc:62: IsOriginSecure(script_url); On 2015/04/23 01:29:58, nhiroki wrote: > IsOriginSecure() is not sufficient here because Service Worker allows only > "http://localhost/" || "https://*/". Could you revive > OriginCanAccessServiceWorkers() and replace "(url.SchemeIsSecure() || > net::IsLocalhost(url.host()))" with IsOriginSecure()? Drive-by comments are welcome! You're right, that's a functional change I did not intend. Done (even if this will probably be moot after https://codereview.chromium.org/1101033003/).
lgarron, you're on the right track, but as it stands, this is not lgtm because of layering violations that I've outlined inline. https://codereview.chromium.org/1099453005/diff/140001/chrome/browser/banners... File chrome/browser/banners/app_banner_manager.cc (right): https://codereview.chromium.org/1099453005/diff/140001/chrome/browser/banners... chrome/browser/banners/app_banner_manager.cc:51: // A secure scheme is required to show banners, so exit early if we see the Please update this comment to reflect the new origin check (not just scheme check) https://codereview.chromium.org/1099453005/diff/140001/components/autofill/co... File components/autofill/content/browser/DEPS (right): https://codereview.chromium.org/1099453005/diff/140001/components/autofill/co... components/autofill/content/browser/DEPS:2: "+chrome/common/origin_util.h", I'm pretty sure this is an invalid include and a layering violation. You'll want to check with a wiser person than me, but I think you're going to need to move origin_util.h into components or content if you want to include it in components. https://codereview.chromium.org/1099453005/diff/140001/components/content_set... File components/content_settings/core/browser/DEPS (right): https://codereview.chromium.org/1099453005/diff/140001/components/content_set... components/content_settings/core/browser/DEPS:2: "+chrome/common/origin_util.h", See earlier comment. I'm pretty sure this is a layering violation. https://codereview.chromium.org/1099453005/diff/140001/content/browser/DEPS File content/browser/DEPS (right): https://codereview.chromium.org/1099453005/diff/140001/content/browser/DEPS#n... content/browser/DEPS:2: "+chrome/common/origin_util.h", Another layering violation. https://codereview.chromium.org/1099453005/diff/140001/content/browser/appcac... File content/browser/appcache/appcache_update_job.cc (right): https://codereview.chromium.org/1099453005/diff/140001/content/browser/appcac... content/browser/appcache/appcache_update_job.cc:13: #include "chrome/common/origin_util.h" Given what's used below, you're not using this header, so it should be removed. https://codereview.chromium.org/1099453005/diff/140001/content/browser/appcac... content/browser/appcache/appcache_update_job.cc:173: if (url_.SchemeIsCryptographic()) { Why this check? Why not OriginIsSecure()?
Don't worry; kinuko has a nicely layered patch coming soon: https://codereview.chromium.org/1101033003/ This CL will be smooth going when kinuko lands that.
On 2015/04/23 18:39:15, palmer wrote: > Don't worry; kinuko has a nicely layered patch coming soon: > https://codereview.chromium.org/1101033003/ > > This CL will be smooth going when kinuko lands that. Ah, that's exactly what the doctor ordered! Once that's in, this should fall into place nicely.
https://codereview.chromium.org/1099453005/diff/140001/chrome/browser/banners... File chrome/browser/banners/app_banner_manager.cc (right): https://codereview.chromium.org/1099453005/diff/140001/chrome/browser/banners... chrome/browser/banners/app_banner_manager.cc:51: // A secure scheme is required to show banners, so exit early if we see the On 2015/04/23 at 18:24:02, jww wrote: > Please update this comment to reflect the new origin check (not just scheme check) Done. https://codereview.chromium.org/1099453005/diff/140001/components/autofill/co... File components/autofill/content/browser/DEPS (right): https://codereview.chromium.org/1099453005/diff/140001/components/autofill/co... components/autofill/content/browser/DEPS:2: "+chrome/common/origin_util.h", On 2015/04/23 at 18:24:02, jww wrote: > I'm pretty sure this is an invalid include and a layering violation. You'll want to check with a wiser person than me, but I think you're going to need to move origin_util.h into components or content if you want to include it in components. This felt wrong, but I just wanted it to compile first and ask later. :-P In this case, the solution appears to be just to move the function: https://codereview.chromium.org/1101033003/ In the future, is there a document that can help me understand what when something is a layering violation? https://codereview.chromium.org/1099453005/diff/140001/content/browser/appcac... File content/browser/appcache/appcache_update_job.cc (right): https://codereview.chromium.org/1099453005/diff/140001/content/browser/appcac... content/browser/appcache/appcache_update_job.cc:13: #include "chrome/common/origin_util.h" On 2015/04/23 at 18:24:02, jww wrote: > Given what's used below, you're not using this header, so it should be removed. Done. https://codereview.chromium.org/1099453005/diff/140001/content/browser/appcac... content/browser/appcache/appcache_update_job.cc:173: if (url_.SchemeIsCryptographic()) { On 2015/04/23 at 18:24:02, jww wrote: > Why this check? Why not OriginIsSecure()? There's a comment in the first patch. The code below makes sure that resources with cert errors are not appcached. Presumably, that only makes sense if the scheme is actually cryptographic.
estade@chromium.org changed reviewers: + estade@chromium.org
https://codereview.chromium.org/1099453005/diff/140001/components/autofill/co... File components/autofill/content/browser/DEPS (right): https://codereview.chromium.org/1099453005/diff/140001/components/autofill/co... components/autofill/content/browser/DEPS:2: "+chrome/common/origin_util.h", On 2015/04/23 23:05:51, lgarron wrote: > On 2015/04/23 at 18:24:02, jww wrote: > > I'm pretty sure this is an invalid include and a layering violation. You'll > want to check with a wiser person than me, but I think you're going to need to > move origin_util.h into components or content if you want to include it in > components. > > This felt wrong, but I just wanted it to compile first and ask later. :-P > > In this case, the solution appears to be just to move the function: > https://codereview.chromium.org/1101033003/ > In the future, is there a document that can help me understand what when > something is a layering violation? Some of the DEPS files have some comments. From src/content/DEPS: # Do NOT add chrome or components to the list below. We shouldn't be # including files from src/chrome or src/components in src/content. From src/components/DEPS: # Do not add chrome/ as an allowed include. # Individual components must explicitly declare their dependencies # on other components. Cycles in the dependency graph within # components/ are not allowed.
Patchset #6 (id:200001) has been deleted
lgarron@chromium.org changed reviewers: + ddorwin@chromium.org, michaeln@chromium.org
ddorwin: Could you check chrome/browser/media/* (4 files) and see if IsOriginSecure is the right check? michaeln@: Could you check chrome/browser/media/ and see if IsSchemeCryptographic is the right check? https://codereview.chromium.org/1099453005/diff/180001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1099453005/diff/180001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:27: #include "content/public/common/origin_util.h" `git cl upload` complains: You added one or more #includes that violate checkdeps rules. components/content_settings/core/browser/host_content_settings_map.cc Illegal include: "content/public/common/origin_util.h" Because of no rule applying. Now that this is the only DEPS flag, I'd like to learn the right way to do this. How can I tell whether this is safe/correct to add to components/content_settings/core/browser/DEPS? If not, what's the right approach? https://codereview.chromium.org/1099453005/diff/180001/content/browser/appcac... File content/browser/appcache/appcache_update_job.cc (right): https://codereview.chromium.org/1099453005/diff/180001/content/browser/appcac... content/browser/appcache/appcache_update_job.cc:172: if (url_.SchemeIsCryptographic()) { michaeln@: I think the following code requires the connection to be, well... cryptographic. In case you're not up to speed on the changes, we're replacing SchemeIsSecure() with SchemeIsCryptographic() or with OriginIsSecure(), depending on what's appropriate. Is SchemeIsCryptographic() correct here, or would something else be more accurate?
ddorwin@chromium.org changed reviewers: + tommi@chromium.org
On 2015/04/27 23:21:53, lgarron wrote: > ddorwin: Could you check chrome/browser/media/* (4 files) and see if > IsOriginSecure is the right check? These files are for WebRTC. +tommi to comment on the intent.
https://codereview.chromium.org/1099453005/diff/180001/chrome/browser/media/m... File chrome/browser/media/media_capture_devices_dispatcher.cc (right): https://codereview.chromium.org/1099453005/diff/180001/chrome/browser/media/m... chrome/browser/media/media_capture_devices_dispatcher.cc:599: IsOriginSecure(request.security_origin) || Does IsOriginSecure return true for extensions?
On 2015/04/28 07:25:56, tommi wrote: > https://codereview.chromium.org/1099453005/diff/180001/chrome/browser/media/m... > File chrome/browser/media/media_capture_devices_dispatcher.cc (right): > > https://codereview.chromium.org/1099453005/diff/180001/chrome/browser/media/m... > chrome/browser/media/media_capture_devices_dispatcher.cc:599: > IsOriginSecure(request.security_origin) || > Does IsOriginSecure return true for extensions? Yes it does.
On 2015/04/28 at 07:39:04, kinuko wrote: > On 2015/04/28 07:25:56, tommi wrote: > > https://codereview.chromium.org/1099453005/diff/180001/chrome/browser/media/m... > > File chrome/browser/media/media_capture_devices_dispatcher.cc (right): > > > > https://codereview.chromium.org/1099453005/diff/180001/chrome/browser/media/m... > > chrome/browser/media/media_capture_devices_dispatcher.cc:599: > > IsOriginSecure(request.security_origin) || > > Does IsOriginSecure return true for extensions? > > Yes it does. Tommi, is IsOriginSecure appropriate, given than it it includes extensions?
https://codereview.chromium.org/1099453005/diff/180001/chrome/browser/banners... File chrome/browser/banners/app_banner_manager.cc (right): https://codereview.chromium.org/1099453005/diff/180001/chrome/browser/banners... chrome/browser/banners/app_banner_manager.cc:51: // A secure origin is required to show banners, so exit early if we see the Nit: I think you can make this comment more concise: "A secure origin is required to show banners." https://codereview.chromium.org/1099453005/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/api/desktop_capture/desktop_capture_api.cc (right): https://codereview.chromium.org/1099453005/diff/180001/chrome/browser/extensi... chrome/browser/extensions/api/desktop_capture/desktop_capture_api.cc:111: !IsOriginSecure(origin_)) { |IsOriginSecure| is not trivial/fast like |SchemeIsSecure|, and since you call it twice on the same argument (|origin_|), it might be good to cache it. Above line 109: bool is_origin_secure = IsOriginSecure(origin_); then use it here and on (what is now) line 115. https://codereview.chromium.org/1099453005/diff/180001/chrome/browser/media/m... File chrome/browser/media/media_capture_devices_dispatcher.cc (right): https://codereview.chromium.org/1099453005/diff/180001/chrome/browser/media/m... chrome/browser/media/media_capture_devices_dispatcher.cc:599: IsOriginSecure(request.security_origin) || > Does IsOriginSecure return true for extensions? Yes. https://codereview.chromium.org/1099453005/diff/180001/content/browser/appcac... File content/browser/appcache/appcache_update_job.cc (right): https://codereview.chromium.org/1099453005/diff/180001/content/browser/appcac... content/browser/appcache/appcache_update_job.cc:172: if (url_.SchemeIsCryptographic()) { > Is SchemeIsCryptographic() correct here, or would something else be more > accurate? I think in general we want to deprecate AppCache in favor of Service Workers, and |SchemeIsCryptographic| keeps the scope of AC strictly narrower than does |IsOriginSecure|. So let's use |SchemeIsCryptographic| unless michaeln has a great reason otherwise. Note also that |SchemeIsCryptographic| is a much closer analogue to |SchemeIsSecure| than |IsOriginSecure| is.
On 2015/04/30 at 18:53:57, lgarron wrote: > On 2015/04/28 at 07:39:04, kinuko wrote: > > On 2015/04/28 07:25:56, tommi wrote: > > > https://codereview.chromium.org/1099453005/diff/180001/chrome/browser/media/m... > > > File chrome/browser/media/media_capture_devices_dispatcher.cc (right): > > > > > > https://codereview.chromium.org/1099453005/diff/180001/chrome/browser/media/m... > > > chrome/browser/media/media_capture_devices_dispatcher.cc:599: > > > IsOriginSecure(request.security_origin) || > > > Does IsOriginSecure return true for extensions? > > > > Yes it does. > > Tommi, is IsOriginSecure appropriate, given than it it includes extensions? Assuming the extension was installed via secure means, then I'd think so. I haven't spent a lot of time thinking about though :-/
On 2015/05/04 at 10:45:56, tommi wrote: > On 2015/04/30 at 18:53:57, lgarron wrote: > > On 2015/04/28 at 07:39:04, kinuko wrote: > > > On 2015/04/28 07:25:56, tommi wrote: > > > > https://codereview.chromium.org/1099453005/diff/180001/chrome/browser/media/m... > > > > File chrome/browser/media/media_capture_devices_dispatcher.cc (right): > > > > > > > > https://codereview.chromium.org/1099453005/diff/180001/chrome/browser/media/m... > > > > chrome/browser/media/media_capture_devices_dispatcher.cc:599: > > > > IsOriginSecure(request.security_origin) || > > > > Does IsOriginSecure return true for extensions? > > > > > > Yes it does. > > > > Tommi, is IsOriginSecure appropriate, given than it it includes extensions? > > Assuming the extension was installed via secure means, then I'd think so. I haven't spent a lot of time thinking about though :-/ The idea of IsOriginSecure() is that it holds true iff this is the case, so I'll consider that a "yes". https://code.google.com/p/chromium/codesearch#chromium/src/content/public/com...
Adding several new reviewers. Would you mind checking if IsOriginSecure() is correct for your files? (See below for details.) benwells@: chrome/browser/banners/app_banner_manager.cc (1 file) bauerb@: permission_context_uma_util.cc, content_setting_bubble_model.cc, permission_menu_model.cc, host_content_settings_map.cc (4 files) dbeam@: components/autofill/content/browser/wallet/wallet_service_url_unittest.cc (1 file) michaeln@: content/browser/appcache/appcache_update_job.cc (1 file) sergeyu@: chrome/browser/extensions/api/desktop_capture/desktop_capture_api.cc (1 file) tommi@: chrome/browser/media/ (4 files) palmer@ recently introduced SchemeIsCryptographic() and IsOriginSecure(), which are meant to replace SchemeIsSecure(). IsOriginSecure() roughly means "do we trust this content not to be tampered with before it reaches the user?" [1] This is a higher-level definition that corresponds to the new "privileged contexts" spec. [2] SchemeIsCryptographic() [4] is close to the old definition of SchemeIsSecure(), and literally just checks if the scheme is a cryptographic scheme (HTTPS or WSS as of right now). The difference is that SchemeIsCryptographic() will not consider file URLs secure. IsOriginSecure() should be correct for most Fizz code. [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/public/com... [2] https://www.chromium.org/Home/chromium-security/prefer-secure-origins-for-pow... and https://w3c.github.io/webappsec/specs/powerfulfeatures/ [3] https://code.google.com/p/chromium/codesearch#chromium/src/url/gurl.h&sq=pack...
Adding several new reviewers. Would you mind checking if IsOriginSecure() is correct for your files? (See below for details.) benwells@: chrome/browser/banners/app_banner_manager.cc (1 file) bauerb@: permission_context_uma_util.cc, content_setting_bubble_model.cc, permission_menu_model.cc, host_content_settings_map.cc (4 files) dbeam@: components/autofill/content/browser/wallet/wallet_service_url_unittest.cc (1 file) michaeln@: content/browser/appcache/appcache_update_job.cc (1 file) sergeyu@: chrome/browser/extensions/api/desktop_capture/desktop_capture_api.cc (1 file) tommi@: chrome/browser/media/ (4 files) palmer@ recently introduced SchemeIsCryptographic() and IsOriginSecure(), which are meant to replace SchemeIsSecure(). IsOriginSecure() roughly means "do we trust this content not to be tampered with before it reaches the user?" [1] This is a higher-level definition that corresponds to the new "privileged contexts" spec. [2] SchemeIsCryptographic() [4] is close to the old definition of SchemeIsSecure(), and literally just checks if the scheme is a cryptographic scheme (HTTPS or WSS as of right now). The difference is that SchemeIsCryptographic() will not consider file URLs secure. IsOriginSecure() should be correct for most Fizz code. [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/public/com... [2] https://www.chromium.org/Home/chromium-security/prefer-secure-origins-for-pow... and https://w3c.github.io/webappsec/specs/powerfulfeatures/ [3] https://code.google.com/p/chromium/codesearch#chromium/src/url/gurl.h&sq=pack...
https://codereview.chromium.org/1099453005/diff/180001/components/autofill/co... File components/autofill/content/browser/wallet/wallet_service_url_unittest.cc (left): https://codereview.chromium.org/1099453005/diff/180001/components/autofill/co... components/autofill/content/browser/wallet/wallet_service_url_unittest.cc:118: EXPECT_TRUE(GetSignInContinueUrl().SchemeIsSecure()); the intent of this is to ensure the transmission of data is encrypted. IsOriginSecure() does not require this, as far as I can tell.
https://codereview.chromium.org/1099453005/diff/180001/components/autofill/co... File components/autofill/content/browser/wallet/wallet_service_url_unittest.cc (left): https://codereview.chromium.org/1099453005/diff/180001/components/autofill/co... components/autofill/content/browser/wallet/wallet_service_url_unittest.cc:118: EXPECT_TRUE(GetSignInContinueUrl().SchemeIsSecure()); On 2015/05/07 20:50:53, Dan Beam wrote: > the intent of this is to ensure the transmission of data is encrypted. > IsOriginSecure() does not require this, as far as I can tell. Is the intent to ensure that it is literally encrypted (using HTTPS/WSS), or that it is trusted as much as an encrypted connection? IsOriginSecure() is meant to capture the latter, but SchemeIsCryptographic() is appropriate for the former.
https://codereview.chromium.org/1099453005/diff/180001/components/autofill/co... File components/autofill/content/browser/wallet/wallet_service_url_unittest.cc (left): https://codereview.chromium.org/1099453005/diff/180001/components/autofill/co... components/autofill/content/browser/wallet/wallet_service_url_unittest.cc:118: EXPECT_TRUE(GetSignInContinueUrl().SchemeIsSecure()); On 2015/05/07 20:50:53, Dan Beam wrote: > the intent of this is to ensure the transmission of data is encrypted. > IsOriginSecure() does not require this, as far as I can tell. Correct: |IsOriginSecure| will also return true for origins that are not cryptographically protected, and for which that does not matter: http://localhost, extension URLs, and so on. It might be that we should say EXPECT_TRUE(GetSignInContinueUrl().SchemeIsCryptographic()); here instead. (|SchemeIsCryptographic| is the more precisely-defined replacement for |SchemeIsSecure|.)
https://codereview.chromium.org/1099453005/diff/180001/components/autofill/co... File components/autofill/content/browser/wallet/wallet_service_url_unittest.cc (left): https://codereview.chromium.org/1099453005/diff/180001/components/autofill/co... components/autofill/content/browser/wallet/wallet_service_url_unittest.cc:118: EXPECT_TRUE(GetSignInContinueUrl().SchemeIsSecure()); On 2015/05/07 20:57:10, palmer wrote: > On 2015/05/07 20:50:53, Dan Beam wrote: > > the intent of this is to ensure the transmission of data is encrypted. > > IsOriginSecure() does not require this, as far as I can tell. > > Correct: |IsOriginSecure| will also return true for origins that are not > cryptographically protected, and for which that does not matter: > http://localhost, extension URLs, and so on. it'd probably allow http:// trusted origins as well, no? > > It might be that we should say > > EXPECT_TRUE(GetSignInContinueUrl().SchemeIsCryptographic()); > > here instead. (|SchemeIsCryptographic| is the more precisely-defined replacement > for |SchemeIsSecure|.) yep
https://codereview.chromium.org/1099453005/diff/180001/components/autofill/co... File components/autofill/content/browser/wallet/wallet_service_url_unittest.cc (left): https://codereview.chromium.org/1099453005/diff/180001/components/autofill/co... components/autofill/content/browser/wallet/wallet_service_url_unittest.cc:118: EXPECT_TRUE(GetSignInContinueUrl().SchemeIsSecure()); On 2015/05/07 20:58:29, Dan Beam wrote: > On 2015/05/07 20:57:10, palmer wrote: > > On 2015/05/07 20:50:53, Dan Beam wrote: > > > the intent of this is to ensure the transmission of data is encrypted. > > > IsOriginSecure() does not require this, as far as I can tell. > > > > Correct: |IsOriginSecure| will also return true for origins that are not > > cryptographically protected, and for which that does not matter: > > http://localhost, extension URLs, and so on. > > it'd probably allow http:// trusted origins as well, no? > It allows HTTP origins if the host is localhost, or if the user has passed an explicit commandline flag to trust it. Given your concern, it seems that SchemeIsSecure() is safe. Since it's in a test, no one should need this to work for localhost or with a special override. I've made the change in the latest patch.
lgarron@chromium.org changed reviewers: + felt@chromium.org
Adding felt@ for chrome/browser/ui/website_settings/permission_menu_model.cc
https://codereview.chromium.org/1099453005/diff/160001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1099453005/diff/160001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:658: IsOriginSecure(primary_url)) { bauerb@: `git cl upload` complains: You added one or more #includes that violate checkdeps rules. components/content_settings/core/browser/host_content_settings_map.cc Illegal include: "content/public/common/origin_util.h" Because of no rule applying. How can I tell whether this is safe/correct to add to components/content_settings/core/browser/DEPS? If not, what's the right approach?
+Miguel for the UMA question. https://codereview.chromium.org/1099453005/diff/160001/chrome/browser/content... File chrome/browser/content_settings/permission_context_uma_util.cc (right): https://codereview.chromium.org/1099453005/diff/160001/chrome/browser/content... chrome/browser/content_settings/permission_context_uma_util.cc:84: bool secure_origin = IsOriginSecure(requesting_origin); This changes the definition of a UMA statistic, which is something that we generally try to avoid. I don't know whether the impact is small enough here though to accept it. https://codereview.chromium.org/1099453005/diff/160001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1099453005/diff/160001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:658: IsOriginSecure(primary_url)) { On 2015/05/08 03:05:49, lgarron wrote: > bauerb@: `git cl upload` complains: > > You added one or more #includes that violate checkdeps rules. > components/content_settings/core/browser/host_content_settings_map.cc > Illegal include: "content/public/common/origin_util.h" > Because of no rule applying. > > How can I tell whether this is safe/correct to add to > components/content_settings/core/browser/DEPS? If not, what's the right > approach? The presubmit check is right, this is indeed *not* correct. content_settings/ is a layered component (which is hinted at by the core/content split), so it can't depend on content/ directly (to allow it to be used by Bling). Now, this check is to allow chrome:// Web UI to do authenticated requests to Google servers, for things like Cloud Print (https://codereview.chromium.org/10826089). The use case here really only is for https: URLs, so I would say that url.SchemeIsCryptographic() should work. https://codereview.chromium.org/1099453005/diff/220001/chrome/browser/ui/cont... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/1099453005/diff/220001/chrome/browser/ui/cont... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:704: if (IsOriginSecure(url)) { Does this compile? I would have though it requires content::...
Should this be broken up into several CLs?
On 2015/05/08 18:11:48, felt wrote: > Should this be broken up into several CLs? Probably. This level of grouping made sense to me at the time, but the number of owners turned out to be wieldy. I'll start by splitting out the media code. |
