Use is-html as a fallback to check if cy.visit() response is HTML (#…

…4321) * use is-html as a fallback to check if response is HTML * end response with passthru stream * clean up network_failures * add test that visit passes with undef content-type * handle empty responses too * try to fix dtslint: bump package version 2 02-00000876 * fix failing tests via develop merge Co-authored-by: Ben Kucera <14625260+Bkucera@users.noreply.github.com> Co-authored-by: Brian Mann <brian.mann86@gmail.com>
cypress-io · Jun 19, 2019 · 60318a7 · 60318a7
1 parent 0c8df29
commit 60318a7
Show file tree

Hide file tree

Showing 8 changed files with 84 additions and 17 deletions.
diff --git a/cli/package.json b/cli/package.json
@@ -100,4 +100,4 @@
     "index.js",
     "types/**/*.d.ts"
   ]
-}
+}
diff --git a/packages/driver/test/cypress/integration/commands/navigation_spec.coffee b/packages/driver/test/cypress/integration/commands/navigation_spec.coffee
@@ -541,6 +541,11 @@ describe "src/cy/commands/navigation", ->
         .then ->
           expect(backend).to.be.calledWithMatch("resolve:url", "http://localhost:3500/timeout", { auth })
 
+    ## https://github.com/cypress-io/cypress/issues/1727
+    it "can visit a page with undefined content type and html-shaped body", ->
+      cy
+        .visit("http://localhost:3500/undefined-content-type")
+
     describe "when only hashes are changing", ->
       it "short circuits the visit if the page will not refresh", ->
         count = 0

diff --git a/packages/driver/test/support/server.coffee b/packages/driver/test/support/server.coffee
@@ -72,6 +72,9 @@ niv.install("react-dom@15.6.1")
     res.setHeader('Content-Type', 'text/html; charset=utf-8,text/html')
     res.end("<html><head><title>Test</title></head><body><center>Hello</center></body></html>")
 
+  app.get '/undefined-content-type', (req, res) ->
+    res.end("<html>some stuff that looks like<span>html</span></html>")
+
   app.all '/dump-method', (req, res) ->
     res.send("<html><body>request method: #{req.method}</body></html>")
 

diff --git a/packages/server/lib/controllers/proxy.coffee b/packages/server/lib/controllers/proxy.coffee
@@ -11,7 +11,6 @@ buffers       = require("../util/buffers")
 rewriter      = require("../util/rewriter")
 blacklist     = require("../util/blacklist")
 conditional   = require("../util/conditional_stream")
-networkFailures = require("../util/network_failures")
 
 REDIRECT_STATUS_CODES = [301, 302, 303, 307, 308]
 NO_BODY_STATUS_CODES = [204, 304]

diff --git a/packages/server/lib/file_server.coffee b/packages/server/lib/file_server.coffee
@@ -29,6 +29,7 @@ onRequest = (req, res, fileServerFolder) ->
   })
   .on "error", (err) ->
     res.setHeader("x-cypress-file-server-error", true)
+    res.setHeader("content-type", "text/html")
     res.statusCode = err.status
     res.end(networkFailures.get(file, err.status))
   .pipe(res)

diff --git a/packages/server/lib/server.coffee b/packages/server/lib/server.coffee
@@ -2,11 +2,13 @@ _            = require("lodash")
 exphbs       = require("express-handlebars")
 url          = require("url")
 http         = require("http")
+concatStream = require("concat-stream")
 cookie       = require("cookie")
 stream       = require("stream")
 express      = require("express")
 Promise      = require("bluebird")
 evilDns      = require("evil-dns")
+isHtml       = require("is-html")
 httpProxy    = require("http-proxy")
 la           = require("lazy-ass")
 check        = require("check-more-types")
@@ -34,6 +36,15 @@ fileServer   = require("./file_server")
 DEFAULT_DOMAIN_NAME    = "localhost"
 fullyQualifiedRe       = /^https?:\/\//
 
+isResponseHtml = (contentType, responseBuffer) ->
+  if contentType
+    return contentType is "text/html"
+
+  if body = _.invoke(responseBuffer, 'toString')
+    return isHtml(body)
+
+  return false
+
 setProxiedUrl = (req) ->
   ## bail if we've already proxied the url
   return if req.proxiedUrl
@@ -418,11 +429,9 @@ class Server
 
               isOk        = statusIs2xxOrAllowedFailure()
               contentType = headersUtil.getContentType(incomingRes)
-              isHtml      = contentType is "text/html"
 
               details = {
                 isOkStatusCode: isOk
-                isHtml
                 contentType
                 url: newUrl
                 status: incomingRes.statusCode
@@ -439,19 +448,22 @@ class Server
 
               debug("setting details resolving url %o", details)
 
-              ## this will allow us to listen to `str`'s `end` event by putting it in flowing mode
-              responseBuffer = stream.PassThrough({
-                ## buffer forever - node's default is only to buffer 16kB
-                highWaterMark: Infinity
-              })
-
-              str.pipe(responseBuffer)
-
-              str.on "end", =>
+              concatStr = concatStream (responseBuffer) =>
                 ## buffer the entire response before resolving.
                 ## this allows us to detect & reject ETIMEDOUT errors
                 ## where the headers have been sent but the
                 ## connection hangs before receiving a body.
+
+                if !_.get(responseBuffer, 'length')
+                  ## concatStream can yield an empty array, which is
+                  ## not a valid chunk
+                  responseBuffer = undefined
+
+                ## if there is not a content-type, try to determine
+                ## if the response content is HTML-like
+                ## https://github.com/cypress-io/cypress/issues/1727
+                details.isHtml = isResponseHtml(contentType, responseBuffer)
+
                 debug("resolve:url response ended, setting buffer %o", { newUrl, details })
 
                 details.totalTime = new Date() - startTime
@@ -460,15 +472,21 @@ class Server
                 ## frontend so that the driver can be in control of
                 ## when the server should cache the request buffer
                 ## and set the domain vs not
-                if isOk and isHtml
+                if isOk and details.isHtml
                   ## reset the domain to the new url if we're not
                   ## handling a local file
                   @_onDomainSet(newUrl, options) if not handlingLocalFile
 
+                  responseBufferStream = new stream.PassThrough({
+                    highWaterMark: Number.MAX_SAFE_INTEGER
+                  })
+
+                  responseBufferStream.end(responseBuffer)
+
                   buffers.set({
                     url: newUrl
                     jar: jar
-                    stream: responseBuffer
+                    stream: responseBufferStream
                     details: details
                     originalUrl: originalUrl
                     response: incomingRes
@@ -479,6 +497,8 @@ class Server
                   restorePreviousState()
 
                 resolve(details)
+
+              str.pipe(concatStr)
             .catch(onReqError)
 
       restorePreviousState = =>

diff --git a/packages/server/package.json b/packages/server/package.json
@@ -143,6 +143,7 @@
     "human-interval": "0.1.6",
     "image-size": "0.7.4",
     "is-fork-pr": "2.3.0",
+    "is-html": "2.0.0",
     "jimp": "0.6.4",
     "jsonlint": "1.6.3",
     "konfig": "0.2.1",

diff --git a/packages/server/test/integration/server_spec.coffee b/packages/server/test/integration/server_spec.coffee
@@ -240,8 +240,8 @@ describe "Server", ->
         .then (obj = {}) ->
           expectToEqDetails(obj, {
             isOkStatusCode: false
-            isHtml: false
-            contentType: undefined
+            isHtml: true
+            contentType: "text/html"
             url: "http://localhost:2000/does-not-exist"
             originalUrl: "/does-not-exist"
             filePath: Fixtures.projectPath("no-server/dev/does-not-exist")
@@ -406,6 +406,44 @@ describe "Server", ->
             cookies: []
           })
 
+      it "yields isHtml true for HTML-shaped responses", ->
+        nock("http://example.com")
+        .get("/")
+        .reply(200, "<html>foo</html>")
+
+        @server._onResolveUrl("http://example.com", {}, @automationRequest)
+        .then (obj = {}) ->
+          expectToEqDetails(obj, {
+            isOkStatusCode: true
+            isHtml: true
+            contentType: undefined
+            url: "http://example.com/"
+            originalUrl: "http://example.com/"
+            status: 200
+            statusText: "OK"
+            redirects: []
+            cookies: []
+          })
+
+      it "yields isHtml false for non-HTML-shaped responses", ->
+        nock("http://example.com")
+        .get("/")
+        .reply(200, '{ foo: "bar" }')
+
+        @server._onResolveUrl("http://example.com", {}, @automationRequest)
+        .then (obj = {}) ->
+          expectToEqDetails(obj, {
+            isOkStatusCode: true
+            isHtml: false
+            contentType: undefined
+            url: "http://example.com/"
+            originalUrl: "http://example.com/"
+            status: 200
+            statusText: "OK"
+            redirects: []
+            cookies: []
+          })
+
       it "can follow multiple http redirects", ->
         nock("http://espn.com")
         .get("/")
-Original file line number
+Diff line change
@@ Expand Up / @@ -100,4 +100,4 @@ @@
         "index.js",
         "types/**/*.d.ts"
       ]
-    }
+    }