From 3e0fb9201231f92587893f4ceff2cf3421a7577c Mon Sep 17 00:00:00 2001
From: chihlasm <michael@chihlas.com>
Date: Thu, 5 Feb 2026 22:04:37 -0500
Subject: [PATCH] fix: critical security hardening (Phase A permissions audit)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

- Remove role field from UserCreate schema, hardcode 'engineer' at registration
- Escape all user content in HTML export with html.escape() (XSS fix)
- Add field_validator to reject default SECRET_KEY when DEBUG=False
- Add CHECK constraint on users.role ('engineer'|'viewer') + migration 011
- Fix test_admin fixture to properly grant is_super_admin via ORM
- Fix circular FK (users↔invite_codes) in test DB setup with DROP SCHEMA CASCADE
- Add 5 new security tests (role validation + XSS prevention)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
---
 CLAUDE.md                                     | 14 +++-
 .../versions/011_add_role_check_constraint.py | 34 ++++++++
 backend/app/api/endpoints/auth.py             |  2 +-
 backend/app/api/endpoints/sessions.py         | 47 +++++------
 backend/app/core/config.py                    | 17 +++-
 backend/app/models/user.py                    |  8 +-
 backend/app/schemas/user.py                   |  1 -
 backend/tests/conftest.py                     | 42 ++++++----
 backend/tests/test_auth.py                    | 38 +++++++--
 backend/tests/test_sessions.py                | 81 +++++++++++++++++++
 10 files changed, 236 insertions(+), 48 deletions(-)
 create mode 100644 backend/alembic/versions/011_add_role_check_constraint.py

diff --git a/CLAUDE.md b/CLAUDE.md
index a65e61d6..06979b9f 100644
--- a/CLAUDE.md
+++ b/CLAUDE.md
@@ -105,6 +105,12 @@ When adding new frontend pages or components, use "ResolutionFlow" for any user-
   - Frontend hook: `frontend/src/hooks/usePermissions.ts`
   - Viewers CAN: browse trees, start sessions, rate steps
   - Viewers CANNOT: create/edit trees, steps, tags, categories
+- **Security Hardening (Phase A):**
+  - Registration role field removed from `UserCreate` — hardcoded to `engineer`
+  - HTML export XSS fix — all user content escaped via `html.escape()`
+  - Secret key validator — rejects default key when `DEBUG=False`
+  - Role CHECK constraint on `users` table (migration 011)
+  - `test_admin` fixture fixed to properly grant `is_super_admin=True`
 - **Session Scratchpad (Floating Overlay):**
   - Fixed-position overlay panel (420px wide, 55vh tall) on right edge
   - Floating button when collapsed, slide-in panel when expanded
@@ -329,7 +335,7 @@ docker exec -it patherly_postgres psql -U postgres -c "CREATE DATABASE patherly_
 pip install -r requirements-dev.txt
 
 # Run tests
-pytest
+pytest --override-ini="addopts="
 ```
 
 ### Frontend Operations
@@ -513,6 +519,10 @@ When using `allow_origin_regex` for wildcard patterns, also include `allow_origi
 - Frontend: use `usePermissions()` hook for all role/permission checks
 - `TreeListItem` includes `team_id` for frontend permission checks (`author_id` and `team_id` are nullable)
 
+### SQLAlchemy Test Fixtures: Circular FK Handling
+
+The `users ↔ invite_codes` tables have circular foreign keys. `Base.metadata.drop_all()` fails with `CircularDependencyError`. The test fixture uses `DROP SCHEMA public CASCADE` instead (split into two `sa.text()` calls — asyncpg rejects multi-statement strings).
+
 ### Alembic Migrations: Test Data State Before Writing WHERE Clauses
 
 Migration 010 had `WHERE role = 'admin'` but the only user already had `role = 'engineer'` (changed by earlier work), so the UPDATE matched zero rows. Always verify actual data values before writing conditional migrations, or use broader conditions.
@@ -753,7 +763,7 @@ Position overlay at `right-2` (not `right-0`) so it sits inside the page scrollb
 
 - Commit message format: `type: description`
 - Types: `feat`, `fix`, `refactor`, `docs`, `test`, `chore`
-- Always include `Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>`
+- Always include `Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>`
 - Always create a feature branch BEFORE committing new work (not retroactively after committing to main)
 - PR workflow: `git checkout -b feat/feature-name` → commit → `git push -u origin feat/feature-name` → `gh pr create`
 
diff --git a/backend/alembic/versions/011_add_role_check_constraint.py b/backend/alembic/versions/011_add_role_check_constraint.py
new file mode 100644
index 00000000..ebc78b6b
--- /dev/null
+++ b/backend/alembic/versions/011_add_role_check_constraint.py
@@ -0,0 +1,34 @@
+"""add role check constraint to users
+
+Revision ID: 011
+Revises: 010
+Create Date: 2026-02-05
+
+"""
+from typing import Sequence, Union
+
+from alembic import op
+
+
+# revision identifiers, used by Alembic.
+revision: str = '011'
+down_revision: Union[str, None] = '010'
+branch_labels: Union[str, Sequence[str], None] = None
+depends_on: Union[str, Sequence[str], None] = None
+
+
+def upgrade() -> None:
+    # Normalize any remaining non-standard role values to 'engineer'
+    # (migration 010 already converted 'admin' -> 'engineer', this is defense-in-depth)
+    op.execute(
+        "UPDATE users SET role = 'engineer' WHERE role NOT IN ('engineer', 'viewer')"
+    )
+    op.create_check_constraint(
+        'ck_users_role_enum',
+        'users',
+        "role IN ('engineer', 'viewer')"
+    )
+
+
+def downgrade() -> None:
+    op.drop_constraint('ck_users_role_enum', 'users', type_='check')
diff --git a/backend/app/api/endpoints/auth.py b/backend/app/api/endpoints/auth.py
index 34bd7e75..49589b4c 100644
--- a/backend/app/api/endpoints/auth.py
+++ b/backend/app/api/endpoints/auth.py
@@ -75,7 +75,7 @@ async def register(
         email=user_data.email,
         password_hash=get_password_hash(user_data.password),
         name=user_data.name,
-        role=user_data.role,
+        role="engineer",
         invite_code_id=invite_code_record.id if invite_code_record else None
     )
     db.add(new_user)
diff --git a/backend/app/api/endpoints/sessions.py b/backend/app/api/endpoints/sessions.py
index 922563b3..f7ec7ab0 100644
--- a/backend/app/api/endpoints/sessions.py
+++ b/backend/app/api/endpoints/sessions.py
@@ -1,3 +1,4 @@
+import html
 from datetime import datetime, timezone
 from typing import Annotated, Optional
 from uuid import UUID
@@ -348,9 +349,9 @@ def _generate_text_export(session: Session, options: SessionExport) -> str:
 
 def _generate_html_export(session: Session, options: SessionExport) -> str:
     """Generate HTML export."""
-    tree_name = session.tree_snapshot.get("name", "Troubleshooting Session")
+    tree_name = html.escape(session.tree_snapshot.get("name", "Troubleshooting Session"))
 
-    html = ['<!DOCTYPE html>', '<html>', '<head>',
+    html_parts = ['<!DOCTYPE html>', '<html>', '<head>',
             '<meta charset="UTF-8">',
             f'<title>{tree_name}</title>',
             '<style>',
@@ -366,40 +367,40 @@ def _generate_html_export(session: Session, options: SessionExport) -> str:
             '</head>', '<body>']
 
     if options.include_tree_info:
-        html.append(f'<h1>{tree_name}</h1>')
-        html.append('<div class="meta">')
+        html_parts.append(f'<h1>{tree_name}</h1>')
+        html_parts.append('<div class="meta">')
         if session.ticket_number:
-            html.append(f'<p><strong>Ticket:</strong> {session.ticket_number}</p>')
+            html_parts.append(f'<p><strong>Ticket:</strong> {html.escape(session.ticket_number)}</p>')
         if session.client_name:
-            html.append(f'<p><strong>Client:</strong> {session.client_name}</p>')
+            html_parts.append(f'<p><strong>Client:</strong> {html.escape(session.client_name)}</p>')
         if options.include_timestamps:
-            html.append(f'<p><strong>Started:</strong> {session.started_at.strftime("%Y-%m-%d %H:%M")}</p>')
+            html_parts.append(f'<p><strong>Started:</strong> {session.started_at.strftime("%Y-%m-%d %H:%M")}</p>')
             if session.completed_at:
-                html.append(f'<p><strong>Completed:</strong> {session.completed_at.strftime("%Y-%m-%d %H:%M")}</p>')
-        html.append('</div>')
+                html_parts.append(f'<p><strong>Completed:</strong> {session.completed_at.strftime("%Y-%m-%d %H:%M")}</p>')
+        html_parts.append('</div>')
 
     # Scratchpad / Evidence section
     scratchpad = getattr(session, 'scratchpad', '') or ''
     if scratchpad.strip():
-        html.append('<h2>Evidence / Reference</h2>')
-        html.append(f'<div class="scratchpad" style="white-space: pre-wrap; background: #f9f9f9; padding: 12px; border-radius: 4px; margin-bottom: 20px;">{scratchpad}</div>')
+        html_parts.append('<h2>Evidence / Reference</h2>')
+        html_parts.append(f'<div class="scratchpad" style="white-space: pre-wrap; background: #f9f9f9; padding: 12px; border-radius: 4px; margin-bottom: 20px;">{html.escape(scratchpad)}</div>')
 
-    html.append('<h2>Troubleshooting Steps</h2>')
+    html_parts.append('<h2>Troubleshooting Steps</h2>')
 
     for i, decision in enumerate(session.decisions, 1):
-        question = decision.get("question") or decision.get("action_performed", "Step")
-        answer = decision.get("answer", "")
-        notes = decision.get("notes", "")
+        question = html.escape(decision.get("question") or decision.get("action_performed", "Step"))
+        answer = html.escape(decision.get("answer", ""))
+        notes = html.escape(decision.get("notes", ""))
 
-        html.append('<div class="step">')
-        html.append(f'<h3>Step {i}: {question}</h3>')
+        html_parts.append('<div class="step">')
+        html_parts.append(f'<h3>Step {i}: {question}</h3>')
         if answer:
-            html.append(f'<p class="answer">Answer: {answer}</p>')
+            html_parts.append(f'<p class="answer">Answer: {answer}</p>')
         if notes:
-            html.append(f'<p class="notes">Notes: {notes}</p>')
+            html_parts.append(f'<p class="notes">Notes: {notes}</p>')
         if options.include_timestamps and decision.get("timestamp"):
-            html.append(f'<p class="timestamp">{decision["timestamp"]}</p>')
-        html.append('</div>')
+            html_parts.append(f'<p class="timestamp">{html.escape(str(decision["timestamp"]))}</p>')
+        html_parts.append('</div>')
 
-    html.extend(['</body>', '</html>'])
-    return "\n".join(html)
+    html_parts.extend(['</body>', '</html>'])
+    return "\n".join(html_parts)
diff --git a/backend/app/core/config.py b/backend/app/core/config.py
index dbc5e404..3a31cdb2 100644
--- a/backend/app/core/config.py
+++ b/backend/app/core/config.py
@@ -3,6 +3,9 @@ from pydantic import field_validator
 from typing import Optional
 
 
+_DEFAULT_SECRET_KEY = "your-secret-key-change-in-production-use-openssl-rand-hex-32"
+
+
 class Settings(BaseSettings):
     # Application
     APP_NAME: str = "ResolutionFlow"
@@ -26,7 +29,19 @@ class Settings(BaseSettings):
         return self.DATABASE_URL.replace("postgresql+asyncpg://", "postgresql://", 1)
 
     # JWT Settings
-    SECRET_KEY: str = "your-secret-key-change-in-production-use-openssl-rand-hex-32"
+    SECRET_KEY: str = _DEFAULT_SECRET_KEY
+
+    @field_validator("SECRET_KEY", mode="after")
+    @classmethod
+    def reject_default_secret_in_production(cls, v: str, info) -> str:
+        """Fail loudly if the default secret key is used outside of DEBUG mode."""
+        debug = info.data.get("DEBUG", False)
+        if v == _DEFAULT_SECRET_KEY and not debug:
+            raise ValueError(
+                "SECRET_KEY must be set to a secure value in production. "
+                "Generate one with: openssl rand -hex 32"
+            )
+        return v
     ALGORITHM: str = "HS256"
     ACCESS_TOKEN_EXPIRE_MINUTES: int = 15
     REFRESH_TOKEN_EXPIRE_DAYS: int = 7
diff --git a/backend/app/models/user.py b/backend/app/models/user.py
index 04b92c93..d21f4478 100644
--- a/backend/app/models/user.py
+++ b/backend/app/models/user.py
@@ -1,7 +1,7 @@
 import uuid
 from datetime import datetime, timezone
 from typing import Optional, TYPE_CHECKING
-from sqlalchemy import String, DateTime, ForeignKey, Boolean
+from sqlalchemy import String, DateTime, ForeignKey, Boolean, CheckConstraint
 from sqlalchemy.orm import Mapped, mapped_column, relationship
 from sqlalchemy.dialects.postgresql import UUID
 from app.core.database import Base
@@ -15,6 +15,12 @@ if TYPE_CHECKING:
 
 class User(Base):
     __tablename__ = "users"
+    __table_args__ = (
+        CheckConstraint(
+            "role IN ('engineer', 'viewer')",
+            name='ck_users_role_enum'
+        ),
+    )
 
     id: Mapped[uuid.UUID] = mapped_column(
         UUID(as_uuid=True),
diff --git a/backend/app/schemas/user.py b/backend/app/schemas/user.py
index 02c9aff1..54266bc6 100644
--- a/backend/app/schemas/user.py
+++ b/backend/app/schemas/user.py
@@ -11,7 +11,6 @@ class UserBase(BaseModel):
 
 class UserCreate(UserBase):
     password: str = Field(..., min_length=10, description="Password must be at least 10 characters")
-    role: str = Field(default="engineer", description="User role: engineer or viewer")
     invite_code: Optional[str] = Field(None, description="Invite code for registration (required when invite system is enabled)")
 
 
diff --git a/backend/tests/conftest.py b/backend/tests/conftest.py
index 518c454c..7845ba82 100644
--- a/backend/tests/conftest.py
+++ b/backend/tests/conftest.py
@@ -7,6 +7,7 @@ Provides test database setup, client fixtures, and authentication helpers.
 import asyncio
 from typing import AsyncGenerator, Generator
 import pytest
+import sqlalchemy as sa
 from httpx import AsyncClient, ASGITransport
 from sqlalchemy.ext.asyncio import AsyncSession, create_async_engine, async_sessionmaker
 from sqlalchemy.pool import NullPool
@@ -35,9 +36,10 @@ async def test_db() -> AsyncGenerator[AsyncSession, None]:
 
     This fixture:
     1. Creates a test database engine
-    2. Creates all tables
-    3. Yields a session for the test
-    4. Drops all tables after the test
+    2. Drops all existing tables (CASCADE to handle circular FKs)
+    3. Creates all tables
+    4. Yields a session for the test
+    5. Drops all tables after the test
     """
     # Create async engine for tests (with NullPool to avoid connection reuse issues)
     engine = create_async_engine(
@@ -46,8 +48,11 @@ async def test_db() -> AsyncGenerator[AsyncSession, None]:
         echo=False
     )
 
-    # Create all tables
+    # Drop and recreate all tables (use raw SQL CASCADE to handle circular FKs
+    # between users <-> invite_codes)
     async with engine.begin() as conn:
+        await conn.execute(sa.text("DROP SCHEMA public CASCADE"))
+        await conn.execute(sa.text("CREATE SCHEMA public"))
         await conn.run_sync(Base.metadata.create_all)
 
     # Create async session maker
@@ -61,9 +66,10 @@ async def test_db() -> AsyncGenerator[AsyncSession, None]:
     async with async_session_maker() as session:
         yield session
 
-    # Drop all tables after test
+    # Drop all tables after test (CASCADE for circular FKs)
     async with engine.begin() as conn:
-        await conn.run_sync(Base.metadata.drop_all)
+        await conn.execute(sa.text("DROP SCHEMA public CASCADE"))
+        await conn.execute(sa.text("CREATE SCHEMA public"))
 
     await engine.dispose()
 
@@ -99,8 +105,7 @@ async def test_user(client):
     user_data = {
         "email": "test@example.com",
         "password": "TestPassword123!",
-        "name": "Test User",
-        "role": "engineer"
+        "name": "Test User"
     }
 
     response = await client.post("/api/v1/auth/register", json=user_data)
@@ -181,23 +186,32 @@ async def test_tree(client, auth_headers):
 
 
 @pytest.fixture
-async def test_admin(client):
+async def test_admin(client, test_db):
     """
-    Create a test admin user and return their credentials.
+    Create a test super-admin user.
 
-    Returns:
-        dict with email, password, and user_data
+    Registers as engineer (the only role available at registration),
+    then promotes to super_admin directly via the DB session.
     """
+    from uuid import UUID as PyUUID
+    from sqlalchemy import select
+    from app.models.user import User
+
     admin_data = {
         "email": "admin@example.com",
         "password": "AdminPassword123!",
-        "name": "Test Admin",
-        "role": "admin"
+        "name": "Test Admin"
     }
 
     response = await client.post("/api/v1/auth/register", json=admin_data)
     assert response.status_code == 200 or response.status_code == 201
 
+    user_id = PyUUID(response.json()["id"])
+    result = await test_db.execute(select(User).where(User.id == user_id))
+    user = result.scalar_one()
+    user.is_super_admin = True
+    await test_db.commit()
+
     return {
         "email": admin_data["email"],
         "password": admin_data["password"],
diff --git a/backend/tests/test_auth.py b/backend/tests/test_auth.py
index 5df9ecd7..9e0727dd 100644
--- a/backend/tests/test_auth.py
+++ b/backend/tests/test_auth.py
@@ -13,8 +13,7 @@ class TestAuthentication:
         user_data = {
             "email": "newuser@example.com",
             "password": "SecurePass123!",
-            "name": "New User",
-            "role": "engineer"
+            "name": "New User"
         }
 
         response = await client.post("/api/v1/auth/register", json=user_data)
@@ -23,7 +22,7 @@ class TestAuthentication:
         data = response.json()
         assert data["email"] == user_data["email"]
         assert data["name"] == user_data["name"]
-        assert data["role"] == user_data["role"]
+        assert data["role"] == "engineer"
         assert "id" in data
         assert "password" not in data  # Password should not be returned
 
@@ -35,8 +34,7 @@ class TestAuthentication:
         user_data = {
             "email": test_user["email"],  # Use existing email
             "password": "AnotherPass123!",
-            "name": "Another User",
-            "role": "engineer"
+            "name": "Another User"
         }
 
         response = await client.post("/api/v1/auth/register", json=user_data)
@@ -93,3 +91,33 @@ class TestAuthentication:
         response = await client.get("/api/v1/auth/me")
 
         assert response.status_code == 401
+
+    @pytest.mark.asyncio
+    async def test_register_with_role_field_ignored(self, client: AsyncClient):
+        """Test that sending a role field at registration is ignored — always engineer."""
+        user_data = {
+            "email": "hacker@example.com",
+            "password": "HackerPass123!",
+            "name": "Hacker",
+            "role": "admin"
+        }
+
+        response = await client.post("/api/v1/auth/register", json=user_data)
+
+        assert response.status_code == 201
+        data = response.json()
+        assert data["role"] == "engineer"
+
+    @pytest.mark.asyncio
+    async def test_register_default_role_is_engineer(self, client: AsyncClient):
+        """Test that omitting role defaults to engineer."""
+        user_data = {
+            "email": "default@example.com",
+            "password": "DefaultPass123!",
+            "name": "Default User"
+        }
+
+        response = await client.post("/api/v1/auth/register", json=user_data)
+
+        assert response.status_code == 201
+        assert response.json()["role"] == "engineer"
diff --git a/backend/tests/test_sessions.py b/backend/tests/test_sessions.py
index 3ad858bc..31bf6cdb 100644
--- a/backend/tests/test_sessions.py
+++ b/backend/tests/test_sessions.py
@@ -603,3 +603,84 @@ class TestSessions:
         assert response.status_code == 200
         content = response.text
         assert "Evidence / Reference" not in content
+
+    @pytest.mark.asyncio
+    async def test_html_export_escapes_script_tags(
+        self, client: AsyncClient, auth_headers: dict, test_tree: dict
+    ):
+        """Test that HTML export escapes script tags in user content (XSS prevention)."""
+        session_data = {
+            "tree_id": test_tree["id"],
+            "ticket_number": '<script>alert("xss")</script>',
+            "client_name": '<img onerror="alert(1)" src=x>'
+        }
+        create_response = await client.post(
+            "/api/v1/sessions", json=session_data, headers=auth_headers
+        )
+        session_id = create_response.json()["id"]
+
+        response = await client.post(
+            f"/api/v1/sessions/{session_id}/export",
+            json={"format": "html", "include_tree_info": True},
+            headers=auth_headers
+        )
+
+        assert response.status_code == 200
+        content = response.text
+        assert '<script>' not in content
+        assert '&lt;script&gt;' in content
+        assert '&lt;img' in content
+
+    @pytest.mark.asyncio
+    async def test_html_export_escapes_special_chars(
+        self, client: AsyncClient, auth_headers: dict, test_tree: dict
+    ):
+        """Test that HTML export properly escapes special characters."""
+        session_data = {
+            "tree_id": test_tree["id"],
+            "ticket_number": 'TICK-001 <b>"bold"</b> & \'quoted\''
+        }
+        create_response = await client.post(
+            "/api/v1/sessions", json=session_data, headers=auth_headers
+        )
+        session_id = create_response.json()["id"]
+
+        response = await client.post(
+            f"/api/v1/sessions/{session_id}/export",
+            json={"format": "html", "include_tree_info": True},
+            headers=auth_headers
+        )
+
+        assert response.status_code == 200
+        content = response.text
+        assert '&amp;' in content
+        assert '&lt;b&gt;' in content
+
+    @pytest.mark.asyncio
+    async def test_html_export_escapes_scratchpad(
+        self, client: AsyncClient, auth_headers: dict, test_tree: dict
+    ):
+        """Test that HTML export escapes scratchpad content."""
+        create_response = await client.post(
+            "/api/v1/sessions",
+            json={"tree_id": test_tree["id"]},
+            headers=auth_headers
+        )
+        session_id = create_response.json()["id"]
+
+        await client.patch(
+            f"/api/v1/sessions/{session_id}/scratchpad",
+            json={"scratchpad": '<script>document.cookie</script>'},
+            headers=auth_headers
+        )
+
+        response = await client.post(
+            f"/api/v1/sessions/{session_id}/export",
+            json={"format": "html"},
+            headers=auth_headers
+        )
+
+        assert response.status_code == 200
+        content = response.text
+        assert '<script>' not in content
+        assert '&lt;script&gt;' in content