Re: [PATCH 2/2] add: refuse to add paths beyond repository boundaries

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 9, 2013, 17:09 UTC
Message-ID: <7v4nfftyu3.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1365499297-8667-3-git-send-email-artagnon@gmail.com>

Ramkumar Ramachandra <artagnon@gmail.com> writes:

Show 11 quoted lines

> Currently, git add has the logic for refusing to add gitlinks using
> treat_path(), which in turn calls check_path_for_gitlink().  However,
> this only checks for an in-index submodule (or gitlink cache_entry).
> A path inside a git repository in the worktree still adds fine, and
> this is a bug.  The logic for denying it is very similar to denying
> adding paths beyond symbolic links: die_if_path_beyond_symlink().
> Follow its example and write a die_if_path_beyond_gitrepo() to fix
> this bug.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---

Show 15 quoted lines

> @@ -166,6 +166,7 @@ static const char **validate_pathspec(const char **argv, const char *prefix)
>  		const char **p;
>  		for (p = pathspec; *p; p++) {
>  			die_if_path_beyond_symlink(*p, prefix);
> +			die_if_path_beyond_gitrepo(*p, prefix);
>  		}
>  	}
> diff --git a/cache.h b/cache.h
> index e1e8ce8..987d7f3 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -962,6 +962,8 @@ struct cache_def {
>  
>  extern int has_symlink_leading_path(const char *name, int len);
> +extern int has_gitrepo_leading_path(const char *name, int len);

I looked at the output from "grep has_symlink_leading_path" and also for "die_if_path_beyond"; all of these places are checking "I have this multi-level path; I want to know if the path does not (should not) be part of the current project", I think. Certainly the one in the "update-index" is about the same operation as "git add" you are patching.

Isn't it a better approach to _rename_ the existing function not to single out "symlink"-ness of the path first ? A symlink in the middle of such a multi-level path that leads to a place outside the project is _not_ the only way to step out of our project boundary. A directory in the middle of a multi-level path that is the top-level of the working tree of a foreign project is another way to step out of our project boundary. Perhaps

	die_if_path_outside_our_project()
        path_outside_our_project()

And then update the implementation of path_outside_our_project(), which only took "symlink in the middle" into account so far, and teach it that such a "top-level of the working tree of a foreign project" is also stepping out of our project?

That way, you do not have to settle on fixing the bug only in "git add" and keep the bug in "git update-index", I think.

I think the hit in builtin/apply.c deals with the same "beyond symlink is outside our project" check and can be updated like so. I didn't look at the ones in diff-lib.c and dir.c so you may want to double check on what they use it for.