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

Access-Control-Allow-Methods should be GET,HEAD #1383

Merged
merged 1 commit into from May 10, 2018
Merged

Access-Control-Allow-Methods should be GET,HEAD #1383

merged 1 commit into from May 10, 2018

Conversation

@feross
Copy link
Member

feross commented May 10, 2018

Fixes: #1267

Fixes: #1267
@feross

This comment has been minimized.

Copy link
Member Author

feross commented May 10, 2018

@diracdeltas - does this look good to you?

@@ -111,7 +111,7 @@ function Server (torrent, opts) {
function serveOptionsRequest () {
res.statusCode = 204 // no content
res.setHeader('Access-Control-Max-Age', '600')
res.setHeader('Access-Control-Allow-Methods', 'GET,HEAD,PUT,PATCH,POST,DELETE')
res.setHeader('Access-Control-Allow-Methods', 'GET,HEAD')

This comment has been minimized.

Copy link
@diracdeltas

diracdeltas May 10, 2018

Contributor

lgtm - do you actually need HEAD though?

This comment has been minimized.

Copy link
@feross

feross May 10, 2018

Author Member

We do currently support HEAD, so it needs to stay in this header if we want it to keep working. You can see how we handle it here:

return res.end()

I don't think it hurts to keep it.

Copy link
Member

DiegoRBaquero left a comment

Makes sense to keep only the read methods.

@DiegoRBaquero DiegoRBaquero merged commit bc9dc8c into master May 10, 2018
6 checks passed
6 checks passed
Node Security No known vulnerabilities found
Details
WIP ready for review
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@DiegoRBaquero DiegoRBaquero deleted the fix-1267 branch May 10, 2018
@feross

This comment has been minimized.

Copy link
Member Author

feross commented May 10, 2018

Nice, thanks for the reviews :)

@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.