-
Notifications
You must be signed in to change notification settings - Fork 322
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WebRTC circumvention #588 #646
Conversation
// WebSockets are broken in old versions of chrome | ||
// and we don't need this hack in new version cause then websocket traffic is intercepted | ||
// https://github.com/AdguardTeam/AdguardBrowserExtension/issues/273 | ||
// https://github.com/AdguardTeam/AdguardBrowserExtension/issues/572 | ||
var userAgent = navigator.userAgent.toLowerCase(); | ||
var cIndex = userAgent.indexOf('chrome/'); | ||
if (cIndex > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it was except Chrome in specified version ranges but now it is only for Chrome? Do you miss other browsers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chrome, Edge, YaBrowser, Opera contain Chrome/<version>
in the user agent string.
return; | ||
if (versionNumber >= 47 && versionNumber <= 57 && | ||
// This is chrome-specific feature for blocking WebSocket connections | ||
// overrideWebSocket function is not defined in case of other browsers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not defined isn't it?
|
||
// Only for dynamically created frames and http/https documents. | ||
if (!isHtml() && | ||
window.location.href !== "about:blank") { | ||
return; | ||
} | ||
|
||
var userAgent = navigator.userAgent.toLowerCase(); | ||
if (userAgent.indexOf('firefox') >= 0) { | ||
// Explicit check, we must not go further in case of Firefox |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we now need it to override WebRTC in Firefox?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was AMO reviewer's requirement.
See task and commit: #379
@@ -32,7 +32,7 @@ | |||
|
|||
regexAnySymbol: ".*", | |||
regexSeparator: "([^ a-zA-Z0-9.%]|$)", | |||
regexStartUrl: "^(http|https|ws|wss)://([a-z0-9-_.]+\\.)?", | |||
regexStartUrl: "^(http://|https://|ws://|wss://|stun:|stuns:|turn:|turns:)([a-z0-9-_.]+\\.)?", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, that is a rather complicated regex.
First of all, we can change stun:
to stun://
, etc.
Then we'll be able to change this expression into something more readable:
^.{2,5}://([a-z0-9-_.]+\\.)?
Extension/lib/pages/log.js
Outdated
return null; | ||
} | ||
if (url.indexOf('//') < 0) { | ||
var index = url.indexOf(':'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We won't need it if we use stun://
prefix
* https://tools.ietf.org/html/rfc4395#section-2.2 | ||
* https://tools.ietf.org/html/draft-nandakumar-rtcweb-stun-uri-08#appendix-B | ||
*/ | ||
firstIdx = url.indexOf(":"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, we won't need it if we use stun://
.
I realize it is against RFC, but anyway
// https://github.com/AdguardTeam/AdguardBrowserExtension/issues/273 | ||
// https://github.com/AdguardTeam/AdguardBrowserExtension/issues/572 | ||
/** | ||
* This is browsers, that support WebExt-API, specific feature for blocking WebSocket connections |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code below is supposed to be used in WebExt extensions. This code overrides WebSocket constructor (except for newer Chrome and FF) and RTCPeerConnection constructor, so that we could inspect & block them.
So, what with my comments? |
We've decided not to replace the original |
Ok then, let's merge it. |
@atropnikov please take a look at the latest changes in ABP, they've improved WebRTC wrapper a bit. Should we also? |
…953_01 to master * commit '00e95f44ebc376124790da8b2785c3aca69dcf1b': update locales delete dots on popup button for issue reporting just make button renaming done
#588