Add allowExpressions option (#244)

matthewloring · web-flow · commit 36a0c2c0125a · 2017-03-07T15:10:35.000-08:00
PR-URL: #244
diff --git a/src/agent/config.js b/src/agent/config.js
@@ -37,6 +37,15 @@ module.exports = {
    */
   description: null,
 
+  /**
+   * @property {boolean} Whether or not it is permitted to evaluate expressions.
+   * Locals and arguments are not displayed and watch expressions and conditions
+   * are dissallowed when this is `false`.
+   * @memberof DebugAgentConfig
+   * @default
+   */
+  allowExpressions: false,
+
   // FIXME(ofrobots): today we prioritize GAE_MODULE_NAME/GAE_MODULE_VERSION
   // over the user specified config. We should reverse that.
   /**
diff --git a/src/agent/debuglet.js b/src/agent/debuglet.js
@@ -38,6 +38,8 @@ var pjson = require('../../package.json');
 
 var assert = require('assert');
 
+var ALLOW_EXPRESSIONS_MESSAGE = 'Expressions and conditions are not allowed' +
+  ' by default. Please set the allowExpressions configuration option to true';
 var NODE_VERSION_MESSAGE = 'Node.js version not supported. Node.js 5.2.0 and ' +
   ' versions older than 0.12 are not supported.';
 var BREAKPOINT_ACTION_MESSAGE = 'The only currently supported breakpoint actions' +
@@ -539,6 +541,15 @@ Debuglet.prototype.removeBreakpoint_ = function(breakpoint) {
 Debuglet.prototype.addBreakpoint_ = function(breakpoint, cb) {
   var that = this;
 
+  if (!that.config_.allowExpressions &&
+      (breakpoint.condition || breakpoint.expressions)) {
+    that.logger_.error(ALLOW_EXPRESSIONS_MESSAGE);
+    breakpoint.status = new StatusMessage(StatusMessage.UNSPECIFIED,
+      ALLOW_EXPRESSIONS_MESSAGE, true);
+    setImmediate(function() { cb(ALLOW_EXPRESSIONS_MESSAGE); });
+    return;
+  }
+
   if (semver.satisfies(process.version, '5.2 || <0.12')) {
     var message = NODE_VERSION_MESSAGE;
     that.logger_.error(message);
diff --git a/src/agent/state.js b/src/agent/state.js
@@ -290,6 +290,8 @@ StateResolver.prototype.isPathInNodeModulesDirectory_ = function(path) {
 StateResolver.prototype.resolveFrame_ = function(frame, underFrameCap) {
   var args = [];
   var locals = [];
+  // Locals and arguments are safe to collect even when `config.allowExpressions=false`
+  // since we properly avoid inspecting interceptors and getters by default.
   if (!underFrameCap) {
     args.push({
       name: 'arguments_not_available',
diff --git a/test/test-debuglet.js b/test/test-debuglet.js
@@ -18,6 +18,7 @@
 var _ = require('lodash');
 var assert = require('assert');
 var DEFAULT_CONFIG = require('../src/agent/config.js');
+DEFAULT_CONFIG.allowExpressions = true;
 var Debuglet = require('../src/agent/debuglet.js');
 var extend = require('extend');
 
@@ -526,6 +527,88 @@ describe('Debuglet', function() {
       debuglet.start();
     });
 
+    it('should reject breakpoints with conditions when allowExpressions=false',
+        function(done) {
+      this.timeout(2000);
+      var debug = require('../src/debug.js')(
+          {projectId: 'fake-project', credentials: fakeCredentials});
+      var debuglet = new Debuglet(debug, defaultConfig);
+      debuglet.config_.allowExpressions = false;
+
+      var scope = nock(API)
+                      .post(REGISTER_PATH)
+                      .reply(200, {debuggee: {id: DEBUGGEE_ID}})
+                      .get(BPS_PATH + '?success_on_timeout=true')
+                      .reply(200, {breakpoints: [{
+                        id: 'test',
+                        action: 'CAPTURE',
+                        condition: 'x === 5',
+                        location: {path: 'fixtures/foo.js', line: 2}
+                      }]})
+                      .put(BPS_PATH + '/test',
+                           function(body) {
+                             var status = body.breakpoint.status;
+                             var hasCorrectDescription = status.description.format.indexOf(
+                                'Expressions and conditions are not allowed by default.') === 0;
+                             return status.isError && hasCorrectDescription;
+                           })
+                      .reply(200);
+
+      debuglet.once('registered', function reg(id) {
+        assert.equal(id, DEBUGGEE_ID);
+        setTimeout(function() {
+          assert.ok(!debuglet.activeBreakpointMap_.test);
+          debuglet.stop();
+          debuglet.config_.allowExpressions = true;
+          scope.done();
+          done();
+        }, 1000);
+      });
+
+      debuglet.start();
+    });
+
+    it('should reject breakpoints with expressions when allowExpressions=false',
+        function(done) {
+      this.timeout(2000);
+      var debug = require('../src/debug.js')(
+          {projectId: 'fake-project', credentials: fakeCredentials});
+      var debuglet = new Debuglet(debug, defaultConfig);
+      debuglet.config_.allowExpressions = false;
+
+      var scope = nock(API)
+                      .post(REGISTER_PATH)
+                      .reply(200, {debuggee: {id: DEBUGGEE_ID}})
+                      .get(BPS_PATH + '?success_on_timeout=true')
+                      .reply(200, {breakpoints: [{
+                        id: 'test',
+                        action: 'CAPTURE',
+                        expressions: ['x'],
+                        location: {path: 'fixtures/foo.js', line: 2}
+                      }]})
+                      .put(BPS_PATH + '/test',
+                           function(body) {
+                             var status = body.breakpoint.status;
+                             var hasCorrectDescription = status.description.format.indexOf(
+                                'Expressions and conditions are not allowed by default.') === 0;
+                             return status.isError && hasCorrectDescription;
+                           })
+                      .reply(200);
+
+      debuglet.once('registered', function reg(id) {
+        assert.equal(id, DEBUGGEE_ID);
+        setTimeout(function() {
+          assert.ok(!debuglet.activeBreakpointMap_.test);
+          debuglet.stop();
+          debuglet.config_.allowExpressions = true;
+          scope.done();
+          done();
+        }, 1000);
+      });
+
+      debuglet.start();
+    });
+
     it('should re-fetch breakpoints on error', function(done) {
       this.timeout(6000);
 
