Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1049533002:
Add IsOriginSecure and GURL::SchemeUsesTLS. (Closed)
DescriptionAdd IsOriginSecure and GURL::SchemeUsesTLS.
Standard functions for people to check if content from an origin can be
considered to have been transferred to the browser securely, as defined in
https://www.w3.org/TR/powerful-features/#is-origin-trustworthy.
BUG=362214, 470142
Committed: https://crrev.com/6c3473c3489dae0bfd2133c22e6b5cd9fb4f5fef
Cr-Commit-Position: refs/heads/master@{#325495}
Patch Set 1 #Patch Set 2 : More precise documentation. #Patch Set 3 : Be even more precise in comments. #
Total comments: 2
Patch Set 4 : Move it to content::, so we can call into net:: and blink::. #
Total comments: 3
Patch Set 5 : One does not simply check schemes and hosts for membership in tiny sets... #
Total comments: 4
Patch Set 6 : More thoroughly document all 3 layers of the functionality. #Patch Set 7 : Update the commit message to reflect the changes. #
Total comments: 4
Patch Set 8 : Fold the content:: version into the chrome/ version. Just 1 now. More tests, including filesystem:. #
Total comments: 10
Patch Set 9 : Make the comments reflect reality. #
Total comments: 2
Patch Set 10 : Fix the comments for real. #
Total comments: 1
Messages
Total messages: 55 (11 generated)
palmer@chromium.org changed reviewers: + brettw@chromium.org
This is 1 of a set of hopefully small CLs to polish off crbug.com/362214.
palmer@chromium.org changed reviewers: + abarth@chromium.org
Adding abarth as the other url/ OWNER in case brettw is too busy. But, abarth is probably pretty busy too. :)
felt@chromium.org changed reviewers: + felt@chromium.org
https://codereview.chromium.org/1049533002/diff/40001/url/gurl.h File url/gurl.h (right): https://codereview.chromium.org/1049533002/diff/40001/url/gurl.h#newcode262 url/gurl.h:262: bool HostIsLocal() const; this seems redundant with net::IsLocalhost (also what about net::IsLocalhostTLD)
> this seems redundant with net::IsLocalhost (also what about net::IsLocalhostTLD) It is — but I don't think url/ can depend on net/ ? It may be that I just need to do all this in content/ ?
https://codereview.chromium.org/1049533002/diff/40001/url/gurl.h File url/gurl.h (right): https://codereview.chromium.org/1049533002/diff/40001/url/gurl.h#newcode262 url/gurl.h:262: bool HostIsLocal() const; Yes, I have 2 reasons to move this code to content/common: Ability to call into net/, and ability to call into blink:: (to get the registered secure schemes). So this is going away.
palmer@chromium.org changed reviewers: + davidben@google.com, tkent@chromium.org
(Yes, I realize the tests should not be in gurl_unittest.cc. I'll move them into the right place once OWNERS signal content:: is the right place for this code to go in.) +tkent for content/common/DEPS. I hope this is OK? +davidben for content/OWNERS.
https://codereview.chromium.org/1049533002/diff/60001/content/common/url_sche... File content/common/url_schemes.cc (right): https://codereview.chromium.org/1049533002/diff/60001/content/common/url_sche... content/common/url_schemes.cc:91: if (blink::WebSecurityPolicy::shouldTreatURLSchemeAsSecure(scheme)) Does this code run in the browser process? If so, we should not use WebSecurityPolicy here. It has a lot of side effects.
> (blink::WebSecurityPolicy::shouldTreatURLSchemeAsSecure(scheme)) > Does this code run in the browser process? If so, we should not use > WebSecurityPolicy here. It has a lot of side effects. I expect this stuff to be general purpose and possibly called by any process, yeah. What are the side effects? How should I avoid WebSecurityPolicy while still taking into account the schemes registered with it as secure schemes?
lgtm https://codereview.chromium.org/1049533002/diff/60001/url/gurl.h File url/gurl.h (right): https://codereview.chromium.org/1049533002/diff/60001/url/gurl.h#newcode229 url/gurl.h:229: SchemeIs(url::kWssScheme) || (SchemeIsFileSystem() && inner_url() && I found the old way of SchemeIsFileSystem on the same line as the remaining stuff was easier to read, so can you break there instead?
davidben@chromium.org changed reviewers: + davidben@chromium.org
https://codereview.chromium.org/1049533002/diff/60001/url/gurl.h File url/gurl.h (right): https://codereview.chromium.org/1049533002/diff/60001/url/gurl.h#newcode228 url/gurl.h:228: return SchemeIs(url::kFileScheme) || SchemeIs(url::kHttpsScheme) || I'm worried changing this behavior outright will break existing consumers of this function. Quite a lot of code uses this as a shorthand for "is this HTTPS or WSS" and "is this HTTPS" (since ws/wss aren't navigable). Really, the filesystem: thing already makes it sad, but file: is a lot more likely to be navigated to. At a glance, these two I don't think should include kFileScheme. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/med... https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/cap... https://code.google.com/p/chromium/codesearch#chromium/src/content/child/reso... https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_netw... (This isn't an exhaustive list. I just clicked through them randomly.) Perhaps we need something like SchemeUsesSSL() which does "https or wss" and nothing else. I almost wonder if the current SchemeIsSecure should also go into //content since this is very much a Web Platform notion of "secure" here.
On 2015/04/06 18:03:57, palmer wrote: > > (blink::WebSecurityPolicy::shouldTreatURLSchemeAsSecure(scheme)) > > Does this code run in the browser process? If so, we should not use > > WebSecurityPolicy here. It has a lot of side effects. > > I expect this stuff to be general purpose and possibly called by any process, > yeah. > > What are the side effects? > > How should I avoid WebSecurityPolicy while still taking into account the schemes > registered with it as secure schemes? WebSecurityPolicy::shouldTreatURLSchemeAsSecure() initializes a Mutex, HashSet, and HashSet and WTF::String initialize PartitionAlloc allocator. Also, some code calls WebSecurityPolicy::registerURLSchemeAsSecure(). I think they are only in renderer process and shouldTreatURLSchemeAsSecure() doesn't work correctly in the browser process. I don't have an alternative idea. We might need to have another scheme registry in Chromium.
OK, given the handy clues from davidben and tkent — thank you! —, I propose the following plan of cyber-action in this CL: 1. Make a GURL::SchemeUsesTLS that behaves as davidben suggests. 2. Revert https://codereview.chromium.org/1049923002 — it won't be needed after all. 3. Change this CL's content::IsOriginSecure to use a local (content::, not blink::) copy (vector<string>) of the schemes registered as secure. Sounds like that'll be lighter-weight than trying to call into blink::. In another CL (or set of CLs): 4. Change all callers of the current SchemeIsSecure to either GURL::SchemeUsesTLS or content::IsOriginSecure, as appropriate. 5. Get rid of GURL::SchemeIsSecure. Does that sound reasonable?
On 2015/04/07 19:25:35, palmer wrote: > OK, given the handy clues from davidben and tkent — thank you! —, I propose the > following plan of cyber-action in this CL: > > 1. Make a GURL::SchemeUsesTLS that behaves as davidben suggests. > > 2. Revert https://codereview.chromium.org/1049923002 — it won't be needed after > all. > > 3. Change this CL's content::IsOriginSecure to use a local (content::, not > blink::) copy (vector<string>) of the schemes registered as secure. Sounds like > that'll be lighter-weight than trying to call into blink::. > > In another CL (or set of CLs): > > 4. Change all callers of the current SchemeIsSecure to either > GURL::SchemeUsesTLS or content::IsOriginSecure, as appropriate. > > 5. Get rid of GURL::SchemeIsSecure. > > Does that sound reasonable? 1, 4, 5 SGTM. I don't have much context on 2 and 3 but that sounds reasonable. You might need to fiddle a bit to deal with it being called in both browser and renderer though. I don't know off-hand how those are registered.
Patchset 5 does this: > > 1. Make a GURL::SchemeUsesTLS that behaves as davidben suggests. I'll do this later: > > 2. Revert https://codereview.chromium.org/1049923002 — it won't be needed > after > > all. And I hard-code a list rather than copy things into a vector, because I decided I don't want code running in the renderer to update a list of secure schemes that the browser will check later. Also, new schemes that are secure don't come along every day: > > 3. Change this CL's content::IsOriginSecure to use a local (content::, not > > blink::) copy (vector<string>) of the schemes registered as secure. Sounds > like > > that'll be lighter-weight than trying to call into blink::. What say you all? Does this look reasonable?
On 2015/04/08 00:00:17, palmer wrote: > Patchset 5 does this: > > > > 1. Make a GURL::SchemeUsesTLS that behaves as davidben suggests. > > I'll do this later: > > > > 2. Revert https://codereview.chromium.org/1049923002 — it won't be needed > > after > > > all. > > And I hard-code a list rather than copy things into a vector, because I decided > I don't want code running in the renderer to update a list of secure schemes > that the browser will check later. Also, new schemes that are secure don't come > along every day: > > > > 3. Change this CL's content::IsOriginSecure to use a local (content::, not > > > blink::) copy (vector<string>) of the schemes registered as secure. Sounds > > like > > > that'll be lighter-weight than trying to call into blink::. > > What say you all? Does this look reasonable? This sounds like a good plan to me, although I too am sad about the duplication between blink and content. Seems like a place for future bugs. :( But I see that it is not immediately avoidable.
and i always forget to mail my comments https://codereview.chromium.org/1049533002/diff/80001/url/gurl.h File url/gurl.h (right): https://codereview.chromium.org/1049533002/diff/80001/url/gurl.h#newcode229 url/gurl.h:229: // Chromium's |content::IsOriginSecure|. Can you add an explanation of when to use which one? You probably want X if Y, or Z if Q. https://codereview.chromium.org/1049533002/diff/80001/url/gurl.h#newcode241 url/gurl.h:241: // security. Can you add a note here saying you might want content::IsOriginSecure if ....
Additional documentation of content::IsOriginSecure and IsOriginSecure in chrome/. https://codereview.chromium.org/1049533002/diff/80001/url/gurl.h File url/gurl.h (right): https://codereview.chromium.org/1049533002/diff/80001/url/gurl.h#newcode229 url/gurl.h:229: // Chromium's |content::IsOriginSecure|. On 2015/04/08 00:10:08, felt wrote: > Can you add an explanation of when to use which one? You probably want X if Y, > or Z if Q. Done. https://codereview.chromium.org/1049533002/diff/80001/url/gurl.h#newcode241 url/gurl.h:241: // security. On 2015/04/08 00:10:08, felt wrote: > Can you add a note here saying you might want content::IsOriginSecure if .... Done.
palmer@chromium.org changed reviewers: - davidben@google.com
https://codereview.chromium.org/1049533002/diff/120001/chrome/common/origin_u... File chrome/common/origin_util.h (right): https://codereview.chromium.org/1049533002/diff/120001/chrome/common/origin_u... chrome/common/origin_util.h:15: // See https://www.w3.org/TR/powerful-features/#is-origin-trustworthy. Having both chrome:: and content:: versions of this function seems like it'd cause problems. Code in content that defines polices will likely need to know about origins that chrome considers secure, no? How does Blink learn that these schemes are secure right now? Probably would make sense to route into that. Or maybe moving it into content/common if it only happens in one process. (I see a ContentClient::AddAdditionalSchemes.) https://codereview.chromium.org/1049533002/diff/120001/content/common/url_sch... File content/common/url_schemes_unittest.cc (right): https://codereview.chromium.org/1049533002/diff/120001/content/common/url_sch... content/common/url_schemes_unittest.cc:5: #include "content/public/common/origin_util.h" This this is a test for origin_util.h, it probably should be named origin_util_unittest.cc. https://codereview.chromium.org/1049533002/diff/120001/content/common/url_sch... content/common/url_schemes_unittest.cc:34: content::IsOriginSecure(GURL("http://[::1].example.com/fun.html"))); Some tests for filesystem:? https://codereview.chromium.org/1049533002/diff/120001/content/public/common/... File content/public/common/origin_util.cc (right): https://codereview.chromium.org/1049533002/diff/120001/content/public/common/... content/public/common/origin_util.cc:19: url.inner_url()->SchemeIsSecure()) { This should be IsOriginSecure(url.inner_url()), right? Though even that is tricky because of chrome::IsOriginSecure.
On 2015/04/08 20:31:00, David Benjamin wrote: > Having both chrome:: and content:: versions of this function seems like it'd > cause problems. Code in content that defines polices will likely need to know > about origins that chrome considers secure, no? As I understand it, no. Other programs that embed content/, like Opera or whatever, don't know or care about the things that chrome/ knows about (such as the extension resource schemes — embedders don't have that, or define their own). > How does Blink learn that these schemes are secure right now? blink::WebSecurityPolicy::registerURLSchemeAsSecure. > Probably would > make sense to route into that. That's what I was trying to do in earlier patchsets, but tkent pointed out that that is bad. > Or maybe moving it into content/common if it only > happens in one process. (I see a ContentClient::AddAdditionalSchemes.) I want to trust only the browser process' view of what schemes are secure. > This this is a test for origin_util.h, it probably should be named > origin_util_unittest.cc. Oh, yeah, will do. > Some tests for filesystem:? Will do. > This should be IsOriginSecure(url.inner_url()), right? Though even that is > tricky because of chrome::IsOriginSecure. Will look into it.
Sorry, that was probably unclear. On 2015/04/08 20:35:49, palmer wrote: > On 2015/04/08 20:31:00, David Benjamin wrote: > > > Having both chrome:: and content:: versions of this function seems like it'd > > cause problems. Code in content that defines polices will likely need to know > > about origins that chrome considers secure, no? > > As I understand it, no. Other programs that embed content/, like Opera or > whatever, don't know or care about the things that chrome/ knows about (such as > the extension resource schemes — embedders don't have that, or define their > own). Right, so some API //content exposes to //chrome needs to allow //chrome to register new secure schemes with //content. But I don't think it'd work to say that code in //content calls content::IsSecureOrigin while code in //chrome calls chrome::IsSecureOrigin. Now they'll disagree and that's bound to lead to trouble. I would think //chrome, //content, and Blink should all be in agreement here. (Imagine if content/ checks content::IsSecureOrigin before asking chrome/ to show a permissions prompt. Now chrome-extension:// can't participate in permissions.) I'm proposing that there be a content/common-level RegisterURLSchemeAsSecure API exposed to chrome. Probably it should be part of the ContentClient::AddAdditionalSchemes hook so it's called the same on both sides. That will allow a //content embedder to customize the behavior of content::IsSecureOrigin while keeping //content and //chrome in sync. And then to keep Blink in sync, we can make the WSP::registerURLSchemeAsSecure calls in ChromeContentRendererClient key off of the same mechanism rather than have another copy of the same list. > > How does Blink learn that these schemes are secure right now? > > blink::WebSecurityPolicy::registerURLSchemeAsSecure. > > > Probably would > > make sense to route into that. > > That's what I was trying to do in earlier patchsets, but tkent pointed out that > that is bad. > > > Or maybe moving it into content/common if it only > > happens in one process. (I see a ContentClient::AddAdditionalSchemes.) > > I want to trust only the browser process' view of what schemes are secure. Within the browser process, certainly. By "it" I meant however things are registered in chrome/. You put content::IsOriginSecure in content/common, so I assume you anticipate callers on both sides. By "route into that" I meant having the code that populates registerURLSchemeAsSecure be the same as the code which populates content/ and chrome/'s belief about which origins are secure. (See above.) Certainly the browser process can't get signals from a renderer about this.
On 2015/04/08 20:58:51, David Benjamin wrote: > Right, so some API //content exposes to //chrome needs to allow //chrome to > register new secure schemes with //content. But I don't think it'd work to say > that code in //content calls content::IsSecureOrigin while code in //chrome > calls chrome::IsSecureOrigin. Now they'll disagree chrome::IsSecureOrigin is a superset of content::IsSecureOrigin, because the former calls the latter as 1 of its checks. > and that's bound to lead to > trouble. I would think //chrome, //content, and Blink should all be in agreement > here. They cannot be. They don't all have the same information, and they don't all even need to have the same information. > (Imagine if content/ checks content::IsSecureOrigin before asking chrome/ > to show a permissions prompt. Now chrome-extension:// can't participate in > permissions.) Permission decisions should happen in chrome/, or in whatever other embedder, at which point all relevant information is available. > I'm proposing that there be a content/common-level RegisterURLSchemeAsSecure API > exposed to chrome. Probably it should be part of the > ContentClient::AddAdditionalSchemes hook so it's called the same on both sides. The API for AddAdditionalSchemes is: 88 // Gives the embedder a chance to register its own standard and saveable 89 // url schemes early on in the startup sequence. That "standard" and "saveable" is not the same as "secure, as described in the Powerful Features" spec. This is getting way too complicated for what is essentially a few simple set membership checks. The layers and layers and layers of the component design are impeding development, rather than aiding development. If other chrome/ and content/ OWNERS agree davidben's approach is the way to go, I'll try it. But otherwise I think we should go simple here. (It's already not simple enough, and complex security mechanisms are bad security mechanisms.)
On 2015/04/08 21:54:57, palmer wrote: > On 2015/04/08 20:58:51, David Benjamin wrote: > > > Right, so some API //content exposes to //chrome needs to allow //chrome to > > register new secure schemes with //content. But I don't think it'd work to say > > that code in //content calls content::IsSecureOrigin while code in //chrome > > calls chrome::IsSecureOrigin. Now they'll disagree > > chrome::IsSecureOrigin is a superset of content::IsSecureOrigin, because the > former calls the latter as 1 of its checks. > > > and that's bound to lead to > > trouble. I would think //chrome, //content, and Blink should all be in > agreement > > here. > > They cannot be. They don't all have the same information, and they don't all > even need to have the same information. > > > (Imagine if content/ checks content::IsSecureOrigin before asking chrome/ > > to show a permissions prompt. Now chrome-extension:// can't participate in > > permissions.) > > Permission decisions should happen in chrome/, or in whatever other embedder, at > which point all relevant information is available. > > > I'm proposing that there be a content/common-level RegisterURLSchemeAsSecure > API > > exposed to chrome. Probably it should be part of the > > ContentClient::AddAdditionalSchemes hook so it's called the same on both > sides. > > The API for AddAdditionalSchemes is: > > 88 // Gives the embedder a chance to register its own standard and saveable > 89 // url schemes early on in the startup sequence. > > That "standard" and "saveable" is not the same as "secure, as described in the > Powerful Features" spec. Sure. You'd need to modify it or add a different, similar hook. > This is getting way too complicated for what is essentially a few simple set > membership checks. The layers and layers and layers of the component design are > impeding development, rather than aiding development. > > If other chrome/ and content/ OWNERS agree davidben's approach is the way to go, > I'll try it. But otherwise I think we should go simple here. (It's already not > simple enough, and complex security mechanisms are bad security mechanisms.) I guess the question is: do you expect content::IsSecureOrigin to ever be called anywhere, outside of chrome::IsSecureOrigin as an implementation detail? Naively I would expect that there exists (or may exist in future) code in //content that will want to do such a check. And if content::IsSecureOrigin gets called there, we'll have two components of the browser have different opinions on what IsSecureOrigin means. If the intent is for it to just be an implementation detail of chrome::IsSecureOrigin and never called in content, it should be documented (and named) as such. Or perhaps we just not put it in content at all and only in chrome. That would mean that content is not allowed to anywhere distinguish between secure and non-secure origins (and if we need to change that later, we add a hook to ContentClient somewhere). I'm fine with that too. I just don't like there being two versions of the same function with subtly different meanings, since mixing them up will cause bugs.
I think the problem is that this patch is too hard to discuss without seeing the users and what they expect and what the alternatives are. While it's nice to keep this patch small and focused, can we have a dependent CL that adds all the callers of this function (or, if there are a lot, some representative sample). I think it will be much easier to discuss looking at such a change.
> If the intent is for it to just be an implementation detail of > chrome::IsSecureOrigin and never called in content, it should be documented (and > named) as such. Or perhaps we just not put it in content at all and only in > chrome. That would mean that content is not allowed to anywhere distinguish > between secure and non-secure origins (and if we need to change that later, we > add a hook to ContentClient somewhere). I'm fine with that too. I just don't > like there being two versions of the same function with subtly different > meanings, since mixing them up will cause bugs. OK, coming up is a CL that has only a function in chrome, not in content. brettw: I will also put up a dependent CL showing invocations of the new stuff.
PTAL, and a CL with some callers is on the way.
First caller: https://codereview.chromium.org/1087983002/
Some callers for GURL::SchemeUsesTLS: https://codereview.chromium.org/1082083004
estark@chromium.org changed reviewers: + estark@chromium.org
https://codereview.chromium.org/1049533002/diff/140001/chrome/common/origin_u... File chrome/common/origin_util.cc (right): https://codereview.chromium.org/1049533002/diff/140001/chrome/common/origin_u... chrome/common/origin_util.cc:22: if (net::IsLocalhost(hostname) || net::IsLocalhostTLD(hostname)) Was just looking at this out of curiosity and noticed that the |IsLocalhostTLD| check isn't necessary here; |IsLocalhost| already checks that. https://code.google.com/p/chromium/codesearch#chromium/src/net/base/net_util.... (Sorry if that wasn't clear... I guess the comment on the |IsLocahost| declaration should say so.) https://codereview.chromium.org/1049533002/diff/140001/url/gurl.h File url/gurl.h (left): https://codereview.chromium.org/1049533002/diff/140001/url/gurl.h#oldcode244 url/gurl.h:244: // This currently identifies only IPv4 addresses (bug 822685). ohh, thanks, I had an open bug for updating this comment, I'm going to give it to you now :)
markusheintz@chromium.org changed reviewers: + markusheintz@chromium.org
https://codereview.chromium.org/1049533002/diff/140001/chrome/common/origin_u... File chrome/common/origin_util.cc (right): https://codereview.chromium.org/1049533002/diff/140001/chrome/common/origin_u... chrome/common/origin_util.cc:13: if (url.SchemeUsesTLS() || url.SchemeIsFile()) hm ... I may remember something wrong here (sorry if that is the case) but didn't you or somebody else from the security team plan to treat file URLs as not secure? They could be from some network share for example.
https://codereview.chromium.org/1049533002/diff/140001/url/gurl.h File url/gurl.h (right): https://codereview.chromium.org/1049533002/diff/140001/url/gurl.h#newcode248 url/gurl.h:248: // |content::IsOriginSecure| and |IsOriginSecure| for a higher-level and more This comment may be outdated since the mentioned code is not in content anymore.
looks good modulo the various comments on comments (hah). Though I don't think I'm useful for OWNERS anymore anyway. https://codereview.chromium.org/1049533002/diff/140001/chrome/common/origin_u... File chrome/common/origin_util.h (right): https://codereview.chromium.org/1049533002/diff/140001/chrome/common/origin_u... chrome/common/origin_util.h:13: // checks for certain Chromium-specific secure URL schemes. (This comment is also outdated.)
Thanks all! https://codereview.chromium.org/1049533002/diff/140001/chrome/common/origin_u... File chrome/common/origin_util.cc (right): https://codereview.chromium.org/1049533002/diff/140001/chrome/common/origin_u... chrome/common/origin_util.cc:13: if (url.SchemeUsesTLS() || url.SchemeIsFile()) > hm ... I may remember something wrong here (sorry if that is the case) but > didn't you or somebody else from the security team plan to treat file URLs as > not secure? > > They could be from some network share for example. We decided that, as far as the browser can know, the files effectively come from the local machine. They could have been copied from a non-secure network filesystem like SMB/CIFS, but we think that is relatively rare and not something the browser can reliably determine. We wanted to err on the side of usability for developers, and make secure-origin-only features available to file://. https://codereview.chromium.org/1049533002/diff/140001/chrome/common/origin_u... chrome/common/origin_util.cc:22: if (net::IsLocalhost(hostname) || net::IsLocalhostTLD(hostname)) > Was just looking at this out of curiosity and noticed that the |IsLocalhostTLD| > check isn't necessary here; |IsLocalhost| already checks that. Ah, thanks. > (Sorry if that wasn't clear... I guess the comment on the |IsLocahost| > declaration should say so.) I think it is clear in retrospect. :) https://codereview.chromium.org/1049533002/diff/140001/chrome/common/origin_u... File chrome/common/origin_util.h (right): https://codereview.chromium.org/1049533002/diff/140001/chrome/common/origin_u... chrome/common/origin_util.h:13: // checks for certain Chromium-specific secure URL schemes. On 2015/04/15 15:45:40, David Benjamin wrote: > (This comment is also outdated.) Done. https://codereview.chromium.org/1049533002/diff/140001/url/gurl.h File url/gurl.h (left): https://codereview.chromium.org/1049533002/diff/140001/url/gurl.h#oldcode244 url/gurl.h:244: // This currently identifies only IPv4 addresses (bug 822685). On 2015/04/15 00:35:52, estark wrote: > ohh, thanks, I had an open bug for updating this comment, I'm going to give it > to you now :) Acknowledged. https://codereview.chromium.org/1049533002/diff/140001/url/gurl.h File url/gurl.h (right): https://codereview.chromium.org/1049533002/diff/140001/url/gurl.h#newcode248 url/gurl.h:248: // |content::IsOriginSecure| and |IsOriginSecure| for a higher-level and more On 2015/04/15 14:15:09, markusheintz_ wrote: > This comment may be outdated since the mentioned code is not in content anymore. Done.
OK, PTAL. Perhaps this CL is ready to fly?
lgtm https://codereview.chromium.org/1049533002/diff/160001/url/gurl.h File url/gurl.h (right): https://codereview.chromium.org/1049533002/diff/160001/url/gurl.h#newcode235 url/gurl.h:235: // |content::IsOriginSecure|, as appropriate. Then remove |SchemeIsSecure|. Missed a spot. :-) content::IsOriginSecure -> IsOriginSecure
https://codereview.chromium.org/1049533002/diff/160001/url/gurl.h File url/gurl.h (right): https://codereview.chromium.org/1049533002/diff/160001/url/gurl.h#newcode235 url/gurl.h:235: // |content::IsOriginSecure|, as appropriate. Then remove |SchemeIsSecure|. On 2015/04/16 17:20:36, David Benjamin wrote: > Missed a spot. :-) > > content::IsOriginSecure -> IsOriginSecure Done.
On 2015/04/16 17:26:07, palmer wrote: > https://codereview.chromium.org/1049533002/diff/160001/url/gurl.h > File url/gurl.h (right): > > https://codereview.chromium.org/1049533002/diff/160001/url/gurl.h#newcode235 > url/gurl.h:235: // |content::IsOriginSecure|, as appropriate. Then remove > |SchemeIsSecure|. > On 2015/04/16 17:20:36, David Benjamin wrote: > > Missed a spot. :-) > > > > content::IsOriginSecure -> IsOriginSecure > > Done. LGTM
The CQ bit was checked by palmer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org, davidben@chromium.org Link to the patchset: https://codereview.chromium.org/1049533002/#ps180001 (title: "Fix the comments for real.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1049533002/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/6c3473c3489dae0bfd2133c22e6b5cd9fb4f5fef Cr-Commit-Position: refs/heads/master@{#325495}
Message was sent while issue was closed.
rsleevi@chromium.org changed reviewers: + rsleevi@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1049533002/diff/180001/url/gurl.h File url/gurl.h (right): https://codereview.chromium.org/1049533002/diff/180001/url/gurl.h#newcode249 url/gurl.h:249: bool SchemeUsesTLS() const { Suuuuuper-annoying pedantry: These schemes could use QUIC, rather than TLS. SchemeUsesSecureTransport? SchemeUsesSecureNetwork? SchemeIsNetworkSecure? IDK. It's not a hard blocker, but at some point I suspect we're going to have to reword and rename this, since it's already not true today for a large # of connections that "TLS" is being used :/
Message was sent while issue was closed.
SchemeIsCryptographic?
Message was sent while issue was closed.
On 2015/04/20 22:52:21, palmer wrote: > SchemeIsCryptographic? while we're painting a bikeshed, I'm in favor of SchemeUsesSecureTransport, or SchemeIsNetworkSecure ;)
Message was sent while issue was closed.
> while we're painting a bikeshed, I'm in favor of SchemeUsesSecureTransport, or > SchemeIsNetworkSecure ;) Ah, but SchemeUsesSecureTransport covers things like file://, since as far as the browser knows, there are no MITMs between it and the filesystem. SchemeIsNetworkSecure is OK, but reads a little oddly to me. SchemeIsCyber? Also a good bikeshed should have eaves sheltering a little tire pump station. And a flower pot.
Message was sent while issue was closed.
On 2015/04/20 23:56:43, palmer wrote: > Also a good bikeshed should have eaves sheltering a little tire pump station. > And a flower pot. Joking aside, I'm not sure I think of the filesystem as a "transport", hence why I suggested SecureTransport. I mean, arguably "SchemeIsTransportLayerSecure" is a viable read, it's just that the "transport layer security" isn't "Transport Layer Security"... I'm admittedly understandably nervous about saying "secure", since "crap TLS" is one of those "a little of column a, a little of column b" cases. SchemeIsCryptographic works, but I fear that will be overloaded with something like .onion / .iot things (which are host, not scheme, but since when has that distinction ever mattered when reading code...) I still lean on "SchemeIsNetworkSecure", but I mean, ultimately this is pedantry that won't really become an issue until we're more standardizing QUIC. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
