Skip to content
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.

Sign up for GitHub

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

Jump to bottom

peerjs needs Content-Security-Policy script-src: unsafe-eval #1247

Closed
1 task done
andreialecu opened this issue Mar 7, 2024 · 3 comments · Fixed by #1248
Closed
1 task done

peerjs needs Content-Security-Policy script-src: unsafe-eval #1247

andreialecu opened this issue Mar 7, 2024 · 3 comments · Fixed by #1248
Assignees
@jonasgloning
Labels
bug client related to peerjs client confirmed acknowledged as an issue

Comments

@andreialecu
Copy link

Please, check for existing issues to avoid duplicates.

  • No similar issues found.

What happened?

Because of the dependency on cbor-x, which does this check, the page where peerjs is used requires a CSP of script-src: unsafe-eval, which is extremely unrecommended.

CleanShot 2024-03-07 at 16 19 54@2x

The fix here would be to use one of the *-no-eval exports: https://github.com/kriszyp/cbor-x/blob/0b5e8807622619c6a7a062f7e771478ecfd52f83/package.json#L58-L59

Meaning that this line:

peerjs/lib/dataconnection/StreamConnection/Cbor.ts

Line 2 in 708c55c

import { Decoder, Encoder } from "cbor-x";

Should probably be:

import { Decoder, Encoder } from "cbor-x/index-no-eval";

How can we reproduce the issue?

Add an HTTP header of:
Content-Security-Policy-Report-Only: script-src 'self' and notice that unsafe-eval is required on this line:
https://github.com/kriszyp/cbor-x/blob/0b5e8807622619c6a7a062f7e771478ecfd52f83/decode.js#L37-L44

What do you expected to happen?

unsafe-eval should not be required. CSP error should not appear.

This will also spam the reporting endpoint with CSP errors, if one is set up.

Environment setup

  • OS:
  • Platform:
  • Browser:

Is this a regression?

No response

Anything else?

No response
@andreialecu andreialecu added the bug label Mar 7, 2024
@andreialecu andreialecu mentioned this issue Mar 8, 2024
Open
@jonasgloning jonasgloning added the confirmed acknowledged as an issue label Mar 9, 2024 — with Linear
@jonasgloning jonasgloning self-assigned this Mar 9, 2024
@jonasgloning jonasgloning added the client related to peerjs client label Mar 9, 2024 — with Linear
@jonasgloning jonasgloning linked a pull request Mar 9, 2024 that will close this issue
fix: remove need for unsafe-eval #1248
Merged
@jonasgloning Linear
Copy link
Member

Hey Andrei, thank you for the bug report!

We agree this is a bug. PeerJS shouldn’t be relying on unsafe-eval.

Sadly this appears to be a bit more complicated than your proposed fix in our case:
The current bundling/typescript setup doesn’t allow for importing from a package export yet. We’ll hopefully address this soon, but this could explode in complexity and will take some time.

@andreialecu
Copy link
Author

kriszyp/cbor-x#102 got merged, I believe the types should behave better now.

@jonasgloning Linear
Copy link
Member

There's a new version on @rc https://www.npmjs.com/package/peerjs/v/1.5.3-rc.1 that includes a fix (#1248). You can install it with npm i --save peerjs@rc

Import paths still break some bundler implementations we test against — one of the reasons for the large delay.
We decided to still merge the PR. Projects affected should (and can) just update their tooling.

The team will meet again and release a new stable version on Saturday, 11th of May.

jonasgloning added a commit that referenced this issue May 14, 2024
@jonasgloning
fix: remove CBOR
dc6f3f6 
Using CBOR forces us to choose between #1271 and #1247. Our complicated
importing and bundling situation makes using this library very hard.

CBOR support has been undocumented, and we are not aware of significant
usage in the wild. Therefore, we do not consider this a breaking change.
To make our expectations clearer, this PR also marks MessagePack as
`experimental`.

We will improve our importing and bundling situation before
reintroducing CBOR via a plugin.

Closes #1271
@jonasgloning jonasgloning mentioned this issue May 14, 2024
Merged
jonasgloning added a commit that referenced this issue May 14, 2024
@jonasgloning
fix: remove CBOR
badc9e8 
Using CBOR forces us to choose between #1271 and #1247. Our complicated
importing and bundling situation makes using this library very hard.

CBOR support has been undocumented, and we are not aware of significant
usage in the wild. Therefore, we do not consider this a breaking change.
To make our expectations clearer, this PR also marks MessagePack as
`experimental`.

We will improve our importing and bundling situation before
reintroducing CBOR via a plugin.

Closes #1271
github-actions bot pushed a commit that referenced this issue May 14, 2024
@semantic-release-bot @jonasgloning
chore(release): 1.5.4 [skip ci]
867b1f7 
## [1.5.4](v1.5.3...v1.5.4) (2024-05-14)

### Bug Fixes

* **deps:** update dependency webrtc-adapter to v9 ([#1266](#1266)) ([5536abf](5536abf))
* remove CBOR ([badc9e8](badc9e8)), closes [#1271](#1271) [#1247](#1247) [#1271](#1271)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jonasgloning jonasgloning

Labels
bug client related to peerjs client confirmed acknowledged as an issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: remove need for unsafe-eval peers/peerjs
2 participants
@andreialecu @jonasgloning