From d8b7f7530c5593b0023d582c999956ce746a87af Mon Sep 17 00:00:00 2001 From: Dmitry Verkhoturov Date: Wed, 1 Jul 2026 12:47:39 +0100 Subject: [PATCH] fix: remove user avatar on deleteme request The delete_me token built by deleteMeCtrl omitted the user's Picture, so the avatar-removal branch in deleteMeRequestCtrl never ran for real requests and avatars survived account deletion. Carry Picture in the token so the stored avatar is removed when the request is processed. Make the removal best-effort: the avatar stores report an already-missing avatar as an error with no distinguishable sentinel, and the user's data is already deleted at that point, so a missing avatar (e.g. a repeated request) no longer fails the whole deletion with a 400. Only remove a well-formed avatar id (".image") so a malformed picture can't make a filesystem-backed store target an unexpected path. --- backend/app/rest/api/admin.go | 24 ++++++-- backend/app/rest/api/admin_test.go | 72 ++++++++++++++++++++++- backend/app/rest/api/rest_private.go | 5 +- backend/app/rest/api/rest_private_test.go | 2 + 4 files changed, 96 insertions(+), 7 deletions(-) diff --git a/backend/app/rest/api/admin.go b/backend/app/rest/api/admin.go index e61924b2..34e32387 100644 --- a/backend/app/rest/api/admin.go +++ b/backend/app/rest/api/admin.go @@ -4,6 +4,7 @@ import ( "fmt" "net/http" "path" + "strings" "time" "github.com/go-pkgz/auth/v2" @@ -122,10 +123,14 @@ func (a *admin) deleteMeRequestCtrl(w http.ResponseWriter, r *http.Request) { } if claims.User.Picture != "" && a.authenticator.AvatarProxy() != nil { - avatarStore := a.authenticator.AvatarProxy().Store - if err = avatarStore.Remove(path.Base(claims.User.Picture)); err != nil { - rest.SendErrorJSON(w, r, http.StatusBadRequest, err, "can't delete user's avatar", rest.ErrInternal) - return + if avatarID := avatarIDFromPicture(claims.User.Picture); avatarID != "" { + // best-effort removal: the user's data is already gone and the avatar store gives no way to tell + // an already-removed avatar from a real failure, so a missing avatar must not fail the deletion + if err = a.authenticator.AvatarProxy().Store.Remove(avatarID); err != nil { + log.Printf("[WARN] can't delete avatar for user %s on site %s: %v", claims.User.ID, audience, err) + } + } else { + log.Printf("[WARN] unexpected avatar picture %q for user %s on site %s, skipping removal", claims.User.Picture, claims.User.ID, audience) } } @@ -133,6 +138,17 @@ func (a *admin) deleteMeRequestCtrl(w http.ResponseWriter, r *http.Request) { R.RenderJSON(w, R.JSON{"user_id": claims.User.ID, "site_id": claims.Audience}) } +// avatarIDFromPicture returns the avatar-store object id for a user picture, or "" if the picture +// does not resolve to a well-formed id (the store names its objects ".image"). Guarding on the +// id shape keeps a malformed picture, e.g. a path sentinel, from making a filesystem-backed store +// target an unexpected path. +func avatarIDFromPicture(picture string) string { + if id := path.Base(picture); strings.HasSuffix(id, ".image") { + return id + } + return "" +} + // PUT /user/{userid}?site=side-id&block=1&ttl=7d - block or unblock user func (a *admin) setBlockCtrl(w http.ResponseWriter, r *http.Request) { userID := r.PathValue("userid") diff --git a/backend/app/rest/api/admin_test.go b/backend/app/rest/api/admin_test.go index 4e48f2e1..ec733e4b 100644 --- a/backend/app/rest/api/admin_test.go +++ b/backend/app/rest/api/admin_test.go @@ -717,7 +717,7 @@ func TestAdmin_DeleteMeRequest(t *testing.T) { }, User: &token.User{ ID: "user1", - Picture: "pic.image", + Picture: "https://demo.remark42.com/api/v1/avatar/pic.image", // production-shaped URL: removal must path.Base it to the avatar id Attributes: map[string]any{ "delete_me": true, }, @@ -747,6 +747,76 @@ func TestAdmin_DeleteMeRequest(t *testing.T) { email, err = srv.DataService.GetUserEmail("remark42", "user1") assert.NoError(t, err) assert.Empty(t, email, "user1 email was deleted") + + assert.NoFileExists(t, os.TempDir()+"/ava-remark42/42/pic.image", "user's avatar should be removed on deleteme") +} + +// a delete_me request whose token carries a picture must still succeed when the avatar is +// already gone from the store: the user data is deleted and a missing avatar is tolerated +func TestAdmin_DeleteMeRequestMissingAvatar(t *testing.T) { + ts, srv, teardown := startupT(t) + defer teardown() + + c1 := store.Comment{Text: "test test #1", Locator: store.Locator{SiteID: "remark42", + URL: "https://radio-t.com/blah"}, User: store.User{Name: "user3 name", ID: "user3"}} + _, err := srv.DataService.Create(c1) + require.NoError(t, err) + + claims := token.Claims{ + SessionOnly: true, + RegisteredClaims: jwt.RegisteredClaims{ + Audience: jwt.ClaimStrings{"remark42"}, + ID: "2345678", + Issuer: "remark42", + NotBefore: jwt.NewNumericDate(time.Now().Add(-1 * time.Minute)), + ExpiresAt: jwt.NewNumericDate(time.Now().Add(30 * time.Minute)), + }, + User: &token.User{ + ID: "user3", + Picture: "missing.image", // no avatar file exists for this picture in the store + Attributes: map[string]any{ + "delete_me": true, + }, + }, + } + + tkn, err := srv.Authenticator.TokenService().Token(claims) + require.NoError(t, err) + + client := http.Client{} + defer client.CloseIdleConnections() + req, err := http.NewRequest(http.MethodGet, fmt.Sprintf("%s/api/v1/admin/deleteme?token=%s", ts.URL, tkn), http.NoBody) + require.NoError(t, err) + req.SetBasicAuth("admin", "password") + resp, err := client.Do(req) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + assert.Equal(t, http.StatusOK, resp.StatusCode, "a missing avatar must not fail the deletion") + + _, err = srv.DataService.User("remark42", "user3", 0, 0, store.User{}) + assert.EqualError(t, err, "no comments for user user3 in store", "user3 comments should be deleted") +} + +func TestAvatarIDFromPicture(t *testing.T) { + tbl := []struct { + name string + picture string + want string + }{ + {"local avatar url", "https://demo.remark42.com/api/v1/avatar/cb42ff493ade696d88a3a590f136ae9e34de7c1b.image", "cb42ff493ade696d88a3a590f136ae9e34de7c1b.image"}, + {"bare avatar id", "pic.image", "pic.image"}, + {"parent sentinel", "https://demo.remark42.com/api/v1/avatar/..", ""}, + {"trailing slash", "https://demo.remark42.com/api/v1/avatar/", ""}, + {"root", "/", ""}, + {"dotdot", "..", ""}, + {"empty", "", ""}, + {"provider url without image suffix", "https://example.com/pic.png", ""}, + } + for _, tc := range tbl { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, avatarIDFromPicture(tc.picture)) + }) + } } func TestAdmin_DeleteMeRequestFailed(t *testing.T) { diff --git a/backend/app/rest/api/rest_private.go b/backend/app/rest/api/rest_private.go index 60569bc6..3f511195 100644 --- a/backend/app/rest/api/rest_private.go +++ b/backend/app/rest/api/rest_private.go @@ -708,8 +708,9 @@ func (s *private) deleteMeCtrl(w http.ResponseWriter, r *http.Request) { NotBefore: jwt.NewNumericDate(time.Now().Add(-1 * time.Minute)), }, User: &token.User{ - ID: user.ID, - Name: user.Name, + ID: user.ID, + Name: user.Name, + Picture: user.Picture, // carried so the avatar can be removed when the request is processed Attributes: map[string]any{ "delete_me": true, // prevents this token from being used for login }, diff --git a/backend/app/rest/api/rest_private_test.go b/backend/app/rest/api/rest_private_test.go index 307bfdb1..fc6276d1 100644 --- a/backend/app/rest/api/rest_private_test.go +++ b/backend/app/rest/api/rest_private_test.go @@ -1512,6 +1512,8 @@ func TestRest_DeleteMe(t *testing.T) { claims, err := srv.Authenticator.TokenService().Parse(tkn) assert.NoError(t, err) assert.Equal(t, "provider1_dev", claims.User.ID) + assert.Equal(t, "http://example.com/pic.png", claims.User.Picture, + "delete_me token must carry the user's picture so the avatar can be removed when the request is processed") assert.Equal(t, "https://demo.remark42.com/web/deleteme.html?token="+tkn, m["link"]) req, err = http.NewRequest(http.MethodPost, fmt.Sprintf("%s/api/v1/deleteme?site=remark42", ts.URL), http.NoBody)