fix: refuse to start if working dir is root dir (#381)

DominicKramer · web-flow · commit 3b97598725b8 · 2017-12-20T16:06:44.000-08:00
fixes: #377
diff --git a/src/agent/config.ts b/src/agent/config.ts
@@ -23,6 +23,8 @@ export type DebugAgentConfig = {
 export interface ResolvedDebugAgentConfig extends common.AuthenticationConfig {
   workingDirectory: string;
 
+  allowRootAsWorkingDirectory: boolean;
+
   /**
    * A user specified way of identifying the service
    */
@@ -174,6 +176,7 @@ export const defaultConfig: ResolvedDebugAgentConfig = {
   // FIXME(ofrobots): presently this is dependent what cwd() is at the time this
   // file is first required. We should make the default config static.
   workingDirectory: process.cwd(),
+  allowRootAsWorkingDirectory: false,
   description: undefined,
   allowExpressions: false,
 
diff --git a/src/agent/debuglet.ts b/src/agent/debuglet.ts
@@ -299,6 +299,20 @@ export class Debuglet extends EventEmitter {
       return;
     }
 
+    const workingDir = that.config.workingDirectory;
+    // Don't continue if the working directory is a root directory
+    // unless the user wants to force using the root directory
+    if (!that.config.allowRootAsWorkingDirectory &&
+        path.join(workingDir, '..') === workingDir) {
+      const message = 'The working directory is a root directory. Disabling ' +
+        'to avoid a scan of the entire filesystem for JavaScript files. ' +
+        'Use config \allowRootAsWorkingDirectory` if you really want to ' +
+        'do this.';
+      that.logger.error(message);
+      that.emit('initError', new Error(message));
+      return;
+    }
+
     // TODO: Verify that it is fine for `id` to be undefined.
     let id: string|undefined;
     if (process.env.GAE_MINOR_VERSION) {
@@ -732,7 +746,6 @@ export class Debuglet extends EventEmitter {
           formatBreakpoints('Server breakpoints: ', updatedBreakpointMap));
     }
     breakpoints.forEach((breakpoint: stackdriver.Breakpoint) => {
-
       // TODO: Address the case when `breakpoint.id` is `undefined`.
       if (!that.completedBreakpointMap[breakpoint.id as string] &&
           !that.activeBreakpointMap[breakpoint.id as string]) {
diff --git a/src/agent/io/sourcemapper.ts b/src/agent/io/sourcemapper.ts
@@ -236,11 +236,11 @@ export class SourceMapper {
       // TODO: The `sourceMap.Position` type definition has a `column`
       //       attribute and not a `col` attribute.  Determine if the type
       //       definition or this code is correct.
-      column: (mappedPos as {} as {
-                col: number
-              }).col  // SourceMapConsumer uses zero-based column
-                      // numbers which is the same as the
-                      // expected output
+      column: (mappedPos as {} as {col: number}).col  // SourceMapConsumer uses
+                                                      // zero-based column
+                                                      // numbers which is the
+                                                      // same as the expected
+                                                      // output
     };
   }
 }
diff --git a/src/agent/state/inspector-state.ts b/src/agent/state/inspector-state.ts
@@ -174,7 +174,6 @@ class StateResolver {
           }
         }
         this.evaluatedExpressions[index2] = evaluated;
-
       });
     }
     // The frames are resolved after the evaluated expressions so that
diff --git a/system-test/test-controller.ts b/system-test/test-controller.ts
@@ -121,5 +121,4 @@ describe('Controller', function() {
   // To be able to write the following test we need a service for adding a
   // breakpoint (need the debugger API). TODO.
   it('should update an active breakpoint');
-
 });
diff --git a/system-test/test-e2e.ts b/system-test/test-e2e.ts
@@ -223,7 +223,6 @@ describe('@google-cloud/debug end-to-end behavior', () => {
           // TODO: Determine if the results parameter should be used.
         })
         .then((results: stackdriver.Breakpoint[]) => {
-
           // Check the contents of the log, but keep the original breakpoint.
 
           // TODO: This is never used.  Determine if it should be used.
diff --git a/test/test-controller.ts b/test/test-controller.ts
@@ -39,7 +39,6 @@ const api = '/v2/controller';
 nock.disableNetConnect();
 
 describe('Controller API', () => {
-
   describe('register', () => {
     it('should get a debuggeeId', (done) => {
       const scope = nock(url).post(api + '/debuggees/register').reply(200, {
@@ -85,11 +84,9 @@ describe('Controller API', () => {
         done();
       });
     });
-
   });
 
   describe('listBreakpoints', () => {
-
     // register before each test
     before((done) => {
       nock(url).post(api + '/debuggees/register').reply(200, {
@@ -268,7 +265,4 @@ describe('Controller API', () => {
           });
     });
   });
-
-
-
 });
diff --git a/test/test-debuggee.ts b/test/test-debuggee.ts
@@ -20,7 +20,6 @@ import {Debuggee} from '../src/debuggee';
 const agentVersion = `SomeName/client/SomeVersion`;
 
 describe('Debuggee', () => {
-
   it('should create a Debuggee instance on valid input', () => {
     const debuggee = new Debuggee({
       project: 'project',
@@ -62,5 +61,4 @@ describe('Debuggee', () => {
       });
     });
   });
-
 });
diff --git a/test/test-debuglet.ts b/test/test-debuglet.ts
@@ -15,7 +15,7 @@
  */
 
 import * as assert from 'assert';
-import * as rawFs from 'fs';
+import * as fs from 'fs';
 import * as _ from 'lodash';
 import * as path from 'path';
 import * as proxyquire from 'proxyquire';
@@ -28,7 +28,7 @@ import * as stackdriver from '../src/types/stackdriver';
 
 DEFAULT_CONFIG.allowExpressions = true;
 DEFAULT_CONFIG.workingDirectory = path.join(__dirname, '..', '..');
-import {Debuglet, CachedPromise} from '../src/agent/debuglet';
+import {Debuglet, CachedPromise, FindFilesResult} from '../src/agent/debuglet';
 import {ScanResults} from '../src/agent/io/scanner';
 import * as dns from 'dns';
 import * as extend from 'extend';
@@ -814,6 +814,122 @@ describe('Debuglet', () => {
       debuglet.start();
     });
 
+    it('should by default error when workingDirectory is a root directory with a package.json',
+       (done) => {
+         const debug = new Debug({}, packageInfo);
+         /*
+          * `path.sep` represents a root directory on both Windows and Unix.
+          * On Windows, `path.sep` resolves to the current drive.
+          *
+          * That is, after opening a command prompt in Windows, relative to the
+          * drive C: and starting the Node REPL, the value of `path.sep`
+          * represents `C:\\'.
+          *
+          * If `D:` is entered at the prompt to switch to the D: drive before
+          * starting the Node REPL, `path.sep` represents `D:\\`.
+          */
+         const root = path.sep;
+         const mockedDebuglet = proxyquire('../src/agent/debuglet', {
+           /*
+            * Mock the 'fs' module to verify that if the root directory is used,
+            * and the root directory is reported to contain a `package.json`
+            * file, then the agent still produces an `initError` when the
+            * working directory is the root directory.
+            */
+           fs: {
+             stat:
+                 (filepath: string|Buffer,
+                  cb: (err: Error|null, stats: {}) => void) => {
+                   if (filepath === path.join(root, 'package.json')) {
+                     // The key point is that looking for `package.json` in the
+                     // root directory does not cause an error.
+                     return cb(null, {});
+                   }
+                   fs.stat(filepath, cb);
+                 }
+           }
+         });
+         const config = extend({}, defaultConfig, {workingDirectory: root});
+         const debuglet = new mockedDebuglet.Debuglet(debug, config);
+         let text = '';
+         debuglet.logger.error = (str: string) => {
+           text += str;
+         };
+
+         debuglet.on('initError', (err: Error) => {
+           const errorMessage = 'The working directory is a root ' +
+             'directory. Disabling to avoid a scan of the entire filesystem ' +
+             'for JavaScript files. Use config \allowRootAsWorkingDirectory` ' +
+             'if you really want to do this.';
+           assert.ok(err);
+           assert.strictEqual(err.message, errorMessage);
+           assert.ok(text.includes(errorMessage));
+           done();
+         });
+
+         debuglet.once('started', () => {
+           assert.fail(
+               'Should not start if workingDirectory is a root directory');
+         });
+
+         debuglet.start();
+       });
+
+    it('should be able to force the workingDirectory to be a root directory',
+       (done) => {
+         const root = path.sep;
+         // Act like the root directory contains a `package.json` file
+         const mockedDebuglet = proxyquire('../src/agent/debuglet', {
+           fs: {
+             stat:
+                 (filepath: string|Buffer,
+                  cb: (err: Error|null, stats: {}) => void) => {
+                   if (filepath === path.join(root, 'package.json')) {
+                     return cb(null, {});
+                   }
+                   fs.stat(filepath, cb);
+                 }
+           }
+         });
+
+         // Don't actually scan the entire filesystem.  Act like the filesystem
+         // is empty.
+         mockedDebuglet.Debuglet.findFiles =
+             (shouldHash: boolean, baseDir: string):
+                 Promise<FindFilesResult> => {
+                   assert.strictEqual(baseDir, root);
+                   return Promise.resolve({
+                     jsStats: {},
+                     mapFiles: [],
+                     errors: new Map<string, Error>()
+                   });
+                 };
+
+         // Act like the debuglet can get a project id
+         mockedDebuglet.Debuglet.getProjectId = () => 'some-project-id';
+
+         // No need to restore `findFiles` and `getProjectId` because we are
+         // modifying a mocked version of `Debuglet` not `Debuglet` itself.
+
+         const config = extend(
+             {}, defaultConfig,
+             {workingDirectory: root, allowRootAsWorkingDirectory: true});
+         const debug = new Debug({}, packageInfo);
+         const debuglet = new mockedDebuglet.Debuglet(debug, config);
+
+         debuglet.on('initError', (err: Error) => {
+           assert.ifError(err);
+           done();
+         });
+
+         debuglet.once('started', () => {
+           debuglet.stop();
+           done();
+         });
+
+         debuglet.start();
+       });
+
     it('should register successfully otherwise', (done) => {
       const debug = new Debug(
           {projectId: 'fake-project', credentials: fakeCredentials},
diff --git a/test/test-module.ts b/test/test-module.ts
@@ -37,5 +37,4 @@ describe('Debug module', () => {
       module.start();
     });
   });
-
 });
diff --git a/test/test-scanner.ts b/test/test-scanner.ts
@@ -33,7 +33,6 @@ import * as scanner from '../src/agent/io/scanner';
 const COFFEE_FILES_REGEX = /^(.*\.js\.map)|(.*\.js)|(.*\.coffee)$/;
 
 describe('scanner', () => {
-
   describe('scan', () => {
     it('should complain when called without a path', (done) => {
       // TODO: The second argument must be a string.  Fix that.
@@ -56,14 +55,13 @@ describe('scanner', () => {
         return;
       }
 
-      scanner.scan(true, fixture('broken-links'), /.*/)
-          .then((fileStats) => {
-            assert.strictEqual(
-                fileStats.selectFiles(/broken-link\.js/, '').length, 0);
-            assert.strictEqual(
-                fileStats.selectFiles(/intended-link\.js/, '').length, 0);
-            done();
-          });
+      scanner.scan(true, fixture('broken-links'), /.*/).then((fileStats) => {
+        assert.strictEqual(
+            fileStats.selectFiles(/broken-link\.js/, '').length, 0);
+        assert.strictEqual(
+            fileStats.selectFiles(/intended-link\.js/, '').length, 0);
+        done();
+      });
     });
 
     it('should be able to return all file stats directly', (done) => {
diff --git a/test/test-v8debugapi.ts b/test/test-v8debugapi.ts
@@ -308,7 +308,6 @@ describe('v8debugapi', () => {
           assert.ok(bp.status!.isError);
           done();
         });
-
       });
     });
 
@@ -349,7 +348,6 @@ describe('v8debugapi', () => {
         done();
       });
     });
-
   });
 
   function conditionTests(
@@ -584,7 +582,6 @@ describe('v8debugapi', () => {
   });
 
   describe('set and wait', () => {
-
     it('should be possible to wait on a breakpoint', (done) => {
       // clone a clean breakpointInFoo
       // TODO: Have this actually implement Breakpoint
@@ -605,7 +602,6 @@ describe('v8debugapi', () => {
           code.foo(1);
         });
       });
-
     });
 
     it('should resolve actual line number hit rather than originally set',
@@ -687,7 +683,6 @@ describe('v8debugapi', () => {
              code.foo(1);
            });
          });
-
        });
 
     it('should capture state', (done) => {
@@ -1312,7 +1307,6 @@ describe('v8debugapi', () => {
              code.foo(3);
            });
          });
-
        });
 
     it('should be possible to set conditional breakpoints', (done) => {
@@ -1342,7 +1336,6 @@ describe('v8debugapi', () => {
           code.foo(5);
         });
       });
-
     });
 
     it('should be possible to set conditional breakpoints in coffeescript',
@@ -1637,7 +1630,6 @@ describe('v8debugapi', () => {
             assert.ifError(err3);
             throw new Error(message);
           });
-
         });
         process.nextTick(() => {
           code.foo(1);
@@ -1710,7 +1702,6 @@ describe('v8debugapi.findScriptsFuzzy', () => {
   });
 
   it('should do suffix matches correctly', () => {
-
     const TESTS = [
       // Exact match.
       {scriptPath: 'foo.js', fileList: ['/foo.js'], result: ['/foo.js']},

Original file line number	Diff line number	Diff line change
`@@ -174,7 +174,6 @@ class StateResolver {`
`174`	`174`	`}`
`175`	`175`	`}`
`176`	`176`	`this.evaluatedExpressions[index2] = evaluated;`
`177`		`-`
`178`	`177`	`});`
`179`	`178`	`}`
`180`	`179`	`// The frames are resolved after the evaluated expressions so that`
-Original file line number
+Diff line change
       module.start();
     });
   });
+-
 });