Replace spaces in username with _

[bundabrg]

Bedrock allows spaces but Java Edition does not (or should not). Bad things happen when a user logs in with a space.

This PR simply replaces spaces with _'s. This has the following consequences:

1. Potentially opens a vulnerability for a user with an _ that another user with a space (or vice versa) could log in and be treated as the same user. I'm not sure if Microsoft already disallow registering a name with an space replaced by an underscore.

2. It may cause other unintended effects so will need testing. One way to test if you don't have a space in your name (because why would you) is to just replace another letter.

It may also be better to move the replacement logic to where MinecraftProtocol is instantiated inside GeyserSession::authenticate so that the replaced username doesn't reach floodgate.

Closes #263

ToDo

• Test that it works with floodgate

[Heath123]

This should probably be in Floodgate because that handles the usernames

[Tim203]

Not everyone is using Floodgate and offline servers without Floodgate should still be able to use Geyser

[RealTriassic]

This must be in the standalone, as it's important, not only floodgate.

[Heath123]

It has to be in both
Geyser shouldn't send the wrong username to Floodgate
Converting the username is Floodgate's job

[Tim203]

Floodgate will still have the correct name even if this is merged.

[Heath123]

Couldn't that break things relying on the Bedrock username and confuse contributors by splitting the username logic across 2 different codebases?

[Heath123]

Oh, never mind, I think I misunderstood you
It needs to be in Floodgate too thiugh

[Tim203]

Yes, it'll be in Floodgate too after account linking is merged

[Redned235]

Bedrock allows spaces but Java Edition does not (or should not). Bad things happen when a user logs in with a space.

This PR simply replaces spaces with _'s. This has the following consequences:

1. Potentially opens a vulnerability for a user with an _ that another user with a space (or vice versa) could log in and be treated as the same user. I'm not sure if Microsoft already disallow registering a name with an space replaced by an underscore.
2. It may cause other unintended effects so will need testing. One way to test if you don't have a space in your name (because why would you) is to just replace another letter.

It may also be better to move the replacement logic to where MinecraftProtocol is instantiated inside GeyserSession::authenticate so that the replaced username doesn't reach floodgate.

Closes #263

1. If you're not using Floodgate, regardless this vulnerability will exist with or without this pull request merged. In order for this to carry over to Floodgate too, you'll need to instead modify the set name in LoginEncryptionUtils. This would allow us to handle all of this within Geyser itself and not have to do anything separate inside of Floodgate.
2. The only intended effect I can think of is something on the end of Floodgate, however this would only really have unintended side effects for plugins still using usernames to store player data to disk (e.g. Towny). UUID will not change, so this is not a large problem. For no reason should plugins be storing data to disk with player usernames anymore, so something like this should not be a major problem. However, I am not opposed to adding a new configuration option to reenable this. Lemme know what you guys think.

[ghost]

Juse make an option if to be enabled or not in config and everyone can enable or not lol

[bundabrg]

It's still a question of where is best to apply it since I'm not 100% sure my position is best. I'll take a look at where @RednedEpic mentioned to apply the change and am dropping this to WIP status for the moment.

EDIT: After looking at that location I'm not sure it's ever called if you are not running flood gate. Ideally what you want is to log into the Java server with the translated name but then send the original name to any authentication system. So maybe changing the username here:

Geyser/connector/src/main/java/org/geysermc/connector/network/session/GeyserSession.java

Lines 188 to 190 in 9b487d7

	protocol = new MinecraftProtocol(username, password);
	} else {
	protocol = new MinecraftProtocol(username);

This way floodgate gets the original and only the java protocol has the change.

[WesleyVanNeck]

I think this causes the chunks to not load

[WesleyVanNeck]

And I agree with @NycuRO

[rtm516]

This has been merged into Floodgate, so this PR can be closed.
GeyserMC/Floodgate#15

[bundabrg]

What if one doesn't run floodgate?

[Tim203]

Like I said earlier, this shouldn't be only in Floodgate.

[RealTriassic]

Bring this to master and inventory too >:(

[Camotoy]

What's the update on this PR?

[bundabrg]

I've been using it fine since the beginning as I have players with spaces in the their names. I don't know how it plays with floodgate that's the only thing.

[rtm516]

Floodgate has this, this was talked about a while back on where its best implemented and we just left it as part of floodgate for now.

[Camotoy]

Are people able to join Geyser if they have a space in their name?

[bundabrg]

It feels like it should not require floodgate.

[rtm516]

Are people able to join Geyser if they have a space in their name?

Yes, but it causes issues with some plugins

[bundabrg]

AuthMe for one will reject connections but tonnes of plugins can't handle it.

[Redned235]

Can there be a config option for this?

[bundabrg]

I think a config option would be a requirement. Or this can be moved to a plugin, though it feels like a core feature.

[bundabrg]

1.16 version

[Camotoy]

This will be closed, as it's a better fit for an extension in the future.

[Camotoy]

After some discussion, we decided this would be better in Geyser itself.

[martinambrus]

In my experience - on my offline Paper server with LoginSecurity (not sure which of these 2 force this behavior), when a user tries to log in with a space in username, they are not allowed to do so and are disconnected with message that their username contain an invalid character.

Wouldn't it be better to follow this princinple instead of trying to convert usernames from bad characters to good ones, which indeed opens a whole new world of possible consequences?

[bundabrg]

The issue is that if you have a space in your bedrock username you can't ever log into the paper server then through geyser unless you remap it.
However I may personally move this to an extension so I don't need to maintain it out of tree ;) and if its handled by geyser one day I can just stop using the extension.

[Camotoy]

This PR does need a rebase.

[valaphee]

As a reminder, underscore is a valid user name character this means people can have the same name i.e. "Vala phee" (which would be "Vala_phee") and "Vala_phee", should be replaced with an invalid character like +, Vala+phee

[Greaper88]

This is currently handled by floodgate.

As for @Camotoy's comment, I'm happy to see that because the space issue is a big deal for the Public GeyserConnect's users.

[Redned235]

Coming back to this after some time - this is something that exists within Floodgate & we don't really have much interest bringing this across to mainline Geyser, since the only setups it would primarily benefit is offline mode which we do not support. Thanks for the PR though!