From 299538ea4e8aa365fe0ebe65f9cb4aa97b71feb9 Mon Sep 17 00:00:00 2001 From: Vykos Date: Sun, 7 Jun 2026 12:52:27 +0200 Subject: [PATCH] Harden note reminder dispatch ownership (#2999) --- routes/note_routes.py | 86 ++++++++++-- src/settings_scrub.py | 8 ++ tests/test_note_reminder_fire_scope.py | 173 +++++++++++++++++++++++++ tests/test_settings_scrub.py | 10 ++ 4 files changed, 264 insertions(+), 13 deletions(-) create mode 100644 tests/test_note_reminder_fire_scope.py diff --git a/routes/note_routes.py b/routes/note_routes.py index 947788a42..3332c1b78 100644 --- a/routes/note_routes.py +++ b/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, ) diff --git a/src/settings_scrub.py b/src/settings_scrub.py index 6c76438d6..7dc462f2e 100644 --- a/src/settings_scrub.py +++ b/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) diff --git a/tests/test_note_reminder_fire_scope.py b/tests/test_note_reminder_fire_scope.py new file mode 100644 index 000000000..dc0a67094 --- /dev/null +++ b/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}}"}', + }, + }] diff --git a/tests/test_settings_scrub.py b/tests/test_settings_scrub.py index fe85fc33f..3f772a88c 100644 --- a/tests/test_settings_scrub.py +++ b/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