Re: [PATCH 1/2] t5516/t5601: avoid using `localhost` for failing HTTPS requests

From: Jeff King <peff@peff.net>
To: Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Derrick Stolee <derrickstolee@github.com>,
	Johannes Schindelin <johannes.schindelin@gmx.de>
Subject: Re: [PATCH 1/2] t5516/t5601: avoid using `localhost` for failing HTTPS requests
Date: Mon, 31 Oct 2022 19:20:11 -0400	[thread overview]
Message-ID: <Y2BYKxxkG57XAV/1@coredump.intra.peff.net> (raw)
In-Reply-To: <25cc0f6d91a9d23eb1b755e1463d672e4958a4e9.1667245639.git.gitgitgadget@gmail.com>

On Mon, Oct 31, 2022 at 07:47:17PM +0000, Johannes Schindelin via GitGitGadget wrote:

> In 6dcbdc0d6616 (remote: create fetch.credentialsInUrl config,
> 2022-06-06), we added four test cases that validate various behavior
> around passing credentials as part of the URL (which is considered
> unsafe in general).
> 
> These tests do not _actually_ try to connect anywhere, but have to use
> the https:// protocol in order to validate the intended code paths.

By "actually" here, I assume you mean "they do not expect to succeed".
But I think the first one (with credentialsInUrl=allow), does try to
make a connection.

> However, using `localhost` for such a connection causes several
> problems:
> 
> - There might be a web server running on localhost, and we do not
>   actually want to connect to that.
> 
> - The DNS resolver, or the local firewall, might take a substantial
>   amount of time (or forever, whichever comes first) to fail to connect,
>   slowing down the test cases unnecessarily.

Right. I think we assume that DNS resolution of localhost is fast-ish,
as we use it in other https tests. But I could certainly imagine a local
firewall causing issues (especially as this is real port 443, whereas
our other tests are usually high ports).

> Let's instead use an IPv4 address that is guaranteed never to offer a
> web server: 224.0.0.1 (which is part of the IP multicast range).

This feels pretty magical. I think it would be pretty unlikely for it to
have a web server, but I wouldn't be surprised if there are systems
where we get similar IP-routing hangs.

Is there a reason not to move all of these tests into t5550 or t5551,
where we have a real http server? That would be less magical, and then
this first test:

>  test_expect_success LIBCURL 'fetch warns or fails when using username:password' '
> -	message="URL '\''https://username:<redacted>@localhost/'\'' uses plaintext credentials" &&
> -	test_must_fail git -c transfer.credentialsInUrl=allow fetch https://username:password@localhost 2>err &&
> +	message="URL '\''https://username:<redacted>@224.0.0.1/'\'' uses plaintext credentials" &&
> +	test_must_fail git -c transfer.credentialsInUrl=allow fetch https://username:password@224.0.0.1 2>err &&
>  	! grep "$message" err &&

could be more robust. It would actually check that we succeeded in using
the URL.

-Peff