diff --git a/data/submissions/drop.py b/data/submissions/drop.py index 62f6f7f..74d148a 100755 --- a/data/submissions/drop.py +++ b/data/submissions/drop.py @@ -29,6 +29,7 @@ SEASONAL_WORLD_TYPE, SeasonalDrop, ) +from utils.valuation_guard import sanitize_quantity, exceeds_value_sanity_limit redis_client = RedisClient() @@ -223,7 +224,9 @@ def log_checkpoint(label: str): value = drop_data["value"] item_id = drop_data.get("item_id", drop_data.get("id", None)) item_name = drop_data.get("item_name", drop_data.get("item", None)) - quantity = drop_data["quantity"] + quantity = sanitize_quantity(drop_data.get("quantity")) + if quantity is None: + return SubmissionResponse(success=False, message="Invalid drop quantity") auth_key = drop_data.get("auth_key", None) player_name = drop_data.get("player_name", drop_data.get("player", None)) account_hash = drop_data["acc_hash"] @@ -346,6 +349,17 @@ def log_checkpoint(label: str): f"Drop value calculated - Raw: {raw_drop_value}, Total: {drop_value} ({quantity}x)" ) + # Reject implausible totals (spoofed quantity on a valuable item). + if exceeds_value_sanity_limit(raw_drop_value, quantity): + print( + f"[DropGuard] Rejected implausible drop total: {item_name} " + f"unit={raw_drop_value} x qty={quantity} from {player_name}" + ) + return SubmissionResponse( + success=False, + message="Drop value exceeds the plausible maximum and was rejected", + ) + if drop_value > 1000000: debug_print(f"High value drop detected, verifying item/NPC combination...") # Offload the wiki lookup to a worker thread so a slow/hanging diff --git a/tests/unit/test_valuation_guard.py b/tests/unit/test_valuation_guard.py new file mode 100644 index 0000000..34204c7 --- /dev/null +++ b/tests/unit/test_valuation_guard.py @@ -0,0 +1,44 @@ +"""Regression tests for the quantity / total-value sanity guards.""" +from utils.valuation_guard import ( + MAX_SINGLE_DROP_TOTAL, + sanitize_quantity, + exceeds_value_sanity_limit, +) + + +def test_sanitize_quantity_accepts_positive_ints(): + assert sanitize_quantity(1) == 1 + assert sanitize_quantity("1000") == 1000 + assert sanitize_quantity(65_535) == 65_535 + + +def test_sanitize_quantity_rejects_non_positive_and_junk(): + assert sanitize_quantity(0) is None + assert sanitize_quantity(-5) is None + assert sanitize_quantity("-5") is None + assert sanitize_quantity("abc") is None + assert sanitize_quantity(None) is None + + +def test_large_but_realistic_stack_is_within_limit(): + assert exceeds_value_sanity_limit(unit_value=1, quantity=1_000_000) is False + assert exceeds_value_sanity_limit(unit_value=1_200_000_000, quantity=1) is False + + +def test_top_tier_single_item_within_limit(): + # 3rd age-tier item (GE integer ceiling) at quantity 1. + assert exceeds_value_sanity_limit(unit_value=2_147_483_647, quantity=1) is False + + +def test_total_above_top_tier_single_item_is_flagged(): + assert exceeds_value_sanity_limit(unit_value=5_000_000_000, quantity=1) is True + + +def test_absurd_total_is_flagged(): + # 1.2B x 1000 = the demonstrated exploit. + assert exceeds_value_sanity_limit(unit_value=1_200_000_000, quantity=1000) is True + + +def test_limit_boundary(): + assert exceeds_value_sanity_limit(MAX_SINGLE_DROP_TOTAL, 1) is False + assert exceeds_value_sanity_limit(MAX_SINGLE_DROP_TOTAL + 1, 1) is True diff --git a/tests/unit/test_value_authoritative.py b/tests/unit/test_value_authoritative.py new file mode 100644 index 0000000..bf04037 --- /dev/null +++ b/tests/unit/test_value_authoritative.py @@ -0,0 +1,62 @@ +"""Regression tests: get_true_item_value must not trust a spoofable client value. + +Loads the real utils.ge_value in isolation (conftest otherwise stubs it), +stubbing its infra imports so the valuation branch runs without Redis / GE / aiohttp. +""" +import importlib.util +import os +import sys +import types +from unittest.mock import AsyncMock, MagicMock + +import pytest + +_REPO_ROOT = os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) + + +def _load_real_ge_value(): + """Import utils/ge_value.py fresh, bypassing the conftest stub.""" + sys.modules["aiohttp"] = MagicMock() + + redis_stub = types.ModuleType("utils.redis") + redis_stub.RedisClient = MagicMock + sys.modules["utils.redis"] = redis_stub + + vo_stub = types.ModuleType("utils.value_overrides") + vo_stub.match = lambda item_id, item_name: None + sys.modules["utils.value_overrides"] = vo_stub + + path = os.path.join(_REPO_ROOT, "utils", "ge_value.py") + spec = importlib.util.spec_from_file_location("_real_ge_value_under_test", path) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +@pytest.fixture() +def ge(): + return _load_real_ge_value() + + +@pytest.mark.asyncio +async def test_nonzero_client_value_is_overridden_by_server_price(ge): + """A huge client value for a priced item is replaced by the real GE price.""" + ge._lookup_and_cache_ge_price = AsyncMock(return_value=1_200_000_000) + result = await ge.get_true_item_value("Twisted bow", 2_000_000_000, item_id=20997) + assert result == 1_200_000_000 # server price wins, not the client's 2,000,000,000 + + +@pytest.mark.asyncio +async def test_client_value_used_when_item_has_no_server_price(ge): + """Untradeables (no GE price) still fall back to the client-reported value.""" + ge._lookup_and_cache_ge_price = AsyncMock(return_value=0) + result = await ge.get_true_item_value("Some untradeable", 5000, item_id=999999) + assert result == 5000 + + +@pytest.mark.asyncio +async def test_zero_client_value_still_uses_server_price(ge): + """Existing behaviour preserved: value=0 recovers the real GE price.""" + ge._lookup_and_cache_ge_price = AsyncMock(return_value=1_200_000_000) + result = await ge.get_true_item_value("Twisted bow", 0, item_id=20997) + assert result == 1_200_000_000 diff --git a/utils/ge_value.py b/utils/ge_value.py index 59780c1..46eb1b9 100755 --- a/utils/ge_value.py +++ b/utils/ge_value.py @@ -102,10 +102,11 @@ async def get_true_item_value(item_name, provided_value: int = 0, item_id=None): fallback = override.get("fallback_value") or 0 return fallback if fallback else provided_value - # No override: when the client reports 0 gp, try to recover the real GE price - # so that notification thresholds and point awards aren't silently skipped. - if provided_value == 0: - return await _lookup_and_cache_ge_price(item_name, item_id) + # The client-supplied value is spoofable, so prefer the server GE price for + # any priceable item; fall back to the client value only when unpriceable. + server_price = await _lookup_and_cache_ge_price(item_name, item_id) + if server_price: + return server_price return provided_value async def get_mapping(): diff --git a/utils/valuation_guard.py b/utils/valuation_guard.py new file mode 100644 index 0000000..af0621b --- /dev/null +++ b/utils/valuation_guard.py @@ -0,0 +1,25 @@ +"""Sanity guards for client-supplied drop quantities and totals (stdlib only).""" +from __future__ import annotations + +from typing import Optional + +# OSRS Grand Exchange integer cap: no legitimate single drop's total value +# exceeds this. Valuable uniques drop at quantity 1; large stacks are low-value. +MAX_SINGLE_DROP_TOTAL = 2_147_483_647 + + +def sanitize_quantity(raw) -> Optional[int]: + """Coerce a submitted quantity to a positive int, or None if invalid.""" + try: + quantity = int(raw) + except (TypeError, ValueError): + return None + return quantity if quantity > 0 else None + + +def exceeds_value_sanity_limit(unit_value, quantity) -> bool: + """True when unit_value * quantity exceeds the limit (fail closed on junk).""" + try: + return int(unit_value) * int(quantity) > MAX_SINGLE_DROP_TOTAL + except (TypeError, ValueError): + return True