* Git Bug Report: git credential 'url' parsing clear other fields
@ 2024-04-26 19:27 Thomas Desveaux
2024-04-29 5:25 ` Jeff King
0 siblings, 1 reply; 2+ messages in thread
From: Thomas Desveaux @ 2024-04-26 19:27 UTC (permalink / raw
To: git
Hi,
I encountered what seems to be a small bug in git credential parsing.
Just want to confirm it's not expected behavior.
I'm willing to work on a patch.
First time submitting a bug to git (and a potential patch),
so let me know if I do anything wrong!
What did you do before the bug happened? (Steps to reproduce your issue)
Run git credential approve with the 'url' field of the form NOT as first entry.
example:
git -c credential.helper= -c "credential.helper=store
--file=.git-credentials" credential approve
input:
```
username=user
password=s3cret
url=https://example.com/repository.git
```
This next input works as intended.
input:
```
url=https://example.com/repository.git
username=user
password=s3cret
```
This was reproduced on both master and next branches.
What did you expect to happen? (Expected behavior)
Credentials should be saved with `username` and `password` provided.
`url` parsing should not clear the `credential` struct.
What happened instead? (Actual behavior)
`username` and `password` are correctly parsed and stored in the struct.
`url` parsing then clear the fields.
Culprit would be this `credential_clear` in `credential_from_url_1`.
On branch `next`, commit `2a3ae87e7f8e9585d7565a8b5d6a6c9c28d6d943`.
Thanks,
Thomas Desveaux
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: Git Bug Report: git credential 'url' parsing clear other fields
2024-04-26 19:27 Git Bug Report: git credential 'url' parsing clear other fields Thomas Desveaux
@ 2024-04-29 5:25 ` Jeff King
0 siblings, 0 replies; 2+ messages in thread
From: Jeff King @ 2024-04-29 5:25 UTC (permalink / raw
To: Thomas Desveaux; +Cc: Carlo Marcelo Arenas Belón, git
On Fri, Apr 26, 2024 at 09:27:03PM +0200, Thomas Desveaux wrote:
> input:
> ```
> username=user
> password=s3cret
> url=https://example.com/repository.git
> ```
>
> This next input works as intended.
> input:
> ```
> url=https://example.com/repository.git
> username=user
> password=s3cret
> ```
> [...]
> `username` and `password` are correctly parsed and stored in the struct.
> `url` parsing then clear the fields.
Thanks for a clear report and example.
However, what you're seeing is the intended behavior. When the "url"
feature was added, it came with these docs in git-credential.txt:
When this special attribute is read by `git credential`, the value is
parsed as a URL and treated as if its constituent parts were read
(e.g., `url=https://example.com` would behave as if `protocol=https`
and `host=example.com` had been provided). This can help callers avoid
parsing URLs themselves. Note that any components which are missing
from the URL (e.g., there is no username in the example above) will be
set to empty; if you want to provide a URL and override some
attributes, provide the URL attribute first, followed by any
overrides.
That last "Note that..." part was lost in 1aed817f99 (credential:
document protocol updates, 2020-05-06). Between digging around in the
thread[1] and my recollection, I think the point was that we might drop
the "override" behavior as a defense-in-depth against injection attacks
(like "path=evil/\nhost=github.com"). I.e., make it an error to specify
"host=" more than once. But we didn't actually make that code change
yet.
However, even if we did so, I think we'd keep the "clear" half of the
behavior (we parsed a URL with no password in it, so the result is a
credential with no password in it). In which case your first example
still wouldn't behave as you expected.
So I think 1aed817f99 may have gone a bit too far and caused the
documentation to be misleading. It should probably keep the "any
components which are missing will be set to empty" part.
-Peff
[1] https://lore.kernel.org/git/20200505013908.4596-1-carenas@gmail.com/
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2024-04-29 5:25 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-26 19:27 Git Bug Report: git credential 'url' parsing clear other fields Thomas Desveaux
2024-04-29 5:25 ` Jeff King
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.