feat: Normalize redirection URLs and globs

[hf]

What kind of change does this PR introduce?

If a user specifies a redirect URL glob of the form:

https://example.com

But they send a redirect_to parameter to /authorize of the form:

https://example.com/

(Note the trailing slash!)

GoTrue will not match the two URLs, even though in the modern web they are equivalent. This PR normalizes HTTP(S) URLs to remove the trailing slash in the redirect_to parameter; as well as ignoring any trailing slashes in the URL allow list.

[kangmingtay]

looks good! but can we also add a test case for this?

[hf]

looks good! but can we also add a test case for this

Yeah I've been trying to find a way to test it but it's a bit too complex of a case, and the methods have to become public otherwise, so I'm not sure if it's worth it.

[kangmingtay]

@hf you can consider adding it as a test case here

[hf]

@hf you can consider adding it as a test case here

Oh that's splendid!

[hf]

@kangmingtay Moving the parsing logic to the configuration doesn't really help much and just duplicates code. I also think it breaks some non-compliant tests as the code has to panic.

Otherwise, I think it's good this way -- the tests are failing because of a rate limit check. I'm trying to find a way to remove it on these tests. Please review again when you can.

[J0]

Hmm throwing out ideas here: off the top of head I can think of three suggestions to deal with the rate limiting.

1. Use build tags for tests

2. Establish a naming convention for tests with rate limits and then filter tests via regexp like go test -p 1 -v -run '<relevant regexp>'

3. Gate the rate limiting at api.go with an env var so that it's only enabled when an env var is set to true

   \3. is a little clunky but 1 and 2 would constrain the separation to only tests. Imagine there would be other cases outside of testing (e.g. benchmarks) where we want to turn off rate limiting. If any of the three would be potential solutions lmk and I can file a PR on Sunday or so

Let me know if there are further suggestions or other options we should consider!

Let me also look into our rate limiter lib to see if there's something built in we can use

[kangmingtay]

Moving the parsing logic to the configuration doesn't really help much and just duplicates code.

@hf why can't we just move the parsing logic to the configuration completely so we do it once when the config is loaded? i don't see why it would be duplicated?

[github-actions]

🎉 This PR is included in version 2.10.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

[hf]

What kind of change does this PR introduce?

If a user specifies a redirect URL glob of the form:

https://example.com

But they send a redirect_to parameter to /authorize of the form:

https://example.com/

(Note the trailing slash!)

GoTrue will not match the two URLs, even though in the modern web they are equivalent. This PR normalizes HTTP(S) URLs to remove the trailing slash in the redirect_to parameter; as well as ignoring any trailing slashes in the URL allow list.

[kangmingtay]

looks good! but can we also add a test case for this?

[hf]

looks good! but can we also add a test case for this

Yeah I've been trying to find a way to test it but it's a bit too complex of a case, and the methods have to become public otherwise, so I'm not sure if it's worth it.

[kangmingtay]

@hf you can consider adding it as a test case here

[hf]

@hf you can consider adding it as a test case here

Oh that's splendid!

[hf]

@kangmingtay Moving the parsing logic to the configuration doesn't really help much and just duplicates code. I also think it breaks some non-compliant tests as the code has to panic.

Otherwise, I think it's good this way -- the tests are failing because of a rate limit check. I'm trying to find a way to remove it on these tests. Please review again when you can.

[J0]

Hmm throwing out ideas here: off the top of head I can think of three suggestions to deal with the rate limiting.

1. Use build tags for tests

2. Establish a naming convention for tests with rate limits and then filter tests via regexp like go test -p 1 -v -run '<relevant regexp>'

3. Gate the rate limiting at api.go with an env var so that it's only enabled when an env var is set to true

   \3. is a little clunky but 1 and 2 would constrain the separation to only tests. Imagine there would be other cases outside of testing (e.g. benchmarks) where we want to turn off rate limiting. If any of the three would be potential solutions lmk and I can file a PR on Sunday or so

Let me know if there are further suggestions or other options we should consider!

Let me also look into our rate limiter lib to see if there's something built in we can use

[kangmingtay]

Moving the parsing logic to the configuration doesn't really help much and just duplicates code.

@hf why can't we just move the parsing logic to the configuration completely so we do it once when the config is loaded? i don't see why it would be duplicated?

[github-actions]

🎉 This PR is included in version 2.10.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

[hf]

What kind of change does this PR introduce?

If a user specifies a redirect URL glob of the form:

https://example.com

But they send a redirect_to parameter to /authorize of the form:

https://example.com/

(Note the trailing slash!)

GoTrue will not match the two URLs, even though in the modern web they are equivalent. This PR normalizes HTTP(S) URLs to remove the trailing slash in the redirect_to parameter; as well as ignoring any trailing slashes in the URL allow list.

[kangmingtay]

looks good! but can we also add a test case for this?

[hf]

looks good! but can we also add a test case for this

Yeah I've been trying to find a way to test it but it's a bit too complex of a case, and the methods have to become public otherwise, so I'm not sure if it's worth it.

[kangmingtay]

@hf you can consider adding it as a test case here

[hf]

@hf you can consider adding it as a test case here

Oh that's splendid!

[hf]

@kangmingtay Moving the parsing logic to the configuration doesn't really help much and just duplicates code. I also think it breaks some non-compliant tests as the code has to panic.

Otherwise, I think it's good this way -- the tests are failing because of a rate limit check. I'm trying to find a way to remove it on these tests. Please review again when you can.

[J0]

Hmm throwing out ideas here: off the top of head I can think of three suggestions to deal with the rate limiting.

1. Use build tags for tests

2. Establish a naming convention for tests with rate limits and then filter tests via regexp like go test -p 1 -v -run '<relevant regexp>'

3. Gate the rate limiting at api.go with an env var so that it's only enabled when an env var is set to true

   \3. is a little clunky but 1 and 2 would constrain the separation to only tests. Imagine there would be other cases outside of testing (e.g. benchmarks) where we want to turn off rate limiting. If any of the three would be potential solutions lmk and I can file a PR on Sunday or so

Let me know if there are further suggestions or other options we should consider!

Let me also look into our rate limiter lib to see if there's something built in we can use

[kangmingtay]

Moving the parsing logic to the configuration doesn't really help much and just duplicates code.

@hf why can't we just move the parsing logic to the configuration completely so we do it once when the config is loaded? i don't see why it would be duplicated?

[github-actions]

🎉 This PR is included in version 2.10.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

[hf]

What kind of change does this PR introduce?

If a user specifies a redirect URL glob of the form:

https://example.com

But they send a redirect_to parameter to /authorize of the form:

https://example.com/

(Note the trailing slash!)

GoTrue will not match the two URLs, even though in the modern web they are equivalent. This PR normalizes HTTP(S) URLs to remove the trailing slash in the redirect_to parameter; as well as ignoring any trailing slashes in the URL allow list.

[kangmingtay]

looks good! but can we also add a test case for this?

[hf]

looks good! but can we also add a test case for this

Yeah I've been trying to find a way to test it but it's a bit too complex of a case, and the methods have to become public otherwise, so I'm not sure if it's worth it.

[kangmingtay]

@hf you can consider adding it as a test case here

[hf]

@hf you can consider adding it as a test case here

Oh that's splendid!

[hf]

@kangmingtay Moving the parsing logic to the configuration doesn't really help much and just duplicates code. I also think it breaks some non-compliant tests as the code has to panic.

Otherwise, I think it's good this way -- the tests are failing because of a rate limit check. I'm trying to find a way to remove it on these tests. Please review again when you can.

[J0]

Hmm throwing out ideas here: off the top of head I can think of three suggestions to deal with the rate limiting.

1. Use build tags for tests

2. Establish a naming convention for tests with rate limits and then filter tests via regexp like go test -p 1 -v -run '<relevant regexp>'

3. Gate the rate limiting at api.go with an env var so that it's only enabled when an env var is set to true

   \3. is a little clunky but 1 and 2 would constrain the separation to only tests. Imagine there would be other cases outside of testing (e.g. benchmarks) where we want to turn off rate limiting. If any of the three would be potential solutions lmk and I can file a PR on Sunday or so

Let me know if there are further suggestions or other options we should consider!

Let me also look into our rate limiter lib to see if there's something built in we can use

[kangmingtay]

Moving the parsing logic to the configuration doesn't really help much and just duplicates code.

@hf why can't we just move the parsing logic to the configuration completely so we do it once when the config is loaded? i don't see why it would be duplicated?

[github-actions]

🎉 This PR is included in version 2.10.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

[hf]

What kind of change does this PR introduce?

If a user specifies a redirect URL glob of the form:

https://example.com

But they send a redirect_to parameter to /authorize of the form:

https://example.com/

(Note the trailing slash!)

GoTrue will not match the two URLs, even though in the modern web they are equivalent. This PR normalizes HTTP(S) URLs to remove the trailing slash in the redirect_to parameter; as well as ignoring any trailing slashes in the URL allow list.

[kangmingtay]

looks good! but can we also add a test case for this?

[hf]

looks good! but can we also add a test case for this

Yeah I've been trying to find a way to test it but it's a bit too complex of a case, and the methods have to become public otherwise, so I'm not sure if it's worth it.

[kangmingtay]

@hf you can consider adding it as a test case here

[hf]

@hf you can consider adding it as a test case here

Oh that's splendid!

[hf]

@kangmingtay Moving the parsing logic to the configuration doesn't really help much and just duplicates code. I also think it breaks some non-compliant tests as the code has to panic.

Otherwise, I think it's good this way -- the tests are failing because of a rate limit check. I'm trying to find a way to remove it on these tests. Please review again when you can.

[J0]

Hmm throwing out ideas here: off the top of head I can think of three suggestions to deal with the rate limiting.

1. Use build tags for tests

2. Establish a naming convention for tests with rate limits and then filter tests via regexp like go test -p 1 -v -run '<relevant regexp>'

3. Gate the rate limiting at api.go with an env var so that it's only enabled when an env var is set to true

   \3. is a little clunky but 1 and 2 would constrain the separation to only tests. Imagine there would be other cases outside of testing (e.g. benchmarks) where we want to turn off rate limiting. If any of the three would be potential solutions lmk and I can file a PR on Sunday or so

Let me know if there are further suggestions or other options we should consider!

Let me also look into our rate limiter lib to see if there's something built in we can use

[kangmingtay]

Moving the parsing logic to the configuration doesn't really help much and just duplicates code.

@hf why can't we just move the parsing logic to the configuration completely so we do it once when the config is loaded? i don't see why it would be duplicated?

[github-actions]

🎉 This PR is included in version 2.10.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

[hf]

What kind of change does this PR introduce?

If a user specifies a redirect URL glob of the form:

https://example.com

But they send a redirect_to parameter to /authorize of the form:

https://example.com/

(Note the trailing slash!)

GoTrue will not match the two URLs, even though in the modern web they are equivalent. This PR normalizes HTTP(S) URLs to remove the trailing slash in the redirect_to parameter; as well as ignoring any trailing slashes in the URL allow list.

[kangmingtay]

looks good! but can we also add a test case for this?

[hf]

looks good! but can we also add a test case for this

Yeah I've been trying to find a way to test it but it's a bit too complex of a case, and the methods have to become public otherwise, so I'm not sure if it's worth it.

[kangmingtay]

@hf you can consider adding it as a test case here

[hf]

@hf you can consider adding it as a test case here

Oh that's splendid!

[hf]

@kangmingtay Moving the parsing logic to the configuration doesn't really help much and just duplicates code. I also think it breaks some non-compliant tests as the code has to panic.

Otherwise, I think it's good this way -- the tests are failing because of a rate limit check. I'm trying to find a way to remove it on these tests. Please review again when you can.

[J0]

Hmm throwing out ideas here: off the top of head I can think of three suggestions to deal with the rate limiting.

1. Use build tags for tests

2. Establish a naming convention for tests with rate limits and then filter tests via regexp like go test -p 1 -v -run '<relevant regexp>'

3. Gate the rate limiting at api.go with an env var so that it's only enabled when an env var is set to true

   \3. is a little clunky but 1 and 2 would constrain the separation to only tests. Imagine there would be other cases outside of testing (e.g. benchmarks) where we want to turn off rate limiting. If any of the three would be potential solutions lmk and I can file a PR on Sunday or so

Let me know if there are further suggestions or other options we should consider!

Let me also look into our rate limiter lib to see if there's something built in we can use

[kangmingtay]

Moving the parsing logic to the configuration doesn't really help much and just duplicates code.

@hf why can't we just move the parsing logic to the configuration completely so we do it once when the config is loaded? i don't see why it would be duplicated?

[github-actions]

🎉 This PR is included in version 2.10.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

[hf]

What kind of change does this PR introduce?

If a user specifies a redirect URL glob of the form:

https://example.com

But they send a redirect_to parameter to /authorize of the form:

https://example.com/

(Note the trailing slash!)

GoTrue will not match the two URLs, even though in the modern web they are equivalent. This PR normalizes HTTP(S) URLs to remove the trailing slash in the redirect_to parameter; as well as ignoring any trailing slashes in the URL allow list.

[kangmingtay]

looks good! but can we also add a test case for this?

[hf]

looks good! but can we also add a test case for this

Yeah I've been trying to find a way to test it but it's a bit too complex of a case, and the methods have to become public otherwise, so I'm not sure if it's worth it.

[kangmingtay]

@hf you can consider adding it as a test case here

[hf]

@hf you can consider adding it as a test case here

Oh that's splendid!

[hf]

@kangmingtay Moving the parsing logic to the configuration doesn't really help much and just duplicates code. I also think it breaks some non-compliant tests as the code has to panic.

Otherwise, I think it's good this way -- the tests are failing because of a rate limit check. I'm trying to find a way to remove it on these tests. Please review again when you can.

[J0]

Hmm throwing out ideas here: off the top of head I can think of three suggestions to deal with the rate limiting.

1. Use build tags for tests

2. Establish a naming convention for tests with rate limits and then filter tests via regexp like go test -p 1 -v -run '<relevant regexp>'

3. Gate the rate limiting at api.go with an env var so that it's only enabled when an env var is set to true

   \3. is a little clunky but 1 and 2 would constrain the separation to only tests. Imagine there would be other cases outside of testing (e.g. benchmarks) where we want to turn off rate limiting. If any of the three would be potential solutions lmk and I can file a PR on Sunday or so

Let me know if there are further suggestions or other options we should consider!

Let me also look into our rate limiter lib to see if there's something built in we can use

[kangmingtay]

Moving the parsing logic to the configuration doesn't really help much and just duplicates code.

@hf why can't we just move the parsing logic to the configuration completely so we do it once when the config is loaded? i don't see why it would be duplicated?

[github-actions]

🎉 This PR is included in version 2.10.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

[hf]

What kind of change does this PR introduce?

If a user specifies a redirect URL glob of the form:

https://example.com

But they send a redirect_to parameter to /authorize of the form:

https://example.com/

(Note the trailing slash!)

GoTrue will not match the two URLs, even though in the modern web they are equivalent. This PR normalizes HTTP(S) URLs to remove the trailing slash in the redirect_to parameter; as well as ignoring any trailing slashes in the URL allow list.

[kangmingtay]

looks good! but can we also add a test case for this?

[hf]

looks good! but can we also add a test case for this

Yeah I've been trying to find a way to test it but it's a bit too complex of a case, and the methods have to become public otherwise, so I'm not sure if it's worth it.

[kangmingtay]

@hf you can consider adding it as a test case here

[hf]

@hf you can consider adding it as a test case here

Oh that's splendid!

[hf]

@kangmingtay Moving the parsing logic to the configuration doesn't really help much and just duplicates code. I also think it breaks some non-compliant tests as the code has to panic.

Otherwise, I think it's good this way -- the tests are failing because of a rate limit check. I'm trying to find a way to remove it on these tests. Please review again when you can.

[J0]

Hmm throwing out ideas here: off the top of head I can think of three suggestions to deal with the rate limiting.

1. Use build tags for tests

2. Establish a naming convention for tests with rate limits and then filter tests via regexp like go test -p 1 -v -run '<relevant regexp>'

3. Gate the rate limiting at api.go with an env var so that it's only enabled when an env var is set to true

   \3. is a little clunky but 1 and 2 would constrain the separation to only tests. Imagine there would be other cases outside of testing (e.g. benchmarks) where we want to turn off rate limiting. If any of the three would be potential solutions lmk and I can file a PR on Sunday or so

Let me know if there are further suggestions or other options we should consider!

Let me also look into our rate limiter lib to see if there's something built in we can use

[kangmingtay]

Moving the parsing logic to the configuration doesn't really help much and just duplicates code.

@hf why can't we just move the parsing logic to the configuration completely so we do it once when the config is loaded? i don't see why it would be duplicated?

[github-actions]

🎉 This PR is included in version 2.10.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀