Skip to content

fix(toolsets): prevent x-account-id leak across concurrent fetchTools#367

Merged
shashi-stackone merged 1 commit into
mainfrom
ENG-47
Apr 27, 2026
Merged

fix(toolsets): prevent x-account-id leak across concurrent fetchTools#367
shashi-stackone merged 1 commit into
mainfrom
ENG-47

Conversation

@shashi-stackone
Copy link
Copy Markdown
Contributor

@shashi-stackone shashi-stackone commented Apr 27, 2026

Summary

fetchTools() mutated this.headers to inject the per-account x-account-id, then awaited the MCP fetch. Under concurrency Promise.all over multiple accountIds, or two fetchTools() calls on one instance those mutations interleaved. createRpcBackedTool re-read this.headers after the await and baked the wrong tenant's x-account-id into the returned tools. For a shared StackOneToolSet serving multiple users, this is a cross-tenant data-leak path.

Closes #365

Fix

Stop mutating this.headers; thread per-request headers through as a parameter into fetchToolsFromMcpcreateRpcBackedToolBaseTool. this.headers is now invariant after construction, so concurrent callers cannot interfere.

const toolsPromises = effectiveAccountIds.map(async (accountId) => {
    const requestHeaders = { ...this.headers, 'x-account-id': accountId };
    const accountTools = await this.fetchToolsFromMcp(requestHeaders);
    return accountTools.toArray();
});

Mirrors the design already used in the Python SDK (stackone_ai/toolset.py:_fetch_for_account). The catalogCache is fixed transitively.

Test plan

  • pnpm vitest run src/toolsets.test.ts 55/55 pass (53 existing + 2 new)
  • pnpm vitest run 559/559 pass, no regressions
  • pnpm lint run locally in nix dev shell

Both new tests fail deterministically against the buggy code and pass with the fix.

Out of scope

  • Python SDK already correct (zero self.headers references in toolset.py); no changes needed.
  • Public API fetchTools signature unchanged.

Summary by cubic

Fixes a cross-tenant header leak in concurrent fetchTools calls by passing per-request x-account-id headers instead of mutating instance headers. Tools now always carry the correct account scope, preventing data leakage in shared StackOneToolSet instances.

  • Bug Fixes
    • Pass request-scoped headers to fetchToolsFromMcp and into tool creation; this.headers is no longer mutated.
    • Ensure each tool is stamped with the correct x-account-id under intra-call and inter-call concurrency.
    • Added regression tests; public API remains unchanged.

Written for commit e7ed38b. Summary will update on new commits.

Copilot AI review requested due to automatic review settings April 27, 2026 09:03
@shashi-stackone shashi-stackone requested a review from a team as a code owner April 27, 2026 09:03
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes a concurrency bug in StackOneToolSet.fetchTools() where mutating this.headers around await boundaries could cause tools to be created with the wrong tenant’s x-account-id, enabling cross-account leakage under concurrent calls.

Changes:

  • Stop mutating this.headers during fetchTools(); instead, build per-request headers and pass them down explicitly.
  • Thread requestHeaders through fetchToolsFromMcp()createRpcBackedTool() so tool instances capture the correct account-scoped headers.
  • Add regressions tests covering intra-call concurrency (Promise.all over multiple accounts) and inter-call concurrency (two fetchTools calls on one instance).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/toolsets.ts Removes shared mutable header mutation and passes per-request headers through the MCP fetch/tool construction chain.
src/toolsets.test.ts Adds regression tests ensuring tools are stamped with the correct x-account-id under concurrent fetch scenarios.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

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

No issues found across 2 files

Requires human review: This PR fixes a security-sensitive cross-tenant data leak by modifying core logic for header management and tool initialization, which requires human verification.

Copy link
Copy Markdown
Contributor

@glebedel glebedel left a comment

Choose a reason for hiding this comment

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

LGTM

@shashi-stackone shashi-stackone merged commit 600b792 into main Apr 27, 2026
21 of 23 checks passed
@shashi-stackone shashi-stackone deleted the ENG-47 branch April 27, 2026 11:39
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.

Bug report: StackOneToolSet.fetchTools() is not safe for concurrent calls — tools captured with wrong x-account-id

3 participants