TeleportCause forced to PLUGIN when setting back a player may affect other plugins

[stevmei]

I'm using build 1066 and a lot of players are teleported back to the ground when running trough the world. I guess this is caused by SurvivalFly violations, because disabling this check seems to fix the issue. That teleport causes a new last-teleport-location and the command /back doens't make sense any more...

---- Version information ----

Server

git-Paper-1076 -MC: 1.11.2- detected: 1.11.2

NoCheatPlus

Plugin: 3.15.0-SNAPSHOT-sMD5NET-b1066 MCAccess: 1.11-1.11.2 / Spigot-CB-1.11_R1 Features: blocks: BlocksMC1_4 | BlocksMC1_5 | BlocksMC1_6_1 | BlocksMC1_7_2 | BlocksMC1_8 | BlocksMC1_9 | BlocksMC1_10 | BlocksMC1_11 checks: FastConsume | Gutenberg | HotFixFallingBlockPortalEnter | AttackFrequency | FlyingFrequency | KeepAliveFrequency defaults: pvpKnockBackVelocity packet-listeners: UseEntityAdapter | MovingFlying | OutgoingPosition | KeepAliveAdapter | SoundDistance Hooks: AllViolations-NCP- 1.0 | Citizens2-cncp- 2.0 | mcMMO-cncp- 2.3

Related Plugins

CompatNoCheatPlus v6.6.3-SNAPSHOT-sMD5NET-b87 | ProtocolLib v4.2.1-SNAPSHOT-b347

This is always happening but some players have to jump around.

No changes, default SurivalFly values. config.yml: https://pastebin.com/B6mw18Yz

Some logs:

17-03-31 08:20:06 [INFO] name1 failed SurvivalFly: tried to move unexpectedly. VL 145.
17-03-31 09:54:05 [INFO] name2 failed SurvivalFly: tried to move unexpectedly. VL 200.
17-03-31 09:54:22 [INFO] name2 failed SurvivalFly: tried to move unexpectedly. VL 104.
17-03-31 10:04:42 [INFO] name3 failed SurvivalFly: tried to move unexpectedly. VL 18.
17-03-31 10:07:43 [INFO] name3 failed SurvivalFly: tried to move unexpectedly. VL 71.
17-03-31 10:17:00 [INFO] name4 failed SurvivalFly: tried to move unexpectedly. VL 41.
(...)
17-03-31 10:53:48 [INFO] name5 failed SurvivalFly: tried to move unexpectedly. VL 156.
17-03-31 10:58:16 [INFO] name6 failed SurvivalFly: tried to move unexpectedly. VL 145.
17-03-31 10:58:34 [INFO] name7 failed SurvivalFly: tried to move unexpectedly. VL 121.
17-03-31 10:59:07 [INFO] name6 failed SurvivalFly: tried to move unexpectedly. VL 145.
17-03-31 11:01:01 [INFO] name6 failed SurvivalFly: tried to move unexpectedly. VL 145.
17-03-31 11:06:08 [INFO] name5 failed SurvivalFly: tried to move unexpectedly. VL 145.
17-03-31 11:06:42 [INFO] name5 failed SurvivalFly: tried to move unexpectedly. VL 145.
17-03-31 11:10:52 [INFO] name8 failed SurvivalFly: tried to move unexpectedly. VL 145.

And my configuration (some checks are deactivated, the rest is default): https://pastebin.com/B6mw18Yz

Does someone has any idea how to fix this without deactivating the whole check? Thanks!

[asofold]

Plugins that provide "/back" for every teleport any other plugin does, should be avoided, if you have any anti cheating plugins, because telelportation is what you have to do to set back players.

In fact it may break anti cheating: Move to an impossible to reach position, provided you don't get kicked for it, and use '/back' upon set-back to reach that location.

The '/back' command should be tied to side conditions, and with the Bukkit API you don't have that huge amount of cross-plugin-compatibility stuff built in. Plugins can't know which other plugin caused a teleport that follows an altered/corrected moving event - with NCP/survivalfly, the teleport happens after the moving event handling, because we have set a new move end point, because we don't teleport during moving event handling. The Bukkit API is lacking there, and i'm not sure what to do about it - the best/easiest thing to do would be to add a new plugin-friendly telepeport cause label for position correction.

Short story told in a lengthy way: Two topics we can cover here: A) Does your '/back' plugin allow to skip the teleport cause 'UNKNOWN' ?`Please check out the configuration - NCP should set back players with that cause set, because that's what the server does - we don't use 'PLUGIN', because other plugins may be fooled into assuming a legitimate teleportation feature and make stuff like '/back' work. If it doesn't, it's worth a feature request (or bug report, depending on interpretation/taste). B) Find the cause of survivalfly violations (doesn't help against cheaters abusing /back).

For B), we would need to encircle issues more, so we have an idea about what's happening there. So first pointers:

• Occasional false positives are hard to come by, given how few the client is telling about special conditions (and the range of moving-related features/specialties to support).
• Can anything more be said about side conditions under which issues arise? Is it during fighting, sprinting, parkour, climbing, with potions or special items, armor weight plugins ....anything specific known?
• Violation logs: Is there a reason for not having [tags] in the strings messages for survivalfly violations (i think it was flylong/flyshort)? Those would show more details about what happened. It's possible to configure the log message that's appended to the log file differently, but it makes the action definition more complicated. If all people remove the [tags] for ingame display, i'll probably make such a new default, so the files always show more details by default and ingame display shows less - but balancing that in a more nice to configure way will need some more work.
• If issues can be reproduced in a systematic way, you could consider on-the-fly debug logging a player while they are reproducing it: https://github.com/NoCheatPlus/Docs/wiki/Debugging#on-the-fly-debug-output-for-individual-players (Don't hesitate to zip and upload here - may replace name(s) ips, if wanted. )

[stevmei]

Thanks @asofold for your answer.

I've tested some reproducings and found out: It is not SurvivalFly alone, that teleports back. As you said, every moving violation is canceled by teleporting / set the player back.

Within the last 12 months, there were no problems (not this count of problems), but I guess this is caused by some huge changes in the client-server-communication with 1.11. I guess, that's why so much violations are happening.

I'm using Essentials (EssentialsX) to provide a /back command. For the worst case, it is possible to deactive register-back-in-listener so the /back is not changed due to some other plugins teleports. But acutally, I'm going to leave this enabled (because of some server-mechanics and events that are based upon this configuration (players can use /back after using a warp or lift sign, ...)).

What I've done now is: I've remove some cancel actions before a vl>x is reached. So no player is teleported back until he got a cetain VL. It seems to be a lot better now, And I know, this is not the best solution, because it can be abused by hackers to glich or something.

My new config.yml: https://pastebin.com/rf4ZLcdf (changes are commented, default is commented out)

You said NCP uses a teleport cause UNKNOWN, but Essentials is just listening to PLUGIN and COMMAND: EssentialsPlayerListener.java That doesn't make any sense...

Edit Are you sure that you don't use the cause PLUGIN? I've found a lot of them in the NCP source code, here an example: MovingListener.java

[asofold]

Hehe/sorry :) - i wrote much text...

The TeleportCause.PLUGIN for the "enforce location" feature has to be regarded as a bug, but the feature should be disabled with recent MC versions, as it's lots of legacy. Will address this next build (won't change issues with survivalfly, though).

The ordinary set backs are UNKNOWN teleport cause, because they result of the server teleporting the player after PlayerMoveEvent handling, due to NCP setting PlayerMoveEvent.setTo(newTo). So if EssentialsX only reacts towards TeleportCause.PLUGIN, then this should not be related to the issues. Can't judge what else happens with /back ...

So the good news is, that we are most likely only dealing with ordinary false positives.

• Note that not cancelling low vls may lead to some kind of gliding/jumping exploits, as it's possible to reduce the current vl with "legitimate moving", so they probably can cheat and then reduce vl and so on - there should exist cheats for that. Amount/impact depends on settings. Perhaps a testing client could be made :), where we allow consuming the config and it attempts to adapt to it for some potential cheats, but that's another job for someone.
• If you don't/can't do on-the-fly debug logging, could you temporarily activate allviolations logging?

Allviolations logging:

• logging.extended.allviolations.backend.trace set to true
• (logging.extended.allviolations.debugonly must be kept at false.)

This would log all violations to the log file, which can be a lot, but it'll be much less than overall debug logging. Test it shortly or monitor file size though :), to get an impression how it behaves (including impact on performance). Under extreme load, NCP would skip messages (including violations), but i can't judge if the logging queue at so and so size is "a lot" for your hardware, probably not. We just don't have feedback for this feature being used on 500 player nodes and similar, so this depends on the amount of violations and disk-io speed.

Looking at the violation logs, they appear like once in several minutes, so if this was with 500 players, it would be a too good average, but i assume less players were online during the displayed period (or not all have been extracted to show here).

• Are any game breaking / systematic issues reported, or is it just random/unspecific?
• Is there an issue with the /back command?

(EDIT: I overlooked the fact, that very small violations don't get logged to file at all, so there could be more small set backs. So i need more details on what/where/how-often - allviolations is a good indicator, as it logs all available details for all violations.)

[stevmei]

Once again, much text 😀

I've used allviolations logging so far, but without trace.

First, Before removing cancel actions: Pastebin Second, after removing cancel actions: Pastebin

Both logs are the same weekday and same hour, so there were a similar amount of players (~40). We have a maximum of 64 players.

I'm going to build my own EssentialsX with some debug-outputs for the TeleportCase behaviour. Maybe I can see why EssentialsX is handling the NCP teleports and changing the last teleport point for /back...

[asofold]

Wait a second. The plugin teleport cause may have the reason, that spigot is using TeleportCause.PLUGIN now, which leads to this. Perhaps it's been a while, not sure. So in this case we have a general incompatibility. Note that ncp (on-the-fly) debug logging prints out the teleport cause.

So i'll have to revert/change switching to UNKNOWN for the other cases, unless server and/or NCP get implemented differently.

Now i can think of two potential ways to deal with things:

1. Pull request (after doing the math): add TeleportCause.CORRECTION_OF_POSITION. Since people might want to distinguish server ("moved too quickly" / "moved wrongly" ) from plugins, this probably should be PLUGIN_CORRECTION_OF_POSITION, and later/extra: SERVER_CORRECTION_OF_POSITION . Downside is that this isn't compatible (out of the box) with servers and networks running on 1.7.x with multi protocol support. This also needs changing PlayerMoveEvent to either automatically use the reason, or to set a specific one - leaving open if it should be setTo, or setCancelReason and setCancelTeleportToPosition (+- naming), having a few options here.
2. Alter the way NCP does set backs to "multi staged". First cancel the player move event, and at the same time schedule a teleport with the desired TeleportCause.UNKNOWN. Upside is backwards compatibility with MC, less intrusion to the Spigot server code, and is more consistent in terms of "the event got cancelled for a reason" and the set back being an explicit teleport code-wise. Downside is that it might break a thousand things in plugins that rely on ncp as it used to be, be difficult to do in a consistent way with all the other events that might happen in-between, might demand extra caution with events that arrive before the teleport actually happens, such as vehicle-enter, forgot the other thing i intended to add in here. This also needs more/longer tracking of the set-back position, until the teleport actually happens (cancelled or not) - not to forget that cancelling the moving event will cause a teleport to the original PlayerMoveEvent.getFrom() location, which can not be overridden by plugins (!).

Currently i think the second method is most likely to get us further, it could also simplify our code by a good bit, if the set-back is an explicit teleport wrapped by NCP plugin code, instead of firing from within server code, though we still have to deal with plugins teleporting players or cancelling set backs or overriding the set-back location.

So for /back there currently is no remedy, also cheat-wise.

Concerning the violation logs: I can't see the allviolations log, perhaps it's a bug or it isn't set up right? There should be entries like [VL] ...

[stevmei]

Thanks, I was unable to build my own EssentialsX fork due to a lot of unavailable depencies (connection time out while downloading depencies in the maven build / install process.. nargh!). I'm not going to download / search them all until I have to...

But, like you said, NCP prints out the TeleportCause. So I don't have to debug this on the EssentialsX side.

This is a lot of information and thats a bit too complicated for me (besides my english is not the best). But if I've understand this correctly, Spigot actually uses the TeleportCause PLUGIN, when NCP is trying to set back a player?

If so, that's the reason why EssentialsX does handle it, just as it should.

[stevmei]

Ok, found a upstream / nms patch: Spigot / Craftbukkit PlayerConnection.patch

[asofold]

New Anser: Lol. "24 Mar 2017" looks "recent".

Old Answer:

Sorry, you don't need to read that all. Since someone who might be interested in the implementation might follow, i put that in.

Spigot/CraftBukkit uses TeleportCause.PLUGIN if a plugin alters the end point of a move. We do that because cancelling a PlayerMoveEvent doesn't give us the control over the position, that the player will be teleported to by the server, after handling of the PlayerMoveEvent. The server teleports to "where the player was last", which neither is a desirable position to set back to (depending on policy ~ e.g. prefer the last-ground-pos), nor can it be controlled by plugins (using Bukkit). Thus NCP used to set a new move end-point instead of cancelling the event, to avoid the potential complexities with waiting for a scheduled teleportation. Teleporting from within the PlayerMoveEvent handling is not nice to have (despite technically being possible - NCP accounts for other plugins doing so, but very often plugin developers don't account for the possibility at all, leading to a lot of potential for trouble, if we do it).

"as it should" <- Matter of taste. The Bukkit API allows plugins to get/alter all sorts of events, but it (more or less surprisingly) doesn't really do much about cross-plugin compatibility, and "a plugin caused a teleport" is not really a reliable information, if you account for region protection plugins preventing entering regions, anti cheating plugins preventing moving and the like.

With cancelling moving events and then scheduling the teleport with TeleportCause.UNKNOWN, compatibility would likely be possible, still the position correcting plugins have the short end with the ominous UNKNOWN teleport reason, which also is used by teleports the server may cause, not being able to let other plugins know what is happening. Given how things are with region protection and anti cheating, distinction clearly is desirable. Bukkit could enable plugins to set a different TeleportCause and position for cancelling moving events.

(Likely i'll switch to cancel + teleport, can't tell how simple/fast, as it has much potential of causing trouble withing NCP, that'll need endless testing.)

(If you can, you could alter/revert that patch and compile spigot that way, for the fastest way "back".)

[stevmei]

OK, thanks. So there's a lot of work to do?

As all the /back issues don't belong to the count of voilations I'm going to rename that issue.

All the voilations are okay if they just set back a player and don't touch their /back location (once again like in old times). But acutally I'm going to leave this config like it is (without that first cancel) on my own risk. If there is any possibility of changing the TeleportCause to something other than PLUGIN or COMMAND, then I can reactivate the default configuration and everything works fine again.

Yea, that event-handling by plugins and the possibility to set back a player is very complicated and Spigot / Bukkit / CraftBukkit does not allow to go further than canceling and make some causes...

Edit Reverting this patch on my server is not recommented due to some changes with other plugins. Like I said, disabling some set-backs is actually the best way for me to handle this issue. But for the furure all the set-backs should be reactivated and I should adjust the whole configuration for our server (due to so much voilations I guess I have to give all the players a little more space to move and so on...)

[asofold]

I'll see what i can do (swiftly?).

[stevmei]

Thank you for your work and that great plugin!

This doesn't have to be fixed within hours, some days or even weeks are okay. I guess some other servers may have the same issue, because NCP and Essentials / EssentialsX are common plugins and all of these servers are now affected with this issue and I guess some players can't stop complaining about their "back position" when loosing it because of a server lag or some small voilations 😀

Edit Oh, and NCP is a very common plugin too 😄

[asofold]

I should have a look at the commits in spigot more often, there is very useful things in there :) - thank you for the report!

Edit: I rate this as pretty important, because the API is all/most people have, so either i'll manage to get towards PR comparable quickly, or i'll do the cancel+schedule thing. Neither is bad, though the cancel+schedule thing potentially is heavy on testing and impact, in comparison. In order to not have people wait too long perhaps that's still the way to go, because i hardly can tell the spigot team to decide about a PR/ticket within so and so time. So i'll likely seek contact to devs and then decide which way to go. After all the cancelling line appears to be most consistent, despite not necessarily resolving "my issues".

[asofold]

Build 1067 contains the change towards cancelling moving events and use a scheduled teleport for set back. It's to be regarded as [BLEEDING], possibly instable, due to not being able to test a lot.

[stevmei]

Thanks, I'm gonna give it a try this week :)

[asofold]

Currently build 1080 contains the most relevant state - previous builds contain bad actions for survivalfly and creativefly.

[asofold]

Closing as fixed, feel free to comment here on whatever may be the case on your setup (things appear to differ amongst mods+versions....).

[stevmei]

Ok, sorry, due to some personal timing problems I wasn't able to test it until now. I'm going to test it within this month :)

Thanks again for your great work!

[asofold]

No problem, thank you :) - there are a couple of more or less distantly related tickets (some of which are still) open, and i'm quite confident that my own debug logs with the latest build are similar to what happens on other versions. So take your time...

[stevmei]

Ok, I'm running build 1084 now. I have the whole default configuration.

I'm having some set backs due to some lags, but there is no information in the trace log.

Sadly, this changed my /back position. But I don't know if this is caused by NCP because:

• The trace log does not have an entry with my name
• No "moved too quickly" or "moved wrongly" appeared in the server log
• I'm having the '*' permission, so I should be exempted from every check

But I was set back and my /back point changed. md_5 told me that Spigot does not use the TeleportCause PLUGIN when a "moved too quickly" happens (see JIRA here).

This is just for your information. I'm going to give it some more testing and I'm going to disable NCP for some days and check if this issue still remains (if so, it is not related to NCP).

Actually I'm a bit confused because I was set back and there was no information in any log (server or NCP)...

Edit I'm not running Spigot, I'm running PaperSpigot, maybe they made some changes to the set back behavior? I don't know...

[asofold]

If you have full bypass permissions, despite nothing appearing in the server log and there is unknown teleport logged with debug on...

• It could still be the server simply putting you "out of a block"
• Another kind of bug. (to be researched if/where to increase amount of cases debug logged +- extra information...)

Other reports indicate such too... some cases with stairs could just be minecraft (with horses for sure), but never know...

Edit: Sure, Minecraft uses UNKNWON teleport reason - does PLUGIN still appear in the log !?

[stevmei]

Because this only happens one or two times a day I don't have on-the-fly debug enabled, only the trace option. So there is acutally no information about the teleport cause for my set back.

This is very annoying because it is so hard to replicate...

[asofold]

uh yeah - i should think about options to log odd stuff or special triggers for temporary debug logging (+ extra rate limiting).

NCP would rate limit the logging queue at 5000 entries (but not make a difference between alerts and debug :p ) - so debugging all might not kill the server on strong hardware, but it'll also lead to stuff not being there that we might want to be there.

[stevmei]

Status report:

Seems to be a lot better now, running default configuration. Actually, no players are reporting issues with their back points. I'm giving this some more time to verify, but it seems to be fixed.

Nice work so far 👍