-
-
Notifications
You must be signed in to change notification settings - Fork 596
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: Fail on infinite recursion in encode.js
#2099
base: alpha
Are you sure you want to change the base?
Changes from all commits
9258b32
f63a5a2
e0c322a
fe3a1bc
d33a54a
21f14c2
12165c4
b22ff51
3b6684d
a7333c4
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 |
---|---|---|
@@ -0,0 +1,21 @@ | ||
/** | ||
* Helper function that turns a string into a unique 53-bit hash. | ||
* @ref https://stackoverflow.com/a/52171480/6456163 | ||
* @param {string} str | ||
* @param {number} seed | ||
* @returns {number} | ||
*/ | ||
export const cyrb53 = (str, seed = 0) => { | ||
let h1 = 0xdeadbeef ^ seed, h2 = 0x41c6ce57 ^ seed; | ||
for (let i = 0, ch; i < str.length; i++) { | ||
ch = str.charCodeAt(i); | ||
h1 = Math.imul(h1 ^ ch, 2654435761); | ||
h2 = Math.imul(h2 ^ ch, 1597334677); | ||
} | ||
h1 = Math.imul(h1 ^ (h1 >>> 16), 2246822507); | ||
h1 ^= Math.imul(h2 ^ (h2 >>> 13), 3266489909); | ||
h2 = Math.imul(h2 ^ (h2 >>> 16), 2246822507); | ||
h2 ^= Math.imul(h1 ^ (h1 >>> 13), 3266489909); | ||
|
||
return 4294967296 * (2097151 & h2) + (h1 >>> 0); | ||
}; | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,85 +9,117 @@ import ParsePolygon from './ParsePolygon'; | |
import ParseObject from './ParseObject'; | ||
import { Op } from './ParseOp'; | ||
import ParseRelation from './ParseRelation'; | ||
import { cyrb53 } from './CryptoUtils'; | ||
|
||
const MAX_RECURSIVE_CALLS = 999; | ||
|
||
function encode( | ||
value: mixed, | ||
disallowObjects: boolean, | ||
forcePointers: boolean, | ||
seen: Array<mixed>, | ||
offline: boolean | ||
offline: boolean, | ||
counter: number, | ||
initialValue: mixed | ||
): any { | ||
counter++; | ||
|
||
if (counter > MAX_RECURSIVE_CALLS) { | ||
const message = 'Encoding object failed due to high number of recursive calls, likely caused by circular reference within object.'; | ||
console.error(message); | ||
console.error('Value causing potential infinite recursion:', initialValue); | ||
|
||
throw new Error(message); | ||
} | ||
|
||
if (value instanceof ParseObject) { | ||
if (disallowObjects) { | ||
throw new Error('Parse Objects not allowed here'); | ||
} | ||
const seenEntry = value.id ? value.className + ':' + value.id : value; | ||
const entryIdentifier = value.id ? value.className + ':' + value.id : value; | ||
if ( | ||
forcePointers || | ||
!seen || | ||
seen.indexOf(seenEntry) > -1 || | ||
seen.includes(entryIdentifier) || | ||
value.dirty() || | ||
Object.keys(value._getServerData()).length < 1 | ||
Object.keys(value._getServerData()).length === 0 | ||
) { | ||
if (offline && value._getId().startsWith('local')) { | ||
return value.toOfflinePointer(); | ||
} | ||
return value.toPointer(); | ||
} | ||
seen = seen.concat(seenEntry); | ||
seen.push(entryIdentifier); | ||
return value._toFullJSON(seen, offline); | ||
} | ||
if ( | ||
} else if ( | ||
value instanceof Op || | ||
value instanceof ParseACL || | ||
value instanceof ParseGeoPoint || | ||
value instanceof ParsePolygon || | ||
value instanceof ParseRelation | ||
) { | ||
return value.toJSON(); | ||
} | ||
if (value instanceof ParseFile) { | ||
} else if (value instanceof ParseFile) { | ||
if (!value.url()) { | ||
throw new Error('Tried to encode an unsaved file.'); | ||
} | ||
return value.toJSON(); | ||
} | ||
if (Object.prototype.toString.call(value) === '[object Date]') { | ||
} else if (Object.prototype.toString.call(value) === '[object Date]') { | ||
if (isNaN(value)) { | ||
throw new Error('Tried to encode an invalid date.'); | ||
} | ||
return { __type: 'Date', iso: (value: any).toJSON() }; | ||
} | ||
if ( | ||
} else if ( | ||
Object.prototype.toString.call(value) === '[object RegExp]' && | ||
typeof value.source === 'string' | ||
) { | ||
return value.source; | ||
} | ||
|
||
if (Array.isArray(value)) { | ||
return value.map(v => { | ||
return encode(v, disallowObjects, forcePointers, seen, offline); | ||
}); | ||
} | ||
|
||
if (value && typeof value === 'object') { | ||
} else if (Array.isArray(value)) { | ||
return value.map(v => encode(v, disallowObjects, forcePointers, seen, offline, counter, initialValue)); | ||
} else if (value && typeof value === 'object') { | ||
const output = {}; | ||
for (const k in value) { | ||
output[k] = encode(value[k], disallowObjects, forcePointers, seen, offline); | ||
try { | ||
// Attempts to get the name of the object's constructor | ||
// Ref: https://stackoverflow.com/a/332429/6456163 | ||
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. Same topic as earlier. If you copy/pasted someone else's code from SO then we cannot accept it. Edit: Now that I looked at the link, this is a basic JS feature, there is really no need to reference SO here. However, the above still holds true. I'd ask you to review the whole PR at this point and identify any code that has been copied from somewhere else, so we can investigate whether we can accept it. |
||
const name = value[k].name || value[k].constructor.name; | ||
if (name && name != "undefined") { | ||
if (seen.includes(name)) { | ||
output[k] = value[k]; | ||
continue; | ||
} else { | ||
seen.push(name); | ||
} | ||
} | ||
} catch (e) { | ||
// Support anonymous functions by hashing the function body, | ||
// preventing infinite recursion in the case of circular references | ||
if (value[k] instanceof Function) { | ||
const funcString = value[k].toString(); | ||
if (seen.includes(funcString)) { | ||
output[k] = value[k]; | ||
continue; | ||
} else { | ||
const hash = cyrb53(funcString); | ||
seen.push(hash); | ||
} | ||
} | ||
} | ||
output[k] = encode(value[k], disallowObjects, forcePointers, seen, offline, counter, initialValue); | ||
} | ||
return output; | ||
} else { | ||
return value; | ||
} | ||
|
||
return value; | ||
} | ||
|
||
export default function ( | ||
value: mixed, | ||
disallowObjects?: boolean, | ||
forcePointers?: boolean, | ||
seen?: Array<mixed>, | ||
offline?: boolean | ||
offline?: boolean, | ||
counter?: number, | ||
initialValue?: mixed | ||
): any { | ||
return encode(value, !!disallowObjects, !!forcePointers, seen || [], offline); | ||
return encode(value, !!disallowObjects, !!forcePointers, seen || [], !!offline, counter || 0, initialValue || value); | ||
} |
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.
We cannot accept this algo (or derivations thereof) due to its problematic usage rights situation. The author has claimed to have made this "public domain", but such a claim is invalid in some jurisdictions. I have not found an OS license attributed to this code by the author. If you have found that anywhere, please provide the link.
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.
Would this solution be satisfactory?
bryc/code#23 (comment)
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.
Thanks for the effort, but unfortunately the underlying issue is still unaddressed. I appreciate the author's rooting for public domain contributions. However, the issue is a legal one. Unless there's an official OS license attributed to the code, we cannot accept it. We also cannot accept any "self-written", non-official OS license on similar grounds.
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.
@mtrezza is the MIT license addition satisfactory?
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.
Yes, but SO is not sufficient as source or link to the author. Has this been published anywhere on GitHub, and can this be added as a dependency? Because if we add code like this it won't be maintained by the author and the maintenance is on us. I'm not sure whether we should maintain this piece of code if there are libs for hash functions out there that are regularly maintained.
Why is this hash code even required in this PR? Are there no native Node hash functions we can use instead? This looks odd.