Fix empty/incorrect CORS headers #1669
Conversation
- Fix empty Access-Control-Allow-Origin - Set CORS headers only if request Origin is existing and allowed - Increase middleware test coverage
ulasakdeniz
force-pushed
the
fix-incorrect-cors-headers
branch
from
501fde9
to
871ed9c
Compare
Codecov Report
@@ Coverage Diff @@
## master #1669 +/- ##
==========================================
+ Coverage 84.06% 84.33% +0.27%
==========================================
Files 28 28
Lines 1901 1909 +8
==========================================
+ Hits 1598 1610 +12
+ Misses 191 189 -2
+ Partials 112 110 -2
Continue to review full report at Codecov.
|
|
Hi @lammel, can you have a look at this PR? It's a very small one that solves an issue I linked in the description. |
| @@ -102,6 +102,17 @@ func CORSWithConfig(config CORSConfig) echo.MiddlewareFunc { | |||
| origin := req.Header.Get(echo.HeaderOrigin) | |||
| allowOrigin := "" | |||
|
|
|||
| preflight := req.Method == http.MethodOptions | |||
There was a problem hiding this comment.
Shouldn't we add OPTIONS also to DefaultCORSConfig.AllowedMethods?
There was a problem hiding this comment.
I don't think it is needed. Because OPTIONS is used for preflight requests whereas AllowedMethods defines allowed methods for simple requests.
Note that other frameworks I tested (play and gin) do not provide any Access-Control-Allow-Methods headers by default.
Thank you for the review. As you said it only checks for Origin, so I don't expect any breaking change. |
|
Hi @lammel, I replied your comments. Can we merge if you don't want any changes? |
|
All questions answered. |
Hi, this PR attempts to fix #1668.