Skip to content

Commit

Permalink
All: Fixes #3334: Prevent notebook to be the parent of itself
Browse files Browse the repository at this point in the history
  • Loading branch information
laurent22 committed Jun 7, 2020
1 parent 5be8c2c commit f36e0c2
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 1 deletion.
7 changes: 6 additions & 1 deletion CliClient/tests/models_Folder.js
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,6 @@ describe('models_Folder', function() {
}));

it('should recursively find folder path', asyncTest(async () => {

const f1 = await Folder.save({ title: 'folder1' });
const f2 = await Folder.save({ title: 'folder2', parent_id: f1.id });
const f3 = await Folder.save({ title: 'folder3', parent_id: f2.id });
Expand Down Expand Up @@ -218,4 +217,10 @@ describe('models_Folder', function() {
expect(sortedFolderTree[1].children[0].id).toBe(f5.id);
expect(sortedFolderTree[2].id).toBe(f6.id);
}));

it('should not allow setting a notebook parent as itself', asyncTest(async () => {
const f1 = await Folder.save({ title: 'folder1' });
const hasThrown = await checkThrowAsync(() => Folder.save({ id: f1.id, parent_id: f1.id }, { userSideValidation: true }));
expect(hasThrown).toBe(true);
}));
});
9 changes: 9 additions & 0 deletions ReactNativeClient/lib/models/Folder.js
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,11 @@ class Folder extends BaseItem {
const folder = foldersById[parentId];
if (!folder) break; // https://github.com/laurent22/joplin/issues/2079
folder.note_count = (folder.note_count || 0) + noteCount.note_count;

// Should not happen anymore but just to be safe, add the check below
// https://github.com/laurent22/joplin/issues/3334
if (folder.id === folder.parent_id) break;

parentId = folder.parent_id;
} while (parentId);
});
Expand Down Expand Up @@ -403,6 +408,10 @@ class Folder extends BaseItem {
if (!('duplicateCheck' in options)) options.duplicateCheck = true;
if (!('reservedTitleCheck' in options)) options.reservedTitleCheck = true;
if (!('stripLeftSlashes' in options)) options.stripLeftSlashes = true;

if (o.id && o.parent_id && o.id === o.parent_id) {
throw new Error('Parent ID cannot be the same as ID');
}
}

if (options.stripLeftSlashes === true && o.title) {
Expand Down

2 comments on commit f36e0c2

@tessus
Copy link
Collaborator

@tessus tessus commented on f36e0c2 Jun 7, 2020

Choose a reason for hiding this comment

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

I think we need a more sophisticated check here. In fact no ID (notebook or note) that is under the current id can be the parent id.

@tessus
Copy link
Collaborator

@tessus tessus commented on f36e0c2 Jun 7, 2020

Choose a reason for hiding this comment

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

Additionally, due to a perf impact, this check should only happen, if this function was called from the API.

Please sign in to comment.