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

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

Open
philipwalton opened this issue May 17, 2024 · 7 comments
Open

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

philipwalton opened this issue May 17, 2024 · 7 comments
Labels
bug Something that isn't working

Comments

@philipwalton
Copy link

philipwalton commented May 17, 2024

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
@philipwalton philipwalton added the bug Something that isn't working label May 17, 2024
@github-project-automation github-project-automation bot moved this to Untriaged in workers-sdk May 17, 2024
@philipwalton philipwalton changed the title 🐛 BUG: 🐛 BUG: Redirects to not work properly in wrangler 3 May 17, 2024
@rameardo
Copy link

Hello Philip Walton!

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.

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
Copy link
Author

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
Copy link

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
Copy link
Author

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
Copy link
Author

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
Copy link
Contributor

penalosa commented May 23, 2024

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 #5125).

#5221 tracks the issue with redirect URLs containing too many ports.

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

@penalosa penalosa added the awaiting reporter response Needs clarification or followup from OP label May 23, 2024
@philipwalton
Copy link
Author

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:

  • I'm still seeing the redirect error when testing fetch() via unstable_dev, and
  • I'm still seeing the issue with redirect URLs having too many ports (which I'm happy to track via #5221 and consider this bug just about the issue with unstable_dev)

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.

@penalosa penalosa added awaiting Cloudflare response Awaiting response from workers-sdk maintainer team and removed awaiting reporter response Needs clarification or followup from OP labels May 23, 2024
@penalosa penalosa moved this from Untriaged to Backlog in workers-sdk Aug 19, 2024
@RamIdeas RamIdeas removed the awaiting Cloudflare response Awaiting response from workers-sdk maintainer team label Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something that isn't working
Projects
Status: Backlog
Development

No branches or pull requests

4 participants