fix: XHR event listener AUT redirect bug #15995

tgriesser · 2021-04-15T00:22:01Z

Fixes Incorrect redirect to malformed url within AUT - adding _/ in url #3975
Fixes Redirect adds __ within directory path when experimentalSourceRewriting is on #7439

User facing changelog

Fix for incorrect redirect when location.href is set to a relative path within the call stack of an XHR event handler, which incorrectly sets the user's AUT to /__/ rather than the correct path.

~~Additionally, adds code to detect when a form submit target is set to _top and sets to '' to avoid redirecting the parent frame.~~ Edit: Split out to #16394

Additional details

This was caused by the changes in this Chromium commit: https://bugs.chromium.org/p/chromium/issues/detail?id=849236 (big thanks to @jennifer-shehane doing the initial range narrowing on that one) which changed the mechanics of how the window is bound around events, which I believe had an inadvertent side affect that affects location.href assignment.

This ticket: https://bugs.chromium.org/p/chromium/issues/detail?id=1195471 seems like it will possibly address the issue, but this behavior is consistent in both Firefox & Chrome, so it's likely worth fixing on our end for now.

By wrapping the handlers in a function defined in the AUT window (@brian-mann had the idea for that one), the event handler is correctly associated with the AUT, and it doesn't impact the location.href assignment.

How has the user experience changed?

User's AUT is no-longer redirected incorrectly to /__/path but instead to the correct relative window location path.

PR Tasks

Have tests been added/updated?
Has the original issue or this PR been tagged with a release in ZenHub?
Has a PR for user-facing changes been opened in cypress-documentation?
Have API changes been updated in the type definitions?
Have new configuration options been added to the cypress.schema.json?

cypress-bot · 2021-04-15T00:22:04Z

Thanks for taking the time to open a PR!

Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

flotwig

awesome, some minor nits

packages/driver/src/cy/listeners.js

…io/cypress into tgriesser/fix/3975-redirect-bug * 'tgriesser/fix/3975-redirect-bug' of github.com:cypress-io/cypress: fix(@cypress/webpack-batteries-included-preprocessor): Ensure typescript options are set if typescript path is provided (#15991) chore: release @cypress/vite-dev-server-v1.2.2 fix: conditionally require vue and update alias if installed (#16000) docs: remove cssFiles from the vue documentation (#15971) chore: release @cypress/webpack-dev-server-v1.1.3 chore: release @cypress/vite-dev-server-v1.2.1 chore: release @cypress/react-v5.3.3 fix: remove lazy-compile-webpack-plugin (#15964)

cypress · 2021-04-15T17:09:38Z

Test summary

13772 • 0 • 164 • 6 • 0

Run details


Project	cypress
Status	Passed
Commit	`42358a9`
Started	May 7, 2021 2:46 PM
Ended	May 7, 2021 2:57 PM
Duration	10:54 💡
OS	Linux Debian - 10.8
Browser	Multiple

View run in Cypress Dashboard ➡️

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

cli/types/cypress.d.ts

jennifer-shehane · 2021-04-16T00:58:29Z

@tgriesser I know this seems unrelated, but I'm wondering if this would also fix #1244 as a number of the users in that thread had a situation of redirecting to a __/ url, but were shown this illegal operation error as a result.

There's a repro example here: #1244 (comment)

tgriesser · 2021-04-16T02:33:49Z

@jennifer-shehane yeah, looks very likely that's related (added it to the list of issues in the PR description). I'm guessing that the behavior when the incorrect redirect takes place could manifest in different ways depending on the situation.

Basically any issue that involves a location change within the same call stack as an event listener that's also observed by Cypress is a likely candidate.

I'd guess the submit or any of the xhr events are probably the common situations where this happens.

tgriesser · 2021-04-28T19:58:24Z

packages/driver/src/cypress/server.js

@@ -421,6 +422,8 @@ const create = (options = {}) => {
      const { send, open, abort } = XHR.prototype
      const srh = XHR.prototype.setRequestHeader

+      const bridgeContentWindowListener = makeContentWindowListener('cypressXhrBridge', contentWindow)


@brian-mann This is now only creating once per window load, since it's outside the XHR patching

* develop: feat(design-system): SpecList - FileTree - VirtualizedTree (#16179) fix: Ensure binary request body is not mutated by cy.intercept (#16306) chore: run driver, runner tests in electron (#16208) fix: accept webapck 4 & 5 as peer dependencies of @cypress/vue and @cypress/react (#16290)

* develop: chore: Remove extra renovate.json file (#16385) Do not print 'uploading' stdout when --quiet mode is passed (#16271) fix(deps): update dependency color-string to version 1.5.5 🌟 (#16362) fix(deps): update dependency cypress-real-events to version 1.4.0 🌟 (#16363) tests: use the proper keys for selecting framework fix: Prevent Firefox from opening custom search when pressing / (#16372) fix: vueCli and webpack key vue@2 fix when guessing tests: update snapshots fix: add return config for vueCli and vueWebpack fix: add return config for vitejs templates chore: release @cypress/vue-v2.2.2 chore: release @cypress/react-v5.5.0 fix: remove all of rollup, not supported anymore fix: typo in the final message (run vs run-ct)

* develop: (28 commits) fix: XHR event listener AUT redirect bug (#15995) chore: fix typo (#16345) chore(design-system): Added missing exports and index.ts (#16351) chore: Remove extra renovate.json file (#16385) Do not print 'uploading' stdout when --quiet mode is passed (#16271) fix(deps): update dependency color-string to version 1.5.5 🌟 (#16362) fix(deps): update dependency cypress-real-events to version 1.4.0 🌟 (#16363) tests: use the proper keys for selecting framework fix: Prevent Firefox from opening custom search when pressing / (#16372) fix(socket): update serialization for circular binary socket messages (#16311) fix: vueCli and webpack key vue@2 fix when guessing tests: update snapshots fix: add return config for vueCli and vueWebpack chore(deps): update dependency classnames to version 2.3.1 🌟 (#8337) fix: add return config for vitejs templates chore: release @cypress/vue-v2.2.2 chore: release @cypress/react-v5.5.0 fix: remove all of rollup, not supported anymore fix: typo in the final message (run vs run-ct) fix: use close event when asking the browser for its version (#16312) ...

fix(deps): update dependency cypress-real-events to version 1.4.0 🌟 (#16363) Co-authored-by: Renovate Bot <[email protected]> fix: typo in the final message (run vs run-ct) fix: remove all of rollup, not supported anymore fix: add return config for vitejs templates fix: add return config for vueCli and vueWebpack tests: update snapshots fix: vueCli and webpack key vue@2 fix when guessing tests: use the proper keys for selecting framework chore: release @cypress/react-v5.5.0 [skip ci] chore: release @cypress/vue-v2.2.2 [skip ci] fix(deps): update dependency color-string to version 1.5.5 🌟 (#16362) Co-authored-by: Renovate Bot <[email protected]> Co-authored-by: Jennifer Shehane <[email protected]> Do not print 'uploading' stdout when --quiet mode is passed (#16271) chore: Remove extra renovate.json file (#16385) chore(design-system): Added missing exports and index.ts (#16351) chore: fix typo (#16345) Co-authored-by: Jennifer Shehane <[email protected]> fix: XHR event listener AUT redirect bug (#15995) Fixes incorrect redirect when location.href is set to a relative path within the call stack of an XHR event handler, which set the user's AUT to /__/ rather than the correct path Linting after merge

drumbeg · 2021-05-08T10:55:39Z

Great work guys. Thanks. Works in my limited testing so far.

cypress-bot · 2021-05-10T23:39:10Z

Released in 7.3.0.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v7.3.0, please open a new issue.

tgriesser added 2 commits April 14, 2021 18:08

Add failing test for #3975

72a9afb

Proposed fix for #3975

d2d8f21

tgriesser requested a review from a team as a code owner April 15, 2021 00:22

tgriesser requested review from chrisbreiding and kuceb and removed request for a team April 15, 2021 00:22

tgriesser requested review from flotwig and brian-mann April 15, 2021 00:22

flotwig previously approved these changes Apr 15, 2021

View reviewed changes

packages/driver/src/cy/listeners.js Outdated Show resolved Hide resolved

packages/driver/src/cy/listeners.js Outdated Show resolved Hide resolved

chrisbreiding reviewed Apr 15, 2021

View reviewed changes

packages/driver/src/cy/listeners.js Outdated Show resolved Hide resolved

Pass a function wrapper rather than new Function

5be3049

tgriesser dismissed flotwig’s stale review via 5be3049 April 15, 2021 16:13

tgriesser requested review from flotwig and chrisbreiding April 15, 2021 16:14

tgriesser added 3 commits April 15, 2021 12:36

Merge branch 'develop' into tgriesser/fix/3975-redirect-bug

379eaba

Add eventHandlerWrap to XHR

fbf8e54

chrisbreiding requested changes Apr 15, 2021

View reviewed changes

cli/types/cypress.d.ts Outdated Show resolved Hide resolved

tgriesser added 2 commits April 15, 2021 16:44

Change impl to set bridge on window, fallback to Fn ctor

5d954b1

Fix object lookup syntax

86c3212

jennifer-shehane requested a review from chrisbreiding April 15, 2021 23:25

Fix test

135b55b

chrisbreiding previously approved these changes Apr 16, 2021

View reviewed changes

Pairing with @brian-mann

94f1fa4

tgriesser dismissed stale reviews from chrisbreiding and ghost via 94f1fa4 April 16, 2021 22:13

tgriesser added 2 commits April 28, 2021 15:57

Give a more descriptive bridge fn name for the debugger

b8e0cee

Forgot to save file

2d06c65

tgriesser commented Apr 28, 2021

View reviewed changes

tgriesser changed the title ~~fix: AUT redirect bug~~ fix: AUT redirect bug(s) Apr 28, 2021

tgriesser requested a review from brian-mann April 28, 2021 19:59

tgriesser added 8 commits April 28, 2021 16:02

Fixes

8fd4a6c

Guard against event already having target descriptor

447c0ad

Fix window stub

bc722aa

Merge branch 'develop' into tgriesser/fix/3975-redirect-bug

745667c

Merge branch 'develop' into tgriesser/fix/3975-redirect-bug

dc956eb

Merge branch 'develop' into tgriesser/fix/3975-redirect-bug

3d0a5e6

improve tests for 1244

42358a9

tgriesser dismissed a stale review via 42358a9 May 4, 2021 14:43

Merge branch 'develop' into tgriesser/fix/3975-redirect-bug

279adca

jennifer-shehane mentioned this pull request May 6, 2021

"Webpack Compilation Error - multi" on redirect #8507

Closed

tgriesser added 2 commits May 7, 2021 10:40

Splitting out changes for #1244 into separate PR

0c69fce

tgriesser changed the title ~~fix: AUT redirect bug(s)~~ fix: event listener AUT redirect bug May 7, 2021

tgriesser mentioned this pull request May 7, 2021

fix: AUT Redirect bug on form / anchor target #16394

Merged

5 tasks

tgriesser changed the title ~~fix: event listener AUT redirect bug~~ fix: XHR event listener AUT redirect bug May 7, 2021

tgriesser merged commit 3f31f09 into develop May 7, 2021

tgriesser deleted the tgriesser/fix/3975-redirect-bug branch May 7, 2021 16:21

This was referenced May 7, 2021

Incorrect redirect to malformed url within AUT - adding _/ in url #3975

Closed

Redirect adds __ within directory path when experimentalSourceRewriting is on #7439

Closed

cypress-bot bot locked as resolved and limited conversation to collaborators May 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: XHR event listener AUT redirect bug #15995

fix: XHR event listener AUT redirect bug #15995

tgriesser commented Apr 15, 2021 •

edited

Loading

cypress-bot bot commented Apr 15, 2021

flotwig left a comment

cypress bot commented Apr 15, 2021 •

edited

Loading

jennifer-shehane commented Apr 16, 2021

tgriesser commented Apr 16, 2021

tgriesser Apr 28, 2021

drumbeg commented May 8, 2021

cypress-bot bot commented May 10, 2021

fix: XHR event listener AUT redirect bug #15995

fix: XHR event listener AUT redirect bug #15995

Conversation

tgriesser commented Apr 15, 2021 • edited Loading

User facing changelog

Additional details

How has the user experience changed?

PR Tasks

cypress-bot bot commented Apr 15, 2021

flotwig left a comment

Choose a reason for hiding this comment

cypress bot commented Apr 15, 2021 • edited Loading

Test summary

Run details

jennifer-shehane commented Apr 16, 2021

tgriesser commented Apr 16, 2021

tgriesser Apr 28, 2021

Choose a reason for hiding this comment

drumbeg commented May 8, 2021

cypress-bot bot commented May 10, 2021

tgriesser commented Apr 15, 2021 •

edited

Loading

cypress bot commented Apr 15, 2021 •

edited

Loading