mirror of
https://github.com/johannesjo/super-productivity.git
synced 2026-07-18 00:46:45 +00:00
fix(sync): improve Dropbox auth dialog UX on sandboxed Linux (#7139)
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 <details> 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.
This commit is contained in:
parent
654f4d80ee
commit
b642415702
8 changed files with 163 additions and 70 deletions
|
|
@ -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<void>; surface the failure reason instead
|
||||
// of silently swallowing it (e.g. when xdg-open / Flatpak portal rejects the URI).
|
||||
// shell.openExternal returns Promise<void>; 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);
|
||||
}
|
||||
});
|
||||
};
|
||||
|
||||
|
|
|
|||
|
|
@ -32,14 +32,11 @@
|
|||
<mat-icon>open_in_new</mat-icon>
|
||||
{{ T.F.SYNC.D_AUTH_CODE.GET_AUTH_CODE | translate }}</a
|
||||
>
|
||||
<button
|
||||
mat-button
|
||||
type="button"
|
||||
(click)="copyUrl()"
|
||||
>
|
||||
<mat-icon>content_copy</mat-icon>
|
||||
{{ T.F.SYNC.D_AUTH_CODE.COPY_URL | translate }}
|
||||
</button>
|
||||
|
||||
<details class="manual-url-fallback">
|
||||
<summary>{{ T.F.SYNC.D_AUTH_CODE.MANUAL_URL_HINT | translate }}</summary>
|
||||
<code class="manual-url">{{ data.url }}</code>
|
||||
</details>
|
||||
</div>
|
||||
</div>
|
||||
}
|
||||
|
|
@ -56,14 +53,12 @@
|
|||
<mat-icon>open_in_new</mat-icon>
|
||||
{{ T.F.SYNC.D_AUTH_CODE.GET_AUTH_CODE | translate }}</a
|
||||
>
|
||||
<button
|
||||
mat-button
|
||||
type="button"
|
||||
(click)="copyUrl()"
|
||||
>
|
||||
<mat-icon>content_copy</mat-icon>
|
||||
{{ T.F.SYNC.D_AUTH_CODE.COPY_URL | translate }}
|
||||
</button>
|
||||
|
||||
<details class="manual-url-fallback">
|
||||
<summary>{{ T.F.SYNC.D_AUTH_CODE.MANUAL_URL_HINT | translate }}</summary>
|
||||
<code class="manual-url">{{ data.url }}</code>
|
||||
</details>
|
||||
|
||||
<br />
|
||||
<br />
|
||||
<div class="oauth-waiting">
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -118,16 +118,4 @@ export class DialogGetAndEnterAuthCodeComponent implements OnDestroy {
|
|||
this.codeRequested.set(true);
|
||||
input.focus();
|
||||
}
|
||||
|
||||
async copyUrl(): Promise<void> {
|
||||
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,
|
||||
});
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -50,6 +50,20 @@ export class Dropbox implements SyncProviderServiceInterface<SyncProviderId.Drop
|
|||
private readonly _appKey: string;
|
||||
private readonly _basePath: string;
|
||||
|
||||
// Cached PKCE material — the verifier paired with the URL the user is
|
||||
// looking at. Reused across `getAuthHelper()` calls so a user who closes
|
||||
// and reopens the auth dialog (e.g. after `shell.openExternal()` fails
|
||||
// silently in Flatpak — issue #7139) can still exchange a code obtained
|
||||
// from the originally-shown URL. Cached as a Promise (not the resolved
|
||||
// value) so concurrent callers share one PKCE generation rather than
|
||||
// racing. Cleared on successful exchange and on explicit credential clear.
|
||||
// The Dropbox provider is a process-singleton via the providers factory,
|
||||
// so the cache spans the user session.
|
||||
private _pkcePromise: Promise<{
|
||||
codeVerifier: string;
|
||||
authUrl: string;
|
||||
}> | null = null;
|
||||
|
||||
public privateCfg: SyncCredentialStore<SyncProviderId.Dropbox>;
|
||||
|
||||
constructor(cfg: DropboxCfg) {
|
||||
|
|
@ -72,6 +86,7 @@ export class Dropbox implements SyncProviderServiceInterface<SyncProviderId.Drop
|
|||
}
|
||||
|
||||
async clearAuthCredentials(): Promise<void> {
|
||||
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<SyncProviderId.Drop
|
|||
* @returns Promise with auth helper object
|
||||
*/
|
||||
async getAuthHelper(): Promise<SyncProviderAuthHelper> {
|
||||
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 <T>(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;
|
||||
},
|
||||
};
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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',
|
||||
},
|
||||
|
|
|
|||
|
|
@ -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..."
|
||||
},
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue