From e090f2b0704bd22374455cf331f69beabf119e57 Mon Sep 17 00:00:00 2001 From: ldm0 Date: Fri, 11 Sep 2026 14:16:45 +0800 Subject: [PATCH] fix(file-api): authorize blob URL revocation by storage key Record the URL creator's storage key and browser partition independently of its backing Blob, and check authorization atomically with URL removal. Use the Worker's environment key and share opaque-origin nonce allocation between Windows and Workers so unrelated opaque origins cannot collide. Validated with cargo fmt, workspace clippy, and nextest (17990 passed, 13 skipped). Cross-global revoke WPT passes 3/3; all 874 contract checks pass, with no regressions across the 68-case WPT suite. --- moli-file-api/src/blob_store.rs | 104 ++++++++++++++++-- moli-renderer-v8/src/blob.rs | 46 ++++++-- .../context_bootstrap/url_form/callbacks.rs | 35 +++--- .../src/runtime/browser_context_runtime.rs | 13 +-- .../src/runtime/page_vm/tests/fetch_xhr.rs | 58 ++++++++++ .../src/script_vm/tests/browser_api/misc.rs | 32 ++++++ 6 files changed, 252 insertions(+), 36 deletions(-) diff --git a/moli-file-api/src/blob_store.rs b/moli-file-api/src/blob_store.rs index 4c86d6325..9e29b9d7a 100644 --- a/moli-file-api/src/blob_store.rs +++ b/moli-file-api/src/blob_store.rs @@ -40,11 +40,12 @@ impl Default for BlobEntries { } } -#[derive(Clone, Copy, Debug)] -struct ObjectUrlState { +#[derive(Debug)] +struct ObjectUrlState { owner_id: Option, lifetime_id: Option, blob_id: BlobId, + access_key: Option, } /// Renderer-neutral Blob and object URL backing store. @@ -53,13 +54,13 @@ struct ObjectUrlState { /// counts. The embedding layer owns JS wrappers and calls the retain/release /// hooks from its finalizers. #[derive(Debug)] -pub struct BlobStore { +pub struct BlobStore { blobs: Mutex>, next_blob_id: AtomicU64, - object_urls: Mutex>>, + object_urls: Mutex>>, } -impl Default for BlobStore { +impl Default for BlobStore { fn default() -> Self { Self { blobs: Mutex::default(), @@ -69,7 +70,7 @@ impl Default for BlobStore { } } -impl BlobStore +impl BlobStore where OwnerId: Copy + Eq + Hash, PartitionId: Eq, @@ -176,6 +177,25 @@ where lifetime_id: Option, blob_id: BlobId, origin: &str, + ) -> Option { + self.create_object_url_with_lifetime_and_access_key( + owner_id, + lifetime_id, + blob_id, + origin, + None, + ) + } + + /// Create an object URL with the creator environment's access key. This + /// key belongs to the URL, independently of the Blob's owner or partition. + pub fn create_object_url_with_lifetime_and_access_key( + &self, + owner_id: Option, + lifetime_id: Option, + blob_id: BlobId, + origin: &str, + access_key: Option, ) -> Option { self.retain_blob_object_url_ref(blob_id)?; let mut object_urls = self.object_urls.lock(); @@ -191,6 +211,7 @@ where owner_id, lifetime_id, blob_id, + access_key, }, ); Some(object_url) @@ -198,8 +219,29 @@ where /// Revoke an object URL and release its Blob object-URL reference. pub fn revoke_object_url(&self, url: &str) -> bool { - let Some(state) = self.object_urls.lock().remove(url) else { - return false; + self.revoke_object_url_if(url, |_| true) + } + + /// Revoke an object URL only when its creator's key matches the caller's. + /// Missing keys are unauthorized; checking and removal are atomic. + pub fn revoke_object_url_with_access_key(&self, url: &str, access_key: &AccessKey) -> bool + where + AccessKey: Eq, + { + self.revoke_object_url_if(url, |state| state.access_key.as_ref() == Some(access_key)) + } + + fn revoke_object_url_if( + &self, + url: &str, + is_authorized: impl FnOnce(&ObjectUrlState) -> bool, + ) -> bool { + let state = { + let mut object_urls = self.object_urls.lock(); + if !object_urls.get(url).is_some_and(is_authorized) { + return false; + } + object_urls.remove(url).expect("authorized entry is locked") }; self.release_blob_object_url_ref(state.blob_id); true @@ -352,6 +394,52 @@ fn random_uuid() -> String { mod tests { use super::*; + #[test] + fn object_url_access_keys_preserve_unauthorized_entries_and_release_authorized_entries() { + let store = BlobStore::::default(); + let blob = store.create_blob(Some(1), Some(10), b"payload".to_vec(), String::new()); + let first_key = "first URL creator".to_owned(); + let second_key = "second URL creator".to_owned(); + let first = store + .create_object_url_with_lifetime_and_access_key( + Some(2), + Some(101), + blob, + "null", + Some(first_key.clone()), + ) + .unwrap(); + let second = store + .create_object_url_with_lifetime_and_access_key( + Some(3), + Some(101), + blob, + "null", + Some(second_key.clone()), + ) + .unwrap(); + let unkeyed = store.create_object_url(Some(1), blob, "null").unwrap(); + store.release_blob_wrapper_ref(blob); + + assert!(!store.revoke_object_url_with_access_key(&first, &second_key)); + assert!(!store.revoke_object_url_with_access_key(&second, &first_key)); + assert!(!store.revoke_object_url_with_access_key(&unkeyed, &first_key)); + assert!(!store.revoke_object_url_with_access_key(&format!("{first}#fragment"), &first_key)); + assert_eq!(store.object_url_body_and_type(&first).unwrap().0, "payload"); + assert!(store.revoke_object_url_with_access_key(&first, &first_key)); + assert!(!store.revoke_object_url_with_access_key(&first, &first_key)); + assert!(store.object_url_body_and_type(&first).is_none()); + assert_eq!( + store.object_url_body_and_type(&second).unwrap().0, + "payload" + ); + assert!(store.revoke_object_url(&unkeyed)); + assert!(store.blob_bytes(blob).is_some()); + assert_eq!(store.cleanup_object_url_lifetime(3, 101), 1); + assert!(store.blob_bytes(blob).is_none()); + assert!(!store.revoke_object_url_with_access_key(&second, &second_key)); + } + #[test] fn captured_object_url_entries_outlive_revocation_and_creator_cleanup() { for cleanup in ["revoke", "owner", "lifetime"] { diff --git a/moli-renderer-v8/src/blob.rs b/moli-renderer-v8/src/blob.rs index 6f5256552..dd36025ce 100644 --- a/moli-renderer-v8/src/blob.rs +++ b/moli-renderer-v8/src/blob.rs @@ -44,10 +44,18 @@ struct BlobPrototypeDeclaration { r#type: (), } -static BLOB_STORE: OnceLock> = - OnceLock::new(); +#[derive(Debug, PartialEq, Eq)] +struct ObjectUrlAccessKey { + partition: RendererStoragePartitionIdentity, + storage_key: moli_storage_key::MoliStorageKey, +} -fn blob_store() -> &'static BlobStore { +type RendererBlobStore = + BlobStore; + +static BLOB_STORE: OnceLock = OnceLock::new(); + +fn blob_store() -> &'static RendererBlobStore { BLOB_STORE.get_or_init(BlobStore::default) } @@ -390,17 +398,41 @@ pub(super) fn blob_mime_type_from_object<'s>( pub(super) fn create_object_url_for_object<'s>( scope: &mut v8::PinScope<'s, '_>, object: v8::Local<'s, v8::Object>, - origin: &str, + storage_key: moli_storage_key::MoliStorageKey, ) -> Option { let blob_id = blob_id_from_object(scope, object)?; + let partition = current_blob_storage_partition_identity(scope)?; let owner_id = current_resource_owner_id(scope); let lifetime_id = native_bridge::current_runtime_observable_context_token(scope) .map(native_bridge::RuntimeObservableContextToken::as_u64); - blob_store().create_object_url_with_lifetime(owner_id, lifetime_id, blob_id, origin) + let origin = storage_key.origin().to_owned(); + blob_store().create_object_url_with_lifetime_and_access_key( + owner_id, + lifetime_id, + blob_id, + &origin, + Some(ObjectUrlAccessKey { + partition, + storage_key, + }), + ) } -pub(super) fn revoke_object_url(url: &str) { - blob_store().revoke_object_url(url); +pub(super) fn revoke_object_url( + scope: &mut v8::PinScope<'_, '_>, + url: &str, + storage_key: moli_storage_key::MoliStorageKey, +) { + let Some(partition) = current_blob_storage_partition_identity(scope) else { + return; + }; + blob_store().revoke_object_url_with_access_key( + url, + &ObjectUrlAccessKey { + partition, + storage_key, + }, + ); } pub(super) fn object_url_body_and_type(url: &str) -> Option<(String, String)> { diff --git a/moli-renderer-v8/src/context_bootstrap/url_form/callbacks.rs b/moli-renderer-v8/src/context_bootstrap/url_form/callbacks.rs index e89762c75..69e5f8ad7 100644 --- a/moli-renderer-v8/src/context_bootstrap/url_form/callbacks.rs +++ b/moli-renderer-v8/src/context_bootstrap/url_form/callbacks.rs @@ -166,16 +166,7 @@ pub(super) fn url_create_object_url_callback<'s>( args: v8::FunctionCallbackArguments<'s>, mut rv: v8::ReturnValue<'_, v8::Value>, ) { - let origin = if let Some(host_ptr) = context_host_ptr_from_global_bridge(scope) { - let active_child_handle = current_child_context_handle(scope) - .or_else(|| crate::native_bridge::active_child_window_handle(scope)); - unsafe { &mut *host_ptr } - .active_storage_context(scope, active_child_handle) - .origin() - .to_owned() - } else if let Some(worker_url) = current_worker_script_url(scope) { - moli_url::origin_ascii_serialization(&worker_url) - } else { + let Some(storage_key) = current_object_url_storage_key(scope) else { rv.set(v8::undefined(scope).into()); return; }; @@ -186,7 +177,7 @@ pub(super) fn url_create_object_url_callback<'s>( ); return; }; - let Some(url) = blob::create_object_url_for_object(scope, object, &origin) else { + let Some(url) = blob::create_object_url_for_object(scope, object, storage_key) else { throw_type_error( scope, "Failed to execute 'createObjectURL' on 'URL': parameter 1 is not of type 'Blob'.", @@ -200,6 +191,22 @@ pub(super) fn url_create_object_url_callback<'s>( } } +fn current_object_url_storage_key( + scope: &mut v8::PinScope<'_, '_>, +) -> Option { + if let Some(host_ptr) = context_host_ptr_from_global_bridge(scope) { + let active_child_handle = current_child_context_handle(scope) + .or_else(|| crate::native_bridge::active_child_window_handle(scope)); + return Some( + unsafe { &mut *host_ptr } + .active_storage_context(scope, active_child_handle) + .storage_key() + .clone(), + ); + } + crate::worker::worker_storage_key(scope) +} + fn current_child_context_handle( scope: &mut v8::PinScope<'_, '_>, ) -> Option { @@ -230,8 +237,10 @@ pub(super) fn url_revoke_object_url_callback<'s>( let Some(parsed) = webidl::parse_args::(scope, &args) else { return; }; - if parsed.url.starts_with("blob:") { - blob::revoke_object_url(&parsed.url); + if parsed.url.starts_with("blob:") + && let Some(storage_key) = current_object_url_storage_key(scope) + { + blob::revoke_object_url(scope, &parsed.url, storage_key); } rv.set_undefined(); } diff --git a/moli-renderer-v8/src/runtime/browser_context_runtime.rs b/moli-renderer-v8/src/runtime/browser_context_runtime.rs index 9a6d230f7..448a50366 100644 --- a/moli-renderer-v8/src/runtime/browser_context_runtime.rs +++ b/moli-renderer-v8/src/runtime/browser_context_runtime.rs @@ -177,7 +177,6 @@ struct RendererBrowserContextRuntimeInner { shared_worker_runtime: shared_workers::LazySharedWorkerRuntime, service_worker_runtime: service_worker_runtime::LazyServiceWorkerRuntime, storage_partition_identity: RendererStoragePartitionIdentity, - next_web_storage_opaque_context_nonce: AtomicU64, next_child_document_loader_id: AtomicU64, next_detached_parser_script_fetch_id: AtomicU64, next_dedicated_worker_instance_id: AtomicU64, @@ -535,7 +534,6 @@ impl RendererBrowserContextRuntime { shared_worker_runtime, service_worker_runtime, storage_partition_identity, - next_web_storage_opaque_context_nonce: AtomicU64::default(), next_child_document_loader_id: AtomicU64::default(), next_detached_parser_script_fetch_id: AtomicU64::default(), next_dedicated_worker_instance_id: AtomicU64::default(), @@ -703,12 +701,11 @@ impl RendererBrowserContextRuntime { pub(crate) fn next_web_storage_opaque_context_nonce( &self, ) -> moli_storage_key::OpaqueOriginNonce { - moli_storage_key::OpaqueOriginNonce::new( - self.inner - .next_web_storage_opaque_context_nonce - .fetch_add(1, Ordering::Relaxed) - .saturating_add(1), - ) + // Windows and Workers compare storage keys in the same partition. + // Their opaque origins must draw from the same nonce namespace. + self.inner + .broadcast_channel_registry + .next_opaque_context_nonce() } pub(crate) fn allocate_child_document_loader_id(&self) -> String { diff --git a/moli-renderer-v8/src/runtime/page_vm/tests/fetch_xhr.rs b/moli-renderer-v8/src/runtime/page_vm/tests/fetch_xhr.rs index 7c726d269..931c81204 100644 --- a/moli-renderer-v8/src/runtime/page_vm/tests/fetch_xhr.rs +++ b/moli-renderer-v8/src/runtime/page_vm/tests/fetch_xhr.rs @@ -4422,6 +4422,64 @@ async fn request_init_exceptions_preserve_identity_without_fetching_or_consuming }).await; } +#[tokio::test] +async fn blob_url_revocation_uses_window_and_worker_creator_storage_keys() { + run_page_vm_async_test(async move { + for document_url in ["https://example.com/", "data:text/html,opaque-parent"] { + let mut page_vm = test_page_vm_with_document_url(Url::parse(document_url).unwrap()); + let local_executor = page_vm.local_executor.clone(); + let result = local_executor.run(async move { + page_vm.vm_mut().eval(r#" + globalThis.__revocationResult = 'pending'; + (async () => { + const check = (value, message) => { if (!value) throw new Error(message); }; + const source = `onmessage = async event => { + const {action, url} = event.data; + if (action === 'create') postMessage(URL.createObjectURL(new Blob(['payload']))); + if (action === 'revoke') { URL.revokeObjectURL(url); postMessage('done'); } + if (action === 'read') { + try { postMessage(await (await fetch(url)).text()); } + catch (error) { postMessage(error.name); } + } + };`; + const sourceUrl = URL.createObjectURL(new Blob([source])); + const workers = [new Worker(sourceUrl), new Worker('data:text/javascript,' + encodeURIComponent(source))]; + const rpc = (worker, action, url) => new Promise((resolve, reject) => { + worker.onmessage = event => resolve(event.data); + worker.onerror = event => reject(new Error(event.message)); + worker.postMessage({action, url}); + }); + try { + for (let i = 0; i < workers.length; i++) { + const worker = workers[i]; + const url = URL.createObjectURL(new Blob(['payload'])); + await rpc(worker, 'revoke', url); + let body; + try { body = await (await fetch(url)).text(); } + catch (error) { body = error.name; } + check(body === (i === 0 ? 'TypeError' : 'payload'), 'worker revocation authority'); + URL.revokeObjectURL(url); + const childUrl = await rpc(worker, 'create'); + URL.revokeObjectURL(childUrl); + check(await rpc(worker, 'read', childUrl) === (i === 0 ? 'TypeError' : 'payload'), 'parent revocation authority'); + await rpc(worker, 'revoke', childUrl); + check(await rpc(worker, 'read', childUrl) === 'TypeError', 'worker can revoke its own opaque URL'); + } + } finally { + for (const worker of workers) worker.terminate(); + URL.revokeObjectURL(sourceUrl); + } + return 'ok'; + })().then(value => { globalThis.__revocationResult = value; }, error => { globalThis.__revocationResult = String(error); }); + "#)?; + drive_websocket_until_done(&mut page_vm, "String(globalThis.__revocationResult !== 'pending')", "revocation checks should finish").await?; + page_vm.vm_mut().eval("globalThis.__revocationResult") + }).await.expect("blob revocation checks should run on owner lane"); + assert_eq!(result, "ok", "document_url={document_url}"); + } + }).await; +} + #[tokio::test] async fn blob_url_entries_survive_request_cloning_and_xhr_open_in_window_and_worker() { run_page_vm_async_test(async move { diff --git a/moli-renderer-v8/src/script_vm/tests/browser_api/misc.rs b/moli-renderer-v8/src/script_vm/tests/browser_api/misc.rs index b13f68d98..4c9d488a1 100644 --- a/moli-renderer-v8/src/script_vm/tests/browser_api/misc.rs +++ b/moli-renderer-v8/src/script_vm/tests/browser_api/misc.rs @@ -3535,6 +3535,38 @@ fn blob_slice_uses_receiver_realm_after_method_realm_is_detached() { ); } +#[test] +fn blob_url_revocation_respects_browser_partitions_and_allows_same_origin_realms() { + for document_url in ["https://blob-url-revocation.test/", "data:text/html,opaque"] { + let markup = ""; + let mut creator = new_parsed_test_vm(document_url, markup); + let mut other_partition = new_parsed_test_vm(document_url, markup); + let url = creator + .eval("URL.createObjectURL(new Blob(['payload']))") + .expect("create object URL"); + let url_literal = serde_json::to_string(&url).unwrap(); + other_partition + .eval(&format!("URL.revokeObjectURL({url_literal})")) + .expect("foreign partition revocation is a silent no-op"); + assert_eq!( + crate::blob::object_url_body_and_type(&url).unwrap().0, + "payload" + ); + // The initial about:blank child inherits even its opaque parent's key. + creator + .eval(&format!( + r#"(() => {{ + const frame = document.createElement('iframe'); + document.body.appendChild(frame); + const revoke = frame.contentWindow.URL.revokeObjectURL; + revoke.call(null, {url_literal}); + }})()"# + )) + .expect("same-origin child can revoke parent URL"); + assert!(crate::blob::object_url_body_and_type(&url).is_none()); + } +} + #[test] fn removing_child_frame_revokes_only_its_blob_object_urls() { let mut vm = new_parsed_test_vm(