From 48328bf33f5b3d136c554d8b3474c5e4a9b7be1f Mon Sep 17 00:00:00 2001 From: Gani Georgiev Date: Tue, 19 Nov 2024 15:59:59 +0200 Subject: [PATCH] added extra validations in case of id pattern validator misuse --- core/field_file.go | 2 +- core/field_text.go | 50 +++++++++++++++++++++++++++------ core/field_text_test.go | 62 +++++++++++++++++++++++++++++++++++++++-- 3 files changed, 103 insertions(+), 11 deletions(-) diff --git a/core/field_file.go b/core/field_file.go index 0dbf7f62..71930184 100644 --- a/core/field_file.go +++ b/core/field_file.go @@ -391,7 +391,7 @@ func (f *FileField) Intercept( } func (f *FileField) getLatestOldValue(app App, record *Record) any { if !record.IsNew() { - latestOriginal, err := app.FindRecordById(record.Collection(), record.Id) + latestOriginal, err := app.FindRecordById(record.Collection(), record.Original().Id) if err == nil { return latestOriginal.GetRaw(f.Name) } diff --git a/core/field_text.go b/core/field_text.go index cbcfc52f..71e9a5fb 100644 --- a/core/field_text.go +++ b/core/field_text.go @@ -2,10 +2,14 @@ package core import ( "context" + "database/sql" + "errors" "fmt" "regexp" + "strings" validation "github.com/go-ozzo/ozzo-validation/v4" + "github.com/pocketbase/dbx" "github.com/pocketbase/pocketbase/core/validators" "github.com/pocketbase/pocketbase/tools/security" "github.com/spf13/cast" @@ -151,6 +155,8 @@ func (f *TextField) PrepareValue(record *Record, raw any) (any, error) { return cast.ToString(raw), nil } +var forbiddenPKChars = []string{"/", "\\"} + // ValidateValue implements [Field.ValidateValue] interface method. func (f *TextField) ValidateValue(ctx context.Context, app App, record *Record) error { newVal, ok := record.GetRaw(f.Name).(string) @@ -158,14 +164,42 @@ func (f *TextField) ValidateValue(ctx context.Context, app App, record *Record) return validators.ErrUnsupportedValueType } - // disallow PK change - if f.PrimaryKey && !record.IsNew() { - oldVal := record.LastSavedPK() - if oldVal != newVal { - return validation.NewError("validation_pk_change", "The record primary key cannot be changed.") - } - if oldVal != "" { - return nil // no need to further validate since the id can't be updated anyway + if f.PrimaryKey { + // disallow PK change + if !record.IsNew() { + oldVal := record.LastSavedPK() + if oldVal != newVal { + return validation.NewError("validation_pk_change", "The record primary key cannot be changed.") + } + if oldVal != "" { + // no need to further validate because the id can't be updated + // and because the id could have been inserted manually by migration from another system + // that may not comply with the user defined PocketBase validations + return nil + } + } else { + // disallow PK special characters no matter of the Pattern validator to minimize + // side-effects when the primary key is used for example in a directory path + for _, c := range forbiddenPKChars { + if strings.Contains(newVal, c) { + return validation.NewError("validation_pk_forbidden", "The record primary key contains forbidden characters."). + SetParams(map[string]any{"forbidden": c}) + } + } + + // this technically shouldn't be necessarily but again to + // prevent minimize misuse of the Pattern validator that could + // cause side-effects on some platforms check for duplicates in a case-insensitive manner + var exists bool + err := app.DB(). + Select("(1)"). + From(record.TableName()). + Where(dbx.NewExp("LOWER(id) = {:id}", dbx.Params{"id": strings.ToLower(newVal)})). + Limit(1). + Row(&exists) + if exists || (err != nil && !errors.Is(err, sql.ErrNoRows)) { + return validation.NewError("validation_pk_invalid", "The record primary key is invalid or already exists.") + } } } diff --git a/core/field_text_test.go b/core/field_text_test.go index c05d26b2..4883a8b1 100644 --- a/core/field_text_test.go +++ b/core/field_text_test.go @@ -68,7 +68,15 @@ func TestTextFieldValidateValue(t *testing.T) { app, _ := tests.NewTestApp() defer app.Cleanup() - collection := core.NewBaseCollection("test_collection") + collection, err := app.FindCollectionByNameOrId("demo1") + if err != nil { + t.Fatal(err) + } + + existingRecord, err := app.FindFirstRecordByFilter(collection, "id != ''") + if err != nil { + t.Fatal(err) + } scenarios := []struct { name string @@ -116,6 +124,46 @@ func TestTextFieldValidateValue(t *testing.T) { }, false, }, + { + "special forbidden character / (non-primaryKey)", + &core.TextField{Name: "test", PrimaryKey: false}, + func() *core.Record { + record := core.NewRecord(collection) + record.SetRaw("test", "/") + return record + }, + false, + }, + { + "special forbidden character \\ (non-primaryKey)", + &core.TextField{Name: "test", PrimaryKey: false}, + func() *core.Record { + record := core.NewRecord(collection) + record.SetRaw("test", "\\") + return record + }, + false, + }, + { + "special forbidden character / (primaryKey)", + &core.TextField{Name: "test", PrimaryKey: true}, + func() *core.Record { + record := core.NewRecord(collection) + record.SetRaw("test", "/") + return record + }, + true, + }, + { + "special forbidden character \\ (primaryKey)", + &core.TextField{Name: "test", PrimaryKey: true}, + func() *core.Record { + record := core.NewRecord(collection) + record.SetRaw("test", "\\") + return record + }, + true, + }, { "zero field value (primaryKey)", &core.TextField{Name: "test", PrimaryKey: true}, @@ -131,11 +179,21 @@ func TestTextFieldValidateValue(t *testing.T) { &core.TextField{Name: "test", PrimaryKey: true}, func() *core.Record { record := core.NewRecord(collection) - record.SetRaw("test", "abc") + record.SetRaw("test", "abcd") return record }, false, }, + { + "case-insensitive duplicated primary key check", + &core.TextField{Name: "test", PrimaryKey: true}, + func() *core.Record { + record := core.NewRecord(collection) + record.SetRaw("test", strings.ToUpper(existingRecord.Id)) + return record + }, + true, + }, { "< min", &core.TextField{Name: "test", Min: 4},