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

External redirects don't work without 200 status #3770

Closed
hrishikesh-k opened this issue Dec 5, 2021 · 3 comments · Fixed by #4109 or #4179
Closed

External redirects don't work without 200 status #3770

hrishikesh-k opened this issue Dec 5, 2021 · 3 comments · Fixed by #4109 or #4179
Assignees
@lukasholzer @tinfoil-knight
Labels
area: redirects type: bug code to address defects in shipped code

Comments

@hrishikesh-k
Copy link

Describe the bug:

A user reported on the forums that they were not able to get the external redirects to work with Netlify CLI. The redirect syntax is valid.

Upon further investigation, it looks like CLI is adding the redirect correctly, but the location header is incorrect. For example, with the following _redirects file:

/foo https://www.netlify.com/ 301!

I see this:

image

It's basically redirecting it back to the home page.

To reproduce:

  1. Create _redirects file as mentioned above and an index.html in a working directory. It can be simple as:
<!doctype html>
<html>
  <head>
    <title>Foo</title>
  </head>
  <body>
    <p>Hi!</p>
  </body>
</html>
  1. Run netlify dev.
  2. Try visiting dev-url/foo.

Configuration:

  System:
    OS: macOS 12.0.1
    CPU: (12) x64 Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
    Memory: 796.11 MB / 16.00 GB
    Shell: 5.8 - /bin/zsh
  Binaries:
    Node: 14.17.5 - ~/.nvm/versions/node/v14.17.5/bin/node
    npm: 6.14.14 - ~/.nvm/versions/node/v14.17.5/bin/npm
  npmPackages:
    netlify-cli: ^8.0.15 => 8.0.15

Expected behaviour:

It should redirect to the required URL.

CLI Output

If applicable, add the CLI output to help explain your problem.

Additional context

This seems to work fine with the 200! configuration.
@hrishikesh-k hrishikesh-k added type: bug code to address defects in shipped code area: redirects labels Dec 5, 2021
@lukasholzer lukasholzer self-assigned this Dec 9, 2021
@tinfoil-knight
Copy link
Contributor

tinfoil-knight commented Jan 19, 2022 •
edited
Loading

@lukasholzer I was trying to debug this issue.

This is the place where redirects are handled:

cli/src/utils/proxy.js

Lines 216 to 227 in 47bf5eb

if (isRedirect(match)) {
res.writeHead(match.status, {
Location: destURL,
'Cache-Control': 'no-cache',
})
res.end(`Redirecting to ${destURL}`)
return
}
if (isExternal(match)) {
return proxyToExternalUrl({ req, res, dest, destURL })
}

The isRedirect function matches all redirects with http status code 301. Therefore, instead of reaching the isExternal branch, the redirect for the external url is handled in the isRedirect branch itself which is causing the issue.

cli/src/utils/proxy.js

Lines 79 to 81 in 47bf5eb

const isRedirect = function (match) {
return match.status && match.status >= 300 && match.status <= 400
}

Switching the order of isRedirect & isExternal blocks should resolve the issue. Should I make a PR?

@lukasholzer
Copy link
Collaborator

@tinfoil-knight do you want to continue and take over on this issue?

@tinfoil-knight
Copy link
Contributor

tinfoil-knight commented Jan 19, 2022 •
edited
Loading

@tinfoil-knight do you want to continue and take over on this issue?

Sure.

@tinfoil-knight tinfoil-knight mentioned this issue Jan 21, 2022
Merged
5 tasks
@kodiakhq kodiakhq bot closed this as completed in #4109 Feb 4, 2022
kodiakhq bot added a commit that referenced this issue Feb 4, 2022
@tinfoil-knight @erezrokah @kodiakhq
fix: external redirects not being matched correctly closes #3770 (#4109)
32714f0 
* test: add failing test to reproduce issue with 301 redirects

* fix: external redirects not being matched correctly

Co-authored-by: Erez Rokah <erezrokah@users.noreply.github.com>
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
@token-generator-app token-generator-app bot mentioned this issue Feb 4, 2022
Merged
@token-generator-app token-generator-app bot mentioned this issue May 26, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lukasholzer lukasholzer

@tinfoil-knight tinfoil-knight

Labels
area: redirects type: bug code to address defects in shipped code
Projects
None yet
Milestone
No milestone
3 participants
@lukasholzer @hrishikesh-k @tinfoil-knight