Skip to content

Commit

Permalink
fix: respect return_to in OIDC API flow error case (#3893)
Browse files Browse the repository at this point in the history
* fix: respect return_to in OIDC API flow error case

This fix ensures that we redirect the user to the return_to URL
when an error occurs during the OIDC login for native flows.

Native flows are initialized through the API, and the browser
URL is retrieved from a 422 response after a POST to submit the
login flow. Successful OIDC flows already returned the `code` to
the `return_to` URL. Now, unsuccessful flows return the `flow` with
the current flow ID (which might have changed), so that the caller
can retrieve the full flow and act accordingly.

* fix: ignore trivvy CVE report

Bump in distroless is still open
  • Loading branch information
hperl authored Apr 25, 2024
1 parent 17f9a4f commit e8f1bcb
Show file tree
Hide file tree
Showing 5 changed files with 70 additions and 21 deletions.
1 change: 1 addition & 0 deletions .trivyignore
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
CVE-2022-30065
CVE-2024-2961
CVE-2023-2650
1 change: 1 addition & 0 deletions internal/client-go/go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ github.com/golang/protobuf v1.2.0/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5y
golang.org/x/net v0.0.0-20180724234803-3673e40ba225/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
golang.org/x/net v0.0.0-20190108225652-1e06a53dbb7e h1:bRhVy7zSSasaqNksaRZiA5EEI+Ei4I1nO5Jh72wfHlg=
golang.org/x/net v0.0.0-20190108225652-1e06a53dbb7e/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
golang.org/x/oauth2 v0.0.0-20190604053449-0f29369cfe45/go.mod h1:gOpvHmFTYa4IltrdGE7lF6nIHvwfUNPOp7c8zoXwtLw=
golang.org/x/sync v0.0.0-20181221193216-37e7f081c4d4 h1:YUO/7uOKsKeq9UokNS62b8FYywz3ker1l1vDZRCRefw=
golang.org/x/sync v0.0.0-20181221193216-37e7f081c4d4/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
Expand Down
15 changes: 11 additions & 4 deletions selfservice/flow/login/handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -547,12 +547,19 @@ func TestFlowLifecycle(t *testing.T) {
})

t.Run("case=returns session exchange code with any truthy value", func(t *testing.T) {
conf.MustSet(ctx, config.ViperKeyURLsAllowedReturnToDomains, []string{"https://www.ory.sh", "https://example.com"})
parameters := []string{"true", "True", "1"}

for i := range parameters {
res, body := initFlow(t, url.Values{"return_session_token_exchange_code": {parameters[i]}}, true)
assert.Contains(t, res.Request.URL.String(), login.RouteInitAPIFlow)
assert.NotEmpty(t, gjson.GetBytes(body, "session_token_exchange_code").String())
for _, param := range parameters {
t.Run("return_session_token_exchange_code="+param, func(t *testing.T) {
res, body := initFlow(t, url.Values{
"return_session_token_exchange_code": {param},
"return_to": {"https://example.com/redirect"},
}, true)
assert.Contains(t, res.Request.URL.String(), login.RouteInitAPIFlow)
assert.NotEmpty(t, gjson.GetBytes(body, "session_token_exchange_code").String())
assert.Equal(t, "https://example.com/redirect", gjson.GetBytes(body, "return_to").String())
})
}
})

Expand Down
19 changes: 17 additions & 2 deletions selfservice/strategy/oidc/strategy.go
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,7 @@ func (s *Strategy) alreadyAuthenticated(w http.ResponseWriter, r *http.Request,
returnTo := s.d.Config().SelfServiceBrowserDefaultReturnTo(ctx)
if redirecter, ok := f.(flow.FlowWithRedirect); ok {
r, err := x.SecureRedirectTo(r, returnTo, redirecter.SecureRedirectToOpts(ctx, s.d)...)
if err != nil {
if err == nil {
returnTo = r
}
}
Expand Down Expand Up @@ -462,6 +462,9 @@ func (s *Strategy) HandleCallback(w http.ResponseWriter, r *http.Request, ps htt
case *login.Flow:
a.TransientPayload = cntnr.TransientPayload
if ff, err := s.processLogin(w, r, a, et, claims, provider, cntnr); err != nil {
if errors.Is(err, flow.ErrCompletedByStrategy) {
return
}
if ff != nil {
s.forwardError(w, r, ff, err)
return
Expand Down Expand Up @@ -631,7 +634,19 @@ func (s *Strategy) handleError(w http.ResponseWriter, r *http.Request, f flow.Fl
return err
}
// return a new login flow with the error message embedded in the login flow.
redirectURL := lf.AppendTo(s.d.Config().SelfServiceFlowLoginUI(r.Context()))
var redirectURL *url.URL
if lf.Type == flow.TypeAPI {
returnTo := s.d.Config().SelfServiceBrowserDefaultReturnTo(r.Context())
if redirecter, ok := f.(flow.FlowWithRedirect); ok {
secureReturnTo, err := x.SecureRedirectTo(r, returnTo, redirecter.SecureRedirectToOpts(r.Context(), s.d)...)
if err == nil {
returnTo = secureReturnTo
}
}
redirectURL = lf.AppendTo(returnTo)
} else {
redirectURL = lf.AppendTo(s.d.Config().SelfServiceFlowLoginUI(r.Context()))
}
if dc, err := flow.DuplicateCredentials(lf); err == nil && dc != nil {
redirectURL = urlx.CopyWithQuery(redirectURL, url.Values{"no_org_ui": {"true"}})

Expand Down
55 changes: 40 additions & 15 deletions selfservice/strategy/oidc/strategy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ func TestStrategy(t *testing.T) {
return res, body
}

makeAPICodeFlowRequest := func(t *testing.T, provider, action string) (returnToCode string) {
makeAPICodeFlowRequest := func(t *testing.T, provider, action string) (returnToURL *url.URL) {
res, err := testhelpers.NewDebugClient(t).Post(action, "application/json", strings.NewReader(fmt.Sprintf(`{
"method": "oidc",
"provider": %q
Expand All @@ -197,13 +197,10 @@ func TestStrategy(t *testing.T) {
res, err = testhelpers.NewClientWithCookieJar(t, nil, true).Get(changeLocation.RedirectBrowserTo)
require.NoError(t, err)

returnToURL := res.Request.URL
returnToURL = res.Request.URL
assert.True(t, strings.HasPrefix(returnToURL.String(), returnTS.URL+"/app_code"))

code := returnToURL.Query().Get("code")
assert.NotEmpty(t, code, "code query param was empty in the return_to URL")

return code
return returnToURL
}

exchangeCodeForToken := func(t *testing.T, codes sessiontokenexchange.Codes) (codeResponse session.CodeExchangeResponse, err error) {
Expand Down Expand Up @@ -553,12 +550,15 @@ func TestStrategy(t *testing.T) {
t.Run("suite=API with session token exchange code", func(t *testing.T) {
scope = []string{"openid"}

loginOrRegister := func(t *testing.T, id uuid.UUID, code string) {
loginOrRegister := func(t *testing.T, flowID uuid.UUID, code string) {
_, err := exchangeCodeForToken(t, sessiontokenexchange.Codes{InitCode: code})
require.Error(t, err)

action := assertFormValues(t, id, "valid")
returnToCode := makeAPICodeFlowRequest(t, "valid", action)
action := assertFormValues(t, flowID, "valid")
returnToURL := makeAPICodeFlowRequest(t, "valid", action)
returnToCode := returnToURL.Query().Get("code")
assert.NotEmpty(t, code, "code query param was empty in the return_to URL")

codeResponse, err := exchangeCodeForToken(t, sessiontokenexchange.Codes{
InitCode: code,
ReturnToCode: returnToCode,
Expand All @@ -568,11 +568,11 @@ func TestStrategy(t *testing.T) {
assert.NotEmpty(t, codeResponse.Token)
assert.Equal(t, subject, gjson.GetBytes(codeResponse.Session.Identity.Traits, "subject").String())
}
register := func(t *testing.T) {
performRegistration := func(t *testing.T) {
f := newAPIRegistrationFlow(t, returnTS.URL+"?return_session_token_exchange_code=true&return_to=/app_code", 1*time.Minute)
loginOrRegister(t, f.ID, f.SessionTokenExchangeCode)
}
login := func(t *testing.T) {
performLogin := func(t *testing.T) {
f := newAPILoginFlow(t, returnTS.URL+"?return_session_token_exchange_code=true&return_to=/app_code", 1*time.Minute)
loginOrRegister(t, f.ID, f.SessionTokenExchangeCode)
}
Expand All @@ -582,23 +582,48 @@ func TestStrategy(t *testing.T) {
first, then func(*testing.T)
}{{
name: "login-twice",
first: login, then: login,
first: performLogin, then: performLogin,
}, {
name: "login-then-register",
first: login, then: register,
first: performLogin, then: performRegistration,
}, {
name: "register-then-login",
first: register, then: login,
first: performRegistration, then: performLogin,
}, {
name: "register-twice",
first: register, then: register,
first: performRegistration, then: performRegistration,
}} {
t.Run("case="+tc.name, func(t *testing.T) {
subject = tc.name + "[email protected]"
tc.first(t)
tc.then(t)
})
}
t.Run("case=should use redirect_to URL on failure", func(t *testing.T) {
ctx := context.Background()
subject = "[email protected]"

i := identity.NewIdentity(config.DefaultIdentityTraitsSchemaID)
i.SetCredentials(identity.CredentialsTypePassword, identity.Credentials{
Identifiers: []string{subject},
})
i.Traits = identity.Traits(`{"subject":"` + subject + `"}`)
require.NoError(t, reg.PrivilegedIdentityPool().CreateIdentity(ctx, i))

f := newAPILoginFlow(t, returnTS.URL+"?return_session_token_exchange_code=true&return_to=/app_code", 1*time.Minute)

_, err := exchangeCodeForToken(t, sessiontokenexchange.Codes{InitCode: f.SessionTokenExchangeCode})
require.Error(t, err)

action := assertFormValues(t, f.ID, "valid")
returnToURL := makeAPICodeFlowRequest(t, "valid", action)
returnedFlow := returnToURL.Query().Get("flow")

require.NotEmpty(t, returnedFlow, "flow query param was empty in the return_to URL")
loginFlow, err := reg.LoginFlowPersister().GetLoginFlow(ctx, uuid.FromStringOrNil(returnedFlow))
require.NoError(t, err)
assert.Equal(t, text.ErrorValidationDuplicateCredentials, loginFlow.UI.Messages[0].ID)
})
})

t.Run("case=submit id_token during registration or login", func(t *testing.T) {
Expand Down

0 comments on commit e8f1bcb

Please sign in to comment.