Add draft of spec #6

mreichhoff · 2022-12-08T20:38:46Z

This ensures that CORS + use-credentials is used in top-level subresource requests, and leaves open the possibility of an rSA call indicating embeddee opt-in for frames (with the latter point to be clarified in a later effort based on the outcome of per-frame discussions elsewhere).

This ensures that CORS or an rSA call indicates embedee opt-in.

johannhof · 2022-12-09T10:20:05Z

Ah, didn't notice this when looking at #5 😆. Similar comments probably apply :)

mreichhoff · 2022-12-09T17:34:51Z

yup, I addressed your comments in this one 😄

I think we're pretty clearly moving to "require explicit rSA invocation" at this point, so this is the better PR. It's just a matter of clarifying the subresource request portion.

mreichhoff · 2022-12-09T20:12:03Z

got one round of review from @johannhof already...wondering if @domfarolino would want to take a look as well (we discussed parts of this today out-of-band). There's probably a good bit of work still needed, but the more we can knock out now, the better.

domfarolino

Took a quick pass, can finish the rest next week but here's a few comments for now.

Btw, feel free to use https://github.com/domfarolino/specfmt if you feel so daring to format the spec incrementally and be one of that project's first guinea pigs :)

requestStorageAccessForOrigin.bs

domfarolino · 2022-12-10T03:41:16Z

requestStorageAccessForOrigin.bs

+1. Run these steps [=in parallel=]:
+    1. Let |hasAccess| be [=a new promise=].
+    1. [=Determine the top-level storage access policy=] with |descriptor|, |doc| and |hasAccess|.
+    1. [=Queue a global task=] on the [=permission task source=] given |global| to resolve or reject |p| based on the result of |hasAccess|.


The result of |hasAccess| is just a promise right? Do you mean to wait for the result of |hasAccess| and resolve/reject |p| with the same value? It seems like "Determine the top-level storage access policy" already queues its own task on the global it receives, to resolve/reject the promise it is given though (which is |hasAccess| IIUC), so I'm a bit confused what is going on here. It's also a little late so maybe I'm just not squinting hard enough :)

ah, the former (again modeled on the storage access API spec, perhaps poorly).

I modified the other function to just resolve or reject the promise it is given, which might help?

"the other function" being "Determine the top-level storage access policy", right? So there's a few things to note here:

We can't resolve/reject promises "in parallel"; that has to be done in a global task

We can't create new promise objects "in parallel"... maybe we could bend the rules on this one for hasAccess since it isn't web-exposed, but that's probably not a good precedent to set

If I understand correctly, the "Determine the top-level storage access policy" algorithm used to queue its own task on a global to resolve/reject the promise it is given accordingly. So can we just pass |p| into that algorithm, instead of create a new promise (|hasAccess|) and use its resolution to inform |p|'s resolution? Does that make sense?

yes, that makes more sense. I do note that this seems to re-add global use in these parallel steps (which I thought was one of the main thread items to watch out for), unless my mechanism for resolving or rejecting the promise is wrong. Alternatively, maybe that's fine since it's queueing a task vs mutating global or relying on its state being unchanged.

domfarolino · 2022-12-10T03:42:33Z

requestStorageAccessForOrigin.bs

+1. If |existing state| is [=permission/granted=], return true.
+1. Return false.
+
+To <dfn type="abstract-op">determine the top-level storage access policy</dfn> for {{TopLevelStorageAccessPermissionDescriptor}} |descriptor|, with {{Document}} |doc| and {{Promise}} |p|, run these steps:


It would be good to know where this is supposed to be running? On the "main thread", or "in parallel"? It seems to kind of interact with main thread state, but earlier it seems to be called in parallel.

ah, to a large extent I was mimicking the equivalent in the storage access API spec (or well, how it is expected to be). I think it does need to run in parallel, since presumably we'd not wait on a permission request (which can prompt the user) to return the promise in the actual rSAFor function call.

I can go through and try to identify which of this state lives on the main thread and react accordingly.

(removed the global variable and the top-level site in favor of plumbing it in; attempted to follow the docs, though I suspect user activation will also have to be plumbed in)

requestStorageAccessForOrigin.bs

domfarolino · 2022-12-19T04:56:50Z

requestStorageAccessForOrigin.bs

+1. Run these steps [=in parallel=]:
+    1. Let |hasAccess| be [=a new promise=].
+    1. [=Determine the top-level storage access policy=] with |descriptor|, |doc| and |hasAccess|.
+    1. [=Queue a global task=] on the [=permission task source=] given |global| to resolve or reject |p| based on the result of |hasAccess|.


"the other function" being "Determine the top-level storage access policy", right? So there's a few things to note here:

We can't resolve/reject promises "in parallel"; that has to be done in a global task

We can't create new promise objects "in parallel"... maybe we could bend the rules on this one for hasAccess since it isn't web-exposed, but that's probably not a good precedent to set

If I understand correctly, the "Determine the top-level storage access policy" algorithm used to queue its own task on a global to resolve/reject the promise it is given accordingly. So can we just pass |p| into that algorithm, instead of create a new promise (|hasAccess|) and use its resolution to inform |p|'s resolution? Does that make sense?

requestStorageAccessForOrigin.bs

domfarolino · 2022-12-19T05:03:20Z

requestStorageAccessForOrigin.bs

+
+<!-- this long chain pulled from fetch, but may be unusual or unsafe -->
+1. Let |settings| be |request|'s [=request/client=]'s [=relevant global object=]'s [=associated document=]'s [=relevant settings object=].
+1. Let |embedded origin| be |request|'s [=request/url=].


This variable is of type "URL" but the variable name is "origin". That's a little confusing, but I guess it matches the fact that TopLevelStorageAccessPermissionDescriptor's requestedOrigin is a USVString (not an origin), but I think we still need to pull the serialized origin out of the URL right? Or else the dictionary's requestedOrigin member will actually be the entire URL string?

hm...I went with USVString for the descriptor, but perhaps it would be better to use origin? I wasn't sure what the correct IDL type would be. Regardless, yes, I at least switched to the request's url's origin here.

johannhof

This seems good to merge from my side :)

requestStorageAccessForOrigin.bs

domfarolino

A few nits mostly around touching main-thread state from an in-parallel context. This is probably good enough for a draft provided we follow-up on some of the comments accordingly. It'll probably be another week until I can take another close look, so feel free to not block on me if you need to publish. I can take another close look after the holidays or maybe in between them etc.

domfarolino · 2022-12-23T15:22:55Z

requestStorageAccessForOrigin.bs

+    1. [=Queue a global task=] on the [=permission task source=] given |global| to [=reject=] |p| with a "{{NotAllowedError}}" {{DOMException}}.
+    1. Return.
+1. Assert that |doc|'s [=node navigable=] is a [=traversable navigable=].
+1. If this algorithm was invoked when |doc|'s {{Window}} object did not have [=transient activation=]:


This is a bit handwavy as we're doing some action-at-a-distance, looking backwards to state that lived on a different thread. I think it's best to pull the state of the user activation out early on the main thread and plumb it through

domfarolino · 2022-12-23T15:24:04Z

requestStorageAccessForOrigin.bs

+    1. [=Queue a global task=] on the [=permission task source=] given |global| to [=resolve=] |p|.
+    1. Return.
+1. If |implicitly denied| is true:
+    1. [=reject=] |p| with a "{{NotAllowedError}}" {{DOMException}}.


We have to queue a task to do this on the main thread right? Also nit: "Reject", since it is the beginning of a sentence

domfarolino · 2022-12-23T15:24:34Z

requestStorageAccessForOrigin.bs

+1. If |permissionState| is [=permission/granted=]:
+    1. [=Queue a global task=] on the [=permission task source=] given |global| to [=resolve=] |p|.
+    1. Return.
+1. If |doc|'s {{Window}} object has [=transient activation=], [=consume user activation=] with it.


User activation querying and consumption should be done on the main thread. How does this work implementation-wise?

domfarolino · 2022-12-23T15:27:58Z

requestStorageAccessForOrigin.bs

+To <dfn type="abstract-op">determine if a request has top-level storage access</dfn> with [=request=] |request|, run these steps:
+
+1. Let |settings| be |request|'s [=request/client=]'s [=relevant global object=]'s [=relevant settings object=].
+1. Let |embedded origin| be |request|'s [=request/url=]'s [=/origin=].


Should "/origin" be "url/origin" here?

mreichhoff · 2023-01-03T21:09:47Z

will address remaining domfarolino@ comments in a follow-up PR (cleaning this up as it was from before moving the repo). Thanks!

Add spec requiring rSA or CORS

ffad349

This ensures that CORS or an rSA call indicates embedee opt-in.

Address johannhof feedback

507fc7e

mreichhoff added 2 commits December 9, 2022 14:09

Clarify subresource requests

65268b3

Remove permissions PR note

674e975

mreichhoff marked this pull request as ready for review December 9, 2022 19:20

domfarolino reviewed Dec 10, 2022

View reviewed changes

mreichhoff added 2 commits December 12, 2022 15:55

Address first round of domfarolino feedback

019c190

Remove some global state in parallel steps

ecb3368

domfarolino reviewed Dec 19, 2022

View reviewed changes

Address additional domfarolino@ feedback

c5ef26d

johannhof approved these changes Dec 20, 2022

View reviewed changes

requestStorageAccessForOrigin.bs Outdated Show resolved Hide resolved

requestStorageAccessForOrigin.bs Outdated Show resolved Hide resolved

mreichhoff changed the title ~~Add spec requiring rSA or CORS~~ Add initial spec Dec 20, 2022

mreichhoff changed the title ~~Add initial spec~~ Add draft of spec Dec 20, 2022

Clarify TODOs

66b5dbb

domfarolino reviewed Dec 23, 2022

View reviewed changes

mreichhoff merged commit 5996517 into main Jan 3, 2023

mreichhoff mentioned this pull request Jan 4, 2023

Plumb activation state into parallel steps #9

Merged

mreichhoff deleted the spec-explicit-rsa branch January 19, 2023 21:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add draft of spec #6

Add draft of spec #6

mreichhoff commented Dec 8, 2022 •

edited

Loading

johannhof commented Dec 9, 2022

mreichhoff commented Dec 9, 2022

mreichhoff commented Dec 9, 2022

domfarolino left a comment

domfarolino Dec 10, 2022

mreichhoff Dec 12, 2022

domfarolino Dec 19, 2022

mreichhoff Dec 19, 2022

domfarolino Dec 10, 2022

mreichhoff Dec 12, 2022

mreichhoff Dec 14, 2022

domfarolino Dec 19, 2022

domfarolino Dec 19, 2022

mreichhoff Dec 19, 2022

johannhof left a comment

domfarolino left a comment

domfarolino Dec 23, 2022

domfarolino Dec 23, 2022

domfarolino Dec 23, 2022

domfarolino Dec 23, 2022

mreichhoff commented Jan 3, 2023

Add draft of spec #6

Add draft of spec #6

Conversation

mreichhoff commented Dec 8, 2022 • edited Loading

johannhof commented Dec 9, 2022

mreichhoff commented Dec 9, 2022

mreichhoff commented Dec 9, 2022

domfarolino left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johannhof left a comment

Choose a reason for hiding this comment

domfarolino left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mreichhoff commented Jan 3, 2023

mreichhoff commented Dec 8, 2022 •

edited

Loading