-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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 "Parse Error" when performing HTTP requests #5988
Conversation
Co-authored-by: Andrew Smith <andrew@andrew.codes>
…press-io/cypress into node-options-and-no-sandbox-in-binary
Thanks for the contribution! Below are some guidelines Cypress uses when doing PR reviews.
PR Review ChecklistIf any of the following requirements can't be met, leave a comment in the review selecting 'Request changes', otherwise 'Approve'. User Experience
Functionality
Maintainability
Quality
Internal
|
Test summaryRun details
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 |
b0b6fd0
to
c656bfa
Compare
* cli: set NODE_OPTIONS=--max-http-header-size=1024*1024 on spawn * electron: remove redundant max-http-header-size * server: add useCli option to make e2e tests go thru cli * server: add test for XHR with body > 100kb via CLI * clean up conditional * cli: don't pass --max-http-header-size in dev w node < 11.10 * add original_node_options to restore o.g. user node_options * force no color
…cookie-parse-error See #5492 for original review and discussion
3d20637
to
89192a8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! some pretty arcane things here, but looks really solid 👍
just out of curiosity -- why do we use the legacy electron http parser? (is the idea just compliance with previous cypress behavior?)
@CypressJoseph yeah, it fixes the regression in Cypress, but imo it's not just about backwards-compatibility - Cypress should be a 100% transparent HTTP proxy, even if the HTTP traffic it's proxying is a little out-of-spec. Cypress should load pages the same as a normal browser does, and browsers are very tolerant. So when Node introduced a new, stricter HTTP parser that's on by default, it made the proxy start rejecting some previously-acceptable traffic. See this comment for technical info on the changes that made this necessary. |
User facing changelog
Additional details
NODE_OPTIONS=--max-http-header-size=${1000 ** 2} --http-parser=legacy
, which fixes the sources of the "Parse Error"sresponse-middleware
to suppress further "Parse Error" errors that still occur for invalid header valuesNotes:
--max-http-header-size
in Electron 5.x electron/electron#20831 and adding a bigger max http header sizecypress/packages/server/test/e2e/6_visit_spec.coffee
Lines 72 to 85 in 390ad4a
--http-parser=legacy
inNODE_OPTIONS
gets rid of the error--http-parser
NODE_OPTION in a packaged app electron/electron#21693--http-parser=legacy
, still fails, but in a different place:setHeader
steps in that case:cypress/packages/proxy/lib/http/response-middleware.ts
Lines 162 to 185 in 390ad4a
kOutHeaders
symbol, can't find a better wayHow has the user experience changed?
PR Tasks
cypress-documentation
?type definitions
?cypress.schema.json
?