Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Don't replace escaped regex / function placeholders in strings #22

Closed
wants to merge 1 commit into from
Closed

Don't replace escaped regex / function placeholders in strings #22

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 21 additions & 4 deletions index.js
View file
Open in desktop
Expand Up @@ -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');
Copy link
Collaborator

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()?

Copy link
Author

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()

Copy link
Collaborator

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.


// 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;
Expand All @@ -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 = {});

Expand Down Expand Up @@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The 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?

Copy link
Author

Choose a reason for hiding this comment

The 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();
}
Expand Down
3 changes: 3 additions & 0 deletions package.json
View file
Open in desktop
Expand Up @@ -30,5 +30,8 @@
"istanbul": "^0.3.2",
"mocha": "^1.21.4",
"xunit-file": "0.0.5"
},
"dependencies": {
"randombytes": "^2.0.3"
}
}
26 changes: 26 additions & 0 deletions test/unit/serialize.js
View file
Open in desktop
@@ -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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

new Buffer() is deprecated. I think we should use Buffer.from() instead.

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');
Expand Down Expand Up @@ -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');
Expand Down