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.

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

Use the url spec to parse ice server urls. #2694

Closed
wants to merge 6 commits into from
Closed

Conversation

jan-ivar
Copy link
Member

@jan-ivar jan-ivar commented Nov 2, 2021

Fixes #2660 (draft).

@annevk I'm open to ideas on how to iterate further while at the same time capturing the normative requirements in rfc7064 and rfc7065, such as "Developers MUST NOT use a generic hierarchical URI parser to parse a "stun" or "stuns" URI."


Preview | Diff

@jan-ivar jan-ivar self-assigned this Nov 2, 2021
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking this!

webrtc.html Outdated Show resolved Hide resolved
webrtc.html Show resolved Hide resolved
webrtc.html Outdated Show resolved Hide resolved
webrtc.html Outdated
<p>If <var>scheme</var> is <code>stun</code> or
<code>stuns</code>, and <var>parsedURL</var> violates
the syntax defined in [[!RFC7064]] section 3.1,
then [= exception/throw =] a {{SyntaxError}}.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't work as parsedURL is a data structure. How is this implemented?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This tries to preserve the existing normative language. I shold s/parsedURL/url/ again perhaps. As I mention in the OP, looking for ideas on how to specify the requirements using the URL spec. We want to prevent hierarchical urls, e.g. stun:foo.bar.com:1234

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Firefox, this is implemented here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That URL isn't hierarchical though, is it? Hierarchical URLs contain slashes, as far as I know.

It sounds like you want https://url.spec.whatwg.org/#url-opaque-path to be true, although you wouldn't have to check for credentials in that case.

And then as per the Firefox code you'd have to parse parsedURL's path (which will be a string in this case) in a custom manner to obtain the host and port (split on :, then handle each?). Potentially this logic could be shared with the URL Standard in some way.

And then you'd handle the parsedURL's query as per Firefox logic for the query.

@jan-ivar
Copy link
Member Author

jan-ivar commented Nov 2, 2021

@dontcallmedom The failing infra checks here seem unrelated, and appear from DOMTimeStamp being no more. Should we perhaps merge #2686 until #2674 has been resolved? Otherwise we appear blocked from submitting any PRs on this repo, unless there's some workaround to suppress the errors?

@jan-ivar
Copy link
Member Author

jan-ivar commented Apr 3, 2023

Closing in favor of #2853.

@jan-ivar jan-ivar closed this Apr 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unclear how URLs are processed
4 participants