revise based on PR comments

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

config/opensearch_dashboards.yml

@@ -106,8 +106,37 @@
 # Supported languages are the following: English - en , by default , Chinese - zh-CN .
 #i18n.locale: "en"
 
-# Set the allowlist to check input graphite url. 
-vis_type_timeline.graphiteUrls: ['https://www.hostedgraphite.com/UID/ACCESS_KEY/graphite']
-
-# Set the blocklist to check input graphite url. 
-#vis_type_timeline.blocklist: []
+# Set the allowlist to check input graphite Url. Allowlist is the default check list.
+vis_type_timeline.graphiteAllowedUrls: ['https://www.hostedgraphite.com/UID/ACCESS_KEY/graphite']
+
+# Set the blocklist to check input graphite Url/IP. Blocklist is an IP list. 
+# Below is an example for reference 
+# vis_type_timeline.graphiteBlockedIPs: [
+#  //Loopback
+#  '127.0.0.0/8',
+#  '::1/128',
+#  // Link-local Address for IPv6
+#  'fe80::/10',
+#  //Private IP address for IPv4
+#  '10.0.0.0/8',
+#  '172.16.0.0/12',
+#  '192.168.0.0/16',
+#  //Unique local address (ULA)
+#  'fc00::/7',
+#  //Reserved IP address
+#  '0.0.0.0/8',
+#  '100.64.0.0/10',
+#  '192.0.0.0/24',
+#  '192.0.2.0/24',
+#  '198.18.0.0/15',
+#  '192.88.99.0/24',
+#  '198.51.100.0/24',
+#  '203.0.113.0/24',
+#  '224.0.0.0/4',
+#  '240.0.0.0/4',
+#  '255.255.255.255/32',
+#  '::/128',
+#  '2001:db8::/32',
+#  'ff00::/8',
+# ]
+#vis_type_timeline.graphiteBlockedIPs: []

src/plugins/vis_type_timeline/config.ts

@@ -36,8 +36,8 @@ export const configSchema = schema.object(
   {
     enabled: schema.boolean({ defaultValue: true }),
     ui: schema.object({ enabled: schema.boolean({ defaultValue: false }) }),
-    graphiteUrls: schema.maybe(schema.arrayOf(schema.string())),
-    blocklist: schema.maybe(schema.arrayOf(schema.string())),
+    graphiteAllowedUrls: schema.maybe(schema.arrayOf(schema.string())),
+    graphiteBlockedIPs: schema.maybe(schema.arrayOf(schema.string())),
   },
   // This option should be removed as soon as we entirely migrate config from legacy Timeline plugin.
   { unknowns: 'allow' }

src/plugins/vis_type_timeline/server/index.ts

@@ -45,6 +45,7 @@ export const config: PluginConfigDescriptor<ConfigSchema> = {
     renameFromRoot('timeline_vis.enabled', 'vis_type_timeline.enabled'),
     renameFromRoot('timeline.enabled', 'vis_type_timeline.enabled'),
     renameFromRoot('timeline.graphiteUrls', 'vis_type_timeline.graphiteUrls'),
+    renameFromRoot('vis_type_timeline.graphiteUrls', 'vis_type_timeline.graphiteAllowedUrls'),
     renameFromRoot('timeline.ui.enabled', 'vis_type_timeline.ui.enabled', true),
   ],
 };

src/plugins/vis_type_timeline/server/lib/config_manager.ts

@@ -36,16 +36,16 @@ import { configSchema } from '../../config';
 
 export class ConfigManager {
   private opensearchShardTimeout: number = 0;
-  private graphiteUrls: string[] = [];
-  private blockedUrls: string[] = [];
+  private graphiteAllowedUrls: string[] = [];
+  private graphiteBlockedIPs: string[] = [];
 
   constructor(config: PluginInitializerContext['config']) {
     config.create<TypeOf<typeof configSchema>>().subscribe((configUpdate) => {
-      this.graphiteUrls = configUpdate.graphiteUrls || [];
+      this.graphiteAllowedUrls = configUpdate.graphiteAllowedUrls || [];
     });
 
     config.create<TypeOf<typeof configSchema>>().subscribe((configUpdate) => {
-      this.blockedUrls = configUpdate.blocklist || [];
+      this.graphiteBlockedIPs = configUpdate.graphiteBlockedIPs || [];
     });
 
     config.legacy.globalConfig$.subscribe((configUpdate) => {
@@ -58,10 +58,10 @@ export class ConfigManager {
   }
 
   getGraphiteUrls() {
-    return this.graphiteUrls;
+    return this.graphiteAllowedUrls;
   }
 
-  getBlockedUrls() {
-    return this.blockedUrls;
+  getGraphiteBlockedIPs() {
+    return this.graphiteBlockedIPs;
   }
 }

src/plugins/vis_type_timeline/server/plugin.ts

@@ -171,7 +171,10 @@ export class Plugin {
           description:
             'The URL should be in the form of https://www.hostedgraphite.com/UID/ACCESS_KEY/graphite',
         }),
-        value: config.graphiteUrls && config.graphiteUrls.length ? config.graphiteUrls[0] : null,
+        value:
+          config.graphiteAllowedUrls && config.graphiteAllowedUrls.length
+            ? config.graphiteAllowedUrls[0]
+            : null,
         description: i18n.translate('timeline.uiSettings.graphiteURLDescription', {
           defaultMessage: '{experimentalLabel} The URL of your graphite host',
           values: { experimentalLabel: `<em>[${experimentalLabel}]</em>` },

src/plugins/vis_type_timeline/server/routes/run.ts

@@ -102,7 +102,7 @@ export function runRoute(
           getFunction,
           getStartServices: core.getStartServices,
           allowedGraphiteUrls: configManager.getGraphiteUrls(),
-          blockedGraphiteUrls: configManager.getBlockedUrls(),
+          blockedGraphiteIPs: configManager.getGraphiteBlockedIPs(),
           opensearchShardTimeout: configManager.getOpenSearchShardTimeout(),
           savedObjectsClient: context.core.savedObjects.client,
         });

src/plugins/vis_type_timeline/server/series_functions/fixtures/tl_config.js

@@ -56,7 +56,7 @@ export default function () {
 
     opensearchShardTimeout: moment.duration(30000),
     allowedGraphiteUrls: ['https://www.hostedgraphite.com/UID/ACCESS_KEY/graphite'],
-    blockedGraphiteUrls: [],
+    blockedGraphiteIPs: [],
   });
 
   tlConfig.time = {
@@ -68,6 +68,10 @@ export default function () {
 
   tlConfig.settings = timelineDefaults();
 
+  tlConfig.allowedGraphiteUrls = timelineDefaults();
+
+  tlConfig.blockedGraphiteIPs = timelineDefaults();
+
   tlConfig.setTargetSeries();
 
   return tlConfig;

src/plugins/vis_type_timeline/server/series_functions/graphite.js

@@ -41,7 +41,7 @@ import IPCIDR from 'ip-cidr';
 const MISS_CHECKLIST_MESSAGE = `Please configure on the opensearch_dashbpards.yml file. 
 You can always enable the default allowlist configuration.`;
 
-const INVALID_URL_MESSAGE = `The Graphite URL provided by you is invalid. 
+const INVALID_URL_MESSAGE = `The Graphite URL/IP provided by you is invalid. 
 Please update your config from OpenSearch Dashboards's Advanced Setting.`;
 
 /**
@@ -65,7 +65,7 @@ function getIpAddress(urlObject) {
   return null;
 }
 
-function isValidURL(configuredUrl, blockedUrls) {
+function isValidURL(configuredUrl, blockedIPs) {
   // Check the format of URL, URL has be in the format as
   // scheme://server/path/resource otherwise an TypeError
   // would be thrown
@@ -85,32 +85,27 @@ function isValidURL(configuredUrl, blockedUrls) {
   // range of an IP address block
   // @param {string} bl
   // @returns {object} cidr
-  for (const bl of blockedUrls) {
-    const cidr = new IPCIDR(bl);
-    if (cidr.contains(ip)) {
-      return false;
-    }
-  }
-  return true;
+  const isBlocked = blockedIPs.some((blockedIP) => new IPCIDR(blockedIP).contains(ip));
+  return !isBlocked;
 }
 
 /**
  * Check configured url using blocklist and allowlist
  * If allowlist is used, return false if allowlist does not contain configured url
  * If blocklist is used, return false if blocklist contains configured url
- * If both allowlist and blocklist are used, return false if allowlist does not contain or if blocklist contains configured url
- * @param {Array|string} blockedUrls
+ * If both allowlist and blocklist are used, check blocklist first then allowlist
+ * @param {Array|string} blockedIPs
  * @param {Array|string} allowedUrls
  * @param {string} configuredUrls
  * @returns {boolean} true if the configuredUrl is valid
  */
-function checkConfigUrls(blockedUrls, allowedUrls, configuredUrl) {
-  if (blockedUrls.length === 0) {
+function isValidConfig(blockedIPs, allowedUrls, configuredUrl) {
+  if (blockedIPs.length === 0) {
     if (!allowedUrls.includes(configuredUrl)) return false;
   } else if (allowedUrls.length === 0) {
-    if (!isValidURL(configuredUrl, blockedUrls)) return false;
+    if (!isValidURL(configuredUrl, blockedIPs)) return false;
   } else {
-    if (!allowedUrls.includes(configuredUrl) || !isValidURL(configuredUrl, blockedUrls))
+    if (!isValidURL(configuredUrl, blockedIPs) || !allowedUrls.includes(configuredUrl))
       return false;
   }
   return true;
@@ -139,20 +134,22 @@ export default new Datasource('graphite', {
       min: moment(tlConfig.time.from).format('HH:mm[_]YYYYMMDD'),
       max: moment(tlConfig.time.to).format('HH:mm[_]YYYYMMDD'),
     };
+
     const allowedUrls = tlConfig.allowedGraphiteUrls;
-    const blockedUrls = tlConfig.blockedGraphiteUrls;
+    const blockedIPs = tlConfig.blockedGraphiteIPs;
     const configuredUrl = tlConfig.settings['timeline:graphite.url'];
-    if (allowedUrls.length === 0 && blockedUrls.length === 0) {
+
+    if (allowedUrls.length === 0 && blockedIPs.length === 0) {
       throw new Error(
-        i18n.translate('timeline.help.functions.missCheckGraphiteUrl', {
+        i18n.translate('timeline.help.functions.missCheckGraphiteConfig', {
           defaultMessage: MISS_CHECKLIST_MESSAGE,
         })
       );
     }
 
-    if (!checkConfigUrls(blockedUrls, allowedUrls, configuredUrl)) {
+    if (!isValidConfig(blockedIPs, allowedUrls, configuredUrl)) {
       throw new Error(
-        i18n.translate('timeline.help.functions.invalidGraphiteUrl', {
+        i18n.translate('timeline.help.functions.invalidGraphiteConfig', {
           defaultMessage: INVALID_URL_MESSAGE,
         })
       );

src/plugins/vis_type_timeline/server/series_functions/graphite.test.js

@@ -35,12 +35,15 @@ const expect = require('chai').expect;
 import fn from './graphite';
 
 const MISS_CHECKLIST_MESSAGE = `Please configure on the opensearch_dashbpards.yml file. 
-You can always enable the default allowlist configuration. `;
+You can always enable the default allowlist configuration.`;
 
-const INVALID_URL_MESSAGE = `The Graphite URL provided by you is invalid. 
+const INVALID_URL_MESSAGE = `The Graphite URL/IP provided by you is invalid. 
 Please update your config from OpenSearch Dashboards's Advanced Setting.`;
 
-jest.mock('node-fetch', () => () => {
+jest.mock('node-fetch', () => (url) => {
+  if (url.includes('redirect')) {
+    return Promise.reject(new Error('maximum redirect reached at: ' + url));
+  }
   return Promise.resolve({
     json: function () {
       return [
@@ -62,28 +65,38 @@ import invoke from './helpers/invoke_series_fn.js';
 
 describe('graphite', function () {
   it('should wrap the graphite response up in a seriesList', function () {
-    return invoke(fn, []).then(function (result) {
+    return invoke(fn, [], {
+      allowedGraphiteUrls: ['https://www.hostedgraphite.com/UID/ACCESS_KEY/graphite'],
+      blockedGraphiteIPs: [],
+    }).then(function (result) {
       expect(result.output.list[0].data[0][1]).to.eql(3);
       expect(result.output.list[0].data[1][1]).to.eql(14);
     });
   });
 
   it('should convert the seconds to milliseconds', function () {
-    return invoke(fn, []).then(function (result) {
+    return invoke(fn, [], {
+      allowedGraphiteUrls: ['https://www.hostedgraphite.com/UID/ACCESS_KEY/graphite'],
+      blockedGraphiteIPs: [],
+    }).then(function (result) {
       expect(result.output.list[0].data[1][0]).to.eql(2000 * 1000);
     });
   });
 
   it('should set the label to that of the graphite target', function () {
-    return invoke(fn, []).then(function (result) {
+    return invoke(fn, [], {
+      allowedGraphiteUrls: ['https://www.hostedgraphite.com/UID/ACCESS_KEY/graphite'],
+      blockedGraphiteIPs: [],
+    }).then(function (result) {
       expect(result.output.list[0].label).to.eql('__beer__');
     });
   });
 
   it('should return error message if both allowlist and blocklist are disabled', function () {
     return invoke(fn, [], {
-      settings: { 'timelion:graphite.url': 'http://127.0.0.1' },
+      settings: { 'timeline:graphite.url': 'http://127.0.0.1' },
       allowedGraphiteUrls: [],
+      blockedGraphiteIPs: [],
     }).catch((e) => {
       expect(e.message).to.eql(MISS_CHECKLIST_MESSAGE);
     });
@@ -92,77 +105,80 @@ describe('graphite', function () {
   it('setting with matched allowlist url should return result ', function () {
     return invoke(fn, [], {
       settings: {
-        'timelion:graphite.url': 'https://www.hostedgraphite.com/UID/ACCESS_KEY/graphite',
+        'timeline:graphite.url': 'https://www.hostedgraphite.com/UID/ACCESS_KEY/graphite',
       },
+      allowedGraphiteUrls: ['https://www.hostedgraphite.com/UID/ACCESS_KEY/graphite'],
+      blockedGraphiteIPs: [],
     }).then((result) => {
-      console.log(result);
       expect(result.output.list.length).to.eql(1);
     });
   });
 
   it('setting with unmatched allowlist url should return error message ', function () {
     return invoke(fn, [], {
-      settings: { 'timelion:graphite.url': 'http://127.0.0.1' },
+      settings: { 'timeline:graphite.url': 'http://127.0.0.1' },
+      allowedGraphiteUrls: ['https://www.hostedgraphite.com/UID/ACCESS_KEY/graphite'],
+      blockedGraphiteIPs: [],
     }).catch((e) => {
       expect(e.message).to.eql(INVALID_URL_MESSAGE);
     });
   });
 
   it('setting with matched blocklist url should return error message', function () {
     return invoke(fn, [], {
-      settings: { 'timelion:graphite.url': 'http://127.0.0.1' },
+      settings: { 'timeline:graphite.url': 'http://127.0.0.1' },
       allowedGraphiteUrls: [],
-      blockedGraphiteUrls: ['127.0.0.0/8'],
+      blockedGraphiteIPs: ['127.0.0.0/8'],
     }).catch((e) => {
       expect(e.message).to.eql(INVALID_URL_MESSAGE);
     });
   });
 
   it('setting with matched blocklist localhost should return error message', function () {
     return invoke(fn, [], {
-      settings: { 'timelion:graphite.url': 'http://localhost' },
+      settings: { 'timeline:graphite.url': 'http://localhost' },
       allowedGraphiteUrls: [],
-      blockedGraphiteUrls: ['127.0.0.0/8'],
+      blockedGraphiteIPs: ['127.0.0.0/8'],
     }).catch((e) => {
       expect(e.message).to.eql(INVALID_URL_MESSAGE);
     });
   });
 
   it('setting with unmatched blocklist https url should return result', function () {
     return invoke(fn, [], {
-      settings: { 'timelion:graphite.url': 'https://www.amazon.com' },
+      settings: { 'timeline:graphite.url': 'https://www.amazon.com' },
       allowedGraphiteUrls: [],
-      blockedGraphiteUrls: ['127.0.0.0/8'],
+      blockedGraphiteIPs: ['127.0.0.0/8'],
     }).then((result) => {
-      console.log(result);
       expect(result.output.list.length).to.eql(1);
     });
   });
 
   it('setting with unmatched blocklist ftp url should return result', function () {
     return invoke(fn, [], {
-      settings: { 'timelion:graphite.url': 'ftp://www.amazon.com' },
+      settings: { 'timeline:graphite.url': 'ftp://www.amazon.com' },
       allowedGraphiteUrls: [],
-      blockedGraphiteUrls: ['127.0.0.0/8'],
+      blockedGraphiteIPs: ['127.0.0.0/8'],
     }).then((result) => {
-      console.log(result);
       expect(result.output.list.length).to.eql(1);
     });
   });
 
   it('setting with invalid url should return error message', function () {
     return invoke(fn, [], {
-      settings: { 'timelion:graphite.url': 'www.amazon.com' },
+      settings: { 'timeline:graphite.url': 'www.amazon.com' },
       allowedGraphiteUrls: [],
-      blockedGraphiteUrls: ['127.0.0.0/8'],
+      blockedGraphiteIPs: ['127.0.0.0/8'],
     }).catch((e) => {
       expect(e.message).to.eql(INVALID_URL_MESSAGE);
     });
   });
 
   it('setting with redirection error message', function () {
     return invoke(fn, [], {
-      settings: { 'timelion:graphite.url': 'https://amazon.com/redirect' },
+      settings: { 'timeline:graphite.url': 'https://amazon.com/redirect' },
+      allowedGraphiteUrls: [],
+      blockedGraphiteIPs: ['127.0.0.0/8'],
     }).catch((e) => {
       expect(e.message).to.includes('maximum redirect reached');
     });