The default bug view has changed. See this FAQ.

HTTP 307 redirect behavior violates RFC2616 [should preserve request method]

RESOLVED FIXED in mozilla1.4alpha

Status

()

Core
Networking: HTTP
P4
normal
RESOLVED FIXED
17 years ago
3 years ago

People

(Reporter: walter, Assigned: Suresh)

Tracking

(Blocks: 1 bug, {embed, topembed+})

Trunk
mozilla1.4alpha
embed, topembed+
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [http/1.1 compliance] [adt3 RTM] checklinux, URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

17 years ago
Mozilla M17 doesn't seem handle HTTP response 303 (See Other)
correctly (see http://www.w3.org/Protocols/rfc2616/rfc2616.html)

Comment 1

17 years ago
walter@data.franken.de can you provide more details.  See the bug writing
guiedlines at http://www.mozilla.org/quality/bug-writing-guidelines.html  Please
at least describe what you see that we're doing wrong and what you expect to see.
(Reporter)

Comment 2

17 years ago
What the RFC says, that Mozilla should do is the following: "The response to 
the request can be found under a different URI and SHOULD be retrieved using a 
GET method on that resource. This method exists primarily to allow the output 
of a POST-activated script to redirect the user agent to a selected resource. 
The new URI is not a substitute reference for the originally requested 
resource. The 303 response MUST NOT be cached, but the response to the second 
(redirected) request might be cacheable. The different URI SHOULD be given by 
the Location field in the response. Unless the request method was HEAD, the 
entity of the response SHOULD contain a short hypertext note with a hyperlink 
to the new URI(s)."

What Mozilla does is display the "entity of the response" as if it was a 200
response, without retrieving the page specified via the "Location" header.

Comment 3

17 years ago
Networking
Assignee: asa → gagan
Status: UNCONFIRMED → NEW
Component: Browser-General → Networking
Ever confirmed: true
QA Contact: doronr → tever

Updated

17 years ago
Target Milestone: --- → Future
This shouldn't be futured. This is missing support for HTTP/1.1.
HTTP/1.1 response 307 (Temporary Redirect) isn't handled, too (2000083008,
Win NT).

Testcase: http://clarence.de/post-test/form.html
(written for bug 46338; problems using it caused by bug 50143).

Walter, you should nominate this bug for nsbeta3 (add keyword "nsbeta3"; I'm
not allowed to do this). Reasons: Standards support, easy to fix.
Summary should read "303 and 307" instead of "303".
(Reporter)

Updated

17 years ago
Keywords: nsbeta3
Summary: HTTP response 303 not handled → HTTP responses 303 and 307 not handled

Comment 5

17 years ago
wont make it for beta3.
Whiteboard: [nsbeta3-]
The 303 case is fixed. See bug 56785. Verified with 10-30-04 trunk, Win NT.

For the 307 case we have a dupe: bug 58150.

I have rewritten my testcase at http://clarence.de/post-test/form.html to make
it more useful for redirect bugs.

Additional issue:
HTTP 301 and 302 are handled wrong when used in the response to a form
submission. They should behave the same way a 307 should, that is re-submitting
the form data to the new location instead of performing a HTTP GET on it like
303 does.
For 302 the correct behavior may be unwise due to incorrect implementation in
most browsers and possible use of the unambiguous 307. But for 301 Mozilla
should conform to RFC 2616.

Comment 7

17 years ago
*** Bug 58150 has been marked as a duplicate of this bug. ***

Comment 8

17 years ago
http bugs to "Networking::HTTP"
Assignee: gagan → darin
Component: Networking → Networking: HTTP
Target Milestone: Future → M19

Comment 9

16 years ago
nominating for moz 0.9
Target Milestone: --- → mozilla0.9
Blocks: 68423

Comment 10

16 years ago
Just an FYI. See the stuff about 301, 302 and 307 in the following:

  http://www.w3.org/TR/2001/NOTE-cuap-20010206#protocols

Comment 11

16 years ago
removed stale nsbeta3 keyword.
added nsbeta1 keyword.
Keywords: nsbeta3 → nsbeta1
Whiteboard: [nsbeta3-]

Updated

16 years ago
Status: NEW → ASSIGNED
Summary: HTTP responses 303 and 307 not handled → HTTP responses 303 and 307 not handled correctly
Target Milestone: mozilla0.9 → mozilla0.9.1

Comment 12

16 years ago
Seeing on linux too please platform/os->all
OS: Windows NT → All
Hardware: PC → All

Comment 13

16 years ago
my changes for bug 76866 will fix this bug as well.
Depends on: 76866

Comment 14

16 years ago
fixed with branch landing
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
307 works now, but not as it should (it seems to behave exactly like 303 does).
We should use the same method that was used on the old URI, i.e. if it was a
POST we should perform a POST to the new Location, using the old POST data.
RFC 2616 requires confirmation by the user before doing so (in case of a POST).
The same rules should be applied to 301.

Reopening.
Changing summary "303 and 307" -> "301 and 307". 303 was fixed in bug 56785.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Summary: HTTP responses 303 and 307 not handled correctly → HTTP responses 301 and 307 not handled correctly

Comment 16

16 years ago
-> 0.9.2
Target Milestone: mozilla0.9.1 → mozilla0.9.2

Updated

16 years ago
Keywords: nsbeta1

Updated

16 years ago
Status: REOPENED → ASSIGNED

Updated

16 years ago
Priority: P3 → P4

Comment 17

16 years ago
->0.9.3
Target Milestone: mozilla0.9.2 → mozilla0.9.3

Comment 18

16 years ago
*** Bug 81636 has been marked as a duplicate of this bug. ***

Updated

16 years ago
Target Milestone: mozilla0.9.3 → mozilla1.0

Updated

16 years ago
Depends on: 101502

Comment 19

16 years ago
Updating dependency on the duplicate.
Depends on: 89419
No longer depends on: 101502

Comment 20

16 years ago
-> 0.9.8
Priority: P4 → P2
Whiteboard: [http/1.1 compliance]
Target Milestone: mozilla1.0 → mozilla0.9.8

Comment 21

15 years ago
-> moz 1.0 hopefully
Keywords: mozilla1.0
Target Milestone: mozilla0.9.8 → mozilla1.0

Comment 22

15 years ago
Is bug 112208 related to this?

Comment 23

15 years ago
unlikely.

Updated

15 years ago
Keywords: nsbeta1

Updated

15 years ago
Keywords: nsbeta1+

Updated

15 years ago
Keywords: nsbeta1

Updated

15 years ago
Whiteboard: [http/1.1 compliance] → [http/1.1 compliance][adt2]

Comment 24

15 years ago
post 1.0
Target Milestone: mozilla1.0 → mozilla1.0.1

Updated

15 years ago
Target Milestone: mozilla1.0.1 → ---

Comment 25

15 years ago
mass futuring of untargeted bugs
Target Milestone: --- → Future

Comment 26

15 years ago
darin/gagan - what are the chnaces that we'd be able to get a fix for this
before 06.14.2002? pls add eta. thanks!
Keywords: mozilla1.0 → mozilla1.0.1
Whiteboard: [http/1.1 compliance][adt2] → [http/1.1 compliance] [adt3 RTM] [eta needed]

Comment 27

15 years ago
targeting mozilla 1.2 to get this fixed.
Keywords: mozilla1.0.1 → mozilla1.2
Target Milestone: Future → mozilla1.2alpha

Updated

15 years ago
Blocks: 65092

Updated

15 years ago
Target Milestone: mozilla1.2alpha → mozilla1.2beta

Comment 28

15 years ago
looking like this might slip...
Severity: major → minor
Keywords: embed
Priority: P2 → P5

Comment 29

15 years ago
-> moz 1.3
Target Milestone: mozilla1.2beta → mozilla1.3alpha

Updated

15 years ago
Keywords: topembed+

Updated

15 years ago
Target Milestone: mozilla1.3alpha → mozilla1.3beta

Comment 30

14 years ago
301 behavior is not going to change.  we are better off matching existing
browser behavior.  307 however should be revised to prompt the user and preserve
the method.
Severity: minor → normal
Priority: P5 → P4
Summary: HTTP responses 301 and 307 not handled correctly → HTTP 307 redirect behavior violates RFC2616 [should preserve request method]
Target Milestone: mozilla1.3beta → mozilla1.4alpha

Updated

14 years ago
Blocks: 156686

Comment 31

14 years ago
-> suresh
Assignee: darin → suresh
Status: ASSIGNED → NEW
(Assignee)

Comment 32

14 years ago
Created attachment 117618 [details] [diff] [review]
patch

Any suggestion for a better wording of the dialog?
(Assignee)

Updated

14 years ago
Attachment #117618 - Flags: superreview?(darin)
(Assignee)

Updated

14 years ago
Status: NEW → ASSIGNED

Comment 33

14 years ago
s/resend/forward/ in the message?

Comment 34

14 years ago
Comment on attachment 117618 [details] [diff] [review]
patch

>Index: src/nsHttpChannel.cpp

i would move the prompting code into a subroutine.

>+        if (redirectType == 307 && mUploadStream) {
...
>+                if (mCallbacks) {
>+                    mCallbacks->GetInterface(NS_GET_IID(nsIPrompt), getter_AddRefs(prompt));

it is not enough to just check mCallbacks, you also need to check the
mLoadGroup's notification callbacks if mCallbacks does not provide a
prompter.  see for example PromptForUserPass.  the code to query a
callback interface should be moved into a subroutine that can be
shared by each of these routines.


>+            nsCOMPtr<nsISeekableStream> seekable = do_QueryInterface(mUploadStream);
>+            if (seekable)
>+                seekable->Seek(nsISeekableStream::NS_SEEK_SET, 0);

it should be an error if the QI fails.	iirc, you can safely bail
from ProcessRedirection and the calling code will clean up ok.


>+
>+            nsCOMPtr<nsIUploadChannel> uploadChannel(do_QueryInterface(httpChannel));
>+            if (uploadChannel)
>+                uploadChannel->SetUploadStream(mUploadStream, NS_LITERAL_CSTRING(""), -1);
>+        }

calling SetUploadStream like this is not quite right.  what about the
case in which SetUploadStream was called with an explicit contentType
and/or contentLength?  you need to duplicate the previous call to
SetUploadStream or this will not work in all cases.


>Index: locale/en-US/necko.properties

>+RepostFormData=The server is redirecting to a new URL. Would you like to resend the data to this new location?

this sounds a bit awkward to me, but i'm no UE expert.	please run
this by someone from UE.  thx!
Attachment #117618 - Flags: superreview?(darin) → superreview-
(Assignee)

Comment 35

14 years ago
Created attachment 117672 [details] [diff] [review]
updated patch.

I'm yet to work with UE folks for a better wording in the dialog.
Attachment #117618 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #117672 - Flags: superreview?(darin)

Comment 36

14 years ago
Comment on attachment 117672 [details] [diff] [review]
updated patch.

>Index: src/nsHttpChannel.cpp

>+    nsCOMPtr<nsIPrompt> prompt;
>+    rv = GetPrompt(getter_AddRefs(prompt));
>+    if (NS_FAILED(rv)) return rv;
>+    nsCOMPtr<nsIAuthPrompt> authPrompt = do_QueryInterface(prompt, &rv);
>+    if (NS_FAILED(rv)) return rv;

unfortunately, this isn't quite right.	the object implementing nsIPrompt
is not guaranteed to implement nsIAuthPrompt.  there may be a separate
object implementing nsIAuthPrompt.  you have to call GetInterface with
the correct IID.  i suggest you rename GetPrompt as GetCallback, and 
allow the caller to pass in a nsIID, just like nsIInterfaceRequestor::
GetInterface.
Attachment #117672 - Flags: superreview?(darin) → superreview-
(Assignee)

Comment 37

14 years ago
Created attachment 117776 [details] [diff] [review]
updated patch.
Attachment #117672 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #117776 - Flags: superreview?(darin)

Comment 38

14 years ago
Comment on attachment 117776 [details] [diff] [review]
updated patch.

>Index: src/nsHttpChannel.cpp

>+nsHttpChannel::GetCallback(const nsIID &aIID, void **aResult)
>+{
>+    nsresult rv = NS_OK;
>+    NS_ENSURE_ARG_POINTER(aResult);
>+    rv = mCallbacks->GetInterface(aIID, aResult);

no need to initialize |rv| since you are just going to overwrite
it immediately anyways.  also, the NS_ENSURE macro should just
be a debug only NS_ASSERTION.

>+nsHttpChannel::PromptTempRedirect()
>+{
>+    nsresult rv = NS_OK;
>+    nsCOMPtr<nsIStringBundleService> bundleService =
>+            do_GetService(NS_STRINGBUNDLE_CONTRACTID, &rv);

no reason to initialize |rv|.  do_GetService always assigns a value.


>+    rv = stringBundle->GetStringFromName(NS_LITERAL_STRING("RepostFormData").get(), 
...
>+    if (NS_SUCCEEDED(rv) && messageString) {

can GetStringFromName really return NS_OK without setting messageString?
seems like you should only have to perform one of these checks, no?  hmm..
in reading nsStringBundle::GetStringFromName, it appears that it does not
null check the result of ToNewUnicode, so yes it is possible for this
function to return NS_OK and a NULL messageString.  that is very lame; we
should file a bug against nsStringBundle to fix that.  documenting this
here would be good, so we can come back and clean it up later.


>+            nsCOMPtr<nsISeekableStream> seekable = do_QueryInterface(mUploadStream, &rv);
>+            if (NS_FAILED(rv)) return rv;
>+            if (seekable)
>+                seekable->Seek(nsISeekableStream::NS_SEEK_SET, 0);

if QI succeeds then |seekable| will be non-null.  so you don't need to check
|seekable| after checking |rv|.  also you should check the return value from
Seek since it could easily result in an I/O error of some sort.


>+            nsCOMPtr<nsIUploadChannel> uploadChannel(do_QueryInterface(httpChannel));
>+            if (uploadChannel)

if this QI fails, then it should be considered an error.  just check |rv|.


>     static void  PR_CALLBACK AsyncCall_EventCleanupFunc(PLEvent *);
>+    nsresult GetCallback(const nsIID &aIID, void **aResult);
>+    nsresult PromptTempRedirect();

please move these function decls up after ProcessAuthentication.  thx!
Attachment #117776 - Flags: superreview?(darin) → superreview-
(Assignee)

Comment 39

14 years ago
Created attachment 117898 [details] [diff] [review]
updated patch.
(Assignee)

Updated

14 years ago
Attachment #117776 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #117898 - Flags: superreview?(darin)

Comment 40

14 years ago
Comment on attachment 117898 [details] [diff] [review]
updated patch.

looks great.. thx suresh!

sr=darin
Attachment #117898 - Flags: superreview?(darin) → superreview+
(Assignee)

Comment 41

14 years ago
Thanks darin for your help and review comments!!
(Assignee)

Updated

14 years ago
Attachment #117898 - Flags: review?(dougt)

Comment 42

14 years ago
Comment on attachment 117898 [details] [diff] [review]
updated patch.

use do_GetInterface instead of a direct call on mCallbacks.  (i am pretty sure
that do_GetInterface does a test for null).

nit: In GetCallback, unwind the nesting and return immediately if getinterface
is successful.

if GetStringFromName does return success but returns a null string, shouldn't
we return an error?

Maybe we should ensure that if GetCallbacks returns rv == NS_OK, then the
resulting object is always valid.  This would avoid two tests.

rv = GetCallback(NS_GET_IID(nsIPrompt), getter_AddRefs(prompt));
+	 if (NS_FAILED(rv)) return rv;
+	 if (prompt) {


fix that stuff and r=dougt.
Attachment #117898 - Flags: review?(dougt) → review+

Comment 43

14 years ago
Suggested wording per Suresh's request:

"This web page is being redirected to a new location. Would you like to resend
the form data you have typed to the new location?" 
(Assignee)

Comment 44

14 years ago
This is fixed in trunk!!
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago14 years ago
Resolution: --- → FIXED

Comment 45

14 years ago
VERIFIED: Mozilla 1.4b, Mach-O, Win98

The dialog comes up for post as described.

Should I also be checking the "don't cache redirect + cache Location:<URL>"
behavior too? I cannot tell if that was broken based on the original description.
QA Contact: tever → httpqa
Whiteboard: [http/1.1 compliance] [adt3 RTM] [eta needed] → [http/1.1 compliance] [adt3 RTM] checklinux
You need to log in before you can comment on or make changes to this bug.