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

Invalid checks/redirects in UI #289

Closed
nsklikas opened this issue Sep 25, 2024 · 4 comments · Fixed by #298
Closed

Invalid checks/redirects in UI #289

nsklikas opened this issue Sep 25, 2024 · 4 comments · Fixed by #298
Labels
bug Something isn't working

Comments

@nsklikas
Copy link
Contributor

Not sure if this is what causes the bug or not, but in mulitple places in the frontend we are checking if the response from the backend contains a request_url (and sometimes we even redirect the user there), eg try to go to https://iam.dev.canonical.com/stg-identity-jaas-dev-login-ui/ui/reset_password. I thought we caught those on review, but it looks like some of these changes went through.

From a quick search:

I am pretty sure that this is not correct, but I am not sure what these checks are trying to accomplish. In a production environment all these are pointing to admin APIs, which are not exposed to the public internet. The request_url is the URL that the backend used to call Kratos, there is no reason to call the same URL from the frontend.

Originally posted by @nsklikas in #247 (comment)

@nsklikas nsklikas added the bug Something isn't working label Sep 25, 2024
Copy link

Thank you for reporting us your feedback!

The internal ticket has been created: https://warthogs.atlassian.net/browse/IAM-1104.

This message was autogenerated

@edlerd
Copy link
Contributor

edlerd commented Sep 28, 2024

We never redirect to the data.request_url, the value is only checked. We redirect to the current page while setting the current flow ID in case the data.request_url is defined on the flow object. Basically reloading the current page with the current flow.

I don't think that is a bug per se. I don't know why we would refresh in case the data.request_url is set. Do you have insights why this value might be present @nsklikas ? Which conditions lead to it being populated, and why would we want to refresh in case it is?

@nsklikas
Copy link
Contributor Author

Well, the browser is redirected to request_url at

window.location.href = data.request_url;
.

I believe that this field will always be populated. AFAICT It is the url that was used to make this request,

The kratos api docs say that it is a required field in the response:

RequestURL is the initial URL that was requested from Ory Kratos. It can be used to forward information contained in the URL's path or query for example.

I don't think that is a bug per se. I don't know why we would refresh in case the data.request_url is set. Do you have insights why this value might be present @nsklikas ? Which conditions lead to it being populated, and why would we want to refresh in case it is?

I don't know, I was not involved in this and that's why I was hesitant to remove it. Perhaps @natalian98 has some insight

edlerd added a commit to edlerd/identity-platform-login-ui that referenced this issue Sep 30, 2024
Signed-off-by: David Edler <david.edler@canonical.com>
@edlerd
Copy link
Contributor

edlerd commented Sep 30, 2024

Well, the browser is redirected to request_url at

window.location.href = data.request_url;

.
I believe that this field will always be populated. AFAICT It is the url that was used to make this request,

Right, that seems incorrect. I just tested and could not see any difference when removing those checks for the request url. So putting up a PR to remove them.

edlerd added a commit to edlerd/identity-platform-login-ui that referenced this issue Sep 30, 2024
Signed-off-by: David Edler <david.edler@canonical.com>
nsklikas added a commit that referenced this issue Oct 1, 2024
fix(request) avoid using request url in the ui. fixes #289
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants