Noissue - Use lowercase for WebRTC and WebSocket resource types

Noissue - Use lowercase for WebRTC and WebSocket resource types

[Manish Jethani, 2017-05-22 14:41:55]

Patch Set 1

Chrome is reporting WebRTC requests as "WEBRTC" (uppercase). We might as well
always use uppercase.

[Manish Jethani, 2017-05-22 14:43:31]

On 2017/05/22 14:41:55, Manish Jethani wrote:
> Patch Set 1
> 
> Chrome is reporting WebRTC requests as "WEBRTC" (uppercase). We might as well
> always use uppercase.

This broke in 29421712 (#5184).

[Sebastian Noack, 2017-05-22 14:50:45]

This shouldn't be necessary, if you just replace "WEBRTC" and "WEBSOCKET" in
inject.preload.js with lowercase. This is where these strings come from. They
don't come from Chrome, but we emulate requests of these types, there.

[Manish Jethani, 2017-05-22 15:00:40]

Patch Set 2: Use lowercase for WebRTC and WebSocket resource types

[Sebastian Noack, 2017-05-22 15:04:51]

LGTM

[kzar, 2017-05-22 15:12:33]

On 2017/05/22 14:50:45, Sebastian Noack wrote:
> This shouldn't be necessary, if you just replace "WEBRTC" and "WEBSOCKET" in
> inject.preload.js with lowercase. This is where these strings come from. They
> don't come from Chrome, but we emulate requests of these types, there.

Are you sure? for example the check in lib/requestBlocker.js that checks if the
request type is inside chrome.webRequest.ResourceType.

[Sebastian Noack, 2017-05-22 15:32:05]

On 2017/05/22 15:12:33, kzar wrote:
> On 2017/05/22 14:50:45, Sebastian Noack wrote:
> > This shouldn't be necessary, if you just replace "WEBRTC" and "WEBSOCKET" in
> > inject.preload.js with lowercase. This is where these strings come from.
They
> > don't come from Chrome, but we emulate requests of these types, there.
> 
> Are you sure? for example the check in lib/requestBlocker.js that checks if
the
> request type is inside chrome.webRequest.ResourceType.

Well spotted. This check would have to be updated as well.

[Manish Jethani, 2017-05-22 15:59:43]

On 2017/05/22 15:32:05, Sebastian Noack wrote:
> On 2017/05/22 15:12:33, kzar wrote:
> > On 2017/05/22 14:50:45, Sebastian Noack wrote:
> > > This shouldn't be necessary, if you just replace "WEBRTC" and "WEBSOCKET"
in
> > > inject.preload.js with lowercase. This is where these strings come from.
> They
> > > don't come from Chrome, but we emulate requests of these types, there.
> > 
> > Are you sure? for example the check in lib/requestBlocker.js that checks if
> the
> > request type is inside chrome.webRequest.ResourceType.
> 
> Well spotted. This check would have to be updated as well.

You're referring this this:

  // Chrome 58 onwards directly supports WebSocket blocking, so we can ignore
  // messages from the wrapper here (see https://crbug.com/129353). Hopefully
  // WebRTC will be supported soon too (see https://crbug.com/707683).
  if (msg.requestType in chrome.webRequest.ResourceType)
    return false;

Here we'll just have to uppercase msg.requestType, right?

[Manish Jethani, 2017-05-22 17:21:59]

Patch Set 3

On 2017/05/22 15:32:05, Sebastian Noack wrote:
> On 2017/05/22 15:12:33, kzar wrote:
> > On 2017/05/22 14:50:45, Sebastian Noack wrote:
> > > This shouldn't be necessary, if you just replace "WEBRTC" and "WEBSOCKET"
in
> > > inject.preload.js with lowercase. This is where these strings come from.
> They
> > > don't come from Chrome, but we emulate requests of these types, there.
> > 
> > Are you sure? for example the check in lib/requestBlocker.js that checks if
> the
> > request type is inside chrome.webRequest.ResourceType.
> 
> Well spotted. This check would have to be updated as well.

Done.

[kzar, 2017-05-23 11:42:59]

LGTM assuming you've tested that request blocking, including WebRTC, still
works.

(If it helps I add the filter $webrtc,domain=news.ycombinator.com , then browse
to https://news.ycombinator.com , open a console and run this `new
RTCPeerConnection({iceServers: [{urls: ["stun:stun.l.google.com:19302"]}]});`
and check it was blocked.)

[Sebastian Noack, 2017-05-23 12:19:13]

LGTM

[Manish Jethani, 2017-05-23 18:58:44]

On 2017/05/23 11:42:59, kzar wrote:
> LGTM assuming you've tested that request blocking, including WebRTC, still
> works.
> 
> (If it helps I add the filter $webrtc,domain=news.ycombinator.com , then
browse
> to https://news.ycombinator.com , open a console and run this `new
> RTCPeerConnection({iceServers: [{urls: ["stun:stun.l.google.com:19302"]}]});`
> and check it was blocked.)

Tested and works.