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.

Sign up for GitHub

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

Jump to bottom

Rails 5: Looping through params before sanitizing causes permitted hashes to be thrown out #23701

Closed
TrevorHinesley opened this issue Feb 16, 2016 · 5 comments
Closed

Rails 5: Looping through params before sanitizing causes permitted hashes to be thrown out #23701

TrevorHinesley opened this issue Feb 16, 2016 · 5 comments
Assignees
@sikachu
Labels
actionpack
Milestone
5.0.0

Comments

@TrevorHinesley
Copy link

Using Rails 5.0.0.beta2

Originally I thought this was a nested parameter sanitization issue, but I'm not so sure now.

If I loop through a nested hash as follows:

  def update
    @song = Song.find(params[:id])
    authorize! :update, @song
    params[:song][:song_audio_files_attributes].each do |key, song_audio_file_attributes|
    end
    @song.update(sanitized_params)
    render_object @song
  end

private

  def sanitized_params
    params.require(:song).permit(
      song_audio_files_attributes: [
        :id, :audio_file_id, :_destroy
      ]
  end

The keys of the nested hashes are considered unpermitted all of a sudden, and the keys and their values are thrown out. For instance, if these are the passed in parameters:

Parameters: {"utf8"=>"✓", "authenticity_token"=>"hbIlGy5QutlqcpAsNVu/bsEoG8fZoHon1girwFX/mHsmZ+o1S7YzgWF/DYMgp2JuznPJjwpbWaj9FomE4uqJDg==", "song"=>{"song_audio_files_attributes"=>{"0"=>{"duration"=>"03:52", "has_vocals"=>"1", "full_length"=>"1", "watermarked"=>"0", "exclusions"=>[""], "audio_file_id"=>"56", "_destroy"=>"false", "id"=>"13"}, "1"=>{"duration"=>"00:50", "has_vocals"=>"1", "full_length"=>"0", "watermarked"=>"1", "exclusions"=>[""], "audio_file_id"=>"72", "_destroy"=>"false", "id"=>"14"}, "1455583805363"=>{"duration"=>"00:50", "has_vocals"=>"1", "full_length"=>"0", "watermarked"=>"0", "exclusions"=>[""], "audio_file_id"=>"73", "_destroy"=>"false"}}, "id"=>"22"}

This shows up in console:

Unpermitted parameters: 0, 1, 1455583805363

But if I don't loop through the parameters before sanitizing, this doesn't happen (commented out the suspect lines below):

  def update
    @song = Song.find(params[:id])
    authorize! :update, @song
    # params[:song][:song_audio_files_attributes].each do |key, song_audio_file_attributes|
    # end
    @song.update(sanitized_params)
    render_object @song
  end

Why is that? I need to loop through to do some conversion on those parameters as they come in, but before they're sanitized. This worked fine in 4.2.5.1.
@maclover7
Copy link
Contributor

Could this have anything to do with AC::Parameters not inheriting from Hash anymore? cc @sikachu

@sikachu sikachu added this to the 5.0.0 milestone Feb 16, 2016
@sikachu sikachu self-assigned this Feb 16, 2016
@sikachu
Copy link
Member

sikachu commented Feb 16, 2016

It shouldn't be. I'll need to look at this further and report back.

Thanks for your report.

@TrevorHinesley
Copy link
Author

@sikachu not sure if this helps, but I forked Rails so I could do a bit of stepping through as the request came in. I put a byebug instance before and after filtering happens in the permit method, and it looks like the hash is getting passed in wrongly, hence the unpermission:

Started PATCH "/songs/22" for ::1 at 2016-02-16 21:40:39 -0600
Processing by SongsController#update as JS
  Parameters: {"utf8"=>"✓", "authenticity_token"=>"HeqIEzG7aI4IJGODib0eKOcoFDcl/ALv9I1/d2I/aNq+P0c9VF3h1gMp/iycQcMo6HPGf/YHIWDfk10z1Sp5rw==", "song"=>{"song_audio_files_attributes"=>{"0"=>{"duration"=>"03:52", "has_vocals"=>"1", "full_length"=>"1", "watermarked"=>"0", "exclusions"=>[""], "audio_file_id"=>"56", "_destroy"=>"false", "id"=>"13"}, "1"=>{"duration"=>"00:50", "has_vocals"=>"1", "full_length"=>"0", "watermarked"=>"1", "exclusions"=>[""], "audio_file_id"=>"72", "_destroy"=>"false", "id"=>"14"}, "1455680429715"=>{"duration"=>"00:50", "has_vocals"=>"1", "full_length"=>"0", "watermarked"=>"0", "exclusions"=>[""], "audio_file_id"=>"73", "_destroy"=>"false"}}, "title"=>"Breathing Loop", "song_artists_attributes"=>{"0"=>{"id"=>"12", "artist_id"=>"19", "_destroy"=>"false"}}, "album_songs_attributes"=>{"0"=>{"id"=>"12", "album_id"=>"3", "_destroy"=>"false"}}, "songwriters_attributes"=>{"0"=>{"percentage"=>"50.0", "id"=>"16", "writer_id"=>"4", "_destroy"=>"false"}}, "song_publishers_attributes"=>{"0"=>{"percentage"=>"50.0", "id"=>"6", "publisher_id"=>"7", "_destroy"=>"false"}}, "playlist_songs_attributes"=>{"0"=>{"id"=>"28", "playlist_id"=>"9", "_destroy"=>"false"}}, "taggable_tags_attributes"=>{"0"=>{"id"=>"24", "tag_id"=>"2", "_destroy"=>"false"}}, "notes"=>"Shit about shit", "licensable"=>"1", "tagging_completed"=>"1", "staff_pick"=>"1"}, "songwriters"=>"", "song_publishers"=>"", "composers"=>"", "playlist_songs"=>"", "mood_taggable_tags"=>"", "pace_taggable_tags"=>"", "genre_taggable_tags"=>"", "button"=>"", "id"=>"22"}
  User Load (0.4ms)  SELECT  "users".* FROM "users" WHERE "users"."id" = $1 ORDER BY "users"."id" ASC LIMIT $2  [["id", 2], ["LIMIT", 1]]
  Song Load (0.3ms)  SELECT  "songs".* FROM "songs" WHERE "songs"."id" = $1 LIMIT $2  [["id", 22], ["LIMIT", 1]]
  Subscription Load (0.5ms)  SELECT  "subscriptions".* FROM "subscriptions" WHERE "subscriptions"."user_id" = $1 AND "subscriptions"."ended_at" IS NULL ORDER BY "subscriptions"."id" ASC LIMIT $2  [["user_id", 2], ["LIMIT", 1]]
  CACHE (0.0ms)  SELECT  "subscriptions".* FROM "subscriptions" WHERE "subscriptions"."user_id" = $1 AND "subscriptions"."ended_at" IS NULL ORDER BY "subscriptions"."id" ASC LIMIT $2  [["user_id", 2], ["LIMIT", 1]]
  Plan Load (0.2ms)  SELECT  "plans".* FROM "plans" WHERE "plans"."id" = $1 LIMIT $2  [["id", 6], ["LIMIT", 1]]
  Plan Load (0.2ms)  SELECT  "plans".* FROM "plans" WHERE "plans"."name" = $1 ORDER BY "plans"."id" ASC LIMIT $2  [["name", "Admin"], ["LIMIT", 1]]
  AudioFile Load (0.3ms)  SELECT  "audio_files".* FROM "audio_files" WHERE "audio_files"."id" = $1 LIMIT $2  [["id", 56], ["LIMIT", 1]]
   (0.1ms)  BEGIN
   (0.2ms)  COMMIT
  AudioFile Load (0.2ms)  SELECT  "audio_files".* FROM "audio_files" WHERE "audio_files"."id" = $1 LIMIT $2  [["id", 72], ["LIMIT", 1]]
   (0.1ms)  BEGIN
   (0.1ms)  COMMIT
  AudioFile Load (0.2ms)  SELECT  "audio_files".* FROM "audio_files" WHERE "audio_files"."id" = $1 LIMIT $2  [["id", 73], ["LIMIT", 1]]
   (0.1ms)  BEGIN
   (0.2ms)  COMMIT

[377, 386] in /Users/Trevor/Work/Rails/rails/actionpack/lib/action_controller/metal/strong_parameters.rb
   377:     #   # => {"contact"=>{"email"=>"none@test.com", "phone"=>"555-1234"}}
   378:     def permit(*filters)
   379:       params = self.class.new
   380: 
   381:       byebug
=> 382:       filters.flatten.each do |filter|
   383:         case filter
   384:         when Symbol, String
   385:           permitted_scalar_filter(params, filter)
   386:         when Hash then
(byebug) params
{}
(byebug) filters
[:title, :isrc, :tagging_completed, :licensable, :has_vocals, :notes, :staff_pick, {:song_audio_files_attributes=>[:id, :audio_file_id, :_destroy], :song_artists_attributes=>[:id, :artist_id, :_destroy], :album_songs_attributes=>[:id, :album_id, :_destroy], :songwriters_attributes=>[:id, :writer_id, :percentage, :_destroy], :song_publishers_attributes=>[:id, :publisher_id, :percentage, :_destroy], :composers_attributes=>[:id, :artist_id, :_destroy], :playlist_songs_attributes=>[:id, :playlist_id, :_destroy], :taggable_tags_attributes=>[:id, :tag_id, :_destroy]}]
(byebug) c

[377, 386] in /Users/Trevor/Work/Rails/rails/actionpack/lib/action_controller/metal/strong_parameters.rb
   377:     #   # => {"contact"=>{"email"=>"none@test.com", "phone"=>"555-1234"}}
   378:     def permit(*filters)
   379:       params = self.class.new
   380: 
   381:       byebug
=> 382:       filters.flatten.each do |filter|
   383:         case filter
   384:         when Symbol, String
   385:           permitted_scalar_filter(params, filter)
   386:         when Hash then
(byebug) params
{}
(byebug) filters
[:id, :audio_file_id, :_destroy]
(byebug) self
{"0"=>{"audio_file_id"=>"56", "_destroy"=>"false", "id"=>"13"}, "1"=>{"audio_file_id"=>"72", "_destroy"=>"false", "id"=>"14"}, "1455680429715"=>{"audio_file_id"=>"73", "_destroy"=>"false"}}
(byebug) self.keys
["0", "1", "1455680429715"]
(byebug) params.keys
[]
(byebug) c

[387, 396] in /Users/Trevor/Work/Rails/rails/actionpack/lib/action_controller/metal/strong_parameters.rb
   387:           hash_filter(params, filter)
   388:         end
   389:       end
   390:       byebug
   391: 
=> 392:       unpermitted_parameters!(params) if self.class.action_on_unpermitted_parameters
   393: 
   394:       params.permit!
   395:     end
   396: 
(byebug) self.keys
["0", "1", "1455680429715"]
(byebug) params.keys
[]
(byebug) c
Unpermitted parameters: 0, 1, 1455680429715

[377, 386] in /Users/Trevor/Work/Rails/rails/actionpack/lib/action_controller/metal/strong_parameters.rb
   377:     #   # => {"contact"=>{"email"=>"none@test.com", "phone"=>"555-1234"}}
   378:     def permit(*filters)
   379:       params = self.class.new
   380: 
   381:       byebug
=> 382:       filters.flatten.each do |filter|
   383:         case filter
   384:         when Symbol, String
   385:           permitted_scalar_filter(params, filter)
   386:         when Hash then
(byebug) self.keys
["id", "artist_id", "_destroy"]
(byebug) params.keys
[]
(byebug)

And since calling each on the nested hash causes this, I'm assuming each_pair is the ultimate culprit.

tenderlove added a commit that referenced this issue Feb 18, 2016
@tenderlove
add a regression test for #23701
77f183a
@tenderlove
Copy link
Member

Hi, I think I fixed this. Since I don't have your exact repro, I had to guess. I think I guessed correctly, but can you please give master a try and see if my commit fixed this? Thank you!

@TrevorHinesley
Copy link
Author

@tenderlove Yep! Well done :)

@speckins speckins mentioned this issue Jun 22, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sikachu sikachu

Labels
actionpack
Projects
None yet
Milestone
5.0.0
Development

No branches or pull requests
4 participants
@tenderlove @sikachu @TrevorHinesley @maclover7