Skip to content

Commit

Permalink
fix(GODT-2638): Validate if messages adhere to rfc5322 header rules
Browse files Browse the repository at this point in the history
See `rfc5322.ValidateMessageHeaderFields`'s comments for more info.
  • Loading branch information
LBeernaertProton committed May 23, 2023
1 parent 633e61c commit 75fd429
Show file tree
Hide file tree
Showing 28 changed files with 391 additions and 192 deletions.
6 changes: 5 additions & 1 deletion internal/session/handle_append.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,12 @@ package session
import (
"context"
"errors"

"github.com/ProtonMail/gluon/imap/command"
"github.com/ProtonMail/gluon/internal/response"
"github.com/ProtonMail/gluon/internal/state"
"github.com/ProtonMail/gluon/profiling"
"github.com/ProtonMail/gluon/reporter"
"github.com/ProtonMail/gluon/rfc5322"
)

func (s *Session) handleAppend(ctx context.Context, tag string, cmd *command.Append, ch chan response.Response) error {
Expand All @@ -25,6 +25,10 @@ func (s *Session) handleAppend(ctx context.Context, tag string, cmd *command.App
return response.Bad(tag).WithError(err)
}

if err := rfc5322.ValidateMessageHeaderFields(cmd.Literal); err != nil {
return response.Bad(tag).WithError(err)
}

if err := s.state.AppendOnlyMailbox(ctx, nameUTF8, func(mailbox state.AppendOnlyMailbox, isSameMBox bool) error {
messageUID, err := mailbox.Append(ctx, cmd.Literal, flags, cmd.DateTime)
if err != nil {
Expand Down
73 changes: 73 additions & 0 deletions rfc5322/validation.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
package rfc5322

import (
"errors"
"fmt"
"github.com/ProtonMail/gluon/rfc822"
)

var ErrInvalidMessage = errors.New("invalid rfc5322 message")

// ValidateMessageHeaderFields checks the headers of message to verify that:
// * From and Date are present.
// * If From has multiple addresses, a Sender field must be present.
// * If Both From and Sender are present and they contain one address, they must not be equal.
func ValidateMessageHeaderFields(literal []byte) error {
headerBytes, _ := rfc822.Split(literal)

header, err := rfc822.NewHeader(headerBytes)
if err != nil {
return err
}

// Check for date.
{
value := header.Get("Date")
if len(value) == 0 {
return fmt.Errorf("%w: Required header field 'Date' not found or empty", ErrInvalidMessage)
}
}

// Check for from.
{
value := header.Get("From")
if len(value) == 0 {
return fmt.Errorf("%w: Required header field 'From' not found or empty", ErrInvalidMessage)
}

// Check if From is a multi address. If so, a sender filed must be present and non-empty.
addresses, err := ParseAddressList(value)
if err != nil {
return fmt.Errorf("%w: failed to parse From header: %v", ErrInvalidMessage, err)
}

if len(addresses) > 1 {
senderValue := header.Get("Sender")
if len(senderValue) == 0 {
return fmt.Errorf("%w: Required header field 'Sender' not found or empty", ErrInvalidMessage)
}
_, err := ParseAddress(senderValue)
if err != nil {
return fmt.Errorf("%w: failed to parse Sender header: %v", ErrInvalidMessage, err)
}
} else {
senderValue, ok := header.GetChecked("Sender")
if ok {
if len(senderValue) == 0 {
return fmt.Errorf("%w: Required header field 'Sender' should not be empty", ErrInvalidMessage)
}

senderAddr, err := ParseAddress(senderValue)
if err != nil {
return fmt.Errorf("%w: failed to parse Sender header: %v", ErrInvalidMessage, err)
}

if len(senderAddr) == 1 && senderAddr[0].Address == addresses[0].Address {
return fmt.Errorf("%w: `Sender` should not be present if equal to `From`", ErrInvalidMessage)
}
}
}
}

return nil
}
54 changes: 54 additions & 0 deletions rfc5322/validation_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
package rfc5322

import (
"github.com/stretchr/testify/require"
"testing"
)

func TestValidateMessageHeaderFields_RequiredFieldsPass(t *testing.T) {
const literal = `From: Foo@bar.com
Date: Mon, 7 Feb 1994 21:52:25 -0800 (PST)
`

require.NoError(t, ValidateMessageHeaderFields([]byte(literal)))
}

func TestValidateMessageHeaderFields_ErrOnMissingFrom(t *testing.T) {
const literal = `Date: Mon, 7 Feb 1994 21:52:25 -0800 (PST)
`

require.Error(t, ValidateMessageHeaderFields([]byte(literal)))
}

func TestValidateMessageHeaderFields_ErrOnMissingDate(t *testing.T) {
const literal = `From: Foo@bar.com
`

require.Error(t, ValidateMessageHeaderFields([]byte(literal)))
}

func TestValidateMessageHeaderFields_ErrOnSingleFromAndSenderEqual(t *testing.T) {
const literal = `From: Foo@bar.com
Date: Mon, 7 Feb 1994 21:52:25 -0800 (PST)
Sender: Foo@bar.com
`

require.Error(t, ValidateMessageHeaderFields([]byte(literal)))
}

func TestValidateMessageHeaderFields_AllowSingleFromWithDifferentSender(t *testing.T) {
const literal = `From: Foo@bar.com
Date: Mon, 7 Feb 1994 21:52:25 -0800 (PST)
Sender: Bar@bar.com
`

require.NoError(t, ValidateMessageHeaderFields([]byte(literal)))
}

func TestValidateMessageHeaderFields_ErrOnMultipleFromAndNoSender(t *testing.T) {
const literal = `From: Foo@bar.com, Bar@bar.com
Date: Mon, 7 Feb 1994 21:52:25 -0800 (PST)
`

require.Error(t, ValidateMessageHeaderFields([]byte(literal)))
}
6 changes: 3 additions & 3 deletions tests/append_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,7 @@ func TestAppendCanHandleOutOfOrderUIDUpdates(t *testing.T) {

appendFN := func(clientIndex int) {
for i := 0; i < MessageCount; i++ {
c[clientIndex+1].doAppend("INBOX", "To: f3@pm.me\r\n", "\\Seen").expect("OK")
c[clientIndex+1].doAppend("INBOX", buildRFC5322TestLiteral("To: f3@pm.me\r\n"), "\\Seen").expect("OK")
}
}

Expand Down Expand Up @@ -414,7 +414,7 @@ func TestGODT2007AppendInternalIDPresentOnDeletedMessage(t *testing.T) {
runOneToOneTestClientWithAuth(t, defaultServerOptions(t), func(client *client.Client, s *testSession) {
// Create message and mark deleted.
mboxID := s.mailboxCreated("user", []string{mailboxName})
messageID := s.messageCreated("user", mboxID, []byte("To: foo@bar.com\r\n"), time.Now())
messageID := s.messageCreated("user", mboxID, []byte(buildRFC5322TestLiteral("To: foo@bar.com\r\n")), time.Now())
s.flush("user")

_, err := client.Select(mailboxName, false)
Expand All @@ -424,7 +424,7 @@ func TestGODT2007AppendInternalIDPresentOnDeletedMessage(t *testing.T) {
// Check if the header is correctly set.
result := newFetchCommand(t, client).withItems("UID", "BODY[HEADER]").fetch("1")
result.forSeqNum(1, func(builder *validatorBuilder) {
builder.ignoreFlags().wantSectionAndSkipGLUONHeaderOrPanic("BODY[HEADER]", "To: foo@bar.com\r\n")
builder.ignoreFlags().wantSectionAndSkipGLUONHeaderOrPanic("BODY[HEADER]", buildRFC5322TestLiteral("To: foo@bar.com\r\n"))
builder.wantUID(1)
})
result.checkAndRequireMessageCount(1)
Expand Down
17 changes: 11 additions & 6 deletions tests/case_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package tests

import (
"fmt"
"testing"
)

Expand All @@ -27,12 +28,16 @@ func TestMailboxCase(t *testing.T) {
c.C(`tag status iNbOx/other (messages)`).OK(`tag`)

// We can append INBOX in any case.
c.C(`tag append INBOX () {11}`).Continue().C(`To: 1@pm.me`).OK(`tag`)
c.C(`tag append inbox () {11}`).Continue().C(`To: 1@pm.me`).OK(`tag`)
c.C(`tag append iNbOx () {11}`).Continue().C(`To: 1@pm.me`).OK(`tag`)
c.C(`tag append INBOX/other () {11}`).Continue().C(`To: 1@pm.me`).OK(`tag`)
c.C(`tag append inbox/other () {11}`).Continue().C(`To: 1@pm.me`).OK(`tag`)
c.C(`tag append iNbOx/other () {11}`).Continue().C(`To: 1@pm.me`).OK(`tag`)

literal := buildRFC5322TestLiteral(`To: 1@pm.me`)
literalLen := len(literal)

c.C(fmt.Sprintf(`tag append INBOX () {%v}`, literalLen)).Continue().C(literal).OK(`tag`)
c.C(fmt.Sprintf(`tag append inbox () {%v}`, literalLen)).Continue().C(literal).OK(`tag`)
c.C(fmt.Sprintf(`tag append iNbOx () {%v}`, literalLen)).Continue().C(literal).OK(`tag`)
c.C(fmt.Sprintf(`tag append INBOX/other () {%v}`, literalLen)).Continue().C(literal).OK(`tag`)
c.C(fmt.Sprintf(`tag append inbox/other () {%v}`, literalLen)).Continue().C(literal).OK(`tag`)
c.C(fmt.Sprintf(`tag append iNbOx/other () {%v}`, literalLen)).Continue().C(literal).OK(`tag`)

// We can list inbox in any case.
c.C(`tag LIST "" "INBOX"`).Sx(`INBOX`).OK(`tag`)
Expand Down
20 changes: 10 additions & 10 deletions tests/close_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,11 @@ func TestClose(t *testing.T) {
c[1].C("b001 CREATE saved-messages")
c[1].S("b001 OK CREATE")

c[1].doAppend(`saved-messages`, `To: 1@pm.me`, `\Seen`).expect("OK")
c[1].doAppend(`saved-messages`, `To: 2@pm.me`).expect("OK")
c[1].doAppend(`saved-messages`, `To: 3@pm.me`, `\Seen`).expect("OK")
c[1].doAppend(`saved-messages`, `To: 4@pm.me`).expect("OK")
c[1].doAppend(`saved-messages`, `To: 5@pm.me`, `\Seen`).expect("OK")
c[1].doAppend(`saved-messages`, buildRFC5322TestLiteral(`To: 1@pm.me`), `\Seen`).expect("OK")
c[1].doAppend(`saved-messages`, buildRFC5322TestLiteral(`To: 2@pm.me`)).expect("OK")
c[1].doAppend(`saved-messages`, buildRFC5322TestLiteral(`To: 3@pm.me`), `\Seen`).expect("OK")
c[1].doAppend(`saved-messages`, buildRFC5322TestLiteral(`To: 4@pm.me`)).expect("OK")
c[1].doAppend(`saved-messages`, buildRFC5322TestLiteral(`To: 5@pm.me`), `\Seen`).expect("OK")

c[1].C(`A002 SELECT saved-messages`)
c[1].Se(`A002 OK [READ-WRITE] SELECT`)
Expand Down Expand Up @@ -92,11 +92,11 @@ func TestCloseWithClient(t *testing.T) {
)
require.NoError(t, client.Create(messageBoxName))

require.NoError(t, client.Append(messageBoxName, []string{goimap.SeenFlag}, time.Now(), strings.NewReader("To: 1@pm.me")))
require.NoError(t, client.Append(messageBoxName, nil, time.Now(), strings.NewReader("To: 2@pm.me")))
require.NoError(t, client.Append(messageBoxName, []string{goimap.SeenFlag}, time.Now(), strings.NewReader("To: 3@pm.me")))
require.NoError(t, client.Append(messageBoxName, nil, time.Now(), strings.NewReader("To: 4@pm.me")))
require.NoError(t, client.Append(messageBoxName, []string{goimap.SeenFlag}, time.Now(), strings.NewReader("To: 5@pm.me")))
require.NoError(t, client.Append(messageBoxName, []string{goimap.SeenFlag}, time.Now(), strings.NewReader(buildRFC5322TestLiteral("To: 1@pm.me"))))
require.NoError(t, client.Append(messageBoxName, nil, time.Now(), strings.NewReader(buildRFC5322TestLiteral("To: 2@pm.me"))))
require.NoError(t, client.Append(messageBoxName, []string{goimap.SeenFlag}, time.Now(), strings.NewReader(buildRFC5322TestLiteral("To: 3@pm.me"))))
require.NoError(t, client.Append(messageBoxName, nil, time.Now(), strings.NewReader(buildRFC5322TestLiteral("To: 4@pm.me"))))
require.NoError(t, client.Append(messageBoxName, []string{goimap.SeenFlag}, time.Now(), strings.NewReader(buildRFC5322TestLiteral("To: 5@pm.me"))))

{
mailboxStatus, err := client.Select(messageBoxName, false)
Expand Down
4 changes: 2 additions & 2 deletions tests/delete_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,8 @@ func TestDeleteMailboxHasChildren(t *testing.T) {
c.C(`A002 CREATE "foo"`).OK(`A002`)
c.C(`A003 CREATE "foo/bar"`).OK(`A003`)

c.doAppend(`foo`, `To: 1@pm.me`).expect("OK")
c.doAppend(`foo/bar`, `To: 2@pm.me`).expect("OK")
c.doAppend(`foo`, buildRFC5322TestLiteral(`To: 1@pm.me`)).expect("OK")
c.doAppend(`foo/bar`, buildRFC5322TestLiteral(`To: 2@pm.me`)).expect("OK")

c.C(`A004 SELECT "INBOX"`).OK(`A004`)
c.C(`A005 LIST "" *`)
Expand Down
8 changes: 4 additions & 4 deletions tests/deleted_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ func TestDeleted(t *testing.T) {
c.S("b001 OK CREATE")

// Create a message in mbox1.
c.doAppend(`mbox1`, `To: 1@pm.me`, `\Seen`).expect("OK")
c.doAppend(`mbox1`, `To: 2@pm.me`, `\Seen`).expect("OK")
c.doAppend(`mbox1`, buildRFC5322TestLiteral(`To: 1@pm.me`), `\Seen`).expect("OK")
c.doAppend(`mbox1`, buildRFC5322TestLiteral(`To: 2@pm.me`), `\Seen`).expect("OK")
c.C(`A002 SELECT mbox1`)
c.Se(`A002 OK [READ-WRITE] SELECT`)

Expand Down Expand Up @@ -85,8 +85,8 @@ func TestUIDDeleted(t *testing.T) {
c.S("b001 OK CREATE")

// Create a message in mbox1
c.doAppend(`mbox1`, `To: 1@pm.me`, `\Seen`).expect("OK")
c.doAppend(`mbox1`, `To: 2@pm.me`, `\Seen`).expect("OK")
c.doAppend(`mbox1`, buildRFC5322TestLiteral(`To: 1@pm.me`), `\Seen`).expect("OK")
c.doAppend(`mbox1`, buildRFC5322TestLiteral(`To: 2@pm.me`), `\Seen`).expect("OK")
c.C(`A002 SELECT mbox1`)
c.Se(`A002 OK [READ-WRITE] SELECT`)

Expand Down
34 changes: 18 additions & 16 deletions tests/deletion_pool_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@ import (

func TestDeletionPool(t *testing.T) {
runManyToOneTestWithAuth(t, defaultServerOptions(t), []int{1, 2, 3, 4}, func(c map[int]*testConnection, s *testSession) {
c[1].doAppend(`INBOX`, `To: 1@pm.me`).expect("OK")
c[1].doAppend(`INBOX`, `To: 2@pm.me`).expect("OK")
c[1].doAppend(`INBOX`, `To: 3@pm.me`).expect("OK")
c[1].doAppend(`INBOX`, buildRFC5322TestLiteral(`To: 1@pm.me`)).expect("OK")
c[1].doAppend(`INBOX`, buildRFC5322TestLiteral(`To: 2@pm.me`)).expect("OK")
c[1].doAppend(`INBOX`, buildRFC5322TestLiteral(`To: 3@pm.me`)).expect("OK")

for _, i := range []int{1, 2, 4} {
c[i].C("A006 SELECT INBOX")
Expand Down Expand Up @@ -64,9 +64,9 @@ func TestDeletionPool(t *testing.T) {

func TestExpungeIssued(t *testing.T) {
runManyToOneTestWithAuth(t, defaultServerOptions(t), []int{1, 2}, func(c map[int]*testConnection, s *testSession) {
c[1].doAppend(`INBOX`, `To: 1@pm.me`).expect("OK")
c[1].doAppend(`INBOX`, `To: 2@pm.me`).expect("OK")
c[1].doAppend(`INBOX`, `To: 3@pm.me`).expect("OK")
c[1].doAppend(`INBOX`, buildRFC5322TestLiteral(`To: 1@pm.me`)).expect("OK")
c[1].doAppend(`INBOX`, buildRFC5322TestLiteral(`To: 2@pm.me`)).expect("OK")
c[1].doAppend(`INBOX`, buildRFC5322TestLiteral(`To: 3@pm.me`)).expect("OK")

// 2 snapshots of INBOX
for i := 1; i <= 2; i++ {
Expand Down Expand Up @@ -115,8 +115,8 @@ func TestExpungeIssued(t *testing.T) {

func TestExpungeUpdate(t *testing.T) {
runManyToOneTestWithAuth(t, defaultServerOptions(t), []int{1, 2}, func(c map[int]*testConnection, s *testSession) {
c[1].doAppend(`INBOX`, `To: 1@pm.me`).expect("OK")
c[1].doAppend(`INBOX`, `To: 2@pm.me`).expect("OK")
c[1].doAppend(`INBOX`, buildRFC5322TestLiteral(`To: 1@pm.me`)).expect("OK")
c[1].doAppend(`INBOX`, buildRFC5322TestLiteral(`To: 2@pm.me`)).expect("OK")

// 2 snapshots of INBOX
for i := 1; i <= 2; i++ {
Expand Down Expand Up @@ -155,7 +155,7 @@ func TestExpungeUpdate(t *testing.T) {
c[2].Sx(`B002 OK \[EXPUNGEISSUED\] command completed in`)

// session 1 adds a message to the box and flags it as answered
c[1].doAppend(`INBOX`, `To: 3@pm.me`).expect(`OK .*`)
c[1].doAppend(`INBOX`, buildRFC5322TestLiteral(`To: 3@pm.me`)).expect("OK")
c[1].C(`A005 STORE 2 +FLAGS (\Answered)`)
c[1].S(`* 2 FETCH (FLAGS (\Answered \Recent))`)
c[1].Sx("A005 OK command completed in")
Expand Down Expand Up @@ -196,10 +196,10 @@ func TestExpungeUpdate(t *testing.T) {
func TestStatusOnUnnotifiedSnapshot(t *testing.T) {
runManyToOneTestWithAuth(t, defaultServerOptions(t), []int{1, 2, 3}, func(c map[int]*testConnection, s *testSession) {
// INBOX with 4 messages
c[1].doAppend(`INBOX`, `To: 1@pm.me`, `\Seen`).expect("OK")
c[1].doAppend(`INBOX`, `To: 2@pm.me`, `\Seen`).expect("OK")
c[1].doAppend(`INBOX`, `To: 3@pm.me`).expect("OK")
c[1].doAppend(`INBOX`, `To: 4@pm.me`).expect("OK")
c[1].doAppend(`INBOX`, buildRFC5322TestLiteral(`To: 1@pm.me`), `\Seen`).expect("OK")
c[1].doAppend(`INBOX`, buildRFC5322TestLiteral(`To: 2@pm.me`), `\Seen`).expect("OK")
c[1].doAppend(`INBOX`, buildRFC5322TestLiteral(`To: 3@pm.me`)).expect("OK")
c[1].doAppend(`INBOX`, buildRFC5322TestLiteral(`To: 4@pm.me`)).expect("OK")

// 2 sessions with INBOX selected (-> 2 snapshots), session 3 is in Authenticated state (no mailbox selected).
for i := 1; i <= 2; i++ {
Expand Down Expand Up @@ -236,7 +236,7 @@ func TestDeletionFlagPropagation(t *testing.T) {
defer done()

// Create a message.
c[1].doAppend(origin, `To: 1@pm.me`).expect(`OK`)
c[1].doAppend(origin, buildRFC5322TestLiteral(`To: 1@pm.me`)).expect(`OK`)

destination, done := c[1].doCreateTempDir()
defer done()
Expand Down Expand Up @@ -267,7 +267,7 @@ func TestDeletionFlagPropagation(t *testing.T) {
func TestDeletionFlagPropagationIDLE(t *testing.T) {
runManyToOneTestWithAuth(t, defaultServerOptions(t), []int{1, 2}, func(c map[int]*testConnection, s *testSession) {
// Create a message.
c[1].doAppend(`INBOX`, `To: 1@pm.me`).expect(`OK`)
c[1].doAppend(`INBOX`, buildRFC5322TestLiteral(`To: 1@pm.me`)).expect(`OK`)

// Create a destination mailbox.
c[1].C("A002 CREATE destination").OK("A002")
Expand Down Expand Up @@ -301,7 +301,7 @@ func TestDeletionFlagPropagationIDLE(t *testing.T) {
func TestDeletionFlagPropagationMulti(t *testing.T) {
runManyToOneTestWithAuth(t, defaultServerOptions(t), []int{1, 2, 3}, func(c map[int]*testConnection, s *testSession) {
// Create a message.
c[1].doAppend(`INBOX`, `To: 1@pm.me`).expect(`OK`)
c[1].doAppend(`INBOX`, buildRFC5322TestLiteral(`To: 1@pm.me`)).expect(`OK`)

// Create two destination mailboxes.
c[1].C("A002 CREATE mbox1").OK("A002")
Expand Down Expand Up @@ -358,13 +358,15 @@ func TestNoopReceivesPendingDeletionUpdates(t *testing.T) {
messageID1 := s.messageCreatedFromFile("user", mailboxID, "testdata/multipart-mixed.eml")
messageID2 := s.messageCreatedFromFile("user", mailboxID, "testdata/afternoon-meeting.eml")

s.flush("user")
// Create a snapshot by selecting in the mailbox.
c.C(`A001 select mbox`).OK(`A001`)

// Remove the messages externally; they're now in the deletion pool.
s.messageRemoved("user", messageID1, mailboxID)
s.messageRemoved("user", messageID2, mailboxID)

s.flush("user")
// Noop should process their deletion.
c.C(`A002 noop`)
c.S(`* 1 EXPUNGE`, `* 1 EXPUNGE`)
Expand Down
Loading

0 comments on commit 75fd429

Please sign in to comment.