Skip to content

Commit d1c293a

Browse files
committed
fix(cli): auth edge cases found in review
- logout no longer blocked when OS keychain is locked: read() throw is caught in both logout/index.ts and logout.ts; local credential cleanup (hosts.yml) always runs regardless of keychain state - FileTokenStore transparently reads pre-upgrade tokens.yml (no version header = legacy format); next write stamps version: 1 - KeychainTokenStore.write() now wraps native keyring errors as KeyringUnavailable, matching the error surface of read() - tests for all three: logout-with-locked-keychain, legacy-file migration, write-error wrapping
1 parent 444d479 commit d1c293a

6 files changed

Lines changed: 73 additions & 9 deletions

File tree

cli/src/commands/auth/logout/index.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,11 @@ export default class Logout extends DifyCommand {
2626

2727
let http: HttpClient | undefined
2828
if (active !== undefined) {
29-
const bearer = getTokenStore(reg.token_storage).read(active.host, active.email)
29+
let bearer = ''
30+
try {
31+
bearer = getTokenStore(reg.token_storage).read(active.host, active.email)
32+
}
33+
catch { /* keyring locked — skip remote revocation, local cleanup still runs */ }
3034
if (bearer !== '') {
3135
http = createHttpClient({ baseURL: openAPIBase(hostWithScheme(active.host, active.scheme)), bearer, retryAttempts: 0 })
3236
}

cli/src/commands/auth/logout/logout.test.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,17 @@ describe('runLogout', () => {
5656
expect(raw).toContain('b@x')
5757
})
5858

59+
it('clears local credentials even when the store.read throws (e.g. keyring locked)', async () => {
60+
const store = new MemStore()
61+
seed(store)
62+
store.read = () => {
63+
throw new Error('keyring locked')
64+
}
65+
await runLogout({ io: bufferStreams(), reg: Registry.load(), store })
66+
const after = Registry.load()
67+
expect(after?.hosts.h1?.accounts['a@x']).toBeUndefined()
68+
})
69+
5970
it('throws NotLoggedIn when no active context', async () => {
6071
Registry.empty('file').save()
6172
await expect(runLogout({ io: bufferStreams(), reg: Registry.load(), store: new MemStore() }))

cli/src/commands/auth/logout/logout.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,11 @@ export async function runLogout(opts: LogoutOptions): Promise<void> {
2222
const active = reg.requireActive()
2323

2424
const store = opts.store ?? getTokenStore(reg.token_storage)
25-
const bearer = store.read(active.host, active.email)
25+
let bearer = ''
26+
try {
27+
bearer = store.read(active.host, active.email)
28+
}
29+
catch { /* keyring locked — skip remote revocation, local cleanup still runs */ }
2630

2731
let revokeWarning = ''
2832
if (bearer !== '' && revokeAllowed(bearer) && opts.http !== undefined) {

cli/src/store/keychain-token-store.test.ts

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ type EntryArgs = { service: string, username: string }
77
const passwords = new Map<string, string>()
88
const constructed: EntryArgs[] = []
99
let getPasswordError: Error | null = null
10+
let setPasswordError: Error | null = null
1011

1112
class FakeEntry {
1213
private readonly key: string
@@ -16,6 +17,8 @@ class FakeEntry {
1617
}
1718

1819
setPassword(value: string): void {
20+
if (setPasswordError !== null)
21+
throw setPasswordError
1922
passwords.set(this.key, value)
2023
}
2124

@@ -45,6 +48,7 @@ beforeEach(() => {
4548
passwords.clear()
4649
constructed.length = 0
4750
getPasswordError = null
51+
setPasswordError = null
4852
})
4953

5054
describe('KeychainTokenStore', () => {
@@ -104,7 +108,7 @@ describe('KeychainTokenStore', () => {
104108
expect(store.read('h', 'e')).toBe('')
105109
})
106110

107-
it('throws KeyringUnavailable (not empty string) when keyring access fails', () => {
111+
it('throws KeyringUnavailable (not empty string) when keyring access fails on read', () => {
108112
getPasswordError = new Error('keyring locked')
109113
const store = new KeychainTokenStore(SERVICE)
110114
let caught: unknown
@@ -117,4 +121,18 @@ describe('KeychainTokenStore', () => {
117121
expect(caught).toBeInstanceOf(BaseError)
118122
expect((caught as BaseError).code).toBe(ErrorCode.KeyringUnavailable)
119123
})
124+
125+
it('throws KeyringUnavailable when keyring access fails on write', () => {
126+
setPasswordError = new Error('keyring locked')
127+
const store = new KeychainTokenStore(SERVICE)
128+
let caught: unknown
129+
try {
130+
store.write('h', 'e', 'dfoa_secret')
131+
}
132+
catch (err) {
133+
caught = err
134+
}
135+
expect(caught).toBeInstanceOf(BaseError)
136+
expect((caught as BaseError).code).toBe(ErrorCode.KeyringUnavailable)
137+
})
120138
})

cli/src/store/token-store.test.ts

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,12 +44,27 @@ describe('FileTokenStore', () => {
4444
expect(raw).toContain('a@x.com')
4545
})
4646

47-
it('reads empty when the document version is unknown', () => {
47+
it('reads empty when the document version is an unknown future version', () => {
4848
writeFileSync(file, 'version: 999\ntokens:\n "h":\n "e": "x"\n')
4949
const s = new FileTokenStore(file)
5050
expect(s.read('h', 'e')).toBe('')
5151
})
5252

53+
it('reads tokens from legacy format (no version field) for transparent migration', () => {
54+
writeFileSync(file, 'tokens:\n "h":\n "e": "dfoa_legacy"\n')
55+
const s = new FileTokenStore(file)
56+
expect(s.read('h', 'e')).toBe('dfoa_legacy')
57+
})
58+
59+
it('preserves existing tokens and stamps version when writing to a legacy file', () => {
60+
writeFileSync(file, 'tokens:\n "h":\n "existing@x": "dfoa_existing"\n')
61+
const s = new FileTokenStore(file)
62+
s.write('h', 'new@x', 'dfoa_new')
63+
expect(s.read('h', 'existing@x')).toBe('dfoa_existing')
64+
expect(s.read('h', 'new@x')).toBe('dfoa_new')
65+
expect(readFileSync(file, 'utf8')).toContain('version: 1')
66+
})
67+
5368
it('remove deletes the credential and prunes the empty host map', () => {
5469
const s = new FileTokenStore(file)
5570
s.write('https://cloud.dify.ai', 'a@x.com', 'A')

cli/src/store/token-store.ts

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,10 @@ export class FileTokenStore implements TokenStore {
2828

2929
read(host: string, email: string): string {
3030
const doc = this.store.getTyped<TokenDoc>()
31-
if (doc === null || doc.version !== DOC_VERSION)
31+
if (doc === null)
32+
return ''
33+
// missing version = legacy pre-v1 format (same data shape); future unknown versions are rejected
34+
if (doc.version !== undefined && doc.version !== DOC_VERSION)
3235
return ''
3336
return doc.tokens?.[host]?.[email] ?? ''
3437
}
@@ -43,7 +46,9 @@ export class FileTokenStore implements TokenStore {
4346

4447
remove(host: string, email: string): void {
4548
const doc = this.store.getTyped<TokenDoc>()
46-
if (doc === null || doc.version !== DOC_VERSION)
49+
if (doc === null)
50+
return
51+
if (doc.version !== undefined && doc.version !== DOC_VERSION)
4752
return
4853
const tokens = doc.tokens ?? {}
4954
const hostMap = tokens[host]
@@ -57,9 +62,11 @@ export class FileTokenStore implements TokenStore {
5762

5863
private load(): { version: number, tokens: Record<string, Record<string, string>> } {
5964
const doc = this.store.getTyped<TokenDoc>()
60-
if (doc === null || doc.version !== DOC_VERSION)
65+
if (doc === null)
6166
return { version: DOC_VERSION, tokens: {} }
62-
return { version: DOC_VERSION, tokens: doc.tokens ?? {} }
67+
if (doc.version !== undefined && doc.version !== DOC_VERSION)
68+
return { version: DOC_VERSION, tokens: {} }
69+
return { version: DOC_VERSION, tokens: (doc.tokens ?? {}) as Record<string, Record<string, string>> }
6370
}
6471
}
6572

@@ -93,7 +100,12 @@ export class KeychainTokenStore implements TokenStore {
93100
}
94101

95102
write(host: string, email: string, bearer: string): void {
96-
new Entry(this.service, entryName(host, email)).setPassword(JSON.stringify(bearer))
103+
try {
104+
new Entry(this.service, entryName(host, email)).setPassword(JSON.stringify(bearer))
105+
}
106+
catch (err) {
107+
throw keyringUnavailableError(err)
108+
}
97109
}
98110

99111
remove(host: string, email: string): void {

0 commit comments

Comments
 (0)