Skip to content

Commit 73570c7

Browse files
committed
fix(desktop): scope credential presence grants to the operation proven
`provenUntil` was keyed on credentialId alone, and `authorizeForSecret` set and read it without reference to what the caller was about to do. `copyCredential` puts the plaintext on the clipboard from inside main and returns a boolean; `revealCredential` returns the string itself to the renderer. With one undifferentiated grant, approving a native "Copy password?" prompt silently authorized a plaintext reveal for the remaining 30s with no second prompt — the OS prompt is the only human-in-the-loop control on plaintext egress, and its label described something weaker than what it granted. Grants now carry their operation and are compared with an ordering rather than equality: reveal is the stronger claim, so a reveal grant still covers a later copy. That is deliberate, not laxity — it preserves the behaviour AUTH_GRACE_MS was designed for (the plaintext is already on screen, so re-prompting to put that same string on the clipboard buys nothing). Only the weaker-implies- stronger direction is closed. `operation` is required rather than optional so the compiler names every call site; it found both. Not addressed, deliberately: the non-biometric fallback still uses `defaultId: 1`, so a reflexive Enter confirms. That is a UX change and is left for a decision rather than folded in here.
1 parent 09b40cb commit 73570c7

3 files changed

Lines changed: 90 additions & 15 deletions

File tree

apps/desktop/src/main/browser-credentials/index.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,7 @@ export async function forgetAllCredentials(): Promise<BrowserCredentialMetadata[
8989
export async function revealCredential(id: string): Promise<string | null> {
9090
const authorized = await authorizeForSecret({
9191
credentialId: id,
92+
operation: 'reveal',
9293
reason: 'show a saved password',
9394
action: 'Show password',
9495
})
@@ -106,6 +107,7 @@ export async function revealCredential(id: string): Promise<string | null> {
106107
export async function copyCredential(id: string): Promise<boolean> {
107108
const authorized = await authorizeForSecret({
108109
credentialId: id,
110+
operation: 'copy',
109111
reason: 'copy a saved password',
110112
action: 'Copy password',
111113
})

apps/desktop/src/main/browser-credentials/os-auth.test.ts

Lines changed: 50 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,22 @@ function setPlatform(platform: NodeJS.Platform): void {
4343
}
4444

4545
function request(credentialId: string) {
46-
return { credentialId, reason: 'show a saved password', action: 'Show password' }
46+
return {
47+
credentialId,
48+
operation: 'reveal' as const,
49+
reason: 'show a saved password',
50+
action: 'Show password',
51+
}
52+
}
53+
54+
/** The weaker of the two operations: plaintext never leaves the main process. */
55+
function copyRequest(credentialId: string) {
56+
return {
57+
credentialId,
58+
operation: 'copy' as const,
59+
reason: 'copy a saved password',
60+
action: 'Copy password',
61+
}
4762
}
4863

4964
describe('authorizeForSecret', () => {
@@ -110,6 +125,39 @@ describe('authorizeForSecret', () => {
110125
expect(promptTouchID).toHaveBeenCalledTimes(2)
111126
})
112127

128+
it('never lets a copy consent stand in for a plaintext reveal', async () => {
129+
await expect(authorizeForSecret(copyRequest('c1'))).resolves.toBe(true)
130+
expect(promptTouchID).toHaveBeenCalledTimes(1)
131+
132+
// The user approved "Copy password?"; revealing hands the string to the
133+
// renderer, so it has to ask again rather than ride the copy grant.
134+
await expect(authorizeForSecret(request('c1'))).resolves.toBe(true)
135+
expect(promptTouchID).toHaveBeenCalledTimes(2)
136+
})
137+
138+
it('lets a reveal consent cover a later copy of the same credential', async () => {
139+
await expect(authorizeForSecret(request('c1'))).resolves.toBe(true)
140+
141+
// The plaintext is already on screen, so putting that same string on the
142+
// clipboard buys nothing by prompting twice.
143+
await expect(authorizeForSecret(copyRequest('c1'))).resolves.toBe(true)
144+
expect(promptTouchID).toHaveBeenCalledTimes(1)
145+
})
146+
147+
it('keeps a copy grant usable for further copies', async () => {
148+
await authorizeForSecret(copyRequest('c1'))
149+
await authorizeForSecret(copyRequest('c1'))
150+
151+
expect(promptTouchID).toHaveBeenCalledTimes(1)
152+
})
153+
154+
it('never widens a grant to another credential', async () => {
155+
await authorizeForSecret(request('c1'))
156+
await authorizeForSecret(request('c2'))
157+
158+
expect(promptTouchID).toHaveBeenCalledTimes(2)
159+
})
160+
113161
it('asks again after the credential is explicitly revoked', async () => {
114162
await authorizeForSecret(request('c1'))
115163
revokeSecretAuthorization('c1')
@@ -130,11 +178,7 @@ describe('authorizeForSecret', () => {
130178

131179
it('labels the fallback dialog with the action it is authorizing', async () => {
132180
canPromptTouchID.mockReturnValue(false)
133-
await authorizeForSecret({
134-
credentialId: 'c1',
135-
reason: 'copy a saved password',
136-
action: 'Copy password',
137-
})
181+
await authorizeForSecret(copyRequest('c1'))
138182

139183
expect(showMessageBox).toHaveBeenCalledWith(
140184
expect.objectContaining({

apps/desktop/src/main/browser-credentials/os-auth.ts

Lines changed: 38 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -12,11 +12,26 @@ const logger = createLogger('BrowserCredentialAuth')
1212
* their way to is typically still on screen — asking a second time to put that
1313
* same string on the clipboard is friction that buys nothing, and teaches
1414
* people to approve prompts without reading them.
15+
*
16+
* That reasoning only runs one way, which is why grants carry the operation
17+
* they were proven for: nothing about a copy implies the user agreed to hand
18+
* the plaintext to the renderer.
1519
*/
1620
const AUTH_GRACE_MS = 30_000
1721

18-
/** Credential id to the moment its proof of presence lapses. */
19-
const provenUntil = new Map<string, number>()
22+
/**
23+
* What a grant was proven for.
24+
*
25+
* The two are not equivalent, and the ordering matters: `copy` puts the
26+
* plaintext on the clipboard from inside the main process and returns only a
27+
* boolean, while `reveal` hands the string itself to the Sim renderer. So
28+
* `reveal` is the stronger claim and a proof of it covers a later `copy` — but
29+
* not the other way round.
30+
*/
31+
export type SecretOperation = 'reveal' | 'copy'
32+
33+
/** Credential id to its standing proof of presence. */
34+
const provenUntil = new Map<string, { expiry: number; operation: SecretOperation }>()
2035

2136
export interface SecretAuthRequest {
2237
/**
@@ -25,20 +40,33 @@ export interface SecretAuthRequest {
2540
* granted for.
2641
*/
2742
credentialId: string
43+
/**
44+
* What the caller is about to do. Required, because a grant that did not
45+
* record it let the weaker consent stand in for the stronger one: approving
46+
* a "Copy password?" prompt silently authorized a plaintext reveal to the
47+
* renderer for the rest of the window, with no second prompt and a label
48+
* that described something else.
49+
*/
50+
operation: SecretOperation
2851
/** Completes "Sim is about to ..." in the prompt. */
2952
reason: string
3053
/** Confirm-button label and title for the non-biometric fallback. */
3154
action: string
3255
}
3356

34-
function hasFreshProof(credentialId: string): boolean {
35-
const expiry = provenUntil.get(credentialId)
36-
if (expiry === undefined) return false
37-
if (Date.now() >= expiry) {
57+
/** Whether a proof of `granted` is enough to perform `requested`. */
58+
function grantCovers(granted: SecretOperation, requested: SecretOperation): boolean {
59+
return granted === 'reveal' || granted === requested
60+
}
61+
62+
function hasFreshProof(credentialId: string, operation: SecretOperation): boolean {
63+
const proof = provenUntil.get(credentialId)
64+
if (proof === undefined) return false
65+
if (Date.now() >= proof.expiry) {
3866
provenUntil.delete(credentialId)
3967
return false
4068
}
41-
return true
69+
return grantCovers(proof.operation, operation)
4270
}
4371

4472
/**
@@ -73,12 +101,13 @@ export function revokeSecretAuthorization(credentialId?: string): void {
73101
*/
74102
export async function authorizeForSecret({
75103
credentialId,
104+
operation,
76105
reason,
77106
action,
78107
}: SecretAuthRequest): Promise<boolean> {
79-
if (hasFreshProof(credentialId)) return true
108+
if (hasFreshProof(credentialId, operation)) return true
80109
if (!(await promptForSecret(reason, action))) return false
81-
provenUntil.set(credentialId, Date.now() + AUTH_GRACE_MS)
110+
provenUntil.set(credentialId, { expiry: Date.now() + AUTH_GRACE_MS, operation })
82111
return true
83112
}
84113

0 commit comments

Comments
 (0)