Fix erroneous socket connections to undefinedCHANGE_ME_PATH (#4799)

* only connect to socket if path has been replaced, fixes #4776

* upgrade sinon, do not instantiate sandbox

* add tests to ensure connect is not called when not in browser mode

* refactor once again to push connect to a separate module to make testable

* move initializing extension connection to init.js

- prevents situation where connect(…) is called when module is required
- simplifies handling automatic connection only when in the browser
without needing to modify globals


Co-authored-by: Brian Mann <brian.mann86@gmail.com>

Author: flotwig

packages/extension/app/background.coffee

@@ -2,10 +2,7 @@ map     = require("lodash/map")
 pick    = require("lodash/pick")
 once    = require("lodash/once")
 Promise = require("bluebird")
-{ client, circularParser } = require("@packages/socket/lib/browser")
-
-HOST = "CHANGE_ME_HOST"
-PATH = "CHANGE_ME_PATH"
+client  = require("./client")
 
 httpRe = /^http/
 
@@ -35,11 +32,7 @@ connect = (host, path) ->
     .catch (err) ->
       fail(id, err)
 
-  ws = client.connect(host, {
-    path: path,
-    transports: ["websocket"]
-    parser: circularParser
-  })
+  ws = client.connect(host, path)
 
   ws.on "automation:request", (id, msg, data) ->
     switch msg
@@ -68,10 +61,7 @@ connect = (host, path) ->
     ws.emit("automation:client:connected")
 
   return ws
-
-## initially connect
-connect(HOST, PATH)
-
+  
 automation = {
   connect
 

packages/extension/app/client.js

@@ -0,0 +1,18 @@
+const { client, circularParser } = require('@packages/socket/lib/browser')
+
+const connect = (host, path) => {
+  return client.connect(host, {
+    path,
+    transports: ['websocket'],
+    // @ts-ignore
+    parser: circularParser,
+  })
+}
+
+module.exports = {
+  connect,
+
+  socketIoClient: client,
+
+  socketIoParser: circularParser,
+}

packages/extension/app/init.js

@@ -0,0 +1,7 @@
+const background = require('./background.coffee')
+
+const HOST = 'CHANGE_ME_HOST'
+const PATH = 'CHANGE_ME_PATH'
+
+// immediately connect
+background.connect(HOST, PATH)

packages/extension/gulpfile.js

@@ -37,7 +37,7 @@ gulp.task('backup', () => {
 
 gulp.task('background', () => {
   return browserify({
-    entries: 'app/background.coffee',
+    entries: 'app/init.js',
     transform: coffeeify,
   })
   .bundle()

packages/extension/package.json

@@ -35,7 +35,8 @@
     "gulp": "4.0.2",
     "gulp-clean": "0.4.0",
     "gulp-rename": "1.4.0",
-    "sinon": "1.17.7",
+    "run-sequence": "1.2.2",
+    "sinon": "7.3.2",
     "sinon-chai": "3.3.0",
     "vinyl-source-stream": "2.0.0"
   },

packages/extension/test/integration/background_spec.coffee

@@ -112,14 +112,14 @@ describe "app/background", ->
     it "emits 'automation:client:connected'", (done) ->
       client = background.connect("http://localhost:#{PORT}", "/__socket.io")
 
-      @sandbox.spy(client, "emit")
+      sinon.spy(client, "emit")
 
       client.on "connect", _.once ->
         expect(client.emit).to.be.calledWith("automation:client:connected")
         done()
 
     it "listens to cookie changes", (done) ->
-      addListener = @sandbox.stub(chrome.cookies.onChanged, "addListener")
+      addListener = sinon.stub(chrome.cookies.onChanged, "addListener")
       client      = background.connect("http://localhost:#{PORT}", "/__socket.io")
 
       client.on "connect", _.once ->
@@ -128,10 +128,10 @@ describe "app/background", ->
 
   context "onChanged", ->
     it "does not emit when cause is overwrite", (done) ->
-      addListener = @sandbox.stub(chrome.cookies.onChanged, "addListener")
+      addListener = sinon.stub(chrome.cookies.onChanged, "addListener")
       client      = background.connect("http://localhost:#{PORT}", "/__socket.io")
 
-      @sandbox.spy(client, "emit")
+      sinon.spy(client, "emit")
 
       client.on "connect", _.once ->
         fn = addListener.getCall(0).args[0]
@@ -144,7 +144,7 @@ describe "app/background", ->
     it "emits 'automation:push:request'", (done) ->
       info = { cause: "explicit", cookie: {name: "foo", value: "bar"} }
 
-      addListener = @sandbox.stub(chrome.cookies.onChanged, "addListener").yieldsAsync(info)
+      addListener = sinon.stub(chrome.cookies.onChanged, "addListener").yieldsAsync(info)
       client      = background.connect("http://localhost:#{PORT}", "/__socket.io")
 
       client.on "connect", ->
@@ -156,7 +156,7 @@ describe "app/background", ->
 
   context ".getAll", ->
     it "resolves with specific cookie properties", ->
-      @sandbox.stub(chrome.cookies, "getAll")
+      sinon.stub(chrome.cookies, "getAll")
       .withArgs({domain: "localhost"})
       .yieldsAsync([
         {name: "foo", value: "f", path: "/", domain: "localhost", secure: true, httpOnly: true, expirationDate: 123}
@@ -175,11 +175,11 @@ describe "app/background", ->
       @code = "var s; (s = document.getElementById('__cypress-string')) && s.textContent"
 
     it "resolves on the 1st tab", ->
-      @sandbox.stub(chrome.tabs, "query")
+      sinon.stub(chrome.tabs, "query")
       .withArgs({windowType: "normal"})
       .yieldsAsync([tab1])
 
-      @sandbox.stub(chrome.tabs, "executeScript")
+      sinon.stub(chrome.tabs, "executeScript")
       .withArgs(tab1.id, {code: @code})
       .yieldsAsync(["1234"])
 
@@ -189,11 +189,11 @@ describe "app/background", ->
       })
 
     it "resolves on the 2nd tab", ->
-      @sandbox.stub(chrome.tabs, "query")
+      sinon.stub(chrome.tabs, "query")
       .withArgs({windowType: "normal"})
       .yieldsAsync([tab1, tab2])
 
-      @sandbox.stub(chrome.tabs, "executeScript")
+      sinon.stub(chrome.tabs, "executeScript")
       .withArgs(tab1.id, {code: @code})
       .yieldsAsync(["foobarbaz"])
       .withArgs(tab2.id, {code: @code})
@@ -205,7 +205,7 @@ describe "app/background", ->
       })
 
     it "filters out tabs that don't start with http", ->
-      @sandbox.stub(chrome.tabs, "query")
+      sinon.stub(chrome.tabs, "query")
       .yieldsAsync([tab3])
 
       background.query({
@@ -219,11 +219,11 @@ describe "app/background", ->
         expect(err).to.be.instanceof(Promise.RangeError)
 
     it "rejects if no tab matches", ->
-      @sandbox.stub(chrome.tabs, "query")
+      sinon.stub(chrome.tabs, "query")
       .withArgs({windowType: "normal"})
       .yieldsAsync([tab1, tab2])
 
-      @sandbox.stub(chrome.tabs, "executeScript")
+      sinon.stub(chrome.tabs, "executeScript")
       .withArgs(tab1.id, {code: @code})
       .yieldsAsync(["foobarbaz"])
       .withArgs(tab2.id, {code: @code})
@@ -241,7 +241,7 @@ describe "app/background", ->
         expect(err).to.be.instanceof(Promise.AggregateError)
 
     it "rejects if no tabs were found", ->
-      @sandbox.stub(chrome.tabs, "query")
+      sinon.stub(chrome.tabs, "query")
       .yieldsAsync([])
 
       background.query({
@@ -263,7 +263,7 @@ describe "app/background", ->
 
     describe "get:cookies", ->
       beforeEach ->
-        @sandbox.stub(chrome.cookies, "getAll")
+        sinon.stub(chrome.cookies, "getAll")
         .withArgs({domain: "google.com"})
         .yieldsAsync([{}, {}])
 
@@ -277,7 +277,7 @@ describe "app/background", ->
 
     describe "get:cookie", ->
       beforeEach ->
-        @sandbox.stub(chrome.cookies, "getAll")
+        sinon.stub(chrome.cookies, "getAll")
         .withArgs({domain: "google.com", name: "session"})
         .yieldsAsync([
           {name: "session", value: "key", path: "/login", domain: "google", secure: true, httpOnly: true, expirationDate: 123}
@@ -305,7 +305,7 @@ describe "app/background", ->
       beforeEach ->
         chrome.runtime.lastError = {message: "some error"}
 
-        @sandbox.stub(chrome.cookies, "set")
+        sinon.stub(chrome.cookies, "set")
         .withArgs({domain: "google.com", name: "session", value: "key", path: "/", secure: false, url: "http://google.com/"})
         .yieldsAsync(
           {name: "session", value: "key", path: "/", domain: "google", secure: false, httpOnly: false}
@@ -358,7 +358,7 @@ describe "app/background", ->
       beforeEach ->
         chrome.runtime.lastError = {message: "some error"}
 
-        @sandbox.stub(chrome.cookies, "getAll")
+        sinon.stub(chrome.cookies, "getAll")
         .withArgs({domain: "google.com"})
         .yieldsAsync([
           {name: "session", value: "key", path: "/",    domain: "google.com", secure: true, httpOnly: true, expirationDate: 123}
@@ -369,7 +369,7 @@ describe "app/background", ->
           {name: "shouldThrow", value: "key", path: "/assets", domain: "cdn.github.com", secure: false, httpOnly: true, expirationDate: 123}
         ])
 
-        @sandbox.stub(chrome.cookies, "remove")
+        sinon.stub(chrome.cookies, "remove")
         .withArgs({name: "session", url: "https://google.com/"})
         .yieldsAsync(
           {name: "session", url: "https://google.com/", storeId: "123"}
@@ -417,7 +417,7 @@ describe "app/background", ->
       beforeEach ->
         chrome.runtime.lastError = {message: "some error"}
 
-        @sandbox.stub(chrome.cookies, "getAll")
+        sinon.stub(chrome.cookies, "getAll")
         .withArgs({domain: "google.com", name: "session"})
         .yieldsAsync([
           {name: "session", value: "key", path: "/", domain: "google.com", secure: true, httpOnly: true, expirationDate: 123}
@@ -429,7 +429,7 @@ describe "app/background", ->
           {name: "shouldThrow", value: "key", path: "/assets", domain: "cdn.github.com", secure: false, httpOnly: true, expirationDate: 123}
         ])
 
-        @sandbox.stub(chrome.cookies, "remove")
+        sinon.stub(chrome.cookies, "remove")
         .withArgs({name: "session", url: "https://google.com/"})
         .yieldsAsync(
           {name: "session", url: "https://google.com/", storeId: "123"}
@@ -468,7 +468,7 @@ describe "app/background", ->
 
     describe "is:automation:client:connected", ->
       beforeEach ->
-        @sandbox.stub(chrome.tabs, "query")
+        sinon.stub(chrome.tabs, "query")
         .withArgs({url: "CHANGE_ME_HOST/*", windowType: "normal"})
         .yieldsAsync([])
 
@@ -482,13 +482,13 @@ describe "app/background", ->
 
     describe "take:screenshot", ->
       beforeEach ->
-        @sandbox.stub(chrome.windows, "getLastFocused").yieldsAsync({id: 1})
+        sinon.stub(chrome.windows, "getLastFocused").yieldsAsync({id: 1})
 
       afterEach ->
         delete chrome.runtime.lastError
 
       it "resolves with screenshot", (done) ->
-        @sandbox.stub(chrome.tabs, "captureVisibleTab").withArgs(1, {format: "png"}).yieldsAsync("foobarbaz")
+        sinon.stub(chrome.tabs, "captureVisibleTab").withArgs(1, {format: "png"}).yieldsAsync("foobarbaz")
 
         @socket.on "automation:response", (id, obj = {}) ->
           expect(id).to.eq(123)
@@ -499,7 +499,7 @@ describe "app/background", ->
 
       it "rejects with chrome.runtime.lastError", (done) ->
         chrome.runtime.lastError = {message: "some error"}
-        @sandbox.stub(chrome.tabs, "captureVisibleTab").withArgs(1, {format: "png"}).yieldsAsync(undefined)
+        sinon.stub(chrome.tabs, "captureVisibleTab").withArgs(1, {format: "png"}).yieldsAsync(undefined)
 
         @socket.on "automation:response", (id, obj = {}) ->
           expect(id).to.eq(123)

packages/extension/test/spec_helper.coffee

@@ -4,10 +4,8 @@ sinonChai = require("sinon-chai")
 
 chai.use(sinonChai)
 
+global.sinon = sinon
 global.expect = chai.expect
 
-beforeEach ->
-  @sandbox = sinon.sandbox.create()
-
 afterEach ->
-  @sandbox.restore()
+  sinon.restore()

packages/extension/test/unit/extension_spec.coffee

@@ -2,12 +2,13 @@ require("../spec_helper")
 
 exec      = require("child_process").exec
 fs        = require("fs-extra")
+EE        = require("events")
+eol       = require("eol")
 path      = require("path")
 Promise   = require("bluebird")
-eol       = require("eol")
 extension = require("../../index")
-cwd       = process.cwd()
 
+cwd = process.cwd()
 fs = Promise.promisifyAll(fs)
 exec = Promise.promisify(exec)
 
@@ -47,7 +48,7 @@ describe "Extension", ->
     beforeEach ->
       @src = path.join(cwd, "test", "helpers", "background.js")
 
-      @sandbox.stub(extension, "getPathToExtension")
+      sinon.stub(extension, "getPathToExtension")
       .withArgs("background.js").returns(@src)
 
     it "rewrites the background.js source", ->
@@ -98,4 +99,3 @@ describe "Extension", ->
         exec(cmd)
         .then (stdout) ->
           expect(stdout).to.eq("caljajdfkjjjdehjdoimjkkakekklcck")
-

packages/server/test/e2e/2_browser_path_spec.coffee

@@ -33,10 +33,10 @@ describe "e2e launching browsers by path", ->
     launcher.detect().then (browsers) =>
       browsers.find (browser) =>
         browser.family == "chrome"
-    .then (browser) =>
+    .tap (browser) =>
       if !browser
         throw new Error("A 'chrome' family browser must be installed for this test")
-      browser.path
+    .get("path")
     ## turn binary browser names ("google-chrome") into their absolute paths
     ## so that server recognizes them as a path, not as a browser name
     .then (absPath)