safe.bareRepository: default to "explicit" with WITH_BREAKING_CHANGES#2072
safe.bareRepository: default to "explicit" with WITH_BREAKING_CHANGES#2072dscho wants to merge 46 commits intogitgitgadget:masterfrom
Conversation
54b6c08 to
a30a801
Compare
|
There are issues in commit 8365979:
|
|
There are issues in commit 9d4fafe:
|
|
There are issues in commit 9d973bb:
|
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. |
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. |
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. |
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. |
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. |
|
There are merge commits in this Pull Request: Please rebase the branch and force-push. |
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. |
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. |
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. |
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>
8d1a744 (setup.c: create `safe.bareRepository`, 2022-07-14) introduced the `safe.bareRepository` config setting that can reject implicit bare repository discovery. It defaults to `all` for now, but the commit message already describes the social engineering attack that motivates changing that default to `explicit` at some point. When that day comes, any `test-tool -C <bare-repo>` invocation will trigger implicit discovery and be refused. Add a `--git-dir=<path>` option that works the same way `git --git-dir=<path>` does: it calls `setenv(GIT_DIR_ENVIRONMENT, ...)` before dispatching to the subcommand, bypassing repository discovery entirely. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
To prepare for `safe.bareRepository` defaulting to `explicit`, pass `--bare` at all bare-repo call sites in t5318 so the helpers use `--git-dir` instead of `-C`. 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>
Currently, the "alternate bare repo" test case relies on Git discovering non-bare and bare repositories alike. However, the automatic discovery of bare repository represents a weakness that leaves Git users vulnerable. To that end, the `safe.bareRepository` config was introduced, but out of backwards-compatibility concerns, the default is not yet secure. To prepare for that default to switch to the secure one, where bare repositories are never discovered automatically but instead must be specified explicitly, let's do exactly that in this test case: specify it explicitly, via setting the environment variable `GIT_DIR`. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
The `test_hook` helper installs hook scripts into a repository. It currently only supports `-C <dir>` to locate the target, which triggers implicit bare repository discovery and would fail under `safe.bareRepository=explicit`. Add a `--git-dir <dir>` option so that the helper can locate bare repositories explicitly. When given, the internal `git rev-parse --absolute-git-dir` call that resolves the hooks directory uses `--git-dir=<dir>` instead of `-C <dir>`. See 8d1a744 (setup.c: create `safe.bareRepository`, 2022-07-14) for the background on implicit bare repository discovery and why it may become the default to reject it. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
In the spirit of 8d1a744 (setup.c: create `safe.bareRepository`, 2022-07-14), Git's test suite should be prepared for a potential default change of that setting from `all` to `explicit`. The `test_config` and `test_unconfig` helpers use `git -C <dir> config` under the hood, and that triggers implicit bare repository discovery when <dir> is a bare repository. Add a `--git-dir` option alongside the existing `-C` option. When specified, the helpers pass `--git-dir=<dir>` instead of `-C <dir>` to the underlying `git config` call, and the `test_when_finished` cleanup registered by `test_config` uses `test_unconfig --git-dir` accordingly. This ensures that both the set and the unset paths bypass discovery. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
The `print_all_reflog_entries` and `test_migration` helpers in t1460-refs-migrate.sh previously hard-coded `-C` to enter the repository. With `safe.bareRepository=explicit`, bare repos accessed via `-C` are rejected. Teach both helpers to accept `--git-dir` as the first argument, and pass the chosen flag through to both `git` and `test-tool` invocations. Callers testing bare repos now pass `--git-dir`, while non-bare callers continue to use `-C`. 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>
When `safe.bareRepository` will change to be safe by default, bare repositories won't be discovered by default anymore. To prepare for this, `git p4` must be explicit about the gitdir when cloning into a bare repository, and no longer rely on that implicit discovery. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
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>
As part of the preparation for `safe.bareRepository` defaulting to `explicit` (see 8d1a744), switch `test_config -C <bare>` to `test_config --git-dir <bare>` so the config helper accesses bare repos without implicit discovery. For your convenience, here is an awk command-line to verify this commit: It mechanically 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(/test_config --git-dir /, "test_config -C ", new) if (old[n++] != new) print old[n-1] " != " new }' 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>
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 `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>
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>
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>
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>
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>
bda3baf to
435e350
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. |
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