feat(authz): enforce skill authorization at assembly and slash-activation (#4063 Phase 3)

[hata33]

Part of #4063

Why

Phase 2A (#4439) connected the AuthorizationProvider to Gateway HTTP route permissions, but skill access remained controlled only by the agent-config allowlist (config.skills) — no role policy could deny a skill. This PR adds skill:activate enforcement so RBAC policies like skills: {allow: ["data-analysis"]} actually take effect. It is the second of three Phase 3 resource-type PRs (Models → Skills → Sandbox).

What changed

Layer 1 (assembly-time, mirrors Phase 1B):

• filter_available_skills_by_authorization() in skill_filter.py — filters the skill-name allowlist (set[str] | None) by the provider's "skill" policy. In the lead agent, runs after _available_skill_names() so the catalog (describe_skill) and SkillActivationMiddleware share one filtered set. In the subagent executor _load_skills(), filters config.skills before any disk I/O.
• filter_resources_by_authorization() in enforcement.py — generic version of filter_tools_by_authorization for any resource with a name attribute (skills, models, etc.).

Layer 2 (slash-activation):

• No new middleware code needed. SkillActivationMiddleware._resolve_activation already checks reference.name not in self._available_skills. Since Layer 1 filters the allowlist at assembly time, denied skills cannot be slash-activated — the existing check enforces the policy for free.

authorization.enabled: false is a complete no-op. available_skills=None (no agent-level allowlist) + enabled authorization resolves all skill names from config and filters them. The RBAC provider already maps "skill" → "skills" (_RESOURCE_POLICY_KEYS), so no schema change is needed. Test-mock safe via getattr + is not True guards (same pattern as Phase 1B).

Authorization RFC continuity confirmation

• Read docs/plans/2026-07-10-pluggable-authorization-rfc.md.
• Read docs/plans/2026-07-10-pluggable-authorization-implementation-notes.md.
• Checked all prior-phase decisions and deferred items.
• Updated the implementation notes with this PR's decisions and deferred items.

Surface area

• Frontend UI
• Backend API
• Agents / LangGraph — lead agent and subagent skill loading now filtered by role.
• Sandbox
• Skills — denied skills are invisible (catalog) and unactivatable (slash).
• Dependencies
• Default behavior change — only when authorization.enabled: true (opt-in). Default false = all skills available, identical to today.
• Docs / tests / CI only — config.example.yaml, implementation notes updated.

Validation

# Skill authorization + authz full regression
cd backend && PYTHONPATH=. uv run pytest \
  tests/test_skills_authorization.py \
  tests/test_authorization_route_permissions.py \
  tests/test_authorization_tool_filter.py \
  tests/test_authorization_enforcement.py \
  tests/test_rbac_authorization_provider.py \
  tests/test_authorization_runtime.py \
  tests/test_authorization_principal.py \
  tests/test_lead_agent_skills.py \
  tests/test_lead_agent_model_resolution.py \
  tests/test_skill_describe.py \
  -q
# -> 255 passed

# Subagent executor (the two skill-loading tests)
cd backend && uv run pytest \
  "tests/test_subagent_executor.py::TestAgentConstruction::test_load_skill_messages_uses_explicit_app_config_for_skill_storage" \
  "tests/test_subagent_executor.py::TestAgentConstruction::test_load_skills_uses_each_subagent_users_scoped_storage" \
  -v
# -> 2 passed

# Lint + format
cd backend && uv run ruff check . && uv run ruff format --check .

Related

• RFC: [RFC] Pluggable fine-grained authorization system #4063
• Phase 0: feat(authz): add pluggable AuthorizationProvider protocol and config scaffolding (Phase 0) #4127 | Phase 1A-1: feat(authz): propagate trusted authorization principal context #4203 | Phase 1A-2: feat(authz): add built-in RBAC provider and provider factory #4260 | Phase 1B: feat(authz): enforce tool authorization at assembly and runtime #4370 | Phase 2A: feat(authz): derive Gateway route permissions from AuthorizationProvider #4439
• Parallel Phase 3 PR (Models): feat(authz): enforce model authorization at Gateway routes and runtime (#4063 Phase 3) #4540

[willem-bd]

Reviewed Phase 3 skill authorization. Layer 1 filtering is wired into both assembly paths (lead agent + subagent _load_skills) and the Layer 2 reuse of _available_skills for slash-activation gating is the right call.

One concrete fail-open bypass worth addressing: when available_skills=None (no agent-level allowlist) and _all_configured_skill_names raises (any storage I/O error is swallowed at line 99), filter_available_skills_by_authorization returns None. That None then propagates to SkillActivationMiddleware (which skips its reference.name not in self._available_skills check when the set is None) and to the subagent _load_skills (else: return all_skills), so every installed skill becomes activatable/loadable even with fail_closed=true. The fail-closed guarantee is enforced for provider errors inside filter_resources but not for candidate-set resolution errors.

Minor: apply_skill_authorization is re-exported in authz/__init__.py __all__ and exercised by tests, but is not called from any production path in this PR (the integrations use filter_available_skills_by_authorization directly).

[willem-bd]

Fail-open bypass when candidate-set resolution errors. When available_skills=None (no agent-level allowlist) and authorization is enabled with fail_closed=True, an exception in _all_configured_skill_names is swallowed at line 99 (except Exception: return []), so this early-exit returns available_skills unchanged — i.e. None. That None then propagates:

• Lead agent: SkillActivationMiddleware is constructed with available_skills=None, so its gate if self._available_skills is not None and reference.name not in self._available_skills (skill_activation_middleware.py:151) is skipped — every installed skill can be slash-activated.
• Subagent: _load_skills hits if allowed is not None: ... else: return all_skills (executor.py:619-621) and returns the full unfiltered set.

This contradicts the fail-closed contract the sibling filter_resources_by_authorization enforces (its docstring: "provider errors and malformed filter results deny every resource when fail_closed is true"). Provider exceptions raised inside filter_resources are handled correctly at line 78, but candidate-set resolution errors bypass it. Consider returning set() here when authz_config.fail_closed is true (or propagating the resolution error instead of swallowing it at line 99).

[WillemJiang]

@hata33 please take a look at the recent review comments.

[hata33]

Thanks @willem-bd — addressed both points. Rebased onto latest main (post #4540 merge) and pushed in c8237f89.

Fail-open bypass — fixed. The root cause was _all_configured_skill_names swallowing storage/I/O errors and returning [], which then made if not candidates: return available_skills return None (the bypass). Fixed by making _all_configured_skill_names raise on resolution errors instead of swallowing them, and catching that in filter_available_skills_by_authorization so it follows the same fail-closed/fail-open branch as a provider error:

• fail-closed → set() (deny all skills), not None
• fail-open → None (unrestricted), matching the existing provider-error fail-open behavior

The if not candidates: return available_skills path now only fires when resolution succeeds and returns a genuinely empty set (the user configured no skills) — returning None there is safe because there is nothing for SkillActivationMiddleware or subagent _load_skills to activate/load.

Added 3 regression tests:

• test_filter_available_skills_candidate_resolution_error_fail_closed — available_skills=None + _all_configured_skill_names raises + fail_closed=true → set() (was the bypass → None)
• test_filter_available_skills_candidate_resolution_error_fail_open — same error + fail_closed=false → None
• test_filter_available_skills_none_with_empty_config_no_bypass — available_skills=None + resolution succeeds with empty list → None (no bypass, nothing to load)

Mutation-tested: reverting only the skill_filter.py fix (keeping the new tests) makes both fail-closed/fail-open resolution-error tests fail. 15 tests total.

apply_skill_authorization — kept as a public export. Agreed it has no production caller in this PR (the integrations use filter_available_skills_by_authorization directly because it operates on the name set before disk I/O). I kept it because it mirrors apply_tool_authorization (same signature shape: (items, *, context, app_config, authorization_provider=None) -> (filtered, provider)) and is part of the authz public API exported from __init__.py. If you'd prefer it removed to avoid a dead-code surface, I'm happy to drop it — let me know.

[willem-bd]

Follow-up at HEAD c8237f89. The fail-open bypass I flagged earlier at skill_filter.py:83 is properly resolved in this revision — _all_configured_skill_names now raises and the caller returns set() under fail_closed (lines 99-101), with explicit regression tests (test_filter_available_skills_candidate_resolution_error_fail_closed/open). One new gap below: the embedded DeerFlowClient path is missing the Layer 1 skill filter, even though PR #4540 wired model authz into the same function for the same bypass reason.

[willem-bd]

Embedded DeerFlowClient path is missing the equivalent filter. DeerFlowClient._ensure_agent (not in this PR's diff) applies model authz (_authorize_model_name at client.py:302) and tool authz (apply_tool_authorization at client.py:327), but self._available_skills — set directly from the constructor's available_skills argument — is passed unfiltered to build_middlewares(..., available_skills=self._available_skills, ...) (client.py:357) and to the skills_list intersection (client.py:312-313), without ever going through filter_available_skills_by_authorization. The model-authz comment at client.py:293-299 calls out this exact bypass pattern ("the role-scoped model policy cannot be bypassed by constructing the agent through DeerFlowClient") — the same risk applies to skills but isn't wired up here.

With authorization.enabled: true, a caller building the agent via DeerFlowClient(available_skills={"denied-skill"}) (or available_skills=None for "all skills") would bypass the role's skills policy: SkillActivationMiddleware (constructed with the unfiltered set) would permit slash-activation of any skill in the set, and the catalog/prompt would advertise skills the role is denied. Mirroring this line in _ensure_agent — e.g. self._available_skills = filter_available_skills_by_authorization(self._available_skills, context=cfg, app_config=self._app_config) right after the _authorize_model_name call — closes it and keeps the two assembly paths symmetric.

[hata33]

Good catch — this is the exact symmetric gap to the model-authz follow-up in #4540. Pushed in 12db2fb9.

Embedded path — wired. _ensure_agent now calls filter_available_skills_by_authorization(self._available_skills, context=cfg, app_config=self._app_config) right after the _authorize_model_name call (client.py:310-316), mirroring _make_lead_agent (agent.py:675). The filtered result is a local available_skills that feeds skills_list intersection, build_middlewares, and apply_prompt_template — so SkillActivationMiddleware and the catalog/prompt all see the authorization-filtered set. self._available_skills is left untouched (it still feeds the cache key at line 282, so a different role still triggers a rebuild).

Added 2 regression tests in test_skills_authorization.py:

• test_client_ensure_agent_filters_skills_by_authorization — real-path: a genuine RBAC provider denies denied-skill for the user role; asserts only allowed-skill reaches build_middlewares.
• test_client_ensure_agent_noop_when_authorization_disabled — disabled leaves the skill set unchanged.

Mutation-tested: reverting only the client.py filter (keeping the new tests) makes the filter test fail ({"denied-skill", "allowed-skill"} != {"allowed-skill"}). 17 skills tests + 15 TestEnsureAgent tests pass.

On the route-layer symmetry (preempting the question): Skills intentionally have no Gateway route-level authorization (decision log, implementation-notes.md: "技能没有 Gateway route... 不为技能新增独立 Gateway route, per §12 Q6"). list_skills / get_skill return metadata only; the sensitive actions — slash-activation and loading SKILL.md content — are enforced at the harness layer (Layer 1 assembly filter + Layer 2 SkillActivationMiddleware reuse of the filtered set). Management endpoints (install/uninstall/custom) stay require_admin_user. Clarified the config.example.yaml roles comment to state models/skills govern runtime use and assembly, not the management-API listing surface.

[willem-bd]

Per-user custom skills are silently dropped when available_skills=None and authz is enabled. _all_configured_skill_names resolves the candidate universe via get_or_new_skill_storage (process-global storage), but per-user custom skills live in UserScopedSkillStorage under {base}/users/{user_id}/skills/custom/ (see storage/__init__.py). So when the agent has no allowlist and authorization.enabled=true, filter_available_skills_by_authorization returns a non-None set of global names only; the provider never sees per-user custom skill names as candidates — even a skills: {allow: "*"} wildcard cannot reach them because they were never in candidates. SkillActivationMiddleware._resolve_activation then blocks them at line 151 (reference.name not in self._available_skills): the middleware resolves skills from user-scoped storage (_storage() -> get_or_new_user_skill_storage(self._user_id, ...) at line 112) but checks membership against the global-derived set, so a per-user custom skill that loads fine is rejected with "not available for this agent." Pre-PR, available_skills=None reached the middleware as None and the line-151 check was skipped, so those skills were activatable. Same gap in subagents/executor.py::_load_skills, where this is also redundant I/O — all_skills is already loaded from user-scoped storage a few lines above the filter call, yet the filter re-loads from global storage to build candidates. Net effect: enabling authorization disables per-user custom skill slash-activation (and subagent loading) regardless of role policy. Consider threading user_id into the resolver (or, in the executor, deriving candidates from the already-loaded all_skills) so the provider filters the same universe the activation path loads.

[hata33]

Excellent catch — this was a real correctness bug, not just a scoping nit. Pushed in 15d462ba.

Root cause confirmed. _all_configured_skill_names used get_or_new_skill_storage (process-global), while the activation path (SkillActivationMiddleware._storage() → get_or_new_user_skill_storage(self._user_id)) and the subagent _load_skills both load from UserScopedSkillStorage. So when available_skills=None + authorization.enabled=true, the candidate universe was global-only — per-user custom skills were never presented to the provider, and even a skills:{allow:"*"} wildcard couldn't reach them. They'd then be blocked at SkillActivationMiddleware line 151 (reference.name not in self._available_skills), disabling per-user custom skill activation entirely once authz was on.

Fix. Threaded user_id through the resolution chain so the candidate universe matches the activation universe:

• filter_available_skills_by_authorization now accepts an optional user_id parameter.
• _all_configured_skill_names(app_config, *, user_id=None) uses get_or_new_user_skill_storage(user_id, app_config=...) when user_id is provided, falling back to get_or_new_skill_storage otherwise.
• All three call sites forward user_id:
  • _make_lead_agent (agent.py) → resolved_user_id
  • DeerFlowClient._ensure_agent (client.py) → cfg.get("user_id")
  • subagent _load_skills (executor.py) → self.user_id

This also removes the redundant I/O in the executor path you noted: _load_skills already loaded all_skills from user-scoped storage above the filter call, but the filter was re-loading from global storage to build candidates — now both use the same user-scoped storage.

Added 2 regression tests:

• test_filter_available_skills_user_id_threads_into_candidate_resolution — asserts user_id is forwarded to the resolver and a per-user custom skill (user-custom-skill) appears in the filtered set under a wildcard policy.
• test_filter_available_skills_omits_user_id_when_not_provided — asserts user_id=None is passed when the caller omits it (global storage fallback).

Mutation-tested: reverting only skill_filter.py makes the threading test fail (got an unexpected keyword argument 'user_id'). 19 skills tests + 15 TestEnsureAgent + 79 subagent executor tests pass (113 total).

[willem-bd]

Round 3 review at cb96d77e. All three prior findings are properly resolved in this revision:

1. Fail-open bypass on candidate-set resolution errors — fixed (_all_configured_skill_names now raises; caller returns set() under fail_closed).
2. Embedded DeerFlowClient._ensure_agent path — wired up with filter_available_skills_by_authorization.
3. Per-user custom skills dropped — user_id is now threaded into _all_configured_skill_names so UserScopedSkillStorage is the candidate universe.

Two new suggestion-level findings below (no blockers).

[willem-bd]

apply_skill_authorization skips the mock-safe guard its sibling has — and has no callers or tests in this PR. Line 157 accesses app_config.authorization directly, while filter_available_skills_by_authorization (line 56) deliberately uses getattr(app_config, "authorization", None) + getattr(authz_config, "enabled", None) is not True so SimpleNamespace/Mock app_config objects in tests don't raise AttributeError (the PR description calls this pattern out as intentional: "Test-mock safe via getattr + is not True guards"). This function doesn't follow it.

It's also exported in authz/__init__.py __all__ but none of the three integration sites in this PR call it — lead agent, DeerFlowClient._ensure_agent, and subagent _load_skills all use filter_available_skills_by_authorization directly — and test_skills_authorization.py has no test for it (every test targets filter_available_skills_by_authorization or filter_resources_by_authorization). If it's intended as a public API for a later phase, adding the getattr guard + a test would keep it consistent with its sibling; if not, dropping it until a caller exists avoids shipping untested surface area (it's easy to assume it's covered because it sits next to the heavily-tested filter_available_skills_by_authorization).

[willem-bd]

Skills are loaded from disk twice when config.skills is None and authz is enabled. Line 626 already calls storage.load_skills(enabled_only=True) from UserScopedSkillStorage into all_skills. This call then enters the available_skills is None branch in filter_available_skills_by_authorization (skill_filter.py:94), which calls _all_configured_skill_names(user_id=self.user_id) — and that resolves the same UserScopedSkillStorage and calls storage.load_skills(enabled_only=True) again. Same storage instance, same flag, same user.

The executor is the one call site that already holds the full loaded list in all_skills, so it could pass [s.name for s in all_skills] as pre-resolved candidates (or filter_available_skills_by_authorization could accept a candidate-set override) and skip the second load_skills. The lead-agent and DeerFlowClient paths don't have this issue because they operate on a name set from the start.

Side note: the comment at line 644 ("so denied skills are never loaded from disk") slightly overstates the effect — by the time the filter runs, all_skills is already loaded; the filter only prunes the returned list, it doesn't avoid the disk read. Consider rewording to match what actually happens (e.g. "so denied skills never reach the returned list / are never turned into tools").

[hata33]

Both suggestions addressed in 593aeeca.

apply_skill_authorization — dropped. Agreed it was untested surface area with an inconsistent mock-safe guard and zero callers (all three integration sites use filter_available_skills_by_authorization directly). Removed the function, its __init__.py export, and the now-unused Sequence / filter_resources_by_authorization imports. If a Skill-object-level API is needed later, it can be added with the same getattr guard + tests at that point.

Executor double load_skills — fixed. Added a candidate_skill_names parameter to filter_available_skills_by_authorization; the executor passes [s.name for s in all_skills] (already loaded at line 626), so the filter skips _all_configured_skill_names and its second load_skills round-trip. The lead-agent and DeerFlowClient paths don't pass it (they operate on name sets from the start, so no redundant I/O there). Also reworded the line-644 comment from "never loaded from disk" to "never reach the returned list (and are never turned into tools)" to match what actually happens.

[hata33]

@WillemJiang @willem-bd friendly ping — this PR is ready for review/merge:

• Rebased onto latest main (c542185a) and pushed in 3a1f4bdd; CI is all green (lint, backend-unit-tests, frontend, layer 1/2, blocking-io).
• All 4 rounds of @willem-bd's review comments are addressed (fail-open bypass, embedded DeerFlowClient path, per-user custom skills, double load_skills, dead apply_skill_authorization removed). @willem-bd confirmed "no blockers" in the last round.
• This is the second of three Phase 3 resource PRs (feat(authz): enforce model authorization at Gateway routes and runtime (#4063 Phase 3) #4540 Models merged; this one Skills; Sandbox to follow).

Would appreciate a review or approval when you have a moment. Happy to make any further changes if needed.

[zhfeng]

[P2] Preserve the agent skill allowlist when filtering provider results (backend/packages/harness/deerflow/authz/skill_filter.py:107)

filter_available_skills_by_authorization() returns set(allowed) directly. Although the provider contract says results must be a subset of candidates, this boundary only validates list[str]. A buggy custom provider can therefore return extra skill names, expanding an explicit agent-level allowlist and making those skills discoverable or slash-activatable.

The generic filter_resources_by_authorization() is defensive because it filters the original resources. This helper should likewise intersect the provider result with candidates, preserving the original candidate boundary, for example:

allowed_names = set(allowed)
return {name for name in candidates if name in allowed_names}

Please add a regression test using a provider that returns ["allowed-skill", "injected-skill"] and verify that the injected name is excluded.

[hata33]

@zhfeng good point — fixed in 548b2cee.

filter_available_skills_by_authorization now intersects the provider result with the candidate set ({name for name in candidates if name in allowed_names}), matching the defensive pattern in filter_resources_by_authorization. A buggy custom provider can no longer inject names outside the candidate boundary.

Added test_filter_available_skills_provider_injected_names_excluded: a provider returning ["allowed-skill", "injected-skill"] against candidates {"allowed-skill"} → result is {"allowed-skill"} only. Mutation-tested: reverting the intersection makes the test fail (injected-skill leaks into the result).

[zhfeng]

LGTM

[hata33]

Rebased onto latest main (post #4911 / #4972 / #4984 / #5030) and force-pushed in ac463cf0.

Conflicts resolved (3 files, all from the sandbox PR #4911):

• authz/__init__.py — kept both new imports (authorize_sandbox_execution + filter_available_skills_by_authorization); __all__ already merged cleanly.
• config.example.yaml — merged the role examples so admin/user now document both sandbox and skills policies; guest gets sandbox: {allow: false} + skills: {allow: []}.
• docs/plans/...implementation-notes.md — kept both dated journal sections (Skills 2026-07-28, Sandbox 2026-08-02) in chronological order, and updated the Skills entry's deferral note to record that sandbox landed in feat(authz): enforce sandbox:execute authorization at sandbox acquisition (#4063 Phase 3) #4911.

Verification: test_skills_authorization + test_models_authorization (46) and test_client (170) all pass; a broad -k "authoriz or skill" run (1352 passed) matches the upstream-main baseline — the only local failures are pre-existing Windows-environment ones (symlink/PVC/docker-mount tests) present on base too, plus two flaky os.replace timing tests that pass on re-run. Ruff lint + format clean.

The dangling reference in sandbox_authz.py to skill_filter.py (introduced by #4911) now resolves once this lands. @WillemJiang this is the last remaining resource type from RFC #4063 Phase 3 — zhfeng already approved; a final look whenever you have a moment would be much appreciated.