The default bug view has changed. See this FAQ.

HTTP status 308 response results in nsIChannelEventSink::REDIRECT_TEMPORARY passed to nsIChannelEventSink instead of nsIChannelEventSink::REDIRECT_PERMANENT

RESOLVED FIXED in mozilla18

Status

()

Core
Networking
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: briansmith, Assigned: Julian Reschke)

Tracking

Trunk
mozilla18
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

+++ This bug was initially created as a clone of Bug #714302 +++

See ContinueProcessRedirectionAfterFallback:

    if (mRedirectType == 301) // Moved Permanently
        redirectFlags = nsIChannelEventSink::REDIRECT_PERMANENT;
    else
        redirectFlags = nsIChannelEventSink::REDIRECT_TEMPORARY;

It is a good idea to grep through all the code (especially in netwerk/ and in content/, where XHR lives) to see all the cases where 301 is handled specially.

I know that before we agreed that, as far as Necko is concerned, there should be no difference in the various kinds of redirects, but we should make sure 307/301 are reported to users of Necko correctly so they can make their own decisions as to what distinctions to make between them.

(Also, FYI, the XMLHttpRequest Level 2 specification enumerates the various statuses that are to be considered redirects in some cases, and it omits status 308.)
Keywords: dev-doc-complete, sec-review-complete
(BTW, it is confusing that 301 is permanent and 302 is temporary, but 307 is temporary and 308 is permanent.)
Summary: HTTP status 307 response results in nsIChannelEventSink::REDIRECT_TEMPORARY passed to nsIChannelEventSink instead of nsIChannelEventSink::REDIRECT_PERMANENT → HTTP status 308 response results in nsIChannelEventSink::REDIRECT_TEMPORARY passed to nsIChannelEventSink instead of nsIChannelEventSink::REDIRECT_PERMANENT
(Assignee)

Comment 2

5 years ago
(In reply to Brian Smith (:bsmith) from comment #0)
> +++ This bug was initially created as a clone of Bug #714302 +++
> 
> See ContinueProcessRedirectionAfterFallback:
> 
>     if (mRedirectType == 301) // Moved Permanently
>         redirectFlags = nsIChannelEventSink::REDIRECT_PERMANENT;
>     else
>         redirectFlags = nsIChannelEventSink::REDIRECT_TEMPORARY;
> 
> It is a good idea to grep through all the code (especially in netwerk/ and
> in content/, where XHR lives) to see all the cases where 301 is handled
> specially.
> 
> I know that before we agreed that, as far as Necko is concerned, there
> should be no difference in the various kinds of redirects, but we should
> make sure 307/301 are reported to users of Necko correctly so they can make
> their own decisions as to what distinctions to make between them.

Ack. Will check and provide a patch.

> (Also, FYI, the XMLHttpRequest Level 2 specification enumerates the various
> statuses that are to be considered redirects in some cases, and it omits
> status 308.)

Indeed. That's the code where XHR requires redirects to be followed, which is problematic in itself.

Optimally. XHR would (a) allow callers to disable following redirects and (b) actually state what to do with 3xx codes in general.
(Assignee)

Comment 3

5 years ago
(In reply to Brian Smith (:bsmith) from comment #0)
> ...
> (Also, FYI, the XMLHttpRequest Level 2 specification enumerates the various
> statuses that are to be considered redirects in some cases, and it omits
> status 308.)

-> <https://www.w3.org/Bugs/Public/show_bug.cgi?id=17222>
(Assignee)

Comment 4

5 years ago
Created attachment 627859 [details] [diff] [review]
proposed patch, work-in-progress

Adds checks for 308 to 2 more places. Added a function for classification in HttpBaseChannel.cpp; but I'm not sure how to use it in nsHttpResponseHead.cpp; maybe it needs to be moved somewhere else. Also, optimally, we had test coverage for this; do we have test cases where the permanence bis is tested somehow?
Attachment #627859 - Flags: review?(bsmith)
Comment on attachment 627859 [details] [diff] [review]
proposed patch, work-in-progress

I recommend that you make IsPermanentRedirect a static member of nsHttpResponseHead so that you can use it in the ComputeFreshnessLife as well as in nsHttpChannel.

The patch looks good to me, but I am going to ask Christian to look at it too in the hope that he might be able to suggest a convenient way to test it.
Attachment #627859 - Flags: review?(bsmith) → review?(cbiesinger)
Comment on attachment 627859 [details] [diff] [review]
proposed patch, work-in-progress

+    if ((mStatus == 300) || (mStatus == 301) || (mStatus == 308)) {

this should probably use the function too. make the function a static member of nsHttp? (in nsHttp.h/cpp)
Attachment #627859 - Flags: review?(cbiesinger) → review-
(Assignee)

Comment 7

5 years ago
Created attachment 642214 [details] [diff] [review]
Proposed patch

Moved as suggested.

I wonder whether we should do the same for IsSafeMethod and ShouldRewriteRedirectToGET (for consistency)?
Attachment #627859 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #642214 - Flags: review?(bsmith)
Comment on attachment 642214 [details] [diff] [review]
Proposed patch

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

If you want to move the existing methods for consistency then please do so in a separate patch and r+ me ahead of time for it.

::: netwerk/protocol/http/nsHttp.cpp
@@ +290,5 @@
> +bool
> +nsHttp::IsPermanentRedirect(PRUint32 httpStatus)
> +{
> +  return httpStatus == 301 || httpStatus == 308;
> +}   

Trailing whitespace.
Attachment #642214 - Flags: review?(bsmith) → review+
(Assignee)

Comment 9

5 years ago
(In reply to Brian Smith (:bsmith) from comment #8)
> If you want to move the existing methods for consistency then please do so
> in a separate patch and r+ me ahead of time for it.

See bug 782235.
(Assignee)

Comment 10

5 years ago
Created attachment 651325 [details] [diff] [review]
Proposed patch
Attachment #642214 - Attachment is obsolete: true
Attachment #651325 - Flags: review?(bsmith)
(Assignee)

Updated

5 years ago
Blocks: 782235
(Assignee)

Comment 11

5 years ago
Brian, can I set this to "r+" myself (the only change was the removal of trailing whitespace)?
Comment on attachment 651325 [details] [diff] [review]
Proposed patch

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

When somebody gives you r+ with a request to fix some trivial issue (naming, whitespace, etc.), you can automatically r+ the new version of the patch when you attach it.
Attachment #651325 - Flags: review?(bsmith) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
I don't see any Try results here, so I pushed. I'll land it if it goes green.
https://tbpl.mozilla.org/?tree=Try&rev=07e44b46b028
(In reply to Ryan VanderMeulen from comment #13)
> https://tbpl.mozilla.org/?tree=Try&rev=07e44b46b028

Green on Try.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c4c2b82b79a2
Flags: in-testsuite?
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c4c2b82b79a2
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.