sanitize region input (#197)

region is sometimes user-controlled and allows for redirect attacks
in order to steal API keys. This is no longer possible.

README.rst

@@ -175,6 +175,8 @@ Added startTime and endTime params for match v5 api
 
 Documented ddragon weirdness
 
+Fix potential security issue with some common usage patterns
+
 v3.1.4 - 8/11/2021
 ~~~~~~~~~~~~~~~~~~
 Add LolStatus-V4 API. Didnt realize this existed until now.

src/riotwatcher/Handlers/IllegalArgumentError.py

@@ -0,0 +1,11 @@
+class IllegalArgumentError(RuntimeError):
+    def __init__(
+        self, argument_name: str, argument_value: str, extra_message: str = None
+    ):
+        message = (
+            f"Illegal value provided for argument '{argument_name}': '{argument_value}'"
+        )
+        if extra_message:
+            message += f" - {extra_message}"
+
+        super().__init__(message)

src/riotwatcher/Handlers/SanitationHandler.py

@@ -0,0 +1,37 @@
+import re
+
+from . import IllegalArgumentError, RequestHandler
+
+
+class SanitationHandler(RequestHandler):
+    """
+    The SanitationHandler class provides some basic sanity checks to parameters to
+    ensure safe usage.
+
+    Only check as of now is ensuring that region doesn't cause HTTP requests to unknown
+    servers, which would allow a malicious user to steal API keys.
+    """
+
+    def __init__(self):
+        self._region_expr = re.compile("[a-zA-Z0-9]+")
+
+    def preview_request(
+        self,
+        region: str,
+        endpoint_name: str,
+        method_name: str,
+        url: str,
+        query_params: dict,
+    ):
+        """
+        called before a request is processed.
+
+        :param string endpoint_name: the name of the endpoint being requested
+        :param string method_name: the name of the method being requested
+        :param url: the URL that is being requested.
+        :param query_params: dict: the parameters to the url that is being queried,
+                                   e.g. ?key1=val&key2=val2
+        """
+        region_ok = self._region_expr.fullmatch(region)
+        if region_ok is None:
+            raise IllegalArgumentError("region", region)

src/riotwatcher/Handlers/__init__.py

@@ -1,8 +1,10 @@
+from .IllegalArgumentError import IllegalArgumentError
 from .RequestHandler import RequestHandler
 
 from .DeprecationHandler import DeprecationHandler
 from .DeserializerAdapter import DeserializerAdapter
 from .DictionaryDeserializer import DictionaryDeserializer
 from .RateLimiterAdapter import RateLimiterAdapter
+from .SanitationHandler import SanitationHandler
 from .TypeCorrectorHandler import TypeCorrectorHandler
 from .ThrowOnErrorHandler import ApiError, ThrowOnErrorHandler

src/riotwatcher/LolWatcher.py

@@ -9,6 +9,7 @@
     DeserializerAdapter,
     DictionaryDeserializer,
     RateLimiterAdapter,
+    SanitationHandler,
     ThrowOnErrorHandler,
     TypeCorrectorHandler,
 )
@@ -68,13 +69,15 @@ def __init__(
 
         if kernel_url:
             handler_chain = [
+                SanitationHandler(),
                 DeserializerAdapter(deserializer),
                 ThrowOnErrorHandler(),
                 TypeCorrectorHandler(),
                 DeprecationHandler(),
             ]
         else:
             handler_chain = [
+                SanitationHandler(),
                 DeserializerAdapter(deserializer),
                 ThrowOnErrorHandler(),
                 TypeCorrectorHandler(),

src/riotwatcher/LorWatcher.py

@@ -6,6 +6,7 @@
     DeserializerAdapter,
     DictionaryDeserializer,
     RateLimiterAdapter,
+    SanitationHandler,
     ThrowOnErrorHandler,
     TypeCorrectorHandler,
 )
@@ -42,6 +43,7 @@ def __init__(
             raise ValueError("api_key must be set!")
 
         handler_chain = [
+            SanitationHandler(),
             DeserializerAdapter(deserializer),
             ThrowOnErrorHandler(),
             TypeCorrectorHandler(),

src/riotwatcher/TftWatcher.py

@@ -6,6 +6,7 @@
     DeserializerAdapter,
     DictionaryDeserializer,
     RateLimiterAdapter,
+    SanitationHandler,
     ThrowOnErrorHandler,
     TypeCorrectorHandler,
 )
@@ -43,6 +44,7 @@ def __init__(
             raise ValueError("api_key must be set!")
 
         handler_chain = [
+            SanitationHandler(),
             DeserializerAdapter(deserializer),
             ThrowOnErrorHandler(),
             TypeCorrectorHandler(),

src/riotwatcher/ValWatcher.py

@@ -6,6 +6,7 @@
     DeserializerAdapter,
     DictionaryDeserializer,
     RateLimiterAdapter,
+    SanitationHandler,
     ThrowOnErrorHandler,
     TypeCorrectorHandler,
 )
@@ -42,6 +43,7 @@ def __init__(
             raise ValueError("api_key must be set!")
 
         handler_chain = [
+            SanitationHandler(),
             DeserializerAdapter(deserializer),
             ThrowOnErrorHandler(),
             TypeCorrectorHandler(),

src/riotwatcher/__init__.py

@@ -1,6 +1,6 @@
 from .__version__ import __author__, __title__, __version__
 
-from .exceptions import ApiError, TimeoutError
+from .exceptions import ApiError, IllegalArgumentError, TimeoutError
 from .Deserializer import Deserializer
 from .RateLimiter import RateLimiter
 from .LolWatcher import LolWatcher
@@ -20,5 +20,6 @@
     "ValWatcher",
     "Handlers",
     "ApiError",
+    "IllegalArgumentError",
     "TimeoutError",
 ]

src/riotwatcher/exceptions.py

@@ -1,6 +1,10 @@
 import requests
 
-from .Handlers import ApiError as _ApiError
+from .Handlers import (
+    ApiError as _ApiError,
+    IllegalArgumentError as _IllegalArgumentError,
+)
 
 ApiError = _ApiError  # should silence code analysis warning
+IllegalArgumentError = _IllegalArgumentError
 TimeoutError = requests.exceptions.Timeout

src/riotwatcher/riotwatcher.py

@@ -6,6 +6,7 @@
     DeserializerAdapter,
     DictionaryDeserializer,
     RateLimiterAdapter,
+    SanitationHandler,
     ThrowOnErrorHandler,
     TypeCorrectorHandler,
 )
@@ -42,6 +43,7 @@ def __init__(
             raise ValueError("api_key must be set!")
 
         handler_chain = [
+            SanitationHandler(),
             DeserializerAdapter(deserializer),
             ThrowOnErrorHandler(),
             TypeCorrectorHandler(),

tests/Handlers/test_SanitationHandler.py

@@ -0,0 +1,22 @@
+import pytest
+
+from riotwatcher.Handlers import IllegalArgumentError, SanitationHandler
+
+
+@pytest.mark.common
+@pytest.mark.unit
+class TestSanitationHandler:
+    @pytest.mark.parametrize(
+        "region", ["na1", "Na1", "aN1", "enue", "americas", "europe", "euw1"]
+    )
+    def test_valid_region_passes(self, region):
+        handler = SanitationHandler()
+
+        handler.preview_request(region, None, None, None, None)
+
+    @pytest.mark.parametrize("region", ["", "google.com/?stolen=", "+", "na1?"])
+    def test_invalid_region_fails(self, region):
+        handler = SanitationHandler()
+
+        with pytest.raises(IllegalArgumentError):
+            handler.preview_request(region, None, None, None, None)

tests/test_LolWatcher.py

@@ -1,6 +1,6 @@
 import pytest
 
-from riotwatcher import LolWatcher
+from riotwatcher import LolWatcher, IllegalArgumentError
 from riotwatcher._apis.league_of_legends import (
     LolStatusApiV3,
     LolStatusApiV4,
@@ -34,3 +34,8 @@ def test_uses_status_v3_when_false(self):
     def test_uses_status_v4_when_true(self):
         watcher = LolWatcher(api_key="RGAPI-this-is-a-fake", default_status_v4=True)
         assert isinstance(watcher.lol_status, LolStatusApiV4)
+
+    def test_stealing_api_key_doesnt_work(self):
+        watcher = LolWatcher(api_key="RGAPI-this-is-a-fake")
+        with pytest.raises(IllegalArgumentError):
+            watcher.lol_status.shard_data("example.com/?stolen-request=")

tests/test_LorWatcher.py

@@ -1,6 +1,6 @@
 import pytest
 
-from riotwatcher import LorWatcher
+from riotwatcher import LorWatcher, IllegalArgumentError
 
 
 @pytest.mark.lor
@@ -15,3 +15,8 @@ def test_allows_positional_api_key(self):
 
     def test_allows_keyword_api_key(self):
         LorWatcher(api_key="RGAPI-this-is-a-fake")
+
+    def test_stealing_api_key_doesnt_work(self):
+        watcher = LorWatcher(api_key="RGAPI-this-is-a-fake")
+        with pytest.raises(IllegalArgumentError):
+            watcher.ranked.leaderboards("example.com/?stolen-request=")

tests/test_RiotWatcher.py

@@ -1,6 +1,6 @@
 import pytest
 
-from riotwatcher import RiotWatcher
+from riotwatcher import RiotWatcher, IllegalArgumentError
 
 
 @pytest.mark.riot
@@ -15,3 +15,8 @@ def test_allows_positional_api_key(self):
 
     def test_allows_keyword_api_key(self):
         RiotWatcher(api_key="RGAPI-this-is-a-fake")
+
+    def test_stealing_api_key_doesnt_work(self):
+        watcher = RiotWatcher(api_key="RGAPI-this-is-a-fake")
+        with pytest.raises(IllegalArgumentError):
+            watcher.account.by_puuid("example.com/?stolen-request=", "fake-puuid")

tests/test_TftWatcher.py

@@ -1,6 +1,6 @@
 import pytest
 
-from riotwatcher import TftWatcher
+from riotwatcher import TftWatcher, IllegalArgumentError
 
 
 @pytest.mark.tft
@@ -15,3 +15,8 @@ def test_allows_positional_api_key(self):
 
     def test_allows_keyword_api_key(self):
         TftWatcher(api_key="RGAPI-this-is-a-fake")
+
+    def test_stealing_api_key_doesnt_work(self):
+        watcher = TftWatcher(api_key="RGAPI-this-is-a-fake")
+        with pytest.raises(IllegalArgumentError):
+            watcher.summoner.by_puuid("example.com/?stolen-request=", "fake-puuid")

tests/test_ValWatcher.py

@@ -1,6 +1,6 @@
 import pytest
 
-from riotwatcher import ValWatcher
+from riotwatcher import ValWatcher, IllegalArgumentError
 
 
 @pytest.mark.val
@@ -15,3 +15,8 @@ def test_allows_positional_api_key(self):
 
     def test_allows_keyword_api_key(self):
         ValWatcher(api_key="RGAPI-this-is-a-fake")
+
+    def test_stealing_api_key_doesnt_work(self):
+        watcher = ValWatcher(api_key="RGAPI-this-is-a-fake")
+        with pytest.raises(IllegalArgumentError):
+            watcher.match.by_id("example.com/?stolen-request=", "fake-match")