1217456 - fetch() manual redirect mode does not work properly when redirecting from secure to non-secure origin

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

If a fetch() call with "manual" redirect mode sees a redirect, its supposed to stop and return an opaquredirect response.  It does not actually follow the redirect.

This is an important feature because some large sites on the web use redirects during navigations.  For example, gmail login.  These sites would not be able to use service workers if a fetch(event.request) followed the redirect and hid the final URL from the window.

So navigations set manual redirect mode and expect an opaqueredirect from the serviecworker in order to work properly.

This mostly works today, except in this condition:

1) Use opens https://a.com/index.html
2) User clicks link to https://a.com/old/content.html
3) Site sends a 301 redirect to http://a.com/old/content.html

In this case our nsMixedContentBlocker fails to propagate the AsyncOnChannelRedirect to FetchDriver.  It thinks the redirect should not be followed, so it does not notify any of the other callback handlers.

nsMixedContentBlocker, though, does NOT abort the channel.

This causes fetch() to return a 301 basic response.

If HTTP caching is also enabled, then it may return a network error instead.  I believe this happens because the 'bad redirect' result is cached.  The nsMixedContentBlocker does not seem to get the redirect callback in this case either.

To see this in action:

1) Go to https://jakearchibald.github.io/trained-to-thrill/
2) Hit shift+reload to ensure you are not running with a service worker.
3) Execute this in the console:

  fetch('https://jakearchibald.github.io/isserviceworkerready/demos/http-redirect/blah', { mode: 'same-origin', redirect: 'manual' }).then(function(r) { console.log(r.type + ' ' + r.status); }).catch(function(e) { console.log(e); });

4) Observe that you either get a 301 basic response or a network error depending on if you have http caching enabled in devtools.

Jake points out this is something real sites like The Guardian need to do, so I think it should block v1:

  https://code.google.com/p/chromium/issues/detail?id=546495

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Jonas, please see comment 0.  Do you have ideas on the best way to deal with this?

I could potentially work around it with evil hacks in FetchDriver.

Alternatively, maybe we should make a new flag or nsIContentPolicyType for manual redirect fetch.  With this policy type the nsMixedContentBlocker would not do any redirect checking.

Thanks.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ben Kelly [:bkelly] from comment #0)
> If HTTP caching is also enabled, then it may return a network error instead.
> I believe this happens because the 'bad redirect' result is cached.  The
> nsMixedContentBlocker does not seem to get the redirect callback in this
> case either.

Actually, nsMixedContentBlocker does see the redirect in this case, but for some reason the channel is sometimes aborted with http caching enabled.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

I think maybe the right thing to do would be to make the http channel do the right thing when its redirect mode flag is manual.  It could simply avoid following the redirect.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Apparently this is real problem in production for the guardian:

  "This is a big bug for us at the Guardian. It's breaking some of our redirects, i.e. https://www.theguardian.com/info/2015/nov/03/removed-gallery which redirects to HTTP."

From:

  https://code.google.com/p/chromium/issues/detail?id=546495#c6

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Add "don't follow redirects" flag

This should fix it, though I need to write a test for the specific case of https->http redirect.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8687637 [details] [diff] [review]
Add "don't follow redirects" flag

Review of attachment 8687637 [details] [diff] [review]:
-----------------------------------------------------------------

Honza, could you review the /network changes?

Ben, could you review the FetchDriver changes?


This patch adds a DONT_FOLLOW_REDIRECTS flag to the security flags in nsILoadInfo, and then makes fetchdriver use that to prevent redirects when appropriate.

Honza, this implements the flag that we discussed before you left for vacation. I made httpchannel not cancel the old channel if the redirect was vetoed when following a cached redirect, but only if it was vetoed due to the new flag being set. We could try to do this any time that a redirect is vetoed, even if it was not due to the flag. I don't have strong opinions one way or another on that.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Oh, also, I'm not sure if I fully fixed the behavior where vetoing a cached redirect cancels the old channel. This looked right to me, but I didn't do too much testing.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Thank you for taking this!  I will review Monday.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8687637 [details] [diff] [review]
Add "don't follow redirects" flag

Review of attachment 8687637 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed.

::: dom/fetch/FetchDriver.cpp
@@ +765,5 @@
> +  // We should only ever get here if we use a "follow" redirect policy,
> +  // or if if we set an "error" policy as a result of a CORS policy.
> +  MOZ_ASSERT(mRequest->GetRedirectMode() == RequestRedirect::Follow ||
> +             (mRequest->GetRedirectMode() == RequestRedirect::Error &&
> +              IsUnsafeRequest()));

I think this is wrong.  Script can also call fetch(url, { redirect: 'error' }) to set an error redirect policy explicitly.  Can we drop the IsUnsafeRequest() and the comment about CORS policy here?

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +602,5 @@
>          // If AsyncProcessRedirection fails, then we have to send out the
>          // OnStart/OnStop notifications.
>          LOG(("ContinueHandleAsyncRedirect got failure result [rv=%x]\n", rv));
> +        if (!redirectsDisabled)
> +            mStatus = rv;

nit: I think there's lots of evidence that if-statements without an explicit block is bad.  It would be nice to include the curly braces.

@@ +608,1 @@
>          DoNotifyListener();

I think the logic here might be easier to read if you completely break out the redirectsDisabled case.  Something like:

  if (redirectsDisabled) {
    DoNotifyListener();
  } else {
    mStatus = rv;
    DoNotifyListener();
    if (mCacheEntry) {
      mCacheEntry->AsyncDoom(nullptr);
    }
  }

@@ +613,1 @@
>              mCacheEntry->AsyncDoom(nullptr);

nit: same comment about explicit block for an if statement.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

If I have time I will try to provide a test for this.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> > +  // We should only ever get here if we use a "follow" redirect policy,
> > +  // or if if we set an "error" policy as a result of a CORS policy.
> > +  MOZ_ASSERT(mRequest->GetRedirectMode() == RequestRedirect::Follow ||
> > +             (mRequest->GetRedirectMode() == RequestRedirect::Error &&
> > +              IsUnsafeRequest()));
> 
> I think this is wrong.  Script can also call fetch(url, { redirect: 'error'
> }) to set an error redirect policy explicitly.  Can we drop the
> IsUnsafeRequest() and the comment about CORS policy here?

If it does, we should set the SEC_DONT_FOLLOW_REDIRECT flag and should not end up AsyncOnChannelRedirect. So I think the assertion and the comment is correct?

This is also confirmed by the fact that the patch passes try, which hopefully has a testcase like the one you mention?

> > +        if (!redirectsDisabled)
> > +            mStatus = rv;
> 
> nit: I think there's lots of evidence that if-statements without an explicit
> block is bad.  It would be nice to include the curly braces.

I agree with you, but not using braces seem to be existing convention in this code. I'll let Honza decide.

> @@ +608,1 @@
> >          DoNotifyListener();
> 
> I think the logic here might be easier to read if you completely break out
> the redirectsDisabled case.  Something like:
> 
>   if (redirectsDisabled) {
>     DoNotifyListener();
>   } else {
>     mStatus = rv;
>     DoNotifyListener();
>     if (mCacheEntry) {
>       mCacheEntry->AsyncDoom(nullptr);
>     }
>   }

What I'd actually like to do is just

        DoNotifyListener();
        if (redirectsEnabled && mCacheEntry)
            mCacheEntry->AsyncDoom(nullptr);

And *never* do the |mStatus = rv| part, since that part seems bogus these days. See question to Honza in comment 6.

Honza, let me know what you prefer.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Jonas Sicking (:sicking) from comment #11)
> > > +  // We should only ever get here if we use a "follow" redirect policy,
> > > +  // or if if we set an "error" policy as a result of a CORS policy.
> > > +  MOZ_ASSERT(mRequest->GetRedirectMode() == RequestRedirect::Follow ||
> > > +             (mRequest->GetRedirectMode() == RequestRedirect::Error &&
> > > +              IsUnsafeRequest()));
> > 
> > I think this is wrong.  Script can also call fetch(url, { redirect: 'error'
> > }) to set an error redirect policy explicitly.  Can we drop the
> > IsUnsafeRequest() and the comment about CORS policy here?
> 
> If it does, we should set the SEC_DONT_FOLLOW_REDIRECT flag and should not
> end up AsyncOnChannelRedirect. So I think the assertion and the comment is
> correct?

Ok, I think you're right.

> This is also confirmed by the fact that the patch passes try, which
> hopefully has a testcase like the one you mention?

Yes, we should hit this condition in this wpt test:

  https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/mozilla/tests/service-workers/service-worker/fetch-event-redirect.https.html

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Put back the NI on myself to write a test here.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8687637 [details] [diff] [review]
Add "don't follow redirects" flag

Review of attachment 8687637 [details] [diff] [review]:
-----------------------------------------------------------------

Lets keep the error handling behavior as it is in the patch. So let me know if you want to change the code to look like Ben requested in the second-to-last section of comment 9.

Comment 15

[Jason Duell]

Comment on attachment 8687637 [details] [diff] [review]
Add "don't follow redirects" flag

Review of attachment 8687637 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/nsILoadInfo.idl
@@ +137,5 @@
> +   *
> +   * Redirects not initiated by a server response, i.e. REDIRECT_INTERNAL and
> +   * REDIRECT_STS_UPGRADE, are still followed.
> +   *
> +   * Note: If this flag is set and a the channel response is a redirect, then

s/and a/and/

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +602,5 @@
>          // If AsyncProcessRedirection fails, then we have to send out the
>          // OnStart/OnStop notifications.
>          LOG(("ContinueHandleAsyncRedirect got failure result [rv=%x]\n", rv));
> +        if (!redirectsDisabled)
> +            mStatus = rv;

We're gradually en-bracing all one-line if statements in necko to match the style guide.  So yeah, add it.  En-brace change, Jonas! :)

@@ +608,1 @@
>          DoNotifyListener();

So I'd like honza's signoff on any change to the regular (not DONT_FOLLOW_REDIRECTS) case of redirect vetoing.  Let's leave that for a followup.  

Meanwhile Ben's suggestion seems clean, and also provides a nice spot to put a comment above the mStatus = rv line, like

  // TODO: stop failing original channel if redirect vetoed?

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/acad68770dba

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ben, do we want to uplift this patch to aurora as well?

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Actually, Ben is on PTO. Ehsan, do you know if we should uplift this patch to Aurora? I don't think it has any dependencies.

Comment 19

[(no longer active)]

(In reply to Jonas Sicking (:sicking) from comment #18)
> Actually, Ben is on PTO. Ehsan, do you know if we should uplift this patch
> to Aurora? I don't think it has any dependencies.

No, I don't think this needs to be uplifted.  Leaving bkelly's needinfo in case he disagrees.

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/acad68770dba

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

I'm happy to let this ride the trains as normal.  Thanks for fixing it!  I wrote bug 1229041 to add a test.