From ad843b6e4a4404a4897b587711f9ce97a4504ed0 Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Mon, 11 Mar 2024 09:35:35 +0100 Subject: [PATCH] LibWeb: Don't crash when accessing property in detached Window object After removing an iframe from the DOM, its contentWindow will be detached from its browsing context, per spec. Because the contentWindow is still accessible, we cannot assume that Window objects always have an associated browsing context. This needs to be fixed in the spec, but let's add a sensible null check in the meantime. --- ...y-Get-after-detaching-from-browsing-context.txt | 3 +++ ...-Get-after-detaching-from-browsing-context.html | 11 +++++++++++ .../LibWeb/HTML/CrossOrigin/Reporting.cpp | 14 +++++++++++--- .../Libraries/LibWeb/HTML/CrossOrigin/Reporting.h | 2 +- Userland/Libraries/LibWeb/HTML/WindowProxy.cpp | 4 ++-- 5 files changed, 28 insertions(+), 6 deletions(-) create mode 100644 Tests/LibWeb/Text/expected/HTML/WindowProxy-Get-after-detaching-from-browsing-context.txt create mode 100644 Tests/LibWeb/Text/input/HTML/WindowProxy-Get-after-detaching-from-browsing-context.html diff --git a/Tests/LibWeb/Text/expected/HTML/WindowProxy-Get-after-detaching-from-browsing-context.txt b/Tests/LibWeb/Text/expected/HTML/WindowProxy-Get-after-detaching-from-browsing-context.txt new file mode 100644 index 00000000000..ff190b46c3d --- /dev/null +++ b/Tests/LibWeb/Text/expected/HTML/WindowProxy-Get-after-detaching-from-browsing-context.txt @@ -0,0 +1,3 @@ + +null +PASS (didn't crash) diff --git a/Tests/LibWeb/Text/input/HTML/WindowProxy-Get-after-detaching-from-browsing-context.html b/Tests/LibWeb/Text/input/HTML/WindowProxy-Get-after-detaching-from-browsing-context.html new file mode 100644 index 00000000000..08764cd4001 --- /dev/null +++ b/Tests/LibWeb/Text/input/HTML/WindowProxy-Get-after-detaching-from-browsing-context.html @@ -0,0 +1,11 @@ + + diff --git a/Userland/Libraries/LibWeb/HTML/CrossOrigin/Reporting.cpp b/Userland/Libraries/LibWeb/HTML/CrossOrigin/Reporting.cpp index e308a024d6f..6c810bf1910 100644 --- a/Userland/Libraries/LibWeb/HTML/CrossOrigin/Reporting.cpp +++ b/Userland/Libraries/LibWeb/HTML/CrossOrigin/Reporting.cpp @@ -12,8 +12,16 @@ namespace Web::HTML { // https://html.spec.whatwg.org/multipage/origin.html#coop-check-access-report -void check_if_access_between_two_browsing_contexts_should_be_reported(BrowsingContext const& accessor, BrowsingContext const& accessed, JS::PropertyKey const& property_key, EnvironmentSettingsObject const& environment) +void check_if_access_between_two_browsing_contexts_should_be_reported( + BrowsingContext const& accessor, + BrowsingContext const* accessed, + JS::PropertyKey const& property_key, + EnvironmentSettingsObject const& environment) { + // FIXME: Spec bug: https://github.com/whatwg/html/issues/10192 + if (!accessed) + return; + // 1. If P is not a cross-origin accessible window property name, then return. if (!is_cross_origin_accessible_window_property_name(property_key)) return; @@ -27,11 +35,11 @@ void check_if_access_between_two_browsing_contexts_should_be_reported(BrowsingCo auto accessor_accessed_relationship = AccessorAccessedRelationship::None; // 5. If accessed's top-level browsing context's opener browsing context is accessor or an ancestor of accessor, then set accessorAccessedRelationship to accessor is opener. - if (auto opener = accessed.top_level_browsing_context()->opener_browsing_context(); opener && (opener == &accessor || opener->is_ancestor_of(accessor))) + if (auto opener = accessed->top_level_browsing_context()->opener_browsing_context(); opener && (opener == &accessor || opener->is_ancestor_of(accessor))) accessor_accessed_relationship = AccessorAccessedRelationship::AccessorIsOpener; // 6. If accessor's top-level browsing context's opener browsing context is accessed or an ancestor of accessed, then set accessorAccessedRelationship to accessor is openee. - if (auto opener = accessor.top_level_browsing_context()->opener_browsing_context(); opener && (opener == &accessed || opener->is_ancestor_of(accessed))) + if (auto opener = accessor.top_level_browsing_context()->opener_browsing_context(); opener && (opener == accessed || opener->is_ancestor_of(*accessed))) accessor_accessed_relationship = AccessorAccessedRelationship::AccessorIsOpenee; // FIXME: 7. Queue violation reports for accesses, given accessorAccessedRelationship, accessor's top-level browsing context's active document's cross-origin opener policy, accessed's top-level browsing context's active document's cross-origin opener policy, accessor's active document's URL, accessed's active document's URL, accessor's top-level browsing context's initial URL, accessed's top-level browsing context's initial URL, accessor's active document's origin, accessed's active document's origin, accessor's top-level browsing context's opener origin at creation, accessed's top-level browsing context's opener origin at creation, accessor's top-level browsing context's active document's referrer, accessed's top-level browsing context's active document's referrer, P, and environment. diff --git a/Userland/Libraries/LibWeb/HTML/CrossOrigin/Reporting.h b/Userland/Libraries/LibWeb/HTML/CrossOrigin/Reporting.h index a7729f57513..a1eac67a72c 100644 --- a/Userland/Libraries/LibWeb/HTML/CrossOrigin/Reporting.h +++ b/Userland/Libraries/LibWeb/HTML/CrossOrigin/Reporting.h @@ -18,6 +18,6 @@ enum class AccessorAccessedRelationship { None, }; -void check_if_access_between_two_browsing_contexts_should_be_reported(BrowsingContext const& accessor, BrowsingContext const& accessed, JS::PropertyKey const&, EnvironmentSettingsObject const&); +void check_if_access_between_two_browsing_contexts_should_be_reported(BrowsingContext const& accessor, BrowsingContext const* accessed, JS::PropertyKey const&, EnvironmentSettingsObject const&); } diff --git a/Userland/Libraries/LibWeb/HTML/WindowProxy.cpp b/Userland/Libraries/LibWeb/HTML/WindowProxy.cpp index 61f99aec427..46e2e9454dc 100644 --- a/Userland/Libraries/LibWeb/HTML/WindowProxy.cpp +++ b/Userland/Libraries/LibWeb/HTML/WindowProxy.cpp @@ -158,7 +158,7 @@ JS::ThrowCompletionOr WindowProxy::internal_get(JS::PropertyKey const // 1. Let W be the value of the [[Window]] internal slot of this. // 2. Check if an access between two browsing contexts should be reported, given the current global object's browsing context, W's browsing context, P, and the current settings object. - check_if_access_between_two_browsing_contexts_should_be_reported(*verify_cast(current_global_object()).browsing_context(), *m_window->browsing_context(), property_key, current_settings_object()); + check_if_access_between_two_browsing_contexts_should_be_reported(*verify_cast(current_global_object()).browsing_context(), m_window->browsing_context(), property_key, current_settings_object()); // 3. If IsPlatformObjectSameOrigin(W) is true, then return ? OrdinaryGet(this, P, Receiver). // NOTE: this is passed rather than W as OrdinaryGet and CrossOriginGet will invoke the [[GetOwnProperty]] internal method. @@ -178,7 +178,7 @@ JS::ThrowCompletionOr WindowProxy::internal_set(JS::PropertyKey const& pro // 1. Let W be the value of the [[Window]] internal slot of this. // 2. Check if an access between two browsing contexts should be reported, given the current global object's browsing context, W's browsing context, P, and the current settings object. - check_if_access_between_two_browsing_contexts_should_be_reported(*verify_cast(current_global_object()).browsing_context(), *m_window->browsing_context(), property_key, current_settings_object()); + check_if_access_between_two_browsing_contexts_should_be_reported(*verify_cast(current_global_object()).browsing_context(), m_window->browsing_context(), property_key, current_settings_object()); // 3. If IsPlatformObjectSameOrigin(W) is true, then: if (is_platform_object_same_origin(*m_window)) {