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

Spec: Allow cross-origin script in addModule & align createWorklet #161

Merged
merged 15 commits into from
Jul 17, 2024

Conversation

pythagoraskitty
Copy link
Collaborator

@pythagoraskitty pythagoraskitty commented Jun 18, 2024

We make the spec changes to accompany #158 .

In particular, we make it possible for sharedStorage.worklet.addModule() to create a worklet with cross-origin worklet script. We also align the behavior of sharedStorage.createWorklet() with addmodule() so that it uses the invoking context's origin as the data partition origin by default, while adding a mechanism to use the script's origin instead if specified.

For a more detailed description, please see #158 .


Preview | Diff

We make the spec changes to accompany #158 . For a more detailed description, please see #158 .
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
Copy link
Collaborator

@xyaoinum xyaoinum left a comment

Choose a reason for hiding this comment

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

Thanks! Added a few nit comments. However, I think the following two sections also need to be updated:

@pythagoraskitty
Copy link
Collaborator Author

pythagoraskitty commented Jun 24, 2024

Thanks! Added a few nit comments. However, I think the following two sections also need to be updated:

Updated, could you please take another look? Thanks

Copy link
Collaborator

@xyaoinum xyaoinum left a comment

Choose a reason for hiding this comment

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

Looks good % I'm not entirely convinced that relying on destination "sharedstorageworkletcontextorigin" is ideal (also as noted in my Gerrit comment). I recommend getting input from a spec owner.

spec.bs Outdated Show resolved Hide resolved
@pythagoraskitty
Copy link
Collaborator Author

Looks good % I'm not entirely convinced that relying on destination "sharedstorageworkletcontextorigin" is ideal (also as noted in my Gerrit comment). I recommend getting input from a spec owner.

Thanks, will do. I will either need to add the additional worklet destination type, or else a new field to request, so that I can distinguish requests that we need to check for the "Shared-Storage-Cross-Origin-Worklet-Allowed" header from ones that we don't. As we don't currently have access to the data origin itself from the request, we either need to pass that, or some bit denoting whether it's cross-origin or not.

@wanderview
Copy link
Collaborator

wanderview commented Jul 3, 2024

Looks good % I'm not entirely convinced that relying on destination "sharedstorageworkletcontextorigin" is ideal (also as noted in my Gerrit comment). I recommend getting input from a spec owner.

Thanks, will do. I will either need to add the additional worklet destination type, or else a new field to request, so that I can distinguish requests that we need to check for the "Shared-Storage-Cross-Origin-Worklet-Allowed" header from ones that we don't. As we don't currently have access to the data origin itself from the request, we either need to pass that, or some bit denoting whether it's cross-origin or not.

I've reviewed this pull request in regards to the destination question. I think it would be preferable to monkey-patch an internal property on request instead.

My reasoning is that "how do we know when to add the header" is an internal implementation detail. In contrast, changing the destination is a change that results in different behavior for web sites. The destination value is exposed to sites a few different ways. So changing the destination for this purpose would require all sites checking the destination to now have to understand an additional enumerated value. In theory this should not be necessary given the information is covered in the newly exposed header.

In addition, using an internal request property is consistent with how the fetch spec tracks whether various headers need to be added.

Another option you could consider, though, is sending the worklet data origin explicitly in a request header. This would maybe benefit the server receiving the request and also give you a comparison you can make to know to add the other header. (Or do you need the other header if you have this?) But I can see reasons why maybe not to do that.

@wanderview
Copy link
Collaborator

Also, just for my curiosity, what is the use case for using the origin of the addWorklet URL as the data origin? The linked explainer pull request #158 says cross-origin worklet scripts are motivated by CDN usage. I don't understand a scenario where someone would want data partitioned according to CDN origin.

Is there another use case that isn't documented here?

@xyaoinum
Copy link
Collaborator

xyaoinum commented Jul 3, 2024

Also, just for my curiosity, what is the use case for using the origin of the addWorklet URL as the data origin?

The use case for allowing the origin of the script URL as the dataOrigin is to enable third-party scripts to load worklets directly, without requiring an iframe. This is a pre-existing use case (#131) and isn't related to CDN. That is, an ad script could call createWorklet('https://ad.com/script.js') in any context. Without this API, they'd need to create an https://ad.com iframe first, and inside the iframe, call addModule('script.js').

However, neither of the two approaches supports hosting the script on a CDN, as cross-origin script hasn't been allowed for addModule(). This PR removes this limitation, so that CDN script is supported, although it would definitely require running inside the ad iframe.

To align addModule() and createWorlet(), this PR also changes createWorklet()'s default dataOrigin to the calling context's origin. As a result, the network stack needs a separate flag to know cross-origin & shared-storage & script-origin case, whereas before, cross-origin & shared-storage could always imply script-origin.

@xyaoinum xyaoinum self-requested a review July 3, 2024 21:10
@xyaoinum
Copy link
Collaborator

xyaoinum commented Jul 3, 2024

(@pythagoraskitty Re-requested change per Ben's comment. To monkey-patch an internal property on request, you can refer to the Topics spec)

Copy link
Collaborator

@wanderview wanderview left a comment

Choose a reason for hiding this comment

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

LGTM with comment addressed.

spec.bs Show resolved Hide resolved
Add opaqueness check to fetch a single module script for sharedstorageworklet
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
Copy link
Collaborator

@wanderview wanderview left a comment

Choose a reason for hiding this comment

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

LGTM

@pythagoraskitty pythagoraskitty merged commit 4320b8d into main Jul 17, 2024
1 check passed
@pythagoraskitty pythagoraskitty deleted the cammie-branch4 branch July 17, 2024 18:23
github-actions bot added a commit that referenced this pull request Jul 17, 2024
SHA: 4320b8d
Reason: push, by pythagoraskitty

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 25, 2024
We align the default data origin for createWorklet with that of
addModule to be the invoking context's origin. We also hook up the
dataOrigin option in createWorklet's options dictionary, so that
the script origin can be manually specified to be used as the data
origin instead.

See WICG/shared-storage#158,
WICG/shared-storage#161, and
https://groups.google.com/a/chromium.org/g/blink-dev/c/YZ4XGewKVuk.

Bug:353738488
Change-Id: I3578e48f14c9fb1005211b94889ce01ef209162c
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 25, 2024
We align the default data origin for createWorklet with that of
addModule to be the invoking context's origin. We also hook up the
dataOrigin option in createWorklet's options dictionary, so that
the script origin can be manually specified to be used as the data
origin instead.

See WICG/shared-storage#158,
WICG/shared-storage#161, and
https://groups.google.com/a/chromium.org/g/blink-dev/c/YZ4XGewKVuk.

Bug: 353738488
Change-Id: I3578e48f14c9fb1005211b94889ce01ef209162c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5716903
Reviewed-by: Yao Xiao <yaoxia@chromium.org>
Commit-Queue: Cammie Smith Barnes <cammie@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1333189}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 25, 2024
We align the default data origin for createWorklet with that of
addModule to be the invoking context's origin. We also hook up the
dataOrigin option in createWorklet's options dictionary, so that
the script origin can be manually specified to be used as the data
origin instead.

See WICG/shared-storage#158,
WICG/shared-storage#161, and
https://groups.google.com/a/chromium.org/g/blink-dev/c/YZ4XGewKVuk.

Bug: 353738488
Change-Id: I3578e48f14c9fb1005211b94889ce01ef209162c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5716903
Reviewed-by: Yao Xiao <yaoxia@chromium.org>
Commit-Queue: Cammie Smith Barnes <cammie@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1333189}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jul 30, 2024
…fault data origin w/addModule's, a=testonly

Automatic update from web-platform-tests
Shared Storage: Align createWorklet's default data origin w/addModule's

We align the default data origin for createWorklet with that of
addModule to be the invoking context's origin. We also hook up the
dataOrigin option in createWorklet's options dictionary, so that
the script origin can be manually specified to be used as the data
origin instead.

See WICG/shared-storage#158,
WICG/shared-storage#161, and
https://groups.google.com/a/chromium.org/g/blink-dev/c/YZ4XGewKVuk.

Bug: 353738488
Change-Id: I3578e48f14c9fb1005211b94889ce01ef209162c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5716903
Reviewed-by: Yao Xiao <yaoxia@chromium.org>
Commit-Queue: Cammie Smith Barnes <cammie@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1333189}

--

wpt-commits: 71b107815a391a469b081cbb9242e1723ede50fb
wpt-pr: 47296
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Aug 5, 2024
…fault data origin w/addModule's, a=testonly

Automatic update from web-platform-tests
Shared Storage: Align createWorklet's default data origin w/addModule's

We align the default data origin for createWorklet with that of
addModule to be the invoking context's origin. We also hook up the
dataOrigin option in createWorklet's options dictionary, so that
the script origin can be manually specified to be used as the data
origin instead.

See WICG/shared-storage#158,
WICG/shared-storage#161, and
https://groups.google.com/a/chromium.org/g/blink-dev/c/YZ4XGewKVuk.

Bug: 353738488
Change-Id: I3578e48f14c9fb1005211b94889ce01ef209162c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5716903
Reviewed-by: Yao Xiao <yaoxia@chromium.org>
Commit-Queue: Cammie Smith Barnes <cammie@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1333189}

--

wpt-commits: 71b107815a391a469b081cbb9242e1723ede50fb
wpt-pr: 47296
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.

3 participants