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: sanitize query by default #452

Merged
merged 1 commit into from
Aug 15, 2018
Merged

fix: sanitize query by default #452

merged 1 commit into from
Aug 15, 2018

Conversation

virkt25
Copy link
Contributor

@virkt25 virkt25 commented Aug 14, 2018

Description

This PR adds a sanitization step to the buildWhere and buildSort function using the new sanitizeFilter function.

This function accepts an option in an options object disableSanitization - which can be any truthy value to disable sanitization (filter passed in is returned as-is)

As per https://docs.mongodb.com/manual/core/server-side-javascript/ only $where and mapReduce properties can execute JavaScript on the Mongo Driver so sanitizeFilter removes those properties if present at the top level of the query object.

Related issues

fixes #403

Checklist

  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style
    guide

if (!filter || typeof filter !== 'object') return filter;

for (const key in filter) {
if (key === '$where' || key === 'mapReduce') {
Copy link
Contributor

Choose a reason for hiding this comment

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

$mapReduce?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to the docs it's mapReduce.

https://docs.mongodb.com/manual/core/server-side-javascript/

Copy link
Contributor

@raymondfeng raymondfeng Aug 14, 2018

Choose a reason for hiding this comment

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

Can the special keys be nested in the filter deeper than the 1st level?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. As pointed out by the test case below that @shimks commented on, Mongo only executes these at the top most level and not at the property level.

Post.create({title: 'Post1', content: 'Post1 content'}, (err, p1) => {
Post.create({title: 'Post2', content: 'Post2 content'}, (err2, p2) => {
Post.create({title: 'Post3', content: 'Post3 data'}, (err3, p3) => {
Post.find({where: {conent: {where: 'function() {return this.content.contains("content")}'}}}, (err, p) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

conent -> content
where -> $where

Post.create({title: 'Post3', content: 'Post3 data'}, (err3, p3) => {
Post.find({where: {content: {$where: 'function() {return this.content.contains("content")}'}}}, (err, p) => {
should.not.exist(err);
p.length.should.be.equal(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let me know if I'm understanding this correctly: $where clause here is not executed, meaning the where filter effectively looks like this: {where: {content: ''}}, so p in this case is just an empty array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well ... it tried to match content to the object {$where: '...'}

@virkt25 virkt25 merged commit 4df3c63 into master Aug 15, 2018
@virkt25 virkt25 deleted the sanitize branch August 15, 2018 14:07
@raymondfeng
Copy link
Contributor

@virkt25

There are more similar cases, such as:

{"where":{"$and":[{"$where":"function(){sleep(1000); return this.username.contains('test');}"}]}}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

$where and Javascript Server-side Code Injection
3 participants