Consume private session proofs before legacy player broadcasts

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<String> 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<Object> propertiesToRemove = new java.util.ArrayList<Object>();
         Collection<String> 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;

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<String, Collection<TestProperty>> getProperties() {
+            Map<String, Collection<TestProperty>> properties = new HashMap<String, Collection<TestProperty>>();
+            properties.put(ForwardedSessionProof.PROPERTY_NAME,
+                    new NonRemovingCollection(new TestProperty(ForwardedSessionProof.PROPERTY_NAME, proof)));
+            return properties;
+        }
+    }
+
+    public static final class NonRemovingCollection extends ArrayList<TestProperty> {
+        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<String, Collection<TestProperty>> entries = new HashMap<String, Collection<TestProperty>>();
+        public Collection<TestProperty> get(Object key) {
+            Collection<TestProperty> values = entries.get(key);
+            if (values == null) {
+                values = new ArrayList<TestProperty>();
+                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;
+        }
+    }
+}