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

Encode Content-Disposition filenames on send_data and send_file #33829

Merged
merged 1 commit into from Sep 23, 2018

Conversation

@mtsmfm
Copy link
Contributor

@mtsmfm mtsmfm commented Sep 9, 2018

Summary

A few years ago, @stanhu tried to support non-ascii encodings for send_data and send_file.
#21461

but it's still not merged because it lacks tests and some points to be fixed.

In the PR, @jeremy told us how he encodes and actually it's the same how activestorage encodes file name now

I changed ActiveStorage::Filename::Parameters to ActionController::DataStreaming::DispositionFilenameParameters to encode in send_data and send_file and share the logic.

I tested on Chrome, IE11, Safari and Firefox

Chrome

chrome

IE11

ie

Safari

safari

Firefox

firefox

Script

# 1. Put this file into the root of rails repo
# 2. `bundle exec rackup -b 0.0.0.0`
# 3. `open localhost:9292`
require 'rails/all'

tmpdir = Dir.mktmpdir
Dir.chdir(tmpdir)

ENV['DATABASE_URL'] = "sqlite3://#{File.join(tmpdir, 'database.sql')}"

class TestApp < Rails::Application
  secrets.secret_key_base = 'secret_key_base'
end

Rails.application.configure do
  config.load_defaults 5.2
  config.eager_load = true
  config.active_storage.service = :local
  config.logger = ActiveSupport::Logger.new(STDOUT)
  config.active_storage.service_configurations = {
    local: {
      service: 'Disk',
      root: tmpdir
    }
  }

  config.after_initialize do |app|
    ActiveRecord::Schema.define do
      create_table :users, force: true do |t|
      end

      load File.join(Bundler.rubygems.find_name('activestorage').first.full_gem_path, 'db/migrate/20170806125915_create_active_storage_tables.rb')

      CreateActiveStorageTables.new.change
    end

    class User < ActiveRecord::Base
      has_one_attached :avatar
    end

    class UsersController < ActionController::Base
      def index
        @user = User.new

        render inline: <<~ERB
          <%= form_with model: @user do |f| %>
            <%= f.file_field :avatar %>
            <%= f.submit %>
          <% end %>

          <ul>
            <%- User.all.each do |u| %>
              <li>
                <div>id: <%= u.id %></div>
                <%= link_to 'download via send_data', user_path(u.id) %>
                <%= link_to 'download via rails_blob_path', rails_blob_path(u.avatar, disposition: 'attachment') %>
              </li>
            <% end %>
          </ul>
        ERB
      end

      def create
        User.create!(params.require(:user).permit(:avatar))

        redirect_to users_path
      end

      def show
        user = User.find(params[:id])

        send_data user.avatar.download, disposition: 'attachment', filename: user.avatar.filename.to_s
      end
    end

    app.routes.prepend do
      root to: redirect('/users')
      resources :users
    end

    user = User.new
    user.avatar.attach(io: File.open(__FILE__), filename: %(あ"い"\\う/え'お.rb))
    user.save!
  end
end
Rails.application.initialize!
run Rails.application

at_exit do
  FileUtils.rm_rf(tmpdir)
end

Other Information

I'm wondering if we can backport this change to older versions
-> I created backport gem https://github.com/mtsmfm/action_dispatch-http-content_disposition

Copy link
Member

@georgeclaghorn georgeclaghorn left a comment

Can we remove ActiveStorage::Filename::Parameters?

@@ -61,7 +61,7 @@ def sanitized
end

def parameters #:nodoc:
Parameters.new self
ActionController::DataStreaming::DispositionFilenameParameters.new(sanitized) # :nodoc:

This comment has been minimized.

@georgeclaghorn

georgeclaghorn Sep 9, 2018
Member

Is this :nodoc: comment necessary? What is it excluding from the docs?

This comment has been minimized.

@mtsmfm

mtsmfm Sep 10, 2018
Author Contributor

Sorry, it's my bad

@mtsmfm mtsmfm force-pushed the mtsmfm:encode-filename branch from 5e35120 to 4fb13b9 Sep 10, 2018
@georgeclaghorn
Copy link
Member

@georgeclaghorn georgeclaghorn commented Sep 11, 2018

Thanks, @mtsmfm! A few more things:

  1. We’re still requiring active_storage/filename/parameters here:

    require_dependency "active_storage/filename/parameters"

  2. There are ASt test failures like this:

    ArgumentError: wrong number of arguments (given 1, expected 0)
    

    I’m not immediately sure what’s causing them. Can you look into it?

  3. This needs a changelog entry.

  4. In response to the last paragraph of the PR description, I’m afraid we can’t backport this change, as it’s a new feature and not a bug fix. See the maintenance policy for more info.

@mtsmfm mtsmfm force-pushed the mtsmfm:encode-filename branch from 4fb13b9 to f6893aa Sep 11, 2018
@mtsmfm
Copy link
Contributor Author

@mtsmfm mtsmfm commented Sep 11, 2018

  1. Sorry, the cause is 1 🙇
  2. I thought it's kind of bug though, OK, I'll create a backport gem for older Rails.
@@ -1,3 +1,7 @@
* Encode Content-Disposition filenames on `send_data` and `send_file`.

This comment has been minimized.

@georgeclaghorn

georgeclaghorn Sep 11, 2018
Member

This merits more detail. At minimum, we ought to name the specifications implemented (RFCs 2231 and 5987) and provide examples.

This comment has been minimized.

@mtsmfm

mtsmfm Sep 12, 2018
Author Contributor

@mtsmfm mtsmfm force-pushed the mtsmfm:encode-filename branch from f6893aa to 1d3fa52 Sep 12, 2018
@@ -61,7 +59,7 @@ def sanitized
end

def parameters #:nodoc:
Parameters.new self
ActionController::DataStreaming::DispositionFilenameParameters.new(sanitized)

This comment has been minimized.

@rafaelfranca

rafaelfranca Sep 12, 2018
Member

Don't we need to require this file one? Also action_controller because it is not in Active Storage.

This comment has been minimized.

@mtsmfm

mtsmfm Sep 12, 2018
Author Contributor

I think we don't have to require because this file is under app and it's on Rails engine

This comment has been minimized.

@matthewd

matthewd Sep 12, 2018
Member

Can we avoid [internally] exposing this class entirely? AFAICS, this method's result is only used in:

(type.to_s.presence_in(%w( attachment inline )) || "inline") + "; #{filename.parameters}"

(And that method seems to have more in common with the surrounding lines in send_file_headers!, too.)

This comment has been minimized.

@mtsmfm

mtsmfm Sep 12, 2018
Author Contributor

Can we avoid [internally] exposing this class entirely?

Hmm, how can I avoid?
AFAIK, there's no way to share this logic across gems without exposing class/module

This comment has been minimized.

@georgeclaghorn

georgeclaghorn Sep 12, 2018
Member

I think Matthew’s suggesting removing the parameters method here and using the new class directly in ActiveStorage::Service#content_disposition_with.

This comment has been minimized.

@mtsmfm

mtsmfm Sep 12, 2018
Author Contributor

gotcha

This comment has been minimized.

@matthewd

matthewd Sep 12, 2018
Member

I was thinking of something like ActionController::ContentDisposition.format(:inline, filename: "hello.jpg") # => "inline; filename=[...]"

So we still need to expose a constant (and following @rafaelfranca's suggestion to move it out of the DataStreaming namespace), but having it do the work internally, and just return a string. It just feels a bit odd to me that we're exposing the class instance, when all we want to do is call to_s on it.

This comment has been minimized.

@mtsmfm

mtsmfm Sep 12, 2018
Author Contributor

Cool, I'll do that


module ActionController
module DataStreaming
class DispositionFilenameParameters # :nodoc:

This comment has been minimized.

@rafaelfranca

rafaelfranca Sep 12, 2018
Member

If we want to reuse inside Active Storage I don't think it should be inside DataStreaming. ActionController:: DispositionFilename is a better name.

This comment has been minimized.

@mtsmfm

mtsmfm Sep 12, 2018
Author Contributor

okay, I'll rename

This comment has been minimized.

@mtsmfm

mtsmfm Sep 12, 2018
Author Contributor

ah, you also meant DispositionFilenameParameters should be renamed to DispositionFilename, right?

This comment has been minimized.

@rafaelfranca

rafaelfranca Sep 12, 2018
Member

Right, but maybe DispositionFilenameParameter is better.

@mtsmfm mtsmfm force-pushed the mtsmfm:encode-filename branch 2 times, most recently from ba2d8aa to 60e29b5 Sep 12, 2018
@mtsmfm
Copy link
Contributor Author

@mtsmfm mtsmfm commented Sep 12, 2018

Failed components: activesupport

seems failed test isn't related to this PR

# frozen_string_literal: true

module ActionController
class ContentDisposition # :nodoc:

This comment has been minimized.

@mtsmfm

mtsmfm Sep 12, 2018
Author Contributor

@rafaelfranca Now I think this class should be put into the same place as ActionDispatch::Http::UploadedFile.
It seems all helper stuff is placed in ActionDispatch.
So I propose to rename this class to ActionDispatch::Http::ContentDisposition.
What do you think?

This comment has been minimized.

@rafaelfranca

rafaelfranca Sep 12, 2018
Member

Good point. Makes sense to me.

@@ -3,6 +3,7 @@
require "test_helper"
require "database/setup"
require "active_support/testing/method_call_assertions"
require "action_controller/content_disposition"

This comment has been minimized.

@rafaelfranca

rafaelfranca Sep 12, 2018
Member

This require should be probably inside lib/active_storage/service.rb

# frozen_string_literal: true

module ActionController
class ContentDisposition # :nodoc:

This comment has been minimized.

@rafaelfranca

rafaelfranca Sep 12, 2018
Member

Good point. Makes sense to me.

@mtsmfm mtsmfm force-pushed the mtsmfm:encode-filename branch from 60e29b5 to 890485c Sep 13, 2018
@mtsmfm
Copy link
Contributor Author

@mtsmfm mtsmfm commented Sep 13, 2018

@georgeclaghorn @rafaelfranca @matthewd I fixed all points you reviewed. Can you review again, please?

@jeremy
jeremy approved these changes Sep 18, 2018
Copy link
Member

@jeremy jeremy left a comment

👏

@mtsmfm
Copy link
Contributor Author

@mtsmfm mtsmfm commented Sep 23, 2018

@georgeclaghorn Can I ask you to review?

@kaspth kaspth merged commit ed56a03 into rails:master Sep 23, 2018
2 checks passed
2 checks passed
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kaspth
Copy link
Member

@kaspth kaspth commented Sep 23, 2018

Thanks!

@mtsmfm mtsmfm deleted the mtsmfm:encode-filename branch Sep 23, 2018
y-yagi added a commit to y-yagi/rails that referenced this pull request Nov 28, 2018
`ActiveStorage::Filename#parameters` was removed by rails#33829.
maxlazio pushed a commit to gitlabhq/gitlabhq that referenced this pull request Feb 5, 2019
Users downloading non-ASCII attachments would see garbled characters.
When used with object storage, AWS S3 would return an InvalidArgument
error: Header value cannot be represented using ISO-8859-1.

Per RFC 5987 and RFC 6266, Content-Disposition should be encoded
properly. This commit takes the Rails 6 implementation of
ActiveSuppport::Http::ContentDisposition
(rails/rails#33829) and ports it here.

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/47673
suketa added a commit to suketa/rails_sandbox that referenced this pull request Jun 8, 2019
Encode Content-Disposition filenames on send_data and send_file
rails/rails#33829
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

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