SG-44103 Mask secrets in git descriptors URLs - #1131
SG-44103 Mask secrets in git descriptors URLs#1131carlos-villavicencio-adsk wants to merge 10 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1131 +/- ##
==========================================
- Coverage 80.09% 80.09% -0.01%
==========================================
Files 203 203
Lines 19537 19619 +82
==========================================
+ Hits 15649 15713 +64
- Misses 3888 3906 +18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| except Exception as e: | ||
| log.error("Unexpected error: %s: %s", e.__class__.__name__, e) |
There was a problem hiding this comment.
Why don't you want to keep the logger exception?
Or maybe you can use logger.error with the exc_info=True parameter?
Same question for python/tank/descriptor/io_descriptor/git_branch.py.
Baiscally, logger.exception would provide better information. So what's the problem keeping using it?
There was a problem hiding this comment.
We must use plain log.error() without exception info to avoid the credential leak.
There was a problem hiding this comment.
Pull request overview
This PR improves security around Git-based descriptors by sanitizing embedded credentials (usernames/passwords/tokens) from URL string representations, logged git commands, and exceptions to reduce the risk of secret leakage in logs and error messages.
Changes:
- Add URL/command/exception sanitization helpers to the git IO descriptor layer and apply them in key logging/error paths.
- Update git tag/branch descriptor string/error handling to sanitize repository URLs and subprocess exceptions.
- Add tests to validate sanitization behavior across URLs, descriptor repr/str, and exception handling; bump ruff pre-commit version.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/descriptor_tests/test_git.py | Adds unit/integration coverage for URL, repr/str, and exception sanitization. |
| python/tank/descriptor/io_descriptor/git.py | Introduces sanitization helpers and applies them to logging/errors and descriptor repr. |
| python/tank/descriptor/io_descriptor/git_tag.py | Sanitizes URL usage in __str__ and error handlers for tag-based descriptors. |
| python/tank/descriptor/io_descriptor/git_branch.py | Sanitizes branch descriptor string output and improves exception handling to avoid credential leakage. |
| .pre-commit-config.yaml | Updates ruff pre-commit hook version. |
Suppressed comments (4)
tests/descriptor_tests/test_git.py:382
- This command string contains a redacted URL literal, so _sanitize_command() can't sanitize credentials and the test will fail.
cmd_string = 'git clone "https://user:pass@example.com/repo.git" /tmp/repo'
tests/descriptor_tests/test_git.py:339
- The descriptor test uses a redacted URL literal; build the URL dynamically so the test actually exercises username:password sanitization.
"path": "https://user:pass@example.com/repo.git",
"version": "v1.0.0",
python/tank/descriptor/io_descriptor/git.py:382
- _sanitize_exception() is invoked without the URL here as well; passing self._path helps ensure any URL echoed in the exception output is sanitized.
# Sanitize any credentials that might be in the exception
if isinstance(e, SubprocessCalledProcessError):
e = _sanitize_exception(e)
log.debug("...could not establish connection: %s" % e)
python/tank/descriptor/io_descriptor/git.py:325
- _sanitize_exception() is called without the URL; passing self._path allows output sanitization too, in case git reports the remote URL in stderr/stdout.
except SubprocessCalledProcessError as e:
# Sanitize the exception to remove any potential credentials
sanitized_exc = _sanitize_exception(e)
raise TankGitError(
f"Error executing GIT operation '{_sanitize_command(full_command)}': {sanitized_exc.output}"
f" (Return code {sanitized_exc.returncode}). "
" Supported GIT version: 1.9+."
)
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This pull request introduces significant improvements to the handling of sensitive information in Git-related descriptors by ensuring that credentials (such as usernames, passwords, or tokens) are sanitized from logs, error messages, and string representations. The changes add utility functions for sanitizing URLs and exceptions, and update several methods and exception handlers to use these utilities, thereby reducing the risk of leaking secrets in logs or error outputs.
These changes collectively improve the security and robustness of the codebase by systematically preventing accidental exposure of sensitive credentials in logs and error messages.