[Webkit-unassigned] [Bug 180451] [Web App Manifest] Prevent out-of-scope navigations

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=180451

Geoffrey Garen <ggaren at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ggaren at apple.com
 Attachment #328536|review?                     |review-
              Flags|                            |

--- 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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20171206/66cf5655/attachment.html>