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

Add ['wss', 'ws'] in "Strip URLs for use in reports" allow-list. #533

Closed
wants to merge 1 commit into from

Conversation

ArthurSonzogni
Copy link
Member

@ArthurSonzogni ArthurSonzogni commented Nov 22, 2021

This is a follow-up to:
#527 (comment)

This is needed, until browsers rewrite wss/ws URL in fetch before going
through CSP:
https://github.com/w3c/webappsec-csp

Associated WPT PR:
web-platform-tests/wpt#31702

This is a follow-up to:
w3c#527 (comment)

This is needed, until browsers rewrite wss/ws URL in fetch before going
through CSP:
https://github.com/w3c/webappsec-csp

Associated WPT PR:
web-platform-tests/wpt#31702
@ArthurSonzogni
Copy link
Member Author

++ @Rob--W FYI.

@annevk
Copy link
Member

annevk commented Nov 22, 2021

As per my comment this doesn't work. If we want to change this just in CSP, which seems to be the safest option, we'd essentially undo the scheme change as part of "strip URLs for use in reports" (perhaps it ought to be named "prepare URLs for use in reports"), based on request's mode.

@ArthurSonzogni
Copy link
Member Author

ArthurSonzogni commented Nov 22, 2021

As per my comment this doesn't work.

Sorry, I don't get why this doesn't work and what you would like to do. I have zero knowledge about WebSocket. Would you have a spec link about the scheme change you are talking about?

I see request.mode can be "websocket":
https://fetch.spec.whatwg.org/#concept-request-mode

And the websocket connection obtain algorithm:
https://fetch.spec.whatwg.org/#concept-websocket-connection-obtain

Happy to abandon this patch or work on something else if you would like. Still, I believe I need to implement it in Chrome in order not to strip useful informations from ws/wss URLs for now.

@annevk
Copy link
Member

annevk commented Nov 22, 2021

Okay, WebSocket schemes change in https://fetch.spec.whatwg.org/#websocket-opening-handshake (which is what new WebSocket() uses). Fetch invokes https://w3c.github.io/webappsec-csp/#should-block-request at some point. Due to request's mode, CSP has sufficient context to change the schemes back wherever it needs to do that. That seems like the easiest solution.

To make the scheme change part of "strip URLs use in reports" you'd have to pass the WebSocket context along somehow. An alternative might be to change the URL earlier.

@ArthurSonzogni
Copy link
Member Author

I see! Thanks!

So you would like:

  • browsers to correctly forward the transformed scheme before reaching "prepare URLs for use in reports".
  • Insert step 1. below to recover the right "ws", "wss" schemes.
prepare-URLs-for-use-in-reports(url as URL, request as request-or-null) 

  1. If `request` is not null and `request`'s mode is "`websocket`"
    a.  Assert `url`'s scheme is either "`http`" or "`https`".
    b.  If `url`'s scheme is "`https`", set `url` 's scheme to "`wss`", else set `url` 's scheme to "`ws`".
  2. If url’s scheme is not "https", "'http'", "wss" or "ws" then return url’s scheme.
  3. Set `url`’s fragment to the empty string.
  4. Set `url`’s username to the empty string.
  5. Set `url`’s password to the empty string.
  6. Return the result of executing the URL serializer on url.

IMO, inserting step 1 could be done independently from this patch. I can land this patch if you want, or we can wait for someone to plumb the request-or-null toward this location. The second option sounds like non trivial amount of work, so I don't believe I will be able to spent this time.

@annevk
Copy link
Member

annevk commented Nov 22, 2021

If you flip the order of 1 and 2, this PR doesn't have to land and you don't need the proposed 1a either.

@ArthurSonzogni
Copy link
Member Author

If you flip the order of 1 and 2, this PR doesn't have to land and you don't need the proposed 1a either.

Yes, that's a way to see this. So I guess I have to abandon this ;-)

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.

None yet

2 participants