-
Notifications
You must be signed in to change notification settings - Fork 332
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
Partition Blob URL fetches by Storage Key #1783
base: main
Are you sure you want to change the base?
Conversation
@annevk would you be able to look at this PR as well? Or could you recommend another reviewer? Thank you! |
@domenic would you mind taking a look at this when you have a chance? |
fetch.bs
Outdated
prevented from succeeding if the <a spec=storage>storage key</a> of the | ||
<a>environment settings object</a> making the request is different than the | ||
<a spec=storage>storage key</a> of the <a>environment settings object</a> corresponding to where | ||
the Blob URL was created. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like it would work better after line 5028, instead of near this algorithm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking this would make sense in the Infrastructure section like how the HTTP cache partitioning
section and determine the HTTP cache partition
algorithm are, but I've moved the algorithm under the Scheme fetch
section now (and removed the note since it seems kinda out of place now. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-editor LGTM
I'm a little worried about this approach as it means obtaining and revoking are maintained in independent specifications, but need to be kept synchronized to maintain the privacy and security boundary. I think a more ideal setup would be that File API keeps the |
It looks like there aren't too many references to the blob URL entry's object... https://dontcallmedom.github.io/webdex/o.html#object%40%40blob%20URL%20entry%40dfn |
Nice, the HTML find is legit and would also need to be updated I suspect. It would benefit from having the obtaining defined in File API so there's less need for duplication. Reporting API seems bogus: w3c/reporting#273. Web Authentication API seems similarly bogus: w3c/webauthn#2212. |
Thanks for the feedback, I've updated my FileAPI PR [1] to expose an obtain algorithm as recommended here, and I've updated this PR to use it. One question I thought more about was what to do when the request doesn't have an associated environment... It seems like for Blob URLs we should allow navigations to bypass the partitioning checks, but otherwise we should just fail (this seems in line with the Chromium implementation at least). WDYT? |
A draft PR for the HTML spec change is at: whatwg/html#10792 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for reworking this so quickly!
return a <a>network error</a>. | ||
|
||
<li><p>Let <var>blob</var> be the result of <a>obtaining a blob object</a> given | ||
<var>blobURLEntry</var>, <var>requestEnvironment</var>, and <var>isNavigation</var>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So when isNavigation is true we don't even care about environment, right? I would merge those parameters into one.
So you have
given an environment or the string "
navigation
" environment
on the other side or some such forcing callers to confront that question. And it should probably be top-level navigation?
<li><p>Let <var>blob</var> be the result of <a>obtaining a blob object</a> given | ||
<var>blobURLEntry</var>, <var>requestEnvironment</var>, and <var>isNavigation</var>. | ||
|
||
<li><p>If <var>blob</var> is failure, or <var>blob</var> is not a {{Blob}} object, then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You only need the second conditional to catch both conditions.
<a for=request>determining the environment</a> given <var>request</var>. | ||
|
||
<li><p>Let <var>isNavigation</var> be true if <var>request</var>'s <a for=request>mode</a> is | ||
not "<code>navigate</code>"; otherwise, false. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps instead of mode we should be checking destination for "document" which is only true for top-level navigations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for calling this out, I had overlooked that we didn't want to exempt all navigations from the partitioning checks and instead only want to exempt top-level navigations. I confirmed that Safari and Firefox both enforce partitioning checks on subframe navigations, so we'll implement it this way in Chromium as well, write a new WPT for this, and I'll update the spec here to look for destination equaling documenting as you suggested.
Partition Blob URL fetches by Storage Key
This change updates the spec to partition Blob URL fetches by Storage Key. This change is part of broader changes discussed in w3c/FileAPI#153 (comment). Specifically, we will also:
I considered incorporating the Storage Key checks into the "resolve a blob URL" algorithm instead, but it seemed that this would require an environment settings object to be available as part of https://url.spec.whatwg.org/#url-parsing, and I'm not sure whether that is the case / a change we want.
(See WHATWG Working Mode: Changes for more details.)
Preview | Diff