Last Comment Bug 785259 - the override in CheckCertOverridesbits are not correctly cleared
: the override in CheckCertOverridesbits are not correctly cleared
Status: RESOLVED FIXED
[qa-]
:
Product: Core
Classification: Components
Component: Security: PSM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla18
Assigned To: Camilo Viecco (:cviecco)
:
:
Mentors:
: 418563 571034 (view as bug list)
Depends on: 820757
Blocks: pkix-default
  Show dependency treegraph
 
Reported: 2012-08-23 16:10 PDT by Camilo Viecco (:cviecco)
Modified: 2012-12-12 03:29 PST (History)
9 users (show)
ryanvm: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
+
fixed
+
fixed
fixed
wontfix


Attachments
Fix to the bug (1.15 KB, text/plain)
2012-08-23 16:10 PDT, Camilo Viecco (:cviecco)
no flags Details
Fix to the bug (1.15 KB, patch)
2012-08-23 16:11 PDT, Camilo Viecco (:cviecco)
brian: review+
honzab.moz: superreview+
Details | Diff | Splinter Review
fix-to-commited patch. Use bitwise NOT instead of logic NOT (1.15 KB, patch)
2012-09-27 11:11 PDT, Camilo Viecco (:cviecco)
brian: review+
brian: superreview+
Details | Diff | Splinter Review
To patch before reverting. Fixing bitwise NOT (1.15 KB, patch)
2012-09-27 11:15 PDT, Camilo Viecco (:cviecco)
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review
T-Shirt for PSM hackers (7.61 KB, image/jpeg)
2012-10-01 14:10 PDT, Honza Bambas (:mayhemer)
no flags Details
test-cases part1 (6.72 KB, patch)
2012-10-03 13:12 PDT, Camilo Viecco (:cviecco)
no flags Details | Diff | Splinter Review
now with full testing of all combinations (certdb patched). (60.06 KB, patch)
2012-10-03 15:11 PDT, Camilo Viecco (:cviecco)
no flags Details | Diff | Splinter Review
test-certoverrides (61.87 KB, patch)
2012-10-03 16:42 PDT, Camilo Viecco (:cviecco)
brian: review-
Details | Diff | Splinter Review
test-certoverrides v2 (61.14 KB, patch)
2012-10-04 13:47 PDT, Camilo Viecco (:cviecco)
no flags Details | Diff | Splinter Review
test-certoverrides v2.1 (61.23 KB, patch)
2012-10-04 14:05 PDT, Camilo Viecco (:cviecco)
no flags Details | Diff | Splinter Review
output of tests with original values (failed tests) (28.10 KB, text/plain)
2012-10-04 14:13 PDT, Camilo Viecco (:cviecco)
no flags Details
test-certoverrides v2.2 (61.35 KB, patch)
2012-10-04 14:53 PDT, Camilo Viecco (:cviecco)
brian: review+
Details | Diff | Splinter Review
test-certoverrides v3 (61.42 KB, patch)
2012-10-10 14:02 PDT, Camilo Viecco (:cviecco)
no flags Details | Diff | Splinter Review
test-certoverrides v3.1 (61.37 KB, patch)
2012-10-11 08:34 PDT, Camilo Viecco (:cviecco)
no flags Details | Diff | Splinter Review
test-certoverrides v4 (58.33 KB, patch)
2012-10-12 10:49 PDT, Camilo Viecco (:cviecco)
no flags Details | Diff | Splinter Review
test-certoverrides v4.1 (58.26 KB, patch)
2012-10-12 10:56 PDT, Camilo Viecco (:cviecco)
no flags Details | Diff | Splinter Review
test-certoverrides v4.2 (58.26 KB, patch)
2012-10-12 11:06 PDT, Camilo Viecco (:cviecco)
no flags Details | Diff | Splinter Review
test-certoverrides v4.3 (58.28 KB, text/plain)
2012-10-29 09:45 PDT, Camilo Viecco (:cviecco)
no flags Details
test-certoverrides v5 (58.27 KB, patch)
2012-10-29 10:02 PDT, Camilo Viecco (:cviecco)
honzab.moz: review+
Details | Diff | Splinter Review
test-certoverrides v6 (58.27 KB, patch)
2012-10-31 09:47 PDT, Camilo Viecco (:cviecco)
honzab.moz: review+
honzab.moz: checkin+
Details | Diff | Splinter Review

Description Camilo Viecco (:cviecco) 2012-08-23 16:10:38 PDT
Created attachment 654830 [details]
Fix to the bug

The heckCertOverridesbits function uses a substraction isntead of a bitwise maninulation for clearing overrides. This causes that when there are overrides than the currently enabled are found (such as when libpix is enabled or if the cert changes) the override service does not returned cleared even when that should be the case.

Steps to reproduce (on a fresh checkout)
1. Enable libpkix verfication in /modules/libpref/src/init/all.js 
2. recompile 
3. Run the test_sdpy.js xpcshell test case (you need node.js).
4. The test will fail.
Comment 1 Camilo Viecco (:cviecco) 2012-08-23 16:11:34 PDT
Created attachment 654831 [details] [diff] [review]
Fix to the bug
Comment 2 Camilo Viecco (:cviecco) 2012-09-21 11:40:30 PDT
This a two byter.. already r+ by ryan smith, now asking Honza for second review so that I can land.
Comment 3 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-09-21 12:22:26 PDT
Comment on attachment 654831 [details] [diff] [review]
Fix to the bug

Please use r? and sr? to handle the two-review requirement in security/.
Comment 4 Honza Bambas (:mayhemer) 2012-09-26 11:58:36 PDT
Comment on attachment 654831 [details] [diff] [review]
Fix to the bug

Review of attachment 654831 [details] [diff] [review]:
-----------------------------------------------------------------

See also https://bugzilla.mozilla.org/show_bug.cgi?id=571034#c1.  That bug is effectively a dup of this one, btw.

sr=honzab.
Comment 5 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-09-26 12:00:12 PDT
*** Bug 571034 has been marked as a duplicate of this bug. ***
Comment 6 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-09-26 12:12:18 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a989993c992
Comment 7 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-09-26 12:49:39 PDT
Comment on attachment 654831 [details] [diff] [review]
Fix to the bug

[Approval Request Comment]
Bug caused by (feature/regressing bug #): This is a longstanding bug.

User impact if declined: The user may encounter the "certificate error" page two or three times (over a period of time) instead of just once, even if they override the error. (This is long-standing behavior.)

Testing completed (on m-c, etc.): This just landed on inbound.

Risk to taking this patch (and alternatives if risky): No risk. This is a trivial change that is an obviously improvement.

String or UUID changes made by this patch: none.
Comment 8 Alex Keybl [:akeybl] 2012-09-26 14:53:03 PDT
(In reply to Brian Smith (:bsmith) from comment #7)
> User impact if declined: The user may encounter the "certificate error" page
> two or three times (over a period of time) instead of just once, even if
> they override the error. (This is long-standing behavior.)

Is the concern here that bug 650355 will make the certificate error much more common?
Comment 9 Alex Keybl [:akeybl] 2012-09-26 14:53:30 PDT
Comment on attachment 654831 [details] [diff] [review]
Fix to the bug

[Triage Comment]
Approving for Aurora in preparation for Beta consideration.
Comment 10 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-09-26 15:30:36 PDT
(In reply to Alex Keybl [:akeybl] from comment #8)
> (In reply to Brian Smith (:bsmith) from comment #7)
> > User impact if declined: The user may encounter the "certificate error" page
> > two or three times (over a period of time) instead of just once, even if
> > they override the error. (This is long-standing behavior.)
> 
> Is the concern here that bug 650355 will make the certificate error much
> more common?

Yes, that's right.
Comment 11 Alex Keybl [:akeybl] 2012-09-26 17:14:45 PDT
Comment on attachment 654831 [details] [diff] [review]
Fix to the bug

[Triage Comment]
Let's get bugs like this nominated sooner in the release cycle. Approving for Beta since this is a trivial fix for what may become an annoying "regression".

If you know of an md5 certificate site that QA can use to verify, please include here.
Comment 12 Ryan VanderMeulen [:RyanVM] 2012-09-26 18:43:51 PDT
https://hg.mozilla.org/mozilla-central/rev/1a989993c992
Comment 13 :Ms2ger (⌚ UTC+1/+2) 2012-09-27 10:03:29 PDT
Wait, |!overrideBits| and not |~overrideBits|?
Comment 14 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-09-27 10:27:13 PDT
(In reply to :Ms2ger from comment #13)
> Wait, |!overrideBits| and not |~overrideBits|?

Camilo, please fix this ASAP.

Thanks, Ms2ger.
Comment 15 Camilo Viecco (:cviecco) 2012-09-27 11:11:59 PDT
Created attachment 665548 [details] [diff] [review]
fix-to-commited patch. Use bitwise NOT instead of logic NOT
Comment 16 Camilo Viecco (:cviecco) 2012-09-27 11:15:03 PDT
Created attachment 665550 [details] [diff] [review]
To patch before reverting. Fixing bitwise NOT

Added two patches for you to choose. this one Assumes the first patch was not placed before
Comment 17 Alex Keybl [:akeybl] 2012-09-27 12:01:25 PDT
Comment on attachment 654831 [details] [diff] [review]
Fix to the bug

[Triage Comment]
Given the churn in this bug, setting the aurora/beta flags back to ?. This patch was meant to be very low risk, but has since been reopened.
Comment 18 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-09-28 11:12:37 PDT
Comment on attachment 665548 [details] [diff] [review]
fix-to-commited patch. Use bitwise NOT instead of logic NOT

Review of attachment 665548 [details] [diff] [review]:
-----------------------------------------------------------------

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d87924d4760
https://hg.mozilla.org/releases/mozilla-aurora/rev/260a27f882b5
https://hg.mozilla.org/releases/mozilla-beta/rev/f95d57abe039

OK, I didn't see Alex's last comment until I'd already checked in the fixed version of the patch into all three channels. Shall I back them out?

It was definitely silly that three different people missed "!" vs "~" in the patch but it's still very low risk. (Now even more risk, now that it is correct!)
Comment 19 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-09-28 11:16:06 PDT
Comment on attachment 665550 [details] [diff] [review]
To patch before reverting. Fixing bitwise NOT

https://hg.mozilla.org/releases/mozilla-aurora/rev/260a27f882b5
https://hg.mozilla.org/releases/mozilla-beta/rev/f95d57abe039

I already checked in this patch based on the previous approval for the one with the typo. See the previous comment.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): longstanding issue
User impact if declined: Under some conditions, certificate error pages may be shown more frequently than necessary and/or some certificate errors may not overridable.
Testing completed (on m-c, etc.): Manual. Just landed.
Risk to taking this patch (and alternatives if risky): Basically none (for real this time.)
String or UUID changes made by this patch: none.
Comment 20 Alex Keybl [:akeybl] 2012-09-28 14:51:42 PDT
Comment on attachment 665550 [details] [diff] [review]
To patch before reverting. Fixing bitwise NOT

[Triage Comment]
We're going to trust your intuition and leave it approved, to prevent a frustrating UX.

Please instruct QA on the best way to verify and find regressions.
Comment 21 Phil Ringnalda (:philor) 2012-09-28 22:25:54 PDT
https://hg.mozilla.org/mozilla-central/rev/5d87924d4760
Comment 23 Alex Keybl [:akeybl] 2012-10-01 10:11:29 PDT
(In reply to Alex Keybl [:akeybl] from comment #20)
> Please instruct QA on the best way to verify and find regressions.

Brian - still waiting on this.
Comment 24 Honza Bambas (:mayhemer) 2012-10-01 14:10:08 PDT
Created attachment 666707 [details]
T-Shirt for PSM hackers

I think I'm gonna by one!
Comment 25 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-10-01 16:49:27 PDT
cviecco, please either write an automated test or respond on comment 23.

If you write an automated test, I think you can do so in a mochitest by directly calling into nsICertOverrideService to set a cert override for some invalid cert that has only one bit set (e.g. expired) for a cert that has two problems (e.g. expired + untrusted issuer), then open up an SSL connection. The test should fail without your patch and it should succeed with your patch. See toolkit/mozapps/extensions/test/browser/browser_installssl.js for an example.

For this, you need to generate a new certificate that has the two errors. This is good because I think you've already done similar work in bug 699874.
Comment 26 Camilo Viecco (:cviecco) 2012-10-02 09:23:04 PDT
Was out of the office until today.

Seems like writing a mochitest would be possible. And Yes I am going to get one of those shirts.

Shall I open a follow bug for the tests?
Comment 27 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-10-02 12:01:58 PDT
Just use this bug.
Comment 28 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-10-02 12:28:44 PDT
BTW, you might not need a cert with two problems; it might be the case that you simply need a cert that has a different problem (e.g. expired) than what the override is for.
Comment 29 Camilo Viecco (:cviecco) 2012-10-03 13:12:48 PDT
Created attachment 667619 [details] [diff] [review]
test-cases part1

This tests the override service for all certoverride bits but only for
1. expired
2. unstrusted
3. expired and mismatch
4. unstrusted and mismatch

Three cases are pending (adding mismatch only, expired and untrusted and all wrong), but those require changing the pgo cert db. This patch is just a preview.
Comment 30 Camilo Viecco (:cviecco) 2012-10-03 15:11:18 PDT
Created attachment 667687 [details] [diff] [review]
now with full testing of all combinations (certdb patched).
Comment 31 Camilo Viecco (:cviecco) 2012-10-03 16:42:21 PDT
Created attachment 667733 [details] [diff] [review]
test-certoverrides
Comment 32 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-10-03 19:14:15 PDT
Comment on attachment 667733 [details] [diff] [review]
test-certoverrides

Review of attachment 667733 [details] [diff] [review]:
-----------------------------------------------------------------

Great work!

Please describe what you did with pgocerts to modify the cert and key databases.

Please attach a log showing the output of this test when your patch for this bug is not applied (i.e. when we use operator- instead of operator&), to show which tests fail (or add a link to a tryserver run that shows that this test fails without your fix.)

::: security/manager/ssl/tests/mochitest/bugs/test_certificate_overrides.html
@@ +1,5 @@
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> +  <title>Test certificate overrides</title>
> +  <script type="text/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>        

Fix trailing whitespace highlighted by splinter everywhere in this file. Also, many parts of this file do not follow the style guide, regarding camelCase naming and whitespace conventions. Please fix that.

Also, restrict the number of blank lines between functions to one.

Remove commented-out code.

@@ +15,5 @@
> +//const Cu = Components.utils;
> +//const Cr = Components.results;
> +
> +var testHost=[];
> +testHost[1]="untrusted.example.com";

s/testHost/testHosts/

@@ +16,5 @@
> +//const Cr = Components.results;
> +
> +var testHost=[];
> +testHost[1]="untrusted.example.com";
> +//mismatch is 2

what does this comment mean?

@@ +49,5 @@
> +  },
> +  QueryInterface: function(aIID) {
> +    if (aIID.equals(Ci.nsIBadCertListener2) ||
> +        aIID.equals(Ci.nsIInterfaceRequestor) ||
> +        aIID.equals(Ci.nsISupports))

Add braces around body of statement to make things easier to read.

@@ +56,5 @@
> +  },
> +  notifyCertProblem: function(socketInfo, sslStatus, targetHost) {
> +    certerrorbits =  (sslStatus.QueryInterface(Ci.nsISSLStatus).isUntrusted ) |
> +                 (sslStatus.QueryInterface(Ci.nsISSLStatus).isDomainMismatch <<1 ) | 
> +                 (sslStatus.QueryInterface(Ci.nsISSLStatus).isNotValidAtThisTime <<2 );            

QueryInterface once.

certErrorBits = 0;
if (sslStatus.isUntrusted) certErrorBits |= Ci.nsICertOverrideService.ERROR_UNTRUSTED;
...

This way, we don't have to worry about whether ERROR_UNTRUSTED is 1, etc.

@@ +77,5 @@
> +     } else {
> +       url = "https://" + host + "/";
> +     }
> +     req.open("GET", url, false);
> +     req.channel.notificationCallbacks = new CertOverrideListener(host, port, bits);

None of the above lines in this block should be in the try block. We want the test to fail if any of them fail.

@@ +78,5 @@
> +       url = "https://" + host + "/";
> +     }
> +     req.open("GET", url, false);
> +     req.channel.notificationCallbacks = new CertOverrideListener(host, port, bits);
> +     req.send(null);

missing ok(false, "the request should have failed");

@@ +80,5 @@
> +     req.open("GET", url, false);
> +     req.channel.notificationCallbacks = new CertOverrideListener(host, port, bits);
> +     req.send(null);
> +   } catch (e) {
> +     // This will fail since the server is not trusted yet

missing ok(true, ...);

@@ +85,5 @@
> +   }
> + }
> +
> +
> +///////////////////////

remove this line and extra blank lines.

@@ +87,5 @@
> +
> +
> +///////////////////////
> +
> +function xhrMustSucceed(domain,message){

why not:

   function xhr(domain, message, mustSucceed)

and...

@@ +96,5 @@
> +   try{
> +        req.open("GET", "https://"+domain+"/",false);
> +        req.send(null);
> +        var headers = req.getAllResponseHeaders();
> +        ok(true, "Page Load success  "+message);

ok(mustSuceeded, "Page load success " + message);

and...

@@ +99,5 @@
> +        var headers = req.getAllResponseHeaders();
> +        ok(true, "Page Load success  "+message);
> +   }
> +      catch(err){
> +         ok(false, "Page failed to load it should succeed message="+message);

ok(!mustSucceed, "page load failed " + message);

and...

@@ +103,5 @@
> +         ok(false, "Page failed to load it should succeed message="+message);
> +  };
> +}
> +
> +function xhrMustFail(domain,message){

...remove this function.

@@ +119,5 @@
> +  };
> +}
> +
> +
> +function doConnectTests(overridebits){

Write a comment about what this function is supposed to do.

Instead of having this function loop through the test hosts, why not put the loop inside testCertOverrides, so that this function would take (testHost, overrideBits) as parameters? This would make it clearer that we're testing the cross product of hosts * override bits, and it would make this function simpler.

@@ +121,5 @@
> +
> +
> +function doConnectTests(overridebits){
> +    var cos = Cc["@mozilla.org/security/certoverride;1"].
> +              getService(Ci.nsICertOverrideService);

might as well make cos a global variable that is initialized in the beginning, so we don't have to repeat this in every function.

@@ +123,5 @@
> +function doConnectTests(overridebits){
> +    var cos = Cc["@mozilla.org/security/certoverride;1"].
> +              getService(Ci.nsICertOverrideService);
> +    for(var i=1;i<8;i++){
> +      if(typeof testHost[i] == 'undefined'){

Why?

@@ +138,5 @@
> +      else{
> +        xhrMustFail(host,"override host="+host+status_info);
> +      }
> +      //reset overrides
> +      cos.clearValidityOverride(host,443); 

isn't this redundant?

@@ +144,5 @@
> +
> +}
> +
> +
> +function checkHostErrorbits(hostindex) {

capitalization in name.

What is this function supposed to do, exactly? Please add a comment.

and, why not:

    function checkHostErrorBits(host)

so that this function doesn't need to know about the array at all.

@@ +148,5 @@
> +function checkHostErrorbits(hostindex) {
> +   var port =443;
> +   var bits = 15;
> +
> +   if(typeof testHost[hostindex] == 'undefined'){

Why?

@@ +167,5 @@
> +       url = "https://" + host + "/";
> +     }
> +     req.open("GET", url, false);
> +     req.channel.notificationCallbacks = new CertOverrideListener(host, port, bits);
> +     req.send(null);

ok(false, ...);

@@ +169,5 @@
> +     req.open("GET", url, false);
> +     req.channel.notificationCallbacks = new CertOverrideListener(host, port, bits);
> +     req.send(null);
> +   } catch (e) {
> +     // This will fail since the server is not trusted yet

ok(true, ...);

@@ +174,5 @@
> +   }
> +   //now compare
> +   is(certerrorbits,hostindex,"errorbits="+certerrorbits);
> +   //and reset host the state
> +       var cos = Cc["@mozilla.org/security/certoverride;1"].

indention.

@@ +178,5 @@
> +       var cos = Cc["@mozilla.org/security/certoverride;1"].
> +              getService(Ci.nsICertOverrideService);
> +   cos.clearValidityOverride(host,443);
> + }
> +

Too many blank lines (here and elsewhere).

@@ +182,5 @@
> +
> +
> +
> +function testCertOverrides(){
> +  for (var i=1;i<8;i++){

var allBits = Ci.nsICertOverrideService.ERROR_UNTRUSTED | 
              Ci.nsICertOverrideService.ERROR_MISMATCH | 
              Ci.nsICertOverrideService.ERROR_TIME;

// Loop through every combination of overrides
for (var i = 1; i < allBits; i++) {

@@ +194,5 @@
> +
> +function onWindowLoad()
> +{
> +/*
> +*/

:(
Comment 33 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-10-03 20:57:59 PDT
*** Bug 418563 has been marked as a duplicate of this bug. ***
Comment 34 Camilo Viecco (:cviecco) 2012-10-04 10:42:21 PDT
Comment on attachment 667733 [details] [diff] [review]
test-certoverrides

Review of attachment 667733 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/tests/mochitest/bugs/test_certificate_overrides.html
@@ +16,5 @@
> +//const Cr = Components.results;
> +
> +var testHost=[];
> +testHost[1]="untrusted.example.com";
> +//mismatch is 2

personal comment (what is the bit value of mismatched?) , removed.

@@ +49,5 @@
> +  },
> +  QueryInterface: function(aIID) {
> +    if (aIID.equals(Ci.nsIBadCertListener2) ||
> +        aIID.equals(Ci.nsIInterfaceRequestor) ||
> +        aIID.equals(Ci.nsISupports))

done

@@ +56,5 @@
> +  },
> +  notifyCertProblem: function(socketInfo, sslStatus, targetHost) {
> +    certerrorbits =  (sslStatus.QueryInterface(Ci.nsISSLStatus).isUntrusted ) |
> +                 (sslStatus.QueryInterface(Ci.nsISSLStatus).isDomainMismatch <<1 ) | 
> +                 (sslStatus.QueryInterface(Ci.nsISSLStatus).isNotValidAtThisTime <<2 );            

We still need to worry for the hostindex, but I like this as it removes one more place to look.

@@ +77,5 @@
> +     } else {
> +       url = "https://" + host + "/";
> +     }
> +     req.open("GET", url, false);
> +     req.channel.notificationCallbacks = new CertOverrideListener(host, port, bits);

agreed.done

@@ +85,5 @@
> +   }
> + }
> +
> +
> +///////////////////////

ack, done

@@ +87,5 @@
> +
> +
> +///////////////////////
> +
> +function xhrMustSucceed(domain,message){

makes sense. Will do

@@ +119,5 @@
> +  };
> +}
> +
> +
> +function doConnectTests(overridebits){

agreed

@@ +121,5 @@
> +
> +
> +function doConnectTests(overridebits){
> +    var cos = Cc["@mozilla.org/security/certoverride;1"].
> +              getService(Ci.nsICertOverrideService);

done, now global const.

@@ +123,5 @@
> +function doConnectTests(overridebits){
> +    var cos = Cc["@mozilla.org/security/certoverride;1"].
> +              getService(Ci.nsICertOverrideService);
> +    for(var i=1;i<8;i++){
> +      if(typeof testHost[i] == 'undefined'){

testHost was sparse at one moment.

@@ +138,5 @@
> +      else{
> +        xhrMustFail(host,"override host="+host+status_info);
> +      }
> +      //reset overrides
> +      cos.clearValidityOverride(host,443); 

no, actually the first one would be the reduntant. as I assume that none of the hosts have overrides to begin with.

@@ +144,5 @@
> +
> +}
> +
> +
> +function checkHostErrorbits(hostindex) {

The function would then need the expected errors (they match the host index) and the signarue would be
 function checkHostErrors(host,expectedErrors).

@@ +148,5 @@
> +function checkHostErrorbits(hostindex) {
> +   var port =443;
> +   var bits = 15;
> +
> +   if(typeof testHost[hostindex] == 'undefined'){

because initial versions of the hostindex was not filled completelly (missing tesing cases) and one more extra check seemed innocuous.

@@ +167,5 @@
> +       url = "https://" + host + "/";
> +     }
> +     req.open("GET", url, false);
> +     req.channel.notificationCallbacks = new CertOverrideListener(host, port, bits);
> +     req.send(null);

Yep missed that one

@@ +169,5 @@
> +     req.open("GET", url, false);
> +     req.channel.notificationCallbacks = new CertOverrideListener(host, port, bits);
> +     req.send(null);
> +   } catch (e) {
> +     // This will fail since the server is not trusted yet

I can add it but is seems redundant.

@@ +182,5 @@
> +
> +
> +
> +function testCertOverrides(){
> +  for (var i=1;i<8;i++){

ok for (var i = 1; i <= allBits; i++)  (or 'for (var i = 1; i < allBits+1; i++)' )

@@ +194,5 @@
> +
> +function onWindowLoad()
> +{
> +/*
> +*/

So sad...... Yes I agree
Comment 35 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-10-04 10:46:30 PDT
Comment on attachment 667733 [details] [diff] [review]
test-certoverrides

Review of attachment 667733 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/tests/mochitest/bugs/test_certificate_overrides.html
@@ +138,5 @@
> +      else{
> +        xhrMustFail(host,"override host="+host+status_info);
> +      }
> +      //reset overrides
> +      cos.clearValidityOverride(host,443); 

Tests should always assume that the previous test messed up and didn't clean up after itself (because some tests are buggy and don't clean up properly). So, you should clear all the overrides at the very start of the test as well.

@@ +182,5 @@
> +
> +
> +
> +function testCertOverrides(){
> +  for (var i=1;i<8;i++){

Good catch. (I prefer <=, but I am not one to nit-pick.)
Comment 36 Camilo Viecco (:cviecco) 2012-10-04 13:47:09 PDT
Created attachment 668156 [details] [diff] [review]
test-certoverrides v2
Comment 37 Camilo Viecco (:cviecco) 2012-10-04 14:05:28 PDT
Created attachment 668166 [details] [diff] [review]
test-certoverrides v2.1
Comment 38 Camilo Viecco (:cviecco) 2012-10-04 14:13:20 PDT
Created attachment 668173 [details]
output of tests with original values (failed tests)

This is the output of running the test with: 
'TEST_PATH=security/ssl/bugs/test_certificate_overrides.html make -C obj-ff-dbg/ mochitest-chrome > /tmp/badout.txt'

the things to notice are the expected fail readings, notice 12 test fail.
Comment 39 Camilo Viecco (:cviecco) 2012-10-04 14:53:18 PDT
Created attachment 668195 [details] [diff] [review]
test-certoverrides v2.2
Comment 40 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-10-09 20:58:07 PDT
Comment on attachment 668195 [details] [diff] [review]
test-certoverrides v2.2

Review of attachment 668195 [details] [diff] [review]:
-----------------------------------------------------------------

Great work. r+ with comments addressed.

::: security/manager/ssl/tests/mochitest/bugs/test_certificate_overrides.html
@@ +24,5 @@
> +testHost[      em | eu ] = "mismatch.untrusted.example.com";
> +testHost[ et           ] = "expired.example.com";
> +testHost[ et      | eu ] = "untrusted-expired.example.com";
> +testHost[ et | em      ] = "mismatch.expired.example.com";
> +testHost[ et | em | eu ] = "mismatch.untrusted-expired.example.com";

You make testing for cert overrides so pretty!

@@ +96,5 @@
> +  var req = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"]
> +             .createInstance(Ci.nsIXMLHttpRequest);
> +
> +  try {
> +    req.open("GET", "https://"+domain+"/", false);

spaces around "+" (two places).

@@ +98,5 @@
> +
> +  try {
> +    req.open("GET", "https://"+domain+"/", false);
> +    req.send(null);
> +    var headers = req.getAllResponseHeaders();

why are you grabbing the headers?

@@ +106,5 @@
> +  }
> +}
> +
> +function checkHostConnect(hostIndex, overrideBits) {
> +  if (typeof testHost[hostIndex] == 'undefined') {

can this happen?

@@ +115,5 @@
> +  var statusMessage = " hostIndex =" + hostIndex + " overrideBits=" + overrideBits;
> +
> +  addCertOverride(host, 443, overrideBits);
> +  if (0 == (hostIndex & ~overrideBits)) {
> +    xhrConnect(host,"override host=" + host + statusMessage,true);

whitespace.

@@ +118,5 @@
> +  if (0 == (hostIndex & ~overrideBits)) {
> +    xhrConnect(host,"override host=" + host + statusMessage,true);
> +  }
> +  else {
> +    xhrConnect(host,"override host=" + host + statusMessage,false);

whitespace.

@@ +145,5 @@
> +  } catch (e) {
> +    // This will fail since the server is not trusted yet
> +  }
> +  //now compare
> +  is(certErrorBits,expectedErrorBits,"errorbits=" +certErrorBits + " host=" + host);

spaces.

@@ +150,5 @@
> +  //and reset host the state
> +  cos.clearValidityOverride(host, port);
> +}
> +
> +function testCertOverrides(){

() {
Comment 41 Camilo Viecco (:cviecco) 2012-10-10 10:20:59 PDT
Comment on attachment 668195 [details] [diff] [review]
test-certoverrides v2.2

Review of attachment 668195 [details] [diff] [review]:
-----------------------------------------------------------------

Should I submit a new version with fixed or wait for Honza's sr?

::: security/manager/ssl/tests/mochitest/bugs/test_certificate_overrides.html
@@ +96,5 @@
> +  var req = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"]
> +             .createInstance(Ci.nsIXMLHttpRequest);
> +
> +  try {
> +    req.open("GET", "https://"+domain+"/", false);

Ack. Done

@@ +98,5 @@
> +
> +  try {
> +    req.open("GET", "https://"+domain+"/", false);
> +    req.send(null);
> +    var headers = req.getAllResponseHeaders();

It was for the failure, but it should be removed.

@@ +106,5 @@
> +  }
> +}
> +
> +function checkHostConnect(hostIndex, overrideBits) {
> +  if (typeof testHost[hostIndex] == 'undefined') {

it should never happen, is just a fail safe in case we ever try to use an invalid hostIndex. I can remove if you wish.

@@ +115,5 @@
> +  var statusMessage = " hostIndex =" + hostIndex + " overrideBits=" + overrideBits;
> +
> +  addCertOverride(host, 443, overrideBits);
> +  if (0 == (hostIndex & ~overrideBits)) {
> +    xhrConnect(host,"override host=" + host + statusMessage,true);

ACK. Done

@@ +118,5 @@
> +  if (0 == (hostIndex & ~overrideBits)) {
> +    xhrConnect(host,"override host=" + host + statusMessage,true);
> +  }
> +  else {
> +    xhrConnect(host,"override host=" + host + statusMessage,false);

ACK. Done

@@ +145,5 @@
> +  } catch (e) {
> +    // This will fail since the server is not trusted yet
> +  }
> +  //now compare
> +  is(certErrorBits,expectedErrorBits,"errorbits=" +certErrorBits + " host=" + host);

ACK. (grrrr, what is that utility that you mentioned to help me catch these?)

@@ +150,5 @@
> +  //and reset host the state
> +  cos.clearValidityOverride(host, port);
> +}
> +
> +function testCertOverrides(){

ACK, Done
Comment 42 Camilo Viecco (:cviecco) 2012-10-10 14:02:47 PDT
Created attachment 670126 [details] [diff] [review]
test-certoverrides v3
Comment 43 Camilo Viecco (:cviecco) 2012-10-10 14:04:20 PDT
Comment on attachment 670126 [details] [diff] [review]
test-certoverrides v3

one typeo removing review requests
Comment 44 Camilo Viecco (:cviecco) 2012-10-11 08:34:00 PDT
Created attachment 670405 [details] [diff] [review]
test-certoverrides v3.1
Comment 45 Honza Bambas (:mayhemer) 2012-10-11 10:07:17 PDT
Camilo, please don't obsolete the current patch, you would destroy my splinter comments.  Thanks.
Comment 46 Camilo Viecco (:cviecco) 2012-10-11 11:46:15 PDT
(In reply to Honza Bambas (:mayhemer) from comment #45)
> Camilo, please don't obsolete the current patch, you would destroy my
> splinter comments.  Thanks.

OK. Thank you
Comment 47 Honza Bambas (:mayhemer) 2012-10-11 11:46:38 PDT
Comment on attachment 670405 [details] [diff] [review]
test-certoverrides v3.1

Review of attachment 670405 [details] [diff] [review]:
-----------------------------------------------------------------

Overall, this is a very good piece of work!

Please regenerate the certificates (where applicable) to expire not sooner then in 100 years.

Many formatting nits, please strictly follow this formatting, mainly missing or unnecessary white spaces and CRs:

gGlobalVariable;

function name()
{
  // Comment has to be a sentence.
  variable = 0;
  for (var i = 0; i < max; ++i) {
  }
}

Please provide certutil (or any other) commands you used to generate the certs, bug comment is enough.

::: build/pgo/server-locations.txt
@@ +97,5 @@
>  https://expired.example.com:443        privileged,cert=expired
>  https://requestclientcert.example.com:443         privileged,clientauth=request
>  https://requireclientcert.example.com:443         privileged,clientauth=require
> +https://mismatch.expired.example.com:443	privileged,cert=expired
> +https://mismatch.untrusted.example.com:443	privileged,cert=untrusted

You would be OK with the self signed cert we already have, but up to you.

@@ +98,5 @@
>  https://requestclientcert.example.com:443         privileged,clientauth=request
>  https://requireclientcert.example.com:443         privileged,clientauth=require
> +https://mismatch.expired.example.com:443	privileged,cert=expired
> +https://mismatch.untrusted.example.com:443	privileged,cert=untrusted
> +https://mismatch.example.com:443		privileged,cert=staticname

I think you are OK with using nocert.example.com host here.  Again, no need for a new cert here.

::: security/manager/ssl/tests/mochitest/bugs/test_certificate_overrides.html
@@ +26,5 @@
> +testHost[ et      | eu ] = "untrusted-expired.example.com";
> +testHost[ et | em      ] = "mismatch.expired.example.com";
> +testHost[ et | em | eu ] = "mismatch.untrusted-expired.example.com";
> +
> +var certErrorBits;

call this gCertErrorBits.

@@ +100,5 @@
> +    req.open("GET", "https://" + domain + "/", false);
> +    req.send(null);
> +    ok(expectedSuccess, "Page Load success  " + message + " expected=" + expectedSuccess);
> +  } catch (err) {
> +    ok(!expectedSuccess, "Page failed to load" + message + " expected=" + expectedSuccess);

Check spaces in logs...

@@ +113,5 @@
> +  var host = testHost[hostIndex];
> +  var statusMessage = " hostIndex =" + hostIndex + " overrideBits=" + overrideBits;
> +
> +  addCertOverride(host, 443, overrideBits);
> +  if (0 == (hostIndex & ~overrideBits)) {

You can just put this condition to the last arg of xhrConnect.

@@ +141,5 @@
> +    req.channel.notificationCallbacks = new CertOverrideListener(host, port, bits);
> +    req.send(null);
> +    ok(false, "Connection to host" + host + "succeeded when it should have failed");
> +  } catch (e) {
> +    // This will fail since the server is not trusted yet

"Failure here is expected" is a bit better comment, but up to you.

@@ +144,5 @@
> +  } catch (e) {
> +    // This will fail since the server is not trusted yet
> +  }
> +  //now compare
> +  is(certErrorBits, expectedErrorBits, "errorbits=" +certErrorBits + " host=" + host);

I think you can do this check in checkHostConnect() when hostIndex == overrideBits after you call addCertOverride().  Then (g)certErrorBits must be equal to hostIndex, right?

This code is mostly a copy of what addCertOverride does.  Since this test makes a lot of requests (7*7+7=56) we may save those 7 at least.

@@ +145,5 @@
> +    // This will fail since the server is not trusted yet
> +  }
> +  //now compare
> +  is(certErrorBits, expectedErrorBits, "errorbits=" +certErrorBits + " host=" + host);
> +  //and reset host the state

wording
Comment 48 Camilo Viecco (:cviecco) 2012-10-12 10:49:10 PDT
Created attachment 670859 [details] [diff] [review]
test-certoverrides v4

Addresses Comments by Honza


To generate the new cert:
cd build/pgo/certs/

../../../obj-ff-dbg/dist/bin/certutil -S -n "untrustedandexpired" -s "CN=untrusted-expired.example.com" -c "Unknown CA" -t "P,," -k rsa -g 1024 -m 98743 -v 0 -d .
Comment 49 Camilo Viecco (:cviecco) 2012-10-12 10:56:22 PDT
Created attachment 670862 [details] [diff] [review]
test-certoverrides v4.1
Comment 50 Camilo Viecco (:cviecco) 2012-10-12 11:06:24 PDT
Created attachment 670872 [details] [diff] [review]
test-certoverrides v4.2
Comment 51 Camilo Viecco (:cviecco) 2012-10-12 11:13:26 PDT
Comment on attachment 670872 [details] [diff] [review]
test-certoverrides v4.2

Addressed review of 3.1. 

-Renamed the certerror bits into  gCertErrorBits.
-Integrated the checks into a single function. (and this is why I am calling this a review) (removed 7 connections)
-Changed style.
-Fixed spaces in two of the log output strings.
-Remove one unnecessary cert from the database. (I am still using 'the untrusted cert' instead of the self-signed for the untrusted cert test).
-Added an extra test for the self-signed case.
Comment 52 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-10-15 16:05:44 PDT
Given there is an automated test for this, is there any necessity for QA to do a verification? If so, please advise on what we can check for in terms of regressions.
Comment 53 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-10-21 13:12:47 PDT
Comment on attachment 665550 [details] [diff] [review]
To patch before reverting. Fixing bitwise NOT

This was fixed on all channels already using the other patch.
Comment 54 Honza Bambas (:mayhemer) 2012-10-22 10:33:49 PDT
Comment on attachment 670872 [details] [diff] [review]
test-certoverrides v4.2

Review of attachment 670872 [details] [diff] [review]:
-----------------------------------------------------------------

Dropping r, still not for to be landed.

You didn't regenerate the 'untrusted' and 'Unknown CA' certs.  Those will expire in 2019.

Please fix the last formatting issues before landing please.  

I personally liked some blank lines you have removed, but that is up to you.

::: security/manager/ssl/tests/mochitest/bugs/test_certificate_overrides.html
@@ +14,5 @@
> +              getService(Ci.nsICertOverrideService);
> +const eu = Ci.nsICertOverrideService.ERROR_UNTRUSTED;
> +const em = Ci.nsICertOverrideService.ERROR_MISMATCH;
> +const et = Ci.nsICertOverrideService.ERROR_TIME;
> +//note:  the host index matches the expected error.

// Note: the host index matches the expected error.

@@ +46,5 @@
> +  },
> +  QueryInterface: function(aIID) {
> +    if (aIID.equals(Ci.nsIBadCertListener2) ||
> +        aIID.equals(Ci.nsIInterfaceRequestor) ||
> +        aIID.equals(Ci.nsISupports)){

aIID.equals(Ci.nsISupports)) {

Space before {

@@ +82,5 @@
> +  req.open("GET", url, false);
> +  req.channel.notificationCallbacks = new CertOverrideListener(host, port, bits);
> +  try {
> +    req.send(null);
> +    ok(false, "Connection to host" + host + "succeeded when it should have failed");

"Connection to host " + host + " succeeded when it should have failed"

@@ +103,5 @@
> +}
> +
> +function checkHostConnect(host, overrideBits, successExpected, overridesMustEqualError)
> +{
> +  var statusMessage = " overrideBits=" + overrideBits;

Why is this variable needed?

@@ +105,5 @@
> +function checkHostConnect(host, overrideBits, successExpected, overridesMustEqualError)
> +{
> +  var statusMessage = " overrideBits=" + overrideBits;
> +  addCertOverride(host, 443, overrideBits);
> +  if(overridesMustEqualError) {

if (overridesMustEqualError) {

Space after if.

@@ +107,5 @@
> +  var statusMessage = " overrideBits=" + overrideBits;
> +  addCertOverride(host, 443, overrideBits);
> +  if(overridesMustEqualError) {
> +    is(gCertErrorBits, overrideBits, "Reported Error match: errorbits=" + gCertErrorBits + " host=" + host);
> +  }

Does it make any sense to check here that gCertErrorBits & ~overrideBits == 0 in an else branch?

@@ +124,5 @@
> +    for (var j = 1; j < testHost.length; ++j){
> +      var expectedError = j;
> +      var successExpected = (0 == (expectedError & ~overrideBits));
> +      var errorMustMatch = (overrideBits == expectedError);
> +      checkHostConnect(testHost[expectedError], i, successExpected, errorMustMatch);

pass overrideBits instead of i?  and maybe testHost[j], but up to you.

@@ +127,5 @@
> +      var errorMustMatch = (overrideBits == expectedError);
> +      checkHostConnect(testHost[expectedError], i, successExpected, errorMustMatch);
> +    }
> +  }
> +  //now we test the self-signed. Must return an overridable untrusted error.

// Now we test the self-signed. Must return an overridable untrusted error.

@@ +129,5 @@
> +    }
> +  }
> +  //now we test the self-signed. Must return an overridable untrusted error.
> +  cos.clearValidityOverride("self-signed.example.com", 443);
> +  checkHostConnect("self-signed.example.com", eu , successExpected, true);

checkHostConnect("self-signed.example.com", eu, successExpected, true);

Unnecessary white space after eu.
Comment 55 Honza Bambas (:mayhemer) 2012-10-22 10:52:42 PDT
(In reply to Honza Bambas (:mayhemer) from comment #54)
> Please fix the last formatting issues before landing please.  

I thought "before resubmitting" of course.
Comment 56 Camilo Viecco (:cviecco) 2012-10-29 09:42:00 PDT
Comment on attachment 670872 [details] [diff] [review]
test-certoverrides v4.2

Review of attachment 670872 [details] [diff] [review]:
-----------------------------------------------------------------

I disagree with the regeneration of both  'untrusted' and 'Unknown CA' certs as these
where in the database before this change. If we desire to update the expiration of certs this should
be in a separate bug.

::: security/manager/ssl/tests/mochitest/bugs/test_certificate_overrides.html
@@ +14,5 @@
> +              getService(Ci.nsICertOverrideService);
> +const eu = Ci.nsICertOverrideService.ERROR_UNTRUSTED;
> +const em = Ci.nsICertOverrideService.ERROR_MISMATCH;
> +const et = Ci.nsICertOverrideService.ERROR_TIME;
> +//note:  the host index matches the expected error.

OK. Agreed. fixed

@@ +46,5 @@
> +  },
> +  QueryInterface: function(aIID) {
> +    if (aIID.equals(Ci.nsIBadCertListener2) ||
> +        aIID.equals(Ci.nsIInterfaceRequestor) ||
> +        aIID.equals(Ci.nsISupports)){

Agreed Done.

@@ +82,5 @@
> +  req.open("GET", url, false);
> +  req.channel.notificationCallbacks = new CertOverrideListener(host, port, bits);
> +  try {
> +    req.send(null);
> +    ok(false, "Connection to host" + host + "succeeded when it should have failed");

Fixed

@@ +103,5 @@
> +}
> +
> +function checkHostConnect(host, overrideBits, successExpected, overridesMustEqualError)
> +{
> +  var statusMessage = " overrideBits=" + overrideBits;

Is not really needed. Just added it for readability. Can remove if you wish.

@@ +105,5 @@
> +function checkHostConnect(host, overrideBits, successExpected, overridesMustEqualError)
> +{
> +  var statusMessage = " overrideBits=" + overrideBits;
> +  addCertOverride(host, 443, overrideBits);
> +  if(overridesMustEqualError) {

Done

@@ +107,5 @@
> +  var statusMessage = " overrideBits=" + overrideBits;
> +  addCertOverride(host, 443, overrideBits);
> +  if(overridesMustEqualError) {
> +    is(gCertErrorBits, overrideBits, "Reported Error match: errorbits=" + gCertErrorBits + " host=" + host);
> +  }

I think that is unncecesary.

@@ +127,5 @@
> +      var errorMustMatch = (overrideBits == expectedError);
> +      checkHostConnect(testHost[expectedError], i, successExpected, errorMustMatch);
> +    }
> +  }
> +  //now we test the self-signed. Must return an overridable untrusted error.

Fixed.

@@ +129,5 @@
> +    }
> +  }
> +  //now we test the self-signed. Must return an overridable untrusted error.
> +  cos.clearValidityOverride("self-signed.example.com", 443);
> +  checkHostConnect("self-signed.example.com", eu , successExpected, true);

Done fixed.
Comment 57 Camilo Viecco (:cviecco) 2012-10-29 09:45:46 PDT
Created attachment 676198 [details]
test-certoverrides v4.3

Solves issues on JS of version 4.2. Does NOT update the db from version 4.2 (ie does not update the certs already in the db).
Comment 58 Camilo Viecco (:cviecco) 2012-10-29 10:02:44 PDT
Created attachment 676209 [details] [diff] [review]
test-certoverrides v5

Addresses comments for 4.2. Does not change the certdb from 4.2 (does not update certs already in the cert db).
Comment 59 Honza Bambas (:mayhemer) 2012-10-30 15:39:38 PDT
(In reply to Camilo Viecco (:cviecco) from comment #56)
> Comment on attachment 670872 [details] [diff] [review]
> test-certoverrides v4.2
> 
> Review of attachment 670872 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I disagree with the regeneration of both  'untrusted' and 'Unknown CA' certs
> as these
> where in the database before this change. 

Ah, I wasn't aware.  You had to enlighten me the last time already :)
Comment 60 Honza Bambas (:mayhemer) 2012-10-30 15:44:54 PDT
Comment on attachment 676209 [details] [diff] [review]
test-certoverrides v5

Review of attachment 676209 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab with few last little nits.

Thanks for all the updates and this pretty good test!

::: security/manager/ssl/tests/mochitest/bugs/test_certificate_overrides.html
@@ +16,5 @@
> +const eu = Ci.nsICertOverrideService.ERROR_UNTRUSTED;
> +const em = Ci.nsICertOverrideService.ERROR_MISMATCH;
> +const et = Ci.nsICertOverrideService.ERROR_TIME;
> +
> +//Note:  the host index matches the expected error.

Space between // and Note (it is coding convention to have a space after //.

@@ +106,5 @@
> +}
> +
> +function checkHostConnect(host, overrideBits, successExpected, overridesMustEqualError)
> +{
> +  var statusMessage = " overrideBits=" + overrideBits;

For me this var was more confusing, but up to you to leave or remove.

@@ +130,5 @@
> +      var errorMustMatch = (overrideBits == expectedError);
> +      checkHostConnect(testHost[j], overrideBits, successExpected, errorMustMatch);
> +    }
> +  }
> +  //Now we test the self-signed. Must return an overridable untrusted error.

Same here.
Comment 61 Camilo Viecco (:cviecco) 2012-10-31 09:47:21 PDT
Created attachment 677044 [details] [diff] [review]
test-certoverrides v6

Fixed nits from v5

Thank you Honza and Brian. This now has r+ from both of you. I am now pushing it to try and once it passes I will mark it for chekin.
Comment 62 Honza Bambas (:mayhemer) 2012-10-31 14:20:29 PDT
Comment on attachment 677044 [details] [diff] [review]
test-certoverrides v6

https://hg.mozilla.org/integration/mozilla-inbound/rev/2fb35fde527d
Comment 63 Ed Morley [:emorley] 2012-11-01 06:53:03 PDT
https://hg.mozilla.org/mozilla-central/rev/2fb35fde527d
Comment 64 Virgil Dicu [:virgil] [QA] 2012-11-12 04:44:45 PST
Is there anything specific that we can do here to verify this? See comment 52.
Comment 65 Ioana (away) 2012-11-19 04:51:14 PST
Marking as [qa-] since there are still no answers for comment 52, and this bug fix does have an automated test verifying it.

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