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

Dashboard should trim spaces between "redirect" links #3191

Closed
richiemcilroy opened this issue Sep 13, 2021 · 23 comments
Closed

Dashboard should trim spaces between "redirect" links #3191

richiemcilroy opened this issue Sep 13, 2021 · 23 comments
Labels
bug Something isn't working frontend Related to supabase dashboard help wanted Extra attention is needed

Comments

@richiemcilroy
Copy link
Contributor

Bug report

redirectTo parameter doesn't seem to work for redirects.

Describe the bug

The redirectTo parameter doesn't seem to work for redirects when using supabase.auth.signIn or supabase.auth.signUp (@supabase/supabase-js), and I can't find any reference to it, apart from in the docs on the website: https://supabase.io/docs/reference/javascript/auth-signin

To Reproduce

Using supabase.auth.signIn or supabase.auth.signUp with the redirectTo parameter (@supabase/supabase-js), as referenced in the docs URL above.

Expected behavior

As per the docs, it should redirect you to a URL (if the base domain is the same as in auth settings)

System information

  • OS: macOS
  • Browser: Chrome
  • @supabase/supabase-js: 1.3.2
@richiemcilroy richiemcilroy added the bug Something isn't working label Sep 13, 2021
@Facundojs
Copy link

Hey could yoy fix this? Im now with same problem

@chipilov
Copy link

chipilov commented Sep 30, 2021

I see the same issue - the redirectTo param is NOT respected regardless of what I have in the "Site URL" or "Additional Redirect URLs" in the dashboard.

@awalias @kiwicopple It seems that the redirectTo param is only appended as a query param to the GoTrue auth requests, but the GoTrue implementation does NOT use this param at all.

If you need help with fixing this, can you give us some pointers about how to go about fixing it (e.g. what is the expected flow)?

@awalias
Copy link
Member

awalias commented Sep 30, 2021

hey guys thanks for the report!

can you share what values you have in the "Site URL" and "Additional Redirect URLs" in the dashboard, and which url you're using in the client

@chipilov
Copy link

chipilov commented Sep 30, 2021

Sure, I am trying to redirect to '/profile' when the user logs in. I tried the following:

redirectTo: '/profile'
Site URL: 'http://localhost:3000'
Additional Redirect URLs: 'http://localhost:3000'

redirectTo: '/profile'
Site URL: 'http://localhost:3000'
Additional Redirect URLs: 'http://localhost:3000/profile'

redirectTo: 'http://localhost:3000/profile'
Site URL: 'http://localhost:3000'
Additional Redirect URLs: 'http://localhost:3000/profile'

Do you think it's a config issue?

Even if it is, I am a bit puzzled because as @richiemcilroy mentioned, I don't see any uses of the redirect_to query param in the GoTrue server and in the JS clients, it seems like the redirectTo param is only used to populate the redirect_to query param.

@pmauchle
Copy link

pmauchle commented Oct 1, 2021

This is my config which works:

image

You have to provide the full path in redirectTo. I set the redirectTo with .env variables so it works both locally and live.

@chipilov
Copy link

chipilov commented Oct 1, 2021

Hi @pmauchle,

I did try to provide the full path in the redirectTo param (as specified in the third example above). The string I pass to the redirectTo parameter is "http://localhost:3000/profile"

I am using @supabase/supabase-js v1.22.6.

Here is what I have in the dashboard:

urls

It's possible that I am doing something wrong but I am not sure where to look because I don't see any places in the code where ther redirect is actually used. I see that the redirectTo is used to add the redirect_to query param i the request to the GoTrue server:

https://github.com/supabase/gotrue-js/blob/4d57cb5db2c833bb08dcb72e1ce025615624efdd/src/GoTrueApi.ts#L89

But I don't see this query param used by the GoTrue server in the Token API:

https://github.com/supabase/gotrue/blob/70ffa779d7f1a58e5463a513289597f3abc9d8e6/api/token.go#L52

Am I missing something?

@pmauchle
Copy link

pmauchle commented Oct 1, 2021

Hey @chipilov I also use v1.22.6 and it works with your site URL and additional URLs in my project, I just tested.

Can you share the part where you do the redirect in your code?

@chipilov
Copy link

chipilov commented Oct 1, 2021

I am literally using the Basic Auth example from supabase/ui: (https://ui.supabase.io/components/auth) and only passing the redirectTo prop to the Auth component, so it looks like this:

const Container = (props: any) => {
  const { user } = Auth.useUser()
  if (user)
    return (
      <>
        <Typography.Text>Signed in: {user.email}</Typography.Text>
        <Button block onClick={() => props.supabaseClient.auth.signOut()}>
          Sign out
        </Button>
      </>
    )
  return props.children
}

function App() {
  return (
    <Auth.UserContextProvider supabaseClient={supabase}>
      <Container supabaseClient={supabase}>
        <Auth supabaseClient={supabase} redirectTo='http://localhost:3000/profile' />
      </Container>
    </Auth.UserContextProvider>
  )
}

export default App;

In the network tab of the dev tools I see that the redirect_to query param is properly added but as soon as I sign in sucessfully, the page remains on the root path (i.e. it is NOT redirected to /profile).

Maybe I misunderstand the meaning of what is expected to happen and I need to do something additional? I know I can use ReactRouter to redirect on sucessful sign-in, but I don't understand the point of the redirectTo parameter.

@leggetter
Copy link

leggetter commented Oct 12, 2021

I'm was seeing a similar problem using v1.2.1 of supabase-js.

For my small app which is based off of this official example I'm not seeing the redirectTo value being POSTed to the /magiclink endpoint - only the email:

{"email":"..."}

Settings for my app:

image

Code:

  const handleMagicLinkSignIn = async (e) => {
    e.preventDefault()
    setError('')
    setMessage('')
    setLoading(true)
    const { error } = await supabaseClient.auth.signIn({ email }, supabaseOptions)
    if (error) setError(error.message)
    else setMessage('Check your email for the magic link')
    setLoading(false)
  }

Where supabaseOptions is:

const supabaseOptions = {
  redirectTo: 'http://localhost:3000/auth'
}

⚠️ Update

Updating to v1.24.0 seems to have fixed the problem (though an app restart and a browser refresh didn't seen to instantly fix things) where the redirect_to is sent in an OPTIONS request:

https://APP.supabase.co/auth/v1/magiclink?redirect_to=http%3A%2F%2Flocalhost%3A3000%2Fauth

@mvxt
Copy link

mvxt commented Nov 5, 2021

redirectTo is similarly not working for me. Using @supabase/supabase-js v1.25.2. I've made sure my Auth settings in Supabase and the URL I'm passing to redirectTo match:

image

In the code, I'm doing a direct call like so:

    const { error } = await supabase.auth.signIn({
      email: info.email,
      password: info.password
    }, {
      redirectTo: 'http://localhost:3000/settings
    });

In the networking tab, I've confirmed that the library is passing the redirect URL correctly as a parameter to Supabase API. It makes the call twice, first with OPTIONS and then a POST.

Screen Shot 2021-11-05 at 2 09 00 AM

Screen Shot 2021-11-05 at 2 10 14 AM

Am I missing something obvious here? This is getting frustrating.

EDIT: Wait a second, does it appear that the call to Supabase API is adding an s to the http?

@leggetter
Copy link

@mvxt are you using the latest version of the SDK?

See my update note here #3191 (comment)

@mvxt
Copy link

mvxt commented Nov 5, 2021

@leggetter Yes, as mentioned I am using v1.25.2

@leggetter
Copy link

leggetter commented Nov 5, 2021

@mvxt how about specifically trying v1.24.0 as that did work for me?

Wait a second, does it appear that the call to Supabase API is adding an s to the http?

From your screenshot, that is happening, yes.

@kiwicopple kiwicopple added the help wanted Extra attention is needed label Nov 7, 2021
@vbylen
Copy link

vbylen commented Dec 9, 2021

Also experiencing this, setting the redirectTo param is not respected.

@riderx
Copy link
Contributor

riderx commented Dec 10, 2021

Same here, it's sometime respected, but seems not reliable, and fun fac if you copy the link in email and add the right URL to the link before load it, it work.
I will try to pseudo fix the issue by editing the email template

@varna
Copy link

varna commented Dec 28, 2021

Seems like the same issue as #2623 #3069 #4540

Only the first argument of Additional Redirect URLs is used while validating redirectURL.

If redirectURL is set to http://localhost:3000

This fails:
This fails

This works:
This works

I'm not sure how to run the tests... supabase/auth#316 should fail

@bnjmnt4n
Copy link
Contributor

There's a space after the comma which isn't recognized. Perhaps try removing the space? Maybe this should be changed on the studio frontend to flag this out as a common error, or GoTrue can be modified as well to trim leading/trailing whitespace.

@varna
Copy link

varna commented Dec 28, 2021

There's a space after the comma which isn't recognized. Perhaps try removing the space? Maybe this should be changed on the studio frontend to flag this out as a common error, or GoTrue can be modified as well to trim leading/trailing whitespace.

This worked :D Thanks!

Input validator sounds nice.

@kiwicopple kiwicopple added the frontend Related to supabase dashboard label Dec 28, 2021
@kiwicopple kiwicopple changed the title redirectTo doesn't seem to work for redirects (supabase.auth.signIn) Dashboard should trim spaces between "redirect" links Dec 28, 2021
@kiwicopple
Copy link
Member

I've renamed this one based on @bnjmnt4n 's comment. We can fix this on the frontend, but afterwards we should also fix GoTrue to be consistent.

I think @MildTomato may want to change the format from a Comma list? Perhaps we can use a safer component here?

@varna
Copy link

varna commented Dec 28, 2021

I suggest Combobox with multiple items:
image

@iamclaytonray
Copy link

It's now Feb 2022 and this is still broken. 2 months have passed without any movement on this very critical bug. And it's not documented, which means I pushed what I thought was a stable API to production. While I love GraphQL, why is that prioritized over simple things like this? Lack of communication here isn't breeding confidence.

@MildTomato
Copy link
Contributor

Hey @iamclaytonray (and everyone else in this thread)

A lot of these configs for Auth are currently in forms that don't have any validation/safe formatting, so I'm not surprised we've had a problem like this. I think this, and all the other inputs on the Auth config settings page need to be overhauled so that the correct format is updated for GoTrue settings.

In the meantime, we'll add a quick fix to just trim the spaces in the current form so at least this particular issue is fixed.

We have been a bit understaffed (see our new position open), so dealing with these front-end issues I agree should have been a lot faster than this.

@iamclaytonray
Copy link

iamclaytonray commented Feb 16, 2022

@MildTomato - appreciate the response.

I was just reading through this issue and my main takeaway was that the Flow maintainers kind of just dropped off the planet and didn't communicate that well. Obviously it's a different comparison (this is clearly an active business, etc) but I think the core pain point remains; if there are issues, transparency and communication often solves it.

I was unaware of the understaffed issue and I would assume most others are under the impression you have a full team as well. Being transparent about this doesn't hurt your credibility; it solidifies it. Openly communicating these problems welcomes the community to help as opposed to potential contributors thinking that you have plenty of people but just neglecting to work on certain issues.

I can't dedicate full time hours to Supabase but happy to give feedback and/or direction if desired, and for free. Also going to look into contributing here and there (if time permits, I run a startup too)


More related on this specific issue; I think trimming will fix the OP issue but I don't see a reason it would fix the intermediate issue. Sometimes the proper redirectTo is used. Sometimes it isn't. All while no settings have been changed. Because this is such a major issue (again, users cannot login to my apps), I'll do my best to take a look around the code and hopefully come up with a solution.

EDIT: I figured startup would be simpler. I don't have docker setup on this machine and don't want to spend a day fighting with it, so I'll pass the torch until I find more time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working frontend Related to supabase dashboard help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests