From c2673aa970b5256eff3b61d624ad0ff225260add Mon Sep 17 00:00:00 2001 From: Charles Kerr Date: Thu, 15 Mar 2018 13:56:46 +0900 Subject: [PATCH] Set appropriate defaults for webview options (#12271) * Persist defaults to webPreferences object to JS land can read the inferred values instead of just user defined values * Test inherited default propogation * Refactor to remove coupling from fetching values and defaults * Test description type * Fix up tests --- atom/browser/api/atom_api_web_contents.cc | 9 +++++ atom/browser/api/atom_api_web_contents.h | 1 + atom/browser/web_contents_preferences.cc | 38 +++++++++++++++++++ atom/browser/web_contents_preferences.h | 7 ++++ lib/browser/guest-view-manager.js | 4 +- lib/browser/guest-window-manager.js | 8 ++-- lib/renderer/security-warnings.js | 2 +- spec/api-browser-window-spec.js | 2 +- spec/chromium-spec.js | 26 +++++++++++-- .../pages/window-opener-no-web-view-tag.html | 15 ++++++++ .../pages/window-opener-web-view.html | 9 +++++ 11 files changed, 110 insertions(+), 11 deletions(-) create mode 100644 spec/fixtures/pages/window-opener-no-web-view-tag.html create mode 100644 spec/fixtures/pages/window-opener-web-view.html diff --git a/atom/browser/api/atom_api_web_contents.cc b/atom/browser/api/atom_api_web_contents.cc index 4010d3d5b98b..081a88485540 100644 --- a/atom/browser/api/atom_api_web_contents.cc +++ b/atom/browser/api/atom_api_web_contents.cc @@ -1825,6 +1825,14 @@ v8::Local WebContents::GetWebPreferences(v8::Isolate* isolate) { return mate::ConvertToV8(isolate, *web_preferences->web_preferences()); } +v8::Local WebContents::GetLastWebPreferences(v8::Isolate* isolate) { + WebContentsPreferences* web_preferences = + WebContentsPreferences::FromWebContents(web_contents()); + if (!web_preferences) + return v8::Null(isolate); + return mate::ConvertToV8(isolate, *web_preferences->last_web_preferences()); +} + v8::Local WebContents::GetOwnerBrowserWindow() { if (owner_window()) return BrowserWindow::From(isolate(), owner_window()); @@ -1976,6 +1984,7 @@ void WebContents::BuildPrototype(v8::Isolate* isolate, .SetMethod("_getZoomFactor", &WebContents::GetZoomFactor) .SetMethod("getType", &WebContents::GetType) .SetMethod("getWebPreferences", &WebContents::GetWebPreferences) + .SetMethod("getLastWebPreferences", &WebContents::GetLastWebPreferences) .SetMethod("getOwnerBrowserWindow", &WebContents::GetOwnerBrowserWindow) .SetMethod("hasServiceWorker", &WebContents::HasServiceWorker) .SetMethod("unregisterServiceWorker", diff --git a/atom/browser/api/atom_api_web_contents.h b/atom/browser/api/atom_api_web_contents.h index 2bb8c9757d21..d94902894c57 100644 --- a/atom/browser/api/atom_api_web_contents.h +++ b/atom/browser/api/atom_api_web_contents.h @@ -224,6 +224,7 @@ class WebContents : public mate::TrackableObject, // Returns the web preferences of current WebContents. v8::Local GetWebPreferences(v8::Isolate* isolate); + v8::Local GetLastWebPreferences(v8::Isolate* isolate); // Returns the owner window. v8::Local GetOwnerBrowserWindow(); diff --git a/atom/browser/web_contents_preferences.cc b/atom/browser/web_contents_preferences.cc index 416508742198..5cd7d9311612 100644 --- a/atom/browser/web_contents_preferences.cc +++ b/atom/browser/web_contents_preferences.cc @@ -48,6 +48,28 @@ WebContentsPreferences::WebContentsPreferences( web_contents->SetUserData(UserDataKey(), base::WrapUnique(this)); instances_.push_back(this); + + // Set WebPreferences defaults onto the JS object + SetDefaultBoolIfUndefined("plugins", false); + SetDefaultBoolIfUndefined(options::kExperimentalFeatures, false); + SetDefaultBoolIfUndefined(options::kExperimentalCanvasFeatures, false); + bool node = SetDefaultBoolIfUndefined(options::kNodeIntegration, true); + SetDefaultBoolIfUndefined(options::kNodeIntegrationInWorker, false); + SetDefaultBoolIfUndefined(options::kWebviewTag, node); + SetDefaultBoolIfUndefined("sandbox", false); + SetDefaultBoolIfUndefined("nativeWindowOpen", false); + SetDefaultBoolIfUndefined(options::kContextIsolation, false); + SetDefaultBoolIfUndefined("javascript", true); + SetDefaultBoolIfUndefined("images", true); + SetDefaultBoolIfUndefined("textAreasAreResizable", true); + SetDefaultBoolIfUndefined("webgl", true); + SetDefaultBoolIfUndefined("webSecurity", true); + SetDefaultBoolIfUndefined("allowRunningInsecureContent", false); + #if defined(OS_MACOSX) + SetDefaultBoolIfUndefined(options::kScrollBounce, false); + #endif + SetDefaultBoolIfUndefined("offscreen", false); + last_web_preferences_.MergeDictionary(&web_preferences_); } WebContentsPreferences::~WebContentsPreferences() { @@ -56,6 +78,16 @@ WebContentsPreferences::~WebContentsPreferences() { instances_.end()); } +bool WebContentsPreferences::SetDefaultBoolIfUndefined(const std::string key, + bool val) { + bool existing; + if (!web_preferences_.GetBoolean(key, &existing)) { + web_preferences_.SetBoolean(key, val); + return val; + } + return existing; +} + void WebContentsPreferences::Merge(const base::DictionaryValue& extend) { web_preferences_.MergeDictionary(&extend); } @@ -80,6 +112,12 @@ void WebContentsPreferences::AppendExtraCommandLineSwitches( base::DictionaryValue& web_preferences = self->web_preferences_; + // We are appending args to a webContents so let's save the current state + // of our preferences object so that during the lifetime of the WebContents + // we can fetch the options used to initally configure the WebContents + self->last_web_preferences_.Clear(); + self->last_web_preferences_.MergeDictionary(&web_preferences); + bool b; // Check if plugins are enabled. if (web_preferences.GetBoolean("plugins", &b) && b) diff --git a/atom/browser/web_contents_preferences.h b/atom/browser/web_contents_preferences.h index 5cc6bd97b23c..b7174525a4f7 100644 --- a/atom/browser/web_contents_preferences.h +++ b/atom/browser/web_contents_preferences.h @@ -57,10 +57,16 @@ class WebContentsPreferences // Returns the web preferences. base::DictionaryValue* web_preferences() { return &web_preferences_; } + base::DictionaryValue* last_web_preferences() { + return &last_web_preferences_; + } private: friend class content::WebContentsUserData; + // Set preference value to given bool if user did not provide value + bool SetDefaultBoolIfUndefined(const std::string key, bool val); + // Get preferences value as integer possibly coercing it from a string bool GetInteger(const std::string& attributeName, int* intValue); @@ -68,6 +74,7 @@ class WebContentsPreferences content::WebContents* web_contents_; base::DictionaryValue web_preferences_; + base::DictionaryValue last_web_preferences_; DISALLOW_COPY_AND_ASSIGN(WebContentsPreferences); }; diff --git a/lib/browser/guest-view-manager.js b/lib/browser/guest-view-manager.js index 0338b776f893..dd8e0ff1a923 100644 --- a/lib/browser/guest-view-manager.js +++ b/lib/browser/guest-view-manager.js @@ -156,7 +156,7 @@ const createGuest = function (embedder, params) { // Forward internal web contents event to embedder to handle // native window.open setup guest.on('-add-new-contents', (...args) => { - if (guest.getWebPreferences().nativeWindowOpen === true) { + if (guest.getLastWebPreferences().nativeWindowOpen === true) { const embedder = getEmbedder(guestInstanceId) if (embedder != null) { embedder.emit('-add-new-contents', ...args) @@ -164,7 +164,7 @@ const createGuest = function (embedder, params) { } }) guest.on('-web-contents-created', (...args) => { - if (guest.getWebPreferences().nativeWindowOpen === true) { + if (guest.getLastWebPreferences().nativeWindowOpen === true) { const embedder = getEmbedder(guestInstanceId) if (embedder != null) { embedder.emit('-web-contents-created', ...args) diff --git a/lib/browser/guest-window-manager.js b/lib/browser/guest-window-manager.js index 0b1155b86cee..ec77a7abdff1 100644 --- a/lib/browser/guest-window-manager.js +++ b/lib/browser/guest-window-manager.js @@ -59,12 +59,12 @@ const mergeBrowserWindowOptions = function (embedder, options) { mergeOptions(options, parentOptions) } else { // Or only inherit webPreferences if it is a webview. - mergeOptions(options.webPreferences, embedder.getWebPreferences()) + mergeOptions(options.webPreferences, embedder.getLastWebPreferences()) } // Inherit certain option values from parent window for (const [name, value] of inheritedWebPreferences) { - if (embedder.getWebPreferences()[name] === value) { + if (embedder.getLastWebPreferences()[name] === value) { options.webPreferences[name] = value } } @@ -177,8 +177,8 @@ const getGuestWindow = function (guestContents) { // The W3C does not have anything on this, but from my understanding of the // security model of |window.opener|, this should be fine. const canAccessWindow = function (sender, target) { - return (target.getWebPreferences().openerId === sender.id) || - (sender.getWebPreferences().nodeIntegration === true) || + return (target.getLastWebPreferences().openerId === sender.id) || + (sender.getLastWebPreferences().nodeIntegration === true) || isSameOrigin(sender.getURL(), target.getURL()) } diff --git a/lib/renderer/security-warnings.js b/lib/renderer/security-warnings.js index 3969819669eb..3c264412f2e7 100644 --- a/lib/renderer/security-warnings.js +++ b/lib/renderer/security-warnings.js @@ -84,7 +84,7 @@ const getWebPreferences = function () { } const { remote } = require('electron') - webPreferences = remote.getCurrentWebContents().getWebPreferences() + webPreferences = remote.getCurrentWebContents().getLastWebPreferences() return webPreferences } catch (error) { return null diff --git a/spec/api-browser-window-spec.js b/spec/api-browser-window-spec.js index 4249b4c4cf17..2f5e1ed13d2d 100644 --- a/spec/api-browser-window-spec.js +++ b/spec/api-browser-window-spec.js @@ -2999,7 +2999,7 @@ describe('BrowserWindow module', () => { }) it('enables context isolation on child windows', (done) => { app.once('browser-window-created', (event, window) => { - assert.equal(window.webContents.getWebPreferences().contextIsolation, true) + assert.equal(window.webContents.getLastWebPreferences().contextIsolation, true) done() }) w.loadURL(`file://${fixtures}/pages/window-open.html`) diff --git a/spec/chromium-spec.js b/spec/chromium-spec.js index 3d1d3f673592..3f78c5c533c4 100644 --- a/spec/chromium-spec.js +++ b/spec/chromium-spec.js @@ -268,6 +268,26 @@ describe('chromium feature', () => { b = window.open(windowUrl, '', 'nodeIntegration=no,show=no') }) + it('disables webviewTag when node integration is disabled on the parent window', (done) => { + let b + listener = (event) => { + assert.equal(event.data.isWebViewUndefined, true) + b.close() + done() + } + window.addEventListener('message', listener) + + const windowUrl = require('url').format({ + pathname: `${fixtures}/pages/window-opener-no-web-view-tag.html`, + protocol: 'file', + query: { + p: `${fixtures}/pages/window-opener-web-view.html` + }, + slashes: true + }) + b = window.open(windowUrl, '', 'nodeIntegration=no,show=no') + }) + it('disables node integration when it is disabled on the parent window for chrome devtools URLs', (done) => { let b app.once('web-contents-created', (event, contents) => { @@ -287,7 +307,7 @@ describe('chromium feature', () => { app.once('web-contents-created', (event, contents) => { contents.once('did-finish-load', () => { app.once('browser-window-created', (event, window) => { - const preferences = window.webContents.getWebPreferences() + const preferences = window.webContents.getLastWebPreferences() assert.equal(preferences.javascript, false) window.destroy() b.close() @@ -509,7 +529,7 @@ describe('chromium feature', () => { done() } window.addEventListener('message', listener) - w = window.open(url, '', 'show=no') + w = window.open(url, '', 'show=no,nodeIntegration=no') }) it('works when origin matches', (done) => { @@ -518,7 +538,7 @@ describe('chromium feature', () => { done() } window.addEventListener('message', listener) - w = window.open(`file://${fixtures}/pages/window-opener-location.html`, '', 'show=no') + w = window.open(`file://${fixtures}/pages/window-opener-location.html`, '', 'show=no,nodeIntegration=no') }) it('works when origin does not match opener but has node integration', (done) => { diff --git a/spec/fixtures/pages/window-opener-no-web-view-tag.html b/spec/fixtures/pages/window-opener-no-web-view-tag.html new file mode 100644 index 000000000000..2596fc18e6e3 --- /dev/null +++ b/spec/fixtures/pages/window-opener-no-web-view-tag.html @@ -0,0 +1,15 @@ + + + + + diff --git a/spec/fixtures/pages/window-opener-web-view.html b/spec/fixtures/pages/window-opener-web-view.html new file mode 100644 index 000000000000..48db5fc7964a --- /dev/null +++ b/spec/fixtures/pages/window-opener-web-view.html @@ -0,0 +1,9 @@ + + + + +