make gsub and sub unavailable in SafeBuffers - Closes #1555 #2248

Merged
merged 2 commits into from Sep 8, 2011

Projects

None yet

10 participants

Contributor

Following #1555, gsub and sub can't be used with Safe Buffers because of incorrect behavior with global variables when using blocks.
I have therefore disabled gsub and sub for safe strings, inviting people to use it on unsafe strings only.

Contributor

+1

Contributor
tbaba commented Aug 17, 2011

+1 I've also got this problem.

ruby-1.8.7-p334 :001 > require 'rubygems'
 => false 
ruby-1.8.7-p334 :002 > require 'active_support/all'
 => true 
ruby-1.8.7-p334 :003 > ActiveSupport::SafeBuffer.new('aaa bbb').sub(/(\w+) (\w+)/){puts $1; puts $2}
nil
nil
 => ""

+1 Ran into this when using CGI.escape on a SafeBuffer.

Contributor
dmathieu commented Sep 4, 2011

I've just rebased this PR against master, fixing a conflict.

@josevalim josevalim and 1 other commented on an outdated diff Sep 7, 2011
...t/lib/active_support/core_ext/string/output_safety.rb
@dirty = true # @dirty = true
super # super
end # end
EOT
end
+ UNAVAILABLE_STRING_METHODS.each do |unavailable_method|
+ class_eval <<-EOT, __FILE__, __LINE__
+ def #{unavailable_method}(*args) # def gsub(*args)
+ raise "#{unavailable_method} cannot be used with a Safe Buffer object. You should use object.to_str.#{unavailable_method}"
josevalim
josevalim Sep 7, 2011 Member

maybe use raise NoMethodError, "...msg..." ?

dmathieu
dmathieu Sep 7, 2011 Contributor

No problem, that's done.

@josevalim josevalim and 1 other commented on an outdated diff Sep 7, 2011
activesupport/lib/active_support/inflector/methods.rb
@@ -21,7 +21,7 @@ module ActiveSupport
# "words".pluralize # => "words"
# "CamelOctopus".pluralize # => "CamelOctopi"
def pluralize(word)
- result = word.to_s.dup
+ result = word.to_s.to_str.dup
josevalim
josevalim Sep 7, 2011 Member

Do we really need to call .to_s.to_str? Can't we simply assume that the object needs to implement to_str?

dmathieu
dmathieu Sep 7, 2011 Contributor

We'd then need to implement to_str in symbols.
Otherwise, it breaks tests here and here

Contributor
dmathieu commented Sep 8, 2011

As discussed with @josevalim yesterday, I have removed the support of symbols on camelize and classify, therefore allowing us to use directly to_str instead of to_s.to_str.

Member

We would need to deprecate it though. Could you please add a quick check if the resource is a symbol and if so, convert it to a string with a deprecation warning? Tks bro!!!

Contributor
dmathieu commented Sep 8, 2011

Using inflections on a symbol is now deprecated :)
I didn't put the deprecation on methods which rely on other inflection method (in order to avoid multiple warnings)

@josevalim josevalim merged commit b4a6e2f into rails:master Sep 8, 2011

The commented def gsub doesn't have a method body in the comment. Suggest moving this to the top of the method.

Contributor

Good idea. Do you want to do it ? I can make a pull request (or push it to docrails, after the next merge with master)

Member

Pls make a PR and ping me, I'll merge.

Test name should read Should not return safe buffer from capitalize!

Member
arunagw commented Sep 8, 2011

FYI this pull broke some number of tests. ActionMailer tests are having 48 Fails.

I think Mail gem needs an update.

ActiveRecord also failing. http://travis-ci.org/#!/rails/rails/builds/140608

Owner

This totally breaks tests. Why are we merging stuff in without running the tests?

Contributor
dmathieu commented Sep 9, 2011

FYI, I have pushed the commit again, this time running all the tests, not only activesupport.
dmathieu/rails@0f3ea07
I'll reopen a pull request when someone merges the same fix on mail : mikel/mail#281

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment