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: decode urls in prerequest #28427

Merged
merged 3 commits into from Nov 29, 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
1 change: 1 addition & 0 deletions cli/CHANGELOG.md
Expand Up @@ -7,6 +7,7 @@ _Released 12/5/2023 (PENDING)_

- 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).
- Fixed an issue where request logging would default the `message` to the `args` of the currently running command even though those `args` would not apply to the request log and are not displayed. If the `args` are sufficiently large (e.g. when running the `cy.task` from the [code-coverage](https://github.com/cypress-io/code-coverage/) plugin) there could be performance/memory implications. Addressed in [#28411](https://github.com/cypress-io/cypress/pull/28411).
- Fixed issue where some URLs would timeout in pre-request correlation. Addressed in [#28427](https://github.com/cypress-io/cypress/pull/28427).

## 13.6.0

Expand Down
6 changes: 3 additions & 3 deletions packages/proxy/lib/http/util/prerequests.ts
Expand Up @@ -148,7 +148,7 @@ export class PreRequests {

addPending (browserPreRequest: BrowserPreRequest) {
metrics.browserPreRequestsReceived++
const key = `${browserPreRequest.method}-${browserPreRequest.url}`
const key = `${browserPreRequest.method}-${decodeURI(browserPreRequest.url)}`
const pendingRequest = this.pendingRequests.shift(key)

if (pendingRequest) {
Expand Down Expand Up @@ -193,7 +193,7 @@ export class PreRequests {
}

addPendingUrlWithoutPreRequest (url: string) {
const key = `GET-${url}`
const key = `GET-${decodeURI(url)}`
const pendingRequest = this.pendingRequests.shift(key)

if (pendingRequest) {
Expand Down Expand Up @@ -236,7 +236,7 @@ export class PreRequests {
const proxyRequestReceivedTimestamp = performance.now() + performance.timeOrigin

metrics.proxyRequestsReceived++
const key = `${req.method}-${req.proxiedUrl}`
const key = `${req.method}-${decodeURI(req.proxiedUrl)}`
const pendingPreRequest = this.pendingPreRequests.shift(key)

if (pendingPreRequest) {
Expand Down
21 changes: 21 additions & 0 deletions packages/proxy/test/unit/http/util/prerequests.spec.ts
Expand Up @@ -275,4 +275,25 @@ describe('http/util/prerequests', () => {

expect(callbackCalled).to.be.true
})

it('decodes the proxied url', () => {
preRequests.get({ proxiedUrl: 'foo%7Cbar', method: 'GET', headers: {} } as CypressIncomingRequest, () => {}, () => {})

expect(preRequests.pendingRequests.length).to.eq(1)
expect(preRequests.pendingRequests.shift('GET-foo|bar')).not.to.be.undefined
})

it('decodes the pending url without pre-request', () => {
preRequests.addPendingUrlWithoutPreRequest('foo%7Cbar')

expect(preRequests.pendingUrlsWithoutPreRequests.length).to.eq(1)
expect(preRequests.pendingUrlsWithoutPreRequests.shift('GET-foo|bar')).not.to.be.undefined
})

it('decodes pending url', () => {
preRequests.addPending({ requestId: '1234', url: 'foo%7Cbar', method: 'GET' } as BrowserPreRequest)

expect(preRequests.pendingPreRequests.length).to.eq(1)
expect(preRequests.pendingPreRequests.shift('GET-foo|bar')).not.to.be.undefined
})
})
145 changes: 144 additions & 1 deletion packages/server/test/integration/http_requests_spec.js
Expand Up @@ -988,7 +988,9 @@ describe('Routes', () => {

context('basic request with correlation', () => {
beforeEach(function () {
return this.setup('http://www.github.com', undefined, undefined, true)
return this.setup('http://www.github.com', undefined, undefined, true).then(() => {
this.networkProxy.setPreRequestTimeout(2000)
})
})

it('properly correlates when CDP failures come first', function () {
Expand Down Expand Up @@ -1087,6 +1089,147 @@ describe('Routes', () => {
})
})
})

it('properly correlates when request has | character', function () {
this.timeout(1500)

nock(this.server.remoteStates.current().origin)
.get('/?foo=bar|baz')
.reply(200, 'hello from bar!', {
'Content-Type': 'text/html',
})

const requestPromise = this.rp({
url: 'http://www.github.com/?foo=bar|baz',
headers: {
'Accept-Encoding': 'identity',
},
})

this.networkProxy.addPendingBrowserPreRequest({
requestId: '1',
method: 'GET',
url: 'http://www.github.com/?foo=bar|baz',
})

return requestPromise.then((res) => {
expect(res.statusCode).to.eq(200)

expect(res.body).to.include('hello from bar!')
})
})

it('properly correlates when request has | character with addPendingUrlWithoutPreRequest', function () {
this.timeout(1500)

nock(this.server.remoteStates.current().origin)
.get('/?foo=bar|baz')
.reply(200, 'hello from bar!', {
'Content-Type': 'text/html',
})

const requestPromise = this.rp({
url: 'http://www.github.com/?foo=bar|baz',
headers: {
'Accept-Encoding': 'identity',
},
})

this.networkProxy.addPendingUrlWithoutPreRequest('http://www.github.com/?foo=bar|baz')

return requestPromise.then((res) => {
expect(res.statusCode).to.eq(200)

expect(res.body).to.include('hello from bar!')
})
})

it('properly correlates when request has encoded | character', function () {
this.timeout(1500)

nock(this.server.remoteStates.current().origin)
.get('/?foo=bar%7Cbaz')
.reply(200, 'hello from bar!', {
'Content-Type': 'text/html',
})

const requestPromise = this.rp({
url: 'http://www.github.com/?foo=bar%7Cbaz',
headers: {
'Accept-Encoding': 'identity',
},
})

this.networkProxy.addPendingBrowserPreRequest({
requestId: '1',
method: 'GET',
url: 'http://www.github.com/?foo=bar%7Cbaz',
})

return requestPromise.then((res) => {
expect(res.statusCode).to.eq(200)

expect(res.body).to.include('hello from bar!')
})
})

it('properly correlates when request has encoded " " (space) character', function () {
this.timeout(1500)

nock(this.server.remoteStates.current().origin)
.get('/?foo=bar%20baz')
.reply(200, 'hello from bar!', {
'Content-Type': 'text/html',
})

const requestPromise = this.rp({
url: 'http://www.github.com/?foo=bar%20baz',
headers: {
'Accept-Encoding': 'identity',
},
})

this.networkProxy.addPendingBrowserPreRequest({
requestId: '1',
method: 'GET',
url: 'http://www.github.com/?foo=bar%20baz',
})

return requestPromise.then((res) => {
expect(res.statusCode).to.eq(200)

expect(res.body).to.include('hello from bar!')
})
})

it('properly correlates when request has encoded " character', function () {
this.timeout(1500)

nock(this.server.remoteStates.current().origin)
.get('/?foo=bar%22')
.reply(200, 'hello from bar!', {
'Content-Type': 'text/html',
})

const requestPromise = this.rp({
url: 'http://www.github.com/?foo=bar%22',
headers: {
'Accept-Encoding': 'identity',
},
})

this.networkProxy.addPendingBrowserPreRequest({
requestId: '1',
method: 'GET',
url: 'http://www.github.com/?foo=bar%22',
})

return requestPromise.then((res) => {
expect(res.statusCode).to.eq(200)

expect(res.body).to.include('hello from bar!')
})
})
})

context('gzip', () => {
Expand Down