From b64241570299cd8bf096955fa4e2d0d06e34f437 Mon Sep 17 00:00:00 2001 From: Johannes Millan Date: Sat, 25 Apr 2026 19:06:58 +0200 Subject: [PATCH] fix(sync): improve Dropbox auth dialog UX on sandboxed Linux (#7139) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "Get Authorization Code" button silently failed on Flatpak because shell.openExternal rejects without renderer feedback when the org.freedesktop.portal.Desktop talk-name isn't granted. After the user gave up and reopened the dialog, the second attempt failed again with `invalid_grant: invalid code verifier` because each call to Dropbox.getAuthHelper() generated a fresh PKCE verifier that no longer matched the originally-shown URL's challenge. Three changes: - Cache the in-flight PKCE Promise on the Dropbox provider so concurrent callers and consecutive dialog opens share one verifier+URL pair. Cleared on successful exchange, on clearAuthCredentials(), and on a rejected generation (so a one-time crypto failure doesn't poison the session). Five regression tests cover reuse, success-clear, explicit clear, concurrent calls, and rejection-recovery. - Render the auth URL as user-selectable text under a
disclosure. Escape hatch when both shell.openExternal and the clipboard portal are denied — the user can triple-click to select and Ctrl+C the URL into a manually-opened browser. Adds D_AUTH_CODE.MANUAL_URL_HINT translation key. - Pipe shell.openExternal rejections through errorHandlerWithFrontendInform so the existing IPC.ERROR snack channel surfaces a "Could not open the link in your browser" message instead of swallowing the failure to electron-log. Wrapped in a try/catch since errorHandlerWithFrontendInform throws synchronously if the renderer isn't ready. The Flathub manifest also needs --talk-name=org.freedesktop.portal.Desktop and --socket=wayland to fully fix the user-reported issue, but that change lives in the flathub repo. --- electron/main-window.ts | 16 +++- ...log-get-and-enter-auth-code.component.html | 27 +++--- ...log-get-and-enter-auth-code.component.scss | 17 ++++ ...ialog-get-and-enter-auth-code.component.ts | 12 --- .../dropbox/dropbox-auth-helper.spec.ts | 93 ++++++++++++++----- .../file-based/dropbox/dropbox.ts | 64 +++++++++---- src/app/t.const.ts | 2 +- src/assets/i18n/en.json | 2 +- 8 files changed, 163 insertions(+), 70 deletions(-) diff --git a/electron/main-window.ts b/electron/main-window.ts index 55d73fda6d..2b323a6767 100644 --- a/electron/main-window.ts +++ b/electron/main-window.ts @@ -361,10 +361,22 @@ function initWinEventListeners(app: Electron.App): void { const urlObj = new URL(url); urlObj.pathname = urlObj.pathname.replace('//', '/'); const wellFormedUrl = urlObj.toString(); - // shell.openExternal returns Promise; surface the failure reason instead - // of silently swallowing it (e.g. when xdg-open / Flatpak portal rejects the URI). + // shell.openExternal returns Promise; surface the failure to the + // renderer (snack via IPC.ERROR) so users on sandboxed packagings + // (Flatpak without OpenURI portal, etc.) see why nothing happened. shell.openExternal(wellFormedUrl).catch((err) => { error('Failed to open external URL via shell.openExternal:', err); + // Best-effort renderer notification — guard against the case where the + // frontend isn't ready (e.g. during shutdown or pre-load), in which + // case errorHandlerWithFrontendInform throws synchronously. + try { + errorHandlerWithFrontendInform( + 'Could not open the link in your browser. Copy the URL manually if available.', + err, + ); + } catch (informErr) { + error('Could not surface open-external failure to renderer:', informErr); + } }); }; diff --git a/src/app/imex/sync/dialog-get-and-enter-auth-code/dialog-get-and-enter-auth-code.component.html b/src/app/imex/sync/dialog-get-and-enter-auth-code/dialog-get-and-enter-auth-code.component.html index 434f18b2d2..ba9b499f02 100644 --- a/src/app/imex/sync/dialog-get-and-enter-auth-code/dialog-get-and-enter-auth-code.component.html +++ b/src/app/imex/sync/dialog-get-and-enter-auth-code/dialog-get-and-enter-auth-code.component.html @@ -32,14 +32,11 @@ open_in_new {{ T.F.SYNC.D_AUTH_CODE.GET_AUTH_CODE | translate }} - + +
+ {{ T.F.SYNC.D_AUTH_CODE.MANUAL_URL_HINT | translate }} + {{ data.url }} +
} @@ -56,14 +53,12 @@ open_in_new {{ T.F.SYNC.D_AUTH_CODE.GET_AUTH_CODE | translate }} - + +
+ {{ T.F.SYNC.D_AUTH_CODE.MANUAL_URL_HINT | translate }} + {{ data.url }} +
+

diff --git a/src/app/imex/sync/dialog-get-and-enter-auth-code/dialog-get-and-enter-auth-code.component.scss b/src/app/imex/sync/dialog-get-and-enter-auth-code/dialog-get-and-enter-auth-code.component.scss index ddda82fe0e..701e9d1b93 100644 --- a/src/app/imex/sync/dialog-get-and-enter-auth-code/dialog-get-and-enter-auth-code.component.scss +++ b/src/app/imex/sync/dialog-get-and-enter-auth-code/dialog-get-and-enter-auth-code.component.scss @@ -9,3 +9,20 @@ flex-direction: column; gap: var(--s2); } + +.manual-url-fallback { + margin-top: var(--s); + font-size: 0.85em; + opacity: 0.8; +} + +.manual-url { + display: block; + margin-top: var(--s); + padding: var(--s); + word-break: break-all; + user-select: all; + font-family: monospace; + background: var(--bg-lighter); + border-radius: 4px; +} diff --git a/src/app/imex/sync/dialog-get-and-enter-auth-code/dialog-get-and-enter-auth-code.component.ts b/src/app/imex/sync/dialog-get-and-enter-auth-code/dialog-get-and-enter-auth-code.component.ts index 93bb4c1c01..06a70162c3 100644 --- a/src/app/imex/sync/dialog-get-and-enter-auth-code/dialog-get-and-enter-auth-code.component.ts +++ b/src/app/imex/sync/dialog-get-and-enter-auth-code/dialog-get-and-enter-auth-code.component.ts @@ -118,16 +118,4 @@ export class DialogGetAndEnterAuthCodeComponent implements OnDestroy { this.codeRequested.set(true); input.focus(); } - - async copyUrl(): Promise { - try { - await navigator.clipboard.writeText(this.data.url); - this._snackService.open(T.GLOBAL_SNACK.COPY_TO_CLIPPBOARD); - } catch { - this._snackService.open({ - type: 'ERROR', - msg: T.PS.FAILED_TO_COPY_TO_CLIPBOARD, - }); - } - } } diff --git a/src/app/op-log/sync-providers/file-based/dropbox/dropbox-auth-helper.spec.ts b/src/app/op-log/sync-providers/file-based/dropbox/dropbox-auth-helper.spec.ts index 5f9b603bba..61994e70e0 100644 --- a/src/app/op-log/sync-providers/file-based/dropbox/dropbox-auth-helper.spec.ts +++ b/src/app/op-log/sync-providers/file-based/dropbox/dropbox-auth-helper.spec.ts @@ -1,18 +1,21 @@ import { Dropbox } from './dropbox'; +import { DropboxApi } from './dropbox-api'; import { generateCodeChallenge } from '../../../../util/pkce.util'; /** - * Regression/demonstration tests for issue #7139 — the secondary PKCE-mismatch bug. + * PKCE auth-helper lifecycle (issue #7139). * - * Scenario: If the auth dialog is opened twice (e.g. user closes it after the - * "Get Authorization Code" button fails to open the system browser), each call - * to `getAuthHelper()` generates a fresh `codeVerifier` + matching `code_challenge`. - * An auth code obtained from the FIRST authorization URL cannot be exchanged - * using the SECOND helper's verifier — Dropbox rejects with `invalid_grant: - * invalid code verifier`, which is exactly what the user reports. + * Background: a user on Flatpak reported `invalid_grant: invalid code verifier` + * after the "Get Authorization Code" button silently failed and they reopened + * the auth dialog. Each call to `getAuthHelper()` used to generate a fresh + * verifier+challenge, so a code obtained from the first URL was no longer + * exchangeable with the second helper's verifier. + * + * Fix: cache the PKCE pair on the Dropbox instance and reuse it until the + * exchange succeeds (or credentials are explicitly cleared). */ describe('Dropbox.getAuthHelper — PKCE lifecycle (issue #7139)', () => { - it('returned authUrl must carry a code_challenge derived from the returned codeVerifier', async () => { + it('returned authUrl carries a code_challenge derived from the returned codeVerifier', async () => { const dropbox = new Dropbox({ appKey: 'test-key', basePath: '/' }); const helper = await dropbox.getAuthHelper(); @@ -25,25 +28,73 @@ describe('Dropbox.getAuthHelper — PKCE lifecycle (issue #7139)', () => { expect(challengeInUrl).toBe(expectedChallenge); }); - it('regenerates codeVerifier on every call — stale auth codes will fail token exchange', async () => { + it('reuses the same codeVerifier across consecutive calls so a stale-but-original auth code still exchanges', async () => { const dropbox = new Dropbox({ appKey: 'test-key', basePath: '/' }); const first = await dropbox.getAuthHelper(); const second = await dropbox.getAuthHelper(); - expect(first.codeVerifier).not.toBe(second.codeVerifier); + expect(first.codeVerifier).toBe(second.codeVerifier); + expect(first.authUrl).toBe(second.authUrl); + }); - const c1 = new URL(first.authUrl as string).searchParams.get('code_challenge'); - const c2 = new URL(second.authUrl as string).searchParams.get('code_challenge'); - expect(c1).not.toBe(c2); + it('serializes concurrent getAuthHelper calls onto a single PKCE generation', async () => { + const dropbox = new Dropbox({ appKey: 'test-key', basePath: '/' }); - // This is the crux of the user-visible bug: if the user ever opens the - // dialog twice (common when the "Get Authorization Code" button silently - // fails in Flatpak), the verifier paired with the first URL's challenge - // is GONE. Entering an auth code obtained from the first URL produces - // `invalid_grant: invalid code verifier`. - const firstChallengeMatchesSecondVerifier = - (await generateCodeChallenge(second.codeVerifier as string)) === c1; - expect(firstChallengeMatchesSecondVerifier).toBe(false); + const [first, second] = await Promise.all([ + dropbox.getAuthHelper(), + dropbox.getAuthHelper(), + ]); + + expect(first.codeVerifier).toBe(second.codeVerifier); + }); + + it('regenerates the codeVerifier after a successful exchange', async () => { + const dropbox = new Dropbox({ appKey: 'test-key', basePath: '/' }); + const api = (dropbox as unknown as { _api: DropboxApi })._api; + spyOn(api, 'getTokensFromAuthCode').and.resolveTo({ + accessToken: 'a', + refreshToken: 'r', + expiresAt: 0, + }); + + const first = await dropbox.getAuthHelper(); + await first.verifyCodeChallenge!('any-code'); + const second = await dropbox.getAuthHelper(); + + expect(second.codeVerifier).not.toBe(first.codeVerifier); + }); + + it('does not poison the cache when PKCE generation rejects', async () => { + const dropbox = new Dropbox({ appKey: 'test-key', basePath: '/' }); + const originalSubtle = (globalThis.crypto as Crypto).subtle; + Object.defineProperty(globalThis.crypto, 'subtle', { + configurable: true, + get: () => { + throw new Error('crypto unavailable'); + }, + }); + + try { + await expectAsync(dropbox.getAuthHelper()).toBeRejected(); + } finally { + Object.defineProperty(globalThis.crypto, 'subtle', { + configurable: true, + value: originalSubtle, + }); + } + + const recovered = await dropbox.getAuthHelper(); + expect(recovered.codeVerifier).toBeTruthy(); + }); + + it('regenerates the codeVerifier after clearAuthCredentials()', async () => { + const dropbox = new Dropbox({ appKey: 'test-key', basePath: '/' }); + + const first = await dropbox.getAuthHelper(); + await dropbox.clearAuthCredentials(); + const second = await dropbox.getAuthHelper(); + + expect(second.codeVerifier).not.toBe(first.codeVerifier); }); }); diff --git a/src/app/op-log/sync-providers/file-based/dropbox/dropbox.ts b/src/app/op-log/sync-providers/file-based/dropbox/dropbox.ts index 22354b2e40..d45f00529c 100644 --- a/src/app/op-log/sync-providers/file-based/dropbox/dropbox.ts +++ b/src/app/op-log/sync-providers/file-based/dropbox/dropbox.ts @@ -50,6 +50,20 @@ export class Dropbox implements SyncProviderServiceInterface | null = null; + public privateCfg: SyncCredentialStore; constructor(cfg: DropboxCfg) { @@ -72,6 +86,7 @@ export class Dropbox implements SyncProviderServiceInterface { + this._pkcePromise = null; const cfg = await this.privateCfg.load(); if (cfg?.accessToken || cfg?.refreshToken) { await this.privateCfg.setComplete({ ...cfg, accessToken: '', refreshToken: '' }); @@ -275,32 +290,47 @@ export class Dropbox implements SyncProviderServiceInterface { - const { codeVerifier, codeChallenge } = await generatePKCECodes(128); - - // Determine redirect URI based on platform (only for mobile) const redirectUri = this._getRedirectUri(); - let authCodeUrl = - `${DROPBOX_AUTH_URL}` + - `?response_type=code&client_id=${this._appKey}` + - '&code_challenge_method=S256' + - '&token_access_type=offline' + - `&code_challenge=${codeChallenge}`; - - // Only add redirect_uri for mobile platforms - if (redirectUri) { - authCodeUrl += `&redirect_uri=${encodeURIComponent(redirectUri)}`; + // Cache the in-flight Promise so concurrent callers (e.g. a double-clicked + // auth button) share one PKCE generation instead of racing. + if (!this._pkcePromise) { + const inFlight = (async () => { + const { codeVerifier, codeChallenge } = await generatePKCECodes(128); + let authCodeUrl = + `${DROPBOX_AUTH_URL}` + + `?response_type=code&client_id=${this._appKey}` + + '&code_challenge_method=S256' + + '&token_access_type=offline' + + `&code_challenge=${codeChallenge}`; + if (redirectUri) { + authCodeUrl += `&redirect_uri=${encodeURIComponent(redirectUri)}`; + } + return { codeVerifier, authUrl: authCodeUrl }; + })(); + // Don't poison the cache with a rejection — let a future call retry. + inFlight.catch(() => { + if (this._pkcePromise === inFlight) { + this._pkcePromise = null; + } + }); + this._pkcePromise = inFlight; } + // Captured by closure so the success-path nulling below doesn't strand an + // in-flight verifyCodeChallenge call. + const cached = await this._pkcePromise; return { - authUrl: authCodeUrl, - codeVerifier, + authUrl: cached.authUrl, + codeVerifier: cached.codeVerifier, verifyCodeChallenge: async (authCode: string) => { - return (await this._api.getTokensFromAuthCode( + const result = (await this._api.getTokensFromAuthCode( authCode, - codeVerifier, + cached.codeVerifier, redirectUri, )) as T; + this._pkcePromise = null; + return result; }, }; } diff --git a/src/app/t.const.ts b/src/app/t.const.ts index e4c19ebcba..62bc7319b2 100644 --- a/src/app/t.const.ts +++ b/src/app/t.const.ts @@ -1051,11 +1051,11 @@ const T = { FORCE_UPLOAD: 'F.SYNC.C.FORCE_UPLOAD', }, D_AUTH_CODE: { - COPY_URL: 'F.SYNC.D_AUTH_CODE.COPY_URL', FOLLOW_LINK: 'F.SYNC.D_AUTH_CODE.FOLLOW_LINK', FOLLOW_LINK_MOBILE: 'F.SYNC.D_AUTH_CODE.FOLLOW_LINK_MOBILE', GET_AUTH_CODE: 'F.SYNC.D_AUTH_CODE.GET_AUTH_CODE', L_AUTH_CODE: 'F.SYNC.D_AUTH_CODE.L_AUTH_CODE', + MANUAL_URL_HINT: 'F.SYNC.D_AUTH_CODE.MANUAL_URL_HINT', TITLE: 'F.SYNC.D_AUTH_CODE.TITLE', WAITING_FOR_AUTH: 'F.SYNC.D_AUTH_CODE.WAITING_FOR_AUTH', }, diff --git a/src/assets/i18n/en.json b/src/assets/i18n/en.json index 299dc0216a..71f7374c00 100644 --- a/src/assets/i18n/en.json +++ b/src/assets/i18n/en.json @@ -1034,11 +1034,11 @@ "FORCE_UPLOAD": "Forcing an upload of your data could lead to data loss. Are you sure you want to continue?" }, "D_AUTH_CODE": { - "COPY_URL": "Copy URL", "FOLLOW_LINK": "Please open the following link and copy the auth code provided there into the input field below.", "FOLLOW_LINK_MOBILE": "Please tap the button below to authorize. You will be automatically redirected back to the app.", "GET_AUTH_CODE": "Get Authorization Code", "L_AUTH_CODE": "Enter Auth Code", + "MANUAL_URL_HINT": "Browser didn't open? Copy this URL manually:", "TITLE": "Login: {{provider}}", "WAITING_FOR_AUTH": "Waiting for authentication..." },