cloudflare / workers-sdk

⛅️ Home to Wrangler, the CLI for Cloudflare Workers®
https://developers.cloudflare.com/workers/
Apache License 2.0
2.58k stars 664 forks source link

🐛 BUG: Redirects to not work properly in wrangler 3 #5860

Open philipwalton opened 4 months ago

philipwalton commented 4 months ago

Which Cloudflare product(s) does this pertain to?

Wrangler core

What version(s) of the tool(s) are you using?

3.56.0 [Wrangler]

What version of Node are you using?

v20.11.0

What operating system and version are you using?

macOS sonoma 14.4.1 (23E224)

Describe the Bug

Observed behavior

When calling return Response.redirect(url.href, 301) from within Worker code, I'm getting errors, both when using the unstable_dev API as well as when using wrangler dev with a custom local host specified.

Expected behavior

My redirect code fully worked with v2 of wrangler, but after upgrading to v3 I'm getting errors in both my tests and my local dev environment.

Steps to reproduce

  1. Visit https://stackblitz.com/edit/stackblitz-starters-vjsh9a?file=index.js
  2. Run node index.js (if it doesn't happen automatically)
  3. View the error logged to the terminal.

Screenshot 2024-05-16 at 8 56 06 PM

Here is the contents of the worker file in the repro:

// worker.js
export default {
  async fetch(request) {
    const url = new URL(request.url);

    if (url.pathname === '/needs-redirect') {
      url.pathname = '/redirected';
      return Response.redirect(url.href, 301);
    }

    return new Response('Hello World!');
  },
};

In addition to the error shown above when using unstable_dev, I'm also seeing errors for the same worker file when running wrangler dev locally with a local host specified:

wrangler dev --host=localhost:3000 ./worker.js

Then, if I use curl to test the redirect, I get an invalid Location header returned with the response. See the following screenshot:

Screenshot 2024-05-16 at 8 52 56 PM

Notice how path was property updated by the redirect logic in the worker, but the URL in the Location header is invalid, as it now includes port 3000 appended to the existing port (which is also repeated for some reason). This also did not happen when using wrangler v2.

Please provide a link to a minimal reproduction

https://stackblitz.com/edit/stackblitz-starters-vjsh9a?file=index.js

Please provide any relevant error logs

Error when using unstable_dev:

✘ [ERROR] failed to start worker registry TypeError: fetch failed

      at fetch
  (/home/projects/stackblitz-starters-vjsh9a/node_modules/wrangler/wrangler-dist/cli.js:17033:19)
      at async getRegisteredWorkers
  (/home/projects/stackblitz-starters-vjsh9a/node_modules/wrangler/wrangler-dist/cli.js:160730:22)
      at async getBoundRegisteredWorkers
  (/home/projects/stackblitz-starters-vjsh9a/node_modules/wrangler/wrangler-dist/cli.js:160750:29)
      at async startDevServer
  (/home/projects/stackblitz-starters-vjsh9a/node_modules/wrangler/wrangler-dist/cli.js:204454:40)
      at async getDevServer
  (/home/projects/stackblitz-starters-vjsh9a/node_modules/wrangler/wrangler-dist/cli.js:205190:12)
      at async startApiDev
  (/home/projects/stackblitz-starters-vjsh9a/node_modules/wrangler/wrangler-dist/cli.js:205251:21)
      at async Module.unstable_dev
  (/home/projects/stackblitz-starters-vjsh9a/node_modules/wrangler/wrangler-dist/cli.js:205672:23)
      at async _0x500592.run
  (https://stackblitzstartersvjsh9a-m5vp.w-credentialless-staticblitz.com/blitz.1d4c3cdd.js:40:785487)
      at async _0x5084b5._evaluate
  (https://stackblitzstartersvjsh9a-m5vp.w-credentialless-staticblitz.com/blitz.1d4c3cdd.js:40:792870)
      at async ModuleJob.run (node:internal/modules/esm/module_job:155:2441) {
    cause: AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:

      assert27(Number.isFinite(client[kMaxHeadersSize]) && client[kMaxHeadersSize] > 0)

        at new Parser
  (file:///home/projects/stackblitz-starters-vjsh9a/node_modules/wrangler/wrangler-dist/cli.js:6858:9)
        at connect
  (file:///home/projects/stackblitz-starters-vjsh9a/node_modules/wrangler/wrangler-dist/cli.js:7376:29)
  {
      generatedMessage: true,
      code: 'ERR_ASSERTION',
      actual: false,
      expected: true,
      operator: '=='
    }
  }

Invalid response when running wrangler with host localhost:3000 specified:

HTTP/1.1 301 Moved Permanently
Content-Length: 0
Location: http://localhost:8787:8787:3000/redirected
rameardo commented 4 months ago

Hello Philip Walton!

You encountered this issue because you need to:

Let's go through the solution step by step. First, modify your code as follows:

worker.js should look like this (your code is ok):

export default {
  async fetch(request) {
    const url = new URL(request.url);
    console.log('incoming request') // to see in console

    if (url.pathname === '/needs-redirect') {
      url.pathname = '/redirected';
      return Response.redirect(url.href, 301);
    }

    return new Response('Hello!');
  },
};

Next, update your index.js:

import { unstable_dev } from 'wrangler';

const worker = await unstable_dev('./worker.js', {
  experimental: { disableExperimentalWarning: true },
});

const response = await worker.fetch('/needs-redirect', { redirect: 'manual' }) // prevent automatic following by option redirect: 'manual'
if (response.status === 301)
  console.log('Redirected to:', response.headers.get('Location'));
else
  console.log('Response status:', response.status);

Testing the Setup

Open two terminals in your project directory.

In the first terminal, run your worker locally on localhost with port 3000:

$ wrangler dev --host=localhost --port=3000 ./worker.js

In the second terminal, run your index.js with the following command:

node index.js

output

You should see the following outputs:

In the first terminal:

>wrangler dev --host=localhost --port=3000 ./worker.js  
 ⛅️ wrangler 3.56.0
-------------------------------------------------------
⎔ Starting local server...
[wrangler:inf] Ready on http://127.0.0.1:3000

In the second terminal:

>node index.js
Redirected to: http://placeholder/redirected

Cheers, Adnan

philipwalton commented 4 months ago

You encountered this issue because you need to:

  • Ensure the fetch method follows redirects correctly by setting the redirect option to manual or handling it in a different way.
  • Use fetch options to prevent automatic following of redirects to handle them explicitly if needed.

Hi @rameardo, I appreciate the suggestion for how to work around this error via setting redirect: 'manual' in the fetch options, but I believe there is still a bug here because Cloudflare workers can definitely follow redirects, and so the local testing environment should be able to as well.

Also, you didn't comment on the second issue I reported, starting with the text:

In addition to the error shown above when using unstable_dev, I'm also seeing errors for the same worker file when running wrangler dev locally with a local host specified:

Do you have any idea as to what's causing the redirect URL to contain multiple ports: http://localhost:8787:8787:3000/redirected?

rameardo commented 4 months ago

Hello Philip Walton!

You are right; usually, Cloudflare Workers follow redirects. However, in local testing, I believe there are two types of execution: the first one is remote as local, and the second one is fully local.

For example, this is fully local:

$ wrangler dev

And this is remote:

wrangler dev -r

I think with remote, it should automatically follow redirects because the -r flag runs directly from Cloudflare infrastructure, as far as I know.

Sorry, I forgot to answer this part:

In addition to the error shown above when using unstable_dev, I'm also seeing errors for the same worker file when running wrangler dev locally with a local host specified:

I think you are right; this is a bug. After investigating the issue, I think your command is incorrect (correct and not correct) let me explain why. (BUT GENERALY, YES THIS IS A BUG)

Example 1: If you are executing your worker like this:

wrangler dev --host=localhost:3000 ./index.js

Here, we have the following (why @CloudflareTeam?) localhost:8787:3000

And yes, you are right; this is a bug! Wrangler should split the host localhost:3000 and replace it with port 8787.

Lets fix the issue

You don't need to specify the port in this manner. According to the Wrangler help page, the correct way is to use the --port flag. For example:

wrangler dev --host=localhost --port 3000 ./index.js

Here, we have the following: localhost:3000

But the issue you mentioned still exists. The reason is a hostname mismatch. Your worker is hosted on 127.0.0.1, which is the same as localhost. However, it seems Wrangler does not treat localhost exactly like 127.0.0.1. (@CloudflareTeam ??)

To fix this issue, you need to serve directly from 127.0.0.1. Use the following command:

wrangler dev --host=127.0.0.1 --port=3000 ./index.js

This command should fix the issue!

lets test

curl -i http://localhost:3000/needs-redirect

Now, the output should be as you want:

HTTP/1.1 301 Moved Permanently
Content-Length: 0
Location: http://localhost:3000/redirected

But generally, you are right; this is indeed a bug. However, the above is the solution for your case if you need an immediate fix.

philipwalton commented 4 months ago

You don't need to specify the port in this manner. According to the Wrangler help page, the correct way is to use the --port flag. For example:

wrangler dev --host=localhost --port 3000 ./index.js

Your example is actually different from mine, and I should clarify that in my real application code I'm setting both "host" and "port" to different port values. Here's what I have in my wrangler.toml file:

host = "localhost:3001" # Origin server
port = 3000 # Port the worker should listen on

Notice how I have a different port set under "host" then I do under "port". The reason I'm doing this is because I have an application server running on port "3001", and I have my Cloudflare worker running on port "3000", which acts as a proxy for my application server.

According to the wrangler documentation, the "port" configuration should not be the port of the application server but rather the port of the worker, which is how I have it configured.

I assume running a local application server during development time is very common, so if there's a better way to configure my worker to support that use case, please let me know.

philipwalton commented 4 months ago

You are right; usually, Cloudflare Workers follow redirects. However, in local testing, I believe there are two types of execution: the first one is remote as local, and the second one is fully local.

By the way, in my case I actually need to test the 'follow redirects' behavior because I have a redirect chain scenario that I want to make sure redirects to completion.

Also, to emphasize something I said above, this behavior worked in wrangler version 2, so this is a regression in version 3.

penalosa commented 3 months ago

Wrangler 2 defaulted to remote mode dev, while Wrangler 3 defaults to local mode dev. I think the --local-upstream flag should preserve the behaviour you're expecting (wrangler dev --local-upstream=localhost:3000 ./worker.js). Arguably --host should do this too (tracked in https://github.com/cloudflare/workers-sdk/issues/5125).

https://github.com/cloudflare/workers-sdk/issues/5221 tracks the issue with redirect URLs containing too many ports.

Could you confirm whether --local-upstream works for you?

philipwalton commented 3 months ago

Could you confirm whether --local-upstream works for you?

--local-upstream does not work for me, i.e. it does not solve either of the issues I've raised here:

The --local-upstream option does allow me to run wranger dev and use my local application server as an upstream, but --host actually already did that, and since host is a supported config option that seems like a better option to use, correct?

Wrangler 2 defaulted to remote mode dev, while Wrangler 3 defaults to local mode dev.

I understand, but to clarify, wrangler 2 (when using the --local flag) did not have either of the redirect issues I've raised here, so the issue it's just a change in defaults. The behavior has also changed.