Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented antispam via feedback replyto ratelimiting #341

Merged
merged 2 commits into from
Apr 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
503 changes: 257 additions & 246 deletions server/api/tumdev/campus_backend.pb.go

Large diffs are not rendered by default.

2 changes: 2 additions & 0 deletions server/api/tumdev/campus_backend.proto
Original file line number Diff line number Diff line change
Expand Up @@ -472,6 +472,8 @@ message CreateFeedbackRequest {
Recipient recipient = 1;
// the email address of the user
string from_email = 2;
// how the person wants to be called
string from_name = 8;
// The actual message
string message = 3;
// Optional location which the user can choose (data protection) to attach or not
Expand Down
7 changes: 7 additions & 0 deletions server/api/tumdev/campus_backend.swagger.json
Original file line number Diff line number Diff line change
Expand Up @@ -493,6 +493,13 @@
"required": false,
"type": "string"
},
{
"name": "fromName",
"description": "how the person wants to be called",
"in": "query",
"required": false,
"type": "string"
},
{
"name": "message",
"description": "The actual message",
Expand Down
8 changes: 5 additions & 3 deletions server/backend/cron/feedback_email.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,13 @@ func messageWithHeaders(feedback *model.Feedback) *gomail.Message {
if feedback.Recipient != "" {
m.SetHeader("To", feedback.Recipient)
} else {
m.SetHeader("To", "app@tum.de") // should not ever happen as checked in the api
m.SetAddressHeader("To", "app@tum.de", "TCA Support") // should not ever happen as checked in the api
}
// ReplyTo
if feedback.ReplyTo.Valid {
m.SetHeader("Reply-To", feedback.ReplyTo.String)
if feedback.ReplyToName.Valid && feedback.ReplyToEmail.Valid {
m.SetAddressHeader("Reply-To", feedback.ReplyToEmail.String, feedback.ReplyToName.String)
} else if feedback.ReplyToEmail.Valid {
m.SetHeader("Reply-To", feedback.ReplyToEmail.String)
}
// Timestamp
if feedback.Timestamp.Valid {
Expand Down
46 changes: 24 additions & 22 deletions server/backend/cron/feedback_email_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,31 +23,33 @@ func TestIterate(t *testing.T) {

func fullFeedback() *model.Feedback {
return &model.Feedback{
EmailId: "magic-id",
Recipient: "tca",
ReplyTo: null.StringFrom("test@example.de"),
Feedback: "This is a Test",
ImageCount: 1,
Latitude: null.FloatFrom(0),
Longitude: null.FloatFrom(0),
AppVersion: null.StringFrom("TCA 10.2"),
OsVersion: null.StringFrom("Android 10.0"),
Timestamp: null.TimeFrom(time.Now()),
EmailId: "magic-id",
Recipient: "tca",
ReplyToEmail: null.StringFrom("test@example.de"),
ReplyToName: null.StringFrom("Erika Mustermann"),
Feedback: "This is a Test",
ImageCount: 1,
Latitude: null.FloatFrom(0),
Longitude: null.FloatFrom(0),
AppVersion: null.StringFrom("TCA 10.2"),
OsVersion: null.StringFrom("Android 10.0"),
Timestamp: null.TimeFrom(time.Now()),
}
}

func emptyFeedback() *model.Feedback {
return &model.Feedback{
EmailId: "",
Recipient: "",
ReplyTo: null.String{},
Feedback: "",
ImageCount: 0,
Latitude: null.Float{},
Longitude: null.Float{},
AppVersion: null.String{},
OsVersion: null.String{},
Timestamp: null.Time{},
EmailId: "",
Recipient: "",
ReplyToEmail: null.String{},
ReplyToName: null.String{},
Feedback: "",
ImageCount: 0,
Latitude: null.Float{},
Longitude: null.Float{},
AppVersion: null.String{},
OsVersion: null.String{},
Timestamp: null.Time{},
}
}

Expand All @@ -58,7 +60,7 @@ func TestHeaderInstantiationWithFullFeedback(t *testing.T) {
m := messageWithHeaders(fb)
assert.Equal(t, []string{`"TUM Campus App" <from@example.de>`}, m.GetHeader("From"))
assert.Equal(t, []string{fb.Recipient}, m.GetHeader("To"))
assert.Equal(t, []string{"test@example.de"}, m.GetHeader("Reply-To"))
assert.Equal(t, []string{"\"Erika Mustermann\" <test@example.de>"}, m.GetHeader("Reply-To"))
assert.Equal(t, []string{fb.Timestamp.Time.Format(time.RFC1123Z)}, m.GetHeader("Date"))
assert.Equal(t, []string{"Feedback via the TUM Campus App"}, m.GetHeader("Subject"))
}
Expand All @@ -68,7 +70,7 @@ func TestHeaderInstantiationWithEmptyFeedback(t *testing.T) {
require.NoError(t, os.Setenv("SMTP_FROM", "from@example.de"))
m := messageWithHeaders(emptyFeedback())
assert.Equal(t, []string{`"TUM Campus App" <from@example.de>`}, m.GetHeader("From"))
assert.Equal(t, []string{"app@tum.de"}, m.GetHeader("To"))
assert.Equal(t, []string{"\"TCA Support\" <app@tum.de>"}, m.GetHeader("To"))
assert.Equal(t, []string(nil), m.GetHeader("Reply-To"))
// Date is set to now in messageWithHeaders => checking that this is actually now is a bit tricker
dates := m.GetHeader("Date")
Expand Down
16 changes: 14 additions & 2 deletions server/backend/feedback.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"path"
"slices"
"strings"
"time"

pb "github.com/TUM-Dev/Campus-Backend/server/api/tumdev"
"github.com/TUM-Dev/Campus-Backend/server/backend/cron"
Expand Down Expand Up @@ -62,10 +63,18 @@ func (s *CampusServer) CreateFeedback(stream pb.Campus_CreateFeedbackServer) err
if feedback.Feedback == "" && feedback.ImageCount == 0 {
return status.Error(codes.InvalidArgument, "Please attach an image or feedback for us")
}
if feedback.ReplyToEmail.Valid {
now := time.Now()
fiveMinutesAgo := now.Add(time.Minute * -5).Unix()
lastFeedback, feedbackExisted := s.feedbackEmailLastReuestAt.LoadOrStore(feedback.ReplyToEmail.String, now.Unix())
if feedbackExisted && lastFeedback.(int64) >= fiveMinutesAgo {
return status.Error(codes.ResourceExhausted, fmt.Sprintf("You have already send a feedback recently. Please wait %d seconds", lastFeedback.(int64)-fiveMinutesAgo))
}
}
// save feedback to db
if err := s.db.WithContext(stream.Context()).Transaction(func(tx *gorm.DB) error {
var existingFeeedbackCnt int64
if err := tx.Model(&feedback).Where("receiver=? AND reply_to=? AND feedback=? AND app_version=?", feedback.Recipient, feedback.ReplyTo, feedback.Feedback, feedback.AppVersion).Count(&existingFeeedbackCnt).Error; err != nil {
if err := tx.Model(&feedback).Where("receiver=? AND reply_to_email=? AND feedback=? AND app_version=?", feedback.Recipient, feedback.ReplyToEmail, feedback.Feedback, feedback.AppVersion).Count(&existingFeeedbackCnt).Error; err != nil {
return err
}
if existingFeeedbackCnt != 0 {
Expand Down Expand Up @@ -165,7 +174,10 @@ func mergeFeedback(feedback *model.Feedback, req *pb.CreateFeedbackRequest) {
feedback.Feedback = req.Message
}
if req.FromEmail != "" {
feedback.ReplyTo = null.StringFrom(req.FromEmail)
feedback.ReplyToEmail = null.StringFrom(req.FromEmail)
}
if req.FromName != "" {
feedback.ReplyToName = null.StringFrom(req.FromEmail)
}
}

Expand Down
17 changes: 9 additions & 8 deletions server/backend/feedback_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"os"
"path"
"regexp"
"sync"
"testing"
"time"

Expand Down Expand Up @@ -99,10 +100,10 @@ func (s *FeedbackSuite) Test_CreateFeedback_OneFile() {
}
}(cron.StorageDir)

server := CampusServer{db: s.DB}
server := CampusServer{db: s.DB, feedbackEmailLastReuestAt: &sync.Map{}}
s.mock.ExpectBegin()
returnedTime := time.Now()
s.mock.ExpectQuery(regexp.QuoteMeta("SELECT count(*) FROM `feedback` WHERE receiver=? AND reply_to=? AND feedback=? AND app_version=?")).
s.mock.ExpectQuery(regexp.QuoteMeta("SELECT count(*) FROM `feedback` WHERE receiver=? AND reply_to_email=? AND feedback=? AND app_version=?")).
WithArgs("app@tum.de", "testing@example.com", "Hello with image", nil).
WillReturnRows(sqlmock.NewRows([]string{"count(*)"}))
s.mock.ExpectQuery(regexp.QuoteMeta("INSERT INTO `files` (`name`,`path`,`downloads`,`downloaded`) VALUES (?,?,?,?) RETURNING `url`,`file`")).
Expand All @@ -111,8 +112,8 @@ func (s *FeedbackSuite) Test_CreateFeedback_OneFile() {
s.mock.ExpectQuery(regexp.QuoteMeta("INSERT INTO `files` (`name`,`path`,`downloads`,`downloaded`) VALUES (?,?,?,?) RETURNING `url`,`file`")).
WithArgs("1.png", sqlmock.AnyArg(), 1, true).
WillReturnRows(sqlmock.NewRows([]string{"url", "file"}).AddRow(nil, 1))
s.mock.ExpectQuery(regexp.QuoteMeta("INSERT INTO `feedback` (`image_count`,`email_id`,`receiver`,`reply_to`,`feedback`,`latitude`,`longitude`,`os_version`,`app_version`,`processed`) VALUES (?,?,?,?,?,?,?,?,?,?) RETURNING `timestamp`,`id`")).
WithArgs(2, sqlmock.AnyArg(), "app@tum.de", "testing@example.com", "Hello with image", nil, nil, nil, nil, false).
s.mock.ExpectQuery(regexp.QuoteMeta("INSERT INTO `feedback` (`image_count`,`email_id`,`receiver`,`reply_to_email`,`reply_to_name`,`feedback`,`latitude`,`longitude`,`os_version`,`app_version`,`processed`) VALUES (?,?,?,?,?,?,?,?,?,?,?) RETURNING `timestamp`,`id`")).
WithArgs(2, sqlmock.AnyArg(), "app@tum.de", "testing@example.com", nil, "Hello with image", nil, nil, nil, nil, false).
WillReturnRows(sqlmock.NewRows([]string{"timestamp", "id"}).AddRow(returnedTime, 1))
s.mock.ExpectCommit()

Expand Down Expand Up @@ -146,13 +147,13 @@ func expectFileMatches(t *testing.T, file os.DirEntry, name string, returnedTime
func (s *FeedbackSuite) Test_CreateFeedback_NoImage() {
cron.StorageDir = "test_no_image/"

server := CampusServer{db: s.DB}
server := CampusServer{db: s.DB, feedbackEmailLastReuestAt: &sync.Map{}}
s.mock.ExpectBegin()
s.mock.ExpectQuery(regexp.QuoteMeta("SELECT count(*) FROM `feedback` WHERE receiver=? AND reply_to=? AND feedback=? AND app_version=?")).
s.mock.ExpectQuery(regexp.QuoteMeta("SELECT count(*) FROM `feedback` WHERE receiver=? AND reply_to_email=? AND feedback=? AND app_version=?")).
WithArgs("app@tum.de", "testing@example.com", "Hello without image", nil).
WillReturnRows(sqlmock.NewRows([]string{"count(*)"}))
s.mock.ExpectQuery(regexp.QuoteMeta("INSERT INTO `feedback` (`image_count`,`email_id`,`receiver`,`reply_to`,`feedback`,`latitude`,`longitude`,`os_version`,`app_version`,`processed`) VALUES (?,?,?,?,?,?,?,?,?,?) RETURNING `timestamp`,`id`")).
WithArgs(0, sqlmock.AnyArg(), "app@tum.de", "testing@example.com", "Hello without image", nil, nil, nil, nil, false).
s.mock.ExpectQuery(regexp.QuoteMeta("INSERT INTO `feedback` (`image_count`,`email_id`,`receiver`,`reply_to_email`,`reply_to_name`,`feedback`,`latitude`,`longitude`,`os_version`,`app_version`,`processed`) VALUES (?,?,?,?,?,?,?,?,?,?,?) RETURNING `timestamp`,`id`")).
WithArgs(0, sqlmock.AnyArg(), "app@tum.de", "testing@example.com", nil, "Hello without image", nil, nil, nil, nil, false).
WillReturnRows(sqlmock.NewRows([]string{"timestamp", "id"}).AddRow(time.Now(), 1))
s.mock.ExpectCommit()

Expand Down
32 changes: 32 additions & 0 deletions server/backend/migration/20240405000000.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
package migration

import (
"github.com/go-gormigrate/gormigrate/v2"
"gorm.io/gorm"
)

// migrate20240405000000
// Split the reply-to fields of the feedback model into name and email
func migrate20240405000000() *gormigrate.Migration {
return &gormigrate.Migration{
ID: "20240405000000",
Migrate: func(tx *gorm.DB) error {
if err := tx.Exec("alter table feedback change reply_to reply_to_email text null").Error; err != nil {
return err
}
if err := tx.Exec("alter table feedback add reply_to_name text null default null after reply_to_email").Error; err != nil {
return err
}
return nil
},
Rollback: func(tx *gorm.DB) error {
if err := tx.Exec("alter table feedback change reply_to_email reply_to text null").Error; err != nil {
return err
}
if err := tx.Exec("alter table feedback drop column reply_to_name").Error; err != nil {
return err
}
return nil
},
}
}
1 change: 1 addition & 0 deletions server/backend/migration/migration.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ func manualMigrate(db *gorm.DB) error {
migrate20240319000000(),
migrate20240327000000(),
migrate20240402000000(),
migrate20240405000000(),
}
return gormigrate.New(db, gormigrateOptions, migrations).Migrate()
}
Expand Down
12 changes: 8 additions & 4 deletions server/backend/rpcserver.go
Original file line number Diff line number Diff line change
@@ -1,15 +1,18 @@
package backend

import (
"sync"

pb "github.com/TUM-Dev/Campus-Backend/server/api/tumdev"
log "github.com/sirupsen/logrus"
"gorm.io/gorm"
)

type CampusServer struct {
pb.UnimplementedCampusServer
db *gorm.DB
deviceBuf *deviceBuffer // deviceBuf stores all devices from recent request and flushes them to db
feedbackEmailLastReuestAt *sync.Map
db *gorm.DB
deviceBuf *deviceBuffer // deviceBuf stores all devices from recent request and flushes them to db
}

// Verify that CampusServer implements the pb.CampusServer interface
Expand All @@ -18,7 +21,8 @@ var _ pb.CampusServer = (*CampusServer)(nil)
func New(db *gorm.DB) *CampusServer {
log.Trace("Server starting up")
return &CampusServer{
db: db,
deviceBuf: newDeviceBuffer(),
db: db,
deviceBuf: newDeviceBuffer(),
feedbackEmailLastReuestAt: &sync.Map{},
}
}
4 changes: 0 additions & 4 deletions server/go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -464,12 +464,8 @@ golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8T
golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
google.golang.org/genproto/googleapis/api v0.0.0-20240325203815-454cdb8f5daa h1:Jt1XW5PaLXF1/ePZrznsh/aAUvI7Adfc3LY1dAKlzRs=
google.golang.org/genproto/googleapis/api v0.0.0-20240325203815-454cdb8f5daa/go.mod h1:K4kfzHtI0kqWA79gecJarFtDn/Mls+GxQcg3Zox91Ac=
google.golang.org/genproto/googleapis/api v0.0.0-20240401170217-c3f982113cda h1:b6F6WIV4xHHD0FA4oIyzU6mHWg2WI2X1RBehwa5QN38=
google.golang.org/genproto/googleapis/api v0.0.0-20240401170217-c3f982113cda/go.mod h1:AHcE/gZH76Bk/ROZhQphlRoWo5xKDEtz3eVEO1LfA8c=
google.golang.org/genproto/googleapis/rpc v0.0.0-20240318140521-94a12d6c2237 h1:NnYq6UN9ReLM9/Y01KWNOWyI5xQ9kbIms5GGJVwS/Yc=
google.golang.org/genproto/googleapis/rpc v0.0.0-20240318140521-94a12d6c2237/go.mod h1:WtryC6hu0hhx87FDGxWCDptyssuo68sk10vYjF+T9fY=
google.golang.org/genproto/googleapis/rpc v0.0.0-20240325203815-454cdb8f5daa h1:RBgMaUMP+6soRkik4VoN8ojR2nex2TqZwjSSogic+eo=
google.golang.org/genproto/googleapis/rpc v0.0.0-20240325203815-454cdb8f5daa/go.mod h1:WtryC6hu0hhx87FDGxWCDptyssuo68sk10vYjF+T9fY=
google.golang.org/grpc v1.62.1 h1:B4n+nfKzOICUXMgyrNd19h/I9oH0L1pizfk1d4zSgTk=
Expand Down
25 changes: 13 additions & 12 deletions server/model/feedback.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,19 @@ import (
)

type Feedback struct {
Id int64 `gorm:"column:id;primary_key;AUTO_INCREMENT;type:int;not null;"`
ImageCount int32 `gorm:"column:image_count;type:int;not null;"`
EmailId string `gorm:"column:email_id;type:text;not null"`
Recipient string `gorm:"column:receiver;type:text;not null;uniqueIndex:receiver_reply_to_feedback_app_version_uindex"`
ReplyTo null.String `gorm:"column:reply_to;type:text;null;uniqueIndex:receiver_reply_to_feedback_app_version_uindex"`
Feedback string `gorm:"column:feedback;type:text;not null;uniqueIndex:receiver_reply_to_feedback_app_version_uindex"`
Latitude null.Float `gorm:"column:latitude;type:float;null;"`
Longitude null.Float `gorm:"column:longitude;type:float;null;"`
OsVersion null.String `gorm:"column:os_version;type:text;null;"`
AppVersion null.String `gorm:"column:app_version;type:text;null;uniqueIndex:receiver_reply_to_feedback_app_version_uindex"`
Processed bool `gorm:"column:processed;type:boolean;default:false;not null;"`
Timestamp null.Time `gorm:"column:timestamp;type:timestamp;default:CURRENT_TIMESTAMP;null;"`
Id int64 `gorm:"column:id;primary_key;AUTO_INCREMENT;type:int;not null;"`
ImageCount int32 `gorm:"column:image_count;type:int;not null;"`
EmailId string `gorm:"column:email_id;type:text;not null"`
Recipient string `gorm:"column:receiver;type:text;not null;uniqueIndex:receiver_reply_to_feedback_app_version_uindex"`
ReplyToEmail null.String `gorm:"column:reply_to_email;type:text;null;uniqueIndex:receiver_reply_to_feedback_app_version_uindex"`
ReplyToName null.String `gorm:"column:reply_to_name;type:text;null"`
Feedback string `gorm:"column:feedback;type:text;not null;uniqueIndex:receiver_reply_to_feedback_app_version_uindex"`
Latitude null.Float `gorm:"column:latitude;type:float;null;"`
Longitude null.Float `gorm:"column:longitude;type:float;null;"`
OsVersion null.String `gorm:"column:os_version;type:text;null;"`
AppVersion null.String `gorm:"column:app_version;type:text;null;uniqueIndex:receiver_reply_to_feedback_app_version_uindex"`
Processed bool `gorm:"column:processed;type:boolean;default:false;not null;"`
Timestamp null.Time `gorm:"column:timestamp;type:timestamp;default:CURRENT_TIMESTAMP;null;"`
}

// TableName sets the insert table name for this struct type
Expand Down
Loading