Skip to content

Commit 0b9fd43

Browse files
Fix JSON injection vulnerability in custom payload handling
- Replaced manual string escaping with `JSON.stringify` to correctly handle all special characters. - Handled quoting logic for placeholders inside and outside quotes. - Escaped `$` characters in replacement values to prevent regex replacement issues. - Added regression tests in `tests/security.test.js`. Co-authored-by: cmuench <211294+cmuench@users.noreply.github.com>
1 parent 4fb243a commit 0b9fd43

2 files changed

Lines changed: 110 additions & 3 deletions

File tree

tests/security.test.js

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,97 @@
1+
const { sendWebhook } = require('../utils/utils');
2+
3+
// Mock browser API
4+
const mockBrowser = {
5+
tabs: {
6+
query: jest.fn()
7+
},
8+
runtime: {
9+
getBrowserInfo: jest.fn(),
10+
getPlatformInfo: jest.fn()
11+
},
12+
i18n: {
13+
getMessage: jest.fn((key) => key)
14+
}
15+
};
16+
17+
global.browser = mockBrowser;
18+
global.fetch = jest.fn();
19+
global.console.error = jest.fn();
20+
21+
describe('sendWebhook Vulnerability and bugs', () => {
22+
beforeEach(() => {
23+
jest.clearAllMocks();
24+
mockBrowser.tabs.query.mockResolvedValue([{
25+
title: 'Test Page',
26+
url: 'http://example.com',
27+
id: 1,
28+
windowId: 1,
29+
index: 0,
30+
pinned: false,
31+
audible: false,
32+
incognito: false,
33+
status: 'complete'
34+
}]);
35+
mockBrowser.runtime.getBrowserInfo.mockResolvedValue({ name: 'Firefox' });
36+
mockBrowser.runtime.getPlatformInfo.mockResolvedValue({ os: 'linux' });
37+
global.fetch.mockResolvedValue({ ok: true });
38+
});
39+
40+
test('should correctly handle backslash in payload (fix JSON syntax error)', async () => {
41+
const webhook = {
42+
url: 'http://localhost/hook',
43+
customPayload: '{ "title": "{{tab.title}}" }',
44+
method: 'POST'
45+
};
46+
47+
mockBrowser.tabs.query.mockResolvedValue([{
48+
title: '\\',
49+
url: 'http://example.com'
50+
}]);
51+
52+
await sendWebhook(webhook, false);
53+
54+
const callArgs = global.fetch.mock.calls[0];
55+
const body = JSON.parse(callArgs[1].body);
56+
expect(body.title).toBe('\\');
57+
});
58+
59+
test('should correctly handle escaped quote in payload', async () => {
60+
const webhook = {
61+
url: 'http://localhost/hook',
62+
customPayload: '{ "title": "{{tab.title}}" }',
63+
method: 'POST'
64+
};
65+
66+
mockBrowser.tabs.query.mockResolvedValue([{
67+
title: '\\"',
68+
url: 'http://example.com'
69+
}]);
70+
71+
await sendWebhook(webhook, false);
72+
73+
const callArgs = global.fetch.mock.calls[0];
74+
const body = JSON.parse(callArgs[1].body);
75+
expect(body.title).toBe('\\"');
76+
});
77+
78+
test('should correctly handle dollar signs in payload', async () => {
79+
const webhook = {
80+
url: 'http://localhost/hook',
81+
customPayload: '{ "title": "{{tab.title}}" }',
82+
method: 'POST'
83+
};
84+
85+
// '$&' is a special replacement pattern (inserts matched string)
86+
mockBrowser.tabs.query.mockResolvedValue([{
87+
title: '$&',
88+
url: 'http://example.com'
89+
}]);
90+
91+
await sendWebhook(webhook, false);
92+
93+
const callArgs = global.fetch.mock.calls[0];
94+
const body = JSON.parse(callArgs[1].body);
95+
expect(body.title).toBe('$&');
96+
});
97+
});

utils/utils.js

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -158,9 +158,19 @@ async function sendWebhook(webhook, isTest = false) {
158158
Object.entries(replacements).forEach(([placeholder, value]) => {
159159
const isPlaceholderInQuotes = customPayloadStr.match(new RegExp(`"[^"]*${placeholder.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}[^"]*"`));
160160

161-
const replaceValue = typeof value === 'string'
162-
? (isPlaceholderInQuotes ? value.replace(/"/g, '\\"') : `"${value.replace(/"/g, '\\"')}"`)
163-
: (value === undefined ? 'null' : JSON.stringify(value));
161+
let replaceValue;
162+
if (typeof value === 'string') {
163+
replaceValue = JSON.stringify(value);
164+
if (isPlaceholderInQuotes) {
165+
// Remove the surrounding quotes added by JSON.stringify
166+
replaceValue = replaceValue.slice(1, -1);
167+
}
168+
} else {
169+
replaceValue = value === undefined ? 'null' : JSON.stringify(value);
170+
}
171+
172+
// Escape special replacement patterns ($) to prevent them from being interpreted by String.prototype.replace
173+
replaceValue = replaceValue.replace(/\$/g, '$$$$');
164174

165175
customPayloadStr = customPayloadStr.replace(
166176
new RegExp(placeholder.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'), 'g'),

0 commit comments

Comments
 (0)