Add SqlString.fn Function Call Proxy

[kevinmartin]

Adds the ability to raw "escape" by calling any SqlString.fn.* property as a function.

It takes the Proxy example from #9 and merges it in directly into the library.

It only works on Node.js v6+ (according to node.green), but there is a check so that users of lower versions can use the library (without SqlString.fn.* support), which is a con #9 didn't have..

Examples:

const a = SqlString.fn.POINT(123, 456);
const b = SqlString.fn.CURRENT_TIMESTAMP();
const c = SqlString.fn.CONCAT(SqlString.fn.UPPER('abc'), SqlString.fn.LOWER('ABC'));

SqlString.escape(a); // -> POINT(123, 456)
SqlString.escape(b); // -> CURRENT_TIMESTAMP()
SqlString.escape(c); // -> CONCAT(UPPER("abc"), LOWER("ABC"))

Should be a lot more secure than #9, since the escape function checks if the value from the SqlString.fn.* call is in an instance of an internal function class (SqlFunction) that doesn't get exported. It also escapes all arguments.

Playing (security) devils advocate:

• If someone has external influence on SqlString.fn or SqlString.escape, you're screwed.
• If someone has external influence on Array.prototype.map, you're screwed.
• If someone has external influence on the global Proxy class, you're screwed.
• If someone has external influence on your app, you're screwed.

[dougwilson]

Looks like there are two main issues:

1. The automated system is marking that there are not enough tests to even get to full coverage.
2. Change should support all the versions of Node.js this module supports (Node.js 0.6+ currently).

[Myhlamaeus]

Should be a lot more secure than #9, since the escape function checks if the value from the SqlString.fn.* call is in an instance of an internal function class (SqlFunction) that doesn't get exported. It also escapes all arguments.

a.js:

const SqlString = require('sqlstring');
SqlString.escape(Object.assign(Object.create(SqlString.fn.something.prototype), {
  toSQL() {
    return 'nope';
  }
})); // 'nope';
SqlString.fn.something.prototype[Symbol.hasInstance] = () => true;
Object.prototype.toSQL = () => 'nopenope';

b.js:

require('./a.js');
require('sqlstring').escape({}); // 'nopenope'

Object.freeze would prevent setting Symbol.hasInstance, but one could still create a new instance with Object.create (or Object.setPrototypeOf).

An alternative way to solve this might be WeakMap, but WeakMap.prototype.get and WeakMap.prototype.has are not exempt from external modifications.

• If someone has external influence on SqlString.fn or SqlString.escape, you're screwed.
• If someone has external influence on Array.prototype.map, you're screwed.
• If someone has external influence on the global Proxy class, you're screwed.
• If someone has external influence on your app, you're screwed.

a.js:

require('sqlstring').escape = () => 'screwed';
Array.prototype.map = () => ['screwed'];
⋮

b.js:

require('./a.js');
require('sqlstring').escape('test'); // 'screwed'
[42].map(a => a ** 2).join(''); // 'screwed'
⋮

2. Change should support all the versions of Node.js this module supports (Node.js 0.6+ currently).

Shouldn't that be fine as long as every user of those functions sets the engine property of the package.json accordingly?

I don't think that any of this is sensible: everyone who wants to harm you will still do so, and almost with the same effort; I also don't think that it is possible to completely remove this.
Shouldn't one then instead use the variant which works for older versions, too? One could make it so that the user would have to explicitly allow the usage of toSQL, either through an alternative method or some attribute.

Also, I would suggest the usage of a Symbol instead—this would definitively prevent incompatibilities between modules as symbols are unique. (However, this would not be more secure: the likes of Object.getOwnPropertySymbols and Object.getOwnPropertyDescriptors return all owned property symbols. Also, it would reintroduce incompatibility to node versions < v0.12.)

[dougwilson]

@kevinmartin I just landed your #9 , do you think that supersedes this PR or still something that should exist?