server, driver: fixes #886 redefine self + parent to prevent framebusting and clickjacking security measures (#1295)

* server, driver: fixes #886 redefine self + parent to prevent framebusting and clickjacking security measures - add modifyObjectiveCode config, true by default * driver: increase timeout for flaky test in CI * server, driver: moved obstructive code rewriting to the proxy layer, out of JS, providing more comprehensive fix * server, driver: fixes failing tests, make regexp much more conservative * server: increase security specificity... down the rabbit hole we go
2026-02-17 12:41:08 -06:00 · 2018-02-12 01:01:00 -05:00
parent 386bed4480
commit a648ee4686
12 changed files with 385 additions and 12 deletions
--- a/packages/driver/test/cypress/fixtures/security.html
+++ b/packages/driver/test/cypress/fixtures/security.html
@@ -0,0 +1,37 @@
+<!DOCTYPE html>
+<html>
+  <body>
+    testing security clickjacking and framebusting
+
+    <script type="text/javascript" src="security.js"></script>
+    <script type="text/javascript">
+      (function () {
+        function run () {
+          let div = document.createElement('div')
+          div.innerText = 'security triggered'
+          document.body.appendChild(div)
+        }
+
+        if (top != self) run()
+        if (top!=self) run()
+        if (top.location != self.location) run()
+        if (top.location != location) run()
+        if (parent.frames.length > 0) run()
+        if (window != top) run()
+        if (window.top !== window.self) run()
+        if (window.top!==window.self) run()
+        if (window.self != window.top) run()
+        if (window.top != window.self) run()
+        if (window["top"] != window["parent"]) run()
+        if (window['top'] != window['parent']) run()
+        if (window["top"] != self['parent']) run()
+        if (parent && parent != window) run()
+        if (parent && parent != self) run()
+        if (parent && window != parent) run()
+        if (parent && self != parent) run()
+        if (parent && parent.frames && parent.frames.length > 0) run()
+        if ((self.parent && !(self.parent === self)) && (self.parent.frames.length != 0)) run()
+      })()
+    </script>
+  </body>
+</html>
--- a/packages/driver/test/cypress/fixtures/security.js
+++ b/packages/driver/test/cypress/fixtures/security.js
@@ -0,0 +1,29 @@
+/* eslint-disable  */
+
+(function () {
+  function run () {
+    let div = document.createElement('div')
+    div.innerText = 'security triggered'
+    document.body.appendChild(div)
+  }
+
+  if (top != self) run()
+  if (top!=self) run()
+  if (top.location != self.location) run()
+  if (top.location != location) run()
+  if (parent.frames.length > 0) run()
+  if (window != top) run()
+  if (window.top !== window.self) run()
+  if (window.top!==window.self) run()
+  if (window.self != window.top) run()
+  if (window.top != window.self) run()
+  if (window["top"] != window["parent"]) run()
+  if (window['top'] != window['parent']) run()
+  if (window["top"] != self['parent']) run()
+  if (parent && parent != window) run()
+  if (parent && parent != self) run()
+  if (parent && window != parent) run()
+  if (parent && self != parent) run()
+  if (parent && parent.frames && parent.frames.length > 0) run()
+  if ((self.parent && !(self.parent === self)) && (self.parent.frames.length != 0)) run()
+})()
--- a/packages/driver/test/cypress/integration/e2e/security_spec.coffee
+++ b/packages/driver/test/cypress/integration/e2e/security_spec.coffee
@@ -0,0 +1,4 @@
+describe "security", ->
+  it "works by replacing obstructive code", ->
+    cy.visit("/fixtures/security.html")
+    cy.get("div").should("not.exist")
--- a/packages/server/lib/config.coffee
+++ b/packages/server/lib/config.coffee
@@ -33,7 +33,8 @@ folders = toWords """
 configKeys = toWords """
  animationDistanceThreshold      fileServerFolder
  baseUrl                         fixturesFolder
-  chromeWebSecurity               integrationFolder
+  chromeWebSecurity
+  modifyObstructiveCode           integrationFolder
  env                             pluginsFile
  hosts                           screenshotsFolder
  numTestsKeptInMemory            supportFile
@@ -100,7 +101,8 @@ defaults = {
  execTimeout:                   60000
  videoRecording:                true
  videoCompression:              32
-  videoUploadOnPasses:          true
+  videoUploadOnPasses:           true
+  modifyObstructiveCode:         true
  chromeWebSecurity:             true
  waitForAnimations:             true
  animationDistanceThreshold:    5
@@ -128,6 +130,7 @@ validationRules = {
  animationDistanceThreshold: v.isNumber
  baseUrl: v.isFullyQualifiedUrl
  blacklistHosts: v.isStringOrArrayOfStrings
+  modifyObstructiveCode: v.isBoolean
  chromeWebSecurity: v.isBoolean
  defaultCommandTimeout: v.isNumber
  env: v.isPlainObject
--- a/packages/server/lib/controllers/proxy.coffee
+++ b/packages/server/lib/controllers/proxy.coffee
@@ -10,6 +10,7 @@ cors          = require("../util/cors")
 buffers       = require("../util/buffers")
 rewriter      = require("../util/rewriter")
 blacklist     = require("../util/blacklist")
+conditional   = require("../util/conditional_stream")
 networkFailures = require("../util/network_failures")

 redirectRe  = /^30(1|2|3|7|8)$/
@@ -83,12 +84,13 @@ module.exports = {
    isInitial = req.cookies["__cypress.initial"] is "true"

    wantsInjection = null
+    wantsSecurityRemoved = null

-    resContentTypeIsHtml = (respHeaders) ->
+    resContentTypeIs = (respHeaders, str) ->
      contentType = respHeaders["content-type"]

-      ## make sure the response includes text/html
-      contentType and contentType.includes("text/html")
+      ## make sure the response includes string type
+      contentType and contentType.includes(str)

    reqAcceptsHtml = ->
      ## don't inject if this is an XHR from jquery
@@ -138,20 +140,34 @@ module.exports = {

      debug("received request response for #{remoteUrl} %o", { headers })

+      encoding = headers["content-encoding"]
+
+      isGzipped = encoding and encoding.includes("gzip")
+
      ## if there is nothing to inject then just
      ## bypass the stream buffer and pipe this back
      if not wantsInjection
-        str.pipe(thr)
+        ## only rewrite if we should
+        if config.modifyObstructiveCode and wantsSecurityRemoved
+          str
+          ## only unzip when it is already gzipped
+          .pipe(conditional(isGzipped, zlib.createGunzip()))
+          .pipe(rewriter.security())
+          .pipe(conditional(isGzipped, zlib.createGzip()))
+          .pipe(thr)
+        else
+          str.pipe(thr)
      else
        rewrite = (body) =>
-          rewriter.html(body.toString(), remoteState.domainName, wantsInjection)
+          rewriter.html(body.toString(), remoteState.domainName, wantsInjection, config.modifyObstructiveCode)

+        ## TODO: we can probably move this to the new
+        ## replacestream rewriter instead of using
+        ## a buffer
        injection = concat (body) =>
-          encoding = headers["content-encoding"]
-
          ## if we're gzipped that means we need to unzip
          ## this content first, inject, and the rezip
-          if encoding and encoding.includes("gzip")
+          if isGzipped
            zlib.gunzipAsync(body)
            .then(rewrite)
            .then(zlib.gzipAsync)
@@ -190,7 +206,7 @@ module.exports = {
      {headers, statusCode} = incomingRes

      wantsInjection ?= do ->
-        return false if not resContentTypeIsHtml(headers)
+        return false if not resContentTypeIs(headers, "text/html")

        return false if not resMatchesOriginPolicy(headers)

@@ -200,6 +216,9 @@ module.exports = {

        return "partial"

+      wantsSecurityRemoved ?= do ->
+        resContentTypeIs(headers, "application/javascript")
+
      @setResHeaders(req, res, incomingRes, wantsInjection)

      ## always proxy the cookies coming from the incomingRes
--- a/packages/server/lib/util/conditional_stream.coffee
+++ b/packages/server/lib/util/conditional_stream.coffee
@@ -0,0 +1,8 @@
+stream = require("stream")
+
+module.exports = (condition, dest) ->
+  ## if truthy return the dest stream
+  return dest if condition
+
+  ## else passthrough the stream
+  stream.PassThrough()
--- a/packages/server/lib/util/rewriter.coffee
+++ b/packages/server/lib/util/rewriter.coffee
@@ -1,10 +1,11 @@
 inject = require("./inject")
+security = require("./security")

 headRe      = /(<head.*?>)/i
 bodyRe      = /(<body.*?>)/i
 htmlRe      = /(<html.*?>)/i

-rewriteHtml = (html, domainName, wantsInjection) ->
+rewriteHtml = (html, domainName, wantsInjection, modifyObstructiveCode) ->
  replace = (re, str) ->
    html.replace(re, str)

@@ -15,6 +16,11 @@ rewriteHtml = (html, domainName, wantsInjection) ->
      when "partial"
        inject.partial(domainName)

+  ## strip clickjacking and framebusting
+  ## from the HTML if we've been told to
+  if modifyObstructiveCode
+    html = security.strip(html)
+
  switch
    when headRe.test(html)
      replace(headRe, "$1 #{htmlToInject}")
@@ -30,4 +36,6 @@ rewriteHtml = (html, domainName, wantsInjection) ->

 module.exports = {
  html: rewriteHtml
+
+  security: security.stripStream
 }
--- a/packages/server/lib/util/security.coffee
+++ b/packages/server/lib/util/security.coffee
@@ -0,0 +1,25 @@
+stream = require("stream")
+replacestream = require("replacestream")
+
+topOrParentRe = /.*(top|parent).*/g
+topOrParentEqualityBeforeRe = /((?:window|self).*[!=][=]\s*(?:(?:window|self)(?:\.|\[['"]))?)(top|parent)/g
+topOrParentEqualityAfterRe = /(top|parent)((?:["']\])?\s*[!=][=].*(?:window|self))/g
+topOrParentLocationOrFramesRe = /([^\da-zA-Z])(top|parent)([.])(location|frames)/g
+
+replacer = (match, p1, offset, string) ->
+  match
+  .replace(topOrParentEqualityBeforeRe, "$1self")
+  .replace(topOrParentEqualityAfterRe, "self$2")
+  .replace(topOrParentLocationOrFramesRe, "$1self$3$4")
+
+strip = (html) ->
+  html.replace(topOrParentRe, replacer)
+
+stripStream = ->
+  replacestream(topOrParentRe, replacer)
+
+module.exports = {
+  strip
+
+  stripStream
+}
--- a/packages/server/package.json
+++ b/packages/server/package.json
@@ -137,6 +137,7 @@
    "progress": "^1.1.8",
    "ramda": "^0.24.0",
    "randomstring": "^1.1.5",
+    "replacestream": "^4.0.3",
    "request": "2.79.0",
    "request-promise": "4.1.1",
    "return-deep-diff": "^0.2.9",
--- a/packages/server/test/integration/http_requests_spec.coffee
+++ b/packages/server/test/integration/http_requests_spec.coffee
@@ -2335,6 +2335,93 @@ describe "Routes", ->

            expect(body).to.eq("<html><head></head></html>")

+    context "security rewriting", ->
+      describe "on by default", ->
+        beforeEach ->
+          @setup("http://www.google.com")
+
+        it "replaces obstructive code in HTML files", ->
+          html = "<html><body><script>if (top !== self) { }</script></body></html>"
+
+          nock(@server._remoteOrigin)
+          .get("/index.html")
+          .reply 200, html, {
+            "Content-Type": "text/html"
+          }
+
+          @rp({
+            url: "http://www.google.com/index.html"
+            headers: {
+              "Cookie": "__cypress.initial=true"
+            }
+          })
+          .then (res) ->
+            expect(res.statusCode).to.eq(200)
+
+            expect(res.body).to.include(
+              "<script>if (self !== self) { }</script>"
+            )
+
+        it "replaces obstructive code in JS files", ->
+          nock(@server._remoteOrigin)
+          .get("/app.js")
+          .reply 200, "if (top !== self) { }", {
+            "Content-Type": "application/javascript"
+          }
+
+          @rp("http://www.google.com/app.js")
+          .then (res) ->
+            expect(res.statusCode).to.eq(200)
+
+            expect(res.body).to.eq(
+              "if (self !== self) { }"
+            )
+
+      describe "off with config", ->
+        beforeEach ->
+          @setup("http://www.google.com", {
+            config: {
+              modifyObstructiveCode: false
+            }
+          })
+
+        it "can turn off security rewriting for HTML", ->
+          html = "<html><body><script>if (top !== self) { }</script></body></html>"
+
+          nock(@server._remoteOrigin)
+          .get("/index.html")
+          .reply 200, html, {
+            "Content-Type": "text/html"
+          }
+
+          @rp({
+            url: "http://www.google.com/index.html"
+            headers: {
+              "Cookie": "__cypress.initial=true"
+            }
+          })
+          .then (res) ->
+            expect(res.statusCode).to.eq(200)
+
+            expect(res.body).to.include(
+              "<script>if (top !== self) { }</script>"
+            )
+
+        it "does not replaces obstructive code in JS files", ->
+          nock(@server._remoteOrigin)
+          .get("/app.js")
+          .reply 200, "if (top !== self) { }", {
+            "Content-Type": "application/javascript"
+          }
+
+          @rp("http://www.google.com/app.js")
+          .then (res) ->
+            expect(res.statusCode).to.eq(200)
+
+            expect(res.body).to.eq(
+              "if (top !== self) { }"
+            )
+
    context "FQDN rewriting", ->
      beforeEach ->
        @setup("http://www.google.com")
--- a/packages/server/test/unit/config_spec.coffee
+++ b/packages/server/test/unit/config_spec.coffee
@@ -121,6 +121,16 @@ describe "lib/config", ->
          @expectValidationFails("be a boolean")
          @expectValidationFails("the value was: 42")

+      context "modifyObstructiveCode", ->
+        it "passes if a boolean", ->
+          @setup({modifyObstructiveCode: false})
+          @expectValidationPasses()
+
+        it "fails if not a boolean", ->
+          @setup({modifyObstructiveCode: 42})
+          @expectValidationFails("be a boolean")
+          @expectValidationFails("the value was: 42")
+
      context "defaultCommandTimeout", ->
        it "passes if a number", ->
          @setup({defaultCommandTimeout: 10})
@@ -573,6 +583,9 @@ describe "lib/config", ->
    it "screenshotOnHeadlessFailure=true", ->
      @defaults "screenshotOnHeadlessFailure", true

+    it "modifyObstructiveCode=true", ->
+      @defaults "modifyObstructiveCode", true
+
    it "supportFile=false", ->
      @defaults "supportFile", false, {supportFile: false}

@@ -718,6 +731,7 @@ describe "lib/config", ->
            animationDistanceThreshold: { value: 5, from: "default" },
            trashAssetsBeforeHeadlessRuns: { value: true, from: "default" },
            watchForFileChanges:        { value: true, from: "default" },
+            modifyObstructiveCode:      { value: true, from: "default" },
            chromeWebSecurity:          { value: true, from: "default" },
            viewportWidth:              { value: 1000, from: "default" },
            viewportHeight:             { value: 660, from: "default" },
@@ -776,6 +790,7 @@ describe "lib/config", ->
            screenshotOnHeadlessFailure:{ value: true, from: "default" },
            trashAssetsBeforeHeadlessRuns: { value: true, from: "default" },
            watchForFileChanges:        { value: true, from: "default" },
+            modifyObstructiveCode:      { value: true, from: "default" },
            chromeWebSecurity:          { value: true, from: "default" },
            viewportWidth:              { value: 1000, from: "default" },
            viewportHeight:             { value: 660, from: "default" },
--- a/packages/server/test/unit/security_spec.coffee
+++ b/packages/server/test/unit/security_spec.coffee
@@ -0,0 +1,137 @@
+require("../spec_helper")
+
+concat = require("concat-stream")
+security = require("#{root}lib/util/security")
+
+original = """
+<html>
+  <body>
+    top1
+    settop
+    settopbox
+    parent1
+    grandparent
+    grandparents
+
+    <div style="left: 1500px; top: 0px;"></div>
+    <div style="left: 1500px; top : 0px;"></div>
+    <div style="left: 1500px; top  : 0px;"></div>
+
+    parent()
+    foo.parent()
+    top()
+    foo.top()
+    foo("parent")
+    foo("top")
+
+    const parent = () => { bar: 'bar' }
+
+    parent.bar
+
+    <script type="text/javascript">
+      if (top != self) run()
+      if (top!=self) run()
+      if (top.location != self.location) run()
+      if (top.location != location) run()
+      if (parent.frames.length > 0) run()
+      if (window != top) run()
+      if (window.top !== window.self) run()
+      if (window.top!==window.self) run()
+      if (window.self != window.top) run()
+      if (window.top != window.self) run()
+      if (window["top"] != window["parent"]) run()
+      if (window['top'] != window['parent']) run()
+      if (window["top"] != self['parent']) run()
+      if (parent && parent != window) run()
+      if (parent && parent != self) run()
+      if (parent && window != parent) run()
+      if (parent && self != parent) run()
+      if (parent && parent.frames && parent.frames.length > 0) run()
+      if ((self.parent && !(self.parent === self)) && (self.parent.frames.length != 0)) run()
+      if (parent !== null && parent.tag !== 'HostComponent' && parent.tag !== 'HostRoot') { }
+      if (null !== parent && parent.tag !== 'HostComponent' && parent.tag !== 'HostRoot') { }
+      if (top===self) return
+      if (top==self) return
+    </script>
+  </body>
+</html>
+"""
+
+expected = """
+<html>
+  <body>
+    top1
+    settop
+    settopbox
+    parent1
+    grandparent
+    grandparents
+
+    <div style="left: 1500px; top: 0px;"></div>
+    <div style="left: 1500px; top : 0px;"></div>
+    <div style="left: 1500px; top  : 0px;"></div>
+
+    parent()
+    foo.parent()
+    top()
+    foo.top()
+    foo("parent")
+    foo("top")
+
+    const parent = () => { bar: 'bar' }
+
+    parent.bar
+
+    <script type="text/javascript">
+      if (self != self) run()
+      if (self!=self) run()
+      if (self.location != self.location) run()
+      if (self.location != location) run()
+      if (self.frames.length > 0) run()
+      if (window != self) run()
+      if (window.self !== window.self) run()
+      if (window.self!==window.self) run()
+      if (window.self != window.self) run()
+      if (window.self != window.self) run()
+      if (window["self"] != window["self"]) run()
+      if (window['self'] != window['self']) run()
+      if (window["self"] != self['self']) run()
+      if (parent && self != window) run()
+      if (parent && self != self) run()
+      if (parent && window != self) run()
+      if (parent && self != self) run()
+      if (parent && self.frames && self.frames.length > 0) run()
+      if ((self.parent && !(self.self === self)) && (self.self.frames.length != 0)) run()
+      if (parent !== null && parent.tag !== 'HostComponent' && parent.tag !== 'HostRoot') { }
+      if (null !== parent && parent.tag !== 'HostComponent' && parent.tag !== 'HostRoot') { }
+      if (self===self) return
+      if (self==self) return
+    </script>
+  </body>
+</html>
+"""
+
+describe "lib/util/security", ->
+  context ".strip", ->
+    it "replaces obstructive code", ->
+      expect(security.strip(original)).to.eq(expected)
+
+  context ".stripStream", ->
+    it "replaces obstructive code", (done) ->
+      haystacks = original.split("\n")
+
+      replacer = security.stripStream()
+
+      replacer.pipe concat {encoding: "string"}, (str) ->
+        str = str.trim()
+
+        try
+          expect(str).to.eq(expected)
+          done()
+        catch err
+          done(err)
+
+      haystacks.forEach (haystack) ->
+        replacer.write(haystack + "\n")
+
+      replacer.end()