git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Nov 1, 2022, 03:18 UTC
Message-ID
<221101.865yfz5pjp.gmgdl@evledraar.gmail.com>
In-Reply-To
<Y2CD4g9z2yVFA06l@coredump.intra.peff.net>
On Mon, Oct 31 2022, Jeff King wrote:
Show 59 quoted lines
> 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.

Previous: Jeff KingNext: Jeff King
Message 10 of 45 in “t5516/t5601: avoid using localhost for failing HTTPS requests”
  1. 0/2 t5516/t5601: avoid using localhost for failing HTTPS requestsJohannes Schindelin via GitGitGadget, Oct 31, 2022
  2. 1/2 t5516/t5601: avoid using `localhost` for failing HTTPS requestsJohannes Schindelin via GitGitGadget, Oct 31, 2022
  3. Ævar Arnfjörð BjarmasonOct 31, 2022
  4. Jeff KingOct 31, 2022
  5. Taylor BlauNov 1, 2022
  6. Jeff KingNov 1, 2022
  7. Jeff KingNov 1, 2022
  8. Jeff KingNov 1, 2022
  9. 1/2 t5516: move plaintext-password tests from t5601 and t5516Jeff King, Nov 1, 2022
  10. Ævar Arnfjörð BjarmasonNov 1, 2022
  11. Jeff KingNov 1, 2022
  12. Taylor BlauNov 1, 2022
  13. 2/2 t5516/t5601: be less strict about the number of credential warningsJeff King, Nov 1, 2022
  14. Ævar Arnfjörð BjarmasonNov 1, 2022
  15. Jeff KingNov 1, 2022
  16. Ævar Arnfjörð BjarmasonNov 1, 2022
  17. Jeff KingNov 1, 2022
  18. Ævar Arnfjörð BjarmasonNov 1, 2022
  19. Junio C HamanoNov 1, 2022
  20. Jeff KingNov 1, 2022
  21. Taylor BlauNov 1, 2022
  22. 2/2 t5516/t5601: be less strict about the number of credential warningsJohannes Schindelin via GitGitGadget, Oct 31, 2022
  23. Jeff KingOct 31, 2022
  24. Taylor BlauNov 1, 2022
  25. Jeff KingNov 1, 2022
  26. fetch: stop emitting duplicate transfer.credentialsInUrl=warn warningsÆvar Arnfjörð Bjarmason, Oct 31, 2022
  27. Taylor BlauNov 1, 2022
  28. Jeff KingNov 1, 2022
  29. Ævar Arnfjörð BjarmasonNov 1, 2022
  30. Taylor BlauNov 1, 2022
  31. Ævar Arnfjörð BjarmasonNov 1, 2022
  32. Taylor BlauNov 2, 2022
  33. 2/2 t5551: be less strict about the number of credential warningsJeff King, Nov 2, 2022
  34. Eric SunshineNov 2, 2022
  35. Jeff KingNov 2, 2022
  36. Eric SunshineNov 2, 2022
  37. Jeff KingNov 2, 2022
  38. Taylor BlauNov 3, 2022
  39. Jeff KingNov 1, 2022
  40. Ævar Arnfjörð BjarmasonNov 1, 2022
  41. Taylor BlauNov 1, 2022
  42. Ævar Arnfjörð BjarmasonNov 1, 2022
  43. Jeff KingNov 2, 2022
  44. Ævar Arnfjörð BjarmasonNov 4, 2022
  45. Jeff KingNov 4, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.