Re: [PATCH 1/2] t5516/t5601: avoid using `localhost` for failing HTTPS requests
- From
Jeff King <peff@peff.net>
- Date
- Oct 31, 2022, 23:20 UTC
- Message-ID
- <Y2BYKxxkG57XAV/1@coredump.intra.peff.net>
- In-Reply-To
- <25cc0f6d91a9d23eb1b755e1463d672e4958a4e9.1667245639.git.gitgitgadget@gmail.com>
On Mon, Oct 31, 2022 at 07:47:17PM +0000, Johannes Schindelin via GitGitGadget wrote:
Show 7 quoted lines
> 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.
Show 9 quoted lines
> 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:
Show 6 quoted lines
> 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