fix: critical security hardening (Phase A permissions audit)
- 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>
This commit is contained in:
14
CLAUDE.md
14
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`
|
||||
|
||||
|
||||
34
backend/alembic/versions/011_add_role_check_constraint.py
Normal file
34
backend/alembic/versions/011_add_role_check_constraint.py
Normal file
@@ -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')
|
||||
@@ -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)
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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),
|
||||
|
||||
@@ -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)")
|
||||
|
||||
|
||||
|
||||
@@ -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"],
|
||||
|
||||
@@ -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"
|
||||
|
||||
@@ -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 '<script>' in content
|
||||
assert '<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 '&' in content
|
||||
assert '<b>' 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 '<script>' in content
|
||||
|
||||
Reference in New Issue
Block a user