Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2714573002:
Enable websocket filtering via SubresourceFilter (Closed)
|
Created:
3 years, 10 months ago by Charlie Harrison Modified:
3 years, 9 months ago CC:
chromium-reviews, tyoshino+watch_chromium.org, subresource-filter-reviews_chromium.org, yhirano+watch_chromium.org, blink-reviews, dglazkov+blink, haraken, blink-reviews-api_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnable websocket filtering via SubresourceFilter
This CL adds a hook into the DocumentWebSocketChannel to filter
WebSocket connections via the DocumentLoader's SubresourceFilter.
BUG=609747
Review-Url: https://codereview.chromium.org/2714573002
Cr-Commit-Position: refs/heads/master@{#455254}
Committed: https://chromium.googlesource.com/chromium/src/+/e86d3f063e9d258dfc9d1a11a9c6a124fb9a6d9f
Patch Set 1 #Patch Set 2 : actually add html/js files :P #
Total comments: 4
Patch Set 3 : yhirano review #Patch Set 4 : rebase #Patch Set 5 : notifyLoad #Patch Set 6 : ugh #Patch Set 7 : return true #
Total comments: 3
Patch Set 8 : implement failWithClosureCode #
Total comments: 4
Patch Set 9 : Enable websocket filtering via WebDocumentSubresourceFilter #
Total comments: 17
Patch Set 10 : respond to comments #Patch Set 11 : rebase #
Total comments: 4
Patch Set 12 : OWNERS #
Total comments: 21
Patch Set 13 : git cl try #
Total comments: 9
Patch Set 14 : engedy review #
Total comments: 2
Patch Set 15 : git cl try #Patch Set 16 : sync to #455176 #Messages
Total messages: 103 (63 generated)
The CQ bit was checked by csharrison@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...
csharrison@chromium.org changed reviewers: + yhirano@chromium.org
yhirano: Here is a draft of the websocket filtering CL. LMK what you think about the async-close style that you discussed in email. Also, I wonder what timing guarantees we can make about a postTask on connect() like this. On local testing, I noticed we never actually get the "open" message, but is that guaranteed? My tests just see if we get any data back from the echo server, which seems very unlikely to be racy with this code. Another thing I was unsure about was where to put the filtering policy, FrameFetchContext right now has some filtering in canRequest(), so I was also considering adding a canRequestWebSocket method there.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by csharrison@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.
I imagined something like this:
// in open:
...
if (blocked by CSP) {
throw;
}
if (blocked by subresource filter) {
post a task which closes this connection;
return;
}
connect;
In short, the check is done synchronously but the notification will arrive
asynchronously. What do you think about it?
https://codereview.chromium.org/2714573002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp
(right):
https://codereview.chromium.org/2714573002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp:689:
close(CloseEventCodeNotSpecified, "");
Is 1008 good for this case?
https://codereview.chromium.org/2714573002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp:689:
close(CloseEventCodeNotSpecified, "");
if (loadPolicy == Allow)
return;
?
The CQ bit was checked by csharrison@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 csharrison@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...
Hey yhirano: couple notes: 1. We can't do this check during CSP checks because it must be on the main thread, that's why it's in the document channel. 2. "Notification" is a little overloaded (can mean a devtools message or a close() message), so I'm not sure what you mean exactly. The current state of the subresource filter doesn't include devtools messaging, but what I've done in this patch is add support for asynchronously messaging the failure to consumers, as well as asynchronously posting a task to close(). We could also synchronously fail before calling connect(). I am not sure which is preferred. https://codereview.chromium.org/2714573002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp (right): https://codereview.chromium.org/2714573002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp:689: close(CloseEventCodeNotSpecified, ""); On 2017/02/24 03:55:29, yhirano wrote: > if (loadPolicy == Allow) > return; > > ? Done. Oops :P https://codereview.chromium.org/2714573002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp:689: close(CloseEventCodeNotSpecified, ""); On 2017/02/24 03:55:30, yhirano wrote: > Is 1008 good for this case? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by csharrison@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: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by csharrison@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.
On 2017/02/24 21:13:02, Charlie Harrison wrote:
> Hey yhirano:
> couple notes:
> 1. We can't do this check during CSP checks because it must be on the main
> thread, that's why it's in the document channel.
>
Oops, sorry, I was wrong about CSP.
// in DocumentWebSocketChannel::connect:
...
if (blocked by mixed content) {
throw;
}
if (blocked by subresource filter) {
post a task which closes this connection;
return;
}
connect;
Does this make sense?
> 2. "Notification" is a little overloaded (can mean a devtools message or a
> close() message), so I'm not sure what you mean exactly. The current state of
> the subresource filter doesn't include devtools messaging, but what I've done
in
> this patch is add support for asynchronously messaging the failure to
consumers,
> as well as asynchronously posting a task to close().
>
> We could also synchronously fail before calling connect().
> I am not sure which is preferred.
>
What I'm suggesting is to check it synchronously, and notify the failure
asynchronously.
On 2017/02/28 10:13:24, yhirano (slow) wrote:
> On 2017/02/24 21:13:02, Charlie Harrison wrote:
> > Hey yhirano:
> > couple notes:
> > 1. We can't do this check during CSP checks because it must be on the main
> > thread, that's why it's in the document channel.
> >
>
> Oops, sorry, I was wrong about CSP.
>
> // in DocumentWebSocketChannel::connect:
> ...
>
> if (blocked by mixed content) {
> throw;
> }
>
> if (blocked by subresource filter) {
> post a task which closes this connection;
> return;
> }
>
> connect;
>
>
> Does this make sense?
>
> > 2. "Notification" is a little overloaded (can mean a devtools message or a
> > close() message), so I'm not sure what you mean exactly. The current state
of
> > the subresource filter doesn't include devtools messaging, but what I've
done
> in
> > this patch is add support for asynchronously messaging the failure to
> consumers,
> > as well as asynchronously posting a task to close().
> >
>
> > We could also synchronously fail before calling connect().
> > I am not sure which is preferred.
> >
>
> What I'm suggesting is to check it synchronously, and notify the failure
> asynchronously.
But "return"ing before we actually connect the handle is not allowed, because
we've never opened the connection. Returning is a synchronous failure.
The code I have now is as you wrote, except for the "return", to allow for
asynchronously closing.
On 2017/02/28 13:42:42, Charlie Harrison wrote:
> On 2017/02/28 10:13:24, yhirano (slow) wrote:
> > On 2017/02/24 21:13:02, Charlie Harrison wrote:
> > > Hey yhirano:
> > > couple notes:
> > > 1. We can't do this check during CSP checks because it must be on the main
> > > thread, that's why it's in the document channel.
> > >
> >
> > Oops, sorry, I was wrong about CSP.
> >
> > // in DocumentWebSocketChannel::connect:
> > ...
> >
> > if (blocked by mixed content) {
> > throw;
> > }
> >
> > if (blocked by subresource filter) {
> > post a task which closes this connection;
> > return;
> > }
> >
> > connect;
> >
> >
> > Does this make sense?
> >
> > > 2. "Notification" is a little overloaded (can mean a devtools message or a
> > > close() message), so I'm not sure what you mean exactly. The current state
> of
> > > the subresource filter doesn't include devtools messaging, but what I've
> done
> > in
> > > this patch is add support for asynchronously messaging the failure to
> > consumers,
> > > as well as asynchronously posting a task to close().
> > >
> >
> > > We could also synchronously fail before calling connect().
> > > I am not sure which is preferred.
> > >
> >
> > What I'm suggesting is to check it synchronously, and notify the failure
> > asynchronously.
>
> But "return"ing before we actually connect the handle is not allowed, because
> we've never opened the connection. Returning is a synchronous failure.
>
> The code I have now is as you wrote, except for the "return", to allow for
> asynchronously closing.
Ah, yes, we can't send a close frame to such a handle. How about calling
handleDidClose(false, 1008, "") asynchronously and returning true?
On 2017/02/28 13:57:20, yhirano (slow) wrote:
> On 2017/02/28 13:42:42, Charlie Harrison wrote:
> > On 2017/02/28 10:13:24, yhirano (slow) wrote:
> > > On 2017/02/24 21:13:02, Charlie Harrison wrote:
> > > > Hey yhirano:
> > > > couple notes:
> > > > 1. We can't do this check during CSP checks because it must be on the
main
> > > > thread, that's why it's in the document channel.
> > > >
> > >
> > > Oops, sorry, I was wrong about CSP.
> > >
> > > // in DocumentWebSocketChannel::connect:
> > > ...
> > >
> > > if (blocked by mixed content) {
> > > throw;
> > > }
> > >
> > > if (blocked by subresource filter) {
> > > post a task which closes this connection;
> > > return;
> > > }
> > >
> > > connect;
> > >
> > >
> > > Does this make sense?
> > >
> > > > 2. "Notification" is a little overloaded (can mean a devtools message or
a
> > > > close() message), so I'm not sure what you mean exactly. The current
state
> > of
> > > > the subresource filter doesn't include devtools messaging, but what I've
> > done
> > > in
> > > > this patch is add support for asynchronously messaging the failure to
> > > consumers,
> > > > as well as asynchronously posting a task to close().
> > > >
> > >
> > > > We could also synchronously fail before calling connect().
> > > > I am not sure which is preferred.
> > > >
> > >
> > > What I'm suggesting is to check it synchronously, and notify the failure
> > > asynchronously.
> >
> > But "return"ing before we actually connect the handle is not allowed,
because
> > we've never opened the connection. Returning is a synchronous failure.
> >
> > The code I have now is as you wrote, except for the "return", to allow for
> > asynchronously closing.
>
> Ah, yes, we can't send a close frame to such a handle. How about calling
> handleDidClose(false, 1008, "") asynchronously and returning true?
Let me try it :) BTW thanks for the fast reply.
The CQ bit was checked by csharrison@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...
csharrison@chromium.org changed reviewers: + engedy@chromium.org
Thanks yhirano, this was a good solution. +engedy for subresource filter bits, but feel free to wait until pkalinnikov gets back. https://codereview.chromium.org/2714573002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/DocumentSubresourceFilterClient.cpp (right): https://codereview.chromium.org/2714573002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/DocumentSubresourceFilterClient.cpp:46: // WebDocumentSubresourceFilter rather than sending the unspecified context. s/unspecified/subresource
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2714573002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp (right): https://codereview.chromium.org/2714573002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp:205: WTF::bind(&DocumentWebSocketChannel::handleDidClose, Thank you! I found we need some more operations such as clearing |connection_handle_for_scheduler_|, calling didError, etc. I think the following would be the easiest. 1. Define DocumentWebSocketChannel::failWithClosureCode which has |code| parameter. The implementation is mostly same as DocumentWebSocketChannel::fail. 2. Make DocumentWebSocketChannel::fail call failWithClosureCode with 1006. 3. Call failWithClosureCode asynchronously here. Sorry for another roundtrip...
Thanks! https://codereview.chromium.org/2714573002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp (right): https://codereview.chromium.org/2714573002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp:205: WTF::bind(&DocumentWebSocketChannel::handleDidClose, On 2017/03/01 03:40:42, yhirano (slow) wrote: > Thank you! > > I found we need some more operations such as clearing > |connection_handle_for_scheduler_|, calling didError, etc. I think the following > would be the easiest. > > 1. Define DocumentWebSocketChannel::failWithClosureCode which has |code| > parameter. The implementation is mostly same as DocumentWebSocketChannel::fail. > 2. Make DocumentWebSocketChannel::fail call failWithClosureCode with 1006. > 3. Call failWithClosureCode asynchronously here. > > Sorry for another roundtrip... Done, how does this look to you?
The CQ bit was checked by csharrison@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...
https://codereview.chromium.org/2714573002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp (right): https://codereview.chromium.org/2714573002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp:199: // If the connection needs to be filtered, asynchronously fail. Note that Please move this section before L187. https://codereview.chromium.org/2714573002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp:317: document()->addConsoleMessage(ConsoleMessage::create( Is showing a console message useful for developers when filtered? If so, can you move this to failWithClosureCode?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2714573002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp (right): https://codereview.chromium.org/2714573002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp:199: // If the connection needs to be filtered, asynchronously fail. Note that On 2017/03/01 06:40:58, yhirano (slow) wrote: > Please move this section before L187. Done. https://codereview.chromium.org/2714573002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp:317: document()->addConsoleMessage(ConsoleMessage::create( On 2017/03/01 06:40:58, yhirano (slow) wrote: > Is showing a console message useful for developers when filtered? If so, can you > move this to failWithClosureCode? It is useful. Eventually we will move the notification logic to the SubresourceFilter code, but for now we can keep the generic error. I've basically replaced fail with failWithClosureCode and sent an empty reason string here.
The CQ bit was checked by csharrison@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.
Thank you! modules/websockets lgtm. Please update the issue title (it's now DocumentSubresourceFilter). Providing a more informative issue description would be good, too.
Description was changed from ========== Enable websocket filtering via WebDocumentSubresourceFilter BUG=TODO ========== to ========== Enable websocket filtering via SubresourceFilter This CL adds a hook into the DocumentWebSocketChannel to filter WebSocket connections via the DocumentLoader's SubresourceFilter. BUG=609747 ==========
Many thanks for the detailed review, yhirano. engedy: PTAL at the SubresourceFilter code. In particular, I am wondering if this is an ok placeholder type for web sockets.
engedy@chromium.org changed reviewers: + pkalinnikov@chromium.org
Thanks a lot and sorry for the slow response. > engedy: PTAL at the SubresourceFilter code. In particular, I am wondering if > this is an ok placeholder type for web sockets. Pavel, you are more familiar with this than I am. WDYT?
Some comments regarding the SubresourceFilter part. https://codereview.chromium.org/2714573002/diff/160001/chrome/test/data/subre... File chrome/test/data/subresource_filter/websocket_connection.js (right): https://codereview.chromium.org/2714573002/diff/160001/chrome/test/data/subre... chrome/test/data/subresource_filter/websocket_connection.js:12: ws.addEventListener('open', function() { Is there a reason why you use such a syntax instead of overriding 'onopen' attribute of the WebSocket object? Example: https://cs.chromium.org/chromium/src/chrome/test/data/extensions/api_test/web... yhirano@: Which one is preferable? https://codereview.chromium.org/2714573002/diff/160001/chrome/test/data/subre... chrome/test/data/subresource_filter/websocket_connection.js:17: messageMainContext("onclose"); nit: Please use single quotes here as well. https://codereview.chromium.org/2714573002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/SubresourceFilter.cpp (right): https://codereview.chromium.org/2714573002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/SubresourceFilter.cpp:45: // WebDocumentSubresourceFilter rather than sending subresource context. Right, there is no RequestContext for WebSocket. We would need to do either of: 1. Add a getLoadPolicyForWebSocket(url) method to WebDocumentSubresourceFilter. 2. Introduce a SubresourceFilter-specific "resource type", similar to the one in the CL https://codereview.chromium.org/2700553002. The type would be a short version of RequestContext, with some types merged together. Plus, it would include the WebSocket type. I would have the same values as s_f::proto::ElementType. 3. Move the WebRequestResourceType (from the CL in option 2) to content/ (probably rename it), and use it just like in option 2. The 2 and 3 seem more canonical, but the 1 is shorter. I don't have preferences for now. Charles, WDYT? https://codereview.chromium.org/2714573002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/SubresourceFilter.cpp:48: resourceUrl, WebURLRequest::RequestContextSubresource); Using this type temporarily looks good. Note that this means that the websocket type becomes equivalent to OTHER, although it's just a subset of those. We need a separate type in a follow-up CL (see options above). We would also need a new proto::ElementType to be able to create rules targeting this specific type. https://codereview.chromium.org/2714573002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/SubresourceFilter.cpp:52: TaskRunnerHelper::get(TaskType::Networking, m_documentLoader->frame()) Is this intended that allowWebSocketConnection reports (via notifyLoad) always, but allowLoad reports only if the |reportingPolicy| permits to do so? Shouldn't this be consistent? https://codereview.chromium.org/2714573002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp (right): https://codereview.chromium.org/2714573002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp:191: // TODO(csharrison): Include a reason string here. Could this be done in this CL? E.g., "Connection blocked|disallowed by subresource_filter.". https://codereview.chromium.org/2714573002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp:693: bool DocumentWebSocketChannel::shouldFilterConnection(const KURL& url) { nit: How about should(Disallow|Block)Connection?
Thanks Pavel! https://codereview.chromium.org/2714573002/diff/160001/chrome/test/data/subre... File chrome/test/data/subresource_filter/websocket_connection.js (right): https://codereview.chromium.org/2714573002/diff/160001/chrome/test/data/subre... chrome/test/data/subresource_filter/websocket_connection.js:12: ws.addEventListener('open', function() { On 2017/03/02 12:33:53, pkalinnikov wrote: > Is there a reason why you use such a syntax instead of overriding 'onopen' > attribute of the WebSocket object? > Example: > https://cs.chromium.org/chromium/src/chrome/test/data/extensions/api_test/web... > > yhirano@: Which one is preferable? My understanding is the addEventListener pattern is slightly more preferred because it allows multiple listeners. I've seen them used interchangeably in JS tests though so I'm not sure there is an enforced policy. https://codereview.chromium.org/2714573002/diff/160001/chrome/test/data/subre... chrome/test/data/subresource_filter/websocket_connection.js:17: messageMainContext("onclose"); On 2017/03/02 12:33:53, pkalinnikov wrote: > nit: Please use single quotes here as well. Done. https://codereview.chromium.org/2714573002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/SubresourceFilter.cpp (right): https://codereview.chromium.org/2714573002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/SubresourceFilter.cpp:45: // WebDocumentSubresourceFilter rather than sending subresource context. On 2017/03/02 12:33:53, pkalinnikov wrote: > Right, there is no RequestContext for WebSocket. We would need to do either of: > > 1. Add a getLoadPolicyForWebSocket(url) method to WebDocumentSubresourceFilter. > 2. Introduce a SubresourceFilter-specific "resource type", similar to the one in > the CL https://codereview.chromium.org/2700553002. The type would be a short > version of RequestContext, with some types merged together. Plus, it would > include the WebSocket type. I would have the same values as > s_f::proto::ElementType. > 3. Move the WebRequestResourceType (from the CL in option 2) to content/ > (probably rename it), and use it just like in option 2. > > The 2 and 3 seem more canonical, but the 1 is shorter. I don't have preferences > for now. Charles, WDYT? I slightly prefer (1), but (2) is also ok to me, though I hate adding an extra layer of abstraction when the alternative is not so bad. https://codereview.chromium.org/2714573002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/SubresourceFilter.cpp:48: resourceUrl, WebURLRequest::RequestContextSubresource); On 2017/03/02 12:33:53, pkalinnikov wrote: > Using this type temporarily looks good. Note that this means that the websocket > type becomes equivalent to OTHER, although it's just a subset of those. We need > a separate type in a follow-up CL (see options above). We would also need a new > proto::ElementType to be able to create rules targeting this specific type. Acknowledged. https://codereview.chromium.org/2714573002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/SubresourceFilter.cpp:52: TaskRunnerHelper::get(TaskType::Networking, m_documentLoader->frame()) On 2017/03/02 12:33:53, pkalinnikov wrote: > Is this intended that allowWebSocketConnection reports (via notifyLoad) always, > but allowLoad reports only if the |reportingPolicy| permits to do so? Shouldn't > this be consistent? This was intentional. The reason is that ReportingPolicy basically stems from whether or not the resource sent to allowLoad is a speculative preload (generated by the preload scanner). We don't really have speculative websocket connections so we should always report. I've added a comment to clarify this. https://codereview.chromium.org/2714573002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp (right): https://codereview.chromium.org/2714573002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp:191: // TODO(csharrison): Include a reason string here. On 2017/03/02 12:33:53, pkalinnikov wrote: > Could this be done in this CL? E.g., "Connection blocked|disallowed by > subresource_filter.". OK. This is a little hard to understand but it is better than nothing. We'll have better devtools strings in notifyLoad later. https://codereview.chromium.org/2714573002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp:693: bool DocumentWebSocketChannel::shouldFilterConnection(const KURL& url) { On 2017/03/02 12:33:53, pkalinnikov wrote: > nit: How about should(Disallow|Block)Connection? Changed to Disallow.
LGTM, thanks! yhirano@: In case you missed, I had a question for you in https://codereview.chromium.org/2714573002/diff/160001/chrome/test/data/subre...
The CQ bit was checked by csharrison@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/2714573002/diff/160001/chrome/test/data/subre... File chrome/test/data/subresource_filter/websocket_connection.js (right): https://codereview.chromium.org/2714573002/diff/160001/chrome/test/data/subre... chrome/test/data/subresource_filter/websocket_connection.js:12: ws.addEventListener('open', function() { On 2017/03/02 14:48:00, Charlie Harrison wrote: > On 2017/03/02 12:33:53, pkalinnikov wrote: > > Is there a reason why you use such a syntax instead of overriding 'onopen' > > attribute of the WebSocket object? > > Example: > > > https://cs.chromium.org/chromium/src/chrome/test/data/extensions/api_test/web... > > > > yhirano@: Which one is preferable? > > My understanding is the addEventListener pattern is slightly more preferred > because it allows multiple listeners. I've seen them used interchangeably in JS > tests though so I'm not sure there is an enforced policy. Either is fine, I think. Personally, I prefer addEventListener for objects we (or libraries) may add event handlers multiple times, such as Window, Document, Element, etc. I don't care about WebSocket because in most cases we set event handlers only once at the creation time.
Still LGTM + last-minute nits. https://codereview.chromium.org/2714573002/diff/160001/chrome/test/data/subre... File chrome/test/data/subresource_filter/websocket_connection.js (right): https://codereview.chromium.org/2714573002/diff/160001/chrome/test/data/subre... chrome/test/data/subresource_filter/websocket_connection.js:12: ws.addEventListener('open', function() { On 2017/03/03 06:15:22, yhirano (slow) wrote: > On 2017/03/02 14:48:00, Charlie Harrison wrote: > > On 2017/03/02 12:33:53, pkalinnikov wrote: > > > Is there a reason why you use such a syntax instead of overriding 'onopen' > > > attribute of the WebSocket object? > > > Example: > > > > > > https://cs.chromium.org/chromium/src/chrome/test/data/extensions/api_test/web... > > > > > > yhirano@: Which one is preferable? > > > > My understanding is the addEventListener pattern is slightly more preferred > > because it allows multiple listeners. I've seen them used interchangeably in > JS > > tests though so I'm not sure there is an enforced policy. > > Either is fine, I think. > > Personally, I prefer addEventListener for objects we (or libraries) may add > event handlers multiple times, such as Window, Document, Element, etc. I don't > care about WebSocket because in most cases we set event handlers only once at > the creation time. > On the second look, I find plenty of "ws.onopen" usage in the codebase: https://cs.chromium.org/search/?q=%22ws.onopen+%3D%22&type=cs But only 2 occurrences of ".addEventListener('open'" here: https://cs.chromium.org/search/?q=%5C.addEventListener%5C(%27open%27&type=cs Could we resort to the more popular one? I guess we don't need multiple handlers in this test. https://codereview.chromium.org/2714573002/diff/200001/chrome/browser/subreso... File chrome/browser/subresource_filter/subresource_filter_browsertest.cc (right): https://codereview.chromium.org/2714573002/diff/200001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:695: bool websocket_received_data = false; nit: Can you call this something more direct, like websocket_connection_succeeded/websocket_connected? https://codereview.chromium.org/2714573002/diff/200001/chrome/test/data/subre... File chrome/test/data/subresource_filter/page_with_websocket.html (right): https://codereview.chromium.org/2714573002/diff/200001/chrome/test/data/subre... chrome/test/data/subresource_filter/page_with_websocket.html:26: addEventListener("message", connectionListener); nit: Please use single quotes consistently in this file.
engedy: WDYT about syncing these OWNERS files? I noticed Pavel is not an owner in //chrome https://codereview.chromium.org/2714573002/diff/160001/chrome/test/data/subre... File chrome/test/data/subresource_filter/websocket_connection.js (right): https://codereview.chromium.org/2714573002/diff/160001/chrome/test/data/subre... chrome/test/data/subresource_filter/websocket_connection.js:12: ws.addEventListener('open', function() { On 2017/03/03 10:19:06, pkalinnikov wrote: > On 2017/03/03 06:15:22, yhirano (slow) wrote: > > On 2017/03/02 14:48:00, Charlie Harrison wrote: > > > On 2017/03/02 12:33:53, pkalinnikov wrote: > > > > Is there a reason why you use such a syntax instead of overriding 'onopen' > > > > attribute of the WebSocket object? > > > > Example: > > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/test/data/extensions/api_test/web... > > > > > > > > yhirano@: Which one is preferable? > > > > > > My understanding is the addEventListener pattern is slightly more preferred > > > because it allows multiple listeners. I've seen them used interchangeably in > > JS > > > tests though so I'm not sure there is an enforced policy. > > > > Either is fine, I think. > > > > Personally, I prefer addEventListener for objects we (or libraries) may add > > event handlers multiple times, such as Window, Document, Element, etc. I don't > > care about WebSocket because in most cases we set event handlers only once at > > the creation time. > > > > On the second look, I find plenty of "ws.onopen" usage in the codebase: > https://cs.chromium.org/search/?q=%22ws.onopen+%3D%22&type=cs > But only 2 occurrences of ".addEventListener('open'" here: > https://cs.chromium.org/search/?q=%5C.addEventListener%5C(%27open%27&type=cs > > Could we resort to the more popular one? I guess we don't need multiple handlers > in this test. Yes. Indeed you are right onopen seems more popular than the event listener, at least for websockets. Changed it. I left the self.addEvenetListener because we do need multiple of these, depending on how this script is executing. https://codereview.chromium.org/2714573002/diff/200001/chrome/browser/subreso... File chrome/browser/subresource_filter/subresource_filter_browsertest.cc (right): https://codereview.chromium.org/2714573002/diff/200001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:695: bool websocket_received_data = false; On 2017/03/03 10:19:06, pkalinnikov wrote: > nit: Can you call this something more direct, like > websocket_connection_succeeded/websocket_connected? Done. https://codereview.chromium.org/2714573002/diff/200001/chrome/test/data/subre... File chrome/test/data/subresource_filter/page_with_websocket.html (right): https://codereview.chromium.org/2714573002/diff/200001/chrome/test/data/subre... chrome/test/data/subresource_filter/page_with_websocket.html:26: addEventListener("message", connectionListener); On 2017/03/03 10:19:06, pkalinnikov wrote: > nit: Please use single quotes consistently in this file. Done.
The CQ bit was checked by csharrison@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/2714573002/diff/220001/chrome/browser/subreso... File chrome/browser/subresource_filter/OWNERS (right): https://codereview.chromium.org/2714573002/diff/220001/chrome/browser/subreso... chrome/browser/subresource_filter/OWNERS:1: file://components/subresource_filter/OWNERS Thanks for fixing this! https://codereview.chromium.org/2714573002/diff/220001/chrome/browser/subreso... File chrome/browser/subresource_filter/subresource_filter_browsertest.cc (right): https://codereview.chromium.org/2714573002/diff/220001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:685: IN_PROC_BROWSER_TEST_P(SubresourceFilterWebSocketBrowserTest, BlockWebSocket) { Have you considered implementing some of these (also) as layout tests? It looks like your implementation would end up invoking TestDocumentSubresourceFilter in layout tests just fine. (In /http/tests/subresource_filter/.) https://codereview.chromium.org/2714573002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/SubresourceFilter.cpp (right): https://codereview.chromium.org/2714573002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/SubresourceFilter.cpp:64: void SubresourceFilter::notifyLoad( nit: WDYT about calling this `reportLoad`? At least to me, notify has very strong connotations of notifying the customer of this class (i.e. whatever called allowLoad) of something completing. https://codereview.chromium.org/2714573002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/SubresourceFilter.cpp:65: const KURL& resourceUrl, nit: |resourceUrl| argument is never used https://codereview.chromium.org/2714573002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp (right): https://codereview.chromium.org/2714573002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp:190: // If the connection needs to be filtered, asynchronously fail. Note that nit: Let's make this comment more specific, as it leaves the reader wondering what a `synchronous security error` is, and why subresource filtering cannot be done synchronously. It looks like we are just using this to avoid triggering throwing a SecurityError in [1], as that code expects the only reason for synchronously failing here would be because of Mixed Content checks? If so, could you please elaborate on this specifically here? 1: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/websoc... https://codereview.chromium.org/2714573002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp:199: wrapPersistent(this), CloseEventCodePolicyViolation, nit: Is this error code really appropriate here? According to [1], code 1008 is sent by an endpoint in a `Close control frame` if it has received a message that violates its policy. Would code 1006 not be more appropriate? It is "designated for use in applications expecting a status code to indicate that the connection was closed abnormally, e.g., without sending or receiving a Close control frame." 1: https://tools.ietf.org/html/rfc6455#section-7.4 https://codereview.chromium.org/2714573002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp:713: if (!m_handle) Can this ever evaluate to false? If not, let's make this a DCHECK.
Thanks! yhirano: FYI there is a question for you about the failure status codes. We've changed back to using 1006. https://codereview.chromium.org/2714573002/diff/220001/chrome/browser/subreso... File chrome/browser/subresource_filter/subresource_filter_browsertest.cc (right): https://codereview.chromium.org/2714573002/diff/220001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:685: IN_PROC_BROWSER_TEST_P(SubresourceFilterWebSocketBrowserTest, BlockWebSocket) { On 2017/03/06 14:12:46, engedy (slow) wrote: > Have you considered implementing some of these (also) as layout tests? It looks > like your implementation would end up invoking TestDocumentSubresourceFilter in > layout tests just fine. (In /http/tests/subresource_filter/.) Yeah I have a layout test in the follow up CL. https://codereview.chromium.org/2714573002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/SubresourceFilter.cpp (right): https://codereview.chromium.org/2714573002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/SubresourceFilter.cpp:64: void SubresourceFilter::notifyLoad( On 2017/03/06 14:12:46, engedy (slow) wrote: > nit: WDYT about calling this `reportLoad`? At least to me, notify has very > strong connotations of notifying the customer of this class (i.e. whatever > called allowLoad) of something completing. Sure, changed to report load. https://codereview.chromium.org/2714573002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/SubresourceFilter.cpp:65: const KURL& resourceUrl, On 2017/03/06 14:12:46, engedy (slow) wrote: > nit: |resourceUrl| argument is never used Removed :) https://codereview.chromium.org/2714573002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp (right): https://codereview.chromium.org/2714573002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp:190: // If the connection needs to be filtered, asynchronously fail. Note that On 2017/03/06 14:12:46, engedy (slow) wrote: > nit: Let's make this comment more specific, as it leaves the reader wondering > what a `synchronous security error` is, and why subresource filtering cannot be > done synchronously. > > It looks like we are just using this to avoid triggering throwing a > SecurityError in [1], as that code expects the only reason for synchronously > failing here would be because of Mixed Content checks? If so, could you please > elaborate on this specifically here? > > 1: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/websoc... It's based on that and based on the fact that we don't want to block the worker thread by posting a message. I'll fix up the comment. https://codereview.chromium.org/2714573002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp:199: wrapPersistent(this), CloseEventCodePolicyViolation, On 2017/03/06 14:12:46, engedy (slow) wrote: > nit: Is this error code really appropriate here? > > According to [1], code 1008 is sent by an endpoint in a `Close control frame` if > it has received a message that violates its policy. > > Would code 1006 not be more appropriate? It is "designated for use in > applications expecting a status code to indicate that the connection was closed > abnormally, e.g., without sending or receiving a Close control frame." > > 1: https://tools.ietf.org/html/rfc6455#section-7.4 Hm yeah I think you're right. Since we never open a connection I think 1006 might be the best option. yhirano: WDYT? https://codereview.chromium.org/2714573002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp:713: if (!m_handle) On 2017/03/06 14:12:46, engedy (slow) wrote: > Can this ever evaluate to false? If not, let's make this a DCHECK. Done.
LGTM % final set of comments. https://codereview.chromium.org/2714573002/diff/220001/chrome/browser/subreso... File chrome/browser/subresource_filter/subresource_filter_browsertest.cc (right): https://codereview.chromium.org/2714573002/diff/220001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:261: GURL GetTestUrl(const std::string& path) { nit: s/path/relative_url/ now that we are passing a query component. https://codereview.chromium.org/2714573002/diff/220001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:696: EXPECT_TRUE(content::ExecuteScriptAndExtractBool( nit: To avoid repeating this, let's make this a helper function in the test fixture. https://codereview.chromium.org/2714573002/diff/220001/chrome/test/data/subre... File chrome/test/data/subresource_filter/page_with_websocket.html (right): https://codereview.chromium.org/2714573002/diff/220001/chrome/test/data/subre... chrome/test/data/subresource_filter/page_with_websocket.html:15: // extra 'inWorker' url param. nit: presence (or absence) of an URL param named `inWorker`. https://codereview.chromium.org/2714573002/diff/220001/chrome/test/data/subre... File chrome/test/data/subresource_filter/websocket_connection.js (right): https://codereview.chromium.org/2714573002/diff/220001/chrome/test/data/subre... chrome/test/data/subresource_filter/websocket_connection.js:26: // Will only receive a message with a URL in it if this JS is executing in a To make this a bit cleaner, what do you think of: -- refactoring connectWebSocket so it takes a url and a callback, -- moving the web worker-related stuff from here into a new JS file that uses self.importScripts to include this one? https://codereview.chromium.org/2714573002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/SubresourceFilter.cpp (right): https://codereview.chromium.org/2714573002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/SubresourceFilter.cpp:57: TaskRunnerHelper::get(TaskType::Networking, m_documentLoader->frame()) nit: Given that the DSF is totally not thread safe, would it be at all customary in Blink to declare that: DCHECK(taskRunner->runsTasksOnCurrentThread()); https://codereview.chromium.org/2714573002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp (right): https://codereview.chromium.org/2714573002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp:194: TaskRunnerHelper::get(TaskType::Networking, document()) nit: For this, do we need include a subset of {WebTaskRunner.h, WebTraceLocation.h, Functional.h}? https://codereview.chromium.org/2714573002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp:199: String("Connection disallowed by the subresource filter"), Phrasing nit: WDYT about "... by subresource filtering rules"? https://codereview.chromium.org/2714573002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp:200: WarningMessageLevel, Do we print anything else to the console other than this? I wonder if this should be a warning or error to be consistent with other failure modes.
https://codereview.chromium.org/2714573002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp (right): https://codereview.chromium.org/2714573002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp:199: String("Connection disallowed by the subresource filter"), On 2017/03/06 16:31:01, engedy (slow) wrote: > Phrasing nit: WDYT about "... by subresource filtering rules"? On second thought, let's hold off on printing the console message until we have the final strings here.
The CQ bit was checked by csharrison@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...
OK I've removed all logging and responded to your concerns. yhirano: Please take another pass at the websocket code now that logging is removed. https://codereview.chromium.org/2714573002/diff/220001/chrome/browser/subreso... File chrome/browser/subresource_filter/subresource_filter_browsertest.cc (right): https://codereview.chromium.org/2714573002/diff/220001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:261: GURL GetTestUrl(const std::string& path) { On 2017/03/06 16:31:01, engedy (slow) wrote: > nit: s/path/relative_url/ now that we are passing a query component. Done. https://codereview.chromium.org/2714573002/diff/220001/chrome/browser/subreso... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:696: EXPECT_TRUE(content::ExecuteScriptAndExtractBool( On 2017/03/06 16:31:01, engedy (slow) wrote: > nit: To avoid repeating this, let's make this a helper function in the test > fixture. Done. https://codereview.chromium.org/2714573002/diff/220001/chrome/test/data/subre... File chrome/test/data/subresource_filter/page_with_websocket.html (right): https://codereview.chromium.org/2714573002/diff/220001/chrome/test/data/subre... chrome/test/data/subresource_filter/page_with_websocket.html:15: // extra 'inWorker' url param. On 2017/03/06 16:31:01, engedy (slow) wrote: > nit: presence (or absence) of an URL param named `inWorker`. Done. https://codereview.chromium.org/2714573002/diff/220001/chrome/test/data/subre... File chrome/test/data/subresource_filter/websocket_connection.js (right): https://codereview.chromium.org/2714573002/diff/220001/chrome/test/data/subre... chrome/test/data/subresource_filter/websocket_connection.js:26: // Will only receive a message with a URL in it if this JS is executing in a On 2017/03/06 16:31:01, engedy (slow) wrote: > To make this a bit cleaner, what do you think of: > -- refactoring connectWebSocket so it takes a url and a callback, > -- moving the web worker-related stuff from here into a new JS file that > uses self.importScripts to include this one? SGTM. Done https://codereview.chromium.org/2714573002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/SubresourceFilter.cpp (right): https://codereview.chromium.org/2714573002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/SubresourceFilter.cpp:57: TaskRunnerHelper::get(TaskType::Networking, m_documentLoader->frame()) On 2017/03/06 16:31:01, engedy (slow) wrote: > nit: Given that the DSF is totally not thread safe, would it be at all customary > in Blink to declare that: > > DCHECK(taskRunner->runsTasksOnCurrentThread()); Yep! Added. https://codereview.chromium.org/2714573002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp (right): https://codereview.chromium.org/2714573002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp:194: TaskRunnerHelper::get(TaskType::Networking, document()) On 2017/03/06 16:31:01, engedy (slow) wrote: > nit: For this, do we need include a subset of {WebTaskRunner.h, > WebTraceLocation.h, Functional.h}? Done. https://codereview.chromium.org/2714573002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp:199: String("Connection disallowed by the subresource filter"), On 2017/03/06 16:31:01, engedy (slow) wrote: > Phrasing nit: WDYT about "... by subresource filtering rules"? Done. https://codereview.chromium.org/2714573002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp:200: WarningMessageLevel, On 2017/03/06 16:31:01, engedy (slow) wrote: > Do we print anything else to the console other than this? I wonder if this > should be a warning or error to be consistent with other failure modes. It does make more sense as an error level since we don't even initiate the connection. I've changed it.
lgtm https://codereview.chromium.org/2714573002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp (right): https://codereview.chromium.org/2714573002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp:699: // |reason| is only for logging and should not be provided for scripts, This comment does not make sense any more. Moving this to fail function would be good.
Thank you! https://codereview.chromium.org/2714573002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp (right): https://codereview.chromium.org/2714573002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp:699: // |reason| is only for logging and should not be provided for scripts, On 2017/03/06 20:39:01, yhirano (slow) wrote: > This comment does not make sense any more. Moving this to fail function would be > good. Done.
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkalinnikov@chromium.org, engedy@chromium.org, yhirano@chromium.org Link to the patchset: https://codereview.chromium.org/2714573002/#ps280001 (title: "git cl try")
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
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by csharrison@chromium.org
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
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by csharrison@chromium.org
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
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by engedy@chromium.org
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
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by csharrison@chromium.org
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
Failed to apply patch for
third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp:
While running git apply --index -p1;
error: patch failed:
third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp:297
error:
third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp: patch
does not apply
Patch:
third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp
Index: third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp
diff --git
a/third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp
b/third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp
index
089e79056e6db6d4035b8c01e96af14484c6d5a6..2ce84da526d2dd572b8a39abb1cca6c848c0d38a
100644
--- a/third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp
+++ b/third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp
@@ -33,26 +33,32 @@
#include <memory>
#include "core/dom/DOMArrayBuffer.h"
#include "core/dom/ExecutionContext.h"
+#include "core/dom/TaskRunnerHelper.h"
#include "core/fileapi/FileReaderLoader.h"
#include "core/fileapi/FileReaderLoaderClient.h"
#include "core/frame/LocalFrame.h"
#include "core/frame/LocalFrameClient.h"
#include "core/inspector/ConsoleMessage.h"
#include "core/inspector/InspectorInstrumentation.h"
+#include "core/loader/DocumentLoader.h"
#include "core/loader/FrameLoader.h"
#include "core/loader/MixedContentChecker.h"
+#include "core/loader/SubresourceFilter.h"
#include "core/loader/ThreadableLoadingContext.h"
#include "modules/websockets/InspectorWebSocketEvents.h"
#include "modules/websockets/WebSocketChannelClient.h"
#include "modules/websockets/WebSocketFrame.h"
#include "modules/websockets/WebSocketHandleImpl.h"
#include "platform/WebFrameScheduler.h"
+#include "platform/WebTaskRunner.h"
#include "platform/loader/fetch/UniqueIdentifier.h"
#include "platform/network/NetworkLog.h"
#include "platform/network/WebSocketHandshakeRequest.h"
#include "platform/weborigin/SecurityOrigin.h"
#include "public/platform/InterfaceProvider.h"
#include "public/platform/Platform.h"
+#include "public/platform/WebTraceLocation.h"
+#include "wtf/Functional.h"
#include "wtf/PtrUtil.h"
namespace blink {
@@ -184,6 +190,18 @@ bool DocumentWebSocketChannel::connect(const KURL& url,
protocol.split(", ", true, protocols);
}
+ // If the connection needs to be filtered, asynchronously fail. Synchronous
+ // failure blocks the worker thread which should be avoided. Note that
+ // returning "true" just indicates that this was not a mixed content error.
+ if (shouldDisallowConnection(url)) {
+ TaskRunnerHelper::get(TaskType::Networking, document())
+ ->postTask(
+ BLINK_FROM_HERE,
+ WTF::bind(&DocumentWebSocketChannel::tearDownFailedConnection,
+ wrapPersistent(this)));
+ return true;
+ }
+
// TODO(kinuko): document() should return nullptr if we don't
// have valid document/frame that returns non-empty interface provider.
if (document() && document()->frame() &&
@@ -297,10 +315,6 @@ void DocumentWebSocketChannel::fail(const String& reason,
MessageLevel level,
std::unique_ptr<SourceLocation> location) {
NETWORK_DVLOG(1) << this << " fail(" << reason << ")";
- // m_handle and m_client can be null here.
-
- connection_handle_for_scheduler_.reset();
-
if (document()) {
InspectorInstrumentation::didReceiveWebSocketFrameError(
document(), m_identifier, reason);
@@ -309,13 +323,9 @@ void DocumentWebSocketChannel::fail(const String& reason,
document()->addConsoleMessage(ConsoleMessage::create(
JSMessageSource, level, message, std::move(location)));
}
-
- if (m_client)
- m_client->didError();
// |reason| is only for logging and should not be provided for scripts,
- // hence close reason must be empty.
- handleDidClose(false, CloseEventCodeAbnormalClosure, String());
- // handleDidClose may delete this object.
+ // hence close reason must be empty in tearDownFailedConnection.
+ tearDownFailedConnection();
}
void DocumentWebSocketChannel::disconnect() {
@@ -681,6 +691,28 @@ void DocumentWebSocketChannel::didFailLoadingBlob(
// |this| can be deleted here.
}
+void DocumentWebSocketChannel::tearDownFailedConnection() {
+ // m_handle and m_client can be null here.
+ connection_handle_for_scheduler_.reset();
+
+ if (m_client)
+ m_client->didError();
+
+ handleDidClose(false, CloseEventCodeAbnormalClosure, String());
+ // handleDidClose may delete this object.
+}
+
+bool DocumentWebSocketChannel::shouldDisallowConnection(const KURL& url) {
+ DCHECK(m_handle);
+ DocumentLoader* loader = document()->loader();
+ if (!loader)
+ return false;
+ SubresourceFilter* subresourceFilter = loader->subresourceFilter();
+ if (!subresourceFilter)
+ return false;
+ return !subresourceFilter->allowWebSocketConnection(url);
+}
+
DEFINE_TRACE(DocumentWebSocketChannel) {
visitor->trace(m_blobLoader);
visitor->trace(m_messages);
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from engedy@chromium.org, yhirano@chromium.org, pkalinnikov@chromium.org Link to the patchset: https://codereview.chromium.org/2714573002/#ps300001 (title: "sync to #455176")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 300001, "attempt_start_ts": 1488916429691510,
"parent_rev": "d9070600ab8f23e9f7d7dc468173157388d56769", "commit_rev":
"e86d3f063e9d258dfc9d1a11a9c6a124fb9a6d9f"}
Message was sent while issue was closed.
Description was changed from ========== Enable websocket filtering via SubresourceFilter This CL adds a hook into the DocumentWebSocketChannel to filter WebSocket connections via the DocumentLoader's SubresourceFilter. BUG=609747 ========== to ========== Enable websocket filtering via SubresourceFilter This CL adds a hook into the DocumentWebSocketChannel to filter WebSocket connections via the DocumentLoader's SubresourceFilter. BUG=609747 Review-Url: https://codereview.chromium.org/2714573002 Cr-Commit-Position: refs/heads/master@{#455254} Committed: https://chromium.googlesource.com/chromium/src/+/e86d3f063e9d258dfc9d1a11a9c6... ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001) as https://chromium.googlesource.com/chromium/src/+/e86d3f063e9d258dfc9d1a11a9c6... |
