Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Display SVG files as images instead of text #14101

Merged
merged 24 commits into from
Jan 13, 2021

Conversation

jtran
Copy link
Contributor

@jtran jtran commented Dec 22, 2020

This is an attempt to close #1095. It makes the following changes:

  • Detects SVG image files and serves them with Content-Type: image/svg+xml instead of text/plain, text/html, or text/xml. This allows users to view an SVG as an image when browsing a repo's tree and when it's embedded in a markdown file.
  • Serves extra security headers for SVG files in raw and media routes: Content-Security-Policy: default-src 'none'; style-src 'unsafe-inline'; sandbox and X-Content-Type-Options: nosniff. This prevents the execution of scripts in untrusted SVG files.
  • Adds regression tests for the above
  • UI toggle on the view file page to switch between the text source and the rendered image for SVG files
  • Adds a config setting to disable the feature
  • Hides the "raw" button when viewing SVG files since unsafe styles aren't displayed, and viewing the image without styles may be confusing. Users can always right-click on the image to view or save it.

I tested this with https://github.com/edgar-bonet/test-svg-mime in Firefox and Chrome.

Copy link
Member

@techknowlogick techknowlogick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this PR, and especially thank you for including tests & documentation :)

This is not an exhaustive review, just a few preliminary comments.

integrations/view_test.go Outdated Show resolved Hide resolved
custom/conf/app.example.ini Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 22, 2020
@a1012112796 a1012112796 added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Dec 22, 2020
@6543 6543 added this to the 1.14.0 milestone Dec 22, 2020
@silverwind
Copy link
Member

silverwind commented Dec 22, 2020

Hides the "raw" button when viewing SVG files since unsafe styles aren't displayed, and viewing the image without styles may be confusing. Users can always right-click on the image to view or save it.

I have to say I like the "Raw" button on GitHub for SVG. It just opens the SVG with the mime type image/svg+xml in a new tab. Which styles are you refering to? SVGs are self-contained.

@jtran
Copy link
Contributor Author

jtran commented Dec 22, 2020

@silverwind,

Without adding style-src 'unsafe-inline'; to the content-security-policy, I haven't been able to view styles that are contained within SVG images. When you view an image directly by navigating to the raw URL in your browser, <img> tag security measures do not apply. So in this case, the content-security-policy is the only thing preventing malicious scripts inside SVGs from executing. Apparently there are also potentially harmful styles too? But I'm not an expert on this.

If someone understands this issue better and can explain why adding style-src 'unsafe-inline'; would be safe without additional security measures (like a separate domain), I would happily add that. But I'm not sure about the implications.

@silverwind
Copy link
Member

silverwind commented Dec 22, 2020

Good read on SVG security: https://www.w3.org/wiki/SVG_Security

You're right, <img> seems to be safe to use while serving raw with image/svg+xml will execute scripts, so I guess we'd need to sanitize the SVG first for this case (stripping script tags and url references using bluemonday as mentioned in #1095, if it can do that).

@jtran
Copy link
Contributor Author

jtran commented Dec 22, 2020

serving raw with image/svg+xml will execute scripts, so I guess we'd need to sanitize the SVG first for this case

That's why we use Content-Security-Policy: default-src 'none'; sandbox, which disables scripts. Sanitizing isn't needed to disable scripts. Please read up on #1095. Although they aren't mutually exclusive, my understanding from that issue is that people favor sandboxing over filtering/sanitizing. A sanitizing pull request #8024 was abandoned.

@silverwind
Copy link
Member

silverwind commented Dec 22, 2020

Few minor UI things:

  1. Display lines of code for SVG, e.g. like GitHub:

image

  1. Editing should be allowed but UI currently shows "Cannot edit binary files"
  2. Also use the rendered view in diffs, currently it's showing source. might actually be undesirable for review, need both views.

I think ultimately we'll need a switchable view with source and rendered but I guess rendered only will work as a start.

CI failure seems unrelated, maybe you can rebase/push once more to get it restarted.

@silverwind
Copy link
Member

silverwind commented Dec 22, 2020

Regarding raw: How about just showing the source in text/plain? I think it makes more sense, I'm usually after the source and currently there's no direct way to show it.

I also noticed SMIL animations (example) do not play in Firefox (they do in Chrome) when right clicking and view image. Is that caused by the sandboxing?

@kdumontnu
Copy link
Contributor

Regarding raw: How about just showing the source in text/plain? I think it makes more sense, I'm usually after the source and currently there's no direct way to show it.

Agree with text/plain view for raw. I've started work on a toggle button for regular file view. I'll continue that once this is merged

@jtran
Copy link
Contributor Author

jtran commented Dec 22, 2020

Regarding raw: How about just showing the source in text/plain? I think it makes more sense, I'm usually after the source and currently there's no direct way to show it.

If we did that, then embedding SVG images in markdown wouldn't work. Assuming the embedding uses the same raw URL, the server cannot distinguish between the two cases.

I also noticed SMIL animations (example) do not play in Firefox (they do in Chrome) when right clicking and view image. Is that caused by the sandboxing?

The animation plays for me in Firefox v84.0.1 when I follow that link. Maybe I misunderstood and you meant using this branch.

@silverwind
Copy link
Member

silverwind commented Dec 22, 2020

The animation plays for me in Firefox v84.0.1 when I follow that link.

Yes they play in GitHub but not in Gitea because of the missing style-src 'unsafe-inline' directive that GitHub serves, see suggestion above.

If we did that, then embedding SVG images in markdown wouldn't work. Assuming the embedding uses the same raw URL, the server cannot distinguish between the two cases.

I think it's unavoidable we add a JS-switchable view for rendered/source mode. Do you think you can implement something like that here?

@jtran
Copy link
Contributor Author

jtran commented Dec 22, 2020

Yes, I can add a way in the UI to toggle between text source and image.

Should I add style-src 'unsafe-inline' also? I just didn't want to add it if there was a security risk.

@jtran
Copy link
Contributor Author

jtran commented Dec 22, 2020

To clarify, it's not clear to me that we can just do what GitHub does in terms of the CSP because GitHub has additional security measures like serving content from a different domain. Presumably, serving from another domain is a much larger change that is out of scope for this pull request. But I could be wrong.

@silverwind
Copy link
Member

Should I add style-src 'unsafe-inline' also? I just didn't want to add it if there was a security risk.

It's the only way to get SMIL to work in Firefox and I think it's an essential part of SVG. I don't see anything wrong allowing inline styles. I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1683972 because I believe classifying SMIL as inline style is wrong in Firefox.

@jtran jtran force-pushed the jt/svg branch 3 times, most recently from cf474b6 to 96b0fa8 Compare December 30, 2020 03:34
@silverwind
Copy link
Member

I agree, JS toggle can come later.

@kdumontnu
Copy link
Contributor

@silverwind @techknowlogick @zeripath If you have a chance can you take a look through this PR? Should be all set & a big improvement to how gitea handles svgs. Most of the files in the diff are for integration test.

@@ -46,6 +46,11 @@ func ServeData(ctx *context.Context, name string, reader io.Reader) error {
} else if base.IsImageFile(buf) || base.IsPDFFile(buf) {
ctx.Resp.Header().Set("Content-Disposition", fmt.Sprintf(`inline; filename="%s"`, name))
ctx.Resp.Header().Set("Access-Control-Expose-Headers", "Content-Disposition")
if base.IsSVGImageFile(buf) {
ctx.Resp.Header().Set("Content-Security-Policy", "default-src 'none'; style-src 'unsafe-inline'; sandbox")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is unsafe-inline necessary here? ref. https://content-security-policy.com/unsafe-inline/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without it, many SVG images look nothing like they're intended to look. Many SVG attributes have a style alternative.

That said, I originally omitted it, but I was asked to add it in another comment in this PR. As long as it's safe, I think that including it is a good thing.

Copy link
Member

@lunny lunny Jan 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see my ref article, It said

When you want to allow inline scripts or styles on a page that uses CSP, there two much better options: nonce or hash.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read the article. I'm having trouble understanding how nonce or hash is actually better. If I understand it correctly, a hash prevents modification between the server and client, which is not more than what HTTPS would do. So this is to make the HTTP case safer.

If you think that's what we should do, it's not clear to me how to actually implement it. It seems like we would need to parse the SVG file enough to either insert nonces at the appropriate places in the SVG or read all the inline styles so that we can compute their hashes. Is this what you're saying? I'm guessing this must include all the style="..." attributes. There could potentially be many of them. I'll think about it some more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lunny The suggestion from the ref article is not applicable in this case, as the CSP is applied when rendering user-generated content (the SVG file). The general purpose of using nonce is so that website can include inline script in non-user controlled HTML content while at the same time benefiting from CSP blocking possible XSS attacks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nonce of CSP requires a change to the svg content dynamiclly which will make the thing complicated.

@kdumontnu
Copy link
Contributor

kdumontnu commented Jan 6, 2021

@lunny Is the "View Raw" case really commonly used? In my opinion we should take the most conservative/secure option for view raw or disable it for SVG and keep the size of this PR reasonable.
This PR already fixes embedding svgs in markdown, renders in view-file, and toggles between render/code, which is a huge improvement. If the view raw case isn't perfect, it can be discussed and improved in a subsequent PR.
My preferences for now:

  1. Add secure CSP headers and disable view raw button for svgs
    OR
  2. Render svg as code in view raw case

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jan 12, 2021
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jan 13, 2021
@techknowlogick techknowlogick merged commit 81467e6 into go-gitea:master Jan 13, 2021
@jtran jtran deleted the jt/svg branch January 13, 2021 05:59
a1012112796 added a commit to a1012112796/gitea that referenced this pull request Jan 14, 2021
* master: (252 commits)
  Issues overview should not show issues from archived repos (go-gitea#13220)
  Display SVG files as images instead of text (go-gitea#14101)
  [skip ci] Updated translations via Crowdin
  Update docs to clarify issues raised in go-gitea#14272 (go-gitea#14318)
  [skip ci] Updated translations via Crowdin
  [Refactor] Passwort Hash/Set (go-gitea#14282)
  Add option to change username to the admin panel (go-gitea#14229)
  fix mailIssueCommentBatch for pull request (go-gitea#14252)
  Remove self from MAINTAINERS (go-gitea#14286)
  Do not reload page after adding comments in Pull Request reviews (go-gitea#13877)
  Fix session bug when introduce chi (go-gitea#14287)
  [skip ci] Updated translations via Crowdin
  Add secure/httpOnly attributes to the lang cookie (go-gitea#9690) (go-gitea#14279)
  Some code improvements (go-gitea#14266)
  [skip ci] Updated translations via Crowdin
  Fix wrong type on hooktask to convert typ from char(16) to varchar(16) (go-gitea#14148)
  Upgrade XORM links in documentation. (go-gitea#14265)
  Check permission for the appropriate unit type (go-gitea#14261)
  Add compliance check for windows to ensure cross platform build (go-gitea#14260)
  [skip ci] Updated translations via Crowdin
  ...
@vmario89
Copy link

hi. saw that this was merged but it was not yet released right? Tried to configure Gitea ui.svg ENABLE_RENDER = true in app.ini but i guess thats another setting, not related to this? Looking forward to directly display SVG files in preview instead of plaintext.

https://github.com/go-gitea/gitea/issues/1095
https://github.com/go-gitea/gitea/pull/14101
https://docs.gitea.io/en-us/config-cheat-sheet/#ui---svg-images-uisvg

@kdumontnu
Copy link
Contributor

hi. saw that this was merged but it was not yet released right? Tried to configure Gitea ui.svg ENABLE_RENDER = true in app.ini but i guess thats another setting, not related to this? Looking forward to directly display SVG files in preview instead of plaintext.

I believe it is queued for release in version 1.14. You can test it in the current master branch.
The setting is enabled by default, so no need to change app.ini

@go-gitea go-gitea locked and limited conversation to collaborators Mar 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gitea can't render SVG files.