Skip to content

Commit 1dcca25

Browse files
sestinjsestinj
authored
Merge pull request #7293 from continuedev/tomasz/prevent-zero-click-image-rendering
Prevent zero click image rendering
2 parents 775fda9 + 6a0594c commit 1dcca25

File tree

3 files changed

+397
-0
lines changed

3 files changed

+397
-0
lines changed

‎gui/src/components/StyledMarkdownPreview/SecureImageComponent.test.tsx‎

Lines changed: 218 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,218 @@
1+
import { fireEvent, render, screen, waitFor } from "@testing-library/react";
2+
import { beforeEach, describe, expect, it, vi } from "vitest";
3+
import { SecureImageComponent } from "./SecureImageComponent";
4+
5+
describe("SecureImageComponent", () => {
6+
beforeEach(() => {
7+
vi.clearAllMocks();
8+
});
9+
10+
describe("Default blocking behavior", () => {
11+
it("should block images by default and show warning message", () => {
12+
render(<SecureImageComponent src="https://example.com/image.jpg" />);
13+
14+
expect(
15+
screen.getByText(
16+
/Image blocked for security.*External images can leak data/,
17+
),
18+
).toBeInTheDocument();
19+
expect(screen.getByText("Load Image")).toBeInTheDocument();
20+
expect(screen.queryByRole("img")).not.toBeInTheDocument();
21+
});
22+
23+
it("should display the image URL", () => {
24+
const testUrl = "https://example.com/test-image.png";
25+
render(<SecureImageComponent src={testUrl} />);
26+
27+
expect(screen.getByText(testUrl)).toBeInTheDocument();
28+
});
29+
30+
it("should handle invalid src prop", () => {
31+
render(<SecureImageComponent src={undefined} />);
32+
33+
expect(
34+
screen.getByText("[Invalid image: no source]"),
35+
).toBeInTheDocument();
36+
expect(screen.queryByText("Load Image")).not.toBeInTheDocument();
37+
});
38+
});
39+
40+
describe("Query parameter detection", () => {
41+
it("should detect and display query parameters", () => {
42+
render(
43+
<SecureImageComponent src="https://example.com/image.jpg?user=123&token=abc" />,
44+
);
45+
46+
expect(
47+
screen.getByText(/Warning: URL contains query parameters/),
48+
).toBeInTheDocument();
49+
expect(screen.getByText(/"user": "123"/)).toBeInTheDocument();
50+
expect(screen.getByText(/"token": "abc"/)).toBeInTheDocument();
51+
});
52+
53+
it("should not show query parameter warning for URLs without parameters", () => {
54+
render(<SecureImageComponent src="https://example.com/image.jpg" />);
55+
56+
expect(
57+
screen.queryByText(/Warning: URL contains query parameters/),
58+
).not.toBeInTheDocument();
59+
});
60+
61+
it("should handle relative URLs with query parameters", () => {
62+
render(
63+
<SecureImageComponent src="/images/test.jpg?id=456&session=xyz" />,
64+
);
65+
66+
expect(
67+
screen.getByText(/Warning: URL contains query parameters/),
68+
).toBeInTheDocument();
69+
expect(screen.getByText(/"id": "456"/)).toBeInTheDocument();
70+
expect(screen.getByText(/"session": "xyz"/)).toBeInTheDocument();
71+
});
72+
73+
it("should handle malformed URLs gracefully", () => {
74+
render(<SecureImageComponent src="not-a-valid-url://image" />);
75+
76+
// Should still display the URL even if it can't be parsed
77+
expect(screen.getByText("not-a-valid-url://image")).toBeInTheDocument();
78+
expect(screen.getByText("Load Image")).toBeInTheDocument();
79+
});
80+
});
81+
82+
describe("User interaction", () => {
83+
it("should show image when Load Image button is clicked", () => {
84+
const testUrl = "https://example.com/image.jpg";
85+
render(<SecureImageComponent src={testUrl} alt="Test image" />);
86+
87+
// Initially no image
88+
expect(screen.queryByRole("img")).not.toBeInTheDocument();
89+
90+
// Click load button
91+
const loadButton = screen.getByText("Load Image");
92+
fireEvent.click(loadButton);
93+
94+
// Image should now be displayed (query by tag since it might be role="presentation")
95+
const image = screen.getByAltText("Test image");
96+
expect(image).toBeInTheDocument();
97+
expect(image).toHaveAttribute("src", testUrl);
98+
expect(image).toHaveAttribute("alt", "Test image");
99+
100+
// Warning message should be gone
101+
expect(
102+
screen.queryByText(
103+
/Image blocked for security.*External images can leak data/,
104+
),
105+
).not.toBeInTheDocument();
106+
});
107+
108+
it("should handle image load errors", async () => {
109+
const testUrl = "https://example.com/broken-image.jpg";
110+
render(<SecureImageComponent src={testUrl} alt="broken image" />);
111+
112+
// Click load button
113+
const loadButton = screen.getByText("Load Image");
114+
fireEvent.click(loadButton);
115+
116+
// Simulate image error (query by alt text since role might be presentation)
117+
const image = screen.getByAltText("broken image");
118+
fireEvent.error(image);
119+
120+
// Should show error message and hide image
121+
await waitFor(() => {
122+
expect(screen.getByText(/Failed to load image/)).toBeInTheDocument();
123+
expect(screen.queryByAltText("broken image")).not.toBeInTheDocument();
124+
});
125+
126+
// Load button should be available again
127+
expect(screen.getByText("Load Image")).toBeInTheDocument();
128+
});
129+
130+
it("should pass through title and className props", () => {
131+
render(
132+
<SecureImageComponent
133+
src="https://example.com/image.jpg"
134+
alt="test image"
135+
title="Image title"
136+
className="custom-class"
137+
/>,
138+
);
139+
140+
// Click load button
141+
fireEvent.click(screen.getByText("Load Image"));
142+
143+
// Check image has title (query by alt text)
144+
const image = screen.getByAltText("test image");
145+
expect(image).toHaveAttribute("title", "Image title");
146+
147+
// Check container has className
148+
const container = image.parentElement;
149+
expect(container).toHaveClass("custom-class");
150+
});
151+
});
152+
153+
describe("Security features", () => {
154+
it("should display query parameters as JSON for transparency", () => {
155+
render(
156+
<SecureImageComponent src="https://malicious.com/[email protected]&id=12345&action=view" />,
157+
);
158+
159+
// Should show all parameters clearly
160+
expect(
161+
screen.getByText(/Warning: URL contains query parameters/),
162+
).toBeInTheDocument();
163+
164+
// Check JSON is properly formatted
165+
const preElement = screen.getByText(/"email": "user@example.com"/);
166+
expect(preElement).toBeInTheDocument();
167+
expect(screen.getByText(/"id": "12345"/)).toBeInTheDocument();
168+
expect(screen.getByText(/"action": "view"/)).toBeInTheDocument();
169+
});
170+
171+
it("should handle encoded query parameters", () => {
172+
render(
173+
<SecureImageComponent src="https://example.com/img.png?data=%7B%22user%22%3A%22test%22%7D" />,
174+
);
175+
176+
// Should decode and display the parameter
177+
expect(
178+
screen.getByText(/Warning: URL contains query parameters/),
179+
).toBeInTheDocument();
180+
// The decoded value should be shown in the pre element
181+
const preElement = document.querySelector("pre");
182+
expect(preElement).toBeTruthy();
183+
// Check that the JSON contains the decoded data
184+
expect(preElement?.textContent).toContain('"data"');
185+
// The value is decoded as a string containing JSON
186+
expect(preElement?.textContent).toContain('"{');
187+
expect(preElement?.textContent).toContain("user");
188+
expect(preElement?.textContent).toContain("test");
189+
});
190+
});
191+
192+
describe("Alt text handling", () => {
193+
it("should use empty string for alt when not provided", () => {
194+
render(<SecureImageComponent src="https://example.com/image.jpg" />);
195+
196+
fireEvent.click(screen.getByText("Load Image"));
197+
198+
// Query by tag name since empty alt makes it role="presentation"
199+
const images = document.querySelectorAll("img");
200+
expect(images.length).toBe(1);
201+
expect(images[0]).toHaveAttribute("alt", "");
202+
});
203+
204+
it("should use provided alt text", () => {
205+
render(
206+
<SecureImageComponent
207+
src="https://example.com/image.jpg"
208+
alt="Description of image"
209+
/>,
210+
);
211+
212+
fireEvent.click(screen.getByText("Load Image"));
213+
214+
const image = screen.getByAltText("Description of image");
215+
expect(image).toHaveAttribute("alt", "Description of image");
216+
});
217+
});
218+
});

‎gui/src/components/StyledMarkdownPreview/SecureImageComponent.tsx‎

Lines changed: 168 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,168 @@
1+
import React, { useState } from "react";
2+
import styled from "styled-components";
3+
import {
4+
lightGray,
5+
vscBackground,
6+
vscButtonBackground,
7+
vscButtonForeground,
8+
vscForeground,
9+
vscInputBorder,
10+
} from "../";
11+
12+
const ImagePlaceholder = styled.div`
13+
border: 1px solid ${vscInputBorder};
14+
border-radius: 4px;
15+
padding: 12px;
16+
margin: 8px 0;
17+
background-color: ${vscBackground};
18+
display: inline-block;
19+
max-width: 100%;
20+
`;
21+
22+
const WarningText = styled.div`
23+
color: ${lightGray};
24+
font-size: 12px;
25+
margin-bottom: 8px;
26+
`;
27+
28+
const UrlDisplay = styled.div`
29+
font-family: var(--vscode-editor-font-family);
30+
font-size: 12px;
31+
color: ${vscForeground};
32+
word-break: break-all;
33+
margin: 8px 0;
34+
padding: 8px;
35+
background-color: rgba(0, 0, 0, 0.1);
36+
border-radius: 3px;
37+
`;
38+
39+
const QueryParamsDisplay = styled.div`
40+
font-family: var(--vscode-editor-font-family);
41+
font-size: 11px;
42+
color: ${vscForeground};
43+
margin: 8px 0;
44+
padding: 8px;
45+
background-color: rgba(128, 128, 128, 0.1);
46+
border-radius: 3px;
47+
border: 1px solid ${lightGray};
48+
`;
49+
50+
const LoadButton = styled.button`
51+
background-color: ${vscButtonBackground};
52+
color: ${vscButtonForeground};
53+
border: 1px solid ${vscInputBorder};
54+
padding: 6px 12px;
55+
border-radius: 3px;
56+
cursor: pointer;
57+
font-size: 12px;
58+
margin-top: 8px;
59+
60+
&:hover {
61+
opacity: 0.9;
62+
}
63+
64+
&:active {
65+
transform: translateY(1px);
66+
}
67+
`;
68+
69+
const ImageContainer = styled.div`
70+
max-width: 100%;
71+
display: inline-block;
72+
73+
img {
74+
max-width: 100%;
75+
height: auto;
76+
display: block;
77+
}
78+
`;
79+
80+
interface SecureImageComponentProps {
81+
src?: string;
82+
alt?: string;
83+
title?: string;
84+
className?: string;
85+
}
86+
87+
export const SecureImageComponent: React.FC<SecureImageComponentProps> = ({
88+
src,
89+
alt,
90+
title,
91+
className,
92+
}) => {
93+
const [showImage, setShowImage] = useState(false);
94+
const [imageError, setImageError] = useState(false);
95+
96+
if (!src) {
97+
return <span>[Invalid image: no source]</span>;
98+
}
99+
100+
// Parse URL to check for query parameters
101+
let queryParams: Record<string, string> = {};
102+
let hasQueryParams = false;
103+
104+
try {
105+
const url = new URL(src, window.location.href);
106+
const params = new URLSearchParams(url.search);
107+
params.forEach((value, key) => {
108+
queryParams[key] = value;
109+
hasQueryParams = true;
110+
});
111+
} catch (e) {
112+
// If URL parsing fails, treat src as a relative path
113+
const queryIndex = src.indexOf("?");
114+
if (queryIndex > -1) {
115+
hasQueryParams = true;
116+
const params = new URLSearchParams(src.substring(queryIndex));
117+
params.forEach((value, key) => {
118+
queryParams[key] = value;
119+
});
120+
}
121+
}
122+
123+
if (showImage && !imageError) {
124+
return (
125+
<ImageContainer className={className}>
126+
<img
127+
src={src}
128+
alt={alt || ""}
129+
title={title}
130+
onError={() => {
131+
setImageError(true);
132+
setShowImage(false);
133+
}}
134+
/>
135+
</ImageContainer>
136+
);
137+
}
138+
139+
return (
140+
<ImagePlaceholder>
141+
<WarningText>
142+
Image blocked for security. External images can leak data through URL
143+
parameters. Click to load if you trust the source.
144+
</WarningText>
145+
146+
<UrlDisplay>
147+
<strong>URL:</strong> {src}
148+
</UrlDisplay>
149+
150+
{hasQueryParams && (
151+
<QueryParamsDisplay>
152+
<strong>Warning: URL contains query parameters:</strong>
153+
<pre style={{ margin: "4px 0", fontSize: "11px" }}>
154+
{JSON.stringify(queryParams, null, 2)}
155+
</pre>
156+
</QueryParamsDisplay>
157+
)}
158+
159+
{imageError && (
160+
<div style={{ color: lightGray, fontSize: "12px", marginTop: "8px" }}>
161+
Failed to load image. The URL may be invalid or inaccessible.
162+
</div>
163+
)}
164+
165+
<LoadButton onClick={() => setShowImage(true)}>Load Image</LoadButton>
166+
</ImagePlaceholder>
167+
);
168+
};

0 commit comments

Comments
 (0)