From 28b056e5bbfec645262914c52f0386d70c787a32 Mon Sep 17 00:00:00 2001 From: hackerman <3372410+aeneasr@users.noreply.github.com> Date: Mon, 4 Nov 2019 19:35:44 +0100 Subject: [PATCH] selfservice: Explicitly whitelist form parser keys (#105) Previously the form parser would try to detect the field type by asserting types for the whole form. That caused passwords containing only numbers to fail to unmarshal into a string value. This patch resolves that issue by introducing a prefix option to the BodyParser Closes #98 --- selfservice/body_decoder.go | 24 +++++++++++++++-------- selfservice/body_decoder_test.go | 14 +++++++++++-- selfservice/oidc/form.go | 4 +++- selfservice/password/registration.go | 2 +- selfservice/password/registration_test.go | 12 ++++++++++++ 5 files changed, 44 insertions(+), 12 deletions(-) diff --git a/selfservice/body_decoder.go b/selfservice/body_decoder.go index 2e8e53a5df26..67a0c5bbb93f 100644 --- a/selfservice/body_decoder.go +++ b/selfservice/body_decoder.go @@ -20,6 +20,10 @@ import ( type BodyDecoder struct{} +type BodyDecoderOptions struct { + AssertTypesForPrefix string +} + func NewBodyDecoder() *BodyDecoder { return &BodyDecoder{} } @@ -33,8 +37,8 @@ func (d BodyDecoder) json(r *http.Request) (json.RawMessage, error) { return p, nil } -func (d *BodyDecoder) DecodeForm(form url.Values, o interface{}) (err error) { - payload, err := d.form(form) +func (d *BodyDecoder) DecodeForm(form url.Values, o interface{}, opts BodyDecoderOptions) (err error) { + payload, err := d.form(form, opts) if err != nil { return err } @@ -83,12 +87,16 @@ func (d *BodyDecoder) ParseFormField(values []string) (typed interface{}, err er return typed, err } -func (d *BodyDecoder) form(form url.Values) (json.RawMessage, error) { +func (d *BodyDecoder) form(form url.Values, opts BodyDecoderOptions) (json.RawMessage, error) { + var err error payload := []byte("{}") for k := range form { - typed, err := d.ParseFormField(form[k]) - if err != nil { - return nil, err + var typed interface{} = form.Get(k) + + if len(opts.AssertTypesForPrefix) == 0 || strings.HasPrefix(k, opts.AssertTypesForPrefix) { + if typed, err = d.ParseFormField(form[k]); err != nil { + return nil, err + } } payload, err = sjson.SetBytes(payload, k, typed) @@ -100,7 +108,7 @@ func (d *BodyDecoder) form(form url.Values) (json.RawMessage, error) { return payload, nil } -func (d *BodyDecoder) Decode(r *http.Request, o interface{}) (err error) { +func (d *BodyDecoder) Decode(r *http.Request, o interface{}, opts BodyDecoderOptions) (err error) { ct, _, err := mime.ParseMediaType(r.Header.Get("Content-Type")) if err != nil { return errors.WithStack(herodot.ErrBadRequest.WithDebug(err.Error()).WithReasonf("Unable to parse HTTP request content type: %s", err.Error())) @@ -113,7 +121,7 @@ func (d *BodyDecoder) Decode(r *http.Request, o interface{}) (err error) { if err := r.ParseForm(); err != nil { return errors.WithStack(herodot.ErrBadRequest.WithDebug(err.Error()).WithReasonf("Unable to parse HTTP form request: %s", err.Error())) } - p, err = d.form(r.PostForm) + p, err = d.form(r.PostForm, opts) } if err != nil { diff --git a/selfservice/body_decoder_test.go b/selfservice/body_decoder_test.go index 94b1a9787d71..887b035a1c20 100644 --- a/selfservice/body_decoder_test.go +++ b/selfservice/body_decoder_test.go @@ -22,6 +22,7 @@ func TestNewBodyDecoder(t *testing.T) { payload url.Values raw string result string + opt BodyDecoderOptions }{ { d: "should work with nested keys", @@ -31,6 +32,15 @@ func TestNewBodyDecoder(t *testing.T) { }, result: `{"request":"bar","traits":{"foo":"bar"}}`, }, + { + d: "should ignore unless prefix is set", + payload: url.Values{ + "traits.foo": {"12345"}, + "password": {"12345"}, + }, + opt: BodyDecoderOptions{AssertTypesForPrefix: "traits."}, + result: `{"password":"12345","traits":{"foo":12345}}`, + }, { d: "should work with true and false", raw: "traits.consent.newsletter=false&traits.consent.newsletter=true&traits.consent.tos=false", @@ -40,7 +50,7 @@ func TestNewBodyDecoder(t *testing.T) { t.Run(fmt.Sprintf("case=%d/description=%s", k, tc.d), func(t *testing.T) { ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { var result json.RawMessage - require.NoError(t, dec.Decode(r, &result)) + require.NoError(t, dec.Decode(r, &result, tc.opt)) require.JSONEq(t, tc.result, string(result), "%s", result) })) defer ts.Close() @@ -75,7 +85,7 @@ func TestNewBodyDecoder(t *testing.T) { t.Run(fmt.Sprintf("case=%d/description=%s", k, tc.d), func(t *testing.T) { ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { var result json.RawMessage - require.NoError(t, dec.Decode(r, &result)) + require.NoError(t, dec.Decode(r, &result, BodyDecoderOptions{})) require.JSONEq(t, tc.result, string(result), "%s", result) })) defer ts.Close() diff --git a/selfservice/oidc/form.go b/selfservice/oidc/form.go index 1f54914c0166..7406b5b79083 100644 --- a/selfservice/oidc/form.go +++ b/selfservice/oidc/form.go @@ -90,7 +90,9 @@ func merge(dc *selfservice.BodyDecoder, form string, traits json.RawMessage) (js Traits map[string]interface{} `json:"traits"` } - if err := dc.DecodeForm(q, &decodedForm); err != nil { + if err := dc.DecodeForm(q, &decodedForm, selfservice.BodyDecoderOptions{ + AssertTypesForPrefix: "traits.", + }); err != nil { return nil, err } diff --git a/selfservice/password/registration.go b/selfservice/password/registration.go index de02114d9f89..fddb1c6ecebc 100644 --- a/selfservice/password/registration.go +++ b/selfservice/password/registration.go @@ -65,7 +65,7 @@ func (s *Strategy) handleRegistration(w http.ResponseWriter, r *http.Request, _ } var p RegistrationFormPayload - if err := s.dec.Decode(r, &p); err != nil { + if err := s.dec.Decode(r, &p, selfservice.BodyDecoderOptions{AssertTypesForPrefix: "traits."}); err != nil { s.handleRegistrationError(w, r, ar, err) return } diff --git a/selfservice/password/registration_test.go b/selfservice/password/registration_test.go index d1f8ec22e0f8..a52163d90d27 100644 --- a/selfservice/password/registration_test.go +++ b/selfservice/password/registration_test.go @@ -241,6 +241,18 @@ func TestRegistration(t *testing.T) { assert.Empty(t, gjson.GetBytes(body, "methods.password.config.error")) assert.Contains(t, gjson.GetBytes(body, "methods.password.config.fields.traits\\.foobar.error").String(), "foobar is required", "%s", body) }) + + t.Run("case=should work even if password is just numbers", func(t *testing.T) { + viper.Set(configuration.ViperKeyDefaultIdentityTraitsSchemaURL, "file://./stub/registration.schema.json") + rr := newRegistrationRequest(t, time.Minute) + body, res := makeRequest(t, rr.ID, url.Values{ + "traits.username": {"registration-identifier-10"}, + "password": {"93172388957812344432"}, + "traits.foobar": {"bar"}, + }.Encode(), http.StatusOK) + assert.Contains(t, res.Request.URL.Path, "return-ts") + assert.Equal(t, `registration-identifier-10`, gjson.GetBytes(body, "identity.traits.username").String(), "%s", body) + }) }) t.Run("method=PopulateSignUpMethod", func(t *testing.T) {