Add CORS headers to microservice #17
Conversation
Current coverage is 93.24% (diff: 100%)@@ master #17 diff @@
==========================================
Files 4 4
Lines 73 74 +1
Methods 13 13
Messages 0 0
Branches 14 14
==========================================
+ Hits 68 69 +1
Misses 5 5
Partials 0 0
|
|
Maybe this could be configurable? Would be nice to set a list of the domains it is used from. |
|
Why would somebody restrict the domains this can be used from, what's the use case? Thanks for the PR @juandjara, this is a great addition! |
|
Filtering out local calls from development of a website using this, but this might not be the best way to do it 😛 |
|
See #11, I think this should be fine? |
| @@ -6,6 +6,8 @@ const { pushView } = require('./utils') | |||
|
|
|||
| module.exports = async function (req, res) { | |||
| const { pathname, query } = url.parse(req.url, /* parseQueryString */ true) | |||
| res.setHeader('Access-Control-Allow-Origin', '*') | |||
| res.setHeader("Access-Control-Allow-Headers", "Origin, X-Requested-With, Content-Type, Accept") | |||
There was a problem hiding this comment.
If I understand this right (I might have this totally wrong though, not an expert so correct me), the Access-Control-Allow-Headers header tells the client which headers it can send and the server will respect, right? But we don't respect Content-Type nor Accept at all, we always just send JSON?
There was a problem hiding this comment.
Yeah, I had not thought of that. I just copied this from another project, I tried it and it worked. I know at least the Origin header is necessary, not sure about X-Requested-With
There was a problem hiding this comment.
I don't think we need X-Requested-With, at least according to this SO answer? https://stackoverflow.com/questions/17478731/whats-the-point-of-the-x-requested-with-header#22533680
Am I reading that right?
There was a problem hiding this comment.
That seems right. I wil check it later today and update this PR
There was a problem hiding this comment.
Thanks for digging into this, appreciated! Definitely want to get this right 😊
There was a problem hiding this comment.
Ok so I tested and found that no headers are actually necessary so I'm reverting last commit.
This reverts commit 04f5916.
|
LGTM, thanks so much! |
No description provided.