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

Webhook crashes when Cache-Control header contains more than the max-age field #527

Closed
trarbr opened this issue Oct 20, 2017 · 5 comments
Closed

Comments

@trarbr
Copy link
Contributor

trarbr commented Oct 20, 2017

Environment

  • VerneMQ Version: 1.1.1 and 1.2.0
  • OS: Debian Jessie
  • Erlang/OTP version (if building from source): N/A

Expected behavior

When VerneMQ invokes a webhook for auth_on_register, auth_on_subscribe and auth_on_publish, the result should be cached if the response contains the Cache-Control header. This should work for any common format of Cache-Control.

Actual behaviour

The webhook handler currently crashes for headers like Cache-Control: max-age=300, private and Cache-Control: max-age=300, public.

Output from error log:

2017-10-20 07:12:59.403 [error] <0.470.0> CRASH REPORT Process <0.470.0> with 0 neighbours crashed with reason: bad argument in call to erlang:binary_to_integer(<<"300, private">>) in vmq_webhooks_plugin:parse_headers/1 line 485

This is because the code to parse the header expects everything after max-age= to be an integer.

Steps to reproduce:

  • Set up a webserver responding with header Cache-Control: max-age=300, private
  • Enable webhook for auth_on_register, pointing to above server
  • Observe crashes in error.log
@larshesel
Copy link
Contributor

Hi - nice catch. Thanks for the detailed bug report - do you have a workaround you can use until we get time to look at this?

@trarbr
Copy link
Contributor Author

trarbr commented Oct 20, 2017

I'm hoping that I can use nginx to overwrite the cache-control header, but haven't had time to look into it yet.

@larshesel
Copy link
Contributor

The fix was reviewed and merged - it will be included in the 1.2.1 release.

@trarbr
Copy link
Contributor Author

trarbr commented Oct 20, 2017

Awesome, thank you to the VerneMQ team for moving fast 👍 Any idea when 1.2.1 might ship?

@larshesel
Copy link
Contributor

Not sure yet - probably in a couple of weeks or so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants