Various fixes around `safe.directory` #3791

derrickstolee · 2022-04-13T19:14:39Z

The first three commits are rebased versions of those in gitgitgadget#1215. These allow the following:

Fix git config --global foo.bar <path> from allowing the <path>. As a bonus, users with a config value starting with / will not get a warning about "old-style" paths needing a "%(prefix)/".
When in WSL, the path starts with / so it needs to be interpolated properly. Update the warning to include %(prefix)/ to get the right value for WSL users. (This is specifically for using Git for Windows from Git Bash, but in a WSL directory.)
When using WSL, the ownership check fails and reports an error message. This is noisy, and happens even if the user has marked the path with safe.directory. Remove that error message.

It is difficult to change the ownership on a directory in our test suite, so insert a new GIT_TEST_ASSUME_DIFFERENT_OWNER environment variable to trick Git into thinking we are in a differently-owned directory. This allows us to test that the config is parsed correctly. Signed-off-by: Derrick Stolee <derrickstolee@github.com>

It seems that nothing is ever checking to make sure the safe directories in the configs actually have the key safe.directory, so some unrelated config that has a value with a certain directory would also make it a safe directory. Signed-off-by: Matheus Valadares <me@m28.io> Signed-off-by: Derrick Stolee <derrickstolee@github.com>

niik · 2022-04-13T19:22:08Z

setup.c

-			sq_quote_buf_pretty(&quoted, dir.buf);
+#ifdef __MINGW32__
+			if (dir.buf[0] == '/')
+				strbuf_addstr(&prequoted, "%(prefix)/");


If this turns out to be the necessary solution we need to prepend %(preffix)/ to the unquoted as well as the quoted value. GitHub Desktop will use the unquoted value since we won't be passing it via a shell first

We do need to add the %(prefix)/. Without it, Git collapses // to / as in this debugger output:

Thread 1 hit Breakpoint 1, safe_directory_cb (key=0x204a56d6278 "safe.directory", value=0x204a56d0fd8 "//wsl.localhost/Ubuntu/home/stolee/_git/git", d=0xf674fff220) at setup.c:1045 1045 const char *interpolated = NULL; (gdb) n 1047 if (!git_config_pathname(&interpolated, key, value) && (gdb) 1048 !fspathcmp(data->path, interpolated ? interpolated : value)) (gdb) p interpolated $1 = 0x204a56d6a88 "/wsl.localhost/Ubuntu/home/stolee/_git/git" (gdb) p data->path $2 = 0x204a56d2808 "//wsl.localhost/Ubuntu/home/stolee/_git/git"

Then, we do add this to the quoted output because prequoted is the buffer being quoted in sq_quote_buf_pretty() (after appending dir.buf to prequoted).

Should we be more stringent and require two slashes before doing this?

I was using this hunk as a precedent for a single slash:

git/path.c

Lines 742 to 747 in 3f8d163

#ifdef __MINGW32__

if (path[0] == '/') {

warning(_("encountered old-style '%s' that should be '%%(prefix)/%s'"), path, path);

return system_path(path + 1);

}

#endif

Then, we do add this to the quoted output because prequoted is the buffer being quoted in sq_quote_buf_pretty() (after appending dir.buf to prequoted).

Right, I'm sorry, I'm not being terribly clear, it's late. What I meant was that with this change (if I'm reading it correctly) the output from the warning for a WSL path will be something like

unsafe repository ('//wsl/something' is owned by someone else) To add an exception for this directory, call: git config --global --add safe.directory %(prefix)//wsl/something

I.e the first path within the first line parenthesis will not have %(prefix) prepended right? What I'm saying is that it should because GitHub Desktop will parse that first path and use it to add the config.

I'm suggesting we swap out dir.buf on line 1392 for prequoted.buf so that both instances of the path has the %(prefix) prepended to them,

Ah, I get what you're saying now, @niik.

I'm not sure that adding %(prefix)/ there is a good idea, because that's not the internal representation of the path. The %(prefix)/ concept is only for parsing paths from config.

setup.c

dscho

I like almost all of this PR.

I'm not super happy with the WSL workaround, but can't seem to find any better solution.

With the addition of the safe.directory in 8959555 (setup_git_directory(): add an owner check for the top-level directory, 2022-03-02) released in v2.35.2, we are receiving feedback from a variety of users about the feature. Some users have a very large list of shared repositories and find it cumbersome to add this config for every one of them. In a more difficult case, certain workflows involve running Git commands within containers. The container boundary prevents any global or system config from communicating `safe.directory` values from the host into the container. Further, the container almost always runs as a different user than the owner of the directory in the host. To simplify the reactions necessary for these users, extend the definition of the safe.directory config value to include a possible '*' value. This value implies that all directories are safe, providing a single setting to opt-out of this protection. Note that an empty assignment of safe.directory clears all previous values, and this is already the case with the "if (!value || !*value)" condition. Signed-off-by: Derrick Stolee <derrickstolee@github.com>

Signed-off-by: Derrick Stolee <derrickstolee@github.com>

In the case of Git for Windows (say, in a Git Bash window) running in a Windows Subsystem for Linux (WSL) directory, the GetNamedSecurityInfoW() call in is_path_owned_By_current_side() returns an error code other than ERROR_SUCCESS. This is consistent behavior across this boundary. In these cases, the owner would always be different because the WSL owner is a different entity than the Windows user. The change here is to suppress the error message that looks like this: error: failed to get owner for '//wsl.localhost/...' (1) Before this change, this warning happens for every Git command, regardless of whether the directory is marked with safe.directory. Signed-off-by: Derrick Stolee <derrickstolee@github.com>

vdye

I only skimmed the first three patches (they, at a high-level, look good and are already planned for integration upstream). The last two look good as well. 🚀

derrickstolee · 2022-04-13T21:42:20Z

Thanks for the review, @vdye. We'll want to be careful about these recent fixes going into main and making sure they get applied to the upcoming v2.35.3 release.

The first three commits are rebased versions of those in gitgitgadget#1215. These allow the following: 1. Fix `git config --global foo.bar <path>` from allowing the `<path>`. As a bonus, users with a config value starting with `/` will not get a warning about "old-style" paths needing a "`%(prefix)/`". 2. When in WSL, the path starts with `/` so it needs to be interpolated properly. Update the warning to include `%(prefix)/` to get the right value for WSL users. (This is specifically for using Git for Windows from Git Bash, but in a WSL directory.) 3. When using WSL, the ownership check fails and reports an error message. This is noisy, and happens even if the user has marked the path with `safe.directory`. Remove that error message.

…irectory` The first three commits are rebased versions of those in gitgitgadget#1215. These allow the following: 1. Fix `git config --global foo.bar <path>` from allowing the `<path>`. As a bonus, users with a config value starting with `/` will not get a warning about "old-style" paths needing a "`%(prefix)/`". 2. When in WSL, the path starts with `/` so it needs to be interpolated properly. Update the warning to include `%(prefix)/` to get the right value for WSL users. (This is specifically for using Git for Windows from Git Bash, but in a WSL directory.) 3. When using WSL, the ownership check fails and reports an error message. This is noisy, and happens even if the user has marked the path with `safe.directory`. Remove that error message.

The first three commits are rebased versions of those in gitgitgadget#1215. These allow the following: 1. Fix `git config --global foo.bar <path>` from allowing the `<path>`. As a bonus, users with a config value starting with `/` will not get a warning about "old-style" paths needing a "`%(prefix)/`". 2. When in WSL, the path starts with `/` so it needs to be interpolated properly. Update the warning to include `%(prefix)/` to get the right value for WSL users. (This is specifically for using Git for Windows from Git Bash, but in a WSL directory.) 3. When using WSL, the ownership check fails and reports an error message. This is noisy, and happens even if the user has marked the path with `safe.directory`. Remove that error message.

derrickstolee and others added 2 commits April 13, 2022 14:49

derrickstolee requested review from dscho and vdye April 13, 2022 19:14

derrickstolee self-assigned this Apr 13, 2022

niik reviewed Apr 13, 2022

View reviewed changes

dscho approved these changes Apr 13, 2022

View reviewed changes

derrickstolee added 3 commits April 13, 2022 15:55

setup: properly use "%(prefix)/" when in WSL

e64daf2

Signed-off-by: Derrick Stolee <derrickstolee@github.com>

derrickstolee force-pushed the wsl-fix branch from 8204d97 to 109ae35 Compare April 13, 2022 19:55

vdye approved these changes Apr 13, 2022

View reviewed changes

derrickstolee merged commit 56cb39b into git-for-windows:main Apr 13, 2022

niik mentioned this pull request Apr 14, 2022

Add UNC path prefix before adding to safe directory list desktop/desktop#14368

Merged

vdye mentioned this pull request Apr 18, 2022

Rebase to v2.36.0-rc2 #3785

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Various fixes around `safe.directory` #3791

Various fixes around `safe.directory` #3791

derrickstolee commented Apr 13, 2022

niik Apr 13, 2022

derrickstolee Apr 13, 2022

dscho Apr 13, 2022

derrickstolee Apr 13, 2022

niik Apr 13, 2022

derrickstolee Apr 14, 2022

dscho left a comment

vdye left a comment

derrickstolee commented Apr 13, 2022

	#ifdef __MINGW32__
	if (path[0] == '/') {
	warning(_("encountered old-style '%s' that should be '%%(prefix)/%s'"), path, path);
	return system_path(path + 1);
	}
	#endif

Various fixes around safe.directory #3791

Various fixes around safe.directory #3791

Conversation

derrickstolee commented Apr 13, 2022

niik Apr 13, 2022

Choose a reason for hiding this comment

derrickstolee Apr 13, 2022

Choose a reason for hiding this comment

dscho Apr 13, 2022

Choose a reason for hiding this comment

derrickstolee Apr 13, 2022

Choose a reason for hiding this comment

niik Apr 13, 2022

Choose a reason for hiding this comment

derrickstolee Apr 14, 2022

Choose a reason for hiding this comment

dscho left a comment

Choose a reason for hiding this comment

vdye left a comment

Choose a reason for hiding this comment

derrickstolee commented Apr 13, 2022

Various fixes around `safe.directory` #3791

Various fixes around `safe.directory` #3791