Bukkit

Describe the problem or enhancement (and any steps needed to reproduce the issue):

- From what I can tell this happens on Steve/Alex heads, when the player has changed their name and skin.

The user has not connected since changing their name/skin. Spam right-clicking the head tells you in chat who's head it is, then the server lag happens. TPS dropped form 20ish, below 10 (probably lower) then kicks everyone online. 

What plugin version are you using? _5.2.17

Does the issue occur in the previous version of the plugin? _ Yes I was about 5 versions behind when I found this bug. Updated and it did not fix it. 

What server type and version did you use it on? _ Paper 1.16.5 build 783

Please add or attach any relevant PlayerHeads plugin configuration.

https://pastebin.com/vzfyne71

Please do add or attach any relevant log information or errors.

https://pastebin.com/Na7VwzeE

https://gyazo.com/43fce52103be6ee3a89127fe32bf22a5

The amount of time required for a lookup of a name depends entirely on the server , so I can't be of much help there - and the call to get the name can't be avoided. (Also, I can't reproduce the issue on our server)

Under normal circumstances, these calls don't usually take very long and the responses are supposed to be cached (the UUID stays with the account, not the username) after making a request to Mojang. The rare exception would be if the Mojang profile API is not able to be contacted or the account was actually deleted, I guess - but there's not a way to know ahead of time.

However, if this is caused by people spam-rightclicking the head, then you have a couple options:

• Use the "clickspamcount" and "clickspamthreshold" configuration options to disallow people from right-clicking a head faster than you want. The default configuration only allows 5 right-clicks per second. you can change this by using
  /ph config set clickspamcount numberhere and
  /ph config set clickspamthreshold milisecondshere
  Or by editing the config file.
  NOTE: you must perform /ph config reload to apply changes to this regardless of how you edited the config.
  
  
  
• Deny the "playerheads.clickinfo" permission to users to completely disable right-clicking for the name of a head. How you disable this depends on the permissions plugin you use - some like Luckperms you can simply set to deny - others require you to add a minus sign at the front to negate the permission.

Note: please be aware that if you are using an offline-mode server that no support will be given, although "dropboringplayerheads" can prevent new heads from being skinned or having ownership - which should prevent lookups like this.

The server is online mode, and this seems to only to happen to people that rename their accounts, and don't join after this.

The players are also banned via litebans, not sure if this makes a difference? 

To prevent click spam, would it be best to reduce the /ph config set clickspamcount to 1 click per second, or to increase it to a higher number? 

 To add to this, it looks like the player head I am currently testing with, they have changed their name 3 times since last being on the server.

I think this could be the issue? How to fix?

I have played around with the clickspamcount and clickspamthreshold  settings. Neither of these fix the issue.

I have denied playerheads.clickinfo permission.
This has stopped the player name from being printed in chat, however it does not fix the lag. The server TPS still drops right down and then crashes...

"This has stopped the player name from being printed in chat, however it does not fix the lag."

This is very strange.  Looking at the code, it seems like I check if the head is a custom head from another plugin to ignore the event before checking permissions and this is what is causing the UUID Lookup for you.

This is a development build that makes the permission and spam checks occur before that  https://ci.meme.tips/job/PlayerHeads-5.x/1153/

WARNING: Development builds have not been approved by the BukkitDev staff. Use them at your own risk.

(I'm required to post that)

If you try this build, use it either with a high clickspamthreshold value or keep playerheads.clickinfo denied like you have.  This should allow you to block the event before the part of the code that triggers lag for you.

Please let me know if this helps the issue for you.

I will have to investigate options to make the lookup asynchronous (not allowed by bukkit standards, strictly speaking).

 Hey I used your dev build and spam clicking heads got my server down to 4 TPS. 
I used clickspamcount: 5 and clickspamthreshold: 30000

I think one of the issues is that some players have about 100 heads on a wall in their base. and if you just run through them spam-clicking them, it causes lag. 
Some of them would have changed their names, some wouldn't have. So I don't think it's solely because of that, but it definitely makes it worse. 

EDIT:

This is when players have the permission to see names of heads
From testing so far, if players do not have permission now, it doesn't display the name and doesn't appear to lag.

I'll do more testing and see if I can make it lag without the permission.

With the above information, is there anything you think you can do to prevent spam if players do have permissions?

Instead of adding spam cooldown per-head. Maybe make it on all heads as a whole, so you can only click one head every X amount of time etc.? 

If you want, I could change the click-spam detection to ignore the location of the head- this would ensure that all heads on a wall would use the same limit rather than be individually limited. I will have to make a new dev build for that.

- although I'm curious how long each UUID Lookup is taking - if it's over half a second for a lookup of a single head/username then there is something seriously wrong with either networking/contacting mojang, or with the uuid cache. How large is your usercache.json ?

EDIT: This dev build considers all right-click events by a player for spam regardless of location. https://ci.meme.tips/job/PlayerHeads-5.x/1155/
WARNING: Development builds have not been approved by the BukkitDev staff. Use them at your own risk.

I would recommend setting clickspamcount low (like 1 or 2) and keeping clickspamthreshold high

My usercache.json in my main server directory is 108,060 bytes

Would you like me to test that build linked in your last message?

Sure, that'd be great.  That build will only change spam-detection mechanics though, not any other fix.

108KB isn't that bad. ours is 105KB (probably could do with a cleaning, but it's not impacting performance for us). Hm.  I'm really not sure why GetOfflinePlayer is so slow for you.

Is there any way that we could discuss this on Discord, and I possibly connect to your test server to try and reproduce this there?

I have tested your latest dev build with these settings:

clickspamcount: 5
clickspamthreshold: 5000

Is the click spam threshold for all heads on the server, individual to each player, or server wide?
Example, If I click a head, I have to wait 5 seconds to click another head, but can you click it before 5 seconds as the timer is on my account, and not the server?

clickspamthreshold: 5000
Setting this to 5000 means I actually had to wait 6 seconds before clicking another head. If I wanted less than 6 seconds the event would be cancelled, and it would also make me wait a further 6 seconds from the point that I clicked it too early. Is that intentional? 

clickspamcount: 5
What exactly does this do in the latest build?
I tried changing this up and down but saw no different results.

Overall while testing myself, this build is definitely the most successful at preventing the lag. 

I have found that anything between 1000-4000 generally can cause the server to lag/crash still.
Setting it to 5000 I did manage to get the TPS to drop ever so slightly. (0.5 maybe), which really wasn't enough to feel any lag, and this definitely prevents the players from accidentally lagging/crashing the server.

I will still get the player that found this exploit to test it and see if they are able to make it lag with my current settings, if they are I will experiment with the settings and feed back to you.

Update:

It appears that if another player clicks a different head before your 5 seconds clickspamthreshold is up, it resets that timer for you allowing you to click the head again.

Following this, if you both hold down right-click on a head the server things you are both spam clicking I guess, and allows you to click it indefinitely causing spam as shown on this gif link:

https://gyazo.com/90258bfdbcad432547d6b92f51fa0df2

Though this spams the client returning the name, it doesn't seem to be causing any server lag when we try this on heads that previously crashed the server. So I guess it isn't trying to grab the name continuously?

EDIT:

It seems that if you have 2 of the "Bad heads" (people that have changed their names and not reconnected)  and with 2 people, if you both spam click one each, the TPS does start to drop:
https://gyazo.com/d70dc90bd5ebf2becce912bac1b9f356

A different person clicking a head shouldn't reset the first person's count unless you have enough click events recorded to fill the spam buffer.

I apologize - in my haste each day (I'm very busy with work lately), I explained the "clickspamcount" setting incorrectly (the config page is correct)

Originally I said that "clickspamcount" was the number of matches in the spam buffer required to mark as spam - this is incorrect.

Reviewing the code, "clickspamcount" is actually the size of the spam buffer history (setting it to 5 records only the last 5 click events) - so you would want to set this high [and config reload] if you are receiving clicks from difference sources.

So this will not impact detection except when enough unrelated clicks from different parties fill the buffer, or if it's so low that it doesn't match anything.

Would you be able to get the tag data/uuid/username for one of the "bad" heads?  I think the command should be something like /data get entity yournamehere Inventory[0] (If I recall correctly) if you have the item in your first slot

or /data get block xposhere yposhere zposhere if it's placed.

You can absolutely DM me on Discord ( crashdemons#0916 ) although again I have limited availability [rarely] before and after work. Best time to catch me is 5pm-8pm US Central on weekdays. I may be able to respond more on the weekend (although I will probably be doing some work on my car during the day this saturday).

We also have a Discord server: https://discord.gg/JUvjHTT9

"A different person clicking a head shouldn't reset the first person's count unless you have enough click events recorded to fill the spam buffer"

This did happen, but I had the clickspamcount set at 1 and at 5 when doing this.

When you advise to set clickspamcount to a "high number" What does this mean? 5000 for example?
I am still not exactly sure what clickspamcount does but when I set it to 5000 it fixed the issue of spamming the screen if two players where holding their mouse down.

These are 2 of the "Bad heads" 
https://gyazo.com/8aa9618fbad95df9792ed72b089e7182

https://gyazo.com/0f9710333759fb61158966ddea004986

New issue...

If someone drinks a haste 2 potion, then breaks any head (in an area they don't have permissions Worldguard / Towny etc.) The server drops TPS and lags, got it down to 10TPS just from 1 person doing it. (takes 1-2 mins of spam breaking to start lagging)
(This is not breaking "Bad heads" but any head at all...) 

It says that you are not accepting friend requests on Discord. I joined the server my nickname is Rangewonk (you should have a ping from me) if you could send the friends request please?

Changed

fixbrokenheads: false 

to see if this fixes the issue with breaking head without permission to build in area. 

for the record to anyone else who reads this:

Two issues:

 - ClickInfo did not check spam detection before requesting username. Normally not an issue, but some servers with slow API requests are hit by this.
 - Some combination of server plugins can cause blockbreak events to be sent to PlayerHeads even when in a protected area, which also needs to perform a request, but has no spam detection possible.

Issue 1 can be mitigated by updating to 5.2.18 (dev build only for now) and adjusting clickspamsettings (clickspamcount >100, large clickspamthreshold >1000).

Issue 2 can be mitigated by disabling "fixbrokenheads" to prevent the update handling check on them.