Re: [PATCH] osxkeychain: avoid incorrectly skipping store operation

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 13, 2025, 20:35 UTC
Message-ID: <xmqqecq1llgj.fsf@gitster.g>
In-Reply-To: <xmqqo6p5llsw.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 24 quoted lines

> "Koji Nakamaru via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> +/*
>> + * NOTE: We could use functions in strbuf.h and/or wrapper.h, but those
>> + * introduce significant dependencies. Therefore, we define simplified
>> + * versions here to keep this code self-contained.
>> + */
>
> Sorry, but I do not quite understand this comment.  The program is
> shipped as a part of Git, and using these functions and linking with
> libgit.a may pull strbuf.o and some other *.o files out of libgit.a
> to link with git-credential-osxkeychain.o to produce the executable,
> but how can that be "significant dependencies"?  For anybody who is
> building git-credential-osxkeychain, the necessary sources come for
> free.
>
> It is not like we are forcing git-credential-osxkeychain to link
> with a shared object libgit.so and making git-credential-osxkeychain
> depend on it, or anything like that, which may require consumers of
> binary distribution of git-credential-osxkeychain to also install
> another package that has libgit.so in it (which is likely to be the
> "git" package).  Even if it were the case (which is not), what good
> would it be to have git-credential-osxkeychain on your system
> without having git on the same system?

The rest of the patch, excluding the poor-man's reimplementation of helper functions, looked like they match what the proposed log message described.

It seems that credential material like username and password are included in plaintext as part of the state[], but is this a safe thing to do? The keychain will give out the credential material in a way the requestor with sufficient priviledges can read, and this state[] is stored in the same place, so I am guessing that this is not adding any extra security concerns, but I just wanted to make sure you've considered any security implications.

Thanks.