Merge pull request #6429 from 2403905/issue-6411

Fix to prevent the email notification X-Site scripting
2026-02-22 17:16:28 -05:00 · 2023-06-02 16:57:51 +03:00
parent 4b78e06e04 cd39dd4073
commit ecc56cee49
10 changed files with 242 additions and 27 deletions
--- a/changelog/unreleased/fix-email-xsite-scripting.md
+++ 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
--- 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:
--- 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=
--- 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
--- 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
+}
--- 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 }}
                        <br><br>
                        {{ .MessageBody }}
-                        {{if ne .CallToAction "" }}
-                        <br><br>{{ .CallToAction }}
-                        {{end}}
+                        {{if ne .CallToAction "" }}<br><br>
+                        {{ .CallToAction }}{{end}}
                    </td>
                </tr>
                <tr>
--- 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 {
--- 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: "<script>alert('Eric Expireling');</script>",
+		}
+		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:  "<script>alert('secrets of the board');</script>",
+				Space: &provider.StorageSpace{Name: "<script>alert('secret space');</script>"}},
+		}, 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 '<script>alert('secrets of the board');</script>' with you",
+			expectedTextBody: `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
+
+
+---
+ownCloud - Store. Share. Work.
+https://owncloud.com
+`,
+			expectedHTMLBody: `<!DOCTYPE html>
+<html>
+<body>
+<table cellspacing="0" cellpadding="0" border="0" width="100%">
+    <tr>
+        <td>
+            <table cellspacing="0" cellpadding="0" border="0" width="600px">
+                <tr>
+                    <td width="20px">&nbsp;</td>
+                    <td style="font-weight:normal; font-size:0.8em; line-height:1.2em; font-family:verdana,'arial',sans;">
+                        Hello &lt;script&gt;alert(&#39;Eric Expireling&#39;);&lt;/script&gt;
+                        <br><br>
+                        Dr. O&#39;reilly has shared "&lt;script&gt;alert(&#39;secrets of the board&#39;);&lt;/script&gt;" with you.
+                        <br><br>
+                        <a href="files/shares/with-me">Click here to view it: files/shares/with-me</a>
+                    </td>
+                </tr>
+                <tr>
+                    <td colspan="2">&nbsp;</td>
+                </tr>
+                <tr>
+                    <td width="20px">&nbsp;</td>
+                    <td style="font-weight:normal; font-size:0.8em; line-height:1.2em; font-family:verdana,'arial',sans;">
+                        <footer>
+                            <br>
+                            <br>
+                            --- <br>
+                            ownCloud - Store. Share. Work.<br>
+                            <a href="https://owncloud.com">https://owncloud.com</a>
+                        </footer>
+                    </td>
+                </tr>
+                <tr>
+                    <td colspan="2">&nbsp;</td>
+                </tr>
+            </table>
+        </td>
+    </tr>
+</table>
+</body>
+</html>
+`,
+			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 <script>alert('secret space');</script>",
+			expectedTextBody: `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
+
+
+---
+ownCloud - Store. Share. Work.
+https://owncloud.com
+`,
+			expectedSender: sharer.GetDisplayName(),
+			expectedHTMLBody: `<!DOCTYPE html>
+<html>
+<body>
+<table cellspacing="0" cellpadding="0" border="0" width="100%">
+    <tr>
+        <td>
+            <table cellspacing="0" cellpadding="0" border="0" width="600px">
+                <tr>
+                    <td width="20px">&nbsp;</td>
+                    <td style="font-weight:normal; font-size:0.8em; line-height:1.2em; font-family:verdana,'arial',sans;">
+                        Hello &lt;script&gt;alert(&#39;Eric Expireling&#39;);&lt;/script&gt;,
+                        <br><br>
+                        Dr. O&#39;reilly has invited you to join "&lt;script&gt;alert(&#39;secret space&#39;);&lt;/script&gt;".
+                        <br><br>
+                        <a href="f/spaceid">Click here to view it: f/spaceid</a>
+                    </td>
+                </tr>
+                <tr>
+                    <td colspan="2">&nbsp;</td>
+                </tr>
+                <tr>
+                    <td width="20px">&nbsp;</td>
+                    <td style="font-weight:normal; font-size:0.8em; line-height:1.2em; font-family:verdana,'arial',sans;">
+                        <footer>
+                            <br>
+                            <br>
+                            --- <br>
+                            ownCloud - Store. Share. Work.<br>
+                            <a href="https://owncloud.com">https://owncloud.com</a>
+                        </footer>
+                    </td>
+                </tr>
+                <tr>
+                    <td colspan="2">&nbsp;</td>
+                </tr>
+            </table>
+        </td>
+    </tr>
+</table>
+</body>
+</html>
+`,
+			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
 }
--- 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())
--- 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())