Skip to content

Commit 978db69

Browse files
jeffreyrainyjeffreyrainy
authored
fix: correctly rejecting NetworkList modifications on unauthorized clients (#2233)
* fix: correctly rejecting NetworkList modifications on unauthorized clients. Adding tests for it. Allowing null IEnumerable to mean empty list in NetworkList
1 parent e610067 commit 978db69

File tree

3 files changed

+262
-2
lines changed

3 files changed

+262
-2
lines changed

‎com.unity.netcode.gameobjects/Runtime/NetworkVariable/Collections/NetworkList.cs

Lines changed: 42 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,9 +39,13 @@ public NetworkList(IEnumerable<T> values = default,
3939
NetworkVariableWritePermission writePerm = DefaultWritePerm)
4040
: base(readPerm, writePerm)
4141
{
42-
foreach (var value in values)
42+
// allow null IEnumerable<T> to mean "no values"
43+
if (values != null)
4344
{
44-
m_List.Add(value);
45+
foreach (var value in values)
46+
{
47+
m_List.Add(value);
48+
}
4549
}
4650
}
4751

@@ -364,6 +368,12 @@ public IEnumerator<T> GetEnumerator()
364368
/// <inheritdoc />
365369
public void Add(T item)
366370
{
371+
// check write permissions
372+
if (!CanClientWrite(m_NetworkBehaviour.NetworkManager.LocalClientId))
373+
{
374+
throw new InvalidOperationException("Client is not allowed to write to this NetworkList");
375+
}
376+
367377
m_List.Add(item);
368378

369379
var listEvent = new NetworkListEvent<T>()
@@ -379,6 +389,12 @@ public void Add(T item)
379389
/// <inheritdoc />
380390
public void Clear()
381391
{
392+
// check write permissions
393+
if (!CanClientWrite(m_NetworkBehaviour.NetworkManager.LocalClientId))
394+
{
395+
throw new InvalidOperationException("Client is not allowed to write to this NetworkList");
396+
}
397+
382398
m_List.Clear();
383399

384400
var listEvent = new NetworkListEvent<T>()
@@ -399,6 +415,12 @@ public bool Contains(T item)
399415
/// <inheritdoc />
400416
public bool Remove(T item)
401417
{
418+
// check write permissions
419+
if (!CanClientWrite(m_NetworkBehaviour.NetworkManager.LocalClientId))
420+
{
421+
throw new InvalidOperationException("Client is not allowed to write to this NetworkList");
422+
}
423+
402424
int index = NativeArrayExtensions.IndexOf(m_List, item);
403425
if (index == -1)
404426
{
@@ -428,6 +450,12 @@ public int IndexOf(T item)
428450
/// <inheritdoc />
429451
public void Insert(int index, T item)
430452
{
453+
// check write permissions
454+
if (!CanClientWrite(m_NetworkBehaviour.NetworkManager.LocalClientId))
455+
{
456+
throw new InvalidOperationException("Client is not allowed to write to this NetworkList");
457+
}
458+
431459
if (index < m_List.Length)
432460
{
433461
m_List.InsertRangeWithBeginEnd(index, index + 1);
@@ -451,6 +479,12 @@ public void Insert(int index, T item)
451479
/// <inheritdoc />
452480
public void RemoveAt(int index)
453481
{
482+
// check write permissions
483+
if (!CanClientWrite(m_NetworkBehaviour.NetworkManager.LocalClientId))
484+
{
485+
throw new InvalidOperationException("Client is not allowed to write to this NetworkList");
486+
}
487+
454488
m_List.RemoveAt(index);
455489

456490
var listEvent = new NetworkListEvent<T>()
@@ -468,6 +502,12 @@ public T this[int index]
468502
get => m_List[index];
469503
set
470504
{
505+
// check write permissions
506+
if (!CanClientWrite(m_NetworkBehaviour.NetworkManager.LocalClientId))
507+
{
508+
throw new InvalidOperationException("Client is not allowed to write to this NetworkList");
509+
}
510+
471511
var previousValue = m_List[index];
472512
m_List[index] = value;
473513

‎com.unity.netcode.gameobjects/Tests/Runtime/OwnerPermissionTests.cs

Lines changed: 209 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,209 @@
1+
using System;
2+
using System.Collections;
3+
using System.Collections.Generic;
4+
using UnityEngine;
5+
using UnityEngine.TestTools;
6+
using Unity.Netcode.TestHelpers.Runtime;
7+
8+
namespace Unity.Netcode.RuntimeTests
9+
{
10+
public class OwnerPermissionObject : NetworkBehaviour
11+
{
12+
// indexed by [object, machine]
13+
public static OwnerPermissionObject[,] Objects = new OwnerPermissionObject[3, 3];
14+
public static int CurrentlySpawning = 0;
15+
16+
public static List<OwnerPermissionObject> ClientTargetedNetworkObjects = new List<OwnerPermissionObject>();
17+
// a client-owned NetworkVariable
18+
public NetworkVariable<int> MyNetworkVariableOwner;
19+
// a server-owned NetworkVariable
20+
public NetworkVariable<int> MyNetworkVariableServer;
21+
22+
// a client-owned NetworkVariable
23+
public NetworkList<int> MyNetworkListOwner;
24+
// a server-owned NetworkVariable
25+
public NetworkList<int> MyNetworkListServer;
26+
27+
// verifies two lists are identical
28+
public static void CheckLists(NetworkList<int> listA, NetworkList<int> listB)
29+
{
30+
Debug.Assert(listA.Count == listB.Count);
31+
for (var i = 0; i < listA.Count; i++)
32+
{
33+
Debug.Assert(listA[i] == listB[i]);
34+
}
35+
}
36+
37+
// verifies all objects have consistent lists on all clients
38+
public static void VerifyConsistency()
39+
{
40+
for (var objectIndex = 0; objectIndex < 3; objectIndex++)
41+
{
42+
CheckLists(Objects[objectIndex, 0].MyNetworkListOwner, Objects[objectIndex, 1].MyNetworkListOwner);
43+
CheckLists(Objects[objectIndex, 0].MyNetworkListOwner, Objects[objectIndex, 2].MyNetworkListOwner);
44+
45+
CheckLists(Objects[objectIndex, 0].MyNetworkListServer, Objects[objectIndex, 1].MyNetworkListServer);
46+
CheckLists(Objects[objectIndex, 0].MyNetworkListServer, Objects[objectIndex, 2].MyNetworkListServer);
47+
}
48+
}
49+
50+
public override void OnNetworkSpawn()
51+
{
52+
Objects[CurrentlySpawning, NetworkManager.LocalClientId] = GetComponent<OwnerPermissionObject>();
53+
Debug.Log($"Object index ({CurrentlySpawning}) spawned on client {NetworkManager.LocalClientId}");
54+
}
55+
56+
private void Awake()
57+
{
58+
MyNetworkVariableOwner = new NetworkVariable<int>(writePerm: NetworkVariableWritePermission.Owner);
59+
MyNetworkVariableOwner.OnValueChanged += OwnerChanged;
60+
61+
MyNetworkVariableServer = new NetworkVariable<int>(writePerm: NetworkVariableWritePermission.Server);
62+
MyNetworkVariableServer.OnValueChanged += ServerChanged;
63+
64+
MyNetworkListOwner = new NetworkList<int>(writePerm: NetworkVariableWritePermission.Owner);
65+
MyNetworkListOwner.OnListChanged += ListOwnerChanged;
66+
67+
MyNetworkListServer = new NetworkList<int>(writePerm: NetworkVariableWritePermission.Server);
68+
MyNetworkListServer.OnListChanged += ListServerChanged;
69+
}
70+
71+
public void OwnerChanged(int before, int after)
72+
{
73+
}
74+
75+
public void ServerChanged(int before, int after)
76+
{
77+
}
78+
79+
public void ListOwnerChanged(NetworkListEvent<int> listEvent)
80+
{
81+
}
82+
83+
public void ListServerChanged(NetworkListEvent<int> listEvent)
84+
{
85+
}
86+
}
87+
88+
public class OwnerPermissionHideTests : NetcodeIntegrationTest
89+
{
90+
protected override int NumberOfClients => 2;
91+
92+
private GameObject m_PrefabToSpawn;
93+
94+
protected override void OnServerAndClientsCreated()
95+
{
96+
m_PrefabToSpawn = CreateNetworkObjectPrefab("OwnerPermissionObject");
97+
m_PrefabToSpawn.AddComponent<OwnerPermissionObject>();
98+
}
99+
100+
[UnityTest]
101+
public IEnumerator OwnerPermissionTest()
102+
{
103+
// create 3 objects
104+
for (var objectIndex = 0; objectIndex < 3; objectIndex++)
105+
{
106+
OwnerPermissionObject.CurrentlySpawning = objectIndex;
107+
108+
NetworkManager ownerManager = m_ServerNetworkManager;
109+
if (objectIndex != 0)
110+
{
111+
ownerManager = m_ClientNetworkManagers[objectIndex - 1];
112+
}
113+
SpawnObject(m_PrefabToSpawn, ownerManager);
114+
115+
// wait for each object to spawn on each client
116+
for (var clientIndex = 0; clientIndex < 3; clientIndex++)
117+
{
118+
while (OwnerPermissionObject.Objects[objectIndex, clientIndex] == null)
119+
{
120+
yield return new WaitForSeconds(0.0f);
121+
}
122+
}
123+
}
124+
125+
var nextValueToWrite = 1;
126+
var serverIndex = 0;
127+
128+
for (var objectIndex = 0; objectIndex < 3; objectIndex++)
129+
{
130+
for (var clientWriting = 0; clientWriting < 3; clientWriting++)
131+
{
132+
// ==== Server-writable NetworkVariable ====
133+
var gotException = false;
134+
Debug.Log($"Writing to server-write variable on object {objectIndex} on client {clientWriting}");
135+
136+
try
137+
{
138+
nextValueToWrite++;
139+
OwnerPermissionObject.Objects[objectIndex, clientWriting].MyNetworkVariableServer.Value = nextValueToWrite;
140+
}
141+
catch (Exception)
142+
{
143+
gotException = true;
144+
}
145+
146+
// Verify server-owned netvar can only be written by server
147+
Debug.Assert(gotException == (clientWriting != serverIndex));
148+
149+
// ==== Owner-writable NetworkVariable ====
150+
gotException = false;
151+
Debug.Log($"Writing to owner-write variable on object {objectIndex} on client {clientWriting}");
152+
153+
try
154+
{
155+
nextValueToWrite++;
156+
OwnerPermissionObject.Objects[objectIndex, clientWriting].MyNetworkVariableOwner.Value = nextValueToWrite;
157+
}
158+
catch (Exception)
159+
{
160+
gotException = true;
161+
}
162+
163+
// Verify client-owned netvar can only be written by owner
164+
Debug.Assert(gotException == (clientWriting != objectIndex));
165+
166+
// ==== Server-writable NetworkList ====
167+
gotException = false;
168+
Debug.Log($"Writing to server-write list on object {objectIndex} on client {clientWriting}");
169+
170+
try
171+
{
172+
nextValueToWrite++;
173+
OwnerPermissionObject.Objects[objectIndex, clientWriting].MyNetworkListServer.Add(nextValueToWrite);
174+
}
175+
catch (Exception)
176+
{
177+
gotException = true;
178+
}
179+
180+
// Verify server-owned networkList can only be written by server
181+
Debug.Assert(gotException == (clientWriting != serverIndex));
182+
183+
// ==== Owner-writable NetworkList ====
184+
gotException = false;
185+
Debug.Log($"Writing to owner-write list on object {objectIndex} on client {clientWriting}");
186+
187+
try
188+
{
189+
nextValueToWrite++;
190+
OwnerPermissionObject.Objects[objectIndex, clientWriting].MyNetworkListOwner.Add(nextValueToWrite);
191+
}
192+
catch (Exception)
193+
{
194+
gotException = true;
195+
}
196+
197+
// Verify client-owned networkList can only be written by owner
198+
Debug.Assert(gotException == (clientWriting != objectIndex));
199+
200+
yield return NetcodeIntegrationTestHelpers.WaitForTicks(m_ServerNetworkManager, 5);
201+
yield return NetcodeIntegrationTestHelpers.WaitForTicks(m_ClientNetworkManagers[0], 5);
202+
yield return NetcodeIntegrationTestHelpers.WaitForTicks(m_ClientNetworkManagers[1], 5);
203+
204+
OwnerPermissionObject.VerifyConsistency();
205+
}
206+
}
207+
}
208+
}
209+
}

‎com.unity.netcode.gameobjects/Tests/Runtime/OwnerPermissionTests.cs.meta

Lines changed: 11 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)