XmlHttpRequest with HTTP method "LINK" yields HTTP request "Link"

VERIFIED FIXED in mozilla31

Status

()

Core
Networking: HTTP
VERIFIED FIXED
9 years ago
3 years ago

People

(Reporter: Julian Reschke, Assigned: mcmanus)

Tracking

Trunk
mozilla31
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

9 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.0.6) Gecko/2009011913 Firefox/3.0.6 Ubiquity/0.1.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.0.6) Gecko/2009011913 Firefox/3.0.6 Ubiquity/0.1.5

For some reason, when doing an XmlHttpRequest with method name "LINK" (defined in RFC 2068), the actual method name transferred is "Link" (last three letters lowercased).

Reproducible: Always
Live HTTP Headers isn't enough for seeing this problem with 
http://greenbytes.de/test/xhrmethods.html
Steps to reproduce:
 1) Load a page that contains an HTTP header called Link as a response header:
http://www.hixie.ch/tests/adhoc/http/link/001.html
 2) Open Live HTTP Headers
 3) Navigate to http://greenbytes.de/test/xhrmethods.html
 4) Press the button.
 5) Inspect the Live HTTP headers log.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Component: General → Networking: HTTP
Product: Firefox → Core
QA Contact: general → networking.http
OS: Windows XP → All
Hardware: x86 → All
Version: unspecified → 1.9.0 Branch
(Reporter)

Comment 3

9 years ago
Further proof of this being related to previous occurance of HTTP headers:

"CONTENT-LENGTH" gets lowercased, while "ONTENT-LENGTH" does not.
Assignee: nobody → michal
This is caused by the way we resolve HTTP atoms. nsHttp::ResolveAtom first look into hashtable if there is already such atom and if it is unknown it is created. The comparison is case insensitive and this is a problem when resolving methods since they are case sensitive (RFC 2616 section 5.1.1). "Link" is defined as atom at http://hg.mozilla.org/mozilla-central/annotate/3f5521e1c5a3/netwerk/protocol/http/src/nsHttpAtomList.h#l97 and this atom is resolved when somebody uses method "LINK". Hotfix for this bug can be removing header atom "Link" and adding method atom "LINK". This would cause sending header Link uppercase but this is OK since headers are case insensitive (RFC 2616 section 4.2).

But I think that we should remove methods from current atom list and handle them separately. With current behavior some buggy or malicious JS code can prevent other JS applications from working properly. For example when somebody issues XMLHttpRequest with method undefined in nsHttpAtomList.h (e.g ACL, CHECKOUT, MKWORKSPACE etc.) with wrong case (e.g. acl) then this wrong command is stored into atom list and no other application can issue correct request. Also it is possible to have HTTP server that handles standard method "GET" and some user defined method "get" which is not possible to call in firefox. Separating methods into case sensitive list would solve this problem.

Any opinions?
Yeah, it doesn't make sense to use case-insensitive atoms for HTTP methods.  They should just use our normal atom table, no?
> They should just use our normal atom table, no?

Yes, I think so. But changing this could break extensions that use lowercase method names. Can we really do it?
You mean use lowercase names for "get", "head", "post"?

We can upcase some whitelist before atomizing if we need to...  The XHR spec has something to say about this too, no?
You're right. XHR doc says:

If stored method case-insensitively matches  CONNECT, DELETE, GET, HEAD, OPTIONS POST, PUT, TRACE, or TRACK let stored method be the canonical uppercase form of the matched method name.

The question is if we should implement it in nsXMLHttpRequest or in nsHttpChannel.
I'm happy with the latter, honestly.
Created attachment 371678 [details] [diff] [review]
patch v1

Changes HTTP methods to case sensitive except methods CONNECT, DELETE, GET, HEAD, OPTIONS, POST, PUT, TRACE, TRACK
Attachment #371678 - Flags: review?(bzbarsky)
Comment on attachment 371678 [details] [diff] [review]
patch v1

>+#define HTTP_METHOD_ATOM(_name, _value) nsIAtom* nsHttp::_name = nsnull;

No need for the " = nsnull" part.

>+_name = NS_NewPermanentAtom(_value); \

I'd think you want static atoms, not permanent atoms here.  See how nsGkAtoms is set up, for example.

>+++ b/netwerk/protocol/http/src/nsHttpChannel.cpp
>@@ -1916,17 +1916,19 @@ nsHttpChannel::CheckCache()

I assume this (and all places where HTTP code gets these method atoms from strings) is only called on the main thread, right?

> nsHttpChannel::SetRequestMethod(const nsACString &method)

>+    if (!nsHttp::IsValidToken(PromiseFlatCString(method)))

You could probably do this after uppercasing, on a known-flat string, and avoid the extra PromiseFlatCString call, no?

>+    // We've changed method names to case sensitive in bug 477578. Following
>+    // methods are kept case insensitive to keep backward compatibility and
>+    // to satisfy XMLHttpRequest specification which demands it.

Is there a reason to not just atomize the uppecase method and then do atom compares?

Also, I'd prefer if we put this information in the atom list.  So instead of just having HTTP_METHOD_ATOM, have HTTP_METHOD_ATOM and HTTP_CASE_INSENSITIVE_METHOD_ATOM, define the latter to the former if it's not defined, then here define METHOD_ATOM to nothing and CASE_INSENSITIVE_METHOD_ATOM to do the compare.

The rest looks fine.
Attachment #371678 - Flags: review?(bzbarsky) → review-
Created attachment 371762 [details] [diff] [review]
patch v2

> I assume this (and all places where HTTP code gets these method atoms from
> strings) is only called on the main thread, right?

Yes, I think so. This happens in nsHttpChannel::SetRequestMethod() and in nsHttpChannel::CheckCache() which is called only from nsHttpChannel::Connect().
Attachment #371678 - Attachment is obsolete: true
Attachment #371762 - Flags: review?(bzbarsky)
Comment on attachment 371762 [details] [diff] [review]
patch v2

Looks good.
Attachment #371762 - Flags: review?(bzbarsky) → review+
Attachment #371762 - Flags: superreview?(cbiesinger)
(Reporter)

Comment 14

6 years ago
Unfortunately, this patch doesn't apply anymore (because files moved, and a few things seem to be refactored).
Created attachment 556591 [details] [diff] [review]
updated patch

Pushed to try server http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=766bc8757176.

Does this need a sr?
Attachment #371762 - Attachment is obsolete: true
Attachment #371762 - Flags: superreview?(cbiesinger)
(Reporter)

Comment 16

6 years ago
works for me (tested: http://www.mnot.net/javascript/xmlhttprequest/)
Attachment #556591 - Flags: superreview?(joshmoz)

Updated

6 years ago
Attachment #556591 - Flags: superreview?(joshmoz) → superreview+
http://hg.mozilla.org/integration/mozilla-inbound/rev/90e4d207f5dd
Whiteboard: [inbound]
Target Milestone: --- → mozilla9
https://hg.mozilla.org/mozilla-central/rev/90e4d207f5dd
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Keywords: dev-doc-needed
This code is broken, it refcounts atoms off the main thread.  See the assertions in the log at https://tbpl.mozilla.org/php/getParsedLog.php?id=6499610&tree=Firefox&full=1.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Ugh.  I didn't realize we create request heads on the socket thread!  That's pretty seriously broken.

Kyle, did you back this out already, or does that still need to be done?
I have it queued up, but haven't pushed yet.
The stacks look like:

###!!! ASSERTION: wrong thread: 'NS_IsMainThread()', file /builds/slave/m-cen-osx64-dbg/build/xpcom/ds/nsAtomTable.cpp, line 306
PermanentAtomImpl::AddRef [xpcom/ds/nsAtomTable.cpp:307]
nsCOMPtr<nsIAtom>::nsCOMPtr [nsCOMPtr.h:568]
nsHttpRequestHead::nsHttpRequestHead [netwerk/protocol/http/nsHttpRequestHead.h:55]
nsHttpConnection::SetupProxyConnect [netwerk/protocol/http/nsHttpConnection.cpp:842]
nsHttpConnection::Activate [netwerk/protocol/http/nsHttpConnection.cpp:183]
nsHttpConnectionMgr::DispatchTransaction [netwerk/protocol/http/nsHttpConnectionMgr.cpp:866]
nsHttpConnectionMgr::nsHalfOpenSocket::OnOutputStreamReady [netwerk/protocol/http/nsHttpConnectionMgr.cpp:1577]
nsSocketOutputStream::OnSocketReady [netwerk/base/src/nsSocketTransport2.cpp:514]
nsSocketTransport::OnSocketReady [netwerk/base/src/nsSocketTransport2.cpp:1534]
nsSocketTransportService::DoPollIteration [netwerk/base/src/nsSocketTransportService2.cpp:736]
nsSocketTransportService::Run [netwerk/base/src/nsSocketTransportService2.cpp:634]
nsThread::ProcessNextEvent [xpcom/threads/nsThread.cpp:631]
NS_ProcessNextEvent_P [obj-firefox/xpcom/build/nsThreadUtils.cpp:245]
nsThread::ThreadFunc [xpcom/threads/nsThread.cpp:271]
_pt_root [nsprpub/pr/src/pthreads/ptthread.c:190]
libSystem.B.dylib + 0x3a536
Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/d300ffff0be7
Target Milestone: mozilla9 → ---
Version: 1.9.0 Branch → Trunk

Updated

6 years ago
Duplicate of this bug: 688278
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #23)
> Backed out:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/d300ffff0be7

Merge of backout: https://hg.mozilla.org/mozilla-central/rev/d300ffff0be7
Keywords: dev-doc-needed
(In reply to Michal Novotny (:michal) from comment #12)
> Yes, I think so. This happens in nsHttpChannel::SetRequestMethod() and in
> nsHttpChannel::CheckCache() which is called only from
> nsHttpChannel::Connect().

CheckCache() will be done off the main thread real soon now.
Depends on: 722034

Comment 27

4 years ago
Any updates on that one? I have to rewrite quite a bunch of frameworks and libraries, that do not know of case-sensitivity for http-verbs ... they all just uppercased them. Even if that's against the docs, it would've worked ;)
(Assignee)

Comment 28

4 years ago
I wasn't aware of this bug - thanks simonsimcity

imo, rather than having two atom tables, methods should just not be atoms. In most cases we'll just be able to strcmp method, GET and set a bool and be done with it rather than worrying about locked hashtables and whatnot.

I'll work on a patch... I expect it will be large but pretty mechanical.
(Assignee)

Comment 29

4 years ago
Created attachment 8393111 [details] [diff] [review]
http methods should be case sensitive
Attachment #8393111 - Flags: review?(hurley)
(Assignee)

Updated

4 years ago
Assignee: michal.novotny → mcmanus
Status: REOPENED → ASSIGNED
(Assignee)

Comment 30

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=211501edecba
(Assignee)

Comment 31

4 years ago
Comment on attachment 8393111 [details] [diff] [review]
http methods should be case sensitive

boris, r? just for the content/base/src/nsXMLHttpRequest.cpp bit

In HTTP the method name is case sensitive, but xhr requires a few particular methods to be upcased and the rest to be left case sensitive... so the patch accommodates (and tests) each different case.

note that xhr is also supposed to throw an error on CONNECT, (along with trace and track which it was doing) and that I plugged that omission.
Attachment #8393111 - Flags: review?(bzbarsky)
(Assignee)

Updated

4 years ago
Attachment #556591 - Attachment is obsolete: true
Comment on attachment 8393111 [details] [diff] [review]
http methods should be case sensitive

r=me on the DOM bits, but mind making the return code there for the trace/connect/track case be NS_ERROR_DOM_SECURITY_ERR, since the spec wants a SecurityError?

And maybe file a followup on doing the other checking the spec calls for here (e.g. making sure the method matches the production at http://tools.ietf.org/html/rfc2616#section-5.1.1
Attachment #8393111 - Flags: review?(bzbarsky) → review+
(Reporter)

Comment 33

4 years ago
(In reply to Boris Zbarsky [:bz] from comment #32)
> And maybe file a followup on doing the other checking the spec calls for
> here (e.g. making sure the method matches the production at
> http://tools.ietf.org/html/rfc2616#section-5.1.1

Nowadays: http://greenbytes.de/tech/webdav/draft-ietf-httpbis-p1-messaging-26.html#rfc.section.3.1.1 (in RFC Editor queue). But no, the ABNF production did not change.
> Nowadays

Probably need a bug on the XHR spec then to get it updated; I linked to the thing it links to.
(Reporter)

Comment 35

4 years ago
(In reply to Boris Zbarsky [:bz] from comment #34)
> Probably need a bug on the XHR spec then to get it updated; I linked to the
> thing it links to.

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

Comment 36

4 years ago
(In reply to Boris Zbarsky [:bz] from comment #32)

> 
> r=me on the DOM bits, but mind making the return code there for the
> trace/connect/track case be NS_ERROR_DOM_SECURITY_ERR, since the spec wants
> a SecurityError?
> 

ok

> And maybe file a followup on doing the other checking the spec calls for
> here (e.g. making sure the method matches the production at
> http://tools.ietf.org/html/rfc2616#section-5.1.1

the channel already enforces that
Comment on attachment 8393111 [details] [diff] [review]
http methods should be case sensitive

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

LGTM, just one comment on the mochitest that may or may not be an issue.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +630,5 @@
> +        method == nsHttpRequestHead::kMethod_Head ||
> +        method == nsHttpRequestHead::kMethod_Options) {
> +        return true;
> +    }
> +    

nit: whitespace

::: netwerk/protocol/http/nsHttpRequestHead.h
@@ +62,5 @@
>              return mHeaders.SetHeader(h, nsDependentCString(v), merge);
>          return NS_OK;
>      }
>  
> +        

nit: whitespace

::: netwerk/test/mochitests/test_xhr_method_case.html
@@ +38,5 @@
> +]
> +
> +function doIter(index)
> +{
> +  SimpleTest.waitForExplicitFinish();

So here comes my complete lack of experience with mochitests: this seems like a case of unbalanced calls to waitForExplicitFinish. Perhaps mochitest doesn't care, in which case it's possibly fine, but it just looks weird to me. If this call in each iteration of the loop isn't required, remove it, to prevent future mochitest-inexperienced people like me from being confused :)
Attachment #8393111 - Flags: review?(hurley) → review+
(Assignee)

Comment 38

4 years ago
nick,

> 
> ::: netwerk/test/mochitests/test_xhr_method_case.html
> @@ +38,5 @@
> > +]
> > +
> > +function doIter(index)
> > +{
> > +  SimpleTest.waitForExplicitFinish();
> 
that wasn't supposed to be there - just a bug you caught. Thanks!
(Assignee)

Comment 39

4 years ago
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/20d8f9639b9c
https://hg.mozilla.org/mozilla-central/rev/20d8f9639b9c
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31

Comment 41

4 years ago
Thanks for all the work you put in here, :mcmanus and all others involved! Haven't thought it would've been fixed that fast (even so I now have to wait for Firefox 31 :) )
Blocks: 918709
Blocks: 918722
I reproduced the issue on Firefox 30 using scenario from comment 3.
Verified as fixed on Win 7 64bit, using Firefox 31 Beta 2:
- User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0
- BuildID: 20140616143923
Status: RESOLVED → VERIFIED
Depends on: 1037339
You need to log in before you can comment on or make changes to this bug.