Merge pull request #74 from mozilla/better-error-messages

for expired certificates or assertions, specify which in error messages ...
mozilla · Nov 20, 2013 · 8e3037a · 8e3037a
2 parents 558712b + f314d2f
commit 8e3037a
Show file tree

Hide file tree

Showing 4 changed files with 28 additions and 22 deletions.
diff --git a/lib/assertion.js b/lib/assertion.js
@@ -77,13 +77,14 @@ exports.verify = function(signedObject, publicKey, now, cb) {
     // check iat
     if (assertionParams.issuedAt) {
       if (assertionParams.issuedAt.valueOf() > now.valueOf())
-        return cb("assertion issued later than verification date");
+        return cb("issued later than verification date");
     }
 
     // check exp expiration
     if (assertionParams.expiresAt) {
-      if (assertionParams.expiresAt.valueOf() < now.valueOf())
-        return cb("assertion has expired");
+      if (assertionParams.expiresAt.valueOf() < now.valueOf()) {
+        return cb("expired");
+      }
     }
 
     cb(null, payload, assertionParams);

diff --git a/lib/cert.js b/lib/cert.js
@@ -29,6 +29,7 @@ var serializeCertParamsInto = function(certParams, params) {
 SERIALIZER._LEGACY_extractCertParamsFrom = function(params) {
   var certParams = {};
 
+
   certParams.publicKey = jwcrypto.loadPublicKey(JSON.stringify(params['public-key']));
   delete params['public-key'];
   certParams.principal = params.principal;
@@ -138,13 +139,24 @@ var verifyChain = function(certs, now, getRoot, cb) {
       // we're done
       cb(null, certParamsArray);
     });
-
   });
-
 };
 
 exports.verifyChain = verifyChain;
 
+// msg is an error message returned by .verify, entity is either 'assertion' or
+// 'certificate'
+function improveVerifyErrorMessage(err, entity) {
+  // allow through the malformed signature
+  if (err === "issued later than verification date" ||
+      err === "expired") {
+    err = entity + " " + err;
+  } else if (err !== 'malformed signature') {
+    err = "bad signature in chain";
+  }
+  return err;
+}
+
 exports.verifyBundle = function(bundle, now, getRoot, cb) {
   // unbundle
   if (typeof(bundle) !== 'string' && !(bundle instanceof String)) {
@@ -162,23 +174,16 @@ exports.verifyBundle = function(bundle, now, getRoot, cb) {
 
   // verify the chain
   verifyChain(certs, now, getRoot, function(err, certParamsArray) {
-    // simplify error message
-    if (err) {
-      // allow through the malformed signature
-      if (err === 'malformed signature' ||
-          err === "assertion issued later than verification date" ||
-          err === "assertion has expired")
-        return cb(err);
-      else
-        return cb("bad signature in chain");
-    }
+    // ergonomic error messages
+    if (err) return cb(improveVerifyErrorMessage(err, 'certificate'));
 
     // what was the last PK in the successful chain?
     var lastPK = certParamsArray[certParamsArray.length - 1].certParams.publicKey;
 
     // now verify the assertion
     assertion.verify(signedAssertion, lastPK, now, function(err, payload, assertionParams) {
-      if (err) return cb(err);
+      // ergonomic error messages
+      if (err) return cb(improveVerifyErrorMessage(err, 'assertion'));
 
       // we're good!
       cb(null, certParamsArray, payload, assertionParams);

diff --git a/test/assertion-test.js b/test/assertion-test.js
@@ -78,7 +78,7 @@ testUtils.addBatches(suite, function(alg, keysize) {
             assert.isNotNull(assertionParams.expiresAt);
             assert.isNotNull(assertionParams.issuer);
             assert.isNotNull(assertionParams.audience);
-            assert.equal(assertionParams.audience, "https://example.com");            
+            assert.equal(assertionParams.audience, "https://example.com");
             assert.equal(assertionParams.expiresAt.valueOf(), in_a_minute.valueOf());
           }
         }
@@ -124,7 +124,7 @@ testUtils.addBatches(suite, function(alg, keysize) {
             assert.isUndefined(payload);
           },
           "returns the right error message": function(err, payload, assertionParams) {
-            assert.equal(err, "assertion has expired");
+            assert.equal(err, "expired");
           }
         }
       },
@@ -156,7 +156,7 @@ testUtils.addBatches(suite, function(alg, keysize) {
             assert.isNotNull(payload.iss);
             assert.isUndefined(payload.aud);
             assert.equal(payload.exp, in_a_minute.valueOf());
-            assert.equal(payload.iat, in_a_minute.valueOf());            
+            assert.equal(payload.iat, in_a_minute.valueOf());
           }
         },
         "when verified with assertion": {
@@ -169,7 +169,7 @@ testUtils.addBatches(suite, function(alg, keysize) {
             assert.isUndefined(payload);
           },
           "returns the right error message": function(err, payload, assertionParams) {
-            assert.equal(err, "assertion issued later than verification date");
+            assert.equal(err, "issued later than verification date");
           }
         }
       }

diff --git a/test/vectors-test.js b/test/vectors-test.js
@@ -142,9 +142,9 @@ suite.addBatch(
           this.callback);
       },
       "fails appropriately": function(err, certParamsArray, payload, assertionParams) {
-        assert.equal(err, "assertion has expired");
+        assert.equal(err, "certificate expired");
       }
-    }    
+    }
 })
 
 suite.addBatch(