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

Test webrtc/content-security-policy integration #32914

Merged

jan-ivar merged 6 commits into web-platform-tests:master from zenhack:webrtc-csp

Apr 18, 2022

Contributor

zenhack commented Feb 20, 2022

...as specified in:

This is a work in progress; in particular:

Right now all of the "allow" tests fail with:

  ▶ ERROR [expected OK] /content-security-policy/webrtc/webrtc-allowed-nopolicy.html
  └   → Unhandled rejection: Unknown ufrag (3a846ca3)

If I silence that one rejection they pass (and the "blocked" tests still fail), but I don't know the right approach to make sure that event doesn't fire before we can handle it.

I'm very fuzzy on whether this is actually the right approach; right now this tries to monitor the ice agent's transitions and make sure it goes straight from new to failed, but my grasp of the intricacies of webrtc is still somewhat tenuous, so I would not be surprised if the logic for this was wrong in some critical way.

I would appreciate feedback.

cc: @jan-ivar @alvestrand @annevk @antosart


          Test webrtc/content-security-policy integration

...as specified in:

- w3c/webappsec-csp#457
- w3c/webrtc-extensions#81

wpt-pr-bot added the content-security-policy label

wpt-pr-bot assigned hillbrad

wpt-pr-bot requested review from andypaicu and hillbrad

February 20, 2022 00:08

zenhack mentioned this pull request

Introduce 'webrtc' as a simple on/off switch w3c/webappsec-csp#457

Merged

zenhack commented

View reviewed changes

content-security-policy/webrtc/webrtc.js Outdated Show resolved Hide resolved

antosart reviewed

View reviewed changes

Contributor

antosart left a comment

Sorry, I don't have enough knowledge of webrtc to review the code. Hope some of the other reviewers does.

content-security-policy/webrtc/webrtc.js Outdated Show resolved Hide resolved

zenhack mentioned this pull request

Ian: Push web standards to allow blocking WebRTC sandstormports/community-project#12

Open

Contributor Author

zenhack commented Mar 7, 2022

@jan-ivar, @alvestrand, any thoughts?

Contributor Author

zenhack commented Mar 18, 2022

Ping?

jan-ivar requested changes

View reviewed changes

content-security-policy/webrtc/webrtc.js Outdated Show resolved Hide resolved

content-security-policy/webrtc/webrtc.js Outdated Show resolved Hide resolved

content-security-policy/webrtc/webrtc.js Show resolved Hide resolved

content-security-policy/webrtc/webrtc.js Outdated Show resolved Hide resolved

Member

jan-ivar commented Mar 21, 2022

@zenhack That should also take care of the "Unknown ufrag" errors. Otherwise the tests look good, thanks for writing them!

jan-ivar reviewed

View reviewed changes

content-security-policy/webrtc/webrtc.js Outdated Show resolved Hide resolved

zenhack and others added 4 commits

March 22, 2022 01:11


          Fix typos in comment.

9282fe2

Co-authored-by: Jan-Ivar Bruaroey <jan-ivar@users.noreply.github.com>


          Fix ice candidate exchange in CSP webrtc tests.

e86a2c3

we should be passing each candidate to the *other* pc.

Tests now behave as expected.


          CSP webrtc tests: listen for connection state change, not gathering.

9598fb6

See web-platform-tests#32914 (comment)


          CSP webrtc tests: drop unnecessary stun server.

800391f

Contributor Author

zenhack commented Mar 22, 2022

Ok, I think I have addressed everything.

zenhack requested a review from jan-ivar

March 22, 2022 05:37

jan-ivar approved these changes

View reviewed changes

content-security-policy/webrtc/webrtc.js Outdated Show resolved Hide resolved


          webrtc csp: simplify state checking

5a3b66e

"new" is the initial state.

Co-authored-by: Jan-Ivar Bruaroey <jan-ivar@users.noreply.github.com>

Contributor Author

zenhack commented Apr 3, 2022

@annevk, @alvestrand, ping?

Contributor Author

zenhack commented Apr 12, 2022

So this has one sign-off and no other comments. What is the next step?

Contributor

youennf commented Apr 14, 2022

LGTM as well.
It might be cool to add a test with two iframes, one being blocked and the other one not being blocked.
In any case, can we merge it as is?

Member

jan-ivar commented Apr 14, 2022 •

edited

Loading

@annevk and @antosart the WebRTC parts here look good to us. Can it be merged?

It might be cool to add a test with two iframes, one being blocked and the other one not being blocked.

That might take some more work, like postMessage between the iframes. A follow-up issue might be good for that.

jan-ivar requested a review from annevk

April 15, 2022 15:26

Member

jan-ivar commented Apr 18, 2022

Based on and earlier comment from @antosart and @annevk's approval of the PR this is testing, I think we're good to merge.

jan-ivar merged commit 0abba58 into web-platform-tests:master

DanielRyanSmith pushed a commit to DanielRyanSmith/wpt that referenced this pull request


          Test webrtc/content-security-policy integration (web-platform-tests#3…

e4ac733

…2914)

* Test webrtc/content-security-policy integration

...as specified in:

- w3c/webappsec-csp#457
- w3c/webrtc-extensions#81

* Fix typos in comment.

Co-authored-by: Jan-Ivar Bruaroey <jan-ivar@users.noreply.github.com>

* Fix ice candidate exchange in CSP webrtc tests.

we should be passing each candidate to the *other* pc.

Tests now behave as expected.

* CSP webrtc tests: listen for connection state change, not gathering.

See web-platform-tests#32914 (comment)

* CSP webrtc tests: drop unnecessary stun server.

* webrtc csp: simplify state checking

"new" is the initial state.

Co-authored-by: Jan-Ivar Bruaroey <jan-ivar@users.noreply.github.com>

Co-authored-by: Jan-Ivar Bruaroey <jan-ivar@users.noreply.github.com>

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request


          Bug 1756328 [wpt PR 32914] - Test webrtc/content-security-policy inte…

f71b1ea

…gration, a=testonly

Automatic update from web-platform-tests
Test webrtc/content-security-policy integration (#32914)

* Test webrtc/content-security-policy integration

...as specified in:

- w3c/webappsec-csp#457
- w3c/webrtc-extensions#81

* Fix typos in comment.

Co-authored-by: Jan-Ivar Bruaroey <jan-ivar@users.noreply.github.com>

* Fix ice candidate exchange in CSP webrtc tests.

we should be passing each candidate to the *other* pc.

Tests now behave as expected.

* CSP webrtc tests: listen for connection state change, not gathering.

See web-platform-tests/wpt#32914 (comment)

* CSP webrtc tests: drop unnecessary stun server.

* webrtc csp: simplify state checking

"new" is the initial state.

Co-authored-by: Jan-Ivar Bruaroey <jan-ivar@users.noreply.github.com>

Co-authored-by: Jan-Ivar Bruaroey <jan-ivar@users.noreply.github.com>
--

wpt-commits: 0abba58602758eb8be11c38788c6f51fed2529e4
wpt-pr: 32914

jamienicol pushed a commit to jamienicol/gecko that referenced this pull request


          Bug 1756328 [wpt PR 32914] - Test webrtc/content-security-policy inte…

71cc253

…gration, a=testonly

Automatic update from web-platform-tests
Test webrtc/content-security-policy integration (#32914)

* Test webrtc/content-security-policy integration

...as specified in:

- w3c/webappsec-csp#457
- w3c/webrtc-extensions#81

* Fix typos in comment.

Co-authored-by: Jan-Ivar Bruaroey <jan-ivar@users.noreply.github.com>

* Fix ice candidate exchange in CSP webrtc tests.

we should be passing each candidate to the *other* pc.

Tests now behave as expected.

* CSP webrtc tests: listen for connection state change, not gathering.

See web-platform-tests/wpt#32914 (comment)

* CSP webrtc tests: drop unnecessary stun server.

* webrtc csp: simplify state checking

"new" is the initial state.

Co-authored-by: Jan-Ivar Bruaroey <jan-ivar@users.noreply.github.com>

Co-authored-by: Jan-Ivar Bruaroey <jan-ivar@users.noreply.github.com>
--

wpt-commits: 0abba58602758eb8be11c38788c6f51fed2529e4
wpt-pr: 32914

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

antosart antosart left review comments

jan-ivar jan-ivar approved these changes

andypaicu Awaiting requested review from andypaicu

hillbrad Awaiting requested review from hillbrad

annevk Awaiting requested review from annevk

Labels

content-security-policy