Repository: firebase/firebase-tools (ts)

Issue:
Dynamic content functions redirect in emulators to :5001 port instead of :5000
<!-- DO NOT DELETE
validate_template=true
template_path=.github/ISSUE_TEMPLATE/bug_report.md
-->

<!--
Thank you for contributing to the Firebase community!

Think you found a bug?
=======================
Yeah, we're definitely not perfect! Please use this template and include a minimal repro when opening the issue. If you know how to solve the issue, please create a Pull Request, and we'd be happy to review it!

Have a feature request?
========================
Great, we love hearing how we can improve our products! However, do not use this template to submit a feature request. Please submit your feature requests to: https://firebase.google.com/support/contact/bugs-features/

Have a usage question?
=======================
We get lots of those and we love helping you, but GitHub is not the best place for them and they will be closed. Please take a look at the guide first: https://firebase.google.com/docs/cli/

If the official documentation doesn't help, try asking through our official support channel: https://firebase.google.com/support/

Additional locations to check for solutions or assistance from the community:
- Stack Overflow: https://stackoverflow.com/
- Firebase Slack Community: https://firebase.community/

*Please avoid duplicate posting across multiple channels!*
-->

### [REQUIRED] Environment info

<!-- Output of `firebase --version` -->

**firebase-tools:** 9.1.0

<!-- e.g. macOS, Windows, Ubuntu -->

**Platform:** macOS

### [REQUIRED] Test case

- Setup serving dynamic content with firebase hosting as per firebase documentation (using Express).
- Trigger a function by navigating in browser to localhost:5000/test
- Return res.redirect("/destination")  at in the express function
- User navigated to localhost:5001/destination
- User gets 404 error (expected for localhost:5001/destination as nothing is there)

<!-- Provide a minimal, complete, and verifiable example (http://stackoverflow.com/help/mcve) -->

### [REQUIRED] Steps to reproduce
See above
<!-- Provide the steps needed to reproduce the issue with the above test case. -->

### [REQUIRED] Expected behavior

Expect the redirect to use the :5000 port to navigate the user to: localhost:5000/destination
NOTE - when deployed to live, this redirect works fine. Only when operating in the emulator do the ports seem to get muddled up.

### [REQUIRED] Actual behavior

Redirects to localhost:5001/destination.

I understand this is because the functions emulator is running at :5001, however production redirects to the hosting path without any issues.

Is there either a fix to be done to the emulator, or a way I can tell express to use :5000 port instead of :5001 ? 


Pull request:
Fixes manipulated location header in Hosting emulator proxy logic
<!--

Thank you for contributing to the Firebase community! Please fill out the form below.

Run the linter and test suite
==============================
Run `npm test` to make sure your changes compile properly and the tests all pass on your local machine. We've hooked up this repo with continuous integration to double check those things for you.

-->

### Description

Fixes #3097

Node fetch attempts to make life easier, but in fact changes behavior that's defined in the spec and doesn't currently document it: https://github.com/node-fetch/node-fetch/blob/4abbfd231f4bce7dbe65e060a6323fc6917fd6d9/src/index.js#L117-L120

I've filed a bug with them: https://github.com/node-fetch/node-fetch/issues/1086

For now, the best we can do is attempt to fix the `location` header that comes back if it's a resolved URL and that URL points us to the same origin as our original proxy request. This allows valid URLs to come back from the proxy target and only strips the origin if it's a redirect to the same origin.

### Scenarios Tested

I have a manual setup where a redirect is returned. I see now that the `location` header is what I expect:

before:

```
» curl -I http://localhost:5000/start
HTTP/1.1 302 Found
x-powered-by: Express
location: http://localhost:5001/somewhereelse
```

after:

```
» curl -I http://localhost:5000/start
HTTP/1.1 302 Found
x-powered-by: Express
location: /somewhereelse
```


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index e69de29bb..29a904456 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -0,0 +1,1 @@
+- Fixes issue where proxied requests to dynamic content through the Hosting emulator would return unexpected `location` headers. (#3097)
diff --git a/src/hosting/proxy.ts b/src/hosting/proxy.ts
index 160a758c5..494dc2f71 100644
--- a/src/hosting/proxy.ts
+++ b/src/hosting/proxy.ts
@@ -141,6 +141,31 @@ export function proxyRequestHandler(url: string, rewriteIdentifier: string): Req
 
     proxyRes.response.headers.set("vary", makeVary(proxyRes.response.headers.get("vary")));
 
+    // Fix the location header that `node-fetch` attempts to helpfully fix:
+    // https://github.com/node-fetch/node-fetch/blob/4abbfd231f4bce7dbe65e060a6323fc6917fd6d9/src/index.js#L117-L120
+    // Filed a bug in `node-fetch` to either document the change or fix it:
+    // https://github.com/node-fetch/node-fetch/issues/1086
+    const location = proxyRes.response.headers.get("location");
+    if (location) {
+      // If parsing the URL fails, it may be because the location header
+      // isn't a helpeful resolved URL (if node-fetch changes behavior). This
+      // try is a preventative measure to ensure such a change shouldn't break
+      // our emulator.
+      try {
+        const locationURL = new URL(location);
+        // Only assume we can fix the location header if the origin of the
+        // "fixed" header is the same as the origin of the outbound request.
+        if (locationURL.origin == u.origin) {
+          const unborkedLocation = location.replace(locationURL.origin, "");
+          proxyRes.response.headers.set("location", unborkedLocation);
+        }
+      } catch (e) {
+        logger.debug(
+          `[hosting] had trouble parsing location header, but this may be okay: "${location}"`
+        );
+      }
+    }
+
     for (const [key, value] of Object.entries(proxyRes.response.headers.raw())) {
       res.setHeader(key, value as string[]);
     }


Test patch:
diff --git a/src/test/hosting/functionsProxy.spec.ts b/src/test/hosting/functionsProxy.spec.ts
index 0c616635c..948d17637 100644
--- a/src/test/hosting/functionsProxy.spec.ts
+++ b/src/test/hosting/functionsProxy.spec.ts
@@ -67,6 +67,48 @@ describe("functionsProxy", () => {
       });
   });
 
+  it("should maintain the location header as returned by the function", async () => {
+    nock("http://localhost:7778")
+      .get("/project-foo/us-central1/bar/")
+      .reply(301, "", { location: "/over-here" });
+
+    const options = cloneDeep(fakeOptions);
+    options.targets = ["functions"];
+
+    const mwGenerator = functionsProxy(options);
+    const mw = await mwGenerator(fakeRewrite);
+    const spyMw = sinon.spy(mw);
+
+    return supertest(spyMw)
+      .get("/")
+      .expect(301)
+      .then((res) => {
+        expect(res.header["location"]).to.equal("/over-here");
+        expect(spyMw.calledOnce).to.be.true;
+      });
+  });
+
+  it("should allow location headers that wouldn't redirect to itself", async () => {
+    nock("http://localhost:7778")
+      .get("/project-foo/us-central1/bar/")
+      .reply(301, "", { location: "https://example.com/foo" });
+
+    const options = cloneDeep(fakeOptions);
+    options.targets = ["functions"];
+
+    const mwGenerator = functionsProxy(options);
+    const mw = await mwGenerator(fakeRewrite);
+    const spyMw = sinon.spy(mw);
+
+    return supertest(spyMw)
+      .get("/")
+      .expect(301)
+      .then((res) => {
+        expect(res.header["location"]).to.equal("https://example.com/foo");
+        expect(spyMw.calledOnce).to.be.true;
+      });
+  });
+
   it("should proxy a request body on a POST request", async () => {
     nock("http://localhost:7778")
       .post("/project-foo/us-central1/bar/", "data")