osxkeychain: avoid incorrectly skipping store operation

git-credential-osxkeychain skips storing a credential if its "get"
action sets "state[]=osxkeychain:seen=1". This behavior was introduced
in e1ab45b2 (osxkeychain: state to skip unnecessary store operations,
2024-05-15), which appeared in v2.46.

However, this state[] persists even if a credential returned by
"git-credential-osxkeychain get" is invalid and a subsequent helper's
"get" operation returns a valid credential. Another subsequent helper
(such as [1]) may expect git-credential-osxkeychain to store the valid
credential, but the "store" operation is incorrectly skipped because it
only checks "state[]=osxkeychain:seen=1".

To solve this issue, "state[]=osxkeychain:seen" needs to contain enough
information to identify whether the current "store" input matches the
output from the previous "get" operation (and not a credential from
another helper).

Set "state[]=osxkeychain:seen" to a value encoding the credential output
by "get", and compare it with a value encoding the credential input by
"store".

[1]: https://github.com/hickford/git-credential-oauth

Reported-by: Petter Sælen <petter@saelen.eu>
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: Koji Nakamaru <koji.nakamaru@gree.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/contrib/credential/osxkeychain/Makefile b/contrib/credential/osxkeychain/Makefile
index 9680717abe44c6d2813d71884c5c8a19875bf36e..c68445b82dc3e552059bd30b31eed8481dca3b28 100644 (file)
--- a/contrib/credential/osxkeychain/Makefile
+++ b/contrib/credential/osxkeychain/Makefile
@@ -1,21 +1,55 @@
 # The default target of this Makefile is...
 all:: git-credential-osxkeychain
 
+include ../../../config.mak.uname
 -include ../../../config.mak.autogen
 -include ../../../config.mak
 
+ifdef ZLIB_NG
+       BASIC_CFLAGS += -DHAVE_ZLIB_NG
+        ifdef ZLIB_NG_PATH
+               BASIC_CFLAGS += -I$(ZLIB_NG_PATH)/include
+               EXTLIBS += $(call libpath_template,$(ZLIB_NG_PATH)/$(lib))
+        endif
+       EXTLIBS += -lz-ng
+else
+        ifdef ZLIB_PATH
+               BASIC_CFLAGS += -I$(ZLIB_PATH)/include
+               EXTLIBS += $(call libpath_template,$(ZLIB_PATH)/$(lib))
+        endif
+       EXTLIBS += -lz
+endif
+ifndef NO_ICONV
+        ifdef NEEDS_LIBICONV
+                ifdef ICONVDIR
+                       BASIC_CFLAGS += -I$(ICONVDIR)/include
+                       ICONV_LINK = $(call libpath_template,$(ICONVDIR)/$(lib))
+                else
+                       ICONV_LINK =
+                endif
+                ifdef NEEDS_LIBINTL_BEFORE_LIBICONV
+                       ICONV_LINK += -lintl
+                endif
+               EXTLIBS += $(ICONV_LINK) -liconv
+        endif
+endif
+ifndef LIBC_CONTAINS_LIBINTL
+       EXTLIBS += -lintl
+endif
+
 prefix ?= /usr/local
 gitexecdir ?= $(prefix)/libexec/git-core
 
 CC ?= gcc
-CFLAGS ?= -g -O2 -Wall
+CFLAGS ?= -g -O2 -Wall -I../../.. $(BASIC_CFLAGS)
+LDFLAGS ?= $(BASIC_LDFLAGS) $(EXTLIBS)
 INSTALL ?= install
 RM ?= rm -f
 
 %.o: %.c
        $(CC) $(CFLAGS) $(CPPFLAGS) -o $@ -c $<
 
-git-credential-osxkeychain: git-credential-osxkeychain.o
+git-credential-osxkeychain: git-credential-osxkeychain.o ../../../libgit.a
        $(CC) $(CFLAGS) -o $@ $^ $(LDFLAGS) \
                -framework Security -framework CoreFoundation
 
@@ -23,6 +57,9 @@ install: git-credential-osxkeychain
        $(INSTALL) -d -m 755 $(DESTDIR)$(gitexecdir)
        $(INSTALL) -m 755 $< $(DESTDIR)$(gitexecdir)
 
+../../../libgit.a:
+       cd ../../..; make libgit.a
+
 clean:
        $(RM) git-credential-osxkeychain git-credential-osxkeychain.o
 

diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
index 611c9798b3ae5c1f8807fdcc86d27337abd0272f..b18026703418a9e2b0ab37fdbc5809b1359be81e 100644 (file)
--- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c
+++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
@@ -2,6 +2,9 @@
 #include <string.h>
 #include <stdlib.h>
 #include <Security/Security.h>
+#include "git-compat-util.h"
+#include "strbuf.h"
+#include "wrapper.h"
 
 #define ENCODING kCFStringEncodingUTF8
 static CFStringRef protocol; /* Stores constant strings - not memory managed */
@@ -12,7 +15,7 @@ static CFStringRef username;
 static CFDataRef password;
 static CFDataRef password_expiry_utc;
 static CFDataRef oauth_refresh_token;
-static int state_seen;
+static char *state_seen;
 
 static void clear_credential(void)
 {
@@ -48,27 +51,6 @@ static void clear_credential(void)
 
 #define STRING_WITH_LENGTH(s) s, sizeof(s) - 1
 
-__attribute__((format (printf, 1, 2), __noreturn__))
-static void die(const char *err, ...)
-{
-       char msg[4096];
-       va_list params;
-       va_start(params, err);
-       vsnprintf(msg, sizeof(msg), err, params);
-       fprintf(stderr, "%s\n", msg);
-       va_end(params);
-       clear_credential();
-       exit(1);
-}
-
-static void *xmalloc(size_t len)
-{
-       void *ret = malloc(len);
-       if (!ret)
-               die("Out of memory");
-       return ret;
-}
-
 static CFDictionaryRef create_dictionary(CFAllocatorRef allocator, ...)
 {
        va_list args;
@@ -112,6 +94,66 @@ static void write_item(const char *what, const char *buf, size_t len)
        putchar('\n');
 }
 
+static void write_item_strbuf(struct strbuf *sb, const char *what, const char *buf, int n)
+{
+       char s[32];
+
+       xsnprintf(s, sizeof(s), "__%s=", what);
+       strbuf_add(sb, s, strlen(s));
+       strbuf_add(sb, buf, n);
+}
+
+static void write_item_strbuf_cfstring(struct strbuf *sb, const char *what, CFStringRef ref)
+{
+       char *buf;
+       int len;
+
+       if (!ref)
+               return;
+       len = CFStringGetMaximumSizeForEncoding(CFStringGetLength(ref), ENCODING) + 1;
+       buf = xmalloc(len);
+       if (CFStringGetCString(ref, buf, len, ENCODING))
+               write_item_strbuf(sb, what, buf, strlen(buf));
+       free(buf);
+}
+
+static void write_item_strbuf_cfnumber(struct strbuf *sb, const char *what, CFNumberRef ref)
+{
+       short n;
+       char buf[32];
+
+       if (!ref)
+               return;
+       if (!CFNumberGetValue(ref, kCFNumberShortType, &n))
+               return;
+       xsnprintf(buf, sizeof(buf), "%d", n);
+       write_item_strbuf(sb, what, buf, strlen(buf));
+}
+
+static void write_item_strbuf_cfdata(struct strbuf *sb, const char *what, CFDataRef ref)
+{
+       char *buf;
+       int len;
+
+       if (!ref)
+               return;
+       buf = (char *)CFDataGetBytePtr(ref);
+       if (!buf || strlen(buf) == 0)
+               return;
+       len = CFDataGetLength(ref);
+       write_item_strbuf(sb, what, buf, len);
+}
+
+static void encode_state_seen(struct strbuf *sb)
+{
+       strbuf_add(sb, "osxkeychain:seen=", strlen("osxkeychain:seen="));
+       write_item_strbuf_cfstring(sb, "host", host);
+       write_item_strbuf_cfnumber(sb, "port", port);
+       write_item_strbuf_cfstring(sb, "path", path);
+       write_item_strbuf_cfstring(sb, "username", username);
+       write_item_strbuf_cfdata(sb, "password", password);
+}
+
 static void find_username_in_item(CFDictionaryRef item)
 {
        CFStringRef account_ref;
@@ -124,6 +166,7 @@ static void find_username_in_item(CFDictionaryRef item)
                write_item("username", "", 0);
                return;
        }
+       username = CFStringCreateCopy(kCFAllocatorDefault, account_ref);
 
        username_buf = (char *)CFStringGetCStringPtr(account_ref, ENCODING);
        if (username_buf)
@@ -163,6 +206,7 @@ static OSStatus find_internet_password(void)
        }
 
        data = CFDictionaryGetValue(item, kSecValueData);
+       password = CFDataCreateCopy(kCFAllocatorDefault, data);
 
        write_item("password",
                   (const char *)CFDataGetBytePtr(data),
@@ -173,7 +217,14 @@ static OSStatus find_internet_password(void)
        CFRelease(item);
 
        write_item("capability[]", "state", strlen("state"));
-       write_item("state[]", "osxkeychain:seen=1", strlen("osxkeychain:seen=1"));
+       {
+               struct strbuf sb;
+
+               strbuf_init(&sb, 1024);
+               encode_state_seen(&sb);
+               write_item("state[]", sb.buf, strlen(sb.buf));
+               strbuf_release(&sb);
+       }
 
 out:
        CFRelease(attrs);
@@ -288,13 +339,22 @@ static OSStatus add_internet_password(void)
        CFDictionaryRef attrs;
        OSStatus result;
 
-       if (state_seen)
-               return errSecSuccess;
-
        /* Only store complete credentials */
        if (!protocol || !host || !username || !password)
                return -1;
 
+       if (state_seen) {
+               struct strbuf sb;
+
+               strbuf_init(&sb, 1024);
+               encode_state_seen(&sb);
+               if (!strcmp(state_seen, sb.buf)) {
+                       strbuf_release(&sb);
+                       return errSecSuccess;
+               }
+               strbuf_release(&sb);
+       }
+
        data = CFDataCreateMutableCopy(kCFAllocatorDefault, 0, password);
        if (password_expiry_utc) {
                CFDataAppendBytes(data,
@@ -403,8 +463,9 @@ static void read_credential(void)
                                                           (UInt8 *)v,
                                                           strlen(v));
                else if (!strcmp(buf, "state[]")) {
-                       if (!strcmp(v, "osxkeychain:seen=1"))
-                               state_seen = 1;
+                       int len = strlen("osxkeychain:seen=");
+                       if (!strncmp(v, "osxkeychain:seen=", len))
+                               state_seen = xstrdup(v);
                }
                /*
                 * Ignore other lines; we don't know what they mean, but
@@ -443,5 +504,8 @@ int main(int argc, const char **argv)
 
        clear_credential();
 
+       if (state_seen)
+               free(state_seen);
+
        return 0;
 }

diff --git a/contrib/credential/osxkeychain/meson.build b/contrib/credential/osxkeychain/meson.build
index 3c7677f736c684ec5c3a82a9bdd4bfd6321717c0..ec91d0c14b0eb1de7497c7512ce01598fd90dacd 100644 (file)
--- a/contrib/credential/osxkeychain/meson.build
+++ b/contrib/credential/osxkeychain/meson.build
@@ -1,6 +1,7 @@
 executable('git-credential-osxkeychain',
   sources: 'git-credential-osxkeychain.c',
   dependencies: [
+    libgit,
     dependency('CoreFoundation'),
     dependency('Security'),
   ],