From 57bb0b68eedb72975d96446a8561322d0d03262a Mon Sep 17 00:00:00 2001 From: Matt Schile Date: Wed, 29 Nov 2023 15:09:12 -0700 Subject: [PATCH] fix: decode urls in prerequest (#28427) --- cli/CHANGELOG.md | 1 + packages/proxy/lib/http/util/prerequests.ts | 6 +- .../test/unit/http/util/prerequests.spec.ts | 21 +++ .../test/integration/http_requests_spec.js | 145 +++++++++++++++++- 4 files changed, 169 insertions(+), 4 deletions(-) diff --git a/cli/CHANGELOG.md b/cli/CHANGELOG.md index 3c0ca352b071..9317000e8d18 100644 --- a/cli/CHANGELOG.md +++ b/cli/CHANGELOG.md @@ -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). **Misc:** diff --git a/packages/proxy/lib/http/util/prerequests.ts b/packages/proxy/lib/http/util/prerequests.ts index cbdd41f9a13f..86b2cd7f9f30 100644 --- a/packages/proxy/lib/http/util/prerequests.ts +++ b/packages/proxy/lib/http/util/prerequests.ts @@ -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) { @@ -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) { @@ -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) { diff --git a/packages/proxy/test/unit/http/util/prerequests.spec.ts b/packages/proxy/test/unit/http/util/prerequests.spec.ts index 391a73ef957d..37d2b9b966a8 100644 --- a/packages/proxy/test/unit/http/util/prerequests.spec.ts +++ b/packages/proxy/test/unit/http/util/prerequests.spec.ts @@ -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 + }) }) diff --git a/packages/server/test/integration/http_requests_spec.js b/packages/server/test/integration/http_requests_spec.js index 1fa37e332fe5..a41df8146216 100644 --- a/packages/server/test/integration/http_requests_spec.js +++ b/packages/server/test/integration/http_requests_spec.js @@ -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 () { @@ -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', () => {