Last Comment Bug 627616 - font-face fonts not loaded over authenticating proxy
: font-face fonts not loaded over authenticating proxy
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: All All
: P1 major with 8 votes (vote)
: mozilla13
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
http://code.google.com/webfonts
: 638395 701019 715124 (view as bug list)
Depends on: 728790
Blocks: 715124 731442
  Show dependency treegraph
 
Reported: 2011-01-20 19:51 PST by Carey Evans
Modified: 2014-09-23 04:43 PDT (History)
22 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.x+


Attachments
Totally untested [Checked in: Comment 38] (2.91 KB, patch)
2011-02-01 18:37 PST, Boris Zbarsky [:bz]
honzab.moz: review+
Details | Diff | Splinter Review
Test for interaction of anonymous loads and proxy authentication. (7.44 KB, patch)
2012-01-08 12:43 PST, Josh Matthews [:jdm]
honzab.moz: review-
Details | Diff | Splinter Review
Test for interaction of anonymous loads and proxy authentication. (12.82 KB, patch)
2012-02-03 14:38 PST, Josh Matthews [:jdm]
honzab.moz: review-
Details | Diff | Splinter Review
Test for interaction of anonymous loads and proxy authentication [Checked in: Comment 38] (14.79 KB, patch)
2012-02-14 14:04 PST, Josh Matthews [:jdm]
honzab.moz: review+
Details | Diff | Splinter Review
(AAv1) Add missing text to is() calls [Checked in: Comment 42] (1.07 KB, patch)
2012-03-07 04:43 PST, Serge Gautherie (:sgautherie)
honzab.moz: review+
Details | Diff | Splinter Review

Description Carey Evans 2011-01-20 19:51:29 PST
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.
Comment 1 Carey Evans 2011-02-01 13:51:28 PST
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.
Comment 2 Boris Zbarsky [:bz] 2011-02-01 16:18:24 PST
> (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...
Comment 3 Jonas Sicking (:sicking) PTO Until July 5th 2011-02-01 16:22:54 PST
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.
Comment 4 Carey Evans 2011-02-01 16:31:41 PST
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.
Comment 5 Jonas Sicking (:sicking) PTO Until July 5th 2011-02-01 16:37:12 PST
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.
Comment 6 Jonas Sicking (:sicking) PTO Until July 5th 2011-02-01 16:40:05 PST
> 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).
Comment 7 Jonas Sicking (:sicking) PTO Until July 5th 2011-02-01 18:08:45 PST
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.
Comment 8 Boris Zbarsky [:bz] 2011-02-01 18:33:02 PST
> 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.
Comment 9 Boris Zbarsky [:bz] 2011-02-01 18:37:28 PST
Created attachment 508987 [details] [diff] [review]
Totally untested
[Checked in: Comment 38]

If someone has a good testing plan for this, let me know!
Comment 10 Honza Bambas (:mayhemer) 2011-02-02 12:42:23 PST
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.
Comment 11 Bjarne (:bjarne) 2011-02-07 12:59:12 PST
Assign to bz since he already made a patch and got it reviewed.
Comment 12 Boris Zbarsky [:bz] 2011-02-07 14:00:05 PST
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....
Comment 14 Sébastien Desvignes 2011-03-16 09:21:51 PDT
*** Bug 638395 has been marked as a duplicate of this bug. ***
Comment 15 Luis Villa 2011-08-22 11:18:53 PDT
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.
Comment 16 Jonas Sicking (:sicking) PTO Until July 5th 2011-08-22 11:34:22 PDT
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.
Comment 17 Boris Zbarsky [:bz] 2011-08-22 12:27:32 PDT
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... :(
Comment 18 Carlo Alberto Ferraris 2011-10-21 01:00:12 PDT
I still experience this same bug on Firefox 7.0.1
Comment 19 Boris Zbarsky [:bz] 2011-10-21 09:25:07 PDT
Yes, because this is not fixed.  If you want to help, write the tests...
Comment 20 Honza Bambas (:mayhemer) 2011-11-16 11:16:05 PST
Isn't this another dup of bug 701019 ?
Comment 21 Boris Zbarsky [:bz] 2011-11-16 12:56:56 PST
Looks like it, albeit with patch.
Comment 22 Josh Matthews [:jdm] 2012-01-08 12:43:09 PST
Created attachment 586828 [details] [diff] [review]
Test for interaction of anonymous loads and proxy authentication.
Comment 23 Honza Bambas (:mayhemer) 2012-01-18 07:40:22 PST
*** Bug 701019 has been marked as a duplicate of this bug. ***
Comment 24 Honza Bambas (:mayhemer) 2012-01-19 09:34:58 PST
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 25 Josh Matthews [:jdm] 2012-02-03 14:38:38 PST
Created attachment 594304 [details] [diff] [review]
Test for interaction of anonymous loads and proxy authentication.
Comment 26 Honza Bambas (:mayhemer) 2012-02-10 09:53:47 PST
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.
Comment 28 Phil Ringnalda (:philor, back in August) 2012-02-12 10:56:37 PST
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.
Comment 29 Honza Bambas (:mayhemer) 2012-02-14 07:35:33 PST
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.
Comment 30 Josh Matthews [:jdm] 2012-02-14 07:55:49 PST
Ouch. Please know that was not intentional; I made all of the changes, but somehow didn't push the updated version.
Comment 31 Josh Matthews [:jdm] 2012-02-14 14:04:41 PST
Created attachment 597181 [details] [diff] [review]
Test for interaction of anonymous loads and proxy authentication
[Checked in: Comment 38]

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.
Comment 32 Mozilla RelEng Bot 2012-02-14 14:45:21 PST
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
Comment 33 Honza Bambas (:mayhemer) 2012-02-15 06:59:11 PST
(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 34 Honza Bambas (:mayhemer) 2012-02-15 07:08:50 PST
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") ?
Comment 35 Honza Bambas (:mayhemer) 2012-02-15 07:10:04 PST
(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?
Comment 36 Josh Matthews [:jdm] 2012-02-15 08:32:38 PST
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.
Comment 39 RNicoletto 2012-02-17 13:32:21 PST
*** Bug 723482 has been marked as a duplicate of this bug. ***
Comment 40 Serge Gautherie (:sgautherie) 2012-03-07 04:43:20 PST
Created attachment 603676 [details] [diff] [review]
(AAv1) Add missing text to is() calls
[Checked in: Comment 42]

Noticed while checking bug 731442.
Comment 41 Serge Gautherie (:sgautherie) 2012-03-07 07:00:09 PST
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 42 Serge Gautherie (:sgautherie) 2012-03-07 13:17:26 PST
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
Comment 43 bugzilla_moz.20.ibyte 2012-03-23 03:51:40 PDT
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?
Comment 44 Jonathan Kew (:jfkthame) 2012-03-23 04:17:54 PDT
(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.
Comment 45 bugzilla_moz.20.ibyte 2012-03-23 05:34:20 PDT
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.
Comment 46 Josh Matthews [:jdm] 2012-04-11 11:33:50 PDT
*** Bug 715124 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.