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: match net module headers & http.IncomingMessage headers #17517

Merged
merged 5 commits into from Apr 2, 2019

Conversation

@codebytere
Copy link
Member

commented Mar 22, 2019

Description of Change

BREAKING CHANGE

Fixes #8117.

Filters net module incoming headers to more accurately match those returned in http.IncomingMessage.

cc @nornagon @MarshallOfSound

More specifically, after this change:

  • Duplicates of age, authorization, content-length, content-type, etag, expires, from, host, if-modified-since, if-unmodified-since, last-modified, location, max-forwards, proxy-authorization, referer, retry-after, or user-agent are discarded.
  • set-cookie is always an array. Duplicates are added to the array.
  • For duplicate cookie headers, the values are joined together with '; '.
  • For all other headers, the values are joined together with ', '.

Tested with:

const { app } = require('electron')

app.on('ready', () => {
  const { net } = require('electron')
  const request = net.request('https://github.com')
  request.on('response', response => {
    console.log('HEADERS: ', response.headers)
  })
  request.end()
})

to ensure that headers conform to the new rules appropriately

Checklist

Release Notes

Notes: Fixed disparity between net module headers and Node.js' http.IncomingMessage headers

@codebytere codebytere force-pushed the net-headers branch from 8e91bd1 to 0fd61c5 Mar 22, 2019

@electron-cation electron-cation bot removed the new-pr 🌱 label Mar 23, 2019

Show resolved Hide resolved spec/api-net-spec.js Outdated
Show resolved Hide resolved spec/api-net-spec.js
Show resolved Hide resolved lib/browser/api/net.js Outdated
Show resolved Hide resolved lib/browser/api/net.js Outdated
Show resolved Hide resolved lib/browser/api/net.js Outdated
Show resolved Hide resolved lib/browser/api/net.js Outdated
Show resolved Hide resolved spec/api-net-spec.js Outdated
Show resolved Hide resolved spec/api-net-spec.js

@codebytere codebytere force-pushed the net-headers branch from f2f23ff to ae7a0f3 Mar 28, 2019

@codebytere codebytere requested a review from nornagon Mar 28, 2019

Show resolved Hide resolved spec/api-net-spec.js Outdated
Show resolved Hide resolved spec/api-net-spec.js Outdated

@codebytere codebytere force-pushed the net-headers branch from ae7a0f3 to c2ebf77 Mar 28, 2019

Show resolved Hide resolved spec/api-net-spec.js
Update spec/api-net-spec.js
Co-Authored-By: codebytere <codebytere@github.com>

@codebytere codebytere force-pushed the net-headers branch from ca91377 to fedb6ed Mar 28, 2019

@codebytere codebytere force-pushed the net-headers branch from fedb6ed to a58a2d1 Mar 29, 2019

@codebytere

This comment has been minimized.

Copy link
Member Author

commented Apr 2, 2019

Failure is:

not ok 139 BrowserWindow module "useContentSize" option make window created with content size when used
  AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
  
  124 !== 400
  
      at Context.<anonymous> (C:\projects\src\electron\spec\api-browser-window-spec.js:1229:14)
      at callFn (C:\projects\src\electron\spec\node_modules\mocha\lib\runnable.js:372:21)
      at Test.Runnable.run (C:\projects\src\electron\spec\node_modules\mocha\lib\runnable.js:364:7)
      at Runner.runTest (C:\projects\src\electron\spec\node_modules\mocha\lib\runner.js:455:10)
      at C:\projects\src\electron\spec\node_modules\mocha\lib\runner.js:573:12
      at next (C:\projects\src\electron\spec\node_modules\mocha\lib\runner.js:369:14)
      at C:\projects\src\electron\spec\node_modules\mocha\lib\runner.js:379:7
      at next (C:\projects\src\electron\spec\node_modules\mocha\lib\runner.js:303:14)
      at Immediate._onImmediate (C:\projects\src\electron\spec\node_modules\mocha\lib\runner.js:347:5)
      at processImmediate (timers.js:632:19)

which is assuredly not related, so merging regardless.

@codebytere codebytere merged commit 8ea33d6 into master Apr 2, 2019

14 of 15 checks passed

appveyor: win-ia32-testing AppVeyor build failed
Details
Artifact Comparison No Changes
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-debug AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-debug AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
build-linux Workflow: build-linux
Details
build-mac Workflow: build-mac
Details
electron-arm-testing Build #20190329.4 succeeded
Details
electron-arm64-testing Build #20190329.4 succeeded
Details
lint Workflow: lint
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

commented Apr 2, 2019

Release Notes Persisted

Fixed disparity between net module headers and Node.js' http.IncomingMessage headers

@codebytere codebytere deleted the net-headers branch Apr 2, 2019

Kiku-Reise added a commit to Kiku-Reise/electron that referenced this pull request May 16, 2019

fix: match net module headers & http.IncomingMessage headers (electro…
…n#17517)

* fix: match net module headers & http.IncomingMessage headers

* update net doc for cleanliness

* address feedback from review

* Update spec/api-net-spec.js

Co-Authored-By: codebytere <codebytere@github.com>

* add special cookie case

@ckerr ckerr added the semver/major label Jul 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.