Repository: ory/kratos (go)

Issue:
Registration/Login form fields should not include "request"
**Describe the bug**

The request field is send in the registration/login form information.

**Expected behavior**

As the request ID is already included in the action url we should filter out this key from the form fields.


Pull request:
Registration/Login HTML form: remove request field and ensure method is set
## Related issue

closes #178 

## Proposed changes

The request field will not be added any more. The tests make sure it is not present.
We also check from now on that the form method is always set.

## Checklist

<!--
Put an `x` in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of
them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.
-->

- [ ] I have read the [contributing guidelines](../blob/master/CONTRIBUTING.md)
- [ ] I have read the [security policy](../security/policy)
- [ ] I confirm that this pull request does not address a security vulnerability. If this pull request addresses a security
vulnerability, I confirm that I got green light (please contact [security@ory.sh](mailto:security@ory.sh)) from the maintainers to push the changes.
- [ ] I have added tests that prove my fix is effective or that my feature works
- [ ] I have added necessary documentation within the code base (if appropriate)
- [ ] I have documented my changes in the [developer guide](https://github.com/ory/docs) (if appropriate)

## Further comments



Patch:
diff --git a/cmd/serve.go b/cmd/serve.go
index 6091cbe82..9b5fc819f 100644
--- a/cmd/serve.go
+++ b/cmd/serve.go
@@ -15,10 +15,11 @@
 package cmd
 
 import (
-	"github.com/ory/x/flagx"
 	"os"
 	"strconv"
 
+	"github.com/ory/x/flagx"
+
 	"github.com/spf13/cobra"
 
 	"github.com/ory/kratos/cmd/daemon"
diff --git a/driver/configuration/provider_viper.go b/driver/configuration/provider_viper.go
index b856b1bc9..582d4a210 100644
--- a/driver/configuration/provider_viper.go
+++ b/driver/configuration/provider_viper.go
@@ -24,8 +24,8 @@ import (
 )
 
 type ViperProvider struct {
-	l  logrus.FieldLogger
-	ss [][]byte
+	l   logrus.FieldLogger
+	ss  [][]byte
 	dev bool
 }
 
@@ -74,7 +74,7 @@ const (
 
 func NewViperProvider(l logrus.FieldLogger, dev bool) *ViperProvider {
 	return &ViperProvider{
-		l: l,
+		l:   l,
 		dev: dev,
 	}
 }
diff --git a/go.mod b/go.mod
index 363aeaeb3..de1f096ec 100644
--- a/go.mod
+++ b/go.mod
@@ -10,6 +10,7 @@ require (
 	github.com/cenkalti/backoff v2.1.1+incompatible
 	github.com/coreos/go-oidc v2.1.0+incompatible
 	github.com/fsnotify/fsnotify v1.4.7
+	github.com/ghodss/yaml v1.0.0
 	github.com/go-errors/errors v1.0.1
 	github.com/go-openapi/errors v0.19.2
 	github.com/go-openapi/runtime v0.19.5
diff --git a/selfservice/flow/login/request_method.go b/selfservice/flow/login/request_method.go
index 56bb4e6ee..f2d36102d 100644
--- a/selfservice/flow/login/request_method.go
+++ b/selfservice/flow/login/request_method.go
@@ -75,7 +75,7 @@ func (c *RequestMethodConfig) Value() (driver.Value, error) {
 }
 
 func (c *RequestMethodConfig) UnmarshalJSON(data []byte) error {
-	c.RequestMethodConfigurator = new(form.HTMLForm)
+	c.RequestMethodConfigurator = form.NewHTMLForm("")
 	return json.Unmarshal(data, c.RequestMethodConfigurator)
 }
 
diff --git a/selfservice/flow/profile/request.go b/selfservice/flow/profile/request.go
index 5ce15c000..3178f5ccc 100644
--- a/selfservice/flow/profile/request.go
+++ b/selfservice/flow/profile/request.go
@@ -80,7 +80,7 @@ func NewRequest(exp time.Duration, r *http.Request, s *session.Session) *Request
 		RequestURL: source.String(),
 		IdentityID: s.Identity.ID,
 		Identity:   s.Identity,
-		Form:       new(form.HTMLForm),
+		Form:       form.NewHTMLForm(""),
 	}
 }
 
diff --git a/selfservice/flow/registration/request_method.go b/selfservice/flow/registration/request_method.go
index 0f4cf8ad1..7a7480969 100644
--- a/selfservice/flow/registration/request_method.go
+++ b/selfservice/flow/registration/request_method.go
@@ -76,7 +76,7 @@ func (c *RequestMethodConfig) Value() (driver.Value, error) {
 }
 
 func (c *RequestMethodConfig) UnmarshalJSON(data []byte) error {
-	c.RequestMethodConfigurator = new(form.HTMLForm)
+	c.RequestMethodConfigurator = form.NewHTMLForm("")
 	return json.Unmarshal(data, c.RequestMethodConfigurator)
 }
 
diff --git a/selfservice/form/fields.go b/selfservice/form/fields.go
index fe6430b51..4ab90e7ab 100644
--- a/selfservice/form/fields.go
+++ b/selfservice/form/fields.go
@@ -5,7 +5,7 @@ import (
 	"fmt"
 )
 
-// Fields contains multiple fields asdfasdf
+// Fields contains multiple fields
 //
 // swagger:model formFields
 type Fields map[string]Field
@@ -15,18 +15,12 @@ type Fields map[string]Field
 // swagger:model formField
 type Field struct {
 	// Name is the equivalent of <input name="{{.Name}}">
-	//
-	// Extensions:
-	// ---
-	// x-go-name: Asdffsdafsad
-	// x-go-asdffasd: Asdffsdafsad
-	// ---
 	Name string `json:"name"`
-	// Name is the equivalent of <input type="{{.Type}}">
+	// Type is the equivalent of <input type="{{.Type}}">
 	Type string `json:"type,omitempty"`
-	// Name is the equivalent of <input required="{{.Required}}">
+	// Required is the equivalent of <input required="{{.Required}}">
 	Required bool `json:"required,omitempty"`
-	// Name is the equivalent of <input value="{{.Value}}">
+	// Value is the equivalent of <input value="{{.Value}}">
 	Value interface{} `json:"value,omitempty" faker:"name"`
 	// Errors contains all validation errors this particular field has caused.
 	Errors []Error `json:"errors,omitempty"`
diff --git a/selfservice/strategy/password/login.go b/selfservice/strategy/password/login.go
index 4404e4336..7c58cb372 100644
--- a/selfservice/strategy/password/login.go
+++ b/selfservice/strategy/password/login.go
@@ -35,7 +35,6 @@ func (s *Strategy) handleLoginError(w http.ResponseWriter, r *http.Request, rr *
 		if method, ok := rr.Methods[identity.CredentialsTypePassword]; ok {
 			method.Config.Reset()
 			method.Config.SetValue("identifier", r.PostForm.Get("identifier"))
-			method.Config.SetValue("request", r.PostForm.Get("request"))
 			method.Config.SetCSRF(s.cg(r))
 			rr.Methods[identity.CredentialsTypePassword] = method
 		}
@@ -118,6 +117,7 @@ func (s *Strategy) PopulateLoginMethod(r *http.Request, sr *login.Request) error
 
 	f := &form.HTMLForm{
 		Action: action.String(),
+		Method: "POST",
 		Fields: form.Fields{
 			"identifier": {
 				Name:     "identifier",
diff --git a/selfservice/strategy/password/registration.go b/selfservice/strategy/password/registration.go
index d9b8ee2db..2c757e23b 100644
--- a/selfservice/strategy/password/registration.go
+++ b/selfservice/strategy/password/registration.go
@@ -68,14 +68,7 @@ func (s *Strategy) handleRegistrationError(w http.ResponseWriter, r *http.Reques
 				}
 			}
 
-			method.Config.SetField("request", form.Field{
-				Name:     "request",
-				Type:     "hidden",
-				Required: true,
-				Value:    r.PostForm.Get("request"),
-			})
 			method.Config.SetCSRF(s.cg(r))
-
 			rr.Methods[identity.CredentialsTypePassword] = method
 		}
 	}


Test patch:
diff --git a/selfservice/strategy/password/login_test.go b/selfservice/strategy/password/login_test.go
index ea188e539..54121fa38 100644
--- a/selfservice/strategy/password/login_test.go
+++ b/selfservice/strategy/password/login_test.go
@@ -44,6 +44,7 @@ func nlr(exp time.Duration) *login.Request {
 				Method: identity.CredentialsTypePassword,
 				Config: &login.RequestMethodConfig{
 					RequestMethodConfigurator: &form.HTMLForm{
+						Method: "POST",
 						Action: "/action",
 						Fields: form.Fields{
 							"identifier": {
@@ -62,12 +63,6 @@ func nlr(exp time.Duration) *login.Request {
 								Required: true,
 								Value:    "anti-rf-token",
 							},
-							"request": {
-								Name:     "request",
-								Type:     "hidden",
-								Required: true,
-								Value:    id.String(),
-							},
 						},
 					},
 				},
@@ -84,12 +79,9 @@ type loginStrategyDependencies interface {
 
 func TestLogin(t *testing.T) {
 	var ensureFieldsExist = func(t *testing.T, body []byte) {
-		for _, k := range []string{"identifier",
+		checkFormContent(t, body, "identifier",
 			"password",
-			"csrf_token",
-		} {
-			assert.Equal(t, k, gjson.GetBytes(body, fmt.Sprintf("methods.password.config.fields.%s.name", k)).String())
-		}
+			"csrf_token")
 	}
 
 	type testCase struct {
@@ -304,6 +296,7 @@ func TestLogin(t *testing.T) {
 						Config: &login.RequestMethodConfig{
 							RequestMethodConfigurator: &password.RequestMethod{
 								HTMLForm: &form.HTMLForm{
+									Method: "POST",
 									Action: "/action",
 									Errors: []form.Error{{Message: "some error"}},
 									Fields: form.Fields{
diff --git a/selfservice/strategy/password/registration_test.go b/selfservice/strategy/password/registration_test.go
index 361583410..ab73cacb0 100644
--- a/selfservice/strategy/password/registration_test.go
+++ b/selfservice/strategy/password/registration_test.go
@@ -30,14 +30,31 @@ import (
 	"github.com/ory/kratos/x"
 )
 
+func checkFormContent(t *testing.T, body []byte, requiredFields ...string) {
+	fieldNameSet(t, body, requiredFields)
+	outdatedFieldsDoNotExist(t, body)
+	formMethodIsPOST(t, body)
+}
+
 // fieldNameSet checks if the fields have the right "name" set.
-func fieldNameSet(t *testing.T, body []byte, fields ...string) {
+func fieldNameSet(t *testing.T, body []byte, fields []string) {
 	for _, f := range fields {
 		fieldid := strings.Replace(f, ".", "\\.", -1) // we need to escape this because otherwise json path will interpret this as a nested object (it is not).
 		assert.Equal(t, gjson.GetBytes(body, fmt.Sprintf("methods.password.config.fields.%s.name", fieldid)).String(), f, "%s", body)
 	}
 }
 
+// checks if some keys are not set, this should be used to catch regression issues
+func outdatedFieldsDoNotExist(t *testing.T, body []byte) {
+	for _, k := range []string{"request"} {
+		assert.Equal(t, false, gjson.GetBytes(body, fmt.Sprintf("methods.password.config.fields.%s", k)).Exists())
+	}
+}
+
+func formMethodIsPOST(t *testing.T, body []byte) {
+	assert.Equal(t, "POST", gjson.GetBytes(body, "methods.password.config.method").String())
+}
+
 func TestRegistration(t *testing.T) {
 	t.Run("case=registration", func(t *testing.T) {
 		_, reg := internal.NewRegistryDefault(t)
@@ -77,6 +94,7 @@ func TestRegistration(t *testing.T) {
 						Config: &registration.RequestMethodConfig{
 							RequestMethodConfigurator: password.RequestMethod{
 								HTMLForm: &form.HTMLForm{
+									Method: "POST",
 									Action: "/action",
 									Fields: form.Fields{
 										"password":   {Name: "password", Type: "password", Required: true},
@@ -141,7 +159,7 @@ func TestRegistration(t *testing.T) {
 			assert.Contains(t, res.Request.URL.Path, "signup-ts")
 			assert.Equal(t, rr.ID.String(), gjson.GetBytes(body, "id").String(), "%s", body)
 			assert.Equal(t, "/action", gjson.GetBytes(body, "methods.password.config.action").String(), "%s", body)
-			fieldNameSet(t, body, "password", "csrf_token", "traits.username", "traits.foobar")
+			checkFormContent(t, body, "password", "csrf_token", "traits.username", "traits.foobar")
 			assert.Contains(t, gjson.GetBytes(body, "methods.password.config.fields.password.errors.0").String(), "data breaches and must no longer be used.", "%s", body)
 		})
 
@@ -154,7 +172,7 @@ func TestRegistration(t *testing.T) {
 			assert.Contains(t, res.Request.URL.Path, "signup-ts")
 			assert.Equal(t, rr.ID.String(), gjson.GetBytes(body, "id").String(), "%s", body)
 			assert.Equal(t, "/action", gjson.GetBytes(body, "methods.password.config.action").String(), "%s", body)
-			fieldNameSet(t, body, "password", "csrf_token", "traits.username", "traits.foobar")
+			checkFormContent(t, body, "password", "csrf_token", "traits.username", "traits.foobar")
 			assert.Contains(t, gjson.GetBytes(body, "methods.password.config.fields.traits\\.foobar.errors.0").String(), "foobar is required", "%s", body)
 		})
 
@@ -222,6 +240,7 @@ func TestRegistration(t *testing.T) {
 						Config: &registration.RequestMethodConfig{
 							RequestMethodConfigurator: &password.RequestMethod{
 								HTMLForm: &form.HTMLForm{
+									Method: "POST",
 									Action: "/action",
 									Errors: []form.Error{{Message: "some error"}},
 									Fields: form.Fields{
@@ -246,7 +265,7 @@ func TestRegistration(t *testing.T) {
 			assert.Contains(t, res.Request.URL.Path, "signup-ts")
 			assert.Equal(t, rr.ID.String(), gjson.GetBytes(body, "id").String(), "%s", body)
 			assert.Equal(t, "/action", gjson.GetBytes(body, "methods.password.config.action").String(), "%s", body)
-			fieldNameSet(t, body, "password", "csrf_token", "traits.username")
+			checkFormContent(t, body, "password", "csrf_token", "traits.username")
 
 			assert.Empty(t, gjson.GetBytes(body, "methods.password.config.fields.traits\\.foo.value"), "%s", body)
 			assert.Empty(t, gjson.GetBytes(body, "methods.password.config.fields.traits\\.foo.error"))