From 8391447b471df69c0299067730827d8b985c8bc0 Mon Sep 17 00:00:00 2001 From: Tom Date: Sun, 3 May 2026 19:15:46 -0700 Subject: [PATCH 1/6] Add .worktrees to gitignore --- .gitignore | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.gitignore b/.gitignore index 24a406a5..272b2a91 100644 --- a/.gitignore +++ b/.gitignore @@ -50,3 +50,6 @@ history/ # Docker docker-compose.yml + +# Git worktrees +.worktrees/ From 939beaf2ddfe701ba6184ffbbe4002381c6fcf9f Mon Sep 17 00:00:00 2001 From: Tom Date: Thu, 7 May 2026 11:30:28 -0700 Subject: [PATCH 2/6] security: fix CORS wildcard, stored XSS, CSP, and validation error disclosure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses findings from pentest 2026-05-07 (initiative-dev.morels.me): CRIT-001 – CORS origin reflection with credentials `allow_origins=["*"]` combined with `allow_credentials=True` allowed any website to make authenticated requests using the victim's session cookie. Replace wildcard with `settings.cors_origins` — an explicit allowlist derived from `APP_URL` and the new `ALLOWED_ORIGINS` env var. CRIT-002 – Stored XSS + DoS via task title/description Task `title` and `description` fields accepted raw HTML/JS. A payload like `` in a task title crashed the React renderer for all users viewing that project (DoS confirmed). Without a CSP, the same vector could execute arbitrary JS. Add `field_validator` sanitisers to `TaskBase` and `TaskUpdate`: - `title`: plain text only — all HTML tags stripped via regex - `description`: dangerous elements stripped (script, iframe, object, …), event handler attributes removed, javascript:/data: URIs blocked MED-001 – No Content-Security-Policy header Add an HTTP middleware that injects a CSP on every response. Covers the frontend SPA served from the same origin as the API. LOW-001 – Pydantic validation errors expose internal field paths FastAPI's default RequestValidationError handler returns full Pydantic field paths and input values. Add a custom handler that returns a single generic message ("Invalid request data") instead. Co-authored-by: Claude Sonnet 4.6 --- backend/app/core/config.py | 21 +++ backend/app/main.py | 39 ++++- backend/app/schemas/task.py | 71 ++++++++ backend/app/schemas/task_sanitization_test.py | 160 ++++++++++++++++++ 4 files changed, 287 insertions(+), 4 deletions(-) create mode 100644 backend/app/schemas/task_sanitization_test.py diff --git a/backend/app/core/config.py b/backend/app/core/config.py index 2a36caca..7b057217 100644 --- a/backend/app/core/config.py +++ b/backend/app/core/config.py @@ -30,6 +30,27 @@ def cookie_secure(self) -> bool: AUTO_APPROVED_EMAIL_DOMAINS: list[str] = Field(default_factory=list) # APP_URL should point to the frontend entry so redirect URIs resolve correctly APP_URL: str = "http://localhost:5173" + # ALLOWED_ORIGINS controls the CORS allowlist. Defaults to APP_URL so a + # single-origin deploy works out of the box. Override with a + # comma-separated list (e.g. "https://app.example.com,https://www.example.com") + # when the frontend is served from a different origin than APP_URL. + # Never set this to "*" in production — that combined with + # allow_credentials=True allows any site to make authenticated requests. + ALLOWED_ORIGINS: list[str] | str | None = None + + @property + def cors_origins(self) -> list[str]: + """Return the resolved CORS origin allowlist.""" + if isinstance(self.ALLOWED_ORIGINS, list) and self.ALLOWED_ORIGINS: + origins = self.ALLOWED_ORIGINS + elif isinstance(self.ALLOWED_ORIGINS, str) and self.ALLOWED_ORIGINS.strip(): + origins = [o.strip() for o in self.ALLOWED_ORIGINS.split(",") if o.strip()] + else: + origins = [] + # Always include APP_URL so single-origin deploys need no extra config + if self.APP_URL not in origins: + origins = [self.APP_URL] + origins + return origins OIDC_ENABLED: bool = False OIDC_ISSUER: str | None = None OIDC_CLIENT_ID: str | None = None diff --git a/backend/app/main.py b/backend/app/main.py index 335cd89f..193bc304 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -6,9 +6,10 @@ from typing import Annotated from fastapi import Depends, FastAPI, HTTPException, Request, status +from fastapi.exceptions import RequestValidationError from fastapi.middleware.cors import CORSMiddleware from fastapi.openapi.utils import get_openapi -from fastapi.responses import FileResponse +from fastapi.responses import FileResponse, JSONResponse from slowapi import _rate_limit_exceeded_handler from slowapi.errors import RateLimitExceeded @@ -52,12 +53,42 @@ app.add_middleware( CORSMiddleware, - allow_origins=["*"], # Update with frontend URL(s) in production + allow_origins=settings.cors_origins, allow_credentials=True, - allow_methods=["*"], - allow_headers=["*"], + allow_methods=["GET", "POST", "PUT", "PATCH", "DELETE", "OPTIONS"], + allow_headers=["Content-Type", "Authorization", "X-Guild-ID"], ) + +@app.middleware("http") +async def add_security_headers(request: Request, call_next): + """Add security headers to every response.""" + response = await call_next(request) + response.headers["Content-Security-Policy"] = ( + "default-src 'self'; " + "script-src 'self'; " + "style-src 'self' 'unsafe-inline'; " + "connect-src 'self'; " + "font-src 'self' https://fonts.gstatic.com https://fonts.googleapis.com; " + "img-src 'self' data: blob:; " + "frame-ancestors 'none'" + ) + return response + + +@app.exception_handler(RequestValidationError) +async def validation_exception_handler(request: Request, exc: RequestValidationError): + """Return a generic error message for validation errors. + + FastAPI's default handler includes full Pydantic field paths and input + values in the response, which leaks internal schema details to clients. + We map every validation failure to a single generic message instead. + """ + return JSONResponse( + status_code=422, + content={"detail": "Invalid request data"}, + ) + @app.get("/uploads/{filename:path}", include_in_schema=False) @limiter.limit("600/minute") async def serve_upload_file( diff --git a/backend/app/schemas/task.py b/backend/app/schemas/task.py index 4b7b1815..fcb1189d 100644 --- a/backend/app/schemas/task.py +++ b/backend/app/schemas/task.py @@ -1,3 +1,4 @@ +import re from datetime import datetime from typing import List, Literal, Optional @@ -106,10 +107,66 @@ def validate_combinations(self) -> "TaskRecurrence": return self +_HTML_TAG_RE = re.compile(r"<[^>]+>", re.DOTALL) +# Event handler attributes e.g. onerror=, onclick=, onmouseover= … +_EVENT_HANDLER_RE = re.compile( + r'\bon\w+\s*=\s*(?:"[^"]*"|\'[^\']*\'|[^\s>]*)', re.IGNORECASE +) +# javascript: and data: URI schemes inside attribute values +_DANGEROUS_URI_RE = re.compile( + r'(?:javascript|data|vbscript)\s*:', re.IGNORECASE +) + + +def _strip_html(value: str) -> str: + """Remove all HTML tags from a plain-text field (e.g. task title).""" + return _HTML_TAG_RE.sub("", value).strip() + + +def _sanitize_html(value: str) -> str: + """Strip dangerous HTML from a rich-text field (e.g. task description). + + Removes: + - All inline event handlers (onerror=, onclick=, …) + - javascript: / data: / vbscript: URI schemes + - hello", project_id=1) + assert "hover' + t = TaskBase(title="Task", description=payload, project_id=1) + assert "notes') + assert " → stripped entirely (tags + content) + # → stripped (self-closing / void) + _dangerous_names = ( + r"script|iframe|object|embed|form|input|base|link|meta|style" + ) + # Paired tags: strip open tag, content, and close tag together + paired = re.compile( + rf"<\s*({_dangerous_names})(?:\s[^>]*)?>.*?", + re.IGNORECASE | re.DOTALL, + ) + value = paired.sub("", value) + # Remaining unpaired / self-closing open tags (e.g. ) + unpaired = re.compile( + rf"<\s*/?\s*(?:{_dangerous_names})(?:\s[^>]*)?>", re.IGNORECASE | re.DOTALL, ) - value = dangerous_tags.sub("", value) + value = unpaired.sub("", value) # Strip event handler attributes from remaining tags value = _EVENT_HANDLER_RE.sub("", value) # Strip dangerous URI schemes diff --git a/backend/app/schemas/task_sanitization_test.py b/backend/app/schemas/task_sanitization_test.py index 2152bad4..7d28b562 100644 --- a/backend/app/schemas/task_sanitization_test.py +++ b/backend/app/schemas/task_sanitization_test.py @@ -24,7 +24,7 @@ def test_plain_text_unchanged(self): def test_img_onerror_stripped(self): """The exact payload used in the pentest.""" payload = '' - t = TaskBase(title=payload, project_id=1) + t = TaskBase(title=payload) assert "" not in t.title assert "bold italic" in t.title - def test_empty_string_stays_empty(self): - # Empty string is falsy — validator should pass it through - # (validation still rejects empty after strip in Pydantic min_length if set, - # but our validator should not crash on it) - with pytest.raises(ValidationError): - TaskBase(title="", project_id=1) + def test_html_only_title_becomes_empty_string(self): + # A title that is ONLY HTML tags reduces to "" after stripping. + # Pydantic's str validator allows empty strings unless min_length is set; + # the sanitiser itself should not crash or raise. + t = TaskBase(title="") + assert t.title == "" def test_task_update_title_sanitized(self): u = TaskUpdate(title=' urgent fix') From 5384f1b86759570ad35c62f7994f80f9a40091f7 Mon Sep 17 00:00:00 2001 From: Tom Date: Thu, 7 May 2026 14:59:10 -0700 Subject: [PATCH 4/6] fix: remove unused pytest and ValidationError imports (ruff F401) --- backend/app/schemas/task_sanitization_test.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/backend/app/schemas/task_sanitization_test.py b/backend/app/schemas/task_sanitization_test.py index 7d28b562..44ec5388 100644 --- a/backend/app/schemas/task_sanitization_test.py +++ b/backend/app/schemas/task_sanitization_test.py @@ -5,9 +5,6 @@ (pentest 2026-05-07, initiative-dev.morels.me) """ -import pytest -from pydantic import ValidationError - from app.schemas.task import TaskBase, TaskUpdate From 62e5d1fc4cc09924606f4782afaeba7d3e07d804 Mon Sep 17 00:00:00 2001 From: Tom Date: Thu, 7 May 2026 15:12:21 -0700 Subject: [PATCH 5/6] fix: stringify exception objects in ctx before JSON serialisation (TypeError) --- backend/app/main.py | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/backend/app/main.py b/backend/app/main.py index 4a5bbe2f..6362a5fe 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -87,10 +87,23 @@ async def validation_exception_handler(request: Request, exc: RequestValidationE ``PROPERTY_OPTIONS_REQUIRED`` that callers legitimately depend on) but strip the ``input`` and ``url`` keys from every entry. """ - sanitized = [ - {k: v for k, v in err.items() if k not in ("input", "url")} - for err in exc.errors() - ] + def _clean(err: dict) -> dict: + cleaned: dict = {} + for k, v in err.items(): + if k in ("input", "url"): + continue + # Pydantic v2 puts the raw exception object in ctx["error"], which + # is not JSON-serialisable. Stringify any exception values. + if k == "ctx" and isinstance(v, dict): + cleaned[k] = { + ck: str(cv) if isinstance(cv, Exception) else cv + for ck, cv in v.items() + } + else: + cleaned[k] = v + return cleaned + + sanitized = [_clean(err) for err in exc.errors()] return JSONResponse(status_code=422, content={"detail": sanitized}) @app.get("/uploads/{filename:path}", include_in_schema=False) From 9d00d316731d5c2e6bf2b9b9f94dbd740ad6c5ed Mon Sep 17 00:00:00 2001 From: Tom Date: Thu, 7 May 2026 15:20:09 -0700 Subject: [PATCH 6/6] fix: convert test classes to plain functions (PYL-R0201 x25) --- backend/app/schemas/task_sanitization_test.py | 263 +++++++++--------- 1 file changed, 135 insertions(+), 128 deletions(-) diff --git a/backend/app/schemas/task_sanitization_test.py b/backend/app/schemas/task_sanitization_test.py index 44ec5388..d7cf59a3 100644 --- a/backend/app/schemas/task_sanitization_test.py +++ b/backend/app/schemas/task_sanitization_test.py @@ -13,44 +13,49 @@ # --------------------------------------------------------------------------- -class TestTaskTitleSanitization: - def test_plain_text_unchanged(self): - t = TaskBase(title="Fix login bug", project_id=1) - assert t.title == "Fix login bug" - - def test_img_onerror_stripped(self): - """The exact payload used in the pentest.""" - payload = '' - t = TaskBase(title=payload) - assert "alert(1)hello", project_id=1) - assert "hello") + assert "hover' - t = TaskBase(title="Task", description=payload, project_id=1) - assert "notes') - assert "hover' + t = TaskBase(title="Task", description=payload) + assert "notes") + assert "