fix: support for redirects with 200 status. Fix crashing dev server w… #7034

VitaliyR · 2025-02-12T20:51:50Z

…hen have socket error. Follow redirects from proxyRes

🎉 Thanks for submitting a pull request! 🎉

Summary

Fixes WRFL-2263

One of the customers who are using Gatsby uses a 200 redirect

[[redirects]]
from = "/de-DE/*"
to = "/:splat"
status = 200

It works on prod but doesn't in netlify dev

Root cause for that we are not overriding the url if the file is not static, and in case of dev server running on gatsby (or next as well) - it is never static.

See the updated redirects.test.ts -> specifically the site-with-redirect > should do local redirect
It creates a simple redirect for /local/* to /:splat with status 200, user navigates for /local/test/exists:

before the change: 404 page not found
after the change: keeping url of /local/test/exists but content of /test/exists (as per docs)

For us to review and ship your PR efficiently, please perform the following steps:

Open a bug/issue before writing your code 🧑‍💻. This ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or something that`s on fire 🔥 (e.g. incident related), you can skip this step.
Read the contribution guidelines 📖. This ensures your code follows our style guide and
passes our tests.
Update or add tests (if any source code was changed or added) 🧪
Update or add documentation (if features were changed or added) 📝
Make sure the status checks below are successful ✅

A picture of a cute animal (not mandatory, but encouraged)

github-actions · 2025-02-12T20:53:01Z

📊 Benchmark results

Comparing with 9cb2f64

Dependency count: 1,192 (no change)
Package size: 306 MB (no change)
Number of ts-expect-error directives: 799 ⬇️ 0.25% decrease vs. 9cb2f64

VitaliyR · 2025-02-12T20:52:08Z

src/utils/proxy.ts

@@ -202,7 +206,7 @@ const handleAddonUrl = function ({ addonUrl, req, res }) {

 // @ts-expect-error TS(7006) FIXME: Parameter 'match' implicitly has an 'any' type.
 const isRedirect = function (match) {
-  return match.status && match.status >= 300 && match.status <= 400


I guess this is a mistake, 400 should not be included

VitaliyR · 2025-02-12T20:52:46Z

src/utils/proxy.ts

  return url?.startsWith('/.netlify/') ?? false
 }

+function isInternal(url?: string): boolean {


I've separated these 2 methods
All places of usage is using isInternalFunctions, and only 1 place which affects URL override use this method

VitaliyR · 2025-02-12T20:54:32Z

src/utils/proxy.ts

-      !isInternal(req.url) &&
-      !isInternal(destURL) &&
+      !isInternalFunctions(req.url) &&
+      !isInternalFunctions(destURL) &&
      (ct.endsWith('/x-www-form-urlencoded') || ct === 'multipart/form-data')
    ) {
      return proxy.web(req, res, { target: options.functionsServer })


on line 433-437
https://github.com/netlify/cli/pull/7034/files#diff-7c2533ca30d8693b3cecb3406646bc13b86b3969b43a0aa0659bb52c7676caa9R437

(GH not allow to leave a comment there, bummer)

instead of checking if it's just a static file, or internalUrl which is now isInternalFunctions -> it uses isInternal which just checks if url starts with / meaning it's internal url.

And then basically it lands into the if-else check and does the status/url override

VitaliyR · 2025-02-12T20:54:48Z

src/utils/proxy.ts

+  proxy.on('error', (err, req, res) => {
+    console.error('Got error from proxy', err);
+
+    if (res instanceof http.ServerResponse) {


I had this issue here and there, wanted to fix it as well

VitaliyR · 2025-02-12T20:54:59Z

src/utils/proxy.ts

-      'Content-Type': 'text/plain',
-    })
+  proxy.on('error', (err, req, res) => {
+    console.error('Got error from proxy', err);


I think more logging is better than less

VitaliyR · 2025-02-12T21:36:19Z

src/utils/proxy.ts

-        options: req.proxyOptions,
-        siteInfo,
-        env,
+    if (isRedirect({ status: proxyRes.statusCode }) && proxyRes.headers.location) {


here is another change.
User has a redirect to the page, which also has internal redirect.
For instance, gatsby sites likes to do a redirection from /page to /page/ (slash in the end).

So we basically want to follow it:

user hits /test/page

it matches redirect /test/* -> /:splat

cli proxies contents of the /:splat, which includes Location header. But status is 200 instead of 3xx (and original is 302 for instance, meaning localhost:8000 returns { 302, Location }, but location:8888 returns { 200, Location }
Users see now this page:

This is browser built-in contents. Location header is there. Browser will never do the redirect since, if you want it to happen, either status should be 3xx, or another header Refresh should be there.

So, instead, here we are changing the flow for the netlify dev (which is typically runs SSGs in develop mode, not in the production mode) to this:

| Target app wants redirect on this route? Follow it

This change can be left intact and for instance, I can enable it only for some of the contexts (e.g. for dev server context). WDYT?

VitaliyR · 2025-02-12T21:36:58Z

tests/integration/__fixtures__/site-with-redirect/index.js

+    res.write('Root page')
+    res.end()
+  } else if (pathname === '/test/exists') {
+    res.writeHead(302, undefined, { location: '/test/exists/' })


examples of the site which makes redirect, so then in netlify toml we want to land on this page, which then wants to jump to the next

VitaliyR · 2025-02-14T15:51:37Z

tests/integration/__fixtures__/hugo-site/netlify.toml

@@ -2,6 +2,6 @@
 command = "npm run serve"
 framework = "#custom"
 functions = "functions/"
-port = 7000
+port = 7001


7000 by default on OS X taken by airplay, failed my tests

…hen have socket error. Follow redirects from proxyRes

VitaliyR · 2025-02-14T20:18:20Z

src/utils/proxy.ts

      req.url = destStaticFile ? destStaticFile + dest.search : destURL
      const { status } = match
-      statusValue = status
+      if (match.force || status !== 200) {


override status code only if forcing or status is not 200, when 200 is just shadowing the page by the url, so it's status should be propagated (e.g. 404 because it's not there or idk 201 because server decided to return 201 by that url)

VitaliyR requested a review from a team as a code owner February 12, 2025 20:51

VitaliyR commented Feb 12, 2025

View reviewed changes

VitaliyR marked this pull request as draft February 12, 2025 22:57

VitaliyR force-pushed the vitaliir/WRFL-2263 branch from 52d39e4 to f334be1 Compare February 14, 2025 15:50

VitaliyR commented Feb 14, 2025

View reviewed changes

VitaliyR force-pushed the vitaliir/WRFL-2263 branch 2 times, most recently from b9b41f6 to 59c1680 Compare February 14, 2025 19:32

fix: support for redirects with 200 status. Fix crashing dev server w…

90cbb20

…hen have socket error. Follow redirects from proxyRes

VitaliyR force-pushed the vitaliir/WRFL-2263 branch from 59c1680 to 90cbb20 Compare February 14, 2025 20:08

VitaliyR commented Feb 14, 2025

View reviewed changes

VitaliyR marked this pull request as ready for review February 20, 2025 17:49

VitaliyR requested a review from eduardoboucas February 20, 2025 17:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: support for redirects with 200 status. Fix crashing dev server w… #7034

fix: support for redirects with 200 status. Fix crashing dev server w… #7034

VitaliyR commented Feb 12, 2025 •

edited

Loading

github-actions bot commented Feb 12, 2025 •

edited

Loading

VitaliyR Feb 12, 2025

VitaliyR Feb 12, 2025

VitaliyR Feb 12, 2025

VitaliyR Feb 12, 2025

VitaliyR Feb 12, 2025

VitaliyR Feb 12, 2025

VitaliyR Feb 12, 2025

VitaliyR Feb 14, 2025

VitaliyR Feb 14, 2025

fix: support for redirects with 200 status. Fix crashing dev server w… #7034

Are you sure you want to change the base?

fix: support for redirects with 200 status. Fix crashing dev server w… #7034

Conversation

VitaliyR commented Feb 12, 2025 • edited Loading

Summary

github-actions bot commented Feb 12, 2025 • edited Loading

📊 Benchmark results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VitaliyR commented Feb 12, 2025 •

edited

Loading

github-actions bot commented Feb 12, 2025 •

edited

Loading