canonical / identity-platform-admin-ui

Admin UI for the Canonical identity broker and identity provider solution
Other
6 stars 4 forks source link

WD-13860 - feat: return to URL that initiated login #376

Closed huwshimi closed 3 months ago

huwshimi commented 3 months ago

changes

https://warthogs.atlassian.net/browse/WD-13860

QA:

huwshimi commented 3 months ago

From a code perspective this looks fine (although I am no frontend expert), but I have a question regarding the safety of this.

What happens if someone opens a link with a malicious next? I may be missing something, but I think that next could be with a link to a malicious server (even though the frontend sets next to always be a path, I think that if I set it to a full URL, the frontend will redirect the user there) OR if they set it to an api endpoint? eg:

I think we need to add some filters on where the user is allowed to be redirected to.

This is a very good point. If a domain is included we could make sure it matches the current domain.

As for malicious paths (?next=/api/delete/all/users), I'm not sure what we can do about that. I'll have a bit more of a think, but it could be tricky to validate the next param as we don't know the source of the value (even if we knew it was returned by the API we wouldn't know who set the original value).

huwshimi commented 3 months ago

As for malicious paths (?next=/api/delete/all/users), I'm not sure what we can do about that. I'll have a bit more of a think, but it could be tricky to validate the next param as we don't know the source of the value (even if we knew it was returned by the API we wouldn't know who set the original value).

@BarcoMasile do you have any thoughts about this?

huwshimi commented 3 months ago

I think we need to add some filters on where the user is allowed to be redirected to.

Would we consider all /ui/... (or /<custom>/ui/...) paths to be safe? If so would could validate that the next path starts with the current basename.

BarcoMasile commented 3 months ago

As for malicious paths (?next=/api/delete/all/users), I'm not sure what we can do about that. I'll have a bit more of a think, but it could be tricky to validate the next param as we don't know the source of the value (even if we knew it was returned by the API we wouldn't know who set the original value).

@BarcoMasile do you have any thoughts about this?

@huwshimi @nsklikas Yes, basically the next param was not intended to be used with actual URL. The idea behind that is to allow "some information" to be brought back to the frontend SPA, to be used to then present the user with the same UI he/she was working with before the login process started.

My suggestion would be to use keywords that the FE can parse and use to map to whatever it needs to show the user. The keyword could then be mapped to a page, or to a modal inside a page, or wathever fits.

Of course the "mapping" process needs to be implementated in the FE code and needs to discard any next value that is not among the prescribed/allowed ones.

This keeps things super simple at the beginning and make it more complex later on. For example, if you have 10 pages in the UI, you could have 10 keywords to be mapped to those 10 full pages. Any other value passed would not be used by the FE code. And all it does is redirect the user to the previous page visited. This way we avoid any security risk.

huwshimi commented 3 months ago

@nsklikas @BarcoMasile I've refactored this to pass a key path that is used to access the correct path in the URL object on return.