search

TCPShield / RealIP

The Spigot, Bungee and Velocity plugin that parses client IP addresses passed from the TCPShield network.
https://tcpshield.com
MIT License
145 stars 52 forks source link

Support FlameCord ProxyPingEvent changes (Cancellable) and check if r… #26

Closed ghost closed 3 years ago

ghost commented 3 years ago

…esponse is null

paulzhng commented 3 years ago

Thanks for doing a PR! I need you to explain what's actually done by the PR as in why those code changes are necessary / beneficial for supporting FlameCord and its API.

Regarding the changes with Cancellable, I see one issue in that. As we're on the lowest event priority, our event should get executed first, thus making the event not cancelled most of the times except in edge cases where multiple plugins are on the lowest event priority & some random magic happens. In addition to that, events further down the event chain are able to "uncancel" the event itself, making the server more susceptible to discovery attacks if faulty plugins are installed. The current way provides a safe method without relying on other plugins to follow the norm.

ghost commented 3 years ago

Hi, Idk about EventPriority works but i added the FlameCord cancelled change to the PR to make more 'compatibility' with others plugins and FlameCord itself, and the response null check is to avoid some types of bot attacks spamming null things in the Pings (Motd, hover board (playerlist), etc)

Sorry for my bad english :)

paulzhng commented 3 years ago

The filtering layer is concentrated on our proxies which filter those attacks automatically. Our plugin is just for making sure the origin of the connections is correct and setting the right IP, thus such a check wouldn't be needed. Regarding the compatibility, we're fine with the current being as all connections except those originating from our network should be closed immediately.

I'd like the input of the other members of the team regarding this issue.

JosiahWhite commented 3 years ago

As Paul said above, the filtering takes place at our end, so I am not sure how this will help. If an attacker sends a null ping attack directly to your backend, I think you have more significant problems than just an exception being fired.

paulzhng commented 3 years ago

Because of the mentioned things, won't merge.