From b859e0090801eaa6e4934feba6a55ca4140a8101 Mon Sep 17 00:00:00 2001 From: Srujan Gaddam Date: Thu, 6 Jul 2023 15:50:48 +0000 Subject: [PATCH] Reland "[dart:html] Throw exception if Window.open opens null window" and "[dart:html] Move NullWindowException to implementation" This is a revert of bd3e6fa1a3a502bee7ecf404a24580f3aa44c9f4 and 2b250992f940288695511f10cc27531193de75c0. This adds some small code change to avoid a null-assertion being emitted that would lead to a browser SecurityError. CoreLibraryReviewExempt: Reland. Change-Id: Iab52bb728b14fd0b2378b8923b0e1ea8ea930b12 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/311922 Reviewed-by: Sigmund Cherem Commit-Queue: Srujan Gaddam --- sdk/lib/html/dart2js/html_dart2js.dart | 33 ++++++++++++++----- tests/lib/html/window_test.dart | 12 +++++++ tests/lib_2/html/window_test.dart | 12 +++++++ tools/dom/src/dart2js_Conversions.dart | 2 +- tools/dom/src/dart2js_DOMImplementation.dart | 16 +++++++-- .../html/impl/impl_Window.darttemplate | 15 ++++++--- tools/dom/web_library_bindings.dart | 1 + 7 files changed, 75 insertions(+), 16 deletions(-) diff --git a/sdk/lib/html/dart2js/html_dart2js.dart b/sdk/lib/html/dart2js/html_dart2js.dart index cc78dfcdd12..f2921afe876 100644 --- a/sdk/lib/html/dart2js/html_dart2js.dart +++ b/sdk/lib/html/dart2js/html_dart2js.dart @@ -32160,11 +32160,9 @@ class Window extends EventTarget * from MDN. */ WindowBase open(String url, String name, [String? options]) { - if (options == null) { - return _DOMWindowCrossFrame._createSafe(_open2(url, name)); - } else { - return _DOMWindowCrossFrame._createSafe(_open3(url, name, options)); - } + final win = + options == null ? _open2(url, name) : _open3(url, name, options); + return _DOMWindowCrossFrame._createSafe(win); } // API level getter and setter for Location. @@ -33777,6 +33775,13 @@ class Window extends EventTarget ? JS('num', '#.scrollY', this).round() : document.documentElement!.scrollTop; } + +class NullWindowException implements Exception { + @override + String toString() { + return 'Attempting to use a null window opened in Window.open.'; + } +} // Copyright (c) 2012, the Dart project authors. Please see the AUTHORS file // for details. All rights reserved. Use of this source code is governed by a // BSD-style license that can be found in the LICENSE file. @@ -40017,7 +40022,7 @@ EventTarget? _convertNativeToDart_EventTarget(e) { return e; } -EventTarget? _convertDartToNative_EventTarget(e) { +_convertDartToNative_EventTarget(e) { if (e is _DOMWindowCrossFrame) { return e._window; } else { @@ -40116,7 +40121,19 @@ class _DOMWindowCrossFrame implements WindowBase { // Private window. Note, this is a window in another frame, so it // cannot be typed as "Window" as its prototype is not patched // properly. Its fields and methods can only be accessed via JavaScript. - final _window; + final Object? __window; + + Object get _window { + // Note that we use a local variable here to avoid a null-assertion. This is + // needed as [__window] can be a cross-origin iframe. dart2js emits + // null-assertions using a field access like .toString, and since accessing + // properties on a cross-origin iframe (with the exception of postMessage) + // is a SecurityError, the null-assertion could trigger a SecurityError. + // This avoids the need to type [__window] as dynamic. + final w = __window; + if (w == null) throw new NullWindowException(); + return w; + } // Fields. HistoryBase get history => @@ -40153,7 +40170,7 @@ class _DOMWindowCrossFrame implements WindowBase { } // Implementation support. - _DOMWindowCrossFrame(this._window); + _DOMWindowCrossFrame(this.__window); static WindowBase _createSafe(w) { if (identical(w, window)) { diff --git a/tests/lib/html/window_test.dart b/tests/lib/html/window_test.dart index ab79e666605..9e6ff6a1bb3 100644 --- a/tests/lib/html/window_test.dart +++ b/tests/lib/html/window_test.dart @@ -11,4 +11,16 @@ main() { expect(window.scrollX, 0); expect(window.scrollY, 0); }); + test('open', () { + final valid = window.open('', 'blank'); + valid.closed; + // A blank page with no access to the original window (noopener) should + // result in null. + final invalid = window.open('', 'invalid', 'noopener=true'); + try { + // Should result in an exception since the underlying window is null. + invalid.closed; + fail('Expected invalid.closed to throw.'); + } on NullWindowException {} + }); } diff --git a/tests/lib_2/html/window_test.dart b/tests/lib_2/html/window_test.dart index 2aa67fcdc39..154cba6fe8a 100644 --- a/tests/lib_2/html/window_test.dart +++ b/tests/lib_2/html/window_test.dart @@ -13,4 +13,16 @@ main() { expect(window.scrollX, 0); expect(window.scrollY, 0); }); + test('open', () { + final valid = window.open('', 'blank'); + valid.closed; + // A blank page with no access to the original window (noopener) should + // result in null. + final invalid = window.open('', 'invalid', 'noopener=true'); + try { + // Should result in an exception since the underlying window is null. + invalid.closed; + fail('Expected invalid.closed to throw.'); + } on NullWindowException {} + }); } diff --git a/tools/dom/src/dart2js_Conversions.dart b/tools/dom/src/dart2js_Conversions.dart index cb943b4ae0b..9641cb5c0c4 100644 --- a/tools/dom/src/dart2js_Conversions.dart +++ b/tools/dom/src/dart2js_Conversions.dart @@ -33,7 +33,7 @@ EventTarget? _convertNativeToDart_EventTarget(e) { return e; } -EventTarget? _convertDartToNative_EventTarget(e) { +_convertDartToNative_EventTarget(e) { if (e is _DOMWindowCrossFrame) { return e._window; } else { diff --git a/tools/dom/src/dart2js_DOMImplementation.dart b/tools/dom/src/dart2js_DOMImplementation.dart index 8016f6ec665..a4cf1aca8c2 100644 --- a/tools/dom/src/dart2js_DOMImplementation.dart +++ b/tools/dom/src/dart2js_DOMImplementation.dart @@ -9,7 +9,19 @@ class _DOMWindowCrossFrame implements WindowBase { // Private window. Note, this is a window in another frame, so it // cannot be typed as "Window" as its prototype is not patched // properly. Its fields and methods can only be accessed via JavaScript. - final _window; + final Object? __window; + + Object get _window { + // Note that we use a local variable here to avoid a null-assertion. This is + // needed as [__window] can be a cross-origin iframe. dart2js emits + // null-assertions using a field access like .toString, and since accessing + // properties on a cross-origin iframe (with the exception of postMessage) + // is a SecurityError, the null-assertion could trigger a SecurityError. + // This avoids the need to type [__window] as dynamic. + final w = __window; + if (w == null) throw new NullWindowException(); + return w; + } // Fields. HistoryBase get history => @@ -46,7 +58,7 @@ class _DOMWindowCrossFrame implements WindowBase { } // Implementation support. - _DOMWindowCrossFrame(this._window); + _DOMWindowCrossFrame(this.__window); static WindowBase _createSafe(w) { if (identical(w, window)) { diff --git a/tools/dom/templates/html/impl/impl_Window.darttemplate b/tools/dom/templates/html/impl/impl_Window.darttemplate index ddc1d04ee5f..0df2cd773c5 100644 --- a/tools/dom/templates/html/impl/impl_Window.darttemplate +++ b/tools/dom/templates/html/impl/impl_Window.darttemplate @@ -56,11 +56,9 @@ $(CLASS_MODIFIERS)class $CLASSNAME$EXTENDS$IMPLEMENTS { * from MDN. */ WindowBase open(String url, String name, [String$NULLABLE options]) { - if (options == null) { - return _DOMWindowCrossFrame._createSafe(_open2(url, name)); - } else { - return _DOMWindowCrossFrame._createSafe(_open3(url, name, options)); - } + final win = + options == null ? _open2(url, name) : _open3(url, name, options); + return _DOMWindowCrossFrame._createSafe(win); } // API level getter and setter for Location. @@ -254,3 +252,10 @@ $!MEMBERS JS('num', '#.scrollY', this).round() : document.documentElement$NULLASSERT.scrollTop; } + +class NullWindowException implements Exception { + @override + String toString() { + return 'Attempting to use a null window opened in Window.open.'; + } +} diff --git a/tools/dom/web_library_bindings.dart b/tools/dom/web_library_bindings.dart index a733d1a5503..0e10f62d5e1 100644 --- a/tools/dom/web_library_bindings.dart +++ b/tools/dom/web_library_bindings.dart @@ -9581,6 +9581,7 @@ final Map> dartTypeToNativeTypes = { 'NoncedElement': {'NoncedElement'}, 'Notification': {'Notification'}, 'NotificationEvent': {'NotificationEvent'}, + 'NullWindowException': {'NullWindowException'}, 'Number': {'SVGNumber'}, 'NumberInputElement': {'NumberInputElement'}, 'NumberList': {'SVGNumberList'},