diff --git a/changelog/unreleased/fix-email-xsite-scripting.md b/changelog/unreleased/fix-email-xsite-scripting.md new file mode 100644 index 00000000000..74f3f163e06 --- /dev/null +++ b/changelog/unreleased/fix-email-xsite-scripting.md @@ -0,0 +1,6 @@ +Enhancement: Fix to prevent the email X-Site scripting + +Fix to prevent the email notification X-Site scripting + +https://github.com/owncloud/ocis/pull/6429 +https://github.com/owncloud/ocis/issues/6411 diff --git a/deployments/examples/ocis_wopi/docker-compose.yml b/deployments/examples/ocis_wopi/docker-compose.yml index d14c1f8bef4..95675736760 100644 --- a/deployments/examples/ocis_wopi/docker-compose.yml +++ b/deployments/examples/ocis_wopi/docker-compose.yml @@ -230,6 +230,10 @@ services: inbucket: image: inbucket/inbucket + ports: + - "9000:9000" + - "1100:1100" + - "2500:2500" networks: ocis-net: entrypoint: diff --git a/go.sum b/go.sum index 1df2b3fe911..14e531a49e6 100644 --- a/go.sum +++ b/go.sum @@ -629,8 +629,6 @@ github.com/crewjam/httperr v0.2.0 h1:b2BfXR8U3AlIHwNeFFvZ+BV1LFvKLlzMjzaTnZMybNo github.com/crewjam/httperr v0.2.0/go.mod h1:Jlz+Sg/XqBQhyMjdDiC+GNNRzZTD7x39Gu3pglZ5oH4= github.com/crewjam/saml v0.4.13 h1:TYHggH/hwP7eArqiXSJUvtOPNzQDyQ7vwmwEqlFWhMc= github.com/crewjam/saml v0.4.13/go.mod h1:igEejV+fihTIlHXYP8zOec3V5A8y3lws5bQBFsTm4gA= -github.com/cs3org/reva/v2 v2.13.4-0.20230531095732-bc9a3b635ec3 h1:T+W3zPmlPAaHlKhzBcW809PvcGUJJ+v1QF+JzdPRegU= -github.com/cs3org/reva/v2 v2.13.4-0.20230531095732-bc9a3b635ec3/go.mod h1:vMQqSn30fEPHO/GKC2WmGimlOPqvfSy4gdhRSpbvrWc= github.com/cs3org/reva/v2 v2.13.4-0.20230531122629-be4a2122a96c h1:gv0m1qVAkUtF/9PAP9xwp+jkjtajCAeGEhiO6dDOMcI= github.com/cs3org/reva/v2 v2.13.4-0.20230531122629-be4a2122a96c/go.mod h1:vMQqSn30fEPHO/GKC2WmGimlOPqvfSy4gdhRSpbvrWc= github.com/cubewise-code/go-mime v0.0.0-20200519001935-8c5762b177d8 h1:Z9lwXumT5ACSmJ7WGnFl+OMLLjpz5uR2fyz7dC255FI= diff --git a/services/notifications/pkg/email/composer.go b/services/notifications/pkg/email/composer.go index 3342cdbea5a..8a9c61bc522 100644 --- a/services/notifications/pkg/email/composer.go +++ b/services/notifications/pkg/email/composer.go @@ -9,7 +9,7 @@ import ( ) // NewTextTemplate replace the body message template placeholders with the translated template -func NewTextTemplate(mt MessageTemplate, locale string, translationPath string, vars map[string]interface{}) (MessageTemplate, error) { +func NewTextTemplate(mt MessageTemplate, locale string, translationPath string, vars map[string]string) (MessageTemplate, error) { var err error t := l10n.NewTranslator(locale, translationPath) mt.Subject, err = composeMessage(t.Translate(mt.Subject), vars) @@ -32,7 +32,7 @@ func NewTextTemplate(mt MessageTemplate, locale string, translationPath string, } // NewHTMLTemplate replace the body message template placeholders with the translated template -func NewHTMLTemplate(mt MessageTemplate, locale string, translationPath string, vars map[string]interface{}) (MessageTemplate, error) { +func NewHTMLTemplate(mt MessageTemplate, locale string, translationPath string, vars map[string]string) (MessageTemplate, error) { var err error t := l10n.NewTranslator(locale, translationPath) mt.Subject, err = composeMessage(t.Translate(mt.Subject), vars) @@ -55,7 +55,7 @@ func NewHTMLTemplate(mt MessageTemplate, locale string, translationPath string, } // composeMessage renders the message based on template -func composeMessage(tmpl string, vars map[string]interface{}) (string, error) { +func composeMessage(tmpl string, vars map[string]string) (string, error) { tpl, err := template.New("").Parse(replacePlaceholders(tmpl)) if err != nil { return "", err diff --git a/services/notifications/pkg/email/email.go b/services/notifications/pkg/email/email.go index e4779779f21..733f86388d9 100644 --- a/services/notifications/pkg/email/email.go +++ b/services/notifications/pkg/email/email.go @@ -6,6 +6,7 @@ package email import ( "bytes" "embed" + "html" "html/template" "io/fs" "os" @@ -23,7 +24,7 @@ var ( ) // RenderEmailTemplate renders the email template for a new share -func RenderEmailTemplate(mt MessageTemplate, locale string, emailTemplatePath string, translationPath string, vars map[string]interface{}) (*channels.Message, error) { +func RenderEmailTemplate(mt MessageTemplate, locale string, emailTemplatePath string, translationPath string, vars map[string]string) (*channels.Message, error) { textMt, err := NewTextTemplate(mt, locale, translationPath, vars) if err != nil { return nil, err @@ -36,8 +37,7 @@ func RenderEmailTemplate(mt MessageTemplate, locale string, emailTemplatePath st if err != nil { return nil, err } - - htmlMt, err := NewHTMLTemplate(mt, locale, translationPath, vars) + htmlMt, err := NewHTMLTemplate(mt, locale, translationPath, escapeStringMap(vars)) if err != nil { return nil, err } @@ -135,3 +135,10 @@ func validateMime(incipit []byte) bool { } return false } + +func escapeStringMap(vars map[string]string) map[string]string { + for k := range vars { + vars[k] = html.EscapeString(vars[k]) + } + return vars +} diff --git a/services/notifications/pkg/email/templates/html/email.html.tmpl b/services/notifications/pkg/email/templates/html/email.html.tmpl index f7345fe96c6..f36f0766693 100644 --- a/services/notifications/pkg/email/templates/html/email.html.tmpl +++ b/services/notifications/pkg/email/templates/html/email.html.tmpl @@ -11,9 +11,8 @@ {{ .Greeting }}

{{ .MessageBody }} - {{if ne .CallToAction "" }} -

{{ .CallToAction }} - {{end}} + {{if ne .CallToAction "" }}

+ {{ .CallToAction }}{{end}} diff --git a/services/notifications/pkg/service/service.go b/services/notifications/pkg/service/service.go index e5f67a3925a..b187f81ddbd 100644 --- a/services/notifications/pkg/service/service.go +++ b/services/notifications/pkg/service/service.go @@ -98,7 +98,7 @@ func (s eventsNotifier) Run() error { } func (s eventsNotifier) render(ctx context.Context, template email.MessageTemplate, - granteeFieldName string, fields map[string]interface{}, granteeList []*user.User, sender string) ([]*channels.Message, error) { + granteeFieldName string, fields map[string]string, granteeList []*user.User, sender string) ([]*channels.Message, error) { // Render the Email Template for each user messageList := make([]*channels.Message, len(granteeList)) for i, usr := range granteeList { diff --git a/services/notifications/pkg/service/service_test.go b/services/notifications/pkg/service/service_test.go index fdc64410c9f..925fcc4bbf7 100644 --- a/services/notifications/pkg/service/service_test.go +++ b/services/notifications/pkg/service/service_test.go @@ -82,7 +82,7 @@ var _ = Describe("Notifications", func() { Entry("Share Created", testChannel{ expectedReceipients: []string{sharee.GetMail()}, expectedSubject: "Dr. S. Harer shared 'secrets of the board' with you", - expectedMessage: `Hello Eric Expireling + expectedTextBody: `Hello Eric Expireling Dr. S. Harer has shared "secrets of the board" with you. @@ -107,7 +107,7 @@ https://owncloud.com Entry("Share Expired", testChannel{ expectedReceipients: []string{sharee.GetMail()}, expectedSubject: "Share to 'secrets of the board' expired at 2023-04-17 16:42:00", - expectedMessage: `Hello Eric Expireling, + expectedTextBody: `Hello Eric Expireling, Your share to secrets of the board has expired at 2023-04-17 16:42:00 @@ -132,7 +132,7 @@ https://owncloud.com Entry("Added to Space", testChannel{ expectedReceipients: []string{sharee.GetMail()}, expectedSubject: "Dr. S. Harer invited you to join secret space", - expectedMessage: `Hello Eric Expireling, + expectedTextBody: `Hello Eric Expireling, Dr. S. Harer has invited you to join "secret space". @@ -157,7 +157,7 @@ https://owncloud.com Entry("Removed from Space", testChannel{ expectedReceipients: []string{sharee.GetMail()}, expectedSubject: "Dr. S. Harer removed you from secret space", - expectedMessage: `Hello Eric Expireling, + expectedTextBody: `Hello Eric Expireling, Dr. S. Harer has removed you from "secret space". @@ -183,7 +183,7 @@ https://owncloud.com Entry("Space Expired", testChannel{ expectedReceipients: []string{sharee.GetMail()}, expectedSubject: "Membership of 'secret space' expired at 2023-04-17 16:42:00", - expectedMessage: `Hello Eric Expireling, + expectedTextBody: `Hello Eric Expireling, Your membership of space secret space has expired at 2023-04-17 16:42:00 @@ -208,11 +208,209 @@ https://owncloud.com ) }) +var _ = Describe("Notifications X-Site Scripting", func() { + var ( + gwc *cs3mocks.GatewayAPIClient + vs *settingssvc.MockValueService + sharer = &user.User{ + Id: &user.UserId{ + OpaqueId: "sharer", + }, + Mail: "sharer@owncloud.com", + DisplayName: "Dr. O'reilly", + } + sharee = &user.User{ + Id: &user.UserId{ + OpaqueId: "sharee", + }, + Mail: "sharee@owncloud.com", + DisplayName: "", + } + resourceid = &provider.ResourceId{ + StorageId: "storageid", + SpaceId: "spaceid", + OpaqueId: "itemid", + } + ) + + BeforeEach(func() { + gwc = &cs3mocks.GatewayAPIClient{} + gwc.On("GetUser", mock.Anything, mock.Anything).Return(&user.GetUserResponse{Status: &rpc.Status{Code: rpc.Code_CODE_OK}, User: sharer}, nil).Once() + gwc.On("GetUser", mock.Anything, mock.Anything).Return(&user.GetUserResponse{Status: &rpc.Status{Code: rpc.Code_CODE_OK}, User: sharee}, nil).Once() + gwc.On("Authenticate", mock.Anything, mock.Anything).Return(&gateway.AuthenticateResponse{Status: &rpc.Status{Code: rpc.Code_CODE_OK}, User: sharer}, nil) + gwc.On("Stat", mock.Anything, mock.Anything).Return(&provider.StatResponse{ + Status: &rpc.Status{Code: rpc.Code_CODE_OK}, + Info: &provider.ResourceInfo{ + Name: "", + Space: &provider.StorageSpace{Name: ""}}, + }, nil) + vs = &settingssvc.MockValueService{} + vs.GetValueByUniqueIdentifiersFunc = func(ctx context.Context, req *settingssvc.GetValueByUniqueIdentifiersRequest, opts ...client.CallOption) (*settingssvc.GetValueResponse, error) { + return nil, nil + } + }) + + DescribeTable("Sending notifications", + func(tc testChannel, ev events.Event) { + cfg := defaults.FullDefaultConfig() + cfg.GRPCClientTLS = &shared.GRPCClientTLS{} + _ = ogrpc.Configure(ogrpc.GetClientOptions(cfg.GRPCClientTLS)...) + ch := make(chan events.Event) + evts := service.NewEventsNotifier(ch, tc, log.NewLogger(), gwc, vs, "", "", "") + go evts.Run() + + ch <- ev + select { + case <-tc.done: + // finished + case <-time.Tick(3 * time.Second): + Fail("timeout waiting for notification") + } + }, + + Entry("Share Created", testChannel{ + expectedReceipients: []string{sharee.GetMail()}, + expectedSubject: "Dr. O'reilly shared '' with you", + expectedTextBody: `Hello + +Dr. O'reilly has shared "" with you. + +Click here to view it: files/shares/with-me + + +--- +ownCloud - Store. Share. Work. +https://owncloud.com +`, + expectedHTMLBody: ` + + + + + + +
+ + + + + + + + + + + + + + + +
  + Hello <script>alert('Eric Expireling');</script> +

+ Dr. O'reilly has shared "<script>alert('secrets of the board');</script>" with you. +

+ Click here to view it: files/shares/with-me +
 
  + +
 
+
+ + +`, + expectedSender: sharer.GetDisplayName(), + + done: make(chan struct{}), + }, events.Event{ + Event: events.ShareCreated{ + Sharer: sharer.GetId(), + GranteeUserID: sharee.GetId(), + CTime: utils.TimeToTS(time.Date(2023, 4, 17, 16, 42, 0, 0, time.UTC)), + ItemID: resourceid, + }, + }), + + Entry("Added to Space", testChannel{ + expectedReceipients: []string{sharee.GetMail()}, + expectedSubject: "Dr. O'reilly invited you to join ", + expectedTextBody: `Hello , + +Dr. O'reilly has invited you to join "". + +Click here to view it: f/spaceid + + +--- +ownCloud - Store. Share. Work. +https://owncloud.com +`, + expectedSender: sharer.GetDisplayName(), + expectedHTMLBody: ` + + + + + + +
+ + + + + + + + + + + + + + + +
  + Hello <script>alert('Eric Expireling');</script>, +

+ Dr. O'reilly has invited you to join "<script>alert('secret space');</script>". +

+ Click here to view it: f/spaceid +
 
  + +
 
+
+ + +`, + done: make(chan struct{}), + }, events.Event{ + Event: events.SpaceShared{ + Executant: sharer.GetId(), + Creator: sharer.GetId(), + GranteeUserID: sharee.GetId(), + ID: &provider.StorageSpaceId{OpaqueId: "spaceid"}, + }, + }), + ) +}) + // NOTE: This is explictitly not testing the message itself. Should we? type testChannel struct { expectedReceipients []string expectedSubject string - expectedMessage string + expectedTextBody string + expectedHTMLBody string expectedSender string done chan struct{} } @@ -220,10 +418,13 @@ type testChannel struct { func (tc testChannel) SendMessage(ctx context.Context, m *channels.Message) error { defer GinkgoRecover() - Expect(m.Recipient).To(Equal(tc.expectedReceipients)) - Expect(m.Subject).To(Equal(tc.expectedSubject)) - Expect(m.TextBody).To(Equal(tc.expectedMessage)) - Expect(m.Sender).To(Equal(tc.expectedSender)) + Expect(tc.expectedReceipients).To(Equal(m.Recipient)) + Expect(tc.expectedSubject).To(Equal(m.Subject)) + Expect(tc.expectedTextBody).To(Equal(m.TextBody)) + Expect(tc.expectedSender).To(Equal(m.Sender)) + if tc.expectedHTMLBody != "" { + Expect(tc.expectedHTMLBody).To(Equal(m.HTMLBody)) + } tc.done <- struct{}{} return nil } diff --git a/services/notifications/pkg/service/shares.go b/services/notifications/pkg/service/shares.go index 1182ea38ee5..c4349436ecd 100644 --- a/services/notifications/pkg/service/shares.go +++ b/services/notifications/pkg/service/shares.go @@ -44,7 +44,7 @@ func (s eventsNotifier) handleShareCreated(e events.ShareCreated) { sharerDisplayName := owner.GetDisplayName() recipientList, err := s.render(ownerCtx, email.ShareCreated, "ShareGrantee", - map[string]interface{}{ + map[string]string{ "ShareSharer": sharerDisplayName, "ShareFolder": resourceInfo.Name, "ShareLink": shareLink, @@ -84,7 +84,7 @@ func (s eventsNotifier) handleShareExpired(e events.ShareExpired) { recipientList, err := s.render(ownerCtx, email.ShareExpired, "ShareGrantee", - map[string]interface{}{ + map[string]string{ "ShareFolder": resourceInfo.GetName(), "ExpiredAt": e.ExpiredAt.Format("2006-01-02 15:04:05"), }, granteeList, owner.GetDisplayName()) diff --git a/services/notifications/pkg/service/spaces.go b/services/notifications/pkg/service/spaces.go index 37af8171330..d764e9f5a86 100644 --- a/services/notifications/pkg/service/spaces.go +++ b/services/notifications/pkg/service/spaces.go @@ -57,7 +57,7 @@ func (s eventsNotifier) handleSpaceShared(e events.SpaceShared) { sharerDisplayName := executant.GetDisplayName() recipientList, err := s.render(executantCtx, email.SharedSpace, "SpaceGrantee", - map[string]interface{}{ + map[string]string{ "SpaceSharer": sharerDisplayName, "SpaceName": resourceInfo.GetSpace().GetName(), "ShareLink": shareLink, @@ -117,7 +117,7 @@ func (s eventsNotifier) handleSpaceUnshared(e events.SpaceUnshared) { sharerDisplayName := executant.GetDisplayName() recipientList, err := s.render(executantCtx, email.UnsharedSpace, "SpaceGrantee", - map[string]interface{}{ + map[string]string{ "SpaceSharer": sharerDisplayName, "SpaceName": resourceInfo.GetSpace().Name, "ShareLink": shareLink, @@ -149,7 +149,7 @@ func (s eventsNotifier) handleSpaceMembershipExpired(e events.SpaceMembershipExp recipientList, err := s.render(ownerCtx, email.MembershipExpired, "SpaceGrantee", - map[string]interface{}{ + map[string]string{ "SpaceName": e.SpaceName, "ExpiredAt": e.ExpiredAt.Format("2006-01-02 15:04:05"), }, granteeList, owner.GetDisplayName())