In case a page and sw with different origins can start communicating.
Created attachment 334426 [details] Patch
For fetch, we need to take into account blob and data URLs frames making loads and being intercepted by the parent frame service worker. We also need to take into account non HTTP registered service workers. For postMessage, I used the same approach but I wonder if there are cases where we would release assert and should not.
Comment on attachment 334426 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=334426&action=review > Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.cpp:188 > + RELEASE_ASSERT(url.isEmpty() || !url.protocolIsInHTTPFamily() || !serviceWorkerURL.protocolIsInHTTPFamily() || protocolHostAndPortAreEqual(url, serviceWorkerURL)); I can probably remove url.isEmpty()
Comment on attachment 334426 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=334426&action=review Same origin checks should be written in terms of SecurityOrigin. It knows how to apply them correctly. > Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:124 > + RELEASE_ASSERT(!sourceClient->url().protocolIsInHTTPFamily() || !serviceWorkerGlobalScope.url().protocolIsInHTTPFamily() || protocolHostAndPortAreEqual(serviceWorkerGlobalScope.url(), sourceClient->url())); > + This does not seem correct. Can we write this in terms of SecurityOrigin? >> Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.cpp:188 >> + String origin = request.httpOrigin(); >> + URL url { URL(), origin.isEmpty() ? referrer : origin }; >> + URL serviceWorkerURL = serviceWorkerThreadProxy->scriptURL(); >> + RELEASE_ASSERT(url.isEmpty() || !url.protocolIsInHTTPFamily() || !serviceWorkerURL.protocolIsInHTTPFamily() || protocolHostAndPortAreEqual(url, serviceWorkerURL)); > > I can probably remove url.isEmpty() How did you come to the decision to make use of the HTTP Origin header or fall back to referrer? Can we write this in terms of SecurityOrigin.
> Same origin checks should be written in terms of SecurityOrigin. It knows > how to apply them correctly. We do not try to enforce same origin checks. We try enforcing service worker partitioning and controlling checks. For instance, a data URL iframe will be controlled by the service worker controlling the parent of the iframe. We could add a SecurityOrigin method for that purpose but this seems unnecessary to me. Note that these checks are in case something really bad happens. > > Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:124 > > + RELEASE_ASSERT(!sourceClient->url().protocolIsInHTTPFamily() || !serviceWorkerGlobalScope.url().protocolIsInHTTPFamily() || protocolHostAndPortAreEqual(serviceWorkerGlobalScope.url(), sourceClient->url())); > > + > > This does not seem correct. Can we write this in terms of SecurityOrigin? I don't see why this is incorrect. It is true that if we had more information, we could make tighter checks. But we try to limit the number of changes here. We could replace protocolHostAndPortAreEqual(...) by something like serviceWorkerGlobalScope.topOrigin().canRequest(...). This does not look like an improvement to me though since what we are really trying to check is protocol/host/port equality. > >> Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.cpp:188 > >> + String origin = request.httpOrigin(); > >> + URL url { URL(), origin.isEmpty() ? referrer : origin }; > >> + URL serviceWorkerURL = serviceWorkerThreadProxy->scriptURL(); > >> + RELEASE_ASSERT(url.isEmpty() || !url.protocolIsInHTTPFamily() || !serviceWorkerURL.protocolIsInHTTPFamily() || protocolHostAndPortAreEqual(url, serviceWorkerURL)); > > > > I can probably remove url.isEmpty() > > How did you come to the decision to make use of the HTTP Origin header or > fall back to referrer? Can we write this in terms of SecurityOrigin. We do not have any notion of the origin of the frame triggering the load at that point. We could pipe it through IPC but this is a best effort patch and we try to limit the number of changes. Hence why using Origin/Referrer.
<rdar://problem/37765052>
(In reply to youenn fablet from comment #5) > > Same origin checks should be written in terms of SecurityOrigin. It knows > > how to apply them correctly. > > We do not try to enforce same origin checks. > We try enforcing service worker partitioning and controlling checks. > For instance, a data URL iframe will be controlled by the service worker > controlling the parent of the iframe. > > We could add a SecurityOrigin method for that purpose but this seems > unnecessary to me. > Note that these checks are in case something really bad happens. > > > > Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:124 > > > + RELEASE_ASSERT(!sourceClient->url().protocolIsInHTTPFamily() || !serviceWorkerGlobalScope.url().protocolIsInHTTPFamily() || protocolHostAndPortAreEqual(serviceWorkerGlobalScope.url(), sourceClient->url())); > > > + > > > > This does not seem correct. Can we write this in terms of SecurityOrigin? > > I don't see why this is incorrect. > It is true that if we had more information, we could make tighter checks. > But we try to limit the number of changes here. OK. > > We could replace protocolHostAndPortAreEqual(...) by something like > serviceWorkerGlobalScope.topOrigin().canRequest(...). > This does not look like an improvement to me though since what we are really > trying to check is protocol/host/port equality. > > > >> Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.cpp:188 > > >> + String origin = request.httpOrigin(); > > >> + URL url { URL(), origin.isEmpty() ? referrer : origin }; > > >> + URL serviceWorkerURL = serviceWorkerThreadProxy->scriptURL(); > > >> + RELEASE_ASSERT(url.isEmpty() || !url.protocolIsInHTTPFamily() || !serviceWorkerURL.protocolIsInHTTPFamily() || protocolHostAndPortAreEqual(url, serviceWorkerURL)); > > > > > > I can probably remove url.isEmpty() > > > > How did you come to the decision to make use of the HTTP Origin header or > > fall back to referrer? Can we write this in terms of SecurityOrigin. > > We do not have any notion of the origin of the frame triggering the load at > that point. > We could pipe it through IPC but this is a best effort patch and we try to > limit the number of changes. > Hence why using Origin/Referrer. OK.
Comment on attachment 334426 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=334426&action=review > Source/WebCore/ChangeLog:5 > + Please add radar before landing. > Source/WebCore/ChangeLog:8 > + No change of behavior. This is OK as-is. Typically we put such a remark after the description. > Source/WebKit/ChangeLog:5 > + Please add radar before landing.
Created attachment 334450 [details] Patch for landing
Comment on attachment 334450 [details] Patch for landing Clearing flags on attachment: 334450 Committed r228919: <https://trac.webkit.org/changeset/228919>
All reviewed patches have been landed. Closing bug.
We need to distinguish navigation from subresource loads as well.
Reopening to attach new patch.
Created attachment 334483 [details] early exit alternative
Resolved as fixed. If we san crashes in trunk Safari, we can always reopen.