260771 2023-08-26 09:07:30 -0700 REGRESSION(266247@main): PDF "Save" button does nothing, "Print" function also broken 2023-11-21 05:29:39 -0800 1 1 1 Unclassified WebKit PDF Other PC Linux RESOLVED FIXED Gtk, InRadar P3 Normal --- 1 pbryan mcatanzaro abrarsl2002 achristensen annevk bugs-noreply mcatanzaro ntim thorton webkit-bug-importer oldest_to_newest 1973778 0 pbryan 2023-08-26 09:07:30 -0700 When viewing a PDF in Epiphany and MiniBrowser, clicking on the "Save" button does nothing. WebKitGTK version (from About Web -> Troubleshooting -> Debugging Information): 2.41.91 Distributor (Linux operating system, Flathub, Epiphany Tech Preview, etc.): Epiphany Tech Preview via Flathub 1980257 1 mcatanzaro 2023-09-26 12:42:35 -0700 There are hints in the web console: [Warning] The download attribute on anchor was ignored because its href URL has a different security origin. (viewer.js, line 12388) [Error] Not allowed to load local resource: blob:webkit-pdfjs-viewer://pdfjs/6e197ddb-9da6-43c4-8fe6-c0710b24c4f8 download (viewer.js:12388) download (viewer.js:12433) (anonymous function) (viewer.js:835) 1980335 2 mcatanzaro 2023-09-26 16:29:24 -0700 Hm, it broke somewhere between 265737@main "Update to PDF.js v3.8.162" and 266427@main "Update to PDF.js v3.9.179". I will bisect this. 1980357 3 mcatanzaro 2023-09-26 17:38:30 -0700 Printing is broken too: [Error] Not allowed to load local resource: blob:webkit-pdfjs-viewer://pdfjs/6921a4a0-4716-45bc-8fe6-df18b3bafb15 (anonymous function) (viewer.js:13401) 1981065 4 mcatanzaro 2023-09-28 12:51:37 -0700 Well I found the commit to blame. We're going to have to think about how to handle this. b1a659b38f09d05fd3e7b3b112066894ab099b8c is the first bad commit commit b1a659b38f09d05fd3e7b3b112066894ab099b8c Author: Anne van Kesteren <annevk@annevk.nl> Date: Mon Jul 24 08:42:00 2023 -0700 Return opaque origin for blob: URL containing inner non-http(s): URL https://bugs.webkit.org/show_bug.cgi?id=257262 rdar://109781193 Reviewed by Alex Christensen and Darin Adler. Refactor SecurityOrigin so it is more clear blob: URLs are the sole special case. And change how we derive the blob: URL origin to align with the URL standard: * No longer perform percent-decoding (matches other browsers). * Restrict non-opaque origins to HTTP(S) URLs (will soon match other browsers). However: * Still give blob: URLs derived from file: origins an origin for now as removing that ability needs a bit more care. This currently goes against the URL standard, but that might change. * Also give registered schemes a pass to allow embedders to continue to use blob: URLs as they see fit. Also change BlobURL to rely more directly on SecurityOrigin. * LayoutTests/imported/w3c/web-platform-tests/url/a-element-origin-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/url/a-element-origin-xhtml-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/url/url-origin.any-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/url/url-origin.any.worker-expected.txt: * Source/WebCore/fileapi/BlobURL.cpp: (WebCore::BlobURL::getOriginURL): (WebCore::BlobURL::isSecureBlobURL): * Source/WebCore/fileapi/ThreadableBlobRegistry.cpp: (WebCore::ThreadableBlobRegistry::getCachedOrigin): * Source/WebCore/page/SecurityOrigin.cpp: (WebCore::SecurityOrigin::create): (WebCore::SecurityOrigin::forBlobURL): (WebCore::SecurityOrigin::isSecure): (WebCore::SecurityOrigin::shouldUseInnerURL): Deleted. (WebCore::SecurityOrigin::extractInnerURL): Deleted. * Source/WebCore/page/SecurityOrigin.h: * Source/WebCore/page/SecurityOriginData.cpp: (WebCore::SecurityOriginData::shouldTreatAsOpaqueOrigin): Canonical link: https://commits.webkit.org/266247@main .../url/a-element-origin-expected.txt | 10 ++-- .../url/a-element-origin-xhtml-expected.txt | 10 ++-- .../url/url-origin.any-expected.txt | 10 ++-- .../url/url-origin.any.worker-expected.txt | 10 ++-- Source/WebCore/fileapi/BlobURL.cpp | 14 ++---- Source/WebCore/fileapi/ThreadableBlobRegistry.cpp | 3 +- Source/WebCore/page/SecurityOrigin.cpp | 57 ++++++++++++---------- Source/WebCore/page/SecurityOrigin.h | 18 ++----- Source/WebCore/page/SecurityOriginData.cpp | 13 ++--- 9 files changed, 66 insertions(+), 79 deletions(-) 1981121 5 mcatanzaro 2023-09-28 15:41:37 -0700 Hey Anne, this is really simple but fixes the bug. Does it look OK? diff --git a/Source/WebCore/page/SecurityOrigin.cpp b/Source/WebCore/page/SecurityOrigin.cpp index 605e0523d462..bdeba62901d8 100644 --- a/Source/WebCore/page/SecurityOrigin.cpp +++ b/Source/WebCore/page/SecurityOrigin.cpp @@ -178,6 +178,7 @@ inline bool isSafelistedBlobProtocol(const URL& url) // except that assert gets hit on certain tests. return url.protocolIsInHTTPFamily() || url.protocolIsFile() + || url.protocol() == "webkit-pdfjs-viewer"_s || LegacySchemeRegistry::schemeIsHandledBySchemeHandler(url.protocol()); } I'm thinking that's even correct rather than a hack. Will propose a pull request tomorrow probably. 1981219 6 annevk 2023-09-29 01:25:02 -0700 The problem with that fix is that then > new URL("webkit-pdfjs-viewer:blah").origin would not return "null" as I understand it. So an implementation detail of a WebKit port would become web-exposed (and non-compliant with the URL standard). I'd prefer not shipping that in Safari. 1981220 7 annevk 2023-09-29 01:27:24 -0700 To elaborate, it's probably acceptable, but if we can scope it with an ifdef or feature flag that would be preferable. And if it's not too much to ask maybe add a test for the specific scheme so it's clear what we don't want to regress? 1981261 8 mcatanzaro 2023-09-29 06:54:28 -0700 (In reply to Anne van Kesteren from comment #6) > I'd prefer not shipping that in Safari. I'll guard it behind ENABLE(PDFJS). If you use PDF.js, you're going to need it. I think you *do* build with that enabled, though? It was originally added for Apple platforms. (In reply to Anne van Kesteren from comment #7) > And if it's not too much to ask > maybe add a test for the specific scheme so it's clear what we don't want to > regress? I'll think about this.... 1981267 9 annevk 2023-09-29 07:31:09 -0700 Bug 242263 suggests we don't. The test could be as simple as asserting that new URL("webkit-pdfjs-viewer:blah").origin is "null" and PDFJS builds would mark that as failure. 1981276 10 mcatanzaro 2023-09-29 07:57:04 -0700 I'd rather do it the other way around and mark it as a failure for ports that don't use PDF.js, since the goal is to ensure we don't break PDF.js. :P 1981282 11 annevk 2023-09-29 08:28:55 -0700 Well, my goal is we don't break the URL Standard, which the current architecture of PDFJS apparently requires (perhaps worth a follow-up bug). I guess you can have the same test twice. 1981288 12 mcatanzaro 2023-09-29 08:53:13 -0700 I'm making PDF.js function the same as all URL schemes registered by applications, so I guess we already violate this standard for all such schemes? 1981306 13 annevk 2023-09-29 09:47:17 -0700 That's right, but the standards really only cover browsers, not a browser engine embedded in an arbitrary app. 1981323 14 mcatanzaro 2023-09-29 10:50:47 -0700 Browsers register lots of URL schemes; at least Epiphany has URL schemes for view source mode, reader mode, and about handlers. (There's also the built-in webkit:// scheme, although that one probably doesn't use LegacySchemeRegistry and so wouldn't hit this codepath.) Anyway, since Apple is clearly planning to turn on PDF.js support, I don't think we need the same test twice with different expectations. 1981329 15 ntim 2023-09-29 11:16:57 -0700 (In reply to Michael Catanzaro from comment #14) > Browsers register lots of URL schemes; at least Epiphany has URL schemes for > view source mode, reader mode, and about handlers. (There's also the > built-in webkit:// scheme, although that one probably doesn't use > LegacySchemeRegistry and so wouldn't hit this codepath.) > > Anyway, since Apple is clearly planning to turn on PDF.js support, I don't > think we need the same test twice with different expectations. There are no plans at this time. I agree we shouldn't regress the standard. If we can somehow use checks against PDFDocument to workaround the origin issues, that might work. 1981360 16 mcatanzaro 2023-09-29 12:26:09 -0700 (In reply to Anne van Kesteren from comment #6) > The problem with that fix is that then > > > new URL("webkit-pdfjs-viewer:blah").origin > > would not return "null" as I understand it. So an implementation detail of a > WebKit port would become web-exposed (and non-compliant with the URL > standard). > > I'd prefer not shipping that in Safari. Actually wait, when testing this I noticed that the origin is already not null without making changes. Testing unmodified WebKitGTK in the inspector console: > new URL("webkit-pdfjs-viewer:blah").origin < "webkit-pdfjs-viewer://" > new URL("webkit-pdfjs-viewerr:blah").origin < "null" That is, it's already treated specially without any changes. I think the change here is only concerned with the security origin of blob URLs specifically. A few more tests, just to see: > new URL("foo:bar").origin < "null" > new URL("ephy-about://blank").origin < "ephy-about://blank" > new URL("webkit://gpu").origin < "webkit://gpu" 1981365 17 mcatanzaro 2023-09-29 12:43:33 -0700 So I think following the spec is impossible and WebKit already does not attempt to do so. I'm looking at https://url.spec.whatwg.org/#origin (currently section 4.7 "Origin") and it's clear that the security origin of URLs for most protocols is defined to be always opaque. The only exceptions are blob, ftp, http, https, ws, wss, and file. But if we were to do that, then there's just no way that custom protocols can ever work. WebKit's SecurityOriginData::shouldTreatAsOpaqueOrigin already has a big list of WebKit protocols that should receive non-opaque security origins, including a bunch of Apple-specific ones (applewebdata, x-apple-ql-id, x-apple-ql-id2, x-apple-ql-magic) and the Linux-specific resource:// protocol. The list is there because the alternative to make the protocols work would be to disable same origin policy entirely, which would be much worse. For example, webkit-pdfjs-viewer pages should be able to load other webkit-pdfjs-viewer pages with the same security origin. resource pages should be able to load other resource pages. If the origins are opaque, it cannot work. So I think this is OK. The spec handles standard web protocols. We can say we're extending it for the benefit of WebKit-specific protocols and custom protocols. I'm now I'm wondering if isSafelistedBlobProtocol can be replaced by SecurityOriginData::shouldTreatAsOpaqueOrigin: diff --git a/Source/WebCore/page/SecurityOrigin.cpp b/Source/WebCore/page/SecurityOrigin.cpp index 605e0523d462..488d13c5befa 100644 --- a/Source/WebCore/page/SecurityOrigin.cpp +++ b/Source/WebCore/page/SecurityOrigin.cpp @@ -169,18 +169,6 @@ Ref<SecurityOrigin> SecurityOrigin::create(const URL& url) return adoptRef(*new SecurityOrigin(url)); } -inline bool isSafelistedBlobProtocol(const URL& url) -{ - if (!url.isValid()) - return false; - - // FIXME: we ought to assert we're in WebKitLegacy or a web content process as per 263652@main, - // except that assert gets hit on certain tests. - return url.protocolIsInHTTPFamily() - || url.protocolIsFile() - || LegacySchemeRegistry::schemeIsHandledBySchemeHandler(url.protocol()); -} - Ref<SecurityOrigin> SecurityOrigin::createForBlobURL(const URL& url) { ASSERT(url.protocolIsBlob()); @@ -189,7 +177,7 @@ Ref<SecurityOrigin> SecurityOrigin::createForBlobURL(const URL& url) return origin.releaseNonNull(); URL pathURL { url.path().toString() }; - if (isSafelistedBlobProtocol(pathURL)) + if (!SecurityOriginData::shouldTreatAsOpaqueOrigin(pathURL)) return adoptRef(*new SecurityOrigin(pathURL)); return createOpaque(); I think I'll try this.... 1981369 18 mcatanzaro 2023-09-29 12:48:32 -0700 I think we're also missing the first step in the algorithm for computing the origin of a blob URL: "If url’s blob URL entry is non-null, then return url’s blob URL entry’s environment’s origin." But I also don't understand what that means. :) 1981409 19 mcatanzaro 2023-09-29 14:16:59 -0700 (In reply to Michael Catanzaro from comment #17) > So I think following the spec is impossible and WebKit already does not > attempt to do so. I'm looking at https://url.spec.whatwg.org/#origin > (currently section 4.7 "Origin") and it's clear that the security origin of > URLs for most protocols is defined to be always opaque. The only exceptions > are blob, ftp, http, https, ws, wss, and file. ... > I'm now I'm wondering if isSafelistedBlobProtocol can be replaced by > SecurityOriginData::shouldTreatAsOpaqueOrigin: OK, SecurityOriginData::shouldTreatAsOpaqueOrigin says it implements "https://url.spec.whatwg.org/#origin with some additions" added by Alex in 233783@main "Non-special URLs should have an opaque origin" so he already read this section of the spec and decided to handle special protocols specially. So I'm increasingly confident this makes sense. 1981414 20 mcatanzaro 2023-09-29 14:26:43 -0700 Pull request: https://github.com/WebKit/WebKit/pull/18438 1990206 21 ews-feeder 2023-11-06 10:07:44 -0800 Committed 270274@main (58df23fe5ac5): <https://commits.webkit.org/270274@main> Reviewed commits have been landed. Closing PR #18438 and removing active labels. 1990207 22 webkit-bug-importer 2023-11-06 10:08:17 -0800 <rdar://problem/118005690> 1994150 23 mcatanzaro 2023-11-21 05:29:39 -0800 *** Bug 265186 has been marked as a duplicate of this bug. ***