fix(authz): recheck policy before sandbox reuse - #5006
Conversation
willem-bd
left a comment
There was a problem hiding this comment.
[P2] Run one asynchronous authorization check per async sandbox tool invocation
ensure_sandbox_initialized_async() performs the new authorization check, then _run_sync_tool_after_async_sandbox_init() executes the synchronous tool body. Every synchronous body calls ensure_sandbox_initialized() again, and because this PR moves the gate before sandbox reuse, that second call authorizes again. I reproduced two authorization-provider calls for one async tool invocation.
Besides duplicating audit and rate-limit effects, the first pass calls synchronous authorize() on the event loop even though the pluggable provider contract exposes aauthorize(). Please make the async tool path await one async sandbox authorization check and avoid repeating it inside the offloaded synchronous body, with coverage asserting exactly one decision per invocation.
Focused validation at 03b0b06d: 31 tests passed; Ruff lint and format checks passed.
03b0b06 to
9032108
Compare
|
@willem-bd Thanks for catching this — fixed in The async tool path now:
I also switched the async eager middleware path to Validation: Ruff lint/format passed; 57 focused authorization, initialization, middleware, and blocking-I/O tests passed. A broader sandbox-tool run had 210 passes and the same 6 unrelated Windows path/symlink baseline failures. |
9032108 to
dfbded7
Compare
willem-bd
left a comment
There was a problem hiding this comment.
One remaining authorization-path issue found at the current head. Focused sandbox and authorization tests pass, but the composed middleware path still duplicates provider decisions.
willem-bd
left a comment
There was a problem hiding this comment.
I found two blocking issues at current head 9ec833e5:
-
[P2] Scope one sandbox authorization decision across the complete middleware invocation. In
backend/packages/harness/deerflow/sandbox/tools.py,_ASYNC_SANDBOX_AUTHORIZATION_CHECKEDonly covers the synchronous tool body dispatched by_run_sync_tool_after_async_sandbox_init. The defaultReadBeforeWriteMiddlewarere-entersensure_sandbox_initialized()after that scope ends while stamping a read mark, and before the scope starts while checking writes. I reproduced two provider decisions for one composedread_filecall: sync performsauthorize()twice; async performs oneaauthorize()plus a second syncauthorize(). This duplicates provider audit/rate-limit effects and can produce conflicting verdicts from a stateful provider. Please scope the marker across the full middleware tool invocation, while ensuringSandboxAuthorizationErrorcannot be swallowed by the read-before-write generic fail-open paths, and add composed read/write coverage. -
[P2] Keep config and provider resolution off the async event loop.
ensure_sandbox_initialized_async()evaluatessafe_app_config()synchronously, andauthorize_sandbox_execution_async()then calls_resolve_authorization_inputs()synchronously before its first await.get_app_config()stats and SHA-256 hashes the complete config file; provider resolution may dynamically import and construct a custom provider. Because this PR moves the check before reuse, that blocking work now runs on every reused async sandbox tool call. The strict blocking-I/O job does not expose it becausesafe_app_config()catches the detectorBlockingErroras a generic exception and returnsNone. Please offload the synchronous resolution work or pass a safely pre-resolved snapshot/provider, and add a strict blocking-I/O regression test.
Validation at the reviewed sandbox code: 108 focused authorization/middleware tests passed, and Ruff lint/format passed. The latest merge only incorporates main changes outside this sandbox diff, so both reproductions remain applicable.
|
Addressed both blocking issues in
Regression coverage now exercises composed sync/async Validation: 139 focused authorization/middleware tests passed; repository-wide Ruff lint and format checks passed. Please take another look when convenient. |
| file load performed by :func:`safe_app_config_async`. | ||
| """ | ||
| context_snapshot = dict(context) | ||
| inputs = await asyncio.to_thread( |
There was a problem hiding this comment.
[P2] Construct async providers on the event loop
This to_thread call includes resolve_authorization_provider(), which imports and constructs the configured custom provider. A structurally valid async provider may initialize a loop-affine client in its constructor; constructing it in this worker has no running event loop. I reproduced that provider failing before aauthorize() is reached: fail_closed denies every async sandbox call, while fail_open proceeds without any provider decision. The same provider can be constructed and used normally by the existing event-loop authorization paths. Please keep config hashing and other blocking discovery off the loop without moving provider construction to an unsupported thread (or reuse an appropriately resolved provider), and add a regression with a loop-affine async provider.
Related to #4063 and follow-up to #4911.
Why
Sandbox execution authorization was checked only when a sandbox was first acquired. If a thread already held a persisted sandbox id, the reuse path returned before authorization ran, so a live role or policy revocation did not take effect until that sandbox was released.
The authorization RFC defines sandbox:execute as a runtime execution decision. A cached sandbox should not become a capability that outlives the current policy.
What changed
Surface area
Screenshots / Recording
Not applicable; backend authorization behavior only.
Bug fix verification
Validation
AI assistance
Tool(s) used: Codex
How you used it: Inspected the authorization and sandbox lifecycle, wrote red regression tests, implemented the minimal gate relocation, reviewed the complete diff, and ran targeted and broader validation.