[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [PATCH 1/2] t5516/t5601: avoid using `localhost` for failing HTTPS requests
From:       Jeff King <peff () peff ! net>
Date:       2022-10-31 23:20:11
Message-ID: Y2BYKxxkG57XAV/1 () coredump ! intra ! peff ! net
[Download RAW message or body]

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


[prev in list] [next in list] [prev in thread] [next in thread]