Skip to content

Enforce resource access during workflow execution#152

Merged
mbakgun merged 5 commits into
heymrun:mainfrom
VykosMolt:codex/execution-resource-auth
Jun 7, 2026
Merged

Enforce resource access during workflow execution#152
mbakgun merged 5 commits into
heymrun:mainfrom
VykosMolt:codex/execution-resource-auth

Conversation

@VykosMolt

Copy link
Copy Markdown
Contributor

This tightens workflow execution so nodes use the same ownership/share rules as the API before decrypting or using resources. It passes an actor user into normal, streaming, trigger, portal, sub-workflow, and HITL resume execution paths, then uses that actor when loading credentials, vector stores, and DataTables.

What changed:

  • centralizes execution-time access checks for credentials, vector stores, and DataTables
  • carries the actor user through workflow execution, streaming execution, triggers, portal runs, sub-workflows, and HITL snapshots
  • keeps shared credentials/tables usable when they are actually shared with the actor
  • adds regression coverage for credential and DataTable access checks

Verified with:

  • uv run ruff format --check app tests/test_agent_node_tools.py tests/test_retry_execution_logs.py tests/test_sse_streaming.py tests/test_workflow_execution_api.py
  • uv run ruff check app tests/test_agent_node_tools.py tests/test_retry_execution_logs.py tests/test_sse_streaming.py tests/test_workflow_execution_api.py
  • ./run_tests.sh

@mbakgun

mbakgun commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

Thanks for putting this together 🙏 This looks like a real and worthwhile access-control fix, and I agree with the overall direction: execution-time resource access should be verified against the actor instead of trusting whatever resource ID was saved in the node config. 👑

The DataTable and credential paths look clearly valid. The DataTable case is especially convincing because the previous check relied on self.workflow_owner_id, which does not appear to be set anywhere, so that access-control branch was effectively dead code. The credential lookups were also loading and decrypting credentials by ID without an ownership or share filter. Nice catch on both.

A few things I would like us to address or verify before merge:

  1. Please make the actor_user_id=None fallback safer. 🔥

    The helper methods currently fail open and fall back to the old unrestricted lookup behavior when no actor is passed. You did thread the actor through the current entry points, so this looks covered today, but a future call site could silently regress if it forgets to pass an actor.

    Prefer failing closed, or at minimum add explicit logging or an assertion when a resource lookup runs without an actor.

  2. Please verify the shared VectorStore semantics.

    This is my main concern. Today, the VectorStore API appears to allow a user with access to a shared vector store to use that store’s backing credential without the credential being shared separately. The PR changes the RAG node to require access to both the vector store and store.credential_id.

    If the intended product behavior is “sharing a vector store includes permission to use its backing credential for that store”, this may break existing shared RAG workflows. My suggestion is to keep the new _get_accessible_vector_store check, since that closes the real store IDOR, but load store.credential_id consistently with the existing VectorStore API behavior.

    If we do want to tighten this model, let us tighten it in both the API and executor together, and add a test for the chosen behavior.

  3. Please check consistency with get_credentials_context, especially team-shared credentials.

    The new executor helper supports team-shared credentials, while the $credentials.NAME context path appears to include owned and direct user-shared credentials only. The single-credential accessor includes team shares, so the helper may be the more correct behavior, but it would be good to align these paths so expression-based and credential-id based access follow the same model.

Overall, the core issue is valid and worth fixing. These notes are mostly about making the fix robust and avoiding an accidental regression around shared resources. Thanks again for tackling this. 🤞

@VykosMolt

Copy link
Copy Markdown
Contributor Author

Thank you for the thorough review and the feedback!

I have addressed all of the brought up points and have pushed the update to the PR.

@mbakgun

mbakgun commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

Thanks for the quick turnaround. The latest commits address the earlier feedback well: the fail-closed _require_actor_user_id behavior, the VectorStore backing-credential handling, and the team-shared credential context consistency all look good now.

One remaining blocker before merge: a few execute_workflow call sites still do not pass actor_user_id. With the new fail-closed behavior, those paths will fail for workflows that use credentials, DataTables, or VectorStores.

Please forward the actor in these call sites:

  • backend/app/api/ai_assistant.py:1282: add actor_user_id=user_id
  • backend/app/api/mcp.py:617: add actor_user_id=mcp_user.id
  • backend/app/api/mcp.py:787: add actor_user_id=mcp_user.id
  • backend/app/api/mcp_servers.py:462: add actor_user_id=user.id

A small regression test for one of these paths would also be helpful, since the current tests do not cover this miss.

Everything else looks solid. Thanks again for the fast fix.

The fail-closed resource lookups require an actor; thread it through the
dashboard chat, MCP tool, and named MCP server execution paths so
credential/DataTable/VectorStore access keeps working. Add a regression
test asserting the dashboard chat path forwards the actor.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@VykosMolt

Copy link
Copy Markdown
Contributor Author

Done!

@mbakgun mbakgun left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for fixing IDOR ! Such a king 👑 Merged once again 🚀

@mbakgun mbakgun merged commit e0c529c into heymrun:main Jun 7, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants