License upload doesn't work in 8.15

I spun up an EE instance to do some testing for a customer and hit a 500 trying to upload the license. The following error is thrown when trying to upload:

Started POST "/admin/license" for 172.16.219.1 at 2016-12-22 19:57:27 +0000
Processing by Admin::LicensesController#create as HTML
  Parameters: {"utf8"=>"✓", "authenticity_token"=>"exd+qXBSuegC1NzmYxEKZfQM25ifh5luigFri3L0u7F0LvRHB48qwSwzjsnJfZkM+uZvDuBQVSCZH9In1nFhrw==", "license_type"=>"file", "license"=>{"data_file"=>#<UploadedFile:0x007f86d0918738 @content_type="application/octet-stream", @original_filename="GitLabInc.gitlab-license", @tempfile=#<File:/opt/gitlab/embedded/service/gitlab-rails/public/uploads/tmp/multipart-877844824/GitLabInc.gitlab-license>>, "data"=>""}}
Completed 500 Internal Server Error in 35ms (ActiveRecord: 5.0ms)

NoMethodError (undefined method `split' for nil:NilClass):
  app/models/license.rb:55:in `license'
  app/models/license.rb:62:in `license?'
  app/models/license.rb:112:in `valid_license'
  app/controllers/admin/licenses_controller.rb:31:in `block in create'
  app/controllers/admin/licenses_controller.rb:30:in `create'
  lib/gitlab/middleware/multipart.rb:98:in `block in call'
  lib/gitlab/middleware/multipart.rb:53:in `with_open_files'
  lib/gitlab/middleware/multipart.rb:97:in `call'
  lib/gitlab/request_profiler/middleware.rb:15:in `call'
  lib/gitlab/middleware/go.rb:16:in `call'
  lib/gitlab/middleware/readonly_geo.rb:29:in `call'

Could be related to changes in workhorse and file uploads?

cc/ @stanhu @DouweM

I'm wondering if that is something related to https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5867 and the strong parameters from http://api.rubyonrails.org/classes/ActionController/Parameters.html

PERMITTED_SCALAR_TYPES = [ 
  String, Symbol, NilClass, Numeric, 
  TrueClass, FalseClass, Date, Time,  
  StringIO, IO, ActionDispatch::Http::UploadedFile, 
  Rack::Test::UploadedFile
]

# Admin::LicensesController#create

params
=> {"utf8"=>"✓",
 "authenticity_token"=>"HJRfdDdr7ulaSUKLbQIlS/EVUPFi8ubvAvg1XFS5E+ncwGqgvSIL7oVz+3fCbmt2zUvSWQf7EN1dG1Aaf9088A==",
 "license_type"=>"file",
 "license"=>
  {"data_file"=>
    #<UploadedFile:0x007fd6568da368
     @content_type="application/octet-stream",
     @original_filename="TEST.gitlab-license",
     @tempfile=#<File:/Users/dbalexandre/Workspace/gitlab-ee/gitlab/public/uploads/tmp/multipart-027175998/TEST.gitlab-license>>,
   "data"=>""},
 "controller"=>"admin/licenses",
 "action"=>"create"}

license_params
=> {"data"=>""}

@jacobvosmaer-gitlab Could you please have a look at that? I think @dbalexandre is on something: ::UploadedFile is not an allowed scalar type in StrongAttributes...

Ouch, of all the things I could have broken, that is the worst one. We do have a copy-paste option too, right?

I will look at it now.

assigned to @jacobvosmaer-gitlab

What I don't understand is why this is only a problem here.

Ironically, this may have worked prior to https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/8215.

We cannot use Rack::Test::File because it copies the entire file. https://github.com/brynary/rack-test/blob/0cd85371dd5a347be78e48559b280ced72fab7f9/lib/rack/test/uploaded_file.rb#L33

Trying to just grab the underlying file in the controller now. That may work but it would be ad hoc. :(

What I don't understand is why this is only a problem here.

I think this is because we're using StrongParameters with:

  def license_params
    license_params = params.require(:license).permit(:data_file, :data)
    license_params.delete(:data) if license_params[:data_file]
    license_params
  end

We could use a trick like that?:

diff --git a/app/controllers/admin/licenses_controller.rb b/app/controllers/admin/licenses_controller.rb
index efd358000c..60e6dea16b 100644
--- a/app/controllers/admin/licenses_controller.rb
+++ b/app/controllers/admin/licenses_controller.rb
@@ -63,8 +63,14 @@ class Admin::LicensesController < Admin::ApplicationController
   end
 
   def license_params
-    license_params = params.require(:license).permit(:data_file, :data)
-    license_params.delete(:data) if license_params[:data_file]
+    license_params = params.require(:license).permit(:data)
+
+    if license_params.permitted?
+      license_params[:data_file] = params[:data_file]
+      license_params.delete(:data) if license_params[:data_file]
+      license_params.permit!
+    end
+
     license_params
   end
 end

license_params.permit! allows to force license_params.permitted? to return true (http://api.rubyonrails.org/classes/ActionController/Parameters.html#method-i-permit).

added regression label

changed milestone to %8.16

Thanks @rymai . What I meant was, why are strong parameters only biting us here. Isn't this a widely used thing?

Making a MR based on your patch now.

We have a bigger problem here because this was not caught by a test. Manual testing suggests we throw a 500 error if the user hits 'upload' without selecting a file to upload first. That is another problem but it is also bad.

mentioned in merge request !997 (closed)

What I meant was, why are strong parameters only biting us here. Isn't this a widely used thing?

I see, sorry for the misunderstanding, we should probably audit all the places where we're doing similar things, indeed.

closed via merge request gitlab-ce!8299

mentioned in issue gitlab-qa#24 (closed)

mentioned in commit maxraab/gitlab-ce@09af8833