From fb264971634d877c8d7f8dc3d93c608d6d879a0f Mon Sep 17 00:00:00 2001 From: max bittker Date: Sun, 11 Oct 2026 19:37:42 -0400 Subject: [PATCH] Harden Miscellania transfers and add coin dupe regression coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 10 ++ package.json | 2 +- server/PATCHES.md | 8 +- .../scripts/managing_miscellania.rs2 | 23 +-- .../test/fixtures/miscellania-coffers.ts | 155 ++++++++++++++++++ .../engine/test/miscellania-coffers.test.ts | 21 +++ 6 files changed, 204 insertions(+), 15 deletions(-) create mode 100644 server/engine/test/fixtures/miscellania-coffers.ts create mode 100644 server/engine/test/miscellania-coffers.test.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f91e99b79..93a6f5735 100644 --- a/.github/workflows/ci.yml +++ b/.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 diff --git a/package.json b/package.json index e54a10c36..b3254ffd2 100644 --- a/package.json +++ b/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" diff --git a/server/PATCHES.md b/server/PATCHES.md index ff1a6351d..d70a4dada 100644 --- a/server/PATCHES.md +++ b/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 diff --git a/server/content/scripts/minigames/game_managing_miscellania/scripts/managing_miscellania.rs2 b/server/content/scripts/minigames/game_managing_miscellania/scripts/managing_miscellania.rs2 index 2faf0ea59..a42c170cc 100644 --- a/server/content/scripts/minigames/game_managing_miscellania/scripts/managing_miscellania.rs2 +++ b/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 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); -} \ No newline at end of file +} diff --git a/server/engine/test/fixtures/miscellania-coffers.ts b/server/engine/test/fixtures/miscellania-coffers.ts new file mode 100644 index 000000000..286156e4e --- /dev/null +++ b/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[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, 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) { + 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'); +}); diff --git a/server/engine/test/miscellania-coffers.test.ts b/server/engine/test/miscellania-coffers.test.ts new file mode 100644 index 000000000..a8dc0ff0c --- /dev/null +++ b/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);