From 440ff28db8b19fe674558961b97a4479cc3304bd Mon Sep 17 00:00:00 2001
From: Will McGugan <willmcgugan@gmail.com>
Date: Tue, 29 Oct 2024 10:13:04 +0000
Subject: [PATCH] Better validation of environment variables (#5192)

* Better validation of environment variables

* wording
---
 src/textual/constants.py | 45 +++++++++++++++++++++++++++++++++-------
 tests/test_constants.py  | 31 +++++++++++++++++++++++++++
 2 files changed, 68 insertions(+), 8 deletions(-)
 create mode 100644 tests/test_constants.py

diff --git a/src/textual/constants.py b/src/textual/constants.py
index a9d7db719..f4d167388 100644
--- a/src/textual/constants.py
+++ b/src/textual/constants.py
@@ -27,24 +27,53 @@ def _get_environ_bool(name: str) -> bool:
     return has_environ
 
 
-def _get_environ_int(name: str, default: int) -> int:
+def _get_environ_int(name: str, default: int, minimum: int | None = None) -> int:
     """Retrieves an integer environment variable.
 
     Args:
         name: Name of environment variable.
         default: The value to use if the value is not set, or set to something other
             than a valid integer.
+        minimum: Optional minimum value.
 
     Returns:
         The integer associated with the environment variable if it's set to a valid int
             or the default value otherwise.
     """
     try:
-        return int(os.environ[name])
+        value = int(os.environ[name])
     except KeyError:
         return default
     except ValueError:
         return default
+    if minimum is not None:
+        return max(minimum, value)
+    return value
+
+
+def _get_environ_port(name: str, default: int) -> int:
+    """Get a port no. from an environment variable.
+
+    Note that there is no 'minimum' here, as ports are more like names than a scalar value.
+
+    Args:
+        name: Name of environment variable.
+        default: The value to use if the value is not set, or set to something other
+            than a valid port.
+
+    Returns:
+        An integer port number.
+
+    """
+    try:
+        value = int(os.environ[name])
+    except KeyError:
+        return default
+    except ValueError:
+        return default
+    if value < 0 or value > 65535:
+        return default
+    return value
 
 
 def _is_valid_animation_level(value: str) -> TypeGuard[AnimationLevel]:
@@ -89,11 +118,11 @@ LOG_FILE: Final[str | None] = get_environ("TEXTUAL_LOG", None)
 DEVTOOLS_HOST: Final[str] = get_environ("TEXTUAL_DEVTOOLS_HOST", "127.0.0.1")
 """The host where textual console is running."""
 
-DEVTOOLS_PORT: Final[int] = _get_environ_int("TEXTUAL_DEVTOOLS_PORT", 8081)
+DEVTOOLS_PORT: Final[int] = _get_environ_port("TEXTUAL_DEVTOOLS_PORT", 8081)
 """Constant with the port that the devtools will connect to."""
 
-SCREENSHOT_DELAY: Final[int] = _get_environ_int("TEXTUAL_SCREENSHOT", -1)
-"""Seconds delay before taking screenshot."""
+SCREENSHOT_DELAY: Final[int] = _get_environ_int("TEXTUAL_SCREENSHOT", -1, minimum=-1)
+"""Seconds delay before taking screenshot, -1 for no screenshot."""
 
 SCREENSHOT_LOCATION: Final[str | None] = get_environ("TEXTUAL_SCREENSHOT_LOCATION")
 """The location where screenshots should be written."""
@@ -107,7 +136,7 @@ PRESS: Final[str] = get_environ("TEXTUAL_PRESS", "")
 SHOW_RETURN: Final[bool] = _get_environ_bool("TEXTUAL_SHOW_RETURN")
 """Write the return value on exit."""
 
-MAX_FPS: Final[int] = _get_environ_int("TEXTUAL_FPS", 60)
+MAX_FPS: Final[int] = _get_environ_int("TEXTUAL_FPS", 60, minimum=1)
 """Maximum frames per second for updates."""
 
 COLOR_SYSTEM: Final[str | None] = get_environ("TEXTUAL_COLOR_SYSTEM", "auto")
@@ -116,10 +145,10 @@ COLOR_SYSTEM: Final[str | None] = get_environ("TEXTUAL_COLOR_SYSTEM", "auto")
 TEXTUAL_ANIMATIONS: Final[AnimationLevel] = _get_textual_animations()
 """Determines whether animations run or not."""
 
-ESCAPE_DELAY: Final[float] = _get_environ_int("ESCDELAY", 100) / 1000.0
+ESCAPE_DELAY: Final[float] = _get_environ_int("ESCDELAY", 100, minimum=1) / 1000.0
 """The delay (in seconds) before reporting an escape key (not used if the extend key protocol is available)."""
 
-SLOW_THRESHOLD: int = _get_environ_int("TEXTUAL_SLOW_THRESHOLD", 500)
+SLOW_THRESHOLD: int = _get_environ_int("TEXTUAL_SLOW_THRESHOLD", 500, minimum=100)
 """The time threshold (in milliseconds) after which a warning is logged 
 if message processing exceeds this duration.
 """
diff --git a/tests/test_constants.py b/tests/test_constants.py
new file mode 100644
index 000000000..9eb30bc02
--- /dev/null
+++ b/tests/test_constants.py
@@ -0,0 +1,31 @@
+from textual.constants import _get_environ_bool, _get_environ_int, _get_environ_port
+
+
+def test_environ_int(monkeypatch):
+    """Check minimum is applied."""
+    monkeypatch.setenv("FOO", "-1")
+    assert _get_environ_int("FOO", 1, minimum=0) == 0
+    monkeypatch.setenv("FOO", "0")
+    assert _get_environ_int("FOO", 1, minimum=0) == 0
+    monkeypatch.setenv("FOO", "1")
+    assert _get_environ_int("FOO", 1, minimum=0) == 1
+
+
+def test_environ_bool(monkeypatch):
+    """Anything other than "1" is False."""
+    monkeypatch.setenv("BOOL", "1")
+    assert _get_environ_bool("BOOL") is True
+    monkeypatch.setenv("BOOL", "")
+    assert _get_environ_bool("BOOL") is False
+    monkeypatch.setenv("BOOL", "0")
+    assert _get_environ_bool("BOOL") is False
+
+
+def test_environ_port(monkeypatch):
+    """Valid ports are between 0 and 65536."""
+    monkeypatch.setenv("PORT", "-1")
+    assert _get_environ_port("PORT", 80) == 80
+    monkeypatch.setenv("PORT", "0")
+    assert _get_environ_port("PORT", 80) == 0
+    monkeypatch.setenv("PORT", "65536")
+    assert _get_environ_port("PORT", 80) == 80