Skip to content

Commit

Permalink
fix(toolkit): scrutiny dialog should fail with no tty (#1382)
Browse files Browse the repository at this point in the history
If STDIN is not connected to a TTY (terminal), and scrutiny is enabled,
we expect the program to fail (exit with non-zero exit code).

This is especially important for CI/CD scenarios where you wouldn't want
to accidentally deploy changes that didn't pass a scrutiny check.

Added integration test.

Fixes #1380
  • Loading branch information
Elad Ben-Israel authored Dec 19, 2018
1 parent 8c733ef commit 478a714
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 0 deletions.
8 changes: 8 additions & 0 deletions packages/aws-cdk/bin/cdk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,14 @@ async function initCommandLine() {
if (requireApproval !== RequireApproval.Never) {
const currentTemplate = await readCurrentTemplate(stack);
if (printSecurityDiff(currentTemplate, stack, requireApproval)) {

// only talk to user if we STDIN is a terminal (otherwise, fail)
if (!process.stdin.isTTY) {
throw new Error(
'"--require-approval" is enabled and stack includes security-sensitive updates, ' +
'but terminal (TTY) is not attached so we are unable to get a confirmation from the user');
}

const confirmed = await confirm(`Do you wish to deploy these changes (y/n)?`);
if (!confirmed) { throw new Error('Aborted by user'); }
}
Expand Down
1 change: 1 addition & 0 deletions packages/aws-cdk/integ-tests/common.bash
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ function cleanup_stack() {
function cleanup() {
cleanup_stack cdk-toolkit-integration-test-1
cleanup_stack cdk-toolkit-integration-test-2
cleanup_stack cdk-toolkit-integration-iam-test
}

function setup() {
Expand Down
16 changes: 16 additions & 0 deletions packages/aws-cdk/integ-tests/test-cdk-deploy-no-tty.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
#!/bin/bash
set -euo pipefail
scriptdir=$(cd $(dirname $0) && pwd)
source ${scriptdir}/common.bash
# ----------------------------------------------------------

setup

# redirect /dev/null to stdin, which means there will not be tty attached
# since this stack includes security-related changes, the deployment should
# immediately fail because we can't confirm the changes
if cdk deploy cdk-toolkit-integration-iam-test < /dev/null; then
fail "test failed. we expect 'cdk deploy' to fail if there are security-related changes and no tty"
fi

echo "✅ success"

0 comments on commit 478a714

Please sign in to comment.