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

• Status: RESOLVED
• Resolution: INVALID
• Component: WebCore Misc.

[David Quesada]

If the applied manifest specifies a scope URL, we should not allow navigations to URLs out of scope of that URL.

[David Quesada]

Created attachment 328536 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/35869860>

[Geoffrey Garen]

Comment on attachment 328536 [details]
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.

[David Quesada]

This is no longer valid with recent version of the Web App Manifest spec:

"Unlike previous versions of this specification, user agents are no longer required or allowed to block off-scope navigations, or open them in a new top-level browsing context. This practice broke some sites that navigate to an off-scope URL"