Skip to content

Commit

Permalink
Improve MongoDB query sanitization
Browse files Browse the repository at this point in the history
This commit changes how the MongoDB queries from the Mongo Ruby Driver
are sanitized.

Previously, the sanitizer filtered almost the whole query, not allowing
users to know which attributes and embedded documents were involved.

Also, the arrays were collapsed, hiding all the elements except the
first one. This might be problematic in Mongo queries as documents don't
have a closed schema so the sanitization could hide some attributes.

This change makes the Mongo query sanitization more similar to what we
do with SQL queries using sql_lexer.

Before:

```js
{
  "$db": "?",
  "documents": "[?]",
  "insert": "posts",
  "lsid": "?",
  "ordered": true
}
```

After:

```js
{
  "$db": "?",
  "documents": [
    {
      "_id": "?",
      "authors": [
        {
          "_id": "?",
          "name": "?"
        },
        {
          "_id": "?",
          "name": "?",
          "surname": "?"
        }
      ],
      "body": "?",
      "title": "?"
    }
  ],
  "insert": "posts",
  "lsid": "?",
  "ordered": true
}
```
  • Loading branch information
luismiramirez committed Feb 10, 2022
1 parent 0e9b72f commit f19d9dc
Show file tree
Hide file tree
Showing 4 changed files with 34 additions and 66 deletions.
9 changes: 9 additions & 0 deletions .changesets/improve-mongo-ruby-driver-sanitization.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
---
bump: "patch"
type: "change"
---

The MongoDB query sanitization now shows all the attributes in the query at all levels.
Only the actual values are filtered with a `?` character. Less MongoDB queries are now marked
as N+1 queries when they weren't the exact same query. This increases the number of unique events
AppSignal tracks for MongoDB queries.
25 changes: 7 additions & 18 deletions lib/appsignal/event_formatter/mongo_ruby_driver/query_formatter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,28 +21,28 @@ class QueryFormatter
},
"insert" => {
"insert" => :allow,
"documents" => :deny_array,
"documents" => :sanitize_document,
"ordered" => :allow
},
"update" => {
"update" => :allow,
"updates" => :sanitize_bulk,
"updates" => :sanitize_document,
"ordered" => :allow
},
"findandmodify" => {
"findandmodify" => :allow,
"query" => :sanitize_document,
"update" => :deny_array,
"update" => :sanitize_document,
"new" => :allow
},
"delete" => {
"delete" => :allow,
"deletes" => :sanitize_bulk,
"deletes" => :sanitize_document,
"ordered" => :allow
},
"bulk" => {
"q" => :sanitize_document,
"u" => :deny_array,
"u" => :sanitize_document,
"limit" => :allow,
"multi" => :allow,
"upsert" => :allow
Expand All @@ -68,20 +68,9 @@ def self.format(strategy, command)
# Applies strategy on hash values based on keys
def self.apply_strategy(strategy, val)
case strategy
when :allow then val
when :deny then "?"
when :deny_array then "[?]"
when :allow then val
when :sanitize_document
Appsignal::Utils::QueryParamsSanitizer.sanitize(val, true, :mongodb)
when :sanitize_bulk
if val.length > 1
[
format(:bulk, val.first),
"[...]"
]
else
val.map { |v| format(:bulk, v) }
end
Appsignal::Utils::QueryParamsSanitizer.sanitize(val, false, :mongodb)
else "?"
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,57 +51,27 @@
end
end

context "when strategy is deny" do
let(:strategy) { :deny }
let(:value) { { "_id" => 1 } }

it "should return a '?'" do
expect(formatter.apply_strategy(strategy, value)).to eql("?")
end
end

context "when strategy is deny_array" do
let(:strategy) { :deny_array }
let(:value) { { "_id" => 1 } }

it "should return a sanitized array string" do
expect(formatter.apply_strategy(strategy, value)).to eql("[?]")
end
end

context "when strategy is sanitize_document" do
let(:strategy) { :sanitize_document }
let(:value) { { "_id" => 1 } }

it "should return a sanitized document" do
expect(formatter.apply_strategy(strategy, value)).to eql("_id" => "?")
end
end

context "when strategy is sanitize_bulk" do
let(:strategy) { :sanitize_bulk }
let(:value) { [{ "q" => { "_id" => 1 }, "u" => [{ "foo" => "bar" }] }] }

it "should return an array of sanitized bulk documents" do
expect(formatter.apply_strategy(strategy, value)).to eql([
{ "q" => { "_id" => "?" }, "u" => "[?]" }
])
let(:value) do
{
"_id" => 1,
"authors" => [
{ "name" => "BarBaz" },
{ "name" => "FooBar" },
{ "name" => "BarFoo", "surname" => "Baz" }
]
}
end

context "when bulk has more than one update" do
let(:value) do
[
{ "q" => { "_id" => 1 }, "u" => [{ "foo" => "bar" }] },
{ "q" => { "_id" => 2 }, "u" => [{ "foo" => "baz" }] }
it "should return a sanitized document" do
expect(formatter.apply_strategy(strategy, value)).to eql(
"_id" => "?",
"authors" => [
{ "name" => "?" },
{ "name" => "?", "surname" => "?" }
]
end

it "should return only the first value of sanitized bulk documents" do
expect(formatter.apply_strategy(strategy, value)).to eql([
{ "q" => { "_id" => "?" }, "u" => "[?]" },
"[...]"
])
end
)
end
end

Expand Down
4 changes: 2 additions & 2 deletions spec/lib/appsignal/utils/query_params_sanitizer_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -117,15 +117,15 @@
context "when value is an array" do
let(:value) { %w[foo bar] }

it "should sanitize all hash values with a single questionmark" do
it "should sanitize all hash values with a single question mark" do
expect(subject).to eq(["?"])
end
end

context "when value is a mixed array" do
let(:value) { [nil, "foo", "bar"] }

it "should sanitize all hash values with a single questionmark" do
it "should sanitize all hash values with a single question mark" do
expect(subject).to eq(["?"])
end
end
Expand Down

0 comments on commit f19d9dc

Please sign in to comment.