Skip to content

Commit b7d2fbf

Browse files
EhabYEhabY
committed
handle review comments
1 parent 6056f2b commit b7d2fbf

File tree

5 files changed

+53
-40
lines changed

5 files changed

+53
-40
lines changed

‎src/uri/uriHandler.ts‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,9 @@ import type { ChatPanelProvider } from "../webviews/chat/chatPanelProvider";
1313

1414
interface UriHandlerDeps {
1515
serviceContainer: ServiceContainer;
16-
deploymentManager: DeploymentManager;
17-
commands: Commands;
18-
chatPanelProvider: ChatPanelProvider;
16+
deploymentManager: Pick<DeploymentManager, "setDeployment">;
17+
commands: Pick<Commands, "open" | "openDevContainer">;
18+
chatPanelProvider: Pick<ChatPanelProvider, "openChat">;
1919
}
2020

2121
interface UriRouteContext extends UriHandlerDeps {
@@ -107,8 +107,8 @@ async function handleOpen(ctx: UriRouteContext): Promise<void> {
107107
}
108108
}
109109

110-
// When the workspace is already open VS Code refocuses without
111-
// reloading, so activate() won't consume the pending chatId.
110+
// Already-open workspace: VS Code refocuses without reloading,
111+
// so activate() won't run. openChat is idempotent if both fire.
112112
if (opened && chatId) {
113113
serviceContainer.getContextManager().set("coder.agentsEnabled", true);
114114
ctx.chatPanelProvider.openChat(chatId);
@@ -152,7 +152,7 @@ async function handleOpenDevContainer(ctx: UriRouteContext): Promise<void> {
152152
async function setupDeployment(
153153
params: URLSearchParams,
154154
serviceContainer: ServiceContainer,
155-
deploymentManager: DeploymentManager,
155+
deploymentManager: Pick<DeploymentManager, "setDeployment">,
156156
): Promise<void> {
157157
const secretsManager = serviceContainer.getSecretsManager();
158158
const mementoManager = serviceContainer.getMementoManager();

‎src/webviews/chat/chatPanelProvider.ts‎

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ export class ChatPanelProvider
2929
private authRetryTimer: ReturnType<typeof setTimeout> | undefined;
3030

3131
constructor(
32-
private readonly client: CoderApi,
32+
private readonly client: Pick<CoderApi, "getHost" | "getSessionToken">,
3333
private readonly logger: Logger,
3434
) {}
3535

@@ -63,6 +63,7 @@ export class ChatPanelProvider
6363
return;
6464
}
6565
this.chatId = chatId;
66+
// No-op if unresolved; the focus command triggers resolveWebviewView().
6667
this.refresh();
6768
void vscode.commands.executeCommand(`${ChatPanelProvider.viewType}.focus`);
6869
}
@@ -72,6 +73,9 @@ export class ChatPanelProvider
7273
_context: vscode.WebviewViewResolveContext,
7374
_token: vscode.CancellationToken,
7475
): void {
76+
// Clean up state from a previous view instance to avoid
77+
// duplicates if VS Code re-resolves the view.
78+
this.disposeView();
7579
this.view = webviewView;
7680
webviewView.webview.options = { enableScripts: true };
7781
this.disposables.push(
@@ -83,7 +87,7 @@ export class ChatPanelProvider
8387
}),
8488
);
8589
this.renderView();
86-
this.disposables.push(webviewView.onDidDispose(() => this.dispose()));
90+
this.disposables.push(webviewView.onDidDispose(() => this.disposeView()));
8791
}
8892

8993
public refresh(): void {
@@ -131,7 +135,17 @@ export class ChatPanelProvider
131135
const url = msg.payload?.url;
132136
const coderUrl = this.client.getHost();
133137
if (url && coderUrl) {
134-
void vscode.env.openExternal(vscode.Uri.parse(coderUrl + url));
138+
try {
139+
const resolved = new URL(url, coderUrl);
140+
const expected = new URL(coderUrl);
141+
if (resolved.origin === expected.origin) {
142+
void vscode.env.openExternal(
143+
vscode.Uri.parse(resolved.toString()),
144+
);
145+
}
146+
} catch {
147+
this.logger.warn(`Chat: invalid navigate URL: ${url}`);
148+
}
135149
}
136150
break;
137151
}
@@ -307,11 +321,15 @@ text-align:center;}</style></head>
307321
<body><p>No active chat session. Open a chat from the Agents tab on your Coder deployment.</p></body></html>`;
308322
}
309323

310-
dispose(): void {
324+
private disposeView(): void {
311325
clearTimeout(this.authRetryTimer);
312326
for (const d of this.disposables) {
313327
d.dispose();
314328
}
315329
this.disposables = [];
316330
}
331+
332+
dispose(): void {
333+
this.disposeView();
334+
}
317335
}

‎test/mocks/vscode.runtime.ts‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -91,10 +91,12 @@ const onDidChangeWorkspaceFolders = new EventEmitter<unknown>();
9191
const onDidChangeActiveColorTheme = new EventEmitter<unknown>();
9292

9393
export const window = {
94-
activeColorTheme: { kind: ColorThemeKind.Dark } as { kind: number },
94+
activeColorTheme: { kind: ColorThemeKind.Dark },
9595
onDidChangeActiveColorTheme: onDidChangeActiveColorTheme.event,
96-
__fireDidChangeActiveColorTheme: (e: unknown) =>
97-
onDidChangeActiveColorTheme.fire(e),
96+
__setActiveColorThemeKind: (kind: number) => {
97+
window.activeColorTheme = { kind };
98+
onDidChangeActiveColorTheme.fire({ kind });
99+
},
98100
showInformationMessage: vi.fn(),
99101
showWarningMessage: vi.fn(),
100102
showErrorMessage: vi.fn(),

‎test/unit/uri/uriHandler.test.ts‎

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,8 @@ import {
1515
MockContextManager,
1616
} from "../../mocks/testHelpers";
1717

18-
import type { Commands } from "@/commands";
1918
import type { ServiceContainer } from "@/core/container";
20-
import type { DeploymentManager } from "@/deployment/deploymentManager";
2119
import type { LoginCoordinator, LoginOptions } from "@/login/loginCoordinator";
22-
import type { ChatPanelProvider } from "@/webviews/chat/chatPanelProvider";
2320

2421
vi.mock("@/promptUtils", () => ({ maybeAskUrl: vi.fn() }));
2522

@@ -97,14 +94,12 @@ function createTestContext() {
9794
.mocked(vscode.window.showErrorMessage)
9895
.mockResolvedValue(undefined);
9996

100-
const chatPanelProvider = {
101-
openChat: vi.fn(),
102-
} as unknown as ChatPanelProvider;
97+
const chatPanelProvider = { openChat: vi.fn() };
10398

10499
registerUriHandler({
105100
serviceContainer: container,
106-
deploymentManager: deploymentManager as unknown as DeploymentManager,
107-
commands: commands as unknown as Commands,
101+
deploymentManager,
102+
commands,
108103
chatPanelProvider,
109104
});
110105

‎test/unit/webviews/chat/chatPanelProvider.test.ts‎

Lines changed: 17 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -5,17 +5,10 @@ import { ChatPanelProvider } from "@/webviews/chat/chatPanelProvider";
55

66
import { createMockLogger, MockCoderApi } from "../../../mocks/testHelpers";
77

8-
import type { CoderApi } from "@/api/coderApi";
9-
10-
type WindowMock = typeof vscode.window & {
11-
activeColorTheme: { kind: number };
12-
__fireDidChangeActiveColorTheme: (e: unknown) => void;
8+
const windowMock = vscode.window as typeof vscode.window & {
9+
__setActiveColorThemeKind: (kind: number) => void;
1310
};
1411

15-
function setMockTheme(kind: number): void {
16-
(vscode.window as WindowMock).activeColorTheme = { kind };
17-
}
18-
1912
interface Harness {
2013
provider: ChatPanelProvider;
2114
postMessage: ReturnType<typeof vi.fn>;
@@ -27,10 +20,7 @@ function createHarness(): Harness {
2720
const client = new MockCoderApi();
2821
client.setCredentials("https://coder.example.com", "test-token");
2922

30-
const provider = new ChatPanelProvider(
31-
client as unknown as CoderApi,
32-
createMockLogger(),
33-
);
23+
const provider = new ChatPanelProvider(client, createMockLogger());
3424

3525
let handler: ((msg: unknown) => void) | null = null;
3626

@@ -89,7 +79,7 @@ function findPostedMessage(
8979
describe("ChatPanelProvider", () => {
9080
beforeEach(() => {
9181
vi.resetAllMocks();
92-
setMockTheme(vscode.ColorThemeKind.Dark);
82+
windowMock.__setActiveColorThemeKind(vscode.ColorThemeKind.Dark);
9383
});
9484

9585
describe("theme sync", () => {
@@ -99,7 +89,7 @@ describe("ChatPanelProvider", () => {
9989
[vscode.ColorThemeKind.HighContrast, "dark"],
10090
[vscode.ColorThemeKind.HighContrastLight, "light"],
10191
])("maps ColorThemeKind %i to %s on chat-ready", (kind, expected) => {
102-
setMockTheme(kind);
92+
windowMock.__setActiveColorThemeKind(kind);
10393
const { sendFromWebview, postMessage } = createHarness();
10494

10595
sendFromWebview({ type: "coder:chat-ready" });
@@ -124,10 +114,7 @@ describe("ChatPanelProvider", () => {
124114
const { postMessage } = createHarness();
125115
postMessage.mockClear();
126116

127-
setMockTheme(vscode.ColorThemeKind.Light);
128-
(vscode.window as WindowMock).__fireDidChangeActiveColorTheme({
129-
kind: vscode.ColorThemeKind.Light,
130-
});
117+
windowMock.__setActiveColorThemeKind(vscode.ColorThemeKind.Light);
131118

132119
expect(postMessage).toHaveBeenCalledWith({
133120
type: "coder:set-theme",
@@ -172,6 +159,17 @@ describe("ChatPanelProvider", () => {
172159

173160
expect(vscode.env.openExternal).not.toHaveBeenCalled();
174161
});
162+
163+
it("blocks cross-origin navigate URLs", () => {
164+
const { sendFromWebview } = createHarness();
165+
166+
sendFromWebview({
167+
type: "coder:navigate",
168+
payload: { url: "https://evil.com/steal" },
169+
});
170+
171+
expect(vscode.env.openExternal).not.toHaveBeenCalled();
172+
});
175173
});
176174

177175
describe("openChat", () => {

0 commit comments

Comments
 (0)