fix: Addons should support sensitive params

.github/workflows/build.yaml

@@ -37,6 +37,7 @@ jobs:
       with:
         node-version: ${{ matrix.node-version }}
     - run: yarn
+    - run: yarn lint
     - run: yarn run test:coverage
       env:
         CI: true

lib/addons/addon-schema.js

@@ -18,6 +18,7 @@ const addonDefinitionSchema = joi.object().keys({
                 description: joi.string(),
                 placeholder: joi.string().allow(''),
                 required: joi.boolean().default(false),
+                sensitive: joi.boolean().default(false),
             }),
         ),
     events: joi

lib/addons/jira-comment-definition.js

@@ -24,6 +24,7 @@ module.exports = {
                 'Add a new key at https://id.atlassian.com/manage-profile/security/api-tokens when logged in as the user you want Unleash to use',
             type: 'text',
             required: true,
+            sensitive: true,
         },
         {
             name: 'user',

lib/addons/slack-definition.js

@@ -18,6 +18,7 @@ module.exports = {
             displayName: 'Slack webhook URL',
             type: 'url',
             required: true,
+            sensitive: true,
         },
         {
             name: 'username',

lib/addons/webhook-definition.js

@@ -19,6 +19,7 @@ module.exports = {
                 '(Required) Unleash will perform a HTTP Post to the specified URL (one retry if first attempt fails)',
             type: 'url',
             required: true,
+            sensitive: true,
         },
         {
             name: 'contentType',

lib/services/addon-service.js

@@ -10,6 +10,8 @@ const SUPPORTED_EVENTS = Object.keys(events).map(k => events[k]);
 
 const ADDONS_CACHE_TIME = 60 * 1000; // 60s
 
+const MASKED_VALUE = '*****';
+
 class AddonService {
     constructor(
         { addonStore, eventStore, featureToggleStore },
@@ -23,7 +25,21 @@ class AddonService {
         this.logger = getLogger('services/addon-service.js');
         this.tagTypeService = tagTypeService;
 
-        this.addonProviders = addonProvidersClasses.reduce((map, Provider) => {
+        this.addonProviders = this.loadProviders(getLogger);
+        this.sensitiveParams = this.loadSensitiveParams(this.addonProviders);
+        if (addonStore) {
+            this.registerEventHandler();
+        }
+
+        // Memoized private function
+        this.fetchAddonConfigs = memoize(
+            () => addonStore.getAll({ enabled: true }),
+            { promise: true, maxAge: ADDONS_CACHE_TIME },
+        );
+    }
+
+    loadProviders(getLogger) {
+        return addonProvidersClasses.reduce((map, Provider) => {
             try {
                 const provider = new Provider({ getLogger });
                 // eslint-disable-next-line no-param-reassign
@@ -33,15 +49,21 @@ class AddonService {
             }
             return map;
         }, {});
-        if (addonStore) {
+    }
-            this.registerEventHandler();
-        }
 
-        // Memoized function
+    loadSensitiveParams(addonProviders) {
-        this.fetchAddonConfigs = memoize(
+        const providerDefinitions = Object.values(addonProviders).map(
-            () => addonStore.getAll({ enabled: true }),
+            p => p.definition,
-            { promise: true, maxAge: ADDONS_CACHE_TIME },
         );
+        return providerDefinitions.reduce((obj, definition) => {
+            const sensitiveParams = definition.parameters
+                .filter(p => p.sensitive)
+                .map(p => p.name);
+
+            const o = { ...obj };
+            o[definition.name] = sensitiveParams;
+            return o;
+        }, {});
     }
 
     registerEventHandler() {
@@ -67,12 +89,31 @@ class AddonService {
         };
     }
 
+    // Should be used by the controller.
     async getAddons() {
-        return this.addonStore.getAll();
+        const addonConfigs = await this.addonStore.getAll();
+        return addonConfigs.map(a => this.filterSensitiveFields(a));
+    }
+
+    filterSensitiveFields(addonConfig) {
+        const { sensitiveParams } = this;
+        const a = { ...addonConfig };
+        a.parameters = Object.keys(a.parameters).reduce((obj, paramKey) => {
+            const o = { ...obj };
+            if (sensitiveParams[a.provider].includes(paramKey)) {
+                o[paramKey] = MASKED_VALUE;
+            } else {
+                o[paramKey] = a.parameters[paramKey];
+            }
+
+            return o;
+        }, {});
+        return a;
     }
 
     async getAddon(id) {
-        return this.addonStore.get(id);
+        const addonConfig = await this.addonStore.get(id);
+        return this.filterSensitiveFields(addonConfig);
     }
 
     getProviderDefinition() {
@@ -122,6 +163,21 @@ class AddonService {
 
     async updateAddon(id, data, userName) {
         const addonConfig = await addonSchema.validateAsync(data);
+        if (this.sensitiveParams[addonConfig.provider].length > 0) {
+            const existingConfig = await this.addonStore.get(id);
+            addonConfig.parameters = Object.keys(addonConfig.parameters).reduce(
+                (params, key) => {
+                    const o = { ...params };
+                    if (addonConfig.parameters[key] === MASKED_VALUE) {
+                        o[key] = existingConfig.parameters[key];
+                    } else {
+                        o[key] = addonConfig.parameters[key];
+                    }
+                    return o;
+                },
+                {},
+            );
+        }
         await this.addonStore.update(id, addonConfig);
         await this.eventStore.store({
             type: events.ADDON_CONFIG_UPDATED,

lib/services/addon-service.test.js

@@ -18,6 +18,8 @@ const {
     ADDON_CONFIG_DELETED,
 } = require('../event-type');
 
+const MASKED_VALUE = '*****';
+
 const definition = {
     name: 'simple',
     displayName: 'Simple ADdon',
@@ -36,6 +38,14 @@ const definition = {
             type: 'text',
             required: false,
         },
+        {
+            name: 'sensitiveParam',
+            displayName: 'Some sensitive param',
+            description: 'Some variable to inject',
+            type: 'text',
+            required: false,
+            sensitive: true,
+        },
     ],
     events: [
         FEATURE_CREATED,
@@ -259,3 +269,53 @@ test('should store ADDON_CONFIG_REMOVE event', async t => {
     t.is(events[2].type, ADDON_CONFIG_DELETED);
     t.is(events[2].data.id, addonConfig.id);
 });
+
+test('should hide sensitive fields when fetching', async t => {
+    const { addonService } = getSetup();
+
+    const config = {
+        provider: 'simple',
+        enabled: true,
+        parameters: {
+            url: 'http://localhost/wh',
+            var: 'some-value',
+            sensitiveParam: 'should be hidden when fetching',
+        },
+        events: [FEATURE_CREATED],
+    };
+
+    const createdConfig = await addonService.createAddon(config, 'me@mail.com');
+    const addons = await addonService.getAddons();
+    const addonRetrieved = await addonService.getAddon(createdConfig.id);
+
+    t.is(addons.length, 1);
+    t.is(addons[0].parameters.sensitiveParam, MASKED_VALUE);
+    t.is(addonRetrieved.parameters.sensitiveParam, MASKED_VALUE);
+});
+
+test('should not overwrite masked values when updating', async t => {
+    const { addonService, stores } = getSetup();
+
+    const config = {
+        provider: 'simple',
+        enabled: true,
+        parameters: {
+            url: 'http://localhost/wh',
+            var: 'some-value',
+        },
+        events: [FEATURE_CREATED],
+    };
+
+    const addonConfig = await addonService.createAddon(config, 'me@mail.com');
+
+    const updated = {
+        ...addonConfig,
+        parameters: { url: MASKED_VALUE, var: 'some-new-value' },
+        description: 'test',
+    };
+    await addonService.updateAddon(addonConfig.id, updated, 'me@mail.com');
+
+    const updatedConfig = await stores.addonStore.get(addonConfig.id);
+    t.is(updatedConfig.parameters.url, 'http://localhost/wh');
+    t.is(updatedConfig.parameters.var, 'some-new-value');
+});

package.json

@@ -37,7 +37,6 @@
     "start:dev": "NODE_ENV=development supervisor --ignore ./node_modules/,website server-dev.js",
     "db-migrate": "db-migrate",
     "lint": "eslint .",
-    "pretest": "yarn run lint",
     "test": "NODE_ENV=test PORT=4243 ava",
     "test:docker": "./scripts/docker-postgres.sh",
     "test:watch": "yarn test --watch",

test/e2e/api/admin/addon.e2e.test.js

@@ -6,6 +6,8 @@ const dbInit = require('../../helpers/database-init');
 const { setupApp } = require('../../helpers/test-helper');
 const getLogger = require('../../../fixtures/no-logger');
 
+const MASKED_VALUE = '*****';
+
 let stores;
 
 test.before(async () => {
@@ -84,7 +86,7 @@ test.serial('should delete addon configuration', async t => {
 });
 
 test.serial('should update addon configuration', async t => {
-    t.plan(1);
+    t.plan(2);
     const request = await setupApp(stores);
 
     const config = {
@@ -122,7 +124,11 @@ test.serial('should update addon configuration', async t => {
         .send(config)
         .expect(200)
         .expect(r => {
-            t.is(r.body.parameters.url, updatedConfig.parameters.url);
+            t.is(r.body.parameters.url, MASKED_VALUE);
+            t.is(
+                r.body.parameters.bodyTemplate,
+                updatedConfig.parameters.bodyTemplate,
+            );
         });
 });
 
@@ -145,7 +151,7 @@ test.serial('should not update with invalid addon configuration', async t => {
         .expect(400);
 });
 
-test.serial('should not update unknwn addon configuration', async t => {
+test.serial('should not update unknown addon configuration', async t => {
     t.plan(0);
     const request = await setupApp(stores);
 
@@ -166,7 +172,7 @@ test.serial('should not update unknwn addon configuration', async t => {
 });
 
 test.serial('should get addon configuration', async t => {
-    t.plan(1);
+    t.plan(3);
     const request = await setupApp(stores);
 
     const config = {
@@ -191,10 +197,15 @@ test.serial('should get addon configuration', async t => {
         .expect(200)
         .expect(r => {
             t.is(r.body.provider, config.provider);
+            t.is(
+                r.body.parameters.bodyTemplate,
+                config.parameters.bodyTemplate,
+            );
+            t.is(r.body.parameters.url, MASKED_VALUE);
         });
 });
 
-test.serial('should not get unkown addon configuration', async t => {
+test.serial('should not get unknown addon configuration', async t => {
     t.plan(0);
     const request = await setupApp(stores);
 

test/fixtures/fake-addon-store.js

@@ -18,7 +18,7 @@ module.exports = () => {
             Promise.resolve();
         },
         get: async id => {
-            return _addons.find(id);
+            return _addons[id];
         },
         getAll: () => Promise.resolve(_addons),
     };