-
Notifications
You must be signed in to change notification settings - Fork 182
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't replace escaped regex / function placeholders in strings #22
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,11 +6,13 @@ See the accompanying LICENSE file for terms. | |
|
||
'use strict'; | ||
|
||
var isRegExp = require('util').isRegExp; | ||
var isRegExp = require('util').isRegExp; | ||
var randomBytes = require('randombytes'); | ||
|
||
// Generate an internal UID to make the regexp pattern harder to guess. | ||
var UID = Math.floor(Math.random() * 0x10000000000).toString(16); | ||
var PLACE_HOLDER_REGEXP = new RegExp('"@__(F|R)-' + UID + '-(\\d+)__@"', 'g'); | ||
var UID_LENGTH = 16; | ||
var UID = generateUID(); | ||
var PLACE_HOLDER_REGEXP = new RegExp('(\\\\)?"@__(F|R)-' + UID + '-(\\d+)__@"', 'g'); | ||
|
||
var IS_NATIVE_CODE_REGEXP = /\{\s*\[native code\]\s*\}/g; | ||
var UNSAFE_CHARS_REGEXP = /[<>\/\u2028\u2029]/g; | ||
|
@@ -29,6 +31,15 @@ function escapeUnsafeChars(unsafeChar) { | |
return ESCAPED_CHARS[unsafeChar]; | ||
} | ||
|
||
function generateUID() { | ||
var bytes = randomBytes(UID_LENGTH); | ||
var result = ''; | ||
for(var i=0; i<UID_LENGTH; ++i) { | ||
result += bytes[i].toString(16); | ||
} | ||
return result; | ||
} | ||
|
||
module.exports = function serialize(obj, options) { | ||
options || (options = {}); | ||
|
||
|
@@ -92,7 +103,13 @@ module.exports = function serialize(obj, options) { | |
// Replaces all occurrences of function and regexp placeholders in the JSON | ||
// string with their string representations. If the original value can not | ||
// be found, then `undefined` is used. | ||
return str.replace(PLACE_HOLDER_REGEXP, function (match, type, valueIndex) { | ||
return str.replace(PLACE_HOLDER_REGEXP, function (match, backSlash, type, valueIndex) { | ||
// The placeholder may not be preceded by a backslash. This is to prevent | ||
// replacing things like `"a\"@__R-<UID>-0__@"` and thus outputting | ||
// invalid JS. | ||
if (backSlash) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now with a better source of entropy, is the backslash escape still required, or did you want to leave it for extra protection? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think there's any harm in having it. I'm more confident that this will correctly prevent user input from escaping strings than I am in my knowledge of CSPRNGs :) |
||
return match; | ||
} | ||
if (type === 'R') { | ||
return regexps[valueIndex].toString(); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,23 @@ | ||
/* global describe, it, beforeEach */ | ||
'use strict'; | ||
|
||
// temporarily monkeypatch `crypto.randomBytes` so we'll have a | ||
// predictable UID for our tests | ||
var crypto = require('crypto'); | ||
var oldRandom = crypto.randomBytes; | ||
crypto.randomBytes = function(len, cb) { | ||
var buf = new Buffer(len); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
buf.fill(0x00); | ||
if (cb) | ||
cb(null, buf); | ||
return buf; | ||
}; | ||
|
||
var serialize = require('../../'), | ||
expect = require('chai').expect; | ||
|
||
crypto.randomBytes = oldRandom; | ||
|
||
describe('serialize( obj )', function () { | ||
it('should be a function', function () { | ||
expect(serialize).to.be.a('function'); | ||
|
@@ -111,6 +125,18 @@ describe('serialize( obj )', function () { | |
}); | ||
}); | ||
|
||
describe('placeholders', function() { | ||
it('should not be replaced within string literals', function () { | ||
// Since we made the UID deterministic this should always be the placeholder | ||
var fakePlaceholder = '"@__R-0000000000000000-0__@'; | ||
var serialized = serialize({bar: /1/i, foo: fakePlaceholder}, {uid: 'foo'}); | ||
var obj = eval('(' + serialized + ')'); | ||
expect(obj).to.be.a('Object'); | ||
expect(obj.foo).to.be.a('String'); | ||
expect(obj.foo).to.equal(fakePlaceholder); | ||
}); | ||
}); | ||
|
||
describe('regexps', function () { | ||
it('should serialize constructed regexps', function () { | ||
var re = new RegExp('asdf'); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious why you chose this package over the built-in
crypto.randomBytes()
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used that so as not to break the serializer when run in a browser. If the serializer intended to be node-only I'll just use
crypto.randomBytes()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My intention is node-only. But it's true that I don't know if people are bundling it for the browser. So let's stick with what you have using
randombytes
.