User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b9) Gecko/20100101 Firefox/4.0b9 Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b9) Gecko/20100101 Firefox/4.0b9 Most sites using font-face fonts do not work for me in Firefox 4.0b9 at work, behind an authenticating proxy using NTLM. This includes the Google Font Directory, typekit.com and webfonts.fonts.com. A few sites work, like http://openfontlibrary.org/files/fuex/279. If I configure Firefox to use Basic authentication instead of NTLM, via network.automatic-ntlm-auth.allow-proxies, it still fails to display the fonts. If I configure Firefox to use NTLMAPS, which does not require authentication, the fonts are displayed correctly. Reproducible: Always Steps to Reproduce: 1. Configure Firefox to use a proxy with Basic or NTLM authentication. 2. Load http://code.google.com/webfonts, http://webfonts.fonts.com/Project/ChooseFonts?fontQuery=frutiger, or another page using font-face fonts. Actual Results: Text configured to use font-face fonts is displayed in Times. Expected Results: Text should use the correct font, like Chrome, Safari, IE, or Firefox without a proxy. The proxy server is a Blue Coat proxy, but this doesn't seem to make any difference.

For what it's worth, I'm speculating that this is caused because (a) web fonts are loaded as cross domain requests, (b) cross domain requests use LOAD_ANONYMOUS, and (c) LOAD_ANONYMOUS prevents authentication to a proxy server. Web fonts are just a very visible result of this.

> (b) cross domain requests use LOAD_ANONYMOUS Only if you pass PR_FALSE for aWithCredentials (or if it's a preflight, which it never is for fonts). Jonas, what is that boolean meant to do? Should the font code be passing false or true? This stuff seems to be completely undocumented. Is there a bug on the fact that preflights and the like don't work through an authenticating proxy, by the way? That seems seriously broken too...

I would say that yes, aWithCredentials should be PR_FALSE. But yes, it needs to be put into the @font-face spec. The LOAD_ANONYMOUS flag is documented here: http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIRequest.idl#216 If setting that flag causes us to fail proxy authentication then that's a bug.

I based my comment on this post: http://adblockplus.org/blog/why-you-do-not-want-to-use-the-load_anonymous-flag which is talking about these changes: https://hg.adblockplus.org/adblockplus/rev/b15a8850e9cb https://hg.adblockplus.org/adblockplus/rev/17a4ed43b108 I don't see a bug here about it, though.

Sorry, should be more clear. (In reply to comment #2) > > (b) cross domain requests use LOAD_ANONYMOUS > > Only if you pass PR_FALSE for aWithCredentials (or if it's a preflight, which > it never is for fonts). > > Jonas, what is that boolean meant to do? Setting that boolean to false is meant to set the LOAD_ANONYMOUS flag on the actual request, as well as allow wildcards in the Access-Control-Allow-Origin response header. In other words, it's supposed to run the CORS spec with the "supports credentials" flag set to false. > Should the font code be passing > false or true? This stuff seems to be completely undocumented. It should IMHO be set to PR_FALSE. Though this needs to be documented by the @font-face spec. > Is there a bug on the fact that preflights and the like don't work through an > authenticating proxy, by the way? I think this bug is that bug. > That seems seriously broken too... Why the "too". It's the only thing that is broken here AFAICT.

> http://adblockplus.org/blog/why-you-do-not-want-to-use-the-load_anonymous-flag Too bad no-one filed a bug on that. It wasn't known to anyone with the ability to fix it. At least we have a bug now (this bug).

Given that we've had this behavior for a while, and this is the first we hear of it, I'd say that this shouldn't block FF4. We should fix it asap after though. Bjarne: Could you have a look at this? You helped out a lot with the LOAD_ANONYMOUS implementation.

> In other words, it's supposed to run the CORS spec with the "supports > credentials" flag set to false. Can we get that documented in the code, please? > If setting that flag causes us to fail proxy authentication then that's a bug. Well... Is it? LOAD_ANONYMOUS says to send no credentials... why are proxy credentials special? That said, biesi also thinks LOAD_ANONYMOUS should not affect proxy auth. So we should fix that. Right not nsHttpChannelAuthProvider bails early if LOAD_ANONYMOUS; presumably we need to not do that if mProxyAuth in the ProcessAuthentication case and doing the proxy thing in AddAuthorizationHeaders even if LOAD_ANONYMOUS.

If someone has a good testing plan for this, let me know!

Comment on attachment 508987 [details] [diff] [review] Totally untested [Checked in: Comment 38] Yes, this could work. A test for this is a must before we land it.

Assign to bz since he already made a patch and got it reviewed.

Bjarne, my problem is that I have no idea how to test this. And the review is conditional on tests. I was sort of hoping you had some idea how to test proxy stuff....

We may use these test files: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/test/authenticate.sjs and http://mxr.mozilla.org/mozilla-central/source/content/base/test/test_bug466080.html?force=1

For what it is worth, this still seems to be broken in FF9 nightlies; at least, I'm seeing symptoms identical to those described by other people in this bug; notably http://openfontlibrary.org/fonts works and Google Web Fonts don't, and turning off my proxy fixes the problem.

Josh: Do we have any way to test these things with necko-net? Or do we have any other ways to test this? If not, I think we need to just check this in.

Actually, Honza sent me some info on testing this back in March: --------------------------------------------------------------------- authenticate.sjs works the following way: - When you send it proxy_user/proxy_pass/proxy_realm arguments, it will check for the Proxy-Authorization header If found and proxy_user/proxy_pass match what sent in the header -> go to check for user auth Otherwise, fail with 407 check for user auth : - When you send it user/pass/realm, it will check for the Authorization header If found and user/pass match -> go to auth check passed Otherwise, fail with 401 auth check passed: - Send 200 What you need for the test is to deploy an anonymous request [1] to authenticate.sjs and set only proxy_user/proxy_pass/proxy_realm arguments to it to check the bug has been fixed. You should also check there is no regression in the user "anonymization" path - i.e. when you instruct authenticate.sjs to also expect the Authorization header by sending it _also_ user/pass/realm, it must fail - return 401. You will probably need to modify authenticate.sjs by adding a new argument to not fail with 401 in that case, but just send 200 OK with positive report of "Authorization header NOT FOUND". Otherwise your test will open the auth dialog and stuck. To prevent auth dialogs, you have to pre-fill the auth cache with desired credentials. Good example is at [2]. [1] http://mxr.mozilla.org/mozilla-central/source/content/base/test/test_bug466080.html?force=1#98 [2] http://hg.mozilla.org/mozilla-central/diff/d6895cb5ac3b/toolkit/components/passwordmgr/test/test_prompt_async.html#l1.81 --------------------------------------------------------------------- You have to prefill the auth cache using code like this: http://hg.mozilla.org/mozilla-central/diff/d6895cb5ac3b/toolkit/components/passwordmgr/test/test_prompt_async.html#l1.81 Mochitests all go thought a proxy (moz-proxy://127.0.0.1:8888/ I believe). If you prefill the auth cache with entry for the proxy, then the first request will contain Proxy-Authorization header filled with the credentials from the auth cache. --------------------------------------------------------------------- I just haven't had a chance to go sort through that stuff yet and write the tests... :(

I still experience this same bug on Firefox 7.0.1

Yes, because this is not fixed. If you want to help, write the tests...

Isn't this another dup of bug 701019 ?

Looks like it, albeit with patch.

Comment on attachment 586828 [details] [diff] [review] Test for interaction of anonymous loads and proxy authentication. Review of attachment 586828 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the test. r-, see the comments please. Please update also browser/base/content/test/authenticate.sjs. The files are currently identical. We should have a bug to make the file shared. Part of the test must also be check the user credentials had really been added to the cache and got sent to the server when the LOAD_ANONYMOUS flag had not been set. And this test must be the first to put the credentials cached in the login manager to the http auth cache (to actually call nsHttpAuthCache::SetAuthEntry). I also found a bug in the current implementation of authenticate.sjs. It fills the |realm| state variable (the user auth realm) with value of proxy_realm= param. match = /realm=([^&]*)/.exec(query); has to be fixed to match = /[^_]realm=([^&]*)/.exec(query); This actually applies to all user=, pass=, auth= arguments parsing. Locally tested. ::: toolkit/components/passwordmgr/test/Makefile.in @@ +75,5 @@ > test_bug_360493_2.html \ > test_bug_391514.html \ > test_bug_427033.html \ > test_bug_444968.html \ > + test_bug627616.html \ Maybe keep the name consistent? ::: toolkit/components/passwordmgr/test/authenticate.sjs @@ +127,5 @@ > response.setStatusLine("1.0", 401, "Authentication required"); > for (i = 0; i < authHeaderCount; ++i) > response.setHeader("WWW-Authenticate", "basic realm=\"" + realm + "\"", true); > + } else if (requestAuth && missingAuth) { > + response.setStatusLine("1.0", 200, "Authorization header not found"); This code you've added is checking something else that is necessary for checking the fix here. What we need to check is that there is no Authorization header present. What you check is that it is OK for the headers value be either not present or present with a wrong value... ::: toolkit/components/passwordmgr/test/test_bug627616.html @@ +9,5 @@ > +<body> > +<script class="testbody" type="text/javascript"> > + SimpleTest.waitForExplicitFinish(); > + > + netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect'); New test are supposed to not use UniversalXPConnect. If that is not that much work, you should expose these APIs you need here in the super-power utilities. But I think we can make this in a followup since there are other tests with UniversalXPConnect in this directory. @@ +29,5 @@ > + login.init(mozproxy, null, "proxy_realm", "proxy_user", "proxy_pass", "", ""); > + pwmgr.addLogin(login); > + > + login2 = Cc["@mozilla.org/login-manager/loginInfo;1"].createInstance(Ci.nsILoginInfo); > + login2.init("http://mochi.test:8888", null, "mochirealm", "user1name", "user2name", "", ""); s/user2name/user1pass/ ? Clearly shows there is something not right with this test ;)

Comment on attachment 594304 [details] [diff] [review] Test for interaction of anonymous loads and proxy authentication. Review of attachment 594304 [details] [diff] [review]: ----------------------------------------------------------------- Thanks r=honzab, please check the comments. ::: browser/base/content/test/authenticate.sjs @@ +68,5 @@ > > + // missingAuthorization=1 > + match = /missingAuthorization=1/.exec(query); > + if (match) > + missingAuth = true; Feel free to oppose, but I'd rename 'missingAuthorization' to 'anonymous', better says what we want to do here. Also 'missingAuth' can be renamed to 'anonymous'. @@ +135,3 @@ > } else { > response.setStatusLine("1.0", 200, "OK"); > } I'd do this a different way, the conditions are quit cryptic this way: if (anonymous) { if (authPresent) "400 Unexpected auth header" else "200 Auth not found" } else { // !anon if (requestAuth) "401 Auth req" else "200 OK" } ::: toolkit/components/passwordmgr/test/test_bug_627616.html @@ +44,5 @@ > + "proxy_realm=proxy_realm&" + > + "user=user1name&" + > + "pass=user1pass&" + > + "realm=mochirealm&" + > + extra); extra || "" @@ +94,5 @@ > + var dialog = doc.getElementById("commonDialog"); > + dialog.acceptDialog(); > + if (testNum == 1) { > + startCallbackTimer(); > + testNum++; You are just incrementing the arg, not the global var. I'd rather compare to the test func that is currently running. Just save result of shift() in runNextTest() to a global var and check on it here. That will also make well visible during which test(s) we expect and accept a dialog.

http://hg.mozilla.org/integration/mozilla-inbound/rev/13c3c54d067b http://hg.mozilla.org/integration/mozilla-inbound/rev/be556c99ef81

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/24ff86e2dab2 - other than the failures and crashes in M2, M5, browser-chrome and ipcplugins, it went fairly well.

Josh, you completely ignored my review comments. When I give 'r+ with comments' I mean to address them before landing or at least make a comment you oppose and why. Please update the patches before next landing and let me take a look.

Ouch. Please know that was not intentional; I made all of the changes, but somehow didn't push the updated version.

The try run is looking good. The const => var change in prompt_common.js is needed so I can override Cc in the new test as part of avoiding UniversalXPConnect.

Try run for 12dcfe95b4d6 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=12dcfe95b4d6 Results (out of 48 total builds): success: 41 warnings: 7 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/josh@joshmatthews.net-12dcfe95b4d6

(In reply to Josh Matthews [:jdm] from comment #30) > Ouch. Please know that was not intentional; I made all of the changes, but > somehow didn't push the updated version. This happened to me ones my self. It's just about to be more careful ;)

Comment on attachment 597181 [details] [diff] [review] Test for interaction of anonymous loads and proxy authentication [Checked in: Comment 38] Review of attachment 597181 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, this is it. r=honzab ::: browser/base/content/test/authenticate.sjs @@ +16,5 @@ > // Allow the caller to drive how authentication is processed via the query. > // Eg, http://localhost:8888/authenticate.sjs?user=foo&realm=bar > + // The extra ? allows the user/pass/realm checks to succeed if the name is > + // at the beginning of the query string. > + var query = "?" + request.queryString; Good catch. @@ +123,5 @@ > } > > + if (anonymous) { > + if (authPresent) { > + response.setStatusLine("1.0", 401, "Unexpected authorization header found"); This should return 400, definitely not 401. ::: toolkit/components/passwordmgr/test/test_bug_627616.html @@ +97,5 @@ > + { > + var dialog = doc.getElementById("commonDialog"); > + dialog.acceptDialog(); > + if (gCurrentTest == testNonAnonymousCredentials) { > + startCallbackTimer(); Nit: shouldn't there be an else branch with ok(false, "unexpected dialog") ?

(In reply to Phil Ringnalda (:philor) from comment #28) > Backed out in > https://hg.mozilla.org/integration/mozilla-inbound/rev/24ff86e2dab2 - other > than the failures and crashes in M2, M5, browser-chrome and ipcplugins, it > went fairly well. Josh, have you figured this out already?

Yes, the test failures were due to the [^_] changes to the regex which failed without adding the ? to the string, for the reasons described. >::: toolkit/components/passwordmgr/test/test_bug_627616.html >@@ +97,5 @@ >> + { >> + var dialog = doc.getElementById("commonDialog"); >> + dialog.acceptDialog(); >> + if (gCurrentTest == testNonAnonymousCredentials) { >> + startCallbackTimer(); > >Nit: shouldn't there be an else branch with ok(false, "unexpected dialog") ? Nope; handleDialog is only called as a one-time response from the timer that is set in startCallbackTimer.

http://hg.mozilla.org/integration/mozilla-inbound/rev/d3a34ad0a76c http://hg.mozilla.org/integration/mozilla-inbound/rev/c4794b033b7e

https://hg.mozilla.org/mozilla-central/rev/d3a34ad0a76c https://hg.mozilla.org/mozilla-central/rev/c4794b033b7e

Noticed while checking bug 731442.

Comment on attachment 603676 [details] [diff] [review] (AAv1) Add missing text to is() calls [Checked in: Comment 42] Succeeded as https://tbpl.mozilla.org/?tree=Try&rev=7d681ec6adeb

Comment on attachment 603676 [details] [diff] [review] (AAv1) Add missing text to is() calls [Checked in: Comment 42] https://hg.mozilla.org/mozilla-central/rev/974e7a3031f3

It looks like this also breaks file uploads to Google Docs. It does work in Chrome when testing from the same location. And I'm not sure whether it was mentioned previously, but it also breaks Google Maps overlays (the drop-shaped location markers). Tested in Fx11.0 Release (Mozilla/5.0 (Windows NT 5.1; rv:11.0) Gecko/20100101 Firefox/11.0). I haven't tested the Nightly builds. Are these issues addressed by the fix?

(In reply to bugzilla_moz.20.ibyte from comment #43) > It looks like this also breaks file uploads to Google Docs. It does work in > Chrome when testing from the same location. And I'm not sure whether it was > mentioned previously, but it also breaks Google Maps overlays (the > drop-shaped location markers). > > Tested in Fx11.0 Release (Mozilla/5.0 (Windows NT 5.1; rv:11.0) > Gecko/20100101 Firefox/11.0). I haven't tested the Nightly builds. Are these > issues addressed by the fix? I have no idea whether those are really the same underlying issue. I'd suggest you test in Nightly (FF14) or Aurora (FF13) to see if the fix here has resolved them, and if not, file a new bug with details.

I have tested it and can confirm that these issues are resolved in Aurora 13.0a2 (Mozilla/5.0 (Windows NT 5.1; rv:13.0) Gecko/20120322 Firefox/13.0a2). Thanks guys.