diff --git a/docs/packages/amplify-cli/src/commands/drift.md b/docs/packages/amplify-cli/src/commands/drift.md index 6d12a1179f7..f3fb1f15cce 100644 --- a/docs/packages/amplify-cli/src/commands/drift.md +++ b/docs/packages/amplify-cli/src/commands/drift.md @@ -100,7 +100,11 @@ graph TB 2. Calls AWS `DetectStackDrift` API for each stack 3. Polls for completion (5-minute timeout, 2-second intervals) 4. Retrieves detailed drift information for all resources -5. Filters out known false positives (e.g., Auth role Deny→Allow changes) +5. Filters out known false positives: + - Auth role Deny→Allow changes (Cognito Identity Pool) + - REST API Description property (null vs empty) + - Auth trigger policies on Lambda execution roles (Cognito trigger policies added during push) + - S3 trigger policies on Lambda execution roles (S3 trigger policies added during push) 6. Filters to only drifted resources (MODIFIED or DELETED) at detection time **Example drift detected:** @@ -316,6 +320,30 @@ function isAmplifyAuthRoleDenyToAllowChange(propDiff, print): boolean { } ``` +### REST API & Trigger Policy Filtering (False Positives) + +Amplify's push pipeline introduces drift that should not be reported: + +```typescript +// From detect-stack-drift.ts — all filters are registered in a single array +const FALSE_POSITIVE_FILTERS = [ + isAmplifyAuthRoleDenyToAllowChange, + isAmplifyRestApiDescriptionDrift, + isAmplifyTriggerPolicyDrift, +] as const; + +// REST API Description: null vs empty mismatch +function isAmplifyRestApiDescriptionDrift(drift, propDiff, print): boolean { + // Filters /Description on AWS::ApiGateway::RestApi where one side is null +} + +// Auth/S3 trigger policies added to Lambda execution roles during push +function isAmplifyTriggerPolicyDrift(drift, propDiff, print): boolean { + // Parses PolicyDocument JSON and checks actions via Set containment + // against known Cognito or S3 trigger policy patterns +} +``` + ### Graceful Degradation Phases can be skipped without failing the entire operation, with results including skip reasons: diff --git a/packages/amplify-cli/src/__tests__/commands/drift-detection/detect-stack-drift.test.ts b/packages/amplify-cli/src/__tests__/commands/drift-detection/detect-stack-drift.test.ts new file mode 100644 index 00000000000..9043e5b217a --- /dev/null +++ b/packages/amplify-cli/src/__tests__/commands/drift-detection/detect-stack-drift.test.ts @@ -0,0 +1,150 @@ +import { type StackResourceDrift, type PropertyDifference } from '@aws-sdk/client-cloudformation'; +import { isAmplifyRestApiDescriptionDrift, isAmplifyTriggerPolicyDrift } from '../../../commands/drift-detection/detect-stack-drift'; +import { SpinningLogger } from '../../../commands/gen2-migration/_spinning-logger'; + +const mockPrinter = new SpinningLogger('test', { debug: true }); + +function makeDrift(overrides: Partial = {}): StackResourceDrift { + return { + StackId: 'arn:aws:cloudformation:us-east-1:123:stack/test/guid', + LogicalResourceId: 'TestResource', + ResourceType: 'AWS::Lambda::Function', + StackResourceDriftStatus: 'MODIFIED', + PropertyDifferences: [], + ...overrides, + } as StackResourceDrift; +} + +function makePropDiff(overrides: Partial = {}): PropertyDifference { + return { + PropertyPath: '/SomeProperty', + ExpectedValue: 'old', + ActualValue: 'new', + DifferenceType: 'NOT_EQUAL', + ...overrides, + }; +} + +describe('isAmplifyRestApiDescriptionDrift', () => { + beforeEach(() => jest.clearAllMocks()); + + it('filters RestApi Description drift where actual is null and expected is empty', () => { + const drift = makeDrift({ ResourceType: 'AWS::ApiGateway::RestApi', LogicalResourceId: 'apinutritionapi' }); + const propDiff = makePropDiff({ PropertyPath: '/Description', ExpectedValue: '', ActualValue: 'null' }); + expect(isAmplifyRestApiDescriptionDrift(drift, propDiff, mockPrinter)).toBe(true); + }); + + it('ignores wrong property, wrong resource type, and genuine description changes', () => { + const drift = makeDrift({ ResourceType: 'AWS::ApiGateway::RestApi' }); + // wrong property + expect( + isAmplifyRestApiDescriptionDrift(drift, makePropDiff({ PropertyPath: '/Name', ExpectedValue: '', ActualValue: 'null' }), mockPrinter), + ).toBe(false); + // wrong resource type + expect( + isAmplifyRestApiDescriptionDrift( + makeDrift({ ResourceType: 'AWS::Lambda::Function' }), + makePropDiff({ PropertyPath: '/Description', ExpectedValue: '', ActualValue: 'null' }), + mockPrinter, + ), + ).toBe(false); + // both non-null + expect( + isAmplifyRestApiDescriptionDrift( + drift, + makePropDiff({ PropertyPath: '/Description', ExpectedValue: 'old', ActualValue: 'new' }), + mockPrinter, + ), + ).toBe(false); + }); +}); + +describe('isAmplifyTriggerPolicyDrift', () => { + beforeEach(() => jest.clearAllMocks()); + + it('filters Cognito trigger policy drift (AdminAddUserToGroup)', () => { + const drift = makeDrift({ ResourceType: 'AWS::IAM::Role', LogicalResourceId: 'LambdaExecutionRole' }); + const policyValue = JSON.stringify({ + PolicyDocument: JSON.stringify({ + Version: '2012-10-17', + Statement: [ + { + Effect: 'Allow', + Action: ['cognito-idp:AdminAddUserToGroup', 'cognito-idp:GetGroup', 'cognito-idp:CreateGroup'], + Resource: 'arn:aws:cognito-idp:us-east-1:123:userpool/us-east-1_abc', + }, + ], + }), + PolicyName: 'AddToGroupCognito', + }); + const propDiff = makePropDiff({ PropertyPath: '/Policies/0', ExpectedValue: 'null', ActualValue: policyValue }); + expect(isAmplifyTriggerPolicyDrift(drift, propDiff, mockPrinter)).toBe(true); + }); + + it('filters S3 storage trigger policy drift', () => { + const drift = makeDrift({ ResourceType: 'AWS::IAM::Role', LogicalResourceId: 'LambdaExecutionRole' }); + const policyValue = JSON.stringify({ + PolicyDocument: JSON.stringify({ + Version: '2012-10-17', + Statement: [ + { Effect: 'Allow', Action: 's3:ListBucket', Resource: 'arn:aws:s3:::storagebucket-main' }, + { + Effect: 'Allow', + Action: ['s3:PutObject', 's3:GetObject', 's3:ListBucket', 's3:DeleteObject'], + Resource: 'arn:aws:s3:::storagebucket-main/*', + }, + ], + }), + PolicyName: 'amplify-lambda-execution-policy-storage', + }); + const propDiff = makePropDiff({ PropertyPath: '/Policies/1', ExpectedValue: 'null', ActualValue: policyValue }); + expect(isAmplifyTriggerPolicyDrift(drift, propDiff, mockPrinter)).toBe(true); + }); + + it('ignores wrong resource type, wrong property path, and non-null expected value', () => { + // wrong resource type + expect( + isAmplifyTriggerPolicyDrift( + makeDrift({ ResourceType: 'AWS::IAM::Policy' }), + makePropDiff({ PropertyPath: '/Policies/0', ExpectedValue: 'null', ActualValue: '{}' }), + mockPrinter, + ), + ).toBe(false); + // wrong property path + expect( + isAmplifyTriggerPolicyDrift( + makeDrift({ ResourceType: 'AWS::IAM::Role' }), + makePropDiff({ PropertyPath: '/AssumeRolePolicyDocument', ExpectedValue: 'null', ActualValue: '{}' }), + mockPrinter, + ), + ).toBe(false); + // expected is not null (policy already existed in template) + const policyValue = JSON.stringify({ PolicyDocument: '{"Statement":[]}', PolicyName: 'amplify-lambda-execution-policy-storage' }); + expect( + isAmplifyTriggerPolicyDrift( + makeDrift({ ResourceType: 'AWS::IAM::Role' }), + makePropDiff({ PropertyPath: '/Policies/0', ExpectedValue: 'some-existing-policy', ActualValue: policyValue }), + mockPrinter, + ), + ).toBe(false); + }); + + it('does not filter unknown policy content', () => { + const drift = makeDrift({ ResourceType: 'AWS::IAM::Role' }); + const policyValue = JSON.stringify({ + PolicyDocument: JSON.stringify({ + Version: '2012-10-17', + Statement: [{ Effect: 'Allow', Action: 'dynamodb:PutItem', Resource: '*' }], + }), + PolicyName: 'SomeCustomPolicy', + }); + const propDiff = makePropDiff({ PropertyPath: '/Policies/0', ExpectedValue: 'null', ActualValue: policyValue }); + expect(isAmplifyTriggerPolicyDrift(drift, propDiff, mockPrinter)).toBe(false); + }); + + it('returns false on malformed JSON instead of throwing', () => { + const drift = makeDrift({ ResourceType: 'AWS::IAM::Role' }); + const propDiff = makePropDiff({ PropertyPath: '/Policies/0', ExpectedValue: 'null', ActualValue: 'not-json' }); + expect(isAmplifyTriggerPolicyDrift(drift, propDiff, mockPrinter)).toBe(false); + }); +}); diff --git a/packages/amplify-cli/src/commands/drift-detection/detect-stack-drift.ts b/packages/amplify-cli/src/commands/drift-detection/detect-stack-drift.ts index 79ac40e3f1e..a6d8cb1513f 100644 --- a/packages/amplify-cli/src/commands/drift-detection/detect-stack-drift.ts +++ b/packages/amplify-cli/src/commands/drift-detection/detect-stack-drift.ts @@ -18,6 +18,12 @@ import { extractCategory } from '../gen2-migration/categories'; import type { SpinningLogger } from '../gen2-migration/_spinning-logger'; import { extractStackNameFromId } from '../gen2-migration/refactor/utils'; +/** + * Known false-positive filters applied to Phase 1 drift results. + * Add new filters here instead of modifying the detection loop. + */ +const FALSE_POSITIVE_FILTERS = [isAmplifyAuthRoleDenyToAllowChange, isAmplifyRestApiDescriptionDrift, isAmplifyTriggerPolicyDrift] as const; + /** * Enriched drift tree node — one per stack (root or nested) */ @@ -92,16 +98,16 @@ export async function detectStackDrift( if (page.StackResourceDrifts) allDrifts.push(...page.StackResourceDrifts); } - // Filter out known Amplify Auth IdP Deny→Allow changes + // Filter out known Amplify-introduced false positives const filteredDrifts = allDrifts.map((drift) => { if ( drift.StackResourceDriftStatus === StackResourceDriftStatus.MODIFIED && drift.PropertyDifferences && drift.PropertyDifferences.length > 0 ) { - drift.PropertyDifferences = drift.PropertyDifferences.filter((propDiff) => { - return !isAmplifyAuthRoleDenyToAllowChange(propDiff, print); - }); + drift.PropertyDifferences = drift.PropertyDifferences.filter( + (propDiff) => !FALSE_POSITIVE_FILTERS.some((filter) => filter(drift, propDiff, print)), + ); if (drift.PropertyDifferences.length === 0) { drift.StackResourceDriftStatus = StackResourceDriftStatus.IN_SYNC; @@ -116,7 +122,7 @@ export async function detectStackDrift( /** * Check if a property difference is an Amplify auth role Deny→Allow change (intended drift) */ -function isAmplifyAuthRoleDenyToAllowChange(propDiff: PropertyDifference, print: SpinningLogger): boolean { +function isAmplifyAuthRoleDenyToAllowChange(_drift: StackResourceDrift, propDiff: PropertyDifference, print: SpinningLogger): boolean { // Check if this is an AssumeRolePolicyDocument change if (!propDiff.PropertyPath || !propDiff.PropertyPath.includes('AssumeRolePolicyDocument')) { return false; @@ -163,7 +169,63 @@ function isAmplifyAuthRoleDenyToAllowChange(propDiff: PropertyDifference, print: } /** - * Wait for a drift detection operation to complete + * Check if a property difference is a REST API Description false positive. + * Amplify may set Description during template generation but the deployed + * resource reports it differently (or as null). + */ +export function isAmplifyRestApiDescriptionDrift(drift: StackResourceDrift, propDiff: PropertyDifference, print: SpinningLogger): boolean { + if (drift.ResourceType !== 'AWS::ApiGateway::RestApi') return false; + if (!propDiff.PropertyPath || propDiff.PropertyPath !== '/Description') return false; + + // The known false positive: actual is null, expected is empty + if (propDiff.ActualValue === 'null' && propDiff.ExpectedValue === '') { + print.debug(`Filtering false positive: REST API Description drift on ${drift.LogicalResourceId}`); + return true; + } + + return false; +} + +/** + * Check if a property difference is an Amplify trigger policy false positive. + * Auth triggers and S3 storage triggers dynamically attach IAM policies to + * Lambda execution roles during push. These appear as /Policies/N diffs where + * ExpectedValue is null and ActualValue contains a known Amplify policy. + */ +export function isAmplifyTriggerPolicyDrift(drift: StackResourceDrift, propDiff: PropertyDifference, print: SpinningLogger): boolean { + if (drift.ResourceType !== 'AWS::IAM::Role') return false; + if (!propDiff.PropertyPath || !/\/Policies\/\d+/.test(propDiff.PropertyPath)) return false; + + // The template has null, the deployed resource has the policy + if (propDiff.ExpectedValue !== 'null') return false; + + try { + const actualPolicy = JSON.parse(propDiff.ActualValue ?? ''); + const policyName: string = actualPolicy.PolicyName; + const policyDoc = JSON.parse(actualPolicy.PolicyDocument as string); + const actions = new Set(policyDoc.Statement.flatMap((s) => [s.Action].flat())); + + // Auth trigger policies: known Cognito trigger policy pattern + const cognitoActions = ['cognito-idp:AdminAddUserToGroup', 'cognito-idp:GetGroup', 'cognito-idp:CreateGroup']; + const isCognitoTriggerPolicy = policyName === 'AddToGroupCognito' && cognitoActions.every((a) => actions.has(a)); + + // S3 storage trigger policies: known S3 trigger policy pattern + const s3Actions = ['s3:ListBucket', 's3:PutObject', 's3:GetObject', 's3:DeleteObject']; + const isS3TriggerPolicy = policyName === 'amplify-lambda-execution-policy-storage' && s3Actions.every((a) => actions.has(a)); + + if (isCognitoTriggerPolicy || isS3TriggerPolicy) { + print.debug(`Filtering false positive: trigger policy drift on ${drift.LogicalResourceId} (${policyName})`); + return true; + } + } catch (e: any) { + print.debug(`Failed to parse trigger policy JSON: ${e.message || 'Unknown error'}`); + return false; + } + + return false; +} + +/** * Based on CDK's polling strategy: 5-minute timeout, 2-second polling interval, 10-second user feedback */ async function waitForDriftDetection(