From e096f425edcd79e0233113d03961da388e9e695a Mon Sep 17 00:00:00 2001 From: LucioMS Date: Wed, 5 Jul 2023 12:01:51 -0700 Subject: [PATCH] bug: Local Credential Hijacking fix => round 2 https://github.com/alexa/ask-cli/pull/475 --- .../authorization-controller/index.js | 17 +++++++++++------ .../authorization-controller/questions.js | 7 +++++++ lib/controllers/authorization-controller/ui.js | 12 ++++++++++++ .../authorization-controller/index-test.js | 4 +++- 4 files changed, 33 insertions(+), 7 deletions(-) diff --git a/lib/controllers/authorization-controller/index.js b/lib/controllers/authorization-controller/index.js index fa30712d..b2518aed 100644 --- a/lib/controllers/authorization-controller/index.js +++ b/lib/controllers/authorization-controller/index.js @@ -153,7 +153,7 @@ module.exports = class AuthorizationController { if (requestUrl.startsWith("/cb?code")) { response.end(messages.ASK_SIGN_IN_SUCCESS_MESSAGE); ui.confirmAllowSignIn((error, confirmSignInChoice) => { - // Closing the socket port with server.destroy() only after confirmation question. + // Closing the socket port with server.destroy() only after confirmation question. // See https://github.com/alexa/ask-cli/issues/476 server.destroy(); @@ -164,17 +164,22 @@ module.exports = class AuthorizationController { 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}`; + const errorMessage = `Error: ${requestQuery.error}\nReason: ${requestQuery.error_description}`.split("\n").join(". "); response.end(messages.ASK_SIGN_IN_FAILURE_MESSAGE(errorMessage)); - callback(errorMessage); + ui.informReceivedError((error, _) => { + // Closing the socket port with server.destroy() only after informing of error. + // See https://github.com/alexa/ask-cli/issues/476 + server.destroy(); + response.statusCode = 403; + callback(errorMessage); + }, errorMessage); } } } diff --git a/lib/controllers/authorization-controller/questions.js b/lib/controllers/authorization-controller/questions.js index 451eb4d2..70f14e3a 100644 --- a/lib/controllers/authorization-controller/questions.js +++ b/lib/controllers/authorization-controller/questions.js @@ -7,4 +7,11 @@ module.exports = { default: true, }, ], + INFORM_ERROR: { + // message is filled out in code + type: "list", + choices: ["Ok"], + name: "choice", + default: "Ok", + }, }; diff --git a/lib/controllers/authorization-controller/ui.js b/lib/controllers/authorization-controller/ui.js index ea429c58..4edeb39f 100644 --- a/lib/controllers/authorization-controller/ui.js +++ b/lib/controllers/authorization-controller/ui.js @@ -3,6 +3,7 @@ const questions = require("./questions"); module.exports = { confirmAllowSignIn, + informReceivedError }; export function confirmAllowSignIn(callback) { inquirer @@ -14,3 +15,14 @@ export function confirmAllowSignIn(callback) { callback(error); }); } + +export function informReceivedError(callback, error) { + inquirer + .prompt([{...questions.INFORM_ERROR, message: `Sign in error: ${error}.`}]) + .then((answer) => { + callback(null, answer.choice); + }) + .catch((error) => { + callback(error); + }); +} \ No newline at end of file diff --git a/test/unit/controller/authorization-controller/index-test.js b/test/unit/controller/authorization-controller/index-test.js index 05f01b9b..19d198a7 100644 --- a/test/unit/controller/authorization-controller/index-test.js +++ b/test/unit/controller/authorization-controller/index-test.js @@ -422,6 +422,7 @@ describe("Controller test - Authorization controller test", () => { // setup sinon.stub(LocalHostServer.prototype, "listen"); sinon.stub(LocalHostServer.prototype, "registerEvent"); + const informReceivedErrorStub = sinon.stub(ui, "informReceivedError").callsArgWith(0, null, "Ok"); const requestDestroyStub = sinon.stub(); const request = { url: "/cb?error", @@ -447,12 +448,13 @@ describe("Controller test - Authorization controller test", () => { // call authorizationController._listenResponseFromLWA(TEST_PORT, (err) => { // verify - const EXPECTED_ERR_MESSAGE = `Error: ${requestQuery.query.error}\nReason: ${requestQuery.query.error_description}`; + const EXPECTED_ERR_MESSAGE = `Error: ${requestQuery.query.error}\nReason: ${requestQuery.query.error_description}`.split("\n").join(". "); expect(spinnerTerminateStub.callCount).eq(1); expect(serverDestroyStub.callCount).eq(1); expect(endStub.callCount).eq(1); expect(endStub.args[0][0].includes(EXPECTED_ERR_MESSAGE)).equal(true); expect(err).eq(EXPECTED_ERR_MESSAGE); + expect(informReceivedErrorStub.called).to.be.true; done(); }); });