Description

An annoying issue has been cropping up in the Discord where editor sessions are impossible to create. Turns out, somehow a group with a space in the name got created. I've been able to find one code path to do so (the applyEdits command), but given that required some manual modification of the applyedits bytebin, I suspect at least one other code path exists that's more likely for a user to stumble across (the API perhaps?).

Reproduction Steps

For the applyEdits method:

1. Create a editor session
2. Create a group in the editor (i.e. test-group) and give it any node
3. Click the save button and go to the corresponding bytebin link
4. Copy the output, modify the group name to have a space in it (i.e. test group), and re-upload to bytebin
5. Apply edits with the new bytebin code

Expected Behaviour

LP should throw an error for any code path that attempts to create a group with a space in the name.

Additionally, as this will cause any attempts to create an editor containing the group to error, among other things, LP should check all groups at startup for spaces and do some combination of throw an error or change the names.

Server Details

git-Paper-510 (MC: 1.19.4)

LuckPerms Version

v5.4.71

Logs and Configs

No response

Extra Details

No response

The error in question (see image) is because a node is malformed (in this particular case, an InheritanceNode with an invalid group name), and trying to load it from storage fails, not group names themselves.

These are the few places I've gathered do not check for group name validity when creating a new group for saving:

• API GroupManager::modifyGroup(String, Consumer<Group>)
• Backup importer from /lp import
• Web editor changes, although the editor already doesn't let one input whitespaces in the name when creating a new group

But it is still possible to load one from storage if the data store is tinkered with.

In hindsight, after PRing #3606 and (privately) acknowledging there was no proper error handling when importing data from the editor and loading from storage before it was merged, having added guards against the same exceptions I introduced is an obvious idea.

@emilyy-dev do you have an idea for a fix?

do you have an idea for a fix?

I was thinking about gracefully ignoring/skipping invalid nodes and logging a warning(/error?) about their existence (pointing out key and holder).
I did consider existing setups with invalid nodes when making that PR (although these would be among the minority), but I can't think of a way to retain behavioural compatibility from before that change was made by including them somehow (without making it unnecessarily complex, like introducing a "raw node".) What do you think?