Skip to content

fix: override max_header_value_length using environment variable#762

Merged
filipecabaco merged 4 commits into
mainfrom
unknown repository
Dec 26, 2023
Merged

fix: override max_header_value_length using environment variable#762
filipecabaco merged 4 commits into
mainfrom
unknown repository

Conversation

@ghost

@ghost ghost commented Dec 19, 2023

Copy link
Copy Markdown

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
@vercel

vercel Bot commented Dec 19, 2023

Copy link
Copy Markdown

@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

ghost commented Dec 19, 2023

Copy link
Copy Markdown
Author

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

ghost commented Dec 19, 2023

Copy link
Copy Markdown
Author

@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
Copy Markdown
Member

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

@ghost

ghost commented Dec 26, 2023

Copy link
Copy Markdown
Author

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
Copy Markdown
Member

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
Copy Markdown
Member

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

ghost commented Dec 26, 2023

Copy link
Copy Markdown
Author

@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
Copy Markdown
Member

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
@ghost

ghost commented Dec 26, 2023

Copy link
Copy Markdown
Author

Appreciate the help, thanks @filipecabaco !

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

Copy link
Copy Markdown
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.

2 participants