GitOps changes for custom org's logo uploads - #44550
Merged
Merged
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #44550 +/- ##
==========================================
- Coverage 66.67% 66.65% -0.02%
==========================================
Files 2652 2657 +5
Lines 213648 214288 +640
Branches 9806 9806
==========================================
+ Hits 142449 142842 +393
- Misses 58236 58422 +186
- Partials 12963 13024 +61
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
marked this pull request as ready for review
May 1, 2026 08:35
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
nulmete
force-pushed
the
44333-gitops-ability-to-upload-a
branch
from
May 1, 2026 20:17
944eede to
6a33f5a
Compare
nulmete
commented
May 4, 2026
| @@ -1954,6 +1955,19 @@ func (c *Client) DoGitOps( | |||
| group.CertificateAuthorities = groupedCAs | |||
| delete(incoming.OrgSettings, "certificate_authorities") | |||
|
|
|||
| // Plan org logo upload/delete actions and strip the gitops-only path | |||
Member
Author
There was a problem hiding this comment.
because OrgInfo in the service layer is not aware of paths, just URLs
nulmete
commented
May 4, 2026
Comment on lines
+222
to
+224
| if err := fleet.ValidateOrgLogoBytes(body); err != nil { | ||
| return fmt.Errorf("logo file at %q: %w", path, err) | ||
| } |
Member
Author
There was a problem hiding this comment.
this validation is also done within DecodeRequest in the PUT org_logo endpoint, but needs to be called here so we fail early (and also for --dry-run)
nulmete
commented
May 4, 2026
| @@ -205,33 +170,12 @@ func parseLogoModeQuery(raw string) (fleet.OrgLogoMode, error) { | |||
| return m, nil | |||
| } | |||
|
|
|||
| func validateOrgLogoBytes(b []byte) error { | |||
Member
Author
There was a problem hiding this comment.
moved to fleet pkg in order to be reused within gitops
nulmete
force-pushed
the
44333-gitops-ability-to-upload-a
branch
from
May 5, 2026 12:54
d8a918b to
da34b3e
Compare
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughAdds GitOps support for mode-aware organization logos. fleetctl gitops accepts org_logo_path_dark_mode / org_logo_path_light_mode to upload local logo files; fleetctl generate-gitops exports Fleet-hosted logos into ./lib/org_logo/. and replaces URL entries with org_logo_path_* keys while preserving external URLs as org_logo_url_dark_mode / org_logo_url_light_mode. Validations prevent providing both a path and a URL for the same mode. New server/client endpoints and helpers handle upload, delete, fetch, format detection, and size validation. Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
server/service/integration_core_test.go (1)
17044-17047: ⚡ Quick winRestore
s.tokenviadeferto prevent cascading failures.
s.tokenis a suite-level field. IfDoRawWithHeadersat line 17046 callst.FailNow()(e.g., the auth check is broken and the server returns 200 instead of 403), the restore at line 17047 is never reached. Subsequent suite tests that rely ons.tokenwill then fail with mysterious auth errors rather than pointing to this test.♻️ Proposed fix
- s.token = s.getCachedUserToken(maintainerEmail, test.GoodPassword) + origToken := s.token + s.token = s.getCachedUserToken(maintainerEmail, test.GoodPassword) + defer func() { s.token = origToken }() body, headers = buildLogoBody("nope.png", pngBytes) s.DoRawWithHeaders("PUT", "/api/v1/fleet/logo?mode=light", body, http.StatusForbidden, headers) - s.token = s.getTestAdminToken()🤖 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 `@server/service/integration_core_test.go` around lines 17044 - 17047, The test temporarily sets s.token to a non-admin value via s.getCachedUserToken before calling DoRawWithHeaders and then resets it with s.getTestAdminToken; change this to save the original token into a local variable (e.g., origToken) immediately after modifying s.token and use defer to restore s.token = origToken so the admin token is always restored even if DoRawWithHeaders triggers a FailNow; reference s.token, getCachedUserToken, DoRawWithHeaders and getTestAdminToken when making the change.pkg/spec/gitops_test.go (1)
1106-1122: ⚡ Quick winAdd a light-mode old+new URL conflict case in this subtest.
This currently validates only
org_logo_url+org_logo_url_dark_mode. Add the symmetricorg_logo_url_light_background+org_logo_url_light_modeconflict case so both migration paths are covered.Proposed test extension
t.Run("old + new URL keys conflict", func(t *testing.T) { @@ _, err := gitOpsFromString(t, config) require.Error(t, err) assert.ErrorContains(t, err, "org_logo_url") + + config = getGlobalConfig([]string{"org_settings"}) + config += ` +org_settings: + server_settings: + server_url: https://fleet.example.com + org_info: + contact_url: https://example.com/contact + org_name: Test Org + org_logo_url_light_background: https://example.com/old-light.png + org_logo_url_light_mode: https://example.com/new-light.png + secrets: +` + _, err = gitOpsFromString(t, config) + require.Error(t, err) + assert.ErrorContains(t, err, "org_logo_url_light_background") }) }🤖 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 `@pkg/spec/gitops_test.go` around lines 1106 - 1122, The test t.Run("old + new URL keys conflict") only covers the dark-mode pair; extend it to also cover the light-mode pair by adding config data containing both org_logo_url_light_background and org_logo_url_light_mode (either in the same config string or as a separate subtest inside this t.Run), call gitOpsFromString as before, require.Error on the result, and use assert.ErrorContains to check the error mentions the light-mode keys (org_logo_url_light_background / org_logo_url_light_mode) in addition to the existing org_logo_url check; ensure you keep the same pattern of using config := getGlobalConfig(...) and the existing require/assert calls.server/service/integration_enterprise_test.go (1)
29598-29610: ⚡ Quick winDefer logo cleanup to avoid state leakage on early failures.
If the assertion on Line 29606 fails, cleanup on Lines 29608-29609 is skipped and test state can leak into later cases.
Proposed change
s.DoRawWithHeaders("PUT", "/api/v1/fleet/logo?mode=dark", body.Bytes(), http.StatusOK, map[string]string{ "Content-Type": w.FormDataContentType(), "Accept": "application/json", "Authorization": "Bearer " + s.token, }) + defer func() { + s.token = s.getTestAdminToken() + s.Do("DELETE", "/api/v1/fleet/logo", nil, http.StatusOK, "mode", "dark") + }() var acResp appConfigResponse s.DoJSON("GET", "/api/v1/fleet/config", nil, http.StatusOK, &acResp) require.Contains(t, acResp.OrgInfo.OrgLogoURLDarkMode, "/api/latest/fleet/logo") - - s.token = s.getTestAdminToken() - s.Do("DELETE", "/api/v1/fleet/logo", nil, http.StatusOK, "mode", "dark") }🤖 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 `@server/service/integration_enterprise_test.go` around lines 29598 - 29610, After uploading the logo but before any assertions, add a deferred cleanup to always delete the logo to avoid state leakage: immediately after the s.DoRawWithHeaders(...) call, insert a defer that captures any needed state and calls s.token = s.getTestAdminToken() followed by s.Do("DELETE", "/api/v1/fleet/logo", nil, http.StatusOK, "mode", "dark") (restore s.token if necessary) so cleanup runs even if require.Contains on acResp.OrgInfo.OrgLogoURLDarkMode fails; reference s.DoRawWithHeaders, s.DoJSON, s.getTestAdminToken, and s.Do to locate where to add the defer.server/service/client_appconfig_test.go (1)
98-102: ⚡ Quick winDeprecated-key deletion is only tested trivially here.
The initial
orgInfoat line 88 does not include"org_logo_url", so the assertion that it is absent after the call is trivially true regardless of whetherdelete(orgInfo, s.deprecatedURLKey)runs. A slightly stronger variant would seed the key and assert it is gone:♻️ Suggested improvement
os := orgSettings(map[string]any{ "org_logo_path_dark_mode": "logo.png", "org_logo_url_dark_mode": "", + "org_logo_url": "https://old.example.com/logo.png", // deprecated alias present })🤖 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 `@server/service/client_appconfig_test.go` around lines 98 - 102, The test currently asserts deprecated keys are absent but never seeds "org_logo_url", making the check trivial; modify the test to pre-populate orgInfo with the deprecated key(s) (e.g., set orgInfo["org_logo_url"] = "placeholder" or similar) before invoking the code under test, then run the same loop that checks orgInfo[k] and assert.False for each key so you validate that the code (which should call delete on s.deprecatedURLKey) actually removes the seeded key; refer to the orgInfo variable and the loop over keys (including "org_logo_url") to locate where to insert the seeding.
🤖 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 `@server/fleet/app.go`:
- Around line 1294-1305: Remove the early strings.HasPrefix check in
IsFleetHostedLogoURL and instead parse the URL first (after the empty-string
guard), then compare the parsed URL's Path exactly to orgLogoServingPathPrefix;
specifically, update the function so it returns false on parse error and only
returns true when u.Path == orgLogoServingPathPrefix (do not use
strings.HasPrefix on rawURL), ensuring relative URLs like
"/api/latest/fleet/logo-proxy" no longer mis-identify as Fleet-hosted.
---
Nitpick comments:
In `@pkg/spec/gitops_test.go`:
- Around line 1106-1122: The test t.Run("old + new URL keys conflict") only
covers the dark-mode pair; extend it to also cover the light-mode pair by adding
config data containing both org_logo_url_light_background and
org_logo_url_light_mode (either in the same config string or as a separate
subtest inside this t.Run), call gitOpsFromString as before, require.Error on
the result, and use assert.ErrorContains to check the error mentions the
light-mode keys (org_logo_url_light_background / org_logo_url_light_mode) in
addition to the existing org_logo_url check; ensure you keep the same pattern of
using config := getGlobalConfig(...) and the existing require/assert calls.
In `@server/service/client_appconfig_test.go`:
- Around line 98-102: The test currently asserts deprecated keys are absent but
never seeds "org_logo_url", making the check trivial; modify the test to
pre-populate orgInfo with the deprecated key(s) (e.g., set
orgInfo["org_logo_url"] = "placeholder" or similar) before invoking the code
under test, then run the same loop that checks orgInfo[k] and assert.False for
each key so you validate that the code (which should call delete on
s.deprecatedURLKey) actually removes the seeded key; refer to the orgInfo
variable and the loop over keys (including "org_logo_url") to locate where to
insert the seeding.
In `@server/service/integration_core_test.go`:
- Around line 17044-17047: The test temporarily sets s.token to a non-admin
value via s.getCachedUserToken before calling DoRawWithHeaders and then resets
it with s.getTestAdminToken; change this to save the original token into a local
variable (e.g., origToken) immediately after modifying s.token and use defer to
restore s.token = origToken so the admin token is always restored even if
DoRawWithHeaders triggers a FailNow; reference s.token, getCachedUserToken,
DoRawWithHeaders and getTestAdminToken when making the change.
In `@server/service/integration_enterprise_test.go`:
- Around line 29598-29610: After uploading the logo but before any assertions,
add a deferred cleanup to always delete the logo to avoid state leakage:
immediately after the s.DoRawWithHeaders(...) call, insert a defer that captures
any needed state and calls s.token = s.getTestAdminToken() followed by
s.Do("DELETE", "/api/v1/fleet/logo", nil, http.StatusOK, "mode", "dark")
(restore s.token if necessary) so cleanup runs even if require.Contains on
acResp.OrgInfo.OrgLogoURLDarkMode fails; reference s.DoRawWithHeaders, s.DoJSON,
s.getTestAdminToken, and s.Do to locate where to add the defer.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2cacbd85-ec6c-4c6b-8eba-d3291f5612ac
📒 Files selected for processing (21)
changes/44333-gitops-custom-org-logo-uploadcmd/fleetctl/fleetctl/generate_gitops.gocmd/fleetctl/fleetctl/generate_gitops_test.gocmd/fleetctl/fleetctl/testdata/generateGitops/expectedOrgSettings-insecure.yamlcmd/fleetctl/fleetctl/testdata/generateGitops/expectedOrgSettings.yamlcmd/fleetctl/fleetctl/testdata/generateGitops/test_dir_free/default.ymlcmd/fleetctl/fleetctl/testdata/generateGitops/test_dir_premium/default.ymlpkg/spec/gitops.gopkg/spec/gitops_deprecations.gopkg/spec/gitops_test.gopkg/spec/gitops_validate.goserver/fleet/app.goserver/fleet/org_logo.goserver/service/client.goserver/service/client_appconfig.goserver/service/client_appconfig_test.goserver/service/integration_core_test.goserver/service/integration_enterprise_test.goserver/service/org_logo.goserver/service/org_logo_test.goserver/service/testing_utils.go
💤 Files with no reviewable changes (4)
- cmd/fleetctl/fleetctl/testdata/generateGitops/expectedOrgSettings-insecure.yaml
- cmd/fleetctl/fleetctl/testdata/generateGitops/expectedOrgSettings.yaml
- cmd/fleetctl/fleetctl/testdata/generateGitops/test_dir_free/default.yml
- cmd/fleetctl/fleetctl/testdata/generateGitops/test_dir_premium/default.yml
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds end-to-end GitOps support for custom organization logo uploads, including new YAML keys for local file paths, GitOps apply behavior to upload/delete Fleet-hosted logo blobs, and generate-gitops behavior to export Fleet-hosted logos back to local files (while preserving true external URLs). It also standardizes org logo URL key names to mode-aware variants and updates tests/fixtures accordingly.
Changes:
fleetctl gitops: acceptorg_logo_path_{light,dark}_mode, upload logos after a successful AppConfig PATCH, and delete stale Fleet-hosted blobs when switching to external URLs or clearing URLs.fleetctl generate-gitops: detect Fleet-hosted logo URLs, download logo bytes, write them underlib/org_logo/, and emitorg_logo_path_*_modeinstead of URL keys.- Refactor org-logo byte validation/content-type sniffing into
server/fleetand expand unit/integration test coverage.
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/testing_utils.go | Wires a temp OrgLogoStore into test service setup so org-logo endpoints can operate in tests. |
| server/service/org_logo.go | Switches to shared org-logo validation/content-type helpers and relaxes write auth to AppConfig write (enabling GitOps role). |
| server/service/org_logo_test.go | Updates auth expectations to allow the global GitOps role to upload/delete org logos. |
| server/service/integration_enterprise_test.go | Adds integration coverage that a GitOps user can upload an org logo. |
| server/service/integration_core_test.go | Adds integration coverage for upload/GET content-type, auth rejection, invalid payload, and delete lifecycle. |
| server/service/client.go | Plans and applies org-logo upload/delete actions during fleetctl gitops. |
| server/service/client_appconfig.go | Implements org-logo action planning, local-file validation, and client-side PUT/DELETE/GET helpers. |
| server/service/client_appconfig_test.go | Adds focused tests for org-logo planning/stripping behavior and local file validation. |
| server/fleet/org_logo.go | Centralizes org-logo validation and content-type sniffing utilities in the fleet package. |
| server/fleet/app.go | Adds IsFleetHostedLogoURL to distinguish Fleet-served logo URLs from external URLs. |
| pkg/spec/gitops.go | Adds GitOpsOrgInfo to allow gitops-only logo path keys and validates mutual exclusivity of path vs URL. |
| pkg/spec/gitops_validate.go | Registers org_info as a typed gitops object for validation/unknown-key checking. |
| pkg/spec/gitops_test.go | Adds parsing tests for new path keys, mutual exclusivity, and deprecated URL key renames. |
| pkg/spec/gitops_deprecations.go | Adds deprecated-key mappings from old logo URL keys to new mode-aware keys. |
| cmd/fleetctl/fleetctl/testdata/generateGitops/test_dir_premium/default.yml | Updates fixtures to remove deprecated logo URL keys and use mode-aware keys. |
| cmd/fleetctl/fleetctl/testdata/generateGitops/test_dir_free/default.yml | Updates fixtures to remove deprecated logo URL keys and use mode-aware keys. |
| cmd/fleetctl/fleetctl/testdata/generateGitops/expectedOrgSettings.yaml | Updates expected output to remove deprecated keys. |
| cmd/fleetctl/fleetctl/testdata/generateGitops/expectedOrgSettings-insecure.yaml | Updates expected output to remove deprecated keys. |
| cmd/fleetctl/fleetctl/generate_gitops.go | Exports Fleet-hosted logos to lib/org_logo/* and emits path keys during generate-gitops. |
| cmd/fleetctl/fleetctl/generate_gitops_test.go | Adds tests for exporting Fleet-hosted logos (success, external URL no-op, and 404 warning behavior). |
| changes/44333-gitops-custom-org-logo-upload | Adds release note for GitOps org logo upload/export behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+329
to
+331
| // a custom org logo from a local file. The path keys are extracted and turned | ||
| // into PUT /api/v1/fleet/logo calls before the OrgInfo is sent to the AppConfig | ||
| // PATCH endpoint. |
Comment on lines
55
to
61
| @@ -91,7 +56,7 @@ func (putOrgLogoRequest) DecodeRequest(_ context.Context, r *http.Request) (any, | |||
| if err != nil { | |||
| return nil, &fleet.BadRequestError{Message: "failed to read uploaded logo", InternalErr: err} | |||
| } | |||
| if err := validateOrgLogoBytes(body); err != nil { | |||
| if err := fleet.ValidateOrgLogoBytes(body); err != nil { | |||
| return nil, err | |||
| } | |||
Member
Author
There was a problem hiding this comment.
👍 makes sense - moved validation to the service function
Comment on lines
+1116
to
+1119
| ext := orgLogoExtFromContentType(contentType) | ||
| fileName := fmt.Sprintf("lib/org_logo/%s%s", mode, ext) | ||
| cmd.FilesToWrite[fileName] = string(body) | ||
|
|
juan-fdz-hawa
previously approved these changes
May 5, 2026
juan-fdz-hawa
approved these changes
May 5, 2026
3 tasks
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 -->
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 #44333
Checklist for submitter
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
Added/updated automated tests. Also added some integration tests as a follow-up of the first PR (Add ability to upload custom org logos #44390).
QA'd all new/changed functionality manually
generate-gitops
gitops
New Fleet configuration settings
fleetctl generate-gitopsSummary by CodeRabbit
New Features
fleetctl generate-gitopsexports Fleet-hosted logos as local files and inserts path references.Deprecated
org_logo_url_dark_mode,org_logo_url_light_mode).Bug Fixes / Validation