Skip to content

tests: scrub inherited git env pointers at package import - #87

Merged
sumitake merged 1 commit into
mainfrom
dev/claude/test-git-env-hardening
Aug 3, 2026
Merged

tests: scrub inherited git env pointers at package import#87
sumitake merged 1 commit into
mainfrom
dev/claude/test-git-env-hardening

Conversation

@sumitake

@sumitake sumitake commented Aug 3, 2026

Copy link
Copy Markdown
Owner

Summary

Defense-in-depth follow-up to #86, operator-directed: tests/__init__.py now 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 every runner of the suite is safe from the incident class (hook-inherited absolute GIT_DIR making temp-dir git calls mutate the real .git/config), not just the sanitized pre-push wrapper.

Scoping decisions (each verified or reasoned in the diff comment):

  • GIT_CONFIG_GLOBAL/GIT_CONFIG_SYSTEM deliberately untouched — temp-repo commits rely on machine identity.
  • scripts/ root deliberately untouched — namespace package (no __init__ to ride) AND empirically unexposed: 327/327 pass under a poisoned absolute GIT_DIR with zero config writes.
  • Tests that deliberately exercise poisoned git env must set variables after package import (documented in the comment; the hooks: full-suite pre-push test gate (both unittest roots, git-env sanitized) #86 wrapper test already does this via subprocess env).

Regression tests (2, in tests/test_git_env_hardening.py): in-process absence of all seven pointers, and a freshly-poisoned child interpreter whose import tests must scrub them all.

Verification: the incident reproduction (poisoned absolute GIT_DIR → 23 errors) now runs 658/658 green with zero config mutation; clean runs unaffected (660/660 with the new tests; 327/327 scripts). Test infrastructure only — no version bump; changelog fragment included.

Cross-check (verbatim)

Codex (gpt-5.6-sol, effort low, direct codex exec under operator broker-bypass authorization):

PROCEED

Concerns (all integrated): document that poisoned-env tests must set variables post-import — added to the comment; regression test asserting variable absence — added (both in-process and child-import forms); global-config exclusion and scripts/ exclusion judged justified as scoped.

author: claude
author_model: claude-fable-5 (effort: default)
standing_directives: operator-directed follow-up (2026-08-03 "Proceed with the test-setUp git-env hardening"); directive #9 defense-in-depth for ledger plugin.tests.inherit.git.hook.env.mutate.real.repo; #7 model+effort stated; #8 no fan-out
tier: 2
cross_check: PROCEED (codex gpt-5.6-sol, effort low, direct codex exec under operator broker-bypass authorization 2026-08-03; raw verdict quoted in PR body; both actionable concerns integrated)
post_condition: 660/660 + 327/327 tests OK; incident reproduction now green (was 23 errors) with zero config mutation; changelog dry-run OK; push traversed the #86 gate
mcp_coverage_gap: none — test infrastructure; gemini_mcp_path: retired / not applicable
contributor_rights: OWNER-AUTHORED
operator_reserved: no
plugin_affected: NONE — test infrastructure, no distributed plugin content
readme_refresh_status: README unaffected (no release, no version bump)

🤖 Generated with Claude Code

Defense-in-depth follow-up to #86: any runner of the tests/ suite is
now safe from hook-inherited GIT_DIR poisoning, not just the sanitized
pre-push wrapper. Regression tests pin in-process absence and a
freshly-poisoned child-import scrub. scripts/ root verified unexposed
(327/327 under poisoned GIT_DIR, zero config writes) and left as-is.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@sumitake
sumitake merged commit fbd629b into main Aug 3, 2026
15 checks passed
@sumitake
sumitake deleted the dev/claude/test-git-env-hardening branch August 3, 2026 21:29

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: dd453149d3

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread tests/__init__.py
Comment on lines +27 to +28
"GIT_COMMON_DIR",
):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Scrub GIT_CONFIG before running temp-repo commands

When the suite inherits GIT_CONFIG=/path/to/config, this allowlist leaves it active, so temp-repository calls such as PublicExportSafetyTests._init() running git config user.email/user.name silently 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 includes GIT_CONFIG in git rev-parse --local-env-vars, and the official documentation says it takes configuration from the named file instead of .git/config (Git documentation). Add GIT_CONFIG to the scrubbed variables while continuing to preserve the intentionally retained global/system settings.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant