zorn-v / nextcloud-social-login

GNU Affero General Public License v3.0
198 stars 137 forks source link

fix malformed redirectUrl with query param #427

Closed BAnd313 closed 9 months ago

BAnd313 commented 9 months ago

Telegram Login flow When the login url already has a query param (e.g. https:///login?redirect_url=/apps/calendar), the redirect url (after the login process) is obtained by a simple concatenation of the ? symbol, without checking if the original url already contains query parameters, thus making the final url malformed and blocking the login flow with an error. immagine

This fix wraps the redirectUrl into an URL object and adds all params using the provided method. Moreover, all params are now url-encoded.

zorn-v commented 9 months ago

What is catch (TypeError) ? And why not something like ?

location = redirectUrl + (redirectUrl.indexOf('?') >= 0 ? '&' : '?') + new URLSearchParams(data)
BAnd313 commented 9 months ago

The catch is meant for security in case the redirectUrl is not a valid url for the URL interface (should never occur, so it's an extra). The example you mentioned would be valid, but I think that manipulating urls as raw strings should be avoided if possible and that standard libraries should be used instead. In this way some errors like this can be avoided (and in this case we have also the url encoding). But in practice, both solutions would work

zorn-v commented 9 months ago

The catch is meant for security in case the redirectUrl is not a valid url for the URL interface

I ask about TypeError in catch. Some mix from typescript ? Is it valid to define variables with existing type names in js ?

Will check later, maybe better just do not include query params when generate redirectUrl on php side.

BAnd313 commented 9 months ago

The doc says that URL can throw this exception, for example even if the input string is a relative url.

Ok, thanks. But in this way we lose a query param that is useful for landing to the correct path

zorn-v commented 9 months ago

Just to remind https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/try...catch#conditional_catch_blocks

But in this way we lose a query param that is useful for landing to the correct path

And actually this I will check

zorn-v commented 9 months ago

Thanks for PR by the way :wink:

BAnd313 commented 9 months ago

Just to remind https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/try...catch#conditional_catch_blocks

Ok, got it, fix it rn!

zorn-v commented 9 months ago

but I think that manipulating urls as raw strings should be avoided if possible and that standard libraries should be used instead

Garbage in input = garbage in output. There is no sense to blow up code for simple things. Your code is less readable, but ok. Let it be.