User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.4) Gecko/20070605 Firefox/2.0.0.4 Flock/0.8.99 Build Identifier: To provide some convenience for app or extension code doing requests in the background without user intervention, I propose the following: 1. Do not attach to a window's load group, so requests aren't cancelled if the window closes (often the window isn't really associated with the request anyway) 2. Do not get an nsIAuthPrompt by default, since we don't want auth dialogs to pop up randomly. 3. Provide an nsIBadCertListener implementation, again to prevent bad cert dialogs to pop up randomly. 4. Provide a mechanism to set an arbitrary string to be sent as an HTTP header or perhaps in the UA, so servers have an idea what components or extensions are causing requests. We've done #1 and #2 already in the Flock tree. Reproducible: Always

To clarify, #1-3 would be turned on via a single boolean flag in the interface. In Fow now, in Flock I've called the flag backgroundRequest.

In light of bug #327181, I don't think #3 is necessary anymore.

Are we talking about enabling this for content? Or just for chrome?

chrome only

Yeah, chrome only. #2 and #4 *may* be useful in light of cross-site XHR support, but let's just get something hashed out for chrome users first.

You should be able to do 4 already using the normal SetRequestHeader API. Code with chrome access should be able to set the UA header.

Spun off bug #405904 to cover #4.

And #3 is relevant again with current trunk UI, as dialogs do pop up for bad certs.

This implements #1-3, and makes the extension manager and update service use it. For the bad cert stuff, I turned toolkit/mozapps/shared/src/badCertHandler.js into a JS component, and XMLHttpRequest references that accordingly, because content can't (won't?) depend on security interfaces. If the component isn't there, it falls back to the current behavior. Extension manager and update server still use badCertHandler.js for the checkCert function, perhaps nsBadCertHandler.js could implement an nsIBadCertHandler interface, which provides checkCert? Then the JS #include could go away entirely. Given that there are already 3 users in the tree, it wouldn't be a big stretch to think other users could benefit.

Oh, and currently badCertHandler.js doesn't implement nsIBadCertListener2, so it's not doing its full job anymore.

I forgot to limit it to chrome context only, plus I limited setting the flag to before open() is called, not because it's currently required, but just to give the freedom to apply behavior that might require that in the future.

Doh, I changed the rules of when backgroundRequest could be set without updating the callers, here's a fixed version.

Hmm.. with so little code left, does it really make sense to write the badcerthandler in javascript? Seems simpler to just write C++ and put it in nsXMLHttpRequest.cpp Sorry, didn't think about that until now :( Also, I'm thinking we should really have some security folks look at this. I don't really know certificate handling and since it's pretty important stuff I wanna make sure we do the right thing.

Yeah, it still needs to be in JS. I originally wanted to do it all in C++ (even before, the code was not that complex), but the security IDLs get built long after layout, and the source even *depends* on layout and dom, so keeping it in JS avoids making a circular compile time dependency. nsIBadCertListener2 and nsISSLErrorListener are single function interfaces, and the docs clearly state that returning true suppresses error dialogs but still cancel the request. http://mxr.mozilla.org/firefox/source/security/manager/ssl/public/nsIBadCertListener2.idl#59 http://mxr.mozilla.org/firefox/source/security/manager/ssl/public/nsISSLErrorListener.idl#56 Flagging dveditz for review, since the cert stuff is based off his work.

I really like the idea of this patch; should make things a fair bit easier for a lot of extension authors.

Comment on attachment 304923 [details] [diff] [review] New patch that incorporates changes discussed on IRC r=dveditz on the badcert part.

Comment on attachment 304923 [details] [diff] [review] New patch that incorporates changes discussed on IRC Dan, do we really want to just ignore any bad certificates? Shouldn't we refuse to load the resource in that case? Resetting review request to make sure it gets reviewed for that issue too.

We *are* refusing to load resources with bad certs. The difference between behavior of the code here and what's in UpdateService/EM is that the latter disallows user whitelisted certs. I filed bug #418992 for that functionality.

Comment on attachment 304923 [details] [diff] [review] New patch that incorporates changes discussed on IRC r=me on the non-cert stuff, sr=me for the whole thing.

Comment on attachment 304923 [details] [diff] [review] New patch that incorporates changes discussed on IRC a=shaver, but two things: 1) please get some tests for this into the tree ASAP; if it's important enough to get into b5, it's important enough to have test coverage 2) what effect will this have on content that uses .backgroundRequest as an expando, perhaps to track their own "background" sorts of requests? (My guess is that they get a cryptic security error now.) Perhaps .mozBackgroundRequest would be a better name, since the property isn't _useful_ unless you're privileged, and we have no intent to standardize.

Comment on attachment 304923 [details] [diff] [review] New patch that incorporates changes discussed on IRC No, I've changed my mind here -- I think we need to at least rename the property name to .mozBackgroundRequest to avoid changing content-visible behaviour between last beta and RC. Should be an easy fix, and I'll quickly approve the result.

I don't think it's that big a risk that someone will set expandos, but there's no harm in using a .moz prefix either, especially given that this is very unlikely to ever be used by web content. And definitely write some tests, sorry should have thought of that myself.

+ * The Original Code is the Update Service. + * + * The Initial Developer of the Original Code is Ben Goodger. + * Portions created by the Initial Developer are Copyright (C) 2004 did you really take enough code to merit this?

(In reply to comment #26) > + * The Original Code is the Update Service. > + * > + * The Initial Developer of the Original Code is Ben Goodger. > + * Portions created by the Initial Developer are Copyright (C) 2004 > > did you really take enough code to merit this? All that code is from toolkit/mozapps/shared/src/badCertHandler.js. The only bits I added were to turn it into a real component via XPCOMUtils.

(In reply to comment #25) > And definitely write some tests, sorry should have thought of that mysel So I'm not really sure how to test this. How would I go about writing a test to verify an auth prompt dialog or a security warning dialog *isn't* showing up? The load group stuff is easy enough to test though.

Add a listener that counts how many times it's called, and check that the count is zero when the request completes?

Ok, bug #422808 made the auth prompt a non-issue anyway. I'm not sure what the best way to trigger an invalid cert warning from the test framework.

Hm, I'm unable to get a cert warning dialog from within the mochitest framework at all. Same code called from within a normal browser instance throws up a dialog.

Ok, I've renamed the attribute to mozBackgroundRequest, and synced up the code with current CVS. As noted before, the auth prompt logic isn't needed anymore. I've written a test for loadGroup detachment, but I can't seem to even get a security dialog to trigger from mochitest land. Can this be landed for b5, with a test for the security dialog stuff happening later once we figure out what's going on there?

Comment on attachment 310343 [details] [diff] [review] New patch, with mozBackgroundRequest attribute a=shaver, please file a bug on the needed test infrastructure, and mark it blocking a bug that describes the specific tests you feel are missing.

Checking in content/base/public/nsIXMLHttpRequest.idl; /cvsroot/mozilla/content/base/public/nsIXMLHttpRequest.idl,v <-- nsIXMLHttpRequest.idl new revision: 1.39; previous revision: 1.38 done Checking in content/base/src/Makefile.in; /cvsroot/mozilla/content/base/src/Makefile.in,v <-- Makefile.in new revision: 1.121; previous revision: 1.120 done RCS file: /cvsroot/mozilla/content/base/src/nsBadCertHandler.js,v done Checking in content/base/src/nsBadCertHandler.js; /cvsroot/mozilla/content/base/src/nsBadCertHandler.js,v <-- nsBadCertHandler.js initial revision: 1.1 done Checking in content/base/src/nsXMLHttpRequest.cpp; /cvsroot/mozilla/content/base/src/nsXMLHttpRequest.cpp,v <-- nsXMLHttpRequest.cpp new revision: 1.229; previous revision: 1.228 done Checking in content/base/test/Makefile.in; /cvsroot/mozilla/content/base/test/Makefile.in,v <-- Makefile.in new revision: 1.63; previous revision: 1.62 done RCS file: /cvsroot/mozilla/content/base/test/test_bug383430.html,v done Checking in content/base/test/test_bug383430.html; /cvsroot/mozilla/content/base/test/test_bug383430.html,v <-- test_bug383430.html initial revision: 1.1 done Landed on trunk

Will file bugs about test framework. Marking bug as FIXED as code has landed.

updated http://developer.mozilla.org/en/docs/XMLHttpRequest#mozBackgroundRequest

Was it intended that doing a background request should always suppress auth prompts? If so that isn't working I realized. Making GetInterface not return an nsIAuthPrompt here simply means that we'll try to get one from the loadgroup, which in many cases will mean that the docshell will provide one. Although currently chrome docshells most of the time don't provide authprompts, except if a proxy requests authentication.

(In reply to comment #38) > Although currently chrome docshells most of the time don't provide authprompts, > except if a proxy requests authentication. Hm, since when you enable mozBackgroundRequest you don't get the document's loadgroup, would you still get a proxy auth dialog?

Ah, good point, no you don't

Does a normal Gecko app like FF or SeaMonkey need to ship nsBadCertHandler.js? Just wondering because this file isn't included in browser/installer/windows/packages-static or other packaging files.

Yes. If that's not shipped inside a jar or elsewhere that seems like a bug.

Should be simple enough to add to packages-static.. shall I open a new bug or reopen this one?

New bug, please.

Filed bug #452135.