Allow a cors custom validation function which receives the full gin context #140
Conversation
dbhoot
changed the title
|
@dbhoot As proposed, this feature is dangerous. Since users of |
|
Related: #67 |
|
@jub0bs If I understand the poisoning issue, #60 was problematic because it didn't have access to the response. Without access to the response there would be no way way to set the Vary header. With the full context, you are able to do so, but I think it's quite bad to do that in the AllowOriginWithContext function. A correctly written allow origin function shouldn't have side effects -- you could make it work but it would be kinda ugly. Currently, the package only sets the Vary header with origin when you're allowing all origins. Perhaps, if you have an allow origin function with context, you have to also specify a custom function on the config which will set the Vary header. I'm happy to add that and update the tests, readme, etc. I do think it makes good sense to allow an origin validation with context so I'm open to suggestions on how to add the capability safely. |
That's true. But would average users think of adequately populating the I'm curious about your use case for AllowOriginWithContextFunc: func(c *gin.Context, origin string) bool {
return origin == "http://sample.com"
},
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #140 +/- ##
==========================================
- Coverage 95.11% 94.87% -0.24%
==========================================
Files 3 3
Lines 225 195 -30
==========================================
- Hits 214 185 -29
+ Misses 11 10 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
I want the path. Allowing
In my example, I initially tried setting a separate cors middleware at each router group level. However, the pre-flight requests fail as I mentioned in the description. You're right that a developer would probably not know to populate the I do think my suggestion of a custom function on the configuration to set it would solve the problem. I can add that if you think it reasonably solves the problem. As I mentioned before, I'm also open to other ideas. My particular use does not require it but it does seem necessary in certain configurations. Regarding footguns: my take is that it is important to provide a way to do necessary things while educating about the risks and providing examples rather than being very restrictive. Consider the http standard already allows headers to be in the cache key for a cors response. That means there are already legit use cases in the wild which are fully supported in the http standard which are impossible to configure using this package. Adding the suggested function on the config would address that concern as well. From another perspective: does it make sense to completely block valid and supported uses entirely because a developer may make a mistake or is it better to support it and show people how to do it correctly? |
I think I understand the problem, but bear in mind that it stems from the design of Gin's router. There's no such issue with
That's what I'm worried about. Cache poisoning can have serious implications, yet most people are not aware of them.
If you follow this rationale to the end, you might as well let people implement their own CORS middleware. In my opinion, a good library should be easy to use but also (!) hard to misuse. But the final decision about this PR doesn't rest with me, obviously. |
|
@jub0bs -- I agree -- changing the router could also work. That could also have risks though since there may be people who depend on the way it currently works. Figured this would be the safer and quicker win.
@appleboy I see I'm triggering linting with lines longer than 80 characters. I'm happy to shorten those but I notice that there are many existing lines which violate the linting rule. Are you expecting to address everything in the entire module? |
|
@dbhoot I found that only this PR violates this rule, so it can be fixed directly. |
Ok, that's good! I just pushed an update to the error string. Hopefully that's the last linting error. |
|
bump to v1.7.0 https://github.com/gin-contrib/cors/releases/tag/v1.7.0 |
|
@appleboy thank you for the review and assist on the linting. 🙏 |
|
Another reason why I'm opposed to this new callback feature (aside from the fact that it's a footgun) is that, if it's justified by the need to inspect the request's URL/path, then you have poor separation of concerns: the path is indeed processed both by the router and by the CORS middleware. |
Problem
You cannot add separate cors config / middleware at the router group level. While adding the middleware works just fine, when a browser executes the pre-flight request, gin will return a 404.
This is because gin checks the existence of a route before running any middlewares on that router group. However, middlewares at the engine level always run. This means you have to apply the cors middle at the global/engine level and cannot have separate configs for different sections of your backend.
The Cors config allows a custom origin validation function. However, that function only receives the origin header.
Example
api.example.com/app1 only allows origins app1.example.com
api.example.com/app2 only allows origins from app2.example.com
api.example.com/v1 allows all origins
This would not be possible in the current cors configuration options.
Solution
Allow the config to specify an origin validation function which also receives the entire gin context.