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.

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

fix: replace named parameters in n1ql query #53

Merged
merged 1 commit into from
Jun 8, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 3 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,9 @@ There are two examples:
Person.query({ where: { name: { regexp: /^C+.*/ } } });
```

- SQL injection: The n1ql is generated via concatenating the string. But the parameters do not include in the query. The parameters will be escaped by CouchBase itself. For the reason, it's free from SQL injection.
- ~SQL injection: The n1ql is generated via concatenating the string. But the parameters do not include in the query. The parameters will be escaped by CouchBase itself. For the reason, it's free from SQL injection.~
Before 1.1.2, this lib was using [named parameters for query](https://docs.couchbase.com/server/6.0/n1ql/n1ql-rest-api/exnamed.html), which we've observed significant impact on the query execute response time, thus it will be dropped in future version.
To prevent n1ql injection, we are following the guidelines in https://blog.couchbase.com/couchbase-and-n1ql-security-centeredgesoftware/


- Only `ready` status index can be used.
Expand Down
6 changes: 5 additions & 1 deletion lib/n1ql.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ const N1qlQuery = require('couchbase').N1qlQuery;
const getFieldName = require('./tokenizer');
const util = require('util');
const Promise = require('bluebird');
const replaceNamedParam = require('./replaceNamedParam');

const symbols = {
nlike: 'NOT LIKE',
Expand Down Expand Up @@ -273,10 +274,13 @@ module.exports = (Model, bootOptions = {}) => {
query,
params
});
// using named parameters makes query significant slower when number of db doc exceeds 2000.
// reason see https://forums.couchbase.com/t/bad-performance-with-named-parameters-in-set-list/16862/5
const queryStr = replaceNamedParam(query, params);
return Model
.getConnector()
.connect()
.call('queryAsync', N1qlQuery.fromString(query), params);
.call('queryAsync', N1qlQuery.fromString(queryStr));
};
Model.query = async(rawFilters = {}, options = {}) => {
// copy the filter
Expand Down
23 changes: 23 additions & 0 deletions lib/replaceNamedParam.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
'use strict';

module.exports = (queryStr, params) => {
if (!params) return queryStr;

for (let key in params) {
if (Object.prototype.hasOwnProperty.call(params, key)) {
let val = params[key];
if (typeof val === 'string' || val instanceof String) {
// escaping to prevent n1ql injection
// see https://blog.couchbase.com/couchbase-and-n1ql-security-centeredgesoftware/
val = val.replace(/'/g, "''");
val = val.replace(/`/g, '``');
val = `'${val}'`;
} else {
val = JSON.stringify(val);
}
let re = new RegExp('\\$' + key, 'g');
queryStr = queryStr.replace(re, val);
}
}
return queryStr;
};
26 changes: 26 additions & 0 deletions test/replaceNamedParam.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/* eslint-disable max-len */
'use strict';

const replaceNamedParam = require('../lib/replaceNamedParam');
const expect = require('chai').expect;

describe('replaceNamedParam test', () => {
it('should replace named parameter with their value: single parameter', async() => {
const ql = "SELECT COUNT(META().id) as total from `products` WHERE _type='Product' AND ( ANY array_element IN SUFFIXES(LOWER(`name`)) SATISFIES array_element like $param_2 END)";
const pa = { param_2: '%耳环%' };
const res = replaceNamedParam(ql, pa);
expect(res).to.be.eql("SELECT COUNT(META().id) as total from `products` WHERE _type='Product' AND ( ANY array_element IN SUFFIXES(LOWER(`name`)) SATISFIES array_element like '%耳环%' END)");
});
it('should replace named parameter with their value: multiple parameters', async() => {
const ql = "SELECT *,TOSTRING(META().id) AS id from `products` WHERE _type='Product' AND ( `name` = $param_1 AND ARRAY_CONTAINS(`tags`, $param_3) AND `user` IN $param_5) ORDER BY `updatedAt` DESC LIMIT 25";
const pa = { param_1: '耳环', param_3: 'sci-fi', param_5: ['name', 'name2'] };
const res = replaceNamedParam(ql, pa);
expect(res).to.be.eql("SELECT *,TOSTRING(META().id) AS id from `products` WHERE _type='Product' AND ( `name` = '耳环' AND ARRAY_CONTAINS(`tags`, 'sci-fi') AND `user` IN [\"name\",\"name2\"]) ORDER BY `updatedAt` DESC LIMIT 25");
});
it('should replace named parameter with their value and do escaping for single quote', async() => {
const ql = "SELECT COUNT(META().id) as total from `products` WHERE _type='Product' AND ( ANY array_element IN SUFFIXES(LOWER(`name`)) SATISFIES array_element like $param_2 END)";
const pa = { param_2: "' OR '1'='1" };
const res = replaceNamedParam(ql, pa);
expect(res).to.be.eql("SELECT COUNT(META().id) as total from `products` WHERE _type='Product' AND ( ANY array_element IN SUFFIXES(LOWER(`name`)) SATISFIES array_element like ''' OR ''1''=''1' END)");
});
});