Re: [PATCH v3 3/3] t0034: enhance framework to allow testing more commands under sudo

[Junio C Hamano <gitster@pobox.com>]

Phillip Wood <phillip.wood123@gmail.com> writes:

(I'd be brief as it is my day-off today).

>> +test_expect_success SUDO 'cannot access with sudo' '
>> +	(
>> +		# TODO: test_must_fail needs additional functionality
>> +		# 6a67c759489 blocks its use with sudo
>> +		cd root/p &&
>> +		! sudo git status
>> +	)
>> +'
>
> I think Junio suggested that this should work and showed it was simple
> to make it work. It seems funny that if sudo is started as root it
> does not work.

It does feel odd.  Any attacker who can prepare a root-owned trap do
not have to trick "sudo git" to fall there, so I personally do not
see much value in stopping this particular pattern, but since
workarounds are easy (like double sudo), I do not mind if this does
not work *IF* there is a good reason to stop this.

>> +test_expect_success SUDO 'can access using a workaround' '
>> +	# run sudo twice
>> +	(
>> +		cd root/p &&
>> +		run_with_sudo <<-END
>> +			sudo git status
>> +		END
>> +	) &&
>> +	# provide explicit GIT_DIR
>> +	(
>> +		cd root/p &&
>> +		run_with_sudo <<-END
>> +			GIT_DIR=.git &&
>> +			GIT_WORK_TREE=. &&
>> +			export GIT_DIR GIT_WORK_TREE &&
>> +			git status
>
> I'm confused by this. Does this mean we don't do the ownership checks
> if GIT_DIR and or GIT_WORK_TREE are set in the environment?

That's by design.  Remember, we are protecting people from "cd"ing
into a (sub)directory, unknowingly, that happens to be controled by
a Git repository somebody else prepared as a trap.  So we try to
ensure who owns such a repository when we do the discovery.  Users
who set these environment variables to tell Git that they are aware
that they are working with that directory are different from the
target audience.

> Thanks for working on this

Ditto.  And thanks for reviewing and raising questions.  I agree
with all the things you said in the part of the message I did not
touch in this reply.