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

RESOLVED FIXED in Firefox 45

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bkelly, Assigned: sicking)

Tracking

(Blocks 1 bug)

unspecified
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(1 attachment)

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
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.
Flags: needinfo?(jonas)
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
(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.
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.
Flags: needinfo?(jonas)
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
This should fix it, though I need to write a test for the specific case of https->http redirect.
Assignee: bkelly → jonas
Attachment #8687637 - Flags: review?(bkelly)
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.
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.
Thank you for taking this!  I will review Monday.
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.
Attachment #8687637 - Flags: review?(bkelly) → review+
If I have time I will try to provide a test for this.
Flags: needinfo?(bkelly)
> > +  // 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.
(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
Flags: needinfo?(bkelly)
Put back the NI on myself to write a test here.
Flags: needinfo?(bkelly)
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.
Attachment #8687637 - Flags: review?(jduell.mcbugs)
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?
Attachment #8687637 - Flags: review?(jduell.mcbugs) → review+
Ben, do we want to uplift this patch to aurora as well?
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.
Flags: needinfo?(ehsan)
(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.
Flags: needinfo?(ehsan)
https://hg.mozilla.org/mozilla-central/rev/acad68770dba
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Blocks: 1229041
I'm happy to let this ride the trains as normal.  Thanks for fixing it!  I wrote bug 1229041 to add a test.
Flags: needinfo?(bkelly)
You need to log in before you can comment on or make changes to this bug.