returnTo query not being honoured in the seamless registration flows when using OIDC.

Urbansson commented 2 years ago

Preflight checklist

[X] I could not find a solution in the existing issues, docs, nor discussions.
[X] I agree to follow this project's Code of Conduct.
[X] I have read and am following this repository's Contribution Guidelines.
[ ] This issue affects my Ory Cloud project.
[X] I have joined the Ory Community Slack.
[ ] I am signed up to the Ory Security Patch Newsletter.

Describe the bug

When logging in for the first time using a OIDC provider the return_to parameter provided in the login flow is not used.

Reproducing the bug

Create a new login flow: GET http://127.0.0.1:3000/self-service/login/browser?aal=&refresh=false&return_to=http://127.0.0.1:3000/foo

Then log in using a new OIDC identity.

POST http://127.0.0.1:4433/self-service/login?flow=a435b2b6-75ed-461c-aed2-64c7205aa0be
{
    "csrf_token": "By5OLwBbRjhFNCX8R3tDZtYTQ+iEWrHHo0kqBNXWLdnjA8WpU+dkkZaIbqlDH+U/yOTreUKdg5Ei+a7ZUh8YsA==",
    "method": "oidc",
    "provider": "google"
}

After completing the login flow you will be redirected to / instead of http://127.0.0.1:3000/foo

Relevant log output

No response

Relevant configuration

selfservice:
  methods:
    oidc:
      enabled: true
      config:
        providers:
          - id: google # this is `<provider-id>` in the Authorization callback URL. DO NOT CHANGE IT ONCE SET!
            provider: google
            client_id: .... # Replace this with the OAuth2 Client ID
            client_secret: .... # Replace this with the OAuth2 Client secret
            #mapper_url: file:///etc/config/kratos/oidc.google.jsonnet
            scope:
              - email
              - profile
              # other supported scopes can be found in Google OAuth 2.0 dev docs
            requested_claims:
              id_token:
                email:
                  essential: true
                email_verified:
                  essential: true
                given_name:
                  essential: true
                family_name: null
                hd: null # If you want the G Suite domain
  flows:
    registration:
      lifespan: 10m
      ui_url: http://127.0.0.1:3000/registration
      after:
        oidc:
          hooks:
            - hook: session

Version

v0.9.0-alpha.3

On which operating system are you observing this issue?

macOS

In which environment are you deploying?

Kubernetes

Additional Context

The issue is that the NewRegistrationFlow function is called with the request from the OIDC callback request and not the initial login request. That means that all the query parameters provided in the login request including the return_to is lost.

Replacing the .RequestURL in the newly create registration flowwith the one from the login flow solves the problem and honors the return_to.

selfservice/strategy/oidc/strategy_login.go | Line 92
  // This flow only works for browsers anyways.
  aa, err := s.d.RegistrationHandler().NewRegistrationFlow(w, r, flow.
  aa.RequestURL = a.RequestURL // This line is added to change back to the initial request and keep the return_to parameters

KajsaEklof commented 2 years ago

Hello, we've come across something similar when re-directing to the 2FA second stage login flow.

We have some middleware checking if a users session is still valid and if it's not they are re-directed to sign-in with a return_to query parameter in the URL which we are also passing in when initialising the selfServiceLoginFlow. If a user has 2FA (totp) set up we handle the error that comes back when they sign in and redirect them to the url in redirect_browser_to (https://localhost:4433/self-service/login/browser?aal=aal2).

After the redirect to complete the second stage of login (we get redirected to /sign-in?flow=[FLOW-ID], if the method for the first stage of login was oidc the return_to is included in the flow and we can access it in the response from kratos.getSelfServiceLoginFlow(this.flowId). But if the method for the first stage was email + password and there is no return_to parameter included in the response from kratos.getSelfServiceLoginFlow(this.flowId).

Could this be a similar bug where the return_to parameter is not being included in the new flow created for the second stage (2FA) of login?

hchagen commented 2 years ago

I came across a very similar issue in the recovery flow when using the link strategy. I also found where the error happens, and the redirect_to is dropped, and it's a pattern I've seen all over the code, so it makes sense that it would surface in more places.

So the code literally says "take over return_to parameter", which it kinda does, here:

    // Take over 'return_to' parameter from recovery flow
    sfRequestURL, err := url.Parse(sf.RequestURL)
    if err != nil {
        return s.retryRecoveryFlowWithError(w, r, flow.TypeBrowser, err)
    }
    fRequestURL, err := url.Parse(f.RequestURL)
    if err != nil {
        return s.retryRecoveryFlowWithError(w, r, flow.TypeBrowser, err)
    }
    sfQuery := sfRequestURL.Query()
    sfQuery.Set("return_to", fRequestURL.Query().Get("return_to"))
    sfRequestURL.RawQuery = sfQuery.Encode()
    sf.RequestURL = sfRequestURL.String()

However, further down it's dropped again, here:

http.Redirect(w, r, sf.AppendTo(s.d.Config(r.Context()).SelfServiceFlowSettingsUI()).String(), http.StatusSeeOther)

Which can be easily hotpatched like this:

http.Redirect(w, r, urlx.CopyWithQuery(sf.AppendTo(s.d.Config(r.Context()).SelfServiceFlowSettingsUI()), url.Values{"return_to": {sfQuery.Get("return_to")}}).String(), http.StatusSeeOther)

This is a pretty dirty solution though, I think maybe it would be better if the (flowType).AppendTo() actually honored the flows entire query, and not just the flow id. The downside to that solution, is that all other queryparams (like the no longer needed token, in the recovery case) will also be appended.

Would anyone official/from Ory like to chime in?

ory / kratos