Security hardening (maziggy/bambuddy-security #15)

backend/app/core/database.py

@@ -5582,6 +5582,12 @@ async def _run_migrations(conn):
     # alone (the email adds the link to any template, at the end).
     await _migrate_password_reset_template_link(conn)
 
+    # Migration: the Welcome Email template no longer holds the new user's
+    # password; Bambuddy adds the login details itself.
+    # A plain UPDATE guarded on the old text, so an edited template is left
+    # alone (a {password} in it renders as nothing).
+    await _migrate_welcome_template_without_password(conn)
+
     # Migration: supplier master list + spool assignments (#2988).
     # create_all() covers fresh installs; this covers upgrades.
     await _migrate_create_supplier_tables(conn)
@@ -6602,6 +6608,27 @@ async def _migrate_password_reset_template_link(conn) -> None:
     )
 
 
+_WELCOME_TEMPLATE_BODY = (
+    "Welcome {username}!\n\nYour account has been created.\nUsername: {username}\nPassword: {password}\n\nLogin at: {login_url}",
+    "Welcome {username}!\n\nYour {app_name} account has been created. Please change your password after your first login.\n\nLogin at: {login_url}\n\nYour login details:",
+)
+
+
+async def _migrate_welcome_template_without_password(conn) -> None:
+    """Take the password out of the Welcome Email template's default text.
+
+    Updates the body only while it is still the old shipped default, so an
+    admin's own wording is kept.
+    """
+    from sqlalchemy import text
+
+    old, new = _WELCOME_TEMPLATE_BODY
+    await conn.execute(
+        text("UPDATE notification_templates SET body_template = :new WHERE event_type = :et AND body_template = :old"),
+        {"new": new, "et": "user_created", "old": old},
+    )
+
+
 async def _migrate_location_ha_sensor_unique_binding(conn) -> None:
     """Unique index on location_ha_sensors (location_id, entity_id) (#2824).
 

backend/app/models/notification_template.py

@@ -222,7 +222,7 @@ DEFAULT_TEMPLATES = [
         "event_type": "user_created",
         "name": "Welcome Email",
         "title_template": "Welcome to {app_name}",
-        "body_template": "Welcome {username}!\n\nYour account has been created.\nUsername: {username}\nPassword: {password}\n\nLogin at: {login_url}",
+        "body_template": "Welcome {username}!\n\nYour {app_name} account has been created. Please change your password after your first login.\n\nLogin at: {login_url}\n\nYour login details:",
     },
     {
         "event_type": "password_reset",

backend/app/schemas/notification_template.py

@@ -130,7 +130,7 @@ EVENT_VARIABLES: dict[str, list[str]] = {
         "app_name",
     ],
     # User management notifications
-    "user_created": ["username", "password", "login_url", "app_name", "timestamp"],
+    "user_created": ["username", "login_url", "app_name", "timestamp"],
     "password_reset": ["username", "app_name", "timestamp"],
     # User email print notifications
     "user_print_start": ["username", "printer", "filename", "timestamp", "app_name"],
@@ -401,7 +401,6 @@ SAMPLE_DATA: dict[str, dict[str, str]] = {
     # User management notifications
     "user_created": {
         "username": "john_doe",
-        "password": "<generated-password>",
         "login_url": "https://bambuddy.example.com/login",
         "app_name": "Bambuddy",
         "timestamp": "2024-01-15 14:30",

backend/app/services/email_service.py

@@ -384,12 +384,9 @@ async def create_password_reset_link_email_from_template(
     """Create the password-reset link email from the "password_reset" template.
 
     The link is never a template variable: the code adds it, on a line of its
-    own at the end and as the button. The template is editable with
-    notification_templates:update (Operators), and a link placed by the
-    template could be wrapped in another URL (https://evil/?next={reset_url})
-    whose page reads the token from its fragment and resets the account, an
-    admin's too. A {reset_url} written into the template renders as nothing.
-    No template: the built-in email.
+    own at the end and as the button, so the template's text cannot change it.
+    A {reset_url} written into the template renders as nothing. No template:
+    the built-in email.
 
     Returns:
         Tuple of (subject, text_body, html_body)
@@ -435,6 +432,10 @@ async def create_welcome_email_from_template(
 ) -> tuple[str, str, str]:
     """Create welcome email content using notification template from database.
 
+    The password is never a template variable: Bambuddy adds the login details
+    itself, at the end of the text and in a box in the HTML, so the template's
+    text cannot change them. A {password} in the template renders as nothing.
+
     Args:
         db: Database session
         username: Username of the new user
@@ -453,16 +454,17 @@ async def create_welcome_email_from_template(
         variables = {
             "app_name": app_name,
             "username": username,
-            "password": password,
             "login_url": login_url,
+            "timestamp": datetime.now().strftime("%Y-%m-%d %H:%M"),
         }
 
         subject = render_template(template.title_template, variables)
-        text_body = render_template(template.body_template, variables)
+        body = render_template(template.body_template, variables).rstrip()
+        text_body = f"{body}\n\nUsername: {username}\nPassword: {password}"
 
         # Create HTML version with embedded login button
-        # Escape text_body to prevent XSS vulnerabilities and convert newlines to <br> tags
-        escaped_text_body = html.escape(text_body).replace("\n", "<br>\n")
+        # Escape the body to prevent XSS vulnerabilities and convert newlines to <br> tags
+        escaped_body = html.escape(body).replace("\n", "<br>\n")
         html_body = f"""<!DOCTYPE html>
 <html>
 <head>
@@ -474,10 +476,15 @@ async def create_welcome_email_from_template(
         <h1 style="color: #ffffff; margin: 0; font-size: 24px; text-shadow: 0 1px 2px rgba(0,0,0,0.3);">{html.escape(subject)}</h1>
     </div>
     <div style="background: #f9f9f9; padding: 30px; border-radius: 0 0 8px 8px; border: 1px solid #ddd; border-top: none;">
-        <div style="font-size: 16px;">{escaped_text_body}</div>
+        <div style="font-size: 16px;">{escaped_body}</div>
+
+        <div style="background: white; padding: 20px; border-radius: 4px; margin: 20px 0; border-left: 4px solid #667eea;">
+            <p style="margin: 0 0 10px 0;"><strong>Username:</strong> <code style="background: #f0f0f0; padding: 2px 6px; border-radius: 3px;">{html.escape(username)}</code></p>
+            <p style="margin: 0;"><strong>Password:</strong> <code style="background: #f0f0f0; padding: 2px 6px; border-radius: 3px;">{html.escape(password)}</code></p>
+        </div>
 
         <div style="text-align: center; margin: 30px 0;">
-            <a href="{login_url}" style="display: inline-block; background-color: #667eea; color: #ffffff; padding: 12px 30px; text-decoration: none; border-radius: 4px; font-weight: bold;">Login Now</a>
+            <a href="{html.escape(login_url)}" style="display: inline-block; background-color: #667eea; color: #ffffff; padding: 12px 30px; text-decoration: none; border-radius: 4px; font-weight: bold;">Login Now</a>
         </div>
     </div>
 </body>

backend/tests/integration/test_welcome_template_password_route.py

@@ -0,0 +1,46 @@
+"""Creating a user sends the Welcome template with the login details added by
+Bambuddy, never a password placed by the template."""
+
+from unittest.mock import patch
+
+import pytest
+from httpx import AsyncClient
+
+from backend.app.models.notification_template import NotificationTemplate
+from backend.tests.integration.test_advanced_auth_api import _setup_admin, _setup_smtp_and_advanced_auth
+
+pytestmark = [pytest.mark.asyncio, pytest.mark.integration]
+
+
+async def test_a_new_users_password_is_not_wrapped_in_the_templates_link(async_client: AsyncClient, db_session):
+    token = await _setup_admin(async_client, "welcadmin", "AdminPass1!")
+    headers = {"Authorization": f"Bearer {token}"}
+    db_session.add(
+        NotificationTemplate(
+            event_type="user_created",
+            name="Welcome Email",
+            title_template="Welcome {password}",
+            body_template="Activate: https://elsewhere.example/a?p={password}",
+            is_default=False,
+        )
+    )
+    await db_session.commit()
+
+    with patch("backend.app.api.routes.users.send_email") as mock_send:
+        await _setup_smtp_and_advanced_auth(async_client, token)
+        created = await async_client.post(
+            "/api/v1/users/",
+            headers=headers,
+            json={"username": "newbie", "email": "newbie@test.com", "role": "user"},
+        )
+
+    assert created.status_code == 201, created.text
+    mock_send.assert_called_once()
+    _smtp, to, subject, text_body, html_body = mock_send.call_args[0][:5]
+    assert to == "newbie@test.com"
+    assert subject == "Welcome "
+    words, details = text_body.split("\n\n")
+    assert words == "Activate: https://elsewhere.example/a?p="
+    assert details.startswith("Username: newbie\nPassword: ")
+    assert len(details.removeprefix("Username: newbie\nPassword: ")) >= 12
+    assert "p=" + details.split("Password: ")[1][:4] not in html_body

backend/tests/unit/services/test_email_service.py

@@ -53,7 +53,9 @@ class TestEmailTemplateFormatting:
         # Verify text body has newlines
         assert "\n\n" in text_body
         assert "Hello testuser!" in text_body
-        assert "Your password is: testpass123" in text_body
+        # The password is added by Bambuddy, never by the template
+        assert "Your password is: \n" in text_body
+        assert text_body.endswith("Username: testuser\nPassword: testpass123")
 
         # Verify HTML body has <br> tags instead of relying on CSS
         assert "<br>" in html_body
@@ -65,7 +67,8 @@ class TestEmailTemplateFormatting:
 
         # Verify that escaped content is present (XSS protection)
         assert "Hello testuser!<br>" in html_body
-        assert "Your password is: testpass123<br>" in html_body
+        assert "Your password is: <br>" in html_body
+        assert ">testpass123</code>" in html_body
 
     @pytest.mark.asyncio
     async def test_email_header_padding(self):

backend/tests/unit/services/test_password_reset_template_email.py

@@ -4,9 +4,8 @@ The template was seeded and editable, but since the reset became a link email
 no code read it: the email was always the built-in one, whatever the admin
 wrote. Its default still described the old flow (a new password emailed in
 plain text). The email now renders the template, and the code adds the link at
-the end. The link is not a template variable: Operators may edit templates,
-and a link the template places could be wrapped in another URL whose page
-reads the token from its fragment and resets the account, an admin's too.
+the end. The link is not a template variable, so the template's text cannot
+change it.
 """
 
 from __future__ import annotations
@@ -59,13 +58,13 @@ async def test_an_edited_template_is_what_the_user_gets_with_the_link_at_the_end
 
 
 async def test_a_template_cannot_place_the_link():
-    """An Operator's body wrapping the link in their own URL: the token would
-    land in the fragment of their page."""
+    """A {reset_url} in the template renders as nothing; the link is only the
+    one the code adds."""
     subject, text_body, html_body = await _render(
-        _template("Reset {reset_url}", "Reset here: https://evil.example/r?next={reset_url}")
+        _template("Reset {reset_url}", "Reset here: https://elsewhere.example/r?next={reset_url}")
     )
 
-    assert text_body == f"Reset here: https://evil.example/r?next=\n\n{RESET_URL}"
+    assert text_body == f"Reset here: https://elsewhere.example/r?next=\n\n{RESET_URL}"
     assert "reset_token" not in subject
     assert "next=https" not in text_body and "next=https" not in html_body
     assert html_body.count("reset_token") == 2  # the line at the end and the button

backend/tests/unit/services/test_welcome_template_password.py

@@ -0,0 +1,169 @@
+"""A new user's initial password never passes through the editable Welcome template.
+
+The Welcome Email template (user_created) rendered {password} as a variable.
+Bambuddy now adds the login details itself, at the end of the text and in the
+HTML, so the template's text cannot change them; a {password} in the template
+renders as nothing.
+"""
+
+from __future__ import annotations
+
+import re
+from unittest.mock import AsyncMock, patch
+
+import pytest
+from sqlalchemy import text
+from sqlalchemy.ext.asyncio import AsyncSession, create_async_engine
+
+from backend.app.core import database as _db
+from backend.app.models.notification_template import DEFAULT_TEMPLATES, NotificationTemplate
+from backend.app.schemas.notification_template import EVENT_VARIABLES, SAMPLE_DATA
+from backend.app.services.email_service import create_welcome_email_from_template
+
+PASSWORD = "S3cr3t&<pw>"
+LOGIN_URL = "https://bambuddy.example.com/login"
+DEFAULT = next(t for t in DEFAULT_TEMPLATES if t["event_type"] == "user_created")
+DETAILS = f"Username: maker\nPassword: {PASSWORD}"
+
+
+def _template(title: str, body: str) -> NotificationTemplate:
+    return NotificationTemplate(
+        event_type="user_created", name="Welcome Email", title_template=title, body_template=body, is_default=False
+    )
+
+
+async def _render(template: NotificationTemplate | None):
+    with patch("backend.app.services.email_service.get_notification_template", return_value=template):
+        return await create_welcome_email_from_template(AsyncMock(spec=AsyncSession), "maker", PASSWORD, LOGIN_URL)
+
+
+async def test_a_template_cannot_carry_the_password_off():
+    subject, text_body, html_body = await _render(
+        _template("Hi {password}", "Activate: https://elsewhere.example/a?p={password}")
+    )
+
+    assert PASSWORD not in subject
+    assert text_body == f"Activate: https://elsewhere.example/a?p=\n\n{DETAILS}"
+    assert "p=S3cr3t" not in html_body
+
+
+async def test_the_default_template_sends_the_details_once():
+    subject, text_body, html_body = await _render(_template(DEFAULT["title_template"], DEFAULT["body_template"]))
+
+    assert subject == "Welcome to BamBuddy"
+    assert text_body.endswith(f"\n\n{DETAILS}")
+    assert text_body.count(PASSWORD) == 1
+    assert LOGIN_URL in text_body
+    assert "{" not in text_body
+    assert f'href="{LOGIN_URL}"' in html_body
+
+
+async def test_the_details_are_escaped_in_the_html():
+    _subject, _text, html_body = await _render(_template("Welcome", "Hello"))
+
+    assert html_body.count("S3cr3t&amp;&lt;pw&gt;") == 1  # in the box only
+    assert PASSWORD not in html_body
+
+
+async def test_an_edited_template_keeps_its_words():
+    _subject, text_body, _html = await _render(_template("Welcome", "Hallo {username}, viel Spass mit {app_name}!"))
+
+    assert text_body == f"Hallo maker, viel Spass mit BamBuddy!\n\n{DETAILS}"
+
+
+async def test_the_timestamp_the_editor_offers_is_filled():
+    _subject, text_body, _html = await _render(_template("Welcome", "Created {timestamp}"))
+
+    assert re.match(r"Created \d{4}-\d{2}-\d{2} \d{2}:\d{2}\n\n", text_body)
+
+
+async def test_without_a_template_the_built_in_email_is_sent():
+    subject, text_body, _html = await _render(None)
+
+    assert subject == "Welcome to BamBuddy - Your Account Details"
+    assert PASSWORD in text_body
+
+
+def test_the_editor_does_not_offer_the_password():
+    assert "password" not in EVENT_VARIABLES["user_created"]
+    assert "password" not in SAMPLE_DATA["user_created"]
+
+
+def test_the_default_template_has_no_password():
+    assert "{password}" not in DEFAULT["body_template"]
+    assert "{password}" not in DEFAULT["title_template"]
+
+
+# --- upgrade: the shipped default is rewritten, an edited one is kept ---
+
+
+@pytest.fixture
+async def engine():
+    engine = create_async_engine("sqlite+aiosqlite:///:memory:", echo=False)
+    async with engine.begin() as conn:
+        await conn.run_sync(NotificationTemplate.__table__.create)
+    try:
+        yield engine
+    finally:
+        await engine.dispose()
+
+
+async def _insert(conn, body: str) -> None:
+    await conn.execute(
+        text(
+            "INSERT INTO notification_templates (event_type, name, title_template, body_template, is_default) "
+            "VALUES ('user_created', 'Welcome Email', 'Welcome to {app_name}', :b, 1)"
+        ),
+        {"b": body},
+    )
+
+
+async def _body(conn) -> str:
+    return (
+        await conn.execute(text("SELECT body_template FROM notification_templates WHERE event_type = 'user_created'"))
+    ).scalar_one()
+
+
+async def test_the_upgrade_rewrites_the_old_default(engine):
+    old, new = _db._WELCOME_TEMPLATE_BODY
+    async with engine.begin() as conn:
+        await _insert(conn, old)
+
+    for _ in range(2):  # runs on every start
+        async with engine.begin() as conn:
+            await _db._migrate_welcome_template_without_password(conn)
+
+    async with engine.begin() as conn:
+        assert await _body(conn) == new
+
+
+async def test_the_upgrade_keeps_an_edited_template(engine):
+    async with engine.begin() as conn:
+        await _insert(conn, "My own words {password}")
+
+    async with engine.begin() as conn:
+        await _db._migrate_welcome_template_without_password(conn)
+        assert await _body(conn) == "My own words {password}"
+
+
+def test_the_upgrade_gives_what_a_fresh_install_gets():
+    assert _db._WELCOME_TEMPLATE_BODY[1] == DEFAULT["body_template"]
+
+
+async def test_start_up_runs_the_upgrade(tmp_path):
+    from backend.app.core.database import Base, run_migrations
+    from backend.app.main import app  # noqa: F401  registers every model, as init_db does
+
+    engine = create_async_engine(f"sqlite+aiosqlite:///{tmp_path / 'old.db'}")
+    try:
+        async with engine.begin() as conn:
+            await conn.run_sync(Base.metadata.create_all)
+            await _insert(conn, _db._WELCOME_TEMPLATE_BODY[0])
+
+        async with engine.begin() as conn:
+            await run_migrations(conn)
+
+        async with engine.connect() as conn:
+            assert await _body(conn) == _db._WELCOME_TEMPLATE_BODY[1]
+    finally:
+        await engine.dispose()