Merge pull request #475 from alexa/lumas/localCred

fix: Local Credential Hijacking fix
alexa · Jun 13, 2023 · 40d98ef · 40d98ef
2 parents aaeee37 + c18201d
commit 40d98ef
Show file tree

Hide file tree

Showing 5 changed files with 101 additions and 35 deletions.
diff --git a/lib/controllers/authorization-controller/index.js b/lib/controllers/authorization-controller/index.js
@@ -11,6 +11,7 @@ const Messenger = require("../../view/messenger");
 const SpinnerView = require("../../view/spinner-view");
 
 const messages = require("./messages");
+const ui = require("./ui");
 
 module.exports = class AuthorizationController {
   /**
@@ -147,11 +148,29 @@ module.exports = class AuthorizationController {
       listenSpinner.terminate();
       const requestUrl = request.url;
       const requestQuery = url.parse(requestUrl, true).query;
-      server.destroy();
+
       if (requestUrl.startsWith("/cb?code")) {
         response.end(messages.ASK_SIGN_IN_SUCCESS_MESSAGE);
-        callback(null, requestQuery.code);
+        ui.confirmAllowSignIn((error, confirmSignInChoice) => {
+          // After confirmed or not browser sign in, closes the socket/port 
+          // with server.destroy().
+          // We need to keep the port open so a local hacker is not be able to 
+          // open that port until we get an answer in confirmAllowSignIn
+          server.destroy();
+
+          if (error) {
+            return callback(error);
+          }
+
+          if (!confirmSignInChoice) {
+            return callback(messages.STOP_UNCONFIRMED_BROWSER_SIGNIN);
+          }
+
+          callback(null, requestQuery.code);
+        });
+        return;
       }
+      server.destroy();
       if (requestUrl.startsWith("/cb?error")) {
         response.statusCode = 403;
         const errorMessage = `Error: ${requestQuery.error}\nReason: ${requestQuery.error_description}`;

diff --git a/lib/controllers/authorization-controller/messages.js b/lib/controllers/authorization-controller/messages.js
@@ -56,3 +56,5 @@ module.exports.ASK_SIGN_IN_FAILURE_MESSAGE = (error) => `
 `;
 
 module.exports.ASK_ENV_VARIABLES_ERROR_MESSAGE = "Could not find either of the environment variables: ASK_ACCESS_TOKEN, ASK_REFRESH_TOKEN";
+
+module.exports.STOP_UNCONFIRMED_BROWSER_SIGNIN = "Stopping configuration due to unconfirmed browser sign in.";
diff --git a/lib/controllers/authorization-controller/questions.js b/lib/controllers/authorization-controller/questions.js
@@ -0,0 +1,10 @@
+module.exports = {
+  CONFIRM_ALLOW_BROWSER_SIGN_IN: [
+    {
+      message: "Do you confirm that you used the browser to sign in to Alexa Skills Kit Tools?",
+      type: "confirm",
+      name: "choice",
+      default: true,
+    },
+  ],
+};
diff --git a/lib/controllers/authorization-controller/ui.js b/lib/controllers/authorization-controller/ui.js
@@ -0,0 +1,16 @@
+const inquirer = require("inquirer");
+const questions = require("./questions");
+
+module.exports = {
+  confirmAllowSignIn,
+};
+export function confirmAllowSignIn(callback) {
+  inquirer
+    .prompt(questions.CONFIRM_ALLOW_BROWSER_SIGN_IN)
+    .then((answer) => {
+      callback(null, answer.choice);
+    })
+    .catch((error) => {
+      callback(error);
+    });
+}
diff --git a/test/unit/controller/authorization-controller/index-test.js b/test/unit/controller/authorization-controller/index-test.js
@@ -14,6 +14,7 @@ const CONSTANTS = require("../../../../lib/utils/constants");
 const LocalHostServer = require("../../../../lib/utils/local-host-server");
 const Messenger = require("../../../../lib/view/messenger");
 const SpinnerView = require("../../../../lib/view/spinner-view");
+const ui = require("../../../../lib/controllers/authorization-controller/ui");
 
 describe("Controller test - Authorization controller test", () => {
   const DEFAULT_CLIENT_ID = CONSTANTS.LWA.CLI_INTERNAL_ONLY_LWA_CLIENT.CLIENT_ID;
@@ -456,40 +457,58 @@ describe("Controller test - Authorization controller test", () => {
       });
     });
 
-    it("| local server returns valid authCode", (done) => {
-      // setup
-      sinon.stub(LocalHostServer.prototype, "listen");
-      sinon.stub(LocalHostServer.prototype, "registerEvent");
-      const requestDestroyStub = sinon.stub();
-      const request = {
-        url: "/cb?code",
-        socket: {
-          destroy: requestDestroyStub,
-        },
-      };
-      const endStub = sinon.stub();
-      const response = {
-        on: sinon.stub().callsArgWith(1),
-        end: endStub,
-      };
-      sinon.stub(SpinnerView.prototype, "terminate");
-      const requestQuery = {
-        query: {
-          code: TEST_AUTH_CODE,
-        },
-      };
-      sinon.stub(url, "parse").returns(requestQuery);
-      sinon.stub(LocalHostServer.prototype, "destroy");
-      sinon.stub(LocalHostServer.prototype, "create").callsArgWith(0, request, response);
+    describe("local server ", () => {
+      let endStub;
+      beforeEach(() => {
+        sinon.stub(LocalHostServer.prototype, "listen");
+        sinon.stub(LocalHostServer.prototype, "registerEvent");
+        const requestDestroyStub = sinon.stub();
+        const request = {
+          url: "/cb?code",
+          socket: {
+            destroy: requestDestroyStub,
+          },
+        };
+        endStub = sinon.stub();
+        const response = {
+          on: sinon.stub().callsArgWith(1),
+          end: endStub,
+        };
+        sinon.stub(SpinnerView.prototype, "terminate");
+        const requestQuery = {
+          query: {
+            code: TEST_AUTH_CODE,
+          },
+        };
+        sinon.stub(url, "parse").returns(requestQuery);
+        sinon.stub(LocalHostServer.prototype, "destroy");
+        sinon.stub(LocalHostServer.prototype, "create").callsArgWith(0, request, response);
+      });
 
-      // call
-      authorizationController._listenResponseFromLWA(TEST_PORT, (err, authCode) => {
-        // verify
-        expect(endStub.callCount).eq(1);
-        expect(endStub.args[0][0]).eq(messages.ASK_SIGN_IN_SUCCESS_MESSAGE);
-        expect(err).eq(null);
-        expect(authCode).eq(TEST_AUTH_CODE);
-        done();
+      it("| returns valid authCode", (done) => {
+        sinon.stub(ui, "confirmAllowSignIn").callsArgWith(0, null, true);
+        // call
+        authorizationController._listenResponseFromLWA(TEST_PORT, (err, authCode) => {
+          // verify
+          expect(endStub.callCount).eq(1);
+          expect(endStub.args[0][0]).eq(messages.ASK_SIGN_IN_SUCCESS_MESSAGE);
+          expect(err).eq(null);
+          expect(authCode).eq(TEST_AUTH_CODE);
+          done();
+        });
+      });
+
+      it("| returns error for unconfirmed browser sign in", (done) => {
+        sinon.stub(ui, "confirmAllowSignIn").callsArgWith(0, null, false);
+        // call
+        authorizationController._listenResponseFromLWA(TEST_PORT, (err, authCode) => {
+          // verify
+          expect(endStub.callCount).eq(1);
+          expect(endStub.args[0][0]).eq(messages.ASK_SIGN_IN_SUCCESS_MESSAGE);
+          expect(err).eq(messages.STOP_UNCONFIRMED_BROWSER_SIGNIN);
+          expect(authCode).eq(undefined);
+          done();
+        });
       });
     });
   });
Original file line number	Diff line number	Diff line change
Expand Up		@@ -56,3 +56,5 @@ module.exports.ASK_SIGN_IN_FAILURE_MESSAGE = (error) => `
		`;

		module.exports.ASK_ENV_VARIABLES_ERROR_MESSAGE = "Could not find either of the environment variables: ASK_ACCESS_TOKEN, ASK_REFRESH_TOKEN";

		module.exports.STOP_UNCONFIRMED_BROWSER_SIGNIN = "Stopping configuration due to unconfirmed browser sign in.";