Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions hooks/_common.sh
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,52 @@ function common::parse_cmdline {
done
}

#######################################################################
# Scrub four dangerous GIT_* env vars before child git operations.
Comment thread
yermulnik marked this conversation as resolved.
Outdated
#
# Without this, `git commit` from a linked git worktree fails at
# tree-build when a hook runs `tofu init` / `terraform init` (which
# spawns `git clone` for each module source):
#
# error: invalid object 100644 <oid> for '<path>'
# error: Error building trees
#
# Root cause: the child `git clone` inherits GIT_INDEX_FILE from the
# parent `git commit` env (pointing at the worktree's index), then
# writes the cloned module's blob OIDs into the parent worktree's
# index. The subsequent commit cannot resolve those OIDs.
#
# Targeted denylist, NOT a mirror of pre-commit's `no_git_env`:
# `no_git_env` (pre_commit/git.py) uses an ALLOWLIST that scrubs all
# GIT_* except ~11 known-safe vars (GIT_EXEC_PATH, GIT_SSH,
# GIT_SSH_COMMAND, GIT_ASKPASS, GIT_SSL_CAINFO, GIT_SSL_NO_VERIFY,
# GIT_CONFIG_KEY_*, GIT_CONFIG_VALUE_*, GIT_CONFIG_COUNT,
# GIT_HTTP_PROXY_AUTHMETHOD, GIT_ALLOW_PROTOCOL). It runs only on
# pre-commit's INTERNAL git calls, not on user hook subprocesses
# (per pre-commit/pre-commit#1849).
#
# We unset only the four vars that pre-commit's own docstring (git.py
# lines 20-38) names as dangerous when leaked into child git:
Comment thread
yermulnik marked this conversation as resolved.
Outdated
#
# GIT_INDEX_FILE proximate cause; "Causes 'error invalid
# object ...' during commit" in git.py.
# GIT_DIR "Causes git clone to clone wrong thing".
# GIT_WORK_TREE defensive; paired with GIT_DIR.
# GIT_OBJECT_DIRECTORY defensive; prevents child writes into the
# parent object DB via non-clone git calls.
Comment thread
yermulnik marked this conversation as resolved.
Outdated
#
# Denylist over allowlist mirror because:
# 1. Bash equivalent of the Python whitelist scan is ~15-20 lines
# versus a one-line `unset`.
# 2. The observed bug is fully fixed by scrubbing GIT_INDEX_FILE
# alone; the other three are belt-and-braces.
# 3. Narrower scrub minimizes risk of breaking hook authors who
# rely on inheritance of vars an aggressive allowlist might drop.
#######################################################################
function common::scrub_git_env {
unset GIT_INDEX_FILE GIT_DIR GIT_WORK_TREE GIT_OBJECT_DIRECTORY
Comment thread
MaxymVlasov marked this conversation as resolved.
Outdated
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

#######################################################################
# Expand environment variables definition into their values in '--args'.
# Support expansion only for ${ENV_VAR} vars, not $ENV_VAR.
Expand Down
1 change: 1 addition & 0 deletions hooks/terraform_docs.sh
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ function main {
common::parse_cmdline "$@"
common::export_provided_env_vars "${ENV_VARS[@]}"
common::parse_and_export_env_vars
common::scrub_git_env
Comment thread
yermulnik marked this conversation as resolved.
Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I can't reproduce the worktree bug in this hook.

Suggested change
common::scrub_git_env

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

There another thing that shows, both for terraform_docs and terraform_fmt hooks.

But it's not an issue at all.

18:57 repro-hoo git:(repro-hoo +) 
➜ gc       
Terraform docs...........................................................Passed
Terraform validate with tflint...........................................Passed
Terraform fmt............................................................Failed
- hook id: terraform_fmt
- files were modified by this hook

main.tf


18:57 repro-hoo git:(repro-hoo !+) 
✘1 ➜ gc
Terraform docs...........................................................Passed
Terraform validate with tflint...........................................Passed
Terraform fmt............................................................Passed
Terraform docs.......................................(no files to check)Skipped
Terraform validate with tflint.......................(no files to check)Skipped
Terraform fmt........................................(no files to check)Skipped
[repro-hoo 44cd287] fmt
 2 files changed, 2 insertions(+), 6 deletions(-)

18:58 repro-hoo git:(repro-hoo) took 9s 
➜ git fsck --no-progress | head
dangling tree bba236455e65937967031c5d25b02e11f5c252f5

18:58 repro-hoo git:(repro-hoo) 
➜ git fsck --no-progress | head
dangling tree bba236455e65937967031c5d25b02e11f5c252f5

18:58 repro-hoo git:(repro-hoo) 
➜ gc                           
Terraform docs...........................................................Failed
- hook id: terraform_docs
- files were modified by this hook
Terraform validate with tflint...........................................Passed
Terraform fmt............................................................Passed

18:58 repro-hoo git:(repro-hoo !+) 
✘1 ➜ gc
Terraform docs...........................................................Passed
Terraform validate with tflint...........................................Passed
Terraform fmt............................................................Passed
Terraform docs.......................................(no files to check)Skipped
Terraform validate with tflint.......................(no files to check)Skipped
Terraform fmt........................................(no files to check)Skipped
[repro-hoo c115ce4] docs
 2 files changed, 2 insertions(+), 2 deletions(-)

18:58 repro-hoo git:(repro-hoo) took 6s 
➜ git fsck --no-progress | head
dangling tree a13f3782d35b8ce91a9d45f5b57acd390ea671a2
dangling tree bba236455e65937967031c5d25b02e11f5c252f5

# Support for setting relative PATH to .terraform-docs.yml config.
for i in "${!ARGS[@]}"; do
ARGS[i]=${ARGS[i]/--config=/--config=$(pwd)\/}
Expand Down
1 change: 1 addition & 0 deletions hooks/terraform_tflint.sh
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ function main {
common::parse_cmdline "$@"
common::export_provided_env_vars "${ENV_VARS[@]}"
common::parse_and_export_env_vars
common::scrub_git_env

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I can't reproduce the worktree bug in this hook.

Suggested change
common::scrub_git_env


# JFYI: tflint color already suppressed via PRE_COMMIT_COLOR=never

Expand Down
1 change: 1 addition & 0 deletions hooks/terraform_validate.sh
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ function main {
common::parse_cmdline "$@"
common::export_provided_env_vars "${ENV_VARS[@]}"
common::parse_and_export_env_vars
common::scrub_git_env

# Suppress terraform validate color
if [ "$PRE_COMMIT_COLOR" = "never" ]; then
Expand Down