Merge pull request #6429 from 2403905/issue-6411

Fix to prevent the email notification X-Site scripting

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

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:

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=

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

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
+}

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>

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 {

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,22 +208,223 @@ 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{}
 }
 
 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
 }