cherry-pick (4.12): Fix double evaluation and filter sensitive settin…

…gs (#3746) * init * init * move on * drop unchecked code
microsoft · Jun 15, 2021 · f3cf2ed · f3cf2ed
1 parent 8f903a6
commit f3cf2ed
Show file tree

Hide file tree

Showing 12 changed files with 128 additions and 45 deletions.
diff --git a/libraries/botbuilder-core/src/activityFactory.ts b/libraries/botbuilder-core/src/activityFactory.ts
@@ -118,6 +118,10 @@ export class ActivityFactory {
      * @param lgResult string result from languageGenerator.
      */
     public static fromObject(lgResult: any): Partial<Activity> {
+        if (lgResult == null) {
+            return { type: ActivityTypes.Message };
+        }
+
         if (typeof lgResult === 'string') {
             return this.buildActivityFromText(lgResult.trim());
         }

diff --git a/...ilder-dialogs-adaptive-testing/tests/resources/ActionTests/Action_SetProperty.test.dialog b/...ilder-dialogs-adaptive-testing/tests/resources/ActionTests/Action_SetProperty.test.dialog
@@ -87,6 +87,20 @@
           {
             "$kind": "Microsoft.SendActivity",
             "activity": "${user.profile.name}"
+          },
+          {
+            "$kind": "Microsoft.TextInput",
+            "property": "user.profile.newname",
+            "prompt": "Please input the name"
+          },
+          {
+            "$kind": "Microsoft.SetProperty",
+            "property": "user.profile.result",
+            "value": "=user.profile.newname"
+          },
+          {
+            "$kind": "Microsoft.SendActivity",
+            "activity": "${user.profile.result}"
           }
         ]
       }
@@ -111,6 +125,18 @@
     {
       "$kind": "Microsoft.Test.AssertReply",
       "text": "Jack"
+    },
+    {
+      "$kind": "Microsoft.Test.AssertReply",
+      "text": "Please input the name"
+    },
+    {
+      "$kind": "Microsoft.Test.UserSays",
+      "text": "=cool"
+    },
+    {
+      "$kind": "Microsoft.Test.AssertReply",
+      "text": "=cool"
     }
   ]
 }
diff --git a/...ve-testing/tests/resources/SettingsStateTests/SettingsStateTests_SettingsTest.test.dialog b/...ve-testing/tests/resources/SettingsStateTests/SettingsStateTests_SettingsTest.test.dialog
@@ -13,11 +13,11 @@
                     },
                     {
                         "$kind": "Microsoft.SendActivity",
-                        "activity": "${settings.MicrosoftAppPassword}"
+                        "activity": "${coalesce(settings.MicrosoftAppPassword, 'no-access')}"
                     },
                     {
                         "$kind": "Microsoft.SendActivity",
-                        "activity": "${settings.ApplicationInsightsInstrumentationKey}"
+                        "activity": "${coalesce(settings.ApplicationInsights.InstrumentationKey, 'no-access')}"
                     }
                 ]
             }
@@ -36,11 +36,11 @@
         },
         {
             "$kind": "Microsoft.Test.AssertReply",
-            "text": "MICROSOFT_APP_PASSWORD"
+            "text": "no-access"
         },
         {
             "$kind": "Microsoft.Test.AssertReply",
-            "text": "00000000-0000-0000-0000-000000000000"
+            "text": "no-access"
         }
     ]
-}
+}
diff --git a/libraries/botbuilder-dialogs-adaptive-testing/tests/settingsState.test.js b/libraries/botbuilder-dialogs-adaptive-testing/tests/settingsState.test.js
@@ -15,9 +15,18 @@ describe('SettingsStateTests', function () {
         true,
         false
     );
-    process.env['MicrosoftAppId'] = 'MICROSOFT_APP_ID';
-    process.env['MicrosoftAppPassword'] = 'MICROSOFT_APP_PASSWORD';
-    process.env['ApplicationInsightsInstrumentationKey'] = '00000000-0000-0000-0000-000000000000';
+
+    beforeEach(() => {
+        process.env['MicrosoftAppId'] = 'MICROSOFT_APP_ID';
+        process.env['MicrosoftAppPassword'] = 'MICROSOFT_APP_PASSWORD';
+        process.env['ApplicationInsights:InstrumentationKey'] = '00000000-0000-0000-0000-000000000000';
+    });
+
+    afterEach(() => {
+        delete process.env['MicrosoftAppId'];
+        delete process.env['MicrosoftAppPassword'];
+        delete process.env['ApplicationInsights:InstrumentationKey'];
+    });
 
     it('SettingsTest', async () => {
         await TestUtils.runTestScript(resourceExplorer, 'SettingsStateTests_SettingsTest');

diff --git a/libraries/botbuilder-dialogs-adaptive/src/actions/baseInvokeDialog.ts b/libraries/botbuilder-dialogs-adaptive/src/actions/baseInvokeDialog.ts
@@ -23,7 +23,7 @@ import {
     DialogConfiguration,
 } from 'botbuilder-dialogs';
 import { DialogExpression } from '../expressions';
-import { replaceJsonRecursively } from '../jsonExtensions';
+import { evaluateExpression } from '../jsonExtensions';
 import { DialogExpressionConverter } from '../converters';
 
 export interface BaseInvokeDialogConfiguration extends DialogConfiguration {
@@ -140,13 +140,7 @@ export class BaseInvokeDialog<O extends object = {}>
 
         for (const key in bindingOptions) {
             const bindingValue = bindingOptions[key];
-            let value = new ValueExpression(bindingValue).getValue(dc.state);
-
-            if (value) {
-                value = replaceJsonRecursively(dc.state, value);
-            }
-
-            boundOptions[key] = value;
+            boundOptions[key] = evaluateExpression(dc.state, new ValueExpression(bindingValue));
         }
 
         return boundOptions;

diff --git a/libraries/botbuilder-dialogs-adaptive/src/actions/endDialog.ts b/libraries/botbuilder-dialogs-adaptive/src/actions/endDialog.ts
@@ -20,7 +20,7 @@ import {
     DialogContext,
     DialogTurnResult,
 } from 'botbuilder-dialogs';
-import { replaceJsonRecursively } from '../jsonExtensions';
+import { evaluateExpression } from '../jsonExtensions';
 
 export interface EndDialogConfiguration extends DialogConfiguration {
     value?: unknown | ValueExpression;
@@ -77,11 +77,7 @@ export class EndDialog<O extends object = {}> extends Dialog<O> implements EndDi
         }
 
         if (this.value) {
-            let value = this.value.getValue(dc.state);
-
-            if (value) {
-                value = replaceJsonRecursively(dc.state, value);
-            }
+            const value = evaluateExpression(dc.state, this.value);
 
             return await this.endParentDialog(dc, value);
         }
@@ -92,7 +88,7 @@ export class EndDialog<O extends object = {}> extends Dialog<O> implements EndDi
     /**
      * Ends the parent [Dialog](xref:botbuilder-dialogs.Dialog).
      * @param dc The [DialogContext](xref:botbuilder-dialogs.DialogContext) for the current turn of conversation.
-     * @param result Optional. Value returned from the dialog that was called. The type 
+     * @param result Optional. Value returned from the dialog that was called. The type
      * of the value returned is dependent on the child dialog.
      * @returns A `Promise` representing the asynchronous operation.
      */

diff --git a/libraries/botbuilder-dialogs-adaptive/src/actions/httpRequest.ts b/libraries/botbuilder-dialogs-adaptive/src/actions/httpRequest.ts
@@ -27,7 +27,7 @@ import {
     DialogTurnResult,
     DialogConfiguration,
 } from 'botbuilder-dialogs';
-import { replaceJsonRecursively } from '../jsonExtensions';
+import { evaluateExpression } from '../jsonExtensions';
 
 type HeadersInput = Record<string, string>;
 type HeadersOutput = Record<string, StringExpression>;
@@ -275,14 +275,12 @@ export class HttpRequest<O extends object = {}> extends Dialog<O> implements Htt
         instanceHeaders['Content-Type'] = contentType;
 
         let instanceBody: string;
-        if (this.body) {
-            const body = this.body.getValue(dc.state);
-            if (body) {
-                if (typeof body === 'string') {
-                    instanceBody = body;
-                } else {
-                    instanceBody = JSON.stringify(replaceJsonRecursively(dc.state, Object.assign({}, body)));
-                }
+        const body = evaluateExpression(dc.state, this.body);
+        if (body) {
+            if (typeof body === 'string') {
+                instanceBody = body;
+            } else {
+                instanceBody = JSON.stringify(Object.assign({}, body));
             }
         }
 

diff --git a/libraries/botbuilder-dialogs-adaptive/src/actions/setProperties.ts b/libraries/botbuilder-dialogs-adaptive/src/actions/setProperties.ts
@@ -21,7 +21,7 @@ import {
     DialogContext,
     DialogTurnResult,
 } from 'botbuilder-dialogs';
-import { replaceJsonRecursively } from '../jsonExtensions';
+import { evaluateExpression } from '../jsonExtensions';
 
 type AssignmentInput<T> = {
     property: string;
@@ -105,11 +105,8 @@ export class SetProperties<O extends object = {}> extends Dialog<O> implements S
 
         for (let i = 0; i < this.assignments.length; i++) {
             const assignment = this.assignments[i];
-            let value = assignment.value.getValue(dc.state);
 
-            if (value) {
-                value = replaceJsonRecursively(dc.state, value);
-            }
+            const value = evaluateExpression(dc.state, assignment.value);
 
             const property = assignment.property.getValue(dc.state);
             dc.state.setValue(property, value);

diff --git a/libraries/botbuilder-dialogs-adaptive/src/actions/setProperty.ts b/libraries/botbuilder-dialogs-adaptive/src/actions/setProperty.ts
@@ -22,7 +22,7 @@ import {
     DialogContext,
     DialogTurnResult,
 } from 'botbuilder-dialogs';
-import { replaceJsonRecursively } from '../jsonExtensions';
+import { evaluateExpression } from '../jsonExtensions';
 
 export interface SetPropertyConfiguration extends DialogConfiguration {
     property?: string | Expression | StringExpression;
@@ -106,13 +106,8 @@ export class SetProperty<O extends object = {}> extends Dialog<O> implements Set
             throw new Error(`${this.id}: no 'value' expression specified.`);
         }
 
-        // Evaluate expression and save value
         const property = this.property.getValue(dc.state);
-        let value = this.value.getValue(dc.state);
-
-        if (value) {
-            value = replaceJsonRecursively(dc.state, value);
-        }
+        const value = evaluateExpression(dc.state, this.value);
 
         dc.state.setValue(property, value);
 

diff --git a/libraries/botbuilder-dialogs-adaptive/src/jsonExtensions.ts b/libraries/botbuilder-dialogs-adaptive/src/jsonExtensions.ts
@@ -42,3 +42,13 @@ export function replaceJsonRecursively(state: DialogStateManager, unit: object):
 
     return unit;
 }
+
+/**
+ * Evaluate ValueExpression according the value type.
+ * @param state Input ValueExpression
+ * @param valExpr A scope for looking up variables.
+ * @returns Deep data binding result.
+ */
+export function evaluateExpression(state: DialogStateManager, valExpr: ValueExpression): any {
+    return valExpr.expressionText == null ? replaceJsonRecursively(state, valExpr.value) : valExpr.getValue(state);
+}
diff --git a/libraries/botbuilder-dialogs/src/memory/scopes/settingsMemoryScope.ts b/libraries/botbuilder-dialogs/src/memory/scopes/settingsMemoryScope.ts
@@ -40,6 +40,19 @@ class Node {
  * SettingsMemoryScope maps "settings" -> dc.context.turnState['settings']
  */
 export class SettingsMemoryScope extends MemoryScope {
+    private static readonly blockingList = [
+        'MicrosoftAppPassword',
+        'cosmosDb:authKey',
+        'blobStorage:connectionString',
+        'BlobsStorage:connectionString',
+        'CosmosDbPartitionedStorage:authKey',
+        'applicationInsights:connectionString',
+        'applicationInsights:InstrumentationKey',
+        'runtimeSettings:telemetry:options:connectionString',
+        'runtimeSettings:telemetry:options:instrumentationKey',
+        'runtimeSettings:features:blobTranscript:connectionString',
+    ];
+
     /**
      * Initializes a new instance of the [SettingsMemoryScope](xref:botbuilder-dialogs.SettingsMemoryScope) class.
      */
@@ -84,7 +97,9 @@ export class SettingsMemoryScope extends MemoryScope {
                 return result;
             }, settings);
         }
-        return settings;
+
+        // filter env configuration settings
+        return this.filterSettings(settings);
     }
 
     /**
@@ -189,4 +204,31 @@ export class SettingsMemoryScope extends MemoryScope {
             return result;
         }, {});
     }
+
+    private static filterSettings(settings: Record<string, unknown>): Record<string, unknown> {
+        const result = Object.assign({}, settings);
+        this.blockingList.forEach((path) => this.deletePropertyPath(result, path));
+        return result;
+    }
+
+    private static deletePropertyPath(obj, path: string): void {
+        if (!obj || !path?.length) {
+            return;
+        }
+
+        const pathArray = path.split(':');
+
+        for (let i = 0; i < pathArray.length - 1; i++) {
+            const realKey = Object.keys(obj).find((key) => key.toLowerCase() === pathArray[i].toLowerCase());
+            obj = obj[realKey];
+
+            if (typeof obj === 'undefined') {
+                return;
+            }
+        }
+
+        const lastPath = pathArray.pop().toLowerCase();
+        const lastKey = Object.keys(obj).find((key) => key.toLowerCase() === lastPath);
+        delete obj[lastKey];
+    }
 }
diff --git a/libraries/botbuilder-dialogs/tests/memory_memoryScopes.test.js b/libraries/botbuilder-dialogs/tests/memory_memoryScopes.test.js
@@ -509,6 +509,9 @@ describe('Memory - Memory Scopes', function () {
             'array:1': 'two',
             'object:array:0': 'one',
             'object:array:1': 'two',
+            MicrosoftAppPassword: 'testpassword',
+            'runtimeSettings:telemetry:options:connectionString': 'testConnectionString',
+            'BlobsStorage:CONNECTIONSTRING': 'testConnectionString',
         };
         dc.context.turnState.set(DialogTurnStateConstants.configuration, configuration);
 
@@ -524,6 +527,9 @@ describe('Memory - Memory Scopes', function () {
         assert(Array.isArray(memory.object.array));
         assert.strictEqual(memory.object.array[0], 'one');
         assert.strictEqual(memory.object.array[1], 'two');
+        assert.strictEqual(memory.MicrosoftAppPassword, undefined);
+        assert.strictEqual(memory.runtimeSettings.telemetry.options.connectionString, undefined);
+        assert.strictEqual(memory.BlobsStorage.CONNECTIONSTRING, undefined);
     });
 
     it('SettingsMemoryScope should get settings from configuration and environment variables.', async function () {
@@ -546,6 +552,9 @@ describe('Memory - Memory Scopes', function () {
         };
         dc.context.turnState.set(DialogTurnStateConstants.configuration, configuration);
         process.env['to_be_overridden'] = 'two';
+        process.env['MicrosoftAppPassword'] = 'testpassword';
+        process.env['runtimeSettings:telemetry:options:connectionString'] = 'testConnectionString';
+        process.env['BlobsStorage:CONNECTIONSTRING'] = 'testConnectionString';
 
         // Run test
         const scope = new SettingsMemoryScope();
@@ -560,6 +569,9 @@ describe('Memory - Memory Scopes', function () {
         assert.strictEqual(memory.object.array[0], 'one');
         assert.strictEqual(memory.object.array[1], 'two');
         assert.strictEqual(memory.to_be_overridden, 'two');
+        assert.strictEqual(memory.MicrosoftAppPassword, undefined);
+        assert.strictEqual(memory.runtimeSettings.telemetry.options.connectionString, undefined);
+        assert.strictEqual(memory.BlobsStorage.CONNECTIONSTRING, undefined);
     });
 
     it('ThisMemoryScope should return active dialogs state.', async function () {