mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
fix(site/src): show tooltip in AppLink + WorkspacesTable when coder_app URL is invalid (#27556)
fixes DEVEX-70 ## Summary Fixes #22350. `getAppHref()` in `site/src/modules/apps/apps.ts` called `new URL(app.url)` unguarded for external apps. When a template author configures a `coder_app` with an unparseable `url` (e.g. a bare string like `"my-repo"` with no scheme), `new URL()` threw `TypeError: Failed to construct 'URL': Invalid URL` during render. Because `getAppHref` runs inside `useAppLink` (used by both `AppLink` and the workspaces table `IconAppLink`), the exception crashed the entire Workspace List and Workspace detail pages, not just the affected app button. ## Changes - `getAppHref()` no longer throws: the external-app protocol parse is wrapped in `try/catch`, so an unparseable URL falls back to the raw value instead of crashing. - Added `isExternalAppUrlInvalid(app)`, a pure predicate used by consumers to decide whether the app can be launched. - `AppLink` renders a disabled button with a warning icon and an explanatory tooltip when the URL is invalid, mirroring the existing "admin has not configured subdomain application access" pattern. The tooltip points the user at the responsible configuration: > This app has an invalid URL and can't be opened. Ask your template administrator to fix the app's `url` in the template's `coder_app` configuration. - `IconAppLink` (workspaces table) renders a non-navigating icon with an equivalent label for invalid URLs. ## Testing - Unit tests in `apps.test.ts`: `getAppHref` no longer throws for invalid URLs, plus coverage of `isExternalAppUrlInvalid`. - New `InvalidExternalAppUrl` Storybook story with a `play` function asserting the button is disabled and the tooltip explains the invalid URL. - `pnpm exec vitest run` (unit + storybook), `pnpm exec tsc --noEmit`, and `biome check` all pass. <details> <summary>Implementation plan</summary> # Plan: Handle invalid `coder_app` URLs gracefully (issue #22350) ## Problem `getAppHref()` in `site/src/modules/apps/apps.ts` calls `new URL(app.url)` unguarded for external apps. When a template author sets an external app with an unparseable `url` (e.g. a bare string like `"my-repo"` with no scheme), `new URL()` throws during render. Because `getAppHref` runs inside `useAppLink` (called during render of `AppLink` and `IconAppLink`), the exception propagates and crashes the entire Workspace List and Workspace detail pages, not just the single app button. ## Goal Never throw from `getAppHref`. Detect the invalid-URL case and let the UI render a disabled button with an explanatory tooltip, mirroring the existing `isAppBlockedByMissingWildcard` pattern. ## Approach 1. Make `getAppHref` non-throwing (defensive), so no render path can crash. 2. Add a pure predicate `isExternalAppUrlInvalid(app)` used by button components to decide whether to disable and what tooltip to show. 3. Wire the predicate into `AppLink` and `IconAppLink`. ## Changes - `apps.ts`: wrap the external protocol parse in `try/catch`; add `isExternalAppUrlInvalid`. - `AppLink.tsx`: disabled state with `text-content-warning` icon and a `ReactNode` (React Fragment) tooltip with `url` and `coder_app` wrapped in inline `<code>` elements. - `WorkspacesTable.tsx` (`IconAppLink`): render a non-navigating icon when the URL is invalid. ## Tests - `apps.test.ts`: `getAppHref` does not throw for invalid URLs; predicate coverage. - `AppLink.stories.tsx`: `InvalidExternalAppUrl` story with a `play` function asserting disabled button and tooltip. ## Out of scope Backend/template-side validation of `coder_app.url` would prevent the misconfiguration at its source; tracked by the parent epic (#22349 / DEVEX-60). </details> --- *Opened by Coder Agents on behalf of @aqandrew.*
This commit is contained in:
@@ -8,6 +8,7 @@ import {
|
||||
getAppHref,
|
||||
getVSCodeHref,
|
||||
isAppBlockedByMissingWildcard,
|
||||
isAppUrlValid,
|
||||
isWorkspaceAppEmbeddable,
|
||||
openAppInNewWindow,
|
||||
SESSION_TOKEN_PLACEHOLDER,
|
||||
@@ -191,6 +192,49 @@ describe("getAppHref", () => {
|
||||
`/path-base/@${MockWorkspace.owner_name}/test-workspace.a-workspace-agent/apps/${app.slug}/`,
|
||||
);
|
||||
});
|
||||
|
||||
it("returns the raw URL without throwing when external app has an invalid URL", () => {
|
||||
const externalApp = {
|
||||
...MockWorkspaceApp,
|
||||
external: true,
|
||||
url: "my-repo",
|
||||
};
|
||||
let href = "";
|
||||
expect(() => {
|
||||
href = getAppHref(externalApp, {
|
||||
host: "*.apps-host.tld",
|
||||
path: "/path-base",
|
||||
agent: MockWorkspaceAgent,
|
||||
workspace: MockWorkspace,
|
||||
token: "user-session-token",
|
||||
});
|
||||
}).not.toThrow();
|
||||
expect(href).toBe("my-repo");
|
||||
});
|
||||
});
|
||||
|
||||
describe("isAppUrlValid", () => {
|
||||
it("returns false for an external app with an unparsable URL", () => {
|
||||
expect(isAppUrlValid(buildApp({ external: true, url: "my-repo" }))).toBe(
|
||||
false,
|
||||
);
|
||||
});
|
||||
|
||||
it("returns true for an external app with a valid HTTP URL", () => {
|
||||
expect(
|
||||
isAppUrlValid(buildApp({ external: true, url: "https://example.com" })),
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
it("returns true for an external app with a valid custom scheme", () => {
|
||||
expect(
|
||||
isAppUrlValid(buildApp({ external: true, url: "vscode://open" })),
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
it("returns true for non-external apps", () => {
|
||||
expect(isAppUrlValid(buildApp({ external: false }))).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe("openAppInNewWindow", () => {
|
||||
|
||||
@@ -117,9 +117,16 @@ export const getAppHref = (
|
||||
{ path, token, workspace, agent, host }: GetAppHrefParams,
|
||||
): string => {
|
||||
if (isExternalApp(app)) {
|
||||
const appProtocol = new URL(app.url).protocol;
|
||||
const isAllowedProtocol =
|
||||
ALLOWED_EXTERNAL_APP_PROTOCOLS.includes(appProtocol);
|
||||
let isAllowedProtocol = false;
|
||||
try {
|
||||
isAllowedProtocol = ALLOWED_EXTERNAL_APP_PROTOCOLS.includes(
|
||||
new URL(app.url).protocol,
|
||||
);
|
||||
} catch {
|
||||
// The URL is unparsable. Leave isAllowedProtocol false and return
|
||||
// the raw URL. Consumers disable the button via
|
||||
// isAppUrlValid, so the href is never followed.
|
||||
}
|
||||
|
||||
return needsSessionToken(app) && isAllowedProtocol
|
||||
? app.url.replaceAll(SESSION_TOKEN_PLACEHOLDER, token ?? "")
|
||||
@@ -179,6 +186,19 @@ export const isWorkspaceAppEmbeddable = (app: WorkspaceApp): boolean => {
|
||||
return !app.hidden && !isExternalApp(app) && !app.command;
|
||||
};
|
||||
|
||||
/**
|
||||
* True when an app is not an external app, or is an external app whose URL can
|
||||
* be parsed by the URL constructor. External apps with an unparsable URL
|
||||
* cannot be launched. Template authors sometimes set a bare string with no
|
||||
* scheme, which would otherwise crash the page during render.
|
||||
*/
|
||||
export const isAppUrlValid = (app: WorkspaceApp): boolean => {
|
||||
if (!isExternalApp(app)) {
|
||||
return true;
|
||||
}
|
||||
return URL.canParse(app.url);
|
||||
};
|
||||
|
||||
/**
|
||||
* True when an app requires subdomain access but the deployment has no wildcard
|
||||
* access URL configured, so the app cannot be launched or embedded.
|
||||
|
||||
@@ -91,6 +91,42 @@ export const ExternalAppShareable: Story = {
|
||||
},
|
||||
};
|
||||
|
||||
export const InvalidExternalAppUrl: Story = {
|
||||
args: {
|
||||
workspace: MockWorkspace,
|
||||
app: {
|
||||
...MockWorkspaceApp,
|
||||
external: true,
|
||||
// A bare string with no scheme is unparsable by the URL constructor.
|
||||
url: "my-repo",
|
||||
},
|
||||
agent: MockWorkspaceAgent,
|
||||
},
|
||||
play: async ({ canvasElement, step }) => {
|
||||
const canvas = within(canvasElement);
|
||||
// A disabled app renders an anchor without an href, which has no
|
||||
// "link" role, so query by its label text instead.
|
||||
const trigger = await canvas.findByText("Test App");
|
||||
// The disabled button sets `pointer-events: none`, so bypass the
|
||||
// pointer-events guard to hover and reveal the tooltip.
|
||||
const user = userEvent.setup({ pointerEventsCheck: 0 });
|
||||
|
||||
await step("button is disabled", async () => {
|
||||
const anchor = trigger.closest("a");
|
||||
expect(anchor).not.toBeNull();
|
||||
expect(anchor).not.toHaveAttribute("href");
|
||||
});
|
||||
|
||||
await step("tooltip explains the invalid URL", async () => {
|
||||
await user.hover(trigger);
|
||||
const tooltip = await screen.findByRole("tooltip");
|
||||
expect(tooltip).toHaveTextContent(
|
||||
"This app has an invalid URL and can't be opened.",
|
||||
);
|
||||
});
|
||||
},
|
||||
};
|
||||
|
||||
export const SharingLevelOwner: Story = {
|
||||
args: {
|
||||
workspace: MockWorkspace,
|
||||
|
||||
@@ -20,6 +20,7 @@ import {
|
||||
import { useProxy } from "#/contexts/ProxyContext";
|
||||
import {
|
||||
isAppBlockedByMissingWildcard,
|
||||
isAppUrlValid,
|
||||
isExternalApp,
|
||||
needsSessionToken,
|
||||
} from "#/modules/apps/apps";
|
||||
@@ -114,6 +115,23 @@ export const AppLink: FC<AppLinkProps> = ({
|
||||
);
|
||||
}
|
||||
|
||||
if (!isAppUrlValid(app)) {
|
||||
canClick = false;
|
||||
icon = (
|
||||
<CircleAlertIcon
|
||||
aria-hidden="true"
|
||||
className="size-icon-sm text-content-warning"
|
||||
/>
|
||||
);
|
||||
primaryTooltip = (
|
||||
<>
|
||||
This app has an invalid URL and can't be opened. Ask your template
|
||||
administrator to fix the app's <code>url</code> in the template's{" "}
|
||||
<code>coder_app</code> configuration.
|
||||
</>
|
||||
);
|
||||
}
|
||||
|
||||
if (isExternalApp(app) && needsSessionToken(app) && !link.hasToken) {
|
||||
canClick = false;
|
||||
}
|
||||
|
||||
@@ -478,6 +478,58 @@ export const ParentAgentApps: Story = {
|
||||
},
|
||||
};
|
||||
|
||||
// An external app with an unparsable URL must not crash the table. Its icon
|
||||
// renders as a non-navigating button with an explanatory label instead of a
|
||||
// broken link.
|
||||
export const InvalidAppUrl: Story = {
|
||||
args: {
|
||||
workspaces: [
|
||||
{
|
||||
...MockWorkspace,
|
||||
name: "invalid-app-url",
|
||||
latest_build: {
|
||||
...MockWorkspace.latest_build,
|
||||
resources: [
|
||||
{
|
||||
...MockWorkspace.latest_build.resources[0],
|
||||
agents: [
|
||||
{
|
||||
...MockWorkspaceAgent,
|
||||
display_apps: [],
|
||||
apps: [
|
||||
{
|
||||
...MockWorkspaceApp,
|
||||
id: "invalid-app",
|
||||
slug: "invalid-app",
|
||||
display_name: "Broken App",
|
||||
health: "healthy",
|
||||
external: true,
|
||||
// A bare string with no scheme is unparsable
|
||||
// by the URL constructor.
|
||||
url: "my-repo",
|
||||
},
|
||||
],
|
||||
},
|
||||
],
|
||||
},
|
||||
],
|
||||
},
|
||||
},
|
||||
],
|
||||
count: allWorkspaces.length,
|
||||
},
|
||||
play: async ({ canvasElement }) => {
|
||||
const canvas = within(canvasElement);
|
||||
// The invalid app renders a non-navigating button, not a link.
|
||||
await canvas.findByRole("button", {
|
||||
name: /Broken App has an invalid URL/i,
|
||||
});
|
||||
expect(
|
||||
canvas.queryByRole("link", { name: /Broken App/i }),
|
||||
).not.toBeInTheDocument();
|
||||
},
|
||||
};
|
||||
|
||||
export const ShowOrganizations: Story = {
|
||||
args: {
|
||||
workspaces: [
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
import {
|
||||
BanIcon,
|
||||
CircleAlertIcon,
|
||||
CloudIcon,
|
||||
EllipsisVerticalIcon,
|
||||
ExternalLinkIcon,
|
||||
@@ -67,6 +68,7 @@ import { useClickableTableRow } from "#/hooks/useClickableTableRow";
|
||||
import {
|
||||
getTerminalHref,
|
||||
getVSCodeHref,
|
||||
isAppUrlValid,
|
||||
openAppInNewWindow,
|
||||
} from "#/modules/apps/apps";
|
||||
import { useAppLink } from "#/modules/apps/useAppLink";
|
||||
@@ -808,6 +810,27 @@ const IconAppLink: FC<IconAppLinkProps> = ({ app, workspace, agent }) => {
|
||||
agent,
|
||||
});
|
||||
|
||||
// A malformed external app URL can't be opened. Render a non-navigating
|
||||
// icon with an explanatory tooltip instead of a broken link.
|
||||
if (!isAppUrlValid(app)) {
|
||||
return (
|
||||
<BaseIconLink
|
||||
key={app.id}
|
||||
label={`${link.label} has an invalid URL`}
|
||||
onClick={() => {}}
|
||||
>
|
||||
{app.icon ? (
|
||||
<ExternalImage src={app.icon} />
|
||||
) : (
|
||||
<CircleAlertIcon
|
||||
aria-hidden="true"
|
||||
className="size-icon-sm text-content-warning"
|
||||
/>
|
||||
)}
|
||||
</BaseIconLink>
|
||||
);
|
||||
}
|
||||
|
||||
return (
|
||||
<BaseIconLink
|
||||
key={app.id}
|
||||
|
||||
Reference in New Issue
Block a user