Skip to content

Commit ec4d628

Browse files
committed
fix(core): preserve OAuth refresh tokens during rotation and retrieval
1 parent 11a9edc commit ec4d628

5 files changed

Lines changed: 65 additions & 5 deletions

File tree

packages/core/src/code_assist/oauth-credential-storage.test.ts

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -242,6 +242,39 @@ describe('OAuthCredentialStorage', () => {
242242
);
243243
});
244244

245+
it('should merge existing refresh token when new payload lacks one', async () => {
246+
const oldCredentials: OAuthCredentials = {
247+
serverName: 'main-account',
248+
token: {
249+
accessToken: 'old-access-token',
250+
refreshToken: 'persistent-refresh-token',
251+
tokenType: 'Bearer',
252+
expiresAt: Date.now() + 3600000,
253+
scope: 'email',
254+
},
255+
updatedAt: Date.now(),
256+
};
257+
vi.spyOn(mockHybridTokenStorage, 'getCredentials').mockResolvedValue(
258+
oldCredentials,
259+
);
260+
261+
const newTokens: Credentials = {
262+
access_token: 'new-access-token',
263+
expiry_date: Date.now() + 3600000,
264+
};
265+
266+
await OAuthCredentialStorage.saveCredentials(newTokens);
267+
268+
expect(mockHybridTokenStorage.setCredentials).toHaveBeenCalledWith(
269+
expect.objectContaining({
270+
token: expect.objectContaining({
271+
accessToken: 'new-access-token',
272+
refreshToken: 'persistent-refresh-token', // correctly merged
273+
}),
274+
}),
275+
);
276+
});
277+
245278
it('should throw an error if access_token is missing', async () => {
246279
const invalidCredentials: Credentials = {
247280
...mockCredentials,

packages/core/src/code_assist/oauth-credential-storage.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,12 +66,16 @@ export class OAuthCredentialStorage {
6666
throw new Error('Attempted to save credentials without an access token.');
6767
}
6868

69+
const existing = await this.storage.getCredentials(MAIN_ACCOUNT_KEY);
70+
const mergedRefreshToken =
71+
credentials.refresh_token || existing?.token.refreshToken;
72+
6973
// Convert Google Credentials to OAuthCredentials format
7074
const mcpCredentials: OAuthCredentials = {
7175
serverName: MAIN_ACCOUNT_KEY,
7276
token: {
7377
accessToken: credentials.access_token,
74-
refreshToken: credentials.refresh_token || undefined,
78+
refreshToken: mergedRefreshToken || undefined,
7579
tokenType: credentials.token_type || 'Bearer',
7680
scope: credentials.scope || undefined,
7781
expiresAt: credentials.expiry_date || undefined,

packages/core/src/mcp/oauth-token-storage.ts

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -143,9 +143,18 @@ export class MCPOAuthTokenStorage implements TokenStorage {
143143
): Promise<void> {
144144
await this.ensureConfigDir();
145145

146+
const existing = await this.getCredentials(serverName);
147+
const mergedRefreshToken =
148+
token.refreshToken || existing?.token.refreshToken;
149+
150+
const mergedToken = {
151+
...token,
152+
refreshToken: mergedRefreshToken,
153+
};
154+
146155
const credential: OAuthCredentials = {
147156
serverName,
148-
token,
157+
token: mergedToken,
149158
clientId,
150159
tokenUrl,
151160
mcpServerUrl,

packages/core/src/mcp/token-storage/keychain-token-storage.test.ts

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ describe('KeychainTokenStorage', () => {
7272
expect(retrieved?.serverName).toBe('test-server');
7373
});
7474

75-
it('should return null if no credentials are found or they are expired', async () => {
75+
it('should return null if no credentials are found or they are expired and unrefreshable', async () => {
7676
expect(await storage.getCredentials('missing')).toBeNull();
7777

7878
const expiredCreds = {
@@ -81,6 +81,20 @@ describe('KeychainTokenStorage', () => {
8181
};
8282
await storage.setCredentials(expiredCreds);
8383
expect(await storage.getCredentials('test-server')).toBeNull();
84+
85+
// Ensure that if it has a refresh token, it is NOT returned as null
86+
const expiredWithRefresh = {
87+
...validCredentials,
88+
token: {
89+
...validCredentials.token,
90+
expiresAt: Date.now() - 1000,
91+
refreshToken: 'some-refresh-token',
92+
},
93+
};
94+
await storage.setCredentials(expiredWithRefresh);
95+
const retrieved = await storage.getCredentials('test-server');
96+
expect(retrieved).not.toBeNull();
97+
expect(retrieved?.token.refreshToken).toBe('some-refresh-token');
8498
});
8599

86100
it('should throw if stored data is corrupted JSON', async () => {

packages/core/src/mcp/token-storage/keychain-token-storage.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ export class KeychainTokenStorage
3636
// eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion
3737
const credentials = JSON.parse(data) as OAuthCredentials;
3838

39-
if (this.isTokenExpired(credentials)) {
39+
if (this.isTokenExpired(credentials) && !credentials.token.refreshToken) {
4040
return null;
4141
}
4242

@@ -104,7 +104,7 @@ export class KeychainTokenStorage
104104
try {
105105
// eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion
106106
const data = JSON.parse(cred.password) as OAuthCredentials;
107-
if (!this.isTokenExpired(data)) {
107+
if (!this.isTokenExpired(data) || data.token.refreshToken) {
108108
result.set(cred.account, data);
109109
}
110110
} catch (error) {

0 commit comments

Comments
 (0)