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

Force :attachment as content disposition for some content types #31639

Merged
merged 1 commit into from Jan 5, 2018

Conversation

@rosa
Copy link
Member

@rosa rosa commented Jan 5, 2018

Summary

Currently, unless a value for disposition is passed to ActiveStorage::Blob#service_url we use inline for everything. However, some content types shouldn't be rendered inline by default. For example:

  • Sending .ai files as application/postscript to Safari opens them in a blank, grey screen, and downloading .ai as application/postscript files in Safari appends .ps to the extension.
  • Sending HTML, SVG, XML and SWF files as binary closes XSS vulnerabilities. Even if these are mitigated by serving the blobs from a different domain from your application, having an HTML file rendered by the browser instead of downloaded opens the door for more realistic phishing attacks.
  • Sending JS files as binary avoids InvalidCrossOriginRequest without compromising security.

This pull request modifies this behaviour to force :attachment as disposition for specific content types like the above, configured in config.active_storage.content_types_to_serve_as_binary.

Other Information

I have included a default list of content types for this new config option, but it could be left empty if you think that makes more sense, and let users decide what they want to be treated as attachments. I think the default list makes sense, though.

r? @georgeclaghorn

@rails-bot
Copy link

@rails-bot rails-bot commented Jan 5, 2018

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @georgeclaghorn (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

activestorage/lib/active_storage/engine.rb Outdated
@@ -18,6 +18,14 @@ class Engine < Rails::Engine # :nodoc:
config.active_storage.analyzers = [ ActiveStorage::Analyzer::ImageAnalyzer, ActiveStorage::Analyzer::VideoAnalyzer ]
config.active_storage.paths = ActiveSupport::OrderedOptions.new
config.active_storage.variable_content_types = [ "image/png", "image/gif", "image/jpg", "image/jpeg", "image/vnd.adobe.photoshop" ]
config.active_storage.content_types_to_render_as_binary = [ "text/html",

This comment has been minimized.

@rosa

rosa Jan 5, 2018
Author Member

Perhaps a better name for this would e content_types_to_render_as_attachment.

This comment has been minimized.

@georgeclaghorn

georgeclaghorn Jan 5, 2018
Member

I’m fine with either binary or attachment, but render doesn’t seem like the right word to me. How about content_types_to_serve_as_binary?

activestorage/app/models/active_storage/blob.rb Outdated
else
:inline
end
end

This comment has been minimized.

@rosa

rosa Jan 5, 2018
Author Member

I had put this method as private before, then I thought it could make sense to expose it, for example if we were to check this for blob's variants in the future (although none of the variable content types makes sense as attachment right now).

This comment has been minimized.

@georgeclaghorn

georgeclaghorn Jan 5, 2018
Member

I think we should keep it private for now. If we need to call it elsewhere, we can make it public at that time.

Let’s also rename it to default_disposition to clarify that it’s a fallback.

Copy link
Member

@georgeclaghorn georgeclaghorn left a comment

Nice work, Rosa!

activestorage/app/models/active_storage/blob.rb Outdated
else
:inline
end
end

This comment has been minimized.

@georgeclaghorn

georgeclaghorn Jan 5, 2018
Member

I think we should keep it private for now. If we need to call it elsewhere, we can make it public at that time.

Let’s also rename it to default_disposition to clarify that it’s a fallback.

activestorage/lib/active_storage/engine.rb Outdated
@@ -18,6 +18,14 @@ class Engine < Rails::Engine # :nodoc:
config.active_storage.analyzers = [ ActiveStorage::Analyzer::ImageAnalyzer, ActiveStorage::Analyzer::VideoAnalyzer ]
config.active_storage.paths = ActiveSupport::OrderedOptions.new
config.active_storage.variable_content_types = [ "image/png", "image/gif", "image/jpg", "image/jpeg", "image/vnd.adobe.photoshop" ]
config.active_storage.content_types_to_render_as_binary = [ "text/html",

This comment has been minimized.

@georgeclaghorn

georgeclaghorn Jan 5, 2018
Member

I’m fine with either binary or attachment, but render doesn’t seem like the right word to me. How about content_types_to_serve_as_binary?

activestorage/lib/active_storage/engine.rb Outdated
"application/x-shockwave-flash",
"text/xml",
"application/xml",
"application/xhtml+xml" ]

This comment has been minimized.

@georgeclaghorn

georgeclaghorn Jan 5, 2018
Member

Style nit: indent the array elements one level under the assignment.

config.active_storage.content_types_to_render_as_binary = [
  "text/html",
  "text/javascript",
  # ...
]

This comment has been minimized.

@rosa

rosa Jan 5, 2018
Author Member

Sure thing! The truth is, I prefer one level under the assignment too! But I wasn't sure which one would be preferred, so I searched in the codebase and found this example:

tmp_dirs = [ "tmp/cache",
"tmp/sockets",
"tmp/pids",
"tmp/cache/assets" ]

And did the same. But it's true in that example the indentation is much smaller, so it's not really applicable directly 😅

@rosa rosa force-pushed the rosa:master branch Jan 5, 2018
@@ -201,8 +201,8 @@ def representable?
# with users. Instead, the +service_url+ should only be exposed as a redirect from a stable, possibly authenticated URL.
# Hiding the +service_url+ behind a redirect also gives you the power to change services without updating all URLs. And
# it allows permanent URLs that redirect to the +service_url+ to be cached in the view.
def service_url(expires_in: service.url_expires_in, disposition: "inline")
service.url key, expires_in: expires_in, disposition: disposition, filename: filename, content_type: content_type
def service_url(expires_in: service.url_expires_in, disposition: :inline)

This comment has been minimized.

@rosa

rosa Jan 5, 2018
Author Member

I've changed this from "inline" to :inline because I've seen it used as symbol in other places like:

def service_url(expires_in: service.url_expires_in, disposition: :inline)
service.url key, expires_in: expires_in, disposition: disposition, filename: filename, content_type: content_type
end

# Returns a signed, temporary URL for the file at the +key+. The URL will be valid for the amount
# of seconds specified in +expires_in+. You most also provide the +disposition+ (+:inline+ or +:attachment+),
# +filename+, and +content_type+ that you wish the file to be served with on request.

@rosa
Copy link
Member Author

@rosa rosa commented Jan 5, 2018

@georgeclaghorn, thanks a lot for your review! I have made the changes you suggested, and also, as you very correctly pointed out, I changed the code to force the content disposition to attachment instead of defaulting to it. Otherwise this wouldn't solve the security concerns, as an attacker could simply add disposition=inline to the URL to get the same effect as now. I have also updated the pull request description to match this new behaviour.

I was thinking... should this be included in the CHANGELOG?

@rosa rosa changed the title Use :attachment as default content disposition for some content types Force :attachment as content disposition for some content types Jan 5, 2018
activestorage/app/models/active_storage/blob.rb Outdated
def service_url(expires_in: service.url_expires_in, disposition: "inline")
service.url key, expires_in: expires_in, disposition: disposition, filename: filename, content_type: content_type
def service_url(expires_in: service.url_expires_in, disposition: :inline)
service.url key, expires_in: expires_in, disposition: forced_disposition || disposition, filename: filename, content_type: content_type

This comment has been minimized.

@georgeclaghorn

georgeclaghorn Jan 5, 2018
Member

What do you think about hoisting the conditional into this method, since we’re not returning more than one override from forced_disposition?

service.url key, expires_in: expires_in, disposition: forcibly_serve_as_binary? ? :attachment : disposition, filename: filename, content_type: content_type

This comment has been minimized.

@rosa

rosa Jan 5, 2018
Author Member

Sounds good to me, it looks long but I think it's clearer 👍

@georgeclaghorn
Copy link
Member

@georgeclaghorn georgeclaghorn commented Jan 5, 2018

I was thinking... should this be included in the CHANGELOG?

Yes please!

@rosa rosa force-pushed the rosa:master branch Jan 5, 2018
Copy link
Member

@georgeclaghorn georgeclaghorn left a comment

❤️

Can you squash your commits into one?

@rosa
Copy link
Member Author

@rosa rosa commented Jan 5, 2018

Of course!

In this way we avoid HTML, XML, SVG and other files that can be rendered
by the browser to be served inline by default. Depending on the origin
from where these files are served, this might lead to XSS
vulnerabilities, and in the best case, to more realistic phishing
attacks and open redirects.

We force it rather than falling back to it when other disposition is not
provided. Otherwise it would be possible for someone to force inline
just by passing `disposition=inline` in the URL.

The list of content types to be served as attachments is configurable.
@rosa rosa force-pushed the rosa:master branch to d40284b Jan 5, 2018
@georgeclaghorn georgeclaghorn merged commit 1e51a38 into rails:master Jan 5, 2018
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
codeclimate All good!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.