From aeffc52fd7ccff668fbb5e6b18fa176a0cf5ebed Mon Sep 17 00:00:00 2001 From: Calixte Denizet Date: Fri, 14 Aug 2026 13:59:54 +0000 Subject: [PATCH] Bug 2058085 - Pick about:pdf files in the parent process. r=mossop,fluent-reviewers,flod Move the file picker and PDF validation to AboutPDFParent so content never supplies a local path. Leave dropped PDFs to native drop handling. Differential Revision: https://phabricator.services.mozilla.com/D318472 --- toolkit/actors/AboutPDFChild.sys.mjs | 37 +-- toolkit/actors/AboutPDFParent.sys.mjs | 80 ++++-- .../components/aboutpdf/content/aboutPDF.html | 6 - .../components/aboutpdf/content/aboutPDF.mjs | 44 ++-- .../tests/browser/browser_aboutPDF.js | 71 ++++- .../tests/browser/browser_aboutPDF_actor.js | 245 +++++++++++++----- .../components/aboutpdf/tests/browser/head.js | 9 +- .../locales/en-US/toolkit/about/aboutPDF.ftl | 2 + .../modules/RemotePageAccessManager.sys.mjs | 2 +- 9 files changed, 349 insertions(+), 147 deletions(-) diff --git a/toolkit/actors/AboutPDFChild.sys.mjs b/toolkit/actors/AboutPDFChild.sys.mjs index c087d27f1b47..3aa6c3bcf6d4 100644 --- a/toolkit/actors/AboutPDFChild.sys.mjs +++ b/toolkit/actors/AboutPDFChild.sys.mjs @@ -4,15 +4,13 @@ import { RemotePageChild } from "resource://gre/actors/RemotePageChild.sys.mjs"; -const PDF_HEADER = "%PDF-"; - export class AboutPDFChild extends RemotePageChild { actorCreated() { super.actorCreated(); this.exportFunctions([ "RPMCanSetDefaultPDFHandler", - "RPMOpenPDFFile", + "RPMPickPDFFile", "RPMSetDefaultPDFHandler", ]); } @@ -21,8 +19,14 @@ export class AboutPDFChild extends RemotePageChild { return this.wrapPromise(this.sendQuery("AboutPDF:CanSetDefaultPDFHandler")); } - RPMOpenPDFFile(file) { - return this.wrapPromise(this.#openPDFFile(file)); + // User activation prevents the page from opening OS UI without a user + // gesture. The parent still treats content messages as untrusted. + RPMPickPDFFile() { + if (!this.contentWindow.navigator.userActivation.isActive) { + throw new Error("User activation is required"); + } + + return this.wrapPromise(this.sendQuery("AboutPDF:PickFile")); } RPMSetDefaultPDFHandler() { @@ -32,27 +36,4 @@ export class AboutPDFChild extends RemotePageChild { return this.wrapPromise(this.sendQuery("AboutPDF:SetDefaultPDFHandler")); } - - async #openPDFFile(file) { - if ( - !file || - ChromeUtils.getClassName(file) !== "File" || - !file.name?.toLowerCase().endsWith(".pdf") || - !file.mozFullPath || - !(await this.#looksLikePDF(file)) - ) { - return false; - } - - await this.sendQuery("AboutPDF:OpenFile", { - fileURL: PathUtils.toFileURI(file.mozFullPath), - }); - return true; - } - - // Cheap pre-filter so the page can flip to its error state instantly without - // a round-trip to the parent. The parent re-validates before navigating. - async #looksLikePDF(file) { - return (await file.slice(0, PDF_HEADER.length).text()) === PDF_HEADER; - } } diff --git a/toolkit/actors/AboutPDFParent.sys.mjs b/toolkit/actors/AboutPDFParent.sys.mjs index d3f69e5cf9ae..05eb0736b490 100644 --- a/toolkit/actors/AboutPDFParent.sys.mjs +++ b/toolkit/actors/AboutPDFParent.sys.mjs @@ -12,13 +12,23 @@ try { )); } catch {} +const lazy = {}; + +ChromeUtils.defineLazyGetter( + lazy, + "l10n", + () => new Localization(["toolkit/about/aboutPDF.ftl"]) +); + export class AboutPDFParent extends JSWindowActorParent { + #filePickerOpenPromise = null; + receiveMessage(message) { switch (message.name) { case "AboutPDF:CanSetDefaultPDFHandler": return this.#canSetDefaultPDFHandler(); - case "AboutPDF:OpenFile": - return this.#openFile(message.data?.fileURL); + case "AboutPDF:PickFile": + return this.#pickFile(); case "AboutPDF:SetDefaultPDFHandler": return this.#setDefaultPDFHandler(); } @@ -43,31 +53,54 @@ export class AboutPDFParent extends JSWindowActorParent { } } - async #openFile(fileURL) { - if (typeof fileURL !== "string") { - throw new Error("Expected a file URL"); + // Never accept a path from content: this load uses the system principal. + // Native drop handling separately verifies dropped links against the drag + // session. + // Returns "opened", "canceled", or "invalid". + async #pickFile() { + if (this.#filePickerOpenPromise) { + return "canceled"; } - let uri = Services.io.newURI(fileURL); - if (!uri.schemeIs("file")) { - throw new Error("Expected a file URL"); + let browsingContext = this.browsingContext.top; + let fp = Cc["@mozilla.org/filepicker;1"].createInstance(Ci.nsIFilePicker); + fp.init( + this.browsingContext, + await lazy.l10n.formatValue("about-pdf-file-picker-title"), + Ci.nsIFilePicker.modeOpen + ); + fp.appendFilters(Ci.nsIFilePicker.filterPDF); + fp.appendFilters(Ci.nsIFilePicker.filterAll); + + let result; + const { promise, resolve } = Promise.withResolvers(); + this.#filePickerOpenPromise = promise; + try { + fp.open(resolve); + result = await this.#filePickerOpenPromise; + } finally { + this.#filePickerOpenPromise = null; } - let nsFile = uri.QueryInterface(Ci.nsIFileURL).file; - if (!nsFile.leafName.toLowerCase().endsWith(".pdf")) { - throw new Error("Expected a PDF file URL"); - } - if (!nsFile.exists() || !nsFile.isFile()) { - throw new Error("Expected an existing PDF file"); - } - let file = await File.createFromNsIFile(nsFile); - if (!(await this.#looksLikePDF(file))) { - throw new Error("Expected PDF content"); + if (result !== Ci.nsIFilePicker.returnOK) { + return "canceled"; } - this.browsingContext.top.loadURI(uri, { + let file = fp.file; + if ( + !file?.leafName.toLowerCase().endsWith(".pdf") || + !(await this.#looksLikePDF(file)) + ) { + return "invalid"; + } + + if (browsingContext.isDiscarded) { + return "canceled"; + } + browsingContext.loadURI(fp.fileURL, { triggeringPrincipal: Services.scriptSecurityManager.getSystemPrincipal(), }); + return "opened"; } async #setDefaultPDFHandler() { @@ -83,6 +116,13 @@ export class AboutPDFParent extends JSWindowActorParent { } async #looksLikePDF(file) { - return (await file.slice(0, PDF_HEADER.length).text()) === PDF_HEADER; + try { + let bytes = await IOUtils.read(file.path, { + maxBytes: PDF_HEADER.length, + }); + return new TextDecoder().decode(bytes) === PDF_HEADER; + } catch { + return false; + } } } diff --git a/toolkit/components/aboutpdf/content/aboutPDF.html b/toolkit/components/aboutpdf/content/aboutPDF.html index 7857f75bfe5e..c1d525dd84ea 100644 --- a/toolkit/components/aboutpdf/content/aboutPDF.html +++ b/toolkit/components/aboutpdf/content/aboutPDF.html @@ -61,12 +61,6 @@ >

-
{ - fileInput.click(); -}); - -fileInput.addEventListener("change", () => { - const file = fileInput.files[0]; - // Reset so the same file can be re-selected after an error. - fileInput.value = ""; - if (file) { - handleFile(file); - } + pickFile(); }); dropzone.addEventListener("dragenter", e => { @@ -48,11 +38,19 @@ dropzone.addEventListener("dragleave", e => { }); dropzone.addEventListener("drop", e => { - e.preventDefault(); dropzone.classList.remove("drag-over"); - const file = e.dataTransfer?.files[0]; - if (file) { - handleFile(file); + // Let native handling open .pdf files with a PDF MIME type; cancel others. + const files = e.dataTransfer?.files; + if ( + !files?.length || + ![...files].every( + file => + file.type === "application/pdf" && + file.name.toLowerCase().endsWith(".pdf") + ) + ) { + e.preventDefault(); + showError("invalid"); } }); @@ -60,7 +58,7 @@ dropzone.addEventListener("drop", e => { // through #browse-files which has the real button semantics. dropzone.addEventListener("click", e => { if (!e.target.closest("#browse-files")) { - fileInput.click(); + pickFile(); } }); @@ -106,14 +104,15 @@ async function updatePromoVisibility() { let processing = false; -async function handleFile(file) { +// The parent validates the selected file before opening it. +async function pickFile() { if (processing) { return; } processing = true; showError(null); try { - if (!(await RPMOpenPDFFile(file))) { + if ((await RPMPickPDFFile()) === "invalid") { showError("invalid"); } } catch (e) { @@ -124,7 +123,7 @@ async function handleFile(file) { } } -// errorType: null (clear), "invalid" (file type), or "generic" (other failure). +// errorType: null, "invalid" (not a PDF), or "generic". function showError(errorType) { if (!errorType) { dropzoneError.hidden = true; @@ -142,8 +141,7 @@ function showError(errorType) { dropzoneHint.hidden = errorType === "invalid"; } -// Enter triggers the picker only when the dropzone itself is hovered and no -// inner control has focus (which would handle Enter itself). +// Enter opens the picker while the dropzone is hovered and no control has focus. document.addEventListener("keydown", e => { if (e.key === "Enter" && dropzone.matches(":hover")) { const active = document.activeElement; @@ -153,7 +151,7 @@ document.addEventListener("keydown", e => { active === document.documentElement ) { e.preventDefault(); - fileInput.click(); + pickFile(); } } }); diff --git a/toolkit/components/aboutpdf/tests/browser/browser_aboutPDF.js b/toolkit/components/aboutpdf/tests/browser/browser_aboutPDF.js index 596a91cd05d8..1b34c43576f1 100644 --- a/toolkit/components/aboutpdf/tests/browser/browser_aboutPDF.js +++ b/toolkit/components/aboutpdf/tests/browser/browser_aboutPDF.js @@ -12,11 +12,6 @@ add_task(async function testPageRenders() { ok(doc.getElementById("dropzone-hint"), "dropzone-hint exists"); ok(doc.getElementById("browse-files"), "browse button exists"); - const fileInput = doc.getElementById("file-input"); - ok(fileInput, "file input exists"); - is(fileInput.accept, ".pdf,application/pdf", "file input accept attribute"); - ok(fileInput.hidden, "file input is hidden"); - await ContentTaskUtils.waitForCondition( () => dropzone.title, "dropzone title is localized" @@ -57,3 +52,69 @@ add_task(async function testDragVisualState() { }); BrowserTestUtils.removeTab(tab); }); + +add_task(async function testDropOnlyCancelsNonPDFs() { + const tab = await openAboutPDF(); + await SpecialPowers.spawn(tab.linkedBrowser, [], () => { + const doc = content.document; + const dropzone = doc.getElementById("dropzone"); + const errorEl = doc.getElementById("dropzone-error"); + + // Build the DataTransfer in the page's principal so its files are readable. + const sandbox = SpecialPowers.Cu.Sandbox(doc.nodePrincipal, { + sandboxPrototype: content, + }); + const buildDataTransfer = SpecialPowers.Cu.evalInSandbox( + `files => { + const dataTransfer = new DataTransfer(); + for (const { name, type } of files) { + dataTransfer.items.add(new File(["%PDF-1.4"], name, { type })); + } + return dataTransfer; + }`, + sandbox + ); + + // Record whether the page canceled the drop before canceling it for the test. + function drop(...files) { + const dataTransfer = buildDataTransfer( + SpecialPowers.Cu.cloneInto(files, sandbox) + ); + let canceledByPage; + doc.addEventListener( + "drop", + e => { + canceledByPage = e.defaultPrevented; + // Prevent native drop handling in this synthetic test. + e.preventDefault(); + }, + { once: true } + ); + dropzone.dispatchEvent( + new content.DragEvent("drop", { + bubbles: true, + cancelable: true, + dataTransfer, + }) + ); + return canceledByPage; + } + + const pdf = { name: "doc.pdf", type: "application/pdf" }; + const text = { name: "doc.txt", type: "text/plain" }; + + ok(!drop(pdf), "a dropped PDF is left to the browser to open"); + ok(errorEl.hidden, "no error is shown for a PDF"); + + ok(drop(text), "a dropped non-PDF is canceled"); + ok(!errorEl.hidden, "the invalid file error is shown"); + + ok( + drop({ name: "doc.pdf", type: "text/plain" }), + "a .pdf name with another type is canceled" + ); + ok(drop(pdf, text), "a mixed set of dropped files is canceled"); + ok(drop(), "a drop without any file is canceled"); + }); + BrowserTestUtils.removeTab(tab); +}); diff --git a/toolkit/components/aboutpdf/tests/browser/browser_aboutPDF_actor.js b/toolkit/components/aboutpdf/tests/browser/browser_aboutPDF_actor.js index 460fd803a7c4..a5cf61eddd00 100644 --- a/toolkit/components/aboutpdf/tests/browser/browser_aboutPDF_actor.js +++ b/toolkit/components/aboutpdf/tests/browser/browser_aboutPDF_actor.js @@ -3,100 +3,83 @@ "use strict"; +const { MockFilePicker } = SpecialPowers; + const PDF_CONTENTS = `%PDF-1.4 1 0 obj<>endobj 2 0 obj<>endobj 3 0 obj<>endobj trailer<>`; -async function callOpenFile(actor, fileURL) { - return actor.receiveMessage({ - name: "AboutPDF:OpenFile", - data: { fileURL }, +add_setup(function () { + MockFilePicker.init(); + registerCleanupFunction(() => { + MockFilePicker.cleanup(); }); -} - -add_task(async function testRejectsNonStringURL() { - const tab = await openAboutPDF(); - const actor = getAboutPDFActor(tab); - await Assert.rejects( - callOpenFile(actor, 42), - /Expected a file URL/, - "non-string fileURL rejected" - ); - await Assert.rejects( - callOpenFile(actor, undefined), - /Expected a file URL/, - "undefined fileURL rejected" - ); - BrowserTestUtils.removeTab(tab); }); -add_task(async function testRejectsNonFileScheme() { +function pickFile(actor, file) { + MockFilePicker.setFiles(file ? [file] : []); + MockFilePicker.returnValue = file + ? MockFilePicker.returnOK + : MockFilePicker.returnCancel; + return actor.receiveMessage({ name: "AboutPDF:PickFile" }); +} + +add_task(async function testCanceledPicker() { const tab = await openAboutPDF(); - const actor = getAboutPDFActor(tab); - await Assert.rejects( - callOpenFile(actor, "https://example.com/foo.pdf"), - /Expected a file URL/, - "https URL rejected" + is( + await pickFile(getAboutPDFActor(tab), null), + "canceled", + "a canceled picker opens nothing" + ); + is( + tab.linkedBrowser.currentURI.spec, + "about:pdf", + "the tab did not navigate" ); BrowserTestUtils.removeTab(tab); }); add_task(async function testRejectsNonPDFExtension() { - const path = await createTempFile(PDF_CONTENTS, { suffix: ".txt" }); + const file = await createTempFile(PDF_CONTENTS, { suffix: ".txt" }); const tab = await openAboutPDF(); - const actor = getAboutPDFActor(tab); - await Assert.rejects( - callOpenFile(actor, PathUtils.toFileURI(path)), - /Expected a PDF file URL/, + is( + await pickFile(getAboutPDFActor(tab), file), + "invalid", ".txt extension rejected" ); BrowserTestUtils.removeTab(tab); - await safeRemove(path); -}); - -add_task(async function testRejectsMissingFile() { - const path = PathUtils.join( - PathUtils.tempDir, - `aboutPDF-missing-${Date.now()}.pdf` - ); - const tab = await openAboutPDF(); - const actor = getAboutPDFActor(tab); - await Assert.rejects( - callOpenFile(actor, PathUtils.toFileURI(path)), - /Expected an existing PDF file/, - "non-existent file rejected" - ); - BrowserTestUtils.removeTab(tab); + await safeRemove(file); }); add_task(async function testRejectsBadMagicBytes() { - const path = await createTempFile("not actually a pdf"); + const file = await createTempFile("not actually a pdf"); const tab = await openAboutPDF(); - const actor = getAboutPDFActor(tab); - await Assert.rejects( - callOpenFile(actor, PathUtils.toFileURI(path)), - /Expected PDF content/, + is( + await pickFile(getAboutPDFActor(tab), file), + "invalid", "file without %PDF- header rejected" ); BrowserTestUtils.removeTab(tab); - await safeRemove(path); + await safeRemove(file); }); add_task(async function testAcceptsValidPDFAndNavigates() { - const path = await createTempFile(PDF_CONTENTS); - const fileURL = PathUtils.toFileURI(path); + const file = await createTempFile(PDF_CONTENTS); + const fileURL = Services.io.newFileURI(file).spec; const tab = await openAboutPDF(); - const actor = getAboutPDFActor(tab); - const navigated = BrowserTestUtils.browserLoaded( tab.linkedBrowser, false, url => url === fileURL ); - await callOpenFile(actor, fileURL); + is( + await pickFile(getAboutPDFActor(tab), file), + "opened", + "the picked PDF is opened" + ); await navigated; is( @@ -111,5 +94,149 @@ add_task(async function testAcceptsValidPDFAndNavigates() { }); BrowserTestUtils.removeTab(tab); - await safeRemove(path); + await safeRemove(file); +}); + +add_task(async function testPickerNeedsUserActivation() { + // Avoid navigation if this unexpectedly succeeds. + MockFilePicker.setFiles([]); + MockFilePicker.returnValue = MockFilePicker.returnCancel; + + const tab = await openAboutPDF(); + await SpecialPowers.spawn(tab.linkedBrowser, [], () => { + let threw = false; + try { + content.wrappedJSObject.RPMPickPDFFile(); + } catch { + threw = true; + } + ok(threw, "a script can't open the picker without a user gesture"); + }); + BrowserTestUtils.removeTab(tab); +}); + +add_task(async function testBrowseButtonOpensPickedPDF() { + const file = await createTempFile(PDF_CONTENTS); + const fileURL = Services.io.newFileURI(file).spec; + MockFilePicker.setFiles([file]); + MockFilePicker.returnValue = MockFilePicker.returnOK; + + const tab = await openAboutPDF(); + const navigated = BrowserTestUtils.browserLoaded( + tab.linkedBrowser, + false, + url => url === fileURL + ); + MockFilePicker.shown = false; + // A scripted click would not grant user activation. + await BrowserTestUtils.synthesizeMouseAtCenter( + "#browse-files", + {}, + tab.linkedBrowser + ); + await TestUtils.waitForCondition( + () => MockFilePicker.shown, + "the browse button opened the picker" + ); + await navigated; + + is( + tab.linkedBrowser.currentURI.spec, + fileURL, + "the browse button opens the picked PDF" + ); + + await SpecialPowers.spawn(tab.linkedBrowser, [], async () => { + const viewer = content.wrappedJSObject.PDFViewerApplication; + await viewer.testingClose(); + }); + + BrowserTestUtils.removeTab(tab); + await safeRemove(file); +}); + +add_task(async function testBrowseButtonReportsANonPDF() { + const file = await createTempFile(PDF_CONTENTS, { suffix: ".txt" }); + MockFilePicker.setFiles([file]); + MockFilePicker.returnValue = MockFilePicker.returnOK; + + const tab = await openAboutPDF(); + await BrowserTestUtils.synthesizeMouseAtCenter( + "#browse-files", + {}, + tab.linkedBrowser + ); + await SpecialPowers.spawn(tab.linkedBrowser, [], async () => { + const error = content.document.getElementById("dropzone-error"); + await ContentTaskUtils.waitForCondition( + () => !error.hidden, + "the page reports the invalid file" + ); + is( + error.getAttribute("data-l10n-id"), + "about-pdf-dropzone-invalid-file", + "the invalid file error is shown" + ); + }); + + BrowserTestUtils.removeTab(tab); + await safeRemove(file); +}); + +add_task(async function testSecondPickerIsIgnored() { + const tab = await openAboutPDF(); + const actor = getAboutPDFActor(tab); + + let opens = 0; + let releaseFirst; + MockFilePicker.setFiles([]); + MockFilePicker.returnValue = MockFilePicker.returnCancel; + MockFilePicker.showCallback = () => { + opens++; + // Keep the first picker open; close an unexpected second. + return opens === 1 + ? new Promise(resolve => { + releaseFirst = resolve; + }) + : Promise.resolve(); + }; + + const showing = actor.receiveMessage({ name: "AboutPDF:PickFile" }); + await TestUtils.waitForCondition(() => opens, "the first picker is showing"); + is( + await actor.receiveMessage({ name: "AboutPDF:PickFile" }), + "canceled", + "the second request is refused" + ); + is(opens, 1, "no second picker was opened while one was up"); + + releaseFirst(); + await showing; + MockFilePicker.showCallback = null; + BrowserTestUtils.removeTab(tab); +}); + +// Ignore file URLs from content; the parent must obtain the path itself. +add_task(async function testIgnoresFileURLFromContent() { + const file = await createTempFile(PDF_CONTENTS); + const tab = await openAboutPDF(); + const actor = getAboutPDFActor(tab); + + is( + await actor.receiveMessage({ + name: "AboutPDF:OpenFile", + data: { fileURL: Services.io.newFileURI(file).spec }, + }), + undefined, + "a file URL coming from the content process is not handled" + ); + await TestUtils.waitForTick(); + is( + tab.linkedBrowser.currentURI.spec, + "about:pdf", + "the tab did not navigate" + ); + + BrowserTestUtils.removeTab(tab); + await safeRemove(file); }); diff --git a/toolkit/components/aboutpdf/tests/browser/head.js b/toolkit/components/aboutpdf/tests/browser/head.js index 849cf46a6648..bab92b2d9d3a 100644 --- a/toolkit/components/aboutpdf/tests/browser/head.js +++ b/toolkit/components/aboutpdf/tests/browser/head.js @@ -21,17 +21,16 @@ async function createTempFile(contents, { suffix = ".pdf" } = {}) { const file = Services.dirsvc.get("TmpD", Ci.nsIFile); file.append(`aboutPDF-test${suffix}`); file.createUnique(Ci.nsIFile.NORMAL_FILE_TYPE, 0o600); - const path = file.path; const bytes = typeof contents === "string" ? new TextEncoder().encode(contents) : contents; - await IOUtils.write(path, bytes); - return path; + await IOUtils.write(file.path, bytes); + return file; } -async function safeRemove(path) { +async function safeRemove(file) { try { - await IOUtils.remove(path, { ignoreAbsent: true }); + await IOUtils.remove(file.path, { ignoreAbsent: true }); } catch {} } diff --git a/toolkit/locales/en-US/toolkit/about/aboutPDF.ftl b/toolkit/locales/en-US/toolkit/about/aboutPDF.ftl index a7a17d165d80..9b39a13b0a5c 100644 --- a/toolkit/locales/en-US/toolkit/about/aboutPDF.ftl +++ b/toolkit/locales/en-US/toolkit/about/aboutPDF.ftl @@ -13,6 +13,8 @@ about-pdf-dropzone-invalid-file = This file isn’t supported. Choose a PDF to c about-pdf-dropzone-error-generic = The upload didn’t go through. Try again later. about-pdf-browse-files = .label = Browse files +# Title of the system file picker opened by the “Browse files” button. +about-pdf-file-picker-title = Open PDF about-pdf-promo = .heading = Make { -brand-short-name } your go-to PDF editor .message = Open PDFs in { -brand-short-name } and handle the basics in one place, from highlights and signatures to merged files and comments. diff --git a/toolkit/modules/RemotePageAccessManager.sys.mjs b/toolkit/modules/RemotePageAccessManager.sys.mjs index 0417aa2502ce..5cdb58279512 100644 --- a/toolkit/modules/RemotePageAccessManager.sys.mjs +++ b/toolkit/modules/RemotePageAccessManager.sys.mjs @@ -87,7 +87,7 @@ export let RemotePageAccessManager = { "about:pdf": { RPMCanSetDefaultPDFHandler: ["*"], RPMGetBoolPref: ["browser.aboutpdf.promo.dismissed"], - RPMOpenPDFFile: ["*"], + RPMPickPDFFile: ["*"], RPMSetDefaultPDFHandler: ["*"], RPMSetPref: ["browser.aboutpdf.promo.dismissed"], },