TPR can teleport outside worldborder

[metype]

When using TPR on the latest version of EssentialsX and Paper, I was teleported outside the worldborder far enough to where I was instantly killed and lost all my items as there was no way to retrieve them. I do believe this is an issue as that does not feel 'safe'.

[ToonTown0909]

Hello there, Metype! Would you be able to please fill out the full bug report format? This will help us resolve your issue as fast as possible, please and thank you! :D

[pop4959]

Can you provide more information? It would be extremely helpful if you can properly fill out the issue template, as @ToonTown0909 has pointed out. In addition to the usual versions, logs, and configs, also provide a paste of your tpr.yml file.

[PatoTheBest]

I believe what he is saying is that it is possible to teleport outside the world border with the random tp command.

The max-range (https://github.com/EssentialsX/Essentials/blob/2.x/Essentials/src/com/earth2me/essentials/RandomTeleport.java#L71) doesn't validate it is inside the world border, and in the case it is inside the world border range, it can end up being outside if the center isn't exactly in the middle of the world (https://github.com/EssentialsX/Essentials/blob/2.x/Essentials/src/com/earth2me/essentials/RandomTeleport.java#L44)

A solution would be to modify the isValidLocation method to also validate the location falls inside the world border.
https://github.com/EssentialsX/Essentials/blob/2.x/Essentials/src/com/earth2me/essentials/RandomTeleport.java#L189

[pop4959]

I've done some testing now and can confirm this can happen, when either the center is not at the center of the world border, or when the maximum range is increased past the world border radius. Although, it seems like it is more of a bug with Essentials safe teleportation system rather than random teleport. I think that the new command just makes this issue more apparent than it was prior.

Thank you @PatoTheBest for the analysis as well, although I have some comments to make about it:

The max-range doesn't validate it is inside the world border, and in the case it is inside the world border range, it can end up being outside if the center isn't exactly in the middle of the world

This was intentional, as the max range is simply a configuration value. In my opinion, validation is better done when selecting the random coordinate (to avoid picking an invalid one) or when teleporting (to move the player to a valid location).

A solution would be to modify the isValidLocation method to also validate the location falls inside the world border

That would probably work, although this still seems like more of a problem for safe teleportation to deal with, rather than location validation. The isValidLocation method is mainly for picking things out that could not otherwise be detected prior to generating a location, which would currently be if there is not a valid block at any height to teleport onto, or it is the wrong biome.

The main problem I see in this issue is that vulnerable players can be killed when teleporting. Safe teleportation checks if the player is vulnerable before getting a safe destination. This means for example, that if someone did want to teleport beyond the border for some reason, the command would still be able to do this given that either safe teleportation is disabled, or they are invulnerable (god mode, creative mode, etc).

That aside, this check being in safe teleportation would also fix accidental teleportation outside of the world border using other commands (such as /tppos, which I was also able to reproduce this being an issue with). It would also be a more effective solution than invalidation locations directly in the random teleport itself, as that would cause a lot of unnecessary failed location searches which could cause odd behavior in and of itself given the right circumstances, such as not being able to find any locations at all (given the world border may be very small, and the max range very large, for example, which can probably be binned as a common occurrence given the defaults). Again, the problem here is killing players. There shouldn't be any problem with moving the player from the random location to one inside the border if it's unsafe. The server owner will of course probably want to adjust the max range though, if players are being teleported to the world border constantly. 😛

[PatoTheBest]

I've done some testing now and can confirm this can happen, when either the center is not at the center of the world border, or when the maximum range is increased past the world border radius. Although, it seems like it is more of a bug with Essentials safe teleportation system rather than random teleport. I think that the new command just makes this issue more apparent than it was prior.

Thank you @PatoTheBest for the analysis as well, although I have some comments to make about it:

The max-range doesn't validate it is inside the world border, and in the case it is inside the world border range, it can end up being outside if the center isn't exactly in the middle of the world

This was intentional, as the max range is simply a configuration value. In my opinion, validation is better done when selecting the random coordinate (to avoid picking an invalid one) or when teleporting (to move the player to a valid location).

A solution would be to modify the isValidLocation method to also validate the location falls inside the world border

That would probably work, although this still seems like more of a problem for safe teleportation to deal with, rather than location validation. The isValidLocation method is mainly for picking things out that could not otherwise be detected prior to generating a location, which would currently be if there is not a valid block at any height to teleport onto, or it is the wrong biome.

The main problem I see in this issue is that vulnerable players can be killed when teleporting. Safe teleportation checks if the player is vulnerable before getting a safe destination. This means for example, that if someone did want to teleport beyond the border for some reason, the command would still be able to do this given that either safe teleportation is disabled, or they are invulnerable (god mode, creative mode, etc).

That aside, this check being in safe teleportation would also fix accidental teleportation outside of the world border using other commands (such as /tppos, which I was also able to reproduce this being an issue with). It would also be a more effective solution than invalidation locations directly in the random teleport itself, as that would cause a lot of unnecessary failed location searches which could cause odd behavior in and of itself given the right circumstances, such as not being able to find any locations at all (given the world border may be very small, and the max range very large, for example, which can probably be binned as a common occurrence given the defaults). Again, the problem here is killing players. There shouldn't be any problem with moving the player from the random location to one inside the border if it's unsafe. The server owner will of course probably want to adjust the max range though, if players are being teleported to the world border constantly. 😛

Of course the server owner must set up a valid max-range, but you cannot rely on them remembering to adjust if they change the border size.

I do agree with you that the isBlockUnsafeForUser (https://github.com/EssentialsX/Essentials/blob/2.x/Essentials/src/com/earth2me/essentials/utils/LocationUtil.java#L95) should also account for locations outside the world border. This should fix all teleports inside the plugin, however, I also feel like it would be a good thing to check if the location is outside the border when calculating it on the RandomTeleport. I don't see how caching or getting unsafe locations would be a good thing if they are going to end up being discarded.

[pop4959]

Of course the server owner must set up a valid max-range, but you cannot rely on them remembering to adjust if they change the border size.

Yes we can! 😛 If a config value is configured incorrectly (or not at all), you should not expect the feature to work properly either. This is universally true for other config in the plugin (if the server owner messes up, we do not try to auto correct it for them). If players are constantly teleporting to the edge of the world border instead of an actual random location, it's a huge hint to the server owner that they have something set up incorrectly. The defaults will work correctly, given that the center of the world border automatically becomes the center of the random teleport, and its radius = max range. There's no requirement for this to be at the world's spawn point, nor the center (0, 0) of the world.

[ShayBox]

Should also add max-range to the tpr config when generating it, nobody would know its a setting otherwise.

[pop4959]

@ShayBox As stated very clearly at the top of tpr.yml, the settings are mostly intended to be changed via /settpr and not directly in the file. The max-range value defaults to your world border unless set otherwise.

[ShayBox]

But it didn't get set to my world border, even after I re-set the world border, and /settpr sets the center of the tpr point... not max range

[pop4959]

But it didn't get set to my world border, even after I re-set the world border, and /settpr sets the center of the tpr point... not max range

@ShayBox So the problem here is that you can have a center point that is not at the center of your world border, and so if you did not set a max range that prevents the randomly selected location from going outside of the border, you can die. This will be addressed by #3536 which adds the world border to the teleportation safety mechanic.

You'll still want to set a correct center point and range however. This is done with /settpr center for setting the center point, and /settpr minrange and /settpr maxrange for setting the respective teleportation ranges.

Keep in mind that you technically do not have to change either if the center point is also the center of the world border. In this case the defaults work perfectly fine as they are.

If you have any further questions please ask on the EssentialsX Discord as this is not really the appropriate place.