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

🚀 [Feature]: Cors AllowOrigin func #2390

Closed
3 tasks done
Jamess-Lucass opened this issue Mar 28, 2023 · 15 comments · Fixed by #2394
Closed
3 tasks done

🚀 [Feature]: Cors AllowOrigin func #2390

Jamess-Lucass opened this issue Mar 28, 2023 · 15 comments · Fixed by #2394

Comments

@Jamess-Lucass
Copy link
Contributor

Jamess-Lucass commented Mar 28, 2023

Feature Description

It's often helpful to set Access-Control-Allow-Origin response header value based on some conditions, for example, if the application is running in a test environment (specified by some arbitrary environment variable) then allow any port on localhost, you would be unable to use * as the value if you're also using Access-Control-Allow-Credentials: true.

In this scenario rather than having to specify every localhost with port origin you could potentially check whether the origin contains localhost which would return true and the implementation can then set the response header to the origin based on if this function returns true or false.

I was utilizing the fiber implementation of what * origin meant which was to not apply * but set whatever Origin header was set. This has been fixed as part of issue #2338.

So I am proposing a way that could be used to set the Access-Control-Allow-Origin header to whatever the origin is via a function. I am aware Gin has the AllowOriginFunc.

The implementation of using both AllowOrigins and this function could either be the function overwrites whatever is stored within AllowOrigins or it performs the origin checks on values specified in AllowOrigins first and the origin does not match any of these values then run the function.

Additional Context (optional)

If this feature gets accepted I am happy to work on it.

Code Snippet (optional)

package main

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

func main() {
  app := fiber.New()
  
  // Example 1 - check whether the origin is any scheme or port on localhost, this would result in any localhost app to access this app from the browser.
  app.Use(cors.New(cors.Config{
	AllowCredentials: true,
	AllowOriginsFunc: func(origin string) bool {
            return strings.Contains(origin, ":://localhost")
        }
  }))

  // Example 2 - check whether the environment variable is set to "development", this would result in any origins being able to access this app from the browser.
  app.Use(cors.New(cors.Config{
	AllowCredentials: true,
	AllowOriginsFunc: func(origin string) bool {
            return os.Getenv("ENVIRONMENT") == "development"
        }
  }))

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

In both examples the Access-Control-Allow-Origin response header would be set to whatever the Origin request header is.

Checklist:

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

welcome bot commented Mar 28, 2023

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

@ReneWerner87
Copy link
Member

ReneWerner87 commented Mar 28, 2023

https://docs.gofiber.io/api/middleware/skip

you can simply use the skip middleware for this or ?

package main

import (
	"log"
    "os"
    "strings"

	"github.com/gofiber/fiber/v2"
	"github.com/gofiber/fiber/v2/middleware/cors"
	"github.com/gofiber/fiber/v2/middleware/skip"
)

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

	// Example 1 - check whether the origin is any scheme or port on localhost
	app.Use(
		skip.New(
			cors.New(cors.Config{AllowCredentials: true}),
			func(ctx *fiber.Ctx) bool {
				return strings.Contains(ctx.Get(fiber.HeaderOrigin, ""), ":://localhost")
			},
		),
	)
    // Example 2 - check whether the environment variable is set to "development"
	app.Use(
		skip.New(
			cors.New(cors.Config{AllowCredentials: true}),
			func(ctx *fiber.Ctx) bool {
				return os.Getenv("ENVIRONMENT") == "development"
			},
		),
	)

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

@Jamess-Lucass
Copy link
Contributor Author

I didn't know about skip, apologies, I have implemented your examples and this results in the response header Access-Control-Allow-Origin: * which causes the error PreflightWildcardOriginNotAllowed because of the response header Access-Control-Allow-Credentials being set to true.

I would need the Access-Control-Allow-Origin response header set to whatever the Origin request header is in order to resolve this. I am not sure if this can be resolved with this skip feature, I have not looked at it

@ReneWerner87
Copy link
Member

Okay, I think I misinterpreted it (I must have been distracted).

you didn't want to skip the middleware for various cases, you wanted when this function is set and the allowedOrigin is "*" that the original origin from the request is used
good point
https://github.com/gin-contrib/cors/blob/c1983b27ecf753efe85d970c701af4eb51b46f12/README.md?plain=1#L51-L53

@ReneWerner87
Copy link
Member

https://expressjs.com/en/resources/middleware/cors.html#configuring-cors

image

@Jamess-Lucass
Copy link
Contributor Author

Okay, I think I misinterpreted it (I must have been distracted).

you didn't want to skip the middleware for various cases, you wanted when this function is set and the allowedOrigin is "*" that the original origin from the request is used good point https://github.com/gin-contrib/cors/blob/c1983b27ecf753efe85d970c701af4eb51b46f12/README.md?plain=1#L51-L53

Yeah, exactly, when true is returned from this arbitrary function, it will set the response allowed origin to whatever the origin is in the request header.

The characteristics between setting both the AllowOrigins property and this new function would be up for debate, whether one is ignored or there is a hierarchy where if one meets a condition the other one is not run.

Hopefully over the coming week I will have some spare time to put a proof-of-concept together and can take it from there?

@jub0bs
Copy link

jub0bs commented Apr 4, 2023

I have some thoughts about this... Basically, I believe AllowOriginsFunc and the likes of it to be dangerous misfeatures for CORS middleware libraries. Case in point:

AllowOriginsFunc: func(origin string) bool {
    return strings.Contains(origin, "://localhost")
}

would allow https://localhost.attacker.com, which is clearly not what you want.

@Jamess-Lucass
Copy link
Contributor Author

Jamess-Lucass commented Apr 4, 2023

Yeah, however, I would never use this feature in production and would always explicitly set cors origins, but when I have many services running locally on my machine or in a development environment not exposed to the internet I think people would benefit from not having to configure cors origins explicitly, especially if your environment has things which are ephemeral.

There's a fine line between developer experience and security, if it is perhaps documented that we discourage the use of AllowOriginsFunc in production that would shed light around it but as with many things if they're misconfigured, you run into security issues.

@jub0bs
Copy link

jub0bs commented Apr 4, 2023

@Jamess-Lucass Josh Bloch says it best :

A good API is not only easy to use but hard to misuse.

The problem is that this feature is easy to misuse/abuse. You may know better, but many Web developers (experienced ones even!) are clueless about CORS and will adopt whatever configuration "makes things work", such as the following (very insecure) configuration:

 app.Use(cors.New(cors.Config{
	AllowCredentials: true,
	AllowOriginsFunc: func(origin string) bool { return true },
  }))

That's the kind of users I have in mind.

Adding something like AllowOriginsFunc to Fiber would essentially bring the framework back to the situation that I decried in #2338. I'd still like to see a detailed/concrete use case of where this feature would actually be needed as opposed to simply expedient.

Ultimately, it's up to the Fiber community to decide, but I thought I'd mention what I've learned from studying CORS libraries out there.

@ReneWerner87
Copy link
Member

Screenshot_2023-04-04-18-28-40-08_40deb401b9ffe8e1df2f1cc5ba480b12.jpg

https://expressjs.com/en/resources/middleware/cors.html#configuring-cors

There are some examples on the express side (origin list is in the database)

@jub0bs
Copy link

jub0bs commented Apr 4, 2023

@Jamess-Lucass
Copy link
Contributor Author

@Jamess-Lucass Josh Bloch says it best :

A good API is not only easy to use but hard to misuse.

The problem is that this feature is easy to misuse/abuse. You may know better, but a lot of developers are clueless about CORS and will adopt whatever configuration "makes things work", such as the following (very insecure) configuration:

 app.Use(cors.New(cors.Config{
	AllowCredentials: true,
	AllowOriginsFunc: func(origin string) bool { return true },
  }))

That's the kind of users I have in mind. I'd still like to see a detailed/concrete use case of where this feature would actually be needed as opposed to simply expedient. Ultimately, it's up to the Fiber community to decide, but I thought I'd mention what I've learned from studying CORS libraries out there.

Yeah I understand your concern about how easy it could be to enable a security risk in a production environment. A layer of protection on top of this could be fiber requiring an environment variable to be set, which would enable the use of AllowOriginsFunc

@ReneWerner87
Copy link
Member

ReneWerner87 commented Apr 4, 2023

Ask chatgpt for more usage examples:


Origin validation based on user permissions. Let's say you have an API that allows different levels of access based on user roles. In this case, you may need to perform dynamic origin validation based on the user's role, which would be difficult to do with a static list of allowed origins.

Origin validation based on client authorization. If your API is used by third-party clients, you may need to perform dynamic origin validation based on the client's authorization level or API usage limits.

Origin validation based on geographic location. If you want to restrict API access based on geographic location, you may need to use a function that determines the origin's geographic location and allows or rejects the request accordingly.

Origin validation based on DNS lookup. If you want to restrict API access to specific domains, you may need to use a function that performs a DNS lookup to validate the domain name.

Origin validation based on user-agent. If you want to restrict API access to specific user-agents (e.g., web browsers or mobile apps), you may need to use a function that validates the user-agent string in the request header.

Origin validation based on time of day. If you want to restrict API access to specific times of day (e.g., during business hours), you may need to use a function that checks the current time and allows or rejects the request accordingly.

Origin validation based on IP address. If you want to restrict API access based on IP address, you may need to use a function that checks the origin's IP address against a whitelist or blacklist.

Origin validation based on external factors. If you need to perform origin validation based on external factors (e.g., weather conditions or stock market fluctuations), you may need to use a function that retrieves data from an external API or service to make the validation decision.


I know what you mean, but any configuration can be used incorrectly, we can not prevent it.
We can only point out that the person should know that his configuration may disable security mechanisms that are not intended as such.

@jub0bs
Copy link

jub0bs commented Apr 4, 2023

@ReneWerner87 I'm a bit puzzled as to why ChatGPT is invoked as a reliable arbiter... In my experience, it's more often wrong than right about security-related topics, including CORS.

To be perfectly honest, none of those ChatGPT examples are good. In particular, because preflight requests do not carry any information of that kind (roles, authorisation, geographic location, time of day, etc.), basing origin validation on it is doomed to failure. Besides, basing origin validation on DNS resolution (of the domain in the request's origin, I presume?) would open the door to cache-poisoning issues.

any configuration can be used incorrectly, we can not prevent it.

I disagree, but that's the last message I'll write on this topic. It's up to you and the rest of the Fiber community to decide what's best.

@ReneWerner87
Copy link
Member

Ok, thanks for your opinion, will discuss this again with the other contributors and we will decide then

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.

3 participants