Merge pull request #432 from mozilla/check-hibp-scan-response-415

Check hibp scan response 415

.env-dist

@@ -44,3 +44,7 @@ HIBP_RELOAD_BREACHES_TIMER=600
 # HIBP API for range search and subscription
 HIBP_KANON_API_ROOT="https://api.haveibeenpwned.com"
 HIBP_KANON_API_TOKEN=""
+# How many milliseconds to wait before retrying an HIBP request
+HIBP_THROTTLE_DELAY=2000
+# Max number of times to try an HIBP request before throwing error
+HIBP_THROTTLE_MAX_TRIES=5

app-constants.js

@@ -29,6 +29,8 @@ const kEnvironmentVariables = [
   "HIBP_API_ROOT",
   "HIBP_API_TOKEN",
   "HIBP_RELOAD_BREACHES_TIMER",
+  "HIBP_THROTTLE_DELAY",
+  "HIBP_THROTTLE_MAX_TRIES",
   "DATABASE_URL",
   "SERVER_URL",
   "DELETE_UNVERIFIED_SUBSCRIBERS_TIMER",

hibp.js

@@ -23,17 +23,43 @@ const HIBP = {
     return Object.assign(options, hibpOptions);
   },
 
+  async _throttledGot (url, reqOptions, tryCount = 1) {
+    let response;
+    try {
+      response = await got(url, reqOptions);
+      return response;
+    } catch (err) {
+      console.error("got an error: " + err);
+      if (err.statusCode === 404) {
+        // 404 can mean "no results", return undefined response; sorry calling code
+        return response;
+      } else if (err.statusCode === 429) {
+        console.log("got a 429, tryCount: ", tryCount);
+        if (tryCount >= AppConstants.HIBP_THROTTLE_MAX_TRIES) {
+          console.error(err.message);
+          throw new Error("Too many connections to HIBP.");
+        } else {
+          tryCount++;
+          await new Promise(resolve => setTimeout(resolve, AppConstants.HIBP_THROTTLE_DELAY * tryCount));
+          return await this._throttledGot(url, reqOptions, tryCount);
+        }
+      } else {
+        throw new Error("Error connecting to HIBP.");
+      }
+    }
+  },
+
   async req(path, options = {}) {
     const url = `${AppConstants.HIBP_API_ROOT}${path}?code=${encodeURIComponent(AppConstants.HIBP_API_TOKEN)}`;
     const reqOptions = this._addStandardOptions(options);
-    return await got(url, reqOptions);
+    return await this._throttledGot(url, reqOptions);
   },
 
   async kAnonReq(path, options = {}) {
     // Construct HIBP url and standard headers
     const url = `${AppConstants.HIBP_KANON_API_ROOT}${path}?code=${encodeURIComponent(AppConstants.HIBP_KANON_API_TOKEN)}`;
     const reqOptions = this._addStandardOptions(options);
-    return await got(url, reqOptions);
+    return await this._throttledGot(url, reqOptions);
   },
 
   async loadBreachesIntoApp(app) {
@@ -52,7 +78,7 @@ const HIBP = {
       app.locals.breachesLoadedDateTime = Date.now();
       app.locals.mostRecentBreachDateTime = this.getLatestBreachDateTime(breaches);
     } catch (error) {
-      console.error(error);
+      throw new Error("Could not load breaches: " + error);
     }
     console.log("Done loading breaches.");
   },
@@ -78,22 +104,22 @@ const HIBP = {
     const sha1Prefix = sha1.slice(0, 6).toUpperCase();
     const path = `/breachedaccount/range/${sha1Prefix}`;
 
-    try {
-      const response = await this.kAnonReq(path);
-      // Parse response body, format:
-      // [
-      //   {"hashSuffix":<suffix>,"websites":[<breach1Name>,...]},
-      //   {"hashSuffix":<suffix>,"websites":[<breach1Name>,...]},
-      // ]
-      for (const breachedAccount of response.body) {
-        if (sha1.toUpperCase() === sha1Prefix + breachedAccount.hashSuffix) {
-          foundBreaches = allBreaches.filter(breach => breachedAccount.websites.includes(breach.Name));
-          break;
-        }
-      }
-    } catch (error) {
-      console.error(error);
+    const response = await this.kAnonReq(path);
+    if (!response) {
+      return [];
     }
+    // Parse response body, format:
+    // [
+    //   {"hashSuffix":<suffix>,"websites":[<breach1Name>,...]},
+    //   {"hashSuffix":<suffix>,"websites":[<breach1Name>,...]},
+    // ]
+    for (const breachedAccount of response.body) {
+      if (sha1.toUpperCase() === sha1Prefix + breachedAccount.hashSuffix) {
+        foundBreaches = allBreaches.filter(breach => breachedAccount.websites.includes(breach.Name));
+        break;
+      }
+    }
+
     if (includeUnsafe) {
       return foundBreaches;
     }
@@ -135,13 +161,7 @@ const HIBP = {
       body: {hashPrefix: sha1Prefix},
     };
 
-    let response;
-    try {
-      response = await this.kAnonReq(path, options);
-    } catch (error) {
-      console.error(error);
-    }
-    return response;
+    return await this.kAnonReq(path, options);
   },
 };
 

package.json

@@ -85,7 +85,7 @@
     "start": "node server.js",
     "test:db:migrate": "knex migrate:latest --knexfile db/knexfile.js --env tests",
     "test:db:seed": "knex seed:run --knexfile db/knexfile.js --env tests",
-    "test:tests": "NODE_ENV=tests jest --runInBand --coverage tests/",
+    "test:tests": "NODE_ENV=tests HIBP_THROTTLE_DELAY=1000 HIBP_THROTTLE_MAX_TRIES=3 jest --runInBand --coverage tests/",
     "test:coveralls": "cat ./coverage/lcov.info | coveralls",
     "test": "run-s test:db:migrate test:db:seed test:tests test:coveralls"
   }

tests/controllers/hibp.test.js

@@ -17,6 +17,8 @@ test("notify POST with breach, subscriber hash prefix and suffixes should call s
   const testPrefix = testHash.slice(0, 6).toUpperCase();
   const testSuffix = testHash.slice(6).toUpperCase();
 
+  HIBPLib.getUnsafeBreachesForEmail = jest.fn();
+
   const mockRequest = { body: { hashPrefix: testPrefix, hashSuffixes: [testSuffix], breachName: "Test" }, app: { locals: { breaches: testBreaches } } };
   const mockResponse = { status: jest.fn(), json: jest.fn() };
 

tests/controllers/ses.test.js

@@ -7,6 +7,7 @@ const getSha1 = require("../../sha1-utils");
 const ses = require("../../controllers/ses");
 
 require("../resetDB");
+jest.mock("../../hibp");
 
 
 const testNotifications = new Map();

tests/controllers/user.test.js

@@ -13,6 +13,7 @@ require("../resetDB");
 
 
 jest.mock("../../email-utils");
+jest.mock("../../hibp");
 
 
 test("user add POST with email adds unverified subscriber and sends verification email", async () => {

tests/hibp.test.js

@@ -3,6 +3,7 @@
 const got = require("got");
 
 const AppConstants = require("../app-constants");
+const getSha1 = require("../sha1-utils");
 const hibp = require("../hibp");
 
 const { testBreaches } = require("./test-breaches");
@@ -59,3 +60,18 @@ test("filterOutUnsafeBreaches removes sensitive breaches", async() => {
     expect(breach.IsVerified).toBe(true);
   }
 });
+
+
+test("getBreachesForEmail HIBP responses with status of 429 cause throttled retries up to HIBP_THROTTLE_MAX_TRIES", async() => {
+  // Assumes running with max tries of 3 and delay of 1000
+  jest.setTimeout(20000);
+  got.mockClear();
+  got.mockRejectedValue( { statusCode: 429 });
+
+  await expect(hibp.getBreachesForEmail(getSha1("unverifiedemail@test.com"), testBreaches)).rejects.toThrow("Too many connections to HIBP.");
+
+  const gotCalls = got.mock.calls;
+  expect(gotCalls.length).toEqual(Number(AppConstants.HIBP_THROTTLE_MAX_TRIES));
+
+  jest.setTimeout(5000);
+});