Skip to content

Commit

Permalink
fix: tolerate more "truthy" values when creating new flows (#3841)
Browse files Browse the repository at this point in the history
Use strconv.ParseBool to accept multiple "truthy" values for the
`refresh` and `return_session_token_exchange_code` query parameters when
creating a new login flow.

For some SDKs (e.g.: Python), these stringification of booleans is not
user-controlled and these endpoints could not be used fully due to the
backend ignoring any value other than `true` (all lowercase).

Closes #3839
  • Loading branch information
ngc7293 committed Mar 24, 2024
1 parent 60537a9 commit 49d93c0
Show file tree
Hide file tree
Showing 4 changed files with 40 additions and 10 deletions.
5 changes: 4 additions & 1 deletion selfservice/flow/login/flow.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"fmt"
"net/http"
"net/url"
"strconv"
"strings"
"time"

Expand Down Expand Up @@ -176,6 +177,8 @@ func NewFlow(conf *config.Config, exp time.Duration, csrf string, r *http.Reques
return nil, err
}

refresh, _ := strconv.ParseBool(r.URL.Query().Get("refresh"))

return &Flow{
ID: id,
OAuth2LoginChallenge: hydraLoginChallenge,
Expand All @@ -188,7 +191,7 @@ func NewFlow(conf *config.Config, exp time.Duration, csrf string, r *http.Reques
RequestURL: requestURL,
CSRFToken: csrf,
Type: flowType,
Refresh: r.URL.Query().Get("refresh") == "true",
Refresh: refresh,
RequestedAAL: identity.AuthenticatorAssuranceLevel(strings.ToLower(stringsx.Coalesce(
r.URL.Query().Get("aal"),
string(identity.AuthenticatorAssuranceLevel1)))),
Expand Down
28 changes: 24 additions & 4 deletions selfservice/flow/login/flow_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,24 @@ func TestNewFlow(t *testing.T) {
assert.Equal(t, identity.AuthenticatorAssuranceLevel1, r.RequestedAAL)
})

t.Run("type=refresh", func(t *testing.T) {
t.Run("case=refresh accepts any truthy value", func(t *testing.T) {
parameters := []string{"true", "True", "1"}

for _, refresh := range parameters {
r, err := login.NewFlow(conf, 0, "csrf", &http.Request{URL: &url.URL{Path: "/", RawQuery: fmt.Sprintf("refresh=%v", refresh)}, Host: "ory.sh"}, flow.TypeBrowser)
require.NoError(t, err)
assert.True(t, r.Refresh)
}
})

t.Run("case=refresh silently ignores invalid values", func(t *testing.T) {
r, err := login.NewFlow(conf, 0, "csrf", &http.Request{URL: &url.URL{Path: "/", RawQuery: "refresh=foo"}, Host: "ory.sh"}, flow.TypeBrowser)
require.NoError(t, err)
assert.False(t, r.Refresh)
})
})

t.Run("type=return_to", func(t *testing.T) {
_, err := login.NewFlow(conf, 0, "csrf", &http.Request{URL: &url.URL{Path: "/", RawQuery: "return_to=https://not-allowed/foobar"}, Host: "ory.sh"}, flow.TypeBrowser)
require.Error(t, err)
Expand All @@ -89,7 +107,8 @@ func TestNewFlow(t *testing.T) {
t.Run("case=regular flow creation", func(t *testing.T) {
r, err := login.NewFlow(conf, 0, "csrf", &http.Request{
URL: urlx.ParseOrPanic("https://ory.sh/"),
Host: "ory.sh"}, flow.TypeBrowser)
Host: "ory.sh",
}, flow.TypeBrowser)
require.NoError(t, err)
assert.Equal(t, "https://ory.sh/", r.RequestURL)
})
Expand All @@ -99,7 +118,8 @@ func TestNewFlow(t *testing.T) {
t.Run("case=flow with refresh", func(t *testing.T) {
r, err := login.NewFlow(conf, 0, "csrf", &http.Request{
URL: urlx.ParseOrPanic("/?refresh=true"),
Host: "ory.sh"}, flow.TypeAPI)
Host: "ory.sh",
}, flow.TypeAPI)
require.NoError(t, err)
assert.Equal(t, r.IssuedAt, r.ExpiresAt)
assert.Equal(t, flow.TypeAPI, r.Type)
Expand All @@ -110,7 +130,8 @@ func TestNewFlow(t *testing.T) {
t.Run("case=flow without refresh", func(t *testing.T) {
r, err := login.NewFlow(conf, 0, "csrf", &http.Request{
URL: urlx.ParseOrPanic("/"),
Host: "ory.sh"}, flow.TypeAPI)
Host: "ory.sh",
}, flow.TypeAPI)
require.NoError(t, err)
assert.Equal(t, r.IssuedAt, r.ExpiresAt)
assert.Equal(t, flow.TypeAPI, r.Type)
Expand All @@ -129,7 +150,6 @@ func TestNewFlow(t *testing.T) {
require.NoError(t, err)
assert.Equal(t, "8aadcb8fc1334186a84c4da9813356d9", string(r.OAuth2LoginChallenge))
})

}

func TestFlow(t *testing.T) {
Expand Down
5 changes: 4 additions & 1 deletion selfservice/flow/login/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package login
import (
"net/http"
"net/url"
"strconv"
"time"

"github.com/gofrs/uuid"
Expand Down Expand Up @@ -141,7 +142,9 @@ func (h *Handler) NewLoginFlow(w http.ResponseWriter, r *http.Request, ft flow.T
sess, err := h.d.SessionManager().FetchFromRequest(r.Context(), r)
if e := new(session.ErrNoActiveSessionFound); errors.As(err, &e) {
// No session exists yet
if ft == flow.TypeAPI && r.URL.Query().Get("return_session_token_exchange_code") == "true" {
returnSessionTokenExchangeCode, _ := strconv.ParseBool(r.URL.Query().Get("return_session_token_exchange_code"))

if ft == flow.TypeAPI && returnSessionTokenExchangeCode {
e, err := h.d.SessionTokenExchangePersister().CreateSessionTokenExchanger(r.Context(), f.ID)
if err != nil {
return nil, nil, errors.WithStack(herodot.ErrInternalServerError.WithWrap(err))
Expand Down
12 changes: 8 additions & 4 deletions selfservice/flow/login/handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -546,10 +546,14 @@ func TestFlowLifecycle(t *testing.T) {
assert.Empty(t, gjson.GetBytes(body, "session_token_exchange_code").String())
})

t.Run("case=returns session exchange code", func(t *testing.T) {
res, body := initFlow(t, urlx.ParseOrPanic("/?return_session_token_exchange_code=true").Query(), true)
assert.Contains(t, res.Request.URL.String(), login.RouteInitAPIFlow)
assert.NotEmpty(t, gjson.GetBytes(body, "session_token_exchange_code").String())
t.Run("case=returns session exchange code with any truthy value", func(t *testing.T) {
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())
}
})

t.Run("case=can not request refresh and aal at the same time on unauthenticated request", func(t *testing.T) {
Expand Down

0 comments on commit 49d93c0

Please sign in to comment.