[ServiceWorker] Fix bugs of wpt tests for Client.navigate

third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerWindowClient.cpp

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "modules/serviceworkers/ServiceWorkerWindowClient.h"
 
 #include <memory>
 #include "bindings/core/v8/CallbackPromiseAdapter.h"
 #include "bindings/core/v8/ScriptPromiseResolver.h"
 #include "core/dom/DOMException.h"
 #include "core/dom/ExceptionCode.h"
 #include "core/dom/ExecutionContext.h"
+#include "core/loader/MixedContentChecker.h"
 #include "core/page/PageVisibilityState.h"
 #include "core/workers/WorkerGlobalScope.h"
 #include "core/workers/WorkerLocation.h"
 #include "modules/serviceworkers/ServiceWorkerError.h"
 #include "modules/serviceworkers/ServiceWorkerGlobalScopeClient.h"
 #include "modules/serviceworkers/ServiceWorkerWindowClientCallback.h"
 #include "platform/wtf/RefPtr.h"
 #include "public/platform/WebString.h"
 
 namespace blink {
@@ skipping 46 matching lines @@
   ScriptPromiseResolver* resolver = ScriptPromiseResolver::Create(script_state);
   ScriptPromise promise = resolver->Promise();
   ExecutionContext* context = ExecutionContext::From(script_state);
 
   KURL parsed_url = KURL(ToWorkerGlobalScope(context)->location()->Url(), url);
   if (!parsed_url.IsValid() || parsed_url.ProtocolIsAbout()) {
     resolver->Reject(V8ThrowException::CreateTypeError(
         script_state->GetIsolate(), "'" + url + "' is not a valid URL."));
     return promise;
   }
+  if (MixedContentChecker::IsMixedContent(context->GetSecurityOrigin(),
+                                          parsed_url)) {

[falken, 2017/05/29 01:07:04]

Can you point me to the test that this fixes? I assume mixed content means the
service worker shouldn't navigate an iframe to an insecure URL if the iframe's
parent is secure.

This doesn't look like the correct way to detect mixed content. IsMixedContent()
just checks if the first arg is an https url, and the second arg is secure.
Since we're passing the service worker's origin as the first arg, I'd expect
this to always reject a navigation to an http url.

It also doesn't account for redirects or browser settings (I thnk you can choose
to enable mixed content).

I'm wondering if the W3C test is valid. Does the spec in fact require throwing a
TypeError if Client.navigate() would create a mixed content violation?

[Mike West, 2017/05/29 08:02:44]

I don't have a strong opinion about what the test result should be (e.g. whether
we should throw as a result of `navigate()`. I do believe that the navigation
should fail, however, as it attempts to navigate an HTTPS page's child frame to
an HTTP URL. As falken@ notes, however, this doesn't look like the right way to
guarantee that. We have mixed content checks in `MixedContentNavigationThrottle`
that should ensure the navigation fails. That seems like enough.

[leonhsl(Using Gerrit), 2017/05/29 13:09:54]

Sorry now I realized that I've done a hurry&incorrect fix for this problem..
The corresponding test is at:
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/...

The corresponding description in spec
https://w3c.github.io/ServiceWorker/#client-navigate:
7.1
HandleNavigate: Navigate browsingContext to url with exceptions enabled. The
source browsing context must be browsingContext.
7.2
If the algorithm steps invoked in the step labeled HandleNavigate throws an
exception, set navigateFailed to true.
7.x
XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
8
XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
9
If navigateFailed is true, reject promise with a TypeError and abort these
steps.

I think the wpt test itself is valid but I was somehow misunderstanding what
mixed content means.. I agree with falken's all comments about mixed content.
And just as Mike said, there is already some logic in navigation code ensuring
failure for such case, and our wpt test fails just because SW impl is lack of
logic catching the navigation failure and throwing TypeError in correspondence. 

Thank you all for all the explanations! I'll try the correct way later.

And for sure I want to confirm with Mike about cases for redirects or browser
settings, I suppose navigation code is already taking account of them? So SW
impl has no need to detect them, but just catching potential navigation failure
is OK.
E.g. If user choosed to enable mixed content, then navigation code will navigate
successfully(allow mixed content). Otherwise it will fail(Should be caught by
our SW impl).

[falken, 2017/05/29 14:29:35]

Thanks for the investigation and links. Yes it seems to me Client.navigate()
needs to detect errors during navigation and reject the promise. Apparently
we're not doing that currently.

[leonhsl(Using Gerrit), 2017/06/10 02:57:44]

Done.

+    resolver->Reject(V8ThrowException::CreateTypeError(
+        script_state->GetIsolate(),
+        "'" + url + "' navigation would create a mixed-content violation."));
+    return promise;
+  }
   if (!context->GetSecurityOrigin()->CanDisplay(parsed_url)) {
     resolver->Reject(V8ThrowException::CreateTypeError(
         script_state->GetIsolate(),
         "'" + parsed_url.ElidedString() + "' cannot navigate."));
     return promise;
   }
 
   ServiceWorkerGlobalScopeClient::From(context)->Navigate(
       Uuid(), parsed_url, WTF::MakeUnique<NavigateClientCallback>(resolver));
   return promise;
 }
 
 DEFINE_TRACE(ServiceWorkerWindowClient) {
   ServiceWorkerClient::Trace(visitor);
 }
 
 }  // namespace blink