-
Notifications
You must be signed in to change notification settings - Fork 0
tests: scrub inherited git env pointers at package import #87
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| ### Changed | ||
|
|
||
| - `tests/__init__.py` scrubs inherited per-repo git environment pointers | ||
| (`GIT_DIR`, `GIT_WORK_TREE`, `GIT_INDEX_FILE`, `GIT_PREFIX`, | ||
| `GIT_OBJECT_DIRECTORY`, `GIT_ALTERNATE_OBJECT_DIRECTORIES`, | ||
| `GIT_COMMON_DIR`) at package import, so any runner — not only the sanitized | ||
| pre-push hook — is safe from the incident where a hook-inherited absolute | ||
| `GIT_DIR` made temp-directory git calls inside the suite mutate the real | ||
| repository config. `GIT_CONFIG_GLOBAL`/`GIT_CONFIG_SYSTEM` are deliberately | ||
| untouched (temp-repo commits rely on machine identity); the `scripts/` root | ||
| is deliberately untouched (namespace package, and empirically unexposed — | ||
| 327/327 under a poisoned `GIT_DIR` with zero config writes). Regression | ||
| tests pin both the in-process absence and a freshly-poisoned child-import | ||
| scrub. Test infrastructure only — no version bump. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,30 @@ | ||
| # Marketplace-level test suite (cross-cutting checks that span both plugins). | ||
| # Per-plugin tests live under plugins/<name>/backend/tests/ or | ||
| # plugins/<name>/mcp-server/tests/. | ||
|
|
||
| import os | ||
|
|
||
| # Tests in this package spawn git in temporary directories. When the suite is | ||
| # run from a git hook (or any process where git has exported its environment), | ||
| # an inherited absolute GIT_DIR makes those temp-dir git calls target the REAL | ||
| # repository — observed 2026-08-03: a pre-push run set core.bare=true and a | ||
| # fixture user identity in the live .git/config (ledger | ||
| # plugin.tests.inherit.git.hook.env.mutate.real.repo). Scrub the per-repo | ||
| # pointers at package import so every runner is safe, not just the hook (the | ||
| # .githooks/pre-push wrapper independently sanitizes its suite subshells). | ||
| # GIT_CONFIG_GLOBAL/GIT_CONFIG_SYSTEM are deliberately left alone: tests that | ||
| # commit in temp repositories rely on the machine's global identity. A test | ||
| # that deliberately exercises poisoned-git-env behavior must set its variables | ||
| # AFTER this package import (e.g., in the test body or a subprocess env), as | ||
| # the scrub runs once at import time. | ||
| for _var in ( | ||
| "GIT_DIR", | ||
| "GIT_WORK_TREE", | ||
| "GIT_INDEX_FILE", | ||
| "GIT_PREFIX", | ||
| "GIT_OBJECT_DIRECTORY", | ||
| "GIT_ALTERNATE_OBJECT_DIRECTORIES", | ||
| "GIT_COMMON_DIR", | ||
| ): | ||
| os.environ.pop(_var, None) | ||
| del _var | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| #!/usr/bin/env python3 | ||
| """Regression test for the tests-package git-env scrub (see tests/__init__.py). | ||
|
|
||
| Inherited per-repo git pointers (an absolute GIT_DIR exported by a git hook) | ||
| made temp-dir git calls in this suite mutate the real repository config. | ||
| The package __init__ scrubs them at import; this test pins that property. | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import os | ||
| import subprocess | ||
| import sys | ||
| import unittest | ||
| from pathlib import Path | ||
|
|
||
| SCRUBBED = ( | ||
| "GIT_DIR", | ||
| "GIT_WORK_TREE", | ||
| "GIT_INDEX_FILE", | ||
| "GIT_PREFIX", | ||
| "GIT_OBJECT_DIRECTORY", | ||
| "GIT_ALTERNATE_OBJECT_DIRECTORIES", | ||
| "GIT_COMMON_DIR", | ||
| ) | ||
|
|
||
| REPO_ROOT = Path(__file__).resolve().parents[1] | ||
|
|
||
|
|
||
| class GitEnvHardeningTests(unittest.TestCase): | ||
| def test_scrubbed_variables_are_absent_after_package_import(self) -> None: | ||
| # By the time any test in this package runs, tests/__init__.py has | ||
| # imported and the pointers must be gone regardless of the runner. | ||
| for var in SCRUBBED: | ||
| self.assertNotIn(var, os.environ, var) | ||
|
|
||
| def test_import_scrubs_a_freshly_poisoned_environment(self) -> None: | ||
| # Run a child interpreter with every pointer poisoned; importing the | ||
| # tests package must remove them all. | ||
| env = dict(os.environ) | ||
| for var in SCRUBBED: | ||
| env[var] = "/nonexistent/poison" | ||
| code = ( | ||
| "import os, sys; sys.path.insert(0, sys.argv[1]); import tests; " | ||
| "leaked = [v for v in sys.argv[2:] if v in os.environ]; " | ||
| "print(','.join(leaked) or 'CLEAN')" | ||
| ) | ||
| res = subprocess.run( | ||
| [sys.executable, "-c", code, str(REPO_ROOT), *SCRUBBED], | ||
| capture_output=True, | ||
| text=True, | ||
| env=env, | ||
| check=False, | ||
| ) | ||
| self.assertEqual(res.returncode, 0, res.stderr) | ||
| self.assertEqual(res.stdout.strip(), "CLEAN") | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| unittest.main() |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GIT_CONFIGbefore running temp-repo commandsWhen the suite inherits
GIT_CONFIG=/path/to/config, this allowlist leaves it active, so temp-repository calls such asPublicExportSafetyTests._init()runninggit config user.email/user.namesilently modify that external file rather than the temporary repo. This reproduces the configuration-corruption class the change is intended to prevent; Git 2.43 also includesGIT_CONFIGingit rev-parse --local-env-vars, and the official documentation says it takes configuration from the named file instead of.git/config(Git documentation). AddGIT_CONFIGto the scrubbed variables while continuing to preserve the intentionally retained global/system settings.Useful? React with 👍 / 👎.