From f5ad05b9995093af173dfec4f2200a868903f621 Mon Sep 17 00:00:00 2001 From: Aditya Jain Date: Sat, 22 Aug 2026 20:20:53 -0700 Subject: [PATCH] fix(toolkit-lib): allowCrossAccountAssetPublishing cache ignores which stack's environment it was computed for MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deployments.allowCrossAccountAssetPublishingForEnv() cached its answer in a single un-keyed instance field. determineAllowCrossAccountAssetPublishing() reads the target environment's bootstrap stack, so its answer is intrinsically per-account/per-region — but a single Deployments instance is reused for every stack in one `cdk deploy` invocation, which can span multiple accounts/regions. The first stack's asset publish would cache its environment's answer and every other stack's asset publish would silently reuse it, regardless of that stack's own environment. Depending on stack ordering this either blocks a perfectly valid deploy with a spurious cross-account error, or — worse — skips the safety check entirely for a stack whose own environment should have blocked cross-account publishing. This is the same class of bug as #1838, fixed for the neighboring publisherCache in #1839: key the cache by resolved environment (account:region), same as cachedPublisher already does. Fixes #1883 Co-Authored-By: Claude Sonnet 5 --- .../lib/api/deployments/deployments.ts | 22 +++++++--- .../cloudformation-deployments.test.ts | 40 +++++++++++++++++++ 2 files changed, 57 insertions(+), 5 deletions(-) diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deployments.ts b/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deployments.ts index 2329e6e50..d27ed7130 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deployments.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deployments.ts @@ -321,7 +321,12 @@ export class Deployments { private readonly publisherCache = new Map>(); - private _allowCrossAccountAssetPublishing: boolean | undefined; + // Cache by resolved environment: this Deployments instance is reused across every + // stack in a single `cdk deploy` invocation, and stacks can target different + // accounts/regions. determineAllowCrossAccountAssetPublishing() reads that specific + // environment's bootstrap stack, so caching a single un-keyed value would silently + // reuse the first stack's environment's answer for every other stack's environment. + private readonly allowCrossAccountAssetPublishingCache = new Map(); private readonly ioHelper: IoHelper; @@ -744,11 +749,18 @@ export class Deployments { } private async allowCrossAccountAssetPublishingForEnv(stack: cxapi.CloudFormationStackArtifact): Promise { - if (this._allowCrossAccountAssetPublishing === undefined) { - const env = await this.envs.accessStackForReadOnlyStackOperations(stack); - this._allowCrossAccountAssetPublishing = await determineAllowCrossAccountAssetPublishing(env.sdk, this.ioHelper, this.props.toolkitStackName); + const resolvedEnvironment = await this.envs.resolveStackEnvironment(stack); + const envKey = `${resolvedEnvironment.account}:${resolvedEnvironment.region}`; + + const cached = this.allowCrossAccountAssetPublishingCache.get(envKey); + if (cached !== undefined) { + return cached; } - return this._allowCrossAccountAssetPublishing; + + const env = await this.envs.accessStackForReadOnlyStackOperations(stack); + const allowed = await determineAllowCrossAccountAssetPublishing(env.sdk, this.ioHelper, this.props.toolkitStackName); + this.allowCrossAccountAssetPublishingCache.set(envKey, allowed); + return allowed; } /** diff --git a/packages/@aws-cdk/toolkit-lib/test/api/deployments/cloudformation-deployments.test.ts b/packages/@aws-cdk/toolkit-lib/test/api/deployments/cloudformation-deployments.test.ts index 7e06b25b5..d92a66437 100644 --- a/packages/@aws-cdk/toolkit-lib/test/api/deployments/cloudformation-deployments.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/api/deployments/cloudformation-deployments.test.ts @@ -12,6 +12,7 @@ import { GetParameterCommand } from '@aws-sdk/client-ssm'; import { CloudFormationStack } from '../../../lib/api/cloudformation'; import { Deployments } from '../../../lib/api/deployments'; import * as cfnApi from '../../../lib/api/deployments/cfn-api'; +import { determineAllowCrossAccountAssetPublishing } from '../../../lib/api/deployments/checks'; import { deployStack, destroyStack } from '../../../lib/api/deployments/deploy-stack'; import { ToolkitInfo } from '../../../lib/api/toolkit-info'; import { testStack } from '../../_helpers/assembly'; @@ -29,6 +30,7 @@ import { FakeCloudformationStack } from '../_helpers/fake-cloudformation-stack'; jest.mock('../../../lib/api/deployments/deploy-stack'); jest.mock('../../../lib/api/deployments/asset-publishing'); +jest.mock('../../../lib/api/deployments/checks'); let sdkProvider: MockSdkProvider; let sdk: MockSdk; @@ -1344,6 +1346,44 @@ describe('cachedPublisher', () => { }); }); +describe('allowCrossAccountAssetPublishingForEnv', () => { + // Regression test: the cross-account-asset-publishing answer used to be cached in a + // single un-keyed instance field, so the first stack's environment's answer was + // silently reused for every other stack's environment on the same Deployments + // instance (which is reused for every stack in one `cdk deploy` invocation). + test('does not reuse the answer across different environments', async () => { + sdkProvider.forEnvironment = jest.fn().mockImplementation(() => ({ sdk: new MockSdk() })); + const mockDetermine = determineAllowCrossAccountAssetPublishing as jest.Mock; + mockDetermine.mockResolvedValueOnce(false).mockResolvedValueOnce(true); + + const stackA = testStack({ stackName: 'StackA', env: 'aws://111111111111/us-east-1' }); + const stackB = testStack({ stackName: 'StackB', env: 'aws://222222222222/eu-west-1' }); + + const allowedForA = await (deployments as any).allowCrossAccountAssetPublishingForEnv(stackA); + const allowedForB = await (deployments as any).allowCrossAccountAssetPublishingForEnv(stackB); + + expect(allowedForA).toBe(false); + expect(allowedForB).toBe(true); + expect(mockDetermine).toHaveBeenCalledTimes(2); + }); + + test('reuses the cached answer for repeat calls with the same environment', async () => { + sdkProvider.forEnvironment = jest.fn().mockImplementation(() => ({ sdk: new MockSdk() })); + const mockDetermine = determineAllowCrossAccountAssetPublishing as jest.Mock; + mockDetermine.mockResolvedValueOnce(true); + + const stackA = testStack({ stackName: 'StackA', env: 'aws://111111111111/us-east-1' }); + const stackAAgain = testStack({ stackName: 'StackA', env: 'aws://111111111111/us-east-1' }); + + const first = await (deployments as any).allowCrossAccountAssetPublishingForEnv(stackA); + const second = await (deployments as any).allowCrossAccountAssetPublishingForEnv(stackAAgain); + + expect(first).toBe(true); + expect(second).toBe(true); + expect(mockDetermine).toHaveBeenCalledTimes(1); + }); +}); + function pushStackResourceSummaries(stackName: string, ...items: StackResourceSummary[]) { if (!currentCfnStackResources[stackName]) { currentCfnStackResources[stackName] = [];