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: pass full response headers in net module #21552

Merged
merged 4 commits into from Jan 14, 2020
Merged

fix: pass full response headers in net module #21552

merged 4 commits into from Jan 14, 2020

Conversation

@zcbenz
Copy link
Member

zcbenz commented Dec 17, 2019

Description of Change

Close #20631.

Use raw headers instead of filtered headers in net module. This is a regression caused by network service refactoring.

Checklist

Release Notes

Notes: Fix set-cookie header not passed in net module.

Copy link
Member

nornagon left a comment

What other kinds of filtering is Chromium doing here? Is it doing any sort of normalization that we might not want to lose? Do we only need the set-cookie headers here?

shell/browser/api/atom_api_url_loader.cc Outdated Show resolved Hide resolved
shell/browser/api/atom_api_url_loader.cc 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
@zcbenz

This comment has been minimized.

Copy link
Member Author

zcbenz commented Dec 18, 2019

What other kinds of filtering is Chromium doing here? Is it doing any sort of normalization that we might not want to lose? Do we only need the set-cookie headers here?

The raw headers are populated from net::URLRequest::response_headers(), and it is the same with 6.x, which uses net::URLRequest directly:
https://cs.chromium.org/chromium/src/services/network/loader_util.cc?type=cs&sq=package:chromium&g=0&l=102-120

Currently I'm only aware the set-cookie has been filtered because Chromium blocks third party cookies. I couldn't find a way to work around this without patching Chromium as the requests of net module are sent from browser process, and setting cookie sites does not seem to work.

@electron-cation electron-cation bot removed the new-pr 🌱 label Dec 18, 2019
@zcbenz

This comment has been minimized.

Copy link
Member Author

zcbenz commented Jan 13, 2020

@nornagon Can you take another look at this?

lib/browser/api/net.js Outdated Show resolved Hide resolved
@zcbenz zcbenz force-pushed the fix-set-cookie branch from 4f020aa to 7bb4e08 Jan 14, 2020
@zcbenz zcbenz force-pushed the fix-set-cookie branch from 7bb4e08 to da40732 Jan 14, 2020
@zcbenz zcbenz requested a review from nornagon Jan 14, 2020
@nornagon nornagon merged commit ceacadb into master Jan 14, 2020
17 checks passed
17 checks passed
Artifact Comparison No Changes
Details
Backportable? - 7-1-x Clean Backport
Details
Backportable? - 8-x-y Clean Backport
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-woa-testing 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 #20200114.5 succeeded
Details
electron-arm64-testing Build #20200114.4 succeeded
Details
electron-woa-testing Build #20200114.5 succeeded
Details
lint Workflow: lint
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

release-clerk bot commented Jan 14, 2020

Release Notes Persisted

Fix set-cookie header not passed in net module.

@nornagon nornagon deleted the fix-set-cookie branch Jan 14, 2020
@trop

This comment has been minimized.

Copy link
Contributor

trop bot commented Jan 14, 2020

I have automatically backported this PR to "8-x-y", please check out #21769

@trop

This comment has been minimized.

Copy link
Contributor

trop bot commented Jan 14, 2020

I have automatically backported this PR to "7-1-x", please check out #21770

@trop trop bot added in-flight/7-1-x and removed target/7-1-x labels Jan 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.