Harden Miscellania transfers and add coin dupe regression coverage

Follow up kadeski’s fix in PR #73 with current-balance and inventory-capacity checks. Keep the original report and authorship credited in the vendor patch checklist. Exercise both reported exploit inputs in compiled-script tests, and build engine content for those tests in CI.

.github/workflows/ci.yml

@@ -25,5 +25,15 @@ jobs:
         working-directory: server/webclient
         run: bun install --frozen-lockfile
 
+      - name: Install engine dependencies
+        working-directory: server/engine
+        run: bun install --frozen-lockfile
+
+      - name: Build game content for compiled-script regression tests
+        working-directory: server/engine
+        env:
+          BUILD_VERIFY: "false"
+        run: bun run build
+
       - name: Type-check, test, and verify generated docs
         run: bun run check

package.json

@@ -6,7 +6,7 @@
     "docs:api": "bun sdk/generate-api-docs.ts",
     "docs:api:check": "bun sdk/generate-api-docs.ts --check",
     "fetch-collision-data": "bun sdk/fetch-collision-data.ts",
-    "test": "bun test sdk/test server/webclient/src/bot server/webclient/src/util server/webclient/src/lite server/webclient/src/mapview server/engine/test/market.test.ts server/engine/test/market-web.test.ts server/engine/test/item-search.test.ts server/engine/test/file-stream.test.ts server/engine/test/get-ip.test.ts server/engine/test/bank-hiscores.test.ts server/engine/test/reconnect.test.ts server/engine/test/worldfeed.test.ts server/gateway/state-fanout.test.ts server/engine/test/hiscores-performance.test.ts",
+    "test": "bun test sdk/test server/webclient/src/bot server/webclient/src/util server/webclient/src/lite server/webclient/src/mapview server/engine/test/market.test.ts server/engine/test/market-web.test.ts server/engine/test/item-search.test.ts server/engine/test/file-stream.test.ts server/engine/test/get-ip.test.ts server/engine/test/bank-hiscores.test.ts server/engine/test/reconnect.test.ts server/engine/test/worldfeed.test.ts server/gateway/state-fanout.test.ts server/engine/test/hiscores-performance.test.ts server/engine/test/miscellania-coffers.test.ts",
     "typecheck": "tsc --noEmit",
     "typecheck:webclient": "tsc --noEmit -p server/webclient/tsconfig.json",
     "test:market:integration": "cd server/engine && bun test/market-integration.ts && bun test/market-recovery.ts && bun test/market-protocol.ts && bun test/market-disabled.ts"

server/PATCHES.md

@@ -374,9 +374,11 @@ Upgrade hazards:
 - [ ] **`scripts/macro events/scripts/macro_events.rs2`** — macro events disabled when off.
 - [ ] **`scripts/shop/configs/shop.varp`** — `transmit=yes` on shop varps (bot shop state).
 - [ ] **`scripts/minigames/game_managing_miscellania/scripts/managing_miscellania.rs2`** — coffers
-      deposit clamps to `min(requested, coffer space, coins held)`; withdraw returns on a count of
-      0 or less. Reported upstream to LostCity; drop this patch once their 289 branch carries a fix.
-      Verify: `grep -n "withdraw_amt <= 0" scripts/minigames/game_managing_miscellania/scripts/managing_miscellania.rs2`
+      deposit clamps to `min(requested, coffer space, current coins held)` after the count dialog;
+      withdraw rejects non-positive counts and checks the current coffer balance and inventory
+      capacity before transferring money. Report and initial fix by kadeski ([PR #73](https://github.com/MaxBittker/rs-sdk/pull/73)).
+      Reported upstream to LostCity; drop this patch once their 289 branch carries a fix.
+      Verify: from the repo root, `bun test server/engine/test/miscellania-coffers.test.ts` (requires built engine content).
 - [x] **README art** — moved out of the vendored tree to `docs/media/` (promo.gif, discord.svg,
       hiscores.svg, task_length.svg); nothing in `content/title/` is locally modified anymore.
 - [ ] Smithing arrowheads + telegrab + nails fixes live in content history; they're additive

server/content/scripts/minigames/game_managing_miscellania/scripts/managing_miscellania.rs2

@@ -85,7 +85,7 @@ if ($inv_coins = 0) {
     ~open_manage_miscellania_interface;
     return;
 } 
-if (%misc_coffers = 5000000) {
+if (%misc_coffers >= 5000000) {
     ~mesbox("The coffers can only hold 5000000 coins.");
     ~open_manage_miscellania_interface;
     return;
@@ -98,7 +98,8 @@ def_int $requested_deposit = last_int;
 if_close;
 p_delay(0);
 def_int $coffer_space = sub(5000000, %misc_coffers);
-def_int $deposit_amt = min($requested_deposit, min($coffer_space, $inv_coins));
+// Re-read the inventory after the dialog and apply both limits independently.
+def_int $deposit_amt = min($requested_deposit, min($coffer_space, inv_total(inv, coins)));
 
 if ($deposit_amt > 0) {
     inv_del(inv, coins, $deposit_amt);
@@ -109,16 +110,11 @@ if ($deposit_amt > 0) {
 
 // Withdraw
 [if_button,misc_both_manage:com_89]
-if (%misc_coffers = 0) {
+if (%misc_coffers <= 0) {
     ~mesbox("There are no coins to withdraw.");
     ~open_manage_miscellania_interface;
     return;
 }
-if(inv_itemspace(inv, coins, %misc_coffers, inv_size(inv)) = false) {
-    ~mesbox("You do not have enough space to hold any coins.");
-    ~open_manage_miscellania_interface;
-    return;
-}
 ~mesbox("The coffers contain <tostring(%misc_coffers)> coins.|Enter an amount to withdraw from the coffers.");
 if_close;
 p_delay(0);
@@ -126,12 +122,17 @@ p_countdialog;
 def_int $withdraw_amt = last_int;
 if_close;
 p_delay(0);
+// Re-read the balance and reject signed, non-positive input before changing money.
+$withdraw_amt = min($withdraw_amt, %misc_coffers);
 if ($withdraw_amt <= 0) {
     ~open_manage_miscellania_interface;
     return;
 }
-if ($withdraw_amt > %misc_coffers) {
-    $withdraw_amt = %misc_coffers;
+// Inventory contents may have changed while the amount dialog was open.
+if (inv_itemspace(inv, coins, $withdraw_amt, inv_size(inv)) = false) {
+    ~mesbox("You do not have enough space to hold any coins.");
+    ~open_manage_miscellania_interface;
+    return;
 }
 %misc_coffers = sub(%misc_coffers, $withdraw_amt);
 inv_add(inv, coins, $withdraw_amt);
@@ -364,4 +365,4 @@ if ($random < 10) {
     return (unidentified_lantadyme, 1);
 } else if ($random < 46) {
     return (unidentified_dwarf_weed, 1);
-}
+}

server/engine/test/fixtures/miscellania-coffers.ts

@@ -0,0 +1,155 @@
+import { expect, mock, test } from 'bun:test';
+import { EventEmitter } from 'node:events';
+import { copyFileSync, mkdirSync, writeFileSync } from 'node:fs';
+import { resolve } from 'node:path';
+import { CompileServerScript } from '@lostcityrs/runescript';
+
+const output = process.env.MISCELLANIA_TEST_OUTPUT!;
+const cache = resolve('data/pack');
+const compile = CompileServerScript;
+mkdirSync(`${output}/source`);
+copyFileSync(resolve('../content/scripts/engine.rs2'), `${output}/source/engine.rs2`);
+copyFileSync(resolve('../content/scripts/minigames/game_managing_miscellania/scripts/managing_miscellania.rs2'), `${output}/source/managing_miscellania.rs2`);
+// Only message rendering is stubbed; the actual money handlers and their dependencies compile normally.
+writeFileSync(`${output}/source/messages.rs2`, '[proc,mesbox](string $message)\nreturn;\n');
+// Keep the production compiler's symbols and pointer checks, but never overwrite the server cache.
+mock.module('@lostcityrs/runescript', () => ({
+    CompileServerScript: (config: Parameters<typeof CompileServerScript>[0]) =>
+        compile({ ...config, sourcePaths: [`${output}/source`], writer: { jag: { output: `${output}/server` } } })
+}));
+const { runServerCompiler } = await import('../../tools/pack/Compiler.js');
+runServerCompiler();
+
+mkdirSync(`${output}/data/config`, { recursive: true });
+copyFileSync(resolve('data/config/private.pem'), `${output}/data/config/private.pem`);
+process.chdir(output);
+class TestWorker extends EventEmitter { postMessage() {} }
+mock.module('worker_threads', () => ({ Worker: TestWorker }));
+await import('../../src/engine/World.js');
+const { default: ObjType } = await import('../../src/cache/config/ObjType.js');
+const { default: InvType } = await import('../../src/cache/config/InvType.js');
+const { default: VarBitType } = await import('../../src/cache/config/VarBitType.js');
+const { default: ScriptProvider } = await import('../../src/engine/script/ScriptProvider.js');
+const { default: ScriptRunner } = await import('../../src/engine/script/ScriptRunner.js');
+const { default: ScriptState } = await import('../../src/engine/script/ScriptState.js');
+const { default: ScriptPointer } = await import('../../src/engine/script/ScriptPointer.js');
+const { NetworkPlayer } = await import('../../src/engine/entity/NetworkPlayer.js');
+const { default: NullClientSocket } = await import('../../src/server/NullClientSocket.js');
+ObjType.load(cache);
+InvType.load(cache);
+VarBitType.load(cache);
+expect(ScriptProvider.load(output)).toBeGreaterThan(0);
+
+const inv = InvType.getId('inv');
+const coins = ObjType.getId('coins');
+const coffers = VarBitType.getId('misc_coffers');
+
+function player(balance: number, cash: number) {
+    const p = new NetworkPlayer('coffertest', 1n, 1n, new NullClientSocket());
+    p.setVarBit(coffers, balance);
+    if (cash > 0) p.invAdd(inv, coins, cash);
+    return p;
+}
+
+function run(p: InstanceType<typeof NetworkPlayer>, button: 88 | 89, input: number, atDialog?: () => void) {
+    const script = ScriptProvider.getByName(`[if_button,misc_both_manage:com_${button}]`)!;
+    expect(script).toBeDefined();
+    const state = ScriptRunner.init(script, p);
+    state.pointerAdd(ScriptPointer.ProtectedActivePlayer);
+    let requested = false;
+    for (let step = 0; step < 20; step++) {
+        const execution = ScriptRunner.execute(state);
+        expect(execution).not.toBe(ScriptState.ABORTED);
+        if (execution === ScriptState.FINISHED) return requested;
+        if (execution === ScriptState.COUNTDIALOG) {
+            expect(requested).toBe(false);
+            requested = true;
+            atDialog?.();
+            state.lastInt = input;
+        } else expect(execution).toBe(ScriptState.SUSPENDED);
+    }
+    throw new Error('Money handler did not finish');
+}
+
+function money(p: InstanceType<typeof NetworkPlayer>) {
+    return [p.getVarBit(coffers), p.invTotal(inv, coins)];
+}
+
+test('deposits and withdrawals preserve money and reject invalid or stale amounts', () => {
+    // The original exploit: coffer-space clamping must ALSO respect the inventory balance.
+    for (const [balance, cash, input, moved] of [
+        [0, 1, 2000000000, 1],
+        [0, 100, 2147483647, 100],
+        [4999000, 100, 2000, 100],
+        [4999000, 2000, 2147483647, 1000],
+        [0, 100, 50, 50],
+        [0, 100, 100, 100],
+        [0, 100, 101, 100],
+        [0, 100, 0, 0],
+        [0, 100, -1, 0],
+        [0, 100, -2147483648, 0],
+        [5000000, 100, 1, 0],
+        [5000001, 100, 1, 0],
+        [0, 0, 100, 0]
+    ]) {
+        const p = player(balance, cash);
+        run(p, 88, input);
+        expect(money(p)).toEqual([balance + moved, cash - moved]);
+    }
+
+    for (const input of [-2147483648, -1, 0, 1, 100, 101, 2147483647]) {
+        const p = player(100, 25);
+        const moved = Math.max(0, Math.min(input, 100));
+        run(p, 89, input);
+        expect(money(p)).toEqual([100 - moved, 25 + moved]);
+    }
+
+    const negative = player(1, 0);
+    run(negative, 89, -134217726);
+    expect(money(negative)).toEqual([1, 0]);
+
+    // Already-inflated coffers can be withdrawn, but new deposits must never expand them.
+    const inflated = player(134217727, 1);
+    run(inflated, 88, 2000000000);
+    expect(money(inflated)).toEqual([134217727, 1]);
+    run(inflated, 89, 134217727);
+    expect(money(inflated)).toEqual([0, 134217728]);
+
+    const changed = player(0, 1000);
+    run(changed, 88, 1000, () => changed.invDel(inv, coins, 900));
+    expect(money(changed)).toEqual([100, 0]);
+
+    const filled = player(4999000, 1000);
+    run(filled, 88, 1000, () => filled.setVarBit(coffers, 5000000));
+    expect(money(filled)).toEqual([5000000, 1000]);
+
+    const spent = player(100, 25);
+    run(spent, 89, 100, () => spent.setVarBit(coffers, 10));
+    expect(money(spent)).toEqual([0, 35]);
+
+    const emptied = player(100, 25);
+    run(emptied, 89, 100, () => emptied.setVarBit(coffers, 0));
+    expect(money(emptied)).toEqual([0, 25]);
+
+    const full = player(100, 0);
+    run(full, 89, 100, () => {
+        for (let slot = 0; slot < full.invSize(inv); slot++) full.invSet(inv, ObjType.getId('tinderbox'), 1, slot);
+    });
+    expect(money(full)).toEqual([100, 0]);
+
+    const overflow = player(100, 25);
+    run(overflow, 89, 100, () => overflow.invSet(inv, coins, 2147483647, 0));
+    expect(money(overflow)).toEqual([100, 2147483647]);
+
+    const partial = player(100, 2147483640);
+    run(partial, 89, 7);
+    expect(money(partial)).toEqual([93, 2147483647]);
+
+    const repeated = player(0, 100);
+    for (let cycle = 0; cycle < 10; cycle++) {
+        run(repeated, 88, 2147483647);
+        run(repeated, 89, 2147483647);
+        expect(money(repeated)).toEqual([0, 100]);
+    }
+    console.log('Miscellania coffer checks passed');
+});

server/engine/test/miscellania-coffers.test.ts

@@ -0,0 +1,21 @@
+import { expect, test } from 'bun:test';
+import { mkdtempSync, rmSync } from 'node:fs';
+import { tmpdir } from 'node:os';
+import { join } from 'node:path';
+
+test('compiled Miscellania money handlers conserve coins across dialog inputs', () => {
+    const dir = mkdtempSync(join(tmpdir(), 'miscellania-coffers-'));
+    try {
+        // Compile into a disposable cache and isolate engine workers/database from the main suite.
+        const result = Bun.spawnSync([process.execPath, 'test', join(import.meta.dir, 'fixtures/miscellania-coffers.ts')], {
+            cwd: join(import.meta.dir, '..'),
+            env: { ...process.env, GE_DATABASE: join(dir, 'market.sqlite'), MISCELLANIA_TEST_OUTPUT: dir },
+            stdout: 'pipe',
+            stderr: 'pipe'
+        });
+        if (result.exitCode !== 0) throw new Error(result.stdout.toString() + result.stderr.toString());
+        expect(result.stdout.toString()).toContain('Miscellania coffer checks passed');
+    } finally {
+        rmSync(dir, { recursive: true, force: true });
+    }
+}, 60000);