safe.bareRepository: default to "explicit" with WITH_BREAKING_CHANGES#2072
Open
dscho wants to merge 46 commits intogitgitgadget:masterfrom
Open
safe.bareRepository: default to "explicit" with WITH_BREAKING_CHANGES#2072dscho wants to merge 46 commits intogitgitgadget:masterfrom
dscho wants to merge 46 commits intogitgitgadget:masterfrom
Conversation
dscho
force-pushed
the
make-safe.bareRepositories-the-default-in-git-3.0
branch
from
54b6c08 to
a30a801
Compare
|
There are issues in commit 8365979:
|
|
There are issues in commit 9d4fafe:
|
|
There are issues in commit 9d973bb:
|
dscho
force-pushed
the
make-safe.bareRepositories-the-default-in-git-3.0
branch
from
a30a801 to
56cd924
Compare
|
The pull request has 46 commits. The max allowed is 30. Please split the patch series into multiple pull requests. Also consider squashing related commits. |
|
There are merge commits in this Pull Request: Please rebase the branch and force-push. |
dscho
force-pushed
the
make-safe.bareRepositories-the-default-in-git-3.0
branch
from
56cd924 to
d394f8c
Compare
|
The pull request has 45 commits. The max allowed is 30. Please split the patch series into multiple pull requests. Also consider squashing related commits. |
|
There are merge commits in this Pull Request: Please rebase the branch and force-push. |
8d1a744 (setup.c: create `safe.bareRepository`, 2022-07-14) introduced a setting to restrict implicit bare repository discovery, mitigating a social-engineering attack where an embedded bare repo's hooks get executed unknowingly. To allow for that default to change at some stage in the future, the tests need to be prepared. This commit adjusts a test accordingly that runs `git aliasedinit` from inside a bare repo to verify that aliased commands work there. The test is about alias resolution, not bare repo discovery, so add `test_config_global safe.bareRepository all` to opt in explicitly. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho
force-pushed
the
make-safe.bareRepositories-the-default-in-git-3.0
branch
from
d394f8c to
027d941
Compare
|
The pull request has 45 commits. The max allowed is 30. Please split the patch series into multiple pull requests. Also consider squashing related commits. |
|
There are merge commits in this Pull Request: Please rebase the branch and force-push. |
To prepare for `safe.bareRepository` defaulting to `explicit` (see 8d1a744), replace `cd <dir> && git config` with `git --git-dir=<dir> config` so the helper does not rely on implicit bare repository discovery. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
The bare repo tests in t0003-attributes.sh currently `cd` into the bare repository inside subshells, relying on implicit discovery. Restructure these tests to pass `--git-dir=bare.git` to the `attr_check` and `attr_check_source` helpers instead. This makes the code much easier to read, and also makes bare repo access explicit, i.e. compatible with an eventual `safe.bareRepository=explicit` default. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
The `git -C c/a.git --work-tree=../a` invocations in t0056-git-C.sh enter what is technically the `.git` directory of a repository to test `-C` combined with `--work-tree`. In doing so, the code relies on implicit discovery of bare repositories, which 8d1a744 (setup.c: create `safe.bareRepository`, 2022-07-14) prepared to be prevented by default. These tests verify the interaction between those flags, so changing them to use `--git-dir` would defeat their purpose. So let's just temporarily force-enable implicit discovery of bare repositories, no matter what `safe.bareRepository` defaults to. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Replace an unnecessarily complex subshell pattern with a much simpler `--git-dir`-based one. The latter is not only simpler, it also no longer relies on implicit bare repo discovery, which would fail with `safe.bareRepository=explicit`. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
To prepare for `safe.bareRepository` defaulting to `explicit` (see 8d1a744), add an optional 6th parameter `repo_flag` (defaulting to `-C`) to the `test_repo_info` helper, and use it in the caller that wants to operate on a bare repository. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
To prepare for `safe.bareRepository` defaulting to `explicit` (see 8d1a744), specify the gitdir specifically in bare-repo `git worktree add` invocations via `--git-dir=.` so Git does not rely on implicit bare repository discovery. While at it, also avoid unnecessary subshells and `cd`ing. This simplifies the logic in a rather pleasant way. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
To prepare for `safe.bareRepository` defaulting to `explicit` (see 8d1a744), the test case t2406.10(repair .git file from bare.git) cannot rely on the implicit discovery of thee bare repository. Simply add a `--git-dir=.` to the invocation. The `-C bare.git` argument is still needed so that the `repair` command realizes works on the intended directory. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
The test case "fetch specific OID with tag following" creates a bare repository and wants to operate on it by changing the working directory and relying on Git's implicit discovery of the bare repository. Once the `safe.bareRepository` default is changed, this is no longer an option. So let's adjust the commands to specify the bare repository explicitly, via `--git-dir`, and avoid changing the working directory. As a bonus, the result is arguably more readable than the original code. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
To prepare for `safe.bareRepository` defaulting to `explicit` (see 8d1a744), export `GIT_DIR=.` right after `git init --bare &&` so subsequent commands access the bare repo explicitly. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
After switching from `-C pushee` to `--git-dir=pushee` as part of the `safe.bareRepository` preparation, `ext::` URLs that used `.` (resolved relative to the `-C` target) must spell out the directory name explicitly. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho
force-pushed
the
make-safe.bareRepositories-the-default-in-git-3.0
branch
from
027d941 to
7287728
Compare
|
The pull request has 43 commits. The max allowed is 30. Please split the patch series into multiple pull requests. Also consider squashing related commits. |
|
There are merge commits in this Pull Request: Please rebase the branch and force-push. |
dscho
force-pushed
the
make-safe.bareRepositories-the-default-in-git-3.0
branch
from
7287728 to
6c246b0
Compare
|
The pull request has 44 commits. The max allowed is 30. Please split the patch series into multiple pull requests. Also consider squashing related commits. |
Preparing for `safe.bareRepository` defaulting to `explicit` (see 8d1a744), change the `post_checkout_hook` helper from hard-coding `test_hook -C "$1"` to forwarding all arguments via `test_hook "$@"`, so callers can pass `--git-dir <dir>` for bare repos. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Preparing for `safe.bareRepository` defaulting to `explicit` (see 8d1a744), add `--git-dir` option parsing to `test_cmp_refs` in t5411/common-functions.sh, parallel to the existing `-C` handling. When given, the helper passes `--git-dir="$gitdir"` to `git show-ref`. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
As part of the preparation for `safe.bareRepository` defaulting to `explicit` (see 8d1a744), switch bare-repo calls from `test_cmp_refs -C` to `test_cmp_refs --git-dir` in the t5411 proc-receive test files. To make reviewing this commit easier, here is a command-line to undo the transformation on the new lines; empty output is success: git diff HEAD^! | awk ' /^diff/,/^\+\+\+/ { next } /^-/ { old[m++] = substr($0,2) } /^\+/ { new = substr($0,2) # undo: --git-dir -> -C gsub(/test_cmp_refs --git-dir /, "test_cmp_refs -C ", new) if (old[n++] != new) print old[n-1] " != " new }' Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho
force-pushed
the
make-safe.bareRepositories-the-default-in-git-3.0
branch
from
6c246b0 to
8a98e1f
Compare
|
The pull request has 46 commits. The max allowed is 30. Please split the patch series into multiple pull requests. Also consider squashing related commits. |
|
There are merge commits in this Pull Request: Please rebase the branch and force-push. |
As part of the preparation for `safe.bareRepository` defaulting to `explicit` (see 8d1a744), switch `test_hook ... -C` to `test_hook ... --git-dir` for bare repos. To verify this commit mechanically, here is an awk command-line that undoes the transformation on the new lines; empty output is success: git diff HEAD^! | awk ' /^diff/,/^\+\+\+/ { next } /^-/ { old[m++] = substr($0,2) } /^\+/ { new = substr($0,2) # undo: --git-dir -> -C gsub(/ --git-dir /, " -C ", new) if (old[n++] != new) print old[n-1] " != " new }' Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
This commit replaces a couple of `-C <dir>` invocations where
`<dir>` is a bare repository with `--git-dir=<dir>` as part of the
`safe.bareRepository` preparation.
Since `--git-dir` does not change the working directory, paths that
duplicated the directory name as a prefix are now redundant.
This commit can be validated in a mechanical way via this command-line:
git diff HEAD^! | awk '
/^diff/,/^\+\+\+/ { next }
/^-/ { old[m++] = substr($0, 2) }
/^\+/ {
line = substr($0,2)
# apply: -C X -> --git-dir=X and strip ../
if (match(line, /--git-dir=([^ ]*)(.* )([^ ]*)\//, a) &&
a[1] == a[3]) {
gsub(/--git-dir=.*\//, "-C " a[1] a[2], line)
}
if (old[n++] != line) print old[n-1] " != " line
}'
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
To avoid having to rely on implicit discovery of bare repositories
(which will go away at some stage by flipping the default of
`safe.bareRepository`), this commit replaces `-C <dir>` with
`--git-dir=<dir>`. Since `--git-dir` does not change the working
directory, a `.` that formerly referred to the `-C` target must become
`..` to reach the same location.
The code changes can be validated via this command-line:
git diff HEAD^! | awk '
/^diff/,/^\+\+\+/ { next }
/^-/ { old[m++] = substr($0, 2) }
/^\+/ {
line = substr($0,2)
# apply: -C X -> --git-dir=X and strip ../
if (match(line, /--git-dir=(.* )\. /, a)) {
gsub(/--git-dir=.* \. /, "-C " a[1] ".. ", line)
}
if (old[n++] != line) print old[n-1] " != " line
}'
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
There are quite a few tests that assume that the default of the
`safe.bareRepository` setting is that it allows discovery of bare
repositories. This is unsafe. To allow the default to change, this
commit tackles a couple of places in the test suite: By setting GIT_DIR
after changing the current working directory to the bare repository,
Git no longer needs to discover it, and by unsetting GIT_DIR as soon
as leaving said directory, Git once again returns to the regular
programming of discovering the `.git` directory.
This commit can be validated mechanically by running the following
command-line:
git diff HEAD^! | awk '
/^diff/,/^\+\+\+/ { next }
/^-/ { old[m++] = substr($0,2) }
/^\+/ {
new = substr($0,2)
# undo: strip GIT_DIR export
gsub(/ GIT_DIR=[.] && export GIT_DIR &&/, "", new)
# undo: strip sane_unset GIT_DIR
gsub(/ && sane_unset GIT_DIR/, "", new)
if (old[n++] != new) print old[n-1] " != " new
}'
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
The default of the `safe.bareRepository` will change at some stage, at
which time bare repositories won't be discovered implicitly in general
anymore.
When `git reset` is called with an explicit gitdir (e.g. by using the
`--git-dir` option), it behaves differently from an implicit gitdir,
though: In the former case, it trusts the user to know what they are
doing, in the latter case it will refuse certain modes because they
require a worktree. For that reason, we have to adjust the test cases
that verify this behavior not by specifying the gitdir explicitly, but
instead enforcing implicit gitdir discovery.
The test cases that want to verify that the Git prompt shows
`(GIT_DIR!)` when a gitdir was discovered implicitly _also_ need the
same treatment.
Mechanical verification of the code changes can be performed via this
here command-line (the awk script undoes the transformation on the new
lines; empty output is success):
git diff HEAD^! | awk '
/^diff/,/^\+\+\+/ { next }
/^-/ { old[m++] = substr($0,2) }
/^\+/ {
new = substr($0,2)
# undo: strip GIT_CONFIG_PARAMETERS and export
gcp="GIT_CONFIG_PARAMETERS"
gsub(" " gcp "=[^ ]* && export " gcp " &&", "", new)
if (old[n++] != new) print old[n-1] " != " new
}'
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
An easy way to prepare the tests for a time when the
`safe.bareRepository` default changes is to replace `-C <dir>` with
`--git-dir=<dir>` in those invocations that concern bare repositories.
However, in some cases, further adjustments are required: Since `-C`
changes the work directory, relative paths specified in the Git command
in parameters have to change, too.
This commit takes care of the `git -C <dir>` invocations that specify
paths in the current directory, via `../<name>` arguments. Naturally,
these arguments now need to drop that `../` prefix after no longer
changing the working directory.
This commit can be verified mechanically via this awk script that
applies the transformation on the old lines; empty output is success:
git diff HEAD^! | awk '
/^diff/,/^\+\+\+/ { next }
/^-/ {
line = substr($0,2)
# apply: -C X -> --git-dir=X and strip ../
if (match(line, /-C ([^ ]*)/, a)) {
gsub(/-C [^ ]*/, "--git-dir=" a[1], line)
}
gsub(/\.\.\//, "", line)
old[m++] = line
}
/^\+/ { new = substr($0,2); if (old[n++] != new) print old[n-1] " != " new }'
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
As part of the preparation for `safe.bareRepository` defaulting to `explicit` (see 8d1a744), add `GIT_DIR=. && export GIT_DIR &&` right after `cd <bare> &&` so that subsequent commands access the bare repo explicitly rather than relying on implicit discovery. Here is a command-line to help verify this commit mechanically (the awk script undoes the transformation on the new lines; empty output is success): git diff HEAD^! | awk ' /^diff/,/^\+\+\+/ { next } /^-/ { old[m++] = substr($0,2) } /^\+/ { new = substr($0,2) # undo: strip GIT_DIR export gsub(/ && GIT_DIR=[.] && export GIT_DIR/, "", new) if (old[n++] != new) print old[n-1] " != " new }' Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho
force-pushed
the
make-safe.bareRepositories-the-default-in-git-3.0
branch
from
8a98e1f to
fe0c788
Compare
|
The pull request has 46 commits. The max allowed is 30. Please split the patch series into multiple pull requests. Also consider squashing related commits. |
|
There are merge commits in this Pull Request: Please rebase the branch and force-push. |
As part of the preparation for `safe.bareRepository` defaulting to `explicit` (see 8d1a744), replace `git -C <bare>` with `git --git-dir=<bare>`. The `-C` flag triggers implicit bare repository discovery, whereas `--git-dir` specifies the repository location explicitly. This commit can be verified mechanically by invoking this command-line (the awk script undoes the transformation on the new lines; empty output is success): git diff HEAD^! | awk ' /^diff/,/^\+\+\+/ { next } /^-/ { old[m++] = substr($0,2) } /^\+/ { new = substr($0,2) # undo: --git-dir=X -> -C X if (match(new, /--git-dir=([^ ]*)/, a)) { gsub(/--git-dir=[^ ]*/, "-C " a[1], new) } if (old[n++] != new) print old[n-1] " != " new }' Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
When an attacker can convince a user to clone a crafted repository that contains an embedded bare repository with malicious hooks, any Git command the user runs after entering that subdirectory will discover the bare repository and execute the hooks. The user does not even need to run a Git command explicitly: many shell prompts run `git status` in the background to display branch and dirty state information, and `git status` in turn may invoke the fsmonitor hook if so configured, making the user vulnerable the moment they `cd` into the directory. The `safe.bareRepository` configuration variable (introduced in 8959555 (setup_git_directory(): add an owner check for the top-level directory, 2022-03-02)) already provides protection against this attack vector by allowing users to set it to "explicit", but the default remained "all" for backwards compatibility. Since Git 3.0 is the natural point to change defaults to safer values, flip the default from "all" to "explicit" when built with `WITH_BREAKING_CHANGES`. This means Git will refuse to work with bare repositories that are discovered implicitly by walking up the directory tree. Bare repositories specified via `--git-dir` or `GIT_DIR` continue to work, and directories that look like `.git`, worktrees, or submodule directories are unaffected (the existing `is_implicit_bare_repo()` whitelist handles those cases). Users who rely on implicit bare repository discovery can restore the previous behavior by setting `safe.bareRepository=all` in their global or system configuration. The test for the "safe.bareRepository in the repository" scenario needed a more involved fix: it writes a `safe.bareRepository=all` entry into the bare repository's own config to verify that repo-local config does not override the protected (global) setting. Previously, `test_config -C` was used to write that entry, but its cleanup runs `git -C <bare-repo> config --unset`, which itself fails when the default is "explicit" and the global config has already been cleaned up. Switching to direct git config --file access avoids going through repository discovery entirely. Assisted-by: Claude Opus 4.6 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho
force-pushed
the
make-safe.bareRepositories-the-default-in-git-3.0
branch
from
fe0c788 to
bda3baf
Compare
|
The pull request has 46 commits. The max allowed is 30. Please split the patch series into multiple pull requests. Also consider squashing related commits. |
|
There are merge commits in this Pull Request: Please rebase the branch and force-push. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
In one of my projects, I work exclusively on bare repositories. During one of the tests, I noticed that
safe.bareRepositoryis not yet enabled by default. This strikes me as undesirable, and I deem Git v3.0 the most logical opportunity to change the default.Cc: Patrick Steinhardt ps@pks.im