fix: throw error in setLocalDescription if media line is missing code…

…cs (#63) * fix: throw error in setLocalDescription if media line is missing codecs * chore: update web-capabilities version --------- Co-authored-by: Bryce Tham <[email protected]>
webex · Oct 12, 2023 · ec5f257 · ec5f257
1 parent 7022ce9
commit ec5f257
Show file tree

Hide file tree

Showing 5 changed files with 66 additions and 0 deletions.
diff --git a/package.json b/package.json
@@ -107,6 +107,7 @@
   },
   "dependencies": {
     "@webex/ts-events": "^1.1.0",
+    "@webex/web-capabilities": "^1.1.0",
     "@webex/web-media-effects": "^2.7.0",
     "events": "^3.3.0",
     "js-logger": "^1.6.1",

diff --git a/src/mocks/rtc-peer-connection-stub.ts b/src/mocks/rtc-peer-connection-stub.ts
@@ -13,6 +13,9 @@ class RTCPeerConnectionStub {
   getStats(): Promise<any> {
     return new Promise(() => {});
   }
+  setLocalDescription(): Promise<any> {
+    return new Promise(() => {});
+  }
   onconnectionstatechange: () => void = () => {};
   oniceconnectionstatechange: () => void = () => {};
 }

diff --git a/src/peer-connection.spec.ts b/src/peer-connection.spec.ts
@@ -1,3 +1,4 @@
+import { BrowserInfo } from '@webex/web-capabilities';
 import { ConnectionState, ConnectionStateHandler } from './connection-state-handler';
 import { mocked } from './mocks/mock';
 import { RTCPeerConnectionStub } from './mocks/rtc-peer-connection-stub';
@@ -245,4 +246,37 @@ describe('PeerConnection', () => {
       connectionStateHandlerListener(ConnectionState.Connecting);
     });
   });
+
+  describe('setLocalDescription', () => {
+    let mockPc: RTCPeerConnectionStub;
+    let setLocalDescriptionSpy: jest.SpyInstance;
+    let pc: PeerConnection;
+
+    beforeEach(() => {
+      jest.clearAllMocks();
+      mockPc = mocked(new RTCPeerConnectionStub(), true);
+      mockCreateRTCPeerConnection.mockReturnValueOnce(mockPc as unknown as RTCPeerConnection);
+      setLocalDescriptionSpy = jest.spyOn(mockPc, 'setLocalDescription');
+      pc = new PeerConnection();
+    });
+
+    it('sets the local description with an SDP offer', async () => {
+      expect.hasAssertions();
+      const description = { type: 'offer', sdp: 'fake sdp' } as RTCSessionDescriptionInit;
+      pc.setLocalDescription(description);
+      expect(setLocalDescriptionSpy).toHaveBeenCalledWith(description);
+    });
+    it('sets the local description with no SDP offer', async () => {
+      expect.hasAssertions();
+      pc.setLocalDescription();
+      expect(setLocalDescriptionSpy).toHaveBeenCalledWith(undefined);
+    });
+    it('throws an error when the SDP has an invalid media line on Firefox', async () => {
+      expect.hasAssertions();
+      jest.spyOn(BrowserInfo, 'isFirefox').mockReturnValue(true);
+      await expect(
+        pc.setLocalDescription({ type: 'offer', sdp: 'm=video 9 UDP/TLS/RTP' })
+      ).rejects.toThrow(Error);
+    });
+  });
 });
diff --git a/src/peer-connection.ts b/src/peer-connection.ts
@@ -1,3 +1,4 @@
+import { BrowserInfo } from '@webex/web-capabilities';
 import { ConnectionState, ConnectionStateHandler } from './connection-state-handler';
 import { EventEmitter, EventMap } from './event-emitter';
 import { createRTCPeerConnection } from './rtc-peer-connection-factory';
@@ -199,6 +200,21 @@ class PeerConnection extends EventEmitter<PeerConnectionEventHandlers> {
   async setLocalDescription(
     description?: RTCSessionDescription | RTCSessionDescriptionInit
   ): Promise<void> {
+    // In Firefox, setLocalDescription will not throw an error if an m-line has no codecs, even
+    // though it violates https://datatracker.ietf.org/doc/html/rfc8866. See
+    // https://bugzilla.mozilla.org/show_bug.cgi?id=1857612. So, we check the media lines here to
+    // preemptively throw an error on Firefox.
+    if (BrowserInfo.isFirefox()) {
+      description?.sdp
+        ?.split(/(\r\n|\r|\n)/)
+        .filter((line) => line.startsWith('m'))
+        .forEach((mediaLine) => {
+          if (mediaLine.split(' ').length < 4) {
+            throw new Error(`Invalid media line ${mediaLine}, expected at least 4 fields`);
+          }
+        });
+    }
+
     return this.pc.setLocalDescription(description);
   }
 

diff --git a/yarn.lock b/yarn.lock
@@ -2685,6 +2685,13 @@
     events "^3.3.0"
     typed-emitter "^2.1.0"
 
+"@webex/web-capabilities@^1.1.0":
+  version "1.1.0"
+  resolved "https://registry.yarnpkg.com/@webex/web-capabilities/-/web-capabilities-1.1.0.tgz#7e0e7c8b67ee9cb6b795fecf099ff748ef7d13c6"
+  integrity sha512-Sh++KBHhy8VmEQZtSFuKrvq82SgEILNkAfWvoJGWpCpV9+VirEpUdCYxwBv03V+DF2EcxmOcRt/n8bzjIG1b/A==
+  dependencies:
+    bowser "^2.11.0"
+
 "@webex/web-media-effects@^2.7.0":
   version "2.7.0"
   resolved "https://registry.yarnpkg.com/@webex/web-media-effects/-/web-media-effects-2.7.0.tgz#f203f9512fb95d0639c74c10f39eda2e58663739"
@@ -3276,6 +3283,11 @@ bottleneck@^2.18.1:
   resolved "https://registry.yarnpkg.com/bottleneck/-/bottleneck-2.19.5.tgz#5df0b90f59fd47656ebe63c78a98419205cadd91"
   integrity sha512-VHiNCbI1lKdl44tGrhNfU3lup0Tj/ZBMJB5/2ZbNXRCPuRCO7ed2mgcK4r17y+KB2EfuYuRaVlwNbAeaWGSpbw==
 
+bowser@^2.11.0:
+  version "2.11.0"
+  resolved "https://registry.yarnpkg.com/bowser/-/bowser-2.11.0.tgz#5ca3c35757a7aa5771500c70a73a9f91ef420a8f"
+  integrity sha512-AlcaJBi/pqqJBIQ8U9Mcpc9i8Aqxn88Skv5d+xBX006BY5u8N3mGLHa5Lgppa7L/HfwgwLgZ6NYs+Ag6uUmJRA==
+
 brace-expansion@^1.1.7:
   version "1.1.11"
   resolved "https://registry.yarnpkg.com/brace-expansion/-/brace-expansion-1.1.11.tgz#3c7fcbf529d87226f3d2f52b966ff5271eb441dd"