Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1224293002:
Introduce 'url::Origin'. (Closed)
DescriptionIntroduce 'url::Origin'.
https://docs.google.com/document/d/19NACt9PXOUTJi60klT2ZGcFlgHM5wM1Owtcw2GQOKPI/edit
describes the plan.
BUG=490074
Committed: https://crrev.com/9f2cc898386458c3fab414666eb205ecb5b88277
Cr-Commit-Position: refs/heads/master@{#339841}
Patch Set 1 #
Total comments: 37
Patch Set 2 : Feedback. #
Total comments: 7
Patch Set 3 : sleevi-feedback #
Total comments: 6
Patch Set 4 : Jochen. #
Total comments: 5
Patch Set 5 : Ryan. #Patch Set 6 : Nits. #
Total comments: 1
Messages
Total messages: 22 (4 generated)
mkwst@chromium.org changed reviewers: + jochen@chromium.org, nasko@chromium.org, rsleevi@chromium.org
WDYT? `url::Origin` strawman... Not entirely sure what to do about `file:`, as we treat it as unique or not based on a command-line flag, which seems like a bad thing to bring into //url. :(
On 2015/07/09 at 14:37:32, mkwst wrote: > WDYT? `url::Origin` strawman... > > Not entirely sure what to do about `file:`, as we treat it as unique or not based on a command-line flag, which seems like a bad thing to bring into //url. :( we could have a static setter that toggles a flag what to do about file
On 2015/07/10 at 08:59:56, jochen wrote: > On 2015/07/09 at 14:37:32, mkwst wrote: > > WDYT? `url::Origin` strawman... > > > > Not entirely sure what to do about `file:`, as we treat it as unique or not based on a command-line flag, which seems like a bad thing to bring into //url. :( > > we could have a static setter that toggles a flag what to do about file Yeah. That does seem like a solution, I just don't like globals. :/ Where would we call this setter? It's not really clear what the earliest reasonable point in execution would be?
On 2015/07/10 at 09:17:04, mkwst wrote: > On 2015/07/10 at 08:59:56, jochen wrote: > > On 2015/07/09 at 14:37:32, mkwst wrote: > > > WDYT? `url::Origin` strawman... > > > > > > Not entirely sure what to do about `file:`, as we treat it as unique or not based on a command-line flag, which seems like a bad thing to bring into //url. :( > > > > we could have a static setter that toggles a flag what to do about file > > Yeah. That does seem like a solution, I just don't like globals. :/ Where would we call this setter? It's not really clear what the earliest reasonable point in execution would be? content::ContentMainRunnerImpl::Initialize maybe
On 2015/07/10 at 09:26:28, jochen wrote: > On 2015/07/10 at 09:17:04, mkwst wrote: > > On 2015/07/10 at 08:59:56, jochen wrote: > > > On 2015/07/09 at 14:37:32, mkwst wrote: > > > > WDYT? `url::Origin` strawman... > > > > > > > > Not entirely sure what to do about `file:`, as we treat it as unique or not based on a command-line flag, which seems like a bad thing to bring into //url. :( > > > > > > we could have a static setter that toggles a flag what to do about file > > > > Yeah. That does seem like a solution, I just don't like globals. :/ Where would we call this setter? It's not really clear what the earliest reasonable point in execution would be? > > content::ContentMainRunnerImpl::Initialize maybe Actually, I'm not sure that will work. The flag ends up being set on a renderer-by-renderer basis. We expose it as a WebView API (http://developer.android.com/reference/android/webkit/WebSettings.html#getAll... ), and as a switch (kAllowFileAccessFromFiles). The latter is global, the former isn't. :/ I think we'll just end up treating `file:` as a unique origin, but adding an `isFile` flag which `OriginAccessPolicy` can use in order to make a decision when asked.
https://codereview.chromium.org/1224293002/diff/1/url/origin.cc File url/origin.cc (right): https://codereview.chromium.org/1224293002/diff/1/url/origin.cc#newcode35 url/origin.cc:35: // to parsing the URL after stripping off the scheme. something something "we" in comments. But more importantly, this doesn't seem to be an "Alternatively", since it seems to be restating the last sentence of the first part of this comment ("so it might be worth ...") https://codereview.chromium.org/1224293002/diff/1/url/origin.h File url/origin.h (right): https://codereview.chromium.org/1224293002/diff/1/url/origin.h#newcode21 url/origin.h:21: // An Origin is a tuple of (scheme, host, port), as described in RFC 6454. I would hope that immediately after stating this, in the first paragraph (e.g. before line 23), explain explicitly how these differ from url::SchemeHostPort. Otherwise, I think the confusion will still exist. Since Origin is composed on SchemeHostPort, that sort of 'downreference' in comments is certainly allowed. My stab, which is probably bad and I should feel bad, is // While both SchemeHostPort and Origin are composed of tuples of (scheme, host, port), it's important to // understand they reflect different conceptual models. A SchemeHostPort reflects the underlying storage of // a standard-schemed URL (that is, the generic URL syntax of RFC 3986, using scheme://authority ), where // authority is a host and optional port (ignoring the user@ portion). However, an Origin is the fundamental // concept of the web's security model, and represents the boundaries in which user agents generally // compartmentalize information, and between which user agents enforce access controls. // // An Origin may be created from a variety of URLs, not just those using the generic syntax, and an // Origin may not necessarily represent a reachable network endpoint. For example, an empty Origin // is a valid concept, indicating that it is unique and different from all other Origins, including itself, // while an empty SchemeHostPort is an invalid object. // // The choice between whether to use Origin or SchemeHostPort depends on the context. For // example, if recording a list of active network connections, a SchemeHostPort may be wholly // appropriate - it's just a storage of the tuple of information used to establish that network connection. // However, if performing access checks for some resource, for persisting things in to long term // storage, or for determining access controls between two arbitrary URLs, it's generally more // appropriate to use an Origin, to ensure that the security separation between Origins is preserved. // // There are a few subtleties to note: That's way wordier, and I'm sure you can further distill it to core points. But I think as we land this, we want to provide clear and canonical documentation for people about when they should prefer things. Likewise, in SchemeHostPort, we want to be clear to indicate that it shouldn't be used for security-relevant access checks, and instead should be seen moreso as a cache key of sorts for network-related information. https://codereview.chromium.org/1224293002/diff/1/url/origin.h#newcode40 url/origin.h:40: // and are difficult to reason about in the abstract. I feel like something more may be warranted to here. https://codereview.chromium.org/1224293002/diff/1/url/origin.h#newcode43 url/origin.h:43: // serializations of two unique origins are '==' to each other, though the s/are '==' to each other/are identical/ https://codereview.chromium.org/1224293002/diff/1/url/origin.h#newcode52 url/origin.h:52: // representation. Why is this? I found it a bit surprising? https://codereview.chromium.org/1224293002/diff/1/url/origin.h#newcode56 url/origin.h:56: // * Origins are generally constructed from GURL objects: When I first read this comment, I had to scroll down to see if you had an explicit scheme/host/port constructor, since that what I was expecting with the way this was worded. Perhaps an alternative wording // * Unless creating a unique Origin, Origins are constructed from an already canonicalized GURL: https://codereview.chromium.org/1224293002/diff/1/url/origin.h#newcode58 url/origin.h:58: // GURL url("https://example.com/"); wrong indent level (2 != 4) https://codereview.chromium.org/1224293002/diff/1/url/origin.h#newcode68 url/origin.h:68: // if (this.IsSameOriginWith(that)) { wrong indent level (2 != 4) https://codereview.chromium.org/1224293002/diff/1/url/origin.h#newcode69 url/origin.h:69: // // Amazingness goes here. wrong indent level (2 != 4) https://codereview.chromium.org/1224293002/diff/1/url/origin.h#newcode92 url/origin.h:92: void ForceUnique() { unique_ = true; } I find this quite surprising. Why allow mutable state at all? I'd much rather say you need to update the Origin to a new, unique origin, rather than have this method. https://codereview.chromium.org/1224293002/diff/1/url/origin.h#newcode106 url/origin.h:106: bool operator>(const Origin& other) const; operator> is not needed. All that's needed for anything on the StrictWeakOrdering is an operator< or an std::less<typename> override, with a preference for the former. With strict weak ordering requirements, operator < is presumed to have symmetric qualities, such that (a < b) == !(b < a). Equality is then equivalent to !(a < b) && !(b < a). https://codereview.chromium.org/1224293002/diff/1/url/origin.h#newcode110 url/origin.h:110: bool unique_; DISALLOW_COPY_AND_ASSIGN ? https://codereview.chromium.org/1224293002/diff/1/url/origin_unittest.cc File url/origin_unittest.cc (right): https://codereview.chromium.org/1224293002/diff/1/url/origin_unittest.cc#newc... url/origin_unittest.cc:11: TEST(OriginTest, uniqueOrigin) { Not in Blink, so Chromium naming TEST(OriginTest, HandlesUniqueOrigin) ? https://codereview.chromium.org/1224293002/diff/1/url/origin_unittest.cc#newc... url/origin_unittest.cc:12: url::Origin uniqueOrigin; unique_origin https://codereview.chromium.org/1224293002/diff/1/url/origin_unittest.cc#newc... url/origin_unittest.cc:19: const char* urls[] = {"data:text/html,Hello!", const char* const urls[] https://codereview.chromium.org/1224293002/diff/1/url/origin_unittest.cc#newc... url/origin_unittest.cc:39: TEST(OriginTest, GURLConstruction) { nit: TEST(OriginTest, ConstructFromGURL) { https://codereview.chromium.org/1224293002/diff/1/url/origin_unittest.cc#newc... url/origin_unittest.cc:40: url::Origin differentOrigin(GURL("https://not-in-the-list.test/")); different_origin https://codereview.chromium.org/1224293002/diff/1/url/origin_unittest.cc#newc... url/origin_unittest.cc:43: const char* url; const char* const (and friends) https://codereview.chromium.org/1224293002/diff/1/url/origin_unittest.cc#newc... url/origin_unittest.cc:46: uint16 port; These are all expected_scheme, expected_host, expected_port, correct? https://codereview.chromium.org/1224293002/diff/1/url/origin_unittest.cc#newc... url/origin_unittest.cc:47: } cases[] = { Perhaps add some test cases for non-generic but registered schemes ( http://www.iana.org/assignments/uri-schemes/uri-schemes.xhtml has a variety of em) https://codereview.chromium.org/1224293002/diff/1/url/origin_unittest.cc#newc... url/origin_unittest.cc:56: // Webby URLs I cringe somewhat at the colloquial "webby" - "generic URLs" https://codereview.chromium.org/1224293002/diff/1/url/origin_unittest.cc#newc... url/origin_unittest.cc:85: GURL url(test.url); perhaps your test should make sure that the GURL itself is valid before you punt to Origin, otherwise you could interpret an "invalid URL that should have been valid and unique" as a test success https://codereview.chromium.org/1224293002/diff/1/url/origin_unittest.cc#newc... url/origin_unittest.cc:99: const char* url; const char* const (and friends)
Thanks Ryan. Mind taking another look? (Nasko, Jochen: feel free to poke at it as well. :) ) https://codereview.chromium.org/1224293002/diff/1/url/origin.cc File url/origin.cc (right): https://codereview.chromium.org/1224293002/diff/1/url/origin.cc#newcode35 url/origin.cc:35: // to parsing the URL after stripping off the scheme. On 2015/07/10 at 11:59:53, Ryan Sleevi (slow through 7-15 wrote: > something something "we" in comments. Something something I (we!) still think that's a terrible idea. > But more importantly, this doesn't seem to be an "Alternatively", since it seems to be restating the last sentence of the first part of this comment ("so it might be worth ...") Dropped it. https://codereview.chromium.org/1224293002/diff/1/url/origin.h File url/origin.h (right): https://codereview.chromium.org/1224293002/diff/1/url/origin.h#newcode21 url/origin.h:21: // An Origin is a tuple of (scheme, host, port), as described in RFC 6454. On 2015/07/10 at 11:59:54, Ryan Sleevi (slow through 7-15 wrote: > That's way wordier, and I'm sure you can further distill it to core points. But I think as we land this, we want to provide clear and canonical documentation for people about when they should prefer things. Likewise, in SchemeHostPort, we want to be clear to indicate that it shouldn't be used for security-relevant access checks, and instead should be seen moreso as a cache key of sorts for network-related information. Poked at the text a bit. https://codereview.chromium.org/1224293002/diff/1/url/origin.h#newcode43 url/origin.h:43: // serializations of two unique origins are '==' to each other, though the On 2015/07/10 at 11:59:54, Ryan Sleevi (slow through 7-15 wrote: > s/are '==' to each other/are identical/ Done. https://codereview.chromium.org/1224293002/diff/1/url/origin.h#newcode52 url/origin.h:52: // representation. On 2015/07/10 at 11:59:53, Ryan Sleevi (slow through 7-15 wrote: > Why is this? I found it a bit surprising? Why wouldn't we do this? It matches the representation you'll get in JavaScript, and the representation you get from GURL. https://codereview.chromium.org/1224293002/diff/1/url/origin.h#newcode56 url/origin.h:56: // * Origins are generally constructed from GURL objects: On 2015/07/10 at 11:59:53, Ryan Sleevi (slow through 7-15 wrote: > // * Unless creating a unique Origin, Origins are constructed from an already canonicalized GURL: Ran with something like that. https://codereview.chromium.org/1224293002/diff/1/url/origin.h#newcode58 url/origin.h:58: // GURL url("https://example.com/"); On 2015/07/10 at 11:59:53, Ryan Sleevi (slow through 7-15 wrote: > wrong indent level (2 != 4) [Insert eye rolling here.] https://codereview.chromium.org/1224293002/diff/1/url/origin.h#newcode68 url/origin.h:68: // if (this.IsSameOriginWith(that)) { On 2015/07/10 at 11:59:53, Ryan Sleevi (slow through 7-15 wrote: > wrong indent level (2 != 4) [And here] https://codereview.chromium.org/1224293002/diff/1/url/origin.h#newcode69 url/origin.h:69: // // Amazingness goes here. On 2015/07/10 at 11:59:54, Ryan Sleevi (slow through 7-15 wrote: > wrong indent level (2 != 4) [Also here] https://codereview.chromium.org/1224293002/diff/1/url/origin.h#newcode92 url/origin.h:92: void ForceUnique() { unique_ = true; } On 2015/07/10 at 11:59:53, Ryan Sleevi (slow through 7-15 wrote: > I find this quite surprising. Why allow mutable state at all? I'd much rather say you need to update the Origin to a new, unique origin, rather than have this method. I think we'll need this for `file:` URLs, as we either treat them as unique or not depending on decisions the embedder makes. This, of course, is absurd, but it's the world we live in. I can drop it for the moment, and add it back in when we need it. *shrug* https://codereview.chromium.org/1224293002/diff/1/url/origin.h#newcode106 url/origin.h:106: bool operator>(const Origin& other) const; On 2015/07/10 at 11:59:54, Ryan Sleevi (slow through 7-15 wrote: > operator> is not needed. All that's needed for anything on the StrictWeakOrdering is an operator< or an std::less<typename> override, with a preference for the former. > > With strict weak ordering requirements, operator < is presumed to have symmetric qualities, such that (a < b) == !(b < a). Equality is then equivalent to !(a < b) && !(b < a). Done. https://codereview.chromium.org/1224293002/diff/1/url/origin_unittest.cc File url/origin_unittest.cc (right): https://codereview.chromium.org/1224293002/diff/1/url/origin_unittest.cc#newc... url/origin_unittest.cc:11: TEST(OriginTest, uniqueOrigin) { On 2015/07/10 at 11:59:54, Ryan Sleevi (slow through 7-15 wrote: > Not in Blink, so Chromium naming GRRRRR. > TEST(OriginTest, HandlesUniqueOrigin) ? UniqueOriginComparison https://codereview.chromium.org/1224293002/diff/1/url/origin_unittest.cc#newc... url/origin_unittest.cc:12: url::Origin uniqueOrigin; On 2015/07/10 at 11:59:54, Ryan Sleevi (slow through 7-15 wrote: > unique_origin GRRR. https://codereview.chromium.org/1224293002/diff/1/url/origin_unittest.cc#newc... url/origin_unittest.cc:19: const char* urls[] = {"data:text/html,Hello!", On 2015/07/10 at 11:59:54, Ryan Sleevi (slow through 7-15 wrote: > const char* const urls[] const char* const const_urls[] const https://codereview.chromium.org/1224293002/diff/1/url/origin_unittest.cc#newc... url/origin_unittest.cc:56: // Webby URLs On 2015/07/10 at 11:59:54, Ryan Sleevi (slow through 7-15 wrote: > I cringe somewhat at the colloquial "webby" - "generic URLs" You are no fun. :(
LGTM % nits that will require another CL :/ https://codereview.chromium.org/1224293002/diff/20001/url/origin.h File url/origin.h (right): https://codereview.chromium.org/1224293002/diff/20001/url/origin.h#newcode27 url/origin.h:27: // STL;SDR: If you're not working in //url or //net, use 'url::Origin'. :( This feels like a layering violation to discuss higher-level components and their usages. I defer to Brett here, but when reviewing comments, I prefer they're treated like code - that is, observe the same knowledge of "Talk about what you use and how to use things, but not about how you're _used_". I know Blink is more... inconsistent... in this respect. https://codereview.chromium.org/1224293002/diff/20001/url/origin.h#newcode92 url/origin.h:92: std::string host() const { return tuple_.host(); } Bummer! I was asleep at the wheel on url::SchemeHostPort! Both it, and this, should preferably be using "const std::string&" as their return types, rather than making a full copy of the string. Before you can fix this, you'd have to fix url::SchemeHostPort. https://codereview.chromium.org/1224293002/diff/20001/url/origin_unittest.cc File url/origin_unittest.cc (right): https://codereview.chromium.org/1224293002/diff/20001/url/origin_unittest.cc#... url/origin_unittest.cc:25: for (const auto& test : urls) { nit: test_url ? https://codereview.chromium.org/1224293002/diff/20001/url/origin_unittest.cc#... url/origin_unittest.cc:89: for (const auto& test : cases) { nit: test_case ? https://codereview.chromium.org/1224293002/diff/20001/url/origin_unittest.cc#... url/origin_unittest.cc:120: for (const auto& test : cases) { nit: test_case ?
mkwst@chromium.org changed reviewers: + brettw@chromium.org
Thanks, Ryan! I'll clean those up. Brett, WDYT?
lgtm with comments https://codereview.chromium.org/1224293002/diff/40001/url/origin.h File url/origin.h (right): https://codereview.chromium.org/1224293002/diff/40001/url/origin.h#newcode87 url/origin.h:87: Origin(const GURL& url); explicit? https://codereview.chromium.org/1224293002/diff/40001/url/origin.h#newcode93 url/origin.h:93: std::string scheme() const { return tuple_.scheme(); } can you document what those return for unique origins? https://codereview.chromium.org/1224293002/diff/40001/url/origin.h#newcode116 url/origin.h:116: }; nit. maybe add operator<< for ostream?
Thanks, Jochen! https://codereview.chromium.org/1224293002/diff/20001/url/origin.h File url/origin.h (right): https://codereview.chromium.org/1224293002/diff/20001/url/origin.h#newcode27 url/origin.h:27: // STL;SDR: If you're not working in //url or //net, use 'url::Origin'. On 2015/07/17 at 19:48:18, Ryan Sleevi wrote: > :( > > This feels like a layering violation to discuss higher-level components and their usages. I defer to Brett here, but when reviewing comments, I prefer they're treated like code - that is, observe the same knowledge of "Talk about what you use and how to use things, but not about how you're _used_". Rephrased. I still think giving advice about how to use things _is_ giving advice about how the thing is _used_. And that that's good. Perhaps you're happy with the new sentence? > I know Blink is more... inconsistent... in this respect. You mean "Awesomer", right? https://codereview.chromium.org/1224293002/diff/20001/url/origin.h#newcode92 url/origin.h:92: std::string host() const { return tuple_.host(); } On 2015/07/17 at 19:48:18, Ryan Sleevi wrote: > Bummer! I was asleep at the wheel on url::SchemeHostPort! > > Both it, and this, should preferably be using "const std::string&" as their return types, rather than making a full copy of the string. > > Before you can fix this, you'd have to fix url::SchemeHostPort. I think you're right. Oops! Will fix in a followup CL: https://codereview.chromium.org/1229003007 https://codereview.chromium.org/1224293002/diff/40001/url/origin.h File url/origin.h (right): https://codereview.chromium.org/1224293002/diff/40001/url/origin.h#newcode87 url/origin.h:87: Origin(const GURL& url); On 2015/07/20 at 08:13:04, jochen wrote: > explicit? Yup. https://codereview.chromium.org/1224293002/diff/40001/url/origin.h#newcode93 url/origin.h:93: std::string scheme() const { return tuple_.scheme(); } On 2015/07/20 at 08:13:04, jochen wrote: > can you document what those return for unique origins? Done. https://codereview.chromium.org/1224293002/diff/40001/url/origin.h#newcode116 url/origin.h:116: }; On 2015/07/20 at 08:13:04, jochen wrote: > nit. maybe add operator<< for ostream? Sounds reasonable. Added, and expanded the `Serialize()` test to cover it.
LGTM % nits https://codereview.chromium.org/1224293002/diff/60001/url/origin.cc File url/origin.cc (right): https://codereview.chromium.org/1224293002/diff/60001/url/origin.cc#newcode66 url/origin.cc:66: return out << origin.Serialize().c_str(); Why .c_str()? Why not just << and let the default serializer work? https://codereview.chromium.org/1224293002/diff/60001/url/origin.h File url/origin.h (right): https://codereview.chromium.org/1224293002/diff/60001/url/origin.h#newcode114 url/origin.h:114: url::SchemeHostPort tuple_; It's worth noting you're in the url:: namespace here, so the qualifier isn't needed. https://codereview.chromium.org/1224293002/diff/60001/url/origin.h#newcode121 url/origin.h:121: const url::Origin& origin); It's worth noting you're in the url:: namespace here, so the qualifier isn't needed.
Friendly ping, Brett. :)
LGTM with nits. https://codereview.chromium.org/1224293002/diff/60001/url/origin.cc File url/origin.cc (right): https://codereview.chromium.org/1224293002/diff/60001/url/origin.cc#newcode24 url/origin.cc:24: return; Indenting. https://codereview.chromium.org/1224293002/diff/60001/url/origin.h File url/origin.h (right): https://codereview.chromium.org/1224293002/diff/60001/url/origin.h#newcode105 url/origin.h:105: // Two Origins are "same-origin" iff their schemes, hosts, and ports are exact iff -> if. It's easier to read and still means the same thing in this context.
The CQ bit was checked by mkwst@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, rsleevi@chromium.org, brettw@chromium.org Link to the patchset: https://codereview.chromium.org/1224293002/#ps100001 (title: "Nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1224293002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/9f2cc898386458c3fab414666eb205ecb5b88277 Cr-Commit-Position: refs/heads/master@{#339841}
Message was sent while issue was closed.
Sorry for the late reply here. Just one nit on a drive-by review. https://codereview.chromium.org/1224293002/diff/100001/url/origin.h File url/origin.h (right): https://codereview.chromium.org/1224293002/diff/100001/url/origin.h#newcode109 url/origin.h:109: // Allows SchemeHostPort to used as a key in STL (for example, a std::set or nit: s/SchemeHostPort/Origin/? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
