fix: Fetch local fingerprint from SDP #109

ckousik · 2023-04-03T16:45:15Z

Since firefox does not support getFingerprints, we fetch the local fingerprint from the PeerConnection's localDescription field by parsing the fingerprint line from the SDP.

mxinden

Thanks for tackling this.

mxinden · 2023-04-04T10:57:36Z

src/sdp.ts

+// import { detect } from 'detect-browser'
+
+// const browser = detect()
+


Left over to be removed, right?

mxinden · 2023-04-04T10:59:03Z

src/transport.ts

-    if (localCert === undefined || localCert.getFingerprints().length === 0) {
-      throw invalidArgument('no fingerprint on local certificate')
-    }


Not sure how this is handled in the js-libp2p codebase in general. Should we still use localCert.getFingerprints in case the API is available (e.g. on Chrome), or should we always use the hack through the SDP?

I would prefer the single code path across browsers.

I think I would prefer trying to use the best option and falling back on the hack. But not blocking this.

I agree with Marco here, but it would be good to know (debug logs) which option was used if we're going to have different paths.

@SgtPooki I've added the fallback to fetching from the SDP as requested.

mxinden · 2023-04-09T14:40:20Z

@achingbrain @MarcoPolo any objections to merging here?

MarcoPolo

Can we have firefox run this test in CI?

SgtPooki · 2023-04-14T09:59:44Z

src/sdp.ts

+const fingerprintRegex = /^a=fingerprint:(?:\w+-[0-9]+)\s(?<fingerprint>(:?[0-9a-fA-F]{2})+)$/gm
+function getFingerprintFromSdp (sdp: string): string | undefined {
+  const searchResult = fingerprintRegex.exec(sdp)
+  if (searchResult == null) {
+    return ''
+  }
+
+  return searchResult.groups?.fingerprint
+}


Prefer regex.match.

Using /g flag with exec can cause unexpected issues with the stored lastIndex being updated and then causing match failures in later .exec calls.

JavaScript RegExp objects are stateful when they have the global or sticky flags set (e.g. /foo/g or /foo/y). They store a lastIndex from the previous match. Using this internally, exec() can be used to iterate over multiple matches in a string of text (with capture groups), as opposed to getting just the matching strings with String.prototype.match().

see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/exec#description

Also see https://stackoverflow.com/a/3811949/592760

Thanks @SgtPooki . I was not aware of this. I've incorporated the change and added a simple test.

SgtPooki · 2023-04-14T10:08:30Z

src/sdp.ts

+const fingerprintRegex = /^a=fingerprint:(?:\w+-[0-9]+)\s(?<fingerprint>(:?[0-9a-fA-F]{2})+)$/gm
+function getFingerprintFromSdp (sdp: string): string | undefined {
+  const searchResult = fingerprintRegex.exec(sdp)


Suggested change

const fingerprintRegex = /^a=fingerprint:(?:\w+-[0-9]+)\s(?<fingerprint>(:?[0-9a-fA-F]{2})+)$/gm

function getFingerprintFromSdp (sdp: string): string | undefined {

const searchResult = fingerprintRegex.exec(sdp)

const fingerprintRegex = /^a=fingerprint:(?:\w+-[0-9]+)\s(?<fingerprint>(:?[0-9a-fA-F]{2})+)$/m

function getFingerprintFromSdp (sdp: string): string | undefined {

const searchResult = sdp.match(fingerprintRegex)

Since firefox does not support `getFingerprints`, we fetch the local fingerprint from the PeerConnection's `localDescription` field by parsing the fingerprint line from the SDP.

p-shahi · 2023-04-18T21:34:11Z

@MarcoPolo ping to re-review

ckousik · 2023-04-19T10:31:30Z

@MarcoPolo I'm still working on firefox browser-to-server tests in CI. Could we move that work to a subsequent PR?

ckousik · 2023-04-24T13:28:14Z

@MarcoPolo The browser-to-server tests is running on Firefox in CI.

SgtPooki

i'm not an expert in the business logic of what is going on here, but i ran through the code and found a single potential failure that should be addressed.

SgtPooki · 2023-04-24T19:47:21Z

src/sdp.ts

+  const searchResult = sdp.match(fingerprintRegex)
+  if (searchResult == null) {
+    return ''
+  }
+
+  return searchResult.groups?.fingerprint


'' == null // false

Returning '' on line 49 will cause the check on line 215 in src/transports.ts, if (localFingerprint == null) { to return false. We should return undefined here, or even better:

Suggested change

const searchResult = sdp.match(fingerprintRegex)

if (searchResult == null) {

return ''

}

return searchResult.groups?.fingerprint

const searchResult = sdp.match(fingerprintRegex)

return searchResult?.groups?.fingerprint

SgtPooki · 2023-04-24T19:48:11Z

test/sdp.spec.ts

@@ -53,6 +53,11 @@ describe('SDP', () => {
    ])
  })

+  it('extracts a fingerprint from sdp', () => {


we should add a negative test here to validate the fallback behaviour

this just tests the regex behaviour. While there are no explicit tests for the fallback behaviour, it gets tested when the tests are run in Firefox as part of CI.

SgtPooki

Lgtm but i’ll defer to primary maintainers for final approval

p-shahi · 2023-05-01T19:45:05Z

@MarcoPolo bump!

MarcoPolo · 2023-05-02T18:13:03Z

src/transport.ts

+      source: {
+        [Symbol.asyncIterator]: async function * () {
+          for await (const list of wrappedChannel.source) {
+            yield list.subarray()


This may copy unnecessarily (I know this was here before).

src/stream.ts

src/transport.ts

examples/browser-to-server/package.json

MarcoPolo · 2023-05-02T20:20:33Z

Thanks a lot @SgtPooki! Your review was very helpful :)

And thanks @wemeetagain for taking a look

they gave a lgtm

## [1.1.10](v1.1.9...v1.1.10) (2023-05-03) ### Bug Fixes * Fetch local fingerprint from SDP ([#109](#109)) ([3673d6c](3673d6c))

github-actions · 2023-05-03T00:17:47Z

🎉 This PR is included in version 1.1.10 🎉

The release is available on:

Your semantic-release bot 📦🚀

p-shahi linked an issue Apr 3, 2023 that may be closed by this pull request

Firefox fails in browser-to-server #96

Closed

ckousik mentioned this pull request Apr 3, 2023

Firefox fails in browser-to-server #96

Closed

ckousik marked this pull request as ready for review April 4, 2023 06:47

ckousik requested review from MarcoPolo and ddimaria April 4, 2023 06:47

mxinden reviewed Apr 4, 2023

View reviewed changes

MarcoPolo reviewed Apr 10, 2023

View reviewed changes

ckousik force-pushed the ckousik/fix-local-fingerprint-from-sdp branch from 58276ef to 7e26a4c Compare April 11, 2023 13:12

p-shahi mentioned this pull request Apr 12, 2023

Remaining tasks for mvp of app libp2p/universal-connectivity#35

Closed

21 tasks

SgtPooki suggested changes Apr 14, 2023

View reviewed changes

SgtPooki reviewed Apr 14, 2023

View reviewed changes

ckousik added 3 commits April 18, 2023 17:01

fix: Fetch local fingerprint from SDP

e08afbe

Since firefox does not support `getFingerprints`, we fetch the local fingerprint from the PeerConnection's `localDescription` field by parsing the fingerprint line from the SDP.

remove erroneous comment

9cd5ce1

update sdp

3c87add

ckousik force-pushed the ckousik/fix-local-fingerprint-from-sdp branch from 7e26a4c to 3c87add Compare April 18, 2023 11:32

ckousik requested a review from SgtPooki April 18, 2023 11:33

p-shahi requested a review from MarcoPolo April 18, 2023 21:33

p-shahi mentioned this pull request Apr 18, 2023

once available, upgrade to js-libp2p-webrtc with Firefox support libp2p/universal-connectivity#58

Open

ckousik added 3 commits April 24, 2023 17:46

working browser-to-server in firefox

e2e0c72

test firefox browser-to-server in CI

cd747c1

remove hack

a426e13

SgtPooki previously requested changes Apr 24, 2023

View reviewed changes

address review comment

57dd380

SgtPooki reviewed Apr 26, 2023

View reviewed changes

Merge branch 'main' into ckousik/fix-local-fingerprint-from-sdp

6261cc0

Review nits

2a481e4

MarcoPolo approved these changes May 2, 2023

View reviewed changes

Rename test file for playwright

777b16a

wemeetagain reviewed May 2, 2023

View reviewed changes

src/stream.ts Show resolved Hide resolved

src/transport.ts Outdated Show resolved Hide resolved

examples/browser-to-server/package.json Outdated Show resolved Hide resolved

Build once for browser tests

def274b

MarcoPolo mentioned this pull request May 2, 2023

refactor: remove use of timeout-abort-controller #148

Merged

Move code back to make diff smaller

7d1561f

MarcoPolo approved these changes May 2, 2023

View reviewed changes

MarcoPolo merged commit 3673d6c into main May 3, 2023

MarcoPolo deleted the ckousik/fix-local-fingerprint-from-sdp branch May 3, 2023 00:13

github-actions bot pushed a commit that referenced this pull request May 3, 2023

chore(release): 1.1.10 [skip ci]

68b446d

## [1.1.10](v1.1.9...v1.1.10) (2023-05-03) ### Bug Fixes * Fetch local fingerprint from SDP ([#109](#109)) ([3673d6c](3673d6c))

github-actions bot added the released label May 3, 2023

mxinden mentioned this pull request Sep 6, 2023

feat(webrtc): add WebRTC for WASM environments libp2p/rust-libp2p#4248

Merged

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Fetch local fingerprint from SDP #109

fix: Fetch local fingerprint from SDP #109

ckousik commented Apr 3, 2023

mxinden left a comment

mxinden Apr 4, 2023

mxinden Apr 4, 2023

ckousik Apr 5, 2023

MarcoPolo Apr 10, 2023

SgtPooki Apr 14, 2023

ckousik Apr 18, 2023

mxinden commented Apr 9, 2023

MarcoPolo left a comment

SgtPooki Apr 14, 2023 •

edited

Loading

ckousik Apr 18, 2023

SgtPooki Apr 14, 2023

ckousik Apr 18, 2023

p-shahi commented Apr 18, 2023

ckousik commented Apr 19, 2023

ckousik commented Apr 24, 2023

SgtPooki left a comment

SgtPooki Apr 24, 2023

ckousik Apr 26, 2023

SgtPooki Apr 24, 2023

ckousik Apr 26, 2023

SgtPooki left a comment

p-shahi commented May 1, 2023

MarcoPolo May 2, 2023

MarcoPolo commented May 2, 2023

github-actions bot commented May 3, 2023

		// import { detect } from 'detect-browser'

		// const browser = detect()

fix: Fetch local fingerprint from SDP #109

fix: Fetch local fingerprint from SDP #109

Conversation

ckousik commented Apr 3, 2023

mxinden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxinden commented Apr 9, 2023

MarcoPolo left a comment

Choose a reason for hiding this comment

SgtPooki Apr 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-shahi commented Apr 18, 2023

ckousik commented Apr 19, 2023

ckousik commented Apr 24, 2023

SgtPooki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SgtPooki left a comment

Choose a reason for hiding this comment

p-shahi commented May 1, 2023

Choose a reason for hiding this comment

MarcoPolo commented May 2, 2023

github-actions bot commented May 3, 2023

SgtPooki Apr 14, 2023 •

edited

Loading