Add ability to upload custom org logos - #44390
Merged
Merged
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #44390 +/- ##
==========================================
- Coverage 66.67% 66.56% -0.12%
==========================================
Files 2647 2656 +9
Lines 212852 213790 +938
Branches 9639 9673 +34
==========================================
+ Hits 141928 142317 +389
- Misses 58025 58539 +514
- Partials 12899 12934 +35
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
nulmete
commented
Apr 29, 2026
Comment on lines
+25
to
+27
| /** If present, replaces the default Graphic on the left of the file | ||
| * details (e.g. to render a preview thumbnail of an uploaded image). */ | ||
| customPreview?: React.ReactNode; |
Member
Author
There was a problem hiding this comment.
used to display the preview of the uploaded logo during setup
nulmete
commented
Apr 29, 2026
| ue.GET("/api/_version_/fleet/config/certificate", getCertificateEndpoint, nil) | ||
| ue.GET("/api/_version_/fleet/config", getAppConfigEndpoint, nil) | ||
| ue.PATCH("/api/_version_/fleet/config", modifyAppConfigEndpoint, modifyAppConfigRequest{}) | ||
| ue.WithRequestBodySizeLimit(100*units.KiB).PUT("/api/_version_/fleet/logo", putOrgLogoEndpoint, putOrgLogoRequest{}) |
Member
Author
There was a problem hiding this comment.
I think 100KB sounds like a reasonable limit for a logo. All ears here.
cc @rachaelshaw
nulmete
commented
Apr 29, 2026
Comment on lines
+261
to
+265
| // eslint-disable-next-line no-restricted-syntax, no-await-in-loop | ||
| for (const op of logoOps) { | ||
| // eslint-disable-next-line no-await-in-loop | ||
| await op(); | ||
| } |
Member
Author
There was a problem hiding this comment.
IMHO, this is fine. (We need these updates to be sequential and not in parallel since we're writing to AppConfig in both calls, so we need to avoid race conditions such as saving only one logo when we want to save both.)
nulmete
requested review from
juan-fdz-hawa
and removed request for
juan-fdz-hawa
April 30, 2026 14:41
nulmete
marked this pull request as ready for review
April 30, 2026 14:42
| return f, st.Size(), nil | ||
| } | ||
|
|
||
| func (s *OrgLogoStore) Put(ctx context.Context, mode fleet.OrgLogoMode, content io.ReadSeeker) error { |
Contributor
There was a problem hiding this comment.
nit: This is not atomic, but I'm guessing that's fine
| }, | ||
| }, | ||
| { | ||
| name: "matching dark old + new -> kept", |
Contributor
There was a problem hiding this comment.
Might want to add a "matching light old + new" here to cover that case
| ue.GET("/api/_version_/fleet/config/certificate", getCertificateEndpoint, nil) | ||
| ue.GET("/api/_version_/fleet/config", getAppConfigEndpoint, nil) | ||
| ue.PATCH("/api/_version_/fleet/config", modifyAppConfigEndpoint, modifyAppConfigRequest{}) | ||
| ue.WithRequestBodySizeLimit(100*units.KiB).PUT("/api/_version_/fleet/logo", putOrgLogoEndpoint, putOrgLogoRequest{}) |
Contributor
There was a problem hiding this comment.
Can we define the file size limit in a single place? Right now this limit is duplicated both there and in the service layer
Member
Author
There was a problem hiding this comment.
Yup, thanks for pointing out, I had a note to do this but totally forgot
| // Magic-byte signatures used to identify accepted image formats. We compare | ||
| // against raw upload bytes rather than trusting the multipart Content-Type | ||
| // header. | ||
| var ( |
Contributor
There was a problem hiding this comment.
I don't think you need to manually validate the payload like this, the image.DecodeConfig can be used for doing this kind of payload validation (take a look at server/service/software_title_icons.go for inspiration)
|
|
||
| func getOrgLogoEndpoint(ctx context.Context, request any, svc fleet.Service) (fleet.Errorer, error) { | ||
| req := request.(getOrgLogoRequest) | ||
| body, _, err := svc.GetOrgLogo(ctx, req.Mode) |
Contributor
There was a problem hiding this comment.
This will load the image into memory, and since this is a public end-point, we need to rate limit this (otherwise this end-point is susceptible to DDoS). We might also want to add some cache headers for this.
Member
Author
There was a problem hiding this comment.
At first I had a cache of 300s but got an AI-generated comment saying that if we updated a logo then we'd serve a stale logo in some cases. Anyways I'll revisit this.
| if err := svc.authz.Authorize(ctx, &fleet.AppConfig{}, fleet.ActionWrite); err != nil { | ||
| return err | ||
| } | ||
| if err := requireGlobalAdmin(ctx); err != nil { |
Contributor
There was a problem hiding this comment.
AFAIK, gitops users can also update the app-config so this check might be wrong?
Member
Author
There was a problem hiding this comment.
Yes, I'm removing this check in the follow-up gitops PR 👍
| return false; | ||
| }; | ||
|
|
||
| export const validateOrgLogoFile = async ( |
Contributor
There was a problem hiding this comment.
I think this could be useful as a generic validator like validateImageFile(payload, {maxSize, allowedTypes})
Member
Author
There was a problem hiding this comment.
I followed this initially, but then kept only the payload(file) as the argument, as the maxSize (ORG_LOGO_MAX_SIZE_BYTES) and allowedTypes (ORG_LOGO_ALLOWED_TYPES) are declared in this file already.
Lmk if this is OK.
| // eslint-disable-next-line no-restricted-syntax, no-await-in-loop | ||
| for (const op of logoOps) { | ||
| // eslint-disable-next-line no-await-in-loop | ||
| await op(); |
Contributor
There was a problem hiding this comment.
There might be a subtle bug here when doing multiple ops. If one of the ops success but the other fails, the whole operation is marked as a failure the whole operation will be marked as failure so when the user tries again all ops will be retried again (even the ones that succeeded) - might be easier to fix on the server side if we make the end-points completely inmutable.
Member
Author
There was a problem hiding this comment.
I changed this so that each upload/delete is tracked individually: we report which mode(s) failed in the flash, and pending state is only cleared for the modes that actually succeeded. So, a retry only re-issues the failed calls.
The only "trade-off" is that we're not using the DELETE endpoint with mode=all, which lets us delete both logos with a single API call -- though that's still supported by the BE if someone wants to use that endpoint directly.
Contributor
|
Done with the first pass! - left some comments to tackle. |
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
7 tasks
juan-fdz-hawa
approved these changes
May 5, 2026
3 tasks
nulmete
added a commit
that referenced
this pull request
May 5, 2026
<!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves #44333 # Checklist for submitter - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. ## Testing - [x] Added/updated automated tests. Also added some integration tests as a follow-up of the first PR (#44390). - [x] QA'd all new/changed functionality manually #### generate-gitops - Branched off to main, no URLs set, then ran generate-gitops on this branch. Deprecated keys gone, new keys present. <img width="447" height="170" alt="nourls_new" src="https://github.com/user-attachments/assets/61931615-d61b-44d3-8095-f7a2b9bd8871" /> - Branched off to main, set external URLs for both light and dark modes, then ran generate-gitops on this branch. Deprecated keys gone, new keys set with the external URLs. <img width="637" height="471" alt="externalurl_main" src="https://github.com/user-attachments/assets/c3782756-acc2-4b99-812d-86e145f11ad5" /> <img width="459" height="168" alt="externalurl_new" src="https://github.com/user-attachments/assets/aa2d8825-3c47-40ba-ab91-bb8202afe81a" /> - Within this branch, after uploading a custom logo for light mode, ran generate-gitops. The logo was saved in lib/org_logo/light.webp <img width="1510" height="639" alt="Screenshot 2026-05-04 at 4 06 59 PM" src="https://github.com/user-attachments/assets/13318c24-8fa4-4e29-b629-ff723d4afe5a" /> <img width="786" height="172" alt="Screenshot 2026-05-04 at 4 07 30 PM" src="https://github.com/user-attachments/assets/b46bd1df-7dcd-4489-b7da-4cbad77b25b8" /> #### gitops - Applied gitops with two external URLs. Verified in the UI that those are still present <img width="944" height="189" alt="Screenshot 2026-05-04 at 7 54 53 AM" src="https://github.com/user-attachments/assets/a34813ca-beb1-403e-9793-d42cc9c72f8b" /> <img width="637" height="259" alt="Screenshot 2026-05-04 at 8 01 04 AM" src="https://github.com/user-attachments/assets/74c2cd56-ab1d-4ddd-9b8e-22c49e9ae9d5" /> - Applied gitops with "" as the URLs to clear them. Verified the default fleet logo is shown. <img width="460" height="201" alt="Screenshot 2026-05-04 at 8 15 11 AM" src="https://github.com/user-attachments/assets/dcbafea3-b4ea-44aa-9045-08c4f5a64e98" /> <img width="648" height="269" alt="Screenshot 2026-05-04 at 8 15 50 AM" src="https://github.com/user-attachments/assets/451a28f9-e929-4b84-93d3-a7dd9afd5eca" /> - Applied gitops with a custom logo for light theme, using **org_logo_path_light_mode**: <img width="948" height="207" alt="Screenshot 2026-05-04 at 4 10 05 PM" src="https://github.com/user-attachments/assets/b1418cd4-31cc-4e53-b566-9af11ec21970" /> <img width="774" height="168" alt="Screenshot 2026-05-04 at 4 10 35 PM" src="https://github.com/user-attachments/assets/63f596eb-308f-4122-ad86-e1d718e9b525" /> ## New Fleet configuration settings - [x] Verified that the setting is exported via `fleetctl generate-gitops` - [x] Verified the setting is documented in a separate PR to [the GitOps documentation](https://github.com/fleetdm/fleet/blob/main/docs/Configuration/yaml-files.md#L485) - See #43808. - [x] Verified that the setting is cleared on the server if it is not supplied in a YAML file (or that it is documented as being optional) - [x] Verified that any relevant UI is disabled when GitOps mode is enabled <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * GitOps support for uploading custom org logos (dark/light) via local files. * `fleetctl generate-gitops` exports Fleet-hosted logos as local files and inserts path references. * New API endpoints to upload, delete, and fetch org logos. * **Deprecated** * Legacy logo keys consolidated into mode-specific URL keys (`org_logo_url_dark_mode`, `org_logo_url_light_mode`). * **Bug Fixes / Validation** * Validation/error when both a path and URL are provided for the same mode; file size and image-format checks enforced. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
nulmete
added a commit
that referenced
this pull request
May 6, 2026
<!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Follow-up to #44390 (BE/FE) and #44550 (GitOps). Parent story #39016. ## Summary Accepts `.svg` for organization logo uploads in addition to PNG/JPEG/WebP, with strict server-side validation since SVGs can carry scripts. # Checklist for submitter - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. ## Testing - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually https://github.com/user-attachments/assets/318d320e-ff78-41fe-ad3a-55d6dace8dc0 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Organization logos now accept SVG in addition to PNG, JPEG, and WebP. * Stored SVG logos are re-validated when served. * **Security** * Server applies strict SVG sanitization to block scripts, unsafe elements, event handlers, and unsafe URL schemes. * SVG logo responses include headers to prevent content-type sniffing and restrict execution. * **Tests** * Added tests covering SVG detection, validation, allowed/rejected cases, and serving behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issue: Resolves #44330, Resolves #44331
Checklist for submitter
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
Added/updated automated tests. (I'd defer integration tests to a separate PR since this one is pretty large already.)
QA'd all new/changed functionality manually. I've tested this on both the setup flow and the organization settings page. I haven't had the time to test this on other places where we render the logo (macOS setup experience / MDM migration dialog).
Screen.Recording.2026-05-01.at.10.04.21.AM.mov
New Fleet configuration settings
Will handle GitOps in a separate PR.
Summary by CodeRabbit