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]: CORS errors #2389

Closed
3 tasks done
jney opened this issue Mar 28, 2023 · 19 comments
Closed
3 tasks done

🐛 [Bug]: CORS errors #2389

jney opened this issue Mar 28, 2023 · 19 comments

Comments

@jney
Copy link
Contributor

jney commented Mar 28, 2023

Bug Description

updating fiber version generate a lot of CORS issue on the frontend.

- github.com/gofiber/fiber/v2 v2.42.0
+ github.com/gofiber/fiber/v2 v2.43.0

How to Reproduce

don't know

Expected Behavior

no cors error

Fiber Version

v2.43.0

Code Snippet (optional)

No response

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.
@ReneWerner87
Copy link
Member

unfortunately, without a description of the problem and an example, we can do nothing

@ReneWerner87
Copy link
Member

ReneWerner87 commented Mar 28, 2023

maybe its related to #2339

but without an observation report or other information we can unfortunately do nothing
@ryand67 did you test your change again after the release ? did you notice anything negative ?

@jney
Copy link
Contributor Author

jney commented Mar 28, 2023

Hi @ReneWerner87, I didn't find out what was the exact cause. But it happens on the frontend so I need to identify the problematic header.
And yes rolling back to v2.42.0 just make it work

@ReneWerner87 ReneWerner87 reopened this Mar 28, 2023
@ReneWerner87
Copy link
Member

@jney please expand your report with error messages and the network traffic for the requests that failed

preferably the browser console messages and the responses of the failed requests

@jney
Copy link
Contributor Author

jney commented Mar 28, 2023

I guess I found out the issue.

- < Access-Control-Allow-Origin: *
+ < Access-Control-Allow-Origin: 

@jney
Copy link
Contributor Author

jney commented Mar 28, 2023

	srv := fiber.New(fiber.Config{
		CaseSensitive: true,
		StrictRouting: true,
	})
	srv.Use(cors.New(cors.Config{AllowCredentials: true}))

@ReneWerner87
Copy link
Member

@ryand67 can you support here

@ryand67
Copy link
Contributor

ryand67 commented Mar 28, 2023

@ReneWerner87 I'm out recovering from a surgery so it may be a week or so before I can really commit much time. And I had tested and it seemed to be working on my tests I had run. I'll try and get on and figure it out but may be a minute.

@ReneWerner87
Copy link
Member

	srv := fiber.New(fiber.Config{
		CaseSensitive: true,
		StrictRouting: true,
	})
	srv.Use(cors.New(cors.Config{AllowCredentials: true}))

with this configuration i recieve
image
inside of the middleware

I guess I found out the issue.

- < Access-Control-Allow-Origin: *
+ < Access-Control-Allow-Origin: 

what do you mean by this?

@jney
Copy link
Contributor Author

jney commented Mar 28, 2023

- < Access-Control-Allow-Origin: *
+ < Access-Control-Allow-Origin: 

what do you mean by this?

Header changed

  • v2.42.0: Access-Control-Allow-Origin: *
  • v2.43.0: Access-Control-Allow-Origin:

@ReneWerner87
Copy link
Member

ReneWerner87 commented Mar 28, 2023

yes that's what I thought, just wanted to have a bit more context, since I can't recreate it in my case

in the browser, in postman ? how is the request ? or the origin header ?

@jney
Copy link
Contributor Author

jney commented Mar 28, 2023

not sure to understand the question. you want a request/response?
Here is an example sort of anonymized

# curl 'https://events.staging.example.com/v1/events'   -H 'sec-ch-ua: "Google Chrome";v="111", "Not(A:Brand";v="8", "Chromium";v="111"'   -H 'sec-ch-ua-platform: "macOS"'   -H 'Referer: https://publisher.staging.missena.click/'   -H 'sec-ch-ua-mobile: ?0'   -H 'User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/111.0.0.0 Safari/537.36'   -H 'Content-Type: application/msgpack'   --data-raw $'\u0084¤name§consentªparameters\u0084§appliesïmissing_purpose®missing_vendoræstringÚ\u0001ECPbOX4APbOX4AAHABBENCVCsAP_AAAAAAAAAI3Nf_X__b3_j-_5___t0eY1f9_7__-0zjhfdt-8N3f_X_L8X_2M7vF36pq4KuR4Eu3LBIQdlHOHcTUmw6okVrzPsbk2cr7NKJ7PEmnMbe2dYGH9_n93T-ZKY7_____77__-_______f__-_f___5_3__-_f_V_997fn9_____9_P___9v__9__________3wAAAEkoAMAAQRuDQAYAAgjcKgAwABBG4pABgACCNw6ADAAEEbiEAGAAII3BIAMAAQRuEQAYAAgjcMgAwABBG4.f_gAAAAAAAAA¤from¨mosquito¨sampling\u0001'   --compressed -v

*   Trying 1.1.1.1:443...
* Connected to events.staging.example.com (1.1.1.1) port 443 (#0)
* ALPN: offers h2
* ALPN: offers http/1.1
*  CAfile: /etc/ssl/cert.pem
*  CApath: none
* (304) (OUT), TLS handshake, Client hello (1):
* (304) (IN), TLS handshake, Server hello (2):
* TLSv1.2 (IN), TLS handshake, Certificate (11):
* TLSv1.2 (IN), TLS handshake, Server key exchange (12):
* TLSv1.2 (IN), TLS handshake, Server finished (14):
* TLSv1.2 (OUT), TLS handshake, Client key exchange (16):
* TLSv1.2 (OUT), TLS change cipher, Change cipher spec (1):
* TLSv1.2 (OUT), TLS handshake, Finished (20):
* TLSv1.2 (IN), TLS change cipher, Change cipher spec (1):
* TLSv1.2 (IN), TLS handshake, Finished (20):
* SSL connection using TLSv1.2 / ECDHE-RSA-AES128-GCM-SHA256
* ALPN: server did not agree on a protocol. Uses default.
* Server certificate:
*  subject: CN=*.missena.io
*  start date: Feb  9 00:00:00 2023 GMT
*  expire date: Jun  2 23:59:59 2023 GMT
*  subjectAltName: host "events.staging.example.com" matched cert's "*.staging.example.com"
*  issuer: C=US; O=Amazon; CN=Amazon RSA 2048 M02
*  SSL certificate verify ok.
> POST /v1/events HTTP/1.1
> Host: events.staging.example.com
> Accept: */*
> Accept-Encoding: deflate, gzip
> sec-ch-ua: "Google Chrome";v="111", "Not(A:Brand";v="8", "Chromium";v="111"
> sec-ch-ua-platform: "macOS"
> Referer: https://publisher.staging.example.com/
> sec-ch-ua-mobile: ?0
> User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/111.0.0.0 Safari/537.36
> Content-Type: application/msgpack
> Content-Length: 461
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 204 No Content
< Date: Tue, 28 Mar 2023 13:40:01 GMT
< Connection: keep-alive
< Vary: Origin
< Access-Control-Allow-Origin: 
< Access-Control-Allow-Credentials: true
< Cache-Control: no-store, no-cache, must-revalidate, post-check=0, pre-check=0
< Expires: Wed, 11 Nov 1998 11:11:11 GMT
< Last-Modified: Tue, 28 Mar 2023 13:40:01 GMT
< Pragma: no-cache

@Jamess-Lucass
Copy link
Contributor

@jney Are you able to provide the code in your project where you're defining the fiber cors config.

I recently ran into a cors issue while upgrading fiber and was a result of the issue #2338 with the PR @ReneWerner87 posted above.

You may be running into the same issue as you're using Access-Control-Allow-Credentials: true and I am assuming you were utilizing something similar to

cors.Config{
    AllowOrigins:     "*",
    AllowCredentials: true,
} 

where previously this would have worked due to how fiber were handling setting the response header for this.

If this does seem familiar then you may either specify the origins explicitly in the AllowOrigins property or I have opened a feature issue #2390 to see if implementation a func where we are able to perform some conditional to set the Access-Control-Allow-Origin to the origin.

Again, reiteration the above is speculation, like others have suggested if you're able to provide code/examples where someone is able to replicate the issue then you're going to receive answers.

@ReneWerner87
Copy link
Member

i was just using the config from the comment
#2389 (comment)

cors.Config{
    AllowOrigins:     "*",
    AllowCredentials: true,
} 

what should have led to the problems here ? would be good if someone shows me how the error looks like, without knowing what the observation of the error is, its very hard for me to help

@Jamess-Lucass
Copy link
Contributor

Jamess-Lucass commented Mar 28, 2023

i was just using the config from the comment #2389 (comment)

cors.Config{
    AllowOrigins:     "*",
    AllowCredentials: true,
} 

what should have led to the problems here ? would be good if someone shows me how the error looks like, without knowing what the observation of the error is, its very hard for me to help

I am 99% sure I know what @jney issues stems from.

Using the config posted here with fiber version v2.42.0 the response headers are: (note i'm sending a http request to this API from the browser running on http://localhost:2000)

Access-Control-Allow-Credentials: true
Access-Control-Allow-Origin: http://localhost:2000

Fiber version v2.43.0

Access-Control-Allow-Credentials: true
Access-Control-Allow-Origin: *

Which is what is causing these issues, it appears when no AllowOrigins is set in the configuration it was defaulting to kicking into this logic to set the response header to whatever the origin header is, but now sets it to *.

As you said in one of your comments it does appear to be as a result of #2339

@jney The fix is to just set the property AllowOrigins to the actual origins rather than leaving it blank or *

@jney
Copy link
Contributor Author

jney commented Mar 29, 2023

Maybe I am wrong, but this is wrong
As far as I remember when the AllowCredentials is set, Access-Control-Allow-Origin must be explicit, see: https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS/Errors/CORSNotSupportingCredentials

@ReneWerner87
Copy link
Member

@jney good found, i think you are right and we(@ryand67 ) made a mistake with the security change
will keep looking and if it turns out this is the problem, we should reverse the change and leave a comment with the articles on rfc or mdn so we don't incorrectly change it again

@jney
Copy link
Contributor Author

jney commented Mar 29, 2023

@ReneWerner87
I was about to edit my comment. I misread the security part @ryand67 was pointing out. So actually security probably can be issue. My bad
I also just saw @Jamess-Lucass PR #2390 which looks to me the way to go, allowing the feature, but forcing it to be explicit

@ReneWerner87
Copy link
Member

can we help otherwise ? otherwise I would close the issue

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

No branches or pull requests

4 participants