ActiveSupport::SafeBuffer#prepend inconsistency #14529

Merged
merged 2 commits into from Apr 2, 2014

Conversation

Projects
None yet
6 participants
@rwz
Contributor

rwz commented Mar 30, 2014

Native ruby String#prepend modifies instance in-place, while ActiveSupport::SafeBuffer returns modified version, but the initial object remains unchanged.

a = "bar"           # => "bar"
a.prepend "foo"     # => "foobar"
a                   # => "foobar"
b = "bar".html_safe # => "bar"
b.class             # => ActiveSupport::SafeBuffer
b.prepend "foo"     # => "foobar"
b                   # => "bar", expected "foobar"
+ assert_equal @string, "otherhello"
+ end
+
+ test "Prepending unsafe unto safe yields escaped safe" do

This comment has been minimized.

@ming-kernel

ming-kernel Mar 30, 2014

unto should be onto?

@ming-kernel

ming-kernel Mar 30, 2014

unto should be onto?

This comment has been minimized.

@rwz

rwz Mar 30, 2014

Contributor

@ming-kernel Yep, you're right. Thanks!

@rwz

rwz Mar 30, 2014

Contributor

@ming-kernel Yep, you're right. Thanks!

@robin850 robin850 added activemodel and removed activemodel labels Mar 30, 2014

+ test "Prepending unsafe onto safe yields escaped safe" do
+ other = "other".html_safe
+ other.prepend "<foo>"
+ assert other.html_safe?

This comment has been minimized.

@rafaelfranca

rafaelfranca Mar 30, 2014

Member

I don't think prepend should even mark an string html safe. We are changing the behavior and also we may opening a XSS vector.

@rafaelfranca

rafaelfranca Mar 30, 2014

Member

I don't think prepend should even mark an string html safe. We are changing the behavior and also we may opening a XSS vector.

This comment has been minimized.

@rwz

rwz Mar 30, 2014

Contributor

@rafaelfranca this is how #concat currently works. I guess it'd make sense to use the same behavior for #prepend

@rwz

rwz Mar 30, 2014

Contributor

@rafaelfranca this is how #concat currently works. I guess it'd make sense to use the same behavior for #prepend

@rwz

This comment has been minimized.

Show comment
Hide comment
@rwz

rwz Apr 1, 2014

Contributor

What's the status of this? Are we waiting for someone to make a decision or an alternative implementation to show up?

Contributor

rwz commented Apr 1, 2014

What's the status of this? Are we waiting for someone to make a decision or an alternative implementation to show up?

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Apr 1, 2014

Member

Sorry, many issues to handle. It is missing CHANGELOG

Member

rafaelfranca commented Apr 1, 2014

Sorry, many issues to handle. It is missing CHANGELOG

@rwz

This comment has been minimized.

Show comment
Hide comment
@rwz

rwz Apr 1, 2014

Contributor

Ah, alright, will add changelog then.

Contributor

rwz commented Apr 1, 2014

Ah, alright, will add changelog then.

@rwz

This comment has been minimized.

Show comment
Hide comment
@rwz

rwz Apr 1, 2014

Contributor

Done.

Contributor

rwz commented Apr 1, 2014

Done.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Apr 1, 2014

Member

We should not remove prepend! without a deprecation message. Mind to add it?

Member

rafaelfranca commented Apr 1, 2014

We should not remove prepend! without a deprecation message. Mind to add it?

@rwz

This comment has been minimized.

Show comment
Hide comment
@rwz

rwz Apr 1, 2014

Contributor

ok

Contributor

rwz commented Apr 1, 2014

ok

Make AS::SafeBuffer#prepend act like String#prepend
Make `#prepend` method modify instance in-place and return self
instead of just returning modified value. That is exactly what
`#prepend!` method was doing previously, so it's deprecated from
now on.
@rwz

This comment has been minimized.

Show comment
Hide comment
@rwz

rwz Apr 1, 2014

Contributor

Done.

Contributor

rwz commented Apr 1, 2014

Done.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Apr 1, 2014

Member

Thank you. I'll merge it

Member

rafaelfranca commented Apr 1, 2014

Thank you. I'll merge it

@matthewd

This comment has been minimized.

Show comment
Hide comment
@matthewd

matthewd Apr 1, 2014

Member

Do we want #concat to be defined via define_method? What about extracting the commonality via a helper...

def concat(value)
  super(maybe_escape(value))
end

def prepend(value)
  super(maybe_escape(value))
end

private
def maybe_escape(value)
  if html_safe? && !value.html_safe?
    ERB::Util.h(value)
  else
    value
  end
end

wdyt?

Member

matthewd commented Apr 1, 2014

Do we want #concat to be defined via define_method? What about extracting the commonality via a helper...

def concat(value)
  super(maybe_escape(value))
end

def prepend(value)
  super(maybe_escape(value))
end

private
def maybe_escape(value)
  if html_safe? && !value.html_safe?
    ERB::Util.h(value)
  else
    value
  end
end

wdyt?

@rwz

This comment has been minimized.

Show comment
Hide comment
@rwz

rwz Apr 1, 2014

Contributor

@matthewd that'll produce two separate methods with exact same implementation, which might be even more confusing than a define_method approach. Also, I was reluctant to introduce another helper method on a global class just for that.

Contributor

rwz commented Apr 1, 2014

@matthewd that'll produce two separate methods with exact same implementation, which might be even more confusing than a define_method approach. Also, I was reluctant to introduce another helper method on a global class just for that.

@matthewd

This comment has been minimized.

Show comment
Hide comment
@matthewd

matthewd Apr 1, 2014

Member

Hmm, I just realised there's actually already html_escape_interpolated_argument.

As for visually matching methods, I personally don't think that's particularly confusing -- especially when the very first word of the method body is 'super'... but I'm just thinking out loud. It's up to @rafaelfranca which he prefers. And the one that someone cared enough to make, sure has a certain argument over my bikeshedding. 💙

Member

matthewd commented Apr 1, 2014

Hmm, I just realised there's actually already html_escape_interpolated_argument.

As for visually matching methods, I personally don't think that's particularly confusing -- especially when the very first word of the method body is 'super'... but I'm just thinking out loud. It's up to @rafaelfranca which he prefers. And the one that someone cared enough to make, sure has a certain argument over my bikeshedding. 💙

@rwz

This comment has been minimized.

Show comment
Hide comment
@rwz

rwz Apr 2, 2014

Contributor

I've added a commit to make use of this helper effectively DRYing the code. Thanks, good catch.

Contributor

rwz commented Apr 2, 2014

I've added a commit to make use of this helper effectively DRYing the code. Thanks, good catch.

rafaelfranca added a commit that referenced this pull request Apr 2, 2014

Merge pull request #14529 from rwz/master
ActiveSupport::SafeBuffer#prepend inconsistency

@rafaelfranca rafaelfranca merged commit 8482895 into rails:master Apr 2, 2014

1 check passed

default The Travis CI build passed
Details
@@ -4,6 +4,7 @@
require 'inflector_test_cases'
require 'constantize_test_cases'
+require 'active_support/deprecation/reporting'

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Apr 2, 2014

Member

Shouldn't the require be on the actual file rather than the test?

@carlosantoniodasilva

carlosantoniodasilva Apr 2, 2014

Member

Shouldn't the require be on the actual file rather than the test?

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 2, 2014

Member

👍. Fixing

@rafaelfranca

rafaelfranca Apr 2, 2014

Member

👍. Fixing

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