{"thread":{"id":"61372","subject":"Git Bug Report: git credential 'url' parsing clear other fields","startedAt":"2024-04-26T19:27:16Z","lastAt":"2024-04-29T05:25:48Z","messageCount":2,"participants":["Thomas Desveaux","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"493526","messageId":"CACPMLSqTA6pY3hiawhTAmhfvWME8NRUehauy6g-oUaHjordWqA@mail.gmail.com","threadId":"61372","inReplyTo":null,"subject":"Git Bug Report: git credential 'url' parsing clear other fields","fromName":"Thomas Desveaux","fromEmail":"desveaux.thomas@gmail.com","sentAt":"2024-04-26T19:27:03Z","receivedAt":"2024-04-26T19:27:16Z","isPatch":false,"sender":{"key":"desveaux.thomas@gmail.com","avatar":null},"body":"Hi,\n\nI encountered what seems to be a small bug in git credential parsing.\nJust want to confirm it's not expected behavior.\nI'm willing to work on a patch.\n\nFirst time submitting a bug to git (and a potential patch),\nso let me know if I do anything wrong!\n\nWhat did you do before the bug happened? (Steps to reproduce your issue)\n\nRun git credential approve with the 'url' field of the form NOT as first entry.\n\nexample:\ngit -c credential.helper= -c \"credential.helper=store\n--file=.git-credentials\" credential approve\n\ninput:\n```\nusername=user\npassword=s3cret\nurl=https://example.com/repository.git\n```\n\nThis next input works as intended.\ninput:\n```\nurl=https://example.com/repository.git\nusername=user\npassword=s3cret\n```\n\nThis was reproduced on both master and next branches.\n\n\nWhat did you expect to happen? (Expected behavior)\n\nCredentials should be saved with `username` and `password` provided.\n`url` parsing should not clear the `credential` struct.\n\n\nWhat happened instead? (Actual behavior)\n\n`username` and `password` are correctly parsed and stored in the struct.\n`url` parsing then clear the fields.\n\n\nCulprit would be this `credential_clear` in `credential_from_url_1`.\nOn branch `next`, commit  `2a3ae87e7f8e9585d7565a8b5d6a6c9c28d6d943`.\n\n\nThanks,\nThomas Desveaux\n"},{"id":"493582","messageId":"20240429052540.GA2892856@coredump.intra.peff.net","threadId":"61372","inReplyTo":"CACPMLSqTA6pY3hiawhTAmhfvWME8NRUehauy6g-oUaHjordWqA@mail.gmail.com","subject":"Re: Git Bug Report: git credential 'url' parsing clear other fields","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-29T05:25:40Z","receivedAt":"2024-04-29T05:25:48Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 26, 2024 at 09:27:03PM +0200, Thomas Desveaux wrote:\n\n> input:\n> ```\n> username=user\n> password=s3cret\n> url=https://example.com/repository.git\n> ```\n> \n> This next input works as intended.\n> input:\n> ```\n> url=https://example.com/repository.git\n> username=user\n> password=s3cret\n> ```\n> [...]\n> `username` and `password` are correctly parsed and stored in the struct.\n> `url` parsing then clear the fields.\n\nThanks for a clear report and example.\n\nHowever, what you're seeing is the intended behavior. When the \"url\"\nfeature was added, it came with these docs in git-credential.txt:\n\n  When this special attribute is read by `git credential`, the value is\n  parsed as a URL and treated as if its constituent parts were read\n  (e.g., `url=https://example.com` would behave as if `protocol=https`\n  and `host=example.com` had been provided). This can help callers avoid\n  parsing URLs themselves.  Note that any components which are missing\n  from the URL (e.g., there is no username in the example above) will be\n  set to empty; if you want to provide a URL and override some\n  attributes, provide the URL attribute first, followed by any\n  overrides.\n\nThat last \"Note that...\" part was lost in 1aed817f99 (credential:\ndocument protocol updates, 2020-05-06). Between digging around in the\nthread[1] and my recollection, I think the point was that we might drop\nthe \"override\" behavior as a defense-in-depth against injection attacks\n(like \"path=evil/\\nhost=github.com\"). I.e., make it an error to specify\n\"host=\" more than once. But we didn't actually make that code change\nyet.\n\nHowever, even if we did so, I think we'd keep the \"clear\" half of the\nbehavior (we parsed a URL with no password in it, so the result is a\ncredential with no password in it). In which case your first example\nstill wouldn't behave as you expected.\n\nSo I think 1aed817f99 may have gone a bit too far and caused the\ndocumentation to be misleading. It should probably keep the \"any\ncomponents which are missing will be set to empty\" part.\n\n-Peff\n\n[1] https://lore.kernel.org/git/20200505013908.4596-1-carenas@gmail.com/\n\n"}]}