From aa17edf1d05b1c44edd510c3a4ef56358dfdc6c0 Mon Sep 17 00:00:00 2001 From: Pouzor Date: Thu, 14 May 2026 12:11:40 +0200 Subject: [PATCH] chore(security): drop passlib for direct bcrypt + reject CLI-flag check targets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Replace passlib.CryptContext with bcrypt 4.2.1 directly (passlib is unmaintained and only emitted warnings when paired with bcrypt 4+). hash_password / verify_password now call bcrypt.checkpw / .hashpw, and verify_password is hardened against empty inputs without raising. scripts/hash_password.py + tests/conftest.py updated to match. - status_checker.check_node() now rejects targets starting with '-' before any subprocess invocation, defending ping/tcp paths against arg-injection if an admin sets a check_target like '-O'. Tests added: - test_auth: expired JWT, malformed JWT, wrong-secret JWT, empty password vs empty server hash, verify_password edge cases. - test_scan: _background_scan failure path marks ScanRun failed, leaves non-running terminal status alone, success path invokes run_scan. - test_status_checker: ping/tcp invocations are bypassed when target or ip starts with '-'. Backend coverage 89% → 90%. --- backend/app/core/security.py | 12 ++-- backend/app/services/status_checker.py | 5 ++ backend/requirements.txt | 3 +- backend/scripts/hash_password.py | 8 +-- backend/tests/conftest.py | 6 +- backend/tests/test_auth.py | 68 ++++++++++++++++++++ backend/tests/test_scan.py | 89 ++++++++++++++++++++++++++ backend/tests/test_status_checker.py | 25 ++++++++ 8 files changed, 199 insertions(+), 17 deletions(-) diff --git a/backend/app/core/security.py b/backend/app/core/security.py index ee9257a..957b1aa 100644 --- a/backend/app/core/security.py +++ b/backend/app/core/security.py @@ -1,22 +1,22 @@ from datetime import datetime, timedelta, timezone +import bcrypt from jose import JWTError, jwt -from passlib.context import CryptContext from app.core.config import settings -pwd_context = CryptContext(schemes=["bcrypt"], deprecated="auto") - def verify_password(plain: str, hashed: str) -> bool: + if not plain or not hashed: + return False try: - return bool(pwd_context.verify(plain, hashed)) - except ValueError: + return bcrypt.checkpw(plain.encode("utf-8"), hashed.encode("utf-8")) + except (ValueError, TypeError): return False def hash_password(password: str) -> str: - return str(pwd_context.hash(password)) + return bcrypt.hashpw(password.encode("utf-8"), bcrypt.gensalt()).decode("utf-8") def create_access_token(subject: str) -> str: diff --git a/backend/app/services/status_checker.py b/backend/app/services/status_checker.py index 4f6ba75..8a75126 100644 --- a/backend/app/services/status_checker.py +++ b/backend/app/services/status_checker.py @@ -24,6 +24,11 @@ async def check_node(check_method: str, target: str | None, ip: str | None) -> d host = target or raw_ip if not host: return {"status": "unknown", "response_time_ms": None} + # Reject hostnames that look like CLI flags — defends ping/tcp invocations + # against arg-injection if a malicious admin sets target like "-O". + if host.startswith("-"): + logger.warning("Rejecting check target that starts with '-': %r", host) + return {"status": "unknown", "response_time_ms": None} start = time.monotonic() try: diff --git a/backend/requirements.txt b/backend/requirements.txt index e504051..53d5bb7 100644 --- a/backend/requirements.txt +++ b/backend/requirements.txt @@ -7,8 +7,7 @@ alembic==1.13.3 pydantic==2.9.2 pydantic-settings==2.5.2 python-jose[cryptography]==3.5.0 -passlib[bcrypt]==1.7.4 -bcrypt==4.0.1 +bcrypt==4.2.1 python-multipart==0.0.27 apscheduler==3.10.4 python-nmap==0.7.1 diff --git a/backend/scripts/hash_password.py b/backend/scripts/hash_password.py index 9ee5c5b..6c1587d 100644 --- a/backend/scripts/hash_password.py +++ b/backend/scripts/hash_password.py @@ -1,13 +1,11 @@ -"""Generate a bcrypt password hash for config.yml.""" +"""Generate a bcrypt password hash for the AUTH_PASSWORD_HASH env var.""" import sys -from passlib.context import CryptContext - -pwd_context = CryptContext(schemes=["bcrypt"], deprecated="auto") +import bcrypt if len(sys.argv) < 2: print("Usage: python scripts/hash_password.py ") sys.exit(1) password = sys.argv[1] -print(pwd_context.hash(password)) +print(bcrypt.hashpw(password.encode("utf-8"), bcrypt.gensalt()).decode("utf-8")) diff --git a/backend/tests/conftest.py b/backend/tests/conftest.py index de3d6dd..7dc8c21 100644 --- a/backend/tests/conftest.py +++ b/backend/tests/conftest.py @@ -5,23 +5,21 @@ os.environ.setdefault("SECRET_KEY", "test-only-secret-key-not-for-production") import pytest from httpx import ASGITransport, AsyncClient -from passlib.context import CryptContext from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker, create_async_engine +from app.core.security import hash_password from app.db.database import Base, get_db from app.main import app TEST_DB_URL = "sqlite+aiosqlite:///:memory:" -_pwd_ctx = CryptContext(schemes=["bcrypt"], deprecated="auto") - @pytest.fixture(autouse=True, scope="session") def test_credentials(): """Configure test auth credentials directly on settings.""" from app.core.config import settings settings.auth_username = "admin" - settings.auth_password_hash = _pwd_ctx.hash("admin") + settings.auth_password_hash = hash_password("admin") @pytest.fixture diff --git a/backend/tests/test_auth.py b/backend/tests/test_auth.py index f65f578..09b569d 100644 --- a/backend/tests/test_auth.py +++ b/backend/tests/test_auth.py @@ -68,3 +68,71 @@ async def test_login_with_malformed_hash_returns_401_not_500(client: AsyncClient assert res.status_code == 401 finally: settings.auth_password_hash = original + + +# --- JWT-level cases --- + +async def test_expired_token_rejected(client: AsyncClient): + """A JWT whose `exp` is in the past must be refused.""" + from datetime import datetime, timedelta, timezone + + from jose import jwt + + from app.core.config import settings + payload = { + "sub": "admin", + "exp": datetime.now(timezone.utc) - timedelta(minutes=1), + } + token = jwt.encode(payload, settings.secret_key, algorithm=settings.algorithm) + res = await client.get("/api/v1/nodes", headers={"Authorization": f"Bearer {token}"}) + assert res.status_code == 401 + + +async def test_malformed_token_rejected(client: AsyncClient): + res = await client.get("/api/v1/nodes", headers={"Authorization": "Bearer not-a-jwt"}) + assert res.status_code == 401 + + +async def test_token_signed_with_wrong_secret_rejected(client: AsyncClient): + """A token signed with a different key must not be accepted.""" + from datetime import datetime, timedelta, timezone + + from jose import jwt + + from app.core.config import settings + payload = { + "sub": "admin", + "exp": datetime.now(timezone.utc) + timedelta(minutes=5), + } + forged = jwt.encode(payload, "different-secret", algorithm=settings.algorithm) + res = await client.get("/api/v1/nodes", headers={"Authorization": f"Bearer {forged}"}) + assert res.status_code == 401 + + +async def test_missing_authorization_header_rejected(client: AsyncClient): + res = await client.get("/api/v1/nodes") + assert res.status_code == 401 + + +async def test_empty_password_does_not_pass_when_hash_empty(client: AsyncClient): + """No credentials configured server-side must not authenticate an empty password.""" + from app.core.config import settings + original_hash = settings.auth_password_hash + settings.auth_password_hash = "" + try: + res = await client.post("/api/v1/auth/login", json={"username": "admin", "password": ""}) + assert res.status_code == 401 + finally: + settings.auth_password_hash = original_hash + + +# --- Password helper --- + +def test_verify_password_handles_empty_inputs(): + """verify_password must be safe against empty plain / empty hash without raising.""" + from app.core.security import hash_password, verify_password + h = hash_password("hunter2") + assert verify_password("hunter2", h) is True + assert verify_password("", h) is False + assert verify_password("hunter2", "") is False + assert verify_password("", "") is False diff --git a/backend/tests/test_scan.py b/backend/tests/test_scan.py index 6b4b640..7199ea9 100644 --- a/backend/tests/test_scan.py +++ b/backend/tests/test_scan.py @@ -37,6 +37,95 @@ async def pending_device(db_session): return device +# --- _background_scan error handling --- + +@pytest.fixture +async def mem_db(): + from sqlalchemy.ext.asyncio import async_sessionmaker, create_async_engine + + from app.db.database import Base + engine = create_async_engine("sqlite+aiosqlite:///:memory:") + async with engine.begin() as conn: + await conn.run_sync(Base.metadata.create_all) + factory = async_sessionmaker(engine, expire_on_commit=False) + yield factory + await engine.dispose() + + +@pytest.mark.asyncio +async def test_background_scan_marks_run_failed_on_exception(mem_db): + """If run_scan() raises, the ScanRun must transition running → failed and the + session rollback path must execute without a follow-on exception.""" + from app.api.routes.scan import _background_scan + + async with mem_db() as session: + run = ScanRun(status="running", ranges=["10.0.0.0/24"]) + session.add(run) + await session.commit() + run_id = run.id + + with ( + patch("app.api.routes.scan.AsyncSessionLocal", mem_db), + patch( + "app.api.routes.scan.run_scan", + new_callable=AsyncMock, + side_effect=RuntimeError("boom"), + ), + ): + await _background_scan(run_id, ["10.0.0.0/24"]) + + async with mem_db() as session: + refreshed = await session.get(ScanRun, run_id) + assert refreshed is not None + assert refreshed.status == "failed" + + +@pytest.mark.asyncio +async def test_background_scan_leaves_non_running_status_alone(mem_db): + """If the run was already stopped/cancelled before run_scan failed, _background_scan + must NOT overwrite that terminal status with 'failed'.""" + from app.api.routes.scan import _background_scan + + async with mem_db() as session: + run = ScanRun(status="cancelled", ranges=["10.0.0.0/24"]) + session.add(run) + await session.commit() + run_id = run.id + + with ( + patch("app.api.routes.scan.AsyncSessionLocal", mem_db), + patch( + "app.api.routes.scan.run_scan", + new_callable=AsyncMock, + side_effect=RuntimeError("boom"), + ), + ): + await _background_scan(run_id, ["10.0.0.0/24"]) + + async with mem_db() as session: + refreshed = await session.get(ScanRun, run_id) + assert refreshed is not None + assert refreshed.status == "cancelled" + + +@pytest.mark.asyncio +async def test_background_scan_success_path_invokes_run_scan(mem_db): + from app.api.routes.scan import _background_scan + + async with mem_db() as session: + run = ScanRun(status="running", ranges=["10.0.0.0/24"]) + session.add(run) + await session.commit() + run_id = run.id + + with ( + patch("app.api.routes.scan.AsyncSessionLocal", mem_db), + patch("app.api.routes.scan.run_scan", new_callable=AsyncMock) as mock_run_scan, + ): + await _background_scan(run_id, ["10.0.0.0/24"]) + mock_run_scan.assert_awaited_once() + + # --- Trigger scan --- @pytest.mark.asyncio diff --git a/backend/tests/test_status_checker.py b/backend/tests/test_status_checker.py index 3d23cd8..205e62d 100644 --- a/backend/tests/test_status_checker.py +++ b/backend/tests/test_status_checker.py @@ -216,6 +216,31 @@ async def test_ping_uses_windows_args_on_win32(): assert "-c" not in captured["args"] +# --- check_node target validation --- + +@pytest.mark.asyncio +async def test_check_node_rejects_flag_like_target(): + """A target starting with '-' must never reach subprocess invocation.""" + from app.services.status_checker import check_node + + with patch("asyncio.create_subprocess_exec") as mock_exec: + result = await check_node("ping", "-O", None) + + mock_exec.assert_not_called() + assert result["status"] == "unknown" + + +@pytest.mark.asyncio +async def test_check_node_rejects_flag_like_ip(): + from app.services.status_checker import check_node + + with patch("asyncio.create_subprocess_exec") as mock_exec: + result = await check_node("ping", None, "-O") + + mock_exec.assert_not_called() + assert result["status"] == "unknown" + + # --- _tcp_connect --- @pytest.mark.asyncio