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

Converter<net::HttpResponseHeaders*>::FromV8 does not allow multiple headers with the same name #14778

Closed
ibash opened this issue Sep 24, 2018 · 0 comments

Comments

Projects
None yet
2 participants
@ibash
Copy link
Contributor

commented Sep 24, 2018

  • Output of node_modules/.bin/electron --version: v2.0.8
  • Operating System (Platform and Version): macos 10.13.6

To Reproduce

  1. protocol.interceptStreamProtocol to handle requests from electron
  2. In the callback, give a header like:
callback({
  headers: {
    'Set-Cookie': ['foo', 'bar', 'baz']
  }
})
  1. Request a url and inspect chrome dev tools

Expected Behavior
In dev tools you can see multiple set-cookie headers, and multiple cookies get set.

Actual behavior
No cookies are set.


To Reproduce
With a sample repo:

  1. clone / checkout the branch I'm on

    git clone git@github.com:ibash/nativefier.git
    cd nativefier
    git checkout offline_2
    
  2. install dependencies and build nativefier

    yarn install
    cd app
    yarn install
    cd ..
    yarn build
    
  3. package an example app and then run it

    node lib/cli.js --name "Queue Offline"  "https://safaribooksonline.com"
    ./Queue\ Offline-darwin-x64/Queue\ Offline.app/Contents/MacOS/Queue\ Offline
    
  4. open dev tools in the app

  5. inspect a network request

Expected Behavior
All the network requests should have three headers: "TEST: first", "TEST: second", "TEST: third"

Actual behavior
A single header like: "TEST: first, second, third"


Additional Information
The problem is that Converter<net::HttpResponseHeaders*>::FromV8 expects each header to only have a single string value, see this line of code.

Oddly enough converting headers to v8 is correct, and handles multiple headers with the same name as a list.

As far as I can tell there's no way to actually respond with multiple header values with the same name.

Last but not least, I found you can crash electron if you send a header value like: {bar: 'foo\0 bar: foo2'}

ibash added a commit to ibash/electron that referenced this issue Sep 30, 2018

fix: allow stream protocols to return headers with multiple values
This allows stream protocols to return headers with multiple values as
an array of values.

Fixes electron#14778

ibash added a commit to ibash/electron that referenced this issue Sep 30, 2018

fix: allow stream protocols to return headers with multiple values
This allows stream protocols to return headers with multiple values as
an array of values.

Fixes electron#14778

ibash added a commit to ibash/electron that referenced this issue Oct 23, 2018

fix: allow stream protocols to return headers with multiple values
This allows stream protocols to return headers with multiple values as
an array of values.

Fixes electron#14778

MarshallOfSound added a commit that referenced this issue Oct 25, 2018

fix: allow stream protocols to return headers with multiple values (#…
…14887)

* fix: allow stream protocols to return headers with multiple values

This allows stream protocols to return headers with multiple values as
an array of values.

Fixes #14778

* Prefer ConvertFromV8

* Cleanup header conversion

1. Deduplicate the code by using a lambda
2. Remove duplicate calls to headers->Get(key)

* Fix broken test

Headers with multiple values are now being converted correctly, this
test asserted the wrong behavior.

ibash added a commit to ibash/electron that referenced this issue Dec 28, 2018

fix: allow stream protocols to return headers with multiple values (e…
…lectron#14887)

* fix: allow stream protocols to return headers with multiple values

This allows stream protocols to return headers with multiple values as
an array of values.

Fixes electron#14778

* Prefer ConvertFromV8

* Cleanup header conversion

1. Deduplicate the code by using a lambda
2. Remove duplicate calls to headers->Get(key)

* Fix broken test

Headers with multiple values are now being converted correctly, this
test asserted the wrong behavior.

ibash added a commit to ibash/electron that referenced this issue Dec 28, 2018

fix: allow stream protocols to return headers with multiple values (e…
…lectron#14887)

* fix: allow stream protocols to return headers with multiple values

This allows stream protocols to return headers with multiple values as
an array of values.

Fixes electron#14778

* Prefer ConvertFromV8

* Cleanup header conversion

1. Deduplicate the code by using a lambda
2. Remove duplicate calls to headers->Get(key)

* Fix broken test

Headers with multiple values are now being converted correctly, this
test asserted the wrong behavior.

ibash added a commit to ibash/electron that referenced this issue Dec 28, 2018

fix: allow stream protocols to return headers with multiple values (e…
…lectron#14887)

notes: Fixes a bug that prevented stream protocols from returning headers with multiple values.

Fixes electron#14778

ibash added a commit to ibash/electron that referenced this issue Dec 28, 2018

fix: allow stream protocols to return headers with multiple values (e…
…lectron#14887)

notes: Fixed returning headers with multiple values for stream protocols.

Fixes electron#14778

miniak added a commit that referenced this issue May 1, 2019

fix: allow stream protocols to return headers with multiple values (#…
…14887)

* fix: allow stream protocols to return headers with multiple values

This allows stream protocols to return headers with multiple values as
an array of values.

Fixes #14778

* Prefer ConvertFromV8

* Cleanup header conversion

1. Deduplicate the code by using a lambda
2. Remove duplicate calls to headers->Get(key)

* Fix broken test

Headers with multiple values are now being converted correctly, this
test asserted the wrong behavior.

MarshallOfSound added a commit that referenced this issue May 1, 2019

fix: allow stream protocols to return headers with multiple values (#…
…14887) (#18094)

* fix: allow stream protocols to return headers with multiple values

This allows stream protocols to return headers with multiple values as
an array of values.

Fixes #14778

* Prefer ConvertFromV8

* Cleanup header conversion

1. Deduplicate the code by using a lambda
2. Remove duplicate calls to headers->Get(key)

* Fix broken test

Headers with multiple values are now being converted correctly, this
test asserted the wrong behavior.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.