From 0b341086fb5a39cb943e9f0dca8365b0b978570c Mon Sep 17 00:00:00 2001 From: RockChinQ Date: Thu, 22 Oct 2026 21:51:01 +0800 Subject: [PATCH] LANG-145: enforce credential-session TTL on a timer, not on traffic (#2756) * fix(platform): enforce credential-session TTL on a timer, not on traffic The four adapter registration flows (Lark one-click app, DingTalk device flow, WeCom bot, QQ Official bind) each park a short-lived secret in an in-memory session dict until the caller persists it: app_secret, client_secret, or the bot secret. Each store had a TTL and a cleanup function, but the cleanup only ran at the top of a route handler. An abandoned flow therefore kept its secret in memory until some *other* request happened to arrive, which for a quiet instance can be far past the validity window (15 min for Lark, 10 for DingTalk, 5 for WeCom/QQ). Changes: - Move the sweep to module level as _sweep_credential_sessions(stores) so it is directly testable, and have it scrub app_secret/client_secret/secret as it evicts, so an evicted session dict no longer retains a readable credential. - Add a background sweeper task started from the router group's initialize(), running every 60s, so the TTL holds regardless of traffic. - Keep the per-route cleanups: they make eviction immediate under load. Tests: 9 new cases (expiry, fresh sessions survive, secret scrubbing, per-store TTL, cancelling a pending handshake task, not cancelling a finished one, malformed session without created_at, multiple evictions, empty stores). Co-authored-by: multica-agent * fix(platform): tolerate a task manager without create_task when starting the sweeper The sweeper is started from the adapter router group's initialize(). Some embeddings of the app provide a task manager that only exposes create_user_task, so an unconditional create_task broke those harnesses. Fall back to the per-route cleanups when create_task is unavailable. Co-authored-by: multica-agent --------- Co-authored-by: multica-agent --- .../controller/groups/platform/adapters.py | 74 +++++++++++++ .../api/test_credential_session_ttl.py | 102 ++++++++++++++++++ 2 files changed, 176 insertions(+) create mode 100644 tests/unit_tests/api/test_credential_session_ttl.py diff --git a/src/langbot/pkg/api/http/controller/groups/platform/adapters.py b/src/langbot/pkg/api/http/controller/groups/platform/adapters.py index d740221f1..6b7f41d16 100644 --- a/src/langbot/pkg/api/http/controller/groups/platform/adapters.py +++ b/src/langbot/pkg/api/http/controller/groups/platform/adapters.py @@ -78,9 +78,42 @@ def _pop_owned_session( _MAX_ADAPTER_SESSIONS = 100 +# How often the credential-session sweeper enforces its TTL, independent of traffic. +_CREDENTIAL_SWEEP_INTERVAL_SECONDS = 60 _MAX_ADAPTER_SESSIONS_PER_WORKSPACE = 10 +def _scrub_credential_fields(session: dict) -> None: + """Overwrite credential fields before a session dict is dropped.""" + for key in ('app_secret', 'client_secret', 'secret'): + if key in session: + session[key] = None + + +def _sweep_credential_sessions(stores: 'list[tuple[dict, float]]') -> int: + """Evict sessions past their TTL and scrub the secrets they held. + + Returns the number of evicted sessions. Each entry is ``(store, ttl)``; a + session is evicted when ``now - created_at > ttl``. + """ + import time + + now = time.time() + evicted = 0 + for store, ttl in stores: + expired = [sid for sid, session in store.items() if now - session.get('created_at', 0) > ttl] + for sid in expired: + session = store.pop(sid, None) + if not session: + continue + task = session.get('task') + if task is not None and not task.done(): + task.cancel() + _scrub_credential_fields(session) + evicted += 1 + return evicted + + def _start_adapter_session_task( ap, coro, @@ -1231,3 +1264,44 @@ class AdaptersRouterGroup(group.RouterGroup): if session and session.get('task') and not session['task'].done(): session['task'].cancel() return self.success(data={}) + + # ---- credential-session TTL enforcement ---- + # + # Every store above holds a short-lived secret (app_secret, + # client_secret, bot secret) between the platform handshake and the + # caller persisting it. The per-route cleanups only run when another + # request arrives, so an abandoned flow would keep its secret in memory + # long past its validity window. This sweeper enforces the TTL on a + # timer instead, and scrubs the secret fields as it evicts. + + def _credential_stores() -> list[tuple[dict, float]]: + return [ + (_create_app_sessions, _SESSION_TTL), + (_dingtalk_sessions, _DINGTALK_SESSION_TTL), + (_wecombot_sessions, _WECOMBOT_SESSION_TTL), + (_qqofficial_sessions, _QQOFFICIAL_SESSION_TTL), + ] + + def _sweep_now() -> int: + return _sweep_credential_sessions(_credential_stores()) + + async def _credential_session_sweeper() -> None: + """Enforce the session TTL on a timer, so it does not depend on traffic.""" + import asyncio as _asyncio + + while True: + await _asyncio.sleep(_CREDENTIAL_SWEEP_INTERVAL_SECONDS) + try: + _sweep_now() + except Exception: # pragma: no cover - a sweep must never kill the app + _stdout_logger.exception('credential session sweep failed') + + self._sweep_credential_sessions = _sweep_now + # Some embeddings (notably test harnesses) provide a task manager without + # create_task; the per-route cleanups still enforce the TTL there. + create_task = getattr(self.ap.task_mgr, 'create_task', None) + if callable(create_task): + create_task( + _credential_session_sweeper(), + name='platform-credential-session-sweeper', + ) diff --git a/tests/unit_tests/api/test_credential_session_ttl.py b/tests/unit_tests/api/test_credential_session_ttl.py new file mode 100644 index 000000000..ed264fdcb --- /dev/null +++ b/tests/unit_tests/api/test_credential_session_ttl.py @@ -0,0 +1,102 @@ +"""Credential-session TTL enforcement for the platform adapter flows. + +Every adapter registration flow (Lark one-click app, DingTalk device flow, WeCom +bot, QQ Official bind) parks a short-lived secret in an in-memory session dict +between the platform handshake and the caller persisting it. The per-route +cleanups only run when another request arrives, so an abandoned flow would keep +its secret in memory past its validity window. These tests cover the sweeper that +enforces the TTL on a timer and scrubs the secret as it evicts. +""" + +import time + +from langbot.pkg.api.http.controller.groups.platform.adapters import ( + _sweep_credential_sessions, +) + + +def _session(age_seconds: float, **extra): + return {'status': 'pending', 'created_at': time.time() - age_seconds, **extra} + + +def test_expired_sessions_are_evicted(): + store = {'old': _session(1000)} + assert _sweep_credential_sessions([(store, 900)]) == 1 + assert store == {} + + +def test_fresh_sessions_survive(): + store = {'new': _session(10)} + assert _sweep_credential_sessions([(store, 900)]) == 0 + assert 'new' in store + + +def test_secret_is_scrubbed_on_eviction(): + """The evicted dict must not keep the credential readable.""" + session = _session(1000, app_secret='super-secret', client_secret='other', secret='third') + store = {'old': session} + _sweep_credential_sessions([(store, 900)]) + + assert store == {} + assert session['app_secret'] is None + assert session['client_secret'] is None + assert session['secret'] is None + + +def test_ttl_is_per_store(): + """A store with a shorter window expires while a longer one keeps its entry.""" + short, long = {'s': _session(400)}, {'l': _session(400)} + assert _sweep_credential_sessions([(short, 300), (long, 900)]) == 1 + assert short == {} + assert 'l' in long + + +def test_pending_task_is_cancelled_on_eviction(): + class FakeTask: + def __init__(self): + self.cancelled = False + + def done(self): + return False + + def cancel(self): + self.cancelled = True + + task = FakeTask() + store = {'old': _session(1000, task=task)} + _sweep_credential_sessions([(store, 900)]) + assert task.cancelled is True + + +def test_finished_task_is_not_cancelled(): + class DoneTask: + def __init__(self): + self.cancelled = False + + def done(self): + return True + + def cancel(self): # pragma: no cover - must not be called + self.cancelled = True + + task = DoneTask() + store = {'old': _session(1000, task=task)} + _sweep_credential_sessions([(store, 900)]) + assert task.cancelled is False + + +def test_session_without_created_at_is_evicted(): + """A malformed session must not become immortal.""" + store = {'broken': {'status': 'pending'}} + assert _sweep_credential_sessions([(store, 900)]) == 1 + assert store == {} + + +def test_multiple_expired_sessions_in_one_store(): + store = {f's{i}': _session(1000) for i in range(5)} + assert _sweep_credential_sessions([(store, 900)]) == 5 + assert store == {} + + +def test_empty_stores_are_harmless(): + assert _sweep_credential_sessions([({}, 900), ({}, 60)]) == 0