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

This function call was fired from a locally started html file

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) ?

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

(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

(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.

(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 :-).

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.

This is in the wrong product, folks.

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.

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

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.

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.

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 :)

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...

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...

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.

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

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.

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.... ;)

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 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.

Checked in with IsCallerTrustedForRead simplification.

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

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'

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.

>+ NS_ENSURE_SUCCESS(rv, NS_OK); This is probably wrong. It should return rv instead, no?

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.

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.

Aah, i see

(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?

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 on attachment 315625 [details] [diff] [review] Followup fix r/sr=dveditz

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.

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).

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()).

Same as above but with a simple test

Comment on attachment 316312 [details] [diff] [review] Followup fix with test a1.9=beltzner

Checked in

> 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.

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

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.

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

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

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.

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

That's correct.

Added a note to: https://developer.mozilla.org/En/Using_XMLHttpRequest