From 0e007425250a4e669516dd539c5e46507c6ba041 Mon Sep 17 00:00:00 2001 From: Tai Lai Date: Fri, 30 Aug 2024 15:45:25 -0700 Subject: [PATCH] fix(amazonq): update security scan languages (#5523) Sometimes security issues have a Diagnostic but not SecurityIssueHover issue. This happens when the document's languageId is not enabled when registering the hover/code actions provider. Update the enabled languageIds for security scans feature. --- License: I confirm that my contribution is made under the terms of the Apache 2.0 license. --- ...-e18d0bfd-d423-4c64-9454-b221eff45b7f.json | 4 ++ .../util/securityScanLanguageContext.test.ts | 12 ++++ packages/core/src/codewhisperer/activation.ts | 4 +- .../src/codewhisperer/models/constants.ts | 6 ++ .../util/securityScanLanguageContext.ts | 6 ++ packages/core/src/shared/featureConfig.ts | 31 +++++++- .../src/test/fake/mockFeatureConfigData.ts | 10 +++ .../src/test/shared/featureConfig.test.ts | 72 +++++++++++++++---- 8 files changed, 129 insertions(+), 16 deletions(-) create mode 100644 packages/amazonq/.changes/next-release/Bug Fix-e18d0bfd-d423-4c64-9454-b221eff45b7f.json diff --git a/packages/amazonq/.changes/next-release/Bug Fix-e18d0bfd-d423-4c64-9454-b221eff45b7f.json b/packages/amazonq/.changes/next-release/Bug Fix-e18d0bfd-d423-4c64-9454-b221eff45b7f.json new file mode 100644 index 00000000000..d3cc074daf8 --- /dev/null +++ b/packages/amazonq/.changes/next-release/Bug Fix-e18d0bfd-d423-4c64-9454-b221eff45b7f.json @@ -0,0 +1,4 @@ +{ + "type": "Bug Fix", + "description": "Add getFeature and isEnabled utility methods to FeatureConfigProvider" +} diff --git a/packages/amazonq/test/unit/codewhisperer/util/securityScanLanguageContext.test.ts b/packages/amazonq/test/unit/codewhisperer/util/securityScanLanguageContext.test.ts index 335e99e9e63..71ece8c66a9 100644 --- a/packages/amazonq/test/unit/codewhisperer/util/securityScanLanguageContext.test.ts +++ b/packages/amazonq/test/unit/codewhisperer/util/securityScanLanguageContext.test.ts @@ -39,6 +39,12 @@ describe('securityScanLanguageContext', function () { ['html', false], ['r', false], ['vb', false], + ['xml', false], + ['toml', false], + ['pip-requirements', false], + ['java-properties', false], + ['go.mod', false], + ['go.sum', false], ] beforeEach(async function () { @@ -100,6 +106,12 @@ describe('securityScanLanguageContext', function () { ['packer', 'tf'], ['plaintext', 'plaintext'], ['jsonc', 'json'], + ['xml', 'plaintext'], + ['toml', 'plaintext'], + ['pip-requirements', 'plaintext'], + ['java-properties', 'plaintext'], + ['go.mod', 'plaintext'], + ['go.sum', 'plaintext'], ] for (const [securityScanLanguageId, expectedCwsprLanguageId] of securityScanLanguageIds) { diff --git a/packages/core/src/codewhisperer/activation.ts b/packages/core/src/codewhisperer/activation.ts index d3ec18623a7..9d43ab9421a 100644 --- a/packages/core/src/codewhisperer/activation.ts +++ b/packages/core/src/codewhisperer/activation.ts @@ -283,11 +283,11 @@ export async function activate(context: ExtContext): Promise { ImportAdderProvider.instance ), vscode.languages.registerHoverProvider( - [...CodeWhispererConstants.platformLanguageIds], + [...CodeWhispererConstants.securityScanLanguageIds], SecurityIssueHoverProvider.instance ), vscode.languages.registerCodeActionsProvider( - [...CodeWhispererConstants.platformLanguageIds], + [...CodeWhispererConstants.securityScanLanguageIds], SecurityIssueCodeActionProvider.instance ), vscode.commands.registerCommand('aws.amazonq.openEditorAtRange', openEditorAtRange) diff --git a/packages/core/src/codewhisperer/models/constants.ts b/packages/core/src/codewhisperer/models/constants.ts index c1a9a5445a4..89f19609e7f 100644 --- a/packages/core/src/codewhisperer/models/constants.ts +++ b/packages/core/src/codewhisperer/models/constants.ts @@ -258,6 +258,12 @@ export const securityScanLanguageIds = [ 'c', 'cpp', 'php', + 'xml', + 'toml', + 'pip-requirements', + 'java-properties', + 'go.mod', + 'go.sum', ] as const export type SecurityScanLanguageId = (typeof securityScanLanguageIds)[number] diff --git a/packages/core/src/codewhisperer/util/securityScanLanguageContext.ts b/packages/core/src/codewhisperer/util/securityScanLanguageContext.ts index 9a65e77a88a..466a558a2eb 100644 --- a/packages/core/src/codewhisperer/util/securityScanLanguageContext.ts +++ b/packages/core/src/codewhisperer/util/securityScanLanguageContext.ts @@ -34,6 +34,12 @@ export class SecurityScanLanguageContext { c: 'c', cpp: 'cpp', php: 'php', + xml: 'plaintext', // xml does not exist in CodewhispererLanguage + toml: 'plaintext', + 'pip-requirements': 'plaintext', + 'java-properties': 'plaintext', + 'go.mod': 'plaintext', + 'go.sum': 'plaintext', }) } diff --git a/packages/core/src/shared/featureConfig.ts b/packages/core/src/shared/featureConfig.ts index 9f5ef487f78..c596713fc58 100644 --- a/packages/core/src/shared/featureConfig.ts +++ b/packages/core/src/shared/featureConfig.ts @@ -32,8 +32,11 @@ const customizationArnOverrideName = 'customizationArnOverride' const featureConfigPollIntervalInMs = 30 * 60 * 1000 // 30 mins const dataCollectionFeatureName = 'IDEProjectContextDataCollection' +const featureNames = [testFeatureName, customizationArnOverrideName, dataCollectionFeatureName] as const +export type FeatureName = (typeof featureNames)[number] + // TODO: add real feature later -export const featureDefinitions = new Map([ +export const featureDefinitions = new Map([ [testFeatureName, new FeatureContext(testFeatureName, 'CONTROL', { stringValue: 'testValue' })], [ customizationArnOverrideName, @@ -181,7 +184,7 @@ export class FeatureConfigProvider { // Get the feature value for the given key. // In case of a misconfiguration, it will return a default feature value of Boolean true. - private getFeatureValueForKey(name: string): FeatureValue { + private getFeatureValueForKey(name: FeatureName): FeatureValue { return this.featureConfigs.get(name)?.value ?? featureDefinitions.get(name)?.value ?? { boolValue: true } } @@ -193,4 +196,28 @@ export class FeatureConfigProvider { public static getFeatureConfigs(): Map { return FeatureConfigProvider.instance.featureConfigs } + + /** + * Retrieves the FeatureContext object for a given feature name. + * + * @param {string} featureName - The name of the feature. + * @returns {FeatureContext | undefined} The FeatureContext object for the specified feature, or undefined if the feature doesn't exist. + */ + public static getFeature(featureName: FeatureName): FeatureContext | undefined { + return FeatureConfigProvider.instance.featureConfigs.get(featureName) + } + + /** + * Checks if a feature is active or not. + * + * @param {string} featureName - The name of the feature to check. + * @returns {boolean} False if the variation is not CONTROL, otherwise True + */ + public static isEnabled(featureName: FeatureName): boolean { + const featureContext = FeatureConfigProvider.getFeature(featureName) + if (featureContext && featureContext.variation.toLocaleLowerCase() !== 'control') { + return true + } + return false + } } diff --git a/packages/core/src/test/fake/mockFeatureConfigData.ts b/packages/core/src/test/fake/mockFeatureConfigData.ts index b75887f4481..52e1bdb41af 100644 --- a/packages/core/src/test/fake/mockFeatureConfigData.ts +++ b/packages/core/src/test/fake/mockFeatureConfigData.ts @@ -11,4 +11,14 @@ export const mockFeatureConfigsData: FeatureEvaluation[] = [ variation: 'TREATMENT', value: { stringValue: 'testValue' }, }, + { + feature: 'featureA', + variation: 'CONTROL', + value: { stringValue: 'testValue' }, + }, + { + feature: 'featureB', + variation: 'TREATMENT', + value: { stringValue: 'testValue' }, + }, ] diff --git a/packages/core/src/test/shared/featureConfig.test.ts b/packages/core/src/test/shared/featureConfig.test.ts index 5c9df0abb0b..1431099de8b 100644 --- a/packages/core/src/test/shared/featureConfig.test.ts +++ b/packages/core/src/test/shared/featureConfig.test.ts @@ -6,12 +6,26 @@ import assert from 'assert' import sinon from 'sinon' import { AWSError, Request } from 'aws-sdk' -import { FeatureConfigProvider, featureDefinitions } from '../../shared/featureConfig' +import { FeatureConfigProvider, featureDefinitions, FeatureName } from '../../shared/featureConfig' import { ListFeatureEvaluationsResponse } from '../../codewhisperer' import { createSpyClient } from '../codewhisperer/testUtil' import { mockFeatureConfigsData } from '../fake/mockFeatureConfigData' describe('FeatureConfigProvider', () => { + beforeEach(async () => { + const clientSpy = await createSpyClient() + sinon.stub(clientSpy, 'listFeatureEvaluations').returns({ + promise: () => + Promise.resolve({ + $response: { + requestId: '', + }, + featureEvaluations: mockFeatureConfigsData, + }), + } as Request) + await FeatureConfigProvider.instance.fetchFeatureConfigs() + }) + afterEach(function () { sinon.restore() }) @@ -32,18 +46,52 @@ describe('FeatureConfigProvider', () => { }) it('test getFeatureConfigsTelemetry will return expected string', async () => { - const clientSpy = await createSpyClient() - sinon.stub(clientSpy, 'listFeatureEvaluations').returns({ - promise: () => - Promise.resolve({ - $response: { - requestId: '', + assert.strictEqual( + FeatureConfigProvider.instance.getFeatureConfigsTelemetry(), + `{testFeature: TREATMENT, featureA: CONTROL, featureB: TREATMENT}` + ) + }) + + it('should should return all feature flags', async () => { + it('should should return all feature flags', async () => { + const featureConfigs = FeatureConfigProvider.getFeatureConfigs() + const expectedFeatureConfigs = { + featureA: { + name: 'featureA', + value: { + stringValue: 'testValue', }, - featureEvaluations: mockFeatureConfigsData, - }), - } as Request) + variation: 'CONTROL', + }, + featureB: { + name: 'featureB', + value: { + stringValue: 'testValue', + }, + variation: 'TREATMENT', + }, + testFeature: { + name: 'testFeature', + value: { + stringValue: 'testValue', + }, + variation: 'TREATMENT', + }, + } - await FeatureConfigProvider.instance.fetchFeatureConfigs() - assert.strictEqual(FeatureConfigProvider.instance.getFeatureConfigsTelemetry(), `{testFeature: TREATMENT}`) + assert.deepStrictEqual(Object.fromEntries(featureConfigs), expectedFeatureConfigs) + }) + }) + + it('should test featureA as disabled', async () => { + assert.strictEqual(FeatureConfigProvider.isEnabled('featureA' as FeatureName), false) + }) + + it('should test featureB as enabled', async () => { + assert.strictEqual(FeatureConfigProvider.isEnabled('featureB' as FeatureName), true) + }) + + it('should test feature-does-not-exist as disabled', async () => { + assert.strictEqual(FeatureConfigProvider.isEnabled('feature-does-not-exist' as FeatureName), false) }) })