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

Expose max_header_value_length for http listeners #2272

Closed

Conversation

DomasM
Copy link

@DomasM DomasM commented Mar 19, 2024

Proposed Changes

Fix for #2267 - expose max_header_value_length, so that websocket connection can be established from frontend applications with bigger cookies.

Types of Changes

What types of changes does your code introduce to this project?
Put an x in the boxes that apply

Checklist

Put an x in the boxes that apply. You can also fill these out after creating
the PR. If you're unsure about any of them, don't hesitate to ask on the
mailing list. We're here to help! This is simply a reminder of what we are
going to look for before merging your code.

  • I have read the CODE_OF_CONDUCT.md document
  • All tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if needed)
  • Any dependent changes have been merged and published in related repositories
  • I have updated changelog (At the bottom of the release version)
  • I have squashed all my commits into one before merging

Further Comments

I have never done any work in Erlang, did not set up local dev env, changes are done by mirroring #2161
I will create another PR in docs repo to save other devs some time next time.

changelog.md Outdated Show resolved Hide resolved
@DomasM DomasM force-pushed the enhancement_max_header_value_length branch from 29fbf12 to a6218a4 Compare March 19, 2024 17:40
@DomasM DomasM force-pushed the enhancement_max_header_value_length branch from a6218a4 to bb41e90 Compare March 20, 2024 13:00
@DomasM DomasM requested a review from ioolkos March 20, 2024 15:28
@mths1
Copy link
Contributor

mths1 commented Mar 22, 2024

I'd recommend that you ignore all the http and https for now, and focus on ws and wss. You'd need to add your new setting to that part of the schema, as well as in vmq_schema.erl. Once you got that working, you can consider to introduce the setting also for http and https (and yes, it means code duplication)

Increase default value of `max_header_value_length` and `max_request_line_length` for Websocket listeners to 16384 bytes
@DomasM DomasM force-pushed the enhancement_max_header_value_length branch from bb41e90 to b23ba27 Compare April 4, 2024 14:03
@DomasM
Copy link
Author

DomasM commented Apr 4, 2024

As simple as it seems, but I was unable to pass settings from vernemq.conf to vmq_ranch_config.erl line 366 as values don't appear in Opts there. Looking at the other settings, I can't figure out what's the difference. I won't be able to allocate more time to this issue in the foreseeable weeks, so my suggestion is to increase default values and set them explicitly in verne code instead of depending on ranch defaults:

max_header_value_length  4096 -> 16384
max_request_line_length  8000 -> 16384

@ioolkos
Copy link
Contributor

ioolkos commented Apr 5, 2024

@DomasM I'll look into it. Thank you for your effort so far!

@ioolkos
Copy link
Contributor

ioolkos commented May 19, 2024

Closing in favour of #2289. Thanks again for kicking this off, @DomasM.

@ioolkos ioolkos closed this May 19, 2024
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 this pull request may close these issues.

None yet

3 participants