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]: version v2.52.2 up to v2.52.3 , cors error #2936

Closed
3 tasks done
febelery opened this issue Mar 26, 2024 · 22 comments · Fixed by #2938
Closed
3 tasks done

🐛 [Bug]: version v2.52.2 up to v2.52.3 , cors error #2936

febelery opened this issue Mar 26, 2024 · 22 comments · Fixed by #2938

Comments

@febelery
Copy link

febelery commented Mar 26, 2024

Bug Description

version v2.52.2 up to v2.52.3 , cors error

version v2.52.2 is correct

How to Reproduce

Steps to reproduce the behavior:

  1. Go to '....'
  2. Click on '....'
  3. Do '....'
  4. See '....'

Expected Behavior

cors correct

Fiber Version

v2.52.3

Code Snippet (optional)

package main

import "github.com/gofiber/fiber/v2"
import "log"

func main() {
  app := fiber.New()

  	app.Use(cors.New(cors.Config{
		AllowOriginsFunc: func(origin string) bool {
			for _, c := range config.Get[[]string]("cors") {
				if c == origin {
					return true
				}
			}
			return false
		},
	}))

  log.Fatal(app.Listen(":3000"))
}

Checklist:

  • I agree to follow Fiber's Code of Conduct.
  • I have checked for existing issues that describe my problem prior to opening this one.
  • I understand that improperly formatted bug reports may be closed without explanation.
Copy link

welcome bot commented Mar 26, 2024

Thanks for opening your first issue here! 🎉 Be sure to follow the issue template! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

@bytesizedwizard
Copy link

bytesizedwizard commented Mar 26, 2024

I am getting the same issue after upgrading to 2.52.3. The cors settings seem to be ignored and all requests are blocked. Downgrading to 2.52.2 fixes it.

Just spent the last 3 hours scratching my head and re-evaluating my life choices only to realise it was an error in the package itself 😅

@LKaemmerling
Copy link

We are affected with the same issue, after a rollback to 2.5.2 it works again.

@ReneWerner87
Copy link
Member

@LKaemmerling @bytesizedwizard just to make sure you mean the last release 2.52.3 and not 2.5.x

can you give us the cors error so that we can fix it as soon as possible
@febelery
and also the output of

app.Use(cors.New(cors.Config{
    AllowOriginsFunc: func(origin string) bool {
        // request origin is important for us
        log.Println("request origin", origin)
        for _, c := range config.Get[[]string]("cors") {
            // your origin is important for us
            log.Println("match origin", c)
            if c == origin {
                return true
            }
        }
        return false
    },
}))

@ReneWerner87
Copy link
Member

@sixcolors FYI

@LKaemmerling
Copy link

@ReneWerner87 yes, sorry of course i was talking about 2.52.3. What i can see is that not a single CORS header is added to the response, so regardless of what we configure.

@ReneWerner87
Copy link
Member

@sixcolors we should also include the case of the consumers in our unittests so that this no longer happens
@LKaemmerling can you please log the input parameter of the function in the old and new version and give it to us

and also the response headers of the old and new version
possibly also the request origin headers

@LKaemmerling
Copy link

@ReneWerner87 we don't use the function directly, we simply configure the middleware with the following values:

	proxy.fiber.Use(
		otelfiber.Middleware(
			otelfiber.WithServerName("networking-proxy"),
			otelfiber.WithTracerProvider(instr.TracerProvider),
			otelfiber.WithPropagators(tracing.TextMapPropagator()),
		),
		cors.New(cors.Config{
			AllowOrigins:     "*",   // needs wildcard, as we provide a public API
			AllowCredentials: false, // false still allows for token bearer auth
			AllowHeaders:     "X-Requested-With,Authorization,Content-Type",
			MaxAge:           86400,
			AllowMethods:     "OPTIONS,GET,POST,PUT,PATCH,DELETE",
			ExposeHeaders:    "Link,X-Correlation-ID",
		}),
		metadataMiddleware(),
		sentryMiddleware(logger),
		metricsMiddleware(instr.MetricsRegisterer),
		authorizationMiddleware(),
	)

@ReneWerner87
Copy link
Member

@LKaemmerling can you give the request origin and also the response headers in the old and new versions for you

we need the possibility to recreate the problem on our site in order to correct it

it is also good to know that you are not using the function, then it probably has something to do with the following process
at first i thought it was something specific to the function

@LKaemmerling
Copy link

LKaemmerling commented Mar 26, 2024

@ReneWerner87 working (with 2.52.2):

(copied from browser)
Req:
curl 'https://api.our-staging-environment.works/v1/datacenters?page=1&per_page=50' \
  -H 'accept: application/json, text/plain, */*' \
  -H 'accept-language: de-DE,de;q=0.9,en-DE;q=0.8,en;q=0.7,en-US;q=0.6,fr;q=0.5' \
  -H 'authorization: Bearer yoOneM2yk4CKfqrwyuYz8erxE8ZlUQp0HcuU3ku2s3LHK8sS672pm800fXJ69DI4' \
  -H 'content-type: application/json' \
  -H 'origin: https://our-staging-environment.works' \
  -H 'referer: https://our-staging-environment.works/' \
  -H 'sec-ch-ua: "Google Chrome";v="123", "Not:A-Brand";v="8", "Chromium";v="123"' \
  -H 'sec-ch-ua-mobile: ?0' \
  -H 'sec-ch-ua-platform: "macOS"' \
  -H 'sec-fetch-dest: empty' \
  -H 'sec-fetch-mode: cors' \
  -H 'sec-fetch-site: same-site' \
  -H 'user-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/123.0.0.0 Safari/537.36'
Resp:
Access-Control-Allow-Origin:
*
Access-Control-Expose-Headers:
Link,X-Correlation-ID
Content-Length:
6538
Content-Type:
application/json
Date:
Tue, 26 Mar 2024 10:15:28 GMT
Link:
<https://staging-environment.work/v1/datacenters?page=1&per_page=50>; rel="last"
Ratelimit-Limit:
3600
Ratelimit-Remaining:
3596
Ratelimit-Reset:
1711448132
Strict-Transport-Security:
max-age=31536000; includeSubDomains
Traceparent:
00-fadc03b7196b8c11be29b53baeabd8ad-c37a7132aefb1a01-01
Uber-Trace-Id:
fadc03b7196b8c11be29b53baeabd8ad:c37a7132aefb1a01:0:1
Vary:
Origin
X-Correlation-Id:
fadc03b7196b8c11be29b53baeabd8ad

Broken: (v2.52.3)

Req is the same

Response:
Content-Length:
6538
Content-Type:
application/json
Date:
Tue, 26 Mar 2024 10:22:11 GMT
Link:
<https://staging-environment.work/v1/datacenters?page=1&per_page=50>; rel="last"
Ratelimit-Limit:
3600
Ratelimit-Remaining:
3593
Ratelimit-Reset:
1711448538
Strict-Transport-Security:
max-age=31536000; includeSubDomains
Traceparent:
00-781e9af000c65354efd7695d6dc5595b-9bc3d5996294ead6-01
Uber-Trace-Id:
781e9af000c65354efd7695d6dc5595b:9bc3d5996294ead6:0:1
X-Correlation-Id:
781e9af000c65354efd7695d6dc5595b

So you can see that the CORS headers are missing.

@ReneWerner87
Copy link
Member

@sixcolors this is the problem

the second condition

if originHeader == "" || c.Get(fiber.HeaderAccessControlRequestMethod) == "" {

@ReneWerner87
Copy link
Member

related issue #2920
image

releated fix #2921
image

@ReneWerner87
Copy link
Member

believe this check should only be added to the options request and not to the actual request of the action

@ReneWerner87
Copy link
Member

we could confirm that our new logic, which is closer to the standard, is unfortunately not decapsulated and also affects the non-preflight request

we will adjust this by the end of the day or tomorrow morning and then release in a timely manner

until then I would suggest skipping the last release

@sixcolors
Copy link
Member

@febelery @LKaemmerling and @bytesizedwizard are you able to verify if #2937 fixes CORS in your dev envs?

@ReneWerner87
Copy link
Member

@LKaemmerling @febelery @bytesizedwizard

can you pls briefly check if everything fits with the following commit before I release and tag it

go.mod

require github.com/gofiber/fiber/v2 v2.52.4-0.20240326212242-a6f4c133bc74

@LKaemmerling
Copy link

LKaemmerling commented Mar 26, 2024

@ReneWerner87 @sixcolors thank you a lot! With your recent changes (`v2.52.4-0.20240326212242-a6f4c133bc74) i can confirm it now works again as we would expect it.

@ReneWerner87
Copy link
Member

thanks, but wait until the other two confirm, then I will release

@luisgarciaalanis
Copy link

luisgarciaalanis commented Mar 27, 2024

Ok, I created a server that serves an html with a script to fetch from another subdomain.
it fails with the normal fiber v2

added the new changed version, go mod tidy, go mod vendor and now it works!

Screenshot 2024-03-26 at 10 36 33 PM

Fix seems to resolve the issue.

@luisgarciaalanis
Copy link

luisgarciaalanis commented Mar 27, 2024

Can this be forward ported to v3?

Should I open an issue for v3 and post the repro app code?

@ReneWerner87
Copy link
Member

ReneWerner87 commented Mar 27, 2024

solved with v2.52.4

Can this be forward ported to v3?

we will do this

@sixcolors
Copy link
Member

sixcolors commented Mar 27, 2024

solved with v2.52.4

Can this be forward ported to v3?

we will do this

@ReneWerner87 #2938 is the v3 fix.

However @LKaemmerling i would caution against using v3 at this time. It’s still under active development and is not stable. Okay to play with. But would not use in production.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants