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

Add support for OPTIONS request to HttpTransport #3234

Merged
merged 4 commits into from Jan 2, 2017
Merged

Add support for OPTIONS request to HttpTransport #3234

merged 4 commits into from Jan 2, 2017

Conversation

@joschi
Copy link
Contributor

@joschi joschi commented Dec 22, 2016

Description

The HttpTransport handler (used by the GELF HTTP input) currently doesn't support HTTP OPTIONS requests which are required for proper CORS handling (preflight requests).

This PR adds support for the OPTIONS HTTP request method.

Fixes #3232

Motivation and Context

If web applications want to log messages to Graylog, the user's web browser will send preflight requests to the GELF HTTP input. If these preflight requests fail, the web browser will refuse to send anything to that URI.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code follows the code style of this project.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
Jochen Schalanda
@joschi joschi added this to the 2.2.0 milestone Dec 22, 2016
@bernd bernd self-assigned this Dec 23, 2016
Copy link
Member

@bernd bernd left a comment

While this fixes the 405 response, I am getting the following error when testing it from the browser with a correct Content-Type header.

XMLHttpRequest cannot load http://devd.io:12202/gelf. Request header field Content-Type is not allowed by Access-Control-Allow-Headers in preflight response.

It works when I add the Content-Type header to the allowed headers in HttpTransport.

If the allowed headers setting is mandatory for CORS, we should probably make it configurable for the user.

This is my test script: https://gist.github.com/bernd/a302402349bf8daaf88a98d5c1b5f5d1

Jochen Schalanda added 2 commits Jan 2, 2017
Jochen Schalanda
Jochen Schalanda
@joschi
Copy link
Contributor Author

@joschi joschi commented Jan 2, 2017

If the allowed headers setting is mandatory for CORS, we should probably make it configurable for the user.

I disagree here. We should allow the absolute minimum of required headers (which seem to be Authorization and Content-Type) and not more – at least in the context of this PR.

We can still make the list of allowed headers configurable in a specific feature request.

Jochen Schalanda
@bernd
bernd approved these changes Jan 2, 2017
@bernd bernd merged commit 8784f93 into master Jan 2, 2017
4 checks passed
4 checks passed
ci-web-linter Jenkins build graylog-pr-linter-check 1223 has succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@bernd bernd deleted the http-input-cors branch Jan 2, 2017
@bernd bernd removed the ready-for-review label Jan 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

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