Conversation
|
We require contributors to sign our Contributor License Agreement, and we don't have you on file. In order for us to review and merge your code, please complete this form and we'll get you added and review your contribution as soon as possible. |
Test coverage90.0% line coverage reported by SimpleCov. |
There was a problem hiding this comment.
Pull request overview
Updates Scratch asset delivery so SVGs are served with the correct MIME type (to address issue #1286).
Changes:
- Add an SVG fixture for testing.
- Update request specs to follow redirects and assert final
Content-Typefor PNG and SVG. - Change Scratch assets
showendpoint to redirect to an ActiveStorage-generated URL with an explicitcontent_type.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
app/controllers/api/scratch/assets_controller.rb |
Generates a direct ActiveStorage URL for the attachment and redirects to it while explicitly setting content_type. |
spec/features/scratch/creating_and_showing_a_scratch_asset_spec.rb |
Adds/updates request assertions to verify the served media type after redirects for PNG and SVG. |
spec/fixtures/files/test_svg_image.svg |
Adds an SVG fixture used by the new/updated tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| scratch_asset = ScratchAsset.find_by!(filename: filename_with_extension) | ||
| redirect_to scratch_asset.file.url(content_type: scratch_asset.file.content_type), allow_other_host: true |
There was a problem hiding this comment.
Passing content_type: scratch_asset.file.content_type into ActiveStorage URL generation overrides Rails’ built-in content_types_to_serve_as_binary safety behavior (notably for image/svg+xml). If these assets can be user-uploaded, this can re-enable serving active SVG/HTML content as its original MIME type and increase XSS risk. Consider restricting this override to a whitelist of expected image types (or only SVG if that’s the requirement) and/or validating/sanitizing SVG uploads before allowing them to be served as image/svg+xml.
There was a problem hiding this comment.
I've updated https://github.com/RaspberryPiFoundation/digital-editor-issues/issues/1263 to make sure we only apply this to Scratch global assets and not project specific ones.
|
We require contributors to sign our Contributor License Agreement, and we don't have you on file. In order for us to review and merge your code, please complete this form and we'll get you added and review your contribution as soon as possible. |
zetter-rpf
changed the title
Closes https://github.com/RaspberryPiFoundation/digital-editor-issues/issues/1286