-
Notifications
You must be signed in to change notification settings - Fork 375
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 Broken Email Links Behind Proxy #1571
base: master
Are you sure you want to change the base?
Conversation
Add `API_FORCE_EXTERNAL_URL` to fix login links
Added `API_FORCE_EXTERNAL_URL` to force the auth client to use the external base url when constructing URLs.
hello👋? Anyone.... |
@hf Do you know why this PR hasn't been reviewed / merged yet? I believe this makes the changes as you proposed in the open issue by adding a new config variable. What can be done to to complete this? |
@kangmingtay not sure who to ping on this. Are you perhaps able to assist on this PR? Any input / review would be great. I believe the solution here is what hf proposed on the original issue: #1228 (comment) |
Any news on this? |
Re: all interested, I'll continue to merge and keep up to date my feature repo based on main until it comes upstream to this repo. Update: CI in my repo is running and follows the releases here to https://hub.docker.com/repository/docker/nickmitchko/gotrue/tags If you need an SBOM, please build your own image. |
chore(master): release 2.153.0
chore(master): release 2.153.1
Merge Down
Hi, anyone from the supabase team here? is it there a reason why this important fix is not merged? Thanks |
@nickmitchko thanks for contributing to fix this issue - the team is quite tight on bandwidth right now so we'll need some time to review this at first glance, i don't think we want to introduce a new config this way just to override the issue with the domain being set incorrectly by an external proxy service like kong my initial thoughts on this would be to expose a config option specifically for the external url to be used in emails - similar to what we did recently for the SAML metadata (#1599) |
@kangmingtay Not the OP of this PR, but do want to point out that this PR aims to fix Supabase Auth's broken behavior introduced in version 2.67.1. When following Supabase's self-hosting documentation and code, the end result is simply broken, since version 2.67.1. As far as everyone in this issue (myself included) understands it, it's Supabase's own self-hosting setup / code that is broken, as that is what is being followed and used. If we're all mistaken about that, please spare a couple of minutes to let all of us in that issue know (or in the documentation) what you mean exactly by domains being set incorrectly* and/or how we should be reading the Supabase documentation / code / env files. Clearly, there are a lot of people running into this issue, and have been for 10 months. * Keeping in mind that in many setups, services should not use external URLs for other services |
@kangmingtay only http://domain.com is used in the email, the route is trimmed. This essentially prevents me from using emails for auth handling. i.e. the core functionality of gotrue is essentially broken |
@kangmingtay Thanks Edit: For anyone else struggling with this and that don't want to build an outdated image: Edit2: |
@y4my4my4m Thank you bro, this issue beat me so hard. Changing the |
chore(master): release 2.154.0
the kong issue still exists but turns out there are many hidden URLs that aren't shown on the UI and can be accessed by directly typing similar to the hosted cloud idk why they would do something like this not good |
MAILER_URLPATHS_CONFIRMATION="${API_EXTERNAL_URL}/auth/v1/verify" this works |
What kind of change does this PR introduce?
Fixes #1228
What is the current behavior?
supabase/auth behind a proxy with incorrect or internal-external X-Forwarded headers cause incorrect email account links
What is the new behavior?
API_FORCE_EXTERNAL_URL=[true | false]
to force theAPI_EXTERNAL_URL
to remain unchained. This new variable is turned off by default. Users reliant on the current (slightly broken) behavior have no changes.Additional context
Total lines changed: 3