[WEB-7887] fix(security): prevent stored XSS via SVG attachment served inline (GHSA-ch8j-vr4r-qf6h)#9312
Open
mguptahub wants to merge 2 commits into
Open
[WEB-7887] fix(security): prevent stored XSS via SVG attachment served inline (GHSA-ch8j-vr4r-qf6h)#9312mguptahub wants to merge 2 commits into
mguptahub wants to merge 2 commits into
Conversation
…d inline (GHSA-ch8j-vr4r-qf6h) Add SCRIPT_CAPABLE_MIME_TYPES frozenset (image/svg+xml, text/javascript, application/javascript, text/html, application/xhtml+xml, text/xml, application/xml) and enforce Content-Disposition: attachment on three download endpoints that previously defaulted to inline serving: - GenericAssetEndpoint.get (api/views/asset.py) - StaticFileAssetEndpoint.get (app/views/asset/v2.py) - EntityAssetEndpoint.get (space/views/asset.py) ATTACHMENT_MIME_TYPES is unchanged — users can still upload SVG, JS, and XML files. The fix closes the XSS vector by ensuring script-capable assets are always downloaded rather than rendered in the application's origin. Co-authored-by: Plane AI <noreply@plane.so>
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAsset download endpoints now derive ChangesAsset download disposition update
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 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 (1)
apps/api/plane/api/views/asset.py (1)
451-460: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the disposition logic into a shared helper.
The
storage/asset_mime_type/dispositionblock is copy-pasted verbatim across three endpoints. A single helper (e.g.disposition_for_mime(mime_type)) keeps the security policy in one place and prevents the three copies from drifting.🤖 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/api/plane/api/views/asset.py` around lines 451 - 460, The disposition calculation is duplicated across multiple asset endpoints, so extract the mime-type-to-disposition policy into a shared helper and reuse it here. Add or reuse a helper such as disposition_for_mime and call it from the asset download logic around S3Storage.generate_presigned_url, replacing the inline asset_mime_type/disposition block so the security rule stays centralized.
🤖 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/api/plane/api/views/asset.py`:
- Around line 452-455: Normalize the MIME type before the allowlist check in the
asset serving logic so case differences and parameters do not bypass the
SCRIPT_CAPABLE_MIME_TYPES block. Update the disposition decision in the asset
view to compare a normalized value (for example, stripped of parameters and
case-normalized) and apply the same fix in the duplicated asset handlers in
app/views/asset/v2.py and space/views/asset.py, preferably via a shared helper
so the behavior stays consistent across AssetView handling.
---
Nitpick comments:
In `@apps/api/plane/api/views/asset.py`:
- Around line 451-460: The disposition calculation is duplicated across multiple
asset endpoints, so extract the mime-type-to-disposition policy into a shared
helper and reuse it here. Add or reuse a helper such as disposition_for_mime and
call it from the asset download logic around S3Storage.generate_presigned_url,
replacing the inline asset_mime_type/disposition block so the security rule
stays centralized.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 30590d93-ded0-4486-883f-5c395dffba00
📒 Files selected for processing (4)
apps/api/plane/api/views/asset.pyapps/api/plane/app/views/asset/v2.pyapps/api/plane/settings/common.pyapps/api/plane/space/views/asset.py
…check Strip MIME parameters and lowercase before the allowlist check so that stored values like "image/svg+xml; charset=utf-8" or "Image/SVG+XML" are correctly identified as script-capable and served as attachment. Applies to all three download endpoints. Co-authored-by: Plane AI <noreply@plane.so>
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.
Summary
SCRIPT_CAPABLE_MIME_TYPESfrozenset tocommon.pylisting MIME types browsers can execute as scripts when served inline (image/svg+xml,text/javascript,application/javascript,text/html,application/xhtml+xml,text/xml,application/xml)Content-Disposition: inlinenow check the stored asset MIME type and forceattachmentfor any script-capable type:GenericAssetEndpoint.get—apps/api/plane/api/views/asset.pyStaticFileAssetEndpoint.get—apps/api/plane/app/views/asset/v2.pyEntityAssetEndpoint.get—apps/api/plane/space/views/asset.pyATTACHMENT_MIME_TYPESis unchanged — users can still upload SVG, JS, and XML files; the fix only affects how they are served on downloadSecurity context
Advisory: GHSA-ch8j-vr4r-qf6h | Severity: High | CWE-79 (Stored XSS)
A Guest-level workspace member could upload a malicious
image/svg+xmlattachment. In the default self-hosted MinIO deployment the presigned download URL is on the same origin as the Plane app. WithContent-Disposition: inline, the browser renders the SVG and executes any embeddedonload/<script>JavaScript in the application's security context → session theft and account takeover of any user who opened the link.Forcing
Content-Disposition: attachmentfor script-capable types causes the browser to download the file instead of rendering it, removing the execution context entirely.Test plan
Content-Disposition: attachmentin the presigned URL response headersContent-Disposition: inlineContent-Disposition: attachmentSummary by CodeRabbit
Content-Dispositionbased on the asset’s MIME type, forcing script-capable types to download instead of opening inline.