Skip to content

Commit

Permalink
Sanitize and validate more join address variations
Browse files Browse the repository at this point in the history
- Added Data_gathering.Preserve_invalid_join_addresses to allow overriding this behavior.

Affects issues:
- Fixed #3545
  • Loading branch information
AuroraLS3 committed Apr 27, 2024
1 parent 24a8c75 commit 9e25f2b
Show file tree
Hide file tree
Showing 10 changed files with 216 additions and 22 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
*/
package com.djrapitops.plan.gathering.listeners.bukkit;

import com.djrapitops.plan.gathering.JoinAddressValidator;
import com.djrapitops.plan.gathering.cache.JoinAddressCache;
import com.djrapitops.plan.gathering.domain.BukkitPlayerData;
import com.djrapitops.plan.gathering.domain.event.PlayerJoin;
Expand All @@ -29,6 +30,7 @@
import com.djrapitops.plan.storage.database.transactions.events.BanStatusTransaction;
import com.djrapitops.plan.storage.database.transactions.events.KickStoreTransaction;
import com.djrapitops.plan.storage.database.transactions.events.StoreAllowlistBounceTransaction;
import com.djrapitops.plan.utilities.dev.Untrusted;
import com.djrapitops.plan.utilities.logging.ErrorContext;
import com.djrapitops.plan.utilities.logging.ErrorLogger;
import org.bukkit.event.EventHandler;
Expand All @@ -51,6 +53,7 @@ public class PlayerOnlineListener implements Listener {

private final PlayerJoinEventConsumer playerJoinEventConsumer;
private final PlayerLeaveEventConsumer playerLeaveEventConsumer;
private final JoinAddressValidator joinAddressValidator;
private final JoinAddressCache joinAddressCache;

private final ServerInfo serverInfo;
Expand All @@ -62,6 +65,7 @@ public class PlayerOnlineListener implements Listener {
public PlayerOnlineListener(
PlayerJoinEventConsumer playerJoinEventConsumer,
PlayerLeaveEventConsumer playerLeaveEventConsumer,
JoinAddressValidator joinAddressValidator,
JoinAddressCache joinAddressCache,
ServerInfo serverInfo,
DBSystem dbSystem,
Expand All @@ -70,6 +74,7 @@ public PlayerOnlineListener(
) {
this.playerJoinEventConsumer = playerJoinEventConsumer;
this.playerLeaveEventConsumer = playerLeaveEventConsumer;
this.joinAddressValidator = joinAddressValidator;
this.joinAddressCache = joinAddressCache;
this.serverInfo = serverInfo;
this.dbSystem = dbSystem;
Expand All @@ -89,17 +94,8 @@ public void onPlayerLogin(PlayerLoginEvent event) {
dbSystem.getDatabase().executeTransaction(new StoreAllowlistBounceTransaction(playerUUID, event.getPlayer().getName(), serverUUID, System.currentTimeMillis()));
}

String address = event.getHostname();
if (!address.isEmpty()) {
if (address.contains(":")) {
address = address.substring(0, address.lastIndexOf(':'));
}
if (address.contains("\u0000")) {
address = address.substring(0, address.indexOf('\u0000'));
}
if (address.contains("fml")) {
address = address.substring(0, address.lastIndexOf("fml"));
}
@Untrusted String address = joinAddressValidator.sanitize(event.getHostname());
if (joinAddressValidator.isValid(address)) {
joinAddressCache.put(playerUUID, address);
}
dbSystem.getDatabase().executeTransaction(new BanStatusTransaction(playerUUID, serverUUID, banned));
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
/*
* This file is part of Player Analytics (Plan).
*
* Plan is free software: you can redistribute it and/or modify
* it under the terms of the GNU Lesser General Public License v3 as published by
* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* Plan is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with Plan. If not, see <https://www.gnu.org/licenses/>.
*/
package com.djrapitops.plan.gathering;

import com.djrapitops.plan.settings.config.PlanConfig;
import com.djrapitops.plan.settings.config.paths.DataGatheringSettings;
import com.djrapitops.plan.utilities.dev.Untrusted;
import org.apache.commons.lang3.StringUtils;

import javax.inject.Inject;
import javax.inject.Singleton;
import java.net.URI;
import java.net.URISyntaxException;

/**
* Utility for validating and sanitizing join addresses.
*
* @author AuroraLS3
*/
@Singleton
public class JoinAddressValidator {

private final PlanConfig config;

@Inject
public JoinAddressValidator(PlanConfig config) {
/* Dagger injection constructor */
this.config = config;
}

@Untrusted
public String sanitize(@Untrusted String address) {
if (address == null) return "";
if (!address.isEmpty()) {
// Remove port
if (address.contains(":")) {
address = address.substring(0, address.lastIndexOf(':'));
}
// Remove data added by Bungeecord/Velocity
if (address.contains("\u0000")) {
address = address.substring(0, address.indexOf('\u0000'));
}
// Remove data added by Forge Mod Loader
if (address.contains("fml")) {
address = address.substring(0, address.lastIndexOf("fml"));
}
if (config.isFalse(DataGatheringSettings.PRESERVE_JOIN_ADDRESS_CASE)) {
address = StringUtils.lowerCase(address);
}
}
return address;
}

public boolean isValid(@Untrusted String address) {
if (address.isEmpty()) return false;
if (config.isTrue(DataGatheringSettings.PRESERVE_INVALID_JOIN_ADDRESS)) return true;
try {
URI uri = new URI(address);
String path = uri.getPath();
return path != null && path.indexOf('.') != -1;
} catch (URISyntaxException uriSyntaxException) {
return false;
}
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
*/
package com.djrapitops.plan.gathering.domain;

import com.djrapitops.plan.utilities.dev.Untrusted;

import java.net.InetAddress;
import java.util.Optional;
import java.util.UUID;
Expand All @@ -24,8 +26,10 @@ public interface PlatformPlayerData {

UUID getUUID();

@Untrusted
String getName();

@Untrusted
default Optional<String> getDisplayName() {
return Optional.empty();
}
Expand All @@ -38,6 +42,7 @@ default Optional<Boolean> isOperator() {
return Optional.empty();
}

@Untrusted
default Optional<String> getJoinAddress() {
return Optional.empty();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,13 @@ public long getTime() {
return time;
}

/**
* Get address used to join the server.
*
* @return Join address of the player.
* @deprecated {@link com.djrapitops.plan.gathering.JoinAddressValidator} should be used when looking at join address.
*/
@Deprecated(since = "2024-04-27")
public String getJoinAddress() {
return player.getJoinAddress().orElse(JoinAddressTable.DEFAULT_VALUE_FOR_LOOKUP);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import com.djrapitops.plan.delivery.export.Exporter;
import com.djrapitops.plan.extension.CallEvents;
import com.djrapitops.plan.extension.ExtensionSvc;
import com.djrapitops.plan.gathering.JoinAddressValidator;
import com.djrapitops.plan.gathering.cache.NicknameCache;
import com.djrapitops.plan.gathering.cache.SessionCache;
import com.djrapitops.plan.gathering.domain.ActiveSession;
Expand All @@ -38,7 +39,6 @@
import com.djrapitops.plan.storage.database.sql.tables.JoinAddressTable;
import com.djrapitops.plan.storage.database.transactions.Transaction;
import com.djrapitops.plan.storage.database.transactions.events.*;
import org.apache.commons.lang3.StringUtils;

import javax.inject.Inject;
import javax.inject.Singleton;
Expand All @@ -52,6 +52,7 @@ public class PlayerJoinEventConsumer {
private final PlanConfig config;
private final DBSystem dbSystem;

private final JoinAddressValidator joinAddressValidator;
private final GeolocationCache geolocationCache;
private final SessionCache sessionCache;
private final NicknameCache nicknameCache;
Expand All @@ -63,7 +64,7 @@ public class PlayerJoinEventConsumer {
public PlayerJoinEventConsumer(
Processing processing,
PlanConfig config,
DBSystem dbSystem,
DBSystem dbSystem, JoinAddressValidator joinAddressValidator,
GeolocationCache geolocationCache,
SessionCache sessionCache,
NicknameCache nicknameCache,
Expand All @@ -73,6 +74,7 @@ public PlayerJoinEventConsumer(
this.processing = processing;
this.config = config;
this.dbSystem = dbSystem;
this.joinAddressValidator = joinAddressValidator;
this.geolocationCache = geolocationCache;
this.sessionCache = sessionCache;
this.nicknameCache = nicknameCache;
Expand Down Expand Up @@ -110,7 +112,8 @@ public void onJoinProxyServer(PlayerJoin join) {

private void storeJoinAddress(PlayerJoin join) {
join.getPlayer().getJoinAddress()
.map(joinAddress -> config.isTrue(DataGatheringSettings.PRESERVE_JOIN_ADDRESS_CASE) ? joinAddress : StringUtils.lowerCase(joinAddress))
.map(joinAddressValidator::sanitize)
.filter(joinAddressValidator::isValid)
.map(StoreJoinAddressTransaction::new)
.ifPresent(dbSystem.getDatabase()::executeTransaction);
}
Expand Down Expand Up @@ -141,7 +144,10 @@ private static long getRegisterDate(PlayerJoin join) {

private CompletableFuture<?> storeGamePlayer(PlayerJoin join) {
long registerDate = getRegisterDate(join);
String joinAddress = join.getPlayer().getJoinAddress().orElse(JoinAddressTable.DEFAULT_VALUE_FOR_LOOKUP);
String joinAddress = join.getPlayer().getJoinAddress()
.map(joinAddressValidator::sanitize)
.filter(joinAddressValidator::isValid)
.orElse(JoinAddressTable.DEFAULT_VALUE_FOR_LOOKUP);
Transaction transaction = new StoreServerPlayerTransaction(
join.getPlayerUUID(), registerDate, join.getPlayer().getName(), join.getServerUUID(), joinAddress
);
Expand Down Expand Up @@ -171,12 +177,16 @@ private void storeInterruptedSession(FinishedSession finishedSession) {
}

private ActiveSession mapToActiveSession(PlayerJoin join) {
String joinAddress = join.getPlayer().getJoinAddress()
.map(joinAddressValidator::sanitize)
.filter(joinAddressValidator::isValid)
.orElse(JoinAddressTable.DEFAULT_VALUE_FOR_LOOKUP);
ActiveSession session = new ActiveSession(join.getPlayerUUID(), join.getServerUUID(), join.getTime(),
join.getPlayer().getCurrentWorld().orElse(null),
join.getPlayer().getCurrentGameMode().orElse(null));
session.getExtraData().put(PlayerName.class, new PlayerName(join.getPlayer().getName()));
session.getExtraData().put(ServerName.class, new ServerName(join.getServer().isProxy() ? join.getServer().getName() : "Proxy Server"));
session.getExtraData().put(JoinAddress.class, new JoinAddress(config.isTrue(DataGatheringSettings.PRESERVE_JOIN_ADDRESS_CASE) ? join.getJoinAddress() : StringUtils.lowerCase(join.getJoinAddress())));
session.getExtraData().put(JoinAddress.class, new JoinAddress(joinAddress));
return session;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ public class DataGatheringSettings {
public static final Setting<Boolean> LOG_UNKNOWN_COMMANDS = new BooleanSetting("Data_gathering.Commands.Log_unknown");
public static final Setting<Boolean> COMBINE_COMMAND_ALIASES = new BooleanSetting("Data_gathering.Commands.Log_aliases_as_main_command");
public static final Setting<Boolean> PRESERVE_JOIN_ADDRESS_CASE = new BooleanSetting("Data_gathering.Preserve_join_address_case");
public static final Setting<Boolean> PRESERVE_INVALID_JOIN_ADDRESS = new BooleanSetting("Data_gathering.Preserve_invalid_join_addresses");

private DataGatheringSettings() {
/* static variable class */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ Data_gathering:
Disk_space: true
# Does not affect already gathered data
Preserve_join_address_case: false
Preserve_invalid_join_addresses: false
# -----------------------------------------------------
# Supported time units: MILLISECONDS, SECONDS, MINUTES, HOURS, DAYS
# -----------------------------------------------------
Expand Down
1 change: 1 addition & 0 deletions Plan/common/src/main/resources/assets/plan/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ Data_gathering:
Log_aliases_as_main_command: true
# Does not affect already gathered data
Preserve_join_address_case: false
Preserve_invalid_join_addresses: false
# -----------------------------------------------------
# Supported time units: MILLISECONDS, SECONDS, MINUTES, HOURS, DAYS
# -----------------------------------------------------
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
/*
* This file is part of Player Analytics (Plan).
*
* Plan is free software: you can redistribute it and/or modify
* it under the terms of the GNU Lesser General Public License v3 as published by
* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* Plan is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with Plan. If not, see <https://www.gnu.org/licenses/>.
*/
package com.djrapitops.plan.gathering;

import com.djrapitops.plan.settings.config.PlanConfig;
import com.djrapitops.plan.settings.config.paths.DataGatheringSettings;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.CsvSource;
import org.mockito.InjectMocks;
import org.mockito.Mock;
import org.mockito.junit.jupiter.MockitoExtension;

import static org.junit.jupiter.api.Assertions.*;
import static org.mockito.Mockito.when;

/**
* @author AuroraLS3
*/
@ExtendWith(MockitoExtension.class)
class JoinAddressValidatorTest {

@Mock
PlanConfig config;
@InjectMocks
JoinAddressValidator joinAddressValidator;

@DisplayName("Join address is valid")
@ParameterizedTest(name = "{0}")
@CsvSource({
"play.domain.com",
"12.34.56.78",
"sub.play.domain.xz",
})
void validJoinAddresses(String address) {
assertTrue(joinAddressValidator.isValid(address));
}

@DisplayName("Join address is invalid")
@ParameterizedTest(name = "{0}")
@CsvSource({
"123",
"kioels8bfbc80hgjpz25uatt5bi1n0ueffoqxvrl+q7wgbgynl9jm2w38pihx1nw", // https://github.com/plan-player-analytics/Plan/issues/3545
"play.domain.com:25565",
"play.domain.com:25565\u0000ouehfaounrfaeiurgea",
"play.domain.com\u0000h59783g9guheorig",
"PLAY.DOMAIN.COM:25565",
})
void invalidJoinAddresses(String address) {
when(config.isTrue(DataGatheringSettings.PRESERVE_INVALID_JOIN_ADDRESS)).thenReturn(false);
assertFalse(joinAddressValidator.isValid(address));
}

@Test
@DisplayName("Empty join address is invalid")
void invalidEmptyJoinAddresses() {
assertFalse(joinAddressValidator.isValid(""));
}

@DisplayName("Join address sanitization works")
@ParameterizedTest(name = "{0} -> {1}")
@CsvSource({
"play.domain.com:25565, play.domain.com",
"play.domain.com:25565\u0000ouehfaounrfaeiurgea, play.domain.com",
"play.domain.com\u0000h59783g9guheorig, play.domain.com",
"play.domain.comfmlJEI=1.32.5, play.domain.com",
"PLAY.DOMAIN.COM:25565, PLAY.DOMAIN.COM", // Preserve case is on in the mocked config
})
void sanitizationTest(String address, String expected) {
assertEquals(expected, joinAddressValidator.sanitize(address));
}
}
Loading

0 comments on commit 9e25f2b

Please sign in to comment.