Recovery link doesn't log out existing users

[andrewbanchich]

To reproduce:

1. Create an identity using [email protected].
2. Generate recovery link for that account.
3. Set password using the recovery link.
4. While logged in on the same computer, create a new identity [email protected].
5. Generate a recovery link for [email protected].
6. Clicking on the link opens the UI, but still logged in as [email protected].

[aeneasr]

Thank you! Added to the 0.6 milestone.

[mattbonnell]

So, when we use the recovery token, we issue a new session by calling CreateAndIssueCookie. Looking at the code for CreateAndIssueCookie here, it would seem that we should be overwriting any old session cookie:

kratos/session/manager_http.go

Lines 63 to 96 in 83aedcb

	func (s *ManagerHTTP) IssueCookie(ctx context.Context, w http.ResponseWriter, r *http.Request, session *Session) error {
	cookie, _ := s.r.CookieManager().Get(r, s.cookieName)
	if s.c.SessionDomain() != "" {
	cookie.Options.Domain = s.c.SessionDomain()
	}
	old, err := s.FetchFromRequest(context.Background(), r)
	if err != nil {
	// No session was set prior -> regenerate anti-csrf token
	_ = s.r.CSRFHandler().RegenerateToken(w, r)
	} else if old.Identity.ID != session.Identity.ID {
	// No session was set prior -> regenerate anti-csrf token
	_ = s.r.CSRFHandler().RegenerateToken(w, r)
	}
	if s.c.SessionPath() != "" {
	cookie.Options.Path = s.c.SessionPath()
	}
	if s.c.SessionSameSiteMode() != 0 {
	cookie.Options.SameSite = s.c.SessionSameSiteMode()
	}
	cookie.Options.MaxAge = 0
	if s.c.SessionPersistentCookie() {
	cookie.Options.MaxAge = int(s.c.SessionLifespan().Seconds())
	}
	cookie.Values["session_token"] = session.Token
	if err := cookie.Save(r, w); err != nil {
	return errors.WithStack(err)
	}
	return nil
	}

We could explicitly purge the cookie from the request here, but it strikes me as strange that this would be necessary.

Can you share your configuration file, @andrewbanchich?

[aeneasr]

I think another good approach would be to try and recreate the issue and see if we encounter it - from there the debugging can start :)

[andrewbanchich]

version: v0.5.5-alpha.1

identity:
  default_schema_url: file:///etc/config/kratos/identity.traits.schema.json

selfservice:
  default_browser_return_url: https://myapp.com/
  whitelisted_return_urls:
    - https://myapp.com/
  flows:
    settings:
      ui_url: https://myapp.com/settings
      lifespan: 1h
      privileged_session_max_age: 1h
      after:
        default_browser_return_url: https://myapp.com/
    logout:
      after:
        default_browser_return_url: https://myapp.com/
    registration:
      ui_url: https://myapp.com/auth/registration
      lifespan: 1h
    login:
      ui_url: https://myapp.com/auth/login
      lifespan: 1h
      after:
        default_browser_return_url: https://myapp.com/
    verification:
      enabled: true
      ui_url: https://myapp.com/verify
      after:
        default_browser_return_url: https://myapp.com/
      lifespan: 1m
    error:
      ui_url: https://myapp.com/error
    recovery:
      enabled: true
      ui_url: https://myapp.com/auth/recovery
  methods:
    profile:
      enabled: true
    password:
      enabled: true
    link:
      enabled: true

courier:
  smtp:
    connection_uri: smtps://smtp:25/?skip_ssl_verify=false
    from_address: [email protected]

serve:
  admin:
    base_url: http://kratos:4434/
    port: 4434
  public:
    base_url: https://myapp.com/.ory/kratos/public
    port: 4433

log:
  level: warning
  format: text

hashers:
  argon2:
    memory: 131072
    iterations: 3
    parallelism: 1
    salt_length: 16
    key_length: 32

session:
  lifespan: 1h
  cookie:
    domain: myapp.com
    same_site: Lax

[dan2kx]

I'm having a similar issue when logging in, if a user is logged in and another user logs in, the profile/settings page shows the original user data, i expect its the same cause?

[kszafran]

I believe the main problem here is that recovery is carried out only for unauthenticated callers:

kratos/selfservice/flow/recovery/handler.go

Line 82 in 9ce2b9e

public.GET(RouteSubmitFlow, h.d.SessionHandler().IsNotAuthenticated(h.submitFlow, redirect))

If the request has the ory_kratos_session cookie attached (as is the case when using samesite=lax), it just gets redirected to default_browser_return_url.

Using samesite=strict solves the problem, but causes another one. While the initial request with the recovery token does not include the current ory_kratos_session cookie (and so account is recovered successfully), when following the 303 redirect, the browser does not include the new ory_kratos_session cookie (although it saves it just fine). So the user lands on the login page, although they are already logged in.

@aeneasr It would seem that the fix would be to run h.submitFlow regardless of whether the caller is authenticated or not. (We'd possibly need to also modify that method, too, if it tries to access the current session. I haven't checked that yet.) What do you think?

[kszafran]

Reading through the code more, I see now that the same endpoint that takes the recovery token, also handles recovery form submissions (when you want to get an email with the recovery link). In this case, we should ignore the existing session only when the request has a recovery token 🤔.

[kszafran]

One workaround for this issue is to strip request cookies on your reverse proxy. For example, with nginx, you can add this to your configuration:

map $request_uri $kratos_cookie {
  default $http_cookie;
  ~^/self-service/recovery\?.+token= '';
}

proxy_set_header Cookie $kratos_cookie;

[aeneasr]

done (not released yet)

[kszafran]

@aeneasr I wanted to test your fix locally (running f96e48f), but I can't get the configuration to validate:

The configuration contains values or keys which are invalid:
selfservice.flows.recovery.after: map[hooks:[map[hook:revoke_active_sessions]]]
                                  ^-- doesn't validate with "#/definitions/selfServiceAfterRecovery"

The configuration contains values or keys which are invalid:
selfservice.flows.recovery.after.hooks: [map[hook:revoke_active_sessions]]
                                        ^-- doesn't validate with "#/definitions/selfServiceHooks"

The configuration contains values or keys which are invalid:
selfservice.flows.recovery.after.hooks.0: map[hook:revoke_active_sessions]
                                          ^-- anyOf failed

The configuration contains values or keys which are invalid:
selfservice.flows.recovery.after.hooks.0: map[hook:revoke_active_sessions]
                                          ^-- doesn't validate with "#/definitions/selfServiceWebHook"

The configuration contains values or keys which are invalid:
selfservice.flows.recovery.after.hooks.0: map[hook:revoke_active_sessions]
                                          ^-- validation failed

The configuration contains values or keys which are invalid:
selfservice.flows.recovery.after.hooks.0.config: <nil>
                                                 ^-- one or more required properties are missing

The configuration contains values or keys which are invalid:
selfservice.flows.recovery.after.hooks.0.hook: revoke_active_sessions
                                               ^-- value must be "web_hook"

FATA[2022-03-07T11:18:16+01:00] Unable to instantiate configuration.          audience=application error=map[message:I[#/selfservice/flows/recovery/after] S[#/properties/selfservice/properties/flows/properties/recovery/properties/after/$ref] doesn't validate with "#/definitions/selfServiceAfterRecovery"
  I[#/selfservice/flows/recovery/after/hooks] S[#/definitions/selfServiceAfterRecovery/properties/hooks/$ref] doesn't validate with "#/definitions/selfServiceHooks"
    I[#/selfservice/flows/recovery/after/hooks/0] S[#/definitions/selfServiceHooks/items/anyOf] anyOf failed
      I[#/selfservice/flows/recovery/after/hooks/0] S[#/definitions/selfServiceHooks/items/anyOf/0/$ref] doesn't validate with "#/definitions/selfServiceWebHook"
        I[#/selfservice/flows/recovery/after/hooks/0] S[#/definitions/selfServiceWebHook] validation failed
          I[#/selfservice/flows/recovery/after/hooks/0] S[#/definitions/selfServiceWebHook/required] missing properties: "config"
          I[#/selfservice/flows/recovery/after/hooks/0/hook] S[#/definitions/selfServiceWebHook/properties/hook/const] value must be "web_hook"] service_name=Ory Kratos service_version=master

The relevant part of the config is:

    recovery:
      enabled: true
      ui_url: http://127.0.0.1:4455/recovery
      after:
        hooks:
          - hook: revoke_active_sessions

[kszafran]

@aeneasr I managed to test revoke_active_sessions after recovery locally with v0.9.0-alpha.2 and it does not solve the original problem.

[kszafran]

I can confirm it's fixed in v0.9.0-alpha.3.