-
Notifications
You must be signed in to change notification settings - Fork 29
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: #186 avoid infinite recursion in filter method #187
Changes from all commits
057a0bf
33ef893
b50e9b0
05c4d32
a9aeda5
0c92329
26513ce
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,18 +31,37 @@ type UserMessageData = RawUserData | string | undefined; | |
const humanString = require("object-to-human-string"); | ||
const packageDetails = require("../package.json"); | ||
|
||
function filterKeys(obj: object, filters: string[]): object { | ||
/** | ||
* Filter properties in obj according to provided filters. | ||
* Also removes any recursive self-referencing object. | ||
* @param obj object to apply filter | ||
* @param filters list of keys to filter | ||
* @param explored Set that contains already explored nodes, used internally | ||
*/ | ||
function filterKeys( | ||
obj: object, | ||
filters: string[], | ||
explored: Set<object> | null = null, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
): object { | ||
if (!obj || !filters || typeof obj !== "object") { | ||
return obj; | ||
} | ||
|
||
// create or update the explored set with the incoming object | ||
const _explored = explored?.add(obj) || new Set([obj]); | ||
|
||
// Make temporary copy of the object to avoid mutating the original | ||
// Cast to Record<string, object> to enforce type check and avoid using any | ||
const _obj = { ...obj } as Record<string, object>; | ||
Object.keys(obj).forEach(function (i) { | ||
if (filters.indexOf(i) > -1) { | ||
delete _obj[i]; | ||
|
||
Object.keys(obj).forEach(function (key) { | ||
// Remove child if: | ||
// - the key is in the filter array | ||
// - the value is already in the explored Set | ||
if (filters.indexOf(key) > -1 || _explored.has(_obj[key])) { | ||
delete _obj[key]; | ||
} else { | ||
_obj[i] = filterKeys(_obj[i], filters); | ||
_obj[key] = filterKeys(_obj[key], filters, _explored); | ||
} | ||
}); | ||
return _obj; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -377,6 +377,10 @@ test("filter keys tests", function (t) { | |
}); | ||
var message = builder.build(); | ||
|
||
// Original object should not be modified | ||
t.equal(body.username, "[email protected]"); | ||
t.equal(body.password, "nice try"); | ||
|
||
Comment on lines
+380
to
+383
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added to ensure that we do the copy in the |
||
t.test("form is filtered", function (tt) { | ||
tt.equal(message.details.request.form.username, undefined); | ||
tt.equal(message.details.request.form.password, undefined); | ||
|
@@ -399,6 +403,49 @@ test("filter keys tests", function (t) { | |
}); | ||
}); | ||
|
||
test("avoid infinite recursion in filter method", function (t) { | ||
let builder = new MessageBuilder({ | ||
filters: ["filtered"], | ||
}); | ||
|
||
// body self-references, causing a potential infinite recursion in the filter method | ||
// Causes exception: Maximum call stack size exceeded | ||
let body = { | ||
key: "value", | ||
filtered: true, | ||
}; | ||
// second level self-reference | ||
let other = { | ||
body: body, | ||
filtered: true, | ||
}; | ||
body.myself = body; | ||
body.other = other; | ||
|
||
let queryString = {}; | ||
let headers = {}; | ||
|
||
// performs filter on set | ||
builder.setRequestDetails({ | ||
body: body, | ||
query: queryString, | ||
headers: headers, | ||
}); | ||
var message = builder.build(); | ||
|
||
// key is preserved | ||
t.equal(message.details.request.form.key, "value"); | ||
// property in "filters" is filtered | ||
t.equal(message.details.request.form.filtered, undefined); | ||
t.equal(message.details.request.form.other.filtered, undefined); | ||
// self-referencing objects are also not included | ||
t.equal(message.details.request.form.myself, undefined); | ||
t.equal(message.details.request.form.other.body, undefined); | ||
|
||
// test should finish | ||
t.end(); | ||
}); | ||
|
||
test("custom tags", function (t) { | ||
t.test("with array", function (tt) { | ||
var builder = new MessageBuilder(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if it's been checked but it may be useful to check around the repo to see if we've done infinite recursion in the past and fix that up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't see any other instances in this repo. Do you mean in all the codebases from Raygun?