From f0842a2253b466b0812e8ceed3f68b8f4253f03b Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Tue, 28 Jan 2020 13:08:11 -0800 Subject: [PATCH 1/2] fix: prevent print crash on bad deviceName --- docs/api/web-contents.md | 2 +- docs/api/webview-tag.md | 23 +++++++++++++++++-- shell/browser/api/atom_api_web_contents.cc | 26 ++++++++++++++++++++++ spec-main/api-web-contents-spec.ts | 16 ++++++++++++- 4 files changed, 63 insertions(+), 4 deletions(-) diff --git a/docs/api/web-contents.md b/docs/api/web-contents.md index 4f2f53cc7d5c0..b8fa9f121a12c 100644 --- a/docs/api/web-contents.md +++ b/docs/api/web-contents.md @@ -1259,7 +1259,7 @@ Returns [`PrinterInfo[]`](structures/printer-info.md) * `silent` Boolean (optional) - Don't ask user for print settings. Default is `false`. * `printBackground` Boolean (optional) - Prints the background color and image of the web page. Default is `false`. - * `deviceName` String (optional) - Set the printer device name to use. Default is `''`. + * `deviceName` String (optional) - Set the printer device name to use. Must be the system-defined name and not the 'friendly' name, e.g 'Brother_QL_820NWB' and not 'Brother QL-820NWB'. * `color` Boolean (optional) - Set whether the printed web page will be in color or grayscale. Default is `true`. * `margins` Object (optional) * `marginType` String (optional) - Can be `default`, `none`, `printableArea`, or `custom`. If `custom` is chosen, you will also need to specify `top`, `bottom`, `left`, and `right`. diff --git a/docs/api/webview-tag.md b/docs/api/webview-tag.md index c0fe3bc33183b..a5eb1ae2cdf28 100644 --- a/docs/api/webview-tag.md +++ b/docs/api/webview-tag.md @@ -545,9 +545,28 @@ Stops any `findInPage` request for the `webview` with the provided `action`. * `options` Object (optional) * `silent` Boolean (optional) - Don't ask user for print settings. Default is `false`. - * `printBackground` Boolean (optional) - Also prints the background color and image of + * `printBackground` Boolean (optional) - Prints the background color and image of the web page. Default is `false`. - * `deviceName` String (optional) - Set the printer device name to use. Default is `''`. + * `deviceName` String (optional) - Set the printer device name to use. Must be the system-defined name and not the 'friendly' name, e.g 'Brother_QL_820NWB' and not 'Brother QL-820NWB'. + * `color` Boolean (optional) - Set whether the printed web page will be in color or grayscale. Default is `true`. + * `margins` Object (optional) + * `marginType` String (optional) - Can be `default`, `none`, `printableArea`, or `custom`. If `custom` is chosen, you will also need to specify `top`, `bottom`, `left`, and `right`. + * `top` Number (optional) - The top margin of the printed web page, in pixels. + * `bottom` Number (optional) - The bottom margin of the printed web page, in pixels. + * `left` Number (optional) - The left margin of the printed web page, in pixels. + * `right` Number (optional) - The right margin of the printed web page, in pixels. + * `landscape` Boolean (optional) - Whether the web page should be printed in landscape mode. Default is `false`. + * `scaleFactor` Number (optional) - The scale factor of the web page. + * `pagesPerSheet` Number (optional) - The number of pages to print per page sheet. + * `collate` Boolean (optional) - Whether the web page should be collated. + * `copies` Number (optional) - The number of copies of the web page to print. + * `pageRanges` Record (optional) - The page range to print. Should have two keys: `from` and `to`. + * `duplexMode` String (optional) - Set the duplex mode of the printed web page. Can be `simplex`, `shortEdge`, or `longEdge`. + * `dpi` Object (optional) + * `horizontal` Number (optional) - The horizontal dpi. + * `vertical` Number (optional) - The vertical dpi. + * `header` String (optional) - String to be printed as page header. + * `footer` String (optional) - String to be printed as page footer. Returns `Promise` diff --git a/shell/browser/api/atom_api_web_contents.cc b/shell/browser/api/atom_api_web_contents.cc index 6cf1aa009b3f3..fcf31ab9c81ec 100644 --- a/shell/browser/api/atom_api_web_contents.cc +++ b/shell/browser/api/atom_api_web_contents.cc @@ -121,6 +121,10 @@ #if BUILDFLAG(ENABLE_PRINTING) #include "chrome/browser/printing/print_view_manager_basic.h" #include "components/printing/common/print_messages.h" + +#if defined(OS_WIN) +#include "printing/backend/win_helper.h" +#endif #endif #if BUILDFLAG(ENABLE_ELECTRON_EXTENSIONS) @@ -350,6 +354,23 @@ base::Optional GetCursorBlinkInterval() { return base::nullopt; } +#if BUILDFLAG(ENABLE_PRINTING) +// This will return false if no printer with the provided device_name can be +// found on the network. We need to check this because Chromium does not do +// sanity checking of device_name validity and so will crash on invalid names. +bool DeviceNameValid(const std::string& device_name) { +#if defined(OS_MACOSX) + base::ScopedCFTypeRef new_printer_id( + base::SysUTF8ToCFStringRef(device_name)); + return PMPrinterCreateFromPrinterID(new_printer_id.get()); +#elif defined(OS_WIN) + printing::ScopedPrinterHandle printer; + return printer.OpenPrinterWithName(base::UTF8ToUTF16(device_name).c_str()); +#endif + return true; +} +#endif + } // namespace WebContents::WebContents(v8::Isolate* isolate, @@ -1772,6 +1793,11 @@ void WebContents::Print(mate::Arguments* args) { // Printer device name as opened by the OS. base::string16 device_name; options.Get("deviceName", &device_name); + if (!device_name.empty() && + !DeviceNameValid(base::UTF16ToUTF8(device_name))) { + args->ThrowError("webContents.print(): Invalid deviceName provided."); + return; + } settings.SetStringKey(printing::kSettingDeviceName, device_name); int scale_factor = 100; diff --git a/spec-main/api-web-contents-spec.ts b/spec-main/api-web-contents-spec.ts index bd9f69f6ee195..171e06595e90a 100644 --- a/spec-main/api-web-contents-spec.ts +++ b/spec-main/api-web-contents-spec.ts @@ -104,20 +104,34 @@ describe('webContents module', () => { }) ifdescribe(features.isPrintingEnabled())('webContents.print()', () => { + let w: BrowserWindow + + beforeEach(() => { + w = new BrowserWindow({ show: false }) + }) + afterEach(closeAllWindows) + it('throws when invalid settings are passed', () => { - const w = new BrowserWindow({ show: false }) expect(() => { // @ts-ignore this line is intentionally incorrect w.webContents.print(true) }).to.throw('webContents.print(): Invalid print settings specified.') + }) + it('throws when an invalid callback is passed', () => { expect(() => { // @ts-ignore this line is intentionally incorrect w.webContents.print({}, true) }).to.throw('webContents.print(): Invalid optional callback provided.') }) + ifit(process.platform !== 'linux')('throws when an invalid deviceName is passed', () => { + expect(() => { + w.webContents.print({ deviceName: 'i-am-a-nonexistent-printer' }, () => {}) + }).to.throw('webContents.print(): Invalid deviceName provided.') + }) + it('does not crash', () => { const w = new BrowserWindow({ show: false }) expect(() => { From dffb7136509279bc5a7598c469c03a3563b4efd0 Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Wed, 29 Jan 2020 08:29:00 -0800 Subject: [PATCH 2/2] address review feedback --- docs/api/webview-tag.md | 23 ++-------------------- shell/browser/api/atom_api_web_contents.cc | 14 +++++++------ spec-main/api-web-contents-spec.ts | 1 - 3 files changed, 10 insertions(+), 28 deletions(-) diff --git a/docs/api/webview-tag.md b/docs/api/webview-tag.md index a5eb1ae2cdf28..c0fe3bc33183b 100644 --- a/docs/api/webview-tag.md +++ b/docs/api/webview-tag.md @@ -545,28 +545,9 @@ Stops any `findInPage` request for the `webview` with the provided `action`. * `options` Object (optional) * `silent` Boolean (optional) - Don't ask user for print settings. Default is `false`. - * `printBackground` Boolean (optional) - Prints the background color and image of + * `printBackground` Boolean (optional) - Also prints the background color and image of the web page. Default is `false`. - * `deviceName` String (optional) - Set the printer device name to use. Must be the system-defined name and not the 'friendly' name, e.g 'Brother_QL_820NWB' and not 'Brother QL-820NWB'. - * `color` Boolean (optional) - Set whether the printed web page will be in color or grayscale. Default is `true`. - * `margins` Object (optional) - * `marginType` String (optional) - Can be `default`, `none`, `printableArea`, or `custom`. If `custom` is chosen, you will also need to specify `top`, `bottom`, `left`, and `right`. - * `top` Number (optional) - The top margin of the printed web page, in pixels. - * `bottom` Number (optional) - The bottom margin of the printed web page, in pixels. - * `left` Number (optional) - The left margin of the printed web page, in pixels. - * `right` Number (optional) - The right margin of the printed web page, in pixels. - * `landscape` Boolean (optional) - Whether the web page should be printed in landscape mode. Default is `false`. - * `scaleFactor` Number (optional) - The scale factor of the web page. - * `pagesPerSheet` Number (optional) - The number of pages to print per page sheet. - * `collate` Boolean (optional) - Whether the web page should be collated. - * `copies` Number (optional) - The number of copies of the web page to print. - * `pageRanges` Record (optional) - The page range to print. Should have two keys: `from` and `to`. - * `duplexMode` String (optional) - Set the duplex mode of the printed web page. Can be `simplex`, `shortEdge`, or `longEdge`. - * `dpi` Object (optional) - * `horizontal` Number (optional) - The horizontal dpi. - * `vertical` Number (optional) - The vertical dpi. - * `header` String (optional) - String to be printed as page header. - * `footer` String (optional) - String to be printed as page footer. + * `deviceName` String (optional) - Set the printer device name to use. Default is `''`. Returns `Promise` diff --git a/shell/browser/api/atom_api_web_contents.cc b/shell/browser/api/atom_api_web_contents.cc index fcf31ab9c81ec..4d05d9cda70c7 100644 --- a/shell/browser/api/atom_api_web_contents.cc +++ b/shell/browser/api/atom_api_web_contents.cc @@ -358,14 +358,17 @@ base::Optional GetCursorBlinkInterval() { // This will return false if no printer with the provided device_name can be // found on the network. We need to check this because Chromium does not do // sanity checking of device_name validity and so will crash on invalid names. -bool DeviceNameValid(const std::string& device_name) { +bool IsDeviceNameValid(const base::string16& device_name) { #if defined(OS_MACOSX) base::ScopedCFTypeRef new_printer_id( - base::SysUTF8ToCFStringRef(device_name)); - return PMPrinterCreateFromPrinterID(new_printer_id.get()); + base::SysUTF16ToCFStringRef(device_name)); + PMPrinter new_printer = PMPrinterCreateFromPrinterID(new_printer_id.get()); + bool printer_exists = new_printer != nullptr; + PMRelease(new_printer); + return printer_exists; #elif defined(OS_WIN) printing::ScopedPrinterHandle printer; - return printer.OpenPrinterWithName(base::UTF8ToUTF16(device_name).c_str()); + return printer.OpenPrinterWithName(device_name.c_str()); #endif return true; } @@ -1793,8 +1796,7 @@ void WebContents::Print(mate::Arguments* args) { // Printer device name as opened by the OS. base::string16 device_name; options.Get("deviceName", &device_name); - if (!device_name.empty() && - !DeviceNameValid(base::UTF16ToUTF8(device_name))) { + if (!device_name.empty() && !IsDeviceNameValid(device_name)) { args->ThrowError("webContents.print(): Invalid deviceName provided."); return; } diff --git a/spec-main/api-web-contents-spec.ts b/spec-main/api-web-contents-spec.ts index 171e06595e90a..546c3731f53f4 100644 --- a/spec-main/api-web-contents-spec.ts +++ b/spec-main/api-web-contents-spec.ts @@ -133,7 +133,6 @@ describe('webContents module', () => { }) it('does not crash', () => { - const w = new BrowserWindow({ show: false }) expect(() => { w.webContents.print({ silent: true }) }).to.not.throw()