threads / discuss / 28585

[PATCHv3 2/5] Learn to handle gitfiles in enter_repo

Subject: [PATCHv3 2/5] Learn to handle gitfiles in enter_repo

## tl;dr

3 messages between Oct 5, 2011 and Oct 6, 2011.

replies: 2people: 2as markdown or json

Phil Hord· Oct 5, 2011, 13:31 UTC · lore

The enter_repo() function is used to navigate into a .git directory. It knows how to find standard alternatives (DWIM) but it doesn't handle gitfiles created by git init --separate-git-dir. This means that git-fetch and others do not work with repositories using the separate-git-dir mechanism.

Teach enter_repo() to deal with the gitfile mechanism by resolving the path to the redirected path and continuing tests on that path instead of the found file.

Signed-off-by: Phil Hord <hordp@cisco.com>
---
Not sure how to resolve this for the 'strict' case.

The actual path followed may be different than the version spelled out on the input path because of resolved symlinks and whatnot. This function normally returns the unmolested "original" path that was validated. In case of a gitfile, I think that means we should return the url resolved from the gitfile contents.

But should we?

The returned path is only used in git-daemon where it gets further checks for path restrictions.

A. If we return the gitfile-resolved path, this may cause these
   path restrictions to fail since the path gets canonicalized
   when the gitfile is created by git.
B. If we do not return the gitfile-resolved path, this can create
   a security-hole by allowing remote users to access files they
   would otherwise have been restricted from.  In effect it creates
   an alternate symlink mechanism of which the administator might
   not be aware.
diff --git a/path.c b/path.c
index f3d96aa..46ba326 100644
--- a/path.c
+++ b/path.c
@@ -295,6 +295,7 @@ const char *enter_repo(const char *path, int strict)
 		static const char *suffix[] = {
 			".git/.git", "/.git", ".git", "", NULL,
 		};
+		const char *gitfile;
 		int len = strlen(path);
 		int i;
 		while ((1 < len) && (path[len-1] == '/'))
@@ -330,7 +331,12 @@ const char *enter_repo(const char *path, int strict)
 				break;
 			}
 		}
-		if (!suffix[i] || chdir(used_path))
+		if (!suffix[i])
+			return NULL;
+		gitfile = read_gitfile(used_path) ;
+		if (gitfile)
+			strcpy(used_path, gitfile);
+		if (chdir(used_path))
 			return NULL;
 		path = validated_path;
 	}

-- 
1.7.7.505.ga09f6
Nguyen Thai Ngoc Duy· Oct 6, 2011, 03:11 UTC · re: Phil Hord · lore

Re: [PATCHv3 2/5] Learn to handle gitfiles in enter_repo

On Thu, Oct 6, 2011 at 12:31 AM, Phil Hord <hordp@cisco.com> wrote:
Show 10 quoted lines
> -               if (!suffix[i] || chdir(used_path))
> +               if (!suffix[i])
> +                       return NULL;
> +               gitfile = read_gitfile(used_path) ;
> +               if (gitfile)
> +                       strcpy(used_path, gitfile);
> +               if (chdir(used_path))
>                        return NULL;
>                path = validated_path;
>        }

This is room for improvement, the patch is fine as it is now. We could improve error reporting here. If .git file points to nowhere, we get "not a repository-kind of message. Except daemon.c, enter_repo() callers always die() if enter_repo() returns NULL. We could move the die() part (with improved error message) into enter_repo().

We could update enter_repo(const char *, int) to enter_repo(const char *, int, int gently). If gently is 1, we never die() nor report anything (ie. what we're doing now). daemon.c will need this, the rest of callers will be happy with gently = 0.

-- 
Duy
Phil Hord· Oct 6, 2011, 19:16 UTC · re: Nguyen Thai Ngoc Duy · lore

Re: [PATCHv3 2/5] Learn to handle gitfiles in enter_repo

Nguyen Thai Ngoc Duy <pclouds@gmail.com> says,
Show 22 quoted lines
> On Thu, Oct 6, 2011 at 12:31 AM, Phil Hord <hordp@cisco.com> wrote:
>> -               if (!suffix[i] || chdir(used_path))
>> +               if (!suffix[i])
>> +                       return NULL;
>> +               gitfile = read_gitfile(used_path) ;
>> +               if (gitfile)
>> +                       strcpy(used_path, gitfile);
>> +               if (chdir(used_path))
>>                        return NULL;
>>                path = validated_path;
>>        }
> 
> This is room for improvement, the patch is fine as it is now. We could
> improve error reporting here. If .git file points to nowhere, we get
> "not a repository-kind of message. Except daemon.c, enter_repo()
> callers always die() if enter_repo() returns NULL. We could move the
> die() part (with improved error message) into enter_repo().
> 
> We could update enter_repo(const char *, int) to enter_repo(const char
> *, int, int gently). If gently is 1, we never die() nor report
> anything (ie. what we're doing now). daemon.c will need this, the rest
> of callers will be happy with gently = 0.

I like that. It wasn't clear to me what the 'gently' moniker meant before, but now I understand it. It could easily apply to this function and the new is_gitfile() to help reduce code duplication.

In a different patch, though.
Phil

← back to recent threads