fix: handle download from element missing download attribute (#28222)

cypress-io · Nov 6, 2023 · eab1730 · eab1730 · cypress-bot · Nov 6, 2023
1 parent db8609e
commit eab1730
Show file tree

Hide file tree

Showing 15 changed files with 188 additions and 18 deletions.
diff --git 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
@@ -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
@@ -4,20 +4,24 @@ describe('src/cypress/downloads', () => {
   let log
   let snapshot
   let end
+  let error
   let downloads
   let downloadItem = {
     id: '1',
     filePath: '/path/to/save/location.csv',
     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
@@ -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
@@ -0,0 +1,2 @@
+"First name","Last name","Occupation","Age","City","State"
+"Joe","Smith","student",20,"Boston","MA"
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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'