From 66dfa123d6babebcdd9ed01958b23d0c8e0952cd Mon Sep 17 00:00:00 2001 From: "brian m. carlson" Date: Thu, 28 Mar 2019 22:06:50 +0000 Subject: [PATCH] Properly handle config options for URLs with upper case letters Git configuration options have a complicated situation with regard to case. For the most part, they are case-insensitive: you may write any case into the file, but Git interprets it as lower case. However, there are exceptions. Because the middle component of a three-component option can be a URL, branch name, or remote name, this component (and only this component) is treated as case sensitive. Since this component can be a URL, which may (and, due to the ".git" portion, usually does) contain a dot, the first component of the config option is read up until the first dot, and the last component is read from the end to the last dot. When git config writes a value into the file, it preserves the case the user has provided, and when it prints the config values, it canonicalizes the keys by folding the case-insensitive portions to lowercase. Git LFS then reads this canonicalized form in as our source of config options, relieving us from having to parse the files ourselves. However, when we read this data in, we forced the key to lowercase, and when we looked up a key, we also forced the key we were looking up to lowercase. While this behavior was wrong (since URLs, at least, are case-sensitive), it did happen to mostly work if the configuration didn't contain settings for two URLs differing in case. In the 2.7.0 cycle, we changed the way we did URL config lookups to match the way Git does them. Previously, we performed configuration lookups on several possible keys (forcing them to lower case, URL portion included) and took the first that matched. Now, we directly compare the URL we're connecting to (which may be in mixed case) to the values we got in the configuration (which we've forced to lowercase). Consequently, we will fail to find configuration options for a mixed-case URL, resulting in things like "http.extraHeader" not being used. Fix this issue by letting Git do the canonicalization of configuration keys for us instead of lowercasing them ourselves and then canonicalizing the key when looking it up in the table. Add tests for this behavior with "http.extraHeader" in the integration suite and several canonicalization assertions in the unit tests. Update several tests to use the canonical version of the data in their test data stores, and add a check to avoid noncanonical test data. Co-authored-by: Taylor Blau --- config/config.go | 10 ++++++++++ config/config_test.go | 12 ++++++------ config/git_fetcher.go | 31 ++++++++++++++++++++++++++++--- config/git_fetcher_test.go | 25 +++++++++++++++++++++++++ t/t-extra-header.sh | 32 ++++++++++++++++++++++++++++++++ 5 files changed, 101 insertions(+), 9 deletions(-) create mode 100644 config/git_fetcher_test.go diff --git a/config/config.go b/config/config.go index 6e38c595..70dcde37 100644 --- a/config/config.go +++ b/config/config.go @@ -11,6 +11,7 @@ import ( "strings" "sync" "time" + "unicode" "github.com/git-lfs/git-lfs/fs" "github.com/git-lfs/git-lfs/git" @@ -155,6 +156,15 @@ func NewFrom(v Values) *Configuration { } for key, values := range v.Git { + parts := strings.Split(key, ".") + isCaseSensitive := len(parts) >= 3 + hasUpper := strings.IndexFunc(key, unicode.IsUpper) > -1 + + // This branch should only ever trigger in + // tests, and only if they'd be broken. + if !isCaseSensitive && hasUpper { + panic(fmt.Sprintf("key %q has uppercase, shouldn't", key)) + } for _, value := range values { fmt.Printf("Config: %s=%s\n", key, value) source.Lines = append(source.Lines, fmt.Sprintf("%s=%s", key, value)) diff --git a/config/config_test.go b/config/config_test.go index 61a4eb8e..69c1131f 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -13,7 +13,7 @@ func TestRemoteDefault(t *testing.T) { cfg := NewFrom(Values{ Git: map[string][]string{ "branch.unused.remote": []string{"a"}, - "branch.unused.pushRemote": []string{"b"}, + "branch.unused.pushremote": []string{"b"}, }, }) assert.Equal(t, "origin", cfg.Remote()) @@ -24,7 +24,7 @@ func TestRemoteBranchConfig(t *testing.T) { cfg := NewFrom(Values{ Git: map[string][]string{ "branch.master.remote": []string{"a"}, - "branch.other.pushRemote": []string{"b"}, + "branch.other.pushremote": []string{"b"}, }, }) cfg.ref = &git.Ref{Name: "master"} @@ -37,8 +37,8 @@ func TestRemotePushDefault(t *testing.T) { cfg := NewFrom(Values{ Git: map[string][]string{ "branch.master.remote": []string{"a"}, - "remote.pushDefault": []string{"b"}, - "branch.other.pushRemote": []string{"c"}, + "remote.pushdefault": []string{"b"}, + "branch.other.pushremote": []string{"c"}, }, }) cfg.ref = &git.Ref{Name: "master"} @@ -51,8 +51,8 @@ func TestRemoteBranchPushDefault(t *testing.T) { cfg := NewFrom(Values{ Git: map[string][]string{ "branch.master.remote": []string{"a"}, - "remote.pushDefault": []string{"b"}, - "branch.master.pushRemote": []string{"c"}, + "remote.pushdefault": []string{"b"}, + "branch.master.pushremote": []string{"c"}, }, }) cfg.ref = &git.Ref{Name: "master"} diff --git a/config/git_fetcher.go b/config/git_fetcher.go index 3554fc64..7b941383 100644 --- a/config/git_fetcher.go +++ b/config/git_fetcher.go @@ -32,7 +32,10 @@ func readGitConfig(configs ...*git.ConfigurationSource) (gf *GitFetcher, extensi } allowed := !gc.OnlySafeKeys - key, val := strings.ToLower(pieces[0]), pieces[1] + + // We don't need to change the case of the key here, + // since Git will already have canonicalized it for us. + key, val := pieces[0], pieces[1] if origKey, ok := uniqKeys[key]; ok { if ShowConfigWarnings && len(vals[key]) > 0 && vals[key][len(vals[key])-1] != val && strings.HasPrefix(key, gitConfigWarningPrefix) { @@ -114,7 +117,8 @@ func readGitConfig(configs ...*git.ConfigurationSource) (gf *GitFetcher, extensi // empty string and false will be returned, signaling that the value was // absent. // -// Map lookup by key is case-insensitive, as per the .gitconfig specification. +// Map lookup by key is case-insensitive, except for the middle part of a +// three-part key, as per the .gitconfig specification. // // Get is safe to call across multiple goroutines. func (g *GitFetcher) Get(key string) (val string, ok bool) { @@ -130,7 +134,7 @@ func (g *GitFetcher) GetAll(key string) []string { g.vmu.RLock() defer g.vmu.RUnlock() - return g.vals[strings.ToLower(key)] + return g.vals[g.caseFoldKey(key)] } func (g *GitFetcher) All() map[string][]string { @@ -148,6 +152,27 @@ func (g *GitFetcher) All() map[string][]string { return newmap } +func (g *GitFetcher) caseFoldKey(key string) string { + parts := strings.Split(key, ".") + last := len(parts) - 1 + + // We check for 3 or more parts here because if the middle part is a + // URL, it may have dots in it. We'll downcase the part before the first + // dot and after the last dot, but preserve the piece in the middle, + // which may be a branch name, remote, or URL, all of which are + // case-sensitive. This is the algorithm Git uses to canonicalize its + // keys. + if len(parts) < 3 { + return strings.ToLower(key) + } + + return strings.Join([]string{ + strings.ToLower(parts[0]), + strings.Join(parts[1:last], "."), + strings.ToLower(parts[last]), + }, ".") +} + func keyIsUnsafe(key string) bool { for _, safe := range safeKeys { if safe == key { diff --git a/config/git_fetcher_test.go b/config/git_fetcher_test.go new file mode 100644 index 00000000..303dc0fa --- /dev/null +++ b/config/git_fetcher_test.go @@ -0,0 +1,25 @@ +package config + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestGetCanonicalization(t *testing.T) { + vals := map[string][]string{ + "user.name": []string{"Pat Doe"}, + "branch.MixedCase.pushremote": []string{"Somewhere"}, + "http.https://example.com/BIG-TEXT.git.extraheader": []string{"X-Foo: Bar"}, + } + + fetcher := GitFetcher{vals: vals} + assert.Equal(t, []string{"Somewhere"}, fetcher.GetAll("bRanch.MixedCase.pushRemote")) + assert.Equal(t, []string{"Somewhere"}, fetcher.GetAll("branch.MixedCase.pushremote")) + assert.Equal(t, []string(nil), fetcher.GetAll("branch.mixedcase.pushremote")) + assert.Equal(t, []string{"Pat Doe"}, fetcher.GetAll("user.name")) + assert.Equal(t, []string{"Pat Doe"}, fetcher.GetAll("User.Name")) + assert.Equal(t, []string{"X-Foo: Bar"}, fetcher.GetAll("http.https://example.com/BIG-TEXT.git.extraheader")) + assert.Equal(t, []string{"X-Foo: Bar"}, fetcher.GetAll("http.https://example.com/BIG-TEXT.git.extraHeader")) + assert.Equal(t, []string(nil), fetcher.GetAll("http.https://example.com/big-text.git.extraHeader")) +} diff --git a/t/t-extra-header.sh b/t/t-extra-header.sh index 995d22be..b5b0d798 100755 --- a/t/t-extra-header.sh +++ b/t/t-extra-header.sh @@ -96,3 +96,35 @@ begin_test "http..extraHeader with authorization (casing)" [ "0" -eq "$(grep -c "creds: git credential reject" curl.log)" ] ) end_test + +begin_test "http..extraHeader with mixed-case URLs" +( + set -e + + reponame="Mixed-Case-Headers" + setup_remote_repo "$reponame" + clone_repo "$reponame" "$reponame" + + # These config options check for several things. + # + # First, we check for mixed-case URLs being read properly and not forced to + # lowercase. Second, we check that the user can specify a config option for + # the Git URL and have that apply to the LFS URL, which exercises the + # URLConfig lookup code. Finally, we also write "ExtraHeader" in mixed-case as + # well to test that we lower-case the rightmost portion of the config key + # during lookup. + url="$(git config remote.origin.url).git" + git config --add "http.$url.ExtraHeader" "X-Foo: bar" + git config --add "http.$url.ExtraHeader" "X-Foo: baz" + + git lfs track "*.dat" + printf "contents" > a.dat + git add .gitattributes a.dat + git commit -m "initial commit" + + GIT_CURL_VERBOSE=1 GIT_TRACE=1 git push origin master 2>&1 | tee curl.log + + grep "> X-Foo: bar" curl.log + grep "> X-Foo: baz" curl.log +) +end_test