Even more scrubbing (dbt-labs#5152)

* Even more scrubbing

* Changelog entry

* Even more

* remove reduendent scrub

* remove reduendent scrub

* fix encoding issue

* keep scrubbed log in args

Co-authored-by: Chenyu Li <[email protected]>

.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"

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)

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()))
 
 

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):