intercepted channels should enforce redirection limits on synthetic redirects

RESOLVED FIXED in Firefox 58

Status

()

enhancement
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

(Blocks 1 bug)

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(4 attachments, 5 obsolete attachments)

1.69 KB, patch
valentin
: review+
Details | Diff | Splinter Review
4.52 KB, patch
asuth
: review+
Details | Diff | Splinter Review
820 bytes, patch
asuth
: review+
Details | Diff | Splinter Review
7.05 KB, patch
valentin
: review+
Details | Diff | Splinter Review
Currently we are failing a test in the WPT redirected-response.https.html file because we don't enforce the 20 redirect limit on synthetic redirects.  We should fix this.
I introduced a new problem in bug 1405739, so I'm going to fix this now.  When I split the mRedirectCount into a separate mInternalRedirectCount I failed to propagate both values across all kinds of redirects.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
I still need to fix e10s case.
(Assignee)

Updated

2 years ago
Depends on: 1411528
Updated to check redirect limit in HttpChannelChild as well.
Attachment #8922402 - Attachment is obsolete: true
Comment on attachment 8922386 [details] [diff] [review]
P1 Propagate mRedirectCount and mInternalRedirectCount across all redirects. r=valentin

Valentin, in bug 1405739 I separated out mInternalRedirectCount from mRedirectCount.  Unfortunately, though, I made a logic error.

The previous code increments the appropriate counter for the type of redirect and propagates that particular counter to the next channel.

The bug, however, is that we don't propagate the *other* counter to the next channel.  So if you have some thing does:

  "normal redirect"->"internal redirect"->"normal redirect"

You end up with an mRedirectCount of 1 and an mInternalRedirectCount of 0.  These should be mRedirect=2 and mInternalRedirectCount=1.

The solution in this patch is to always propagate both counters to the next channel regardless of the kind of redirect.  We still only update one or the other of course.
Attachment #8922386 - Flags: review?(valentin.gosu)
I need to investigate some orange before flagging the rest of the patches.
I need to fix P2 patch a bit more.  It doesn't look like nsHttpChannel enforces the redirect limit on internal redirects at all right now.
This gets a consistent check, but it has issues in e10s still.  We're not getting canceled correctly there in this test case if the redirect fails.
Attachment #8922502 - Attachment is obsolete: true
Attachment #8922386 - Flags: review?(valentin.gosu) → review+
Comment on attachment 8923031 [details] [diff] [review]
P2 Create a shared method in HttpBaseChannel to check the redirection limit. r=valentin

Valentin, this patch unifies the redirection limit check by moving it into a helper routine on HttpBaseChannel.  I think ensure that its called for all parent-side redirects by calling it in both nsHttpChannel and InterceptedHttpChannel SetupReplacementChannel() methods.

I also had to add an extra check on the client-side for synthetic redirects coming from a service worker.  If a service worker provides a synthetic redirect it can then re-intercept and do it again.  In these cases the redirection count is not propagated in the parent since we are triggering the redirect in the child.  Therefore this patch also checks the limit when triggering a synthetic redirect.

Note, for service workers to function at the limits (20 synthetic redirects, etc) we need to allow slightly more internal redirects than real redirects.  For now I created a constant for the extra amount.  It seemed like overkill to create a pref for it.

Please note, this patch will make nsHttpChannel check the redirect limit on internal redirects.  I found many internal redirects were not being checked against the limit and could in theory infinitely recurse.
Attachment #8923031 - Flags: review?(valentin.gosu)
Comment on attachment 8922403 [details] [diff] [review]
P3 Check ResetInterception result and CancelInterception if a failure occurs. r=asuth

Andrew, the additional redirect checking means we can now fail ResetInterception() due to hitting the limit on internal redirects.  In these cases we should cancel the channel so that OnStopRequest fires.
Attachment #8922403 - Flags: review?(bugmail)
Comment on attachment 8922504 [details] [diff] [review]
P4 Update redirected-response.https.html WPT expectation for redirect limit test. r=asuth

Update redirects-response.https.html WPT expectations.
Attachment #8922504 - Flags: review?(bugmail)
Attachment #8922403 - Flags: review?(bugmail) → review+
Attachment #8922504 - Flags: review?(bugmail) → review+
(Assignee)

Updated

2 years ago
See Also: → 1404524
Comment on attachment 8923031 [details] [diff] [review]
P2 Create a shared method in HttpBaseChannel to check the redirection limit. r=valentin

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

Looks good! Thanks, Ben!

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +5565,5 @@
>      if (NS_FAILED(rv))
>          return rv;
>  
> +    rv = CheckRedirectLimit(redirectFlags);
> +    NS_ENSURE_SUCCESS(rv, rv);

nit: Is there a reason why we can't add the check to SetupReplacementChannel?
Attachment #8923031 - Flags: review?(valentin.gosu) → review+
(In reply to Valentin Gosu [:valentin] from comment #19)
> ::: netwerk/protocol/http/nsHttpChannel.cpp
> @@ +5565,5 @@
> >      if (NS_FAILED(rv))
> >          return rv;
> >  
> > +    rv = CheckRedirectLimit(redirectFlags);
> > +    NS_ENSURE_SUCCESS(rv, rv);
> 
> nit: Is there a reason why we can't add the check to SetupReplacementChannel?

Do you mean HttpBaseChannel::SetupReplacementChannel()?  I did try that, but enforcing the limit in the child broke some of the machinery around redirects in e10s mode.  I don't remember the exact problems, but this patch ended up being less intrusive.  We continue to do redirection limit enforcement in the parent process like before with the one exception for service worker interception.  (And hopefully we will remove service worker interception in the child soonish so that exception will be temporary.)
(In reply to Ben Kelly [:bkelly] from comment #20)
> (In reply to Valentin Gosu [:valentin] from comment #19)
> > 
> > nit: Is there a reason why we can't add the check to SetupReplacementChannel?
> 
> Do you mean HttpBaseChannel::SetupReplacementChannel()?  I did try that, but
> enforcing the limit in the child broke some of the machinery around
> redirects in e10s mode.  I don't remember the exact problems, but this patch
> ended up being less intrusive.  We continue to do redirection limit
> enforcement in the parent process like before with the one exception for
> service worker interception.  (And hopefully we will remove service worker
> interception in the child soonish so that exception will be temporary.)

That sounds good. Thanks!

Comment 22

2 years ago
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/39cb0f9cde37
P1 Propagate mRedirectCount and mInternalRedirectCount across all redirects. r=valentin
https://hg.mozilla.org/integration/mozilla-inbound/rev/07285260f735
P2 Create a shared method in HttpBaseChannel to check the redirection limit. r=valentin
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d552d6d1581
P3 Check ResetInterception result and CancelInterception if a failure occurs. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/b594a19da9dc
P4 Update redirected-response.https.html WPT expectation for redirect limit test. r=asuth
You need to log in before you can comment on or make changes to this bug.