Silent turn servers filtering without "transport=udp" causing harm on Edge 42.17134.1.0

[ceztko]

• I have provided steps to reproduce
• I have provided browser name, version and adapter.js version
• This issue only happens when adapter.js is used

Versions affected

Browser name including version
Microsoft Edge 42.17134.1.0 (menu -> Settings -> About this app)

adapter.js
git 55eb749

Description

The filterIceServers function is currently filtering all turn server without the ?transport=udp suffix. On the stated Edge version it seems the filter does no longer apply. Even if the filter still applies, filtering without any provided feedback (in the form of console logging) is harmful and should be changed to be more informative.

Steps to reproduce

• Configure any example using adapter.js needing turn servers;
• Configure a RTCPeerConnection with a RTCConfiguration with a turn server without ?transport=udp;
• The turn servers are filtered without any feedback and the connection doesn't establish.

In my code, I then apply the following patch to adapter.js:

diff --git a/release/adapter.js b/release/adapter.js
index be8efeaa..a1c66e7e 100644
--- a/release/adapter.js
+++ b/release/adapter.js
@@ -1617,7 +1617,7 @@ function filterIceServers(iceServers, edgeVersion) {
         urls = [urls];
       }
       urls = urls.filter(function (url) {
-        var validTurn = url.startsWith('turn') && !url.startsWith('turn:[') && url.includes('transport=udp') && !hasTurn;
+        var validTurn = url.startsWith('turn') && !url.startsWith('turn:[') && !hasTurn;
 
         if (validTurn) {
           hasTurn = true;
@@ -2634,7 +2634,6 @@ function filterIceServers(iceServers, edgeVersion) {
       }
       urls = urls.filter(function(url) {
         var validTurn = url.indexOf('turn:') === 0 &&
-            url.indexOf('transport=udp') !== -1 &&
             url.indexOf('turn:[') === -1 &&
             !hasTurn;
 

And the connection, at least in my case, establish correctly. By the way: why the function filterIceServers is duplicated?

Expected results

The filterIceServers function should:

• (if the filter doesn't apply anymore on the stated Edge version) selectively allow turn servers without ?transport=udp suffix to work;
• warn when any ice server (turn or stun, doesn't matter) is removed, to not cause harm and unexpected behavior.

Actual results

The filter works silently without any user provided feedback, letting correctly defined connections to fail silently with Edge browser.

[fippo]

This also happens in version 6.4.8 right?

It seems Edge improved its turn url parsing (while breaking #903) so this is no longer required. Tagging @aboba who might know a better fix version but 17134 tentatively sounds good.

By the way: why the function filterIceServers is duplicated?

Yes. Removing it from https://github.com/otalk/rtcpeerconnection-shim is going to require a major version bump of that package. See here.
I'll have to do the fix twice in the meantime.

warn when any ice server (turn or stun, doesn't matter) is removed, to not cause harm and unexpected behavior.

Which sadly will make people complain about using console.warn in a library :-(

[ceztko]

This also happens in version 6.4.8 right?

Yes, it definitely happens on 6.4.8 also.

warn when any ice server (turn or stun, doesn't matter) is removed, to not cause harm and unexpected behavior.

Which sadly will make people complain about using console.warn in a library :-(

But console.warn is already being used in the adapter. Consider this, which actually helped me to harden my code, so I'm assuming there's already a consensus in issuing warnings. If the adapter is performing aggressive filtering to protect from unwanted exceptions, at the cost of potentially breaking correctly configured connections, I prefer it warns me instead of just silently breaking.

[ceztko]

Unfortunately the problem is still present in 7.0.1 and the very latest 7.1.1. I see one problem for sure, possibly two:

1. That duplicated filterIceServers hit you :D . In my code that still unfixed[1] version is called first. I have no idea where it gets included from, since I can't find it in the source and I'm pointing it in the generated release. Is there some issue in the generation of the release, like local machine spurious data? Please fix this first as with duplicated code in code generation things gets REALLY messy;
2. Because of 1) I can't properly test, but nothing seems really fixed with regard to TURN: we said that with edge version >= 17134 it should accept urls like turn:hostname, without ?transport=udp, then the condition in [2] should be something like:

        let validTurn;
        if (edgeVersion >= 17134) {
            validTurn = url.startsWith('turn') &&
                !url.startsWith('turn:[');
        } else {
            validTurn = url.startsWith('turn') &&
                !url.startsWith('turn:[') &&
                url.includes('transport=udp');
        }
        if (validTurn && !hasTurn) {
            hasTurn = true;
            return true;
        }

Tell me if you want me to open a new issue or we can re-use this.

[1]

adapter/release/adapter.js

Line 1669 in a5e8cea

function filterIceServers(iceServers, edgeVersion) {

[2]

adapter/src/js/edge/filtericeservers.js

Line 36 in a5e8cea

const validTurn = url.startsWith('turn') &&

[ceztko]

@fippo after previous comment, I ask if you can re-open this issue or I should create a new one.

[fippo]

the other version comes from https://github.com/otalk/rtcpeerconnection-shim (and is removed in master)