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>
Koji Nakamaru committed
Nov 14, 2025 at 06:04 UTC
4580bcd2354aab9369164d936f7ccaa21fc98c98
3 files changed
+132
-30
contrib/credential/osxkeychain/Makefile
+39
-2
@@ -1,21 +1,55 @@
1
# The default target of this Makefile is...
2
all:: git-credential-osxkeychain
3
4
+include ../../../config.mak.uname
5
-include ../../../config.mak.autogen
6
-include ../../../config.mak
7
8
+ifdef ZLIB_NG
9
+ BASIC_CFLAGS += -DHAVE_ZLIB_NG
10
+ ifdef ZLIB_NG_PATH
11
+ BASIC_CFLAGS += -I$(ZLIB_NG_PATH)/include
12
+ EXTLIBS += $(call libpath_template,$(ZLIB_NG_PATH)/$(lib))
13
+ endif
14
+ EXTLIBS += -lz-ng
15
+else
16
+ ifdef ZLIB_PATH
17
+ BASIC_CFLAGS += -I$(ZLIB_PATH)/include
18
+ EXTLIBS += $(call libpath_template,$(ZLIB_PATH)/$(lib))
19
+ endif
20
+ EXTLIBS += -lz
21
+endif
22
+ifndef NO_ICONV
23
+ ifdef NEEDS_LIBICONV
24
+ ifdef ICONVDIR
25
+ BASIC_CFLAGS += -I$(ICONVDIR)/include
26
+ ICONV_LINK = $(call libpath_template,$(ICONVDIR)/$(lib))
27
+ else
28
+ ICONV_LINK =
29
+ endif
30
+ ifdef NEEDS_LIBINTL_BEFORE_LIBICONV
31
+ ICONV_LINK += -lintl
32
+ endif
33
+ EXTLIBS += $(ICONV_LINK) -liconv
34
+ endif
35
+endif
36
+ifndef LIBC_CONTAINS_LIBINTL
37
+ EXTLIBS += -lintl
38
+endif
39
+
40
prefix ?= /usr/local
41
gitexecdir ?= $(prefix)/libexec/git-core
42
43
CC ?= gcc
11
-CFLAGS ?= -g -O2 -Wall
44
+CFLAGS ?= -g -O2 -Wall -I../../.. $(BASIC_CFLAGS)
45
+LDFLAGS ?= $(BASIC_LDFLAGS) $(EXTLIBS)
46
INSTALL ?= install
47
RM ?= rm -f
48
49
%.o: %.c
50
$(CC) $(CFLAGS) $(CPPFLAGS) -o $@ -c $<
51
18
-git-credential-osxkeychain: git-credential-osxkeychain.o
52
+git-credential-osxkeychain: git-credential-osxkeychain.o ../../../libgit.a
53
$(CC) $(CFLAGS) -o $@ $^ $(LDFLAGS) \
54
-framework Security -framework CoreFoundation
55
@@ -23,6 +57,9 @@ install: git-credential-osxkeychain
57
$(INSTALL) -d -m 755 $(DESTDIR)$(gitexecdir)
58
$(INSTALL) -m 755 $< $(DESTDIR)$(gitexecdir)
59
60
+../../../libgit.a:
61
+ cd ../../..; make libgit.a
62
+
63
clean:
64
$(RM) git-credential-osxkeychain git-credential-osxkeychain.o
65
contrib/credential/osxkeychain/git-credential-osxkeychain.c
+92
-28
@@ -2,6 +2,9 @@
2
#include <string.h>
3
#include <stdlib.h>
4
#include <Security/Security.h>
5
+#include "git-compat-util.h"
6
+#include "strbuf.h"
7
+#include "wrapper.h"
8
9
#define ENCODING kCFStringEncodingUTF8
10
static CFStringRef protocol; /* Stores constant strings - not memory managed */
@@ -12,7 +15,7 @@ static CFStringRef username;
15
static CFDataRef password;
16
static CFDataRef password_expiry_utc;
17
static CFDataRef oauth_refresh_token;
15
-static int state_seen;
18
+static char *state_seen;
19
20
static void clear_credential(void)
21
{
@@ -48,27 +51,6 @@ static void clear_credential(void)
51
52
#define STRING_WITH_LENGTH(s) s, sizeof(s) - 1
53
51
-__attribute__((format (printf, 1, 2), __noreturn__))
52
-static void die(const char *err, ...)
53
-{
54
- char msg[4096];
55
- va_list params;
56
- va_start(params, err);
57
- vsnprintf(msg, sizeof(msg), err, params);
58
- fprintf(stderr, "%s\n", msg);
59
- va_end(params);
60
- clear_credential();
61
- exit(1);
62
-}
63
-
64
-static void *xmalloc(size_t len)
65
-{
66
- void *ret = malloc(len);
67
- if (!ret)
68
- die("Out of memory");
69
- return ret;
70
-}
71
-
54
static CFDictionaryRef create_dictionary(CFAllocatorRef allocator, ...)
55
{
56
va_list args;
@@ -112,6 +94,66 @@ static void write_item(const char *what, const char *buf, size_t len)
94
putchar('\n');
95
}
96
97
+static void write_item_strbuf(struct strbuf *sb, const char *what, const char *buf, int n)
98
+{
99
+ char s[32];
100
+
101
+ xsnprintf(s, sizeof(s), "__%s=", what);
102
+ strbuf_add(sb, s, strlen(s));
103
+ strbuf_add(sb, buf, n);
104
+}
105
+
106
+static void write_item_strbuf_cfstring(struct strbuf *sb, const char *what, CFStringRef ref)
107
+{
108
+ char *buf;
109
+ int len;
110
+
111
+ if (!ref)
112
+ return;
113
+ len = CFStringGetMaximumSizeForEncoding(CFStringGetLength(ref), ENCODING) + 1;
114
+ buf = xmalloc(len);
115
+ if (CFStringGetCString(ref, buf, len, ENCODING))
116
+ write_item_strbuf(sb, what, buf, strlen(buf));
117
+ free(buf);
118
+}
119
+
120
+static void write_item_strbuf_cfnumber(struct strbuf *sb, const char *what, CFNumberRef ref)
121
+{
122
+ short n;
123
+ char buf[32];
124
+
125
+ if (!ref)
126
+ return;
127
+ if (!CFNumberGetValue(ref, kCFNumberShortType, &n))
128
+ return;
129
+ xsnprintf(buf, sizeof(buf), "%d", n);
130
+ write_item_strbuf(sb, what, buf, strlen(buf));
131
+}
132
+
133
+static void write_item_strbuf_cfdata(struct strbuf *sb, const char *what, CFDataRef ref)
134
+{
135
+ char *buf;
136
+ int len;
137
+
138
+ if (!ref)
139
+ return;
140
+ buf = (char *)CFDataGetBytePtr(ref);
141
+ if (!buf || strlen(buf) == 0)
142
+ return;
143
+ len = CFDataGetLength(ref);
144
+ write_item_strbuf(sb, what, buf, len);
145
+}
146
+
147
+static void encode_state_seen(struct strbuf *sb)
148
+{
149
+ strbuf_add(sb, "osxkeychain:seen=", strlen("osxkeychain:seen="));
150
+ write_item_strbuf_cfstring(sb, "host", host);
151
+ write_item_strbuf_cfnumber(sb, "port", port);
152
+ write_item_strbuf_cfstring(sb, "path", path);
153
+ write_item_strbuf_cfstring(sb, "username", username);
154
+ write_item_strbuf_cfdata(sb, "password", password);
155
+}
156
+
157
static void find_username_in_item(CFDictionaryRef item)
158
{
159
CFStringRef account_ref;
@@ -124,6 +166,7 @@ static void find_username_in_item(CFDictionaryRef item)
166
write_item("username", "", 0);
167
return;
168
}
169
+ username = CFStringCreateCopy(kCFAllocatorDefault, account_ref);
170
171
username_buf = (char *)CFStringGetCStringPtr(account_ref, ENCODING);
172
if (username_buf)
@@ -163,6 +206,7 @@ static OSStatus find_internet_password(void)
206
}
207
208
data = CFDictionaryGetValue(item, kSecValueData);
209
+ password = CFDataCreateCopy(kCFAllocatorDefault, data);
210
211
write_item("password",
212
(const char *)CFDataGetBytePtr(data),
@@ -173,7 +217,14 @@ static OSStatus find_internet_password(void)
217
CFRelease(item);
218
219
write_item("capability[]", "state", strlen("state"));
176
- write_item("state[]", "osxkeychain:seen=1", strlen("osxkeychain:seen=1"));
220
+ {
221
+ struct strbuf sb;
222
+
223
+ strbuf_init(&sb, 1024);
224
+ encode_state_seen(&sb);
225
+ write_item("state[]", sb.buf, strlen(sb.buf));
226
+ strbuf_release(&sb);
227
+ }
228
229
out:
230
CFRelease(attrs);
@@ -288,13 +339,22 @@ static OSStatus add_internet_password(void)
339
CFDictionaryRef attrs;
340
OSStatus result;
341
291
- if (state_seen)
292
- return errSecSuccess;
293
-
342
/* Only store complete credentials */
343
if (!protocol || !host || !username || !password)
344
return -1;
345
346
+ if (state_seen) {
347
+ struct strbuf sb;
348
+
349
+ strbuf_init(&sb, 1024);
350
+ encode_state_seen(&sb);
351
+ if (!strcmp(state_seen, sb.buf)) {
352
+ strbuf_release(&sb);
353
+ return errSecSuccess;
354
+ }
355
+ strbuf_release(&sb);
356
+ }
357
+
358
data = CFDataCreateMutableCopy(kCFAllocatorDefault, 0, password);
359
if (password_expiry_utc) {
360
CFDataAppendBytes(data,
@@ -403,8 +463,9 @@ static void read_credential(void)
463
(UInt8 *)v,
464
strlen(v));
465
else if (!strcmp(buf, "state[]")) {
406
- if (!strcmp(v, "osxkeychain:seen=1"))
407
- state_seen = 1;
466
+ int len = strlen("osxkeychain:seen=");
467
+ if (!strncmp(v, "osxkeychain:seen=", len))
468
+ state_seen = xstrdup(v);
469
}
470
/*
471
* Ignore other lines; we don't know what they mean, but
@@ -443,5 +504,8 @@ int main(int argc, const char **argv)
504
505
clear_credential();
506
507
+ if (state_seen)
508
+ free(state_seen);
509
+
510
return 0;
511
}
contrib/credential/osxkeychain/meson.build
+1
@@ -1,6 +1,7 @@
1
executable('git-credential-osxkeychain',
2
sources: 'git-credential-osxkeychain.c',
3
dependencies: [
4
+ libgit,
5
dependency('CoreFoundation'),
6
dependency('Security'),
7
],