diff --git a/backend-bukkit/src/main/java/dev/moonbridge/bukkit/OwnedBukkitSessionService.java b/backend-bukkit/src/main/java/dev/moonbridge/bukkit/OwnedBukkitSessionService.java index 37ea6c3..ef2de80 100644 --- a/backend-bukkit/src/main/java/dev/moonbridge/bukkit/OwnedBukkitSessionService.java +++ b/backend-bukkit/src/main/java/dev/moonbridge/bukkit/OwnedBukkitSessionService.java @@ -12,6 +12,7 @@ import java.util.function.Supplier; import java.util.concurrent.ConcurrentHashMap; import org.bukkit.entity.Player; import org.bukkit.event.EventHandler; +import org.bukkit.event.EventPriority; import org.bukkit.event.Listener; import org.bukkit.event.player.PlayerJoinEvent; import org.bukkit.event.player.PlayerQuitEvent; @@ -37,15 +38,28 @@ final class OwnedBukkitSessionService implements BukkitSessionService, Listener, this.proxyEpoch = java.util.Objects.requireNonNull(proxyEpoch, "proxyEpoch"); } - @EventHandler + @EventHandler(priority = EventPriority.LOWEST) public void onPlayerJoin(PlayerJoinEvent event) { - if (closed) return; Player player = event.getPlayer(); - String proof = forwardedProof(player).orElse(null); - if (proof == null) return; + if (player == null) return; + // The proof is transport metadata, not a game-profile property. Uranium + // broadcasts every profile property when it spawns this player; its + // legacy packet writer expects every property signature to be non-null. + // Remove the private property before any normal-priority join handler + // or the playerLoggedIn spawn broadcast can observe the profile. + BoundSession previous = sessions.get(player.getUniqueId()); + if (previous != null && previous.player == player) sessions.remove(player.getUniqueId(), previous); + if (closed) return; + ProofProperty stripped = stripForwardedProof(player); + if (!stripped.removed) { + // Fail closed: do not bind a session when the forwarding credential + // could not be removed from the profile before it is broadcast. + if (stripped.foundProperty) player.kickPlayer("Unable to validate forwarded session"); + return; + } long now = System.currentTimeMillis(); ForwardedSessionProof.Claims claims = ForwardedSessionProof.verify( - proof, secret, backendName, proxyEpoch.get(), backendEpoch.getAsLong(), + stripped.proof, secret, backendName, proxyEpoch.get(), backendEpoch.getAsLong(), player.getUniqueId(), now).orElse(null); if (claims == null || !consumeNonce(claims.nonce(), claims.expiresAtEpochMillis(), now)) return; @@ -77,25 +91,35 @@ final class OwnedBukkitSessionService implements BukkitSessionService, Listener, return true; } - private static Optional forwardedProof(Player player) { + static ProofProperty stripForwardedProof(Player player) { Object profile = invokeNoArg(player, "getProfile"); if (profile == null) profile = invokeNoArg(player, "getGameProfile"); - if (profile == null) return Optional.empty(); + if (profile == null) return ProofProperty.unavailable(); Object properties = invokeNoArg(profile, "getProperties"); - if (properties == null) return Optional.empty(); + if (properties == null) return ProofProperty.unavailable(); Object matches = invoke(properties, "get", new Class[]{Object.class}, ForwardedSessionProof.PROPERTY_NAME); - if (!(matches instanceof Iterable)) return Optional.empty(); - Iterable iterable = (Iterable) matches; + if (!(matches instanceof Iterable)) return ProofProperty.unavailable(); + Collection propertiesToRemove = new java.util.ArrayList(); Collection values = new java.util.ArrayList<>(2); - for (Object property : iterable) { - Object name = invokeNoArg(property, "getName"); - if (!ForwardedSessionProof.PROPERTY_NAME.equals(name)) continue; + boolean allValuesValid = true; + for (Object property : (Iterable) matches) { + if (property == null) return ProofProperty.failed(true); + propertiesToRemove.add(property); Object value = invokeNoArg(property, "getValue"); if (value instanceof String && ((String) value).length() <= 2048) values.add((String) value); - else return Optional.empty(); - if (values.size() > 1) return Optional.empty(); + else allValuesValid = false; } - return values.size() == 1 ? Optional.of(values.iterator().next()) : Optional.empty(); + for (Object property : propertiesToRemove) { + Object removed = invoke(properties, "remove", new Class[]{Object.class, Object.class}, + ForwardedSessionProof.PROPERTY_NAME, property); + if (!Boolean.TRUE.equals(removed)) return ProofProperty.failed(true); + } + Object remaining = invoke(properties, "get", new Class[]{Object.class}, + ForwardedSessionProof.PROPERTY_NAME); + if (!(remaining instanceof Iterable) || ((Iterable) remaining).iterator().hasNext()) + return ProofProperty.failed(!propertiesToRemove.isEmpty()); + return ProofProperty.removed(propertiesToRemove.size() == 1 && allValuesValid && values.size() == 1 + ? values.iterator().next() : null); } private static Object invokeNoArg(Object receiver, String methodName) { @@ -118,6 +142,22 @@ final class OwnedBukkitSessionService implements BukkitSessionService, Listener, } } + static final class ProofProperty { + final String proof; + final boolean removed; + final boolean foundProperty; + + private ProofProperty(String proof, boolean removed, boolean foundProperty) { + this.proof = proof; + this.removed = removed; + this.foundProperty = foundProperty; + } + + private static ProofProperty unavailable() { return new ProofProperty(null, false, false); } + private static ProofProperty failed(boolean found) { return new ProofProperty(null, false, found); } + private static ProofProperty removed(String proof) { return new ProofProperty(proof, true, proof != null); } + } + @Override public void close() { closed = true; diff --git a/backend-bukkit/src/test/java/dev/moonbridge/bukkit/OwnedBukkitSessionServiceTest.java b/backend-bukkit/src/test/java/dev/moonbridge/bukkit/OwnedBukkitSessionServiceTest.java new file mode 100644 index 0000000..d38e584 --- /dev/null +++ b/backend-bukkit/src/test/java/dev/moonbridge/bukkit/OwnedBukkitSessionServiceTest.java @@ -0,0 +1,215 @@ +package dev.moonbridge.bukkit; + +import dev.moonbridge.messaging.session.ForwardedSessionProof; +import org.bukkit.entity.Player; +import org.bukkit.event.EventPriority; +import org.bukkit.event.player.PlayerJoinEvent; +import org.junit.jupiter.api.Test; + +import java.lang.reflect.Proxy; +import java.util.ArrayList; +import java.util.Collection; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.UUID; +import java.util.concurrent.atomic.AtomicBoolean; + +import static org.junit.jupiter.api.Assertions.*; + +class OwnedBukkitSessionServiceTest { + private static final String BACKEND = "island"; + private static final byte[] SECRET = new byte[32]; + private static final UUID PROXY_EPOCH = UUID.fromString("3b8f58d7-430b-40ca-8e59-c55598ee7f13"); + private static final long BACKEND_EPOCH = 7L; + + @Test void validProofIsConsumedBeforeBroadcastAndOtherProfilePropertiesSurvive() throws Exception { + UUID playerId = UUID.randomUUID(); + UUID nonce = UUID.randomUUID(); + String proof = proof(playerId, nonce, System.currentTimeMillis() + 20_000L); + TestProfile profile = new TestProfile(playerId, "player"); + TestProperty credential = new TestProperty(ForwardedSessionProof.PROPERTY_NAME, proof, null); + TestProperty texture = new TestProperty("textures", "texture-value", "texture-signature"); + profile.getProperties().put(ForwardedSessionProof.PROPERTY_NAME, credential); + profile.getProperties().put("textures", texture); + TestPlayer fixture = player(playerId, profile); + OwnedBukkitSessionService service = service(); + + service.onPlayerJoin(new PlayerJoinEvent(fixture.player, "joined")); + + assertTrue(profile.getProperties().get(ForwardedSessionProof.PROPERTY_NAME).isEmpty()); + assertSame(texture, profile.getProperties().get("textures").iterator().next()); + assertSame(profile, fixture.profile); + BackendPlayerSession bound = service.find(fixture.player).orElseThrow(AssertionError::new); + assertEquals(playerId, bound.getPlayerId()); + assertEquals(23L, bound.getConnectionId()); + assertFalse(fixture.kicked.get()); + assertEquals(EventPriority.LOWEST, + OwnedBukkitSessionService.class.getMethod("onPlayerJoin", PlayerJoinEvent.class) + .getAnnotation(org.bukkit.event.EventHandler.class).priority()); + service.close(); + } + + @Test void invalidAndDuplicateProofsAreRemovedButNeverBindASession() { + UUID playerId = UUID.randomUUID(); + TestProfile invalidProfile = new TestProfile(playerId, "invalid"); + invalidProfile.getProperties().put(ForwardedSessionProof.PROPERTY_NAME, + new TestProperty(ForwardedSessionProof.PROPERTY_NAME, "not-a-proof", null)); + TestPlayer invalidPlayer = player(playerId, invalidProfile); + OwnedBukkitSessionService invalidService = service(); + + invalidService.onPlayerJoin(new PlayerJoinEvent(invalidPlayer.player, "joined")); + + assertTrue(invalidProfile.getProperties().get(ForwardedSessionProof.PROPERTY_NAME).isEmpty()); + assertFalse(invalidService.find(invalidPlayer.player).isPresent()); + assertFalse(invalidPlayer.kicked.get()); + invalidService.close(); + + TestProfile duplicateProfile = new TestProfile(playerId, "duplicate"); + String proof = proof(playerId, UUID.randomUUID(), System.currentTimeMillis() + 20_000L); + duplicateProfile.getProperties().put(ForwardedSessionProof.PROPERTY_NAME, + new TestProperty(ForwardedSessionProof.PROPERTY_NAME, proof, null)); + duplicateProfile.getProperties().put(ForwardedSessionProof.PROPERTY_NAME, + new TestProperty(ForwardedSessionProof.PROPERTY_NAME, proof, null)); + TestPlayer duplicatePlayer = player(playerId, duplicateProfile); + OwnedBukkitSessionService duplicateService = service(); + + duplicateService.onPlayerJoin(new PlayerJoinEvent(duplicatePlayer.player, "joined")); + + assertTrue(duplicateProfile.getProperties().get(ForwardedSessionProof.PROPERTY_NAME).isEmpty()); + assertFalse(duplicateService.find(duplicatePlayer.player).isPresent()); + assertFalse(duplicatePlayer.kicked.get()); + duplicateService.close(); + } + + @Test void failedRemovalKicksAndDoesNotBindTheForwardedIdentity() { + UUID playerId = UUID.randomUUID(); + String proof = proof(playerId, UUID.randomUUID(), System.currentTimeMillis() + 20_000L); + NonRemovingProfile profile = new NonRemovingProfile(playerId, proof); + TestPlayer fixture = player(playerId, profile); + OwnedBukkitSessionService service = service(); + + service.onPlayerJoin(new PlayerJoinEvent(fixture.player, "joined")); + + assertTrue(fixture.kicked.get()); + assertFalse(service.find(fixture.player).isPresent()); + service.close(); + } + + @Test void staleJoinWithoutProofCannotEraseAnotherPlayerInstancesBinding() { + UUID playerId = UUID.randomUUID(); + TestProfile currentProfile = new TestProfile(playerId, "current"); + currentProfile.getProperties().put(ForwardedSessionProof.PROPERTY_NAME, + new TestProperty(ForwardedSessionProof.PROPERTY_NAME, + proof(playerId, UUID.randomUUID(), System.currentTimeMillis() + 20_000L), null)); + TestPlayer current = player(playerId, currentProfile); + TestPlayer stale = player(playerId, new TestProfile(playerId, "stale")); + OwnedBukkitSessionService service = service(); + + service.onPlayerJoin(new PlayerJoinEvent(current.player, "joined")); + BackendPlayerSession original = service.find(current.player).orElseThrow(AssertionError::new); + service.onPlayerJoin(new PlayerJoinEvent(stale.player, "late stale join")); + + assertSame(original, service.find(current.player).orElseThrow(AssertionError::new)); + assertFalse(service.find(stale.player).isPresent()); + service.close(); + } + + private static OwnedBukkitSessionService service() { + return new OwnedBukkitSessionService(BACKEND, SECRET, () -> BACKEND_EPOCH, () -> PROXY_EPOCH); + } + + private static String proof(UUID playerId, UUID nonce, long expiry) { + return ForwardedSessionProof.create(PROXY_EPOCH, playerId, 23L, BACKEND, BACKEND_EPOCH, + nonce, expiry, SECRET); + } + + private static TestPlayer player(UUID playerId, Object profile) { + AtomicBoolean kicked = new AtomicBoolean(); + Player player = (Player) Proxy.newProxyInstance(Player.class.getClassLoader(), + new Class[]{Player.class, ProfileHolder.class}, (proxy, method, arguments) -> { + if (method.getName().equals("getUniqueId")) return playerId; + if (method.getName().equals("isOnline")) return true; + if (method.getName().equals("getGameProfile")) return profile; + if (method.getName().equals("kickPlayer")) { kicked.set(true); return null; } + if (method.getName().equals("toString")) return "test-player"; + if (method.getName().equals("hashCode")) return System.identityHashCode(proxy); + if (method.getName().equals("equals")) return proxy == arguments[0]; + return null; + }); + return new TestPlayer(player, profile, kicked); + } + + public interface ProfileHolder { + Object getGameProfile(); + } + + public static final class NonRemovingProfile { + private final UUID id; + private final String proof; + NonRemovingProfile(UUID id, String proof) { this.id = id; this.proof = proof; } + public UUID getId() { return id; } + public String getName() { return "non-removing"; } + public Map> getProperties() { + Map> properties = new HashMap>(); + properties.put(ForwardedSessionProof.PROPERTY_NAME, + new NonRemovingCollection(new TestProperty(ForwardedSessionProof.PROPERTY_NAME, proof))); + return properties; + } + } + + public static final class NonRemovingCollection extends ArrayList { + private static final long serialVersionUID = 1L; + NonRemovingCollection(TestProperty property) { add(property); } + } + + public static final class TestProperty { + private final String name; + private final String value; + private final String signature; + TestProperty(String name, String value) { this(name, value, null); } + TestProperty(String name, String value, String signature) { + this.name = name; + this.value = value; + this.signature = signature; + } + public String getName() { return name; } + public String getValue() { return value; } + public String getSignature() { return signature; } + } + + public static final class TestProfile { + private final UUID id; + private final String name; + private final TestPropertyMap properties = new TestPropertyMap(); + TestProfile(UUID id, String name) { this.id = id; this.name = name; } + public UUID getId() { return id; } + public String getName() { return name; } + public TestPropertyMap getProperties() { return properties; } + } + + public static final class TestPropertyMap { + private final Map> entries = new HashMap>(); + public Collection get(Object key) { + Collection values = entries.get(key); + if (values == null) { + values = new ArrayList(); + entries.put((String) key, values); + } + return values; + } + public void put(String key, TestProperty property) { get(key).add(property); } + public boolean remove(Object key, Object property) { return get(key).remove(property); } + } + + private static final class TestPlayer { + final Player player; + final Object profile; + final AtomicBoolean kicked; + TestPlayer(Player player, Object profile, AtomicBoolean kicked) { + this.player = player; + this.profile = profile; + this.kicked = kicked; + } + } +}