Make JavaScript responsible for blocking illegal vehicle modifications

Author: RussellLVP

data/server/callbacks.txt

@@ -49,7 +49,7 @@ forward OnPlayerRequestSpawn(playerid);
 forward OnObjectMoved(objectid);
 forward OnPlayerObjectMoved(playerid, objectid);
 forward OnPlayerPickUpPickup(playerid, pickupid);
-forward OnVehicleMod(playerid, vehicleid, componentid);
+[Cancelable] forward OnVehicleMod(playerid, vehicleid, componentid);
 forward OnEnterExitModShop(playerid, enterexit, interiorid);
 forward OnVehiclePaintjob(playerid, vehicleid, paintjobid);
 forward OnVehicleRespray(playerid, vehicleid, color1, color2);

javascript/entities/vehicle_manager.js

@@ -4,6 +4,8 @@
 
 import { ScopedCallbacks } from 'base/scoped_callbacks.js';
 
+import { canVehicleModelHaveComponent } from 'entities/vehicle_components.js';
+
 // The vehicle manager is in control of all vehicles that have been created by the JavaScript code
 // of Las Venturas Playground. It deliberately does not provide access to the Pawn vehicles.
 export class VehicleManager {
@@ -116,16 +118,36 @@ export class VehicleManager {
 
     // ---------------------------------------------------------------------------------------------
 
+    // Called when a vehicle is being modified by a particular player. This event is cancelable, and
+    // should be canceled when an illegal modification is being made. This is a tactic that players
+    // with malicious intent often use for crashing others.
     onVehicleMod(event) {
         const player = server.playerManager.getById(event.playerid);
-        if (!player)
+        const vehicle = this.vehicles_.get(event.vehicleid);
+
+        // If either the |player| or the |vehicle| is invalid, bail out and prevent the default
+        // processing from happening. We don't have enough information to blame a cheater.
+        if (!player || !vehicle) {
+            console.log(`[exception] Vehicle modification found with invalid player/vehicle.`);
+
+            event.preventDefault();
             return;
+        }
 
-        const vehicle = this.vehicles_.get(event.vehicleid);
-        if (!vehicle)
+        const componentId = event.componentid;
+
+        // If the |vehicle| is not allowed to have the |componentId|, we cancel the event, notify
+        // observers of an illegal vehicle modification, and bail out.
+        if (!canVehicleModelHaveComponent(vehicle.modelId, componentId)) {
+            this.notifyObservers('onVehicleIllegalModification', player, vehicle, componentId);
+
+            event.preventDefault();
             return;
+        }
+
+        // TODO: Track modifications for the vehicle.
 
-        this.notifyObservers('onVehicleMod', player, vehicle, event.componentid);
+        this.notifyObservers('onVehicleMod', player, vehicle, componentId);
     }
 
     onVehiclePaintjob(event) {

javascript/features/abuse/abuse_monitor.js

@@ -5,6 +5,8 @@
 import { AbuseDatabase } from 'features/abuse/abuse_database.js';
 import { AbuseDetector } from 'features/abuse/abuse_detector.js';
 
+import { getComponentName } from 'entities/vehicle_components.js';
+
 // The abuse monitor keeps track of instances of abuse across the active players. A series of abuse
 // detectors are responsible for detecting it, after which it ends up here for decision making and
 // distribution of the knowledge to in-game administrators.
@@ -15,6 +17,8 @@ export class AbuseMonitor {
         this.settings_ = settings;
 
         provideNative('ReportAbuse', 'iss', AbuseMonitor.prototype.reportAbusePawn.bind(this));
+
+        server.vehicleManager.addObserver(this);
     }
 
     // Reports abuse by the given |player|, indicating that they've been observed exercising the
@@ -61,7 +65,28 @@ export class AbuseMonitor {
         return 1;
     }
 
+    // ---------------------------------------------------------------------------------------------
+
+    // Called when the |player| has made an illegal vehicle modification. This could be used to
+    // crash other players on the server, and thus is considered certain abuse.
+    onVehicleIllegalModification(player, vehicle, componentId) {
+        // (1) Report abuse for the |player|.
+        this.reportAbuse(player, 'illegal vehicle modification', AbuseDetector.kDetected, {
+            vehicleModelId: vehicle.modelId,
+            vehicleModelName: vehicle.model.name,
+            vehicleComponentId: componentId,
+            vehicleComponentName: getComponentName(componentId),
+        });
+
+        // (2) Immediately, without delay, remove the |player| from the server.
+        player.kick();
+    }
+
+    // ---------------------------------------------------------------------------------------------
+
     dispose() {
+        server.vehicleManager.removeObserver(this);
+
         provideNative('ReportAbuse', 'iss', (playerid, detectorName, certainty) => 0);
     }
 }

javascript/features/abuse/abuse_monitor.test.js

@@ -2,40 +2,58 @@
 // Use of this source code is governed by the MIT license, a copy of which can
 // be found in the LICENSE file.
 
-import { AbuseMonitor } from 'features/abuse/abuse_monitor.js';
-
 describe('AbuseMonitor', (it, beforeEach) => {
-    return;  // disabled for now
-
+    let gunther = null;
     let monitor = null;
     let settings = null;
 
     beforeEach(() => {
+        gunther = server.playerManager.getById(/* Gunther= */ 0);
         monitor = server.featureManager.loadFeature('abuse').monitor_;
         settings = server.featureManager.loadFeature('settings');
     });
 
-    it('should be able to detect and kick fake non-player characters', assert => {
-        const russell = server.playerManager.getById(1 /* Russell */);
+    it('should kick players for illegal vehicle modifications', assert => {
+        const vehicle = server.vehicleManager.createVehicle({
+            modelId: 411,  // Infernus
+
+            position: new Vector(0, 0, 0),
+            rotation: 180,
+        });
+
+        gunther.enterVehicle(vehicle);
+
+        assert.isTrue(gunther.isConnected());
+        assert.isTrue(vehicle.isConnected());
 
-        russell.level = Player.LEVEL_ADMINISTRATOR;
+        assert.strictEqual(gunther.vehicle, vehicle);
 
-        // Connect the evil bot to the server. They should be kicked immediately after.
-        server.playerManager.onPlayerConnect({
-            playerid: 42,
-            name: 'EvilBot',
-            ip: '42.42.42.42',
-            npc: true
+        let defaultPrevented = false;
+
+        // (1) Have |gunther| make a valid modification to the |vehicle|.
+        dispatchEvent('vehiclemod', {
+            preventDefault: () => defaultPrevented = true,
+
+            playerid: gunther.id,
+            vehicleid: vehicle.id,
+            componentid: 1075,  // Rimshine Wheels
         });
 
-        assert.isNull(server.playerManager.getById(42 /* evilbot */));
+        assert.isFalse(defaultPrevented);
+        assert.isTrue(gunther.isConnected());
+        // TODO: Verify that |vehicle| has the component.
 
-        assert.equal(russell.messages.length, 1);
-        assert.isTrue(
-            russell.messages[0].includes(
-                Message.format(Message.ABUSE_ANNOUNCE_KICKED, 'EvilBot', 42,
-                               'illegal non-player character')));
+        // (2) Have |gunther| make an illegal modification to the |vehicle|.
+        dispatchEvent('vehiclemod', {
+            preventDefault: () => defaultPrevented = true,
 
-    });
+            playerid: gunther.id,
+            vehicleid: vehicle.id,
+            componentid: 1144,  // Left Square Vents, not available for Infernus
+        });
 
+        assert.isTrue(defaultPrevented);
+        assert.isFalse(gunther.isConnected());
+        // TODO: Verify that |vehicle| does not have the component.
+    });
 });

javascript/features/finance/financial_disposition_monitor.test.js

@@ -95,7 +95,7 @@ describe('FinancialDispositionMonitor', (it, beforeEach, afterEach) => {
             dispatchEvent('vehiclemod', {
                 playerid: gunther.id,
                 vehicleid: vehicle.id,
-                componentid: 1000  // spoiler
+                componentid: 1008  // 5x Nitro
             });
 
             await server.clock.advance(kDispositionMonitorSpinDelay);

pawn/Entities/Vehicles/VehicleEvents.pwn

@@ -141,19 +141,6 @@ public OnVehicleMod(playerid, vehicleid, componentid) {
     if (Player(playerid)->isConnected() == false || Player(playerid)->isNonPlayerCharacter() == true)
         return 0; // don't handle invalid players or NPCs
 
-    if (!IsLegalModification(vehicleid, componentid)) {
-        new message[128];
-
-        format(message, sizeof(message), "%s (Id:%d) made an illegal vehicle modification with component %d.",
-            Player(playerid)->nicknameString(), playerid, componentid);
-        Admin(playerid, message);
-
-        // Kick them from Las Venturas Playground
-        Player(playerid)->kick("Illegal vehicle modification");
-
-        return 0; // don't handle illegal modifications
-    }
-
     if (!VehicleModel(GetVehicleModel(vehicleid))->isValidComponent(componentid))
         return false;