This should be better in case the vanilla command allows UUIDs some day.
At least during testing in uuids did not work, so we added a warning
message if uuids are used for banning.
Using player names for actions should be safe anyway.
Access by id may or may not be changed internally, depending on what
happens on the API side.
For performance reasons we might like to do without passing on extra
stuff like Material, or even do String-comparison.
If ids will become more expensive to use than other ways, or if it will
be possible/accepted that ids get reused during runtime, then we will
have to change all internals.
A merge can happen if the maximum distance between any two points
exceeds the merge distance, because LocationTrace attempts to balance
distances entries. Thus this test must not bne able to walk further than
the merge distance.
For whatever reason it was commented out, it is now put back, assuming
that the reason was "it did not help with mcMMO", because mcMMO used to
fire extra BlockBreakEventS.
Now we use a tri-state and set it to MAYBE if instantBreak is set
already on lowest priority, to have a rough indicator if a plugin set
it, or if the server might have done it. Later we might let FastBreak
assume some maximal duration for either case, instead of ignoring these.
Fixes:
* On passable-violations useLoc might have been passed as newTo.
Reduce potential for accidental future misuse:
* Call useLoc.setWorld(null) as late as possible.
* Do not reset world of MoveInfo.useLoc in MoveInfo.set.
An implementation for NCPCompatBukkit is missing.
Note that the native access can not set lastYaw nor lastPitch in
PlayerConnection (private), but it could set lastYaw and lastPitch in
EntityPlayer.
Can't guarantee this actually helps with stuff like derp/magnet, because
we can not really set the outcome of a PlayerMoveEvent without
rubberbanding the whole planet. Hacks could send enough packets per tick
to keep freezing people - we might be able to keep track of yaw/pitch
correction and cancel (most) attacking for the same tick after yaw
correction (invalidate "same" with in-bounds yaw).
"Magnet" refers to a player seemingly freezing other nearby players onto
the spot, causing them to yaw-glitch, seemingly getting attracted by the
cheater.
Thanks again @Iceee for reporting and @Amaranth for finding cause +
suggested fix.
Not sure this both helps and prevents false positives, so it could get
redone a couple of times, might also result in being impossible to fix
with a plugin in an acceptable way.
- Never skip undoing changes. This is necessary to also process commands
that are not registered or that are fall-back aliases.
- Check adding all plugin commands always.
- Check for aliases as well for decision if to match a command.
These seem not to have been affected beforehand, due to the nature of
the SimpleCommandMap, however we don't want to exclusively rely on
fetching that one successfully.
Setting a set-back location with a null World will now lead to a
NullPointerException. Setting null worlds on set-back locations is not
intended by contract, however due to using temporary Location instances
with Entity.getLocation(Location) and setting the world to null
afterwards to prevent memory leaks, also considering the number of
places where setSetBack is used, this seems to be an appropriate measure
to track better or help excluding violations of that contract.
This does not change the used methods much, thus it will rather allow
much more cheating, however it allows some basic testing for false
positives. The reach implementation has been slightly optimized to run
faster. The current implementation is not final and only uses trace
elements that were added in the current tick, the latest is always
included.
Next steps will probably be:
* Stricter methods for an individual TraceEntry (demand near-exact hit).
* Don't allow random latency shifting. Maintain a window-thing.
* Probably get rid of the classic method for attacking other entities.
No change for the function of the plugin, except if there are new bugs.
Actual changes:
* Keep track of squared distance to the previous element in a trace.
* Balance mergeDist: prevent merging the latest entry, if the squared
distance from the latest to the second latest element is greater
than mergeDist. Never merge if there are only two entries in total.
* Add convenience methods for resetting and updating the trace.
* Maintain the moving traces actively. With intended use being
player-player interaction, we will not reset with every teleport.
Just the raw implementation + initial test cases. To keep memory use
constant, a ring-buffer with some maximal size will be used. The
iterators are meant for faster implementation, rather than fastest
iteration. Later some trigonometric functions could be added to
LocationTrace, depending on if that may gain a lot of performance.
Next we will add the logics for adding entries and resetting the trace
to NCP (moving, teleporting, joining), on Logout the trace must not stay
in MovingData but should be garbage collected. That should be a
milestone dev build, though it does nothing for the user, it might help
finding crash bugs :p.
Soon to follow will be changing some fight checks to be able to use the
moving trace, then alter them to actually use it. Fight and interact
checks could also do moving consistency checking (tp exploit).
Who reads this?
The cancelled BlockPlaceEvent will lead to an extra sign being dropped,
while the item in hand stays. Odd enough, the cactus is removed before
the BlockPlaceEvent - need to check if a ticket exists for CraftBukkit
already...
Now players are forced to create moving events with CraftBukit,
in order to get above the trigger-distance, thus moving checks can
complement this (in theory).
This could lead to more false positives.
This is a workaround for the case that sprinting events are missing,
wrongly set up or for events firing in an unusual order.
This does allow speeding hacks that allow players to go at sprinting
speed without telling the server, so it should only be turned on if
there is any issues.
The option has to be turned on: checks.moving.assumesprint
Refactored penalty time handling to use a PenaltyTime object, taking
into account time running backwards, also unify attack (close combat)
penalties to a generic attack penalty. Combined.yawrate still keeps the
timeFreeze penalty, due to also cancelling other actions than melee,
still changed to a PenaltyTime object.
Changing the item in hand now leads to an attack penalty (that also goes
for not changing the item, but changing the slot).
"Quick" addition, not much testing, except few unit tests.
Note that this could change false detection behavior of other sub-checks
of fight, because the penalty time is checked last. Previously checks
like direction or reach would have cancelled already if the player was
within their penalty time. Hard to say if this creates new false
positives, but it will be more strict on continuous violations.
This will probably not be pulled through for all checks, because
the overall design does not support to do this in an efficient way.
Some checks will be added to allow pinpointing data removal,
mainly to allow compatibility tweaks, e.g. with actions.
Roughly this line of develpment has to do with:
* Prevent destroying ones own vehicle [INCOMPLETE: conflict with older
MC versions.].
* More careful set-back handling.
* Reset the players position to that of the vehicle if the player moves
too far off (likely does not have effect, needs more testing). In
principle this is intended to trigger a teleport, the normal player is
intended to not notice, but no guarantees yet.
Not configurable yet, might not work 100%, yet.
There seem to be cases with a repeated horizontal speed increase which
should be covered by the bunnyhop mechanisms. The first increase will
not go as high as possible but have 0 y-diff, while the second one will
trigger a violation with a higher y-diff and another increas in the
horizontal distance. This commit does not cover a general multi-step
speed-increase case, but just attempts to catch this special case.
The command lists for handleaschat and exclusions are now also fed with
a leading "/" if missing. Allows more convenient setup and less
confusion potential for relating comamnd lists in "protect plugins" to
other lists.
opinconsoleonly has been made a list of commands:
protection.commands.consoleonly.
Further the message sent to the player is the permission message,
if the player does not have permission to use the command,
provided NCP can find the command.
All three command lists are also checked with the original
command label.
Commands to change to "no permission" or "unknown command" behavior,
can now be configured with a string list each. Commands that have a
permission set will have the default set to false, while commands that
don't have a permission will be altered to have a filter permission,
namely nocheatplus.filter.command.<commandname>.
* Fix color replacing in command protection (had no effect previously).
* Use "no permission" message for default bukkit command protection.
* Use command protection only, no more parsing pre process for /pl etc.
* Other tweaks (update descriptions, add shortcuts / child permissions).
Changes are mostly backwards compatible.
Fixed:
* Filter permissions were wrong (not starting with nocheatplus).
Changed:
* Commands are grouped under nocheatplus.command now.
* Notification permission is independent of the notify command,
changed to nocheatplus.notify.
New:
* Shortcut permissions (nocheatplus.shortcut...) for safer use.
* Shortcut permission for testers: nocheatplus.tester
This change addresses the fact that the bunny hop thing also applies
when not sprinting, also without actually jumping. In future the buffer
will be much more confined to special cases and/or limited in how it
regains level (nu pogodi).
* Currently bunny applies very often on sprinting, thus it can be pre-
checked before permchecks, and if necessary re-checked after
permchecks.
* Remove heuristic permchecks, since the above should do better.
* Fix bunny not trigger for assumeonground && jumpphase == 0.
Further we will try to get rid of the horizontal buffer as we know it,
confining it to special cases. However we have to extend bunny hop to
apply without sprinting as well. Might need a more general concept
for ground-move-modifiers and friction to achieve it.
Setting to something on creation of data is ineffective, because it gets
set to 0 on passable violations.
Keeping the buffer means other abuse potential.
Probably better postpone until re-design/removal of the buffer concept.
Might consider for future:
The problems of the type lost-sprint, bunny seem to be client mechanics.
The ground-modifiers apply and stay valid during jump, thus turning off
sprint for attacking makes sense.
So i revert the statement about "could be worth a ticket" for the
moment. However with looking forward to have a Minecraft API, it could
could be appropriate to provide something to allow server-side checking,
yet again finding the right ground modifier needs knowledge of the
exact moves the client did, so client side could change to send all the
transition events as suggested before.
Horizontal speed after-failure checks:
* More conditions to confine use more (on-ground, web/fluid/etc.).
* Increase bunny counter during bunny-fly until touching the ground.
* Check bunny fly first and ignore sprinting for that one.
* Attempt to fix "double bunny", i.e. small blocks on blocks.
* Redo part of checking order, do permission checks first, otherwise
there might happen unwanted invalidation of velocity.
* Force permission check with the first getting of hDistance on
certain conditions, to prevent certain redundant call.
* Remove code duplication by checking buffer regain after all other.
The usual case is that regain is checked anyway.
* Don't use buffer for bunnyfly. Makes tags more unique.
* Horizontal buffer is not modeled such that it can't become negative.
* hvel tag added for using horizontal velocity.
* hspeed tag is only added on hspeed violations.
Altered model for bunny-hop problem. We reduce the sprinting-modificator
by a lot, and add extra buffer during the now introduced "bunny fly"
phase. Conditions have been refined.
Certainly needs more testing, also regarding effects.
The problems with attacking + sprint/bunny seems to be that the player
moves on as if sprinting, thus we reformulate it as the "lost sprint"
problem. The sprintback check is skipped during lostsprint phases.
This should be worth a ticket for Mojang/CB, sprint + jump + attack
somewhere around touch down time (+-) - server side sprinting turns off,
but the player moves on as if sprinting.
Since the sprinting time has been fixed, it should not be necessary to
consume buffers/velocity, because it will not trigger at all.
(Context: throwing ender pearls at entities.)
Potential false positives are survivalfly violations with vacc[...].
* Touch down near ground (lost-ground miss).
* Slow falling after login/teleports.
* Further split off rather seldom triggering code parts into methods.
* Accounting does not manipulate the accumulator anymore.
* Remove some unused parameters form signature(s).
* Method renaming.
* Comment cleanups.
(Expect more with time...)
SetupOrder allows to define a priority, so you can register
ActionFactoryFactory instances before any checks get them.
Default priorities are NCP core at -100, DataManager -80, rest at 0.
Allows to set a new ActionFactoryFactory without the ConfigManager.init
already trying to parse actions, thus implementing special actions
should be easier. Still three calls might be necessary to be on the
safe side:
1. ConfigManager.setActionFactoryFactory(new factory)
2. ConfigManaher.setAllActionFactories()
3. DataManager.clearConfigs()
Not entirely convinced if concurrency issues might arise with reload or
even on startup (chat).
Hint at per-velocity friction factor. Could be used to add short term
velocity that does not decrease. However it can't replace extra-buffer
due to special invalidation conditions.
Move the after-failure permission-, velocity- and
extra-buffer-checks into an extra method.
Move special checks like web/liquid to extra methods.
Alter checking order slightly.
Other cleanups (comments, declaration order, grouping).
Shuffle methods.
Finished? No.
This reduces false positives with throwing enderpearls at nearby
entities or for horizontal velocity.
Missing: Use up horizontal velocity if sprintback is triggered and
queued velocity is present.
Jump (+run) and throw an ender pearl at a nearby entity.
Survivalfly might trigger with limitjump, since the MediumLiftOff gets
reset on teleporting (would appreciate MC adding sequence numbers to
moving packets :p).
Set-back handling:
* Make the VehicleSetBack a class in the moving package.
* For set-backs use the debug flag from the time of checking.
Debugging:
* Alter debug output for moves slightly (differing locations).
* Add debug output for vehicle moves with players involved.
Note on 1.6:
* Not even PlayerMove is fired for pigs and horses, so no tracking.
This workaround allows the block transition from above a 1.5 high block
into the block, for the case both from and to have been inside of the
bounds of the block. This should fix the annoying bug with jumping
inside of an open fence gate, closing it while just above 1.0 height,
so that passable would alert for moving back from air into the block.
Passable/ray-tracing might disallow players moving out of blocks which
they are stuck in on some occasions.
In order to allow moving out of a block, ray tracing is repeated
with the first block ignored for some cases.
Like with pigs, players can ride other entities than would fire
VehicleMove events. To counter this NCP will detect for which entities
those events are fired and do the packet checks for those that don't.
The permission is set as child of all command permissions
(sub commands of the root command). This is done in post enable to not
have to add it 100 times to the plugin.yml. Hopefully permission plugins
handle this right, superperms (permissions.yml) does.
* Add a new base class for better sub-command handling also for
tab-completion (AbstractCommand).
* Alter package structure slightly, to group command-classes by purpose.
* Some renaming.
Not sure this is that much final, but PlayerData can now carry tags.
NoCheatPlus.sendAdminNotify... will now check PlayerData for the
"notify_off" tag, as a preparation for a command to turn off
notifications.
* Add processing for a new @Moved annotation for moving content to other
paths.
* Remove deprecated paths.
* Changed visibility of some static methods.
* Moderate renaming.
* Remove unused set.
* Ensure only string fields are processed (potential future issue).
Alternate positions are checked for both passable and ray-tracing. This
lacks a little bit abuse prevention, because up to nine times
ray-tracing can be checked, which can be used to cause load for the
server, limited by blockinteract.speed.
Currently only applies to the player-inconsistencies (DataMan). A
summary message will be written to console on disabling the plugin if
inconsistencies were found.
* Uses unmodified block bounds as returned by the server, this is not
the final version but does fix the door problem.
* Users that use the CompatBukkit (bukkitapionly) module should beware
of the current dev-line, because the block shapes can not be retrieved
with using the Bukkit-API only, thus some blocks squeezed between other
blocks can not be interacted with in a reliable way. This concerns
interacting with the sides that are directed towards solid blocks
mostly, so in most cases there still is a side possible to interact
with, but it might be the short side of a door, not convenient.
