Skip to content

Commit 5bcfb80

Browse files
ZenoWang1999claude
andauthored
XS⚠️ ◾ ✨ Fail fast when a desktop authorization is declined (#1000)
* Fail fast when an authorization is declined (#965) Both OAuth flows only ever heard back on success, so a user who declined consent — or hit a provider error — watched a spinner for the full 60s timeout before being told anything. The backend result pages now deep-link back with an `error` parameter on failure. Route that to a new auth-failed event on each token store, which the waiting authorization listens for alongside tokens-updated, so it rejects as soon as the outcome is known. YouTube gets a dedicated "declined" reason rather than reusing the timeout copy, which would have blamed a missing Google prompt for what was actually a deliberate cancellation. Backwards compatible in both directions: an older backend never sends `error` so the branch stays dormant, and the success path is untouched. * Resolve dev database paths from the app directory, not the cwd Returning from a browser authorization crashed the app with EPERM on `mkdir 'C:\WINDOWS\system32\data'`. Windows launches the app to service a `yakshaver-desktop-dev://` deep link from its own directory, so every cwd-relative dev path resolved somewhere unwritable — and because the db module opens its connection as it loads, this threw before the single-instance handler could hand the URL to the already-running app. The deep link was therefore lost entirely, leaving the waiting authorization to sit out its full timeout. Both paths need it: fixing only the database file moves the failure one step along to the migrations lookup, which resolves the same way. Development only — production already used absolute userData paths. * Match the auth result pages to the app's dark theme The app hardcodes class="dark" (src/ui/index.html), so these browser pages were the only light surface a user could hit mid-flow. Colours now come from the app's own dark tokens in src/ui/src/App.css, and the declined page picks up the shared --warning amber instead of the ad hoc shade it was mixing. Also documents why the MCP and YouTube branches fail fast on an `error` deep link rather than waiting out the authorization timeout (#965). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Correlate auth failures with the attempt that is waiting A failure callback was matched only by the server it named, so a tab left over from a timed-out attempt could cancel a newer authorization: the user retries, closes the stale tab, and the retry dies with it. The YouTube flow was worse — with a single binding there was nothing to match on at all, so any stale tab cancelled whatever was waiting. Each attempt now carries an id on the redirect URI it sends out, and a waiter ignores failures that name a different attempt. No backend change is needed: BuildFailureNotifyUrl appends `error=` while preserving the existing query, so the id round-trips on its own. Callbacks with no id are still honoured, so an attempt started before this shipped fails fast rather than regressing into a hang. Raised by the review bot on #1000. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Cover the attempt id reaching the failure signal Pins the end of the round trip: the router must forward the callback's attempt id to the waiter, and must still report a failure when an older callback carries none. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Trim the attempt-id comments They had grown to restate the mechanism at each call site. Keeps the reason the no-id case is accepted, since that one is not obvious from the code. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 5cfd430 commit 5bcfb80

15 files changed

Lines changed: 409 additions & 47 deletions

src/backend/assets/auth/errorTemplate.html

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
<title>YakShaver | Authorization Error</title>
88
<style>
99
:root {
10-
color-scheme: light;
10+
color-scheme: dark;
1111
}
1212

1313
* {
@@ -36,19 +36,19 @@
3636
Helvetica,
3737
Arial,
3838
sans-serif;
39-
background: linear-gradient(180deg, #fafafa 0%, #f2f2f2 100%);
40-
color: #1a1414;
39+
background: #1a1414;
40+
color: #faf9f8;
4141
}
4242

4343
.card {
4444
width: 100%;
4545
max-width: 420px;
46-
background: #ffffff;
47-
border: 1px solid #ececec;
46+
background: #2b2523;
47+
border: 1px solid rgba(255, 255, 255, 0.1);
4848
border-radius: 16px;
4949
padding: 40px 32px 32px;
5050
text-align: center;
51-
box-shadow: 0 20px 45px rgba(26, 20, 20, 0.08);
51+
box-shadow: 0 20px 45px rgba(0, 0, 0, 0.35);
5252
}
5353

5454
.logo {
@@ -69,7 +69,7 @@
6969
width: 28px;
7070
height: 28px;
7171
border-radius: 999px;
72-
background: #cc4141;
72+
background: #ff453a;
7373
color: #ffffff;
7474
display: flex;
7575
align-items: center;
@@ -87,13 +87,13 @@
8787
line-height: 28px;
8888
font-weight: 600;
8989
margin: 0 0 8px;
90-
color: #1a1414;
90+
color: #faf9f8;
9191
}
9292

9393
p {
9494
font-size: 14px;
9595
line-height: 21px;
96-
color: #6b6262;
96+
color: rgba(255, 255, 255, 0.75);
9797
margin: 0 0 24px;
9898
}
9999

@@ -103,10 +103,10 @@
103103
}
104104

105105
.hint {
106-
margin-top: 4px;
106+
margin-top: 18px;
107107
margin-bottom: 0;
108108
font-size: 12px;
109-
color: #a39a9a;
109+
color: rgba(255, 255, 255, 0.5);
110110
}
111111
</style>
112112
</head>

src/backend/assets/auth/failureTemplate.html

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
<title>YakShaver | Authorization Declined</title>
88
<style>
99
:root {
10-
color-scheme: light;
10+
color-scheme: dark;
1111
}
1212

1313
* {
@@ -36,19 +36,19 @@
3636
Helvetica,
3737
Arial,
3838
sans-serif;
39-
background: linear-gradient(180deg, #fafafa 0%, #f2f2f2 100%);
40-
color: #1a1414;
39+
background: #1a1414;
40+
color: #faf9f8;
4141
}
4242

4343
.card {
4444
width: 100%;
4545
max-width: 420px;
46-
background: #ffffff;
47-
border: 1px solid #ececec;
46+
background: #2b2523;
47+
border: 1px solid rgba(255, 255, 255, 0.1);
4848
border-radius: 16px;
4949
padding: 40px 32px 32px;
5050
text-align: center;
51-
box-shadow: 0 20px 45px rgba(26, 20, 20, 0.08);
51+
box-shadow: 0 20px 45px rgba(0, 0, 0, 0.35);
5252
}
5353

5454
.logo {
@@ -87,13 +87,13 @@
8787
line-height: 28px;
8888
font-weight: 600;
8989
margin: 0 0 8px;
90-
color: #1a1414;
90+
color: #faf9f8;
9191
}
9292

9393
p {
9494
font-size: 14px;
9595
line-height: 21px;
96-
color: #6b6262;
96+
color: rgba(255, 255, 255, 0.75);
9797
margin: 0 0 24px;
9898
}
9999

@@ -103,10 +103,10 @@
103103
}
104104

105105
.hint {
106-
margin-top: 4px;
106+
margin-top: 18px;
107107
margin-bottom: 0;
108108
font-size: 12px;
109-
color: #a39a9a;
109+
color: rgba(255, 255, 255, 0.5);
110110
}
111111
</style>
112112
</head>

src/backend/assets/auth/successTemplate.html

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
<title>YakShaver | Signed In</title>
88
<style>
99
:root {
10-
color-scheme: light;
10+
color-scheme: dark;
1111
}
1212

1313
* {
@@ -36,19 +36,19 @@
3636
Helvetica,
3737
Arial,
3838
sans-serif;
39-
background: linear-gradient(180deg, #fafafa 0%, #f2f2f2 100%);
40-
color: #1a1414;
39+
background: #1a1414;
40+
color: #faf9f8;
4141
}
4242

4343
.card {
4444
width: 100%;
4545
max-width: 420px;
46-
background: #ffffff;
47-
border: 1px solid #ececec;
46+
background: #2b2523;
47+
border: 1px solid rgba(255, 255, 255, 0.1);
4848
border-radius: 16px;
4949
padding: 40px 32px 32px;
5050
text-align: center;
51-
box-shadow: 0 20px 45px rgba(26, 20, 20, 0.08);
51+
box-shadow: 0 20px 45px rgba(0, 0, 0, 0.35);
5252
}
5353

5454
.logo {
@@ -87,13 +87,13 @@
8787
line-height: 28px;
8888
font-weight: 600;
8989
margin: 0 0 8px;
90-
color: #1a1414;
90+
color: #faf9f8;
9191
}
9292

9393
p {
9494
font-size: 14px;
9595
line-height: 21px;
96-
color: #6b6262;
96+
color: rgba(255, 255, 255, 0.75);
9797
margin: 0 0 24px;
9898
}
9999

@@ -128,7 +128,7 @@
128128
margin-top: 18px;
129129
margin-bottom: 0;
130130
font-size: 12px;
131-
color: #a39a9a;
131+
color: rgba(255, 255, 255, 0.5);
132132
}
133133
</style>
134134
</head>

src/backend/db/client.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,12 @@ const configureSqlite = (sqliteInstance: Database.Database): void => {
1414
export const getDbPath = (): string => {
1515
const databaseFileName = "database.sqlite";
1616
if (process.env.NODE_ENV === "development") {
17-
return path.join(process.cwd(), "data", databaseFileName);
17+
// Anchored to the app directory rather than the working directory: when Windows launches the
18+
// app to service a `yakshaver-desktop-dev://` deep link it starts the process from its own
19+
// directory (system32), and a cwd-relative path sent this at an unwritable location. Because
20+
// this module opens the database as it loads, that threw before the single-instance handler
21+
// could hand the URL to the running app.
22+
return path.join(app.getAppPath(), "data", databaseFileName);
1823
}
1924
return path.join(app.getPath("userData"), databaseFileName);
2025
};

src/backend/db/migrate.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import fs from "node:fs";
22
import path from "node:path";
33
import { sql } from "drizzle-orm";
44
import { migrate } from "drizzle-orm/better-sqlite3/migrator";
5+
import { app } from "electron";
56
import { getDb, getDbPath } from "./client";
67
import { DatabaseBackupService } from "./services/backup-service";
78

@@ -33,9 +34,10 @@ function ensureRequiredTablesExist(): void {
3334
function getMigrationsPath(): string {
3435
const isDev = process.env.NODE_ENV === "development";
3536

36-
// Development: use project source directly
37+
// Development: use project source directly. Anchored to the app directory for the same reason as
38+
// getDbPath — a deep-link launch starts the process from an unrelated working directory.
3739
if (isDev) {
38-
const devPath = path.join(process.cwd(), "src", "backend", "db", "migrations");
40+
const devPath = path.join(app.getAppPath(), "src", "backend", "db", "migrations");
3941
if (fs.existsSync(devPath)) {
4042
return devPath;
4143
}

src/backend/protocol/protocol-router.test.ts

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,35 @@ describe("protocol-router", () => {
8080
expect(send).not.toHaveBeenCalled();
8181
});
8282

83+
it("forwards the attempt id so a stale tab cannot cancel a newer authorization", async () => {
84+
const notifyAuthFailed = vi.fn();
85+
vi.mocked(McpOAuthTokenStorage.getInstance).mockReturnValue({
86+
notifyAuthFailed,
87+
} as unknown as McpOAuthTokenStorage);
88+
89+
await handleProtocolUrl(
90+
"yakshaver-desktop://oauth/callback?serverId=server-1&error=authorization_failed&attemptId=attempt-7",
91+
mockWindow(vi.fn()),
92+
);
93+
94+
expect(notifyAuthFailed).toHaveBeenCalledWith("server-1", "attempt-7");
95+
});
96+
97+
// A callback from before the attempt id shipped must still fail fast rather than hang.
98+
it("still reports failure when the callback carries no attempt id", async () => {
99+
const notifyAuthFailed = vi.fn();
100+
vi.mocked(McpOAuthTokenStorage.getInstance).mockReturnValue({
101+
notifyAuthFailed,
102+
} as unknown as McpOAuthTokenStorage);
103+
104+
await handleProtocolUrl(
105+
"yakshaver-desktop://oauth/callback?serverId=server-1&error=authorization_failed",
106+
mockWindow(vi.fn()),
107+
);
108+
109+
expect(notifyAuthFailed).toHaveBeenCalledWith("server-1", null);
110+
});
111+
83112
it("stores tokens for valid OAuth callback", async () => {
84113
const send = vi.fn();
85114
const window = mockWindow(send);

src/backend/protocol/protocol-router.ts

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import type { OAuthTokens } from "@ai-sdk/mcp";
22
import type { BrowserWindow } from "electron";
33
import { IPC_CHANNELS } from "../ipc/channels";
4+
import { AUTH_ATTEMPT_PARAM } from "../services/auth/auth-attempt";
45
import { IdentityServerAuthService } from "../services/auth/identity-server-auth";
56
import type { TokenData } from "../services/auth/types";
67
import { MCPServerManager } from "../services/mcp/mcp-server-manager";
@@ -32,13 +33,31 @@ const routeHandlers: Record<string, ProtocolRouteHandler> = {
3233
const accessToken = params.get("access_token");
3334
const refreshToken = params.get("refresh_token");
3435
const serverId = params.get("serverId");
36+
const authError = params.get("error");
37+
const attemptId = params.get(AUTH_ATTEMPT_PARAM);
3538

3639
console.log("[ProtocolRouter] Handling MCP OAuth callback", {
3740
serverId,
3841
hasAccessToken: !!accessToken,
3942
hasRefreshToken: !!refreshToken,
43+
authError,
4044
});
4145

46+
// The declined/error result pages ping back with `error` and no tokens so the waiting
47+
// authorization fails immediately instead of hanging until it times out (#965). The attempt id
48+
// travels with it so a stale tab cannot cancel whichever authorization is waiting now.
49+
if (authError) {
50+
console.warn("[ProtocolRouter] MCP OAuth callback reported failure", {
51+
serverId,
52+
authError,
53+
attemptId,
54+
});
55+
if (serverId) {
56+
McpOAuthTokenStorage.getInstance().notifyAuthFailed(serverId, attemptId);
57+
}
58+
return;
59+
}
60+
4261
if (!accessToken || !refreshToken || !serverId) {
4362
const missing = [
4463
!accessToken ? "access_token" : null,
@@ -115,12 +134,26 @@ const routeHandlers: Record<string, ProtocolRouteHandler> = {
115134
const refreshToken = params.get("refresh_token");
116135
const expiresIn = params.get("expires_in");
117136
const scope = params.get("scope");
137+
const authError = params.get("error");
138+
const attemptId = params.get(AUTH_ATTEMPT_PARAM);
118139

119140
console.log("[ProtocolRouter] Handling YouTube OAuth callback", {
120141
hasAccessToken: !!accessToken,
121142
hasRefreshToken: !!refreshToken,
143+
authError,
122144
});
123145

146+
// Mirrors the MCP branch above. There is only one YouTube binding, so the attempt id is the sole
147+
// thing separating a stale tab's failure from the attempt currently waiting.
148+
if (authError) {
149+
console.warn("[ProtocolRouter] YouTube OAuth callback reported failure", {
150+
authError,
151+
attemptId,
152+
});
153+
YoutubeStorage.getInstance().notifyAuthFailed(attemptId);
154+
return;
155+
}
156+
124157
if (!accessToken || !refreshToken) {
125158
const missing = [
126159
!accessToken ? "access_token" : null,
Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
import { describe, expect, it } from "vitest";
2+
import { isCurrentAuthAttempt } from "./auth-attempt";
3+
4+
describe("auth-attempt", () => {
5+
it("accepts a callback from the attempt being waited on", () => {
6+
expect(isCurrentAuthAttempt("attempt-1", "attempt-1")).toBe(true);
7+
});
8+
9+
it("rejects a callback from an earlier attempt", () => {
10+
expect(isCurrentAuthAttempt("attempt-1", "attempt-2")).toBe(false);
11+
});
12+
13+
// An attempt started before this shipped, or a flow that carries no id, must still fail fast
14+
// rather than hang until its timeout — so a missing id on either side is not a mismatch.
15+
it.each([
16+
["callback has no id", null, "attempt-2"],
17+
["waiter has no id", "attempt-1", undefined],
18+
["neither has an id", undefined, undefined],
19+
])("accepts the callback when %s", (_case, callbackId, waitingId) => {
20+
expect(isCurrentAuthAttempt(callbackId, waitingId)).toBe(true);
21+
});
22+
});
Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
/**
2+
* Query parameter carrying the id of the authorization attempt a callback belongs to.
3+
*
4+
* The desktop builds its own `redirectUri`, and the backend echoes it back verbatim when deep-linking
5+
* a failure, so an id added here survives the round trip with no backend change.
6+
*/
7+
export const AUTH_ATTEMPT_PARAM = "attemptId";
8+
9+
/**
10+
* Whether a callback belongs to the attempt currently being waited on. Without this, a tab left open
11+
* from an earlier attempt can report a failure that cancels the retry the user just started.
12+
*
13+
* Callbacks with no id are accepted — a flow that does not carry one must still fail fast rather
14+
* than hang until it times out. Only a callback naming a *different* attempt is discarded.
15+
*/
16+
export function isCurrentAuthAttempt(
17+
callbackAttemptId: string | null | undefined,
18+
waitingAttemptId: string | null | undefined,
19+
): boolean {
20+
if (!callbackAttemptId || !waitingAttemptId) {
21+
return true;
22+
}
23+
return callbackAttemptId === waitingAttemptId;
24+
}

src/backend/services/auth/youtube-auth-error.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@ export type YouTubeAuthErrorReason =
1111
| "backend_unreachable"
1212
| "auth_start_failed"
1313
| "timeout"
14+
/** The user declined consent, or the provider/backend failed the authorization (#965). */
15+
| "declined"
1416
| "unknown";
1517

1618
interface YouTubeAuthErrorOptions {
@@ -62,6 +64,8 @@ export function describeYouTubeAuthError(reason: YouTubeAuthErrorReason): string
6264
return "YouTube sign-in couldn't be started. Please try again in a moment.";
6365
case "timeout":
6466
return "Didn't hear back from YouTube. If no verification prompt appeared, check your Google or authenticator app, then click Connect to try again.";
67+
case "declined":
68+
return "YouTube wasn't connected because the authorization was cancelled. Click Connect to try again.";
6569
default:
6670
return "YouTube sign-in didn't complete. Please click Connect to try again.";
6771
}

0 commit comments

Comments
 (0)