[webkit-reviews] review denied: [Bug 180451] [Web App Manifest] Prevent out-of-scope navigations : [Attachment 328536] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied David Quesada
<david_quesada at apple.com>'s request for review:
Bug 180451: [Web App Manifest] Prevent out-of-scope navigations
https://bugs.webkit.org/show_bug.cgi?id=180451

Attachment 328536: Patch

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




--- Comment #3 from Geoffrey Garen <ggaren at apple.com> ---
Comment on attachment 328536
  --> https://bugs.webkit.org/attachment.cgi?id=328536
Patch

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

> Source/WebCore/Modules/applicationmanifest/ApplicationManifest.cpp:36
> +    if (scopeURL.isNull() || scopeURL.isEmpty())

Just check isEmpty().

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:206
>	   logDeveloperWarning(ASCIILiteral("The start URL is not within scope
of the provided scope URL."));

You should probably log the URLs here.

> Source/WebCore/loader/PolicyChecker.cpp:109
> +    // WebKitTestRunner expects to reset the state of the web view by
loading about:blank before running tests.
> +    // Support this (and avoid hanging WKTR by blocking the navigation) by
allowing blank URL navigations in certain cases.
> +    bool shouldCheckApplicationManifestScope = m_frame.isMainFrame() &&
m_frame.mainFrame().applicationManifest() &&
!m_frame.loader().originalRequest().url().isEmpty() &&
!request.url().isBlankURL();
> +    if (shouldCheckApplicationManifestScope &&
!isInApplicationScope(m_frame.mainFrame().applicationManifest()->scope,
request.url())) {
> +	   function(WTFMove(request), nullptr, false);

I'm skeptical of this approach.

If WebKitTestRunner applies an application manifest to a WebView for testing,
and then wants to reset that application manifest to run a new test, I would
prefer having explicit API or SPI to unapply the application manifest. I think
that approach would be better because:

(1) What are the consequences of leaving an application manifest applied when
running other tests?

(2) This behavior seems like a spec violation.

(3) Tight coupling between a framework and the parochial needs of a given app
over time makes frameworks unmaintainable. 

(4) We totally control WebKitTestRunner and we can make it do whatever we want.

Maybe you can explain more about why we couldn't address this through a change
to WebKitTestRunner?

> Source/WebCore/loader/PolicyChecker.cpp:110
> +	   m_frame.page()->console().addMessage(MessageSource::Other,
MessageLevel::Log, ASCIILiteral("Navigation to '") + request.url().string() +
ASCIILiteral("' was blocked because it is not within scope of the application
manifest."));

Should log the scope URL too.

Should say "was not within scope" instead of "is not within scope" because
grammar.