Add SVG support for custom organization logos - #44748
Conversation
Follow-up to #44390 (BE/FE) and #44550 (GitOps). Accepts .svg in addition to PNG/JPEG/WebP. Server-side validation parses the XML and rejects script/foreignObject/iframe/object/embed elements, on* attributes, javascript:/data: URLs in href/src, and DOCTYPE/ENTITY declarations (XXE, billion-laughs vectors). Defense-in-depth: GET re-validates on read so a blob planted directly in the object store is still rejected, sets X-Content-Type-Options: nosniff for all logos, and a strict CSP for SVGs so a user pasting the URL into the address bar can't trigger scripts.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #44748 +/- ##
==========================================
- Coverage 66.68% 66.67% -0.01%
==========================================
Files 2664 2664
Lines 214605 214680 +75
Branches 9876 9881 +5
==========================================
+ Hits 143106 143145 +39
- Misses 58478 58500 +22
- Partials 13021 13035 +14
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:
|
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.
Addresses CodeQL "Incomplete URL scheme check" on #44748: a blocklist of script-bearing schemes (javascript:, data:) misses vbscript:, file:, livescript:, mocha:, and any future scheme. Allow only fragment, relative, or http(s):// and reject everything else.
|
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:
WalkthroughAdds SVG support for custom organization logos. Frontend: accepts .svg, expands allowed types, increases sniff window to 1024 bytes, and detects SVG by scanning the head for an element. Backend: detects image/svg+xml, re-validates stored logo bytes using a new SVG sanitizer that enforces an root, rejects disallowed elements (e.g., script, foreignObject, iframe), blocks on* attributes, restricts href/src/xml:base to fragments/relative or http(s), and disallows DOCTYPE/DTD/processing instructions. GET logo responses set X-Content-Type-Options: nosniff and a restrictive CSP for SVG. Tests added for detection and validation. Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR extends Fleet’s custom organization logo feature to accept SVG uploads in addition to raster formats, with server-side SVG validation and additional hardening headers when serving SVG content. It fits into the existing org logo upload/serve flow by adding SVG sniffing, XML-based sanitization, and tightening response headers for direct SVG access.
Changes:
- Add SVG detection and strict XML token-based validation for org logo uploads (rejecting common script/XXE vectors).
- Re-validate stored org logo bytes on read and harden logo responses with
X-Content-Type-Options: nosniffand an SVG-specific CSP. - Update frontend logo file validation/accept list to include
.svg, and add backend test coverage plus a real-world SVG fixture.
Reviewed changes
Copilot reviewed 3 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/testdata/icons/org_logo_css.svg | Adds a real-world SVG fixture used to validate the server-side SVG parser behavior. |
| server/service/org_logo.go | Implements SVG sniffing, SVG XML sanitization, GET hardening headers, and re-validation on read. |
| server/service/org_logo_test.go | Adds unit tests covering accepted/rejected SVG cases and SVG content-type detection. |
| frontend/utilities/file/orgLogoFile.ts | Allows selecting .svg and updates client-side sniffing to recognize SVG for UX validation. |
| changes/add-svg-support-custom-logos | Adds release note entry for SVG logo support and sanitization. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
server/service/org_logo.go (1)
486-490: 💤 Low valueRe-validation on every GET — note for hot paths.
Defense-in-depth re-validation here is reasonable given the threat model (objects planted directly in the bucket). Worth being aware that combined with
Cache-Control: no-store(line 180) every logo fetch round-trips through XML parsing. For org logos this is fine, but if this handler is later reused for higher-traffic asset paths, consider validating once on retrieval and caching the validated bytes (or at least the validation result keyed by content hash).🤖 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/org_logo.go` around lines 486 - 490, The current re-validation in validateOrgLogoBytes on every GET (called from the org logo read path) causes repeated XML parsing on hot paths because responses are served with Cache-Control: no-store; to fix, add a short-lived in-memory cache keyed by the object's immutable identifier (e.g., ETag or content hash) and store either the validated bytes or a validation-success flag so subsequent reads skip validateOrgLogoBytes when the cache entry exists; ensure the cache is consulted in the org logo retrieval flow before calling validateOrgLogoBytes and that entries are evicted/invalidated when the object ETag changes or after a configurable TTL to preserve defense-in-depth.
🤖 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/service/org_logo.go`:
- Around line 263-278: The isSafeSVGURL function currently treats URLs with
empty Scheme as safe, which lets protocol-relative URLs (e.g. "//evil.com/x")
slip through because url.Parse sets Scheme=="" but Host is populated; update
isSafeSVGURL to reject authority-bearing URLs with an empty scheme by returning
false when u.Scheme=="" && u.Host!="" (i.e., treat network-relative URLs as
unsafe), keep allowing relative paths/fragments (empty scheme and empty host),
and retain explicit allow for "http"/"https" schemes; also add the test case
{"protocol-relative", "//evil.com/x.png", false} to the existing tests to cover
this scenario.
- Around line 280-344: Update the SVG validator to block SMIL animation elements
by adding "set", "animate", "animateTransform", and "animateMotion" to the
disallowedSVGElements map and ensure validateSVG will reject any occurrence of
those tags; then add a unit test in TestValidateOrgLogoBytesSVG that includes an
SVG using a <set> or <animate> element (e.g., <a href="#safe"><set
attributeName="href" to="javascript:..."/></a>) to assert the validator rejects
it to prevent regression.
---
Nitpick comments:
In `@server/service/org_logo.go`:
- Around line 486-490: The current re-validation in validateOrgLogoBytes on
every GET (called from the org logo read path) causes repeated XML parsing on
hot paths because responses are served with Cache-Control: no-store; to fix, add
a short-lived in-memory cache keyed by the object's immutable identifier (e.g.,
ETag or content hash) and store either the validated bytes or a
validation-success flag so subsequent reads skip validateOrgLogoBytes when the
cache entry exists; ensure the cache is consulted in the org logo retrieval flow
before calling validateOrgLogoBytes and that entries are evicted/invalidated
when the object ETag changes or after a configurable TTL to preserve
defense-in-depth.
🪄 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: 2459095e-1f2e-4ef1-b4da-e793b7196458
📒 Files selected for processing (2)
server/service/org_logo.goserver/service/org_logo_test.go
| func isSafeSVGURL(raw string) bool { | ||
| raw = strings.TrimSpace(raw) | ||
| if raw == "" || strings.HasPrefix(raw, "#") { | ||
| return true | ||
| } | ||
| u, err := url.Parse(raw) | ||
| if err != nil { | ||
| return false | ||
| } | ||
| if u.Scheme == "" { | ||
| // Relative path or fragment — no scheme, no script surface. | ||
| return true | ||
| } | ||
| s := strings.ToLower(u.Scheme) | ||
| return s == "http" || s == "https" | ||
| } |
There was a problem hiding this comment.
Protocol-relative URLs slip through isSafeSVGURL.
url.Parse("//evil.com/x") returns Scheme=="" with Host set, so the u.Scheme == "" branch returns true. That allows network-relative URLs in href/xlink:href, which a browser will resolve against the page's scheme. The CSP blocks subresource loads, but navigation from an in-document <a xlink:href="//evil.com"> click isn't covered by default-src 'none' and would just redirect.
If protocol-relative isn't intentionally supported, consider rejecting authority-bearing URLs without an explicit http/https scheme:
🛡️ Suggested fix
if u.Scheme == "" {
- // Relative path or fragment — no scheme, no script surface.
- return true
+ // Reject protocol-relative ("//host/...") URLs; a relative
+ // path/fragment has no Host.
+ return u.Host == ""
}And add a test case:
{"protocol-relative", "//evil.com/x.png", false},🤖 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/org_logo.go` around lines 263 - 278, The isSafeSVGURL function
currently treats URLs with empty Scheme as safe, which lets protocol-relative
URLs (e.g. "//evil.com/x") slip through because url.Parse sets Scheme=="" but
Host is populated; update isSafeSVGURL to reject authority-bearing URLs with an
empty scheme by returning false when u.Scheme=="" && u.Host!="" (i.e., treat
network-relative URLs as unsafe), keep allowing relative paths/fragments (empty
scheme and empty host), and retain explicit allow for "http"/"https" schemes;
also add the test case {"protocol-relative", "//evil.com/x.png", false} to the
existing tests to cover this scenario.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
server/fleet/org_logo.go (2)
147-153:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSMIL animation elements can mutate validated attributes after parse-time — add to blocklist.
<set>,<animate>,<animateTransform>, and<animateMotion>are well-documented sanitizer-bypass vectors. The validator inspectshref/srcstatically, but SMIL can rewrite a sibling'shrefto any URL viato/values/fromattributes — which aren't checked. Example currently passing validation:<svg xmlns="http://www.w3.org/2000/svg"> <a href="#safe"> <set attributeName="href" to="javascript:alert(1)"/> <rect width="10" height="10"/> </a> </svg>The CSP blocks inline
javascript:execution on click in modern browsers, but<set attributeName="href" to="https://attacker.example">isn't a script-src violation, so users can still be silently redirected.🛡️ Suggested addition
var disallowedSVGElements = map[string]struct{}{ "script": {}, "foreignobject": {}, "iframe": {}, "object": {}, "embed": {}, + // SMIL animation elements can rewrite href/xlink:href at runtime, + // defeating the static href/src checks below. + "set": {}, + "animate": {}, + "animatetransform": {}, + "animatemotion": {}, }Add a corresponding regression test in
TestValidateOrgLogoBytesSVG.🤖 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/fleet/org_logo.go` around lines 147 - 153, The SVG sanitizer's disallowedSVGElements map in org_logo.go misses SMIL animation elements that can mutate attributes at runtime; add "set", "animate", "animateTransform", and "animateMotion" to disallowedSVGElements and update the validator accordingly, then add a regression unit test in TestValidateOrgLogoBytesSVG that verifies an SVG using <set>/<animate*> to change href/src is rejected; reference the disallowedSVGElements map and TestValidateOrgLogoBytesSVG when making the changes.
127-142:⚠️ Potential issue | 🟠 Major | ⚡ Quick winProtocol-relative URLs (
//host/path) bypassisSafeSVGURL.
url.Parse("//evil.com/x")returnsScheme==""withHostpopulated, so the empty-scheme branch returnstrue. This allows network-relative URLs to slip into renderedxlink:hrefvalues. The CSP blocks subresource loads, but in-document<a xlink:href="//evil.com">navigation isn't covered bydefault-src 'none'.🛡️ Suggested fix
if u.Scheme == "" { - // Relative path or fragment — no scheme, no script surface. - return true + // Reject protocol-relative ("//host/...") URLs; a true relative + // path/fragment has no Host. + return u.Host == "" }Add a corresponding test case
{"protocol-relative", "//evil.com/x.png", false}toTestValidateOrgLogoBytesSVG.🤖 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/fleet/org_logo.go` around lines 127 - 142, isSafeSVGURL currently treats URLs with an empty scheme as safe, which lets protocol-relative URLs like "//evil.com/x" bypass checks; update isSafeSVGURL to treat protocol-relative URLs as unsafe by returning false when u.Scheme == "" but u.Host != "" (i.e., detect empty scheme with a populated Host and reject it), and add a unit test entry {"protocol-relative", "//evil.com/x.png", false} to TestValidateOrgLogoBytesSVG to cover this case.
🧹 Nitpick comments (1)
server/fleet/org_logo_test.go (1)
73-104: ⚡ Quick winConsider extending the URL scheme matrix.
While here, two additions would harden the existing matrix once the
isSafeSVGURLprotocol-relative fix lands (seeserver/fleet/org_logo.goreview):
{"protocol-relative", "//evil.com/x.png", false}— guards theScheme==""branch.- A
tab/newline injectioncase like"java\tscript:alert(1)"—url.Parsemay still parse a recognizable scheme depending on browser leniency, worth a regression bar.🤖 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/fleet/org_logo_test.go` around lines 73 - 104, Update the href/src URL schemes matrix in the test under t.Run("href/src URL schemes") to include two more failing cases that guard the Scheme=="" and whitespace-trick parsing: add {"protocol-relative", "//evil.com/x.png", false} and a control with embedded whitespace such as {"tab/newline injection", "java\tscript:alert(1)", false}; ensure these are passed into ValidateOrgLogoBytes (the same table-driven loop around ValidateOrgLogoBytes) and keep the same assertions (require.Error for false cases and require.NoError for true cases), optionally checking the error contains the existing indicator (e.g., "fragment") so the regression will fail if isSafeSVGURL or ValidateOrgLogoBytes incorrectly treat protocol-relative or whitespace-split schemes as safe.
🤖 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.
Duplicate comments:
In `@server/fleet/org_logo.go`:
- Around line 147-153: The SVG sanitizer's disallowedSVGElements map in
org_logo.go misses SMIL animation elements that can mutate attributes at
runtime; add "set", "animate", "animateTransform", and "animateMotion" to
disallowedSVGElements and update the validator accordingly, then add a
regression unit test in TestValidateOrgLogoBytesSVG that verifies an SVG using
<set>/<animate*> to change href/src is rejected; reference the
disallowedSVGElements map and TestValidateOrgLogoBytesSVG when making the
changes.
- Around line 127-142: isSafeSVGURL currently treats URLs with an empty scheme
as safe, which lets protocol-relative URLs like "//evil.com/x" bypass checks;
update isSafeSVGURL to treat protocol-relative URLs as unsafe by returning false
when u.Scheme == "" but u.Host != "" (i.e., detect empty scheme with a populated
Host and reject it), and add a unit test entry {"protocol-relative",
"//evil.com/x.png", false} to TestValidateOrgLogoBytesSVG to cover this case.
---
Nitpick comments:
In `@server/fleet/org_logo_test.go`:
- Around line 73-104: Update the href/src URL schemes matrix in the test under
t.Run("href/src URL schemes") to include two more failing cases that guard the
Scheme=="" and whitespace-trick parsing: add {"protocol-relative",
"//evil.com/x.png", false} and a control with embedded whitespace such as
{"tab/newline injection", "java\tscript:alert(1)", false}; ensure these are
passed into ValidateOrgLogoBytes (the same table-driven loop around
ValidateOrgLogoBytes) and keep the same assertions (require.Error for false
cases and require.NoError for true cases), optionally checking the error
contains the existing indicator (e.g., "fragment") so the regression will fail
if isSafeSVGURL or ValidateOrgLogoBytes incorrectly treat protocol-relative or
whitespace-split schemes as safe.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8038329d-7731-4e76-9a30-376a7732c573
⛔ Files ignored due to path filters (1)
server/fleet/testdata/icons/org_logo_css.svgis excluded by!**/*.svg
📒 Files selected for processing (3)
server/fleet/org_logo.goserver/fleet/org_logo_test.goserver/service/org_logo.go
- Block <set>, <animate>, <animateTransform>, <animateMotion>: SMIL can
rewrite an ancestor's href/xlink:href to javascript:... at runtime,
bypassing the static href allowlist (CodeRabbit feedback).
- Reject protocol-relative URLs ("//host/x") in href/src: url.Parse
returns Scheme=="" with Host populated, which the previous check
treated as safe (CodeRabbit feedback).
- Update client_appconfig_test.go assertions to match the new
"PNG, JPEG, WebP, or SVG" error message that came in with the merge.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 7 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for _, attr := range t.Attr { | ||
| attrName := strings.ToLower(attr.Name.Local) | ||
| // on* (onclick, onload, …) is SVG's main XSS vector. | ||
| if strings.HasPrefix(attrName, "on") { | ||
| return &BadRequestError{Message: "SVG event-handler attributes are not allowed"} |
| switch t := tok.(type) { | ||
| case xml.StartElement: | ||
| name := strings.ToLower(t.Name.Local) |
| // SVG is text — search the sniff window for "<svg" (case-insensitive). | ||
| // Real SVGs put the root tag near the top, after at most an XML | ||
| // declaration, comments, or a DOCTYPE. Strict safety checks happen | ||
| // server-side; the FE check is just for early UX feedback. | ||
| const text = new TextDecoder("utf-8", { fatal: false }).decode(bytes); | ||
| if (/<svg\b/i.test(text)) { | ||
| return "svg"; |
| }) | ||
| t.Run("rejects unknown format", func(t *testing.T) { | ||
| err := validateOrgLogoFile(writeTempFile(t, "logo.txt", []byte("not an image"))) | ||
| require.Error(t, err) | ||
| assert.ErrorContains(t, err, "PNG, JPEG, or WebP") | ||
| assert.ErrorContains(t, err, "PNG, JPEG, WebP, or SVG") |
Copilot review follow-ups:
- xml:base routed through isSafeSVGURL: a hostile base ("javascript:")
would re-anchor every relative href in the subtree and bypass the
static href allowlist.
- Reject XML processing instructions other than <?xml ...?>: notably
<?xml-stylesheet href=...?> pulls external resources when the SVG
loads as a document.
- FE detection now requires <svg as the first start tag (after BOM,
whitespace, XML decl, comments, DOCTYPE, PIs). Previously any text
containing "<svg" anywhere was accepted, so an HTML file with an
inline <svg> only got caught after the upload round-trip.
- Add positive `accepts svg` case to TestValidateOrgLogoFile so the
gitops preflight path keeps exercising SVG.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/fleet/org_logo.go (1)
106-125: 💤 Low valueConsider checking image magic bytes before the SVG sniff to stay consistent with
ContentTypeForOrgLogo.
ContentTypeForOrgLogochecks PNG/JPEG/WebP magic first and only then falls back tolooksLikeSVG.ValidateOrgLogoBytesdoes the opposite: it routes anything containing<svgin the first 512 bytes tovalidateSVG, even when the leading bytes are clearly a PNG/JPEG/WebP. BecauselooksLikeSVGusesbytes.Containsrather than a leading match, a legitimate PNG/JPEG/WebP that happens to embed the literal<svgin early text/EXIF metadata would now be misrouted into XML parsing and rejected, despiteimage.DecodeConfigbeing able to accept it.Mirroring the order used in
ContentTypeForOrgLogo(or reusing it) keeps the two entry points in agreement and removes the edge case.♻️ Sketch
func ValidateOrgLogoBytes(b []byte) error { if int64(len(b)) > OrgLogoMaxFileSize { return &BadRequestError{Message: "logo must be 100KB or less"} } - if looksLikeSVG(b) { - return validateSVG(b) - } + // Prefer magic-byte detection so binary images carrying "<svg" in + // metadata aren't misrouted into the XML validator. + switch { + case bytes.HasPrefix(b, orgLogoPNGMagic), + bytes.HasPrefix(b, orgLogoJPEGMagic), + hasWebPMagic(b): + // fall through to image.DecodeConfig + case looksLikeSVG(b): + return validateSVG(b) + } _, format, err := image.DecodeConfig(bytes.NewReader(b))🤖 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/fleet/org_logo.go` around lines 106 - 125, ValidateOrgLogoBytes currently checks looksLikeSVG before image magic bytes which can misroute valid PNG/JPEG/WebP that contain "<svg" in metadata; update ValidateOrgLogoBytes to first detect PNG/JPEG/WebP using the same magic-byte logic as ContentTypeForOrgLogo (or call ContentTypeForOrgLogo) and only call looksLikeSVG/validateSVG if magic-byte detection fails, keeping image.DecodeConfig as the fallback for raster formats and preserving existing error handling.
🤖 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.
Nitpick comments:
In `@server/fleet/org_logo.go`:
- Around line 106-125: ValidateOrgLogoBytes currently checks looksLikeSVG before
image magic bytes which can misroute valid PNG/JPEG/WebP that contain "<svg" in
metadata; update ValidateOrgLogoBytes to first detect PNG/JPEG/WebP using the
same magic-byte logic as ContentTypeForOrgLogo (or call ContentTypeForOrgLogo)
and only call looksLikeSVG/validateSVG if magic-byte detection fails, keeping
image.DecodeConfig as the fallback for raster formats and preserving existing
error handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 83a0a772-c870-423a-903b-17ea557dd545
📒 Files selected for processing (4)
frontend/utilities/file/orgLogoFile.tsserver/fleet/org_logo.goserver/fleet/org_logo_test.goserver/service/client_appconfig_test.go
✅ Files skipped from review due to trivial changes (1)
- server/service/client_appconfig_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/utilities/file/orgLogoFile.ts
- server/fleet/org_logo_test.go
Related issue: Follow-up to #44390 (BE/FE) and #44550 (GitOps). Parent story #39016.
Summary
Accepts
.svgfor organization logo uploads in addition to PNG/JPEG/WebP, with strict server-side validation since SVGs can carry scripts.Checklist for submitter
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
Added/updated automated tests
QA'd all new/changed functionality manually
Screen.Recording.2026-05-05.at.8.59.12.PM.mov
Summary by CodeRabbit
New Features
Security
Tests