From d30d98bbef23df15d9c64093f5a79c61ef814848 Mon Sep 17 00:00:00 2001 From: Tofik Hasanov Date: Thu, 4 Jun 2026 17:54:44 -0400 Subject: [PATCH 1/3] fix(cloud-security): support AWS Config new recording model in checks + auto-remediation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Customer-reported: AWS auto-remediation for "AWS Config recorder not fully active" generated empty configs, the Retry button did nothing, and applying a generated fix failed and fell back to dated manual steps. Root cause: the entire Config check + auto-fix assumed the legacy `recordingGroup.allSupported` model, but the customer's recorder uses AWS's current model ("Record all resource types with customizable overrides" = recordingStrategy / exclusionByResourceTypes). - config.adapter: the recorder check now treats recordingStrategy.useOnly === 'ALL_SUPPORTED_RESOURCE_TYPES' (and legacy allSupported) as "records all", eliminating false positives on the new model. Genuine EXCLUSION/INCLUSION recorders stay flagged. - config.adapter: remediation guidance now produces an AWS-valid call — read the existing recorder, then PutConfigurationRecorder with a clean recordingGroup { allSupported: true, includeGlobalResourceTypes: true } and NO recordingStrategy/exclusionByResourceTypes/resourceTypes (those are mutually exclusive with allSupported and trigger a ValidationException). This also records the global IAM resource types the customer was missing. - aws-command-executor: deterministic guardrail (normalizeConfigRecordingGroup) collapses any all-supported-intent PutConfigurationRecorder to the single valid shape right before the SDK call, regardless of what the AI emits. - remediation.service: never cache an empty / non-auto-fixable plan and drop the stale entry on execute — this is what made "Retry" a guaranteed no-op (it reloaded the same dead plan). Retry now regenerates. - ai-remediation.service: generateFixPlan retries once at a higher temperature when the first pass yields zero fix steps (temp 0 would reproduce it). - prompts: discourage S3 ACL steps (cause of empty plans), reinforce the valid Config recorder call, and base manual steps on the current AWS Console. - RemediationDialog: disable "Apply Fix" on an empty plan and explain why. Tests: new config.adapter.spec, recordingGroup-normalizer and retry-on-empty cases; full cloud-security jest suite green (288 passing). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../cloud-security/ai-remediation.prompt.ts | 9 + .../ai-remediation.service.spec.ts | 97 +++++++++- .../cloud-security/ai-remediation.service.ts | 52 ++++-- .../aws-command-executor.spec.ts | 99 +++++++++++ .../cloud-security/aws-command-executor.ts | 40 +++++ .../providers/aws/config.adapter.spec.ts | 166 ++++++++++++++++++ .../providers/aws/config.adapter.ts | 32 +++- .../remediation.service.spec.ts | 31 ++++ .../src/cloud-security/remediation.service.ts | 68 ++++--- .../components/RemediationDialog.tsx | 15 +- 10 files changed, 569 insertions(+), 40 deletions(-) create mode 100644 apps/api/src/cloud-security/providers/aws/config.adapter.spec.ts diff --git a/apps/api/src/cloud-security/ai-remediation.prompt.ts b/apps/api/src/cloud-security/ai-remediation.prompt.ts index f179d32ef1..810a8425db 100644 --- a/apps/api/src/cloud-security/ai-remediation.prompt.ts +++ b/apps/api/src/cloud-security/ai-remediation.prompt.ts @@ -162,6 +162,15 @@ A human will ALWAYS review your plan before execution. Be precise and correct. - ALWAYS make changes reversible when possible - For service-linked roles: create them as a setup step using IAM CreateServiceLinkedRoleCommand +## S3 PUBLIC ACCESS AND ACLs (IMPORTANT) +- NEVER use PutBucketAclCommand or bucket/object ACLs. Modern buckets use Object Ownership = BucketOwnerEnforced, which disables ACLs — the call fails, and the executor strips ACL steps, which can leave an EMPTY plan. +- To block public access on a bucket: use s3:PutPublicAccessBlockCommand with PublicAccessBlockConfiguration set to { BlockPublicAcls: true, IgnorePublicAcls: true, BlockPublicPolicy: true, RestrictPublicBuckets: true }. +- To remediate a public bucket POLICY: read it first with GetBucketPolicy, then use s3:PutBucketPolicyCommand with a corrected least-privilege policy. Never rely on ACLs to fix public access. + +## AWS CONFIG RECORDER (IMPORTANT) +- To make a recorder record ALL supported resource types, first read the existing recorder with config-service:DescribeConfigurationRecordersCommand (readSteps) to get its exact name and roleARN, then call config-service:PutConfigurationRecorderCommand with ConfigurationRecorder = { name, roleARN, recordingGroup: { allSupported: true, includeGlobalResourceTypes: true } }. +- NEVER set allSupported:true together with recordingStrategy, exclusionByResourceTypes, or resourceTypes — they are mutually exclusive and AWS rejects the request with a ValidationException. Omit those fields entirely (this also overwrites an existing exclusion-based strategy so global IAM resources are recorded). + ## IDEMPOTENCY (CRITICAL) - All fix steps MUST be safe to run even if the resource already exists - For Create operations: our executor automatically handles "already exists" errors — they are treated as success, not failure diff --git a/apps/api/src/cloud-security/ai-remediation.service.spec.ts b/apps/api/src/cloud-security/ai-remediation.service.spec.ts index 22bb90eb62..d40c92eb85 100644 --- a/apps/api/src/cloud-security/ai-remediation.service.spec.ts +++ b/apps/api/src/cloud-security/ai-remediation.service.spec.ts @@ -142,9 +142,12 @@ describe('AiRemediationService.generateFixPlan empty-state backstop', () => { it('leaves the plan untouched when AI returns {}/{} but the plan has no actionable steps', async () => { // Verify-only plans (only readSteps) should still be left alone — - // we never fabricate state when there's nothing to act on. + // we never fabricate state when there's nothing to act on. A plan with no + // fix steps is not auto-fixable, so canAutoFix is false (which also means + // the empty-plan retry does not apply to it). generateObjectMock.mockResolvedValueOnce({ object: basePlan({ + canAutoFix: false, readSteps: [ { service: 's3', command: 'GetBucketVersioningCommand', params: {}, purpose: 'check' }, ], @@ -172,6 +175,17 @@ describe('AiRemediationService.generateFixPlan empty-state backstop', () => { object: basePlan({ currentState: { versioning: 'Disabled' }, proposedState: { versioning: 'Enabled' }, + fixSteps: [ + { + service: 's3', + command: 'PutBucketVersioningCommand', + params: { + Bucket: 'logs-archive', + VersioningConfiguration: { Status: 'Enabled' }, + }, + purpose: 'enable versioning', + }, + ], }), }); @@ -226,8 +240,11 @@ describe('AiRemediationService.generateFixPlan empty-state backstop', () => { }); it('leaves a plan alone when only one side is empty (legitimate verify-only case)', async () => { + // Verify-only: no fix steps, so the plan is not auto-fixable (canAutoFix + // false) and the empty-plan retry does not apply. generateObjectMock.mockResolvedValueOnce({ object: basePlan({ + canAutoFix: false, currentState: { someField: 'X' }, proposedState: {}, }), @@ -508,3 +525,81 @@ describe('AiRemediationService.generateManualSteps', () => { expect(callArgs.prompt).toContain('account-level'); }); }); + +describe('AiRemediationService.generateFixPlan empty-plan retry', () => { + const generateObjectMock = generateObject as unknown as jest.Mock; + + beforeEach(() => { + generateObjectMock.mockReset(); + }); + + it('retries once when the first plan has canAutoFix=true but zero fixSteps, and uses the non-empty retry', async () => { + // First pass: empty fix plan (the "AI generated an empty fix plan" case). + generateObjectMock.mockResolvedValueOnce({ + object: basePlan({ canAutoFix: true, fixSteps: [] }), + }); + // Second pass (higher temperature): a real plan. + generateObjectMock.mockResolvedValueOnce({ + object: basePlan({ + canAutoFix: true, + fixSteps: [ + { + service: 'config-service', + command: 'PutConfigurationRecorderCommand', + params: { ConfigurationRecorder: { name: 'default' } }, + purpose: 'Record all resources', + }, + ], + }), + }); + + const service = new AiRemediationService(); + const plan = await service.generateFixPlan({ + title: 'AWS Config recorder not fully active', + description: null, + severity: 'high', + resourceType: 'AwsConfigRecorder', + resourceId: 'default', + remediation: null, + findingKey: 'config-recorder-incomplete', + evidence: {}, + }); + + expect(generateObjectMock).toHaveBeenCalledTimes(2); + // The retry runs at a non-zero temperature so it is a genuinely different sample. + expect(generateObjectMock.mock.calls[0][0].temperature).toBe(0); + expect(generateObjectMock.mock.calls[1][0].temperature).toBeGreaterThan(0); + expect(plan.fixSteps).toHaveLength(1); + expect(plan.fixSteps[0].command).toBe('PutConfigurationRecorderCommand'); + }); + + it('does not retry when the first plan already has fix steps', async () => { + generateObjectMock.mockResolvedValueOnce({ + object: basePlan({ + canAutoFix: true, + fixSteps: [ + { + service: 'iam', + command: 'UpdateAccountPasswordPolicyCommand', + params: {}, + purpose: 'fix', + }, + ], + }), + }); + + const service = new AiRemediationService(); + await service.generateFixPlan({ + title: 'Weak password policy', + description: null, + severity: null, + resourceType: 'AwsIamPolicy', + resourceId: 'account-level', + remediation: null, + findingKey: 'iam-weak-password', + evidence: {}, + }); + + expect(generateObjectMock).toHaveBeenCalledTimes(1); + }); +}); diff --git a/apps/api/src/cloud-security/ai-remediation.service.ts b/apps/api/src/cloud-security/ai-remediation.service.ts index 0bafbbdb7d..ba09a5b95f 100644 --- a/apps/api/src/cloud-security/ai-remediation.service.ts +++ b/apps/api/src/cloud-security/ai-remediation.service.ts @@ -53,20 +53,23 @@ export class AiRemediationService { /** Phase 1: Generate initial plan (read steps + preliminary fix plan). */ async generateFixPlan(finding: FindingContext): Promise { try { - const { object } = await generateObject({ - model: MODEL, - schema: fixPlanSchema, - system: SYSTEM_PROMPT, - prompt: buildFixPlanPrompt(finding), - temperature: 0, - }); + let plan = await this.requestFixPlan(finding, 0); + + // The model occasionally returns canAutoFix=true with zero fixSteps, or + // the normalizer strips every step (e.g. unsupported S3 ACL calls). That + // surfaces to the user as "AI generated an empty fix plan. Cannot + // proceed." and — combined with plan caching — a Retry that does + // nothing. Generation is non-deterministic, so retry ONCE at a higher + // temperature to force a genuinely different sample before giving up. + if (plan.canAutoFix && plan.fixSteps.length === 0) { + this.logger.warn( + `Empty fix plan for ${finding.findingKey}; regenerating once at higher temperature`, + ); + const retry = await this.requestFixPlan(finding, 0.5); + if (retry.fixSteps.length > 0) plan = retry; + } - this.logger.log( - `AI plan for ${finding.findingKey}: canAutoFix=${object.canAutoFix}, risk=${object.risk}`, - ); - return normalizeFixPlan(enrichEmptyState(object), { - resourceId: finding.resourceId, - }); + return plan; } catch (err) { this.logger.error( `AI plan failed: ${err instanceof Error ? err.message : String(err)}`, @@ -75,6 +78,27 @@ export class AiRemediationService { } } + /** Single fix-plan generation pass (generate → enrich → normalize). */ + private async requestFixPlan( + finding: FindingContext, + temperature: number, + ): Promise { + const { object } = await generateObject({ + model: MODEL, + schema: fixPlanSchema, + system: SYSTEM_PROMPT, + prompt: buildFixPlanPrompt(finding), + temperature, + }); + + this.logger.log( + `AI plan for ${finding.findingKey}: canAutoFix=${object.canAutoFix}, risk=${object.risk}`, + ); + return normalizeFixPlan(enrichEmptyState(object), { + resourceId: finding.resourceId, + }); + } + /** * Phase 2: Refine fix steps using REAL data from AWS. * Called after read steps executed successfully. @@ -394,7 +418,7 @@ INSTRUCTIONS: ), }), system: - 'You are an AWS security expert writing manual remediation steps for a customer whose automatic fix failed. Be concrete: name exact services, exact resources, and exact actions. Prefer AWS Console clicks over CLI when the path is short, but include CLI commands when they are clearer. Never reference SDK class names. Never apologize. Never speculate about "if the issue persists" — just give the steps.', + 'You are an AWS security expert writing manual remediation steps for a customer whose automatic fix failed. Be concrete: name exact services, exact resources, and exact actions. Prefer AWS Console clicks over CLI when the path is short, but include CLI commands when they are clearer. Never reference SDK class names. Never apologize. Never speculate about "if the issue persists" — just give the steps. Base every instruction on the CURRENT AWS Console; do NOT describe deprecated layouts or removed menu options. For AWS Config recorder settings specifically: the current console is Config → Settings, which shows a "Recording method"/"Recording strategy" under a customer managed recorder — to record everything, click Edit, choose to record all resource types, enable "Include global resource types (IAM resources)", and remove any per-resource-type overrides or exclusions. Do not instruct the user to select an old "Record all resource types supported in this region" radio option if it is not present in the current console.', prompt: `A finding could not be auto-remediated. Generate clear manual steps the customer can follow. FINDING: diff --git a/apps/api/src/cloud-security/aws-command-executor.spec.ts b/apps/api/src/cloud-security/aws-command-executor.spec.ts index 4a88777172..94748773a5 100644 --- a/apps/api/src/cloud-security/aws-command-executor.spec.ts +++ b/apps/api/src/cloud-security/aws-command-executor.spec.ts @@ -2,6 +2,7 @@ import type { AwsCommandStep } from './ai-remediation.prompt'; import { REQUIRED_PARAMS, looksLikeValidationError, + normalizeConfigRecordingGroup, validatePlanSteps, } from './aws-command-executor'; @@ -302,3 +303,101 @@ describe('validatePlanSteps — pre-existing behavior preserved', () => { ).toBeDefined(); }); }); + +/** + * AWS Config recorder: `allSupported:true` is mutually exclusive with + * `recordingStrategy` / `exclusionByResourceTypes` / `resourceTypes`. The AI + * (and a customer's existing exclusion-based recorder) frequently echoes those + * fields back alongside allSupported:true, which AWS rejects with a + * ValidationException. normalizeConfigRecordingGroup collapses the group to the + * single valid "record everything (incl. global IAM)" shape. + */ +describe('normalizeConfigRecordingGroup', () => { + it('strips conflicting fields when an exclusion-based group is converted to all-supported', () => { + const input: Record = { + ConfigurationRecorder: { + name: 'default', + roleARN: 'arn:aws:iam::123:role/aws-service-role/config', + recordingGroup: { + allSupported: true, + recordingStrategy: { useOnly: 'EXCLUSION_BY_RESOURCE_TYPES' }, + exclusionByResourceTypes: { + resourceTypes: ['AWS::IAM::User', 'AWS::IAM::Role'], + }, + }, + }, + }; + normalizeConfigRecordingGroup(input); + const recorder = input.ConfigurationRecorder as Record; + expect(recorder.recordingGroup).toEqual({ + allSupported: true, + includeGlobalResourceTypes: true, + }); + // name + roleARN are preserved untouched. + expect(recorder.name).toBe('default'); + expect(recorder.roleARN).toBe( + 'arn:aws:iam::123:role/aws-service-role/config', + ); + }); + + it('converts a pure exclusion strategy (allSupported absent) to all-supported', () => { + const input: Record = { + ConfigurationRecorder: { + name: 'default', + recordingGroup: { + recordingStrategy: { useOnly: 'EXCLUSION_BY_RESOURCE_TYPES' }, + exclusionByResourceTypes: { resourceTypes: ['AWS::IAM::Role'] }, + }, + }, + }; + normalizeConfigRecordingGroup(input); + const recorder = input.ConfigurationRecorder as Record; + expect(recorder.recordingGroup).toEqual({ + allSupported: true, + includeGlobalResourceTypes: true, + }); + }); + + it('cleans an ALL_SUPPORTED_RESOURCE_TYPES strategy to the minimal valid shape', () => { + const input: Record = { + ConfigurationRecorder: { + name: 'default', + recordingGroup: { + allSupported: true, + recordingStrategy: { useOnly: 'ALL_SUPPORTED_RESOURCE_TYPES' }, + }, + }, + }; + normalizeConfigRecordingGroup(input); + expect( + (input.ConfigurationRecorder as Record).recordingGroup, + ).toEqual({ allSupported: true, includeGlobalResourceTypes: true }); + }); + + it('leaves an INCLUSION_BY_RESOURCE_TYPES recorder untouched (records only specific types)', () => { + const recordingGroup = { + allSupported: false, + recordingStrategy: { useOnly: 'INCLUSION_BY_RESOURCE_TYPES' }, + resourceTypes: ['AWS::S3::Bucket'], + }; + const input: Record = { + ConfigurationRecorder: { name: 'default', recordingGroup }, + }; + normalizeConfigRecordingGroup(input); + expect( + (input.ConfigurationRecorder as Record).recordingGroup, + ).toEqual(recordingGroup); + }); + + it('is a no-op when there is no ConfigurationRecorder/recordingGroup', () => { + const input: Record = {}; + expect(() => normalizeConfigRecordingGroup(input)).not.toThrow(); + expect(input).toEqual({}); + + const input2: Record = { + ConfigurationRecorder: { name: 'default' }, + }; + normalizeConfigRecordingGroup(input2); + expect(input2).toEqual({ ConfigurationRecorder: { name: 'default' } }); + }); +}); diff --git a/apps/api/src/cloud-security/aws-command-executor.ts b/apps/api/src/cloud-security/aws-command-executor.ts index 26a913a4c5..5893beee6a 100644 --- a/apps/api/src/cloud-security/aws-command-executor.ts +++ b/apps/api/src/cloud-security/aws-command-executor.ts @@ -264,6 +264,46 @@ function normaliseInputParams( if (!input.IsMultiRegionTrail) input.IsMultiRegionTrail = true; if (!input.EnableLogFileValidation) input.EnableLogFileValidation = true; } + + // Rule 4: AWS Config recorder — `allSupported: true` is mutually exclusive + // with `recordingStrategy`, `exclusionByResourceTypes`, and `resourceTypes`. + // AWS rejects the combination with a ValidationException. The AI (and a + // customer's existing exclusion-based recorder, e.g. one that excludes the + // IAM resource types) frequently echoes those fields back alongside + // allSupported:true. When the intent is "record all supported types", + // collapse recordingGroup to the single valid shape that records everything, + // including the global IAM resource types. + if (command === 'PutConfigurationRecorderCommand') { + normalizeConfigRecordingGroup(input); + } +} + +export function normalizeConfigRecordingGroup( + input: Record, +): void { + const recorder = input.ConfigurationRecorder; + if (!recorder || typeof recorder !== 'object' || Array.isArray(recorder)) { + return; + } + const recorderObj = recorder as Record; + const group = recorderObj.recordingGroup; + if (!group || typeof group !== 'object' || Array.isArray(group)) return; + + const groupObj = group as Record; + const strategy = groupObj.recordingStrategy as + | { useOnly?: string } + | undefined; + const wantsAllSupported = + groupObj.allSupported === true || + strategy?.useOnly === 'ALL_SUPPORTED_RESOURCE_TYPES' || + groupObj.exclusionByResourceTypes != null; + + if (!wantsAllSupported) return; + + recorderObj.recordingGroup = { + allSupported: true, + includeGlobalResourceTypes: true, + }; } /** diff --git a/apps/api/src/cloud-security/providers/aws/config.adapter.spec.ts b/apps/api/src/cloud-security/providers/aws/config.adapter.spec.ts new file mode 100644 index 0000000000..c50f8bf46d --- /dev/null +++ b/apps/api/src/cloud-security/providers/aws/config.adapter.spec.ts @@ -0,0 +1,166 @@ +import { + DescribeConfigurationRecordersCommand, + DescribeConfigurationRecorderStatusCommand, + type ConfigurationRecorder, +} from '@aws-sdk/client-config-service'; +import { ConfigAdapter } from './config.adapter'; +import type { SecurityFinding } from '../../cloud-security.service'; + +type SendHandler = (command: unknown) => unknown; + +function buildClient(handler: SendHandler) { + return { + send: jest.fn((command: unknown) => Promise.resolve(handler(command))), + } as unknown as Parameters< + ConfigAdapter['scan'] + >[0] extends infer _ + ? import('@aws-sdk/client-config-service').ConfigServiceClient + : never; +} + +/** Invoke the private checkRecorders() with a mocked Config client. */ +function runCheckRecorders(args: { + recorders: ConfigurationRecorder[]; + recording: boolean; +}): Promise { + const adapter = new ConfigAdapter(); + const handler: SendHandler = (command) => { + if (command instanceof DescribeConfigurationRecordersCommand) { + return { ConfigurationRecorders: args.recorders }; + } + if (command instanceof DescribeConfigurationRecorderStatusCommand) { + return { + ConfigurationRecordersStatus: args.recorders.length + ? [{ recording: args.recording }] + : [], + }; + } + return {}; + }; + const client = buildClient(handler); + const fn = ( + adapter as unknown as { + checkRecorders: ( + c: unknown, + region: string, + ) => Promise; + } + ).checkRecorders; + return fn.call(adapter, client, 'us-east-1'); +} + +describe('ConfigAdapter — checkRecorders recording-model awareness', () => { + it('passes a legacy recorder with recordingGroup.allSupported=true', async () => { + const findings = await runCheckRecorders({ + recorders: [{ name: 'default', recordingGroup: { allSupported: true } }], + recording: true, + }); + expect(findings).toHaveLength(1); + expect(findings[0].passed).toBe(true); + expect(findings[0].title).toBe('AWS Config recorder is active'); + }); + + it('passes a new-model recorder using recordingStrategy ALL_SUPPORTED_RESOURCE_TYPES (regression: no longer false-flagged)', async () => { + const findings = await runCheckRecorders({ + recorders: [ + { + name: 'default', + // New model: allSupported is false/absent; the strategy is the source of truth. + recordingGroup: { + allSupported: false, + recordingStrategy: { useOnly: 'ALL_SUPPORTED_RESOURCE_TYPES' }, + }, + }, + ], + recording: true, + }); + expect(findings).toHaveLength(1); + expect(findings[0].passed).toBe(true); + expect(findings[0].title).toBe('AWS Config recorder is active'); + }); + + it('flags a recorder using the EXCLUSION_BY_RESOURCE_TYPES strategy (e.g. IAM excluded)', async () => { + const findings = await runCheckRecorders({ + recorders: [ + { + name: 'default', + recordingGroup: { + allSupported: false, + recordingStrategy: { useOnly: 'EXCLUSION_BY_RESOURCE_TYPES' }, + exclusionByResourceTypes: { + resourceTypes: [ + 'AWS::IAM::User', + 'AWS::IAM::Role', + 'AWS::IAM::Group', + 'AWS::IAM::Policy', + ], + }, + }, + }, + ], + recording: true, + }); + expect(findings).toHaveLength(1); + expect(findings[0].passed).toBeFalsy(); + expect(findings[0].title).toBe('AWS Config recorder not fully active'); + expect(findings[0].severity).toBe('high'); + }); + + it('flags an INCLUSION_BY_RESOURCE_TYPES recorder (records only specific types)', async () => { + const findings = await runCheckRecorders({ + recorders: [ + { + name: 'default', + recordingGroup: { + allSupported: false, + recordingStrategy: { useOnly: 'INCLUSION_BY_RESOURCE_TYPES' }, + resourceTypes: ['AWS::S3::Bucket'], + }, + }, + ], + recording: true, + }); + expect(findings[0].passed).toBeFalsy(); + expect(findings[0].title).toBe('AWS Config recorder not fully active'); + }); + + it('flags an all-supported recorder that is stopped', async () => { + const findings = await runCheckRecorders({ + recorders: [{ name: 'default', recordingGroup: { allSupported: true } }], + recording: false, + }); + expect(findings[0].passed).toBeFalsy(); + expect(findings[0].description).toContain('not recording'); + }); + + it('produces AWS-valid remediation guidance (clean recordingGroup, no conflicting fields)', async () => { + const findings = await runCheckRecorders({ + recorders: [ + { + name: 'default', + recordingGroup: { + allSupported: false, + recordingStrategy: { useOnly: 'EXCLUSION_BY_RESOURCE_TYPES' }, + exclusionByResourceTypes: { resourceTypes: ['AWS::IAM::Role'] }, + }, + }, + ], + recording: true, + }); + const remediation = findings[0].remediation ?? ''; + expect(remediation).toContain('allSupported: true'); + expect(remediation).toContain('includeGlobalResourceTypes: true'); + expect(remediation).toContain('ValidationException'); + // Must instruct AGAINST mixing the conflicting fields. + expect(remediation).toMatch(/Do NOT include recordingStrategy/); + }); + + it('flags a missing recorder with includeGlobalResourceTypes in the create guidance', async () => { + const findings = await runCheckRecorders({ + recorders: [], + recording: false, + }); + expect(findings[0].title).toBe('AWS Config recorder not configured'); + expect(findings[0].remediation).toContain('includeGlobalResourceTypes'); + }); +}); diff --git a/apps/api/src/cloud-security/providers/aws/config.adapter.ts b/apps/api/src/cloud-security/providers/aws/config.adapter.ts index 2994e1181c..0130317e01 100644 --- a/apps/api/src/cloud-security/providers/aws/config.adapter.ts +++ b/apps/api/src/cloud-security/providers/aws/config.adapter.ts @@ -1,5 +1,6 @@ import { ConfigServiceClient, + type ConfigurationRecorder, DescribeConfigurationRecordersCommand, DescribeConfigurationRecorderStatusCommand, DescribeDeliveryChannelsCommand, @@ -63,7 +64,7 @@ export class ConfigAdapter implements AwsServiceAdapter { severity: 'high', resourceId: `arn:aws:config:${region}`, remediation: - 'Step 1: Create a service-linked role using iam:CreateServiceLinkedRoleCommand with AWSServiceName set to "config.amazonaws.com" (skip if the role already exists). Step 2: Create a configuration recorder using config-service:PutConfigurationRecorderCommand with ConfigurationRecorder containing name "compai-config-recorder", roleARN set to the Config service role ARN, and recordingGroup with allSupported set to true. Step 3: Create a delivery channel using config-service:PutDeliveryChannelCommand with DeliveryChannel containing name "compai-delivery-channel" and s3BucketName set to the target bucket. Step 4: Start the recorder using config-service:StartConfigurationRecorderCommand with ConfigurationRecorderName "compai-config-recorder". Rollback by calling config-service:StopConfigurationRecorderCommand with ConfigurationRecorderName "compai-config-recorder".', + 'Step 1: Create a service-linked role using iam:CreateServiceLinkedRoleCommand with AWSServiceName set to "config.amazonaws.com" (skip if the role already exists). Step 2: Create a configuration recorder using config-service:PutConfigurationRecorderCommand with ConfigurationRecorder containing name "compai-config-recorder", roleARN set to the Config service role ARN, and recordingGroup set to exactly { allSupported: true, includeGlobalResourceTypes: true }. Do NOT include recordingStrategy, exclusionByResourceTypes, or resourceTypes in recordingGroup — allSupported:true is mutually exclusive with those and AWS rejects the request with a ValidationException. Step 3: Create a delivery channel using config-service:PutDeliveryChannelCommand with DeliveryChannel containing name "compai-delivery-channel" and s3BucketName set to the target bucket. Step 4: Start the recorder using config-service:StartConfigurationRecorderCommand with ConfigurationRecorderName "compai-config-recorder". Rollback by calling config-service:StopConfigurationRecorderCommand with ConfigurationRecorderName "compai-config-recorder".', }), ); return findings; @@ -76,9 +77,9 @@ export class ConfigAdapter implements AwsServiceAdapter { const status = ConfigurationRecordersStatus?.[0]; const recorder = ConfigurationRecorders[0]; const isRecording = status?.recording === true; - const allSupported = recorder?.recordingGroup?.allSupported === true; + const recordsAllSupported = this.recordsAllSupportedResources(recorder); - if (isRecording && allSupported) { + if (isRecording && recordsAllSupported) { findings.push( this.makeFinding({ id: `config-recorder-enabled-${region}`, @@ -98,7 +99,7 @@ export class ConfigAdapter implements AwsServiceAdapter { severity: 'high', resourceId: recorder.name ?? `config-recorder-${region}`, remediation: - 'Use config-service:PutConfigurationRecorderCommand with ConfigurationRecorder containing the existing recorder name, roleARN, and recordingGroup with allSupported set to true. Then call config-service:StartConfigurationRecorderCommand with ConfigurationRecorderName set to the recorder name. Rollback by calling config-service:StopConfigurationRecorderCommand with ConfigurationRecorderName set to the recorder name.', + 'Step 1: Read the existing recorder with config-service:DescribeConfigurationRecordersCommand to get its exact name and roleARN. Step 2: Call config-service:PutConfigurationRecorderCommand with ConfigurationRecorder set to { name: , roleARN: , recordingGroup: { allSupported: true, includeGlobalResourceTypes: true } }. Do NOT include recordingStrategy, exclusionByResourceTypes, or resourceTypes in recordingGroup — allSupported:true is mutually exclusive with those and AWS rejects the request with a ValidationException. This overwrites any exclusion-based recording strategy so all supported resource types (including the global IAM resource types) are recorded. Step 3: Call config-service:StartConfigurationRecorderCommand with ConfigurationRecorderName set to the recorder name. Rollback by calling config-service:StopConfigurationRecorderCommand with ConfigurationRecorderName set to the recorder name.', }), ); } @@ -106,6 +107,29 @@ export class ConfigAdapter implements AwsServiceAdapter { return findings; } + /** + * Whether the recorder captures ALL supported resource types. + * + * AWS Config has two recording models: + * - Legacy: `recordingGroup.allSupported === true`. + * - Current ("Record all resource types with customizable overrides"): + * `recordingGroup.recordingStrategy.useOnly === 'ALL_SUPPORTED_RESOURCE_TYPES'`. + * On this model `allSupported` is typically false, so the legacy-only + * check produced false positives for recorders that DO record everything. + * + * A recorder using `EXCLUSION_BY_RESOURCE_TYPES` (records all-except) or + * `INCLUSION_BY_RESOURCE_TYPES` (records only-listed) does NOT capture + * every supported type, so it stays flagged for remediation. + */ + private recordsAllSupportedResources( + recorder: ConfigurationRecorder | undefined, + ): boolean { + const group = recorder?.recordingGroup; + if (!group) return false; + if (group.allSupported === true) return true; + return group.recordingStrategy?.useOnly === 'ALL_SUPPORTED_RESOURCE_TYPES'; + } + private async checkDeliveryChannels( client: ConfigServiceClient, region: string, diff --git a/apps/api/src/cloud-security/remediation.service.spec.ts b/apps/api/src/cloud-security/remediation.service.spec.ts index 7436e8dcc4..8c23c01baf 100644 --- a/apps/api/src/cloud-security/remediation.service.spec.ts +++ b/apps/api/src/cloud-security/remediation.service.spec.ts @@ -75,3 +75,34 @@ describe('RemediationService.previewRemediation', () => { expect(getDecryptedCredentials).not.toHaveBeenCalled(); }); }); + +describe('RemediationService.isUsablePlan (plan-cache guard)', () => { + const service = makeService(); + const callIsUsable = (plan: unknown): boolean => + ( + service as unknown as { isUsablePlan: (p: unknown) => boolean } + ).isUsablePlan(plan); + + it('treats an empty fix plan as unusable so it is never cached/reused (Retry can regenerate)', () => { + expect(callIsUsable({ canAutoFix: true, fixSteps: [] })).toBe(false); + }); + + it('treats a non-auto-fixable plan as unusable', () => { + expect( + callIsUsable({ canAutoFix: false, fixSteps: [{ command: 'X' }] }), + ).toBe(false); + }); + + it('treats undefined as unusable', () => { + expect(callIsUsable(undefined)).toBe(false); + }); + + it('treats an auto-fixable plan with at least one fix step as usable', () => { + expect( + callIsUsable({ + canAutoFix: true, + fixSteps: [{ command: 'PutConfigurationRecorderCommand' }], + }), + ).toBe(true); + }); +}); diff --git a/apps/api/src/cloud-security/remediation.service.ts b/apps/api/src/cloud-security/remediation.service.ts index 7b7275b605..9a2e760726 100644 --- a/apps/api/src/cloud-security/remediation.service.ts +++ b/apps/api/src/cloud-security/remediation.service.ts @@ -37,6 +37,18 @@ export class RemediationService { private readonly PLAN_CACHE_MAX = 100; private readonly PLAN_CACHE_TTL = 5 * 60 * 1000; + /** + * A plan is only worth caching/reusing if it can actually be auto-applied. + * Caching an empty or non-auto-fixable plan makes "Retry" a guaranteed + * no-op: execute would reload the same dead plan and fail identically, + * never re-running the (non-deterministic) AI generation that might succeed. + */ + private isUsablePlan(plan: FixPlan | undefined): boolean { + return Boolean( + plan?.canAutoFix && plan.fixSteps && plan.fixSteps.length > 0, + ); + } + private evictStalePlans() { if (this.planCache.size <= this.PLAN_CACHE_MAX) return; const now = Date.now(); @@ -350,12 +362,16 @@ export class RemediationService { this.buildStaticPermissionScript(permissionsList); } - // Cache the refined plan + permissions for execute and Recheck - this.evictStalePlans(); - this.planCache.set( - `${params.connectionId}:${params.checkResultId}:${params.remediationKey}`, - { plan: refined, timestamp: Date.now(), permissionsList }, - ); + // Cache the refined plan + permissions for execute and Recheck. + // Never cache an unusable (empty / non-auto-fixable) plan — caching + // one turns "Retry" into a no-op that reloads the same dead plan. + if (this.isUsablePlan(refined)) { + this.evictStalePlans(); + this.planCache.set( + `${params.connectionId}:${params.checkResultId}:${params.remediationKey}`, + { plan: refined, timestamp: Date.now(), permissionsList }, + ); + } return { currentState: refined.currentState, @@ -381,16 +397,19 @@ export class RemediationService { } } - // Fallback: show initial AI plan without real data - this.evictStalePlans(); - this.planCache.set( - `${params.connectionId}:${params.checkResultId}:${params.remediationKey}`, - { - plan, - timestamp: Date.now(), - permissionsList: plan.requiredPermissions, - }, - ); + // Fallback: show initial AI plan without real data. Only cache it when + // usable — caching an empty/non-auto-fixable plan makes Retry a no-op. + if (this.isUsablePlan(plan)) { + this.evictStalePlans(); + this.planCache.set( + `${params.connectionId}:${params.checkResultId}:${params.remediationKey}`, + { + plan, + timestamp: Date.now(), + permissionsList: plan.requiredPermissions, + }, + ); + } return { currentState: plan.currentState, @@ -440,12 +459,21 @@ export class RemediationService { // Get plan from cache or regenerate let plan: FixPlan; - const cached = this.planCache.get( - `${params.connectionId}:${params.checkResultId}:${params.remediationKey}`, - ); - if (cached && Date.now() - cached.timestamp < 5 * 60 * 1000) { + const cacheKey = `${params.connectionId}:${params.checkResultId}:${params.remediationKey}`; + const cached = this.planCache.get(cacheKey); + // Only reuse a cached plan if it is still fresh AND usable. Reusing a + // stale empty / non-auto-fixable plan is exactly what made "Retry" a + // no-op — execute reloaded the same dead plan and failed identically. + // Falling through (and dropping the dead entry) regenerates a fresh plan, + // which is what gives Retry a chance to succeed. + if ( + cached && + Date.now() - cached.timestamp < this.PLAN_CACHE_TTL && + this.isUsablePlan(cached.plan) + ) { plan = cached.plan; } else { + this.planCache.delete(cacheKey); const evidence = (finding.evidence ?? {}) as Record; plan = await this.aiRemediationService.generateFixPlan({ title: finding.title ?? 'Unknown', diff --git a/apps/app/src/app/(app)/[orgId]/cloud-tests/components/RemediationDialog.tsx b/apps/app/src/app/(app)/[orgId]/cloud-tests/components/RemediationDialog.tsx index f847545787..03d2dcced8 100644 --- a/apps/app/src/app/(app)/[orgId]/cloud-tests/components/RemediationDialog.tsx +++ b/apps/app/src/app/(app)/[orgId]/cloud-tests/components/RemediationDialog.tsx @@ -512,6 +512,11 @@ export function RemediationDialog({ }; const isGuided = preview?.guidedOnly; + // An auto-fix plan with no API calls has nothing to apply (the AI returned + // an empty/unusable plan). Block Apply so the user can't submit a no-op that + // fails server-side — they can Cancel and re-open to regenerate. + const hasNothingToApply = + !isGuided && (preview?.apiCalls?.length ?? 0) === 0; return ( @@ -766,6 +771,14 @@ export function RemediationDialog({ )} + {hasNothingToApply && ( +

+ We couldn't build an automatic fix for this finding. + Close this dialog and try again, or follow the manual + remediation guidance for this finding. +

+ )} +