feat: Add filter registration option for random teleport locations#1292
feat: Add filter registration option for random teleport locations#1292
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a flexible filtering system for random teleport locations, allowing external plugins to add custom validation logic via a new RandomTeleportLocationFilter interface and API methods. A critical security concern was identified: the current filter unregistration process allows any plugin to unregister any filter by name, which could be exploited to bypass security constraints. It is recommended to update the API to require the filter instance for unregistration to ensure better isolation and security. Additionally, a circular dependency between RandomTeleportServiceImpl and RandomTeleportSafeLocationService needs to be resolved, and an improvement in the filter registration logic is suggested for robustness.
...main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSafeLocationService.java
Show resolved
Hide resolved
| public boolean unregisterFilter(String filterName) { | ||
| if (filterName == null || filterName.isEmpty()) { | ||
| return false; | ||
| } | ||
|
|
||
| return this.registeredFilters.remove(filterName) != null; | ||
| } |
There was a problem hiding this comment.
The unregisterFilter method allows any caller with access to the RandomTeleportService to remove any registered filter by its name. Since filter names are discoverable via getRegisteredFilters(), a malicious or poorly-behaved plugin can unregister filters registered by other plugins. This can lead to a bypass of critical security checks, such as those preventing teleportation into protected regions or claimed plots. It is recommended to modify the API to require the RandomTeleportLocationFilter instance itself for unregistration, ensuring that only a caller with a reference to the filter object can remove it.
| if (this.registeredFilters.containsKey(filterName)) { | ||
| throw new IllegalArgumentException("Filter with name '" + filterName + "' is already registered"); | ||
| } | ||
|
|
||
| this.registeredFilters.put(filterName, filter); |
There was a problem hiding this comment.
The current implementation for checking if a filter is already registered and then adding it is not an atomic operation. While ConcurrentHashMap is thread-safe, using putIfAbsent is a more idiomatic and robust way to perform this check-then-act operation atomically. This makes the code's intent clearer and prevents potential race conditions if multiple threads try to register a filter with the same name concurrently.
| if (this.registeredFilters.containsKey(filterName)) { | |
| throw new IllegalArgumentException("Filter with name '" + filterName + "' is already registered"); | |
| } | |
| this.registeredFilters.put(filterName, filter); | |
| if (this.registeredFilters.putIfAbsent(filterName, filter) != null) { | |
| throw new IllegalArgumentException("Filter with name '" + filterName + "' is already registered"); | |
| } |
No description provided.