Skip to content

Commit

Permalink
fix: use and test for csrf tokens and prevent api misuse
Browse files Browse the repository at this point in the history
  • Loading branch information
aeneasr committed Aug 25, 2020
1 parent 1a6e847 commit a4e3bc5
Show file tree
Hide file tree
Showing 4 changed files with 37 additions and 9 deletions.
3 changes: 1 addition & 2 deletions selfservice/strategy/password/login.go
Expand Up @@ -6,7 +6,6 @@ import (
"net/http"

"github.com/julienschmidt/httprouter"
"github.com/justinas/nosurf"
"github.com/pkg/errors"

"github.com/ory/x/decoderx"
Expand Down Expand Up @@ -111,7 +110,7 @@ func (s *Strategy) handleLogin(w http.ResponseWriter, r *http.Request, _ httprou
return
}

if ar.Type == flow.TypeBrowser && !nosurf.VerifyToken(s.d.GenerateCSRFToken(r), p.CSRFToken) {
if err := flow.VerifyRequest(r,ar.Type,s.d.GenerateCSRFToken,p.CSRFToken); err != nil {
s.handleLoginError(w, r, ar, &p, x.ErrInvalidCSRFToken)
return
}
Expand Down
4 changes: 2 additions & 2 deletions selfservice/strategy/password/login_test.go
Expand Up @@ -210,15 +210,15 @@ func TestCompleteLogin(t *testing.T) {
require.Contains(t, res.Request.URL.Path, "error-ts")
assert.Equal(t, int64(http.StatusBadRequest), gjson.GetBytes(body, "0.code").Int(), "%s", body)
assert.Equal(t, "Bad Request", gjson.GetBytes(body, "0.status").String(), "%s", body)
assert.Contains(t, gjson.GetBytes(body, "0.reason").String(), "request query parameter is missing or invalid", "%s", body)
assert.Contains(t, gjson.GetBytes(body, "0.reason").String(), "flow query parameter is missing or invalid", "%s", body)
})

t.Run("type=api", func(t *testing.T) {
res, body := run(t, true)
require.Contains(t, res.Request.URL.Path, password.RouteLogin, res.Request.URL)
assert.Equal(t, int64(http.StatusBadRequest), gjson.GetBytes(body, "error.code").Int(), "%s", body)
assert.Equal(t, "Bad Request", gjson.GetBytes(body, "error.status").String(), "%s", body)
assert.Contains(t, gjson.GetBytes(body, "error.reason").String(), "request query parameter is missing or invalid", "%s", body)
assert.Contains(t, gjson.GetBytes(body, "error.reason").String(), "flow query parameter is missing or invalid", "%s", body)
})
})

Expand Down
16 changes: 11 additions & 5 deletions selfservice/strategy/password/registration.go
Expand Up @@ -8,14 +8,15 @@ import (
"github.com/pkg/errors"
"github.com/tidwall/sjson"

"github.com/ory/kratos/driver/configuration"

"github.com/ory/x/errorsx"

_ "github.com/ory/jsonschema/v3/fileloader"
_ "github.com/ory/jsonschema/v3/httploader"
"github.com/ory/x/decoderx"

"github.com/ory/kratos/driver/configuration"
"github.com/ory/kratos/selfservice/flow"

"github.com/ory/herodot"
"github.com/ory/x/urlx"

Expand All @@ -32,8 +33,9 @@ const (
)

type RegistrationFormPayload struct {
Password string `json:"password"`
Traits json.RawMessage `json:"traits"`
Password string `json:"password"`
Traits json.RawMessage `json:"traits"`
CSRFToken string `json:"csrf_token"`
}

func (s *Strategy) RegisterRegistrationRoutes(public *x.RouterPublic) {
Expand Down Expand Up @@ -94,7 +96,6 @@ func (s *Strategy) decoderRegistration() (decoderx.HTTPDecoderOption, error) {
return o, nil
}


// swagger:parameters completeSelfServiceRegistrationFlowWithPasswordMethod
type completeSelfServiceRegistrationFlowWithPasswordMethod struct {
// Flow is flow ID.
Expand Down Expand Up @@ -163,6 +164,11 @@ func (s *Strategy) handleRegistration(w http.ResponseWriter, r *http.Request, _
return
}

if err := flow.VerifyRequest(r,ar.Type,s.d.GenerateCSRFToken,p.CSRFToken); err != nil {
s.handleRegistrationError(w, r, ar, &p, err)
return
}

if len(p.Password) == 0 {
s.handleRegistrationError(w, r, ar, &p, schema.NewRequiredError("#/password", "password"))
return
Expand Down
23 changes: 23 additions & 0 deletions selfservice/strategy/password/registration_test.go
Expand Up @@ -285,6 +285,29 @@ func TestRegistration(t *testing.T) {
})
})

t.Run("case=should fail because of missing CSRF token/type=browser", func(t *testing.T) {
rr := newRegistrationRequest(t, time.Minute, false)
body, _ := makeRequest(t, rr.ID, false, url.Values{
"csrf_token": {"invalid_token"},
"traits.username": {"registration-identifier-csrf-browser"},
"password": {x.NewUUID().String()},
"traits.foobar": {"bar"},
}.Encode(), http.StatusOK)
assertx.EqualAsJSON(t, x.ErrInvalidCSRFToken,
json.RawMessage(gjson.GetBytes(body, "0").Raw), "%s", body)
})

t.Run("case=should pass even without CSRF token/type=api", func(t *testing.T) {
rr := newRegistrationRequest(t, time.Minute, true)
body, _ := makeRequest(t, rr.ID, true, `{
"csrf_token": "invalid_token",
"traits.username": "registration-identifier-csrf-api",
"traits.foobar": "bar",
"password": "5216f2ef-f14b-4c92-bd91-08c2c2fe1448"
}`, http.StatusOK)
assert.NotEmpty(t, gjson.GetBytes(body, "identity.id").Raw, "%s", body) // registration successful
})

t.Run("case=should fail because schema does not exist", func(t *testing.T) {
viper.Set(configuration.ViperKeyDefaultIdentitySchemaURL, "file://./stub/i-do-not-exist.schema.json")

Expand Down

0 comments on commit a4e3bc5

Please sign in to comment.