[webkit-reviews] review granted: [Bug 220665] Safari says "Blocked Plug-in" instead showing a PDF : [Attachment 417725] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted katherine_cheney at apple.com's request
for review:
Bug 220665: Safari says "Blocked Plug-in" instead showing a PDF
https://bugs.webkit.org/show_bug.cgi?id=220665

Attachment 417725: Patch

https://bugs.webkit.org/attachment.cgi?id=417725&action=review




--- Comment #4 from Darin Adler <darin at apple.com> ---
Comment on attachment 417725
  --> https://bugs.webkit.org/attachment.cgi?id=417725
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417725&action=review

Seems really important to fix this, very good that we will have a regression
test now!

I’m going to say review+ even though I have concerns about the content of the
patch.

> Source/WebCore/ChangeLog:3
> +	   Safari says "Blocked Plug-in" instead showing a PDF

instead *of* showing a PDF

Shouldn’t the bug title pinpoint where/when we regressed? I assume this is a
regression.

> Source/WebCore/html/HTMLPlugInImageElement.cpp:281
> +bool HTMLPlugInImageElement::shouldBypassCSPForPDFPlugin() const
> +{
> +    if (document().frame()->isMainFrame() &&
document().loader()->writer().mimeType() == "application/pdf") {
> +	   for (auto& pluginInfo : document().page()->pluginData().plugins()) {
> +	       if (pluginInfo.bundleIdentifier ==
"com.apple.webkit.builtinpdfplugin"_s)
> +		   return true;
> +	   }
> +    }
> +    return false;
> +}

This seems loosely coupled. We don’t want another plug-in to be able to claim
application/pdf and get run because the built-in PDF plug-in is present. But
that’s what this code says, and so it relies on code elsewhere.

Is there a way to target this more precisely, so it only targets the one
plug-in rather than all plug-ins. It’s not great that this counts on the fact
that this plug-in if present always "wins".

I suppose given this, I’d like to at least eventually see a test that proves
that trick doesn’t work, with a test plug-in that claims application/pdf and
prove that it doesn't run.

It’s *critical* to quickly fix this regression; I don’t have good insight how
it was introduced and how important it is to use this fix we have here even if
it’s not perfectly target.