Skip to content

Commit

Permalink
Blacklist a few other unsafe opts from passing in data obj
Browse files Browse the repository at this point in the history
  • Loading branch information
mde committed Dec 2, 2016
1 parent 7eaba6a commit 49264e0
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 7 deletions.
14 changes: 9 additions & 5 deletions lib/ejs.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,12 @@ var _REGEX_STRING = '(<%%|%%>|<%=|<%-|<%_|<%#|<%|%>|-%>|_%>)';
var _OPTS = [ 'cache', 'filename', 'delimiter', 'scope', 'context',
'debug', 'compileDebug', 'client', '_with', 'root', 'rmWhitespace',
'strict', 'localsName'];
var _OPTS_IN_DATA_BLACKLIST = {
cache: true,
filename: true,
root: true,
localsName: true
};
var _BOM = /^\uFEFF/;

/**
Expand Down Expand Up @@ -268,11 +274,9 @@ function rethrow(err, str, filename, lineno){
function cpOptsInData(data, opts) {
_OPTS.forEach(function (p) {
if (typeof data[p] != 'undefined') {
// Disallow setting the root opt for includes via a passed data obj
// Unsanitized, parameterized use of `render` could allow the
// include directory to be reset, opening up the possibility of
// remote code execution
if (p == 'root') {
// Disallow passing potentially dangerous opts in the data
// These opts should not be settable via a `render` call
if (_OPTS_IN_DATA_BLACKLIST[p]) {
return;
}
opts[p] = data[p];
Expand Down
18 changes: 16 additions & 2 deletions test/ejs.js
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,21 @@ suite('ejs.compile(str, options)', function () {

});

/* Old API -- remove when this shim goes away */
suite('ejs.render(str, dataAndOpts)', function () {
test('render the template with data/opts passed together', function () {
assert.equal(ejs.render('<p><?= foo ?></p>', {foo: 'yay', delimiter: '?'}),
'<p>yay</p>');
});

test('disallow unsafe opts passed along in data', function () {
assert.equal(ejs.render('<p><?= locals.foo ?></p>',
// localsName should not get reset because it's blacklisted
{_with: false, foo: 'yay', delimiter: '?', localsName: '_'}),
'<p>yay</p>');
});
});

suite('ejs.render(str, data, opts)', function () {
test('render the template', function () {
assert.equal(ejs.render('<p>yay</p>'), '<p>yay</p>');
Expand Down Expand Up @@ -753,7 +768,6 @@ suite('include()', function () {
var viewsPath = path.join(__dirname, 'fixtures');
assert.equal(ejs.render(fixture('include-root.ejs'), {pets: users}, {filename: file, delimiter: '@',root:viewsPath}),
fixture('include.html'));

});

test('work when nested', function () {
Expand Down Expand Up @@ -918,7 +932,7 @@ suite('preprocessor include', function () {
var template = fixture('include_preprocessor_line_slurp.ejs');
var expected = fixture('include_preprocessor_line_slurp.html');
var options = {rmWhitespace: true, filename: file};
assert.equal(ejs.render(template, options),
assert.equal(ejs.render(template, {}, options),
expected);
});

Expand Down

2 comments on commit 49264e0

@ryptozee
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So version older than 2.5.5 is vulnarable to cross-site scripting in the ejs.renderFile(). Have noticed the fix for code injection. Thumbs up!

@abdulhannanali
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup thanks for fixing this, I too got an alert today from Github for a practice project I made quite sometime ago. Github's Dependency Graph is pretty cool

Screenshot

Please sign in to comment.