feat(authz): enforce sandbox:execute authorization at sandbox acquisition (#4063 Phase 3) - #4911
Conversation
github-actions
Bot
added
area:backend
willem-bd
left a comment
willem-bd
left a comment
There was a problem hiding this comment.
Reviewed at head 069aeee. Overall this looks well put together — gating at the single sandbox-acquisition entry point (rather than a tool-name set in middleware) is the right call, the uploads/artifacts deny-skips-sync behavior is handled consistently (including the sandbox is not None guard before the sync loop), and the test coverage hits allow/deny/no-policy/fail-closed/fail-open plus the router integration paths. Three notes below: the eager-path deny won't get the RFC §9 friendly-message treatment, provider-resolution failures invert fail-open semantics in the harness paths, and a question about the no-user branch in the routers.
| return super().before_agent(state, runtime) | ||
| # Phase 3: enforce sandbox:execute authorization before acquiring | ||
| # (eager path). The lazy path is gated inside ensure_sandbox_initialized. | ||
| authorize_sandbox_execution( |
There was a problem hiding this comment.
Suggestion: on this eager path, a SandboxAuthorizationError raised from before_agent is not inside a tool call, so ToolErrorHandlingMiddleware never converts it into a friendly ToolMessage — it will surface as a run-level graph error instead of the RFC §9 behavior the PR description describes (which only holds for the lazy path). The Gateway factory only constructs SandboxMiddleware(lazy_init=True) today (agents/factory.py:232), so this is currently unreachable, but if lazy_init=False is ever used with authorization enabled, a denied role crashes the whole run rather than getting the friendly per-turn message. Consider catching the error here and starting the agent without a sandbox (the lazy gate in ensure_sandbox_initialized would then deny per-tool with the friendly message), or noting the limitation explicitly so the two paths' deny semantics are documented as intentional.
| if authz_config is None or getattr(authz_config, "enabled", None) is not True: | ||
| return | ||
|
|
||
| provider = resolve_authorization_provider(authz_config) |
There was a problem hiding this comment.
resolve_authorization_provider runs outside the try, so a misconfigured provider (bad class path, constructor failure → ValueError) propagates as a raw exception rather than a SandboxAuthorizationError. The Gateway wrapper (authorize_sandbox_for_request) handles that explicitly and degrades per fail_closed, but in the harness paths (middleware and ensure_sandbox_initialized) the outcome inverts fail_open: a provider resolution error effectively denies sandbox (raw exception through the tool) even though every other provider error under fail_open is meant to allow. Consider moving the resolution inside the try / wrapping it so resolution failures follow the same fail_closed/fail_open decision as authorize() errors.
| # thread uploads dir; the agent cannot consume them via sandbox anyway). | ||
| try: | ||
| user = await get_optional_user_from_request(request) | ||
| if user is not None: |
There was a problem hiding this comment.
Nit/question: when get_optional_user_from_request returns None, the sync proceeds with no authorization check at all (same in artifacts.py). Is that combination reachable — e.g. authentication middleware disabled while authorization.enabled: true? If yes, this is a fail-open hole in the sync path; if it can't happen in practice, a one-line comment saying why would help future readers. (Same applies to the artifacts router's copy of this block.)
17a509d to
c6a17da
Compare
|
All three notes addressed in 1. Eager-path deny (run-level error) — fixed by deferring to the lazy gate. 2. Provider-resolution failures inverting fail-open — fixed. 3. Also fixed while re-verifying: my two uploads-router tests were writing into the real global uploads directory — now isolated to |
willem-bd
left a comment
willem-bd
left a comment
There was a problem hiding this comment.
Re-reviewed at head c6a17da (previous review was at 069aeee). All three round-1 findings are resolved and each carries a regression test: the eager-path deny now skips acquisition instead of raising a graph-level error, provider-resolution failures follow fail_closed/fail_open inside authorize_sandbox_execution, and the user is None branch is documented in both routers. The delta looks correct - I verified the uploads/artifacts deny paths leave sandbox = None so the sync loop and the release/rollback branches are safely skipped, and the middleware return None is equivalent to the base-class no-op. Two small nits below: a stale (now unreachable) fallback layer in authorize_sandbox_for_request, and a missing deny-path regression test for the artifacts router's copy of the gate.
| ) | ||
| except SandboxAuthorizationError: | ||
| raise | ||
| except Exception: |
There was a problem hiding this comment.
Nit: after the fix in authorize_sandbox_execution (this push moved provider resolution inside its try), this except Exception fallback is unreachable in practice - resolution and authorize() errors are both converted to SandboxAuthorizationError (or swallowed under fail-open) one layer down, and the context built by _route_authz_context has no authz_attributes, so build_principal_from_context can't raise here either. The comment ("Provider resolution failures ... must not 500 the route") is now stale since those never reach this layer. If you keep it as defense-in-depth, worth rewording the comment; also note this fallback consults config.fail_closed from _get_route_authorization_config() while the helper uses the route-injected app_config's authorization config - two config sources in one gate, which would diverge if the injected config ever differs from the global one.
| # above (401), and auth-disabled mode stamps a user via | ||
| # AuthMiddleware. Same reasoning as the uploads router. | ||
| if user is not None: | ||
| authorize_sandbox_for_request(user, is_internal=_is_internal_caller(request, user), app_config=get_config()) |
There was a problem hiding this comment.
Nit: this copy of the deny-skips-sync gate has no regression test - test_sandbox_authorization.py covers the uploads router's deny/allow paths (test_upload_sandbox_sync_skipped_when_denied / ..._proceeds_when_allowed) but nothing asserts the artifacts route skips acquire_async (and still completes the host-side update) for a denied role. Since the PR's mutation-testing claim covers only the tools.py gate, a reverted gate here would pass the suite unnoticed. The _make_upload_app harness could be reused with the artifacts router for a two-line deny test.
|
@hata33 Please fix the unit test and backend IO test errors. |
c6a17da to
c17f85d
Compare
github-actions
Bot
added
the
area:agents
|
@WillemJiang CI failures fixed in CI CI Nit 1 (stale wrapper fallback): the Nit 2 (artifacts deny coverage): added Verified locally: 21 sandbox-authz tests, the full affected set including |
willem-bd
left a comment
willem-bd
left a comment
There was a problem hiding this comment.
Round 3 at c17f85d — all five prior findings are verified fixed at this head (resolution moved inside the try, eager-path deny now skips acquisition instead of raising, artifacts deny regression test added, request=None guard, defense-in-depth comment reworded). The gate placement, reuse-path skip, uploads/artifacts deny-skips-sync behavior, and fail-closed/open handling all check out against the head files. Two new suggestion-level findings, both documentation-adjacent: (1) the guest example role allows read_file while denying sandbox:execute, but read_file is itself a sandbox-dependent tool (it unconditionally calls ensure_sandbox_initialized), so the advertised "read-only role" permits a tool that can only ever return the deny error; (2) the AGENTS.md addition documents the previous phase's model authorization while this PR's own sandbox gate gets no module-guide coverage. Code itself looks good from my side.
| # tools: {allow: ["web_search", "read_file"]} | ||
| # routes: {allow: ["threads:read", "runs:read"]} | ||
| # models: {allow: ["gpt-4o-mini"]} | ||
| # sandbox: {allow: false} # deny sandbox execution (read-only role) |
There was a problem hiding this comment.
This example role combination is self-contradicting: guest allows read_file at the tool level while sandbox: {allow: false} denies the sandbox — but read_file is itself a sandbox-dependent tool (read_file_tool → _read_file_from_sandbox → ensure_sandbox_initialized, unconditionally, verified at this head). With this policy, every guest read_file call hits the new gate and returns "Error: Sandbox execution is not permitted for your role". So the "read-only role" comment doesn't hold: the one read tool it allows can never succeed, and the role is effectively web_search-only. Suggest either dropping read_file from the guest tools allow list, or rewording the comment to make explicit that read_file/glob/grep all require sandbox:execute and will error for this role (operators copying this example otherwise ship a tool allow-list whose entries always fail).
|
|
||
| Gateway route authorization uses `authz.py::resolve_route_permissions()` as the single provider integration point for both `AuthMiddleware` and decorator-only authentication. When enabled, it evaluates the six registered `threads:*` / `runs:*` permissions as `resource="route"` requests whose targets are the full `resource:action` strings. Decisions use the async provider API and are cached for the request in `AuthContext`; decorators do not call the provider again. Provider resolution or decision errors follow `authorization.fail_closed`, scoped per permission for decision errors. When authorization is disabled, the legacy complete permission set is returned without resolving a provider. Existing `owner_check` enforcement and `require_admin_user()` management gates remain independent and unchanged. Tests: `tests/test_authorization_route_permissions.py`, `tests/test_auth.py`, and `tests/test_auth_middleware.py`. | ||
|
|
||
| Model authorization uses `authz.py::resolve_model_authorization()` (same cached-provider, internal-role, and principal-building path as route authorization) as the Gateway integration point for the `models` router: `list_models` filters names through `filter_resources(principal, "model", ...)`, and `get_model` enforces `authorize(resource="model", action="use")` with a deny surfacing as 403; provider errors follow `authorization.fail_closed` (fail-open returns the unfiltered list / proceeds). At runtime, `lead_agent/agent.py::_authorize_model_name` — called from `_make_lead_agent` and from `DeerFlowClient._ensure_agent` — applies the same `model:use` check to the resolved model name. On deny it scans the `filter_resources`-visible names (excluding the denied model), re-verifying each candidate with `authorize("model", "use")` before falling back, because a custom provider may allow `list` while denying `use`; no usable fallback raises under `fail_closed` and keeps the original model under fail-open. The built-in RBAC provider maps this to the per-role `models` policy key. Tests: `tests/test_models_authorization.py`. |
There was a problem hiding this comment.
Doc-sync gap: this added paragraph backfills the models phase (#4540) documentation, but this PR's own change — the sandbox:execute gate — is documented in neither module guide. sandbox/AGENTS.md (which owns acquisition lifecycle, fail-closed semantics, and tools.py behavior in detail) is untouched by this PR, and no sandbox paragraph lands here next to the models one, even though SandboxMiddleware.before_agent/abefore_agent are modified in this diff. Per the repo's documentation-update policy ("update the relevant AGENTS.md for development/architecture changes in the same change set"), consider adding a short paragraph covering: gate at ensure_sandbox_initialized + the eager-path skip-defer-to-lazy-gate decision, SandboxAuthorizationError surfacing as a ToolMessage via the existing except SandboxError handling in the tools, the uploads/artifacts deny-skips-sync behavior, and the deferred feishu/dingtalk paths — otherwise the next agent touching the sandbox module has to rediscover the gate from the implementation notes.
c17f85d to
5ca1b2e
Compare
|
Both documentation suggestions addressed in Guest role example — fixed. Module-guide coverage — added. The |
willem-bd
left a comment
willem-bd
left a comment
There was a problem hiding this comment.
Round 4 at 5ca1b2e (squashed head; re-verified the full diff plus surrounding code at this commit). All seven findings from rounds 1-3 are confirmed fixed here: the eager-path deny now skips acquisition with a regression test, provider resolution moved inside the try with fail-closed/fail-open tests, the request=None/user=None guards and the defense-in-depth comment are in place, the artifacts deny path has coverage, the guest example no longer allows sandbox-dependent tools alongside sandbox: {allow: false}, and both module guides document the gate. I also re-verified the completeness claim at this head: every provider.acquire/acquire_async call site outside tests is either gated (tools.py, middleware eager path, uploads, artifacts) or is the documented feishu/dingtalk deferral; the RBAC provider maps the sandbox key and handles "*"/bool/list allow forms as described; and the friendly-ToolMessage claim holds because the tool bodies and _run_sync_tool_after_async_sandbox_init convert SandboxError to an error string. Three remaining items, all suggestion/nit level: deny-path regression coverage for the async gate copies, the duplicated router gate blocks, and the app_config type annotation.
|
|
||
| # Phase 3: enforce sandbox:execute authorization before acquiring (async | ||
| # counterpart of the sync gate in ``ensure_sandbox_initialized``). | ||
| authorize_sandbox_execution( |
There was a problem hiding this comment.
Suggestion: the async copies of the gate have no deny-path regression coverage. test_ensure_sandbox_initialized_denies_on_authz_reject and test_eager_before_agent_deny_skips_acquisition cover only the sync ensure_sandbox_initialized / before_agent, while the async runtime paths the agent actually executes go through ensure_sandbox_initialized_async (here) and abefore_agent (middleware.py). These are verbatim copies of the gated sync code, so reverting or deleting either async gate would leave the whole suite green — which undercuts the PR's mutation-testing claim. One deny test each (mirroring the sync ones, asserting acquire_async is never called) would close that gap.
| # semantics as the models routes' anonymous bypass. | ||
| user = await get_optional_user_from_request(request) if request is not None else None | ||
| if user is not None: | ||
| authorize_sandbox_for_request(user, is_internal=_is_internal_caller(request, user), app_config=config) |
There was a problem hiding this comment.
Suggestion: uploads.py and artifacts.py now carry near-identical ~15-line gate blocks (optional-user lookup → authorize_sandbox_for_request → except-log → else acquire) plus duplicated rationale comments. The PR's own design principle for the harness side was to gate at a single acquisition entry point precisely so N copies can't drift; the same applies here. Consider hoisting a shared helper next to authorize_sandbox_for_request — e.g. an async try_acquire_sandbox_for_request(request, provider, thread_id, user_id, *, app_config) -> Sandbox | None that returns None on deny — so both routers call one function and the deny/skip semantics (including the log line) live in one place.
| _SANDBOX_TARGET = "*" | ||
|
|
||
|
|
||
| def authorize_sandbox_execution(*, context: Mapping[str, Any], app_config: AppConfig) -> None: |
There was a problem hiding this comment.
Nit: the annotation says app_config: AppConfig, but every harness caller passes safe_app_config(), which returns None whenever the config is unreadable — and test_authorize_sandbox_no_config_file_is_noop passes app_config=None explicitly. safe_app_config itself has no return annotation either. Typing these as AppConfig | None would document the "None is treated as disabled" contract at the signature instead of only in the docstring.
5ca1b2e to
e426dd1
Compare
|
All three round-4 suggestions addressed in Async gate coverage — added. Duplicated router gate blocks — hoisted. New shared Type annotations — corrected. Also synced during re-review: the sandbox AGENTS.md guide and implementation notes now name the shared helper, and the README's RBAC policy list includes |
willem-bd
left a comment
willem-bd
left a comment
There was a problem hiding this comment.
Round 5 at e426dd1 (squashed head; re-verified the full diff plus the surrounding code at this commit). All findings from rounds 1-4 are confirmed fixed here: the duplicated uploads/artifacts gate blocks are replaced by the shared try_acquire_sandbox_for_request (artifacts still passes sandbox_id through to the finally release, uploads guards the sync loop on sandbox is not None), the async deny paths now have regressions (test_ensure_sandbox_initialized_async_denies_on_authz_reject, test_abefore_agent_deny_skips_acquisition), AppConfig | None annotations are in place at both the gate and safe_app_config, the guest example allows only web_search alongside sandbox: {allow: false}, and both the sandbox and middlewares module guides document the gate. I also re-checked the new helper's edge cases at this head: _is_internal_caller exists and is getattr-guarded, the middleware gate sits after the lazy_init skip and inside the no-existing-sandbox branch, and the sync authorize() call from the async routes matches the established models-route pattern (the provider Protocol requires both sync and async methods). One new nit below — the implementation notes' evidence section under-counts the test file.
| （直接返回）。RBAC provider 的 `_RESOURCE_POLICY_KEYS` 已包含 `"sandbox": "sandbox"`， | ||
| `provider.py` 已声明 `"sandbox"` 为有效 resource，无需 schema 变更。对 test mock | ||
| （SimpleNamespace app_config）安全：使用 `getattr` + `is not True` 防御。 | ||
| - **证据：** `tests/test_sandbox_authorization.py`（14 tests）覆盖 disabled/RBAC allow/deny/ |
There was a problem hiding this comment.
Nit: this evidence section says tests/test_sandbox_authorization.py（14 tests）, but at this head the file contains 23 tests — the enumeration predates the round 1-4 regressions and now under-reports the coverage: eager-path deny-skip for both before_agent and abefore_agent, provider-resolution error under fail-open/fail-closed, ensure_sandbox_initialized_async deny, the artifacts-router deny-skips-sync path, request=None tolerance, the no-config no-op, and the mock-app_config guard are all tested but unlisted. Since the notes are the cumulative handoff record for reviewer feedback and required regression coverage, updating the count and coverage list here keeps the Phase 3 → Phase 4 handoff accurate.
e426dd1 to
3aa6fb7
Compare
|
Fixed in |
|
@hata33 please fix the unit test errors. |
…tion (bytedance#4063 Phase 3) Sandbox is an execution environment, not a named resource: multiple tools (bash, read_file, write_file, glob, grep, ...) depend on it, all funneled through ensure_sandbox_initialized / ensure_sandbox_initialized_async. Gate the single acquisition entry point (single source of truth) instead of maintaining a sandbox-tool-name set in middleware: - authorize_sandbox_execution helper (authz/sandbox_authz.py) checks authorize("sandbox", "execute", target="*") — a binary judgment (can this role use the sandbox at all); RBAC allow:"*"/true permits, allow:[]/false denies. - lazy path: ensure_sandbox_initialized (+ async) calls the gate before provider.acquire. - eager path: SandboxMiddleware.before_agent / abefore_agent call the gate before _acquire_sandbox. - deny raises SandboxAuthorizationError (SandboxError subclass) which propagates through tool execution as a friendly ToolMessage (RFC §9: 'not a crash'). - authorization.enabled: false is a no-op everywhere; provider errors follow fail_closed (deny) / fail_open (allow). 12 tests in tests/test_sandbox_authorization.py cover disabled/allow/deny/ deny-via-bool/no-policy-unrestricted/provider-error-fail-closed/open/ internal-caller + ensure_sandbox_initialized deny (never acquires) and allow (acquires) integration paths.
3aa6fb7 to
8841ff6
Compare
|
@WillemJiang fixed in What failed: the three Fix: the artifacts router now passes |
3 participants
Part of #4063
Why
Phase 3 Models (#4540) and Skills (#4541) connected the
AuthorizationProviderto model and skill access, but sandbox execution remained gated only by config presence (feat.sandbox is not False) — any authenticated user got full sandbox execution (bash, file I/O). This PR addssandbox:executeenforcement so an RBAC policy likesandbox: {allow: false}actually takes effect. It is the last of three Phase 3 resource-type PRs (Models → Skills → Sandbox).Sandbox differs from models/skills: it is an execution environment, not a named catalog — multiple tools (
bash,read_file,write_file,glob,grep, …) depend on it. Instead of maintaining a sandbox-tool-name set in middleware (which would need updating every time a sandbox tool is added), the gate lives at the single sandbox-acquisition entry point, so it cannot be bypassed regardless of which tool triggers it.What changed
Gate helper —
authorize_sandbox_execution()(authz/sandbox_authz.py):authorize("sandbox", "execute", target="*")— a binary judgment ("can this role use the sandbox at all"). RBACallow: "*"/truepermits,allow: []/falsedenies; no policy for a role → unrestricted (consistent with every other resource type).authorization.enabledis false; provider decision errors followfail_closed(deny) /fail_open(allow); mock-safegetattrguards mirrorfilter_available_skills_by_authorization.Deny behavior —
SandboxAuthorizationError(SandboxError)(sandbox/exceptions.py):role; propagates through tool execution and surfaces as a friendly errorToolMessage("sandbox execution is not permitted for your role"), not a crash (RFC §9).Gates at every sandbox-acquisition entry point (verified by grepping all
provider.acquire/acquire_asynccall sites):ensure_sandbox_initialized+ async (lazy path — all sandbox tools)authorize_sandbox_executionbeforeprovider.acquireSandboxMiddleware.before_agent/abefore_agent(eager path,lazy_init=False)_acquire_sandboxuploadsrouter (upload → sandbox sync)authorize_sandbox_for_request(); deny skips the sync — the upload itself still succeeds (a sandbox-denied agent cannot consume the files anyway)artifactsrouter (artifact edit → sandbox sync)authorize_sandbox_for_request()(gateway/authz.py) builds the Principal fromrequest.state.uservia a new shared_route_authz_contexthelper (also adopted byresolve_model_authorization), including theINTERNAL_SYSTEM_ROLE → Nonepop. Provider-resolution failures degrade perfail_closedinstead of 500-ing the route.authorization.enabled: falseis a complete no-op on every gate. The RBAC provider already maps"sandbox" → "sandbox"(_RESOURCE_POLICY_KEYS), so no schema change is needed.Authorization RFC continuity confirmation
docs/plans/2026-07-10-pluggable-authorization-rfc.md.docs/plans/2026-07-10-pluggable-authorization-implementation-notes.md.target="*"semantics, auxiliary-path decisions, deferred channel paths).Surface area
fail_closedinstead of 500.ensure_sandbox_initializedgates all sandbox tools at runtime.SandboxMiddlewareeager path + acquisition entry gated.authorization.enabled: true(opt-in). Defaultfalse= identical to today.sandbox:policy examples), implementation notes updated.Validation
Mutation-tested: reverting the
tools.pygate or the defensive guards makes the corresponding regression tests fail.Related