Harden note reminder dispatch ownership (#2999)

routes/note_routes.py

@@ -95,6 +95,32 @@ def _note_to_dict(note: Note) -> Dict[str, Any]:
     }
 
 
+def _reminder_text_from_note(note: Note) -> tuple[str, str]:
+    """Return the reminder title/body from a stored note row."""
+    title = (note.title or "Note reminder").strip() or "Note reminder"
+    if note.items:
+        try:
+            items = json.loads(note.items)
+        except (json.JSONDecodeError, TypeError):
+            items = None
+        if isinstance(items, list):
+            pending: list[str] = []
+            for item in items:
+                if not isinstance(item, dict):
+                    continue
+                if item.get("done") or item.get("checked"):
+                    continue
+                text = str(item.get("text") or "").strip()
+                if text:
+                    pending.append(text)
+            if pending:
+                shown = "\n".join(f"- {text}" for text in pending[:8])
+                extra = f"\n...and {len(pending) - 8} more" if len(pending) > 8 else ""
+                return title, f"Pending ({len(pending)}):\n{shown}{extra}"
+            return title, f"{len(items)} item{'s' if len(items) != 1 else ''}"
+    return title, (note.content or "").strip()[:400]
+
+
 
 # ---------------------------------------------------------------------------
 # Reminder dispatch — module-level so background tasks (built-in actions)
@@ -542,6 +568,23 @@ def setup_note_routes(task_scheduler=None):
     def _owner(request: Request) -> Optional[str]:
         return get_current_user(request)
 
+    def _is_admin_or_single_user(request: Request, user: str | None) -> bool:
+        if user == "internal-tool":
+            return True
+        if not user:
+            # require_user() already admitted this request, which only happens
+            # for auth-disabled, loopback-bypass, or unconfigured single-user
+            # modes. There is no separate non-admin account boundary there.
+            return True
+        try:
+            from core.auth import AuthManager
+            auth_mgr = getattr(request.app.state, "auth_manager", None) or AuthManager()
+            if not getattr(auth_mgr, "is_configured", True):
+                return True
+            return bool(auth_mgr.is_admin(user))
+        except Exception:
+            return False
+
     # --- LIST ---
     @router.get("")
     def list_notes(
@@ -759,27 +802,44 @@ def setup_note_routes(task_scheduler=None):
         """
         # Gate against anonymous callers — LLM synthesis can burn tokens.
         from src.auth_helpers import require_user as _ru
-        _ru(request)
+        user = _ru(request)
         body = await request.json()
-        note_id = body.get("note_id")
-        title = (body.get("title") or "").strip()
-        note_body = (body.get("body") or "").strip()
+        note_id = str(body.get("note_id") or "").strip()
         if not note_id:
             raise HTTPException(400, "note_id required")
 
-        # Optional overrides let the test button pass the current UI values
-        # directly so the test never races against a pending settings save.
+        caller = _owner(request)
+        is_test = note_id.startswith("test-")
+        is_admin = _is_admin_or_single_user(request, user or caller)
         _override: dict = {}
-        if body.get("channel"):
-            _override["reminder_channel"] = body["channel"]
-        if body.get("webhook_integration_id"):
-            _override["reminder_webhook_integration_id"] = body["webhook_integration_id"]
-        if body.get("webhook_payload_template"):
-            _override["reminder_webhook_payload_template"] = body["webhook_payload_template"]
+        if is_test:
+            if not is_admin:
+                raise HTTPException(403, "Admin only")
+            title = (body.get("title") or "Test Reminder").strip() or "Test Reminder"
+            note_body = (body.get("body") or "").strip()
+            # Optional overrides let the admin settings test button pass the
+            # current UI values directly so it never races a pending save.
+            if body.get("channel"):
+                _override["reminder_channel"] = body["channel"]
+            if body.get("webhook_integration_id"):
+                _override["reminder_webhook_integration_id"] = body["webhook_integration_id"]
+            if body.get("webhook_payload_template"):
+                _override["reminder_webhook_payload_template"] = body["webhook_payload_template"]
+        else:
+            db = SessionLocal()
+            try:
+                note = db.query(Note).filter(Note.id == note_id).first()
+                if not note:
+                    raise HTTPException(404, "Note not found")
+                if caller is not None and note.owner != caller:
+                    raise HTTPException(404, "Note not found")
+                title, note_body = _reminder_text_from_note(note)
+            finally:
+                db.close()
 
         return await dispatch_reminder(
             title=title, note_body=note_body, note_id=note_id,
-            owner=_owner(request) or "",
+            owner=caller or "",
             queue_browser=False,
             settings_override=_override or None,
         )

src/settings_scrub.py

@@ -18,12 +18,20 @@ _SECRET_KEY_PATTERNS = (
     "_credential", "_credentials", "_key",
 )
 _SECRET_KEY_ALLOW = ("google_pse_cx",)  # public identifiers, not secrets
+_SENSITIVE_KEY_EXACT = (
+    # A stable global integration id is a capability handle for routes that can
+    # trigger outbound webhook sends; do not expose it to non-admin settings
+    # callers even though it is not secret-shaped.
+    "reminder_webhook_integration_id",
+)
 
 
 def is_secret_key(name: str) -> bool:
     n = (name or "").lower()
     if n in _SECRET_KEY_ALLOW:
         return False
+    if n in _SENSITIVE_KEY_EXACT:
+        return True
     return any(n.endswith(p) or n == p.lstrip("_") for p in _SECRET_KEY_PATTERNS)
 
 

tests/test_note_reminder_fire_scope.py

@@ -0,0 +1,173 @@
+import asyncio
+from types import SimpleNamespace
+
+import pytest
+from fastapi import HTTPException
+
+
+class _AuthManager:
+    is_configured = True
+
+    def __init__(self, admins=()):
+        self._admins = set(admins)
+
+    def is_admin(self, user):
+        return user in self._admins
+
+
+class _Request:
+    def __init__(self, body, *, user="alice", admins=()):
+        self._body = body
+        self.state = SimpleNamespace(current_user=user)
+        self.client = SimpleNamespace(host="127.0.0.1")
+        self.app = SimpleNamespace(
+            state=SimpleNamespace(auth_manager=_AuthManager(admins))
+        )
+
+    async def json(self):
+        return self._body
+
+
+class _Query:
+    def __init__(self, note):
+        self.note = note
+
+    def filter(self, *args, **kwargs):
+        return self
+
+    def first(self):
+        return self.note
+
+
+class _Db:
+    def __init__(self, note):
+        self.note = note
+        self.closed = False
+
+    def query(self, model):
+        return _Query(self.note)
+
+    def close(self):
+        self.closed = True
+
+
+def _endpoint(monkeypatch, note=None):
+    import routes.note_routes as note_routes
+
+    calls = []
+    db = _Db(note)
+
+    async def fake_dispatch_reminder(**kwargs):
+        calls.append(kwargs)
+        return {"ok": True}
+
+    monkeypatch.setattr(note_routes, "SessionLocal", lambda: db)
+    monkeypatch.setattr(note_routes, "dispatch_reminder", fake_dispatch_reminder)
+
+    router = note_routes.setup_note_routes()
+    endpoint = next(
+        route.endpoint for route in router.routes
+        if route.path == "/api/notes/fire-reminder" and "POST" in route.methods
+    )
+    return endpoint, calls, db
+
+
+def _note(**overrides):
+    data = {
+        "id": "note-1",
+        "owner": "alice",
+        "title": "Stored title",
+        "content": "Stored body",
+        "items": None,
+    }
+    data.update(overrides)
+    return SimpleNamespace(**data)
+
+
+def test_real_reminder_requires_owned_note(monkeypatch):
+    endpoint, calls, _db = _endpoint(monkeypatch, _note(owner="bob"))
+
+    with pytest.raises(HTTPException) as exc:
+        asyncio.run(endpoint(_Request({"note_id": "note-1"}, user="alice")))
+
+    assert exc.value.status_code == 404
+    assert calls == []
+
+
+def test_real_reminder_uses_stored_note_and_ignores_overrides(monkeypatch):
+    endpoint, calls, db = _endpoint(monkeypatch, _note())
+
+    result = asyncio.run(endpoint(_Request({
+        "note_id": "note-1",
+        "title": "Forged title",
+        "body": "Forged body",
+        "channel": "webhook",
+        "webhook_integration_id": "global-webhook",
+        "webhook_payload_template": '{"content":"owned"}',
+    }, user="alice")))
+
+    assert result == {"ok": True}
+    assert db.closed is True
+    assert calls == [{
+        "title": "Stored title",
+        "note_body": "Stored body",
+        "note_id": "note-1",
+        "owner": "alice",
+        "queue_browser": False,
+        "settings_override": None,
+    }]
+
+
+def test_real_checklist_reminder_body_is_built_from_stored_items(monkeypatch):
+    endpoint, calls, _db = _endpoint(monkeypatch, _note(items=(
+        '[{"text":"first","done":false},'
+        '{"text":"finished","done":true},'
+        '{"text":"second","checked":false}]'
+    )))
+
+    asyncio.run(endpoint(_Request({"note_id": "note-1"}, user="alice")))
+
+    assert calls[0]["note_body"] == "Pending (2):\n- first\n- second"
+
+
+def test_non_admin_cannot_fire_synthetic_test_reminder(monkeypatch):
+    endpoint, calls, _db = _endpoint(monkeypatch)
+
+    with pytest.raises(HTTPException) as exc:
+        asyncio.run(endpoint(_Request({
+            "note_id": "test-123",
+            "title": "Test Reminder",
+            "body": "Test body",
+            "channel": "webhook",
+            "webhook_integration_id": "global-webhook",
+        }, user="alice")))
+
+    assert exc.value.status_code == 403
+    assert calls == []
+
+
+def test_admin_test_reminder_can_use_current_ui_overrides(monkeypatch):
+    endpoint, calls, _db = _endpoint(monkeypatch)
+
+    result = asyncio.run(endpoint(_Request({
+        "note_id": "test-123",
+        "title": "Test Reminder",
+        "body": "Test body",
+        "channel": "webhook",
+        "webhook_integration_id": "global-webhook",
+        "webhook_payload_template": '{"content":"{{message}}"}',
+    }, user="admin", admins={"admin"})))
+
+    assert result == {"ok": True}
+    assert calls == [{
+        "title": "Test Reminder",
+        "note_body": "Test body",
+        "note_id": "test-123",
+        "owner": "admin",
+        "queue_browser": False,
+        "settings_override": {
+            "reminder_channel": "webhook",
+            "reminder_webhook_integration_id": "global-webhook",
+            "reminder_webhook_payload_template": '{"content":"{{message}}"}',
+        },
+    }]

tests/test_settings_scrub.py

@@ -49,6 +49,16 @@ def test_google_pse_cx_is_public():
     assert scrub_settings({"google_pse_cx": "cx123"})["google_pse_cx"] == "cx123"
 
 
+def test_webhook_integration_handle_blanked():
+    out = scrub_settings({
+        "reminder_webhook_integration_id": "global-webhook",
+        "reminder_webhook_payload_template": '{"content":"{{message}}"}',
+    })
+    assert is_secret_key("reminder_webhook_integration_id") is True
+    assert out["reminder_webhook_integration_id"] == ""
+    assert out["reminder_webhook_payload_template"] == '{"content":"{{message}}"}'
+
+
 def test_empty_and_nonstring_secret_values_untouched():
     out = scrub_settings({"api_key": "", "feature_key": 7, "x_token": None})
     assert out["api_key"] == ""     # already empty