Partition Blob URL revocation by Storage Key #201

recvfrom · 2024-10-25T20:16:32Z

Part of the changes discussed in #153 (comment)

This updates URL.revokeObjectURL to not allow revoking a Blob URL except from contexts with the same Storage Key as the one in which the Blob URL was created. A corresponding PR will update the Fetch spec to incorporate similar Storage Key checks into Blob URL fetches.

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.

For normative changes, the following tasks have been completed:

Modified Web platform tests (link to pull request)

https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/FileAPI/BlobURL/cross-partition.tentative.https.html

Implementation commitment:

WebKit - Already implemented partitioning by top-level origin, considering partitioning by top-level site - Blob URL store partitioning #153 (comment)
Chromium (https://bugs.chromium.org/p/chromium/issues/detail?id=40057646)
Gecko - Already implemented - Blob URL store partitioning #153 (comment)

Preview | Diff

miketaylr · 2024-11-13T14:12:00Z

Mind taking a look @mkruisselbrink?

index.bs

mkruisselbrink · 2024-11-20T21:03:03Z

index.bs

+1. If |entry| is failure, return.
+1. Let |blobStorageKey| be the result of [=obtaining a storage key for non-storage purposes=] with |entry|'s [=environment settings object=].
+1. Let |currentStorageKey| be the result of [=obtaining a storage key for non-storage purposes=] with the [=current settings object=].
+1. If |blobStorageKey| is not [=storage key/equal=] to |currentStorageKey|, return.
 1. [=Remove an entry from the Blob URL Store=] for |url|.

 Note: This means that rather than throwing some kind of error, attempting to revoke a URL that isn't registered will silently fail.


Might make sense to mention that it's not just URLs that aren't registered that would silently fail, something like (although not sure that's the best way to phrase it):

Suggested change

Note: This means that rather than throwing some kind of error, attempting to revoke a URL that isn't registered will silently fail.

Note: This means that rather than throwing some kind of error, attempting to revoke a URL that isn't registered, or is registered from a different storage key, will silently fail.

Good catch, I updated this (although made it a bit more verbose). WDYT?

miketaylr · 2024-11-22T14:58:44Z

I guess we need to update spec-prod (or wait for the update to be propagated?):

  $ bikeshed update
    Bikeshed now requires Python 3.9; you are on 3.8.10.
        If you're seeing this message in your CI run, you are
        likely specifying an old OS; try `ubuntu-latest`.
        If you're seeing this on the command line, see the docs
        for instructions:
        https://speced.github.io/bikeshed/#installing
  Command `bikeshed update` failed with exit code: 1.

w3c/spec-prod#193

mkruisselbrink · 2024-11-22T17:00:35Z

I guess we need to update spec-prod (or wait for the update to be propagated?):

  $ bikeshed update
    Bikeshed now requires Python 3.9; you are on 3.8.10.
        If you're seeing this message in your CI run, you are
        likely specifying an old OS; try `ubuntu-latest`.
        If you're seeing this on the command line, see the docs
        for instructions:
        https://speced.github.io/bikeshed/#installing
  Command `bikeshed update` failed with exit code: 1.

w3c/spec-prod#193

I think #204 will fix that.

recvfrom · 2024-11-25T05:21:32Z

Per the feedback here [1], I've updated this PR to also change how the blob URL entry's object member is exposed so that the partitioning checks can be fully contained in the FileAPI. WDYT?

[1] whatwg/fetch#1783 (comment)

annevk · 2024-11-25T15:12:10Z

index.bs

 [=map/Keys=] in the [=blob URL store=] (also known as <dfn lt="blob URL|object URL" export>blob URLs</dfn>)
 are [=valid URL strings=] that when [=URL parser|parsed=]
 result in a [=/URL=] with a [=url/scheme=] equal to "`blob`",
 an [=empty host=], and a [=url/path=] consisting of one element itself also a [=valid URL string=].

+A <dfn export>blob URL entry</dfn> is used to store an object of type {{Blob}} or {{MediaSource}}.


Looking at this. I would continue to give a blob URL entry an object field, but just no longer export it. And then explain in a note that specifications have to use "obtain a blob object" to get hold of the object.

annevk · 2024-11-25T15:12:43Z

index.bs

+To <dfn export id=blob-url-obtain-object>obtain a blob object</dfn> given a [=blob URL entry=] |blobUrlEntry|, an [=environment=] |environment|, and an optional boolean |isNavigation| (default false):
+
+1. Let |isAuthorized| be true.
+1. If |isNavigation| is false, let |isAuthorized| be the result of [=checking for same-partition blob URL usage=] with |blobUrlEntry| and |environment|.


If you manipulate an initialized variable, you need to use "set ... to". See the Infra standard for more complete guidance.

annevk · 2024-11-25T15:14:22Z

index.bs

@@ -1632,12 +1658,13 @@ The <dfn method for=URL id="dfn-revokeObjectURL">revokeObjectURL(|url|)</dfn> st

 1. Let |url record| be the result of [=URL parser|parsing=] |url|.


Maybe change this to urlRecord while here? Having these as separate words is confusing with the url variable.

annevk · 2024-11-25T15:14:33Z

index.bs

-1. Let |settings| be the [=current settings object=].
-1. If |origin| is not [=same origin=] with |settings|'s [=environment settings object/origin=], return.
+1. Let |entry| be |url record|'s [=blob URL entry=].
+1. If |entry| is null, return.


then return*

annevk · 2024-11-25T15:15:11Z

index.bs

+1. Let |entry| be |url record|'s [=blob URL entry=].
+1. If |entry| is null, return.
+1. Let |isAuthorized| be the result of [=checking for same-partition blob URL usage=] with |entry| and the [=current settings object=].
+1. If |isAuthorized| is false, return.


then return*

recvfrom added 2 commits October 25, 2024 15:46

Partition Blob URL revocation

b988256

Fix duplicate list numbering

bb24aca

This was referenced Oct 25, 2024

Partition Blob URL fetches by Storage Key whatwg/fetch#1783

Open

Note that Blob URL usage is restricted due to storage partitioning mdn/content#36542

Open

recvfrom marked this pull request as ready for review October 28, 2024 17:11

recvfrom mentioned this pull request Oct 28, 2024

Blob URL store partitioning #153

Open

mkruisselbrink reviewed Nov 20, 2024

View reviewed changes

Address review feedback

c97d7f9

Move partitioning checks into FileAPI

fd3d0f8

annevk reviewed Nov 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Partition Blob URL revocation by Storage Key #201

Partition Blob URL revocation by Storage Key #201

recvfrom commented Oct 25, 2024 •

edited by pr-preview bot

Loading

miketaylr commented Nov 13, 2024

mkruisselbrink Nov 20, 2024

recvfrom Nov 22, 2024

miketaylr commented Nov 22, 2024

mkruisselbrink commented Nov 22, 2024

recvfrom commented Nov 25, 2024

annevk Nov 25, 2024

annevk Nov 25, 2024

annevk Nov 25, 2024

annevk Nov 25, 2024

annevk Nov 25, 2024

	Note: This means that rather than throwing some kind of error, attempting to revoke a URL that isn't registered will silently fail.
	Note: This means that rather than throwing some kind of error, attempting to revoke a URL that isn't registered, or is registered from a different storage key, will silently fail.

		@@ -1632,12 +1658,13 @@ The <dfn method for=URL id="dfn-revokeObjectURL">revokeObjectURL(\|url\|)</dfn> st

		1. Let \|url record\| be the result of [=URL parser\|parsing=] \|url\|.

Partition Blob URL revocation by Storage Key #201

Are you sure you want to change the base?

Partition Blob URL revocation by Storage Key #201

Conversation

recvfrom commented Oct 25, 2024 • edited by pr-preview bot Loading

miketaylr commented Nov 13, 2024

mkruisselbrink Nov 20, 2024

Choose a reason for hiding this comment

recvfrom Nov 22, 2024

Choose a reason for hiding this comment

miketaylr commented Nov 22, 2024

mkruisselbrink commented Nov 22, 2024

recvfrom commented Nov 25, 2024

annevk Nov 25, 2024

Choose a reason for hiding this comment

annevk Nov 25, 2024

Choose a reason for hiding this comment

annevk Nov 25, 2024

Choose a reason for hiding this comment

annevk Nov 25, 2024

Choose a reason for hiding this comment

annevk Nov 25, 2024

Choose a reason for hiding this comment

recvfrom commented Oct 25, 2024 •

edited by pr-preview bot

Loading