Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1985703003:
Add isUnique check to isSameSchemeHostPort. (Closed)
|
Created:
4 years, 7 months ago by Marijn Kruisselbrink Modified:
4 years, 6 months ago CC:
blink-reviews, chromium-reviews, kinuko+watch Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd isUnique check to isSameSchemeHostPort.
The method is documented as "checks for equality", but before considered
any pair of unique origins to be equal. With this change only unique origins
that are the same instance are considered equal.
Committed: https://crrev.com/0fb92722bc63b2cf98bf6502494a6f77e430fc1a
Cr-Commit-Position: refs/heads/master@{#399890}
Patch Set 1 #Patch Set 2 : change check rather than comment #
Total comments: 2
Patch Set 3 : separate out == check, and add some tests #
Total comments: 2
Messages
Total messages: 30 (10 generated)
mek@chromium.org changed reviewers: + tsepez@chromium.org
Maybe adding a true isSameOrigin method would also make sense, but at least being more explicit in the description of isSameSchemeHostPort is probably a good idea. It does kind of feel like a method that treats any pair of unique origins as equal is a kind of a foot-gun, but probably there is good reasons why this behaves that way?
Description was changed from ========== Clarify surprising (to me) behavior of isSameSchemeHostPort. The method was commented as "checks for equality", but doesn't mention that it considers two unique origins to be equal (even though they aren't). A true isSameOrigin method doesn't seem to exist on SecurityOrigin. ========== to ========== Clarify surprising (to me) behavior of isSameSchemeHostPort. The method was commented as "checks for equality", but doesn't mention that it considers two unique origins to be equal (even though they aren't). A true isSameOrigin method doesn't seem to exist on SecurityOrigin. ==========
tsepez@chromium.org changed reviewers: + jww@chromium.org, mkwst@chromium.org
That's surprising to me, too. Mike, Joel?
On 2016/05/16 19:52:36, Tom Sepez wrote: > That's surprising to me, too. Mike, Joel? This is very surprising to me. Doing a quick audit of all callers of isSameSchemeHostPort(), I'm pretty convinced that this is a bug. My quick look makes me think that there are no callers that rely on this, and for at least a few of the callers, I think they *believe* it is a "same origin" check. The one maybe exception to this is in History::canChangeToUrl (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...) which does an explicit isUnique check first, so that author may have known about this quirk. So, personally, I think that we should add a check to see if the origin is unique first, and return false if that's not the case. But no matter what we choose, we should add a quick unit test to SecurityOriginTest.cpp to verify the behavior. And now is when Mike explains to me why I'm totally wrong ;-)
On 2016/05/17 at 04:20:40, jww wrote: > On 2016/05/16 19:52:36, Tom Sepez wrote: > > That's surprising to me, too. Mike, Joel? > > This is very surprising to me. Doing a quick audit of all callers of isSameSchemeHostPort(), I'm pretty convinced that this is a bug. My quick look makes me think that there are no callers that rely on this, and for at least a few of the callers, I think they *believe* it is a "same origin" check. The one maybe exception to this is in History::canChangeToUrl (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...) which does an explicit isUnique check first, so that author may have known about this quirk. > > So, personally, I think that we should add a check to see if the origin is unique first, and return false if that's not the case. But no matter what we choose, we should add a quick unit test to SecurityOriginTest.cpp to verify the behavior. > > And now is when Mike explains to me why I'm totally wrong ;-) Naah. This seems like a reasonable approach. Unique origins don't have schemes, hosts, or ports, so they certainly can't have the _same_ scheme, host, and port. Making that explicit in the code seems like the right way to go.
Description was changed from ========== Clarify surprising (to me) behavior of isSameSchemeHostPort. The method was commented as "checks for equality", but doesn't mention that it considers two unique origins to be equal (even though they aren't). A true isSameOrigin method doesn't seem to exist on SecurityOrigin. ========== to ========== Add isUnique check to isSameSchemeHostPort. The method is documented as "checks for equality", but before considered any pair of unique origins to be equal. With this change only unique origins that are the same instance are considered equal. ==========
I changed the CL to add a check for isUnique in isSameSchemeHostPort. Does that seem reasonable/correct?
mek, would you mind adding some unit tests to SecurityOriginTest.cpp? Thanks! https://codereview.chromium.org/1985703003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp (right): https://codereview.chromium.org/1985703003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp:534: if (this != other && (isUnique() || other->isUnique())) IMO, if would simpler to first have a check: if (this == other) return true; followed by: if (isUnique() || other->isUnique()) return false; In fact, I think without that, there may be another bug below in the isLocal() check.
On 2016/06/02 at 20:49:57, jww wrote: > mek, would you mind adding some unit tests to SecurityOriginTest.cpp? Thanks! I added some tests for the behavior with regard to unique origins. It probably wouldn't hurt to have tests covering other cases too though... https://codereview.chromium.org/1985703003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp (right): https://codereview.chromium.org/1985703003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp:534: if (this != other && (isUnique() || other->isUnique())) On 2016/06/02 at 20:49:56, jww wrote: > IMO, if would simpler to first have a check: > if (this == other) > return true; > followed by: > if (isUnique() || other->isUnique()) > return false; > > In fact, I think without that, there may be another bug below in the isLocal() check. Done. Assuming that that was indeed a bug in the isLocal check, since there was in fact a test explicitly testing that case.
lgtm. Thanks!
https://codereview.chromium.org/1985703003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/weborigin/SecurityOriginTest.cpp (right): https://codereview.chromium.org/1985703003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/weborigin/SecurityOriginTest.cpp:443: EXPECT_TRUE(uniqueOrigin->isSameSchemeHostPort(uniqueOrigin.get())); Why is this true? I'd expect it to be false (because unique origins aren't same-origin with any other origin, including themselves).
https://codereview.chromium.org/1985703003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/weborigin/SecurityOriginTest.cpp (right): https://codereview.chromium.org/1985703003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/weborigin/SecurityOriginTest.cpp:443: EXPECT_TRUE(uniqueOrigin->isSameSchemeHostPort(uniqueOrigin.get())); On 2016/06/03 at 05:22:04, Mike West (OOO until 30th) wrote: > Why is this true? I'd expect it to be false (because unique origins aren't same-origin with any other origin, including themselves). According to the HTML specification of origins an unique origin (there called an opaque origin) is in fact same-origin with itself (https://html.spec.whatwg.org/multipage/browsers.html#same-origin). And we certainly treat them that way various places in the code-base (a sandboxed iframe with an opaque origin for example can create a blob URL with the same origin as itself, which it can then read/use to create a same-origin worker, etc).
On 2016/06/03 at 05:25:12, mek wrote: > https://codereview.chromium.org/1985703003/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/weborigin/SecurityOriginTest.cpp (right): > > https://codereview.chromium.org/1985703003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/weborigin/SecurityOriginTest.cpp:443: EXPECT_TRUE(uniqueOrigin->isSameSchemeHostPort(uniqueOrigin.get())); > On 2016/06/03 at 05:22:04, Mike West (OOO until 30th) wrote: > > Why is this true? I'd expect it to be false (because unique origins aren't same-origin with any other origin, including themselves). > > According to the HTML specification of origins an unique origin (there called an opaque origin) is in fact same-origin with itself (https://html.spec.whatwg.org/multipage/browsers.html#same-origin). And we certainly treat them that way various places in the code-base (a sandboxed iframe with an opaque origin for example can create a blob URL with the same origin as itself, which it can then read/use to create a same-origin worker, etc). Hrm. That's not the way I've thought about unique origins. It also doesn't match `url::Origin::IsSameOriginWith`, `SecurityOrigin::canRequest`, or the text at https://url.spec.whatwg.org/#origin.
On 2016/06/03 at 12:29:52, Mike West (OOO until 30th) wrote: > On 2016/06/03 at 05:25:12, mek wrote: > > https://codereview.chromium.org/1985703003/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/platform/weborigin/SecurityOriginTest.cpp (right): > > > > https://codereview.chromium.org/1985703003/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/platform/weborigin/SecurityOriginTest.cpp:443: EXPECT_TRUE(uniqueOrigin->isSameSchemeHostPort(uniqueOrigin.get())); > > On 2016/06/03 at 05:22:04, Mike West (OOO until 30th) wrote: > > > Why is this true? I'd expect it to be false (because unique origins aren't same-origin with any other origin, including themselves). > > > > According to the HTML specification of origins an unique origin (there called an opaque origin) is in fact same-origin with itself (https://html.spec.whatwg.org/multipage/browsers.html#same-origin). And we certainly treat them that way various places in the code-base (a sandboxed iframe with an opaque origin for example can create a blob URL with the same origin as itself, which it can then read/use to create a same-origin worker, etc). > > Hrm. That's not the way I've thought about unique origins. It also doesn't match `url::Origin::IsSameOriginWith`, `SecurityOrigin::canRequest`, or the text at https://url.spec.whatwg.org/#origin. In other words, I agree with you that that apparently _does_ work, but I'm not at all convinced that it _should_. :)
On 2016/06/03 at 12:35:11, mkwst wrote: > On 2016/06/03 at 12:29:52, Mike West (OOO until 30th) wrote: > > On 2016/06/03 at 05:25:12, mek wrote: > > > https://codereview.chromium.org/1985703003/diff/40001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/platform/weborigin/SecurityOriginTest.cpp (right): > > > > > > https://codereview.chromium.org/1985703003/diff/40001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/platform/weborigin/SecurityOriginTest.cpp:443: EXPECT_TRUE(uniqueOrigin->isSameSchemeHostPort(uniqueOrigin.get())); > > > On 2016/06/03 at 05:22:04, Mike West (OOO until 30th) wrote: > > > > Why is this true? I'd expect it to be false (because unique origins aren't same-origin with any other origin, including themselves). > > > > > > According to the HTML specification of origins an unique origin (there called an opaque origin) is in fact same-origin with itself (https://html.spec.whatwg.org/multipage/browsers.html#same-origin). And we certainly treat them that way various places in the code-base (a sandboxed iframe with an opaque origin for example can create a blob URL with the same origin as itself, which it can then read/use to create a same-origin worker, etc). > > > > Hrm. That's not the way I've thought about unique origins. It also doesn't match `url::Origin::IsSameOriginWith`, `SecurityOrigin::canRequest`, or the text at https://url.spec.whatwg.org/#origin. > > In other words, I agree with you that that apparently _does_ work, but I'm not at all convinced that it _should_. :) I agree that it is a bit of a mess (and my example might have been a bad one since as you point out this is in fact not the way the origin of a URL is specified. See also https://github.com/whatwg/html/issues/1322 for some discussion about opaque origins in blob URLs). You're right that according to the spec the origin that is parsed from a URL should always be a new unique opaque origin, and thus not be equal to any other existing origin. However in our (and apparently firefox's) implementation we separately keep a map of blob-url => securityorigin to be able to get back the same origin instance for a blob url as what was used to create it. The "cachedOrigin(url) == this" check in canRequest is what covers that case. Or in other words, both canAccess and canRequest already have the == this check to check for same originness, so it seems sense that a more general "is origin the same" check that "isSameSchemeHostPort" is would do the same. Maybe isSameSchemeHostPort is not the right name for this method, but it seems like there should at least be some method that does a same origin check in blink.
mkwst: ping?
Description was changed from ========== Add isUnique check to isSameSchemeHostPort. The method is documented as "checks for equality", but before considered any pair of unique origins to be equal. With this change only unique origins that are the same instance are considered equal. ========== to ========== Add isUnique check to isSameSchemeHostPort. The method is documented as "checks for equality", but before considered any pair of unique origins to be equal. With this change only unique origins that are the same instance are considered equal. ==========
tsepez@chromium.org changed reviewers: - tsepez@chromium.org
tsepez@chromium.org changed reviewers: + tsepez@chromium.org
Removing myself as reviewer in deference to Mike and JWW.
On 2016/06/08 at 16:07:15, mek wrote: > mkwst: ping? I guess this is an improvement over the status quo, so LGTM to land it (though I still don't think that the behavior makes sense or matches the spec). Can you please follow up on the HTML/Fetch/URL issue to ensure that either Chrome/Firefox or the relevant specs are updated?
On 2016/06/10 at 09:46:16, mkwst wrote: > On 2016/06/08 at 16:07:15, mek wrote: > > mkwst: ping? > > I guess this is an improvement over the status quo, so LGTM to land it (though I still don't think that the behavior makes sense or matches the spec). (The definition of opaque origin in https://html.spec.whatwg.org/multipage/browsers.html#origin also mentions at least that testing for equality is a meaningful operation. Although I guess meaningful doesn't necessarily mean that there are any values for which it would be equal, I'm pretty sure that's intended). > Can you please follow up on the HTML/Fetch/URL issue to ensure that either Chrome/Firefox or the relevant specs are updated? I filed https://github.com/whatwg/url/issues/127 for the blob url origin thing.
The CQ bit was checked by mek@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1985703003/40001
Message was sent while issue was closed.
Description was changed from ========== Add isUnique check to isSameSchemeHostPort. The method is documented as "checks for equality", but before considered any pair of unique origins to be equal. With this change only unique origins that are the same instance are considered equal. ========== to ========== Add isUnique check to isSameSchemeHostPort. The method is documented as "checks for equality", but before considered any pair of unique origins to be equal. With this change only unique origins that are the same instance are considered equal. ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add isUnique check to isSameSchemeHostPort. The method is documented as "checks for equality", but before considered any pair of unique origins to be equal. With this change only unique origins that are the same instance are considered equal. ========== to ========== Add isUnique check to isSameSchemeHostPort. The method is documented as "checks for equality", but before considered any pair of unique origins to be equal. With this change only unique origins that are the same instance are considered equal. Committed: https://crrev.com/0fb92722bc63b2cf98bf6502494a6f77e430fc1a Cr-Commit-Position: refs/heads/master@{#399890} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/0fb92722bc63b2cf98bf6502494a6f77e430fc1a Cr-Commit-Position: refs/heads/master@{#399890} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
