425201 - xmlHttpRequest does not work with file://

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Armin Müller]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b5pre) Gecko/2008032605 Minefield/3.0b5pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b5pre) Gecko/2008032605 Minefield/3.0b5pre

The following code snipped brings a security error

var req = new XMLHttpRequest();
req.open('GET', 'file:///home/user/file.json', false); 
req.send(null);
if(req.status == 0)
  dump(req.responseText);

Reproducible: Always

Steps to Reproduce:
1.
2.
3.
Actual Results:  
Does not work

Expected Results:  
Should work

Comment 1

[Armin Müller]

This function call was fired from a locally started html file 

Comment 2

[Matthias Versen [:Matti]]

This bug is incomplete.

>The following code snipped brings a security error
copy the security error message ?

Do you load the example also as file:// or from a http server ?
Do you mean that this is not a dupe of bug 424875 and why (you must have found this because the bug is very easy to find) ?

Comment 3

[Jesse Ruderman]

Where does your HTML file reside?  Is it in the same directory as file.json or a different directory?

Comment 4

[Armin Müller]

(In reply to comment #2)
> copy the security error message ?
Permission denied to call method XMLHttpRequest.open

> Do you load the example also as file:// or from a http server ?
it is loaded a local file

> Do you mean that this is not a dupe of bug 424875 and why (you must have found
> this because the bug is very easy to find) ?
no, because i got a security message

I have attached an example. Please start the mapview/index.hml locally. Click on one of the points on the map. You will get an alert which shows the path to the xml file which should be loaded and then the error comes up. You will find the code in mapview/embfiles/testit.js

Comment 5

[Armin Müller]

Attachment: testcase for this bug

Comment 6

[Foteos Macrides]

(In reply to comment #2)
> Do you load the example also as file:// or from a http server ?
> Do you mean that this is not a dupe of bug 424875 and why (you must have found
> this because the bug is very easy to find) ?

Bug 42485 was a feature request to permit access to HTTP URLs when using XMLHttpRequest via local files.  An aspect of the Fx3.0b4+ behavior when an HTTP URL is used via a local file is a regression, and so bzbarsky added that keyword, but the bug, itself, requests a feature which has never existed in Fx.  The present bug deals with accessing other local files when using XMLHttpRequest via a local file, which has long been possible in Fx, through Fx3.0b4, but is no longer possible in Minefield, and now instead generates an access violation error.  So this bug appears to be fully a regression.

Comment 7

[Foteos Macrides]

(In reply to comment #6)
> Bug 42485 was a feature request to permit access to HTTP URLs when using
> XMLHttpRequest via local files.

I of course was referring to Bug 424875 (sorry that I'm so typo-prone :-).

Comment 8

[Foteos Macrides]

This regression is present in today's Fx3.0b5 release, but was not present when I filed Bug 424875 on 2008-03-24, so it occurred at some point between then and the Beta 5 freeze.

Comment 9

[Boris Zbarsky [:bzbarsky]]

This is in the wrong product, folks.

Comment 10

[Boris Zbarsky [:bzbarsky]]

So what's happening here is that the CheckConnect() call that nsXMLHttpRequest::Open() makes is failing.  The reason it fails is that it calls through to CheckPropertyAccessImpl(), which does a principal compare with aCheckConnect set to true.  This fails because of the changes for bug 402983.  IT didn't show up immediately, presumably, because we still allowed cross-site XHR at that point.

I think CheckPropertyAccessImpl() should probably be calling nsIPrincipal::CheckMayLoad() in the special case when it's got a target URI instead of doing a hacked-up CheckSameOriginDOMProp on a principal it synthesizes on the spot.  If we do that we might be able to get rid of the aCheckConnect stuff in CheckSameOriginDOMProp too.

Comment 11

[Damon Sicore (:damons)]

+'ing this.  Let's get a resolution.  Assigning to Jonas.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So I wonder if the correct fix here is to use nsIPrincipal::CheckMayLoad and then make that function allow files to load files from subdirectories. I.e. teach it about the new file policy. Seems like that is something we'd want for other callers of CheckMayLoad.

Comment 13

[Boris Zbarsky [:bzbarsky]]

CheckMayLoad already knows about the new file policy.  We needed that to fix various test failures when the new file policy landed.  Yay for tests.  ;)

We could just switch the nsXMLHttpRequest callsite to CheckMayLoad, though that will make it impossible to change the setting on a per-site basis via CAPS prefs.  That might be fine.  Just changing CheckPropertyAccessImpl is a safer fix from the XMLHttpRequest point of view, but we'd need to double-check that it's the right thing for the other URI callers of that method.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'd prefer to change nsXMLHttpRequest. I sort of think that the pref is bogus anyway (since it's real easy for a site to work around). And actually, won't things get disabled anyway since the site won't be able to access the .open property at all?

The whole CheckConnect API is really weird and ugly to me, I really think we should kill it.

And yes, yay for tests :)

Comment 15

[Boris Zbarsky [:bzbarsky]]

I think the idea of the pref was to white-list certain sites' ability to do cross-site XMLHttpRequest to arbitrary locations and to disable other sites' ability to open anything at all.  The former might be desirable, even, for intranet apps...

Comment 16

[Boris Zbarsky [:bzbarsky]]

But it's worth checking the CVS blame.  It's also possible that this was just a quick-and-dirty way to do the cross-site check...

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix

This fixes it by using CheckMayLoad rather than CheckConnect. I also realized that document.load suffered from the same problem and used the same fix there. Though document.load was using both CheckSameOrigin and CheckConnect, replaced both with a call to CheckMayLoad.

Once that was done I realized that there were no more users of CheckConnect so I killed that off.

I also added some error logging for chrome code no longer being able to use document.load to do cross-site loads. That's not strictly part of this bug, but since I was around there I figured we should do it. It's come up a few times in the forums already.

Not really sure how to use automated tests for this. I wrote some local tests, but they rely on being loaded from file so I can't add them to mochitest.

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix (with whitespace changes)

Previous patch was with -w for your reviewing pleasure. This one is not.

Comment 19

[Jeff Walden [:Waldo]]

Well, you could certainly create the file manually in the Mochitest and run from there; a pain, but it's doable.  We really should make this easier, tho, sometime soon.

Comment 20

[Boris Zbarsky [:bzbarsky]]

sicking, I assume you did do the CVS archeology to find why we were using this exact mechanism and it wasn't to allow whitelisting of sites?

You probably need to rev the iid.

I like the secman simplifications.... ;)

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

It was simply how the same-origin check was originally implemented (bug 47905). No word on why it was implemented this way rather than simply calling CheckSameOrigin. My guess in order to allow override through preferences.

Will rev the iid before checking in.

Comment 22

[Daniel Veditz [:dveditz]]

Comment on attachment 314295 [details] [diff] [review]
Patch to fix

>     PRBool crossSiteAccessEnabled;
>     rv = secMan->IsCapabilityEnabled("UniversalBrowserRead",
>                                      &crossSiteAccessEnabled);
>-    if (NS_FAILED(rv)) return rv;
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>     if (crossSiteAccessEnabled) {
>       mState |= XML_HTTP_REQUEST_XSITEENABLED;

Seems like this ought to be nsContentUtils::IsCallerTrustedForRead(), which covers the UniversalXPConnect case too. Existing bug, but might want to fix it while you're here.

Would also simplify the above into two lines, although the behavior would be slightly different in the extremely unlikely case secMan returns an error: we'd assume FALSE and continue on instead of having XHR.open() throw as currently.

<tangent>
I don't like the name "CheckMayLoad". It makes me think of "CheckLoadURI" but it has completely different meaning, the behavior is more like CheckSameOrigin().
</tangent>

What about redirects? I guess we don't have to worry about a file:// redirecting to another file://, and the existing CheckSameOrigin checks in both XHR and XMLDocument will catch http->file redirects. Alrighty, sold.

r/sr=dveditz with the IID change, with or without the IsCallerTrustedForRead() simplification.

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in with IsCallerTrustedForRead simplification.

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Sort of test

This is what I used for testing. The outer test.html file should succeed, where as both code blocks in the inner should fail.

Comment 25

[Fabien Tassin]

webservices no longer build since the patch landed:

g++-4.2 -o nsHTTPSOAPTransport.o -c -I../../../../dist/include/system_wrappers -include ../../../../config/gcc_hidden.h -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_COM_OBSOLETE -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DOSTYPE=\"Linux2.6\" -DOSARCH=Linux  -I. -I. -I../../../../dist/include/xpcom -I../../../../dist/include/string -I../../../../dist/include/caps -I../../../../dist/include/dom -I../../../../dist/include/js -I../../../../dist/include/xpconnect -I../../../../dist/include/necko -I../../../../dist/include/xmlextras -I../../../../dist/include/content -I../../../../dist/include/layout -I../../../../dist/include/widget -I../../../../dist/include   -I../../../../dist/include/websrvcs -I/usr/include/nspr   -I/usr/include  -I../../../../dist/sdk/include    -fPIC   -fno-rtti -fno-exceptions -Wall -Wconversion -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-long-long -pedantic -g -fno-strict-aliasing -fshort-wchar -pthread -pipe  -DNDEBUG -DTRIMMED -Os -freorder-blocks -fno-reorder-functions -finline-limit=50   -DMOZILLA_CLIENT -include ../../../../mozilla-config.h -Wp,-MD,.deps/nsHTTPSOAPTransport.pp nsHTTPSOAPTransport.cpp
nsHTTPSOAPTransport.cpp: In function 'nsresult GetTransportURI(nsISOAPCall*, nsAString_internal&)':
nsHTTPSOAPTransport.cpp:162: error: 'class nsDerivedSafe<nsIScriptSecurityManager>' has no member named 'CheckConnect'
nsHTTPSOAPTransport.cpp:177: error: 'class nsDerivedSafe<nsIScriptSecurityManager>' has no member named 'CheckConnect'
make[6]: *** [nsHTTPSOAPTransport.o] Error 1
make[6]: Leaving directory `/src/buildbot/xulrunner-1.9-1.9~cvs20080409t0027/mozilla/extensions/webservices/soap/src'

Comment 26

[Foteos Macrides]

This patch appears to have broken the ability to do a try() / catch(e) for the XHR open() successfully.  If a local file attempts an XHR open() for an http URL, both Fx2 and Minefield fail (see Bug 424875), but Fx2 (as well as Fx3b5 and Minefield before this patch) triggers the catch with the reason (a Security Error), whereas Minefield now does not and thus proceeds to an uncaught exception.

Comment 27

[Boris Zbarsky [:bzbarsky]]

>+   NS_ENSURE_SUCCESS(rv, NS_OK);

This is probably wrong.  It should return rv instead, no?

Comment 28

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I thought that was weird, but here's what the old code did:

rv = secMan->CheckConnect(cx, targetURI, "XMLHttpRequest", "open");
if (NS_FAILED(rv))
{
  // Security check failed.
  return NS_OK;
}

And that has been there for a looong time. I figured that people were relying on .send throwing after a "failed" .open?

Can someone provide a testcase so we can test FF2 behavior. I really don't want to change this from how FF2 behaved this late in the game.

Comment 29

[Boris Zbarsky [:bzbarsky]]

You can use the "replacement testcase" in bug 424875 to test this.  If open() throws, it'll alert that cross-site requests are not allowed.  Otherwise send() will throw.  In Gecko 1.8 you get the alert.

The point is that CheckConnect called CheckSameOriginDOMProp, which explicitly throws an exception on the JSContext it's given if the check fails.  So the code was returning NS_OK so that xpconnect wouldn't clobber that exception.  The new code, however, needs to return an error code so that XPConnect will throw the exception.

Comment 30

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Aah, i see

Comment 31

[Foteos Macrides]

(In reply to comment #29)
> The new code, however, needs to return an error code so that XPConnect will
> throw the exception.

Why is this bug still marked RESOLVED FIXED?  Shouldn't it be re-open()-ed (pun intended) until the failure to throw the exception is dealt with?

Comment 32

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Followup fix

So this puts back the CheckConnect API :(

Turns out that not only does the SOAP code use this API, they've also been advertising how to set the prefs in order to allow cross-site calls. So just switching SOAP over to CheckMayLoad is likely going to break people that have mucked around with the prefs.

The patch also makes .open throw for failed cross-site calls.

Comment 33

[Daniel Veditz [:dveditz]]

Comment on attachment 315625 [details] [diff] [review]
Followup fix

r/sr=dveditz

Comment 34

[Boris Zbarsky [:bzbarsky]]

Ugh.  Can we file a separate bug to remove it early in the post-1.9 phase of things?  It's just making things more confusing in already-complex code, and honestly I don't think we'll need to cater to SOAP at that point: they can figure out some other way of subverting the same-origin policy if they really need it.

Comment 35

[Steven Michaud [:smichaud] (Retired)]

When the new patch lands, please add a warning (near the UUID in
nsIScriptSecurityManager.idl) about the need to notify the author of
the Java Embedding Plugin whenever a change is made to the
nsIScriptSecurityManager interface that requires its IID to be
changed.  Probably the best way to do this is add the new patch's bug
number to the dep list of bug 293973 ("Metabug to follow CAPS changes
relevant to the Java Embedding Plugin"), together with some kind of
comment in that bug.

The JEP needs to call nsIScriptSecurityManager::GetSubjectPrincipal()
to support JavaScript-to-Java LiveConnect.  So every change to the
nsIScriptSecurityManager interface (big enough to change its IID) also
breaks JavaScript-to-Java LiveConnect.

(I'm the author of the Java Embedding Plugin, which for a couple of
years has been bundled with OS X distros of every Mozilla.org
browser.)

There are other interface changes that would also break the JEP (for
example a change to nsIPrincipal that changed GetOrigin(), or its
location in the vtable).  But it's changes to nsIScriptSecurityManager
that have occurred the most often, and have therefore caused the most
trouble.

Something like this has already been suggested by bzbarsky (bug 408329
comment #30) and sicking (bug 428606 comment #15).

Comment 36

[Steven Michaud [:smichaud] (Retired)]

By the way, I'm very grateful that the current patch, when it restores
CheckConnect(), goes back to the previous UUID (rather than creating a
new one).  It's just possible this will allow me to get away without
having to release yet another version (0.9.6.5) of the JEP (just after
the one I released last week (0.9.6.4) to catch up with the changes
from January til the dropping of CheckConnect()).

Comment 38

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Followup fix with test

Same as above but with a simple test

Comment 39

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 316312 [details] [diff] [review]
Followup fix with test

a1.9=beltzner

Comment 40

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in

Comment 41

[Steven Michaud [:smichaud] (Retired)]

> When the new patch lands, please add a warning (near the UUID in
> nsIScriptSecurityManager.idl) about the need to notify the author of
> the Java Embedding Plugin whenever a change is made to the
> nsIScriptSecurityManager interface that requires its IID to be
> changed.

Thanks, Jonas, for doing this in the patch you landed.  I appreciate
it.

Comment 42

[Boris Zbarsky [:bzbarsky]]

Need developer docs about the capability.policy thing no longer working in Gecko 1.9...

Comment 43

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

What needs documenting is that it is no longer possible to set

capability.policy.<yourpolicyhere>.XMLHttpRequest.open

to allAccess to give specific sites cross-site access.

Comment 45

[Carsten Book [:Tomcat]]

verified fixed using Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9pre) Gecko/2008042705 Minefield/3.0pre ID:2008042705 and the testcase from this bug.

--> Verified fixed

Comment 47

[William J. Edney]

Folks -

Sorry to bug you all, but I'm really confused here. I understand the change that was made in https://bugzilla.mozilla.org/show_bug.cgi?id=230606 (and some follow-on bugs) to restrict the 'same origin policy' for local files. My understanding was that by setting the 'security.fileuri.origin_policy' pref to '4' that I would allow the XMLHTTPRequest object to read files from any directory on the host system, including directory listings, as documented here: http://kb.mozillazine.org/Security.fileuri.origin_policy.

However, as of FF 3.0b5, I have to also set 'security.fileuri.strict_origin_policy' to 'false' in order for the same functionality to work. This is also true in FF 3.0RC1. Is this intended behavior?

Note that the XMLHTTPRequest is being made from script loaded from the file system itself.

Thanks in advance!

Cheers,

- Bill

Comment 48

[Boris Zbarsky [:bzbarsky]]

William, the file policy setup and the preferences that control it got changed in bug 402983.  So the mozillazine article is just out of date.

Comment 49

[William J. Edney]

Boris -

Thanks for the quick reply!

So, in other words, the old 'security.fileuri.origin_policy' pref is obsolete and has been replaced with the 'security.fileuri.strict_origin_policy' pref that simply takes 'true' or 'false' values, correct?

Cheers,

- Bill

Comment 50

[Boris Zbarsky [:bzbarsky]]

That's correct.

Comment 51

[Eric Shepherd [:sheppy]]

Added a note to:

https://developer.mozilla.org/En/Using_XMLHttpRequest