Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Ensure basic auth headers are set on extra target requests #28387

Merged
merged 4 commits into from Nov 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions cli/CHANGELOG.md
@@ -1,4 +1,12 @@
<!-- See the ../guides/writing-the-cypress-changelog.md for details on writing the changelog. -->
## 13.6.1

_Released 12/5/2023 (PENDING)_

**Bugfixes:**

- Fixed an issue where pages or downloads opened in a new tab were missing basic auth headers. Fixes [#28350](https://github.com/cypress-io/cypress/issues/28350).

## 13.6.0

_Released 11/21/2023_
Expand Down
20 changes: 20 additions & 0 deletions packages/driver/cypress/e2e/cypress/downloads.cy.ts
@@ -1,4 +1,5 @@
import $Downloads from '../../../src/cypress/downloads'
import { authCreds } from '../../fixtures/auth_creds'

describe('src/cypress/downloads', () => {
let log
Expand Down Expand Up @@ -94,6 +95,7 @@ describe('download behavior', () => {
.should('contain', '"Joe","Smith"')
})

// NOTE: webkit opens a new window and doesn't download the file
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')
Expand All @@ -110,3 +112,21 @@ describe('download behavior', () => {
cy.readFile(`${Cypress.config('downloadsFolder')}/downloads_does_not_exist.csv`).should('not.exist')
})
})

describe('basic auth download behavior', () => {
beforeEach(() => {
cy.visit('/fixtures/downloads.html', {
auth: authCreds,
})
})

// NOTE: webkit opens a new window and doesn't download the file
it('downloads basic auth protected file that opens in a new tab', { browser: '!webkit' }, () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this assert that a new window was opened successfully rather than skipping it in webkit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having a new window open isn't the desired behavior, so I would hesitate to codify that behavior. It's supposed to download the file, but it seems that doesn't work in webkit for some reason. Might be something we fix in the future, but I'd say it's out of scope for this bug fix.

cy.exec(`rm -f ${Cypress.config('downloadsFolder')}/download-basic-auth.csv`)
cy.readFile(`${Cypress.config('downloadsFolder')}/download-basic-auth.csv`).should('not.exist')

cy.get('[data-cy=download-basic-auth]').click()
cy.readFile(`${Cypress.config('downloadsFolder')}/download-basic-auth.csv`)
.should('contain', '"Joe","Smith"')
})
})
10 changes: 3 additions & 7 deletions packages/driver/cypress/e2e/e2e/origin/commands/navigation.cy.ts
@@ -1,3 +1,4 @@
import { authCreds } from '../../../../fixtures/auth_creds'
import { findCrossOriginLogs } from '../../../../support/utils'

context('cy.origin navigation', { browser: '!webkit' }, () => {
Expand Down Expand Up @@ -309,13 +310,8 @@ context('cy.origin navigation', { browser: '!webkit' }, () => {
})

it('supports auth options and adding auth to subsequent requests', () => {
cy.origin('http://www.foobar.com:3500', () => {
cy.visit('http://www.foobar.com:3500/basic_auth', {
auth: {
username: 'cypress',
password: 'password123',
},
})
cy.origin('http://www.foobar.com:3500', { args: authCreds }, (auth) => {
cy.visit('http://www.foobar.com:3500/basic_auth', { auth })

cy.get('body').should('have.text', 'basic auth worked')

Expand Down
4 changes: 4 additions & 0 deletions packages/driver/cypress/fixtures/auth_creds.ts
@@ -0,0 +1,4 @@
export const authCreds = {
username: 'cypress',
password: 'password123',
}
3 changes: 3 additions & 0 deletions packages/driver/cypress/fixtures/downloads.html
Expand Up @@ -9,6 +9,9 @@ <h3>Download CSV</h3>
<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 Basic Auth CSV</h3>
<a data-cy="download-basic-auth" href="/download-basic-auth.csv" target="_blank">download csv that's basic auth protected</a>

<h3>Download CSV</h3>
<a data-cy="invalid-download" href="downloads_does_not_exist.csv" download>broken download link</a>
</body>
Expand Down
16 changes: 15 additions & 1 deletion packages/driver/cypress/plugins/server.js
Expand Up @@ -9,6 +9,7 @@ const Promise = require('bluebird')
const multer = require('multer')
const upload = multer({ dest: 'cypress/_test-output/' })
const { cors } = require('@packages/network')
const { authCreds } = require('../fixtures/auth_creds')

const PATH_TO_SERVER_PKG = path.dirname(require.resolve('@packages/server'))

Expand Down Expand Up @@ -128,7 +129,7 @@ const createApp = (port) => {
app.get('/basic_auth', (req, res) => {
const user = auth(req)

if (user && ((user.name === 'cypress') && (user.pass === 'password123'))) {
if (user?.name === authCreds.username && user?.pass === authCreds.password) {
return res.send('<html><body>basic auth worked</body></html>')
}

Expand Down Expand Up @@ -342,6 +343,19 @@ const createApp = (port) => {
res.send(_var)
})

app.get('/download-basic-auth.csv', (req, res) => {
const user = auth(req)

if (user?.name === authCreds.username && user?.pass === authCreds.password) {
return res.sendFile(path.join(__dirname, '..', 'fixtures', 'downloads_records.csv'))
}

return res
.set('WWW-Authenticate', 'Basic')
.type('html')
.sendStatus(401)
})

app.post('/upload', (req, res) => {
res.sendStatus(200)
})
Expand Down
1 change: 1 addition & 0 deletions packages/proxy/lib/http/request-middleware.ts
Expand Up @@ -52,6 +52,7 @@ const ExtractCypressMetadataHeaders: RequestMiddleware = function () {
delete this.req.headers['x-cypress-is-from-extra-target']

this.onlyRunMiddleware([
'MaybeSetBasicAuthHeaders',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really want to attach basic auth to all requests in the new target? The download use-case seems like the exception not the default.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we would only attach if the origins match, correct? So, that's probably fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, they have to cy.visit() the origin with the credentials first

'SendRequestOutgoing',
])
}
Expand Down
2 changes: 1 addition & 1 deletion packages/proxy/test/unit/http/request-middleware.spec.ts
Expand Up @@ -83,7 +83,7 @@ describe('http/request-middleware', () => {

expect(ctx.req.headers!['x-cypress-is-from-extra-target']).not.to.exist
expect(ctx.req.isFromExtraTarget).to.be.true
expect(ctx.onlyRunMiddleware).to.be.calledWith(['SendRequestOutgoing'])
expect(ctx.onlyRunMiddleware).to.be.calledWith(['MaybeSetBasicAuthHeaders', 'SendRequestOutgoing'])
})

it('when it does not exist, removes header and sets in on the req', async () => {
Expand Down