From 60044c9c660965713b0d9aae94b9ac9f7f090d48 Mon Sep 17 00:00:00 2001 From: plocket <52798256+plocket@users.noreply.github.com> Date: Sat, 21 May 2022 22:19:43 -0400 Subject: [PATCH] Fix #559, improve security for secrets --- CHANGELOG.md | 4 ++++ lib/scope.js | 21 +++++++++++++++++---- lib/steps.js | 19 ++++++++++++++----- tests/features/reports.feature | 18 +++++++++++++++--- 4 files changed, 50 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1047d0a8..ceca00e5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,6 +42,10 @@ Format: ### Added - Appends the results of the cucumber [summary formatter](https://github.com/cucumber/cucumber-js/blob/main/docs/formatters.md#summary) to the `debug_log.txt`, which includes useful stack traces into the Kiln code when tests fail. +### Fixed +- Prevent login info from being saved in the report or screenshots being taken on error. [#599](https://github.com/SuffolkLITLab/ALKiln/issues/559). +- Prevent error screenshots of screens that used a secret. + ## [4.3.0] - 2022-04-07 ### Added - GitHub environment variable `MAX_SECONDS_FOR_SETUP` to set a custom maximum time for setup and takedown for packages that take a long time to load. diff --git a/lib/scope.js b/lib/scope.js index 57f923fd..8fe806a2 100644 --- a/lib/scope.js +++ b/lib/scope.js @@ -276,9 +276,15 @@ module.exports = { if ( options.waitForShowIf ) { await scope.waitForShowIf( scope ); } } // ends if scope.page - // Reset navigation (TODO: discuss use for observational steps) - if ( options.navigated ) { scope.navigated = true; } // Consider `daPageLoad` event - else { scope.navigated = false; } + if ( options.navigated ) { + // Reset navigation (TODO: discuss use for observational steps) + // TODO: Consider `daPageLoad` event + scope.navigated = true; + // Allow screenshots of the next page (may have been disabled in order to set secret values) + scope.disable_error_screenshot = false; + } else { + scope.navigated = false; + } // Plain old waiting. Here to avoid doing this in each individual place if ( options.waitForTimeout ) { await time.waitForTimeout( options.waitForTimeout ); } @@ -1836,6 +1842,8 @@ module.exports = { set_secret_var: async ( scope, var_name, set_to_env_name ) => { /** Sets a non-story table variable to a "secret" value (i.e. an env var) */ + // Prevent pictures of a screen with a secret. + scope.disable_error_screenshot = true; let var_data = await scope.normalizeTable( scope, { var_data: [{ var: var_name, @@ -1963,6 +1971,9 @@ module.exports = { /** Allow the developer to log a user into their server using GitHub * secrets to authenticate. */ + // Don't take a picture of a failed login in case one of the inputs is correct + scope.disable_error_screenshot = true; + // If there is no browser open, start a new one if (!scope.browser) { scope.browser = await scope.driver.launch({ headless: !session_vars.DEBUG }); @@ -2003,8 +2014,10 @@ module.exports = { await scope.addToReport( scope, { type: `row`, - value: `Logged into ${ session_vars.get_da_server_url() }/user/sign-in as a ${ email } with ${ password }.` + value: `Signed into ${ session_vars.get_da_server_url() }/user/sign-in successfully.` }); + + await scope.afterStep(scope, {waitForShowIf: false}); }, // Ends sign_in }, // ends scope.steps diff --git a/lib/steps.js b/lib/steps.js index 28a7f84b..32030c7f 100644 --- a/lib/steps.js +++ b/lib/steps.js @@ -83,6 +83,7 @@ Before(async (scenario) => { // Device interaction scope.device = 'pc'; scope.activate = click_with[ scope.device ]; + scope.disable_error_screenshot = false; scope.page_id = null; @@ -882,11 +883,19 @@ After(async function(scenario) { await scope.addToReport(scope, { type: `outcome`, value: `**-- Scenario Failed --**` }); if ( scope.page ) { - // Save/download a picture of the screen that's showing during the unexpected status - let safe_filename = await scope.getSafeScenarioFilename( scope, { prefix: `error` }); - await scope.page.screenshot({ path: `${ safe_filename }.jpg`, type: `jpeg`, fullPage: true }); - } - } // Ends if not passed + if ( scope.disable_error_screenshot ) { + scope.addToReport(scope, { + type: `row`, + value: `For security, ALKiln will not create a screenshot for this error. It's possible a secret is being used on this screen.` + }); + } else { + // Save/download a picture of the screen that's showing during the unexpected status + let safe_filename = await scope.getSafeScenarioFilename( scope, { prefix: `error` }); + await scope.page.screenshot({ path: `${ safe_filename }.jpg`, type: `jpeg`, fullPage: true }); + } // ends if scope.disable_error_screenshot + } // ends if scope.page exists + + } // ends if not passed if (scenario.result.status === `FAILED`) { await scope.addToReport(scope, { type: `outcome`, value: `**-- Scenario Failed --**` }); diff --git a/tests/features/reports.feature b/tests/features/reports.feature index e3f6dc40..3f7dd998 100644 --- a/tests/features/reports.feature +++ b/tests/features/reports.feature @@ -346,7 +346,7 @@ Scenario: I can't match JSON page var to str """ @fast @rf22 @signin -Scenario: Fail with wrong email secret +Scenario: Fail with wrong email secret name Given the final Scenario status should be "failed" Given the Scenario report should include: """ @@ -355,7 +355,7 @@ Scenario: Fail with wrong email secret   Given I log on with the email "WRONG_EMAIL_NAME" and the password "USER1_PASSWORD" @fast @rf23 @signin -Scenario: Fail with wrong password secret +Scenario: Fail with wrong password secret name Given the final Scenario status should be "failed" Given the Scenario report should include: """ @@ -364,7 +364,7 @@ Scenario: Fail with wrong password secret   Given I sign in with the email "USER1_EMAIL" and the password "WRONG_PASSWORD_NAME" @fast @rf24 @signin -Scenario: Fail with 2 wrong signin secrets +Scenario: Fail with 2 wrong signin secret names Given the final Scenario status should be "failed" Given the Scenario report should include: """ @@ -393,6 +393,10 @@ Scenario: Fail to find var while keeping value secret """ Did not find a field on this page for the variable "missing_var" that could be set to "SECRET_FOR_MISSING_FIELD" """ + And the Scenario report should include: + """ + For security, ALKiln will not create a screenshot for this error. + """ And I start the interview at "test_secret_vars" And I set the var "missing_var" to the secret "SECRET_FOR_MISSING_FIELD" @@ -518,3 +522,11 @@ Scenario: Report lists unused table rows | button_continue | True | | | buttons_other | button_2 | | | buttons_yesnomaybe | True | | + +@fast @rp3 @signin +Scenario: Sign in to server successfully + Given the Scenario report should include: + """ + Signed in + """ +  Given I sign in with the email "USER1_EMAIL" and the password "USER1_PASSWORD"