From b6b7ac75342af46ed624965e58be4df8cd2dcb2f Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Thu, 22 Oct 2026 03:03:18 +0200 Subject: [PATCH] fix: registration fills only an explicit allow-list and never takes the IP columns from the body User.RegisterFillable lists what a registration body may set: name, surname, email, password, preferred_locale and marketing_consent. RegisterUser fills from it instead of Fillable, so organisation_id, organisation_role, is_onboarded and must_change_password can no longer be mass-assigned through POST /register, and created_ip_address and last_ip_address come only from RegisterOptions.IP. Fillable is unchanged for the admin form and trusted application code. --- README.md | 8 ++-- controllers/registration.go | 33 ++++++++++------ models/user.go | 22 ++++++++++- register_test.go | 75 +++++++++++++++++++++++++++++++++++++ 4 files changed, 121 insertions(+), 17 deletions(-) diff --git a/README.md b/README.md index dbcf1c6..f8c9e47 100644 --- a/README.md +++ b/README.md @@ -16,7 +16,7 @@ It owns the `users` table extensions, `user_throttle`, `jwt_blacklist`, `user_ap ## Features -- **Registration and activation.** `POST /register`, `POST /activate`, `POST /activate-by-code`. Registration can be switched off and throttled; `activate_mode` is `auto`, `user` (a mailed code) or `admin`, with codes from `IssueActivationCode` and `VerifyActivationCode`. +- **Registration and activation.** `POST /register`, `POST /activate`, `POST /activate-by-code`. Registration can be switched off and throttled; `activate_mode` is `auto`, `user` (a mailed code) or `admin`, with codes from `IssueActivationCode` and `VerifyActivationCode`. A `/register` body sets only the `User.RegisterFillable` columns: `name`, `surname`, `email`, `password` (stored as its hash), `preferred_locale` and `marketing_consent`. It cannot set `organisation_id` or `organisation_role` (membership is the administrator's or the application's), `is_onboarded`, `must_change_password`, or the activation, reset, address proof, handle and permission columns; other keys are ignored (they still reach `RegisterEvent.Payload`). `created_ip_address` and `last_ip_address` are server-set from the client address only. - **JWT sessions.** `POST /login`, `GET /fetch`, `POST /refresh`, `POST /logout`. Boot registers the `jwt` guard with the framework's `bouncer.Registry` and exposes it as the `jwt.auth` middleware for other plugins. Logged-out tokens go to the Postgres `jwt_blacklist`, which a background sweep prunes. - **Password reset and change.** `POST /forgot-password` answers the same body for known and unknown addresses; `POST /reset-password` consumes a code from `IssueResetCode`. `POST /change-password` requires the current password. - **Must-change-password lock.** The `user:require-password-change` console command flags an account. The flag rides on the authenticated principal (`bouncer.Principal.MustChangePassword`) for guards and other plugins to act on, and a successful `POST /change-password` clears it. @@ -168,7 +168,7 @@ Migrations added for address proof, all additive: Packages: - `controllers`: the route handlers above, `AdminControllers` (the admin controller list), `PermissionAccessUsers` and `PermissionAccessGroups`, plus the registration steps `POST /register` runs, for other plugins: - - `RegisterUser(ctx, app, db, fields, RegisterOptions)` validates `fields` against the user model's rules, hashes the password with `golem15.user.password.bcrypt_cost` and creates the user on `db` (which may be a transaction). It returns the validation errors instead of writing a response and never modifies `fields`. `RegisterOptions.Activate` creates the account activated; `RegisterOptions.IP` records the client IP; `RegisterOptions.PasswordLess` stores `has_self_set_password=false` for an account whose random password the user never knew (a social sign-in), while the zero value keeps `true`. + - `RegisterUser(ctx, app, db, fields, RegisterOptions)` validates `fields` against the user model's rules, hashes the password with `golem15.user.password.bcrypt_cost` and creates the user on `db` (which may be a transaction). It fills only the `User.RegisterFillable` columns and ignores every other key of `fields`: an organisation membership and role are set with an explicit update after creation, as an application's onboarding does. It returns the validation errors instead of writing a response and never modifies `fields`. `RegisterOptions.Activate` creates the account activated; `RegisterOptions.IP` records the client IP and is the only source of `created_ip_address` and `last_ip_address` (both stay NULL without it, whatever `fields` says); `RegisterOptions.PasswordLess` stores `has_self_set_password=false` for an account whose random password the user never knew (a social sign-in), while the zero value keeps `true`. - `FireRegisterEvent(ctx, app, user, fields)` fires `RegisterEvent` with `Payload` set to a copy of `fields` without `password` and `password_confirmation`, and returns the listeners' error (`/register` ignores it). Fire it after the user row is committed. - `FireCreatedEvent(ctx, app, user)` fires `CreatedEvent` (`user.Created`). `/register` ignores that error. - `FireDeletingEvent(ctx, app, user, tx)` fires `DeletingEvent` (`user.Deleting`) inside the delete transaction. @@ -188,7 +188,7 @@ Packages: - `AvatarURL(ctx, app, user, size)`, the display picture rule of the user payload at any size: the avatar thumbnail (size by size, mode auto) when the user has an avatar and a file bucket is published, the file path when the thumbnail fails, otherwise the gravatar URL. The URL may be protocol-relative or relative. The payload's `avatar_url` comes from it at 128. - `HandleGenerator`, `AssignHandle(ctx, db, user)`, `CreatedEvent` / `CreatedEventName`, `DeletingEvent` / `DeletingEventName`, `DeactivatingEvent` / `DeactivatingEventName`, `AuthorizeUser` and `RegisterUserAuthorizer`. - `middleware`: `APITokenGuard`, `Scope`, `Permission` and `BearerToken`. -- `models`: `User`, `Throttle`, `Organisation`, `UserGroup`, `UsersGroup` (the `users_groups` pivot), `APIToken`, `OAuthIdentity`, `FrontendPermission` and `PermissionSet`. `PermissionSet` reads a stored permission column tolerantly (NULL, an empty string, `[]`, numeric strings) and writes a JSON object of integers; `ParsePermissionSet` and `PermissionSet.Text` do the same for a column that is a plain string on its model. `UserGroup.UsersCount` is filled only by a query that selects `users_count` (the admin list does). `Organisation.Members` is the relation behind the members manager, `Organisation.AttachRelations` declares its avatar (`OrganisationAvatarField`), and `Slugify` is the slug rule. `User.FilterScope` answers the admin list scope `FilterByGroup`, and `User.AttachRelations` declares the avatar (`AvatarField`). +- `models`: `User`, `Throttle`, `Organisation`, `UserGroup`, `UsersGroup` (the `users_groups` pivot), `APIToken`, `OAuthIdentity`, `FrontendPermission` and `PermissionSet`. `PermissionSet` reads a stored permission column tolerantly (NULL, an empty string, `[]`, numeric strings) and writes a JSON object of integers; `ParsePermissionSet` and `PermissionSet.Text` do the same for a column that is a plain string on its model. `UserGroup.UsersCount` is filled only by a query that selects `users_count` (the admin list does). `Organisation.Members` is the relation behind the members manager, `Organisation.AttachRelations` declares its avatar (`OrganisationAvatarField`), and `Slugify` is the slug rule. `User.FilterScope` answers the admin list scope `FilterByGroup`, and `User.AttachRelations` declares the avatar (`AvatarField`). `User.RegisterFillable` is the registration allow-list; `User.Fillable` is the wider mass-assignment backstop of the admin Users form (cabana intersects it with the form's fields) and of trusted application code. - `updates`: the gormigrate migrations, returned by `updates.All()`. - `console`: `RequirePasswordChange`, `BackfillHandles` and `APITokenCommands`. @@ -248,4 +248,6 @@ go test ./... # full run, starts Postgres with testcontainers (needs D `TestCommandsDocumented` (in `-short`) logs, without failing, each command whose help lacks a description, long help, examples or an option description (`bonfire.CheckDocs`). +`TestRegisterIgnoresProtectedFields` posts organisation, IP and account-flag keys to `/register` and `RegisterUser` and checks that only the allow-listed columns and the server-side IP are stored. + The admin tests (`TestAdmin*`) boot the plugin with the admin API mounted on a database of their own and sign in backend administrators with a chosen permission set; the helpers are in `admin_harness_test.go`. diff --git a/controllers/registration.go b/controllers/registration.go index 999e567..bc35863 100644 --- a/controllers/registration.go +++ b/controllers/registration.go @@ -22,7 +22,9 @@ import ( // RegisterOptions tunes RegisterUser. Activate creates the account already // activated (activate_mode auto, or a caller that activates explicitly). -// IP, when set, is recorded as created_ip_address and last_ip_address. +// IP, when set, is recorded as created_ip_address and last_ip_address; it is +// the only source of those columns, so with an empty IP they stay NULL +// whatever the fields say. type RegisterOptions struct { Activate bool IP string @@ -34,12 +36,16 @@ type RegisterOptions struct { } // RegisterUser validates fields against the user model's rules, hashes the -// password with golem15.user.password.bcrypt_cost (default 10), fills the -// fillable columns and creates the users row on db, which may be a -// transaction. Validation failures are returned as Laravel's errors object -// (attribute to messages) with a nil user and nothing written; they are not -// written to a response. fields is not modified. The register event is not -// fired: call FireRegisterEvent once the row is committed. +// password with golem15.user.password.bcrypt_cost (default 10), fills only +// the models.User RegisterFillable columns (name, surname, email, password, +// preferred_locale, marketing_consent) and creates the users row on db, +// which may be a transaction. Any other key in fields is ignored: an +// organisation membership and role are set with an explicit update after +// creation, as an application's onboarding does, and the IP columns come +// only from opts.IP. Validation failures are returned as Laravel's errors +// object (attribute to messages) with a nil user and nothing written; they +// are not written to a response. fields is not modified. The register event +// is not fired: call FireRegisterEvent once the row is committed. func RegisterUser(ctx context.Context, app *backpack.App, db *gorm.DB, fields map[string]any, opts RegisterOptions) (*models.User, map[string][]string, error) { if db == nil { return nil, nil, errors.New("golem15.user: gorm handle is missing") @@ -64,15 +70,18 @@ func RegisterUser(ctx context.Context, app *backpack.App, db *gorm.DB, fields ma return nil, nil, err } input["password"] = hashed - if opts.IP != "" { - input["created_ip_address"] = opts.IP - input["last_ip_address"] = opts.IP - } user := &models.User{} production := app == nil || app.Config == nil || !app.Config.Bool("app.debug") - if err := lagoon.Fill(user, user.Fillable(), input, production); err != nil { + if err := lagoon.Fill(user, user.RegisterFillable(), input, production); err != nil { return nil, nil, err } + // The IP columns are server-set after the fill, so a body value can never + // reach them. + if opts.IP != "" { + created, last := opts.IP, opts.IP + user.CreatedIPAddress = &created + user.LastIPAddress = &last + } user.HasSelfSetPassword = !opts.PasswordLess if opts.Activate { user.IsActivated = true diff --git a/models/user.go b/models/user.go index 4846eaa..3c2f36b 100644 --- a/models/user.go +++ b/models/user.go @@ -81,8 +81,12 @@ func (User) AttachRelations() []attach.Relation { return []attach.Relation{{Name: AvatarField, Public: true}} } -// Fillable is the register/update field list. Username, PIN, and GDPR columns -// stay out of Phase 7 because no ported handler reads them. +// Fillable is the mass-assignment backstop of the admin Users form and of +// trusted application code: cabana intersects it with the form's writable +// fields (and an application's form extensions) and keeps organisation_id +// protected. Registration does not use it: RegisterUser fills only +// RegisterFillable. Username, PIN and GDPR columns stay out because no +// ported handler reads them. func (User) Fillable() []string { return []string{ "name", "surname", "email", "password", "created_ip_address", "last_ip_address", @@ -91,6 +95,20 @@ func (User) Fillable() []string { } } +// RegisterFillable is the explicit allow-list of the columns a registration +// body may set (POST /register and controllers.RegisterUser): name, surname, +// email, password (stored as its hash), preferred_locale and +// marketing_consent. A registration body can never set organisation_id or +// organisation_role (membership is the administrator's or the +// application's), created_ip_address or last_ip_address (server-set from +// RegisterOptions.IP only), is_onboarded (application-owned onboarding +// state) or must_change_password (set by an administrator or the +// require-password-change command). The activation, reset, address proof, +// handle and permission columns are never fillable at all. +func (User) RegisterFillable() []string { + return []string{"name", "surname", "email", "password", "preferred_locale", "marketing_consent"} +} + // Hidden mirrors PHP User::$hidden for the secrets this model stores. func (User) Hidden() []string { return []string{"password", "reset_password_code", "activation_code"} diff --git a/register_test.go b/register_test.go index e1c4117..90c1039 100644 --- a/register_test.go +++ b/register_test.go @@ -248,6 +248,81 @@ func TestRegisterEventPayload(t *testing.T) { } } +// TestRegisterIgnoresProtectedFields: a registration body sets only the +// RegisterFillable columns. Organisation membership, the IP columns and the +// account flags in the body are ignored, over HTTP and through RegisterUser, +// and the IP columns come only from the server-side address. +func TestRegisterIgnoresProtectedFields(t *testing.T) { + app, gdb := sessionApp(t, "0s") + disableRegisterThrottle(t, app) + org := &models.Organisation{Name: "Mass Assign Org", Slug: "mass-assign-org"} + if err := gdb.Create(org).Error; err != nil { + t.Fatal(err) + } + protected := func(email string) map[string]any { + return map[string]any{ + "email": email, "password": "correct-horse", "password_confirmation": "correct-horse", "name": "Mal", "surname": "Lory", + "preferred_locale": "pl", "marketing_consent": true, + "organisation_id": org.ID, "organisation_role": "owner", + "created_ip_address": "203.0.113.9", "last_ip_address": "203.0.113.9", + "is_onboarded": true, "must_change_password": true, + } + } + check := func(t *testing.T, email string, wantIP *string) { + t.Helper() + var stored models.User + if err := gdb.Where("email = ?", email).Take(&stored).Error; err != nil { + t.Fatal(err) + } + if stored.Name == nil || *stored.Name != "Mal" || stored.Surname == nil || *stored.Surname != "Lory" || + stored.PreferredLocale == nil || *stored.PreferredLocale != "pl" || !stored.MarketingConsent { + t.Fatalf("allowed columns not stored: %+v", stored) + } + if stored.OrganisationID != nil || stored.OrganisationRole != nil { + t.Fatalf("organisation from the body: id %v role %v", stored.OrganisationID, stored.OrganisationRole) + } + if stored.IsOnboarded || stored.MustChangePassword { + t.Fatalf("account flags from the body: onboarded %v must_change %v", stored.IsOnboarded, stored.MustChangePassword) + } + for name, got := range map[string]*string{"created_ip_address": stored.CreatedIPAddress, "last_ip_address": stored.LastIPAddress} { + switch { + case wantIP == nil && got != nil: + t.Fatalf("%s = %q, want NULL", name, *got) + case wantIP != nil && (got == nil || *got != *wantIP): + t.Fatalf("%s = %v, want %q", name, got, *wantIP) + } + } + } + + t.Run("http", func(t *testing.T) { + body, err := json.Marshal(protected("mass-http@example.test")) + if err != nil { + t.Fatal(err) + } + rec := postRegister(t, app, "192.0.2.44", string(body)) + if rec.Code != http.StatusOK { + t.Fatalf("register %d %s", rec.Code, rec.Body.String()) + } + ip := "192.0.2.44" + check(t, "mass-http@example.test", &ip) + }) + t.Run("RegisterUser with IP", func(t *testing.T) { + _, errs, err := controllers.RegisterUser(t.Context(), app, gdb, protected("mass-ip@example.test"), controllers.RegisterOptions{IP: "198.51.100.7"}) + if err != nil || len(errs) != 0 { + t.Fatalf("RegisterUser = %v, %v", errs, err) + } + ip := "198.51.100.7" + check(t, "mass-ip@example.test", &ip) + }) + t.Run("RegisterUser without IP", func(t *testing.T) { + _, errs, err := controllers.RegisterUser(t.Context(), app, gdb, protected("mass-noip@example.test"), controllers.RegisterOptions{}) + if err != nil || len(errs) != 0 { + t.Fatalf("RegisterUser = %v, %v", errs, err) + } + check(t, "mass-noip@example.test", nil) + }) +} + func TestRegisterImportDirection(t *testing.T) { cmd := exec.Command("go", "list", "-deps", "./...") cmd.Dir = moduleRoot(t)