-
Notifications
You must be signed in to change notification settings - Fork 115
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.
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
Unclear how URLs are processed #2660
Comments
There's other issues that follow from this as well, such as whether new RTCPeerConnection({ iceServers: [{ urls:[] }] }); ought to throw. It does in Firefox, but not Chrome and Safari. Are there no web-platform-tests for all these cases? |
Regarding construction, 4.4.1.1 (the constructor), step 11 says:
Which references 4.4.1.6. Regarding empty URLs, step 4.4.1.6, 10.4 says:
Missing tests is a big no no of course. 🙂 |
Thanks! Not sure how I missed that. So I guess this comes down to:
|
The tests already exist:
|
the URL specified in https://datatracker.ietf.org/doc/html/rfc7065#section-3.1 explicitly does not allow a path component. I don't know if the URL spec parse algorithm has an argument to it that says "fail if a path is attempted". Does it? |
Note: the browsers do have a parser for TURN URL parsing; for Chrome, it is https://source.chromium.org/chromium/chromium/src/+/main:third_party/webrtc/pc/ice_server_parsing.cc;drc=65c9257f1777731d6d0669598f6fe6fe65fa61d3;l=149?q=webrtc::ParseIceServers&ss=chromium |
@alvestrand you'll get a path given how these URLs look. (That's also the case with RFC3986 btw, see https://datatracker.ietf.org/doc/html/rfc3986#section-3.) I tried to spell out in #2694 (comment) in some detail how the resulting URL record ought to be processed. (And yeah, some browsers have per-scheme URL parsers. That goes against a generic URL syntax and is something we should avoid enshrining to the extent possible.) |
I sketched an algorithm for this in https://bugs.webkit.org/show_bug.cgi?id=164508#c10. I would appreciate feedback from the WG. |
FWIW. I am not sure that attempting to fit them into a generic URL scheme is something that should be done at W3C level . |
What does "are special" mean? We need to define processing for them that is compatible with the URL Standard. We can certainly ignore query parameters as well. My processing model attempted to match the RFC syntax requirements, but we can also do something else, I don't really care, as long as we don't need a different URL parser. |
It is not clear to me why webrtc-pc even tries to specify how to parse those urls. These are passed to the "ICE agent" (however, JSEP is silent about this here).
Why? (and why is it not consistent with the underlying one...) |
Well, that's consistent with how we treat URLs elsewhere in the platform. Parse them early to avoid surprises with IDNA, IPv4, etc. Also, the lower layer has to be consistent with that anyway so either way this needs to be defined. |
What surprises do you expect? The browser does not interact with those urls much, it passes them down to the "ICE Agent" which then handles all the interactions. |
Sure, but that shouldn't handle them differently from the browser or have a different idea as to what the host might be. And we do validate and normalize hosts upfront in the web platform and I don't think this has to be different here. |
Thanks! I can do a new PR based on that. Except this line:
I think here we need to not throw on |
https://w3c.github.io/webrtc-pc/#set-pc-configuration does input validation of the configuration object ahead of giving it to the ice agent, so JSEP doesn't need to.
Thanks for finding that. I think this is a good argument for specifying url validation better, and we should probably fix Firefox here. |
Chrome throws an exception for
whereas Firefox and Safari do not.
Where does https://w3c.github.io/webrtc-pc/#dom-peerconnection process these?
Some processing appears to happen in https://w3c.github.io/webrtc-pc/#set-the-configuration but that does not seem to be invoked from the constructor. (That also suggests using a URL parser that browsers do not have, which seems somewhat dubious.)
The text was updated successfully, but these errors were encountered: