Incorrect handling of permalink values (possible SQL injection)

[maximkulkin]

In lib/spree/core/permalinks.rb there is save_permalink() method that ensures permalink uniqueness.
There is code

other = self.class.first(                                                                    
  :conditions => "#{field} LIKE '#{permalink_value}%'",                                      
  :order => "#{order} LENGTH(#{field}) DESC, #{field} DESC"                                  
)

If permalink value contains "'" (single quote) character, the rest of permalink is treated as SQL statement (causing treats from non-working functionality to SQL injections).

The problem is seen in normal (non-hacker) use when permalink is generated from russian product names: some russian char combinations translate into single quotes thus exposing this issue.

The proper way would be to use sql argument substitution:

other = self.class.first(
  :conditions => ["#{field} LIKE ?", "#{permalink_value}%"],
  :order => "#{order} LENGTH(#{field}) DESC, #{field} DESC"
)

[radar]

This has now been fixed in 1-1-stable and master. Thanks for bringing this to our attention.

[radar]

I have reverted this fix in 1-1-stable and master because it led to the broken build for spree_volume_pricing, showing this issue: http://travis-ci.org/#!/spree/spree_volume_pricing/jobs/1272629

Could you please look into a patch for this issue that would not cause this issue?

Thanks.

[maximkulkin]

radar, read the issue description, I have already outlined the proper fix.

other = self.class.where("#{field} LIKE ?", "#{permalink_value}%").all

We can go even more paranoid and escape special chars:

other = self.class.where("#{field} LIKE ?", "#{permalink_value.gsub(/[%_]/) { |c| '\'+c }}%").all

Although I think this should be part of DB adapter.

[radar]

Please see the broken build I linked to above. Your solution causes that SQL error. Please devise an alternate solution.

[maximkulkin]

What was committed and broke the build differs from my solution. Please, read carefully.

[maximkulkin]

Any update on this?