Skip to content

Commit

Permalink
patch: fixes security issue with non-escaping inputs [GHSL-2020-373]
Browse files Browse the repository at this point in the history
  • Loading branch information
@mikaelbr
mikaelbr committed Mar 11, 2021
1 parent 0704106 commit 51ed238
Show file tree
Hide file tree
Showing 3 changed files with 67 additions and 55 deletions.
8 changes: 3 additions & 5 deletions lib/utils.js
View file
Expand Up @@ -298,9 +298,9 @@ module.exports.constructArgumentList = function (options, extra) {
var keepNewlines = !!extra.keepNewlines;
var wrapper = extra.wrapper === undefined ? '"' : extra.wrapper;

var escapeFn = function (arg) {
var escapeFn = function escapeFn(arg) {
if (isArray(arg)) {
return removeNewLines(arg.join(','));
return removeNewLines(arg.map(escapeFn).join(','));
}

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

initial.forEach(function (val) {
if (typeof val === 'string') {
args.push(escapeFn(val));
}
args.push(escapeFn(val));
});
for (var key in options) {
if (
Expand Down
20 changes: 17 additions & 3 deletions test/notify-send.js
View file
Expand Up @@ -70,13 +70,27 @@ describe('notify-send', function () {
notifier.notify({ message: 'some\n "me\'ss`age`"' });
});

it('should only include strings as arguments', function (done) {
var expected = ['"HACKED"', '--expire-time', '"10000"'];
it('should escape array items as normal items', function (done) {
var expected = [
'"Hacked"',
'"\\`touch HACKED\\`"',
'--app-name',
'"foo\\`touch exploit\\`"',
'--category',
'"foo\\`touch exploit\\`"',
'--expire-time',
'"10000"'
];

expectArgsListToBe(expected, done);
var notifier = new Notify({ suppressOsdCheck: true });
var options = JSON.parse(
'{"title":"HACKED", "message":["`touch HACKED`"]}'
`{
"title": "Hacked",
"message":["\`touch HACKED\`"],
"app-name": ["foo\`touch exploit\`"],
"category": ["foo\`touch exploit\`"]
}`
);
notifier.notify(options);
});
Expand Down
94 changes: 47 additions & 47 deletions test/terminal-notifier.js
View file
Expand Up @@ -11,104 +11,104 @@ var originalUtils = utils.fileCommandJson;
var originalMacVersion = utils.isMountainLion;
var originalType = os.type;

describe('Mac fallback', function() {
describe('Mac fallback', function () {
var original = utils.isMountainLion;
var originalMac = utils.isMac;

afterEach(function() {
afterEach(function () {
utils.isMountainLion = original;
utils.isMac = originalMac;
});

it('should default to Growl notification if older Mac OSX than 10.8', function(done) {
utils.isMountainLion = function() {
it('should default to Growl notification if older Mac OSX than 10.8', function (done) {
utils.isMountainLion = function () {
return false;
};
utils.isMac = function() {
utils.isMac = function () {
return true;
};
var n = new NotificationCenter({ withFallback: true });
n.notify({ message: 'Hello World' }, function(_, response) {
n.notify({ message: 'Hello World' }, function (_, response) {
expect(this).toBeInstanceOf(Growl);
done();
});
});

it('should not fallback to Growl notification if withFallback is false', function(done) {
utils.isMountainLion = function() {
it('should not fallback to Growl notification if withFallback is false', function (done) {
utils.isMountainLion = function () {
return false;
};
utils.isMac = function() {
utils.isMac = function () {
return true;
};
var n = new NotificationCenter();
n.notify({ message: 'Hello World' }, function(err, response) {
n.notify({ message: 'Hello World' }, function (err, response) {
expect(err).toBeTruthy();
expect(this).not.toBeInstanceOf(Growl);
done();
});
});
});

describe('terminal-notifier', function() {
beforeEach(function() {
os.type = function() {
describe('terminal-notifier', function () {
beforeEach(function () {
os.type = function () {
return 'Darwin';
};

utils.isMountainLion = function() {
utils.isMountainLion = function () {
return true;
};
});

beforeEach(function() {
beforeEach(function () {
notifier = new NotificationCenter();
});

afterEach(function() {
afterEach(function () {
os.type = originalType;
utils.isMountainLion = originalMacVersion;
});

// Simulate async operation, move to end of message queue.
function asyncify(fn) {
return function() {
return function () {
var args = arguments;
setTimeout(function() {
setTimeout(function () {
fn.apply(null, args);
}, 0);
};
}

describe('#notify()', function() {
beforeEach(function() {
utils.fileCommandJson = asyncify(function(n, o, cb) {
describe('#notify()', function () {
beforeEach(function () {
utils.fileCommandJson = asyncify(function (n, o, cb) {
cb(null, '');
});
});

afterEach(function() {
afterEach(function () {
utils.fileCommandJson = originalUtils;
});

it('should notify with a message', function(done) {
notifier.notify({ message: 'Hello World' }, function(err, response) {
it('should notify with a message', function (done) {
notifier.notify({ message: 'Hello World' }, function (err, response) {
expect(err).toBeNull();
done();
});
});

it('should be chainable', function(done) {
it('should be chainable', function (done) {
notifier
.notify({ message: 'First test' })
.notify({ message: 'Second test' }, function(err, response) {
.notify({ message: 'Second test' }, function (err, response) {
expect(err).toBeNull();
done();
});
});

it('should be able to list all notifications', function(done) {
utils.fileCommandJson = asyncify(function(n, o, cb) {
it('should be able to list all notifications', function (done) {
utils.fileCommandJson = asyncify(function (n, o, cb) {
cb(
null,
fs
Expand All @@ -117,14 +117,14 @@ describe('terminal-notifier', function() {
);
});

notifier.notify({ list: 'ALL' }, function(_, response) {
notifier.notify({ list: 'ALL' }, function (_, response) {
expect(response).toBeTruthy();
done();
});
});

it('should be able to remove all messages', function(done) {
utils.fileCommandJson = asyncify(function(n, o, cb) {
it('should be able to remove all messages', function (done) {
utils.fileCommandJson = asyncify(function (n, o, cb) {
cb(
null,
fs
Expand All @@ -133,39 +133,39 @@ describe('terminal-notifier', function() {
);
});

notifier.notify({ remove: 'ALL' }, function(_, response) {
notifier.notify({ remove: 'ALL' }, function (_, response) {
expect(response).toBeTruthy();

utils.fileCommandJson = asyncify(function(n, o, cb) {
utils.fileCommandJson = asyncify(function (n, o, cb) {
cb(null, '');
});

notifier.notify({ list: 'ALL' }, function(_, response) {
notifier.notify({ list: 'ALL' }, function (_, response) {
expect(response).toBeFalsy();
done();
});
});
});
});

describe('arguments', function() {
beforeEach(function() {
describe('arguments', function () {
beforeEach(function () {
this.original = utils.fileCommandJson;
});

afterEach(function() {
afterEach(function () {
utils.fileCommandJson = this.original;
});

function expectArgsListToBe(expected, done) {
utils.fileCommandJson = asyncify(function(notifier, argsList, callback) {
utils.fileCommandJson = asyncify(function (notifier, argsList, callback) {
expect(argsList).toEqual(expected);
callback();
done();
});
}

it('should allow for non-sensical arguments (fail gracefully)', function(done) {
it('should allow for non-sensical arguments (fail gracefully)', function (done) {
var expected = [
'-title',
'"title"',
Expand All @@ -191,8 +191,8 @@ describe('terminal-notifier', function() {
});
});

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

it('should convert list of actions to flat list', function(done) {
it('should convert list of actions to flat list', function (done) {
var expected = [
'-title',
'"title \\"message\\""',
'-message',
'"body \\"message\\""',
'-actions',
'foo,bar,baz "foo" bar',
'"foo","bar","baz \\"foo\\" bar"',
'-timeout',
'"10"',
'-json',
Expand All @@ -232,7 +232,7 @@ describe('terminal-notifier', function() {
});
});

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

it('should let timeout set precedence over wait', function(done) {
it('should let timeout set precedence over wait', function (done) {
var expected = [
'-title',
'"Title"',
Expand All @@ -277,7 +277,7 @@ describe('terminal-notifier', function() {
});
});

it('should not set a default timeout if explicitly false', function(done) {
it('should not set a default timeout if explicitly false', function (done) {
var expected = [
'-title',
'"Title"',
Expand All @@ -299,7 +299,7 @@ describe('terminal-notifier', function() {
});
});

it('should escape all title and message', function(done) {
it('should escape all title and message', function (done) {
var expected = [
'-title',
'"title \\"message\\""',
Expand Down

0 comments on commit 51ed238

Please sign in to comment.