From f579497c81fc95df836055ee4be3408d85e6b5f9 Mon Sep 17 00:00:00 2001 From: Matthew Meszaros Date: Thu, 3 Sep 2026 01:06:36 -0700 Subject: [PATCH 1/2] feat: make profile and workspace avatar changes survive a refresh: user avatar upload and remove go through the user service so the cached /auth/me copy is dropped instead of serving the old avatar for 30 minutes, the org list mapper carries avatar_url into the persisted workspace pointer it previously discarded, the avatar mutation hooks patch the cached user, org pointer and org queries with the server's answer before invalidating, all four handlers delete the replaced or removed object under our own avatars/ prefix with millisecond keys so quick replacements cannot collide on an immutable URL, the org avatar removal is audited, the uploader copy stops promising WebP and GIF, and the API docs note that remove deletes the stored file (fixes #295) --- .../docs/api/reference/account-org.mdx | 4 +- internal/api/handler/avatar.go | 106 +++++++++++++++--- internal/app/user/onboarding.go | 13 +++ internal/app/user/service.go | 1 + .../components/app/avatar/AvatarUploader.tsx | 2 +- .../app/organizations/getOrganizations.ts | 2 + .../lib/api/hooks/app/avatar/useOrgAvatar.ts | 41 +++++-- .../lib/api/hooks/app/avatar/useUserAvatar.ts | 18 +-- .../models/app/organizations/Organization.ts | 2 + 9 files changed, 157 insertions(+), 32 deletions(-) diff --git a/docs/content/docs/api/reference/account-org.mdx b/docs/content/docs/api/reference/account-org.mdx index 6f6003d9..53475676 100644 --- a/docs/content/docs/api/reference/account-org.mdx +++ b/docs/content/docs/api/reference/account-org.mdx @@ -470,7 +470,7 @@ Auth: Session only (not available to API keys). `DELETE /auth/me/avatar` -Clears the profile image. Returns `204 No Content`. +Clears the profile image and deletes the stored file. Returns `204 No Content`. Auth: Session only (not available to API keys). @@ -1456,7 +1456,7 @@ Auth: Session only (not available to API keys). Requires a selected organization `DELETE /organization/avatar` -Clears the workspace image. Owner only. Returns `204 No Content`. +Clears the workspace image and deletes the stored file. Owner only. Returns `204 No Content`. Auth: Session only (not available to API keys). Requires a selected organization (owner only). diff --git a/internal/api/handler/avatar.go b/internal/api/handler/avatar.go index a2231afd..d4b22fb9 100644 --- a/internal/api/handler/avatar.go +++ b/internal/api/handler/avatar.go @@ -8,11 +8,14 @@ // Constants: // // - max size: 2 MiB. Anything larger gets a 400. -// - accepted MIME: image/png, image/jpeg, image/webp, image/gif. -// - object key: avatars/{kind}/{id}-{epoch}.{ext} +// - accepted MIME: image/png, image/jpeg (see allowedAvatarMIME). +// - object key: avatars/{kind}/{id}-{epoch_ms}.{ext} // // The epoch suffix forces cache busting on replacement so the -// browser doesn't keep showing the old avatar at the same URL. +// browser doesn't keep showing the old avatar at the same URL (the +// objects are served immutable, so a reused key would never refresh). +// The previous object is deleted best-effort once the row points at +// the new one, so replacing or removing an avatar doesn't leak blobs. package handler @@ -42,6 +45,9 @@ import ( const ( avatarMaxBytes int64 = 2 * 1024 * 1024 avatarMaxDimension = 1024 // px — reject anything bigger so a phone-camera dump doesn't sneak through + + userAvatarKeyPrefix = "avatars/users/" + orgAvatarKeyPrefix = "avatars/organizations/" ) // Intentionally narrow allowlist: only PNG and JPEG. WebP, GIF and @@ -77,17 +83,23 @@ func (h *Handler) UploadUserAvatar(c *gin.Context) { return } - key := fmt.Sprintf("avatars/users/%s-%d%s", userID.String(), time.Now().Unix(), ext) - url, xerr := putPublicObject(c.Request.Context(), h.Storage, key, bytesRead, mime) + ctx := c.Request.Context() + previous := h.currentUserAvatarURL(ctx, userID) + + key := fmt.Sprintf("%s%s-%d%s", userAvatarKeyPrefix, userID.String(), time.Now().UnixMilli(), ext) + url, xerr := putPublicObject(ctx, h.Storage, key, bytesRead, mime) if xerr != nil { errx.Handle(c, xerr) return } - if err := h.UserRepo.UpdateAvatar(c.Request.Context(), userID, &url); err != nil { - errx.Handle(c, errx.InternalError()) + // Through the service, not the repo: /auth/me is served from a cached + // copy, and only the service drops it. + if xerr := h.UserService.UpdateAvatar(ctx, userID, &url); xerr != nil { + errx.Handle(c, xerr) return } + h.deleteAvatarObject(ctx, previous, userAvatarKeyPrefix, key) h.auditOrg(c, models.AuditActionUpdate, models.AuditEntityUser, &userID, nil, map[string]string{"field": "avatar_url"}) @@ -103,10 +115,14 @@ func (h *Handler) DeleteUserAvatar(c *gin.Context) { return } - if err := h.UserRepo.UpdateAvatar(c.Request.Context(), userID, nil); err != nil { - errx.Handle(c, errx.InternalError()) + ctx := c.Request.Context() + previous := h.currentUserAvatarURL(ctx, userID) + + if xerr := h.UserService.UpdateAvatar(ctx, userID, nil); xerr != nil { + errx.Handle(c, xerr) return } + h.deleteAvatarObject(ctx, previous, userAvatarKeyPrefix, "") h.auditOrg(c, models.AuditActionUpdate, models.AuditEntityUser, &userID, nil, map[string]string{"field": "avatar_url", "value": "cleared"}) @@ -140,17 +156,21 @@ func (h *Handler) UploadOrganizationAvatar(c *gin.Context) { return } - key := fmt.Sprintf("avatars/organizations/%s-%d%s", orgID.String(), time.Now().Unix(), ext) - url, xerr := putPublicObject(c.Request.Context(), h.Storage, key, bytesRead, mime) + ctx := c.Request.Context() + previous := h.currentOrgAvatarURL(ctx, *orgID) + + key := fmt.Sprintf("%s%s-%d%s", orgAvatarKeyPrefix, orgID.String(), time.Now().UnixMilli(), ext) + url, xerr := putPublicObject(ctx, h.Storage, key, bytesRead, mime) if xerr != nil { errx.Handle(c, xerr) return } - if err := h.OrgRepo.UpdateAvatar(c.Request.Context(), *orgID, &url); err != nil { + if err := h.OrgRepo.UpdateAvatar(ctx, *orgID, &url); err != nil { errx.Handle(c, errx.InternalError()) return } + h.deleteAvatarObject(ctx, previous, orgAvatarKeyPrefix, key) h.auditOrg(c, models.AuditActionUpdate, models.AuditEntityOrganization, orgID, nil, map[string]string{"field": "avatar_url"}) @@ -175,14 +195,74 @@ func (h *Handler) DeleteOrganizationAvatar(c *gin.Context) { return } - if err := h.OrgRepo.UpdateAvatar(c.Request.Context(), *orgID, nil); err != nil { + ctx := c.Request.Context() + previous := h.currentOrgAvatarURL(ctx, *orgID) + + if err := h.OrgRepo.UpdateAvatar(ctx, *orgID, nil); err != nil { errx.Handle(c, errx.InternalError()) return } + h.deleteAvatarObject(ctx, previous, orgAvatarKeyPrefix, "") + + // Audited like the upload so teammates' org switcher refreshes live. + h.auditOrg(c, models.AuditActionUpdate, models.AuditEntityOrganization, orgID, nil, map[string]string{"field": "avatar_url", "value": "cleared"}) c.Status(http.StatusNoContent) } +// currentUserAvatarURL returns the avatar URL stored on the user row, or "" +// when there is none or the lookup fails (cleanup is best-effort). +func (h *Handler) currentUserAvatarURL(ctx context.Context, userID uuid.UUID) string { + u, xerr := h.UserService.GetUser(ctx, userID) + if xerr != nil || u == nil || u.AvatarURL == nil { + return "" + } + return *u.AvatarURL +} + +// currentOrgAvatarURL is the organization counterpart of currentUserAvatarURL. +func (h *Handler) currentOrgAvatarURL(ctx context.Context, orgID uuid.UUID) string { + org, xerr := h.OrganizationService.Get(ctx, orgID) + if xerr != nil || org == nil || org.AvatarURL == nil { + return "" + } + return *org.AvatarURL +} + +// deleteAvatarObject removes the object behind a previous avatar URL once the +// row no longer points at it. Only keys under our own prefix are touched, so +// an external URL (an OAuth profile picture, say) is left alone, and keepKey +// guards the freshly written object. Failures are ignored: an orphaned blob +// is harmless, a failed request after a successful update is not. +func (h *Handler) deleteAvatarObject(ctx context.Context, previousURL, prefix, keepKey string) { + if h.Storage == nil || previousURL == "" { + return + } + key := avatarKeyFromURL(previousURL, prefix) + if key == "" || key == keepKey { + return + } + _ = h.Storage.Delete(ctx, key) +} + +// avatarKeyFromURL recovers the object key from a public avatar URL. Both +// storage backends build the URL differently, but the key always starts with +// the known prefix, so that is what is looked for. +func avatarKeyFromURL(url, prefix string) string { + idx := strings.Index(url, prefix) + if idx < 0 { + return "" + } + key := url[idx:] + if q := strings.IndexAny(key, "?#"); q >= 0 { + key = key[:q] + } + if key == prefix || strings.Contains(key, "..") { + return "" + } + return key +} + func (h *Handler) requireOrgOwner(c *gin.Context, orgID, userID uuid.UUID) *errx.Error { m, err := h.OrgRepo.GetMember(c.Request.Context(), orgID, userID) if err != nil || m == nil { diff --git a/internal/app/user/onboarding.go b/internal/app/user/onboarding.go index 70d43bb8..18a1a1ea 100644 --- a/internal/app/user/onboarding.go +++ b/internal/app/user/onboarding.go @@ -41,3 +41,16 @@ func (s *userService) UpdateUndoSendSeconds(ctx context.Context, userID uuid.UUI return nil } + +// UpdateAvatar sets (or clears, with nil) the user's avatar URL. It must go +// through the service so the cached /auth/me copy is dropped; writing the +// repository directly leaves the old avatar served until UserTTL expires. +func (s *userService) UpdateAvatar(ctx context.Context, userID uuid.UUID, avatarURL *string) *errx.Error { + if err := s.userRepository.UpdateAvatar(ctx, userID, avatarURL); err != nil { + return errx.InternalError() + } + + s.cache.Del(ctx, getUserKey(userID)) + + return nil +} diff --git a/internal/app/user/service.go b/internal/app/user/service.go index f88cda4a..e6644dcc 100644 --- a/internal/app/user/service.go +++ b/internal/app/user/service.go @@ -16,6 +16,7 @@ type UserService interface { CompleteOnboarding(ctx context.Context, userID uuid.UUID, firstName, lastName, referralSource, role, teamSize string) *errx.Error UpdateProfile(ctx context.Context, userID uuid.UUID, firstName, lastName string) *errx.Error UpdateUndoSendSeconds(ctx context.Context, userID uuid.UUID, seconds int) *errx.Error + UpdateAvatar(ctx context.Context, userID uuid.UUID, avatarURL *string) *errx.Error } type userService struct { diff --git a/web/src/components/app/avatar/AvatarUploader.tsx b/web/src/components/app/avatar/AvatarUploader.tsx index aa66ae28..0c5c8837 100644 --- a/web/src/components/app/avatar/AvatarUploader.tsx +++ b/web/src/components/app/avatar/AvatarUploader.tsx @@ -139,7 +139,7 @@ export function AvatarUploader({
{uploading ? "Uploading…" - : `PNG, JPG, WebP or GIF. We resize to ${AVATAR_OUTPUT_DIMENSION}px before upload.`} + : `PNG or JPG. We resize to ${AVATAR_OUTPUT_DIMENSION}px before upload.`}