Give URLKeepingBlobAlive a top-origin SecurityOriginData #9184

sysrqb · 2023-01-26T19:31:20Z

`6f0e501`

Give URLKeepingBlobAlive a top-origin SecurityOriginData
https://bugs.webkit.org/show_bug.cgi?id=251218
rdar://104704879

Reviewed by Chris Dumez.

In this patch we construct the URLKeepingBlobAlive with its associated
top-level SecurityOrigin(Data). This will be helpful in a future patch where we
partition the Blob registry using that top-level origin. URLKeepingBlobAlive
needs this information, in particular, because it is responsible for
automatically registering and unregistering its Blob URL handle - thus keeping
the blob alive as long as it exists, as the class name implies.

* Source/WebCore/Modules/fetch/FetchRequest.cpp:
(WebCore::FetchRequest::FetchRequest):
(WebCore::m_signal):
(WebCore::FetchRequest::initializeWith):
(WebCore::FetchRequest::create):
(WebCore::FetchRequest::clone):
(WebCore::FetchRequest::stop):
* Source/WebCore/Modules/fetch/FetchRequest.h:
(WebCore::FetchRequest::FetchRequest): Deleted.
* Source/WebCore/dom/Document.cpp:
(WebCore::Document::setURL):
* Source/WebCore/fileapi/Blob.cpp:
(WebCore::Blob::Blob):
(WebCore::Blob::handle const):
* Source/WebCore/fileapi/Blob.h:
* Source/WebCore/fileapi/URLKeepingBlobAlive.cpp:
(WebCore::URLKeepingBlobAlive::URLKeepingBlobAlive):
(WebCore::URLKeepingBlobAlive::clear):
(WebCore::URLKeepingBlobAlive::operator=):
(WebCore::URLKeepingBlobAlive::isolatedCopy const):
(WebCore::URLKeepingBlobAlive::isolatedCopy):
* Source/WebCore/fileapi/URLKeepingBlobAlive.h:
(WebCore::URLKeepingBlobAlive::URLKeepingBlobAlive): Deleted.
(WebCore::URLKeepingBlobAlive::operator=): Deleted.
* Source/WebCore/loader/NavigationScheduler.cpp:
* Source/WebCore/loader/PolicyChecker.cpp:
(WebCore::FrameLoader::PolicyChecker::extendBlobURLLifetimeIfNecessary const):
(WebCore::FrameLoader::PolicyChecker::checkNavigationPolicy):
(WebCore::FrameLoader::PolicyChecker::checkNewWindowPolicy):
* Source/WebCore/loader/PolicyChecker.h:
* Source/WebCore/workers/WorkerGlobalScope.cpp:
(WebCore::WorkerGlobalScope::importScripts):
* Source/WebCore/workers/shared/SharedWorker.cpp:
(WebCore::SharedWorker::SharedWorker):
(WebCore::SharedWorker::didFinishLoading):
* Source/WebCore/xml/XMLHttpRequest.cpp:
(WebCore::XMLHttpRequest::open):
(WebCore::XMLHttpRequest::createRequest):
(WebCore::XMLHttpRequest::clearRequest):
(WebCore::XMLHttpRequest::didFinishLoading):

Canonical link: https://commits.webkit.org/260266@main

54ff216

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk1	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch-sim
✅ 🛠 🧪 unsafe-merge

webkit-early-warning-system · 2023-01-26T19:41:04Z

EWS run on previous version of this PR (hash b51bd49)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk1	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch-sim

cdumez

I am not opposed to adding a topOrigin to this class (since this class has Blob in its name). However, I have some other comments.

Source/WebCore/fileapi/Blob.cpp

cdumez · 2023-01-26T19:58:40Z

Source/WebCore/fileapi/Blob.cpp

 {
+    ASSERT(context);


I don't think we should have the assertion, not sure what makes this a guarantee

I didn't find any call sites where the ScriptExecutionContext should be null, so this was only a sanity check.

If call sites never passed null, we would take in a C++ reference, not a pointer. If something is a pointer in WebKit, always assume it can be null. If it really cannot, then update the parameter to be a c++ reference.

cdumez · 2023-01-26T19:58:43Z

Source/WebCore/fileapi/Blob.cpp

 {
+    ASSERT(context);


I don't think we should have the assertion, not sure what makes this a guarantee

cdumez · 2023-01-26T20:17:43Z

Source/WebCore/dom/Document.cpp

@@ -3542,7 +3542,9 @@ void Document::setURL(const URL& url)

    if (SecurityOrigin::shouldIgnoreHost(newURL))
        newURL.setHostAndPort({ });
-    m_url = WTFMove(newURL);
+    // SecurityContext::securityOrigin may not be initialized at this time if setURL() is called in the constructor, therefore calling topOrigin() is not always safe.
+    auto topOrigin = isTopDocument() && !SecurityContext::securityOrigin() ? SecurityOriginData::fromURL(url) : this->topOrigin().data();


What if the URL is a blob URL? (which is the base we care about I believe).
I see 2 likely issues:

SecurityOriginData::fromURL(url) likely won't be able to extra the origin from a blob URL

Even if we addressed the first issue, how would it enforce partitioning?

Imagine this case:

Frame from origin B (under top level origin A) creates a Blob and gets a URL for it. Then it opens a new window and uses this blob URL as URL for the popup. What's supposed to happen? I imagine the blob should have the "A / B" partitioned origin. Seems like we shouldn't be able to load this Blob in a top level popup? If so, I worry that your code here is not going to achieve this (even if we fix issue 1). Am I missing something?

Right, I originally used SecurityOrigin::create(url) for this but I change it in this patch, and that is wrong - we do need SecurityOrigin's logic for handling blob URLs. But, as for (2), there are two options:

the popup has "A / B" blob partitioning (therefore has access to the blob data)

the popup has "B" blob partitioning (therefore doesn't have access to the blob data)

I was following (2) because creating a top-level navigation with subframe partitioning was conceptually confusing and required more invasive code changes. Following (1) will provide better web compat, but I don't know how common is that use case. If you think we should use (1), then this patch will need changes.

I think 2 makes sense. The popup would have "B" origin and thus shouldn't be able to load the blob.

Okay, thanks. I also just remembered and realized you are missing some other context. In w3c/FileAPI#153 there is discussion about how to partition blobs, and the general agreement is that cross-origin top-level navigations to a Blob URL should enforce no-opener.

webkit-early-warning-system · 2023-01-27T08:11:38Z

EWS run on previous version of this PR (hash 6d7ad48)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	~~🧪 api-mac~~	✅ 🧪 gtk-wk2
	~~🧪 api-ios~~	✅ 🧪 mac-wk1	~~🧪 api-gtk~~
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch-sim

cdumez

LGTM

cdumez · 2023-01-27T15:48:06Z

Source/WebCore/Modules/fetch/FetchRequest.cpp

+inline FetchRequest::FetchRequest(ScriptExecutionContext* context, std::optional<FetchBody>&& body, Ref<FetchHeaders>&& headers, ResourceRequest&& request, FetchOptions&& options, String&& referrer)
+    : FetchBodyOwner(context, WTFMove(body), WTFMove(headers))
+    , m_request(WTFMove(request))
+    , m_requestURL({ m_request.url(), context->topOrigin().data() })


We shouldn't dereference context without a null check. If context really truly can't be null, then we should change the parameter type to be a ScriptExecutionContext&.

Changing the parameter type seemed safe here, so I went ahead and did that.

webkit-early-warning-system · 2023-01-27T16:45:53Z

EWS run on previous version of this PR (hash 79f9ad0)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	❌ 🛠 gtk
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	❌ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk1	❌ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch-sim

sysrqb · 2023-01-27T23:50:10Z

This needs to wait until #9253, and I don't understand why this failed to compile on Linux because that happened before #9000 was reverted. I'll rebase this after #9253 is merged and let EWS run again.

webkit-early-warning-system · 2023-02-09T00:25:34Z

EWS run on previous version of this PR (hash c6db0fa)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk1	✅ 🧪 api-gtk
✅ 🛠 🧪 jsc	✅ 🛠 tv	✅ 🧪 mac-wk2	✅ 🛠 jsc-armv7
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 jsc-armv7-tests
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress	✅ 🛠 jsc-mips
	✅ 🛠 watch-sim		✅ 🧪 jsc-mips-tests

webkit-early-warning-system · 2023-02-13T20:31:51Z

EWS run on current version of this PR (hash 54ff216)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk1	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch-sim
✅ 🛠 🧪 unsafe-merge

sysrqb · 2023-02-13T20:44:41Z

EWS was all green on the last run, this run is just rebasing onto main now that #9253 is merged.

https://bugs.webkit.org/show_bug.cgi?id=251218 rdar://104704879 Reviewed by Chris Dumez. In this patch we construct the URLKeepingBlobAlive with its associated top-level SecurityOrigin(Data). This will be helpful in a future patch where we partition the Blob registry using that top-level origin. URLKeepingBlobAlive needs this information, in particular, because it is responsible for automatically registering and unregistering its Blob URL handle - thus keeping the blob alive as long as it exists, as the class name implies. * Source/WebCore/Modules/fetch/FetchRequest.cpp: (WebCore::FetchRequest::FetchRequest): (WebCore::m_signal): (WebCore::FetchRequest::initializeWith): (WebCore::FetchRequest::create): (WebCore::FetchRequest::clone): (WebCore::FetchRequest::stop): * Source/WebCore/Modules/fetch/FetchRequest.h: (WebCore::FetchRequest::FetchRequest): Deleted. * Source/WebCore/dom/Document.cpp: (WebCore::Document::setURL): * Source/WebCore/fileapi/Blob.cpp: (WebCore::Blob::Blob): (WebCore::Blob::handle const): * Source/WebCore/fileapi/Blob.h: * Source/WebCore/fileapi/URLKeepingBlobAlive.cpp: (WebCore::URLKeepingBlobAlive::URLKeepingBlobAlive): (WebCore::URLKeepingBlobAlive::clear): (WebCore::URLKeepingBlobAlive::operator=): (WebCore::URLKeepingBlobAlive::isolatedCopy const): (WebCore::URLKeepingBlobAlive::isolatedCopy): * Source/WebCore/fileapi/URLKeepingBlobAlive.h: (WebCore::URLKeepingBlobAlive::URLKeepingBlobAlive): Deleted. (WebCore::URLKeepingBlobAlive::operator=): Deleted. * Source/WebCore/loader/NavigationScheduler.cpp: * Source/WebCore/loader/PolicyChecker.cpp: (WebCore::FrameLoader::PolicyChecker::extendBlobURLLifetimeIfNecessary const): (WebCore::FrameLoader::PolicyChecker::checkNavigationPolicy): (WebCore::FrameLoader::PolicyChecker::checkNewWindowPolicy): * Source/WebCore/loader/PolicyChecker.h: * Source/WebCore/workers/WorkerGlobalScope.cpp: (WebCore::WorkerGlobalScope::importScripts): * Source/WebCore/workers/shared/SharedWorker.cpp: (WebCore::SharedWorker::SharedWorker): (WebCore::SharedWorker::didFinishLoading): * Source/WebCore/xml/XMLHttpRequest.cpp: (WebCore::XMLHttpRequest::open): (WebCore::XMLHttpRequest::createRequest): (WebCore::XMLHttpRequest::clearRequest): (WebCore::XMLHttpRequest::didFinishLoading): Canonical link: https://commits.webkit.org/260266@main

webkit-commit-queue · 2023-02-14T19:17:51Z

Committed 260266@main (6f0e501): https://commits.webkit.org/260266@main

Reviewed commits have been landed. Closing PR #9184 and removing active labels.

sysrqb requested review from cdumez and rniwa as code owners January 26, 2023 19:31

cdumez reviewed Jan 26, 2023

View reviewed changes

sysrqb force-pushed the eng/Give-URLKeepingBlobAlive-a-top-origin-SecurityOriginData branch from b51bd49 to 6d7ad48 Compare January 27, 2023 08:11

cdumez approved these changes Jan 27, 2023

View reviewed changes

sysrqb force-pushed the eng/Give-URLKeepingBlobAlive-a-top-origin-SecurityOriginData branch from 6d7ad48 to 79f9ad0 Compare January 27, 2023 16:45

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jan 27, 2023

sysrqb force-pushed the eng/Give-URLKeepingBlobAlive-a-top-origin-SecurityOriginData branch from 79f9ad0 to c6db0fa Compare February 9, 2023 00:23

sysrqb requested a review from a team as a code owner February 9, 2023 00:23

sysrqb force-pushed the eng/Give-URLKeepingBlobAlive-a-top-origin-SecurityOriginData branch from c6db0fa to 54ff216 Compare February 13, 2023 20:29

achristensen07 added unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing and removed merging-blocked Applied to prevent a change from being merged labels Feb 14, 2023

webkit-early-warning-system force-pushed the eng/Give-URLKeepingBlobAlive-a-top-origin-SecurityOriginData branch from 54ff216 to 6f0e501 Compare February 14, 2023 19:17

webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Feb 14, 2023

webkit-early-warning-system merged commit 6f0e501 into WebKit:main Feb 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Give URLKeepingBlobAlive a top-origin SecurityOriginData #9184

Give URLKeepingBlobAlive a top-origin SecurityOriginData #9184

sysrqb commented Jan 26, 2023 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Jan 26, 2023 •

edited

cdumez left a comment

cdumez Jan 26, 2023

sysrqb Jan 26, 2023

cdumez Jan 26, 2023

cdumez Jan 26, 2023

cdumez Jan 26, 2023

sysrqb Jan 26, 2023

cdumez Jan 26, 2023

sysrqb Jan 26, 2023

webkit-early-warning-system commented Jan 27, 2023 •

edited

cdumez left a comment

cdumez Jan 27, 2023

sysrqb Jan 27, 2023

webkit-early-warning-system commented Jan 27, 2023 •

edited

sysrqb commented Jan 27, 2023

webkit-early-warning-system commented Feb 9, 2023 •

edited

webkit-early-warning-system commented Feb 13, 2023 •

edited

sysrqb commented Feb 13, 2023

webkit-commit-queue commented Feb 14, 2023

Give URLKeepingBlobAlive a top-origin SecurityOriginData #9184

Give URLKeepingBlobAlive a top-origin SecurityOriginData #9184

Conversation

sysrqb commented Jan 26, 2023 • edited by webkit-early-warning-system

webkit-early-warning-system commented Jan 26, 2023 • edited

cdumez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webkit-early-warning-system commented Jan 27, 2023 • edited

cdumez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webkit-early-warning-system commented Jan 27, 2023 • edited

sysrqb commented Jan 27, 2023

webkit-early-warning-system commented Feb 9, 2023 • edited

webkit-early-warning-system commented Feb 13, 2023 • edited

sysrqb commented Feb 13, 2023

webkit-commit-queue commented Feb 14, 2023

sysrqb commented Jan 26, 2023 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Jan 26, 2023 •

edited

webkit-early-warning-system commented Jan 27, 2023 •

edited

webkit-early-warning-system commented Jan 27, 2023 •

edited

webkit-early-warning-system commented Feb 9, 2023 •

edited

webkit-early-warning-system commented Feb 13, 2023 •

edited