Fix/feedback preview xss#7532
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the star-rating/feedback logo upload + preview flow against XSS by avoiding trust in stored/client-provided MIME types, adding image “magic byte” sniffing, tightening upload authorization, and emitting safer response headers for image endpoints.
Changes:
- Add
image-utilshelper to sniff image MIME types from bytes and validate feedback logo IDs. - Update
/i/feedback/uploadand/feedback/preview/*to use byte-sniffed MIME types (instead of trusting stored data URI MIME) and enforce stricter authorization for global vs per-app uploads. - Add
X-Content-Type-Options: nosniff(and additional headers on some routes) and introduce unit tests for the new image utilities.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
test/unit-tests/star-rating.image-utils.js |
Adds unit tests covering image type sniffing and feedback logo name parsing. |
plugins/star-rating/frontend/app.js |
Hardens feedback preview response headers and validates preview IDs. |
plugins/star-rating/api/image-utils.js |
New helper for MIME sniffing and feedback logo name parsing. |
plugins/star-rating/api/api.js |
Uses sniffed MIME types for uploads, tightens feedback logo upload authorization, and validates logo names. |
plugins/dashboards/frontend/app.js |
Adds nosniff header to PNG responses. |
frontend/express/app.js |
Adds nosniff header to PNG responses. |
The /i/feedback/upload + /feedback/preview/* pair stored the client- supplied MIME and reflected it back as Content-Type, letting an app admin plant active HTML/JS served from the dashboard origin. A higher-privileged viewer's same-origin XHR would then surface countlyGlobal auth/csrf tokens to the attacker. Fixes layered defense-in-depth: - Sniff magic bytes server-side at upload; reject anything not in the PNG/JPEG/GIF/WebP allowlist instead of trusting myfile.type. - Re-derive Content-Type from sniffed bytes at preview time so any pre-existing payload also can't be served as text/html. - Add X-Content-Type-Options: nosniff, Content-Security-Policy: "sandbox; default-src 'none'", and Content-Disposition: inline on the preview response. The sandbox directive forces the response into an opaque origin so even a payload that did execute could not read /dashboard via same-origin XHR. - Restrict accepted upload names to the global feedback_logo or feedback_logo<24-char-hex-app-id>, and apply the same charset check on the preview path component. - Split authorization: the global logo now requires actual global admin (not any app admin via global_plugins), and per-app logos require admin on the specific app id encoded in the filename (not whichever app id the request happens to carry).
Covers sniffImageType (PNG/JPEG/GIF87a/GIF89a/WebP allowlist; rejects HTML, SVG, plain text, empty, short, and corrupted inputs; including the case where a real GIF header is followed by an HTML payload — the sniffer correctly classifies as gif and we rely on the Content-Type/nosniff/CSP layers to neutralize the trailing bytes) and parseFeedbackLogoName (accepts the global feedback_logo and the per-app feedback_logo<24-hex-app-id> shape; rejects path traversal, HTML, wrong charset/length, and non-string input).
Follow-up to the feedback/preview XSS hardening, applying the same patterns to the rest of the image upload/serve surface. uploadFile (/i/feedback/logo): the ratings widget logo upload trusted client-supplied myfile.type and the filename extension. Replace with magic-byte sniffing via image-utils.sniffImageType (the helper already introduced for /i/feedback/upload). Restricted to png/jpeg/gif to preserve the existing route contract. The saved extension now derives from the sniffed type, not the client filename. /star-rating/images/* serve route: add X-Content-Type-Options: nosniff, Content-Security-Policy: "sandbox; default-src 'none'", and Content-Disposition: inline. The route hardcodes Content-Type: image/png, so direct XSS was not possible, but stored bytes are user-supplied; the sandbox directive walls off any future regression where bytes might be served with a different Content-Type. /appimages/*, /memberimages/*, */screenshots/*, and /dashboards/images /screenshots/*: stored bytes here are guaranteed-valid PNGs (jimp re-encoded for icons, puppeteer-generated for screenshots) and the dashboard's add_headers middleware already injects nosniff globally based on dashboard_additional_headers config. Adding the header explicitly at the route level makes the security guarantee local to the route and survives any customer config tampering with dashboard_additional_headers.
- uploadFile (/i/feedback/logo): unlink tmp file on every exit path, including when countlyFs.saveData errors and when the try block throws. Previously the tmp file leaked on those paths. - uploadFeedbackFile (/i/feedback/upload): same fix — unlink tmp file when fs.readFile errors, when data is falsy, and when the try block throws. - /i/feedback/upload: replace the placeholder "feedback.invalid-name" message key with "feedback.imagef-error" which is already localized across all star-rating locale files. The UI would otherwise show the raw key for malformed uploads. - /star-rating/images/*: derive the response Content-Type from the filename extension (.png/.jpg/.jpeg/.gif) instead of always returning image/png. The route already serves jpg/gif files saved by the upload path, and with X-Content-Type-Options: nosniff added in the prior commit, an image/png label on jpg/gif bytes would be incorrect even though <img> rendering still worked. Falls back to the default app icon for any unrecognized extension.
ar2rsawseen
force-pushed
the
fix/feedback-preview-xss
branch
from
f35e0a4 to
2fc5236
Compare
Cookiezaurs
approved these changes
May 5, 2026
ar2rsawseen
added a commit
that referenced
this pull request
May 6, 2026
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 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.
No description provided.