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

fix: override max_header_value_length using environment variable #762

Merged
merged 4 commits into from Dec 26, 2023
Merged

fix: override max_header_value_length using environment variable #762

merged 4 commits into from Dec 26, 2023

Conversation

ghost
Copy link

@ghost ghost commented Dec 19, 2023

What kind of change does this PR introduce?

Bug fix for #761:

  • introduces a new environment variable MAX_HEADER_LENGTH that can be used to override config -> http -> protocol_options -> max_header_value_length from its default value in Elixr (4096)
  • If no value is set, the Elixr default of 4096 will continue to be used
  • updates README to reflect these changes

What is the current behavior?

With SSR and the use of cookies to authenticate on the server, all cookies need to be sent to Supabase requests, including Realtime. If these cookies exceed the (rather low) default of 4096 (which, with an auth provider for supabase, can happen extremely easily), realtime subscriptions fail with a 431 error.

Edit: we are using Keycloak as our Supabase authentication provider, and it looks like Keycloak is a bit notorious for large cookies (see #761 (comment)). Hence the further need to increase the limit, but in a dynamic fashion (for users who wish to retain the security benefits of smaller limits and don't use Keycloak or other services with larger cookies).

What is the new behavior?

With the new behavior, a user could specify a MAX_HEADER_LENGTH environment variable to a higher value (such as 8192, doubling the limit) to cease the 431 errors.

Additional context

See #761

Edit: I test-built with these changes locally and it 100% fixed the issue.

Nicholas Barrow added 2 commits December 18, 2023 18:59
Solves #761 by introducing an environment variable to override config -> http -> protocol_options -> max_header_value_length or leaves it at the default 4096 if no other value is specified
Copy link

vercel bot commented Dec 19, 2023

@nbarrow-inspire-labs is attempting to deploy a commit to the Supabase Team on Vercel.

A member of the Team first needs to authorize it.

@ghost ghost mentioned this pull request Dec 19, 2023
2 tasks
@ghost ghost changed the title Patch 1 fix: override max_header_value_length using environment variable Dec 19, 2023
@ghost
Copy link
Author

ghost commented Dec 19, 2023

I did test-build this locally and it 100% fixed the issue with MAX_HEADER_LENGTH: 8192 in docker compose.

Edit: you should not need to set these Kong variables (for docker compose with kong image, these variables are for Kong not realtime), but posting in case you run into errors (I was able to get everything working without them):

KONG_NGINX_PROXY_CLIENT_HEADER_BUFFER_SIZE: 160k
KONG_NGINX_PROXY_LARGE_CLIENT_HEADER_BUFFERS: 64 160k
KONG_NGINX_HTTP_CLIENT_HEADER_BUFFER_SIZE: 160k
KONG_NGINX_HTTP_LARGE_CLIENT_HEADER_BUFFERS: 64 160k

@ghost
Copy link
Author

ghost commented Dec 19, 2023

@filipecabaco when you have a chance, can you take a look at this? It seems partially related to #746 which you helped me with a week or so ago. This should fully solve the rest of the problem (#761).

@filipecabaco
Copy link
Contributor

Hey! just a FYI that I will take a look later today 👁️

@ghost
Copy link
Author

ghost commented Dec 26, 2023

Just wanted to see if anyone's had the chance to take a look at this yet? It should be a pretty quick review (hopefully!)

@filipecabaco
Copy link
Contributor

not yet sorry... 😓 tricky week. there's one change needed in the mix.exs file to bump the minor version to (2.25.50) and will check the remainder of the code now

@filipecabaco
Copy link
Contributor

yep looks good, just needs that change in the mix.exs to 2.25.50 since I'm going to merge 2.25.49 now

@ghost
Copy link
Author

ghost commented Dec 26, 2023

@filipecabaco awesome (and no worries on the delay!) thanks for checking. I just bumped the version in mix.esx so it should be good to go after your pull.

@filipecabaco
Copy link
Contributor

going to rebase and merge to fix the conflict. thank you for the help 🙏

@filipecabaco filipecabaco merged commit 97b214e into supabase:main Dec 26, 2023
2 of 3 checks passed
@ghost
Copy link
Author

ghost commented Dec 26, 2023

Appreciate the help, thanks @filipecabaco !

@ghost ghost deleted the patch-1 branch December 26, 2023 16:53
@kiwicopple
Copy link
Member

🎉 This PR is included in version 2.25.50 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

None yet

2 participants