Skip to content

Commit 8c616ad

Browse files
alexluck-siftalexluck-sift
authored
Fix: add noopener and noreferring to window.open (#36)
1 parent c49083e commit 8c616ad

7 files changed

Lines changed: 556 additions & 23 deletions

‎src/components/sharelink/OpenInSiftButton.test.tsx‎

Lines changed: 55 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ describe('OpenInSiftButton', () => {
6464
const button = screen.getByRole('button', { name: 'Open in Sift' });
6565
fireEvent.click(button);
6666

67-
expect(openSpy).toHaveBeenCalledWith('https://sift.example.com/explorer', '_blank');
67+
expect(openSpy).toHaveBeenCalledWith('https://sift.example.com/explorer', '_blank', 'noopener,noreferrer');
6868
openSpy.mockRestore();
6969
});
7070

@@ -96,7 +96,7 @@ describe('OpenInSiftButton', () => {
9696
const button = screen.getByRole('button', { name: 'Open in Sift' });
9797
fireEvent.click(button);
9898

99-
expect(openSpy).toHaveBeenCalledWith('https://sift.example.com/explorer', '_blank');
99+
expect(openSpy).toHaveBeenCalledWith('https://sift.example.com/explorer', '_blank', 'noopener,noreferrer');
100100
openSpy.mockRestore();
101101
});
102102

@@ -148,7 +148,7 @@ describe('OpenInSiftButton', () => {
148148
const button = screen.getByRole('button', { name: 'Open in Sift' });
149149
fireEvent.click(button);
150150

151-
expect(openSpy).toHaveBeenCalledWith('https://sift.example.com/explorer', '_blank');
151+
expect(openSpy).toHaveBeenCalledWith('https://sift.example.com/explorer', '_blank', 'noopener,noreferrer');
152152
openSpy.mockRestore();
153153
});
154154

@@ -210,4 +210,56 @@ describe('OpenInSiftButton', () => {
210210
expect(screen.getByRole('menuitem', { name: /Open Link/i })).toBeDisabled();
211211
});
212212
});
213+
214+
it('handles errors from generateLinkFromQuery gracefully', async () => {
215+
const items = {
216+
channelIds: ['channel-1'],
217+
assetIds: ['asset-1'],
218+
runIds: ['run-1'],
219+
calculatedChannels: [],
220+
};
221+
222+
generateLinkFromQueryMock.mockImplementation(() => {
223+
throw new Error('Test error');
224+
});
225+
226+
render(<OpenInSiftButton items={items} apiBaseUrl="https://api.sift.dev" />);
227+
228+
expect(generateLinkFromQueryMock).toHaveBeenCalled();
229+
expect(errorSpy).toHaveBeenCalledWith('Failed to generate share link:', expect.any(Error));
230+
231+
const menuButton = screen.getByRole('button', { name: 'Open in Sift' });
232+
expect(menuButton).toHaveAttribute('aria-disabled', 'true');
233+
234+
fireEvent.contextMenu(menuButton);
235+
236+
await waitFor(() => {
237+
expect(screen.getByRole('menuitem', { name: /Open Link/i })).toBeDisabled();
238+
});
239+
});
240+
241+
it('handles null hostname from getFrontendHostnameDefaults', async () => {
242+
const items = {
243+
channelIds: ['channel-1'],
244+
assetIds: ['asset-1'],
245+
runIds: ['run-1'],
246+
calculatedChannels: [],
247+
};
248+
249+
getFrontendHostnameDefaultsMock.mockReturnValue(null);
250+
251+
render(<OpenInSiftButton items={items} apiBaseUrl="https://unknown-api.example.com" />);
252+
253+
expect(getFrontendHostnameDefaultsMock).toHaveBeenCalledWith('https://unknown-api.example.com');
254+
expect(generateLinkFromQueryMock).not.toHaveBeenCalled();
255+
256+
const menuButton = screen.getByRole('button', { name: 'Open in Sift' });
257+
expect(menuButton).toHaveAttribute('aria-disabled', 'true');
258+
259+
fireEvent.contextMenu(menuButton);
260+
261+
await waitFor(() => {
262+
expect(screen.getByRole('menuitem', { name: /Open Link/i })).toBeDisabled();
263+
});
264+
});
213265
});

‎src/components/sharelink/OpenInSiftButton.tsx‎

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ interface SharelinkMenuItemProps {
1616
}
1717

1818
function openLink(link: string) {
19-
window.open(link, '_blank');
19+
window.open(link, '_blank', 'noopener,noreferrer');
2020
}
2121

2222
async function copyToClipboard(value: string) {
@@ -63,10 +63,19 @@ export const OpenInSiftButton = ({ className, items, apiBaseUrl, frontendUrl, ti
6363
disabledReason: 'Configure the Sift API REST URL to enable share links',
6464
};
6565
}
66-
return {
67-
shareLink: generateLinkFromQuery(hostname, items, timeRange),
68-
disabledReason: undefined,
69-
};
66+
67+
try {
68+
return {
69+
shareLink: generateLinkFromQuery(hostname, items, timeRange),
70+
disabledReason: undefined,
71+
};
72+
} catch (error) {
73+
console.error('Failed to generate share link:', error);
74+
return {
75+
shareLink: null,
76+
disabledReason: 'Failed to generate share link',
77+
};
78+
}
7079
}, [apiBaseUrl, frontendUrl, items, timeRange]);
7180

7281
return (

‎src/components/sharelink/generateLinkFromQuery.test.ts‎

Lines changed: 250 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,250 @@
1+
import { generateLinkFromQuery } from './generateLinkFromQuery';
2+
import type { SharelinkItems, SharelinkTimeRange } from '../../types';
3+
4+
describe('generateLinkFromQuery', () => {
5+
const mockItems: SharelinkItems = {
6+
channelIds: ['channel-1', 'channel-2'],
7+
assetIds: ['asset-1'],
8+
runIds: ['run-1'],
9+
calculatedChannels: [],
10+
};
11+
12+
describe('URL construction with hostname normalization', () => {
13+
it('should handle hostname without protocol and prepend https://', () => {
14+
const result = generateLinkFromQuery('app.siftstack.com', mockItems);
15+
16+
expect(result).toMatch(/^https:\/\/app\.siftstack\.com\/explorer#/);
17+
});
18+
19+
it('should handle hostname with https:// protocol', () => {
20+
const result = generateLinkFromQuery('https://app.siftstack.com', mockItems);
21+
22+
expect(result).toMatch(/^https:\/\/app\.siftstack\.com\/explorer#/);
23+
});
24+
25+
it('should handle hostname with http:// protocol', () => {
26+
const result = generateLinkFromQuery('http://localhost:3000', mockItems);
27+
28+
expect(result).toMatch(/^http:\/\/localhost:3000\/explorer#/);
29+
});
30+
31+
it('should handle hostname with port', () => {
32+
const result = generateLinkFromQuery('localhost:8080', mockItems);
33+
34+
expect(result).toMatch(/^https:\/\/localhost:8080\/explorer#/);
35+
});
36+
37+
it('should extract origin from full URL with path', () => {
38+
const result = generateLinkFromQuery('https://app.siftstack.com/some/path', mockItems);
39+
40+
// Should use only the origin, not the path
41+
expect(result).toMatch(/^https:\/\/app\.siftstack\.com\/explorer#/);
42+
expect(result).not.toContain('/some/path');
43+
});
44+
45+
it('should handle URL with query parameters by using only origin', () => {
46+
const result = generateLinkFromQuery('https://app.siftstack.com?foo=bar', mockItems);
47+
48+
expect(result).toMatch(/^https:\/\/app\.siftstack\.com\/explorer#/);
49+
expect(result).not.toContain('?foo=bar');
50+
});
51+
52+
it('should handle URL with trailing slash', () => {
53+
const result = generateLinkFromQuery('https://app.siftstack.com/', mockItems);
54+
55+
expect(result).toMatch(/^https:\/\/app\.siftstack\.com\/explorer#/);
56+
});
57+
});
58+
59+
describe('hash parameters encoding', () => {
60+
it('should include assets in hash', () => {
61+
const items: SharelinkItems = {
62+
channelIds: ['channel-1'],
63+
assetIds: ['asset-1', 'asset-2'],
64+
runIds: [],
65+
calculatedChannels: [],
66+
};
67+
68+
const result = generateLinkFromQuery('app.siftstack.com', items);
69+
70+
expect(result).toContain('assets=asset-1%2Casset-2');
71+
});
72+
73+
it('should include runs in hash', () => {
74+
const items: SharelinkItems = {
75+
channelIds: ['channel-1'],
76+
assetIds: [],
77+
runIds: ['run-1', 'run-2'],
78+
calculatedChannels: [],
79+
};
80+
81+
const result = generateLinkFromQuery('app.siftstack.com', items);
82+
83+
expect(result).toContain('runs=run-1%2Crun-2');
84+
});
85+
86+
it('should base64 encode legend configuration', () => {
87+
const result = generateLinkFromQuery('app.siftstack.com', mockItems);
88+
89+
// Should contain base64 encoded legend
90+
expect(result).toContain('legend=');
91+
// Base64 strings typically contain these characters
92+
expect(result).toMatch(/legend=[A-Za-z0-9+/=%]+/);
93+
});
94+
95+
it('should include time range in legend when provided', () => {
96+
const timeRange: SharelinkTimeRange = {
97+
from: '2024-01-01T00:00:00Z',
98+
to: '2024-01-02T00:00:00Z',
99+
};
100+
101+
const result = generateLinkFromQuery('app.siftstack.com', mockItems, timeRange);
102+
103+
// The time range should be encoded in the legend parameter
104+
expect(result).toContain('legend=');
105+
});
106+
107+
it('should handle calculated channels', () => {
108+
const itemsWithCalc: SharelinkItems = {
109+
channelIds: ['channel-1'],
110+
assetIds: ['asset-1'],
111+
runIds: ['run-1'],
112+
calculatedChannels: [
113+
{
114+
name: 'Calculated Channel',
115+
sourceChannels: ['channel-1', 'channel-2'],
116+
expression: '$1 + $2',
117+
expressionDataType: 'DOUBLE',
118+
},
119+
],
120+
};
121+
122+
const result = generateLinkFromQuery('app.siftstack.com', itemsWithCalc);
123+
124+
// Should still generate a valid URL with legend
125+
expect(result).toMatch(/^https:\/\/app\.siftstack\.com\/explorer#/);
126+
expect(result).toContain('legend=');
127+
});
128+
129+
it('should handle multiple calculated channels', () => {
130+
const itemsWithMultipleCalc: SharelinkItems = {
131+
channelIds: ['channel-1'],
132+
assetIds: ['asset-1'],
133+
runIds: ['run-1'],
134+
calculatedChannels: [
135+
{
136+
name: 'Calc 1',
137+
sourceChannels: ['channel-1'],
138+
expression: '$1 * 2',
139+
expressionDataType: 'DOUBLE',
140+
},
141+
{
142+
name: 'Calc 2',
143+
sourceChannels: ['channel-1', 'channel-2'],
144+
expression: '$1 + $2',
145+
expressionDataType: 'DOUBLE',
146+
},
147+
],
148+
};
149+
150+
const result = generateLinkFromQuery('app.siftstack.com', itemsWithMultipleCalc);
151+
152+
expect(result).toMatch(/^https:\/\/app\.siftstack\.com\/explorer#/);
153+
expect(result).toContain('legend=');
154+
});
155+
});
156+
157+
describe('edge cases', () => {
158+
it('should handle empty asset and run arrays', () => {
159+
const items: SharelinkItems = {
160+
channelIds: ['channel-1'],
161+
assetIds: [],
162+
runIds: [],
163+
calculatedChannels: [],
164+
};
165+
166+
const result = generateLinkFromQuery('app.siftstack.com', items);
167+
168+
expect(result).toMatch(/^https:\/\/app\.siftstack\.com\/explorer#/);
169+
// Should not include assets or runs parameters
170+
expect(result).not.toContain('assets=');
171+
expect(result).not.toContain('runs=');
172+
});
173+
174+
it('should handle undefined asset and run arrays', () => {
175+
const items: SharelinkItems = {
176+
channelIds: ['channel-1'],
177+
assetIds: undefined,
178+
runIds: undefined,
179+
calculatedChannels: [],
180+
};
181+
182+
const result = generateLinkFromQuery('app.siftstack.com', items);
183+
184+
expect(result).toMatch(/^https:\/\/app\.siftstack\.com\/explorer#/);
185+
expect(result).not.toContain('assets=');
186+
expect(result).not.toContain('runs=');
187+
});
188+
189+
it('should handle special characters in channel IDs', () => {
190+
const items: SharelinkItems = {
191+
channelIds: ['channel-with-dash', 'channel_with_underscore'],
192+
assetIds: ['asset-1'],
193+
runIds: ['run-1'],
194+
calculatedChannels: [],
195+
};
196+
197+
const result = generateLinkFromQuery('app.siftstack.com', items);
198+
199+
expect(result).toMatch(/^https:\/\/app\.siftstack\.com\/explorer#/);
200+
expect(result).toContain('legend=');
201+
});
202+
203+
it('should handle IPv4 addresses', () => {
204+
const result = generateLinkFromQuery('192.168.1.1:8080', mockItems);
205+
206+
expect(result).toMatch(/^https:\/\/192\.168\.1\.1:8080\/explorer#/);
207+
});
208+
209+
it('should handle localhost variations', () => {
210+
const result1 = generateLinkFromQuery('localhost', mockItems);
211+
expect(result1).toMatch(/^https:\/\/localhost\/explorer#/);
212+
213+
const result2 = generateLinkFromQuery('http://localhost:3000', mockItems);
214+
expect(result2).toMatch(/^http:\/\/localhost:3000\/explorer#/);
215+
});
216+
});
217+
218+
describe('URL structure validation', () => {
219+
it('should generate valid URL structure', () => {
220+
const result = generateLinkFromQuery('app.siftstack.com', mockItems);
221+
222+
// Should be a valid URL
223+
expect(() => new URL(result)).not.toThrow();
224+
225+
const url = new URL(result);
226+
expect(url.protocol).toMatch(/^https?:$/);
227+
expect(url.pathname).toBe('/explorer');
228+
expect(url.hash).toMatch(/^#.+/);
229+
});
230+
231+
it('should properly encode hash parameters', () => {
232+
const result = generateLinkFromQuery('app.siftstack.com', mockItems);
233+
const url = new URL(result);
234+
235+
// Hash should be parseable as URLSearchParams (without the # prefix)
236+
const hashParams = new URLSearchParams(url.hash.slice(1));
237+
238+
expect(hashParams.has('legend')).toBe(true);
239+
expect(hashParams.has('assets')).toBe(true);
240+
expect(hashParams.has('runs')).toBe(true);
241+
});
242+
243+
it('should not double-encode URL components', () => {
244+
const result = generateLinkFromQuery('app.siftstack.com', mockItems);
245+
246+
// Should not contain double-encoded characters like %25 (encoded %)
247+
expect(result).not.toContain('%25');
248+
});
249+
});
250+
});

0 commit comments

Comments
 (0)