mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
fix(site): defer external app API key generation to on-click (#27907)
> 🤖 This PR was written by Coder Agents on behalf of Jake Howell. Resolves [DEVEX-460](https://linear.app/codercom/issue/DEVEX-460/defer-api-key-generation-in-useapplink-to-on-click-instead-of-page). ## Problem `useAppLink` minted a session key on mount via a `useQuery`: ```ts const { data: apiKeyResponse } = useQuery({ ...apiKey(), enabled: isExternalApp(app) && needsSessionToken(app), }); ``` Whenever any page mounted `useAppLink` for an external app that embeds `$SESSION_TOKEN` in its URL (JetBrains Gateway, Coder Desktop, etc.), it fired `POST /api/v2/users/me/keys` on render, even if the user never clicked the link. Each call minted a fresh session key and produced a `created token` audit-log entry. Simply navigating the dashboard generated a stream of `created token` entries with no real connection activity. This is the follow-up to #22318 (AIGOV-24), which only fixed the built-in VS Code / VS Code Insiders buttons in `WorkspacesTable`. ## Change Mirror the on-click minting pattern from #22318, applied to the shared `useAppLink` hook: - Replace the eager `useQuery(apiKey())` with a `useMutation(() => API.getApiKey())` that runs only when the user clicks a token-bearing external app. `onClick` mints the key, builds the final URL, and navigates via `location.href` (these are always custom-protocol, non-HTTP external apps). - The returned `href` no longer embeds a token; the hook owns opening for token apps. - Replace the `hasToken` field with `isLoading` (mint in-flight) and update call sites so token apps are always clickable, with a loading affordance while a key is being minted. - `TaskApps` external-app menu items now render an anchor with the hook's `onClick` (instead of a `RouterLink` that bypassed it), so the deferred mint runs there too. No API key is minted until the user actually opens an external app. ## Affected call sites - `site/src/modules/apps/useAppLink.ts` (core change) - `site/src/modules/resources/AppLink/AppLink.tsx` - `site/src/pages/AgentsPage/components/WorkspacePill.tsx` - `site/src/pages/TaskPage/TaskApps.tsx` ## Testing Added Storybook interaction tests (`play` functions) in `AppLink.stories.tsx` that assert the fix directly: - `ExternalAppDefersSessionToken`: a token-bearing external app mints **no** key on render, and mints exactly one on click. - `ExternalAppWithoutSessionTokenNeverMints`: an external app without `$SESSION_TOKEN` never mints a key, even on click. Also verified: - `tsc -p .` (frontend typecheck) passes - `biome check` on changed files passes - `vitest` for `AppLink` (incl. new stories), `apps`, `WorkspacePill`, `TaskApps`, and `AppStatuses` stories/tests pass - pre-commit hook (gen/fmt/lint/build) passes ## Manual verification checklist (recommended before merge) - With an external app that uses `$SESSION_TOKEN` (e.g. JetBrains Gateway), open DevTools → Network filtered on `users/me/keys`, reload `/workspaces` and the workspace detail page, and confirm **no** `POST` fires on render. - Confirm a `POST` fires only when the app link is clicked, and the app opens correctly.
This commit is contained in:
@@ -63,7 +63,7 @@ export const WorkspaceAppFrame: FC<WorkspaceAppFrameProps> = ({
|
||||
variant="subtle"
|
||||
onClick={(e) => {
|
||||
e.preventDefault();
|
||||
if (frameRef.current?.contentWindow) {
|
||||
if (link.href && frameRef.current?.contentWindow) {
|
||||
frameRef.current.contentWindow.location.href = link.href;
|
||||
}
|
||||
}}
|
||||
@@ -83,7 +83,11 @@ export const WorkspaceAppFrame: FC<WorkspaceAppFrameProps> = ({
|
||||
</DropdownMenuTrigger>
|
||||
<DropdownMenuContent align="end">
|
||||
<DropdownMenuItem asChild>
|
||||
<RouterLink to={link.href} target="_blank" rel="noreferrer">
|
||||
<RouterLink
|
||||
to={link.href ?? ""}
|
||||
target="_blank"
|
||||
rel="noreferrer"
|
||||
>
|
||||
<ExternalLinkIcon />
|
||||
Open app in new tab
|
||||
</RouterLink>
|
||||
|
||||
@@ -1,7 +1,8 @@
|
||||
import type React from "react";
|
||||
import { useQuery } from "react-query";
|
||||
import { useMutation } from "react-query";
|
||||
import { toast } from "sonner";
|
||||
import { apiKey } from "#/api/queries/users";
|
||||
import { API } from "#/api/api";
|
||||
import { getErrorMessage } from "#/api/errors";
|
||||
import type {
|
||||
Workspace,
|
||||
WorkspaceAgent,
|
||||
@@ -21,10 +22,12 @@ type UseAppLinkParams = {
|
||||
};
|
||||
|
||||
type AppLink = {
|
||||
href: string;
|
||||
// Token-backed external apps intentionally expose no href: their URL is only
|
||||
// complete once a session token is minted on click.
|
||||
href: string | undefined;
|
||||
onClick: (e: React.MouseEvent) => void;
|
||||
label: string;
|
||||
hasToken: boolean;
|
||||
isLoading: boolean;
|
||||
};
|
||||
|
||||
export const useAppLink = (
|
||||
@@ -33,20 +36,100 @@ export const useAppLink = (
|
||||
): AppLink => {
|
||||
const label = app.display_name ?? app.slug;
|
||||
const { proxy } = useProxy();
|
||||
const { data: apiKeyResponse } = useQuery({
|
||||
...apiKey(),
|
||||
enabled: isExternalApp(app) && needsSessionToken(app),
|
||||
|
||||
// External apps that embed the session token in their URL need a freshly
|
||||
// minted key. We defer minting until the user clicks (see `onClick`) rather
|
||||
// than on mount, so that merely rendering a link no longer mints (and
|
||||
// audits) a session key for an app the user may never open.
|
||||
const requiresSessionToken = isExternalApp(app) && needsSessionToken(app);
|
||||
|
||||
const buildHref = (token: string): string =>
|
||||
getAppHref(app, {
|
||||
agent,
|
||||
workspace,
|
||||
token,
|
||||
path: proxy.preferredPathAppURL,
|
||||
host: proxy.preferredWildcardHostname,
|
||||
});
|
||||
|
||||
// Custom-protocol (non-HTTP) external apps can silently fail when the target
|
||||
// application isn't installed. The browser blurs when it hands control to
|
||||
// the protocol handler, which clears the timeout before the error fires.
|
||||
const notifyOnOpenExternalAppFailed = () => {
|
||||
const openAppExternallyFailedTimeout = 1500;
|
||||
const openAppExternallyFailed = setTimeout(() => {
|
||||
// Check if this is a JetBrains IDE app
|
||||
// starts with "jetbrains-gateway://connect#type=coder" (from https://registry.coder.com/modules/coder/jetbrains-gateway)
|
||||
const isJetBrainsGateway = app.url?.startsWith("jetbrains-gateway:");
|
||||
// starts with "jetbrains://gateway/coder" (from https://registry.coder.com/modules/coder/jetbrains)
|
||||
const isJetBrainsToolbox = app.url?.startsWith("jetbrains:");
|
||||
|
||||
// Check if this is a coder:// URL
|
||||
const isCoderApp = app.url?.startsWith("coder:");
|
||||
|
||||
if (isJetBrainsGateway) {
|
||||
toast.error(`Failed to open "${label}".`, {
|
||||
description: "JetBrains Gateway must be installed.",
|
||||
});
|
||||
} else if (isJetBrainsToolbox) {
|
||||
toast.error(`Failed to open "${label}".`, {
|
||||
description: "JetBrains Toolbox must be installed.",
|
||||
});
|
||||
} else if (isCoderApp) {
|
||||
toast.error(`Failed to open "${label}".`, {
|
||||
description: "Coder Desktop must be installed.",
|
||||
});
|
||||
} else {
|
||||
toast.error(`Failed to open "${label}".`, {
|
||||
description: "The app must be installed first.",
|
||||
});
|
||||
}
|
||||
}, openAppExternallyFailedTimeout);
|
||||
window.addEventListener(
|
||||
"blur",
|
||||
() => {
|
||||
clearTimeout(openAppExternallyFailed);
|
||||
},
|
||||
{ once: true },
|
||||
);
|
||||
};
|
||||
|
||||
// The success/error handlers live on the mutation (not on the `mutate` call)
|
||||
// so they still run when the triggering element unmounts before the request
|
||||
// settles, e.g. a dropdown menu item that closes on select. Callbacks passed
|
||||
// to `mutate` are dropped once the observer unmounts, which would otherwise
|
||||
// swallow both the navigation and the failure toast.
|
||||
const generateKeyMutation = useMutation({
|
||||
mutationFn: () => API.getApiKey(),
|
||||
onSuccess: ({ key }) => {
|
||||
notifyOnOpenExternalAppFailed();
|
||||
location.href = buildHref(key);
|
||||
},
|
||||
onError: (error) => {
|
||||
toast.error(getErrorMessage(error, `Failed to open "${label}".`));
|
||||
},
|
||||
});
|
||||
|
||||
const href = getAppHref(app, {
|
||||
agent,
|
||||
workspace,
|
||||
token: apiKeyResponse?.key,
|
||||
path: proxy.preferredPathAppURL,
|
||||
host: proxy.preferredWildcardHostname,
|
||||
});
|
||||
// Token-backed apps expose no navigable href: the token is minted on click
|
||||
// and the final URL is built then. Exposing a tokenless href would let
|
||||
// middle-click or "Open link" launch the custom protocol with an empty
|
||||
// token, so we omit it entirely. Non-token apps still render as anchors.
|
||||
const href = requiresSessionToken ? undefined : buildHref("");
|
||||
|
||||
const onClick = (e: React.MouseEvent) => {
|
||||
// Apps that embed a session token mint it on click instead of on mount.
|
||||
// These are always custom-protocol (non-HTTP) external apps, so we build
|
||||
// the final URL with the freshly minted token and navigate to it via
|
||||
// `location.href`, relying on the browser's protocol handler.
|
||||
if (requiresSessionToken) {
|
||||
e.preventDefault();
|
||||
if (generateKeyMutation.isPending) {
|
||||
return;
|
||||
}
|
||||
generateKeyMutation.mutate();
|
||||
return;
|
||||
}
|
||||
|
||||
if (!e.currentTarget.getAttribute("href")) {
|
||||
return;
|
||||
}
|
||||
@@ -57,41 +140,7 @@ export const useAppLink = (
|
||||
app.external && app.url && !app.url.startsWith("http");
|
||||
|
||||
if (isExternalProtocolApp) {
|
||||
// When browser recognizes the protocol and is able to navigate to the app,
|
||||
// it will blur away, and will stop the timer. Otherwise,
|
||||
// an error message will be displayed.
|
||||
const openAppExternallyFailedTimeout = 1500;
|
||||
const openAppExternallyFailed = setTimeout(() => {
|
||||
// Check if this is a JetBrains IDE app
|
||||
// starts with "jetbrains-gateway://connect#type=coder" (from https://registry.coder.com/modules/coder/jetbrains-gateway)
|
||||
const isJetBrainsGateway = app.url?.startsWith("jetbrains-gateway:");
|
||||
// starts with "jetbrains://gateway/coder" (from https://registry.coder.com/modules/coder/jetbrains)
|
||||
const isJetBrainsToolbox = app.url?.startsWith("jetbrains:");
|
||||
|
||||
// Check if this is a coder:// URL
|
||||
const isCoderApp = app.url?.startsWith("coder:");
|
||||
|
||||
if (isJetBrainsGateway) {
|
||||
toast.error(`Failed to open "${label}".`, {
|
||||
description: "JetBrains Gateway must be installed.",
|
||||
});
|
||||
} else if (isJetBrainsToolbox) {
|
||||
toast.error(`Failed to open "${label}".`, {
|
||||
description: "JetBrains Toolbox must be installed.",
|
||||
});
|
||||
} else if (isCoderApp) {
|
||||
toast.error(`Failed to open "${label}".`, {
|
||||
description: "Coder Desktop must be installed.",
|
||||
});
|
||||
} else {
|
||||
toast.error(`Failed to open "${label}".`, {
|
||||
description: "The app must be installed first.",
|
||||
});
|
||||
}
|
||||
}, openAppExternallyFailedTimeout);
|
||||
window.addEventListener("blur", () => {
|
||||
clearTimeout(openAppExternallyFailed);
|
||||
});
|
||||
notifyOnOpenExternalAppFailed();
|
||||
|
||||
// Custom protocol external apps don't support open_in since they
|
||||
// rely on the browser's protocol handling.
|
||||
@@ -101,7 +150,9 @@ export const useAppLink = (
|
||||
switch (app.open_in) {
|
||||
case "slim-window": {
|
||||
e.preventDefault();
|
||||
openAppInNewWindow(href);
|
||||
if (href) {
|
||||
openAppInNewWindow(href);
|
||||
}
|
||||
return;
|
||||
}
|
||||
}
|
||||
@@ -111,6 +162,6 @@ export const useAppLink = (
|
||||
href,
|
||||
onClick,
|
||||
label,
|
||||
hasToken: Boolean(apiKeyResponse?.key),
|
||||
isLoading: generateKeyMutation.isPending,
|
||||
};
|
||||
};
|
||||
|
||||
@@ -1,5 +1,13 @@
|
||||
import type { Meta, StoryObj } from "@storybook/react-vite";
|
||||
import { expect, screen, spyOn, userEvent, within } from "storybook/test";
|
||||
import {
|
||||
expect,
|
||||
screen,
|
||||
spyOn,
|
||||
userEvent,
|
||||
waitFor,
|
||||
within,
|
||||
} from "storybook/test";
|
||||
import { API } from "#/api/api";
|
||||
import { getPreferredProxy } from "#/contexts/ProxyContext";
|
||||
import {
|
||||
MockPrimaryWorkspaceProxy,
|
||||
@@ -257,6 +265,91 @@ export const WithTooltip: Story = {
|
||||
},
|
||||
};
|
||||
|
||||
// Regression test for DEVEX-460: external apps that embed the session token
|
||||
// must not mint an API key on render. The key is minted only when the user
|
||||
// clicks the link.
|
||||
export const ExternalAppDefersSessionToken: Story = {
|
||||
decorators: [withToaster],
|
||||
// Install the spy before the component renders. `play` runs after render and
|
||||
// its effects, so a regression back to eager (on-mount) minting would fetch a
|
||||
// key before a spy installed in `play` exists, and the later
|
||||
// `not.toHaveBeenCalled()` assertion would still pass.
|
||||
beforeEach: () => {
|
||||
// Never resolve: we only assert whether/when the request fires, and
|
||||
// leaving it pending avoids the subsequent protocol-handler navigation.
|
||||
const getApiKey = spyOn(API, "getApiKey").mockImplementation(
|
||||
() => new Promise(() => {}),
|
||||
);
|
||||
return () => {
|
||||
getApiKey.mockRestore();
|
||||
};
|
||||
},
|
||||
args: {
|
||||
workspace: MockWorkspace,
|
||||
app: {
|
||||
...MockWorkspaceApp,
|
||||
external: true,
|
||||
url: "jetbrains-gateway://connect?token=$SESSION_TOKEN",
|
||||
},
|
||||
agent: MockWorkspaceAgent,
|
||||
},
|
||||
play: async ({ canvasElement, step }) => {
|
||||
const canvas = within(canvasElement);
|
||||
// Token-backed apps expose no href and render as a button (the token is
|
||||
// minted on click), so query by role "button".
|
||||
const trigger = await canvas.findByRole("button");
|
||||
const user = userEvent.setup();
|
||||
|
||||
await step("no API key is minted on render", async () => {
|
||||
expect(API.getApiKey).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
await step("clicking mints the API key on demand", async () => {
|
||||
await user.click(trigger);
|
||||
await waitFor(() => expect(API.getApiKey).toHaveBeenCalledTimes(1));
|
||||
});
|
||||
},
|
||||
};
|
||||
|
||||
// External apps that do not embed the session token must never mint a key,
|
||||
// even on click, so we don't create session keys for apps that don't need one.
|
||||
export const ExternalAppWithoutSessionTokenNeverMints: Story = {
|
||||
decorators: [withToaster],
|
||||
// Install the spy before render so an on-mount mint would be observed.
|
||||
beforeEach: () => {
|
||||
const getApiKey = spyOn(API, "getApiKey").mockResolvedValue({
|
||||
key: "test-key",
|
||||
});
|
||||
return () => {
|
||||
getApiKey.mockRestore();
|
||||
};
|
||||
},
|
||||
args: {
|
||||
workspace: MockWorkspace,
|
||||
app: {
|
||||
...MockWorkspaceApp,
|
||||
external: true,
|
||||
url: "https://example.com",
|
||||
open_in: "slim-window",
|
||||
},
|
||||
agent: MockWorkspaceAgent,
|
||||
},
|
||||
play: async ({ canvasElement, step }) => {
|
||||
// The app opens in a slim window, so stub window.open to keep the click
|
||||
// from navigating the test frame.
|
||||
spyOn(window, "open").mockReturnValue(null);
|
||||
const canvas = within(canvasElement);
|
||||
const link = await canvas.findByRole("link");
|
||||
const user = userEvent.setup();
|
||||
|
||||
await step("no API key is minted on render or click", async () => {
|
||||
expect(API.getApiKey).not.toHaveBeenCalled();
|
||||
await user.click(link);
|
||||
expect(API.getApiKey).not.toHaveBeenCalled();
|
||||
});
|
||||
},
|
||||
};
|
||||
|
||||
export const SlimWindowPopupBlocked: Story = {
|
||||
decorators: [withToaster],
|
||||
args: {
|
||||
|
||||
@@ -21,8 +21,6 @@ import { useProxy } from "#/contexts/ProxyContext";
|
||||
import {
|
||||
isAppBlockedByMissingWildcard,
|
||||
isAppUrlValid,
|
||||
isExternalApp,
|
||||
needsSessionToken,
|
||||
} from "#/modules/apps/apps";
|
||||
import { useAppLink } from "#/modules/apps/useAppLink";
|
||||
import { docs } from "#/utils/docs";
|
||||
@@ -132,8 +130,11 @@ export const AppLink: FC<AppLinkProps> = ({
|
||||
);
|
||||
}
|
||||
|
||||
if (isExternalApp(app) && needsSessionToken(app) && !link.hasToken) {
|
||||
canClick = false;
|
||||
// The session token for external apps is minted on click, so key generation
|
||||
// no longer gates clickability. While a click is minting a token, show a
|
||||
// spinner to reflect the in-flight request.
|
||||
if (link.isLoading) {
|
||||
icon = <Spinner loading />;
|
||||
}
|
||||
|
||||
if (
|
||||
@@ -156,32 +157,44 @@ export const AppLink: FC<AppLinkProps> = ({
|
||||
shareIcon: null,
|
||||
};
|
||||
|
||||
const button = grouped ? (
|
||||
<DropdownMenuItem asChild>
|
||||
<a
|
||||
href={canClick ? link.href : undefined}
|
||||
onClick={link.onClick}
|
||||
target={app.open_in === "tab" ? "_blank" : undefined}
|
||||
rel={app.open_in === "tab" ? "noreferrer" : undefined}
|
||||
>
|
||||
{icon}
|
||||
{link.label}
|
||||
{ShareIcon && <ShareIcon />}
|
||||
</a>
|
||||
</DropdownMenuItem>
|
||||
// Token-minting external apps expose no navigable href (see useAppLink): the
|
||||
// URL is only complete after the on-click mint. Render them as a button so
|
||||
// they stay interactive. A bare anchor without href is styled and treated as
|
||||
// disabled by AgentButton, and middle-clicking one would otherwise launch
|
||||
// the custom protocol with an empty token.
|
||||
const opensViaClick = link.href === undefined;
|
||||
|
||||
const content = (
|
||||
<>
|
||||
{icon}
|
||||
{link.label}
|
||||
{ShareIcon && <ShareIcon />}
|
||||
</>
|
||||
);
|
||||
|
||||
const trigger = opensViaClick ? (
|
||||
<button
|
||||
type="button"
|
||||
onClick={link.onClick}
|
||||
disabled={!canClick || link.isLoading}
|
||||
>
|
||||
{content}
|
||||
</button>
|
||||
) : (
|
||||
<AgentButton asChild>
|
||||
<a
|
||||
href={canClick ? link.href : undefined}
|
||||
onClick={link.onClick}
|
||||
target={app.open_in === "tab" ? "_blank" : undefined}
|
||||
rel={app.open_in === "tab" ? "noreferrer" : undefined}
|
||||
>
|
||||
{icon}
|
||||
{link.label}
|
||||
{ShareIcon && <ShareIcon />}
|
||||
</a>
|
||||
</AgentButton>
|
||||
<a
|
||||
href={canClick ? link.href : undefined}
|
||||
onClick={link.onClick}
|
||||
target={app.open_in === "tab" ? "_blank" : undefined}
|
||||
rel={app.open_in === "tab" ? "noreferrer" : undefined}
|
||||
>
|
||||
{content}
|
||||
</a>
|
||||
);
|
||||
|
||||
const button = grouped ? (
|
||||
<DropdownMenuItem asChild>{trigger}</DropdownMenuItem>
|
||||
) : (
|
||||
<AgentButton asChild>{trigger}</AgentButton>
|
||||
);
|
||||
|
||||
if (primaryTooltip || app.tooltip) {
|
||||
|
||||
@@ -39,8 +39,6 @@ import { useIsBelowMdViewport } from "#/hooks/useIsBelowMdViewport";
|
||||
import {
|
||||
getTerminalHref,
|
||||
getVSCodeHref,
|
||||
isExternalApp,
|
||||
needsSessionToken,
|
||||
openAppInNewWindow,
|
||||
} from "#/modules/apps/apps";
|
||||
import { useAppLink } from "#/modules/apps/useAppLink";
|
||||
@@ -338,13 +336,10 @@ const AppMenuItem: FC<{
|
||||
}> = ({ app, workspace, agent, isRunning }) => {
|
||||
const link = useAppLink(app, { workspace, agent });
|
||||
|
||||
const canClick =
|
||||
!isExternalApp(app) || !needsSessionToken(app) || link.hasToken;
|
||||
|
||||
return (
|
||||
<DropdownMenuItem asChild disabled={!canClick || !isRunning}>
|
||||
<DropdownMenuItem asChild disabled={!isRunning || link.isLoading}>
|
||||
<a
|
||||
href={canClick && isRunning ? link.href : undefined}
|
||||
href={isRunning ? link.href : undefined}
|
||||
onClick={link.onClick}
|
||||
target="_blank"
|
||||
rel="noreferrer"
|
||||
|
||||
@@ -172,12 +172,19 @@ const ExternalAppMenuItem: FC<{
|
||||
workspace,
|
||||
});
|
||||
|
||||
// External apps mint their session token on click via `link.onClick`, so we
|
||||
// render a plain anchor (not a RouterLink) and let the hook handle opening.
|
||||
return (
|
||||
<DropdownMenuItem asChild>
|
||||
<RouterLink to={link.href}>
|
||||
<a
|
||||
href={link.href}
|
||||
onClick={link.onClick}
|
||||
target="_blank"
|
||||
rel="noreferrer"
|
||||
>
|
||||
{app.icon ? <ExternalImage src={app.icon} /> : <LayoutGridIcon />}
|
||||
{link.label}
|
||||
</RouterLink>
|
||||
</a>
|
||||
</DropdownMenuItem>
|
||||
);
|
||||
};
|
||||
@@ -201,7 +208,7 @@ const TaskAppTab: FC<TaskAppTabProps> = ({
|
||||
});
|
||||
|
||||
return (
|
||||
<TaskTab active={active} to={link.href} onClick={onClick}>
|
||||
<TaskTab active={active} to={link.href ?? ""} onClick={onClick}>
|
||||
{app.icon ? <ExternalImage src={app.icon} /> : <LayoutGridIcon />}
|
||||
{link.label}
|
||||
{app.health === "unhealthy" && (
|
||||
|
||||
Reference in New Issue
Block a user