Skip to content

Commit 02268db

Browse files
committed
fix(tasks): fail closed on unconfirmed MCP rebind for unknown bindings
The session marker self-expires at half the OAuth token lifetime, so an absent marker can mean the previous actor's session is still live, not that the sandbox is fresh. Treating that as a best-effort first bind delivered a new actor's follow-up under the previous actor's credentials when the mint or refresh failed in the marker-expired window. Fail closed whenever a rebind can't be confirmed, not only on a known transition.
1 parent 6e5f3f5 commit 02268db

2 files changed

Lines changed: 16 additions & 11 deletions

File tree

products/tasks/backend/temporal/process_task/activities/send_followup_to_sandbox.py

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -263,11 +263,14 @@ def _refresh_sandbox_mcp(
263263
) -> bool:
264264
"""Rebind the sandbox's MCP session to this message's actor.
265265
266-
Returns ``True`` when the session is safe to use (unchanged actor, first
267-
bind, or a successful rebind) and ``False`` only when an actor *transition*
268-
could neither rebind nor clear the previous actor's session — the caller
269-
then fails the follow-up closed. Same-actor rotation and first binds stay
270-
best-effort. Retries the refresh once before giving up.
266+
Returns ``True`` when the session is safe to use (unchanged actor or a
267+
successful rebind) and ``False`` when a rebind could not be confirmed — the
268+
caller then fails the follow-up closed. A rebind is unconfirmed whenever the
269+
mint or refresh fails and the binding is not known to be this actor's,
270+
including an *unknown* binding: the marker self-expires at half the token
271+
lifetime, so an absent marker can mean the previous actor's session is still
272+
live, not that the sandbox is fresh. Retries the refresh once before giving
273+
up.
271274
"""
272275
run_id = str(task_run.id)
273276
if actor_user is None:
@@ -293,7 +296,7 @@ def _refresh_sandbox_mcp(
293296
access_token = create_oauth_access_token_for_run(task_run.task, state, scopes=scopes)
294297
except Exception as e:
295298
logger.warning("refresh_mcp_token_mint_failed", run_id=run_id, error=str(e))
296-
return not is_transition # first-bind: best-effort; transition: fail closed
299+
return False # rebind unconfirmed → fail closed (unknown binding may hide a live session)
297300

298301
mcp_configs = get_sandbox_ph_mcp_configs(
299302
token=access_token,
@@ -372,7 +375,7 @@ def _refresh_sandbox_mcp(
372375
error=retry.error,
373376
status_code=retry.status_code,
374377
)
375-
return not is_transition # transition that never rebound → fail closed
378+
return False # rebind never confirmed → fail closed (unknown binding may hide a live session)
376379

377380

378381
def _get_stop_reason(result_data: dict[str, Any] | None) -> str:

products/tasks/backend/temporal/process_task/tests/test_send_followup_to_sandbox.py

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -223,11 +223,13 @@ def test_transition_refresh_failure_reports_unsafe(
223223
assert safe is False
224224
assert get_sandbox_mcp_session_user("run-1") == 99
225225

226-
def test_first_bind_refresh_failure_stays_best_effort(
226+
def test_unknown_binding_refresh_failure_fails_closed(
227227
self, mock_oauth, mock_ph_configs, mock_user_configs, mock_send_refresh, _sleep
228228
):
229-
# No prior binding: a refresh failure is non-fatal (no earlier actor's
230-
# session to leak), so the gate reports safe and delivery proceeds.
229+
# No marker for this scope: the marker self-expires before the OAuth
230+
# session does, so an absent one may hide the previous actor's still-live
231+
# session rather than a fresh sandbox. When the refresh can't confirm the
232+
# rebind, the gate reports unsafe so the caller fails closed.
231233
mock_oauth.return_value = "fresh-token"
232234
mock_ph_configs.return_value = [_make_mcp_config()]
233235
mock_user_configs.return_value = []
@@ -236,7 +238,7 @@ def test_first_bind_refresh_failure_stays_best_effort(
236238
actor = MagicMock(id=42)
237239
safe = _refresh_sandbox_mcp(_make_task_run_mock(), "read_only", None, actor_user=actor, state=None)
238240

239-
assert safe is True
241+
assert safe is False
240242

241243

242244
@patch("products.tasks.backend.temporal.process_task.activities.send_followup_to_sandbox.send_refresh_session")

0 commit comments

Comments
 (0)