feat(profile): allow users to upload a custom profile picture - #1282
feat(profile): allow users to upload a custom profile picture#1282themarolt wants to merge 24 commits into
Conversation
Adds an object-store-backed upload path so users can override their Auth0-sourced avatar. The BFF validates and writes the image directly to S3 via a new ObjectStoreService, then persists the resulting CDN-fronted URL to user_metadata.picture through the existing NATS update path (auth-service is unchanged). LFXV2-2628 Signed-off-by: Uroš Marolt <uros@marolt.me>
Moves the avatar edit badge to the bottom-right corner to match the profile-panel pencil icon, and redirects to Flow C authorization when picture upload returns a management-token-required 403, matching the existing profile-save handling. Signed-off-by: Uroš Marolt <uros@marolt.me>
Adds S4 to profile-edit-drawer.spec.ts, stubbing a 403 management_token_required response from picture-upload and the authorize route to verify the redirect fires deterministically. Signed-off-by: Uroš Marolt <uros@marolt.me>
Signed-off-by: Uroš Marolt <uros@marolt.me>
Signed-off-by: Uroš Marolt <uros@marolt.me>
Signed-off-by: Uroš Marolt <uros@marolt.me>
The prior comment claimed a null public_url meant degraded mode with a fallback to the Auth0-sourced picture. The controller actually rejects a null url as a hard CDN_NOT_CONFIGURED error before building a response, so callers never see null in practice. Signed-off-by: Uroš Marolt <uros@marolt.me>
PR SummaryMedium Risk Overview The BFF gains The drawer adds avatar preview/upload, client validation, and on Shared constants/types ( Reviewed by Cursor Bugbot for commit 86a48a4. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds profile picture uploads. It validates image files, accepts raw image data, stores pictures in S3-compatible storage, persists the resulting URL, and updates the profile drawer with upload and authorization handling. ChangesProfile picture upload
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ProfileEditDrawerComponent
participant UserService
participant profileRoute
participant ProfileController
participant ObjectStoreService
ProfileEditDrawerComponent->>UserService: uploadProfilePicture(file)
UserService->>profileRoute: POST raw image bytes
profileRoute->>ProfileController: validate request and invoke upload
ProfileController->>ObjectStoreService: store image
ObjectStoreService-->>ProfileController: CDN URL or null
ProfileController-->>UserService: ProfilePictureUploadResponse
UserService-->>ProfileEditDrawerComponent: upload result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
apps/lfx-one/src/app/modules/profile/components/profile-edit-drawer/profile-edit-drawer.component.ts (1)
338-341: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive the size text from
MAX_AVATAR_SIZE_BYTES.The message hardcodes "20MB" while the check uses
MAX_AVATAR_SIZE_BYTES. If the constant changes, the message becomes wrong. The same literal also appears in the template hint text.♻️ Proposed change
if (file.size > MAX_AVATAR_SIZE_BYTES) { - this.messageService.add({ severity: 'error', summary: 'Error', detail: 'Image must be 20MB or smaller.' }); + const maxMb = Math.floor(MAX_AVATAR_SIZE_BYTES / (1024 * 1024)); + this.messageService.add({ severity: 'error', summary: 'Error', detail: `Image must be ${maxMb}MB or smaller.` }); return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/lfx-one/src/app/modules/profile/components/profile-edit-drawer/profile-edit-drawer.component.ts` around lines 338 - 341, Update the avatar size validation message and its corresponding template hint to derive the displayed size from MAX_AVATAR_SIZE_BYTES instead of hardcoding “20MB”. Reuse a shared formatted size value so both the check feedback and hint remain consistent when the constant changes.apps/lfx-one/src/server/controllers/profile.controller.ts (3)
401-424: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated auth-service token selection.
Lines 401-424 duplicate the token-selection logic in
updateUserMetadata(lines 271-300), including the Authelia detection, the Flow C management-token gate, the 403 payload, and the M2M fallback. Two copies of an auth path can diverge. Extract a private helper that returns the token or signals the 403 challenge, in the same way that this PR centralizesmapUserMetadataUpdateError.♻️ Suggested shape
/** Resolve the auth-service token. Returns null after it writes the Flow C 403 challenge. */ private async resolveAuthServiceToken(req: Request, res: Response, operation: string, username: string): Promise<string | null> { const issuerBaseUrl = process.env['M2M_AUTH_ISSUER_BASE_URL'] || ''; const isAuthelia = issuerBaseUrl.includes('auth.k8s.orb.local'); if (!isAuthelia && this.profileAuthService.isProfileAuthConfigured()) { const mgmtToken = this.profileAuthService.getManagementToken(req); if (!mgmtToken) { logger.warning(req, operation, 'Management token required but not present in session', { username }); res.status(403).json({ error: 'management_token_required', message: 'Profile authorization required', authorize_url: '/api/profile/auth/start?returnTo=/profile', }); return null; } return mgmtToken; } return generateM2MToken(req, { audience: new URL('api/v2/', issuerBaseUrl).toString() }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/lfx-one/src/server/controllers/profile.controller.ts` around lines 401 - 424, Extract the duplicated token-selection flow from updateUserMetadata and upload_profile_picture into a private resolveAuthServiceToken helper. Preserve Authelia detection, management-token validation, the existing 403 response and warning using the operation and username, and the M2M fallback; return null after writing the challenge. Replace both call sites with the helper and handle the null result by returning from the controller action.
379-399: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider verifying the image bytes, not only the declared
Content-Type.The handler trusts the client-declared
Content-Typeand stores the object with that value. The allowlist limits the value to PNG, JPEG, or WebP, so a mismatched body cannot be served as HTML or SVG. The stored object can still be a non-image labelled as an image. A magic-byte check on the first bytes ofbufferwould reject such uploads before they reach the object store.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/lfx-one/src/server/controllers/profile.controller.ts` around lines 379 - 399, Update the upload validation in the profile picture handler around the content-type and buffer checks to verify the buffer’s magic bytes match the declared allowed image type before storing it. Reject mismatched or unrecognized PNG, JPEG, and WebP signatures with a ServiceValidationError for the body, preserving the existing empty-buffer and unsupported-content-type validation behavior.
2108-2108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNarrow the return type to
Error.Every branch returns an error instance (
MicroserviceError,AuthenticationError,AuthorizationError,ResourceNotFoundError, orServiceValidationError).unknownhides that contract from callers and permits a non-error return in a future edit. Declare the return type asError.♻️ Proposed change
- private mapUserMetadataUpdateError(response: UserMetadataUpdateResponse, username: string, operation: string, path: string): unknown { + private mapUserMetadataUpdateError(response: UserMetadataUpdateResponse, username: string, operation: string, path: string): Error {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/lfx-one/src/server/controllers/profile.controller.ts` at line 2108, Update the return type of mapUserMetadataUpdateError to Error instead of unknown, preserving its existing error-instance branches and behavior.apps/lfx-one/src/server/services/object-store.service.ts (1)
90-100: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConfigure bounded S3 request timeouts.
The Node.js handler defaults
connectionTimeoutandrequestTimeoutto0. Set both values andthrowOnRequestTimeout: trueso stalledPutObjectandHeadBucketcalls abort and reject instead of only emitting a warning.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/lfx-one/src/server/services/object-store.service.ts` around lines 90 - 100, Update the S3Client configuration in getClient to set bounded connectionTimeout and requestTimeout values and enable throwOnRequestTimeout: true, ensuring stalled PutObject and HeadBucket requests abort and reject while preserving the existing endpoint, region, and path-style settings.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/lfx-one/src/app/modules/profile/components/profile-edit-drawer/profile-edit-drawer.component.html`:
- Around line 48-65: Update the avatar upload control around the
drawer-edit-avatar-input and its label so the file input remains in the
accessibility tree using a visually-hidden style instead of the hidden class,
and place the input before the label to support peer-focus-visible styling. Add
a visible focus indicator to the label and preserve the existing disabled state,
upload behavior, and accessibility labeling.
---
Nitpick comments:
In
`@apps/lfx-one/src/app/modules/profile/components/profile-edit-drawer/profile-edit-drawer.component.ts`:
- Around line 338-341: Update the avatar size validation message and its
corresponding template hint to derive the displayed size from
MAX_AVATAR_SIZE_BYTES instead of hardcoding “20MB”. Reuse a shared formatted
size value so both the check feedback and hint remain consistent when the
constant changes.
In `@apps/lfx-one/src/server/controllers/profile.controller.ts`:
- Around line 401-424: Extract the duplicated token-selection flow from
updateUserMetadata and upload_profile_picture into a private
resolveAuthServiceToken helper. Preserve Authelia detection, management-token
validation, the existing 403 response and warning using the operation and
username, and the M2M fallback; return null after writing the challenge. Replace
both call sites with the helper and handle the null result by returning from the
controller action.
- Around line 379-399: Update the upload validation in the profile picture
handler around the content-type and buffer checks to verify the buffer’s magic
bytes match the declared allowed image type before storing it. Reject mismatched
or unrecognized PNG, JPEG, and WebP signatures with a ServiceValidationError for
the body, preserving the existing empty-buffer and unsupported-content-type
validation behavior.
- Line 2108: Update the return type of mapUserMetadataUpdateError to Error
instead of unknown, preserving its existing error-instance branches and
behavior.
In `@apps/lfx-one/src/server/services/object-store.service.ts`:
- Around line 90-100: Update the S3Client configuration in getClient to set
bounded connectionTimeout and requestTimeout values and enable
throwOnRequestTimeout: true, ensuring stalled PutObject and HeadBucket requests
abort and reject while preserving the existing endpoint, region, and path-style
settings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 50f3dd6f-ef9c-438b-9616-8f7880ba98a0
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
apps/lfx-one/e2e/profile-edit-drawer.spec.tsapps/lfx-one/package.jsonapps/lfx-one/src/app/modules/profile/components/profile-edit-drawer/profile-edit-drawer.component.htmlapps/lfx-one/src/app/modules/profile/components/profile-edit-drawer/profile-edit-drawer.component.tsapps/lfx-one/src/app/shared/services/user.service.tsapps/lfx-one/src/server/controllers/profile.controller.spec.tsapps/lfx-one/src/server/controllers/profile.controller.tsapps/lfx-one/src/server/routes/profile.route.tsapps/lfx-one/src/server/services/object-store.service.spec.tsapps/lfx-one/src/server/services/object-store.service.tspackages/shared/src/constants/avatar.constants.tspackages/shared/src/interfaces/user-profile.interface.ts
There was a problem hiding this comment.
Pull request overview
Adds custom profile-picture uploads through S3-compatible storage, persisting the CDN URL to user metadata.
Changes:
- Adds shared upload contracts, limits, and S3 infrastructure.
- Adds the protected backend upload and metadata-update flow.
- Adds drawer upload UI with Vitest and Playwright coverage.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
yarn.lock |
Locks AWS SDK dependencies. |
packages/shared/src/interfaces/user-profile.interface.ts |
Defines the upload response. |
packages/shared/src/constants/avatar.constants.ts |
Defines allowed formats and size. |
apps/lfx-one/src/server/services/object-store.service.ts |
Implements bucket checks and uploads. |
apps/lfx-one/src/server/services/object-store.service.spec.ts |
Tests object-storage behavior. |
apps/lfx-one/src/server/routes/profile.route.ts |
Registers the raw upload route. |
apps/lfx-one/src/server/controllers/profile.controller.ts |
Orchestrates upload and metadata persistence. |
apps/lfx-one/src/server/controllers/profile.controller.spec.ts |
Tests controller upload flows. |
apps/lfx-one/src/app/shared/services/user.service.ts |
Adds the client upload request. |
apps/lfx-one/src/app/modules/profile/components/profile-edit-drawer/profile-edit-drawer.component.ts |
Handles selection, validation, and upload state. |
apps/lfx-one/src/app/modules/profile/components/profile-edit-drawer/profile-edit-drawer.component.html |
Adds the avatar upload control. |
apps/lfx-one/package.json |
Adds the S3 client dependency. |
apps/lfx-one/e2e/profile-edit-drawer.spec.ts |
Tests the Flow C redirect. |
Fix five bugs surfaced by CodeQL/CodeRabbit/Copilot on the profile picture upload pilot: - Guard content-type header read against array values (proxied/duplicate headers), fixing a CodeQL type-confusion finding. - Store the S3 avatar key unencoded and percent-encode only at CDN URL construction time, matching buildMyprofileAvatarUrl's convention and fixing an encoding mismatch that could 404 on lookup. - Gate ensureBucket's CreateBucket call on a confirmed 404/NotFound instead of any HeadBucket failure, so a 403 or outage isn't masked as "bucket missing" and silently attempted as a create. - Convert the raw-body parser's entity.too.large error into a 413 before it reaches the global handler, instead of flattening oversized uploads into a generic 500. - Replace the avatar upload trigger's non-interactive label with a real button so it's keyboard-operable. Signed-off-by: Uroš Marolt <uros@marolt.me>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
apps/lfx-one/src/server/controllers/profile.controller.ts (2)
2109-2109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReturn a typed error from
mapUserMetadataUpdateError.The method returns
unknown. Every branch returns a concrete error instance, so the annotation discards useful type information and blocks callers from inspectingstatusCodeorcodewithout a cast.Declare the union of returned error types, or at minimum
Error.♻️ Proposed change
- private mapUserMetadataUpdateError(response: UserMetadataUpdateResponse, username: string, operation: string, path: string): unknown { + private mapUserMetadataUpdateError( + response: UserMetadataUpdateResponse, + username: string, + operation: string, + path: string + ): MicroserviceError | AuthenticationError | AuthorizationError | ResourceNotFoundError | ServiceValidationError {
402-425: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the management-token selection into a private helper.
This block repeats the token selection from
updateUserMetadata(lines 274-300) andverifyAndLinkEmail(lines 1996-2026), including the Authelia detection and the 403management_token_requiredpayload. Three copies will drift when the Flow C rules change.Move the selection into one private method that returns either the token or a "challenge sent" signal, and call it from each write path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/lfx-one/src/server/controllers/profile.controller.ts` around lines 402 - 425, Extract the duplicated token-selection logic from the current upload flow, updateUserMetadata, and verifyAndLinkEmail into a private helper that performs Authelia/profile-auth detection, retrieves the management token, and sends the existing 403 management_token_required response when needed. Have the helper return either the selected token or a challenge-sent signal, then make all three write paths handle that result and return early when a challenge was sent.apps/lfx-one/src/app/modules/profile/components/profile-edit-drawer/profile-edit-drawer.component.ts (1)
320-328: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the first JSDoc block to
onAvatarFileSelected.Two doc comments precede
triggerAvatarUpload. The first block describes validation, the cached-profile update, and thesavedemission. That behavior belongs toonAvatarFileSelected, not to the trigger method.♻️ Proposed change
- /** - * Validate and upload a newly-selected profile picture. On success, updates the locally-cached - * profile (so the drawer's own preview reflects the change if reopened) and emits `saved` so the - * host layout refreshes the avatar shown elsewhere in the Me lens. - */ /** Open the OS file picker via the hidden input — keeps the trigger a real, keyboard-operable `<button>`. */ public triggerAvatarUpload(): void { this.avatarInput()?.nativeElement.click(); } + /** + * Validate and upload a newly-selected profile picture. On success, updates the locally-cached + * profile (so the drawer's own preview reflects the change if reopened) and emits `saved` so the + * host layout refreshes the avatar shown elsewhere in the Me lens. + */ public onAvatarFileSelected(event: Event): void {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/lfx-one/src/app/modules/profile/components/profile-edit-drawer/profile-edit-drawer.component.ts` around lines 320 - 328, Move the JSDoc describing profile-picture validation, upload, cache updates, and the saved emission from triggerAvatarUpload to onAvatarFileSelected, leaving triggerAvatarUpload documented only by its file-picker behavior.apps/lfx-one/src/app/modules/profile/components/profile-edit-drawer/profile-edit-drawer.component.html (1)
60-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBind
acceptto the shared MIME list.The literal
accept="image/png,image/jpeg,image/webp"repeatsALLOWED_AVATAR_MIME_TYPES, which the component already imports for its runtime check. If the shared list changes, the file picker filter goes stale while validation still passes or rejects on the new list.Expose the joined list from the component and bind it.
♻️ Proposed change
Add the accessor in
profile-edit-drawer.component.ts:public readonly avatarAccept = ALLOWED_AVATAR_MIME_TYPES.join(',');Then bind it in the template:
type="file" id="drawer-edit-avatar-input" - accept="image/png,image/jpeg,image/webp" + [accept]="avatarAccept" class="hidden"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/lfx-one/src/app/modules/profile/components/profile-edit-drawer/profile-edit-drawer.component.html` around lines 60 - 68, Replace the hard-coded accept value on the avatarInput element with a binding to a component accessor. In the profile edit drawer component, expose ALLOWED_AVATAR_MIME_TYPES joined by commas as avatarAccept, then bind the template’s accept attribute to avatarAccept so the picker uses the shared MIME list.apps/lfx-one/src/server/services/object-store.service.ts (1)
95-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake
forcePathStyleconfigurable.Do not infer this setting from
S3_ENDPOINT_URL, because that variable also supports non-local S3-compatible backends. Use a dedicatedS3_FORCE_PATH_STYLEflag and update the client-construction tests for both values.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/lfx-one/src/server/services/object-store.service.ts` around lines 95 - 105, Update getClient so forcePathStyle is controlled by the dedicated S3_FORCE_PATH_STYLE environment flag rather than inferred from S3_ENDPOINT_URL; parse both enabled and disabled values explicitly, preserve the existing endpoint handling, and update the S3 client-construction tests to cover both flag states.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/lfx-one/src/server/services/object-store.service.ts`:
- Around line 60-61: Validate username against a strict non-empty allowlist
before the sanitizedUsername assignment in the object-store flow, rejecting
whitespace-only values and characters such as “/” that can diverge between the
S3 key and CDN URL. Keep key and URL construction unchanged for valid usernames,
and apply the same validation to the corresponding path that builds the CDN URL.
---
Nitpick comments:
In
`@apps/lfx-one/src/app/modules/profile/components/profile-edit-drawer/profile-edit-drawer.component.html`:
- Around line 60-68: Replace the hard-coded accept value on the avatarInput
element with a binding to a component accessor. In the profile edit drawer
component, expose ALLOWED_AVATAR_MIME_TYPES joined by commas as avatarAccept,
then bind the template’s accept attribute to avatarAccept so the picker uses the
shared MIME list.
In
`@apps/lfx-one/src/app/modules/profile/components/profile-edit-drawer/profile-edit-drawer.component.ts`:
- Around line 320-328: Move the JSDoc describing profile-picture validation,
upload, cache updates, and the saved emission from triggerAvatarUpload to
onAvatarFileSelected, leaving triggerAvatarUpload documented only by its
file-picker behavior.
In `@apps/lfx-one/src/server/controllers/profile.controller.ts`:
- Around line 402-425: Extract the duplicated token-selection logic from the
current upload flow, updateUserMetadata, and verifyAndLinkEmail into a private
helper that performs Authelia/profile-auth detection, retrieves the management
token, and sends the existing 403 management_token_required response when
needed. Have the helper return either the selected token or a challenge-sent
signal, then make all three write paths handle that result and return early when
a challenge was sent.
In `@apps/lfx-one/src/server/services/object-store.service.ts`:
- Around line 95-105: Update getClient so forcePathStyle is controlled by the
dedicated S3_FORCE_PATH_STYLE environment flag rather than inferred from
S3_ENDPOINT_URL; parse both enabled and disabled values explicitly, preserve the
existing endpoint handling, and update the S3 client-construction tests to cover
both flag states.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 22966cda-0ae8-43db-9dc0-d37770a9fba6
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
apps/lfx-one/e2e/profile-edit-drawer.spec.tsapps/lfx-one/package.jsonapps/lfx-one/src/app/modules/profile/components/profile-edit-drawer/profile-edit-drawer.component.htmlapps/lfx-one/src/app/modules/profile/components/profile-edit-drawer/profile-edit-drawer.component.tsapps/lfx-one/src/app/shared/services/user.service.tsapps/lfx-one/src/server/controllers/profile.controller.spec.tsapps/lfx-one/src/server/controllers/profile.controller.tsapps/lfx-one/src/server/routes/profile.route.tsapps/lfx-one/src/server/services/object-store.service.spec.tsapps/lfx-one/src/server/services/object-store.service.tspackages/shared/src/constants/avatar.constants.tspackages/shared/src/interfaces/user-profile.interface.ts
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (2)
apps/lfx-one/src/server/controllers/profile.controller.ts:427
- The upload commits bytes to the stable per-user key before the metadata write. If
updateUserMetadatathen fails, the endpoint reports failure but the olduser_metadata.picturestill points to this same object, so a CDN cache miss or expiry can expose the newly uploaded image anyway. Use a versioned object key and switch metadata only after upload (cleaning up the new object on metadata failure), or provide an equivalent rollback strategy.
const { url } = await this.objectStoreService.uploadProfilePicture(req, username, buffer, contentType);
apps/lfx-one/src/app/modules/profile/components/profile-edit-drawer/profile-edit-drawer.component.html:39
- Using initials as the image alternative text makes a screen reader announce only something like “JD,” without identifying it as the current profile picture. Give the image meaningful alt text; the sibling profile panel uses the user's display name for this purpose.
[alt]="avatarInitials()"
Escape "%" and "/" in the username when building the avatar S3 key instead of erroring the upload: these are the only two characters that break the raw-key/percent-encoded-URL round trip (a literal "/" would add S3 key segments that don't survive as "%2F" consistently across CDNs/S3 gateways; a literal "%" would collide with our own escaping). Escaping "%" before "/" keeps the mapping collision-free. Every other character is left as-is, so a normal profile picture upload never fails over unusual-but-legitimate username characters. Signed-off-by: Uroš Marolt <uros@marolt.me>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (4)
apps/lfx-one/src/app/modules/profile/components/profile-edit-drawer/profile-edit-drawer.component.ts:379
- If the form is already dirty, this Flow C navigation discards those unsaved profile edits. The normal save path persists the form before redirecting, but the avatar path only notes that the
Filecannot be persisted and navigates immediately. Preserve and restore the draft separately, or prevent avatar authorization from starting until the dirty form has been saved.
window.location.href = error.error.authorize_url;
apps/lfx-one/src/app/modules/profile/components/profile-edit-drawer/profile-edit-drawer.component.ts:366
- The avatar upload's successful client behavior is not covered by the added E2E test, which only exercises the Flow C error. A regression in the raw POST, local preview update, or
savedemission would therefore pass despite this suite already testing the analogous optimistic form-save behavior. Add a stubbed 201 upload case that selects a file and verifies the panel/drawer avatar switches to the returned URL.
const url = response.public_url;
this.messageService.add({ severity: 'success', summary: 'Success', detail: 'Profile picture updated!' });
this.combinedProfile.update((profile) => (profile ? { ...profile, profile: { ...(profile.profile ?? {}), picture: url } } : profile));
this.saved.emit({ picture: url });
apps/lfx-one/src/app/modules/profile/components/profile-edit-drawer/profile-edit-drawer.component.html:39
- Initials such as “JD” do not provide a meaningful text alternative for the profile image. The existing profile panel uses the person's display name for this same avatar; use an equivalent descriptive name here (or explicitly mark the preview decorative if that is the intended semantics).
[alt]="avatarInitials()"
packages/shared/src/interfaces/user-profile.interface.ts:146
- This response type contradicts the endpoint contract documented directly above it: successful responses always contain a URL, while the null case is emitted as an error and never serialized as this payload. Keeping
public_urlnullable forces every client to handle an impossible success state (as the drawer now does). Model the wire response truthfully as a non-null string.
export interface ProfilePictureUploadResponse {
success: boolean;
public_url: string | null;
req.body is typed any, so a TypeScript "as Buffer" cast doesn't rule out an array or string arriving at runtime. Add explicit Array.isArray / typeof checks alongside the existing Buffer.isBuffer guard so the type-confusion is ruled out at the same point req.body is first read, clearing CodeQL's js/type-confusion-through-parameter-tampering alert. Behavior is unchanged: malformed bodies were already rejected by the Buffer.isBuffer check. Signed-off-by: Uroš Marolt <uros@marolt.me>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (3)
apps/lfx-one/src/server/routes/profile.route.ts:23
- This middleware is the only place that preserves the intended 413 for an oversized raw body, but the controller specs cannot exercise parser failures and the new E2E covers only the Flow C redirect. Add a route-level test that posts more than
MAX_AVATAR_SIZE_BYTESand asserts a 413/PAYLOAD_TOO_LARGE; otherwise middleware ordering or error-shape regressions silently return the generic 500 again.
function handlePictureUploadParseError(err: unknown, req: Request, _res: Response, next: NextFunction): void {
if (err && typeof err === 'object' && (err as { type?: string }).type === 'entity.too.large') {
next(
new MicroserviceError('Image exceeds the maximum upload size', 413, 'PAYLOAD_TOO_LARGE', {
apps/lfx-one/src/app/modules/profile/components/profile-edit-drawer/profile-edit-drawer.component.ts:366
- The success path updates only the drawer-local profile and the profile-layout output. The shared header, sidebar, and lens switcher instead render
UserService.user().picture, so they continue showing the old Auth0/social avatar after this upload. Update the shared user signal (or a central avatar signal) with the returned URL as part of the optimistic update.
this.combinedProfile.update((profile) => (profile ? { ...profile, profile: { ...(profile.profile ?? {}), picture: url } } : profile));
this.saved.emit({ picture: url });
apps/lfx-one/src/server/services/object-store.service.ts:90
- This stores the original upload unchanged even though the endpoint accepts 20 MB, and that object is rendered directly in 32–96 px avatar slots across the shell. A valid maximum-sized photo therefore makes each cold avatar load transfer and decode the full 20 MB. Normalize/re-encode the image to bounded avatar dimensions and quality before
PutObject; the 20 MB limit can remain the ingress cap.
Body: buffer,
ContentType: contentType,
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (3)
apps/lfx-one/src/server/services/object-store.service.ts:105
- The cache-busting value has only one-second resolution, so two re-uploads completed within the same second produce the same public URL. Since that URL can already be cached for 24 hours, the second upload can continue displaying the first image even though the endpoint reports success. Use a per-upload unique value (for example a UUID or the S3 version/ETag) and test that successive uploads cannot reuse a cache key.
const versionHint = Math.floor(Date.now() / 1000);
const url = cdnPrefix ? `${cdnPrefix}/avatars/${encodeURIComponent(keySegment)}?v=${versionHint}` : null;
apps/lfx-one/src/server/controllers/profile.controller.ts:381
- This allowlist validates only the caller-controlled
Content-Type; the bytes are never checked. A request containing arbitrary or corrupt data withContent-Type: image/pngis therefore uploaded, persisted, and returned as a successful profile picture (the new tests useBuffer.from('img')), leaving the user with a broken avatar and exposing arbitrary blobs through the CDN. Detect/decode the image beforePutObjectand reject data whose actual format is invalid or does not match the allowlist.
const rawContentType = req.headers['content-type'];
const contentType = (Array.isArray(rawContentType) ? rawContentType[0] : rawContentType || '').split(';')[0].trim();
if (!(ALLOWED_AVATAR_MIME_TYPES as readonly string[]).includes(contentType)) {
apps/lfx-one/src/server/services/object-store.service.ts:163
- This regex checks only the prefix text, not whether the value is a valid URL suitable for path concatenation. Values such as
https://pass and can resolve the generated avatar URL to an unintended host, while a prefix containing a query or fragment produces a malformed avatar path that is then persisted. Parse the value withURLand reject credentials, query strings, and fragments before building the public URL.
if (!/^https?:\/\//i.test(cdnPrefix)) {
throw new Error(`CDN_URL_PREFIX must be an absolute http(s) URL, got: "${cdnPrefix}"`);
}
return cdnPrefix.replace(/\/+$/, '');
# Conflicts: # apps/lfx-one/src/app/layouts/profile-layout/profile-layout.component.ts Signed-off-by: Uroš Marolt <uros@marolt.me>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (3)
apps/lfx-one/src/app/modules/profile/components/profile-edit-drawer/profile-edit-drawer.component.ts:397
- The avatar Flow C path stores the raw form, but
handleProfileAuthReturnsends everyformenvelope throughmapLegacyFormEnvelope, which converts cleared free-text fields ('') toundefined. If a user clears bio, address, city, etc. and then selects an avatar, those edits are silently omitted after authorization, unlike the normal submit path that deliberately preserves clears as''. Build the same mapped metadata used byonSubmitand store it asuserMetadataalongsideavatarPending.
JSON.stringify({
savedAt: Date.now(),
avatarPending: true,
...(this.profileForm.dirty ? { form: this.profileForm.value } : {}),
})
apps/lfx-one/src/server/services/object-store.service.ts:105
- This cache-busting value has one-second granularity. Two uploads completed in the same second produce the same public URL while overwriting the stable S3 key, so a CDN entry created by the first upload can keep serving the wrong bytes for the full 24-hour TTL. Use a per-upload unique version value rather than rounded seconds.
const versionHint = Math.floor(Date.now() / 1000);
const url = cdnPrefix ? `${cdnPrefix}/avatars/${encodeURIComponent(keySegment)}?v=${versionHint}` : null;
apps/lfx-one/src/app/modules/profile/components/profile-edit-drawer/profile-edit-drawer.component.html:39
- The image's alternative text is just the user's initials, so a screen reader announces an unexplained value such as “T” rather than the image's purpose. Give the avatar a meaningful description (or an empty alt if it is intentionally decorative).
[alt]="avatarInitials()"
Avatar upload no longer fabricates a nested profile object when metadata never loaded, which was incorrectly flipping metadataLoaded and enabling clear-to-empty on a later save. The avatar-upload Flow C redirect now stashes the same mapped userMetadata onSubmit uses, instead of the raw form, so intentional empty-string clears survive the management-token-required redirect instead of being dropped by the legacy raw-form mapper. Signed-off-by: Uroš Marolt <uros@marolt.me>
Signed-off-by: Uroš Marolt <uros@marolt.me>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (3)
apps/lfx-one/src/app/modules/profile/components/profile-edit-drawer/profile-edit-drawer.component.html:39
- The image's alternative text is only an initial such as “J”, which does not identify the image or its purpose to screen-reader users. The existing profile avatar uses the display name (
profile-panel.component.html:22-25); use an equivalent descriptive name here, or at minimum label it as the current profile picture.
[alt]="avatarInitials()"
apps/lfx-one/src/server/services/object-store.service.ts:105
- The cache-buster has only one-second resolution. Two successful re-uploads that finish within the same second produce the same persisted URL, so the CDN can keep serving the first response for the configured 24-hour TTL even though S3 now contains the newer image. Generate a per-upload unique value (for example a UUID, or the S3 version identifier) and update the URL assertion accordingly.
const versionHint = Math.floor(Date.now() / 1000);
const url = cdnPrefix ? `${cdnPrefix}/avatars/${encodeURIComponent(keySegment)}?v=${versionHint}` : null;
apps/lfx-one/src/server/services/object-store.service.ts:82
- This request-scoped service starts and completes a second logging lifecycle inside the controller's
upload_profile_picturelifecycle. Because the operation names differ, deduplication does not apply and every successful write emits two INFO completion logs with competing duration semantics. This conflicts with.claude/rules/logging-patterns.md:47-75,109-117, which assigns the HTTP lifecycle to the controller and limits services to internal tracing. Remove this nested lifecycle (and its matching success/error handling); use a DEBUG trace only if the S3 step needs separate visibility.
const startTime = logger.startOperation(req, 'object_store_upload_profile_picture', {
key,
content_type: contentType,
size: buffer.length,
});
Signed-off-by: Uroš Marolt <uros@marolt.me>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1761907. Configure here.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (3)
apps/lfx-one/src/server/services/object-store.service.ts:104
- The cache-busting token has only one-second granularity. Two rapid or concurrent successful uploads can overwrite the same S3 key and persist the same URL, allowing the first CDN/browser response to remain cached for the full 24-hour
max-ageeven though the second upload succeeded. Generate a unique value per upload (for example, a UUID, S3 version ID, or ETag) instead of truncating the timestamp.
const versionHint = Math.floor(Date.now() / 1000);
apps/lfx-one/src/server/services/object-store.service.ts:164
- This prefix check accepts strings that begin with
http://orhttps://but are not usable CDN base URLs, and it also accepts query/fragment suffixes that cause/avatars/...to be appended inside the query or fragment. A deployment typo can therefore produce and persist the wrong profile URL. Parse the value as a URL and reject non-HTTP(S), query, and fragment components before constructing object URLs.
if (!/^https?:\/\//i.test(cdnPrefix)) {
throw new Error(`CDN_URL_PREFIX must be an absolute http(s) URL, got: "${cdnPrefix}"`);
}
return cdnPrefix.replace(/\/+$/, '');
apps/lfx-one/src/app/modules/profile/components/profile-edit-drawer/profile-edit-drawer.component.html:39
- Using the initials as image alternative text makes a screen reader announce only a single letter such as “T”, which is not an equivalent description of the profile picture. The sibling profile panel uses the display name (
profile-panel.component.html:24); use the user's name/username here as well, or usealt=""if this preview is intentionally decorative.
[alt]="avatarInitials()"
takeUntilDestroyed on the upload subscription aborted the underlying HTTP request if the drawer's host was destroyed mid-upload (e.g. route navigation), silently dropping a user-visible file upload. The service already applies take(1), so dropping takeUntilDestroyed here still satisfies the no-bare-subscribe rule while letting the upload finish. Signed-off-by: Uroš Marolt <uros@marolt.me>
Signed-off-by: Uroš Marolt <uros@marolt.me>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated 2 comments.
Suppressed comments (3)
packages/shared/src/constants/avatar.constants.ts:15
- The comment says “20MB” but the calculation uses MiB (20 * 1024 * 1024). Either update the comment to “20MiB” (or “~20MB”) or change the arithmetic to match 20,000,000 bytes, so the documentation matches the actual enforced limit.
/** Maximum profile picture upload size in bytes (20MB), per the LFX object-store design contract. */
export const MAX_AVATAR_SIZE_BYTES = 20 * 1024 * 1024;
apps/lfx-one/src/server/services/object-store.service.ts:186
- For AWS S3,
CreateBucketCommandtypically requiresCreateBucketConfiguration.LocationConstraintwhen creating buckets outsideus-east-1. IfS3_CREATE_MISSING_BUCKETis ever used against real AWS inus-west-2(your enforced region), this call can fail. Consider conditionally including the location constraint when no custom endpoint is set and the region is notus-east-1(while keeping compatibility with local S3 backends).
await client.send(new CreateBucketCommand({ Bucket: bucket }));
logger.success(undefined, 'object_store_ensure_bucket', startTime, { bucket, created: true });
apps/lfx-one/src/app/modules/profile/components/profile-edit-drawer/profile-edit-drawer.component.html:66
- The
acceptlist is hard-coded in the template while the allowed MIME types are also defined inALLOWED_AVATAR_MIME_TYPES. To avoid drift (template vs. validation/constants), consider deriving anacceptstring in the component from the shared constant and binding it (e.g.,[attr.accept]="avatarAccept").
<input
#avatarInput
type="file"
id="drawer-edit-avatar-input"
accept="image/png,image/jpeg,image/webp"
class="hidden"
[disabled]="avatarUploading() || busy()"
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated 3 comments.
Suppressed comments (1)
packages/shared/src/interfaces/user-profile.interface.ts:147
- The new route/controller logic indicates
public_urlis a hard requirement for a successful response (requests error out when a CDN URL can’t be generated). To reflect the actual API contract and reduce defensive null-checks in clients, consider makingpublic_urlnon-nullable (string) inProfilePictureUploadResponseand keeping the nullable shape only for the internal object-store service return.
export interface ProfilePictureUploadResponse {
success: boolean;
public_url: string | null;
}
…udience
new URL('api/v2/', issuerBaseUrl) throws a TypeError when the env var
is unset, surfacing as an unhelpful 500. Fail with a clear
M2M_ISSUER_NOT_CONFIGURED error instead, matching the existing
CDN_URL_PREFIX config-validation pattern in the same handler.
Signed-off-by: Uroš Marolt <uros@marolt.me>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (2)
apps/lfx-one/src/server/services/object-store.service.ts:104
- The cache-busting token has only one-second resolution. Two successful re-uploads in the same second produce the same URL even though the stable S3 object was overwritten, so Angular will not reload the
<img>and the CDN/browser may keep serving the first image for up to the 24-hour TTL. Use at least millisecond resolution (or an object version/ETag) so every upload gets a distinct URL.
const versionHint = Math.floor(Date.now() / 1000);
apps/lfx-one/src/app/modules/profile/components/profile-edit-drawer/profile-edit-drawer.component.html:39
- Using the initials as image alternative text makes a screen reader announce only a bare letter rather than what the preview represents. The sibling profile panel uses a meaningful name (
profile-panel.component.html:24); give this preview a descriptive alt (or the user's display name) as well.
[alt]="avatarInitials()"
CreateBucket without CreateBucketConfiguration.LocationConstraint defaults to us-east-1 regardless of the client's configured region, so a missing-bucket auto-create against a us-west-2 target would fail or land in the wrong region. us-east-1 itself must omit the constraint entirely, so branch on that one exception. Signed-off-by: Uroš Marolt <uros@marolt.me>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (3)
apps/lfx-one/src/app/modules/profile/components/profile-edit-drawer/profile-edit-drawer.component.ts:381
- A dirty but invalid form is serialized before this full-page redirect. For example, a 51-character first name makes the form invalid; after authorization,
handleProfileAuthReturnremoves this snapshot and auto-submits it, the backend rejects it, and the user's edits are lost after only a generic error toast. Keep the user in the drawer and surface validation before redirecting when dirty edits are invalid (or retain the envelope until replay succeeds).
savedAt: Date.now(),
avatarPending: true,
...(this.profileForm.dirty ? { userMetadata: this.buildUserMetadataPayload(this.profileForm.value) } : {}),
})
apps/lfx-one/src/server/services/object-store.service.ts:163
- This regex does not actually validate an absolute URL. Values such as
https://orhttps://cdn.example.com?source=xpass, but string concatenation then produces a URL with the wrong host/path or puts/avatars/...inside the query, which is subsequently persisted as the user's picture. Parse the prefix withURL, require HTTP(S), and reject query/fragment components before constructing object URLs.
if (!/^https?:\/\//i.test(cdnPrefix)) {
throw new Error(`CDN_URL_PREFIX must be an absolute http(s) URL, got: "${cdnPrefix}"`);
}
return cdnPrefix.replace(/\/+$/, '');
apps/lfx-one/src/server/services/object-store.service.ts:105
- The cache-busting value has only one-second resolution. Two uploads that complete in the same second publish the same URL even though this object is cached for 24 hours, so the second successful upload can continue displaying the first image. Use a per-upload value that cannot collide here (at minimum millisecond resolution; a UUID is stronger).
This issue also appears on line 160 of the same file.
const versionHint = Math.floor(Date.now() / 1000);
const url = cdnPrefix ? `${cdnPrefix}/avatars/${encodeURIComponent(keySegment)}?v=${versionHint}` : null;
Summary
Pilot of the LFX v2 object-storage rollout: users can now upload a custom profile picture from the Me lens profile edit drawer, instead of only showing the Auth0/social avatar.
lfx-v2-auth-serviceneeds no changes — the BFF uploads the image directly to S3 and writes the resulting CDN URL intouser_metadata.picturethrough the existing NATS update path.Ref: LFXV2-2628 (epic LFXV2-2008)
Changes
ObjectStoreService(@aws-sdk/client-s3, directPutObject, no presigned URLs) — lazy bucket ensure, stable per-user key (avatars/<username>), CDN URL with cache-busting query param whenCDN_URL_PREFIXis configured.ProfileController.uploadProfilePicture— validates auth + content-type + body, uploads via the object-store service, then persists the URL through the existingupdateUserMetadatapath (same Flow-C management-token / M2M token selection already used elsewhere in this controller).POST /api/profile/picture-upload, guarded byblockDuringImpersonation,express.rawlimited toimage/png|jpeg|webpat 20 MB.403 management_token_requiredresponse (Flow C), redirects to the authorization flow instead of failing silently.ProfilePictureUploadResponseinterface,ALLOWED_AVATAR_MIME_TYPESconstant.ObjectStoreServiceandProfileController.uploadProfilePicture(auth, content-type/size validation, happy path, CDN-not-configured, Flow C 403, upload failure); new Playwright e2e case for the Flow C redirect.Protected files touched
apps/lfx-one/package.jsonandyarn.lock— adding the@aws-sdk/client-s3dependency for the new object-store service. No other protected file (server.ts, middleware, CI/tooling config) is touched.Trade-offs / follow-ups
uploadProfilePicturerepeats the same Flow-C/M2M token-selection block already duplicated 8x elsewhere inprofile.controller.tsonmain. This PR adds the 9th occurrence of an existing convention rather than introducing new duplication; extracting a shared helper would touch all 9 call sites and is out of scope for this pilot ticket.DELETE /api/profile/picture(reset to Auth0 default), ops provisioning of the avatar bucket/CloudFront + IRSA S3 policy on thepccrole, org-logo upload inmember-service.Test plan
yarn format:check/yarn lint:check/yarn check-types/yarn buildall cleanyarn test— full suite green (lfx-one-ui 288 tests, @lfx-one/shared 578 tests)main, all commits DCO + GPG signednats-s3: upload a PNG, confirm object lands in bucket,pictureupdates over NATS, avatar refreshes, re-upload overwrites the same key with a new?v=