From 95e8bbd5f0434c5c377666524a9a496ac06f783f Mon Sep 17 00:00:00 2001 From: floss4good Date: Sun, 20 Jul 2025 23:52:58 +0200 Subject: [PATCH] fix: make sure to use unaltered fields when saving a shadow copy for updated profiles or comments (#8533) Follow-up of !6977 ### Manual testing - User **S** creates an organization **O** and posts a comment **C** (on a random issue); - User **R** report as abuse the comment **C**, the organization **O** as well as the user **S**; - User **S** changes the content of comment **C** and the description of organization **O** as well as the description of their own profile; - Check (within DB) that shadow copies are being created (and linked to corresponding abuse reports) for comment **C**, organization **O** and user **S** and the content is the one from the moment when the reports were submitted (therefore before the updates made by **S**). Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/8533 Reviewed-by: Gusted Co-authored-by: floss4good Co-committed-by: floss4good --- .../ModerationFeatures/abuse_report.yml | 21 +++++++ .../fixtures/ModerationFeatures/comment.yml | 7 +++ models/fixtures/ModerationFeatures/user.yml | 22 ++++++++ models/issues/comment.go | 4 +- models/issues/moderation.go | 8 ++- models/user/moderation.go | 19 +++++-- models/user/user.go | 4 +- services/issue/comments_test.go | 39 +++++++++++++ services/user/delete.go | 2 +- services/user/email.go | 2 +- services/user/user_test.go | 56 +++++++++++++++++++ 11 files changed, 173 insertions(+), 11 deletions(-) create mode 100644 models/fixtures/ModerationFeatures/abuse_report.yml create mode 100644 models/fixtures/ModerationFeatures/comment.yml create mode 100644 models/fixtures/ModerationFeatures/user.yml diff --git a/models/fixtures/ModerationFeatures/abuse_report.yml b/models/fixtures/ModerationFeatures/abuse_report.yml new file mode 100644 index 0000000000..f2e371ee35 --- /dev/null +++ b/models/fixtures/ModerationFeatures/abuse_report.yml @@ -0,0 +1,21 @@ +- + id: 1 + status: 1 + reporter_id: 2 # @user2 + content_type: 4 # Comment + content_id: 18 # user2/repo2/issues/2#issuecomment-18 + category: 2 # Spam + remarks: The comment I'm reporting is pure SPAM. + shadow_copy_id: null + created_unix: 1752697980 # 2025-07-16 20:33:00 + +- + id: 2 + status: 1 # Open + reporter_id: 2 # @user2 + content_type: 1 # User (users or organizations) + content_id: 1002 # @alexsmith + category: 2 # Spam + remarks: This user just posted a spammy comment on my issue. + shadow_copy_id: null + created_unix: 1752698010 # 2025-07-16 20:33:30 diff --git a/models/fixtures/ModerationFeatures/comment.yml b/models/fixtures/ModerationFeatures/comment.yml new file mode 100644 index 0000000000..a4d41ad997 --- /dev/null +++ b/models/fixtures/ModerationFeatures/comment.yml @@ -0,0 +1,7 @@ +- # This is a spam comment (abusive content), created for testing moderation functionalities. + id: 18 + type: 0 # Standard comment + poster_id: 1002 # @alexsmith + issue_id: 7 # user2/repo2#2 + content: If anyone needs help for promoting their business online using SEO, just contact me (check my profile page). + created_unix: 1752697860 # 2025-07-16 20:31:00 diff --git a/models/fixtures/ModerationFeatures/user.yml b/models/fixtures/ModerationFeatures/user.yml new file mode 100644 index 0000000000..662c61a3e9 --- /dev/null +++ b/models/fixtures/ModerationFeatures/user.yml @@ -0,0 +1,22 @@ +- # This user is a spammer and will create abusive content (for testing moderation functionalities). + id: 1002 + lower_name: alexsmith + name: alexsmith + full_name: Alex Smith + email: alexsmith@example.org + keep_email_private: false + passwd: passwdSalt:password + passwd_hash_algo: dummy + type: 0 + location: '@master@seo.net' + website: http://promote-your-business.biz + pronouns: SEO + salt: passwdSalt + description: I can help you promote your business online using SEO. + created_unix: 1752697800 # 2025-07-16 20:30:00 + is_active: true + is_admin: false + is_restricted: false + avatar: avatar-hash-1002 + avatar_email: alexsmith@example.org + use_custom_avatar: false diff --git a/models/issues/comment.go b/models/issues/comment.go index a81221caf4..523a6ba9b9 100644 --- a/models/issues/comment.go +++ b/models/issues/comment.go @@ -1156,7 +1156,7 @@ func UpdateComment(ctx context.Context, c *Comment, contentVersion int, doer *us defer committer.Close() // If the comment was reported as abusive, a shadow copy should be created before first update. - if err := IfNeededCreateShadowCopyForComment(ctx, c); err != nil { + if err := IfNeededCreateShadowCopyForComment(ctx, c, true); err != nil { return err } @@ -1197,7 +1197,7 @@ func DeleteComment(ctx context.Context, comment *Comment) error { e := db.GetEngine(ctx) // If the comment was reported as abusive, a shadow copy should be created before deletion. - if err := IfNeededCreateShadowCopyForComment(ctx, comment); err != nil { + if err := IfNeededCreateShadowCopyForComment(ctx, comment, false); err != nil { return err } diff --git a/models/issues/moderation.go b/models/issues/moderation.go index 635d295db0..921f770d4d 100644 --- a/models/issues/moderation.go +++ b/models/issues/moderation.go @@ -87,13 +87,19 @@ func IfNeededCreateShadowCopyForIssue(ctx context.Context, issue *Issue) error { // IfNeededCreateShadowCopyForComment checks if for the given comment there are any reports of abusive content submitted // and if found a shadow copy of relevant comment fields will be stored into DB and linked to the above report(s). // This function should be called before a comment is deleted or updated. -func IfNeededCreateShadowCopyForComment(ctx context.Context, comment *Comment) error { +func IfNeededCreateShadowCopyForComment(ctx context.Context, comment *Comment, forUpdates bool) error { shadowCopyNeeded, err := moderation.IsShadowCopyNeeded(ctx, moderation.ReportedContentTypeComment, comment.ID) if err != nil { return err } if shadowCopyNeeded { + if forUpdates { + // get the unaltered comment fields (for updates the provided variable is already altered but not yet saved) + if comment, err = GetCommentByID(ctx, comment.ID); err != nil { + return err + } + } commentData := newCommentData(comment) content, err := json.Marshal(commentData) if err != nil { diff --git a/models/user/moderation.go b/models/user/moderation.go index afda497f02..f9c16a17b3 100644 --- a/models/user/moderation.go +++ b/models/user/moderation.go @@ -73,16 +73,20 @@ var userDataColumnNames = sync.OnceValue(func() []string { // and if found a shadow copy of relevant user fields will be stored into DB and linked to the above report(s). // This function should be called before a user is deleted or updated. // +// In case the User object was already altered before calling this method, just provide the userID and +// nil for unalteredUser; when it is decided that a shadow copy should be created and unalteredUser is nil, +// the user will be retrieved from DB based on the provided userID. +// // For deletions alteredCols argument must be omitted. // // In case of updates it will first checks whether any of the columns being updated (alteredCols argument) // is relevant for moderation purposes (i.e. included in the UserData struct). -func IfNeededCreateShadowCopyForUser(ctx context.Context, user *User, alteredCols ...string) error { +func IfNeededCreateShadowCopyForUser(ctx context.Context, userID int64, unalteredUser *User, alteredCols ...string) error { // TODO: this can be triggered quite often (e.g. by routers/web/repo/middlewares.go SetDiffViewStyle()) shouldCheckIfNeeded := len(alteredCols) == 0 // no columns being updated, therefore a deletion if !shouldCheckIfNeeded { - // for updates we need to go further only if certain column are being changed + // for updates we need to go further only if certain columns are being changed for _, colName := range userDataColumnNames() { if shouldCheckIfNeeded = slices.Contains(alteredCols, colName); shouldCheckIfNeeded { break @@ -94,18 +98,23 @@ func IfNeededCreateShadowCopyForUser(ctx context.Context, user *User, alteredCol return nil } - shadowCopyNeeded, err := moderation.IsShadowCopyNeeded(ctx, moderation.ReportedContentTypeUser, user.ID) + shadowCopyNeeded, err := moderation.IsShadowCopyNeeded(ctx, moderation.ReportedContentTypeUser, userID) if err != nil { return err } if shadowCopyNeeded { - userData := newUserData(user) + if unalteredUser == nil { + if unalteredUser, err = GetUserByID(ctx, userID); err != nil { + return err + } + } + userData := newUserData(unalteredUser) content, err := json.Marshal(userData) if err != nil { return err } - return moderation.CreateShadowCopyForUser(ctx, user.ID, string(content)) + return moderation.CreateShadowCopyForUser(ctx, userID, string(content)) } return nil diff --git a/models/user/user.go b/models/user/user.go index b124572bb6..6b54776adf 100644 --- a/models/user/user.go +++ b/models/user/user.go @@ -927,7 +927,9 @@ func UpdateUserCols(ctx context.Context, u *User, cols ...string) error { // If the user was reported as abusive and any of the columns being updated is relevant // for moderation purposes a shadow copy should be created before first update. - if err := IfNeededCreateShadowCopyForUser(ctx, u, cols...); err != nil { + // Since u is already altered at this point we are sending nil instead as an argument + // so that the unaltered version will be retrieved from DB. + if err := IfNeededCreateShadowCopyForUser(ctx, u.ID, nil, cols...); err != nil { return err } diff --git a/services/issue/comments_test.go b/services/issue/comments_test.go index 8fa410c0f0..fcf06d9ec8 100644 --- a/services/issue/comments_test.go +++ b/services/issue/comments_test.go @@ -8,9 +8,11 @@ import ( "forgejo.org/models/db" issues_model "forgejo.org/models/issues" + "forgejo.org/models/moderation" "forgejo.org/models/unittest" user_model "forgejo.org/models/user" webhook_model "forgejo.org/models/webhook" + "forgejo.org/modules/json" "forgejo.org/modules/setting" "forgejo.org/modules/test" issue_service "forgejo.org/services/issue" @@ -148,3 +150,40 @@ func TestUpdateComment(t *testing.T) { unittest.AssertNotExistsBean(t, &issues_model.ContentHistory{CommentID: comment.ID}) }) } + +func TestCreateShadowCopyOnCommentUpdate(t *testing.T) { + defer unittest.OverrideFixtures("models/fixtures/ModerationFeatures")() + require.NoError(t, unittest.PrepareTestDatabase()) + + userAlexSmithID := int64(1002) + spamCommentID := int64(18) // posted by @alexsmith + abuseReportID := int64(1) // submitted for above comment + newCommentContent := "If anyone needs help, just contact me." + + // Retrieve the abusive user (@alexsmith), their SPAM comment and the abuse report already created for this comment. + poster := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: userAlexSmithID}) + comment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: spamCommentID, PosterID: poster.ID}) + report := unittest.AssertExistsAndLoadBean(t, &moderation.AbuseReport{ + ID: abuseReportID, + ContentType: moderation.ReportedContentTypeComment, + ContentID: comment.ID, + }) + // The report should not already have a shadow copy linked. + assert.False(t, report.ShadowCopyID.Valid) + + // The abusive user is updating their comment. + oldContent := comment.Content + comment.Content = newCommentContent + require.NoError(t, issue_service.UpdateComment(t.Context(), comment, 0, poster, oldContent)) + + // Reload the report. + report = unittest.AssertExistsAndLoadBean(t, &moderation.AbuseReport{ID: report.ID}) + // A shadow copy should have been created and linked to our report. + assert.True(t, report.ShadowCopyID.Valid) + // Retrieve the newly created shadow copy and unmarshal the stored JSON so that we can check the values. + shadowCopy := unittest.AssertExistsAndLoadBean(t, &moderation.AbuseReportShadowCopy{ID: report.ShadowCopyID.Int64}) + shadowCopyCommentData := new(issues_model.CommentData) + require.NoError(t, json.Unmarshal([]byte(shadowCopy.RawValue), &shadowCopyCommentData)) + // Check to see if the initial content of the comment was stored within the shadow copy. + assert.Equal(t, oldContent, shadowCopyCommentData.Content) +} diff --git a/services/user/delete.go b/services/user/delete.go index 9caa24c373..bed7abde07 100644 --- a/services/user/delete.go +++ b/services/user/delete.go @@ -218,7 +218,7 @@ func deleteUser(ctx context.Context, u *user_model.User, purge bool) (err error) // ***** END: ExternalLoginUser ***** // If the user was reported as abusive, a shadow copy should be created before deletion. - if err = user_model.IfNeededCreateShadowCopyForUser(ctx, u); err != nil { + if err = user_model.IfNeededCreateShadowCopyForUser(ctx, u.ID, u); err != nil { return err } diff --git a/services/user/email.go b/services/user/email.go index 7a01fa77b3..36a1145aec 100644 --- a/services/user/email.go +++ b/services/user/email.go @@ -205,7 +205,7 @@ func MakeEmailAddressPrimary(ctx context.Context, u *user_model.User, newPrimary oldPrimaryEmail := u.Email // If the user was reported as abusive, a shadow copy should be created before first update (of certain columns). - if err = user_model.IfNeededCreateShadowCopyForUser(ctx, u, "email"); err != nil { + if err = user_model.IfNeededCreateShadowCopyForUser(ctx, u.ID, u, "email"); err != nil { return err } diff --git a/services/user/user_test.go b/services/user/user_test.go index 4678d3bc9a..36f2776ad8 100644 --- a/services/user/user_test.go +++ b/services/user/user_test.go @@ -15,10 +15,13 @@ import ( asymkey_model "forgejo.org/models/asymkey" "forgejo.org/models/auth" "forgejo.org/models/db" + "forgejo.org/models/moderation" "forgejo.org/models/organization" repo_model "forgejo.org/models/repo" "forgejo.org/models/unittest" user_model "forgejo.org/models/user" + "forgejo.org/modules/json" + "forgejo.org/modules/optional" "forgejo.org/modules/setting" "forgejo.org/modules/test" "forgejo.org/modules/timeutil" @@ -277,3 +280,56 @@ func TestDeleteInactiveUsers(t *testing.T) { unittest.AssertExistsIf(t, true, newUser) unittest.AssertExistsIf(t, true, newEmail) } + +func TestCreateShadowCopyOnUserUpdate(t *testing.T) { + defer unittest.OverrideFixtures("models/fixtures/ModerationFeatures")() + require.NoError(t, unittest.PrepareTestDatabase()) + + userAlexSmithID := int64(1002) + abuseReportID := int64(2) // submitted for @alexsmith + newDummyValue := "[REDACTED]" // used for updating profile text fields + + // Retrieve the abusive user (@alexsmith) and the abuse report already created for this user. + abuser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: userAlexSmithID}) + report := unittest.AssertExistsAndLoadBean(t, &moderation.AbuseReport{ + ID: abuseReportID, + ContentType: moderation.ReportedContentTypeUser, + ContentID: abuser.ID, + }) + // The report should not already have a shadow copy linked. + assert.False(t, report.ShadowCopyID.Valid) + + // Keep a copy of old field values before updating them. + oldUserData := user_model.UserData{ + FullName: abuser.FullName, + Location: abuser.Location, + Website: abuser.Website, + Pronouns: abuser.Pronouns, + Description: abuser.Description, + } + + // The abusive user is updating their profile. + opts := &UpdateOptions{ + FullName: optional.Some(newDummyValue), + Location: optional.Some(newDummyValue), + Website: optional.Some(newDummyValue), + Pronouns: optional.Some(newDummyValue), + Description: optional.Some(newDummyValue), + } + require.NoError(t, UpdateUser(t.Context(), abuser, opts)) + + // Reload the report. + report = unittest.AssertExistsAndLoadBean(t, &moderation.AbuseReport{ID: report.ID}) + // A shadow copy should have been created and linked to our report. + assert.True(t, report.ShadowCopyID.Valid) + // Retrieve the newly created shadow copy and unmarshal the stored JSON so that we can check the values. + shadowCopy := unittest.AssertExistsAndLoadBean(t, &moderation.AbuseReportShadowCopy{ID: report.ShadowCopyID.Int64}) + shadowCopyUserData := new(user_model.UserData) + require.NoError(t, json.Unmarshal([]byte(shadowCopy.RawValue), &shadowCopyUserData)) + // Check to see if the initial field values of the user were stored within the shadow copy. + assert.Equal(t, oldUserData.FullName, shadowCopyUserData.FullName) + assert.Equal(t, oldUserData.Location, shadowCopyUserData.Location) + assert.Equal(t, oldUserData.Website, shadowCopyUserData.Website) + assert.Equal(t, oldUserData.Pronouns, shadowCopyUserData.Pronouns) + assert.Equal(t, oldUserData.Description, shadowCopyUserData.Description) +}