feat(http): add flag/config to allow CORS requests, close #229 #293
Conversation
you should be able to write a functional test as you would in Go but I believe we don't have a test case available to get you inspired |
|
@Kocal did you get a change to have a look for a test? |
|
@tucksaun not at all, I can try to take a look in the next days |
|
@Kocal I guess we are good, it's not worst than the actual situation :) |
local/http/http.go
Outdated
| @@ -212,6 +213,10 @@ func (s *Server) ProxyHandler(w http.ResponseWriter, r *http.Request) { | |||
|
|
|||
| // Handler handles HTTP requests | |||
| func (s *Server) Handler(w http.ResponseWriter, r *http.Request) { | |||
| if s.AllowCORS { | |||
| w.Header().Add("Access-Control-Allow-Origin", "*") | |||
There was a problem hiding this comment.
I'm wondering if we should not add default CORS values (see what Gorillw used to have: https://github.com/gorilla/handlers/blob/546854cf1d61a016260b224e2526a29ff5b5c5ae/cors.go#L28-L32)
There was a problem hiding this comment.
We should also be careful not to override header sent by the upstream app
There was a problem hiding this comment.
Is it fine like this? I don't really know Go :D
if s.AllowCORS {
if w.Header().Get("Access-Control-Allow-Origin") != "" {
w.Header().Add("Access-Control-Allow-Origin", "*")
}
}There was a problem hiding this comment.
no this would not work because at this stage the header will never be already defined because the upstream server has not been contacted yet (contacting the upstream server is done in s.Callback)
|
I use What is the use case for this feature? For all application request I'd rather let the application handle the headers. This would be useful only for static files? Though we normally don't load static files over XHR/fetch. I think this should only be added to headers if symfony-cli is serving static file. If servering a request via php, let php handle the headers. |
|
Serving static files is one use case indeed. But you can also imagine running a JS development server alongside the CLI and both can be on different domain/port and this causes issues. But only in dev, so you might not want to (or not allowed to) introduce nelmio cors. |
|
I just had a need for this as I am serving a static json file from public folder and its not handled by |
4baa1ca to
7ccbb9b
Compare
|
PR rebased and conflicts fixed, I will see to add a test if I can |
|
I think I will need some help to finalize this PR (fix the "add header" logic and tests). |
If you happen to be at the Forum PHP in 15 days we can catch-up there and have a look :) |
@tucksaun I will! :D |
commands/local_server_start.go
Outdated
| @@ -52,6 +52,7 @@ import ( | |||
|
|
|||
| var localWebServerProdWarningMsg = "The local web server is optimized for local development and MUST never be used in a production setup." | |||
| var localWebServerTlsKeyLogWarningMsg = "Logging TLS master key is enabled. It means TLS connections between the client and this server will be INSECURE. This is NOT recommended unless you are debugging the connections." | |||
| var localWebServerAllowCorsLogWarningMsg = "Cross-origin resource sharing (CORS) is enabled for every requests.\nYou may want to use https://github.com/nelmio/NelmioCorsBundle to have more control on HTTP headers." | |||
There was a problem hiding this comment.
Maybe add another sentence informing user that if they rely on this behavior to make their application work, it won't work in production server oob.
|
@Kocal after a week thinking about our peer-programing session, I stand by what I was saying before leaving the Forum PHP: I believe we can accept your PR where the CLI prepends the headers and ignore the case where the application also add some headers. If this happens then I think this is the user’s responsibility to decide between disabling CORS headers addition in their app or in the CLI. what we might want to do, just to be clear for the user, is add a warning if we detect such scenario but that’s it. |
|
Hey @tucksaun, sorry for the late reply, I've been quite busy those last weeks :) That's fine to me, a warning should be enough, I'll check it out as soon as I can! PS: it was a real pleasure to meet you at the ForumPHP :D |
|
Nice! Let me know if you need some help to finalize it. PS: shared pleasure :) |
|
Please, Merge this and create a release! This is the Feature i am waiting for :) |
|
Ah! My bad, I didn't find the time to complete this PR, I just needed to commit and push :) It's should be ok now, cc @tucksaun :) |
bb8a8b0 to
c08556b
Compare
c08556b to
ac66e13
Compare
|
We dit it 🙌🏻 |
|
Thank you @Kocal for keeping up the effort for a year and half ! 🥳 |
|
Thank you for our pair-programming sessions and your patience 🙏🏻 |
Hi everyone, this PR is a proposal for #229.
A new flag
--allow-corshas been added toserver:startcommand, and to confighttp.allow_corsin.symfony.local.yaml.When allowing CORS, the follows headers are added to each response:
Access-Control-Allow-Origin: *Access-Control-Allow-Methods: *Access-Control-Allow-Header: *Is there a way to add automated tests for that?
Thanks!
#SymfonyHackday