remove try catch from iast plugin (#4804)

* remove try catch from iast plugin

* fix linter

Author: IlyasShabi

packages/dd-trace/src/appsec/iast/iast-plugin.js

@@ -60,24 +60,10 @@ class IastPlugin extends Plugin {
     this.pluginSubs = []
   }
 
-  _wrapHandler (handler) {
-    return (message, name) => {
-      try {
-        handler(message, name)
-      } catch (e) {
-        log.error('[ASM] Error executing IAST plugin handler', e)
-      }
-    }
-  }
-
   _getTelemetryHandler (iastSub) {
     return () => {
-      try {
-        const iastContext = getIastContext(storage.getStore())
-        iastSub.increaseExecuted(iastContext)
-      } catch (e) {
-        log.error('[ASM] Error increasing handler executed metrics', e)
-      }
+      const iastContext = getIastContext(storage.getStore())
+      iastSub.increaseExecuted(iastContext)
     }
   }
 
@@ -99,11 +85,11 @@ class IastPlugin extends Plugin {
 
   addSub (iastSub, handler) {
     if (typeof iastSub === 'string') {
-      super.addSub(iastSub, this._wrapHandler(handler))
+      super.addSub(iastSub, handler)
     } else {
       iastSub = this._getAndRegisterSubscription(iastSub)
       if (iastSub) {
-        super.addSub(iastSub.channelName, this._wrapHandler(handler))
+        super.addSub(iastSub.channelName, handler)
 
         if (iastTelemetry.isEnabled()) {
           super.addSub(iastSub.channelName, this._getTelemetryHandler(iastSub))

packages/dd-trace/test/appsec/iast/analyzers/vulnerability-analyzer.spec.js

@@ -133,27 +133,6 @@ describe('vulnerability-analyzer', () => {
     )
   })
 
-  it('should wrap subscription handler and catch thrown Errors', () => {
-    const vulnerabilityAnalyzer = new VulnerabilityAnalyzer(ANALYZER_TYPE)
-    const handler = sinon.spy(() => {
-      throw new Error('handler Error')
-    })
-    const wrapped = vulnerabilityAnalyzer._wrapHandler(handler)
-
-    const iastContext = {
-      name: 'test'
-    }
-    iastContextHandler.getIastContext.returns(iastContext)
-
-    expect(typeof wrapped).to.be.equal('function')
-    const message = {}
-    const name = 'test'
-    expect(() => wrapped(message, name)).to.not.throw()
-    const args = handler.firstCall.args
-    expect(args[0]).to.be.equal(message)
-    expect(args[1]).to.be.equal(name)
-  })
-
   it('should catch thrown Errors inside subscription handlers', () => {
     const vulnerabilityAnalyzer = new VulnerabilityAnalyzer(ANALYZER_TYPE)
     vulnerabilityAnalyzer.addSub({ channelName: 'dd-trace:test:error:sub' }, () => {

packages/dd-trace/test/appsec/iast/iast-plugin.spec.js

@@ -4,6 +4,7 @@ const { expect } = require('chai')
 const { channel } = require('dc-polyfill')
 const proxyquire = require('proxyquire')
 const { getExecutedMetric, getInstrumentedMetric, TagKey } = require('../../../src/appsec/iast/telemetry/iast-metric')
+const { IastPlugin } = require('../../../src/appsec/iast/iast-plugin')
 
 const VULNERABILITY_TYPE = TagKey.VULNERABILITY_TYPE
 const SOURCE_TYPE = TagKey.SOURCE_TYPE
@@ -71,33 +72,23 @@ describe('IAST Plugin', () => {
     })
 
     describe('addSub', () => {
-      it('should call Plugin.addSub with channelName and wrapped handler', () => {
+      it('should call Plugin.addSub with channelName and handler', () => {
         iastPlugin.addSub('test', handler)
 
         expect(addSubMock).to.be.calledOnce
         const args = addSubMock.getCall(0).args
         expect(args[0]).equal('test')
-
-        const wrapped = args[1]
-        expect(wrapped).to.be.a('function')
-        expect(wrapped).to.not.be.equal(handler)
-        expect(wrapped()).to.not.throw
-        expect(logError).to.be.calledOnce
+        expect(args[1]).to.equal(handler)
       })
 
-      it('should call Plugin.addSub with channelName and wrapped handler after registering iastPluginSub', () => {
+      it('should call Plugin.addSub with channelName and handler after registering iastPluginSub', () => {
         const iastPluginSub = { channelName: 'test' }
         iastPlugin.addSub(iastPluginSub, handler)
 
         expect(addSubMock).to.be.calledOnce
         const args = addSubMock.getCall(0).args
         expect(args[0]).equal('test')
-
-        const wrapped = args[1]
-        expect(wrapped).to.be.a('function')
-        expect(wrapped).to.not.be.equal(handler)
-        expect(wrapped()).to.not.throw
-        expect(logError).to.be.calledOnce
+        expect(args[1]).to.equal(handler)
       })
 
       it('should infer moduleName from channelName after registering iastPluginSub', () => {
@@ -117,20 +108,15 @@ describe('IAST Plugin', () => {
       })
 
       it('should not call _getTelemetryHandler', () => {
-        const wrapHandler = sinon.stub()
-        iastPlugin._wrapHandler = wrapHandler
         const getTelemetryHandler = sinon.stub()
         iastPlugin._getTelemetryHandler = getTelemetryHandler
         iastPlugin.addSub({ channelName, tagKey: VULNERABILITY_TYPE }, handler)
 
-        expect(wrapHandler).to.be.calledOnceWith(handler)
         expect(getTelemetryHandler).to.be.not.called
 
-        wrapHandler.reset()
         getTelemetryHandler.reset()
 
         iastPlugin.addSub({ channelName, tagKey: SOURCE_TYPE, tag: 'test-tag' }, handler)
-        expect(wrapHandler).to.be.calledOnceWith(handler)
         expect(getTelemetryHandler).to.be.not.called
       })
     })
@@ -235,20 +221,15 @@ describe('IAST Plugin', () => {
 
     describe('addSub', () => {
       it('should call _getTelemetryHandler with correct metrics', () => {
-        const wrapHandler = sinon.stub()
-        iastPlugin._wrapHandler = wrapHandler
         const getTelemetryHandler = sinon.stub()
         iastPlugin._getTelemetryHandler = getTelemetryHandler
         iastPlugin.addSub({ channelName, tagKey: VULNERABILITY_TYPE }, handler)
 
-        expect(wrapHandler).to.be.calledOnceWith(handler)
         expect(getTelemetryHandler).to.be.calledOnceWith(iastPlugin.pluginSubs[0])
 
-        wrapHandler.reset()
         getTelemetryHandler.reset()
 
         iastPlugin.addSub({ channelName, tagKey: SOURCE_TYPE, tag: 'test-tag' }, handler)
-        expect(wrapHandler).to.be.calledOnceWith(handler)
         expect(getTelemetryHandler).to.be.calledOnceWith(iastPlugin.pluginSubs[1])
       })
 
@@ -399,4 +380,50 @@ describe('IAST Plugin', () => {
       })
     })
   })
+
+  describe('Add sub to iast plugin', () => {
+    class BadPlugin extends IastPlugin {
+      static get id () { return 'badPlugin' }
+
+      constructor () {
+        super()
+        this.addSub('appsec:badPlugin:start', this.start)
+      }
+
+      start () {
+        throw new Error('this is one bad plugin')
+      }
+    }
+    class GoodPlugin extends IastPlugin {
+      static get id () { return 'goodPlugin' }
+
+      constructor () {
+        super()
+        this.addSub('appsec:goodPlugin:start', this.start)
+      }
+
+      start () {}
+    }
+
+    const badPlugin = new BadPlugin()
+    const goodPlugin = new GoodPlugin()
+
+    it('should disable bad plugin', () => {
+      badPlugin.configure({ enabled: true })
+      expect(badPlugin._enabled).to.be.true
+
+      channel('appsec:badPlugin:start').publish({ foo: 'bar' })
+
+      expect(badPlugin._enabled).to.be.false
+    })
+
+    it('should not disable good plugin', () => {
+      goodPlugin.configure({ enabled: true })
+      expect(goodPlugin._enabled).to.be.true
+
+      channel('appsec:goodPlugin:start').publish({ foo: 'bar' })
+
+      expect(goodPlugin._enabled).to.be.true
+    })
+  })
 })