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 <[email protected]>

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))

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"}

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 {

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"))
+}

t/t-extra-header.sh

@@ -96,3 +96,35 @@ begin_test "http.<url>.extraHeader with authorization (casing)"
   [ "0" -eq "$(grep -c "creds: git credential reject" curl.log)" ]
 )
 end_test
+
+begin_test "http.<url>.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