From eab1730dbb055efa2ad02bc2ae36f5a9ac5ef8d9 Mon Sep 17 00:00:00 2001
From: Emily Rohrbough <emilyrohrbough@users.noreply.github.com>
Date: Mon, 6 Nov 2023 14:06:59 -0600
Subject: [PATCH] fix: handle download from element missing download attribute
 (#28222)

---
 cli/CHANGELOG.md                              |  2 +
 packages/app/src/runner/event-manager.ts      |  3 ++
 .../cypress/e2e/cypress/downloads.cy.ts       | 52 ++++++++++++++++++-
 .../driver/cypress/fixtures/downloads.html    | 15 ++++++
 .../cypress/fixtures/downloads_records.csv    |  2 +
 packages/driver/src/cy/commands/navigation.ts | 18 +++++++
 packages/driver/src/cypress.ts                |  3 ++
 packages/driver/src/cypress/downloads.ts      | 10 +++-
 packages/extension/app/v2/background.js       | 16 ++++--
 .../test/integration/background_spec.js       | 17 ++++++
 packages/server/lib/automation/automation.ts  |  1 +
 packages/server/lib/browsers/chrome.ts        | 17 +++---
 packages/server/lib/browsers/electron.ts      | 12 +++--
 .../server/test/unit/browsers/chrome_spec.js  | 15 ++++++
 .../test/unit/browsers/electron_spec.js       | 23 +++++++-
 15 files changed, 188 insertions(+), 18 deletions(-)
 create mode 100644 packages/driver/cypress/fixtures/downloads.html
 create mode 100644 packages/driver/cypress/fixtures/downloads_records.csv

diff --git a/cli/CHANGELOG.md b/cli/CHANGELOG.md
index 5272767f891e..1d2b998a51ca 100644
--- a/cli/CHANGELOG.md
+++ b/cli/CHANGELOG.md
@@ -5,6 +5,8 @@ _Released 11/7/2023 (PENDING)_
 
 **Bugfixes:**
 
+- Fixed an issue where clicking a link to download a file could cause a page load timeout when the download attribute was missing. Note: download behaviors in experimental Webkit are still an issue. Fixes [#14857](https://github.com/cypress-io/cypress/issues/14857).
+- Fixed an issue to account for canceled and failed downloads to correctly reflect these status in Command log as a download failure where previously it would be pending. Fixed in [#28222](https://github.com/cypress-io/cypress/pull/28222).
 - Fixed an issue determining visibility when an element is hidden by an ancestor with a shared edge. Fixes [#27514](https://github.com/cypress-io/cypress/issues/27514).
 - We now pass a flag to Chromium browsers to disable Chrome translation, both the manual option and the popup prompt, when a page with a differing language is detected. Fixes [#28225](https://github.com/cypress-io/cypress/issues/28225).
 - Fixed an issue where in chromium based browsers, global style updates can trigger flooding of font face requests in DevTools and Test Replay. This can affect performance due to the flooding of messages in CDP. Fixes [#28150](https://github.com/cypress-io/cypress/issues/28150) and [#28215](https://github.com/cypress-io/cypress/issues/28215).
diff --git a/packages/app/src/runner/event-manager.ts b/packages/app/src/runner/event-manager.ts
index eb02090ad72f..13dca2e60f5a 100644
--- a/packages/app/src/runner/event-manager.ts
+++ b/packages/app/src/runner/event-manager.ts
@@ -148,6 +148,9 @@ export class EventManager {
         case 'complete:download':
           Cypress.downloads.end(data)
           break
+        case 'canceled:download':
+          Cypress.downloads.end(data, true)
+          break
         default:
           break
       }
diff --git a/packages/driver/cypress/e2e/cypress/downloads.cy.ts b/packages/driver/cypress/e2e/cypress/downloads.cy.ts
index 46e915836a88..078d5199cade 100644
--- a/packages/driver/cypress/e2e/cypress/downloads.cy.ts
+++ b/packages/driver/cypress/e2e/cypress/downloads.cy.ts
@@ -4,6 +4,7 @@ describe('src/cypress/downloads', () => {
   let log
   let snapshot
   let end
+  let error
   let downloads
   let downloadItem = {
     id: '1',
@@ -11,13 +12,16 @@ describe('src/cypress/downloads', () => {
     url: 'http://localhost:1234/location.csv',
     mime: 'text/csv',
   }
+  let action
 
   beforeEach(() => {
     end = cy.stub()
-    snapshot = cy.stub().returns({ end })
+    error = cy.stub()
+    snapshot = cy.stub().returns({ end, error })
     log = cy.stub().returns({ snapshot })
+    action = cy.stub()
 
-    downloads = $Downloads.create({ log })
+    downloads = $Downloads.create({ action, log })
   })
 
   context('#start', () => {
@@ -51,9 +55,21 @@ describe('src/cypress/downloads', () => {
       downloads.start(downloadItem)
       downloads.end({ id: '1' })
 
+      expect(action).to.be.calledWith('app:download:received')
+      expect(snapshot).to.be.called
       expect(end).to.be.called
     })
 
+    it('fails with snapshot if matching log exists', () => {
+      downloads.start(downloadItem)
+      downloads.end({ id: '1' }, true)
+
+      expect(action).to.be.calledWith('app:download:received')
+      expect(snapshot).to.be.called
+      expect(end).not.to.be.called
+      expect(error).to.be.called
+    })
+
     it('is a noop if matching log does not exist', () => {
       downloads.end({ id: '1' })
 
@@ -62,3 +78,35 @@ describe('src/cypress/downloads', () => {
     })
   })
 })
+
+describe('download behavior', () => {
+  beforeEach(() => {
+    cy.visit('/fixtures/downloads.html')
+  })
+
+  it('downloads from anchor tag with download attribute', () => {
+    cy.exec(`rm -f ${Cypress.config('downloadsFolder')}/downloads_records.csv`)
+    cy.readFile(`${Cypress.config('downloadsFolder')}/downloads_records.csv`).should('not.exist')
+
+    // trigger download
+    cy.get('[data-cy=download-csv]').click()
+    cy.readFile(`${Cypress.config('downloadsFolder')}/downloads_records.csv`)
+    .should('contain', '"Joe","Smith"')
+  })
+
+  it('downloads from anchor tag without download attribute', { browser: '!webkit' }, () => {
+    cy.exec(`rm -f ${Cypress.config('downloadsFolder')}/downloads_records.csv`)
+    cy.readFile(`${Cypress.config('downloadsFolder')}/downloads_records.csv`).should('not.exist')
+
+    // trigger download
+    cy.get('[data-cy=download-without-download-attr]').click()
+    cy.readFile(`${Cypress.config('downloadsFolder')}/downloads_records.csv`)
+    .should('contain', '"Joe","Smith"')
+  })
+
+  it('invalid download path from anchor tag with download attribute', () => {
+    // attempt to download
+    cy.get('[data-cy=invalid-download]').click()
+    cy.readFile(`${Cypress.config('downloadsFolder')}/downloads_does_not_exist.csv`).should('not.exist')
+  })
+})
diff --git a/packages/driver/cypress/fixtures/downloads.html b/packages/driver/cypress/fixtures/downloads.html
new file mode 100644
index 000000000000..b5042407e355
--- /dev/null
+++ b/packages/driver/cypress/fixtures/downloads.html
@@ -0,0 +1,15 @@
+<!DOCTYPE html>
+<html>
+<head>
+</head>
+<body>
+  <h3>Download CSV</h3>
+  <a data-cy="download-csv" href="downloads_records.csv" download>downloads_records.csv</a>
+
+  <h3>Download CSV</h3>
+  <a data-cy="download-without-download-attr" href="downloads_records.csv">download csv from anchor tag without download attr</a>
+
+  <h3>Download CSV</h3>
+  <a data-cy="invalid-download" href="downloads_does_not_exist.csv" download>broken download link</a>
+</body>
+</html>
diff --git a/packages/driver/cypress/fixtures/downloads_records.csv b/packages/driver/cypress/fixtures/downloads_records.csv
new file mode 100644
index 000000000000..54b734d103e6
--- /dev/null
+++ b/packages/driver/cypress/fixtures/downloads_records.csv
@@ -0,0 +1,2 @@
+"First name","Last name","Occupation","Age","City","State"
+"Joe","Smith","student",20,"Boston","MA"
\ No newline at end of file
diff --git a/packages/driver/src/cy/commands/navigation.ts b/packages/driver/src/cy/commands/navigation.ts
index 73e0c66f2daf..1d3ec096832c 100644
--- a/packages/driver/src/cy/commands/navigation.ts
+++ b/packages/driver/src/cy/commands/navigation.ts
@@ -302,6 +302,23 @@ const stabilityChanged = async (Cypress, state, config, stable) => {
     debug('waiting for window:load')
 
     const promise = new Promise((resolve) => {
+      const handleDownloadUnloadEvent = () => {
+        cy.state('onPageLoadErr', null)
+        cy.isStable(true, 'download')
+
+        options._log
+        .set({
+          message: 'download fired beforeUnload event',
+          consoleProps () {
+            return {
+              Note: 'This event fired when the download was initiated.',
+            }
+          },
+        }).snapshot().end()
+
+        resolve()
+      }
+
       const onWindowLoad = ({ url }) => {
         const href = state('autLocation').href
         const count = getRedirectionCount(href)
@@ -351,6 +368,7 @@ const stabilityChanged = async (Cypress, state, config, stable) => {
         }
       }
 
+      cy.once('download:received', handleDownloadUnloadEvent)
       cy.once('internal:window:load', onInternalWindowLoad)
 
       // If this request is still pending after the test run, resolve it, no commands were waiting on its result.
diff --git a/packages/driver/src/cypress.ts b/packages/driver/src/cypress.ts
index c45b9462a4ff..34d5351e93de 100644
--- a/packages/driver/src/cypress.ts
+++ b/packages/driver/src/cypress.ts
@@ -712,6 +712,9 @@ class $Cypress {
       case 'app:navigation:changed':
         return this.emit('navigation:changed', ...args)
 
+      case 'app:download:received':
+        return this.emit('download:received')
+
       case 'app:form:submitted':
         return this.emit('form:submitted', args[0])
 
diff --git a/packages/driver/src/cypress/downloads.ts b/packages/driver/src/cypress/downloads.ts
index d33c353cee8b..25436855890c 100644
--- a/packages/driver/src/cypress/downloads.ts
+++ b/packages/driver/src/cypress/downloads.ts
@@ -25,11 +25,17 @@ export default {
       return log.snapshot()
     }
 
-    const end = ({ id }) => {
+    const end = ({ id }, isCanceled = false) => {
+      Cypress.action('app:download:received')
+
       const log = logs[id]
 
       if (log) {
-        log.snapshot().end()
+        if (isCanceled) {
+          log.snapshot().error(new Error('Download was canceled.'))
+        } else {
+          log.snapshot().end()
+        }
 
         // don't need this anymore since the download has ended
         // and won't change anymore
diff --git a/packages/extension/app/v2/background.js b/packages/extension/app/v2/background.js
index e70d5729f6e5..6fe74d24dcf0 100644
--- a/packages/extension/app/v2/background.js
+++ b/packages/extension/app/v2/background.js
@@ -51,11 +51,19 @@ const connect = function (host, path, extraOpts) {
     })
 
     browser.downloads.onChanged.addListener((downloadDelta) => {
-      if ((downloadDelta.state || {}).current !== 'complete') return
+      const state = (downloadDelta.state || {}).current
 
-      ws.emit('automation:push:request', 'complete:download', {
-        id: `${downloadDelta.id}`,
-      })
+      if (state === 'complete') {
+        ws.emit('automation:push:request', 'complete:download', {
+          id: `${downloadDelta.id}`,
+        })
+      }
+
+      if (state === 'canceled') {
+        ws.emit('automation:push:request', 'canceled:download', {
+          id: `${downloadDelta.id}`,
+        })
+      }
     })
   })
 
diff --git a/packages/extension/test/integration/background_spec.js b/packages/extension/test/integration/background_spec.js
index b0d4617a275b..1dd316c89198 100644
--- a/packages/extension/test/integration/background_spec.js
+++ b/packages/extension/test/integration/background_spec.js
@@ -231,6 +231,23 @@ describe('app/background', () => {
       })
     })
 
+    it('onChanged emits automation:push:request canceled:download', async function () {
+      const downloadDelta = {
+        id: '1',
+        state: {
+          current: 'canceled',
+        },
+      }
+
+      sinon.stub(browser.downloads.onChanged, 'addListener').yields(downloadDelta)
+
+      const ws = await this.connect()
+
+      expect(ws.emit).to.be.calledWith('automation:push:request', 'canceled:download', {
+        id: `${downloadDelta.id}`,
+      })
+    })
+
     it('onChanged does not emit if state does not exist', async function () {
       const downloadDelta = {
         id: '1',
diff --git a/packages/server/lib/automation/automation.ts b/packages/server/lib/automation/automation.ts
index 53d4bbf0f94f..22b2abfaa15f 100644
--- a/packages/server/lib/automation/automation.ts
+++ b/packages/server/lib/automation/automation.ts
@@ -127,6 +127,7 @@ export class Automation {
         case 'change:cookie':
           return this.cookies.changeCookie(data)
         case 'create:download':
+        case 'canceled:download':
         case 'complete:download':
           return data
         default:
diff --git a/packages/server/lib/browsers/chrome.ts b/packages/server/lib/browsers/chrome.ts
index a4764bdac5ba..8ff81eab95cd 100644
--- a/packages/server/lib/browsers/chrome.ts
+++ b/packages/server/lib/browsers/chrome.ts
@@ -302,11 +302,17 @@ const _handleDownloads = async function (client, downloadsFolder: string, automa
   })
 
   client.on('Page.downloadProgress', (data) => {
-    if (data.state !== 'completed') return
+    if (data.state === 'completed') {
+      automation.push('complete:download', {
+        id: data.guid,
+      })
+    }
 
-    automation.push('complete:download', {
-      id: data.guid,
-    })
+    if (data.state === 'canceled') {
+      automation.push('canceled:download', {
+        id: data.guid,
+      })
+    }
   })
 
   await client.send('Page.setDownloadBehavior', {
@@ -528,13 +534,12 @@ export = {
 
     await pageCriClient.send('Page.enable')
 
-    await utils.handleDownloadLinksViaCDP(pageCriClient, automation)
-
     await options['onInitializeNewBrowserTab']?.()
 
     await Promise.all([
       options.videoApi && this._recordVideo(cdpAutomation, options.videoApi, Number(options.browser.majorVersion)),
       this._handleDownloads(pageCriClient, options.downloadsFolder, automation),
+      utils.handleDownloadLinksViaCDP(pageCriClient, automation),
     ])
 
     await this._navigateUsingCRI(pageCriClient, url)
diff --git a/packages/server/lib/browsers/electron.ts b/packages/server/lib/browsers/electron.ts
index 52a7b1e33c73..fd4f819b4c8c 100644
--- a/packages/server/lib/browsers/electron.ts
+++ b/packages/server/lib/browsers/electron.ts
@@ -337,7 +337,7 @@ export = {
   },
 
   _handleDownloads (win, dir, automation) {
-    const onWillDownload = (event, downloadItem) => {
+    const onWillDownload = (_event, downloadItem) => {
       const savePath = path.join(dir, downloadItem.getFilename())
 
       automation.push('create:download', {
@@ -347,8 +347,14 @@ export = {
         url: downloadItem.getURL(),
       })
 
-      downloadItem.once('done', () => {
-        automation.push('complete:download', {
+      downloadItem.once('done', (_event, state) => {
+        if (state === 'completed') {
+          return automation.push('complete:download', {
+            id: downloadItem.getETag(),
+          })
+        }
+
+        automation.push('canceled:download', {
           id: downloadItem.getETag(),
         })
       })
diff --git a/packages/server/test/unit/browsers/chrome_spec.js b/packages/server/test/unit/browsers/chrome_spec.js
index 987afc4c8313..20dc11dfe6d7 100644
--- a/packages/server/test/unit/browsers/chrome_spec.js
+++ b/packages/server/test/unit/browsers/chrome_spec.js
@@ -338,6 +338,21 @@ describe('lib/browsers/chrome', () => {
           })
         })
       })
+
+      it('pushes canceled:download when download is incomplete', function () {
+        const downloadData = {
+          guid: '1',
+          state: 'canceled',
+        }
+        const options = { downloadsFolder: 'downloads' }
+
+        return this.onCriEvent('Page.downloadProgress', downloadData, options)
+        .then(() => {
+          expect(this.automation.push).to.be.calledWith('canceled:download', {
+            id: '1',
+          })
+        })
+      })
     })
 
     describe('adding header to AUT iframe request', function () {
diff --git a/packages/server/test/unit/browsers/electron_spec.js b/packages/server/test/unit/browsers/electron_spec.js
index 1fe7fc2d5c76..ecb0101d2f16 100644
--- a/packages/server/test/unit/browsers/electron_spec.js
+++ b/packages/server/test/unit/browsers/electron_spec.js
@@ -322,7 +322,7 @@ describe('lib/browsers/electron', () => {
         getFilename: () => 'file.csv',
         getMimeType: () => 'text/csv',
         getURL: () => 'http://localhost:1234/file.csv',
-        once: sinon.stub().yields(),
+        once: sinon.stub().yields({}, 'completed'),
       }
 
       this.win.webContents.session.on.withArgs('will-download').yields({}, downloadItem)
@@ -337,6 +337,27 @@ describe('lib/browsers/electron', () => {
       })
     })
 
+    it('pushes canceled:download when download is incomplete', function () {
+      const downloadItem = {
+        getETag: () => '1',
+        getFilename: () => 'file.csv',
+        getMimeType: () => 'text/csv',
+        getURL: () => 'http://localhost:1234/file.csv',
+        once: sinon.stub().yields({}, 'canceled'),
+      }
+
+      this.win.webContents.session.on.withArgs('will-download').yields({}, downloadItem)
+      this.options.downloadsFolder = 'downloads'
+      sinon.stub(this.automation, 'push')
+
+      return electron._launch(this.win, this.url, this.automation, this.options, undefined, undefined, { attachCDPClient: sinon.stub() })
+      .then(() => {
+        expect(this.automation.push).to.be.calledWith('canceled:download', {
+          id: '1',
+        })
+      })
+    })
+
     it('sets download behavior', function () {
       this.options.downloadsFolder = 'downloads'