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: Fail on infinite recursion in encode.js #2099

Open
wants to merge 10 commits into
base: alpha
Choose a base branch
from
21 changes: 21 additions & 0 deletions src/CryptoUtils.js
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

Check failure on line 3 in src/CryptoUtils.js

View workflow job for this annotation

GitHub Actions / build (Node 18, 18.19.0)

Invalid JSDoc tag name "ref"

Check failure on line 3 in src/CryptoUtils.js

View workflow job for this annotation

GitHub Actions / build (Node 20, 20.10.0)

Invalid JSDoc tag name "ref"
* @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);
};
Comment on lines +1 to +21
Copy link
Member

@mtrezza mtrezza Apr 12, 2024

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.

Copy link
Author

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)

Copy link
Member

@mtrezza mtrezza Apr 12, 2024

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.

Copy link
Author

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?

Copy link
Member

@mtrezza mtrezza Apr 13, 2024

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.

2 changes: 1 addition & 1 deletion src/ParseOp.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ export class SetOp extends Op {
}

toJSON(offline?: boolean) {
return encode(this._value, false, true, undefined, offline);
return encode(this._value, false, true, undefined, offline, 0);
}
}

Expand Down
16 changes: 16 additions & 0 deletions src/__tests__/ParseObject-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@ const ParseObject = require('../ParseObject').default;
const ParseOp = require('../ParseOp');
const RESTController = require('../RESTController');
const SingleInstanceStateController = require('../SingleInstanceStateController');
const encode = require('../encode').default;
const unsavedChildren = require('../unsavedChildren').default;

const mockXHR = require('./test_helpers/mockXHR');
Expand Down Expand Up @@ -3862,4 +3863,19 @@ describe('ParseObject pin', () => {
});
CoreManager.set('ALLOW_CUSTOM_OBJECT_ID', false);
});

it('handles unsaved circular references', async () => {
const a = {};
const b = {};
a.b = b;
b.a = a;

const object = new ParseObject('Test');
object.set('a', a);
expect(() => {
object.save();
}).toThrowError(
'Traversing object failed due to high number of recursive calls, likely caused by circular reference within object.'
);
});
});
9 changes: 9 additions & 0 deletions src/__tests__/encode-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -183,4 +183,13 @@ describe('encode', () => {
str: 'abc',
});
});

it('handles circular references', () => {
const circularObject = {};
circularObject.circularReference = circularObject;

expect(() => {
encode(circularObject, false, false, [], false);
}).not.toThrow();
});
});
88 changes: 60 additions & 28 deletions src/encode.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

@mtrezza mtrezza Apr 13, 2024

Choose a reason for hiding this comment

The 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);
}
31 changes: 20 additions & 11 deletions src/unsavedChildren.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import ParseFile from './ParseFile';
import ParseObject from './ParseObject';
import ParseRelation from './ParseRelation';

const MAX_RECURSIVE_CALLS = 999;

type EncounterMap = {
objects: { [identifier: string]: ParseObject | boolean },
files: Array<ParseFile>,
Expand Down Expand Up @@ -48,8 +50,20 @@ function traverse(
obj: ParseObject,
encountered: EncounterMap,
shouldThrow: boolean,
allowDeepUnsaved: boolean
allowDeepUnsaved: boolean,
counter: number = 0
) {
counter++;

if (counter > MAX_RECURSIVE_CALLS) {
const message = 'Traversing object failed due to high number of recursive calls, likely caused by circular reference within object.';
console.error(message);
console.error('Encountered objects:', encountered);
console.error('Object causing potential infinite recursion:', obj);

throw new Error(message);
}

if (obj instanceof ParseObject) {
if (!obj.id && shouldThrow) {
throw new Error('Cannot create a pointer to an unsaved Object.');
Expand All @@ -60,7 +74,7 @@ function traverse(
const attributes = obj.attributes;
for (const attr in attributes) {
if (typeof attributes[attr] === 'object') {
traverse(attributes[attr], encountered, !allowDeepUnsaved, allowDeepUnsaved);
traverse(attributes[attr], encountered, !allowDeepUnsaved, allowDeepUnsaved, counter);
}
}
}
Expand All @@ -75,16 +89,11 @@ function traverse(
if (obj instanceof ParseRelation) {
return;
}
if (Array.isArray(obj)) {
obj.forEach(el => {
if (typeof el === 'object') {
traverse(el, encountered, shouldThrow, allowDeepUnsaved);
if (Array.isArray(obj) || typeof obj === 'object') {
for (const k in obj) {
if (typeof obj[k] === 'object') {
traverse(obj[k], encountered, shouldThrow, allowDeepUnsaved, counter);
}
});
}
for (const k in obj) {
if (typeof obj[k] === 'object') {
traverse(obj[k], encountered, shouldThrow, allowDeepUnsaved);
}
}
}