Skip to content

Conversation

@MarkoMin
Copy link
Contributor

@MarkoMin MarkoMin commented Oct 9, 2025

Integer stays as a possible config value so that this stays backward compatible.
IMO there are two possibilities to add this feature:

  1. #{<<"authorization">>=>8192, '_' => 4096} - _ atom is inconvenient to specify a default, but can bi placed in a config file
  2. fun(<<"authorization">>=>8192; (_) -> 4096 end - match-all is convenient, but can't be placed in a config file (directly!). If one want to change this in runtime (which I think is pretty rare) it must recompile the code or fetch each value as a separate env value.

I tried to test this but it's tricky to get it without breaking current test patterns - i.e. without using init_per_testcase/1 which is not used in this codebase. rfc7230_SUITE starts all groups with the same option and what I'd what is to test for 431 status code when max_header_value_length is set differently.

Closes #1677

@essen
Copy link
Member

essen commented Oct 9, 2025

I think we can just have an option for authorization and cookie headers like max_authorization_header_value_length and max_cookie_header_value_length that override the max_header_value_length limit. I don't think there are other request headers that grow that large.

@MarkoMin MarkoMin force-pushed the feat/max_header_value_length branch from fabab35 to 16bda52 Compare October 10, 2025 22:10
@MarkoMin
Copy link
Contributor Author

I think we can just have an option for authorization and cookie headers like max_authorization_header_value_length and max_cookie_header_value_length that override the max_header_value_length limit. I don't think there are other request headers that grow that large.

Fair, just pushed the implementation with those opts. Are defaults okay?

@essen
Copy link
Member

essen commented Oct 13, 2025

Thank you. Yes keeping the same default for all headers is likely the least surprising. I will work on merging this soon.

@essen
Copy link
Member

essen commented Oct 13, 2025

I'll take this opportunity to add a test for max_header_value_length as it seems to be missing (only default value is tested).

@essen
Copy link
Member

essen commented Oct 14, 2025

Merged, thanks!

@essen essen closed this Oct 14, 2025
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.

Separate max_header_value_len for specific headers

2 participants