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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 9, 2013, 17:41 UTC
Message-ID: <7vr4ijsish.fsf@alter.siamese.dyndns.org>
In-Reply-To: <7vvc7vsj4p.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

Show 33 quoted lines

>> 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.
>
> The first step (renaming and adjusting comments) would look like
> this.

Actually, there is another function "check_leading_path()" you may want also adjust.

        /*
         * Return zero if path 'name' has a leading symlink component or
         * if some leading path component does not exists.
         *
         * Return -1 if leading path exists and is a directory.
         *
         * Return path length if leading path exists and is neither a
         * directory nor a symlink.
         */
        int check_leading_path(const char *name, int len)
        {
            return threaded_check_leading_path(&default_cache, name, len);
        }

I think what the callers of this function care about is if the name is a path that should not be added to our index (i.e. points "outside the repository"). If you had a symlink d that points at e when our project does have a subdirectory e with file f,

	check_leading_path("d/f")

wants to say "bad", even though the real file pointed at, i.e. "e/f" is inside our working tree, so "outside our working tree" is not quite correct in the strict sense (this applies equally to has_symlink_leading_path), but I think we should treat the case where "d" (and "d/f") belongs to the working tree of a repository for a separate project, that is embedded in our working tree the same way.