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.

Sign up for GitHub

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

Jump to bottom

Emulated Hosting redirects to Cloud Functions erroneously combines Set-Cookie headers #2931

Closed
adam-remotesocial opened this issue Dec 14, 2020 · 2 comments
Closed

Emulated Hosting redirects to Cloud Functions erroneously combines Set-Cookie headers #2931

adam-remotesocial opened this issue Dec 14, 2020 · 2 comments

Comments

@adam-remotesocial
Copy link
Contributor

adam-remotesocial commented Dec 14, 2020 •
edited
Loading

[REQUIRED] Environment info

firebase-tools: 8.18.1

Platform: OSX

[REQUIRED] Test case

Write a function that sets multiple cookies:

exports.myFunc = require('firebase-functions').https.onRequest((req, res) => {
    res.setHeader('Set-Cookie', [
        'some=cookie',
        'other=one'
    ]);
    res.statusCode = 200;
    res.end('');
});

Use Hosting as a proxy to the function. firebase.json:

{
    "hosting": {
        "rewrites": [
            { "source": "/wherever", "function": "myFunc" }
        ]
    }
}

[REQUIRED] Steps to reproduce

Hit the proxied function URL: http://myhostingsubdomain.blah.com/wherever. Check the response headers in Chrome DevTools and check the Application Cookies - only the first will be registered.

[REQUIRED] Expected behavior

The response should include multiple Set-Cookie headers - one for each cookie. Browsers (including Chrome) don't handle a single Set-Cookie header with multiple cookies in it and will ignore all but the first cookie (especially with cookie fields like Expires which can include commas in them). This prevents usage like https://dev.to/johncarroll/how-to-share-firebase-authentication-across-subdomains-1ka8 where your function has to be hosted at the same origin as your Hosting sites (so cloudfunctions.net/blah is no good).

I expect to see response headers like:

Set-Cookie: some=cookie
Set-Cookie: other=one

[REQUIRED] Actual behavior

The response has a single header like:

set-cookie: some=cookie, other=one

Cause

node-fetch is being used incorrectly, indirectly from lib/hosting/proxy.js via lib/apiv2.js's Client::request. See node-fetch/node-fetch#582 for a similar case. The headers are being extracted from proxyRes.response.headers in lib/hosting/proxy.js:

for (const [key, value] of proxyRes.response.headers) {
    res.setHeader(key, value);
}

But because node-fetch stringifies the incoming response headers (which for an Array, effectively concatenates them with commas), we lose our array of headers in the proxied response. The recommended approach is to use response.headers.raw() to grab those raw Array values. When I make that change in lib/hosting/proxy.js, it works:

for (const [key, value] of Object.entries(proxyRes.response.headers.raw())) {
    res.setHeader(key, value);
}

I have no yet validated whether the same problem happens in production or not. Really hoping it doesn't 🤞
UPDATE: Production is fine! This is an emulator-only problem.
@google-oss-bot
Copy link
Contributor

This issue does not have all the information required by the template. Looks like you forgot to fill out some sections. Please update the issue with more information.

@adam-remotesocial
Copy link
Contributor Author

Apologies to the electronic overlord. :) I think it's fixed now.

adam-remotesocial added a commit to adam-remotesocial/firebase-tools that referenced this issue Dec 14, 2020
@adam-remotesocial
Fixes firebase#2931 - Support multiple Set-Cookie headers
c92dc3d 
apiv2.js uses node-fetch under the hood, which will concatenate duplicate headers with ', '. This is invalid for Set-Cookie headers.
The solution is to grab the raw headers (which will be an array) and call `res.setHeader()` with that value.
@adam-remotesocial adam-remotesocial mentioned this issue Dec 14, 2020
Closed
bkendall added a commit that referenced this issue Dec 15, 2020
@bkendall @yuchenshi
@adam-remotesocial @google-oss-bot
npm audit: breaking edition (#2941)
396316d 
* Fix #2880 - generating email link from Admin SDK fails with Auth Emulator (#2933)

* Fix #2880.

* Add changelog entry.

* fixes set-cookie issue (updated #2932) (#2939)

* Fixes #2931 - Support multiple Set-Cookie headers

apiv2.js uses node-fetch under the hood, which will concatenate duplicate headers with ', '. This is invalid for Set-Cookie headers.
The solution is to grab the raw headers (which will be an array) and call `res.setHeader()` with that value.

* add test

* changelog

Co-authored-by: Adam Ahmed <[email protected]>

* 8.20.0

* [firebase-release] Removed change log and reset repo after 8.20.0 release

* auto fix audit issues

* force audit update major versions

* remove google-gax

Co-authored-by: Yuchen Shi <[email protected]>
Co-authored-by: Adam Ahmed <[email protected]>
Co-authored-by: Google Open Source Bot <[email protected]>
bkendall added a commit that referenced this issue Dec 15, 2020
@bkendall @joehan
@mbleigh @yuchenshi @adam-remotesocial @google-oss-bot
v9.0.0! (#2942)
8c22040 
* Removing code to handle legacy functions (#2935)

* Disable Node.js 8 deploys for Cloud Functions. (#2934)

* drop testing and support for node 8 (#2620)

* remove/bump node 8s to node 10s

* update some dev dependencies

* update build target to es2017

* formatting

* upgrade mocha

* remove async types

* update check in binary to node 8

* Update package.json

* npm audit: breaking edition (#2941)

* Fix #2880 - generating email link from Admin SDK fails with Auth Emulator (#2933)

* Fix #2880.

* Add changelog entry.

* fixes set-cookie issue (updated #2932) (#2939)

* Fixes #2931 - Support multiple Set-Cookie headers

apiv2.js uses node-fetch under the hood, which will concatenate duplicate headers with ', '. This is invalid for Set-Cookie headers.
The solution is to grab the raw headers (which will be an array) and call `res.setHeader()` with that value.

* add test

* changelog

Co-authored-by: Adam Ahmed <[email protected]>

* 8.20.0

* [firebase-release] Removed change log and reset repo after 8.20.0 release

* auto fix audit issues

* force audit update major versions

* remove google-gax

Co-authored-by: Yuchen Shi <[email protected]>
Co-authored-by: Adam Ahmed <[email protected]>
Co-authored-by: Google Open Source Bot <[email protected]>

Co-authored-by: joehan <[email protected]>
Co-authored-by: Michael Bleigh <[email protected]>
Co-authored-by: Yuchen Shi <[email protected]>
Co-authored-by: Adam Ahmed <[email protected]>
Co-authored-by: Google Open Source Bot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@google-oss-bot @adam-remotesocial