Re: [PATCH 1/2] t5516: move plaintext-password tests from t5601 and t5516

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Jeff King <peff@peff.net>
Cc: Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, Derrick Stolee <derrickstolee@github.com>,
	Johannes Schindelin <johannes.schindelin@gmx.de>
Subject: Re: [PATCH 1/2] t5516: move plaintext-password tests from t5601 and t5516
Date: Tue, 01 Nov 2022 04:18:32 +0100	[thread overview]
Message-ID: <221101.865yfz5pjp.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <Y2CD4g9z2yVFA06l@coredump.intra.peff.net>

On Mon, Oct 31 2022, Jeff King wrote:

> Commit 6dcbdc0d66 (remote: create fetch.credentialsInUrl config,
> 2022-06-06) added tests for our handling of passwords in URLs. Since the
> obvious URL to be affected is git-over-http, the tests use http. However
> they don't set up a test server; they just try to access
> https://localhost, assuming it will fail (because the nothing is
> listening there).
>
> This causes some possible 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 tests cases unnecessarily.
>
>   - Since there's no server, our tests for "allow" and "warn" still
>     expect the clone/fetch/push operations to fail, even though in the
>     real world we'd expect these to succeed. We scrape stderr to see
>     what happened, but it's not as robust as a more realistic test.
>
> Let's instead move these to t5551, which is all about testing http and
> where we have a real server. That eliminates any issues with contacting
> a strange URL, and lets the "allow" and "warn" tests confirm that the
> operation actually succeeds.
>
> It's not quite a verbatim move for a few reasons:
>
>   - we can drop the LIBCURL dependency; it's already part of
>     lib-httpd.sh
>
>   - we'll use HTTPD_URL_USER_PASS, etc, instead of our fake URL. To
>     avoid repetition, we'll add a few extra variables.
>
>   - the "https://username:@localhost" test uses a funny URL that
>     lib-httpd.sh doesn't provide. We'll similarly construct it in a
>     variable. Note that we're hard-coding the lib-httpd username here,
>     but t5551 already does that everywhere.
>
>   - for the "domain:port" test, the URL provided by lib-httpd is fine,
>     since our test server will always be on an exotic port. But we'll
>     confirm in the test that this is so.
>
>   - since our message-matching is done via grep, I simplified it to use
>     a regex, rather than trying to massage lib-httpd's variables.
>     Arguably this makes it more readable, too, while retaining the bits
>     we care about: the fatal/warning distinction, the "uses plaintext"
>     message, and the fact that the password was redacted.
>
>   - we'll use the /auth/ path for the repo, which shows that we are
>     indeed making use of the auth information when needed.
>
>   - we'll also use /smart/; most of these tests could be done via /dumb/
>     in t5550, but setting up pushes there requires extra effort and
>     dependencies. The smart protocol is what most everyone is using
>     these days anyway.
>
> This patch is my own, but I stole the analysis and a few bits of the
> commit message from a patch by Johannes Schindelin.

Did you consider just using git://; on the WIP branch I linked to where
I fixed these tests quite a bit already I left them in their own file in
anticipation of having to test that (but didn't finish that yet). I.e.:

	+ git -C /home/avar/g/git/t/trash directory.t5700-protocol-v1/repo/parent/ tag two
	+ GIT_TRACE_PACKET=1 git -C daemon_child -c protocol.version=1 -c transfer.credentialsInUrl=die fetch git://foo:bar@127.0.0.1:5700/parent
	fatal: URL 'git://foo:<redacted>@127.0.0.1:5700/parent' uses plaintext credentials

I can't remember if anything can do anything sensible with
user:passwords over non-http(s), but at the point where we emit this
error in remote.c we don't care, so perhaps we could just test it that
way.