diff --git a/server/controllers/email/send.go b/server/controllers/email/send.go index 0d57dc9..129d6bc 100644 --- a/server/controllers/email/send.go +++ b/server/controllers/email/send.go @@ -5,6 +5,7 @@ import ( "encoding/base64" "fmt" "git.pcux.cn/xuans/Smail/config" + "git.pcux.cn/xuans/Smail/consts" "git.pcux.cn/xuans/Smail/db" "git.pcux.cn/xuans/Smail/dto/parsemail" "git.pcux.cn/xuans/Smail/dto/response" @@ -24,6 +25,7 @@ import ( "strings" "time" "unicode" + "xorm.io/xorm" ) const ( @@ -159,9 +161,9 @@ func Send(ctx *context.Context, w http.ResponseWriter, req *http.Request) { } } - e.Text = []byte(reqData.Text) - e.HTML = []byte(reqData.HTML) - e.Subject = reqData.Subject + e.Text = []byte(parsemail.SanitizeText(reqData.Text)) + e.HTML = []byte(parsemail.SanitizeHTML(reqData.HTML)) + e.Subject = parsemail.SanitizeText(reqData.Subject) totalAttachmentSize := 0 for _, att := range reqData.Attachments { contentType, content, err := decodeAttachment(att.Data) @@ -222,8 +224,9 @@ func Send(ctx *context.Context, w http.ResponseWriter, req *http.Request) { return } + now := time.Now() modelEmail := models.Email{ - Type: 1, + Type: consts.EmailTypeSend, Subject: e.Subject, ReplyTo: storedFields.ReplyTo, FromName: e.From.Name, @@ -238,18 +241,38 @@ func Send(ctx *context.Context, w http.ResponseWriter, req *http.Request) { SPFCheck: 1, DKIMCheck: 1, SendUserID: ctx.UserID, - SendDate: time.Now(), - CronSendTime: time.Now(), - Status: 1, - CreateTime: time.Now(), + SendDate: now, + CronSendTime: now, + Status: consts.EmailStatusSent, + CreateTime: now, MsgID: msgID, } - _, err = db.Instance.Insert(&modelEmail) + err = db.Transaction(db.Instance, func(session *xorm.Session) error { + candidate := modelEmail + candidate.Id = 0 + if _, insertErr := session.Insert(&candidate); insertErr != nil { + return insertErr + } + if candidate.Id <= 0 { + return fmt.Errorf("insert outgoing email returned no identifier") + } + relation := models.UserEmail{ + UserID: ctx.UserID, + EmailID: candidate.Id, + Status: consts.EmailStatusSent, + IsRead: 1, + } + if _, insertErr := session.Insert(&relation); insertErr != nil { + return insertErr + } + modelEmail = candidate + return nil + }) if err != nil || modelEmail.Id <= 0 { if err != nil { - log.WithContext(ctx).WithError(err).Error("insert outgoing email failed") + log.WithContext(ctx).WithError(err).Error("insert outgoing email transaction failed") } else { log.WithContext(ctx).Error("insert outgoing email returned no identifier") } @@ -280,36 +303,29 @@ func Send(ctx *context.Context, w http.ResponseWriter, req *http.Request) { if err != nil { errMsg = err.Error() - _, err := db.Instance.Exec(db.WithContext(ctx, "update email set status =2 ,error=? where id = ? "), errMsg, modelEmail.Id) - if err != nil { - log.WithContext(ctx).Errorf("sql Error :%+v", err) - } - - ue := models.UserEmail{ - UserID: ctx.UserID, - EmailID: modelEmail.Id, - Status: 2, - IsRead: 1, - } - if _, insertErr := db.Instance.Insert(&ue); insertErr != nil { - log.WithContext(ctx).WithError(insertErr).Error("insert failed outgoing email relation failed") - } - + _ = db.Transaction(db.Instance, func(session *xorm.Session) error { + if _, updateErr := session.Exec(db.WithContext(ctx, "update email set status = ?, error = ? where id = ?"), consts.EmailStatusFail, errMsg, modelEmail.Id); updateErr != nil { + log.WithContext(ctx).Errorf("update email status failed: %+v", updateErr) + return updateErr + } + if _, updateErr := session.Exec(db.WithContext(ctx, "update user_email set status = ? where email_id = ? and user_id = ?"), consts.EmailStatusFail, modelEmail.Id, ctx.UserID); updateErr != nil { + log.WithContext(ctx).Errorf("update user_email status failed: %+v", updateErr) + return updateErr + } + return nil + }) } else { - _, err := db.Instance.Exec(db.WithContext(ctx, "update email set status =1 where id = ? "), modelEmail.Id) - if err != nil { - log.WithContext(ctx).Errorf("sql Error :%+v", err) - } - - ue := models.UserEmail{ - UserID: ctx.UserID, - EmailID: modelEmail.Id, - Status: 1, - IsRead: 1, - } - if _, insertErr := db.Instance.Insert(&ue); insertErr != nil { - log.WithContext(ctx).WithError(insertErr).Error("insert sent email relation failed") - } + _ = db.Transaction(db.Instance, func(session *xorm.Session) error { + if _, updateErr := session.Exec(db.WithContext(ctx, "update email set status = ?, error = '' where id = ?"), consts.EmailStatusSent, modelEmail.Id); updateErr != nil { + log.WithContext(ctx).Errorf("update email status failed: %+v", updateErr) + return updateErr + } + if _, updateErr := session.Exec(db.WithContext(ctx, "update user_email set status = ? where email_id = ? and user_id = ?"), consts.EmailStatusSent, modelEmail.Id, ctx.UserID); updateErr != nil { + log.WithContext(ctx).Errorf("update user_email status failed: %+v", updateErr) + return updateErr + } + return nil + }) } }, nil) diff --git a/server/controllers/email/send_test.go b/server/controllers/email/send_test.go new file mode 100644 index 0000000..ce1d459 --- /dev/null +++ b/server/controllers/email/send_test.go @@ -0,0 +1,108 @@ +package email + +import ( + "testing" + "time" + + "git.pcux.cn/xuans/Smail/consts" + "git.pcux.cn/xuans/Smail/db" + "git.pcux.cn/xuans/Smail/models" + "git.pcux.cn/xuans/Smail/utils/context" + "xorm.io/xorm" +) + +func TestSendTransactionalPersistenceConsistency(t *testing.T) { + engine, err := xorm.NewEngine("sqlite", "file:"+t.Name()+"?mode=memory&cache=shared") + if err != nil { + t.Fatalf("create test engine: %v", err) + } + engine.SetMaxOpenConns(1) + if err := engine.Sync2(&models.Email{}, &models.UserEmail{}); err != nil { + t.Fatalf("sync test database: %v", err) + } + defer engine.Close() + + ctx := &context.Context{ + UserID: 1, + UserAccount: "testuser", + UserName: "Test User", + } + + now := time.Now() + modelEmail := models.Email{ + Type: consts.EmailTypeSend, + Subject: "Test Email", + FromName: "Test User", + FromAddress: "testuser@example.com", + SendUserID: ctx.UserID, + SendDate: now, + CronSendTime: now, + Status: consts.EmailStatusSent, + CreateTime: now, + MsgID: "msg-123@example.com", + } + + err = db.Transaction(engine, func(session *xorm.Session) error { + candidate := modelEmail + candidate.Id = 0 + if _, insertErr := session.Insert(&candidate); insertErr != nil { + return insertErr + } + relation := models.UserEmail{ + UserID: ctx.UserID, + EmailID: candidate.Id, + Status: consts.EmailStatusSent, + IsRead: 1, + } + if _, insertErr := session.Insert(&relation); insertErr != nil { + return insertErr + } + modelEmail = candidate + return nil + }) + if err != nil { + t.Fatalf("insert email transaction failed: %v", err) + } + + var email models.Email + has, err := engine.ID(modelEmail.Id).Get(&email) + if err != nil || !has { + t.Fatalf("email not found in database: %v", err) + } + if email.Status != consts.EmailStatusSent || email.Type != consts.EmailTypeSend { + t.Fatalf("unexpected email status/type: status=%d, type=%d", email.Status, email.Type) + } + + var userEmail models.UserEmail + has, err = engine.Where("email_id = ? AND user_id = ?", modelEmail.Id, ctx.UserID).Get(&userEmail) + if err != nil || !has { + t.Fatalf("user_email not found: %v", err) + } + if userEmail.Status != consts.EmailStatusSent { + t.Fatalf("unexpected user_email status: %d", userEmail.Status) + } +} + +func TestSafeAttachmentName(t *testing.T) { + tests := []struct { + name string + input string + expected string + }{ + {name: "normal", input: "test.pdf", expected: "test.pdf"}, + {name: "path traversal", input: "../../etc/passwd", expected: ".._.._etc_passwd"}, + {name: "windows path", input: "C:\\Users\\file.txt", expected: "C:_Users_file.txt"}, + {name: "control chars", input: "file\r\nname\x00.png", expected: "file__name_.png"}, + {name: "empty", input: "", expected: "attachment"}, + {name: "dots", input: "..", expected: "attachment"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := safeAttachmentName(tt.input) + if result != tt.expected { + t.Errorf("safeAttachmentName(%q) = %q, want %q", tt.input, result, tt.expected) + } + }) + } +} diff --git a/server/dto/parsemail/email.go b/server/dto/parsemail/email.go index 6928878..799e84b 100644 --- a/server/dto/parsemail/email.go +++ b/server/dto/parsemail/email.go @@ -195,7 +195,7 @@ func init() { relaxedPolicy.SkipElementsContent("script", "object", "embed", "iframe", "frame", "frameset") } -func sanitizeHTML(htmlContent string) string { +func SanitizeHTML(htmlContent string) string { if htmlContent == "" { return "" } @@ -269,7 +269,7 @@ func sanitizeHTMLNode(node *xhtml.Node) bool { } // Sanitize Text -func sanitizeText(text string) string { +func SanitizeText(text string) string { return strictPolicy.Sanitize(text) } @@ -461,8 +461,8 @@ func formatContent(entity *message.Entity, ret *Email) error { fileName := getFileName(entity, p) ret.Attachments = append(ret.Attachments, &Attachment{ - Filename: sanitizeText(fileName), - ContentType: sanitizeText(strings.TrimSpace(contentType)), + Filename: SanitizeText(fileName), + ContentType: SanitizeText(strings.TrimSpace(contentType)), Content: body, ContentID: strings.TrimPrefix(strings.TrimSuffix(entity.Header.Get("Content-Id"), ">"), "<"), }) @@ -481,8 +481,8 @@ func formatContent(entity *message.Entity, ret *Email) error { fileName := getFileName(entity, p) ret.Attachments = append(ret.Attachments, &Attachment{ - Filename: sanitizeText(fileName), - ContentType: sanitizeText(strings.TrimSpace(contentType)), + Filename: SanitizeText(fileName), + ContentType: SanitizeText(strings.TrimSpace(contentType)), Content: body, ContentID: strings.TrimPrefix(strings.TrimSuffix(entity.Header.Get("Content-Id"), ">"), "<"), }) @@ -490,7 +490,7 @@ func formatContent(entity *message.Entity, ret *Email) error { return nil } - ret.HTML = []byte(sanitizeHTML(string(body))) + ret.HTML = []byte(SanitizeHTML(string(body))) case "multipart/related": default: c, err := io.ReadAll(entity.Body) @@ -500,8 +500,8 @@ func formatContent(entity *message.Entity, ret *Email) error { fileName := getFileName(entity, p) ret.Attachments = append(ret.Attachments, &Attachment{ - Filename: sanitizeText(fileName), - ContentType: sanitizeText(strings.TrimSpace(contentType)), + Filename: SanitizeText(fileName), + ContentType: SanitizeText(strings.TrimSpace(contentType)), Content: c, ContentID: strings.TrimPrefix(strings.TrimSuffix(entity.Header.Get("Content-Id"), ">"), "<"), }) diff --git a/server/dto/parsemail/email_xss_test.go b/server/dto/parsemail/email_xss_test.go index 70f9fe6..c8b5c72 100644 --- a/server/dto/parsemail/email_xss_test.go +++ b/server/dto/parsemail/email_xss_test.go @@ -85,9 +85,9 @@ func TestSanitizeHTML(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - result := sanitizeHTML(tc.input) + result := SanitizeHTML(tc.input) if result != tc.expected { - t.Errorf("sanitizeHTML(%q) = %q, want %q", tc.input, result, tc.expected) + t.Errorf("SanitizeHTML(%q) = %q, want %q", tc.input, result, tc.expected) } }) } @@ -143,9 +143,9 @@ func TestSanitizeText(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - result := sanitizeText(tc.input) + result := SanitizeText(tc.input) if result != tc.expected { - t.Errorf("sanitizeText(%q) = %q, want %q", tc.input, result, tc.expected) + t.Errorf("SanitizeText(%q) = %q, want %q", tc.input, result, tc.expected) } }) } @@ -328,13 +328,13 @@ func TestAttachment_XSS(t *testing.T) { // Test that sanitizeText function works correctly on attachment data dangerousFilename := `malicious.txt` - sanitizedFilename := sanitizeText(dangerousFilename) + sanitizedFilename := SanitizeText(dangerousFilename) if strings.Contains(sanitizedFilename, "` - sanitizedContentType := sanitizeText(dangerousContentType) + sanitizedContentType := SanitizeText(dangerousContentType) if strings.Contains(sanitizedContentType, "