Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2196983002:
Allow doc.written scripts with a matching domain and registry to execute. (Closed)
|
Created:
4 years, 4 months ago by Bryan McQuade Modified:
4 years, 4 months ago CC:
blink-reviews, chromium-reviews, gavinp+loader_chromium.org, Nate Chapin, loading-reviews_chromium.org, tyoshino+watch_chromium.org, Yoav Weiss Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow doc.written scripts with a matching domain and registry to execute.
We currently allow scripts to execute as long as they are on the same
hostname as the document.
This change also allows scripts to execute if they have the same
domain+registry as the document. For example, if a script is on
static.example.com, and the main document is on www.example.com,
the script will be allowed to execute, since the domain+registry
for both script and document is example.com.
I didn't see a way to write a layout test for this, as layout tests
don't allow serving resources from actual domains (only 127.0.0.1
and localhost).
BUG=632986
Committed: https://crrev.com/c3892ef2fb30045b8f9173839818914c1162fcc3
Cr-Commit-Position: refs/heads/master@{#409875}
Patch Set 1 #Patch Set 2 : revert unintended change #Patch Set 3 : fix comment #
Total comments: 2
Patch Set 4 : add test #
Total comments: 4
Patch Set 5 : address comments #Patch Set 6 : fix compiler error #
Total comments: 1
Messages
Total messages: 43 (30 generated)
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Allow doc.written scripts with a matching domain and registry to execute. BUG= ========== to ========== Allow doc.written scripts with a matching domain and registry to execute. BUG=632986 ==========
Description was changed from ========== Allow doc.written scripts with a matching domain and registry to execute. BUG=632986 ========== to ========== Allow doc.written scripts with a matching domain and registry to execute. We currently allow scripts to execute as long as they are on the same hostname as the document. This change also allows scripts to execute if they have the same domain+registry as the document. For example, if a script is on static.example.com, and the main document is on www.example.com, the script will be allowed to execute, since the domain+registry for both script and document is example.com. I didn't see a way to write a layout test for this, as layout tests don't allow serving resources from actual domains (only 127.0.0.1 and localhost). BUG=632986 ==========
bmcquade@chromium.org changed reviewers: + shivanisha@chromium.org
PTAL
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2196983002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/network/NetworkUtilsTest.cpp (right): https://codereview.chromium.org/2196983002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/NetworkUtilsTest.cpp:83: EXPECT_EQ("appspot.com", NetworkUtils::getDomainAndRegistry("www.foo.appspot.com", false)); Can we also include the example mentioned in the comment in ShouldDisallowFetchForMainFrameScript: static.example.com and www.example.com to test both of them return example.com. As per the test on www.foo.appspot.com returning foo.appspot.com, won't static.example.com return static.example.com?
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/01 at 16:35:55, shivanisha wrote: > https://codereview.chromium.org/2196983002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/network/NetworkUtilsTest.cpp (right): > > https://codereview.chromium.org/2196983002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/network/NetworkUtilsTest.cpp:83: EXPECT_EQ("appspot.com", NetworkUtils::getDomainAndRegistry("www.foo.appspot.com", false)); > Can we also include the example mentioned in the comment in ShouldDisallowFetchForMainFrameScript: > static.example.com and www.example.com to test both of them return example.com. As per the test on www.foo.appspot.com returning foo.appspot.com, won't static.example.com return static.example.com? Yes, good idea. I just added static.example.com, and www.example.com was already there. Both return example.com. The getDomainAndRegistry function is backed by a lookup trie that has a bunch of hostnames encoded in it, so it knows that appspot.com is a dedicated private registry which it will handle differently from example.com which is not a registry. Thanks!
PTAL, thanks! https://codereview.chromium.org/2196983002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/network/NetworkUtilsTest.cpp (right): https://codereview.chromium.org/2196983002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/NetworkUtilsTest.cpp:83: EXPECT_EQ("appspot.com", NetworkUtils::getDomainAndRegistry("www.foo.appspot.com", false)); On 2016/08/01 at 16:35:55, shivanisha wrote: > Can we also include the example mentioned in the comment in ShouldDisallowFetchForMainFrameScript: > static.example.com and www.example.com to test both of them return example.com. As per the test on www.foo.appspot.com returning foo.appspot.com, won't static.example.com return static.example.com? Ah, replied to the overall change rather than the comment. Repeating here: Yes, good idea. I just added static.example.com, and www.example.com was already there, above. Both return example.com. The getDomainAndRegistry function is backed by a lookup trie that has a bunch of hostnames encoded in it, so it knows that appspot.com is a dedicated private registry which it will handle differently from example.com which is not a registry. Thanks!
On 2016/08/01 at 18:43:51, bmcquade wrote: > PTAL, thanks! > > https://codereview.chromium.org/2196983002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/network/NetworkUtilsTest.cpp (right): > > https://codereview.chromium.org/2196983002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/network/NetworkUtilsTest.cpp:83: EXPECT_EQ("appspot.com", NetworkUtils::getDomainAndRegistry("www.foo.appspot.com", false)); > On 2016/08/01 at 16:35:55, shivanisha wrote: > > Can we also include the example mentioned in the comment in ShouldDisallowFetchForMainFrameScript: > > static.example.com and www.example.com to test both of them return example.com. As per the test on www.foo.appspot.com returning foo.appspot.com, won't static.example.com return static.example.com? > > Ah, replied to the overall change rather than the comment. Repeating here: > > Yes, good idea. I just added static.example.com, and www.example.com was already there, above. Both return example.com. The getDomainAndRegistry function is backed by a lookup trie that has a bunch of hostnames encoded in it, so it knows that appspot.com is a dedicated private registry which it will handle differently from example.com which is not a registry. Thanks! LGTM, Thanks!
bmcquade@chromium.org changed reviewers: + japhet@chromium.org
japhet, PTAL, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2196983002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/2196983002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:124: if (!requestDomain.isEmpty() && !documentDomain.isEmpty() && requestDomain == documentDomain) Nit: we don't need to call isEmpty() for both strings. https://codereview.chromium.org/2196983002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/network/NetworkUtils.h (right): https://codereview.chromium.org/2196983002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/NetworkUtils.h:19: PLATFORM_EXPORT String getDomainAndRegistry(const String& host, bool includePrivateRegistries); Enum instead of bool for includePrivateRegistries?
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
bmcquade@chromium.org changed reviewers: + kinuko@chromium.org
Thanks! Addressed japhet's comments. kinuko, PTAL for blink platform, thanks! https://codereview.chromium.org/2196983002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/2196983002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:124: if (!requestDomain.isEmpty() && !documentDomain.isEmpty() && requestDomain == documentDomain) On 2016/08/03 at 21:44:09, Nate Chapin wrote: > Nit: we don't need to call isEmpty() for both strings. Ah, getDomainAndRegistry can return empty string in some cases. If it returns empty for both, we don't want to consider that a match, so these are needed. I added a comment to make that clearer. https://codereview.chromium.org/2196983002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/network/NetworkUtils.h (right): https://codereview.chromium.org/2196983002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/NetworkUtils.h:19: PLATFORM_EXPORT String getDomainAndRegistry(const String& host, bool includePrivateRegistries); On 2016/08/03 at 21:44:09, Nate Chapin wrote: > Enum instead of bool for includePrivateRegistries? Good idea, done, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm for platform/ I guess you could possibly layout-test this, we have some cross-origin tests. You can take a look at http/tests/resources/get-host-info.js and how some host info, e.g. HTTP_REMOTE_ORIGIN, is used in existing tests. https://codereview.chromium.org/2196983002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/network/NetworkUtils.cpp (right): https://codereview.chromium.org/2196983002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/network/NetworkUtils.cpp:28: } nit: Probably we can just have static_assert's (like we do in several places with STATIC_ASSERT_ENUM macro) and use static_cast to get the net:: enum value
On 2016/08/04 at 18:08:04, kinuko wrote: > lgtm for platform/ > > I guess you could possibly layout-test this, we have some cross-origin tests. You can take a look at http/tests/resources/get-host-info.js and how some host info, e.g. HTTP_REMOTE_ORIGIN, is used in existing tests. > > https://codereview.chromium.org/2196983002/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/network/NetworkUtils.cpp (right): > > https://codereview.chromium.org/2196983002/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/network/NetworkUtils.cpp:28: } > nit: Probably we can just have static_assert's (like we do in several places with STATIC_ASSERT_ENUM macro) and use static_cast to get the net:: enum value Thanks! I agree with this feedback & will address it, but I'd like to land this in the meantime. I opened https://bugs.chromium.org/p/chromium/issues/detail?id=634440 to remind myself to address these.
The CQ bit was checked by bmcquade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from shivanisha@chromium.org Link to the patchset: https://codereview.chromium.org/2196983002/#ps100001 (title: "fix compiler error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Allow doc.written scripts with a matching domain and registry to execute. We currently allow scripts to execute as long as they are on the same hostname as the document. This change also allows scripts to execute if they have the same domain+registry as the document. For example, if a script is on static.example.com, and the main document is on www.example.com, the script will be allowed to execute, since the domain+registry for both script and document is example.com. I didn't see a way to write a layout test for this, as layout tests don't allow serving resources from actual domains (only 127.0.0.1 and localhost). BUG=632986 ========== to ========== Allow doc.written scripts with a matching domain and registry to execute. We currently allow scripts to execute as long as they are on the same hostname as the document. This change also allows scripts to execute if they have the same domain+registry as the document. For example, if a script is on static.example.com, and the main document is on www.example.com, the script will be allowed to execute, since the domain+registry for both script and document is example.com. I didn't see a way to write a layout test for this, as layout tests don't allow serving resources from actual domains (only 127.0.0.1 and localhost). BUG=632986 Committed: https://crrev.com/c3892ef2fb30045b8f9173839818914c1162fcc3 Cr-Commit-Position: refs/heads/master@{#409875} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/c3892ef2fb30045b8f9173839818914c1162fcc3 Cr-Commit-Position: refs/heads/master@{#409875} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
