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

Explicitly provide the "Origin" header on session establishment #370

Merged
merged 1 commit into from
Oct 18, 2021

Conversation

yutakahirano
Copy link
Contributor

@yutakahirano yutakahirano commented Oct 13, 2021

Use |transport|'s relevant settings object's origin.

Fixes #368.


Preview | Diff

Use |transport|'s relevant settings object's origin.
@yutakahirano
Copy link
Contributor Author

@zcorpan can you take a look?

Copy link
Member

@zcorpan zcorpan left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@zcorpan
Copy link
Member

zcorpan commented Oct 13, 2021

For web-platform-tests, you can test this (verify that the origin comes from the right environment settings object) by using the same pattern as one of these:

$ find . -name "multi-globals"
./websockets/multi-globals
./html/cross-origin-embedder-policy/multi-globals
./fetch/api/response/multi-globals
./fetch/api/request/multi-globals
./workers/multi-globals
./webmessaging/multi-globals
./service-workers/service-worker/multi-globals

The websockets tests can be used as a basis for which globals to include in the test, since it also uses a constructor.

However, here we'd want to test the value of the Origin header, and so the entry page should be a different origin from the other pages. That means you have to use subdomains, and use document.domain to opt-in to DOM access.

@yutakahirano
Copy link
Contributor Author

We have a test ('Echo back request headers') in https://github.com/web-platform-tests/wpt/blob/master/webtransport/connect.https.any.js.

@zcorpan
Copy link
Member

zcorpan commented Oct 14, 2021

Nice, though it doesn't seem to exercise multiple globals and the possibility of the browser advertising the origin of the wrong environment settings object (e.g. incumbent settings object instead of the relevant settings object).

I don't think such a test needs to block this PR, though.

@yutakahirano yutakahirano merged commit 0a62f0c into main Oct 18, 2021
@yutakahirano yutakahirano deleted the yhirano/origin branch October 18, 2021 00:26
github-actions bot added a commit that referenced this pull request Oct 18, 2021
SHA: 0a62f0c
Reason: push, by @yutakahirano

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

Where is the value of the Origin header set?
3 participants