From 09dbf3bbd574635071c8b269b7aca9bd81fef145 Mon Sep 17 00:00:00 2001 From: Andreas Motl Date: Sat, 26 Oct 2024 17:11:15 +0200 Subject: [PATCH] CLI: Improve code using suggestions by @coderabbitai --- responder/util/cmd.py | 9 +++++++ responder/util/python.py | 2 +- tests/test_cli.py | 24 +++++++++++++++---- tests/util.py | 51 +++++++++++++++++++++++++++++----------- 4 files changed, 67 insertions(+), 19 deletions(-) diff --git a/responder/util/cmd.py b/responder/util/cmd.py index 4a9644dc..36271fdf 100644 --- a/responder/util/cmd.py +++ b/responder/util/cmd.py @@ -39,8 +39,17 @@ def __init__(self, target: str, port: int = 5042, limit_max_requests: int = None super().__init__() # Sanity checks, as suggested by @coderabbitai. Thanks. + + # Validate path, also prevent path traversal. if not target or not isinstance(target, str): raise ValueError("Target must be a non-empty string") + target_path = Path(target).resolve() + if not target_path.exists(): + raise ValueError(f"Target path does not exist: {target}") + if ".." in target_path.parts: + raise ValueError("Path traversal detected in target") + + # Validate port and other options. if not isinstance(port, int) or port < 1: raise ValueError("Port must be a positive integer") if limit_max_requests is not None and ( diff --git a/responder/util/python.py b/responder/util/python.py index 7c1984f9..f9d4ae2f 100644 --- a/responder/util/python.py +++ b/responder/util/python.py @@ -10,7 +10,7 @@ def load_target(target: str, default_property: str = "api", method: str = "run") """ # Sanity checks, as suggested by @coderabbitai. Thanks. - if not target or ":" in target and len(target.split(":")) != 2: + if not target or (":" in target and len(target.split(":")) != 2): raise ValueError( "Invalid target format. " "Use '', ':', " diff --git a/tests/test_cli.py b/tests/test_cli.py index 4c6ce8f7..89730a08 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -6,6 +6,7 @@ import pytest import requests from _pytest.capture import CaptureFixture +from requests_toolbelt.multipart.encoder import to_list from responder.__version__ import __version__ from responder.util.cmd import ResponderProgram, ResponderServer @@ -64,19 +65,34 @@ def test_cli_build_missing_package_json(capfd, tmp_path): assert "Error: ENOENT: no such file or directory" in stderr -def test_cli_build_invalid_package_json(capfd, tmp_path): +@pytest.mark.parametrize( + "invalid_content,npm_error,expected_error", + [ + ("foobar", "code EJSONPARSE", ["is not valid JSON", "Failed to parse JSON data"]), + ("{", "code EJSONPARSE", "Unexpected end of JSON input"), + ('{"scripts": }', "code EJSONPARSE", "Unexpected token"), + ( + '{"scripts": null}', + "Cannot convert undefined or null to object", + "scripts.build script not found", + ), + ], +) +def test_cli_build_invalid_package_json( + capfd, tmp_path, invalid_content, npm_error, expected_error +): """ Verify `responder build` using an invalid `package.json` file. """ # Temporary surrogate `package.json` file. package_json_file = tmp_path / "package.json" - package_json_file.write_text("foobar") + package_json_file.write_text(invalid_content) # Invoke `responder build`. stdout, stderr = responder_build(tmp_path, capfd) - assert "npm error code EJSONPARSE" in stderr - assert '"foobar" is not valid JSON' in stderr + assert f"npm error {npm_error}" in stderr + assert any(item in stderr for item in to_list(expected_error)) def test_cli_run(capfd): diff --git a/tests/util.py b/tests/util.py index 3fb866a9..b964e771 100644 --- a/tests/util.py +++ b/tests/util.py @@ -1,30 +1,53 @@ +import socket import time import requests -def random_port(): +def random_port() -> int: """ - Return a random available port. - """ - import socket + Return a random available port by binding to port 0. + Returns: + int: An available port number that can be used for testing. + """ sock = socket.socket() - sock.bind(("", 0)) - port = sock.getsockname()[1] - sock.close() - return port + try: + sock.bind(("", 0)) + return sock.getsockname()[1] + finally: + sock.close() -def wait_server(port: int): +def wait_server( + port: int, + host: str = "127.0.0.1", + protocol: str = "http", + attempts: int = 20, + timeout: float = 0.1, +): """ - Wait for server to be ready. + Wait for server to be ready by attempting to connect to it. + + Args: + port: The port number to connect to + host: The host to connect to (default: "127.0.0.1") + protocol: The protocol to use (default: "http") + attempts: Number of connection attempts (default: 20) + timeout: Timeout per attempt in seconds (default: 0.1) + + Raises: + RuntimeError: If server is not ready after all attempts """ - for _ in range(20): # 2 second timeout + url = f"{protocol}://{host}:{port}/" + for _ in range(1, attempts + 1): try: - requests.get(f"http://127.0.0.1:{port}/", timeout=0.1) + requests.get(url, timeout=timeout) break except requests.exceptions.RequestException: - time.sleep(0.1) + time.sleep(timeout) else: - raise RuntimeError("Server failed to start") + raise RuntimeError( + f"Server at {url} failed to respond after {attempts} attempts " + f"(total wait time: {attempts * timeout * 2:.1f}s)" + )