From 363f737d07e010c1dd3f2a6c6319e94e5cbf33d2 Mon Sep 17 00:00:00 2001 From: VortexUK Date: Sat, 11 Jul 2026 13:17:02 +0100 Subject: [PATCH] fix(security): clear all open CodeQL code-scanning alerts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Triage + fixes for the 32 open alerts under the Security tab: Real fixes: - py/log-injection (17): every live site now scrubs the user-controlled value (character/guild names, search queries, cache keys, audit actor) via backend.core.log_safety.scrub before it reaches a log line — views, characters, claim, guild, guild_cache, cache, aa, parses ingest, audit_log. Several flagged sites had already been deleted by the catalogue rearchitecture; their live equivalents are covered. - py/xml-bomb: the ACT paste-import now parses with defusedxml (new dependency) so entity-expansion bombs are rejected, not expanded; DefusedXmlException joins ParseError in the 400 handler. Verified a billion-laughs payload returns 400. - py/incomplete-url-substring-sanitization: test_auth asserts urlparse(location).hostname == "discord.com" instead of a substring. Dismissed as false positives (documented on each alert): - py/weak-sensitive-data-hashing: SHA-256 over a 192-bit random bearer token, not a password. - py/clear-text-logging-sensitive-data (9): the logged values are token_id (integer row id), display prefix, user_id, access_status, remote IP — operational identifiers; raw tokens/signatures are never logged per CLAUDE.md's sensitive-values rule. CodeQL taints entire rows/dicts derived from a token. - py/path-injection app.py: the SPA fallback resolves + containment- checks (relative_to) right below the flagged line. Staleness fix: the CodeQL workflow gains workflow_dispatch so the main-branch alert set can be refreshed on demand (it deliberately has no push trigger — Railway's deploy gate waits on push workflows; the weekly cron was the only main refresh, leaving alerts pointing at code deleted weeks ago, e.g. the aa.py path-injection pair from the pre-#141 tree-JSON reads). 1527 tests green, ruff + pyright clean. Co-Authored-By: Claude Fable 5 --- .github/workflows/codeql.yml | 3 +++ backend/server/api/aa.py | 5 +++-- backend/server/api/act/xml_import.py | 8 ++++++-- backend/server/api/character/views.py | 5 +++-- backend/server/api/characters.py | 5 +++-- backend/server/api/claim.py | 2 +- backend/server/api/guild.py | 2 +- backend/server/api/parses/ingest.py | 3 ++- backend/server/cache.py | 9 +++++---- backend/server/core/audit_log.py | 4 ++-- backend/server/guild_cache.py | 3 ++- pyproject.toml | 2 ++ tests/server/test_auth.py | 4 +++- uv.lock | 11 +++++++++++ 14 files changed, 47 insertions(+), 19 deletions(-) diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index 4add1fa7..a9bc1190 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -15,6 +15,9 @@ on: schedule: # Mondays at 06:00 UTC. - cron: '0 6 * * 1' + # Manual refresh of the main-branch alert set (e.g. right after merging + # security fixes) — dispatch runs don't gate Railway deploys. + workflow_dispatch: permissions: actions: read diff --git a/backend/server/api/aa.py b/backend/server/api/aa.py index 27232ca1..cb6156ee 100644 --- a/backend/server/api/aa.py +++ b/backend/server/api/aa.py @@ -11,6 +11,7 @@ from backend.census.store import StoreRecord from backend.census.store import store as census_store +from backend.core.log_safety import scrub from backend.eq2db.aas import catalogue as aa_db from backend.eq2db.spells import catalogue as spells_db from backend.server.cache import aa_cache @@ -225,7 +226,7 @@ def _write() -> None: await run_sync(_write) aa_cache.set(cache_key, result) except Exception as exc: - _log.warning("[cache] Background AA refresh failed for %s: %s", name, exc) + _log.warning("[cache] Background AA refresh failed for %s: %s", scrub(name), exc) @router.get("/character/{name}/aas", response_model=CharAAsResponse) @@ -270,7 +271,7 @@ def _read() -> StoreRecord | None: from backend.server import census_health if census_health.is_down(): - _log.debug("[aa] Skipping live fetch — census_health=down (name=%s)", name) + _log.debug("[aa] Skipping live fetch — census_health=down (name=%s)", scrub(name)) raise HTTPException( status_code=503, detail=f"'{name}' AA data not cached yet and Census is unavailable. Try again shortly.", diff --git a/backend/server/api/act/xml_import.py b/backend/server/api/act/xml_import.py index 60d36fcf..d83d8f21 100644 --- a/backend/server/api/act/xml_import.py +++ b/backend/server/api/act/xml_import.py @@ -18,6 +18,10 @@ import xml.etree.ElementTree as ET +# User-pasted XML — parse with defusedxml so entity-expansion bombs +# (billion laughs) and external entities are rejected, not expanded. +from defusedxml import ElementTree as SafeET +from defusedxml.common import DefusedXmlException from fastapi import HTTPException @@ -136,8 +140,8 @@ def parse_import_xml(text: str) -> tuple[list[dict], list[dict]]: wrapped = f"<__import_root>{body}" try: - root = ET.fromstring(wrapped) - except ET.ParseError as exc: + root = SafeET.fromstring(wrapped) + except (ET.ParseError, DefusedXmlException) as exc: raise HTTPException(status_code=400, detail=f"Invalid XML: {exc}") from exc triggers = [_trigger_from_element(t) for t in root.iter("Trigger") if (t.get("Regex") or t.get("R"))] diff --git a/backend/server/api/character/views.py b/backend/server/api/character/views.py index b1585e67..80b2c2ac 100644 --- a/backend/server/api/character/views.py +++ b/backend/server/api/character/views.py @@ -14,6 +14,7 @@ from pydantic import BaseModel from backend.census.item_level import adorn_bonus, character_ilvl +from backend.core.log_safety import scrub from backend.eq2db.items import GearRow from backend.eq2db.items import catalogue as _items from backend.server.api.character import router # the package-level router @@ -506,7 +507,7 @@ async def get_character(request: Request, name: str) -> CharacterResponse: finally: conn2.close() except Exception as exc: - _log.debug("[character] self-heal cache write skipped for %s: %s", name, exc) + _log.debug("[character] self-heal cache write skipped for %s: %s", scrub(name), exc) character_cache.set(cache_key, resp) return resp @@ -515,7 +516,7 @@ async def get_character(request: Request, name: str) -> CharacterResponse: from backend.server import census_health if census_health.is_down(): - _log.debug("[character] Skipping live fetch — census_health=down (name=%s)", name) + _log.debug("[character] Skipping live fetch — census_health=down (name=%s)", scrub(name)) raise HTTPException( status_code=503, detail=f"'{name}' not cached yet and Census is unavailable. Try again shortly.", diff --git a/backend/server/api/characters.py b/backend/server/api/characters.py index 23e533ea..80c5e73b 100644 --- a/backend/server/api/characters.py +++ b/backend/server/api/characters.py @@ -6,6 +6,7 @@ from fastapi import APIRouter, Request from pydantic import BaseModel +from backend.core.log_safety import scrub from backend.server.cache import character_cache from backend.server.core.cache_keys import char_cache_key from backend.server.core.census_lifecycle import shared_census_client @@ -85,9 +86,9 @@ async def search_characters(request: Request, name: str = "") -> CharSearchRespo if brief: raw = [brief] except Exception as exc: - _log.debug("[characters] Exact-name fallback failed for %r: %s", q, exc) + _log.debug("[characters] Exact-name fallback failed for %r: %s", scrub(q), exc) except Exception as exc: - _log.warning("[characters] Census search failed for %r: %s", q, exc) + _log.warning("[characters] Census search failed for %r: %s", scrub(q), exc) raw = [] if raw: diff --git a/backend/server/api/claim.py b/backend/server/api/claim.py index eb2225a8..706b38cd 100644 --- a/backend/server/api/claim.py +++ b/backend/server/api/claim.py @@ -225,7 +225,7 @@ async def create_claim(request: Request, body: SubmitClaimRequest) -> ClaimRespo from backend.server import census_health if census_health.is_down(): - _log.debug("[claim] Skipping live fetch — census_health=down (name=%s)", name) + _log.debug("[claim] Skipping live fetch — census_health=down (name=%s)", _safe_for_log(name)) raise HTTPException( status_code=503, detail="Census is unavailable. Cannot verify character existence — try again shortly.", diff --git a/backend/server/api/guild.py b/backend/server/api/guild.py index 71ba44f7..4bd93d05 100644 --- a/backend/server/api/guild.py +++ b/backend/server/api/guild.py @@ -393,7 +393,7 @@ async def search_guilds(name: str = "") -> GuildSearchResponse: async with shared_census_client() as client: raw = await client.search_guilds_by_name(q, current_world()) except Exception as exc: - _log.warning("[guild] Census guild search failed for %r: %s", q, exc) + _log.warning("[guild] Census guild search failed for %r: %s", _scrub(q), exc) raw = [] if raw: diff --git a/backend/server/api/parses/ingest.py b/backend/server/api/parses/ingest.py index 0c00ebe9..ea2b05d9 100644 --- a/backend/server/api/parses/ingest.py +++ b/backend/server/api/parses/ingest.py @@ -17,6 +17,7 @@ from fastapi import BackgroundTasks, HTTPException, Request from backend.census.store import store as census_store +from backend.core.log_safety import scrub from backend.server.api.parses import router from backend.server.api.parses.list import _classify_zone from backend.server.api.parses.models import ( @@ -140,7 +141,7 @@ async def _resolve_uploader_guild_async( async with shared_census_client() as client: guild_name = await client.get_character_guild_name(uploader, effective_world) except Exception as exc: - _log.warning("Census guild lookup failed for %r: %s", uploader, exc) + _log.warning("Census guild lookup failed for %r: %s", scrub(uploader), exc) return CENSUS_UNAVAILABLE if not guild_name: diff --git a/backend/server/cache.py b/backend/server/cache.py index 3946f08c..aa82b199 100644 --- a/backend/server/cache.py +++ b/backend/server/cache.py @@ -9,6 +9,7 @@ import time from typing import Any, Literal +from backend.core.log_safety import scrub from backend.server.constants import CACHE_MAX_AGE_S, CACHE_STALE_TTL_S from backend.server.core.silent_swallow import swallow @@ -103,11 +104,11 @@ def get_stale(self, key: str) -> tuple[Any | None, bool]: if self._max_age is not None and age > self._max_age: del self._store[key] self._update_size() - _log.debug("[cache] EXPIRED %s (%.1f min old)", key, age / 60) + _log.debug("[cache] EXPIRED %s (%.1f min old)", scrub(key), age / 60) self._inc_miss() return None, False is_stale = age > self._ttl - _log.debug("[cache] %s %s", "STALE" if is_stale else "HIT ", key) + _log.debug("[cache] %s %s", "STALE" if is_stale else "HIT ", scrub(key)) if is_stale: self._inc_stale() else: @@ -115,7 +116,7 @@ def get_stale(self, key: str) -> tuple[Any | None, bool]: return value, is_stale def set(self, key: str, value: Any) -> None: - _log.debug("[cache] SET %s", key) + _log.debug("[cache] SET %s", scrub(key)) # Evict oldest entry if we're at capacity and this is a new key. if key not in self._store and len(self._store) >= self._maxsize: oldest_key = next(iter(self._store)) @@ -148,7 +149,7 @@ def sweep(self) -> int: def delete(self, key: str) -> None: self._store.pop(key, None) - _log.debug("[cache] DEL %s", key) + _log.debug("[cache] DEL %s", scrub(key)) self._update_size() diff --git a/backend/server/core/audit_log.py b/backend/server/core/audit_log.py index 67270ea7..9e323d1f 100644 --- a/backend/server/core/audit_log.py +++ b/backend/server/core/audit_log.py @@ -90,10 +90,10 @@ def audit_log(action: str, actor: str | None, **fields: Any) -> None: """ safe_fields: dict[str, Any] = {(f"{k}_" if k in _LOGRECORD_RESERVED else k): scrub(v) for k, v in fields.items()} safe_fields["action"] = action - safe_fields["actor"] = actor or "-" + safe_fields["actor"] = scrub(actor) if actor else "-" safe_fields["request_id"] = request_id_var.get() or "-" safe_fields["world"] = world_var.get() or "-" # The message string is intentionally short — dashboards filter on # extra= fields, not on message body. Keep the human-readable part # action + actor so text logs are still grep-friendly. - _log.info("audit: %s actor=%s", action, actor or "-", extra=safe_fields) + _log.info("audit: %s actor=%s", action, scrub(actor) if actor else "-", extra=safe_fields) diff --git a/backend/server/guild_cache.py b/backend/server/guild_cache.py index 8569ff50..db887b75 100644 --- a/backend/server/guild_cache.py +++ b/backend/server/guild_cache.py @@ -27,6 +27,7 @@ from backend.census.constants import SPELL_TIER_ORDER as _TIER_ORDER from backend.census.models import CharacterOverview, GuildData, SpellEntry from backend.census.store import store as census_store +from backend.core.log_safety import scrub from backend.eq2db.spells import ( DB_PATH as _SPELLS_DB, ) @@ -395,7 +396,7 @@ async def _do_fetch(): try: return await task except Exception as exc: - _log.warning("[cache] Guild fetch failed for %s: %s", guild_name, exc) + _log.warning("[cache] Guild fetch failed for %s: %s", scrub(guild_name), exc) return None finally: if _guild_fetch_tasks.get(task_key) is task: diff --git a/pyproject.toml b/pyproject.toml index 9246e03c..9b6ae1e1 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -17,6 +17,8 @@ dependencies = [ "httpx>=0.27.0", "itsdangerous>=2.2.0", "slowapi>=0.1.9", + # Hardened XML parsing for the ACT paste-import (entity-expansion safe) + "defusedxml>=0.7.1", # IANA timezone database — Python's zoneinfo has no bundled tz data on # Windows (and minimal containers), so available_timezones()/ZoneInfo need # this for the raid-schedule timezone validation + live-window checks to diff --git a/tests/server/test_auth.py b/tests/server/test_auth.py index f5ef1ec3..ef71366e 100644 --- a/tests/server/test_auth.py +++ b/tests/server/test_auth.py @@ -138,4 +138,6 @@ async def test_login_redirects_to_discord(app): async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test", follow_redirects=False) as client: response = await client.get("/api/auth/login") assert response.status_code in (302, 307) - assert "discord.com" in response.headers["location"] + from urllib.parse import urlparse + + assert urlparse(response.headers["location"]).hostname == "discord.com" diff --git a/uv.lock b/uv.lock index 13fbcebd..4c205b69 100644 --- a/uv.lock +++ b/uv.lock @@ -247,6 +247,15 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/d1/d6/3965ed04c63042e047cb6a3e6ed1a63a35087b6a609aa3a15ed8ac56c221/colorama-0.4.6-py2.py3-none-any.whl", hash = "sha256:4f1d9991f5acc0ca119f9d443620b77f9d6b33703e51011c16baf57afb285fc6", size = 25335, upload-time = "2022-10-25T02:36:20.889Z" }, ] +[[package]] +name = "defusedxml" +version = "0.7.1" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/0f/d5/c66da9b79e5bdb124974bfe172b4daf3c984ebd9c2a06e2b8a4dc7331c72/defusedxml-0.7.1.tar.gz", hash = "sha256:1bb3032db185915b62d7c6209c5a8792be6a32ab2fedacc84e01b52c51aa3e69", size = 75520, upload-time = "2021-03-08T10:59:26.269Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/07/6c/aa3f2f849e01cb6a001cd8554a88d4c77c5c1a31c95bdf1cf9301e6d9ef4/defusedxml-0.7.1-py2.py3-none-any.whl", hash = "sha256:a352e7e428770286cc899e2542b6cdaedb2b4953ff269a210103ec58f6198a61", size = 25604, upload-time = "2021-03-08T10:59:24.45Z" }, +] + [[package]] name = "deprecated" version = "1.3.1" @@ -280,6 +289,7 @@ dependencies = [ { name = "aiohttp" }, { name = "aiosqlite" }, { name = "better-profanity" }, + { name = "defusedxml" }, { name = "discord-py" }, { name = "fastapi" }, { name = "httpx" }, @@ -307,6 +317,7 @@ requires-dist = [ { name = "aiohttp", specifier = ">=3.9.0" }, { name = "aiosqlite", specifier = ">=0.20.0" }, { name = "better-profanity", specifier = ">=0.7.0" }, + { name = "defusedxml", specifier = ">=0.7.1" }, { name = "discord-py", specifier = ">=2.3.0" }, { name = "fastapi", specifier = ">=0.111.0" }, { name = "httpx", specifier = ">=0.27.0" },