WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
183025
Add release asserts for service worker fetch and postMessage events
https://bugs.webkit.org/show_bug.cgi?id=183025
Summary
Add release asserts for service worker fetch and postMessage events
youenn fablet
Reported
2018-02-21 16:55:20 PST
In case a page and sw with different origins can start communicating.
Attachments
Patch
(5.92 KB, patch)
2018-02-21 17:02 PST
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Patch for landing
(5.97 KB, patch)
2018-02-22 07:18 PST
,
youenn fablet
no flags
Details
Formatted Diff
Diff
early exit alternative
(6.41 KB, patch)
2018-02-22 16:34 PST
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
youenn fablet
Comment 1
2018-02-21 17:02:58 PST
Created
attachment 334426
[details]
Patch
youenn fablet
Comment 2
2018-02-21 17:05:49 PST
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.
youenn fablet
Comment 3
2018-02-21 17:50:47 PST
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()
Daniel Bates
Comment 4
2018-02-21 20:04:44 PST
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.
youenn fablet
Comment 5
2018-02-21 20:42:01 PST
> 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.
youenn fablet
Comment 6
2018-02-21 21:01:59 PST
<
rdar://problem/37765052
>
Daniel Bates
Comment 7
2018-02-22 00:42:29 PST
(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.
Daniel Bates
Comment 8
2018-02-22 00:44:53 PST
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.
youenn fablet
Comment 9
2018-02-22 07:18:14 PST
Created
attachment 334450
[details]
Patch for landing
Chris Dumez
Comment 10
2018-02-22 08:56:00 PST
Comment on
attachment 334450
[details]
Patch for landing Clearing flags on attachment: 334450 Committed
r228919
: <
https://trac.webkit.org/changeset/228919
>
Chris Dumez
Comment 11
2018-02-22 08:56:01 PST
All reviewed patches have been landed. Closing bug.
youenn fablet
Comment 12
2018-02-22 11:08:18 PST
We need to distinguish navigation from subresource loads as well.
youenn fablet
Comment 13
2018-02-22 16:34:58 PST
Reopening to attach new patch.
youenn fablet
Comment 14
2018-02-22 16:34:59 PST
Created
attachment 334483
[details]
early exit alternative
youenn fablet
Comment 15
2018-02-28 17:53:58 PST
Resolved as fixed. If we san crashes in trunk Safari, we can always reopen.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug