Skip to content

Commit a141580

Browse files
mikaelbrmikaelbr
committed
patch: fixes security issue with non-escaping inputs [GHSL-2020-373]
1 parent 5d62799 commit a141580

File tree

3 files changed

+67
-55
lines changed

3 files changed

+67
-55
lines changed

‎lib/utils.js

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -298,9 +298,9 @@ module.exports.constructArgumentList = function (options, extra) {
298298
var keepNewlines = !!extra.keepNewlines;
299299
var wrapper = extra.wrapper === undefined ? '"' : extra.wrapper;
300300

301-
var escapeFn = function (arg) {
301+
var escapeFn = function escapeFn(arg) {
302302
if (isArray(arg)) {
303-
return removeNewLines(arg.join(','));
303+
return removeNewLines(arg.map(escapeFn).join(','));
304304
}
305305

306306
if (!noEscape) {
@@ -313,9 +313,7 @@ module.exports.constructArgumentList = function (options, extra) {
313313
};
314314

315315
initial.forEach(function (val) {
316-
if (typeof val === 'string') {
317-
args.push(escapeFn(val));
318-
}
316+
args.push(escapeFn(val));
319317
});
320318
for (var key in options) {
321319
if (

‎test/notify-send.js

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -70,13 +70,27 @@ describe('notify-send', function () {
7070
notifier.notify({ message: 'some\n "me\'ss`age`"' });
7171
});
7272

73-
it('should only include strings as arguments', function (done) {
74-
var expected = ['"HACKED"', '--expire-time', '"10000"'];
73+
it('should escape array items as normal items', function (done) {
74+
var expected = [
75+
'"Hacked"',
76+
'"\\`touch HACKED\\`"',
77+
'--app-name',
78+
'"foo\\`touch exploit\\`"',
79+
'--category',
80+
'"foo\\`touch exploit\\`"',
81+
'--expire-time',
82+
'"10000"'
83+
];
7584

7685
expectArgsListToBe(expected, done);
7786
var notifier = new Notify({ suppressOsdCheck: true });
7887
var options = JSON.parse(
79-
'{"title":"HACKED", "message":["`touch HACKED`"]}'
88+
`{
89+
"title": "Hacked",
90+
"message":["\`touch HACKED\`"],
91+
"app-name": ["foo\`touch exploit\`"],
92+
"category": ["foo\`touch exploit\`"]
93+
}`
8094
);
8195
notifier.notify(options);
8296
});

‎test/terminal-notifier.js

Lines changed: 47 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -11,104 +11,104 @@ var originalUtils = utils.fileCommandJson;
1111
var originalMacVersion = utils.isMountainLion;
1212
var originalType = os.type;
1313

14-
describe('Mac fallback', function() {
14+
describe('Mac fallback', function () {
1515
var original = utils.isMountainLion;
1616
var originalMac = utils.isMac;
1717

18-
afterEach(function() {
18+
afterEach(function () {
1919
utils.isMountainLion = original;
2020
utils.isMac = originalMac;
2121
});
2222

23-
it('should default to Growl notification if older Mac OSX than 10.8', function(done) {
24-
utils.isMountainLion = function() {
23+
it('should default to Growl notification if older Mac OSX than 10.8', function (done) {
24+
utils.isMountainLion = function () {
2525
return false;
2626
};
27-
utils.isMac = function() {
27+
utils.isMac = function () {
2828
return true;
2929
};
3030
var n = new NotificationCenter({ withFallback: true });
31-
n.notify({ message: 'Hello World' }, function(_, response) {
31+
n.notify({ message: 'Hello World' }, function (_, response) {
3232
expect(this).toBeInstanceOf(Growl);
3333
done();
3434
});
3535
});
3636

37-
it('should not fallback to Growl notification if withFallback is false', function(done) {
38-
utils.isMountainLion = function() {
37+
it('should not fallback to Growl notification if withFallback is false', function (done) {
38+
utils.isMountainLion = function () {
3939
return false;
4040
};
41-
utils.isMac = function() {
41+
utils.isMac = function () {
4242
return true;
4343
};
4444
var n = new NotificationCenter();
45-
n.notify({ message: 'Hello World' }, function(err, response) {
45+
n.notify({ message: 'Hello World' }, function (err, response) {
4646
expect(err).toBeTruthy();
4747
expect(this).not.toBeInstanceOf(Growl);
4848
done();
4949
});
5050
});
5151
});
5252

53-
describe('terminal-notifier', function() {
54-
beforeEach(function() {
55-
os.type = function() {
53+
describe('terminal-notifier', function () {
54+
beforeEach(function () {
55+
os.type = function () {
5656
return 'Darwin';
5757
};
5858

59-
utils.isMountainLion = function() {
59+
utils.isMountainLion = function () {
6060
return true;
6161
};
6262
});
6363

64-
beforeEach(function() {
64+
beforeEach(function () {
6565
notifier = new NotificationCenter();
6666
});
6767

68-
afterEach(function() {
68+
afterEach(function () {
6969
os.type = originalType;
7070
utils.isMountainLion = originalMacVersion;
7171
});
7272

7373
// Simulate async operation, move to end of message queue.
7474
function asyncify(fn) {
75-
return function() {
75+
return function () {
7676
var args = arguments;
77-
setTimeout(function() {
77+
setTimeout(function () {
7878
fn.apply(null, args);
7979
}, 0);
8080
};
8181
}
8282

83-
describe('#notify()', function() {
84-
beforeEach(function() {
85-
utils.fileCommandJson = asyncify(function(n, o, cb) {
83+
describe('#notify()', function () {
84+
beforeEach(function () {
85+
utils.fileCommandJson = asyncify(function (n, o, cb) {
8686
cb(null, '');
8787
});
8888
});
8989

90-
afterEach(function() {
90+
afterEach(function () {
9191
utils.fileCommandJson = originalUtils;
9292
});
9393

94-
it('should notify with a message', function(done) {
95-
notifier.notify({ message: 'Hello World' }, function(err, response) {
94+
it('should notify with a message', function (done) {
95+
notifier.notify({ message: 'Hello World' }, function (err, response) {
9696
expect(err).toBeNull();
9797
done();
9898
});
9999
});
100100

101-
it('should be chainable', function(done) {
101+
it('should be chainable', function (done) {
102102
notifier
103103
.notify({ message: 'First test' })
104-
.notify({ message: 'Second test' }, function(err, response) {
104+
.notify({ message: 'Second test' }, function (err, response) {
105105
expect(err).toBeNull();
106106
done();
107107
});
108108
});
109109

110-
it('should be able to list all notifications', function(done) {
111-
utils.fileCommandJson = asyncify(function(n, o, cb) {
110+
it('should be able to list all notifications', function (done) {
111+
utils.fileCommandJson = asyncify(function (n, o, cb) {
112112
cb(
113113
null,
114114
fs
@@ -117,14 +117,14 @@ describe('terminal-notifier', function() {
117117
);
118118
});
119119

120-
notifier.notify({ list: 'ALL' }, function(_, response) {
120+
notifier.notify({ list: 'ALL' }, function (_, response) {
121121
expect(response).toBeTruthy();
122122
done();
123123
});
124124
});
125125

126-
it('should be able to remove all messages', function(done) {
127-
utils.fileCommandJson = asyncify(function(n, o, cb) {
126+
it('should be able to remove all messages', function (done) {
127+
utils.fileCommandJson = asyncify(function (n, o, cb) {
128128
cb(
129129
null,
130130
fs
@@ -133,39 +133,39 @@ describe('terminal-notifier', function() {
133133
);
134134
});
135135

136-
notifier.notify({ remove: 'ALL' }, function(_, response) {
136+
notifier.notify({ remove: 'ALL' }, function (_, response) {
137137
expect(response).toBeTruthy();
138138

139-
utils.fileCommandJson = asyncify(function(n, o, cb) {
139+
utils.fileCommandJson = asyncify(function (n, o, cb) {
140140
cb(null, '');
141141
});
142142

143-
notifier.notify({ list: 'ALL' }, function(_, response) {
143+
notifier.notify({ list: 'ALL' }, function (_, response) {
144144
expect(response).toBeFalsy();
145145
done();
146146
});
147147
});
148148
});
149149
});
150150

151-
describe('arguments', function() {
152-
beforeEach(function() {
151+
describe('arguments', function () {
152+
beforeEach(function () {
153153
this.original = utils.fileCommandJson;
154154
});
155155

156-
afterEach(function() {
156+
afterEach(function () {
157157
utils.fileCommandJson = this.original;
158158
});
159159

160160
function expectArgsListToBe(expected, done) {
161-
utils.fileCommandJson = asyncify(function(notifier, argsList, callback) {
161+
utils.fileCommandJson = asyncify(function (notifier, argsList, callback) {
162162
expect(argsList).toEqual(expected);
163163
callback();
164164
done();
165165
});
166166
}
167167

168-
it('should allow for non-sensical arguments (fail gracefully)', function(done) {
168+
it('should allow for non-sensical arguments (fail gracefully)', function (done) {
169169
var expected = [
170170
'-title',
171171
'"title"',
@@ -191,8 +191,8 @@ describe('terminal-notifier', function() {
191191
});
192192
});
193193

194-
it('should validate and transform sound to default sound if Windows sound is selected', function(done) {
195-
utils.fileCommandJson = asyncify(function(notifier, argsList, callback) {
194+
it('should validate and transform sound to default sound if Windows sound is selected', function (done) {
195+
utils.fileCommandJson = asyncify(function (notifier, argsList, callback) {
196196
expect(testUtils.getOptionValue(argsList, '-title')).toBe('"Heya"');
197197
expect(testUtils.getOptionValue(argsList, '-sound')).toBe('"Bottle"');
198198
callback();
@@ -206,14 +206,14 @@ describe('terminal-notifier', function() {
206206
});
207207
});
208208

209-
it('should convert list of actions to flat list', function(done) {
209+
it('should convert list of actions to flat list', function (done) {
210210
var expected = [
211211
'-title',
212212
'"title \\"message\\""',
213213
'-message',
214214
'"body \\"message\\""',
215215
'-actions',
216-
'foo,bar,baz "foo" bar',
216+
'"foo","bar","baz \\"foo\\" bar"',
217217
'-timeout',
218218
'"10"',
219219
'-json',
@@ -232,7 +232,7 @@ describe('terminal-notifier', function() {
232232
});
233233
});
234234

235-
it('should still support wait flag with default timeout', function(done) {
235+
it('should still support wait flag with default timeout', function (done) {
236236
var expected = [
237237
'-title',
238238
'"Title"',
@@ -252,7 +252,7 @@ describe('terminal-notifier', function() {
252252
notifier.notify({ title: 'Title', message: 'Message', wait: true });
253253
});
254254

255-
it('should let timeout set precedence over wait', function(done) {
255+
it('should let timeout set precedence over wait', function (done) {
256256
var expected = [
257257
'-title',
258258
'"Title"',
@@ -277,7 +277,7 @@ describe('terminal-notifier', function() {
277277
});
278278
});
279279

280-
it('should not set a default timeout if explicitly false', function(done) {
280+
it('should not set a default timeout if explicitly false', function (done) {
281281
var expected = [
282282
'-title',
283283
'"Title"',
@@ -299,7 +299,7 @@ describe('terminal-notifier', function() {
299299
});
300300
});
301301

302-
it('should escape all title and message', function(done) {
302+
it('should escape all title and message', function (done) {
303303
var expected = [
304304
'-title',
305305
'"title \\"message\\""',

0 commit comments

Comments
 (0)