From a677dbece0de312e083aa38963ac324e53139b8a Mon Sep 17 00:00:00 2001 From: aman Date: Fri, 3 Jul 2026 15:39:41 +0530 Subject: [PATCH] feat(avatar): validate user and org avatar fields Avatars must now be base64 JPEG or PNG data URLs within a configurable size cap (app.avatar.max_size_bytes, default 1MB). Validation runs in the user and organization services, and the admin UI server now sets a Content-Security-Policy header for images. Co-Authored-By: Claude Fable 5 --- cmd/serve.go | 7 +- core/avatar/avatar.go | 78 ++++++++++++ core/avatar/avatar_test.go | 50 ++++++++ core/organization/service.go | 14 ++- core/organization/service_test.go | 49 +++++++- core/user/service.go | 11 +- core/user/service_test.go | 118 +++++++++++------- internal/api/v1beta1connect/organization.go | 7 ++ .../api/v1beta1connect/organization_test.go | 17 +++ internal/api/v1beta1connect/user.go | 7 ++ internal/api/v1beta1connect/user_test.go | 16 +++ pkg/server/config.go | 3 + pkg/server/security_headers.go | 19 +++ pkg/server/security_headers_test.go | 20 +++ pkg/server/server.go | 2 +- 15 files changed, 361 insertions(+), 57 deletions(-) create mode 100644 core/avatar/avatar.go create mode 100644 core/avatar/avatar_test.go create mode 100644 pkg/server/security_headers.go create mode 100644 pkg/server/security_headers_test.go diff --git a/cmd/serve.go b/cmd/serve.go index 3bbf77820..2ccfb4e8a 100644 --- a/cmd/serve.go +++ b/cmd/serve.go @@ -8,6 +8,7 @@ import ( "net/http" "os" "os/signal" + "slices" "strings" "syscall" "time" @@ -29,8 +30,6 @@ import ( "github.com/raystack/frontier/core/prospect" "github.com/raystack/frontier/core/userpat" - "slices" - "github.com/doug-martin/goqu/v9" "github.com/jackc/pgx/v4" "github.com/stripe/stripe-go/v79" @@ -433,13 +432,13 @@ func buildAPIDependencies( // back here because role.Service depends on permission.Service permissionService.SetRoleService(roleService) policyService := policy.NewService(policyPGRepository, relationService, roleService) - userService := user.NewService(userRepository, relationService, sessionService, auditRecordRepository) + userService := user.NewService(userRepository, relationService, sessionService, auditRecordRepository, cfg.App.Avatar) patValidator := userpat.NewValidator(logger, userPATRepo, cfg.App.PAT) authnService := authenticate.NewService(logger, cfg.App.Authentication, postgres.NewFlowRepository(logger, dbc), mailDialer, tokenService, sessionService, userService, serviceUserService, webAuthConfig, patValidator) groupService := group.NewService(groupRepository, relationService, authnService, policyService) organizationService := organization.NewService(organizationRepository, relationService, userService, - authnService, policyService, preferenceService, roleService) + authnService, policyService, preferenceService, roleService, cfg.App.Avatar) projectRepository := postgres.NewProjectRepository(dbc) projectService := project.NewService(projectRepository, relationService, userService, policyService, authnService, serviceUserService, groupService, roleService) diff --git a/core/avatar/avatar.go b/core/avatar/avatar.go new file mode 100644 index 000000000..f253e750f --- /dev/null +++ b/core/avatar/avatar.go @@ -0,0 +1,78 @@ +package avatar + +import ( + "bytes" + "encoding/base64" + "errors" + "strings" +) + +// DefaultMaxBytes caps the stored avatar string at 1 MiB when no limit is +// configured. The limit is measured on the encoded data URL string. +const DefaultMaxBytes = 1 << 20 + +// Config holds avatar validation settings. +type Config struct { + // MaxSizeBytes is the largest allowed size of a stored avatar string. An + // avatar is a base64 data URL, so this is measured on the encoded string. + MaxSizeBytes int `yaml:"max_size_bytes" mapstructure:"max_size_bytes" default:"1048576"` +} + +const ( + jpegDataURLPrefix = "data:image/jpeg;base64," + pngDataURLPrefix = "data:image/png;base64," +) + +// ErrInvalid is returned when an avatar is not an allowed image data URL. +var ErrInvalid = errors.New("avatar must be a base64 JPEG or PNG data URL") + +var ( + jpegMagic = []byte{0xFF, 0xD8, 0xFF} + pngMagic = []byte{0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A} +) + +// Validate checks that an avatar is either empty or a base64 JPEG/PNG data +// URL within the size limit. +func Validate(value string, cfg Config) error { + if value == "" { + return nil + } + + maxBytes := cfg.MaxSizeBytes + if maxBytes <= 0 { + maxBytes = DefaultMaxBytes + } + + // Check the length before decoding so oversized input is rejected cheaply. + if len(value) > maxBytes { + return ErrInvalid + } + + payload, magic, ok := splitDataURL(value) + if !ok { + return ErrInvalid + } + + raw, err := base64.StdEncoding.DecodeString(payload) + if err != nil { + return ErrInvalid + } + + if !bytes.HasPrefix(raw, magic) { + return ErrInvalid + } + return nil +} + +// splitDataURL returns the base64 payload and the file signature the decoded +// bytes must start with. ok is false for anything but a JPEG or PNG data URL. +func splitDataURL(value string) (payload string, magic []byte, ok bool) { + switch { + case strings.HasPrefix(value, jpegDataURLPrefix): + return value[len(jpegDataURLPrefix):], jpegMagic, true + case strings.HasPrefix(value, pngDataURLPrefix): + return value[len(pngDataURLPrefix):], pngMagic, true + default: + return "", nil, false + } +} diff --git a/core/avatar/avatar_test.go b/core/avatar/avatar_test.go new file mode 100644 index 000000000..9663a71a5 --- /dev/null +++ b/core/avatar/avatar_test.go @@ -0,0 +1,50 @@ +package avatar + +import ( + "encoding/base64" + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestValidate(t *testing.T) { + jpeg := jpegDataURLPrefix + base64.StdEncoding.EncodeToString([]byte{0xFF, 0xD8, 0xFF, 0xE0, 0x00, 0x10}) + png := pngDataURLPrefix + base64.StdEncoding.EncodeToString([]byte{0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A}) + pngBytesAsJPEG := jpegDataURLPrefix + base64.StdEncoding.EncodeToString([]byte{0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A}) + notAnImage := jpegDataURLPrefix + base64.StdEncoding.EncodeToString([]byte("hello there")) + oversized := jpegDataURLPrefix + strings.Repeat("A", DefaultMaxBytes) + + tests := []struct { + name string + value string + cfg Config + wantErr bool + }{ + {name: "empty is allowed", value: "", wantErr: false}, + {name: "valid jpeg base64", value: jpeg, wantErr: false}, + {name: "valid png base64", value: png, wantErr: false}, + {name: "zero limit falls back to default", value: jpeg, cfg: Config{MaxSizeBytes: 0}, wantErr: false}, + {name: "valid jpeg rejected under tiny limit", value: jpeg, cfg: Config{MaxSizeBytes: 8}, wantErr: true}, + {name: "rejects svg data url", value: "data:image/svg+xml;base64,PHN2ZyBvbmxvYWQ9ImFsZXJ0KDEpIi8+", wantErr: true}, + {name: "rejects javascript scheme", value: "javascript:alert(document.cookie)", wantErr: true}, + {name: "rejects http url", value: "http://169.254.169.254/latest/meta-data/", wantErr: true}, + {name: "rejects https url", value: "https://attacker.example/collect?u=admin", wantErr: true}, + {name: "rejects oversized payload", value: oversized, wantErr: true}, + {name: "rejects invalid base64", value: jpegDataURLPrefix + "@@@not-base64@@@", wantErr: true}, + {name: "rejects png bytes declared as jpeg", value: pngBytesAsJPEG, wantErr: true}, + {name: "rejects non-image bytes", value: notAnImage, wantErr: true}, + {name: "rejects data url without base64", value: "data:image/jpeg,rawdata", wantErr: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := Validate(tt.value, tt.cfg) + if tt.wantErr { + assert.ErrorIs(t, err, ErrInvalid) + } else { + assert.NoError(t, err) + } + }) + } +} diff --git a/core/organization/service.go b/core/organization/service.go index 45d80ab1e..d5e3de62f 100644 --- a/core/organization/service.go +++ b/core/organization/service.go @@ -13,6 +13,7 @@ import ( "github.com/raystack/frontier/core/role" "github.com/raystack/frontier/core/authenticate" + "github.com/raystack/frontier/core/avatar" "github.com/raystack/frontier/pkg/str" "github.com/raystack/frontier/pkg/utils" @@ -79,11 +80,12 @@ type Service struct { prefService PreferencesService roleService RoleService membershipService MembershipService + avatarConfig avatar.Config } func NewService(repository Repository, relationService RelationService, userService UserService, authnService AuthnService, policyService PolicyService, - prefService PreferencesService, roleService RoleService) *Service { + prefService PreferencesService, roleService RoleService, avatarConfig avatar.Config) *Service { return &Service{ repository: repository, relationService: relationService, @@ -92,6 +94,7 @@ func NewService(repository Repository, relationService RelationService, policyService: policyService, prefService: prefService, roleService: roleService, + avatarConfig: avatarConfig, } } @@ -149,6 +152,9 @@ func (s Service) GetDefaultOrgStateOnCreate(ctx context.Context) (State, error) } func (s Service) Create(ctx context.Context, org Organization) (Organization, error) { + if err := avatar.Validate(org.Avatar, s.avatarConfig); err != nil { + return Organization{}, err + } principal, err := s.authnService.GetPrincipal(ctx) if err != nil { return Organization{}, fmt.Errorf("%w: %s", user.ErrNotExist, err.Error()) @@ -239,6 +245,9 @@ func (s Service) List(ctx context.Context, f Filter) ([]Organization, error) { } func (s Service) Update(ctx context.Context, org Organization) (Organization, error) { + if err := avatar.Validate(org.Avatar, s.avatarConfig); err != nil { + return Organization{}, err + } if org.ID != "" { return s.repository.UpdateByID(ctx, org) } @@ -281,6 +290,9 @@ func (s Service) MemberCount(ctx context.Context, orgID string) (int64, error) { } func (s Service) AdminCreate(ctx context.Context, org Organization, ownerEmail string) (Organization, error) { + if err := avatar.Validate(org.Avatar, s.avatarConfig); err != nil { + return Organization{}, err + } // Validate email if !user.IsValidEmail(ownerEmail) { return Organization{}, user.ErrInvalidEmail diff --git a/core/organization/service_test.go b/core/organization/service_test.go index e88f8399e..7920d0546 100644 --- a/core/organization/service_test.go +++ b/core/organization/service_test.go @@ -6,6 +6,7 @@ import ( "testing" "github.com/google/uuid" + "github.com/raystack/frontier/core/avatar" "github.com/raystack/frontier/core/organization" "github.com/raystack/frontier/core/organization/mocks" "github.com/raystack/frontier/core/preference" @@ -15,6 +16,44 @@ import ( "github.com/stretchr/testify/mock" ) +func TestService_CreateRejectsInvalidAvatar(t *testing.T) { + svc := organization.NewService( + mocks.NewRepository(t), + mocks.NewRelationService(t), + mocks.NewUserService(t), + mocks.NewAuthnService(t), + mocks.NewPolicyService(t), + mocks.NewPreferencesService(t), + mocks.NewRoleService(t), + avatar.Config{}, + ) + + _, err := svc.Create(context.Background(), organization.Organization{ + Name: "acme", + Avatar: "data:image/svg+xml;base64,PHN2Zy8+", + }) + assert.ErrorIs(t, err, avatar.ErrInvalid) +} + +func TestService_UpdateRejectsInvalidAvatar(t *testing.T) { + svc := organization.NewService( + mocks.NewRepository(t), + mocks.NewRelationService(t), + mocks.NewUserService(t), + mocks.NewAuthnService(t), + mocks.NewPolicyService(t), + mocks.NewPreferencesService(t), + mocks.NewRoleService(t), + avatar.Config{}, + ) + + _, err := svc.Update(context.Background(), organization.Organization{ + ID: uuid.New().String(), + Avatar: "http://169.254.169.254/latest/meta-data/", + }) + assert.ErrorIs(t, err, avatar.ErrInvalid) +} + func TestService_Get(t *testing.T) { mockRepo := mocks.NewRepository(t) mockRelationSvc := mocks.NewRelationService(t) @@ -24,7 +63,7 @@ func TestService_Get(t *testing.T) { mockPrefSvc := mocks.NewPreferencesService(t) mockRoleSvc := mocks.NewRoleService(t) - svc := organization.NewService(mockRepo, mockRelationSvc, mockUserSvc, mockAuthnSvc, mockPolicySvc, mockPrefSvc, mockRoleSvc) + svc := organization.NewService(mockRepo, mockRelationSvc, mockUserSvc, mockAuthnSvc, mockPolicySvc, mockPrefSvc, mockRoleSvc, avatar.Config{}) t.Run("should return orgs when fetched by id (by calling repo.GetByID)", func(t *testing.T) { IDParam := uuid.New() @@ -82,7 +121,7 @@ func TestService_GetRaw(t *testing.T) { mockPrefSvc := mocks.NewPreferencesService(t) mockRoleSvc := mocks.NewRoleService(t) - svc := organization.NewService(mockRepo, mockRelationSvc, mockUserSvc, mockAuthnSvc, mockPolicySvc, mockPrefSvc, mockRoleSvc) + svc := organization.NewService(mockRepo, mockRelationSvc, mockUserSvc, mockAuthnSvc, mockPolicySvc, mockPrefSvc, mockRoleSvc, avatar.Config{}) t.Run("should return an org based on ID passed", func(t *testing.T) { IDParam := uuid.New() @@ -139,7 +178,7 @@ func TestService_GetDefaultOrgStateOnCreate(t *testing.T) { mockPrefSvc := mocks.NewPreferencesService(t) mockRoleSvc := mocks.NewRoleService(t) - svc := organization.NewService(mockRepo, mockRelationSvc, mockUserSvc, mockAuthnSvc, mockPolicySvc, mockPrefSvc, mockRoleSvc) + svc := organization.NewService(mockRepo, mockRelationSvc, mockUserSvc, mockAuthnSvc, mockPolicySvc, mockPrefSvc, mockRoleSvc, avatar.Config{}) t.Run("should return org state to be set on creation, as per preferences", func(t *testing.T) { expectedPrefs := map[string]string{ @@ -170,7 +209,7 @@ func TestService_AttachToPlatform(t *testing.T) { mockPrefSvc := mocks.NewPreferencesService(t) mockRoleSvc := mocks.NewRoleService(t) - svc := organization.NewService(mockRepo, mockRelationSvc, mockUserSvc, mockAuthnSvc, mockPolicySvc, mockPrefSvc, mockRoleSvc) + svc := organization.NewService(mockRepo, mockRelationSvc, mockUserSvc, mockAuthnSvc, mockPolicySvc, mockPrefSvc, mockRoleSvc, avatar.Config{}) inputOrgID := "some-org-id" relationToBeCreated := relation.Relation{ @@ -212,7 +251,7 @@ func TestService_List(t *testing.T) { mockPrefSvc := mocks.NewPreferencesService(t) mockRoleSvc := mocks.NewRoleService(t) svc := organization.NewService(mockRepo, mockRelationSvc, mockUserSvc, mockAuthnSvc, - mockPolicySvc, mockPrefSvc, mockRoleSvc) + mockPolicySvc, mockPrefSvc, mockRoleSvc, avatar.Config{}) return svc, mockRepo } diff --git a/core/user/service.go b/core/user/service.go index 3a97d7adf..1d556e0ae 100644 --- a/core/user/service.go +++ b/core/user/service.go @@ -15,6 +15,7 @@ import ( "github.com/raystack/frontier/pkg/utils" "github.com/raystack/frontier/core/auditrecord/models" + "github.com/raystack/frontier/core/avatar" "github.com/raystack/frontier/core/relation" "github.com/raystack/frontier/internal/bootstrap/schema" pkgAuditRecord "github.com/raystack/frontier/pkg/auditrecord" @@ -45,16 +46,18 @@ type Service struct { relationService RelationService sessionService SessionService auditRecordRepository AuditRecordRepository + avatarConfig avatar.Config Now func() time.Time } func NewService(repository Repository, relationRepo RelationService, - sessionService SessionService, auditRecordRepository AuditRecordRepository) *Service { + sessionService SessionService, auditRecordRepository AuditRecordRepository, avatarConfig avatar.Config) *Service { return &Service{ repository: repository, relationService: relationRepo, sessionService: sessionService, auditRecordRepository: auditRecordRepository, + avatarConfig: avatarConfig, Now: func() time.Time { return time.Now().UTC() }, @@ -82,6 +85,9 @@ func (s Service) GetByEmail(ctx context.Context, email string) (User, error) { } func (s Service) Create(ctx context.Context, user User) (User, error) { + if err := avatar.Validate(user.Avatar, s.avatarConfig); err != nil { + return User{}, err + } return s.repository.Create(ctx, User{ Name: strings.ToLower(user.Name), Email: strings.ToLower(user.Email), @@ -102,6 +108,9 @@ func (s Service) List(ctx context.Context, flt Filter) ([]User, error) { // one security concern is that we need to ensure users can't misuse it to takeover // invitations created for other users. func (s Service) Update(ctx context.Context, toUpdate User) (User, error) { + if err := avatar.Validate(toUpdate.Avatar, s.avatarConfig); err != nil { + return User{}, err + } id := toUpdate.ID toUpdate.Email = strings.ToLower(toUpdate.Email) toUpdate.Name = strings.ToLower(toUpdate.Name) diff --git a/core/user/service_test.go b/core/user/service_test.go index 0a4e2ff37..a62411e38 100644 --- a/core/user/service_test.go +++ b/core/user/service_test.go @@ -9,6 +9,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/google/uuid" "github.com/raystack/frontier/core/auditrecord/models" + "github.com/raystack/frontier/core/avatar" "github.com/raystack/frontier/core/relation" "github.com/raystack/frontier/core/user" "github.com/raystack/frontier/core/user/mocks" @@ -28,6 +29,33 @@ func mockService(t *testing.T) (*mocks.Repository, *mocks.RelationService, *mock return repo, relationService, sessionService, auditRecordRepository } +func TestService_CreateRejectsInvalidAvatar(t *testing.T) { + repo, relationService, sessionService, auditRecordRepository := mockService(t) + svc := user.NewService(repo, relationService, sessionService, auditRecordRepository, avatar.Config{}) + + _, err := svc.Create(context.Background(), user.User{ + Email: "test@email.com", + Avatar: "javascript:alert(document.cookie)", + }) + if !errors.Is(err, avatar.ErrInvalid) { + t.Fatalf("expected ErrInvalidAvatar, got %v", err) + } +} + +func TestService_UpdateRejectsInvalidAvatar(t *testing.T) { + repo, relationService, sessionService, auditRecordRepository := mockService(t) + svc := user.NewService(repo, relationService, sessionService, auditRecordRepository, avatar.Config{}) + + _, err := svc.Update(context.Background(), user.User{ + ID: uuid.New().String(), + Email: "test@email.com", + Avatar: "https://attacker.example/collect", + }) + if !errors.Is(err, avatar.ErrInvalid) { + t.Fatalf("expected ErrInvalidAvatar, got %v", err) + } +} + func TestService_GetByID(t *testing.T) { testID := uuid.New() tests := []struct { @@ -51,7 +79,7 @@ func TestService_GetByID(t *testing.T) { ID: testID.String(), Name: "test", }, nil) - return user.NewService(repo, relationService, sessionService, auditRecordRepository) + return user.NewService(repo, relationService, sessionService, auditRecordRepository, avatar.Config{}) }, }, { @@ -70,7 +98,7 @@ func TestService_GetByID(t *testing.T) { Name: "test", Email: "test@test.com", }, nil) - return user.NewService(repo, relationService, sessionService, auditRecordRepository) + return user.NewService(repo, relationService, sessionService, auditRecordRepository, avatar.Config{}) }, }, { @@ -87,7 +115,7 @@ func TestService_GetByID(t *testing.T) { ID: testID.String(), Name: "test", }, nil) - return user.NewService(repo, relationService, sessionService, auditRecordRepository) + return user.NewService(repo, relationService, sessionService, auditRecordRepository, avatar.Config{}) }, }, { @@ -98,7 +126,7 @@ func TestService_GetByID(t *testing.T) { setup: func() *user.Service { repo, relationService, sessionService, auditRecordRepository := mockService(t) repo.EXPECT().GetByName(mock.Anything, "invalid").Return(user.User{}, errors.New("not found")) - return user.NewService(repo, relationService, sessionService, auditRecordRepository) + return user.NewService(repo, relationService, sessionService, auditRecordRepository, avatar.Config{}) }, }, } @@ -132,7 +160,7 @@ func TestService_Create(t *testing.T) { Name: "test", Email: "test@email.com", State: "enable", - Avatar: "abc", + Avatar: "data:image/jpeg;base64,/9j/", Title: "tesT", Metadata: map[string]any{ "key": "value", @@ -142,7 +170,7 @@ func TestService_Create(t *testing.T) { ID: "test-id", Name: "test", Email: "test@email.com", - Avatar: "abc", + Avatar: "data:image/jpeg;base64,/9j/", Title: "tesT", State: user.Enabled, Metadata: map[string]any{ @@ -158,7 +186,7 @@ func TestService_Create(t *testing.T) { Name: "test", Email: "test@email.com", State: user.Enabled, - Avatar: "abc", + Avatar: "data:image/jpeg;base64,/9j/", Title: "tesT", Metadata: map[string]any{ "key": "value", @@ -168,7 +196,7 @@ func TestService_Create(t *testing.T) { Name: "test", Email: "test@email.com", State: user.Enabled, - Avatar: "abc", + Avatar: "data:image/jpeg;base64,/9j/", Title: "tesT", Metadata: map[string]any{ "key": "value", @@ -176,7 +204,7 @@ func TestService_Create(t *testing.T) { CreatedAt: time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC), UpdatedAt: time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC), }, nil) - return user.NewService(repo, relationService, sessionService, auditRecordRepository) + return user.NewService(repo, relationService, sessionService, auditRecordRepository, avatar.Config{}) }, }, { @@ -194,7 +222,7 @@ func TestService_Create(t *testing.T) { Email: "test", State: user.Enabled, }).Return(user.User{}, errors.New("failed to create")) - return user.NewService(repo, relationService, sessionService, auditRecordRepository) + return user.NewService(repo, relationService, sessionService, auditRecordRepository, avatar.Config{}) }, }, } @@ -251,7 +279,7 @@ func TestService_List(t *testing.T) { Name: "test-2", }, }, nil) - return user.NewService(repo, relationService, sessionService, auditRecordRepository) + return user.NewService(repo, relationService, sessionService, auditRecordRepository, avatar.Config{}) }, }, } @@ -284,7 +312,7 @@ func TestService_Update(t *testing.T) { user: user.User{ ID: "test@email.com", Name: "test", - Avatar: "abc", + Avatar: "data:image/jpeg;base64,/9j/", Title: "tesT", Metadata: map[string]any{ "key": "value", @@ -294,7 +322,7 @@ func TestService_Update(t *testing.T) { ID: "test-id", Name: "test", Email: "test@email.com", - Avatar: "abc", + Avatar: "data:image/jpeg;base64,/9j/", Title: "tesT", State: user.Enabled, Metadata: map[string]any{ @@ -309,7 +337,7 @@ func TestService_Update(t *testing.T) { repo.EXPECT().UpdateByEmail(mock.Anything, user.User{ ID: "test@email.com", Name: "test", - Avatar: "abc", + Avatar: "data:image/jpeg;base64,/9j/", Title: "tesT", Metadata: map[string]any{ "key": "value", @@ -319,7 +347,7 @@ func TestService_Update(t *testing.T) { Name: "test", Email: "test@email.com", State: user.Enabled, - Avatar: "abc", + Avatar: "data:image/jpeg;base64,/9j/", Title: "tesT", Metadata: map[string]any{ "key": "value", @@ -327,7 +355,7 @@ func TestService_Update(t *testing.T) { CreatedAt: time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC), UpdatedAt: time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC), }, nil) - return user.NewService(repo, relationService, sessionService, auditRecordRepository) + return user.NewService(repo, relationService, sessionService, auditRecordRepository, avatar.Config{}) }, }, { @@ -335,7 +363,7 @@ func TestService_Update(t *testing.T) { user: user.User{ ID: "test", Name: "test", - Avatar: "abc", + Avatar: "data:image/jpeg;base64,/9j/", Title: "tesT", Metadata: map[string]any{ "key": "value", @@ -345,7 +373,7 @@ func TestService_Update(t *testing.T) { ID: "test-id", Name: "test", Email: "test@email.com", - Avatar: "abc", + Avatar: "data:image/jpeg;base64,/9j/", Title: "tesT", State: user.Enabled, Metadata: map[string]any{ @@ -360,7 +388,7 @@ func TestService_Update(t *testing.T) { repo.EXPECT().UpdateByName(mock.Anything, user.User{ ID: "test", Name: "test", - Avatar: "abc", + Avatar: "data:image/jpeg;base64,/9j/", Title: "tesT", Metadata: map[string]any{ "key": "value", @@ -370,7 +398,7 @@ func TestService_Update(t *testing.T) { Name: "test", Email: "test@email.com", State: user.Enabled, - Avatar: "abc", + Avatar: "data:image/jpeg;base64,/9j/", Title: "tesT", Metadata: map[string]any{ "key": "value", @@ -378,7 +406,7 @@ func TestService_Update(t *testing.T) { CreatedAt: time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC), UpdatedAt: time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC), }, nil) - return user.NewService(repo, relationService, sessionService, auditRecordRepository) + return user.NewService(repo, relationService, sessionService, auditRecordRepository, avatar.Config{}) }, }, { @@ -386,7 +414,7 @@ func TestService_Update(t *testing.T) { user: user.User{ ID: testID.String(), Name: "test", - Avatar: "abc", + Avatar: "data:image/jpeg;base64,/9j/", Title: "tesT", Metadata: map[string]any{ "key": "value", @@ -396,7 +424,7 @@ func TestService_Update(t *testing.T) { ID: testID.String(), Name: "test", Email: "test@email.com", - Avatar: "abc", + Avatar: "data:image/jpeg;base64,/9j/", Title: "tesT", State: user.Enabled, Metadata: map[string]any{ @@ -411,7 +439,7 @@ func TestService_Update(t *testing.T) { repo.EXPECT().UpdateByID(mock.Anything, user.User{ ID: testID.String(), Name: "test", - Avatar: "abc", + Avatar: "data:image/jpeg;base64,/9j/", Title: "tesT", Metadata: map[string]any{ "key": "value", @@ -421,7 +449,7 @@ func TestService_Update(t *testing.T) { Name: "test", Email: "test@email.com", State: user.Enabled, - Avatar: "abc", + Avatar: "data:image/jpeg;base64,/9j/", Title: "tesT", Metadata: map[string]any{ "key": "value", @@ -429,7 +457,7 @@ func TestService_Update(t *testing.T) { CreatedAt: time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC), UpdatedAt: time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC), }, nil) - return user.NewService(repo, relationService, sessionService, auditRecordRepository) + return user.NewService(repo, relationService, sessionService, auditRecordRepository, avatar.Config{}) }, }, { @@ -446,7 +474,7 @@ func TestService_Update(t *testing.T) { Name: "test ", Email: "test", }).Return(user.User{}, errors.New("failed to update")) - return user.NewService(repo, relationService, sessionService, auditRecordRepository) + return user.NewService(repo, relationService, sessionService, auditRecordRepository, avatar.Config{}) }, }, } @@ -480,7 +508,7 @@ func TestService_Disable(t *testing.T) { repo, relationService, sessionService, auditRecordRepository := mockService(t) repo.EXPECT().SetState(mock.Anything, validID, user.Disabled).Return(nil) sessionService.EXPECT().DeleteByUserID(mock.Anything, validID).Return(nil) - return user.NewService(repo, relationService, sessionService, auditRecordRepository) + return user.NewService(repo, relationService, sessionService, auditRecordRepository, avatar.Config{}) }, }, { @@ -489,7 +517,7 @@ func TestService_Disable(t *testing.T) { wantErr: user.ErrInvalidID, setup: func() *user.Service { repo, relationService, sessionService, auditRecordRepository := mockService(t) - return user.NewService(repo, relationService, sessionService, auditRecordRepository) + return user.NewService(repo, relationService, sessionService, auditRecordRepository, avatar.Config{}) }, }, { @@ -498,7 +526,7 @@ func TestService_Disable(t *testing.T) { wantErr: user.ErrInvalidID, setup: func() *user.Service { repo, relationService, sessionService, auditRecordRepository := mockService(t) - return user.NewService(repo, relationService, sessionService, auditRecordRepository) + return user.NewService(repo, relationService, sessionService, auditRecordRepository, avatar.Config{}) }, }, { @@ -508,7 +536,7 @@ func TestService_Disable(t *testing.T) { setup: func() *user.Service { repo, relationService, sessionService, auditRecordRepository := mockService(t) repo.EXPECT().SetState(mock.Anything, validID, user.Disabled).Return(user.ErrNotExist) - return user.NewService(repo, relationService, sessionService, auditRecordRepository) + return user.NewService(repo, relationService, sessionService, auditRecordRepository, avatar.Config{}) }, }, } @@ -541,7 +569,7 @@ func TestService_Enable(t *testing.T) { setup: func() *user.Service { repo, relationService, sessionService, auditRecordRepository := mockService(t) repo.EXPECT().SetState(mock.Anything, validID, user.Enabled).Return(nil) - return user.NewService(repo, relationService, sessionService, auditRecordRepository) + return user.NewService(repo, relationService, sessionService, auditRecordRepository, avatar.Config{}) }, }, { @@ -550,7 +578,7 @@ func TestService_Enable(t *testing.T) { wantErr: user.ErrInvalidID, setup: func() *user.Service { repo, relationService, sessionService, auditRecordRepository := mockService(t) - return user.NewService(repo, relationService, sessionService, auditRecordRepository) + return user.NewService(repo, relationService, sessionService, auditRecordRepository, avatar.Config{}) }, }, { @@ -559,7 +587,7 @@ func TestService_Enable(t *testing.T) { wantErr: user.ErrInvalidID, setup: func() *user.Service { repo, relationService, sessionService, auditRecordRepository := mockService(t) - return user.NewService(repo, relationService, sessionService, auditRecordRepository) + return user.NewService(repo, relationService, sessionService, auditRecordRepository, avatar.Config{}) }, }, { @@ -569,7 +597,7 @@ func TestService_Enable(t *testing.T) { setup: func() *user.Service { repo, relationService, sessionService, auditRecordRepository := mockService(t) repo.EXPECT().SetState(mock.Anything, validID, user.Enabled).Return(user.ErrNotExist) - return user.NewService(repo, relationService, sessionService, auditRecordRepository) + return user.NewService(repo, relationService, sessionService, auditRecordRepository, avatar.Config{}) }, }, } @@ -606,7 +634,7 @@ func TestService_Delete(t *testing.T) { Namespace: schema.UserPrincipal, }}).Return(nil) repo.EXPECT().Delete(mock.Anything, "test-id").Return(nil) - return user.NewService(repo, relationService, sessionService, auditRecordRepository) + return user.NewService(repo, relationService, sessionService, auditRecordRepository, avatar.Config{}) }, }, { @@ -619,7 +647,7 @@ func TestService_Delete(t *testing.T) { ID: "test-id", Namespace: schema.UserPrincipal, }}).Return(errors.New("failed to delete relation")) - return user.NewService(repo, relationService, sessionService, auditRecordRepository) + return user.NewService(repo, relationService, sessionService, auditRecordRepository, avatar.Config{}) }, }, } @@ -702,7 +730,7 @@ func TestService_Sudo(t *testing.T) { auditRecordRepository.EXPECT().Create(mock.Anything, mock.MatchedBy(func(r models.AuditRecord) bool { return r.Event == pkgAuditRecord.PlatformAdminAddedEvent && r.Target != nil && r.Target.ID == "test-id" })).Return(models.AuditRecord{}, nil) - return user.NewService(repo, relationService, sessionService, auditRecordRepository) + return user.NewService(repo, relationService, sessionService, auditRecordRepository, avatar.Config{}) }, }, { @@ -747,7 +775,7 @@ func TestService_Sudo(t *testing.T) { Status: true, }, }, nil) - return user.NewService(repo, relationService, sessionService, auditRecordRepository) + return user.NewService(repo, relationService, sessionService, auditRecordRepository, avatar.Config{}) }, }, { @@ -816,7 +844,7 @@ func TestService_Sudo(t *testing.T) { auditRecordRepository.EXPECT().Create(mock.Anything, mock.MatchedBy(func(r models.AuditRecord) bool { return r.Event == pkgAuditRecord.PlatformMemberAddedEvent && r.Target != nil && r.Target.ID == "test-id" })).Return(models.AuditRecord{}, nil) - return user.NewService(repo, relationService, sessionService, auditRecordRepository) + return user.NewService(repo, relationService, sessionService, auditRecordRepository, avatar.Config{}) }, }, } @@ -902,7 +930,7 @@ func TestService_UnSudo(t *testing.T) { auditRecordRepository.EXPECT().Create(mock.Anything, mock.MatchedBy(func(r models.AuditRecord) bool { return r.Event == pkgAuditRecord.PlatformAdminRemovedEvent && r.Target != nil && r.Target.ID == "test-id" })).Return(models.AuditRecord{}, nil) - return user.NewService(repo, relationService, sessionService, auditRecordRepository) + return user.NewService(repo, relationService, sessionService, auditRecordRepository, avatar.Config{}) }, }, { @@ -922,7 +950,7 @@ func TestService_UnSudo(t *testing.T) { relationService.EXPECT().BatchCheckPermission(mock.Anything, adminCheckRelations). Return([]relation.CheckPair{{Relation: adminCheckRelations[0], Status: false}}, nil) // no Delete, no audit - return user.NewService(repo, relationService, sessionService, auditRecordRepository) + return user.NewService(repo, relationService, sessionService, auditRecordRepository, avatar.Config{}) }, }, { @@ -957,7 +985,7 @@ func TestService_UnSudo(t *testing.T) { auditRecordRepository.EXPECT().Create(mock.Anything, mock.MatchedBy(func(r models.AuditRecord) bool { return r.Event == pkgAuditRecord.PlatformMemberRemovedEvent && r.Target != nil && r.Target.ID == "test-id" })).Return(models.AuditRecord{}, nil) - return user.NewService(repo, relationService, sessionService, auditRecordRepository) + return user.NewService(repo, relationService, sessionService, auditRecordRepository, avatar.Config{}) }, }, { @@ -977,7 +1005,7 @@ func TestService_UnSudo(t *testing.T) { relationService.EXPECT().BatchCheckPermission(mock.Anything, memberCheckRelations). Return([]relation.CheckPair{{Relation: memberCheckRelations[0], Status: false}}, nil) // no Delete, no audit - return user.NewService(repo, relationService, sessionService, auditRecordRepository) + return user.NewService(repo, relationService, sessionService, auditRecordRepository, avatar.Config{}) }, }, { @@ -989,7 +1017,7 @@ func TestService_UnSudo(t *testing.T) { }, setup: func() *user.Service { repo, relationService, sessionService, auditRecordRepository := mockService(t) - return user.NewService(repo, relationService, sessionService, auditRecordRepository) + return user.NewService(repo, relationService, sessionService, auditRecordRepository, avatar.Config{}) }, }, } diff --git a/internal/api/v1beta1connect/organization.go b/internal/api/v1beta1connect/organization.go index 3f87ee008..b10d2c84e 100644 --- a/internal/api/v1beta1connect/organization.go +++ b/internal/api/v1beta1connect/organization.go @@ -10,6 +10,7 @@ import ( "github.com/raystack/frontier/core/audit" "github.com/raystack/frontier/core/authenticate" + "github.com/raystack/frontier/core/avatar" "github.com/raystack/frontier/core/membership" "github.com/raystack/frontier/core/organization" "github.com/raystack/frontier/core/project" @@ -130,6 +131,8 @@ func (h *ConnectHandler) CreateOrganization(ctx context.Context, request *connec "org_name", request.Msg.GetBody().GetName(), "org_title", request.Msg.GetBody().GetTitle()) return nil, connect.NewError(connect.CodePermissionDenied, ErrUnauthorized) + case errors.Is(err, avatar.ErrInvalid): + return nil, connect.NewError(connect.CodeInvalidArgument, err) default: return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("CreateOrganization.Create: org_name=%s org_title=%s: %w", request.Msg.GetBody().GetName(), request.Msg.GetBody().GetTitle(), err)) } @@ -171,6 +174,8 @@ func (h *ConnectHandler) AdminCreateOrganization(ctx context.Context, request *c return nil, connect.NewError(connect.CodeInvalidArgument, ErrBadRequest) case errors.Is(err, organization.ErrConflict): return nil, connect.NewError(connect.CodeAlreadyExists, ErrConflictRequest) + case errors.Is(err, avatar.ErrInvalid): + return nil, connect.NewError(connect.CodeInvalidArgument, err) default: return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("AdminCreateOrganization.AdminCreate: org_name=%s org_title=%s owner_email=%s: %w", request.Msg.GetBody().GetName(), request.Msg.GetBody().GetTitle(), request.Msg.GetBody().GetOrgOwnerEmail(), err)) } @@ -223,6 +228,8 @@ func (h *ConnectHandler) UpdateOrganization(ctx context.Context, request *connec return nil, connect.NewError(connect.CodeNotFound, ErrNotFound) case errors.Is(err, organization.ErrConflict): return nil, connect.NewError(connect.CodeAlreadyExists, ErrConflictRequest) + case errors.Is(err, avatar.ErrInvalid): + return nil, connect.NewError(connect.CodeInvalidArgument, err) default: return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("UpdateOrganization.Update: org_id=%s org_name=%s org_title=%s: %w", request.Msg.GetId(), request.Msg.GetBody().GetName(), request.Msg.GetBody().GetTitle(), err)) } diff --git a/internal/api/v1beta1connect/organization_test.go b/internal/api/v1beta1connect/organization_test.go index 126306137..f30e07e86 100644 --- a/internal/api/v1beta1connect/organization_test.go +++ b/internal/api/v1beta1connect/organization_test.go @@ -8,6 +8,7 @@ import ( "connectrpc.com/connect" "github.com/raystack/frontier/core/authenticate" + "github.com/raystack/frontier/core/avatar" "github.com/raystack/frontier/core/membership" "github.com/raystack/frontier/core/organization" "github.com/raystack/frontier/core/project" @@ -112,6 +113,22 @@ func TestHandler_ListOrganizations(t *testing.T) { } } +func TestHandler_CreateOrganization_InvalidAvatar(t *testing.T) { + mockOrgSrv := new(mocks.OrganizationService) + mockMetaSrv := new(mocks.MetaSchemaService) + mockMetaSrv.EXPECT().Validate(mock.Anything, mock.Anything).Return(nil) + mockOrgSrv.EXPECT().Create(mock.Anything, mock.Anything).Return(organization.Organization{}, avatar.ErrInvalid) + h := &ConnectHandler{orgService: mockOrgSrv, metaSchemaService: mockMetaSrv} + + _, err := h.CreateOrganization(context.Background(), connect.NewRequest(&frontierv1beta1.CreateOrganizationRequest{ + Body: &frontierv1beta1.OrganizationRequestBody{ + Name: "acme", + Avatar: "data:image/svg+xml;base64,PHN2Zy8+", + }, + })) + assert.Equal(t, connect.CodeInvalidArgument, connect.CodeOf(err)) +} + func TestHandler_CreateOrganization(t *testing.T) { email := "user@raystack.org" tests := []struct { diff --git a/internal/api/v1beta1connect/user.go b/internal/api/v1beta1connect/user.go index 94d281d37..36564000c 100644 --- a/internal/api/v1beta1connect/user.go +++ b/internal/api/v1beta1connect/user.go @@ -12,6 +12,7 @@ import ( "github.com/pkg/errors" "github.com/raystack/frontier/core/audit" "github.com/raystack/frontier/core/authenticate" + "github.com/raystack/frontier/core/avatar" "github.com/raystack/frontier/core/group" "github.com/raystack/frontier/core/membership" "github.com/raystack/frontier/core/organization" @@ -140,6 +141,8 @@ func (h *ConnectHandler) CreateUser(ctx context.Context, request *connect.Reques return nil, connect.NewError(connect.CodeAlreadyExists, ErrConflictRequest) case errors.Is(errors.Unwrap(err), user.ErrKeyDoesNotExists): return nil, connect.NewError(connect.CodeInvalidArgument, ErrBadRequest) + case errors.Is(err, avatar.ErrInvalid): + return nil, connect.NewError(connect.CodeInvalidArgument, err) default: return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("CreateUser: user_email=%s user_name=%s: %w", email, name, err)) } @@ -276,6 +279,8 @@ func (h *ConnectHandler) UpdateUser(ctx context.Context, request *connect.Reques return nil, connect.NewError(connect.CodeInvalidArgument, ErrBadRequest) case errors.Is(err, user.ErrConflict): return nil, connect.NewError(connect.CodeAlreadyExists, ErrConflictRequest) + case errors.Is(err, avatar.ErrInvalid): + return nil, connect.NewError(connect.CodeInvalidArgument, err) default: return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("UpdateUser: user_id=%s user_email=%s: %w", id, email, err)) } @@ -339,6 +344,8 @@ func (h *ConnectHandler) UpdateCurrentUser(ctx context.Context, request *connect return nil, connect.NewError(connect.CodeNotFound, ErrUserNotExist) case errors.Is(err, user.ErrInvalidDetails): return nil, connect.NewError(connect.CodeInvalidArgument, ErrBadRequest) + case errors.Is(err, avatar.ErrInvalid): + return nil, connect.NewError(connect.CodeInvalidArgument, err) default: return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("UpdateCurrentUser: principal_id=%s principal_type=%s: %w", principal.ID, principal.Type, err)) } diff --git a/internal/api/v1beta1connect/user_test.go b/internal/api/v1beta1connect/user_test.go index c6cf10c30..19015056e 100644 --- a/internal/api/v1beta1connect/user_test.go +++ b/internal/api/v1beta1connect/user_test.go @@ -9,6 +9,7 @@ import ( "connectrpc.com/connect" "github.com/google/uuid" "github.com/raystack/frontier/core/authenticate" + "github.com/raystack/frontier/core/avatar" "github.com/raystack/frontier/core/group" "github.com/raystack/frontier/core/organization" "github.com/raystack/frontier/core/permission" @@ -123,6 +124,21 @@ func TestConnectHandler_ListUsers(t *testing.T) { } } +func TestConnectHandler_CreateUser_InvalidAvatar(t *testing.T) { + mockUserSrv := new(mocks.UserService) + mockUserSrv.EXPECT().Create(mock.Anything, mock.Anything).Return(user.User{}, avatar.ErrInvalid) + h := &ConnectHandler{userService: mockUserSrv} + + ctx := authenticate.SetContextWithEmail(context.Background(), "test@test.com") + _, err := h.CreateUser(ctx, connect.NewRequest(&frontierv1beta1.CreateUserRequest{ + Body: &frontierv1beta1.UserRequestBody{ + Email: "test@test.com", + Avatar: "javascript:alert(1)", + }, + })) + assert.Equal(t, connect.CodeInvalidArgument, connect.CodeOf(err)) +} + func TestConnectHandler_CreateUser(t *testing.T) { email := "user@raystack.org" _ = email diff --git a/pkg/server/config.go b/pkg/server/config.go index 4eb2dab3a..14f4bebec 100644 --- a/pkg/server/config.go +++ b/pkg/server/config.go @@ -1,6 +1,7 @@ package server import ( + "github.com/raystack/frontier/core/avatar" "github.com/raystack/frontier/core/userpat" "github.com/raystack/frontier/core/webhook" @@ -85,6 +86,8 @@ type Config struct { Webhook webhook.Config `yaml:"webhook" mapstructure:"webhook"` PAT userpat.Config `yaml:"pat" mapstructure:"pat"` + Avatar avatar.Config `yaml:"avatar" mapstructure:"avatar"` + // AdditionalTraitsPath is a file path to a YAML file containing additional preference traits // These traits are merged with DefaultTraits at startup AdditionalTraitsPath string `yaml:"additional_traits_path" mapstructure:"additional_traits_path"` diff --git a/pkg/server/security_headers.go b/pkg/server/security_headers.go new file mode 100644 index 000000000..502ab5413 --- /dev/null +++ b/pkg/server/security_headers.go @@ -0,0 +1,19 @@ +package server + +import "net/http" + +// uiSecurityHeaders are set on every UI server response. img-src limits the +// admin console to images from its own origin, data URLs, and https hosts. +var uiSecurityHeaders = map[string]string{ + "Content-Security-Policy": "img-src 'self' data: https:", +} + +// withSecurityHeaders sets the given headers on every response. +func withSecurityHeaders(next http.Handler, headers map[string]string) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + for key, value := range headers { + w.Header().Set(key, value) + } + next.ServeHTTP(w, r) + }) +} diff --git a/pkg/server/security_headers_test.go b/pkg/server/security_headers_test.go new file mode 100644 index 000000000..0d26ca4ee --- /dev/null +++ b/pkg/server/security_headers_test.go @@ -0,0 +1,20 @@ +package server + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestWithSecurityHeaders(t *testing.T) { + h := withSecurityHeaders(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + }), uiSecurityHeaders) + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/", nil)) + + assert.Equal(t, "img-src 'self' data: https:", rec.Header().Get("Content-Security-Policy")) +} diff --git a/pkg/server/server.go b/pkg/server/server.go index 315da19a5..2fdbc21ca 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -105,7 +105,7 @@ func ServeUI(ctx context.Context, logger *slog.Logger, uiConfig UIConfig, apiSer } logger.Info("ui server starting", "http-port", uiConfig.Port) - if err := http.ListenAndServe(fmt.Sprintf(":%d", uiConfig.Port), nil); err != nil { + if err := http.ListenAndServe(fmt.Sprintf(":%d", uiConfig.Port), withSecurityHeaders(http.DefaultServeMux, uiSecurityHeaders)); err != nil { logger.Error("ui server failed", "err", err) } }