diff --git a/.changes/unreleased/Fixes-20220425-203924.yaml b/.changes/unreleased/Fixes-20220425-203924.yaml new file mode 100644 index 00000000000..97ddc3b7aee --- /dev/null +++ b/.changes/unreleased/Fixes-20220425-203924.yaml @@ -0,0 +1,7 @@ +kind: Fixes +body: Scrub secret env vars from CommandError in exception stacktrace +time: 2022-04-25T20:39:24.365495+02:00 +custom: + Author: jtcohen6 + Issue: "5151" + PR: "5152" diff --git a/core/dbt/clients/git.py b/core/dbt/clients/git.py index 6d3c484f371..9eaa93203e0 100644 --- a/core/dbt/clients/git.py +++ b/core/dbt/clients/git.py @@ -28,7 +28,7 @@ def _is_commit(revision: str) -> bool: def _raise_git_cloning_error(repo, revision, error): - stderr = error.stderr.decode("utf-8").strip() + stderr = error.stderr.strip() if "usage: git" in stderr: stderr = stderr.split("\nusage: git")[0] if re.match("fatal: destination path '(.+)' already exists", stderr): @@ -115,8 +115,8 @@ def checkout(cwd, repo, revision=None): try: return _checkout(cwd, repo, revision) except CommandResultError as exc: - stderr = exc.stderr.decode("utf-8").strip() - bad_package_spec(repo, revision, stderr) + stderr = exc.stderr.strip() + bad_package_spec(repo, revision, stderr) def get_current_sha(cwd): @@ -142,7 +142,7 @@ def clone_and_checkout( subdirectory=subdirectory, ) except CommandResultError as exc: - err = exc.stderr.decode("utf-8") + err = exc.stderr exists = re.match("fatal: destination path '(.+)' already exists", err) if not exists: raise_git_cloning_problem(repo) diff --git a/core/dbt/exceptions.py b/core/dbt/exceptions.py index b760c3a9d3a..1293dce1090 100644 --- a/core/dbt/exceptions.py +++ b/core/dbt/exceptions.py @@ -383,10 +383,11 @@ class FailedToConnectException(DatabaseException): class CommandError(RuntimeException): def __init__(self, cwd, cmd, message="Error running command"): + cmd_scrubbed = list(scrub_secrets(cmd_txt, env_secrets()) for cmd_txt in cmd) super().__init__(message) self.cwd = cwd - self.cmd = cmd - self.args = (cwd, cmd, message) + self.cmd = cmd_scrubbed + self.args = (cwd, cmd_scrubbed, message) def __str__(self): if len(self.cmd) == 0: @@ -411,9 +412,9 @@ class CommandResultError(CommandError): def __init__(self, cwd, cmd, returncode, stdout, stderr, message="Got a non-zero returncode"): super().__init__(cwd, cmd, message) self.returncode = returncode - self.stdout = stdout - self.stderr = stderr - self.args = (cwd, cmd, returncode, stdout, stderr, message) + self.stdout = scrub_secrets(stdout.decode("utf-8"), env_secrets()) + self.stderr = scrub_secrets(stderr.decode("utf-8"), env_secrets()) + self.args = (cwd, self.cmd, returncode, self.stdout, self.stderr, message) def __str__(self): return "{} running: {}".format(self.msg, self.cmd) @@ -704,7 +705,6 @@ def missing_materialization(model, adapter_type): def bad_package_spec(repo, spec, error_message): msg = "Error checking out spec='{}' for repo {}\n{}".format(spec, repo, error_message) - raise InternalException(scrub_secrets(msg, env_secrets())) diff --git a/test/integration/013_context_var_tests/test_context_vars.py b/test/integration/013_context_var_tests/test_context_vars.py index b2a16b14759..aac480065d3 100644 --- a/test/integration/013_context_var_tests/test_context_vars.py +++ b/test/integration/013_context_var_tests/test_context_vars.py @@ -221,6 +221,36 @@ def test_postgres_allow_secrets(self): self.assertFalse("first_dependency" in log_output) +class TestCloneFailSecretScrubbed(DBTIntegrationTest): + + def setUp(self): + os.environ[SECRET_ENV_PREFIX + "GIT_TOKEN"] = "abc123" + DBTIntegrationTest.setUp(self) + + @property + def packages_config(self): + return { + "packages": [ + {"git": "https://fakeuser:{{ env_var('DBT_ENV_SECRET_GIT_TOKEN') }}@github.com/dbt-labs/fake-repo.git"}, + ] + } + + @property + def schema(self): + return "context_vars_013" + + @property + def models(self): + return "models" + + @use_profile('postgres') + def test_postgres_fail_clone_with_scrubbing(self): + with self.assertRaises(dbt.exceptions.InternalException) as exc: + _, log_output = self.run_dbt_and_capture(['deps']) + + assert "abc123" not in str(exc.exception) + + class TestEmitWarning(DBTIntegrationTest): @property def schema(self):