Merge branch 'bc/allow-upload-pack-from-other-people' into seen

Loosen overly strict ownership check introduced in the recent past,
to keep the promise "cloning a suspicious repository is a safe
first step to inspect it".

Comments?

* bc/allow-upload-pack-from-other-people:
  Allow cloning from repositories owned by another user

Author: gitster

Documentation/git-clone.txt

@@ -63,6 +63,9 @@ symbolic link, the clone will fail. This is a security measure to
 prevent the unintentional copying of files by dereferencing the symbolic
 links.
 +
+This option does not work with repositories owned by other users for security
+reasons, and `--no-local` must be specified for the clone to succeed.
++
 *NOTE*: this operation can race with concurrent modification to the
 source repository, similar to running `cp -r <src> <dst>` while modifying
 _<src>_.
@@ -384,6 +387,12 @@ $ cd my-linux
 $ git clone --bare -l /home/proj/.git /pub/scm/proj.git
 ------------
 
+* Clone a local repository from a different user:
++
+------------
+$ git clone --no-local /home/otheruser/proj.git /pub/scm/proj.git
+------------
+
 CONFIGURATION
 -------------
 

builtin/upload-pack.c

@@ -39,6 +39,7 @@ int cmd_upload_pack(int argc,
 			    N_("interrupt transfer after <n> seconds of inactivity")),
 		OPT_END()
 	};
+	unsigned enter_repo_flags = ENTER_REPO_ANY_OWNER_OK;
 
 	packet_trace_identity("upload-pack");
 	disable_replace_refs();
@@ -54,7 +55,9 @@ int cmd_upload_pack(int argc,
 
 	dir = argv[0];
 
-	if (!enter_repo(dir, strict))
+	if (strict)
+		enter_repo_flags |= ENTER_REPO_STRICT;
+	if (!enter_repo(dir, enter_repo_flags))
 		die("'%s' does not appear to be a git repository", dir);
 
 	switch (determine_protocol_version_server()) {

daemon.c

@@ -152,6 +152,7 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
 	size_t rlen;
 	const char *path;
 	const char *dir;
+	unsigned enter_repo_flags;
 
 	dir = directory;
 
@@ -242,14 +243,15 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
 		dir = rpath;
 	}
 
-	path = enter_repo(dir, strict_paths);
+	enter_repo_flags = strict_paths ? ENTER_REPO_STRICT : 0;
+	path = enter_repo(dir, enter_repo_flags);
 	if (!path && base_path && base_path_relaxed) {
 		/*
 		 * if we fail and base_path_relaxed is enabled, try without
 		 * prefixing the base path
 		 */
 		dir = directory;
-		path = enter_repo(dir, strict_paths);
+		path = enter_repo(dir, enter_repo_flags);
 	}
 
 	if (!path) {

path.c

@@ -684,15 +684,15 @@ char *interpolate_path(const char *path, int real_home)
  * links.  User relative paths are also returned as they are given,
  * except DWIM suffixing.
  */
-const char *enter_repo(const char *path, int strict)
+const char *enter_repo(const char *path, unsigned flags)
 {
 	static struct strbuf validated_path = STRBUF_INIT;
 	static struct strbuf used_path = STRBUF_INIT;
 
 	if (!path)
 		return NULL;
 
-	if (!strict) {
+	if (!(flags & ENTER_REPO_STRICT)) {
 		static const char *suffix[] = {
 			"/.git", "", ".git/.git", ".git", NULL,
 		};
@@ -736,7 +736,8 @@ const char *enter_repo(const char *path, int strict)
 		if (!suffix[i])
 			return NULL;
 		gitfile = read_gitfile(used_path.buf);
-		die_upon_dubious_ownership(gitfile, NULL, used_path.buf);
+		if (!(flags & ENTER_REPO_ANY_OWNER_OK))
+			die_upon_dubious_ownership(gitfile, NULL, used_path.buf);
 		if (gitfile) {
 			strbuf_reset(&used_path);
 			strbuf_addstr(&used_path, gitfile);
@@ -747,7 +748,8 @@ const char *enter_repo(const char *path, int strict)
 	}
 	else {
 		const char *gitfile = read_gitfile(path);
-		die_upon_dubious_ownership(gitfile, NULL, path);
+		if (!(flags & ENTER_REPO_ANY_OWNER_OK))
+			die_upon_dubious_ownership(gitfile, NULL, path);
 		if (gitfile)
 			path = gitfile;
 		if (chdir(path))

path.h

@@ -156,7 +156,22 @@ int calc_shared_perm(int mode);
 int adjust_shared_perm(const char *path);
 
 char *interpolate_path(const char *path, int real_home);
-const char *enter_repo(const char *path, int strict);
+
+/* The bits are as follows:
+ *
+ * - ENTER_REPO_STRICT: callers that require exact paths (as opposed
+ *   to allowing known suffixes like ".git", ".git/.git" to be
+ *   omitted) can set this bit.
+ *
+ * - ENTER_REPO_ANY_OWNER_OK: callers that are willing to run without
+ *   ownership check can set this bit.
+ */
+enum {
+	ENTER_REPO_STRICT = (1<<0),
+	ENTER_REPO_ANY_OWNER_OK = (1<<1),
+};
+
+const char *enter_repo(const char *path, unsigned flags);
 const char *remove_leading_path(const char *in, const char *prefix);
 const char *relative_path(const char *in, const char *prefix, struct strbuf *sb);
 int normalize_path_copy_len(char *dst, const char *src, int *prefix_len);

t/t0411-clone-from-partial.sh

@@ -28,7 +28,6 @@ test_expect_success 'local clone must not fetch from promisor remote and execute
 	test_must_fail git clone \
 		--upload-pack="GIT_TEST_ASSUME_DIFFERENT_OWNER=true git-upload-pack" \
 		evil clone1 2>err &&
-	test_grep "detected dubious ownership" err &&
 	test_grep ! "fake-upload-pack running" err &&
 	test_path_is_missing script-executed
 '
@@ -38,7 +37,6 @@ test_expect_success 'clone from file://... must not fetch from promisor remote a
 	test_must_fail git clone \
 		--upload-pack="GIT_TEST_ASSUME_DIFFERENT_OWNER=true git-upload-pack" \
 		"file://$(pwd)/evil" clone2 2>err &&
-	test_grep "detected dubious ownership" err &&
 	test_grep ! "fake-upload-pack running" err &&
 	test_path_is_missing script-executed
 '
@@ -48,7 +46,6 @@ test_expect_success 'fetch from file://... must not fetch from promisor remote a
 	test_must_fail git fetch \
 		--upload-pack="GIT_TEST_ASSUME_DIFFERENT_OWNER=true git-upload-pack" \
 		"file://$(pwd)/evil" 2>err &&
-	test_grep "detected dubious ownership" err &&
 	test_grep ! "fake-upload-pack running" err &&
 	test_path_is_missing script-executed
 '

t/t5605-clone-local.sh

@@ -153,6 +153,16 @@ test_expect_success 'cloning a local path with --no-local does not hardlink' '
 	! repo_is_hardlinked force-nonlocal
 '
 
+test_expect_success 'cloning a local path with --no-local from a different user succeeds' '
+	git clone --upload-pack="GIT_TEST_ASSUME_DIFFERENT_OWNER=true git-upload-pack" \
+		--no-local a nonlocal-otheruser 2>err &&
+	! repo_is_hardlinked nonlocal-otheruser &&
+	# Verify that this is a git repository.
+	git -C nonlocal-otheruser rev-parse --show-toplevel &&
+	! test_grep "detected dubious ownership" err
+
+'
+
 test_expect_success 'cloning locally respects "-u" for fetching refs' '
 	test_must_fail git clone --bare -u false a should_not_work.git
 '