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
