RSPEED: harden /v1/workflows/run submissions (#51 Phase 0a)

[jameswnl]

Summary

Phase 0a of the #51 design plan. Closes G1, G2, G9, G10, G11 on POST /v1/workflows/run.

• Caller-sent credentials_secret (run provider or definition.provider) is now 400; the stack picks the credential.
• Sandbox image (request, workflow/step spawn_config, skills.image) must be the spawner default unless the caller is ADMIN (403).
• advisory: true and effective spawn none/local need ADMIN (403). Approval steps are ignored.
• Ephemeral steps with no spawner_configuration are 403.
• Byte cap (413) and step / MCP / secret_headers count caps (422).
• Order: 413, then shape 422, then 400/422 caps, then 403.
• New is_admin() in authorization/middleware.py: ADMIN is never in authorized_actions (get_actions expands it), so it asks the resolver using roles now stored on request.state.user_roles. Plan scheduled this for Phase 2; pulled forward because the ADMIN checks need it.
• New workflow/limits.py holds the definition size and count caps.

Breaking (accepted in D2): callers sending credentials_secret or a custom image now get 400/403.

Target state (feature-level TDD)

examples/workflows/ is the end state for #51 + cloud-agents#269: operator config, K8s secrets, two caller workflows, and verify.py (uv run python examples/workflows/verify.py) which pins the contract. Later phases make more of it real. Chat on cloud-agents (/query/direct) is out of scope (removal PR separate, tracked in #59).

Admin caveat: is_admin() uses the access resolver. With k8s, noop, noop-with-token or api-key auth the resolvers are no-ops, so every caller counts as admin and the admin-only 403s only apply under jwk-token or rh-identity with access rules. The example uses jwk-token.

Breaking

On POST /v1/workflows/run: a caller-sent credentials_secret (run provider or definition.provider) is now 400 for everyone; a custom sandbox image, advisory: true or spawn: none/local is now 403 without ADMIN; ephemeral steps without a spawner are 403; definitions over 256 KiB are 413. Under k8s/noop/api-key auth every caller counts as admin, so the 403s apply only with jwk-token or rh-identity plus access rules.

Test plan

• New test_submission_guard.py (per-rule, admin vs non-admin, caps) and endpoint tests incl. non-admin happy path
• is_admin tests against a real GenericAccessResolver
• uv run make test-unit: 3416 passed; format, ruff, schema regenerated
• Independent opus evaluator: PASS WITH NOTES; notes addressed
• Not run: e2e suite (noop auth gives admin, so not expected to break; JWK/rh-identity e2e would need ADMIN for none/local)

🤖 Generated with Claude Code

[beesarmy]

Reviewed the full diff. The PR is well-scoped, correctly closes G1/G2/G9/G10/G11 on POST /v1/workflows/run, and the implementation is defensive (pure checks, _as_dict coercion, fail-closed on missing roles). I traced each path against the vendored cloud-agents (APPROVED_INFERENCE_PROVIDERS, SkillsSpec, WorkflowStepSpec defaults, _validate_workflow_submission's shape order) and the orderings all line up with the design plan. A few observations, non-blocking:

Things I'd tighten before merge

1. reject_oversized_definition uses json.dumps(definition, default=str). default=str is dead code in practice — FastAPI has already coerced the request body to a JSON-clean dict[str, Any], and any value that needs default=str would have been a 422 long before reaching this point. Recommend removing it (json.dumps(definition) and let a TypeError surface as 500 if it ever happens). The byte cap is a safety net; it shouldn't be where we discover a non-serializable body.

2. is_admin silently returns False when request.state.user_roles is unset. That's the correct fail-closed behavior, but in production it'll be invisible if a future endpoint calls is_admin outside an @authorize-decorated handler. Worth a logger.debug ("is_admin called without resolved user_roles") so it's diagnosable, and a docstring note that is_admin is only valid after the auth check.

3. enforce_submission_hardening doesn't write an audit log for denials. Intentional (Phase 4 owns audit), but worth calling out in the docstring so reviewers don't expect it: "this gate raises HTTPException directly; audit logging lands in Phase 4." A logger.info with the denial reasons is harmless and gives an interim signal — optional.

Edge cases the design claims but the tests don't cover

4. provider={"name": "openai", "credentials_secret": null} — RunWorkflowRequest.provider is Optional[dict[str, Any]], so the caller can send null. Trace: "credentials_secret" in provider is True (key present, value None), so 400 fires. Good. Worth a parametrized test entry alongside the existing credentials_secret_rejected cases ("MY_CUSTOM_KEY", None, "") so the rejection isn't tied to the value being a non-empty string.

5. Definition with both definition.provider.credentials_secret AND malformed spec.steps. Today the 422 shape gate fires first (from _validate_workflow_submission), masking the credential issue. This is fine (both deny) but means the caller never learns the credential route was closed unless they fix the shape first. If that's the design intent, the rule is "shape errors take precedence over policy errors" and is worth one line in the docstring; if not, swap the order in the handler. My read is "shape first" is right.

6. _PRIVILEGED_SPAWN_MODES allows admins to run none/local even with no spawner and even though the plan says these stay admin-only until lightspeed-core#269. Test test_none_without_spawner_fine_for_admin documents this. Worth adding one test that confirms a non-admin with spawn: "none" and spawner_configured=False is still 403 — i.e., the admin gate is the only bypass, not the absence of a spawner.

Handler order note

The new pipeline is reject_oversized_definition → _validate_workflow_submission → enforce_submission_hardening. That's 413 → 422 shape → 400/422 caps/403. Matches the plan. One observation: _validate_workflow_submission is still called with provider containing the stack-injected credentials_secret, while enforce_submission_hardening is called with caller_provider (pre-injection). That's correct — they test different things — but the dual dict is easy to miss. A one-line note above the call site would help: "caller_provider keeps the caller's keys for the credential-rejection check; provider carries the stack's injected reference for the shape check."

Risk register for Phase 0a

• The credential-rejection at 400 is unconditional (even admins). Design said "D2 accepts the break," but worth a CHANGELOG/release note since this is a behavior change callers can hit.
• Admins retain the right to set sandbox_image and advisory: true and spawn: none/local. There's no audit log for these (Phase 4). For deployments where the ADMIN role is broadly granted, this is a noticeable gap until Phase 4 lands.
• /query/direct is untouched (G5 still open, plus the G7 secret-headers-on-none question from the design review). Title says "Phase 0a" — confirm Phase 0b is the next PR so the half-open paths aren't left dangling.

What I'd actually block on

Nothing. The PR correctly implements Phase 0a as planned and the test surface is solid (per-rule, admin vs non-admin, boundary on caps, approval-step carve-out, non_admin_ephemeral_workflow_accepted happy path). The mutate parametrize over image sources (workflow spawn_config / step spawn_config / skills image) is a particularly nice touch — covers all three G10 vectors in one test. Approving.

Verification I did:

• All spawn_modes and _effective_spawn_modes traces against WorkflowStepSpec.type = "agent" default and _DEFAULT_SPAWN = "ephemeral" (matches cloud-agents execution.py:1035).
• SkillsSpec has only image (and paths) — no per-skill images — so the skills.image check covers the surface.
• WorkflowSpec has no provider field; top-level WorkflowDefinition.provider is the only path the PR's definition.get("provider") reaches.
• validate_definition in cloud-agents runs the definition-level provider.credentials_secret check via validate_credential_reference (shape only) before this PR's enforce_submission_hardening runs — so a credential-shaped value is 422 and a well-formed reference is 400 (no overlap on the same input).
• is_admin correctly calls access_resolver.check_access(Action.ADMIN, user_roles) directly (bypassing the recurse-into-ADMIN branch in AccessResolver.check_access), so it doesn't false-positive when only non-ADMIN actions are granted.
• The e2e tests in test_step_executor_e2e.py pass credentials_secret to cloud_agents.workflow.core.step_runner.run_step directly (not through the HTTP handler), so they bypass the new gate and are unaffected.
• get_authorization_resolvers is @lru_cache(maxsize=1) and is already called by _perform_authorization_check via @authorize, so is_admin's second call is free.

[jameswnl]

Thanks. Pushed the review nits: removed dead default=str; is_admin now logs when roles are missing and documents it's only valid after @authorize; denials are logged and the docstring notes shape-first ordering and that audit events land in Phase 4; comment at the call site explains caller_provider vs provider; added tests for credentials_secret None/"" and for non-admin spawn: none without a spawner. Phase 0b is the next PR. Unit suite: 3419 passed.