Skip to content

Commit

Permalink
Fix to prevent the email notification X-Site scripting
Browse files Browse the repository at this point in the history
  • Loading branch information
2403905 committed Jun 1, 2023
1 parent 798fb56 commit e6ec327
Show file tree
Hide file tree
Showing 9 changed files with 238 additions and 27 deletions.
6 changes: 6 additions & 0 deletions changelog/unreleased/fix-email-xsite-scripting.md
Original file line number Diff line number Diff line change
@@ -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/6386
https://github.com/owncloud/ocis/issues/6429
2 changes: 0 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -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=
Expand Down
6 changes: 3 additions & 3 deletions services/notifications/pkg/email/composer.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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
Expand Down
13 changes: 10 additions & 3 deletions services/notifications/pkg/email/email.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package email
import (
"bytes"
"embed"
"html"
"html/template"
"io/fs"
"os"
Expand All @@ -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
Expand All @@ -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
}
Expand Down Expand Up @@ -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
}
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
2 changes: 1 addition & 1 deletion services/notifications/pkg/service/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
221 changes: 211 additions & 10 deletions services/notifications/pkg/service/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
Expand All @@ -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".
Expand All @@ -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".
Expand All @@ -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
Expand All @@ -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: "[email protected]",
DisplayName: "Dr. O'reilly",
}
sharee = &user.User{
Id: &user.UserId{
OpaqueId: "sharee",
},
Mail: "[email protected]",
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
}
4 changes: 2 additions & 2 deletions services/notifications/pkg/service/shares.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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())
Expand Down
Loading

0 comments on commit e6ec327

Please sign in to comment.