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

Rewrite JS/HTML using AST-based approach #5273

Merged
merged 103 commits into from
May 11, 2020

Conversation

flotwig
Copy link
Contributor

@flotwig flotwig commented Oct 2, 2019

User facing changelog

Experimental fixes

To access these fixes, enable experimental source rewriting with the config value experimentalSourceRewriting: true:

Additional details

  • Adds AST-based rewriting behind experimental flag experimentalSourceRewriting:
    image
  • Explore serving source maps so users can still debug their original source code: https://github.com/benjamn/recast#source-maps
    • may possibly conflict with Error Improvements #6724 go-to-line functionality error improvements only consume sourcemaps for specfiles, not AUT JS (yet) - so we can get away with simply inlining sourcemaps for now
    • sourcemaps will be served over HTTP, we don't want the overhead of generating them for every single JS loaded - unfortunately this requires some caching of JS
    • also intercept SourceMap and X-SourceMap headers
      • order of preference: inlined sourcemappingurl, sourcemap header, x-sourcemap header
  • Strip script integrity parameters - fixes Cypress cannot test sites that implement SRI #2393
    • SRI integrity attributes in <script type="text/javascript"> tags will be rewritten to cypress:stripped-integrity attributes - <script type="text/javascript" integrity="foo"> becomes <script type="text/javascript" cypress:stripped-integrity="foo">
  • Cache rewritten code by ETags and potentially with a hash so it only needs 1 rewrite per session
    • HTTP caching semantics are hard, and existing cache mechanisms will still work inside a single browser session, so this can be implemented later
  • Fix SIGSEGV when exiting with threads active: SIGABRT when calling process.exit with worker_threads active electron/electron#23366
    • fixed with an ugly hack on intercepting process.exit, 😢
  • Throw error in driver if AST rewriting fails, since this is a bug in Cypress.

Note: This PR is huge because it does not replace the regex-based rewriting (yet), so there are some duplicate code paths and tests.

Checklist before removing "experimental" status: #7297

How has the user experience changed?

More user sites will work OOTB with Cypress 🎉

PR Tasks

@cypress
Copy link

cypress bot commented Oct 2, 2019



Test summary

7402 0 102 0


Run details

Project cypress
Status Passed
Commit 5877e66
Started May 11, 2020 4:26 PM
Ended May 11, 2020 4:32 PM
Duration 06:05 💡
OS Linux Debian - 10.1
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

@brian-mann
Copy link
Member

@flotwig can we also fix this issue with this PR? #5099

@brian-mann
Copy link
Member

@flotwig another important note - if we apply these rules to the entire proxy then we will be rewriting virtually all JS files - which will break <script> tags using the integrity attribute.

We'll need to strip those too. https://developer.mozilla.org/en-US/docs/Web/Security/Subresource_Integrity

@brian-mann
Copy link
Member

@flotwig another note - we should capture when window.location setters are used to navigate the page in JS such as window.location = '...' and window.location.href = '...' - so we can notify Cypress and capture the stack trace as well.

@flotwig
Copy link
Contributor Author

flotwig commented Oct 18, 2019

@flotwig another important note - if we apply these rules to the entire proxy then we will be rewriting virtually all JS files - which will break <script> tags using the integrity attribute.

We'll need to strip those too. developer.mozilla.org/en-US/docs/Web/Security/Subresource_Integrity

This uses hyntax to do streaming parsing of HTML, so this should be easy to add. It will be the first transform that we do directly to the HTML.

@jennifer-shehane
Copy link
Member

Will this also address this issue? #3994

@flotwig
Copy link
Contributor Author

flotwig commented Jan 16, 2020

@jennifer-shehane it will enable us to build a workaround for that issue, I believe, by making it so we can rewrite the use of window.location.(href|replace)

@flotwig flotwig mentioned this pull request Jan 17, 2020
flotwig added a commit to cypress-io/cypress-documentation that referenced this pull request May 8, 2020
packages/rewriter/lib/js-rules.ts Outdated Show resolved Hide resolved
packages/driver/src/cypress/resolvers.ts Outdated Show resolved Hide resolved
@flotwig flotwig marked this pull request as ready for review May 8, 2020 17:04
Copy link
Contributor

@kuceb kuceb left a comment

Choose a reason for hiding this comment

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

made suggested changes as commit

@drumbeg
Copy link

drumbeg commented May 21, 2020

Early testing looking very promising on #4576.

@jennifer-shehane
Copy link
Member

@drumbeg This fix is available starting in 4.6.0 as an experiment which you can access by setting this config option in your cypress.json or elsewhere:

{
	"experimentalSourceRewriting": true
}

The fix is experimental, so there may be some situations where the this is not fixed.

If you're still this issue while setting the experimentalSourceRewriting to true in 4.6.0 - open a new issue with a reproducible example + screenshots, etc - filling out our issue template.

@cypress-io cypress-io locked as resolved and limited conversation to collaborators May 21, 2020
@flotwig flotwig deleted the issue-5271-fix-window-references branch January 24, 2022 18:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
8 participants